Add basic sessions generator #52328
Add basic sessions generator #52328
Conversation
|
This is not meant to be a system where all factors are accounted for. User is the most common model name for the credentials holder. An account has_many users, but adding an account model is an exercise for the programmer. |
Maybe, it would be more sensible to generate resets by default, and have |
|
Genuine questions: why use a separate |
|
@smitssjors Because then you can do session indexes, alert when new sessions are created from unknown locations, do statistics on user agents, etc. Session tokens in the DB with a drip of metadata is a wonderful pattern for all of this, and something the majority of applications should use. |
|
Actually, will tackle the idea of a password reset mailer/flow as a separate upgrade, if necessary. There are a significant number of views involved with that flow. On the fence about whether that's a good fit for a generator like this. |
|
Unrelated test failures (again!). |
|
Wdyt about enabling automatic squashing when merging PRs? |
* Add basic sessions generator * Excess CR * Use required fields * Add sessions generator test * Fix generated migration * Test migration content * Appease rubocop * Add CHANGELOG
| @@ -0,0 +1,64 @@ | |||
| module Authentication | |||
There was a problem hiding this comment.
Why isn't this called Authenticate like in 37signals apps? Makes more sense imo since controllers are almost always plural, and they authenticate requests versus the singular more object-oriented approach for models.
There was a problem hiding this comment.
The module is named Authentication because it covers everything related to authentication, not just a single action. Rails prefers naming modules after what they do overall, which makes the code easier to understand and maintain.
Rails conventionally prefers naming modules and concerns in a way that represents concepts or groups of behaviours. This makes the naming consistent with other modules such as Authorization, Logging, Monitoring, etc.
Besides, Writebook name their model Authentication.
| rate_limit to: 10, within: 3.minutes, only: :create, with: -> { redirect_to new_session_url, alert: "Try again later." } | ||
|
|
||
| def new | ||
| redirect_to root_url if authenticated? |
There was a problem hiding this comment.
I don't think this redirect will work. It looks like Current.session is only being set through require_authentication which gets skipped when using allow_unauthenticated_access. So Current.session would always end up nil here.
|
|
||
| def start_new_session_for(user) | ||
| user.sessions.create!(user_agent: request.user_agent, ip_address: request.remote_ip).tap do |session| | ||
| set_current_session session |
There was a problem hiding this comment.
Nitpick: The name session could be confused with request's session object. Should it be called current_session for clarity ?
Same for set_current_session(session)
There was a problem hiding this comment.
Yup, in fact authenticated? seems pointless with this implementation. There's 3 paths here
- require_authentication && resume > authenticated? is always true
- require_authentication && request > it redirects to login
- skip_authentication > authenticated? is always false
Seems like resume session needs to be called regardless, and request_authentication is the skippable part
In my setup, which is very similar to this I have 3 before action type helpers
require_authentication (resume || redirect_to new_session)
allow_authentication (resume)
despise_authentication (resume && redirect_to root)
require covers most of the cases, allow is useful in some areas where it's not necessary but we don't forbid, and despise will kick you out if your signed in. Think invitation/signup type controllers.
|
Why store a long string in database and then run a sequential find by query for finding a Session? @dhh Why can't we use signed_id for this purpose here so that the look up would be done using the integer primary key? So, we can drop the token column for sessions and instead of doing Session.find_by(token: token)
# and
cookies.signed.permanent[:session_token] = { value: session.token, httponly: true, same_site: :lax }we could do Session.find_signed(token)
# and
cookies.signed.permanent[:session_token] = { value: session.signed_id, httponly: true, same_site: :lax }PR => #52504 |
* Add basic sessions generator * Excess CR * Use required fields * Add sessions generator test * Fix generated migration * Test migration content * Appease rubocop * Add CHANGELOG
|
Could it be possible to change what id is for us who prefer uuid v4, without hassle with migrations? The generator seems to assume everyone uses sequential bigint for ids. |
|
mongo? |
| end | ||
|
|
||
| private | ||
| def authenticated? |
There was a problem hiding this comment.
We need to adjust the indentation in this file
I agree with the |
Adds a basic sessions generator to get people started with their own authentication system. This is not intended to be an all-singing, all-dancing answer to every possible authentication concern. It's merely intended to illuminate the basic path, and reveal that rolling your own authentication system is not some exotic adventure.
So do not expect magic links or passkeys or 2FA. That's not going to happen with this generator.
Outstanding work: