Improve authentication #1348
Improve authentication #1348
Conversation
This adds the GDS SSO shared "linting" examples to make sure the `User` model satisfies its contract.
This is a model concern and shouldn't be on the controller. Also adds tests.
This is neater, and will let us re-use it in request specs too.
This adds a request spec that tests that all `ApplicationController` descendants actually enforce authentication.
This creates an idiomatic Rails per-request attributes singleton `Current`, using [`ActiveSupport::CurrentAttributes`], and stores the current user in it. This will allow us to access the currently active user in cross-cutting concerns, without having to deeply pass it around. `CurrentAttributes` is a "sharp tool" that should be used with caution as it creates global state for a request, but is an ideal tool for dealing with auditing and logging concerns (which we'll soon introduce) where you need to have global awareness of certain request attributes outside of just the controller layer. We have prior art for this pattern, for example in Whitehall. [`ActiveSupport::CurrentAttributes`]: https://edgeapi.rubyonrails.org/classes/ActiveSupport/CurrentAttributes.html
| @@ -0,0 +1,14 @@ | |||
| # Enforces user authentication for a controller and stores the authenticated user in `Current` | |||
| module AuthenticatesUser | |||
There was a problem hiding this comment.
To anticipate a conversation we've had before: the default naming convention for concerns tends to be _____able, but I do like to diverge from that in two cases:
- something sets up a relationship (where I'd go for
Has_____), or - non-model concerns, which tend to be a more diverse bunch (like here)
DoesThing (AuthenticatesUser) feels natural here to me but I'm open for debate!
I'll point to Gitlab's controller concerns as some prior art, with concerns like:
AuthenticatesWithTwoFactorChecksCollaborationRendersNotes
There was a problem hiding this comment.
To be fair -able doesn't really fit with Authenticate, so I have no concerns (see what I did there?) about the name AuthenticatesUser.
Thanks for explaining your choice though 🎉
| @@ -0,0 +1,14 @@ | |||
| # Enforces user authentication for a controller and stores the authenticated user in `Current` | |||
| module AuthenticatesUser | |||
There was a problem hiding this comment.
To be fair -able doesn't really fit with Authenticate, so I have no concerns (see what I did there?) about the name AuthenticatesUser.
Thanks for explaining your choice though 🎉
This improves various aspects of the authentication logic in this app, including:
Currentsingleton throughActiveSupport::CurrentAttributesto store the user for cross-cutting concerns