feat: add `OrgAuthToken` model #50409

mydea · 2023-06-06T12:20:10Z

This adds a new OrgAuthToken model:

Tied to an organization
Has a token which is expected to be a JWT token
Add two basic utilities for token generation/parsing
It is possible to assign project(s) to a token

mydea · 2023-06-06T12:45:42Z

src/sentry/models/orgauthtoken.py

+ "sentry.Project", related_name="projects", through=OrgAuthTokenProject
+ )
+ # max JWT token length is 2048, +7 for our custom prefix (sntrys_)
+ token = models.CharField(max_length=2055, unique=True, null=False)


Not 100% sure what I should make the max length here. on the RFC there is a comment that we should store this in hashed form, if we do this I guess we need a more generic length here...?

@mitsuhiko why would we need to store it hashed?

see: getsentry/rfcs#91 (comment)

Great thanks! I supposed it was for security reasons then it makes sense.

Here we should make the length, the length of the hash. I don't expect we will need to store the token in any other format besides the hashed one.

Are you planning on storing the encoded JWT? or only the important claims within the token?

I updated this to be explicit that we store token_hashed in the DB.

codecov · 2023-06-06T13:02:53Z

Codecov Report

Merging #50409 (7518568) into master (12d0ad9) will increase coverage by 1.01%.
The diff coverage is 87.87%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #50409      +/-   ##
==========================================
+ Coverage   80.14%   81.15%   +1.01%     
==========================================
  Files        4843     4845       +2     
  Lines      203733   203799      +66     
  Branches    11130    11130              
==========================================
+ Hits       163275   165401    +2126     
+ Misses      40212    38152    -2060     
  Partials      246      246

Impacted Files	Coverage Δ
src/sentry/models/orgauthtoken.py	`86.66% <86.66%> (ø)`
src/sentry/utils/security/orgauthtoken_jwt.py	`90.00% <90.00%> (ø)`
src/sentry/models/__init__.py	`100.00% <100.00%> (ø)`

... and 113 files with indirect coverage changes

mydea · 2023-06-07T07:41:59Z

Note: I put these new models into the control silo, mirroring the api token model, I think that is the correct place?

iambriccardo · 2023-06-07T09:16:01Z

src/sentry/models/orgauthtokenproject.py

+
+
+@control_silo_only_model
+class OrgAuthTokenProject(Model):


You should put this model in orgauthtoken.py since it belongs to the same cluster of models related to org auth tokens.

after talking this through with @mitsuhiko , we decided to skip the projects for now!

Due to the nature of the relationship, it's not a big deal at all for the db.

iambriccardo · 2023-06-07T09:17:07Z

src/sentry/models/orgauthtoken.py

+ "sentry.Project", related_name="projects", through=OrgAuthTokenProject
+ )
+ # max JWT token length is 2048, +7 for our custom prefix (sntrys_)
+ token = models.CharField(max_length=2055, unique=True, null=False)


@mitsuhiko why would we need to store it hashed?

iambriccardo · 2023-06-07T09:18:14Z

src/sentry/models/orgauthtoken.py

+ # max JWT token length is 2048, +7 for our custom prefix (sntrys_)
+ token = models.CharField(max_length=2055, unique=True, null=False)
+ label = models.CharField(max_length=255, null=False)
+ scope_list = ArrayField(of=models.TextField)


Should we statically type the list of scopes? Having such a generic field is a bit suspicious.

for reference, this is how the scope list is currently stored in other models. But we may improve on this here :D

If possible i would like to bound it list of elements, maybe like:

from django.db import models from django.core.exceptions import ValidationError class MyModel(models.Model): OPTIONS = [ ('option1', 'Option 1'), ('option2', 'Option 2'), ('option3', 'Option 3'), ] choices_array = models.ArrayField( models.TextField(), validators=[], ) def validate_choices_array(value): for choice in value: if choice not in dict(MyModel.OPTIONS).keys(): raise ValidationError(f"{choice} is not a valid choice.") choices_array.validators.append(validate_choices_array)

Good point, I implemented this!

iambriccardo · 2023-06-07T09:18:45Z

src/sentry/models/orgauthtoken.py

+ label = models.CharField(max_length=255, null=False)
+ scope_list = ArrayField(of=models.TextField)
+
+ user_added = FlexibleForeignKey("sentry.User", null=True)


