fix(integration-github): fetch primary email from /user/emails#367
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app-modules/integration-github/src/OAuth/GitHubOAuthClient.php`:
- Around line 82-84: The GitHubOAuthClient currently returns null when the
/user/emails API response fails (the if (!$response->ok()) check), allowing the
login flow to continue with incomplete identity data and risking duplicate
account creation. Replace this silent failure with explicit fail-closed behavior
by throwing an exception instead of returning null, ensuring the application
explicitly fails when email recovery fails rather than silently proceeding with
unresolved identity information.
In `@app-modules/integration-github/tests/Feature/GitHubOAuthClientTest.php`:
- Around line 56-93: The test file is missing coverage for the scenario when the
/user/emails endpoint returns a non-OK response. Add a new test case that mocks
a failed response from the GetUserEmails class (using MockResponse or similar to
simulate an error) in the githubOAuthClient helper, then verify the expected
behavior of the getAuthenticatedUser method in the GitHubOAuthAccessDTO context.
This ensures the resolvePrimaryEmail method properly handles and recovers from
failed email endpoint requests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 31d4b997-b735-48ac-b6fa-c1e591aa4204
📒 Files selected for processing (3)
app-modules/integration-github/src/OAuth/GitHubOAuthClient.phpapp-modules/integration-github/src/Transport/Requests/Users/GetUserEmails.phpapp-modules/integration-github/tests/Feature/GitHubOAuthClientTest.php
|
Kept as a draft intentionally — the minimal fix here closes the realistic private-email case, but there are unresolved edge cases (no verified email / failed cc @danielhe4rt — please validate the approach and weigh in on the open edge cases in this comment before we mark ready. |
GitHub's /user endpoint only exposes a public email, so accounts with a private email return null. OAuth login then can't match an existing user by email and forks a duplicate. Fall back to /user/emails (covered by the user:email scope) and use the single primary verified address. Closes #366
When /user/emails yields no primary+verified address there is no trustworthy email to correlate on, so abort the login instead of silently forking a duplicate user. Refs #366
Document the callback orchestrator: linking a second provider while logged in attaches to the same user (email mismatch is a non-event), while a logged-out login with a different email forks a new account.
897e838 to
fdec0c4
Compare
Summary
/userendpoint only returns a public email, so accounts with a private email yieldnull. On "Sign in with GitHub" (OAuthIntent::Login),FindOrCreateUserByProvidercan't match an existing user by email and forks a duplicate user (observed: a Discord-linked user got a second…-2account on GitHub login).GetUserEmailsrequest (GET /user/emails, covered by the existinguser:emailscope) and falls back to it when/userreturns null, selecting the single primary + verified address — requiring both flags so secondary verified emails (work/school/noreply) can't match the wrong account.Closes #366
Test plan
pest --filter=GitHubOAuthClient— 4 cases: public email used as-is; private → primary-verified fallback; verified-but-not-primary → null; primary-but-not-verified → null.