[3/n] /v1/me/access-tokens list and delete
#8227
Merged
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
david-crespo
commented
May 27, 2025
| } | ||
|
|
||
| Ok(()) | ||
| } |
Contributor
Author
There was a problem hiding this comment.
This is a hard delete — might want to put that in the name, since it's unusual. We do that in the session delete method. If we wanted a soft delete, we could set time_expires to now instead. That feels kind of bad to me, but so does hard delete.
Member
There was a problem hiding this comment.
I think the hard delete is fine, setting time_expires feels kind of worse especially with regards to errors when that token is used. Adding hard_delete to the name is fine with me, if you think that's important.
This was referenced May 28, 2025
david-crespo
force-pushed
the
device-tokens-api
branch
from
0cd9b5a to
214811b
Compare
david-crespo
commented
May 29, 2025
| // typical authz check, instead effectively encoding the policy here that | ||
| // any user is allowed to list and delete their own tokens. When we add the | ||
| // ability for silo admins to list and delete tokens from any user, we will | ||
| // have to model these permissions properly in the polar policy. |
Contributor
Author
There was a problem hiding this comment.
This is important. We hack around the harder authz problem of modeling token properly by restricting these functions to the current actor (for now).
david-crespo
changed the title
/v1/me/tokens list and delete /v1/me/device-tokens list and delete
david-crespo
force-pushed
the
device-tokens-api
branch
2 times, most recently
from
f18022c to
6811931
Compare
david-crespo
force-pushed
the
silo-token-settings
branch
from
f68b227 to
3c36e26
Compare
david-crespo
force-pushed
the
device-tokens-api
branch
from
6811931 to
1467bfd
Compare
david-crespo
force-pushed
the
silo-token-settings
branch
from
3c36e26 to
26d25ab
Compare
david-crespo
force-pushed
the
device-tokens-api
branch
from
1467bfd to
3e15398
Compare
7 tasks
david-crespo
force-pushed
the
device-tokens-api
branch
3 times, most recently
from
66254ea to
9e014d0
Compare
david-crespo
force-pushed
the
silo-token-settings
branch
2 times, most recently
from
d2fc15e to
14c544c
Compare
david-crespo
force-pushed
the
device-tokens-api
branch
from
9e014d0 to
6772411
Compare
david-crespo
changed the title
/v1/me/device-tokens list and delete /v1/me/access-tokens list and delete
david-crespo
force-pushed
the
device-tokens-api
branch
from
6772411 to
7fa6757
Compare
david-crespo
added a commit
that referenced
this pull request
Jun 3, 2025
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
force-pushed
the
device-tokens-api
branch
from
7fa6757 to
fd19e2d
Compare
david-crespo
force-pushed
the
silo-token-settings
branch
from
14c544c to
4dd9321
Compare
hawkw
approved these changes
Jun 3, 2025
Member
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
this one is pretty straightforwards, no big concerns from me!
Comment on lines
+292
to
+293
| current_user_access_token_delete DELETE /v1/me/access-tokens/{token_id} | ||
| current_user_access_token_list GET /v1/me/access-tokens |
Member
There was a problem hiding this comment.
I think I'm in favor of putting this under /v1/me, personally!
| } | ||
|
|
||
| Ok(()) | ||
| } |
Member
There was a problem hiding this comment.
I think the hard delete is fine, setting time_expires feels kind of worse especially with regards to errors when that token is used. Adding hard_delete to the name is fine with me, if you think that's important.
david-crespo
force-pushed
the
device-tokens-api
branch
from
fd19e2d to
ab6ec2b
Compare
david-crespo
force-pushed
the
device-tokens-api
branch
from
ab6ec2b to
bbcc90d
Compare
david-crespo
added a commit
that referenced
this pull request
Jun 4, 2025
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.
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.
Built on top of #8137 and #8214.
This is only for a user to list and delete their own tokens. It doesn't quite match RFD 570, which says
/v1/device-tokensinstead of/v1/me/access-tokens, but it feels good under/v1/me, and after trying to make the UI too, I think "access tokens" is much more intuitive. If I stick with this, I will update RFD 570 to match.I'm not sure about the pathWent with/v1/device-tokens— in the API we call themDevice Access Tokens. I think/v1/access-tokensmight be more intuitive because thedeviceis sort of an implementation detail, it refers to the OAuth device auth flow, which we are using. In practice, the user just gets a token with the CLI and pastes a code into the web UI and they don't have to think too much about it, so exposing that detail in the name might not be worth it./v1/me/access-tokens.