Skip to content

ref(auth): Extract reusable methods from SSO identity handler - #121013

Open
nora-shap wants to merge 2 commits into
masterfrom
nora/auth/ID-1747
Open

ref(auth): Extract reusable methods from SSO identity handler#121013
nora-shap wants to merge 2 commits into
masterfrom
nora/auth/ID-1747

Conversation

@nora-shap

Copy link
Copy Markdown
Member

Summary

Preparatory refactors for the upcoming SSO email verification at signup feature. No behavior changes.

  • Add is_email_verified_by_trusted_provider() utility to email_verification.py — centralizes the trust check for Google/GitHub email_verified claims so both sentry and getsentry can share it
  • Extract complete_and_login() from handle_unknown_identity() — the login + state cleanup tail is now callable independently (needed by the future verification view)
  • Add skip_confirm_emails param to handle_new_user() — allows skipping the advisory confirmation email when the address has already been verified
  • Update _post_login_redirect() to accept an explicit is_new_user flag, falling back to the existing request.POST check when None

@linear-code

linear-code Bot commented Jul 31, 2026

Copy link
Copy Markdown

ID-1747

@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Jul 31, 2026
@nora-shap

Copy link
Copy Markdown
Member Author

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 935fd03. Configure here.

@nora-shap
nora-shap marked this pull request as ready for review July 31, 2026 21:37
@nora-shap
nora-shap requested review from a team as code owners July 31, 2026 21:37

@control_silo_test
class HandleNewUserTest(AuthIdentityHandlerTest, HybridCloudTestMixin):
@mock.patch("sentry.analytics.record")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks unused?

def test_skip_confirm_emails_suppresses_email(self, mock_record: mock.MagicMock) -> None:
with mock.patch.object(User, "send_confirm_emails") as mock_send:
self.handler.handle_new_user(skip_confirm_emails=True)
mock_send.assert_not_called()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we add the inverse too? Nothing currently asserts the email is sent when skip_confirm_emails=False, so a future default flip would go unnoticed.

If False: the identity requires our own verification step.
"""
return (
provider_key in TRUSTED_EMAIL_VERIFIED_PROVIDERS and identity.get("email_verified") is True

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This might not be relevant but are you sure email_verified output a Boolean?

# "email_verified":"true",
Seems to imply that it would be a "true" / "false" string?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants