Skip to content

Commit

Permalink
fix(auth): better audit log events for fly sso + test disable (#77985)
Browse files Browse the repository at this point in the history
In #75167, we made it possible
to disable Fly SSO by introducing a new provider class that was
identical to the original, but designated as "non-partner." As such, it
needed a unique key, "fly-non-partner," but we shouldn't use this name
in the audit log.

This PR changes the `get_audit_log_data` method to always use "fly" as
the provider name if the AuthProvider provider is either of the Fly SSO
classes. This PR also clarifies the difference between the two classes,
and adds a missing test for disabling Fly SSO.

for context, this is what the disable audit log entry looks like before
this change:
![sso disable
ss](https://github.com/user-attachments/assets/765c5d11-f3ac-4202-a8a8-46988bef259a)
  • Loading branch information
isabellaenriquez authored Sep 24, 2024
1 parent c07f913 commit 7dd9748
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 2 deletions.
5 changes: 5 additions & 0 deletions src/sentry/auth/providers/fly/provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,5 +94,10 @@ def build_identity(self, state):


class NonPartnerFlyOAuth2Provider(FlyOAuth2Provider):
"""
When a customer is no longer on a Fly.io sponsored plan, we change their provider
to the "non-partner" version of Fly SSO so that it can be disabled.
"""

name = SPONSOR_OAUTH_NAME[ChannelName.FLY_NON_PARTNER]
is_partner = False
8 changes: 7 additions & 1 deletion src/sentry/auth/services/auth/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,13 @@ def __hash__(self) -> int:
return hash((self.id, self.organization_id, self.provider))

def get_audit_log_data(self) -> dict[str, Any]:
return {"provider": self.provider, "config": self.config}
provider = self.provider
# NOTE(isabella): for both standard fly SSO and fly-non-partner SSO, we should record the
# provider as "fly" in the audit log entry data; the only difference between the two is
# that the latter can be disabled by customers
if "fly" in self.provider:
provider = "fly"
return {"provider": provider, "config": self.config}

def get_provider(self) -> "Provider":
from sentry.auth import manager
Expand Down
8 changes: 7 additions & 1 deletion src/sentry/models/authprovider.py
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,13 @@ def disable_scim(self):
self.flags.scim_enabled = False

def get_audit_log_data(self):
return {"provider": self.provider, "config": self.config}
provider = self.provider
# NOTE(isabella): for both standard fly SSO and fly-non-partner SSO, we should record the
# provider as "fly" in the audit log entry data; the only difference between the two is
# that the latter can be disabled by customers
if "fly" in self.provider:
provider = "fly"
return {"provider": provider, "config": self.config}

def outboxes_for_mark_invalid_sso(self, user_id: int) -> list[ControlOutbox]:
return [
Expand Down
6 changes: 6 additions & 0 deletions tests/sentry/auth/providers/fly/test_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ def test_build_config(self):
resource = {"id": "nathans-org", "role": "member"}
result = provider.build_config(resource=resource)
assert result == {"org": {"id": "nathans-org"}}
assert provider.is_partner == (self.auth_provider.provider == ChannelName.FLY_IO.value)

def test_build_identity(self):
provider = self.auth_provider.get_provider()
Expand Down Expand Up @@ -71,6 +72,11 @@ def test_build_identity(self):
"email_verified": False,
}

def test_audit_log_data(self):
audit_log_data = self.auth_provider.get_audit_log_data()
assert audit_log_data["provider"] == "fly"
assert audit_log_data["config"] == {}


@control_silo_test
class NonPartnerFlyOAuth2ProviderTest(FlyOAuth2ProviderTest):
Expand Down
17 changes: 17 additions & 0 deletions tests/sentry/web/frontend/test_organization_auth_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -331,12 +331,29 @@ def test_disable_partner_provider(self):
organization, auth_provider = self.create_org_and_auth_provider("Fly.io")
self.create_om_and_link_sso(organization)
path = reverse("sentry-organization-auth-provider-settings", args=[organization.slug])
assert AuthProvider.objects.filter(organization_id=organization.id).exists()
assert AuthProvider.objects.filter(id=auth_provider.id).exists()

self.login_as(self.user, organization_id=organization.id)

resp = self.client.post(path, {"op": "disable"})
assert resp.status_code == 405

# can disable after partner plan end (changes to "non-partner" fly sso)
auth_provider.update(provider="fly-non-partner")
assert AuthProvider.objects.filter(id=auth_provider.id, provider="fly-non-partner").exists()

resp = self.client.post(path, {"op": "disable"})
assert resp.status_code == 302

assert not AuthProvider.objects.filter(organization_id=organization.id).exists()
assert not AuthProvider.objects.filter(id=auth_provider.id).exists()
disable_audit_log = AuditLogEntry.objects.filter(
event=audit_log.get_event_id("SSO_DISABLE")
).first()
assert disable_audit_log
assert disable_audit_log.data["provider"] == "fly"

def test_disable__scim_missing(self):
organization, auth_provider = self.create_org_and_auth_provider()
auth_provider.flags.scim_enabled = True
Expand Down

0 comments on commit 7dd9748

Please sign in to comment.