Pass along remember_me to #auto_login
#136
Conversation
When overriding the `#auto_login` method when implementing our own session handling we never got passed the `should_remember` value from the `#login` method. The `Sorcery::Controller::Submodules::RememberMe` never used the value of the `should_remember` arguments passed to the `#auto_login` method and instead relied on a callback being executed. This commits removes the callback and instead calls the `#remember_me!` method from the `#auto_login` method instead.
|
Looks good. Thanks! |
| @@ -47,7 +47,7 @@ def login(*credentials) | |||
| end | |||
| form_authenticity_token | |||
|
|
|||
| auto_login(user) | |||
| auto_login(user, credentials[2]) | |||
There was a problem hiding this comment.
The _should_remember param in auto_login appears to be unused, and doesn't actually kick off the remember me functionality.
| @@ -18,7 +18,6 @@ def merge_remember_me_defaults! | |||
| merge_remember_me_defaults! | |||
| end | |||
| Config.login_sources << :login_from_cookie | |||
| Config.after_login << :remember_me_if_asked_to | |||
There was a problem hiding this comment.
I believe this is still required, or its functionality needs to be added to the auto_login method.
| # calls remember_me! if a third credential was passed to the login method. | ||
| # Runs as a hook after login. | ||
| def remember_me_if_asked_to(_user, credentials) | ||
| remember_me! if credentials.size == 3 && credentials[2] && credentials[2] != '0' |
There was a problem hiding this comment.
I believe the reason for using a hook here instead of injecting directly into the original login method is due to the submodule loading model. We can't guarantee that the remember_me functionality is always present.
There was a problem hiding this comment.
Hey @athix - I know this is an older PR, I believe this did break the remember me functionality.
This change may introduce a security concern to those that use the remember_me_token_persist_globally configuration option implemented in NoamB/sorcery#690.
We use the option mentioned above, and due to the logic implemented here, any user with a remember_me_token persisted is automatically set to be remembered, even if the should_remember flag is false.
Would you accept a PR that brings back the remember_me_if_asked_to method?
When overriding the
#auto_loginmethod when implementing our ownsession handling we never got passed the
should_remembervalue fromthe
#loginmethod.The
Sorcery::Controller::Submodules::RememberMenever used the valueof the
should_rememberarguments passed to the#auto_loginmethodand instead relied on a callback being executed.
This commits removes the callback and instead calls the
#remember_me!method from the
#auto_loginmethod instead.