From ccdb879984cc11668130f254a657aa2cca6aca18 Mon Sep 17 00:00:00 2001 From: Edgar Zanella Alvarenga Date: Mon, 1 Jun 2026 15:20:51 +0200 Subject: [PATCH 1/7] fix: enforce email_verified claim on SSO login (#497) --- app/Http/Controllers/Auth/SsoController.php | 54 +++++++++-- tests/Feature/SsoControllerTest.php | 101 +++++++++++++++++++- 2 files changed, 147 insertions(+), 8 deletions(-) diff --git a/app/Http/Controllers/Auth/SsoController.php b/app/Http/Controllers/Auth/SsoController.php index 4c6556c8..9f3ad4f2 100644 --- a/app/Http/Controllers/Auth/SsoController.php +++ b/app/Http/Controllers/Auth/SsoController.php @@ -11,6 +11,7 @@ use Illuminate\Http\RedirectResponse; use Illuminate\Http\Request; use Illuminate\Support\Facades\Http; +use Illuminate\Support\Facades\Log; use Illuminate\Support\Str; use Laravel\Socialite\Facades\Socialite; @@ -89,6 +90,19 @@ public function callback(Request $request): RedirectResponse /** @var \SocialiteProviders\Manager\OAuth2\User $socialiteUser */ $socialiteUser = $driver->stateless()->user(); + $idToken = $socialiteUser->accessTokenResponseBody['id_token'] ?? null; + $idTokenData = $this->decodeIdTokenPayload($idToken); + + if (! $this->isEmailVerified($idTokenData)) { + Log::warning('SSO: rejecting login because email_verified claim is not true', [ + 'tenant' => $instanceCode, + 'sub' => $socialiteUser->getId(), + 'reason' => $idTokenData === null ? 'id_token_missing_or_unparseable' : 'email_verified_not_true', + ]); + + return $this->frontendError('email_not_verified'); + } + $user = $this->ssoUserService->resolveUser($socialiteUser->getEmail(), $socialiteUser->getId()); if ($user === null) { @@ -256,21 +270,49 @@ protected function fetchIdpIdToken(?string $accessToken, ?string $provider): ?st } /** - * Build an IdP logout URL by discovering the OIDC end_session_endpoint. - * Works for any OIDC-compliant provider (Keycloak realms, iServ, VIDIS, etc.) + * Decode an OIDC id_token (JWT) payload without verifying the signature. + * Returns null when the token is missing, malformed, or the payload is not valid JSON. */ - protected function buildIdpLogoutUrl(?string $idpIdToken, string $redirectUri): ?string + protected function decodeIdTokenPayload(?string $idToken): ?array { - if (! $idpIdToken) { + if (! $idToken) { return null; } - $parts = explode('.', $idpIdToken); + $parts = explode('.', $idToken); if (count($parts) !== 3) { return null; } - $payload = json_decode(base64_decode(str_pad($parts[1], strlen($parts[1]) + (4 - strlen($parts[1]) % 4) % 4, '=')), true); + $padded = str_pad($parts[1], strlen($parts[1]) + (4 - strlen($parts[1]) % 4) % 4, '='); + $decoded = base64_decode(strtr($padded, '-_', '+/'), true); + if ($decoded === false) { + return null; + } + + $payload = json_decode($decoded, true); + if (! is_array($payload)) { + return null; + } + + return $payload; + } + + /** + * OIDC: the email claim is only trustworthy when email_verified is strictly true. + */ + protected function isEmailVerified(?array $idTokenPayload): bool + { + return is_array($idTokenPayload) && ($idTokenPayload['email_verified'] ?? null) === true; + } + + /** + * Build an IdP logout URL by discovering the OIDC end_session_endpoint. + * Works for any OIDC-compliant provider (Keycloak realms, iServ, VIDIS, etc.) + */ + protected function buildIdpLogoutUrl(?string $idpIdToken, string $redirectUri): ?string + { + $payload = $this->decodeIdTokenPayload($idpIdToken); $issuer = rtrim($payload['iss'] ?? '', '/'); if (! $issuer) { diff --git a/tests/Feature/SsoControllerTest.php b/tests/Feature/SsoControllerTest.php index 88086e34..454ac9fb 100644 --- a/tests/Feature/SsoControllerTest.php +++ b/tests/Feature/SsoControllerTest.php @@ -186,6 +186,87 @@ public function test_callback_inactive_user_redirects_to_account_inactive_error( $this->assertStringContainsString('sso_error=account_inactive', $response->headers->get('Location')); } + // ========================================================= + // callback — email_verified claim enforcement + // ========================================================= + + public function test_callback_rejects_when_email_verified_is_false(): void + { + $idToken = $this->makeIdToken([ + 'sub' => 'sub-unverified-001', + 'email' => 'sso_unverified@test.example', + 'email_verified' => false, + ]); + + $this->mockSocialiteCallback('sub-unverified-001', 'sso_unverified@test.example', 'Unverified', 'unverified', $idToken); + + $state = $this->buildState(self::INSTANCE_CODE); + $response = $this->get("/api/v2/auth/sso/callback?state={$state}"); + + $response->assertRedirect(); + $this->assertStringContainsString('sso_error=email_not_verified', $response->headers->get('Location')); + + self::$testTenant->run(function () { + $this->assertEquals(0, LegacyUser::where('email', 'sso_unverified@test.example')->count()); + $this->assertEquals(0, LegacyUser::where('sso_sub', 'sub-unverified-001')->count()); + }); + } + + public function test_callback_rejects_when_email_verified_claim_is_missing(): void + { + $idToken = $this->makeIdToken([ + 'sub' => 'sub-missing-claim-001', + 'email' => 'sso_missingclaim@test.example', + ]); + + $this->mockSocialiteCallback('sub-missing-claim-001', 'sso_missingclaim@test.example', 'Missing Claim', 'missingclaim', $idToken); + + $state = $this->buildState(self::INSTANCE_CODE); + $response = $this->get("/api/v2/auth/sso/callback?state={$state}"); + + $response->assertRedirect(); + $this->assertStringContainsString('sso_error=email_not_verified', $response->headers->get('Location')); + + self::$testTenant->run(function () { + $this->assertEquals(0, LegacyUser::where('sso_sub', 'sub-missing-claim-001')->count()); + }); + } + + public function test_callback_rejects_when_id_token_is_missing(): void + { + $socialiteUser = \Mockery::mock(\Laravel\Socialite\Two\User::class); + $socialiteUser->token = 'access-token-mock'; + $socialiteUser->refreshToken = 'refresh-token-mock'; + $socialiteUser->accessTokenResponseBody = []; + $socialiteUser->shouldReceive('getId')->andReturn('sub-no-idtoken'); + $socialiteUser->shouldReceive('getEmail')->andReturn('sso_noidtoken@test.example'); + $socialiteUser->shouldReceive('getName')->andReturn('No IdToken'); + $socialiteUser->shouldReceive('getNickname')->andReturn('noidtoken'); + + $provider = \Mockery::mock(); + $provider->shouldReceive('stateless')->andReturnSelf(); + $provider->shouldReceive('user')->andReturn($socialiteUser); + + Socialite::shouldReceive('driver')->with('keycloak')->andReturn($provider); + + $state = $this->buildState(self::INSTANCE_CODE); + $response = $this->get("/api/v2/auth/sso/callback?state={$state}"); + + $response->assertRedirect(); + $this->assertStringContainsString('sso_error=email_not_verified', $response->headers->get('Location')); + } + + public function test_callback_rejects_when_id_token_is_malformed(): void + { + $this->mockSocialiteCallback('sub-malformed-001', 'sso_malformed@test.example', 'Malformed', 'malformed', 'not.a.valid.jwt'); + + $state = $this->buildState(self::INSTANCE_CODE); + $response = $this->get("/api/v2/auth/sso/callback?state={$state}"); + + $response->assertRedirect(); + $this->assertStringContainsString('sso_error=email_not_verified', $response->headers->get('Location')); + } + // ========================================================= // resolveUser — collision handling // ========================================================= @@ -297,12 +378,14 @@ private function buildState(string $instanceCode): string return $payload . '.' . $signature; } - private function mockSocialiteCallback(string $sub, string $email, string $name, string $nickname): void + private function mockSocialiteCallback(string $sub, string $email, string $name, string $nickname, ?string $idToken = null): void { $socialiteUser = \Mockery::mock(\Laravel\Socialite\Two\User::class); $socialiteUser->token = 'access-token-mock'; $socialiteUser->refreshToken = 'refresh-token-mock'; - $socialiteUser->accessTokenResponseBody = ['id_token' => 'aula-id-token-mock']; + $socialiteUser->accessTokenResponseBody = [ + 'id_token' => $idToken ?? $this->makeIdToken(['sub' => $sub, 'email' => $email, 'email_verified' => true]), + ]; $socialiteUser->shouldReceive('getId')->andReturn($sub); $socialiteUser->shouldReceive('getEmail')->andReturn($email); $socialiteUser->shouldReceive('getName')->andReturn($name); @@ -315,6 +398,20 @@ private function mockSocialiteCallback(string $sub, string $email, string $name, Socialite::shouldReceive('driver')->with('keycloak')->andReturn($provider); } + private function makeIdToken(array $claims): string + { + $header = $this->base64UrlEncode(json_encode(['alg' => 'RS256', 'typ' => 'JWT'])); + $payload = $this->base64UrlEncode(json_encode($claims)); + $sig = $this->base64UrlEncode('signature-not-verified'); + + return "{$header}.{$payload}.{$sig}"; + } + + private function base64UrlEncode(string $data): string + { + return rtrim(strtr(base64_encode($data), '+/', '-_'), '='); + } + private function jwtForUser(LegacyUser $user): string { return self::$testTenant->run( From 0686853d5c525095d59ebdb8cc8af91707b60a6e Mon Sep 17 00:00:00 2001 From: Edgar Zanella Alvarenga Date: Tue, 2 Jun 2026 08:57:32 +0200 Subject: [PATCH 2/7] feat: password-proof account linking for SSO/legacy users (#495) --- app/Http/Controllers/Auth/SsoController.php | 135 +++++++++++- app/Services/SsoUserService.php | 37 +--- routes/tenant.php | 1 + tests/Feature/SsoControllerTest.php | 224 +++++++++++++++++++- tests/Unit/SsoUserServiceTest.php | 45 ++-- 5 files changed, 374 insertions(+), 68 deletions(-) diff --git a/app/Http/Controllers/Auth/SsoController.php b/app/Http/Controllers/Auth/SsoController.php index 9f3ad4f2..be330f5e 100644 --- a/app/Http/Controllers/Auth/SsoController.php +++ b/app/Http/Controllers/Auth/SsoController.php @@ -10,6 +10,8 @@ use Illuminate\Http\JsonResponse; use Illuminate\Http\RedirectResponse; use Illuminate\Http\Request; +use Illuminate\Support\Facades\Cache; +use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Http; use Illuminate\Support\Facades\Log; use Illuminate\Support\Str; @@ -17,6 +19,8 @@ class SsoController extends Controller { + private const LINK_INTENT_TTL_MINUTES = 10; + public function __construct( protected LegacyJwtService $jwtService, protected SsoUserService $ssoUserService, @@ -103,19 +107,55 @@ public function callback(Request $request): RedirectResponse return $this->frontendError('email_not_verified'); } - $user = $this->ssoUserService->resolveUser($socialiteUser->getEmail(), $socialiteUser->getId()); + /** @var Tenant $callbackTenant */ + $callbackTenant = tenant(); + $sub = $socialiteUser->getId(); + $email = $socialiteUser->getEmail(); + + $user = $this->ssoUserService->findBySub($sub); if ($user === null) { - $user = $this->ssoUserService->provisionUser($socialiteUser); + $emailMatch = $this->ssoUserService->findByEmail($email); + + if ($emailMatch === null) { + $user = $this->ssoUserService->provisionUser($socialiteUser); + } else { + if (! $emailMatch->isActive()) { + return $this->frontendError('account_inactive'); + } + + if ($emailMatch->sso_sub !== null) { + Log::warning('SSO: email matches a user already bound to a different sso_sub', [ + 'tenant' => $instanceCode, + 'incoming_sub' => $sub, + 'existing_sub' => $emailMatch->sso_sub, + 'matched_user_id' => $emailMatch->id, + ]); + + return $this->frontendError('sub_collision'); + } + + $linkToken = $this->storeLinkIntent($emailMatch, $socialiteUser, $callbackTenant); + + return $this->frontendError('account_link_required', ['sso_link' => $linkToken]); + } + } else { + $strayEmailMatch = $this->ssoUserService->findByEmail($email); + if ($strayEmailMatch && $strayEmailMatch->id !== $user->id) { + Log::warning('SSO: email and sso_sub match different users — prioritising sso_sub match.', [ + 'email' => $email, + 'sub' => $sub, + 'sso_sub_user' => $user->id, + 'email_user' => $strayEmailMatch->id, + ]); + } } if (! $user->isActive()) { return $this->frontendError('account_inactive'); } - /** @var Tenant $callbackTenant */ - $callbackTenant = tenant(); - $user->sso_id_token = $socialiteUser->accessTokenResponseBody['id_token'] ?? null; + $user->sso_id_token = $idToken; $user->sso_refresh_token = $socialiteUser->refreshToken ?? null; $user->sso_idp_id_token = $this->fetchIdpIdToken($socialiteUser->token, $callbackTenant->sso_provider); $user->save(); @@ -125,6 +165,58 @@ public function callback(Request $request): RedirectResponse return $this->frontendRedirect($token); } + /** + * Link an SSO identity to an authenticated legacy user. + * + * Auth: bearer JWT (legacy.jwt middleware). The bearer user proves possession + * of the legacy account; the link-intent token proves possession of the IdP + * identity. Both must point to the same user_id. + */ + public function link(Request $request): JsonResponse + { + $request->validate([ + 'sso_link_token' => 'required|string', + ]); + + /** @var LegacyUser $authUser */ + $authUser = $request->attributes->get('authenticated_user'); + $token = $request->input('sso_link_token'); + + $intent = Cache::get($this->linkIntentCacheKey($token)); + + if (! is_array($intent)) { + return response()->json(['success' => false, 'error' => 'link_intent_not_found'], 404); + } + + if (($intent['user_id'] ?? null) !== $authUser->id) { + Log::warning('SSO: link rejected — bearer JWT user does not match link intent', [ + 'authenticated_user' => $authUser->id, + 'intent_user' => $intent['user_id'] ?? null, + ]); + + return response()->json(['success' => false, 'error' => 'user_mismatch'], 403); + } + + $fresh = LegacyUser::find($authUser->id); + + if ($fresh->sso_sub !== null && $fresh->sso_sub !== $intent['sso_sub']) { + return response()->json(['success' => false, 'error' => 'already_linked'], 409); + } + + DB::transaction(function () use ($fresh, $intent) { + $fresh->sso_sub = $intent['sso_sub']; + $fresh->sso_provider = $intent['sso_provider'] ?? null; + $fresh->sso_id_token = $intent['sso_id_token'] ?? null; + $fresh->sso_refresh_token = $intent['sso_refresh_token'] ?? null; + $fresh->sso_idp_id_token = $intent['sso_idp_id_token'] ?? null; + $fresh->save(); + }); + + Cache::forget($this->linkIntentCacheKey($token)); + + return response()->json(['success' => true]); + } + /** * SSO logout endpoint. * @@ -342,10 +434,39 @@ protected function frontendRedirect(string $token): RedirectResponse return redirect("{$frontendUrl}/oauth-login/{$token}"); } - protected function frontendError(string $code): RedirectResponse + protected function frontendError(string $code, array $extra = []): RedirectResponse { $frontendUrl = rtrim(config('app.frontend_url', '/'), '/'); + $query = http_build_query(['sso_error' => $code] + $extra); + + return redirect("{$frontendUrl}/login?{$query}"); + } + + /** + * Persist an account-link intent in the cache and return the opaque token. + * The intent carries everything the link endpoint needs to stamp the row + * once the user has proven legacy-account possession via password. + */ + protected function storeLinkIntent(LegacyUser $emailMatch, $socialiteUser, Tenant $tenant): string + { + $token = bin2hex(random_bytes(16)); + + Cache::put($this->linkIntentCacheKey($token), [ + 'user_id' => $emailMatch->id, + 'email' => $emailMatch->email, + 'sso_sub' => $socialiteUser->getId(), + 'sso_provider' => $tenant->sso_provider, + 'sso_id_token' => $socialiteUser->accessTokenResponseBody['id_token'] ?? null, + 'sso_refresh_token' => $socialiteUser->refreshToken ?? null, + 'sso_idp_id_token' => $this->fetchIdpIdToken($socialiteUser->token, $tenant->sso_provider), + 'instance_code' => $tenant->instance_code, + ], now()->addMinutes(self::LINK_INTENT_TTL_MINUTES)); + + return $token; + } - return redirect("{$frontendUrl}/login?sso_error={$code}"); + protected function linkIntentCacheKey(string $token): string + { + return "sso_link:{$token}"; } } diff --git a/app/Services/SsoUserService.php b/app/Services/SsoUserService.php index 0e732654..e68297ff 100644 --- a/app/Services/SsoUserService.php +++ b/app/Services/SsoUserService.php @@ -5,45 +5,22 @@ use App\Models\LegacyUser; use App\Models\Tenant; use Illuminate\Support\Facades\DB; -use Illuminate\Support\Facades\Log; use Laravel\Socialite\Contracts\User as SocialiteUser; class SsoUserService { - /** - * Find an existing user by email or sso_sub in a single query. - * - * If both columns match different rows (corrupt state), the sso_sub match - * takes priority and a warning is logged so the duplicate can be cleaned up - * manually. - */ - public function resolveUser(?string $email, string $sub): ?LegacyUser + public function findBySub(string $sub): ?LegacyUser { - $candidates = LegacyUser::where('email', $email) - ->orWhere('sso_sub', $sub) - ->get(); + return LegacyUser::where('sso_sub', $sub)->first(); + } - if ($candidates->isEmpty()) { + public function findByEmail(?string $email): ?LegacyUser + { + if ($email === null || $email === '') { return null; } - if ($candidates->count() === 1) { - return $candidates->first(); - } - - $bySubMatch = $candidates->firstWhere('sso_sub', $sub); - $byEmailMatch = $candidates->firstWhere('email', $email); - - if ($bySubMatch && $byEmailMatch && $bySubMatch->id !== $byEmailMatch->id) { - Log::warning('SSO: email and sso_sub match different users — prioritising sso_sub match.', [ - 'email' => $email, - 'sub' => $sub, - 'sso_sub_user' => $bySubMatch->id, - 'email_user' => $byEmailMatch->id, - ]); - } - - return $bySubMatch ?? $byEmailMatch; + return LegacyUser::where('email', $email)->first(); } /** diff --git a/routes/tenant.php b/routes/tenant.php index 0d8d20e6..aa9f0ff5 100644 --- a/routes/tenant.php +++ b/routes/tenant.php @@ -36,6 +36,7 @@ Route::middleware('legacy.jwt')->group(function () { Route::post('/sso/logout', [\App\Http\Controllers\Auth\SsoController::class, 'logout'])->name('sso.logout'); + Route::post('/sso/link', [\App\Http\Controllers\Auth\SsoController::class, 'link'])->name('sso.link'); }); }); diff --git a/tests/Feature/SsoControllerTest.php b/tests/Feature/SsoControllerTest.php index 454ac9fb..5ee5fe26 100644 --- a/tests/Feature/SsoControllerTest.php +++ b/tests/Feature/SsoControllerTest.php @@ -130,23 +130,69 @@ public function test_callback_provisions_new_user_and_redirects_to_frontend(): v }); } - public function test_callback_finds_existing_user_by_email(): void + public function test_callback_with_email_match_no_sub_redirects_to_link_flow(): void { $existing = self::$testTenant->run(function () { - return $this->createUser('sso_existing@test.example', null); + return $this->createUser('sso_link@test.example', null); }); - Http::fake(['*/broker/*/token' => Http::response(['id_token' => 'idp.token.test'], 200)]); - $this->mockSocialiteCallback('sub-existing-email', 'sso_existing@test.example', 'Existing', 'existing'); + $this->mockSocialiteCallback('sub-link-001', 'sso_link@test.example', 'Linker', 'linker'); - $state = $this->buildState(self::INSTANCE_CODE); + $state = $this->buildState(self::INSTANCE_CODE); $response = $this->get("/api/v2/auth/sso/callback?state={$state}"); + $response->assertRedirect(); + $location = $response->headers->get('Location'); + $this->assertStringContainsString('sso_error=account_link_required', $location); + $this->assertMatchesRegularExpression('/sso_link=[a-f0-9]{32,}/', $location); + + // The legacy row must NOT be stamped yet — linking is gated on password proof. + self::$testTenant->run(function () use ($existing) { + $fresh = LegacyUser::find($existing->id); + $this->assertNull($fresh->sso_sub); + $this->assertNull($fresh->sso_id_token); + $this->assertNull($fresh->sso_provider); + }); + } + + public function test_callback_with_email_match_to_inactive_user_rejects_account_inactive_not_link(): void + { self::$testTenant->run(function () { - $this->assertEquals(1, LegacyUser::where('email', 'sso_existing@test.example')->count()); + $this->createUser('sso_inactive_email@test.example', null, LegacyUser::STATUS_SUSPENDED); }); - $this->assertRedirectAuthenticatesUser($response, $existing); + $this->mockSocialiteCallback('sub-inactive-email-001', 'sso_inactive_email@test.example', 'Inactive', 'inactive'); + + $state = $this->buildState(self::INSTANCE_CODE); + $response = $this->get("/api/v2/auth/sso/callback?state={$state}"); + + $response->assertRedirect(); + $location = $response->headers->get('Location'); + $this->assertStringContainsString('sso_error=account_inactive', $location); + $this->assertStringNotContainsString('sso_link=', $location); + } + + public function test_callback_with_email_match_to_user_having_different_sso_sub_rejects_sub_collision(): void + { + self::$testTenant->run(function () { + $this->createUser('sso_owned@test.example', 'existing-sub-aaa'); + }); + + $this->mockSocialiteCallback('intruder-sub-bbb', 'sso_owned@test.example', 'Intruder', 'intruder'); + + $state = $this->buildState(self::INSTANCE_CODE); + $response = $this->get("/api/v2/auth/sso/callback?state={$state}"); + + $response->assertRedirect(); + $location = $response->headers->get('Location'); + $this->assertStringContainsString('sso_error=sub_collision', $location); + $this->assertStringNotContainsString('sso_link=', $location); + + // No mutation on the original row. + self::$testTenant->run(function () { + $fresh = LegacyUser::where('email', 'sso_owned@test.example')->first(); + $this->assertEquals('existing-sub-aaa', $fresh->sso_sub); + }); } public function test_callback_finds_existing_user_by_sso_sub(): void @@ -267,6 +313,155 @@ public function test_callback_rejects_when_id_token_is_malformed(): void $this->assertStringContainsString('sso_error=email_not_verified', $response->headers->get('Location')); } + // ========================================================= + // POST /sso/link — password-proof account linking + // ========================================================= + + public function test_link_endpoint_stamps_sso_sub_and_tokens_when_bearer_matches_intent(): void + { + $user = self::$testTenant->run(fn () => $this->createUser('sso_linkme@test.example', null)); + + $linkToken = $this->primeLinkIntent([ + 'user_id' => $user->id, + 'email' => $user->email, + 'sso_sub' => 'sub-fresh-001', + 'sso_provider' => 'mock-iserv', + 'sso_id_token' => 'aula-id-token-linktest', + 'sso_refresh_token' => 'refresh-token-linktest', + 'sso_idp_id_token' => 'idp-id-token-linktest', + 'instance_code' => self::INSTANCE_CODE, + ]); + + $jwt = $this->jwtForUser($user); + + $response = $this->postJson('/api/v2/auth/sso/link', ['sso_link_token' => $linkToken], [ + 'aula-instance-code' => self::INSTANCE_CODE, + 'Authorization' => "Bearer {$jwt}", + ]); + + $response->assertOk()->assertJson(['success' => true]); + + self::$testTenant->run(function () use ($user) { + $fresh = LegacyUser::find($user->id); + $this->assertEquals('sub-fresh-001', $fresh->sso_sub); + $this->assertEquals('mock-iserv', $fresh->sso_provider); + $this->assertEquals('aula-id-token-linktest', $fresh->sso_id_token); + $this->assertEquals('refresh-token-linktest', $fresh->sso_refresh_token); + $this->assertEquals('idp-id-token-linktest', $fresh->sso_idp_id_token); + }); + } + + public function test_link_endpoint_rejects_when_bearer_jwt_user_does_not_match_intent(): void + { + [$victim, $attacker] = self::$testTenant->run(function () { + return [ + $this->createUser('sso_victim@test.example', null), + $this->createUser('sso_attacker@test.example', null), + ]; + }); + + $linkToken = $this->primeLinkIntent([ + 'user_id' => $victim->id, + 'email' => $victim->email, + 'sso_sub' => 'sub-take-over', + 'sso_provider' => 'mock-iserv', + 'sso_id_token' => 'tok', + 'instance_code' => self::INSTANCE_CODE, + ]); + + $jwt = $this->jwtForUser($attacker); + + $response = $this->postJson('/api/v2/auth/sso/link', ['sso_link_token' => $linkToken], [ + 'aula-instance-code' => self::INSTANCE_CODE, + 'Authorization' => "Bearer {$jwt}", + ]); + + $response->assertForbidden(); + + self::$testTenant->run(function () use ($victim) { + $fresh = LegacyUser::find($victim->id); + $this->assertNull($fresh->sso_sub); + }); + } + + public function test_link_endpoint_rejects_invalid_or_expired_token(): void + { + $user = self::$testTenant->run(fn () => $this->createUser('sso_bad@test.example', null)); + $jwt = $this->jwtForUser($user); + + $response = $this->postJson('/api/v2/auth/sso/link', ['sso_link_token' => 'does-not-exist-12345'], [ + 'aula-instance-code' => self::INSTANCE_CODE, + 'Authorization' => "Bearer {$jwt}", + ]); + + $response->assertStatus(404); + } + + public function test_link_endpoint_requires_bearer_jwt(): void + { + $response = $this->postJson('/api/v2/auth/sso/link', ['sso_link_token' => 'whatever'], [ + 'aula-instance-code' => self::INSTANCE_CODE, + ]); + + $response->assertUnauthorized(); + } + + public function test_link_endpoint_is_one_shot_consumes_intent_after_success(): void + { + $user = self::$testTenant->run(fn () => $this->createUser('sso_oneshot@test.example', null)); + + $linkToken = $this->primeLinkIntent([ + 'user_id' => $user->id, + 'email' => $user->email, + 'sso_sub' => 'sub-oneshot', + 'sso_provider' => 'mock-iserv', + 'sso_id_token' => 'tok', + 'instance_code' => self::INSTANCE_CODE, + ]); + + $jwt = $this->jwtForUser($user); + + $first = $this->postJson('/api/v2/auth/sso/link', ['sso_link_token' => $linkToken], [ + 'aula-instance-code' => self::INSTANCE_CODE, + 'Authorization' => "Bearer {$jwt}", + ]); + $first->assertOk(); + + $second = $this->postJson('/api/v2/auth/sso/link', ['sso_link_token' => $linkToken], [ + 'aula-instance-code' => self::INSTANCE_CODE, + 'Authorization' => "Bearer {$jwt}", + ]); + $second->assertStatus(404); + } + + public function test_link_endpoint_rejects_when_target_user_already_has_sso_sub(): void + { + $user = self::$testTenant->run(fn () => $this->createUser('sso_alreadylinked@test.example', 'sub-already-set')); + + $linkToken = $this->primeLinkIntent([ + 'user_id' => $user->id, + 'email' => $user->email, + 'sso_sub' => 'sub-different-new', + 'sso_provider' => 'mock-iserv', + 'sso_id_token' => 'tok', + 'instance_code' => self::INSTANCE_CODE, + ]); + + $jwt = $this->jwtForUser($user); + + $response = $this->postJson('/api/v2/auth/sso/link', ['sso_link_token' => $linkToken], [ + 'aula-instance-code' => self::INSTANCE_CODE, + 'Authorization' => "Bearer {$jwt}", + ]); + + $response->assertStatus(409); + + self::$testTenant->run(function () use ($user) { + $fresh = LegacyUser::find($user->id); + $this->assertEquals('sub-already-set', $fresh->sso_sub); + }); + } + // ========================================================= // resolveUser — collision handling // ========================================================= @@ -398,6 +593,21 @@ private function mockSocialiteCallback(string $sub, string $email, string $name, Socialite::shouldReceive('driver')->with('keycloak')->andReturn($provider); } + /** + * Seed a link intent directly into the cache and return the opaque token. + * Must run inside tenant context — CacheTenancyBootstrapper applies a + * per-tenant prefix, so a central write would not be visible to the + * tenant-scoped controller read. + */ + private function primeLinkIntent(array $intent): string + { + $token = bin2hex(random_bytes(16)); + self::$testTenant->run(function () use ($token, $intent) { + \Illuminate\Support\Facades\Cache::put("sso_link:{$token}", $intent, now()->addMinutes(10)); + }); + return $token; + } + private function makeIdToken(array $claims): string { $header = $this->base64UrlEncode(json_encode(['alg' => 'RS256', 'typ' => 'JWT'])); diff --git a/tests/Unit/SsoUserServiceTest.php b/tests/Unit/SsoUserServiceTest.php index a352efb3..b27de709 100644 --- a/tests/Unit/SsoUserServiceTest.php +++ b/tests/Unit/SsoUserServiceTest.php @@ -4,7 +4,6 @@ use App\Models\LegacyUser; use App\Services\SsoUserService; -use Illuminate\Support\Facades\Log; use Mockery; use Tests\Concerns\CreatesTestTenant; use Tests\TestCase; @@ -30,59 +29,57 @@ protected function tearDown(): void } // ========================================================= - // resolveUser + // findBySub / findByEmail // ========================================================= - public function test_resolve_user_returns_null_when_no_match(): void + public function test_find_by_sub_returns_null_when_no_match(): void { $result = self::$testTenant->run( - fn () => $this->service->resolveUser('nobody@sso.test', 'sub-nobody') + fn () => $this->service->findBySub('sub-nobody') ); $this->assertNull($result); } - public function test_resolve_user_matches_by_email(): void + public function test_find_by_sub_matches_existing_user(): void { - $user = self::$testTenant->run(fn () => $this->makeUser('unit_email@sso.test', null)); + $user = self::$testTenant->run(fn () => $this->makeUser('unit_sub@sso.test', 'sub-match-001')); $result = self::$testTenant->run( - fn () => $this->service->resolveUser('unit_email@sso.test', 'sub-not-in-db') + fn () => $this->service->findBySub('sub-match-001') ); $this->assertNotNull($result); $this->assertEquals($user->id, $result->id); } - public function test_resolve_user_matches_by_sso_sub(): void + public function test_find_by_email_returns_null_when_no_match(): void { - $user = self::$testTenant->run(fn () => $this->makeUser('unit_sub@sso.test', 'sub-match-001')); - $result = self::$testTenant->run( - fn () => $this->service->resolveUser('other@sso.test', 'sub-match-001') + fn () => $this->service->findByEmail('unit_nobody@sso.test') ); - $this->assertNotNull($result); - $this->assertEquals($user->id, $result->id); + $this->assertNull($result); } - public function test_resolve_user_prioritises_sso_sub_on_collision_and_logs_warning(): void + public function test_find_by_email_matches_existing_user(): void { - self::$testTenant->run(function () { - $this->makeUser('unit_collision@sso.test', null); - $this->makeUser('unit_other@sso.test', 'sub-collision-unit'); - }); - - Log::shouldReceive('warning') - ->once() - ->with(Mockery::pattern('/SSO: email and sso_sub match different users/'), Mockery::any()); + $user = self::$testTenant->run(fn () => $this->makeUser('unit_email@sso.test', null)); $result = self::$testTenant->run( - fn () => $this->service->resolveUser('unit_collision@sso.test', 'sub-collision-unit') + fn () => $this->service->findByEmail('unit_email@sso.test') ); $this->assertNotNull($result); - $this->assertEquals('sub-collision-unit', $result->sso_sub); + $this->assertEquals($user->id, $result->id); + } + + public function test_find_by_email_returns_null_for_null_or_empty(): void + { + self::$testTenant->run(function () { + $this->assertNull($this->service->findByEmail(null)); + $this->assertNull($this->service->findByEmail('')); + }); } // ========================================================= From f0bb23d338c597f0cb87f4cbe2cbc37f205c2d51 Mon Sep 17 00:00:00 2001 From: Edgar Zanella Alvarenga Date: Tue, 2 Jun 2026 17:39:22 +0200 Subject: [PATCH 3/7] feat: block password login and recovery for SSO users (#496) --- .../Auth/LegacyLoginController.php | 25 ++++ app/Models/Tenant.php | 1 + ...0001_add_sso_required_to_tenants_table.php | 29 +++++ legacy/src/controllers/forgot_password.php | 5 +- tests/Feature/LegacyLoginControllerTest.php | 117 ++++++++++++++++++ 5 files changed, 176 insertions(+), 1 deletion(-) create mode 100644 database/migrations/2026_06_02_000001_add_sso_required_to_tenants_table.php create mode 100644 tests/Feature/LegacyLoginControllerTest.php diff --git a/app/Http/Controllers/Auth/LegacyLoginController.php b/app/Http/Controllers/Auth/LegacyLoginController.php index b692d65a..1a02dfb4 100644 --- a/app/Http/Controllers/Auth/LegacyLoginController.php +++ b/app/Http/Controllers/Auth/LegacyLoginController.php @@ -4,6 +4,7 @@ use App\Http\Controllers\Controller; use App\Models\LegacyUser; +use App\Models\Tenant; use App\Services\LegacyJwtService; use Illuminate\Http\JsonResponse; use Illuminate\Http\Request; @@ -28,6 +29,19 @@ public function login(Request $request): JsonResponse $username = $request->input('username'); $password = $request->input('password'); + /** @var Tenant|null $tenant */ + $tenant = tenant(); + + // Tenants flagged sso_required reject password login for everyone, regardless + // of whether the specific user has finished SSO linking yet. + if ($tenant && $tenant->sso_required) { + return response()->json([ + 'success' => false, + 'error_code' => 3, + 'error' => 'tenant_requires_sso', + ]); + } + // Find user by username $user = LegacyUser::where('username', $username)->first(); @@ -38,6 +52,17 @@ public function login(Request $request): JsonResponse ]); } + // SSO-linked users must authenticate via the IdP. A local password is bypass + // surface — refuse the login so the local secret can never substitute for the + // IdP session. + if ($user->sso_sub !== null) { + return response()->json([ + 'success' => false, + 'error_code' => 3, + 'error' => 'use_sso', + ]); + } + // Check if user is active if (!$user->isActive()) { return response()->json([ diff --git a/app/Models/Tenant.php b/app/Models/Tenant.php index ec780245..7461bba1 100644 --- a/app/Models/Tenant.php +++ b/app/Models/Tenant.php @@ -32,6 +32,7 @@ public static function getCustomColumns(): array 'sso_provider', 'sso_idp_config', 'sso_force_logout', + 'sso_required', ]); } diff --git a/database/migrations/2026_06_02_000001_add_sso_required_to_tenants_table.php b/database/migrations/2026_06_02_000001_add_sso_required_to_tenants_table.php new file mode 100644 index 00000000..6447947d --- /dev/null +++ b/database/migrations/2026_06_02_000001_add_sso_required_to_tenants_table.php @@ -0,0 +1,29 @@ +boolean('sso_required')->default(false)->after('sso_force_logout') + ->comment('When true, password login is refused for all users in this tenant — SSO-only'); + } + }); + } + + public function down(): void + { + Schema::table('tenants', function (Blueprint $table) { + if (Schema::hasColumn('tenants', 'sso_required')) { + $table->dropColumn('sso_required'); + } + }); + } +}; diff --git a/legacy/src/controllers/forgot_password.php b/legacy/src/controllers/forgot_password.php index 942d86ac..840e3cf2 100644 --- a/legacy/src/controllers/forgot_password.php +++ b/legacy/src/controllers/forgot_password.php @@ -19,7 +19,10 @@ if ($_SERVER['REQUEST_METHOD'] === 'GET') { $email = $_GET["email"]; - $stmt = $db->query('SELECT id,username,realname FROM au_users_basedata WHERE email = :email'); + // SSO-linked users (sso_sub IS NOT NULL) cannot reset a local password — their + // identity lives in the IdP. Filtering at the query level keeps the response + // shape identical to the no-match case, preserving anti-enumeration. + $stmt = $db->query('SELECT id,username,realname FROM au_users_basedata WHERE email = :email AND sso_sub IS NULL'); $db->bind(':email', $email); $results = $db->resultSet(); diff --git a/tests/Feature/LegacyLoginControllerTest.php b/tests/Feature/LegacyLoginControllerTest.php new file mode 100644 index 00000000..0f7e5197 --- /dev/null +++ b/tests/Feature/LegacyLoginControllerTest.php @@ -0,0 +1,117 @@ +ensureTestTenantExists(); + self::$testTenant->update([ + 'sso_enabled' => true, + 'sso_required' => false, + ]); + } + + protected function tearDown(): void + { + self::$testTenant->run(fn () => LegacyUser::where('email', 'like', 'login_%@test.example')->delete()); + parent::tearDown(); + } + + public function test_login_succeeds_for_user_without_sso_sub_on_non_required_tenant(): void + { + self::$testTenant->run(fn () => $this->createUser('login_plain@test.example', 'login_plain', null)); + + $response = $this->postJson('/api/v2/legacy-auth/login', [ + 'username' => 'login_plain', + 'password' => self::PASSWORD, + ], ['aula-instance-code' => self::INSTANCE_CODE]); + + $response->assertOk()->assertJson(['success' => true])->assertJsonStructure(['JWT']); + } + + public function test_login_succeeds_for_user_without_sso_sub_when_tenant_allows_password_during_link_flow(): void + { + // Regression guard for #495 link flow — the SSO callback redirects email-matched + // users (sso_sub still NULL) to the legacy login so they can prove possession of + // their legacy password before linking. That login MUST still succeed. + self::$testTenant->run(fn () => $this->createUser('login_linkable@test.example', 'login_linkable', null)); + + $response = $this->postJson('/api/v2/legacy-auth/login', [ + 'username' => 'login_linkable', + 'password' => self::PASSWORD, + ], ['aula-instance-code' => self::INSTANCE_CODE]); + + $response->assertOk()->assertJson(['success' => true]); + } + + public function test_login_refused_for_user_with_sso_sub_set(): void + { + self::$testTenant->run(fn () => $this->createUser('login_ssoUser@test.example', 'login_ssouser', 'sub-already-linked')); + + $response = $this->postJson('/api/v2/legacy-auth/login', [ + 'username' => 'login_ssouser', + 'password' => self::PASSWORD, + ], ['aula-instance-code' => self::INSTANCE_CODE]); + + $response->assertOk(); + $response->assertJson(['success' => false, 'error_code' => 3, 'error' => 'use_sso']); + $this->assertArrayNotHasKey('JWT', $response->json()); + } + + public function test_login_refused_when_tenant_has_sso_required_even_without_user_sso_sub(): void + { + self::$testTenant->update(['sso_required' => true]); + self::$testTenant->run(fn () => $this->createUser('login_required@test.example', 'login_required', null)); + + $response = $this->postJson('/api/v2/legacy-auth/login', [ + 'username' => 'login_required', + 'password' => self::PASSWORD, + ], ['aula-instance-code' => self::INSTANCE_CODE]); + + $response->assertOk(); + $response->assertJson(['success' => false, 'error_code' => 3, 'error' => 'tenant_requires_sso']); + $this->assertArrayNotHasKey('JWT', $response->json()); + } + + public function test_login_refused_for_wrong_password_returns_generic_error(): void + { + self::$testTenant->run(fn () => $this->createUser('login_wrong@test.example', 'login_wrong', null)); + + $response = $this->postJson('/api/v2/legacy-auth/login', [ + 'username' => 'login_wrong', + 'password' => 'wrong-password', + ], ['aula-instance-code' => self::INSTANCE_CODE]); + + $response->assertOk(); + $response->assertJson(['success' => false, 'error_code' => 2]); + } + + private function createUser(string $email, string $username, ?string $sub): LegacyUser + { + $user = new LegacyUser; + $user->email = $email; + $user->username = $username; + $user->sso_sub = $sub; + $user->status = LegacyUser::STATUS_ACTIVE; + $user->hash_id = md5($email . microtime(true)); + $user->userlevel = 20; + $user->roles = json_encode([]); + $user->refresh_token = false; + $user->pw = password_hash(self::PASSWORD, PASSWORD_BCRYPT); + $user->save(); + + return $user; + } +} From 8da65cc2934473abedd81020daa34489a8708fbe Mon Sep 17 00:00:00 2001 From: Edgar Zanella Alvarenga Date: Wed, 3 Jun 2026 11:48:18 +0200 Subject: [PATCH 4/7] fix: enforce sso_sub and sso_required blocks in legacy login.php --- legacy/src/classes/helpers/InstanceConfig.php | 11 +++++-- legacy/src/controllers/login.php | 31 +++++++++++++++++++ 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/legacy/src/classes/helpers/InstanceConfig.php b/legacy/src/classes/helpers/InstanceConfig.php index 8b08783e..8b938c68 100644 --- a/legacy/src/classes/helpers/InstanceConfig.php +++ b/legacy/src/classes/helpers/InstanceConfig.php @@ -10,6 +10,7 @@ class InstanceConfig public string $dbname; public string $jwt_key; public string $instance_api_url; + public bool $sso_required; private static ?PDO $centralDb = null; @@ -21,7 +22,8 @@ public function __construct( string $dbname, string $jwt_key, string $instance_api_url, - string $port = '3306' + string $port = '3306', + bool $sso_required = false ) { $this->code = $code; $this->host = $host; @@ -31,6 +33,7 @@ public function __construct( $this->dbname = $dbname; $this->jwt_key = $jwt_key; $this->instance_api_url = $instance_api_url; + $this->sso_required = $sso_required; } private static function getCentralDb(): PDO @@ -53,7 +56,7 @@ private static function getCentralDb(): PDO private static function findTenantByCode(string $code): ?array { $stmt = self::getCentralDb()->prepare( - 'SELECT instance_code, jwt_key, api_base_url, data FROM tenants WHERE instance_code = ? LIMIT 1' + 'SELECT instance_code, jwt_key, api_base_url, data, sso_required FROM tenants WHERE instance_code = ? LIMIT 1' ); $stmt->execute([$code]); $row = $stmt->fetch(PDO::FETCH_ASSOC); @@ -61,7 +64,7 @@ private static function findTenantByCode(string $code): ?array } public static function findAll(): ?array { - $stmt = self::getCentralDb()->prepare('SELECT instance_code, jwt_key, api_base_url, data FROM tenants'); + $stmt = self::getCentralDb()->prepare('SELECT instance_code, jwt_key, api_base_url, data, sso_required FROM tenants'); $stmt->execute(); $rows = $stmt->fetchAll(PDO::FETCH_ASSOC); @@ -83,6 +86,7 @@ public static function findAll(): ?array { jwt_key: $tenant['jwt_key'] ?? '', instance_api_url: $tenant['api_base_url'] ?? '', port: getenv('CENTRAL_DB_PORT') ?: '3306', + sso_required: (bool) ($tenant['sso_required'] ?? false), ); } @@ -150,6 +154,7 @@ public static function createFromCode(string $code): InstanceConfig jwt_key: $tenant['jwt_key'] ?? '', instance_api_url: $tenant['api_base_url'] ?? '', port: getenv('CENTRAL_DB_PORT') ?: '3306', + sso_required: (bool) ($tenant['sso_required'] ?? false), ); } } diff --git a/legacy/src/controllers/login.php b/legacy/src/controllers/login.php index 50af6290..6b89a885 100644 --- a/legacy/src/controllers/login.php +++ b/legacy/src/controllers/login.php @@ -21,6 +21,19 @@ header('Content-Type: application/json; charset=utf-8'); + +// SSO-only tenants reject password login outright, regardless of which user is +// attempting it. The check happens before any DB work so unauthenticated +// requests cannot probe the user table on SSO-locked tenants. +if ($instance->sso_required) { + echo json_encode([ + 'success' => false, + 'error_code' => 3, + 'error' => 'tenant_requires_sso', + ]); + return; +} + $json = file_get_contents('php://input'); // Converts it into a PHP object @@ -34,6 +47,24 @@ } if ($loginResult["success"] && $loginResult["error_code"] == 0) { + // Refuse password login for SSO-linked users — local password is bypass surface + // for an identity that lives in the IdP. Mirrors the Laravel LegacyLoginController + // check; this controller is the one the React frontend actually hits. + $user_id = $loginResult["data"]["id"] ?? null; + if ($user_id !== null) { + $stmt = $db->query('SELECT sso_sub FROM ' . $db->au_users_basedata . ' WHERE id = :id'); + $db->bind(':id', $user_id); + $row = $db->resultSet(); + if (!empty($row[0]['sso_sub'])) { + echo json_encode([ + 'success' => false, + 'error_code' => 3, + 'error' => 'use_sso', + ]); + return; + } + } + $current_settings = $settings->getInstanceSettings(); if ($current_settings["data"]["online_mode"] != 1 && $loginResult["data"]["userlevel"] < 50) { echo json_encode([ From cde89dafe75b0b65002ad95a94c22a05f90d1898 Mon Sep 17 00:00:00 2001 From: Edgar Zanella Alvarenga Date: Wed, 3 Jun 2026 12:48:07 +0200 Subject: [PATCH 5/7] fix: block legacy forgot_password on sso_required tenants --- legacy/src/controllers/forgot_password.php | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/legacy/src/controllers/forgot_password.php b/legacy/src/controllers/forgot_password.php index 840e3cf2..a5d8a318 100644 --- a/legacy/src/controllers/forgot_password.php +++ b/legacy/src/controllers/forgot_password.php @@ -18,6 +18,15 @@ $jwt = new JWT($instance->jwt_key, $db, $crypt, $syslog); if ($_SERVER['REQUEST_METHOD'] === 'GET') { + // SSO-only tenants have no concept of local password recovery. Short-circuit + // before any DB work, returning the same response shape as the no-match case + // so the endpoint cannot be used to enumerate which tenants are SSO-locked. + if ($instance->sso_required) { + header('Content-Type: application/json; charset=utf-8'); + echo json_encode(["success" => true]); + return; + } + $email = $_GET["email"]; // SSO-linked users (sso_sub IS NOT NULL) cannot reset a local password — their // identity lives in the IdP. Filtering at the query level keeps the response From 52b49271b017626be44e7c7d6205aaab58de6ed9 Mon Sep 17 00:00:00 2001 From: Edgar Zanella Alvarenga Date: Thu, 11 Jun 2026 09:41:57 +0200 Subject: [PATCH 6/7] fix: address Psalm warnings on SsoController - Declare int type on LINK_INTENT_TTL_MINUTES const - Null-guard LegacyUser::find() in link() before property access - Mark decodeIdTokenPayload and linkIntentCacheKey as @psalm-pure - Use strict comparison on optional id_token check - Type-hint $socialiteUser param (Two\\User base + OAuth2\\User docblock) --- app/Http/Controllers/Auth/SsoController.php | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/app/Http/Controllers/Auth/SsoController.php b/app/Http/Controllers/Auth/SsoController.php index be330f5e..d6cf2b60 100644 --- a/app/Http/Controllers/Auth/SsoController.php +++ b/app/Http/Controllers/Auth/SsoController.php @@ -19,7 +19,7 @@ class SsoController extends Controller { - private const LINK_INTENT_TTL_MINUTES = 10; + private const int LINK_INTENT_TTL_MINUTES = 10; public function __construct( protected LegacyJwtService $jwtService, @@ -199,6 +199,10 @@ public function link(Request $request): JsonResponse $fresh = LegacyUser::find($authUser->id); + if ($fresh === null) { + return response()->json(['success' => false, 'error' => 'user_not_found'], 404); + } + if ($fresh->sso_sub !== null && $fresh->sso_sub !== $intent['sso_sub']) { return response()->json(['success' => false, 'error' => 'already_linked'], 409); } @@ -364,10 +368,12 @@ protected function fetchIdpIdToken(?string $accessToken, ?string $provider): ?st /** * Decode an OIDC id_token (JWT) payload without verifying the signature. * Returns null when the token is missing, malformed, or the payload is not valid JSON. + * + * @psalm-pure */ protected function decodeIdTokenPayload(?string $idToken): ?array { - if (! $idToken) { + if ($idToken === null || $idToken === '') { return null; } @@ -392,6 +398,8 @@ protected function decodeIdTokenPayload(?string $idToken): ?array /** * OIDC: the email claim is only trustworthy when email_verified is strictly true. + * + * @psalm-pure */ protected function isEmailVerified(?array $idTokenPayload): bool { @@ -446,8 +454,10 @@ protected function frontendError(string $code, array $extra = []): RedirectRespo * Persist an account-link intent in the cache and return the opaque token. * The intent carries everything the link endpoint needs to stamp the row * once the user has proven legacy-account possession via password. + * + * @param \SocialiteProviders\Manager\OAuth2\User $socialiteUser */ - protected function storeLinkIntent(LegacyUser $emailMatch, $socialiteUser, Tenant $tenant): string + protected function storeLinkIntent(LegacyUser $emailMatch, \Laravel\Socialite\Two\User $socialiteUser, Tenant $tenant): string { $token = bin2hex(random_bytes(16)); @@ -465,6 +475,9 @@ protected function storeLinkIntent(LegacyUser $emailMatch, $socialiteUser, Tenan return $token; } + /** + * @psalm-pure + */ protected function linkIntentCacheKey(string $token): string { return "sso_link:{$token}"; From 3b27cefc62954832b384a793337398d7485c92e9 Mon Sep 17 00:00:00 2001 From: Edgar Zanella Alvarenga Date: Thu, 11 Jun 2026 09:42:06 +0200 Subject: [PATCH 7/7] refactor: drop error_code from v2 legacy login, use error slugs Addresses PR #532 review feedback: the v1 login response uses both `error_code` and `error` which is redundant. v2 is a new endpoint, so this drops `error_code` entirely and relies on `error` slugs: - tenant_requires_sso (was error_code: 3) - use_sso (was error_code: 3) - bad_credentials (was error_code: 2, no slug previously) - inactive user keeps success: true plus user_status/user_id/data/count (only error_code: 2 dropped, behavior preserved) Tests updated to assert the new slugs and that error_code is absent. The duplicate `_when_tenant_allows_password_during_link_flow` test was removed; its #495 regression-guard comment was folded into the remaining `_on_non_required_tenant` test, which exercises the same path. Also replace `assertArrayNotHasKey('JWT', $response->json())` with `assertJsonMissingPath('JWT')` per reviewer nitpick. --- .../Auth/LegacyLoginController.php | 23 ++++++-------- tests/Feature/LegacyAuthTest.php | 10 +++--- tests/Feature/LegacyLoginControllerTest.php | 31 +++++++------------ 3 files changed, 26 insertions(+), 38 deletions(-) diff --git a/app/Http/Controllers/Auth/LegacyLoginController.php b/app/Http/Controllers/Auth/LegacyLoginController.php index 1a02dfb4..98d0bad9 100644 --- a/app/Http/Controllers/Auth/LegacyLoginController.php +++ b/app/Http/Controllers/Auth/LegacyLoginController.php @@ -36,9 +36,8 @@ public function login(Request $request): JsonResponse // of whether the specific user has finished SSO linking yet. if ($tenant && $tenant->sso_required) { return response()->json([ - 'success' => false, - 'error_code' => 3, - 'error' => 'tenant_requires_sso', + 'success' => false, + 'error' => 'tenant_requires_sso', ]); } @@ -48,7 +47,7 @@ public function login(Request $request): JsonResponse if ($user === null) { return response()->json([ 'success' => false, - 'error_code' => 2, + 'error' => 'bad_credentials', ]); } @@ -57,21 +56,19 @@ public function login(Request $request): JsonResponse // IdP session. if ($user->sso_sub !== null) { return response()->json([ - 'success' => false, - 'error_code' => 3, - 'error' => 'use_sso', + 'success' => false, + 'error' => 'use_sso', ]); } // Check if user is active if (!$user->isActive()) { return response()->json([ - 'success' => true, - 'error_code' => 2, + 'success' => true, 'user_status' => $user->status, - 'user_id' => $user->id, - 'data' => $this->getReactivationDate($user), - 'count' => 1, + 'user_id' => $user->id, + 'data' => $this->getReactivationDate($user), + 'count' => 1, ]); } @@ -79,7 +76,7 @@ public function login(Request $request): JsonResponse if (!$user->checkPassword($password)) { return response()->json([ 'success' => false, - 'error_code' => 2, + 'error' => 'bad_credentials', ]); } diff --git a/tests/Feature/LegacyAuthTest.php b/tests/Feature/LegacyAuthTest.php index 38646c2a..48b9cf84 100644 --- a/tests/Feature/LegacyAuthTest.php +++ b/tests/Feature/LegacyAuthTest.php @@ -191,7 +191,7 @@ public function test_login_wrong_password(): void ]); $response->assertStatus(200) - ->assertJson(['success' => false, 'error_code' => 2]) + ->assertJson(['success' => false, 'error' => 'bad_credentials']) ->assertJsonMissing(['JWT']); $tenant->run(function () { @@ -211,7 +211,7 @@ public function test_login_nonexistent_user(): void ]); $response->assertStatus(200) - ->assertJson(['success' => false, 'error_code' => 2]); + ->assertJson(['success' => false, 'error' => 'bad_credentials']); } public function test_login_inactive_user(): void @@ -242,11 +242,11 @@ public function test_login_inactive_user(): void $response->assertStatus(200) ->assertJson([ - 'success' => true, - 'error_code' => 2, + 'success' => true, 'user_status' => LegacyUser::STATUS_SUSPENDED, ]) - ->assertJsonMissing(['JWT']); + ->assertJsonMissing(['JWT']) + ->assertJsonMissingPath('error_code'); $tenant->run(function () { LegacyUser::where('username', 'phpunit_inactive')->delete(); diff --git a/tests/Feature/LegacyLoginControllerTest.php b/tests/Feature/LegacyLoginControllerTest.php index 0f7e5197..a5653a6b 100644 --- a/tests/Feature/LegacyLoginControllerTest.php +++ b/tests/Feature/LegacyLoginControllerTest.php @@ -29,6 +29,9 @@ protected function tearDown(): void parent::tearDown(); } + // Also acts as the regression guard for #495 link flow — the SSO callback redirects + // email-matched users (sso_sub still NULL) to the legacy login so they can prove + // possession of their legacy password before linking. That login MUST still succeed. public function test_login_succeeds_for_user_without_sso_sub_on_non_required_tenant(): void { self::$testTenant->run(fn () => $this->createUser('login_plain@test.example', 'login_plain', null)); @@ -41,21 +44,6 @@ public function test_login_succeeds_for_user_without_sso_sub_on_non_required_ten $response->assertOk()->assertJson(['success' => true])->assertJsonStructure(['JWT']); } - public function test_login_succeeds_for_user_without_sso_sub_when_tenant_allows_password_during_link_flow(): void - { - // Regression guard for #495 link flow — the SSO callback redirects email-matched - // users (sso_sub still NULL) to the legacy login so they can prove possession of - // their legacy password before linking. That login MUST still succeed. - self::$testTenant->run(fn () => $this->createUser('login_linkable@test.example', 'login_linkable', null)); - - $response = $this->postJson('/api/v2/legacy-auth/login', [ - 'username' => 'login_linkable', - 'password' => self::PASSWORD, - ], ['aula-instance-code' => self::INSTANCE_CODE]); - - $response->assertOk()->assertJson(['success' => true]); - } - public function test_login_refused_for_user_with_sso_sub_set(): void { self::$testTenant->run(fn () => $this->createUser('login_ssoUser@test.example', 'login_ssouser', 'sub-already-linked')); @@ -66,8 +54,9 @@ public function test_login_refused_for_user_with_sso_sub_set(): void ], ['aula-instance-code' => self::INSTANCE_CODE]); $response->assertOk(); - $response->assertJson(['success' => false, 'error_code' => 3, 'error' => 'use_sso']); - $this->assertArrayNotHasKey('JWT', $response->json()); + $response->assertJson(['success' => false, 'error' => 'use_sso']); + $response->assertJsonMissingPath('JWT'); + $response->assertJsonMissingPath('error_code'); } public function test_login_refused_when_tenant_has_sso_required_even_without_user_sso_sub(): void @@ -81,8 +70,9 @@ public function test_login_refused_when_tenant_has_sso_required_even_without_use ], ['aula-instance-code' => self::INSTANCE_CODE]); $response->assertOk(); - $response->assertJson(['success' => false, 'error_code' => 3, 'error' => 'tenant_requires_sso']); - $this->assertArrayNotHasKey('JWT', $response->json()); + $response->assertJson(['success' => false, 'error' => 'tenant_requires_sso']); + $response->assertJsonMissingPath('JWT'); + $response->assertJsonMissingPath('error_code'); } public function test_login_refused_for_wrong_password_returns_generic_error(): void @@ -95,7 +85,8 @@ public function test_login_refused_for_wrong_password_returns_generic_error(): v ], ['aula-instance-code' => self::INSTANCE_CODE]); $response->assertOk(); - $response->assertJson(['success' => false, 'error_code' => 2]); + $response->assertJson(['success' => false, 'error' => 'bad_credentials']); + $response->assertJsonMissingPath('error_code'); } private function createUser(string $email, string $username, ?string $sub): LegacyUser