[WIP] Start drafting sudo mode RFC #10653
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
A couple of thoughts:
|
sampoder
reviewed
Jun 18, 2025
Comment on lines
+81
to
+85
| **Option 1 - Treat it as a new `Login`** | ||
|
|
||
| Given that we have an existing flow for authenticating users, we could treat re-authentication as a new login flow which, once complete, swaps in a new `UserSession`. | ||
|
|
||
| We would ideally link the new `Login` to the previous one so we can distinguish it as a re-authentication, apply different rules (e.g. only requiring one factor), and maintain a chain of `Login` records (for auditing purposes). |
There was a problem hiding this comment.
I think this a good idea, we could do something like voided_by_id to store the ID of the past Login
sampoder
reviewed
Jun 18, 2025
|
|
||
| <img src="login_notification_email.png" width="200"/> | ||
| 3. How do we want to handle sudo actions for users that are being impersonated? Is re-authenticating the impersonator enough? | ||
| - _Opinion_: We should strongly consider making this question go away by capping impersonated sessions to 2 hours. I don't think there's a use case for having these persist for longer than that. |
sampoder
reviewed
Jun 18, 2025
| <img src="login_notification_email.png" width="200"/> | ||
| 3. How do we want to handle sudo actions for users that are being impersonated? Is re-authenticating the impersonator enough? | ||
| - _Opinion_: We should strongly consider making this question go away by capping impersonated sessions to 2 hours. I don't think there's a use case for having these persist for longer than that. | ||
| 4. Should re-authentications appear in activities? |
sampoder
reviewed
Jun 18, 2025
| 1. We currently show users a list of their sessions including how long ago they logged in. Should we also surface information about re-authentications? | ||
|
|
||
| <img src="sessions.png" width="200"/> | ||
| 2. We send users an email when there is a new login on their account. Would we want to do the same for re-authentications? |
There was a problem hiding this comment.
We normally only do this if the context is unrecognised, I think it's ok to not do this
sampoder
reviewed
Jun 18, 2025
|
|
||
| **Open questions** | ||
|
|
||
| 1. We currently show users a list of their sessions including how long ago they logged in. Should we also surface information about re-authentications? |
There was a problem hiding this comment.
Yes but imo we should flatten them into one Login
This was referenced Jun 19, 2025
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jun 24, 2025
## Summary of the problem Admin users have the ability to impersonate other users for debugging and support purposes. However while thinking through the implications of implementing a sudo mode (see #10653 (comment)) we realized these impersonated sessions live for as long as the impersonated user's `session_duration_seconds`, which could be much longer than necessary. ## Describe your changes - `spec/rails_helper.rb` now eagerly-loads all files in `spec/support` so we can put shared code in there - Added `SessionSupport` module to make writing controller tests easier - Updated the `:user_session` factory to populate the `session_token` field - Updated the logic in `SessionHelper#sign_in` to cap impersonated sessions to one hour - Added a test for the above --------- Co-authored-by: Manu G <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of the problem
#6346
Describe your changes
Rendered