Feature for #27518, users can revoke active sessions again.
This MR would resolve #27518 (closed), restoring a user's ability to revoke active sessions.
Details
As discussed in #27518 (closed), the ability to revoke active sessions was temporarily removed for security reasons.
The Revoke button and associated controller action were removed in commit 038d5305.
Also, impersonated sessions were filtered from a user's list of active sessions in commit 44c4aad9.
This MR is a work in progress, as I am still writing the tests. The changes here simply reuse the code that was previously removed with one exception -- session IDs are no longer used within a webpage, or within a route, for revoking/destroying sessions. Instead, a new "public session ID" is used, see L26 in app/models/active_session.rb. The intent here is to completely avoid exposing session IDs.
As there are a number of UX and security implications that go along with these changes, I'm not expecting this MR is be merge-ready without further feedback.
Special Notes
Impersonated Sessions
Because the exposure of session IDs for impersonated sessions was the focal point of the security issue from before (as I read in #27518 (closed)), I made no attempts here to guess on how to handle the process of revoking impersonated sessions. That said, I explicitly prohibited impersonated sessions from being revoked through the destroy method called by the active sessions controller, see L107 in app/models/active_session.rb. I figured those decisions could be discussed in another issue.
I'm Not A Security Expert
My assumption here is that the lingering security concern was the continued exposure of session IDs to the user. By creating a UUID independent of the session id, and only exposing that UUID to the user, I believe that problem is resolved. I cannot foresee how this new UUID exposes any information about the session ID, but I will leave it to the security experts to provide a definitive answer, and perhaps an alternative solution.
Conformity
-
Changelog entry -
Documentation created/updated or follow-up review issue created -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Performance and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
-
Label as security and @ mention @gitlab-com/gl-security/appsec -
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods -
Security reports checked/validated by a reviewer from the AppSec team
Closes #27518 (closed)