-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
chore(auth): Clean up identity linking quick inc fix #103215
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
| the case where the user is linked to a different identity or the | ||
| identity is linked to a different user. | ||
| """ | ||
| # NOTE(vgrozdanic): temporary fix for #inc-1373 to stop the bleed | ||
| if is_demo_user(user) and options.get( | ||
| "identity.prevent-link-identity-for-demo-users.enabled" | ||
| ): | ||
| logger.warning( | ||
| "Preventing link identity for demo user", | ||
| extra={"user_id": user.id, "idp_id": idp.id, "external_id": external_id}, | ||
| stack_info=True, | ||
| ) | ||
| return None | ||
|
|
||
| from sentry.integrations.slack.analytics import SlackIntegrationIdentityLinked | ||
|
|
||
| defaults = { |
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.
Bug: Demo users can link identities via non-SSO paths due to insufficient is_demo_user checks.
Severity: CRITICAL | Confidence: 0.95
🔍 Detailed Analysis
The removal of the identity.prevent-link-identity-for-demo-users.enabled check allows demo users to link identities through non-SSO code paths. The new check in _handle_membership() only protects the SSO auth flow, leaving Identity Pipeline and Integration Pipeline vulnerable. These pipelines directly call Identity.objects.link_identity() or Identity.objects.update_external_id_and_defaults() without checking if the user is a demo user, potentially reintroducing the leak mentioned in issue #inc-1373.
💡 Suggested Fix
Re-implement a comprehensive is_demo_user check across all identity linking code paths, specifically in Identity Pipeline and Integration Pipeline, to prevent demo users from linking identities.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/sentry/users/models/identity.py#L89-L94
Potential issue: The removal of the
`identity.prevent-link-identity-for-demo-users.enabled` check allows demo users to link
identities through non-SSO code paths. The new check in `_handle_membership()` only
protects the SSO auth flow, leaving `Identity Pipeline` and `Integration Pipeline`
vulnerable. These pipelines directly call `Identity.objects.link_identity()` or
`Identity.objects.update_external_id_and_defaults()` without checking if the user is a
demo user, potentially reintroducing the leak mentioned in [issue
#inc-1373](https://sentry.sentry.io/issues/inc-1373).
Did we get this right? 👍 / 👎 to inform future reviews.
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.
Bug: Demo Users Bypass Identity Link Protection
Removing demo user checks from link_identity, create_identity, reattach, and update_external_id_and_defaults exposes unprotected identity linking flows. While the proper fix in auth/helper.py prevents demo users from joining organizations via SSO auth providers, these Identity manager methods are also called from integration pipelines, social auth pipelines, and messaging integration linking flows that don't go through auth/helper.py. Demo users can now link identities through these alternative paths.
src/sentry/users/models/identity.py#L81-L119
sentry/src/sentry/users/models/identity.py
Lines 81 to 119 in 52098c8
| user: User | RpcUser, | |
| idp: IdentityProvider | RpcIdentityProvider, | |
| external_id: str, | |
| should_reattach: bool = True, | |
| defaults: Mapping[str, Any | None] | None = None, | |
| ) -> Identity | None: | |
| """ | |
| Link the user with the identity. If `should_reattach` is passed, handle | |
| the case where the user is linked to a different identity or the | |
| identity is linked to a different user. | |
| """ | |
| from sentry.integrations.slack.analytics import SlackIntegrationIdentityLinked | |
| defaults = { | |
| **(defaults or {}), | |
| "status": IdentityStatus.VALID, | |
| "date_verified": timezone.now(), | |
| } | |
| try: | |
| identity, created = self.get_or_create( | |
| idp_id=idp.id, user_id=user.id, external_id=external_id, defaults=defaults | |
| ) | |
| if not created: | |
| identity.update(**defaults) | |
| except IntegrityError: | |
| if not should_reattach: | |
| raise | |
| return self.reattach(idp, external_id, user, defaults) | |
| analytics.record( | |
| SlackIntegrationIdentityLinked( | |
| provider=IntegrationProviderSlug.SLACK.value, | |
| # Note that prior to circa March 2023 this was user.actor_id. It changed | |
| # when actor ids were no longer stable between regions for the same user | |
| actor_id=user.id, | |
| actor_type="user", | |
| ) | |
| ) | |
| return identity |
Cleans up `identity.prevent-link-identity-for-demo-users.enabled` option which was introduce to stop the leak for the #inc-1373 The proper fix has been implemented in the mean time, and this quick fix is no longer required. Part of TET-1350
Cleans up `identity.prevent-link-identity-for-demo-users.enabled` option which was introduce to stop the leak for the #inc-1373 The proper fix has been implemented in the mean time, and this quick fix is no longer required. Part of TET-1350
Cleans up
identity.prevent-link-identity-for-demo-users.enabledoption which was introduce to stop the leak for the #inc-1373The proper fix has been implemented in the mean time, and this quick fix is no longer required.
Part of TET-1350