fix(user token): Stop leaking API token #61941

ykamo001 · 2023-12-18T18:05:37Z

Update API contract to stop sending API token and send new field of lastTokenCharacters instead. Currently the API token in user settings can always be seen, even after creation. This experience also deviates from the org auth tokens. To keep both experiences consistent, and increase security around tokens from leaking, we are updating the API token contract and UI. The current PR focuses on BE, and updating the contract to send a field lastTokenCharacters

Serializer has an option to include full token value. This is to setup for the future to stop leaking the value in the API. Currently we need this for the delete endpoint to work. Future work captured in https://github.com/getsentry/team-enterprise/issues/21#issue-2047202292
The API contract for /api-tokens will include a new field of tokenLastCharacters

This PR should go after #61939

Resolves https://github.com/getsentry/team-enterprise/issues/1#issue-2003077670

src/sentry/api/serializers/models/apitoken.py

codecov · 2023-12-18T18:48:13Z

Codecov Report

Merging #61941 (0031657) into master (b545add) will decrease coverage by 0.01%.
Report is 41 commits behind head on master.
The diff coverage is 83.33%.

❗ Current head 0031657 differs from pull request most recent head 1e0a9bc. Consider uploading reports for the commit 1e0a9bc to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #61941      +/-   ##
==========================================
- Coverage   81.22%   81.21%   -0.01%     
==========================================
  Files        5185     5185              
  Lines      228404   228533     +129     
  Branches    38328    38371      +43     
==========================================
+ Hits       185511   185613     +102     
- Misses      37246    37265      +19     
- Partials     5647     5655       +8

Files	Coverage Δ
src/sentry/api/endpoints/api_tokens.py	`96.49% <ø> (ø)`
...atic/app/utils/analytics/replayAnalyticsEvents.tsx	`100.00% <ø> (ø)`
static/app/utils/replays/replayReader.tsx	`50.00% <ø> (ø)`
static/app/views/monitors/details.tsx	`0.00% <ø> (ø)`
src/sentry/api/serializers/models/apitoken.py	`89.47% <83.33%> (-4.28%)`	⬇️

... and 30 files with indirect coverage changes

ykamo001 · 2023-12-18T18:58:10Z

src/sentry/api/serializers/models/apitoken.py

@@ -27,6 +27,13 @@ def serialize(self, obj, attrs, user):
            "state": attrs.get("state"),
        }
        if not attrs["application"]:
-            data["token"] = obj.token
+            include_token = kwargs.get("include_token", True)


This should default to false in the future, but currently is true because other endpoints and services are also utilizing this serializer

rather than passing include_token in the kwargs, can we be explicit in the args, and just default it to False if that's what we expect?

This is related to the comment above, but in relation to the False matter:
Unfortunately we cannot default to False, as other API endpoints are referencing this serializer for their contract, and we would need to change more domain code than necessary for this change. This is the result of not using repository patterns and proper contracts/DTOs :/

Here's another service/endpoint that uses the same serliazier:

sentry/src/sentry/api/endpoints/integrations/sentry_apps/internal_app_token/index.py

Line 69 in cd40441

token = ApiTokenSerializer().serialize(api_token, attrs, request.user)

schew2381 · 2023-12-18T20:19:46Z

Could you write a test in test_apitoken.py that ensures the new serializer checks work as expected?

Update API contract to stop sending API token and send new field of lastTokenCharacters instead add comments and update logic default to true for other endpoints get last 4 elements instead of first 4 add test file and cases

ykamo001 · 2023-12-18T22:13:42Z

Could you write a test in test_apitoken.py that ensures the new serializer checks work as expected?

Yes of course! Added!

nhsiehgit

quick comment

nhsiehgit · 2023-12-18T22:49:43Z

src/sentry/api/serializers/models/apitoken.py

@@ -4,7 +4,7 @@

 @register(ApiToken)
 class ApiTokenSerializer(Serializer):
-    def get_attrs(self, item_list, user):
+    def get_attrs(self, item_list, user, **kwargs):


Do we need to capture the kwargs here?

Would rather be explicit in the api args, rather than allowing any kwargs to be passed

Unfortunately no, we cannot make a change the would break the contract of the base class function https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides

Other incompatible signature changes in method overrides, such as adding an extra required parameter, or removing an optional parameter, will also generate errors.

This fails on our linter as well, as I tried that initially https://github.com/getsentry/sentry/actions/runs/7227889927/job/19696388755

nhsiehgit · 2023-12-18T22:52:07Z

src/sentry/api/serializers/models/apitoken.py

@@ -27,6 +27,13 @@ def serialize(self, obj, attrs, user):
            "state": attrs.get("state"),
        }
        if not attrs["application"]:
-            data["token"] = obj.token
+            include_token = kwargs.get("include_token", True)


rather than passing include_token in the kwargs, can we be explicit in the args, and just default it to False if that's what we expect?

schew2381

small nits but lgtm otherwise 👍

schew2381 · 2023-12-18T23:06:47Z

tests/sentry/api/serializers/test_apitoken.py

+
+class TestApiTokenSerializer(TestCase):
+    def setUp(self) -> None:
+        self._user = self.create_user()


self.user is automatically set in TestCase so you can just use that directly in your tests which is convenient.

Same thing for self.project, self.team, and self.organization. The only difference is I think those are lazy so they only get created after you call it in the test.

This is good to know, thank you!

schew2381 · 2023-12-18T23:18:30Z

tests/sentry/api/serializers/test_apitoken.py

