Skip to content

Commit

Permalink
fix(auth): handle uniqueness of ApiAuthorization per org (getsentry#8…
Browse files Browse the repository at this point in the history
…1160)

I added a new unique index in
getsentry#80996 and this PR is using it
in code.

Basically:
1. If organization id is null then ApiAuthorization of an app must be
unique per user
2. If organization id is not null then ApiAuthorization of an app must
be unique per user and org (per organization member)

Why is this a bug today?
1. App with org level scope goes through authorize once. Users chooses
one of their orgs. We create ApiAuthorize(user1, org1, app1)
2. Same app goes through authorize again with the same user. We find
ApiAuthorize(user1, org1, app1) so we go through autoapprove. But in
autoapprove we don't know what org user chose (because they weren't
involve at all) and create ApiAuthorize(user1, app1, org=None).

This is against the way we defined our unique index so it breaks the
code.
  • Loading branch information
sentaur-athena authored Nov 22, 2024
1 parent 327f975 commit c70f656
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 1 deletion.
4 changes: 3 additions & 1 deletion src/sentry/web/frontend/oauth_authorize.py
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,9 @@ def get(self, request: HttpRequest, **kwargs) -> HttpResponseBase:
if not request.user.is_authenticated:
return super().get(request, application=application)

if not force_prompt:
# If the application expects org level access, we need to prompt the user to choose which
# organization they want to give access to every time. We should not presume the user intention
if not (force_prompt or application.requires_org_level_access):
try:
existing_auth = ApiAuthorization.objects.get(
user_id=request.user.id, application=application
Expand Down
65 changes: 65 additions & 0 deletions tests/sentry/web/frontend/test_oauth_authorize.py
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,7 @@ def setUp(self):
super().setUp()
self.owner = self.create_user(email="[email protected]")
self.create_member(user=self.owner, organization=self.organization, role="owner")
self.another_organization = self.create_organization(owner=self.owner)
self.application = ApiApplication.objects.create(
owner=self.user,
redirect_uris="https://example.com",
Expand Down Expand Up @@ -419,3 +420,67 @@ def test_exceed_scope(self):

assert resp.status_code == 302
assert resp["Location"] == "https://example.com?error=invalid_scope&state=foo"

def test_second_time(self):
self.login_as(self.owner)

# before hitting the authorize endpoint we expect that ApiAuthorization does not exist
before_apiauth = ApiAuthorization.objects.filter(
user=self.owner, application=self.application
)
assert before_apiauth.exists() is False

# The first time the app hits the endpoint for the user, it is expected that
# 1. User sees the view to choose an organization
# 2. ApiAuthorization is created with the selected organization
resp = self.client.get(
f"{self.path}?response_type=code&client_id={self.application.client_id}&scope=org:read&state=foo"
)

assert resp.status_code == 200
self.assertTemplateUsed("sentry/oauth-authorize.html")
assert resp.context["application"] == self.application

resp = self.client.post(
self.path, {"op": "approve", "selected_organization_id": self.organization.id}
)

grant = ApiGrant.objects.get(user=self.owner)
assert grant.redirect_uri == self.application.get_default_redirect_uri()
# There is only one ApiAuthorization for this user and app which is related to the right organization
api_auth = ApiAuthorization.objects.get(user=self.owner, application=self.application)
assert api_auth.organization_id == self.organization.id

# The second time the app hits the endpoint for the user, it is expected that
# 1. User still sees the view to choose an organization
# 2. ApiAuthorization is not created again if the user chooses the same organization
resp = self.client.get(
f"{self.path}?response_type=code&client_id={self.application.client_id}&scope=org:read&state=foo"
)
assert resp.status_code == 200
self.assertTemplateUsed("sentry/oauth-authorize.html")
assert resp.context["application"] == self.application
resp = self.client.post(
self.path, {"op": "approve", "selected_organization_id": self.organization.id}
)
same_api_auth = ApiAuthorization.objects.get(user=self.owner, application=self.application)
assert api_auth.id == same_api_auth.id

# The other time the app hits the endpoint for the user, it is expected that
# 1. User still sees the view to choose an organization
# 2. New ApiAuthorization is created again if the user chooses another organization
resp = self.client.get(
f"{self.path}?response_type=code&client_id={self.application.client_id}&scope=org:read&state=foo"
)
assert resp.status_code == 200
self.assertTemplateUsed("sentry/oauth-authorize.html")
assert resp.context["application"] == self.application
resp = self.client.post(
self.path, {"op": "approve", "selected_organization_id": self.another_organization.id}
)
another_api_auth = ApiAuthorization.objects.get(
user=self.owner,
application=self.application,
organization_id=self.another_organization.id,
)
assert api_auth.id != another_api_auth.id

0 comments on commit c70f656

Please sign in to comment.