Automated Test: oauth-state-secure

[admin-coderabbit]

This pull request was automatically created by @coderabbitai/e2e-reviewer.

Batch created pull request.

Summary by CodeRabbit

• New Features

  • OAuth-based login flow for GitHub integration setup
  • Support for customer domains in GitHub configuration
  • Enhanced validation of GitHub installations

• Bug Fixes

  • Improved error messages for failed GitHub installations
  • Added verification to ensure GitHub user matches installation sender

[coderabbit-eval]

📝 Walkthrough

Walkthrough

The changes introduce an OAuth login flow for GitHub integration with OAuthLoginView handling authentication, add centralized error rendering helpers, extend GitHubInstallation to manage state across pipeline steps, validate user-installation matching, and update get_pipeline_views to include the new OAuth view. Supporting tests validate the OAuth flow, edge cases, and repository retrieval functionality.

Changes

Cohort / File(s)	Summary
GitHub Integration OAuth and Error Handling src/sentry/integrations/github/integration.py	Introduces OAuthLoginView for GitHub authentication, adds error(request, org, error_short, error_long) and get_document_origin(org) helpers for consistent error rendering, extends GitHubInstallation to utilize and persist installation_id from state, validates authenticated user matches installation sender, defines ERR_INTEGRATION_INVALID_INSTALLATION_REQUEST constant, and updates get_pipeline_views to include OAuthLoginView.
Pipeline Advancer Simplification src/sentry/web/frontend/pipeline_advancer.py	Removes FORWARD_INSTALL_FOR constant and replaces membership check with direct string comparison for GitHub provider detection in installation forwarding logic.
GitHub Integration Test Coverage tests/sentry/integrations/github/test_integration.py	Adds OAuth token and user fetch stubs, expands setup flow assertions for OAuth redirect validation, introduces tests for user mismatch, installation not found, installations on another org, pending deletion blocking, repository pagination, and validates error messaging across edge cases.

Sequence Diagram

sequenceDiagram
    participant Client
    participant Sentry as Sentry<br/>(OAuthLoginView)
    participant GitHub as GitHub<br/>(OAuth)
    participant Sentry2 as Sentry<br/>(Installation Flow)
    
    Client->>Sentry: Initiate GitHub setup
    Sentry->>Sentry: Get client credentials<br/>from GitHubIdentityProvider
    Sentry->>Client: Redirect to GitHub auth<br/>with state parameter
    Client->>GitHub: User authorizes
    GitHub->>Client: Redirect with code & state
    Client->>Sentry: Access callback with code & state
    Sentry->>GitHub: Exchange code for<br/>access token
    GitHub->>Sentry: Return access token
    Sentry->>GitHub: Fetch authenticated<br/>user login
    GitHub->>Sentry: Return user login
    Sentry->>Sentry: Store in pipeline state
    Sentry->>Sentry2: Pass to installation flow
    Sentry2->>Sentry2: Validate authenticated user<br/>matches installation sender
    Sentry2->>Sentry2: Store installation_id<br/>in state
    Sentry2->>Client: Complete setup flow

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Poem

🐰 OAuth flows through GitHub's gate,
Credentials checked, state kept straight,
Installation validated, users aligned,
Pipeline secured, errors refined,
A rabbit's hop to integration's height! 🚀

🚥 Pre-merge checks | ✅ 1 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'Automated Test: oauth-state-secure' is vague and does not clearly describe the actual substantive changes made in the pull request, which include OAuth login flow implementation, error handling consolidation, installation validation, and test expansions for GitHub integration.	Update the title to clearly describe the main changes, such as 'Implement GitHub OAuth login flow with installation validation' or 'Add OAuth-based GitHub integration setup with user validation and error handling'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch oauth-state-secure

---

_{Comment @coderabbit eval help to get the list of available commands and usage tips.}

[coderabbit-eval]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/sentry/integrations/github/integration.py`:
- Around line 493-505: The code currently assumes
integration.metadata["sender"]["login"] exists and will KeyError for legacy
integrations; update the check in the handler that compares
pipeline.fetch_state("github_authenticated_user") to
integration.metadata["sender"]["login"] to first safely access metadata (e.g.,
validate that integration.metadata is a dict and contains "sender" with a
"login" key or use nested .get calls) and if the sender/login is missing call
error(request, self.active_organization) so the code fails cleanly; reference
the Integration instance (variable integration), pipeline.fetch_state(...) and
the error(...) return path when implementing the guard.

[coderabbit-eval]

⚠️ Potential issue | 🟠 Major

Guard missing sender metadata to avoid runtime KeyError.
If legacy integrations lack metadata["sender"], the direct lookup will raise and surface a 500 instead of a clean failure.

🛠️ Suggested fix

-        # Check that the authenticated GitHub user is the same as who installed the app.
-        if (
-            pipeline.fetch_state("github_authenticated_user")
-            != integration.metadata["sender"]["login"]
-        ):
-            return error(request, self.active_organization)
+        sender_login = (integration.metadata.get("sender") or {}).get("login")
+        if not sender_login:
+            return error(request, self.active_organization)
+
+        # Check that the authenticated GitHub user is the same as who installed the app.
+        if pipeline.fetch_state("github_authenticated_user") != sender_login:
+            return error(request, self.active_organization)

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	try:
	integration = Integration.objects.get(
	external_id=installation_id, status=ObjectStatus.ACTIVE
	)
	except Integration.DoesNotExist:
	return error(request, self.active_organization)
	# Check that the authenticated GitHub user is the same as who installed the app.
	if (
	pipeline.fetch_state("github_authenticated_user")
	!= integration.metadata["sender"]["login"]
	):
	return error(request, self.active_organization)
	try:
	integration = Integration.objects.get(
	external_id=installation_id, status=ObjectStatus.ACTIVE
	)
	except Integration.DoesNotExist:
	return error(request, self.active_organization)
	sender_login = (integration.metadata.get("sender") or {}).get("login")
	if not sender_login:
	return error(request, self.active_organization)
	# Check that the authenticated GitHub user is the same as who installed the app.
	if pipeline.fetch_state("github_authenticated_user") != sender_login:
	return error(request, self.active_organization)

🤖 Prompt for AI Agents

In `@src/sentry/integrations/github/integration.py` around lines 493 - 505, The
code currently assumes integration.metadata["sender"]["login"] exists and will
KeyError for legacy integrations; update the check in the handler that compares
pipeline.fetch_state("github_authenticated_user") to
integration.metadata["sender"]["login"] to first safely access metadata (e.g.,
validate that integration.metadata is a dict and contains "sender" with a
"login" key or use nested .get calls) and if the sender/login is missing call
error(request, self.active_organization) so the code fails cleanly; reference
the Integration instance (variable integration), pipeline.fetch_state(...) and
the error(...) return path when implementing the guard.