+        self._scopes = ["test_scope"]
+        self._token = self.create_user_auth_token(user=self._user, scope_list=self._scopes)


Sorry I realize I should've defaulted to empty scopes in create_user_auth_token if it wasn't relevant to the test

Suggested change

self._scopes = ["test_scope"]

self._token = self.create_user_auth_token(user=self._user, scope_list=self._scopes)

self._token = self.create_user_auth_token(user=self._user, scope_list=[])

That's okay, perfectly fine to be explicit about having empty scopes. Is that even a feasible token? Does that equate to unbounded (i.e. all permissions)?

We should probably add more tests here that make sure scopes are also returned correctly, so it's okay to keep a not empty list. This area in general needs more coverage, but something to follow up on!

We do allow empty scopes, the token would just be useless.

Is that something we should change?

hmm that's a good question. The only use case I can think of for having empty scopes is we allow users to create internal integrations without scopes. They're not required when you follow the creation flow.

I'm not super familiar with how people use integrations, or what you can do without a token/no scopes token. There might be a use case there

For user tokens, we don't allow it in the UI and you can't create user tokens through other tokens (which is a recent-ish change) so it should be fine.

It's not great though that it's not immediately obvious this is the case for user tokens and is implicit/localized knowledge

schew2381 · 2023-12-18T23:20:57Z

small nits but lgtm otherwise 👍

Oh whoops, didnt realize auto merge was on lol

ykamo001 self-assigned this Dec 18, 2023

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Dec 18, 2023

vercel bot deployed to Preview December 18, 2023 18:06 View deployment

ykamo001 commented Dec 18, 2023

View reviewed changes

src/sentry/api/serializers/models/apitoken.py Outdated Show resolved Hide resolved

ykamo001 force-pushed the enterprise-1-be branch from 49664b6 to 93023e3 Compare December 18, 2023 18:20

vercel bot deployed to Preview December 18, 2023 18:23 View deployment

ykamo001 force-pushed the enterprise-1-be branch from 93023e3 to 0031657 Compare December 18, 2023 18:50

vercel bot deployed to Preview December 18, 2023 18:52 View deployment

ykamo001 commented Dec 18, 2023

View reviewed changes

ykamo001 marked this pull request as ready for review December 18, 2023 19:51

ykamo001 requested a review from a team December 18, 2023 19:51

ykamo001 force-pushed the enterprise-1-be branch from 0031657 to 1e0a9bc Compare December 18, 2023 20:06

vercel bot deployed to Preview December 18, 2023 20:08 View deployment

fix(user token): Stop leaking API token

8829bbd

Update API contract to stop sending API token and send new field of lastTokenCharacters instead add comments and update logic default to true for other endpoints get last 4 elements instead of first 4 add test file and cases

ykamo001 force-pushed the enterprise-1-be branch from 1e0a9bc to 8829bbd Compare December 18, 2023 22:13

vercel bot deployed to Preview December 18, 2023 22:15 View deployment

ykamo001 mentioned this pull request Dec 18, 2023

fix(user token): Hide API token after creation #61884

Closed

nhsiehgit reviewed Dec 18, 2023

View reviewed changes

ykamo001 enabled auto-merge (squash) December 18, 2023 23:11

schew2381 approved these changes Dec 18, 2023

View reviewed changes

ykamo001 merged commit 633debb into master Dec 18, 2023
49 checks passed

ykamo001 deleted the enterprise-1-be branch December 18, 2023 23:20

github-actions bot locked and limited conversation to collaborators Jan 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(user token): Stop leaking API token #61941

fix(user token): Stop leaking API token #61941

ykamo001 commented Dec 18, 2023 •

edited

Loading

codecov bot commented Dec 18, 2023 •

edited

Loading

ykamo001 Dec 18, 2023

nhsiehgit Dec 18, 2023

ykamo001 Dec 18, 2023

ykamo001 Dec 18, 2023

schew2381 commented Dec 18, 2023 •

edited

Loading

ykamo001 commented Dec 18, 2023

nhsiehgit left a comment

nhsiehgit Dec 18, 2023

ykamo001 Dec 18, 2023

nhsiehgit Dec 18, 2023

schew2381 left a comment

schew2381 Dec 18, 2023

ykamo001 Dec 18, 2023

schew2381 Dec 18, 2023

ykamo001 Dec 18, 2023

ykamo001 Dec 18, 2023

schew2381 Dec 18, 2023

ykamo001 Dec 18, 2023

schew2381 Dec 18, 2023 •

edited

Loading

schew2381 Dec 18, 2023 •

edited

Loading

schew2381 commented Dec 18, 2023

		self._scopes = ["test_scope"]
		self._token = self.create_user_auth_token(user=self._user, scope_list=self._scopes)

fix(user token): Stop leaking API token #61941

fix(user token): Stop leaking API token #61941

Conversation

ykamo001 commented Dec 18, 2023 • edited Loading

codecov bot commented Dec 18, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schew2381 commented Dec 18, 2023 • edited Loading

ykamo001 commented Dec 18, 2023

nhsiehgit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schew2381 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schew2381 Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

schew2381 Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

schew2381 commented Dec 18, 2023

ykamo001 commented Dec 18, 2023 •

edited

Loading

codecov bot commented Dec 18, 2023 •

edited

Loading

schew2381 commented Dec 18, 2023 •

edited

Loading

schew2381 Dec 18, 2023 •

edited

Loading

schew2381 Dec 18, 2023 •

edited

Loading