Uh oh!
There was an error while loading. Please reload this page.
feat: add OrgAuthToken model - #50409
Conversation
There was a problem hiding this comment.
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...?
There was a problem hiding this comment.
There was a problem hiding this 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.
There was a problem hiding this comment.
Are you planning on storing the encoded JWT? or only the important claims within the token?
There was a problem hiding this comment.
I updated this to be explicit that we store token_hashed in the DB.
Codecov Report
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
|
mydea
commented
Jun 7, 2023
Note: I put these new models into the control silo, mirroring the api token model, I think that is the correct place? |
There was a problem hiding this comment.
You should put this model in orgauthtoken.py since it belongs to the same cluster of models related to org auth tokens.
There was a problem hiding this comment.
after talking this through with @mitsuhiko , we decided to skip the projects for now!
There was a problem hiding this comment.
Due to the nature of the relationship, it's not a big deal at all for the db.
There was a problem hiding this comment.
Should we statically type the list of scopes? Having such a generic field is a bit suspicious.
There was a problem hiding this comment.
for reference, this is how the scope list is currently stored in other models. But we may improve on this here :D
There was a problem hiding this comment.
If possible i would like to bound it list of elements, maybe like:
fromdjango.dbimportmodelsfromdjango.core.exceptionsimportValidationErrorclassMyModel(models.Model):
OPTIONS= [
('option1', 'Option 1'),
('option2', 'Option 2'),
('option3', 'Option 3'),
]
choices_array=models.ArrayField(
models.TextField(),
validators=[],
)
defvalidate_choices_array(value):
forchoiceinvalue:
ifchoicenotindict(MyModel.OPTIONS).keys():
raiseValidationError(f"{choice} is not a valid choice.")
choices_array.validators.append(validate_choices_array)There was a problem hiding this comment.
user_added refers to the User that created this token? If so, I would call this user or added_by.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Oh yeah, that's a good point!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
IMHO it's useful to know that the token was never used, I'd say?
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Are you planning on storing the encoded JWT? or only the important claims within the token?
There was a problem hiding this comment.
Are you sure you want cascading here? If the last used project is deleted should the org token be deleted?
There was a problem hiding this comment.
@mydea we can just put the project id here as a BoundedBigIntegerField and forget about possible accidental cascading deletions.
There was a problem hiding this comment.
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...!
There was a problem hiding this comment.
(BTW also created_by should not cascade but set to null as well, adjusted that too!)
There was a problem hiding this comment.
Should this be use even when the instance is self-hosted/single-tenant?
There was a problem hiding this comment.
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, ...)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
In the future, organizations will have a couple URLs:
sentryUrlwhich is our root domain and is the same assystem.url-prefix. ex.sentry.ioorganizationUrlwhich is domain for the organization's UI. exacme.sentry.ioregionUrlWhich is the domain that the organization's API endpoints are available on. exus.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).
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
| project_last_used=HybridCloudForeignKey( | |
| "sentry.Project", null=True, blank=True, on_delete="cascade" | |
| ) | |
| project_last_used=BoundedBigIntegerFieldd(db_index=True) |
There was a problem hiding this comment.
is this preferred over the HybridCouldForeignKey? 🤔 Feels like that is designed for exactly this use case, but honestly not entirely sure... 😅
There was a problem hiding this comment.
The other option would be to use HybridCloudForeignKey("sentry.Project", null=True, blank=True, on_delete="SET_NULL")
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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?
This PR has a migration; here is the generated SQL for ---- Create model OrgAuthToken--CREATETABLE "sentry_orgauthtoken" ("id"bigserialNOT NULLPRIMARY KEY, "organization_id"bigintNOT 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 zoneNOT NULL, "date_last_used"timestamp with time zoneNULL, "project_last_used_id"bigintNULL, "date_deactivated"timestamp with time zoneNULL, "created_by_id"integerNULL);
ALTERTABLE"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;
ALTERTABLE"sentry_orgauthtoken" VALIDATE CONSTRAINT"sentry_orgauthtoken_created_by_id_5e2288f9_fk_auth_user_id";
CREATEINDEXCONCURRENTLY"sentry_orgauthtoken_organization_id_17552a60"ON"sentry_orgauthtoken" ("organization_id");
CREATEINDEXCONCURRENTLY"sentry_orgauthtoken_token_hashed_4c36306d_like"ON"sentry_orgauthtoken" ("token_hashed" varchar_pattern_ops);
CREATEINDEXCONCURRENTLY"sentry_orgauthtoken_project_last_used_id_0b9aa639"ON"sentry_orgauthtoken" ("project_last_used_id");
CREATEINDEXCONCURRENTLY"sentry_orgauthtoken_created_by_id_5e2288f9"ON"sentry_orgauthtoken" ("created_by_id"); |
mydea
commented
Jun 9, 2023
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
left a comment
There was a problem hiding this comment.
PR LGTM but I can't judge the usage of the HybridCloudForeignKey. The only thing I can say is that semantically it makes sense.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
iambriccardo
left a comment
There was a problem hiding this comment.
PR LGTM but I can't judge the usage of the HybridCloudForeignKey. The only thing I can say is that semantically it makes sense.
Uh oh!
There was an error while loading. Please reload this page.
This PR has a migration; here is the generated SQL for ---- Create model OrgAuthToken--CREATETABLE "sentry_orgauthtoken" ("id"bigserialNOT NULLPRIMARY KEY, "organization_id"bigintNOT NULL, "token_hashed"textNOT NULL UNIQUE, "token_last_characters"varchar(4) NULL, "name"varchar(255) NOT NULL, "scope_list"text[] NULL, "date_added"timestamp with time zoneNOT NULL, "date_last_used"timestamp with time zoneNULL, "project_last_used_id"bigintNULL, "date_deactivated"timestamp with time zoneNULL, "created_by_id"integerNULL);
ALTERTABLE"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;
ALTERTABLE"sentry_orgauthtoken" VALIDATE CONSTRAINT"sentry_orgauthtoken_created_by_id_5e2288f9_fk_auth_user_id";
CREATEINDEXCONCURRENTLY"sentry_orgauthtoken_organization_id_17552a60"ON"sentry_orgauthtoken" ("organization_id");
CREATEINDEXCONCURRENTLY"sentry_orgauthtoken_token_hashed_4c36306d_like"ON"sentry_orgauthtoken" ("token_hashed" text_pattern_ops);
CREATEINDEXCONCURRENTLY"sentry_orgauthtoken_project_last_used_id_0b9aa639"ON"sentry_orgauthtoken" ("project_last_used_id");
CREATEINDEXCONCURRENTLY"sentry_orgauthtoken_created_by_id_5e2288f9"ON"sentry_orgauthtoken" ("created_by_id"); |
This PR has a migration; here is the generated SQL for ---- Create model OrgAuthToken--CREATETABLE "sentry_orgauthtoken" ("id"bigserialNOT NULLPRIMARY KEY, "organization_id"bigintNOT NULL, "token_hashed"textNOT NULL UNIQUE, "token_last_characters"varchar(4) NULL, "name"varchar(255) NOT NULL, "scope_list"text[] NULL, "date_added"timestamp with time zoneNOT NULL, "date_last_used"timestamp with time zoneNULL, "project_last_used_id"bigintNULL, "date_deactivated"timestamp with time zoneNULL, "created_by_id"integerNULL);
ALTERTABLE"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;
ALTERTABLE"sentry_orgauthtoken" VALIDATE CONSTRAINT"sentry_orgauthtoken_created_by_id_5e2288f9_fk_auth_user_id";
CREATEINDEXCONCURRENTLY"sentry_orgauthtoken_organization_id_17552a60"ON"sentry_orgauthtoken" ("organization_id");
CREATEINDEXCONCURRENTLY"sentry_orgauthtoken_token_hashed_4c36306d_like"ON"sentry_orgauthtoken" ("token_hashed" text_pattern_ops);
CREATEINDEXCONCURRENTLY"sentry_orgauthtoken_project_last_used_id_0b9aa639"ON"sentry_orgauthtoken" ("project_last_used_id");
CREATEINDEXCONCURRENTLY"sentry_orgauthtoken_created_by_id_5e2288f9"ON"sentry_orgauthtoken" ("created_by_id"); |
This adds a new
OrgAuthTokenmodel:tokenwhich is expected to be a JWT tokenref #50144
based on RFC getsentry/rfcs#91