-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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.
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
Conversation
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.
I am not sure if we need to update our API docs for this change.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #73593 +/- ##
===========================================
+ Coverage 56.87% 78.02% +21.15%
===========================================
Files 6628 6638 +10
Lines 296613 296915 +302
Branches 51085 51134 +49
===========================================
+ Hits 168693 231676 +62983
+ Misses 123205 58963 -64242
- Partials 4715 6276 +1561
|
@@ -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 |
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.
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?
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.
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).
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.
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.
According to @isabellaenriquez an update would be overwritten by #73293 anyway, so we can add it afterward. |
Supports #73521