Skip to content

[4/n] Let user request a specific TTL for a token #8231

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
david-crespo merged 4 commits into from
Jun 4, 2025
Merged

[4/n] Let user request a specific TTL for a token #8231

david-crespo merged 4 commits into from
Jun 4, 2025

Conversation

@david-crespo
Copy link
Contributor

@david-crespo david-crespo commented May 28, 2025 •
edited
Loading

Closes #8147.

Built on #8137, #8214, and #8227.

This is pretty straightforward, I think. The user gives us a TTL in seconds at token request time. We store it on the request row. When they come back in the later step to confirm the code and generate the token, we retrieve the TTL, validate that it is less than the silo max (if one is set), and we use it to generate the time_expires timestamp, which cannot be changed later.

One slightly surprising bit is that we can't validate the TTL against the silo max at initial request time because we don't have any idea what silo the user is associated with until the confirm step. So probably want to make sure we are handling TTL validation errors nicely in the web console, because I think that's where they will show up.

@david-crespo david-crespo force-pushed the device-auth-ttl branch 2 times, most recently from 67e596d to 1f03596 Compare May 29, 2025 15:23
@david-crespo david-crespo mentioned this pull request May 29, 2025
Merged
7 tasks
@david-crespo david-crespo force-pushed the device-auth-ttl branch 2 times, most recently from fb2efbe to 72c81dd Compare May 29, 2025 22:25
@david-crespo david-crespo marked this pull request as ready for review May 29, 2025 22:54
@david-crespo david-crespo force-pushed the device-auth-ttl branch 2 times, most recently from e9b8ab1 to 28485f3 Compare May 30, 2025 16:56
@david-crespo david-crespo mentioned this pull request May 31, 2025
Merged
7 tasks
@david-crespo david-crespo force-pushed the device-tokens-api branch 2 times, most recently from f6a9934 to 66254ea Compare June 2, 2025 15:22
@david-crespo david-crespo force-pushed the device-auth-ttl branch 2 times, most recently from 2c789de to 81eccee Compare June 2, 2025 21:34
@david-crespo david-crespo force-pushed the device-tokens-api branch 2 times, most recently from 9e014d0 to 6772411 Compare June 3, 2025 00:01
@david-crespo
Copy link
Contributor Author

david-crespo commented Jun 3, 2025

Good point, Gemini! It would make sense to use NonZeroU32 for the token TTL, like we do for the silo setting.

https://gist.github.com/david-crespo/adc7e570eb0341f8529f505972756873

image

@david-crespo david-crespo mentioned this pull request Jun 3, 2025
Merged
david-crespo added a commit that referenced this pull request Jun 3, 2025
@david-crespo
[1/n] Add id column on tokens and sessions (#8137)
c0f8d23 
Closes #8139. Token IDs are used in #8227 and #8231 for token CRUD.

WIP. Everything compiles and existing tests work, but we are not yet
testing retrieval by ID and we need to think about authz checks on the
datastore functions for retrieving tokens and sessions by token string.

This PR would be a lot shorter if not for the fact that the `token`
column was the primary key on both tables, so adding the `id` requires a
bunch of migration steps to change the primary key. There were also a
lot of changes to code and tests around making things ID-centric instead
of token-centric.

* [x] SQL migrations for adding `id` col and changing primary key to
that
* [x] Add `TypedUuid` to session and token DB models
* [x] Update `authz_resource!` and `lookup_resource!` calls for new
primary key
* [x] Update `Session` trait methods to use `id` instead of `token`
* [x] Update app and datastore session and token methods to use ID
instead of token
* [x] Update two distinct but nearly identical `FakeSession`
implementations (one for unit tests, one for integration tests) to a)
track sessions in a `Vec` instead of a `HashMap` with token keys
* [x] Figure out authz situation in new fetch session/token by token
string methods
@david-crespo david-crespo requested a review from hawkw June 3, 2025 17:11
@david-crespo david-crespo mentioned this pull request Jun 3, 2025
Merged
hawkw
hawkw approved these changes Jun 3, 2025
View reviewed changes
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.

some minor nits, but seems good to me!

Base automatically changed from device-tokens-api to main June 4, 2025 04:52
@david-crespo david-crespo merged commit 1b08ebd into main Jun 4, 2025
17 checks passed
@david-crespo david-crespo deleted the device-auth-ttl branch June 4, 2025 12:28
david-crespo added a commit that referenced this pull request Jun 4, 2025
@david-crespo
[5/n] Expire sessions by token again (#8260)
99ffcbe 
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.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hawkw hawkw hawkw approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Let user specify access token TTL at create time

3 participants

@david-crespo @hawkw