user_added refers to the User that created this token? If so, I would call this user or added_by.

yes, that's the idea - I'll rename it to added_by (IMHO user is a potentially confusing as it may imply this token belongs to that user, which it doesn't).

Oh yeah, that's a good point!

iambriccardo · 2023-06-07T09:20:09Z

src/sentry/models/orgauthtoken.py

+
+ user_added = FlexibleForeignKey("sentry.User", null=True)
+ date_added = models.DateTimeField(default=timezone.now, null=False)
+ date_last_used = models.DateTimeField(null=True)


Instead of having a null field, we could have it default to timezone.now and update it when the user uses it, but this changes the semantics. I don't know which ones are more representative of the situation.

IMHO it's useful to know that the token was never used, I'd say?

You could technically infer it if the two dates are equal but ofc it's not the best way. My main concern was that having nullability in the db is not very nice but I understand your point.

src/sentry/utils/security/orgauthtoken_jwt.py

markstory · 2023-06-07T13:42:52Z

src/sentry/models/orgauthtoken.py

+ "sentry.Project", related_name="projects", through=OrgAuthTokenProject
+ )
+ # max JWT token length is 2048, +7 for our custom prefix (sntrys_)
+ token = models.CharField(max_length=2055, unique=True, null=False)


Are you planning on storing the encoded JWT? or only the important claims within the token?

markstory · 2023-06-07T13:43:49Z

src/sentry/models/orgauthtoken.py

+ project_last_used = HybridCloudForeignKey(
+ "sentry.Project", null=True, blank=True, on_delete="cascade"
+ )


Are you sure you want cascading here? If the last used project is deleted should the org token be deleted?

Very good point @markstory, I missed this.

@mydea we can just put the project id here as a BoundedBigIntegerField and forget about possible accidental cascading deletions.

It's a good point, true! So it should def. not cascade, but should we use HubridCloudForeignKey or BoundedBigIntegerField then? Happy to use BoundedBigIntegerField, just wanting to make sure to get the hybrid could stuff right...!

(BTW also created_by should not cascade but set to null as well, adjusted that too!)

markstory · 2023-06-07T13:45:21Z

src/sentry/utils/security/orgauthtoken_jwt.py

