Devise: Devise should trigger the :authentication event on every `sign_in` call

[jemmaissroff]

I am still experiencing the same problem described in #2614. I see that #2614 was closed, but it seems as though there are quite a few people also interested in this behavior.

The only way to hook into warden#authenticate! is through a SessionsController#create call. If we use Devise's sign_in method, there's no trigger to Warden's authentication event.

We'd like to track when users log in, but using the suggested after_authentication case is missing cases where we have a manual call to sign_in. Using after_set_user, as suggested in #2614, won't work because after_set_user is called on every authenticated request and we only want this behavior when a user actually signs in.

[dmitry]

What is the solution for the correct behavior?

[JacobEvelyn]

The workaround I found (not really documented well that I could see) is to change sign_in(resource, user) to sign_in(resource, user, event: :authentication). But I still think this should be the default behavior.

[colinross]

If you are talking about hooks in the authentication strategy layer, that is in warden's wheel house.

we only want this behavior when a user actually signs in

Then I think what you are looking for is after_authentication from Warden, eg.

Warden::Manager.after_authentication do |user, auth, opts|
  ... #called when user actually authenticates
end

Although I must note that

missing cases where we have a manual call to sign_in.

won't call after_authentication since sign_in is specifically for setting the user without going through authentication

[JacobEvelyn]

@colinross so we are indeed using Warden::Manager.after_authentication (to track when users log in). The issue is as you described—that with a custom login route in which we call sign_in, this doesn't get triggered unless we do sign_in(resource, user, event: :authentication), which is easy to forget and not well-documented. Does that make sense?

[colinross]

Your use case makes sense. What doesn't quite make sense is the requested (default) behavior change.
sign_in literally states it is for signing in 'a user that already was authenticated. ' so why should it implicitly trigger authentication again? Would we then have a skip authentication flag on sign_in?

[JacobEvelyn]

@colinross putting what sign_in's documentation _says_ aside for a moment, I think it's best to speak to actual use cases for the Warden callback. In our codebase (and it seems the codebases of others who've commented and 👍ed this issue and #2614), whenever a user "gets logged in" (to use a term that's not tied to the code) we want to log it, and the only feasible way to do that is in after_authentication. That seems to match Warden's example, and I'm personally struggling to think of a case where you'd want a user to "get logged in" but not do whatever you do for every other user who gets logged in.

However, I'm completely open to the possibility that I'm being short-sighted and ignoring other use cases that expect the current behavior to remain as-is. If that's the case, the best solution would be to just improve documentation for sign_in. The fact that folks have created issues for this and reported difficulty in 2013, 2018, and 2019 suggests to me that it's not at all clear how this code should behave. Anecdotally, it took me over a year (from this to this) to learn the solution myself, and better documentation would have gotten me there much faster.

But I guess my point is that before rushing to improve documentation for sign_in, maybe someone who knows more than me should consider which behavior is actually the best default for most users. It might be the current behavior, or it might not.

[colinross]

My main concern here is that changing the default would be a breaking change, and as someone who manages many rails projects that leverage devise, I like to minimize and mitigate those types of changes for everyone.

If you have a manual call to sign_in, why not just also include a manual call to the callback as well, or override the event used. To me the current behavior _is_ actually pretty intuitive.

If you want to sign someone in programmatically (without going through an authentication form/request), you use sign_in. If you want to make sure the authentication hooks are called _as if they just authenticated_, you can override the event warden uses with event: :authentication. Notice how the override usage corresponds to overriding behavior?

I'm personally struggling to think of a case where you'd want a user to "get logged in" but not do whatever you do for every other user who gets logged in.

'Switch user' / 'Log in as' functionality is one such case off the top of my head.

We need better docs? Ok, cool. We need a new helper method for something commonly done? Ok, not a problem.
We want to change default existing functionality/expectations to match one of many common use cases? Not Cool.

[JacobEvelyn]

Totally fair. As mentioned in my previous comment, I'm all for just resolving this with better documentation. How can I help make that happen?

[colinross]

I don't see where this helper is specifically called out in the
documentation (README) but I would assert it would belong in the Controller
Filters and Helpers section
https://github.com/plataformatec/devise#controller-filters-and-helpers.

Something along the lines of

The 'sign_in' controller helper method sets the user to be used in the
session without calling the underlying warden authentication strategies
or after_authentication hooks. (calling the underlying warden helper
set_user). If you wish to have the authentication callbacks triggered,
override the event sent to warden by appending it as an option (last
argument) which gets passed down to the warden helper.

To sign in a user while triggering authentication callback
(after_authentication)

sign_in(resource, user, event: :authentication)

On Fri, Sep 27, 2019 at 10:55 AM Jacob Evelyn [e-mail]
wrote:

Totally fair. As mentioned in my previous comment, I'm all for just
resolving this with better documentation. How can I help make that happen?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/plataformatec/devise/issues/5126?email_source=notifications&email_token=AABNFT4D7WW7EVQNW374KTLQLZCHTA5CNFSM4IQGC7W2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7ZUVRI#issuecomment-536038085,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABNFT2AVKDMYYHR4XIGJHLQLZCHTANCNFSM4IQGC7WQ
.

[tegon]

@colinross that seems good to me! Do you want to send a pull request with that documentation?
I'd be happy to accept it!

[colinross]

Will do, time permitting