Skip to content

Simplify session with trust check to only consider user#10290

Merged
aduth merged 8 commits intomainfrom
aduth-simplify-session-with-trust
Apr 3, 2024
Merged

Simplify session with trust check to only consider user#10290
aduth merged 8 commits intomainfrom
aduth-simplify-session-with-trust

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Mar 22, 2024

🛠 Summary of changes

Replaces session_with_trust? with user_signed_in?, removing consideration of current page.

Users::SessionController#new cannot be visited if a user is present (source), so the second part of this condition is redundant.

This also fixes an issue obfuscated by previous tests where the return value was not always a boolean but could instead return the full user object when present. It now always returns a boolean. Edit: Removed helper altogether in b2cb5a9.

📜 Testing Plan

Verify DAP is loaded only if signed out.

  1. Set participate_in_dap: true in config/application.yml
  2. Visit http://localhost:3000
  3. Inspect page to verify DAP is present
  4. Click "Forgot your password?"
  5. Inspect page to verify DAP is present
  6. Return to http://localhost:3000
  7. Click "Create an account"
  8. Inspect page to verify DAP is present
  9. Return to http://localhost:3000
  10. Sign in
  11. Inspect page to verify DAP is NOT present

@aduth
Copy link
Contributor Author

aduth commented Mar 25, 2024

I had a misunderstanding of the purpose of page_with_trust? here. The idea is to default to considering a page "trusted" to catch additional scenarios where a user may not be considered signed-in, but we still do not want to render DAP (e.g. new user confirmation).

In practice, this means that DAP is really only rendered on the "Sign In" page.

I think this should still be simplified, but I'm inclined to either (a) target the new session page more precisely (e.g. variable assigned from the controller action) or (b) reuse parts of #10292 if we want to generalize this idea of a "public, crawlable page".

@aduth aduth marked this pull request as draft March 25, 2024 12:58
@aduth aduth force-pushed the aduth-simplify-session-with-trust branch from b29fd2f to 083b746 Compare March 25, 2024 15:34
@aduth aduth marked this pull request as ready for review March 25, 2024 15:35
@aduth aduth marked this pull request as draft March 27, 2024 12:48
@aduth aduth force-pushed the aduth-simplify-session-with-trust branch from 083b746 to bfb83c7 Compare March 27, 2024 13:11
@aduth aduth marked this pull request as ready for review March 27, 2024 13:12
@aduth aduth force-pushed the aduth-simplify-session-with-trust branch from bfb83c7 to 8c9b7bd Compare April 2, 2024 19:35
@aduth aduth merged commit 6412664 into main Apr 3, 2024
@aduth aduth deleted the aduth-simplify-session-with-trust branch April 3, 2024 12:09
@aduth aduth mentioned this pull request Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants