Save session.id in session #53
Conversation
|
Nice!
Thank you for doing this! |
JoeSouthan
force-pushed
the
joesouthan/save-session
branch
from
3eea8f7
to
10621c0
Compare
JoeSouthan
force-pushed
the
joesouthan/save-session
branch
from
10621c0
to
4c8d282
Compare
|
Thanks for taking a look at the PR! I've
As it stood, cookies did expose implementation details (model type) and allowed for session fixation. I've added As to expiry, it will now honour whatever is in |
| end | ||
|
|
||
| def cookie_key(authenticatable_class) | ||
| DeprecatedSecretsController.new.send(:cookie_key, authenticatable_class) |
There was a problem hiding this comment.
Let's pull this method out into its own file so we can test it on its own and we don't need weird (in a Rails context) initialisation?
| @@ -12,6 +12,10 @@ def create_session_for(user) | |||
| ) | |||
| end | |||
|
|
|||
| def session_key(authenticatable_class) | |||
| Passwordless::SessionsController.new.send(:session_key, authenticatable_class) | |||
| touch(:claimed_at) | ||
| end | ||
|
|
||
| def claimed? | ||
| !!claimed_at | ||
| end | ||
|
|
||
| def valid_session? |
There was a problem hiding this comment.
session.valid_session? has a double session. Let's rename to session.valid? ?
| # Returns the {Passwordless::Session} (if set) from the session. | ||
| # @return [Session, nil] | ||
| def current_passwordless_session(authenticatable_class) | ||
| @current_passwordless_session ||= Passwordless::Session.find_by(id: session[session_key(authenticatable_class)]) |
There was a problem hiding this comment.
I don't really like to memoize on behalf of the user. My guess is it is only going to lead to weird, hard to figure out inconsistencies? Rose memoization is widely enough known that we can assume people know to do it if they want it.
The readme already suggests to memoize current_user which is, probably, mostly what these methods will be used for.
| # @see ModelHelpers#passwordless_with | ||
| def authenticate_by_session(authenticatable_class) | ||
| return unless current_passwordless_session(authenticatable_class)&.valid_session? | ||
| @current_authenticatable ||= current_passwordless_session(authenticatable_class).authenticatable |
There was a problem hiding this comment.
Same concern about memoization as above.
| build_passwordless_session(record).tap { |s| s.save! } | ||
| end | ||
|
|
||
| sign_out(authenticatable_class) |
There was a problem hiding this comment.
Is this to make sure to clear the old cookies?
| key = cookie_name(authenticatable_class) | ||
| cookies.encrypted.permanent[key] = {value: nil} | ||
| cookies.delete(key) | ||
| # /deprecated | ||
|
|
||
| reset_session |
|
Thanks a bunch for yet another great addition 💙💚💛💜❤️ ! |
|
@JoeSouthan I'm not seeing session persistence in my app using this. Sessions behave like session and reset between browser sessions, no matter what I set as |
|
Actually, I think that might've been due to this and not a session vs cookies thing: #61 |
Fixes #44
I've refactored the sign/in out flow to use the session rather than a cookie.
Couple of things of note:
authenticatableControllerHelpers.passwordless_foris now required to set up controllers with the correct resource to authenticate against, however I've made some of the other method params optional. Wondering if it's better to just remove the superfluous params?ControllerHelpers#authenticate_by_cookie, what do you think of removing it since it's a breaking change?Let me know what you think!