-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(auth): Return auth error if application is requesting a wrong org #81193
base: athena/add-scoping-org-to-all
Are you sure you want to change the base?
feat(auth): Return auth error if application is requesting a wrong org #81193
Conversation
a09ec51
to
9c162e6
Compare
@@ -422,7 +423,7 @@ def authenticate_token(self, request: Request, token_str: str) -> tuple[Any, Any | |||
if application_is_inactive: | |||
raise AuthenticationFailed("UserApplication inactive or deleted") | |||
|
|||
if token.organization_id: | |||
if token.scoping_organization_id: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SystemToken
and ApiTokenReplica
types don't have scoping_organization_id
attributes. I think you'd need to make sure that attribute exists first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markstory I added it to ApiTokenReplica below in src/sentry/auth/services/auth/serial.py
Why does SystemToken need it? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh got it. I need to add it to the model. But maybe skip the check for systemtoken? I don't see a case where this will used in system token at all so we want this line to skip.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added both in #81213
❌ 2 Tests Failed:
View the top 2 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
If an application is organization scope application, their tokens will only have access to one organization of a user. So we should return auth error if:
In a previous PR I added some logging to make sure this doesn't break other integrations. It actually does, so I have to limit this to token.scoping_organization_id vs token.organization_id