+
+def generate_token(org_slug: str):
+ jwt_payload = {
+ "iss": "sentry.io",


Should this be use even when the instance is self-hosted/single-tenant?

yeah, valid question... took this from here: https://github.com/getsentry/rfcs/pull/91/files#diff-3109d1f30b4b81085e85d841deafefb0d1b43cc5345c64514475512a4ce9fdeeR94

IMHO it should be something consistent that says "this is from sentry", and not differ when it is self-hosted/single-tenant. Not sure if sentry.io is the best value for this then, but I'd say it's fine, maybe (just "Sentry" may also be ambiguous, ...)

markstory · 2023-06-07T13:47:04Z

src/sentry/utils/security/orgauthtoken_jwt.py

+ "iat": datetime.utcnow(),
+ "nonce": uuid4().hex,
+ "sentry_site": settings.SENTRY_OPTIONS["system.url-prefix"],
+ "sentry_org": org_slug,


I think you'll also need the organizations 'root' URL as in the future, we'll have multiple regions (with different API urls). While customer traffic can continue using https://sentry.io/api/0/** there will be latency overhead (and cost to us) so we should encourage using regional domains instead.

is this different from "sentry_site": settings.SENTRY_OPTIONS["system.url-prefix"],? that is what this is designed to do (but honestly I am not 100% sure what fields are what there, @mitsuhiko pointed me to this settings field)

So to be clear, the only point of the sentry_site field is exactly what you mentioned, if we should store something else in there, happy to be pointed to it!

In the future, organizations will have a couple URLs:

sentryUrl which is our root domain and is the same as system.url-prefix. ex. sentry.io

organizationUrl which is domain for the organization's UI. ex acme.sentry.io

regionUrl Which is the domain that the organization's API endpoints are available on. ex us.sentry.io

Any API requests sent to sentryUrl will be routed to the correct region, but will suffer a latency penalty (because we're proxying the request to the region) and the request will go to the US (this can matter for EU customers).

For this scenario, I think you'll want both the sentryUrl and regionUrl available in the JWT. There are a small number of API endpoints that are only available on sentryUrl (users, integrations, sentry apps).

where do I get regionUrl from? (I left a comment in the RFC as well to make sure this is aligned - I'd go with sentry_url and sentry_region_url then)

iambriccardo · 2023-06-07T13:50:23Z

src/sentry/models/orgauthtoken.py

+ project_last_used = HybridCloudForeignKey(
+ "sentry.Project", null=True, blank=True, on_delete="cascade"
+ )


Suggested change

project_last_used = HybridCloudForeignKey(

"sentry.Project", null=True, blank=True, on_delete="cascade"

)

project_last_used = BoundedBigIntegerFieldd(db_index=True)

is this preferred over the HybridCouldForeignKey? 🤔 Feels like that is designed for exactly this use case, but honestly not entirely sure... 😅

The other option would be to use HybridCloudForeignKey("sentry.Project", null=True, blank=True, on_delete="SET_NULL")

I don't have the expertise to judge what is better, since I lack in-depth knowledge of hybrid cloud. @markstory could you give an input here, thanks!

AniketDas-Tekky

Copying my comment from the issue:
#50144 (comment)

Basically, is there a tech spec for this? And have we properly investigated what introducing a new token type means?

github-actions · 2023-06-09T07:59:16Z

This PR has a migration; here is the generated SQL for src/sentry/migrations/0483_add_orgauthtoken.py ()

--
-- Create model OrgAuthToken
--
CREATE TABLE "sentry_orgauthtoken" ("id" bigserial NOT NULL PRIMARY KEY, "organization_id" bigint NOT NULL, "token_hashed" varchar(3000) NOT NULL UNIQUE, "token_last_characters" varchar(4) NULL, "name" varchar(255) NOT NULL, "scope_list" text[] NULL, "date_added" timestamp with time zone NOT NULL, "date_last_used" timestamp with time zone NULL, "project_last_used_id" bigint NULL, "date_deactivated" timestamp with time zone NULL, "created_by_id" integer NULL);
ALTER TABLE "sentry_orgauthtoken" ADD CONSTRAINT "sentry_orgauthtoken_created_by_id_5e2288f9_fk_auth_user_id" FOREIGN KEY ("created_by_id") REFERENCES "auth_user" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_orgauthtoken" VALIDATE CONSTRAINT "sentry_orgauthtoken_created_by_id_5e2288f9_fk_auth_user_id";
CREATE INDEX CONCURRENTLY "sentry_orgauthtoken_organization_id_17552a60" ON "sentry_orgauthtoken" ("organization_id");
CREATE INDEX CONCURRENTLY "sentry_orgauthtoken_token_hashed_4c36306d_like" ON "sentry_orgauthtoken" ("token_hashed" varchar_pattern_ops);
CREATE INDEX CONCURRENTLY "sentry_orgauthtoken_project_last_used_id_0b9aa639" ON "sentry_orgauthtoken" ("project_last_used_id");
CREATE INDEX CONCURRENTLY "sentry_orgauthtoken_created_by_id_5e2288f9" ON "sentry_orgauthtoken" ("created_by_id");

mydea · 2023-06-09T09:25:07Z

Copying my comment from the issue: #50144 (comment)

Basically, is there a tech spec for this? And have we properly investigated what introducing a new token type means?

Hey, there is an RFC getsentry/rfcs#91 by @mitsuhiko. If you have any concerns about this on a fundamental level, that's the place to discuss this, I guess. My understanding is that the basic decision that we want to add new org-level tokens has been taken, but @mitsuhiko can probably give you more information if you have more fundamental questions!

iambriccardo

PR LGTM but I can't judge the usage of the HybridCloudForeignKey. The only thing I can say is that semantically it makes sense.

src/sentry/utils/security/orgauthtoken_jwt.py

iambriccardo

PR LGTM but I can't judge the usage of the HybridCloudForeignKey. The only thing I can say is that semantically it makes sense.

pyproject.toml

file isn't in the review any more

github-actions · 2023-06-14T07:29:18Z

This PR has a migration; here is the generated SQL for src/sentry/migrations/0487_add_orgauthtoken.py ()

--
-- Create model OrgAuthToken
--
CREATE TABLE "sentry_orgauthtoken" ("id" bigserial NOT NULL PRIMARY KEY, "organization_id" bigint NOT NULL, "token_hashed" text NOT NULL UNIQUE, "token_last_characters" varchar(4) NULL, "name" varchar(255) NOT NULL, "scope_list" text[] NULL, "date_added" timestamp with time zone NOT NULL, "date_last_used" timestamp with time zone NULL, "project_last_used_id" bigint NULL, "date_deactivated" timestamp with time zone NULL, "created_by_id" integer NULL);
ALTER TABLE "sentry_orgauthtoken" ADD CONSTRAINT "sentry_orgauthtoken_created_by_id_5e2288f9_fk_auth_user_id" FOREIGN KEY ("created_by_id") REFERENCES "auth_user" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_orgauthtoken" VALIDATE CONSTRAINT "sentry_orgauthtoken_created_by_id_5e2288f9_fk_auth_user_id";
CREATE INDEX CONCURRENTLY "sentry_orgauthtoken_organization_id_17552a60" ON "sentry_orgauthtoken" ("organization_id");
CREATE INDEX CONCURRENTLY "sentry_orgauthtoken_token_hashed_4c36306d_like" ON "sentry_orgauthtoken" ("token_hashed" text_pattern_ops);
CREATE INDEX CONCURRENTLY "sentry_orgauthtoken_project_last_used_id_0b9aa639" ON "sentry_orgauthtoken" ("project_last_used_id");
CREATE INDEX CONCURRENTLY "sentry_orgauthtoken_created_by_id_5e2288f9" ON "sentry_orgauthtoken" ("created_by_id");

github-actions · 2023-06-14T10:27:48Z

This PR has a migration; here is the generated SQL for src/sentry/migrations/0488_add_orgauthtoken.py ()

--
-- Create model OrgAuthToken
--
CREATE TABLE "sentry_orgauthtoken" ("id" bigserial NOT NULL PRIMARY KEY, "organization_id" bigint NOT NULL, "token_hashed" text NOT NULL UNIQUE, "token_last_characters" varchar(4) NULL, "name" varchar(255) NOT NULL, "scope_list" text[] NULL, "date_added" timestamp with time zone NOT NULL, "date_last_used" timestamp with time zone NULL, "project_last_used_id" bigint NULL, "date_deactivated" timestamp with time zone NULL, "created_by_id" integer NULL);
ALTER TABLE "sentry_orgauthtoken" ADD CONSTRAINT "sentry_orgauthtoken_created_by_id_5e2288f9_fk_auth_user_id" FOREIGN KEY ("created_by_id") REFERENCES "auth_user" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_orgauthtoken" VALIDATE CONSTRAINT "sentry_orgauthtoken_created_by_id_5e2288f9_fk_auth_user_id";
CREATE INDEX CONCURRENTLY "sentry_orgauthtoken_organization_id_17552a60" ON "sentry_orgauthtoken" ("organization_id");
CREATE INDEX CONCURRENTLY "sentry_orgauthtoken_token_hashed_4c36306d_like" ON "sentry_orgauthtoken" ("token_hashed" text_pattern_ops);
CREATE INDEX CONCURRENTLY "sentry_orgauthtoken_project_last_used_id_0b9aa639" ON "sentry_orgauthtoken" ("project_last_used_id");
CREATE INDEX CONCURRENTLY "sentry_orgauthtoken_created_by_id_5e2288f9" ON "sentry_orgauthtoken" ("created_by_id");

I think this has been addressed!

mydea added the Impact: Migration label Jun 6, 2023

mydea requested review from mitsuhiko, oioki and iambriccardo June 6, 2023 12:20

mydea self-assigned this Jun 6, 2023

mydea requested a review from a team as a code owner June 6, 2023 12:20

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

vercel bot deployed to Preview June 6, 2023 12:34 View deployment

mydea commented Jun 6, 2023

View reviewed changes

mydea force-pushed the fn/org-auth-token branch from 69eb8a8 to 5d1c4f2 Compare June 7, 2023 07:41

vercel bot deployed to Preview June 7, 2023 07:45 View deployment

vercel bot deployed to Preview June 7, 2023 08:39 View deployment

iambriccardo reviewed Jun 7, 2023

View reviewed changes

vercel bot deployed to Preview June 7, 2023 09:36 View deployment

mydea force-pushed the fn/org-auth-token branch from f730e99 to 6c8d77e Compare June 7, 2023 10:00

vercel bot deployed to Preview June 7, 2023 10:02 View deployment

vercel bot deployed to Preview June 7, 2023 10:54 View deployment

mydea force-pushed the fn/org-auth-token branch from 3d7e3f3 to 0c250f3 Compare June 7, 2023 12:40

vercel bot deployed to Preview June 7, 2023 12:46 View deployment

getsentry deleted a comment from github-actions bot Jun 7, 2023

markstory reviewed Jun 7, 2023

View reviewed changes

iambriccardo reviewed Jun 7, 2023

View reviewed changes

AniketDas-Tekky previously requested changes Jun 8, 2023

View reviewed changes

mydea force-pushed the fn/org-auth-token branch from 3c20c78 to fcd2ccc Compare June 9, 2023 07:56

vercel bot deployed to Preview June 9, 2023 08:00 View deployment

vercel bot deployed to Preview June 9, 2023 08:42 View deployment

mydea mentioned this pull request Jun 12, 2023

Simplified Upload Tokens for CI Usage getsentry/rfcs#91

Merged

mydea force-pushed the fn/org-auth-token branch from 931a721 to 87942c4 Compare June 12, 2023 09:17

vercel bot deployed to Preview June 12, 2023 09:20 View deployment

mydea force-pushed the fn/org-auth-token branch from 87942c4 to e05d994 Compare June 12, 2023 11:24

vercel bot deployed to Preview June 12, 2023 11:27 View deployment

mydea requested a review from iambriccardo June 12, 2023 11:53

iambriccardo reviewed Jun 12, 2023

View reviewed changes

src/sentry/utils/security/orgauthtoken_jwt.py Outdated Show resolved Hide resolved

src/sentry/utils/security/orgauthtoken_jwt.py Outdated Show resolved Hide resolved

iambriccardo reviewed Jun 12, 2023

View reviewed changes

vercel bot deployed to Preview June 12, 2023 12:29 View deployment

asottile-sentry previously requested changes Jun 12, 2023

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

mydea force-pushed the fn/org-auth-token branch from ee532cd to 3e3b67a Compare June 12, 2023 13:12

vercel bot deployed to Preview June 12, 2023 13:14 View deployment

mydea mentioned this pull request Jun 12, 2023

Adjustments to RFC #91 getsentry/rfcs#102

Merged

mydea force-pushed the fn/org-auth-token branch from 3e3b67a to bb55897 Compare June 12, 2023 14:14

vercel bot deployed to Preview June 12, 2023 14:17 View deployment

mydea force-pushed the fn/org-auth-token branch from bb55897 to bee4489 Compare June 14, 2023 07:25

iambriccardo self-requested a review June 14, 2023 07:28

iambriccardo approved these changes Jun 14, 2023

View reviewed changes

vercel bot deployed to Preview June 14, 2023 07:29 View deployment

mydea added 2 commits June 14, 2023 12:23

feat: Add OrgAuthToken model

45db4ce

add migration

7518568

mydea force-pushed the fn/org-auth-token branch from bee4489 to 7518568 Compare June 14, 2023 10:24

vercel bot deployed to Preview June 14, 2023 10:27 View deployment

mydea merged commit fdc8f5a into master Jun 14, 2023
54 of 55 checks passed

mydea deleted the fn/org-auth-token branch June 14, 2023 12:17

github-actions bot locked and limited conversation to collaborators Jun 29, 2023

feat: add OrgAuthToken model #50409

feat: add OrgAuthToken model #50409

Conversation

mydea commented Jun 6, 2023 • edited

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

codecov bot commented Jun 6, 2023 • edited

Codecov Report

mydea commented Jun 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iambriccardo Jun 7, 2023 • edited

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iambriccardo Jun 12, 2023 • edited

Choose a reason for hiding this comment

iambriccardo Jun 12, 2023 • edited

Choose a reason for hiding this comment

AniketDas-Tekky left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 9, 2023 • edited

mydea commented Jun 9, 2023

iambriccardo left a comment

Choose a reason for hiding this comment

iambriccardo left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 14, 2023

github-actions bot commented Jun 14, 2023

feat: add `OrgAuthToken` model #50409

feat: add `OrgAuthToken` model #50409

mydea commented Jun 6, 2023 •

edited

codecov bot commented Jun 6, 2023 •

edited

iambriccardo Jun 7, 2023 •

edited

iambriccardo Jun 12, 2023 •

edited

iambriccardo Jun 12, 2023 •

edited

github-actions bot commented Jun 9, 2023 •

edited