diff --git a/app-modules/identity/src/Auth/Exceptions/OAuthFlowException.php b/app-modules/identity/src/Auth/Exceptions/OAuthFlowException.php index 5ea3d836..5aa830ad 100644 --- a/app-modules/identity/src/Auth/Exceptions/OAuthFlowException.php +++ b/app-modules/identity/src/Auth/Exceptions/OAuthFlowException.php @@ -33,4 +33,12 @@ public static function tokenExchangeFailed(string $provider, string $error): sel { return new self(sprintf('Token exchange failed for "%s": %s', $provider, $error)); } + + public static function emailUnavailable(string $provider): self + { + return new self(sprintf( + 'Could not resolve a primary verified email from "%s"; aborting login to avoid forking a duplicate user.', + $provider, + )); + } } diff --git a/app-modules/identity/tests/Feature/Auth/HandleOAuthCallbackActionTest.php b/app-modules/identity/tests/Feature/Auth/HandleOAuthCallbackActionTest.php new file mode 100644 index 00000000..cd8c6fd6 --- /dev/null +++ b/app-modules/identity/tests/Feature/Auth/HandleOAuthCallbackActionTest.php @@ -0,0 +1,172 @@ +instance(GitHubOAuthClient::class, new readonly class($githubUser) implements OAuthClientContract + { + public function __construct(private OAuthUserDTO $githubUser) {} + + public function redirectUrl(?OAuthStateDTO $state = null): string + { + return 'https://github.test/oauth'; + } + + public function auth(string $code): OAuthAccessDTO + { + return fakeOAuthAccess(); + } + + public function getAuthenticatedUser(OAuthAccessDTO $credentials): OAuthUserDTO + { + return $this->githubUser; + } + }); +} + +test('links a second provider to the logged-in user even when the email differs', function (): void { + // A user who originally signed up via Discord (discord@discord.com)... + $tenant = Tenant::factory()->create(); + $discordUser = User::factory()->create(['email' => 'discord@discord.com']); + + ExternalIdentity::factory()->create([ + 'tenant_id' => $tenant->id, + 'model_type' => (new User)->getMorphClass(), + 'model_id' => $discordUser->id, + 'connected_by' => $discordUser->id, + 'provider' => IdentityProvider::Discord, + 'external_account_id' => 'discord-123', + ]); + + actingAs($discordUser); + + // ...now logs in with GitHub, whose account carries a *different* email. + bindFakeGithubClient(fakeGithubUser(email: 'github@github.com', providerId: 'gh-999')); + + $state = new OAuthStateDTO( + intent: OAuthIntent::Link, + provider: IdentityProvider::GitHub, + panel: 'app', + tenant: $tenant->slug, + returnUrl: 'https://he4rt.test/app', + ); + + $result = resolve(HandleOAuthCallbackAction::class) + ->execute($state, IdentityProvider::GitHub, 'auth-code'); + + // The flow links to the existing logged-in user — it does not fork a new + // one, and the differing email is a non-event (no merge conflict). + expect($result->intent)->toBe(OAuthIntent::Link) + ->and($result->user->id)->toBe($discordUser->id) + ->and($result->mergeConflict)->toBeNull(); + + // The current user keeps their original email column untouched. + expect($discordUser->fresh()->email)->toBe('discord@discord.com'); + + // No user is created from the GitHub email — nothing was forked. + expect(User::query()->where('email', 'github@github.com')->exists())->toBeFalse(); + + // The GitHub identity is attached to the Discord user, and the GitHub email + // lives only in the identity metadata. + $githubIdentity = ExternalIdentity::query() + ->where('provider', IdentityProvider::GitHub) + ->where('external_account_id', 'gh-999') + ->first(); + + expect($githubIdentity)->not->toBeNull() + ->and($githubIdentity->model_id)->toBe($discordUser->id) + ->and($githubIdentity->metadata['email'])->toBe('github@github.com'); +}); + +test('forks a separate account when a logged-out github login has a different email', function (): void { + // Same starting point — a Discord user (discord@discord.com). + $tenant = Tenant::factory()->create(); + $discordUser = User::factory()->create(['email' => 'discord@discord.com']); + + ExternalIdentity::factory()->create([ + 'tenant_id' => $tenant->id, + 'model_type' => (new User)->getMorphClass(), + 'model_id' => $discordUser->id, + 'connected_by' => $discordUser->id, + 'provider' => IdentityProvider::Discord, + 'external_account_id' => 'discord-123', + ]); + + // ...but this time they are LOGGED OUT (no actingAs) and sign in from the + // login page with GitHub, intent = Login. With no shared email and no prior + // GitHub link, there is nothing to correlate on, so a new account is forked + // (Option B: email is the only merge key on the Login path). + bindFakeGithubClient(fakeGithubUser(email: 'github@github.com', providerId: 'gh-999')); + + $state = new OAuthStateDTO( + intent: OAuthIntent::Login, + provider: IdentityProvider::GitHub, + panel: 'app', + tenant: $tenant->slug, + returnUrl: 'https://he4rt.test/app', + ); + + $result = resolve(HandleOAuthCallbackAction::class) + ->execute($state, IdentityProvider::GitHub, 'auth-code'); + + // A brand-new user is created — NOT the existing Discord user. + expect($result->intent)->toBe(OAuthIntent::Login) + ->and($result->user->id)->not->toBe($discordUser->id) + ->and($result->user->email)->toBe('github@github.com'); + + // The original Discord account is left completely untouched. + expect($discordUser->fresh()->email)->toBe('discord@discord.com'); + + // The GitHub identity is attached to the new forked user, not the Discord one. + $githubIdentity = ExternalIdentity::query() + ->where('provider', IdentityProvider::GitHub) + ->where('external_account_id', 'gh-999') + ->first(); + + expect($githubIdentity)->not->toBeNull() + ->and($githubIdentity->model_id)->toBe($result->user->id) + ->and($githubIdentity->model_id)->not->toBe($discordUser->id); + + assertDatabaseCount(User::class, 3); +}); diff --git a/app-modules/integration-github/src/OAuth/GitHubOAuthClient.php b/app-modules/integration-github/src/OAuth/GitHubOAuthClient.php index 8c8986bc..67659331 100644 --- a/app-modules/integration-github/src/OAuth/GitHubOAuthClient.php +++ b/app-modules/integration-github/src/OAuth/GitHubOAuthClient.php @@ -14,6 +14,7 @@ use He4rt\IntegrationGithub\Transport\GitHubOAuthConnector; use He4rt\IntegrationGithub\Transport\Requests\OAuth\ExchangeCodeForToken; use He4rt\IntegrationGithub\Transport\Requests\Users\GetCurrentUser; +use He4rt\IntegrationGithub\Transport\Requests\Users\GetUserEmails; final readonly class GitHubOAuthClient implements OAuthClientContract { @@ -61,9 +62,39 @@ public function getAuthenticatedUser(OAuthAccessDTO $credentials): GitHubOAuthUs /** @var array $userPayload */ $userPayload = $response->json(); + // GitHub's /user only exposes a *public* email, so it is null when the + // user keeps their address private. Recover the primary verified email + // from /user/emails (granted by the user:email scope); otherwise login + // can't match an existing account by email and forks a duplicate user. + // When even the fallback can't yield a primary+verified address there is + // nothing trustworthy to correlate on, so hard-fail instead of forking. + if (($userPayload['email'] ?? null) === null) { + $userPayload['email'] = $this->resolvePrimaryEmail($credentials->accessToken) + ?? throw OAuthFlowException::emailUnavailable('github'); + } + return GitHubOAuthUserDTO::make($credentials, $userPayload); } + private function resolvePrimaryEmail(string $accessToken): ?string + { + $response = $this->apiConnector->send(new GetUserEmails( + accessToken: $accessToken, + )); + + if (!$response->ok()) { + return null; + } + + // Only the single primary + verified address identifies the account. A + // GitHub user often has several verified emails (work, school, noreply), + // so "any verified" could match the wrong one — require both flags. + return $response->collect() + ->where('primary', operator: true) + ->where('verified', operator: true) + ->value('email'); + } + private function callbackUrl(): string { return mb_rtrim(config('app.url'), '/').'/auth/oauth/github'; diff --git a/app-modules/integration-github/src/Transport/Requests/Users/GetUserEmails.php b/app-modules/integration-github/src/Transport/Requests/Users/GetUserEmails.php new file mode 100644 index 00000000..fb245e38 --- /dev/null +++ b/app-modules/integration-github/src/Transport/Requests/Users/GetUserEmails.php @@ -0,0 +1,28 @@ +accessToken); + } +} diff --git a/app-modules/integration-github/tests/Feature/GitHubOAuthClientTest.php b/app-modules/integration-github/tests/Feature/GitHubOAuthClientTest.php new file mode 100644 index 00000000..35f2b5b3 --- /dev/null +++ b/app-modules/integration-github/tests/Feature/GitHubOAuthClientTest.php @@ -0,0 +1,100 @@ + $responses + */ +function githubOAuthClient(array $responses): GitHubOAuthClient +{ + $api = tap( + new GitHubApiConnector(), + fn (GitHubApiConnector $connector) => $connector->withMockClient(new MockClient($responses)), + ); + + return new GitHubOAuthClient( + oauthConnector: new GitHubOAuthConnector(clientId: 'cid', clientSecret: 'secret'), + apiConnector: $api, + ); +} + +/** + * @param array $overrides + */ +function githubUser(array $overrides = []): MockResponse +{ + return MockResponse::make([ + 'id' => 48_625_433, + 'login' => 'gvieira18', + 'name' => 'Gabriel Vieira', + 'email' => null, + 'avatar_url' => 'https://avatars.githubusercontent.com/u/48625433', + ...$overrides, + ]); +} + +test('uses the public /user email when GitHub exposes one', function (): void { + $user = githubOAuthClient([ + GetCurrentUser::class => githubUser(['email' => 'public@example.com']), + ])->getAuthenticatedUser(GitHubOAuthAccessDTO::make(['access_token' => 'tok'])); + + expect($user->email)->toBe('public@example.com') + ->and($user->provider)->toBe(IdentityProvider::GitHub) + ->and($user->providerId)->toBe('48625433'); +}); + +test('falls back to the primary verified email when /user email is private', function (): void { + $user = githubOAuthClient([ + GetCurrentUser::class => githubUser(['email' => null]), + GetUserEmails::class => MockResponse::make([ + ['email' => 'secondary@example.com', 'primary' => false, 'verified' => true], + ['email' => 'primary@example.com', 'primary' => true, 'verified' => true], + ['email' => 'unverified@example.com', 'primary' => false, 'verified' => false], + ]), + ])->getAuthenticatedUser(GitHubOAuthAccessDTO::make(['access_token' => 'tok'])); + + expect($user->email)->toBe('primary@example.com'); +}); + +test('hard-fails when no email is primary', function (): void { + // Several verified addresses (work, school, noreply) but none primary — + // none can be picked, so login must abort instead of forking a duplicate. + expect(fn () => githubOAuthClient([ + GetCurrentUser::class => githubUser(['email' => null]), + GetUserEmails::class => MockResponse::make([ + ['email' => 'noreply@example.com', 'primary' => false, 'verified' => true], + ['email' => 'school@example.com', 'primary' => false, 'verified' => true], + ]), + ])->getAuthenticatedUser(GitHubOAuthAccessDTO::make(['access_token' => 'tok']))) + ->toThrow(OAuthFlowException::class); +}); + +test('hard-fails when the primary email is not verified', function (): void { + expect(fn () => githubOAuthClient([ + GetCurrentUser::class => githubUser(['email' => null]), + GetUserEmails::class => MockResponse::make([ + ['email' => 'primary-unverified@example.com', 'primary' => true, 'verified' => false], + ['email' => 'verified@example.com', 'primary' => false, 'verified' => true], + ]), + ])->getAuthenticatedUser(GitHubOAuthAccessDTO::make(['access_token' => 'tok']))) + ->toThrow(OAuthFlowException::class); +}); + +test('hard-fails when /user/emails is empty', function (): void { + expect(fn () => githubOAuthClient([ + GetCurrentUser::class => githubUser(['email' => null]), + GetUserEmails::class => MockResponse::make([]), + ])->getAuthenticatedUser(GitHubOAuthAccessDTO::make(['access_token' => 'tok']))) + ->toThrow(OAuthFlowException::class); +});