feat: improved user auth tokens #68148
Conversation
github-actions
bot
added
the
Scope: Backend
mdtro
force-pushed
the
mdtro/apitoken-sntryu
branch
from
2ea65f7
to
b165afe
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #68148 +/- ##
==========================================
+ Coverage 78.75% 79.68% +0.93%
==========================================
Files 6428 6429 +1
Lines 285278 285379 +101
Branches 49136 49158 +22
==========================================
+ Hits 224681 227415 +2734
+ Misses 60229 57596 -2633
Partials 368 368
|
Bundle ReportChanges will increase total bundle size by 12.94kB ⬆️
|
mdtro
force-pushed
the
mdtro/apitoken-sntryu
branch
from
85a7a85
to
76f4c0e
Compare
mdtro
force-pushed
the
mdtro/apitoken-sntryu
branch
2 times, most recently
from
e82c0bd
to
29c3810
Compare
mdtro
force-pushed
the
mdtro/apitoken-sntryu
branch
from
29c3810
to
3a86fa6
Compare
| @@ -28,10 +29,98 @@ def default_expiration(): | |||
| return timezone.now() + DEFAULT_EXPIRATION | |||
|
|
|||
|
|
|||
| def generate_token(): | |||
| def generate_token(token_type: AuthTokenType | str | None = AuthTokenType.__empty__) -> str: | |||
There was a problem hiding this comment.
Do we want to support these many types? Also it looks like we default to this empty type, would we need the other types and None?
It might be helpful to add some doc strings to this function so people know how to use it and what the intention/difference is based on the input params
There was a problem hiding this comment.
I was running in to some issues with mypy on this when I originally just had the type as AuthTokenType.
The particular field on the class is a CharField so it's expecting a str.
Is there a way around this?
sentry/src/sentry/models/apitoken.py
Line 135 in 3a86fa6
There was a problem hiding this comment.
It just depends what you want to support in the method, and the caller has to make sure to appease the contract. What is the expected flow or contract we want to have?
There was a problem hiding this comment.
Ultimately, as we improve all of the various token types, I would like to require AuthTokenType. All newly generated tokens would be classified with this and we wouldn't allow a None. Right now, the None is there for backwards compatibility.
Since we've set choices on the CharField, then I suspect we will end up with a validation error if someone provides a string that doesn't match one of the choices. 🤔 I'll test that out as we progress through the token types.
- Setting the plaintext values on the manager class is wrong and would
cause issues when creating multiple instances of ApiToken.
- it results in the plaintext value always being the latest instance
of ApiToken that was created
- moving this to the model fixes the issue
- introduce setter functions for the plaintext token values
- add docstrings
- remove leading `_` on property functions that return the the plaintext
token values
- after reading, set the token to string value stored in `TOKEN_REDACTED` so
it can still be printed and we can search for the string in log data
where accidental leaks may happen
- we still throw `PlaintextSecretAlreadyRead` when attempting to read
the value more than once
- update tests to access correct property - ignore typing error
mdtro
force-pushed
the
mdtro/apitoken-sntryu
branch
from
0b2e227
to
42bb344
Compare
In the wizard endpoint, we’d reuse existing user auth tokens of the authenticated user if: 1. the user was part of multiple orgs (==> we can't create an org-based token) 2. AND we found one that satisfied the necessary permissions for sourcemap upload. With #68148 being merged, we cannot do this anymore. Plain user auth token values are only gonna be available directly after the token was created. For the fix, this PR makes a change to the wizard endpoint to always create a new user API token. This now works just like when we create an org token for single-org users. Closes: #69381 --------- Co-authored-by: Daniel Griesser <[email protected]>
In the wizard endpoint, we’d reuse existing user auth tokens of the authenticated user if: 1. the user was part of multiple orgs (==> we can't create an org-based token) 2. AND we found one that satisfied the necessary permissions for sourcemap upload. With #68148 being merged, we cannot do this anymore. Plain user auth token values are only gonna be available directly after the token was created. For the fix, this PR makes a change to the wizard endpoint to always create a new user API token. This now works just like when we create an org token for single-org users. Closes: #69381 --------- Co-authored-by: Daniel Griesser <[email protected]>
Supports getsentry/rfcs#32.
sntryu_.ApiTokento handle the unique creation logic where we need to hash the token values and store them.token_type(currently optional) parameter/field onApiTokenwhen creating user auth tokens to let the model do the heavy lifting on generating and hashing the values. This will keep the logic out of views and simplify calls to justnew_token = ApiToken.objects.create(user=user, token_type=AuthTokenType.USER).refreshTokenon applicable token types.I introduce a "read once" pattern in this PR for the token secrets to prevent leaking of them in logs, exceptions, etc. It works like this when creating a new
ApiToken:__plaintext_tokenand__plaintext_refresh_tokenthat store the respective plaintext values for temporary reading._plaintext_tokenproperty onApiToken(notice the single prepended underscore) the string value is returned and__plaintext_tokenis immediately set toNone._plaintext_tokenproperty again, an exception will be raised,PlaintextSecretAlreadyRead.Noneas I feel it's more explicit and will prevent unexpected behavior when we accidentally expect a value to be there.