Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set Current.session if signed-in in unauthenticated access #53220

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

t27duck
Copy link
Contributor

@t27duck t27duck commented Oct 7, 2024

Motivation / Background

This is related to the authentication generator.

Currently, if you call allow_unauthenticated_access in a controller, Current.session won't be set. This can result in rendered views making the user appear to be signed out if they are already signed in.

Detail

This updates allow_unauthenticated_access to set a before_action of resume_session to allow the user to still be signed in for requests that do not require authentication.

Additional information

Also included in this PR are two quick fixup commits:

  • Skip querying the Session model needlessly if the cookie isn't set.
  • SessionsController#destroy should redirect with see_other to match existing scaffolding controller destroy actions.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@rails-bot rails-bot bot added the railties label Oct 7, 2024
@t27duck t27duck changed the title Set Current.session if signed in in unauthenticated access Set Current.session if signed-in in unauthenticated access Oct 7, 2024
@excid3
Copy link
Contributor

excid3 commented Oct 7, 2024

We were discussing this in Discord.

For unauthenticated page views, the resume session will generate a query for User where id IS NULL that could also be skipped.

    def find_session_by_cookie
      if (id = cookies.signed[:session_id])
        Session.find_by(id: id)
      end
    end

Currently, if you call `allow_unauthenticated_access` in a controller,
`Current.session` won't be set. This can result in rendered views making
the user appear to be signed out if they are already signed in.

This updates `allow_unauthenticated_access` to set a `before_action` of
`resume_session` to allow the user to still be signed in for requests
that do not require authentication.
@gabrielso
Copy link
Contributor

Seems like this only happens if you call Current.session or Current.user directly in the view? You can just call the authenticated? helper in the view, that would give you the right behavior. Maybe style/opinion but I avoid calling models or modules straight in the view...

Instead of:

<% if Current.session %>
  show me this
<% end %>

Try:

<% if authenticated? %>
  show me this
<% end %>

If you call allow_unauthenticated_access I think it's fair that you can't rely on Current.session to exist? Then you only try to resume the session if the helper is called.

(see #53557)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants