Skip to content

Conversation

david-crespo
Copy link
Contributor

@david-crespo david-crespo commented Jun 3, 2025

Built on top of #8231. Fix for an issue with the internals of session expiration discussed in #8137 (comment).

When I added ID primary keys to sessions and tokens, I changed the session delete method on the Session trait to operate by ID as well, and that meant a more complex authz situation compared to deleting by token. A bearer token is its own authz, at least according to the implicit policy we decided on. When I changed it to delete by ID, I made the delete function (now removed in this PR) use the user ID from the opctx to filter tokens to ensure that users could only delete their own sessions.

The problem is that the opctx used in that case is the external authenticator op context, which has its own special actor that is distinct from the user in question. This means we would never actually delete a token this way because the query would always come up empty. This is not actually a problem from a correctness point of view because on subsequent requests we always check the expiration time on the session we pull out of the DB anyway. But it does mean that we probably end up with a lot more sessions piling up in the DB than we otherwise would. Not a big deal either way, but I feel more comfortable having it keep working the way it has worked for several years.

It's also nice that we are deleting some app code here and replacing it with a test for a behavior that was previously untested. (I added the test in the first commit and confirmed that it fails without the changes from the second commit.)

@david-crespo david-crespo requested a review from hawkw June 3, 2025 20:00
@david-crespo david-crespo force-pushed the session-expire-by-token branch 2 times, most recently from 0ed648a to 6e444f1 Compare June 3, 2025 21:53
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, so you really weren't kidding when you said this was mostly just deleting app code. Seems good to me!

@david-crespo david-crespo force-pushed the session-expire-by-token branch from 6e444f1 to 5bb08cc Compare June 4, 2025 01:36
@david-crespo david-crespo force-pushed the device-auth-ttl branch 2 times, most recently from 9ba7e6c to b2a452d Compare June 4, 2025 05:06
@david-crespo david-crespo force-pushed the session-expire-by-token branch from 5bb08cc to a18a3bb Compare June 4, 2025 05:06
Base automatically changed from device-auth-ttl to main June 4, 2025 12:28
@david-crespo david-crespo force-pushed the session-expire-by-token branch from a18a3bb to fa08824 Compare June 4, 2025 12:46
@david-crespo david-crespo enabled auto-merge (squash) June 4, 2025 12:46
@david-crespo david-crespo merged commit 99ffcbe into main Jun 4, 2025
16 checks passed
@david-crespo david-crespo deleted the session-expire-by-token branch June 4, 2025 16:04
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