Skip to content

feat(api): In org details, show if SSO is required #73593

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 8, 2024
Merged

feat(api): In org details, show if SSO is required #73593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/sentry/api/serializers/models/organization.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,7 @@ class DetailedOrganizationSerializerResponse(_DetailedOrganizationSerializerResp
metricsActivatePercentiles: bool
metricsActivateLastForGauges: bool
extrapolateMetrics: bool
requiresSso: bool


class DetailedOrganizationSerializer(OrganizationSerializer):
Expand Down Expand Up @@ -598,6 +599,7 @@ def serialize( # type: ignore[explicit-override, override]
if access.role is not None:
context["role"] = access.role # Deprecated
context["orgRole"] = access.role
context["requiresSso"] = access.requires_sso
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there another way we can make this check without having to update the org serializer? I'm sure we must know somewhere on the frontend if the org requires SSO no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chatted with @RyanSkonnord about this. We really don't expose this anywhere to the UI for the most part. I think adding this here is fine (despite it being just another field on an organization).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some notes on my investigation, for the record:

The source of truth for access.requires_sso is query_sso_state at https://github.com/getsentry/sentry/blob/master/src/sentry/auth/services/access/service.py#L60, which inspects the associated AuthProvider object. The flag is generally available through an Access object and isn’t associated directly with the org model. Given that the org serializer is already adding few other Access attributes to the API output (right above here), there seems to be precedent. However, I did have the same initial intuition as @evanpurkhiser — it's surprising that it wasn’t already visible somehow.

context["pendingAccessRequests"] = OrganizationAccessRequest.objects.filter(
team__organization=obj
).count()
Expand Down
1 change: 1 addition & 0 deletions tests/sentry/api/serializers/test_organization.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ def test_detailed(self):
assert result["relayPiiConfig"] is None
assert isinstance(result["orgRoleList"], list)
assert isinstance(result["teamRoleList"], list)
assert result["requiresSso"] == acc.requires_sso


class DetailedOrganizationSerializerWithProjectsAndTeamsTest(TestCase):
Expand Down
Loading