From 8c2742a2ad9df67dd676251288ed583ee13a5dfb Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 16 Mar 2026 11:26:16 -0300 Subject: [PATCH] fix(oauth2): prevent profile redirect during OIDC consent flow During an OIDC authorization code flow, users were intermittently redirected to the IDP profile page instead of back to the client application after submitting the consent form. --- .github/workflows/pull_request_unit_tests.yml | 2 + app/Strategies/OAuth2LoginStrategy.php | 6 +- .../GrantTypes/InteractiveGrantType.php | 28 +- tests/unit/InteractiveGrantTypeTest.php | 606 ++++++++++++++++++ tests/unit/OAuth2LoginStrategyTest.php | 216 +++++++ 5 files changed, 853 insertions(+), 5 deletions(-) create mode 100644 tests/unit/InteractiveGrantTypeTest.php create mode 100644 tests/unit/OAuth2LoginStrategyTest.php diff --git a/.github/workflows/pull_request_unit_tests.yml b/.github/workflows/pull_request_unit_tests.yml index 319b1d1d..158ab56a 100644 --- a/.github/workflows/pull_request_unit_tests.yml +++ b/.github/workflows/pull_request_unit_tests.yml @@ -35,6 +35,8 @@ jobs: SSL_ENABLED: false SESSION_DRIVER: redis PHP_VERSION: 8.3 + OTEL_SDK_DISABLED: true + OTEL_SERVICE_ENABLED: false services: mysql: image: mysql:8.0 diff --git a/app/Strategies/OAuth2LoginStrategy.php b/app/Strategies/OAuth2LoginStrategy.php index ba4a234d..bcbd5123 100644 --- a/app/Strategies/OAuth2LoginStrategy.php +++ b/app/Strategies/OAuth2LoginStrategy.php @@ -61,8 +61,10 @@ public function getLogin() { Log::debug("OAuth2LoginStrategy::getLogin"); - if (!Auth::guest()) - return Redirect::action("UserController@getProfile"); + if (!Auth::guest()) { + Log::debug("OAuth2LoginStrategy::getLogin user already authenticated, redirecting to auth endpoint"); + return Redirect::action("OAuth2\OAuth2ProviderController@auth"); + } $this->login_hint_process_strategy->process(); diff --git a/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php b/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php index 9eb22477..b1dbfa52 100644 --- a/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php +++ b/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php @@ -461,7 +461,16 @@ protected function processUserHint(OAuth2AuthenticationRequest $request, Client $token_hint = $request->getIdTokenHint(); $otp_login_hint = $request->getOTPLoginHint(); - Log::debug(sprintf("InteractiveGrant::processUserHint request %s client %s", $request->__toString(), $client->getId())); + Log::debug + ( + sprintf + ( + "InteractiveGrant::processUserHint request %s client %s", + $request->__toString(), + $client->getId() + ) + ); + // process login hint $user = null; if (!empty($otp_login_hint) && !empty ($login_hint) @@ -484,7 +493,7 @@ protected function processUserHint(OAuth2AuthenticationRequest $request, Client $user = $this->auth_service->getUserById($user_id); } $request->markParamAsProcessed(OAuth2Protocol::OAuth2Protocol_LoginHint); - } else if(!empty($token_hint)) { + } else if(!empty($token_hint) && !$request->isProcessedParam(OAuth2Protocol::OAuth2Protocol_IDTokenHint)) { Log::debug("InteractiveGrant::processUserHint processing Token hint..."); $jwt = BasicJWTFactory::build($token_hint); @@ -544,6 +553,7 @@ protected function processUserHint(OAuth2AuthenticationRequest $request, Client $this->auth_service->reloadSession($jti->getValue()); + $request->markParamAsProcessed(OAuth2Protocol::OAuth2Protocol_IDTokenHint); } if(!is_null($user)) { @@ -689,6 +699,15 @@ protected function shouldForceReLogin(OAuth2AuthorizationRequest $request, IClie */ protected function mustAuthenticateUser(OAuth2AuthorizationRequest $request, Client $client) { + Log::debug + ( + sprintf + ( + "InteractiveGrant::mustAuthenticateUser: request %s client %s", + $request->__toString(), + $client->getClientId() + ) + ); if ($request instanceof OAuth2AuthenticationRequest) { try { @@ -702,19 +721,22 @@ protected function mustAuthenticateUser(OAuth2AuthorizationRequest $request, Cli throw $ex; } catch (Exception $ex){ - Log::warning($ex); + Log::warning("InteractiveGrant::mustAuthenticateUser processUserHint generic error", [ 'error' => $ex]); + $this->auth_service->logout(false); return true; } } if($this->shouldPromptLogin($request)) { + Log::warning("InteractiveGrant::mustAuthenticateUser: shouldPromptLogin"); $this->auth_service->logout(false); return true; } if($this->shouldForceReLogin($request, $client)) { + Log::warning("InteractiveGrant::mustAuthenticateUser: shouldForceReLogin"); $this->auth_service->logout(false); return true; } diff --git a/tests/unit/InteractiveGrantTypeTest.php b/tests/unit/InteractiveGrantTypeTest.php new file mode 100644 index 00000000..5c10fe39 --- /dev/null +++ b/tests/unit/InteractiveGrantTypeTest.php @@ -0,0 +1,606 @@ +handle($request); + } +} + +/** + * Class InteractiveGrantTypeTest + * + * Tests for InteractiveGrantType focusing on: + * - Fix A: mustAuthenticateUser logs out user when processUserHint throws + * - Fix C: id_token_hint is only processed once (isProcessedParam guard) + * - Regression: the old buggy behavior that redirected to profile + * + * @package Tests\unit + */ +class InteractiveGrantTypeTest extends TestCase +{ + private $auth_service; + private $memento_service; + private $client_repository; + private $token_service; + private $client_service; + private $log_service; + private $security_context_service; + private $principal_service; + private $user_consent_service; + private $scope_service; + private $auth_strategy; + private $server_private_key_repository; + private $jwk_set_reader_service; + private $grant_type; + + protected function setUp(): void + { + parent::setUp(); + + // Clear any resolved facade instances left by prior tests (e.g., + // integration tests that bootstrap the full Laravel Application). + Facade::clearResolvedInstances(); + Facade::setFacadeApplication(null); + + // Set up a minimal facade root so Log:: calls don't crash. + $_SERVER['REMOTE_ADDR'] = '127.0.0.1'; + $app = new Container(); + $logger = Mockery::mock(LoggerInterface::class); + $logger->shouldReceive('debug', 'info', 'warning', 'error', 'critical', 'log') + ->zeroOrMoreTimes() + ->andReturnNull(); + $app->instance('log', $logger); + Facade::setFacadeApplication($app); + + $this->auth_service = Mockery::mock(IAuthService::class); + $this->memento_service = Mockery::mock(IMementoOAuth2SerializerService::class); + $this->client_repository = Mockery::mock(IClientRepository::class); + $this->token_service = Mockery::mock(ITokenService::class); + $this->client_service = Mockery::mock(IClientService::class); + $this->log_service = Mockery::mock(ILogService::class); + $this->security_context_service = Mockery::mock(ISecurityContextService::class); + $this->principal_service = Mockery::mock(IPrincipalService::class); + $this->user_consent_service = Mockery::mock(IUserConsentService::class); + $this->scope_service = Mockery::mock(IApiScopeService::class); + $this->auth_strategy = Mockery::mock(IOAuth2AuthenticationStrategy::class); + $this->server_private_key_repository = Mockery::mock(IServerPrivateKeyRepository::class); + $this->jwk_set_reader_service = Mockery::mock(IClientJWKSetReader::class); + + // Suppress log calls + $this->log_service->shouldReceive('debug_msg')->andReturnNull(); + $this->log_service->shouldReceive('warning')->andReturnNull(); + $this->log_service->shouldReceive('warning_msg')->andReturnNull(); + $this->log_service->shouldReceive('error')->andReturnNull(); + $this->log_service->shouldReceive('error_msg')->andReturnNull(); + $this->log_service->shouldReceive('info')->andReturnNull(); + + // Default principal service mock (used by shouldForceReLogin) + $principal = new Principal(); + $principal->setState([0, time(), '']); + $this->principal_service->shouldReceive('get')->andReturn($principal)->byDefault(); + + $this->grant_type = new TestableInteractiveGrantType( + $this->client_service, + $this->client_repository, + $this->token_service, + $this->log_service, + $this->security_context_service, + $this->principal_service, + $this->auth_service, + $this->user_consent_service, + $this->scope_service, + $this->auth_strategy, + $this->memento_service, + $this->server_private_key_repository, + $this->jwk_set_reader_service + ); + } + + protected function tearDown(): void + { + Mockery::close(); + Facade::clearResolvedInstances(); + Facade::setFacadeApplication(null); + parent::tearDown(); + } + + /** + * Build a valid OAuth2AuthenticationRequest (OIDC) with the given extra params merged in. + */ + private function buildOIDCRequest(array $extra = []): OAuth2AuthenticationRequest + { + $params = array_merge([ + OAuth2Protocol::OAuth2Protocol_ResponseType => OAuth2Protocol::OAuth2Protocol_ResponseType_Code, + OAuth2Protocol::OAuth2Protocol_ClientId => 'test-client-id', + OAuth2Protocol::OAuth2Protocol_RedirectUri => 'https://client.example.com/callback', + OAuth2Protocol::OAuth2Protocol_Scope => 'openid profile', + OAuth2Protocol::OAuth2Protocol_State => 'random-state', + OAuth2Protocol::OAuth2Protocol_Nonce => 'random-nonce', + ], $extra); + + $msg = new OAuth2Message($params); + $auth_request = new OAuth2AuthorizationRequest($msg); + return new OAuth2AuthenticationRequest($auth_request); + } + + /** + * Set up common mocks for a client that passes all validation checks. + */ + private function setupValidClient(): Client + { + $client = Mockery::mock(Client::class); + $client->shouldReceive('isActive')->andReturn(true); + $client->shouldReceive('isLocked')->andReturn(false); + $client->shouldReceive('getApplicationName')->andReturn('Test App'); + $client->shouldReceive('getId')->andReturn(1); + $client->shouldReceive('getClientId')->andReturn('test-client-id'); + $client->shouldReceive('isUriAllowed')->andReturn(true); + $client->shouldReceive('isScopeAllowed')->andReturn(true); + $client->shouldReceive('getDefaultMaxAge')->andReturn(0); + $client->shouldReceive('getMaxAllowedUserSessions')->andReturn(0); + + $this->client_repository + ->shouldReceive('getClientById') + ->with('test-client-id') + ->andReturn($client); + + return $client; + } + + /** + * Set up mocks for a logged-in user who has already authenticated. + */ + private function setupLoggedInUser(): User + { + $user = Mockery::mock(User::class); + $user->shouldReceive('getId')->andReturn(42); + + $this->auth_service->shouldReceive('isUserLogged')->andReturn(true); + $this->auth_service->shouldReceive('getCurrentUser')->andReturn($user); + $this->auth_service->shouldReceive('getUserAuthenticationResponse') + ->andReturn(IAuthService::AuthenticationResponse_None); + + // principal_service->get() is called by shouldForceReLogin() + $principal = new Principal(); + $principal->setState([42, time(), 'test-opbs']); + $this->principal_service->shouldReceive('get')->andReturn($principal); + + return $user; + } + + /** + * Set up security context mock. + */ + private function setupSecurityContext(): void + { + $security_context = new SecurityContext(); + + $this->security_context_service + ->shouldReceive('get') + ->andReturn($security_context); + + $this->security_context_service + ->shouldReceive('save') + ->andReturnNull(); + } + + /** + * Allow the auth_service and memento_service cleanup calls that happen + * in the outer catch block of handle() or in normal flow cleanup. + */ + private function allowCleanupCalls(): void + { + $this->auth_service->shouldReceive('clearUserAuthorizationResponse')->andReturnNull(); + $this->auth_service->shouldReceive('clearUserAuthenticationResponse')->andReturnNull(); + $this->memento_service->shouldReceive('forget')->andReturnNull(); + } + + // ----------------------------------------------------------------------- + // Fix A: mustAuthenticateUser logs user out when processUserHint throws + // ----------------------------------------------------------------------- + + /** + * When processUserHint throws a generic exception during handle(), + * the user MUST be logged out before being redirected to login. + * + * This is the fix for the bug where the user stayed authenticated, + * causing OAuth2LoginStrategy::getLogin() to redirect to profile + * instead of showing the login page. + */ + public function testHandleLogsOutUserWhenProcessUserHintThrowsException(): void + { + $request = $this->buildOIDCRequest([ + OAuth2Protocol::OAuth2Protocol_LoginHint => 'nonexistent@example.com', + ]); + + $client = $this->setupValidClient(); + $this->setupSecurityContext(); + $this->allowCleanupCalls(); + + // The user is currently logged in + $this->auth_service->shouldReceive('isUserLogged')->andReturn(true); + $this->auth_service->shouldReceive('getUserAuthenticationResponse') + ->andReturn(IAuthService::AuthenticationResponse_None); + + // getUserByUsername will throw, simulating a DB error or similar failure + $this->auth_service->shouldReceive('getUserByUsername') + ->with('nonexistent@example.com') + ->andThrow(new Exception('User lookup failed')); + + // FIX A: logout(false) MUST be called before redirecting to login + $this->auth_service->shouldReceive('logout') + ->with(false) + ->once(); + + // After the exception, the flow should save the memento and redirect to login + $this->memento_service->shouldReceive('serialize')->once(); + + $login_redirect = 'login-redirect-response'; + $this->auth_strategy->shouldReceive('doLogin') + ->once() + ->andReturn($login_redirect); + + $result = $this->grant_type->publicHandle($request); + + $this->assertEquals($login_redirect, $result); + } + + /** + * Regression test: Without fix A, the old code did NOT call logout() + * when processUserHint threw, leaving the user authenticated. + * This verifies the fix is in place by asserting logout IS called. + */ + public function testRegressionProcessUserHintExceptionWithoutLogoutCausedProfileRedirect(): void + { + // Simulate the exact scenario that caused the bug: + // 1. User is logged in (just submitted consent) + // 2. processUserHint throws on re-entry (e.g., login_hint lookup fails) + // 3. Without logout, doLogin -> getLogin() -> user is logged in -> profile redirect + + $request = $this->buildOIDCRequest([ + OAuth2Protocol::OAuth2Protocol_LoginHint => 'bad-hint', + ]); + + $client = $this->setupValidClient(); + $this->setupSecurityContext(); + $this->allowCleanupCalls(); + + $this->auth_service->shouldReceive('isUserLogged')->andReturn(true); + $this->auth_service->shouldReceive('getUserAuthenticationResponse') + ->andReturn(IAuthService::AuthenticationResponse_None); + + // login_hint is not a valid email, so unwrapUserId is called + $this->auth_service->shouldReceive('unwrapUserId') + ->with('bad-hint') + ->andThrow(new Exception('Invalid user identifier')); + + // The critical assertion: logout MUST be called + $this->auth_service->shouldReceive('logout') + ->with(false) + ->once(); + + $this->memento_service->shouldReceive('serialize')->once(); + $this->auth_strategy->shouldReceive('doLogin')->once()->andReturn('login-response'); + + $result = $this->grant_type->publicHandle($request); + + $this->assertEquals('login-response', $result); + } + + // ----------------------------------------------------------------------- + // Fix C: id_token_hint processed only once + // ----------------------------------------------------------------------- + + /** + * After id_token_hint is processed, subsequent passes through handle() + * must NOT re-process it (the isProcessedParam guard must work). + */ + public function testIdTokenHintIsNotReprocessedOnSubsequentPasses(): void + { + // Build a request where login_hint and id_token_hint are both already processed + $request = $this->buildOIDCRequest([ + OAuth2Protocol::OAuth2Protocol_LoginHint => 'user@example.com', + OAuth2Protocol::OAuth2Protocol_IDTokenHint => 'some.jwt.token', + ]); + + // Mark both as already processed (simulating previous passes) + $request->markParamAsProcessed(OAuth2Protocol::OAuth2Protocol_LoginHint); + $request->markParamAsProcessed(OAuth2Protocol::OAuth2Protocol_IDTokenHint); + + $client = $this->setupValidClient(); + $user = $this->setupLoggedInUser(); + $this->setupSecurityContext(); + + $this->token_service->shouldReceive('canCreateAccessToken') + ->with($user, $client) + ->andReturn(true); + + // User has already consented + $this->auth_service->shouldReceive('getUserAuthorizationResponse') + ->andReturn(IAuthService::AuthorizationResponse_AllowOnce); + $this->auth_service->shouldReceive('registerRPLogin')->once(); + $this->auth_service->shouldReceive('clearUserAuthorizationResponse')->once(); + + $user->shouldReceive('findFirstConsentByClientAndScopes') + ->andReturn(null); + $this->user_consent_service->shouldReceive('addUserConsent')->once(); + + $this->memento_service->shouldReceive('forget')->once(); + + // The key assertion: user-lookup and JWT methods should NOT be called + // because both hints are already processed. + $this->auth_service->shouldNotReceive('getUserByUsername'); + $this->auth_service->shouldNotReceive('unwrapUserId'); + $this->auth_service->shouldNotReceive('reloadSession'); + + $result = $this->grant_type->publicHandle($request); + + // Should get an OAuth2Response (successful authorization), not a redirect to login + $this->assertInstanceOf(OAuth2Response::class, $result); + } + + /** + * Verify that the isProcessedParam / markParamAsProcessed mechanism + * works correctly for id_token_hint through the memento round-trip. + */ + public function testIdTokenHintProcessedParamPersistsThroughMemento(): void + { + $request = $this->buildOIDCRequest([ + OAuth2Protocol::OAuth2Protocol_LoginHint => 'user@example.com', + OAuth2Protocol::OAuth2Protocol_IDTokenHint => 'some.jwt.token', + ]); + + // Mark login_hint as already processed + $request->markParamAsProcessed(OAuth2Protocol::OAuth2Protocol_LoginHint); + + // id_token_hint is NOT yet processed + $this->assertFalse( + $request->isProcessedParam(OAuth2Protocol::OAuth2Protocol_IDTokenHint) + ); + + // Mark it as processed (simulating what the code does after first pass) + $request->markParamAsProcessed(OAuth2Protocol::OAuth2Protocol_IDTokenHint); + + // Verify it IS now processed + $this->assertTrue( + $request->isProcessedParam(OAuth2Protocol::OAuth2Protocol_IDTokenHint) + ); + + // Verify it survives a memento round-trip + $memento = $request->getMessage()->createMemento(); + $rebuilt_msg = OAuth2Message::buildFromMemento($memento); + $rebuilt_auth = new OAuth2AuthorizationRequest($rebuilt_msg); + $rebuilt_request = new OAuth2AuthenticationRequest($rebuilt_auth); + + $this->assertTrue( + $rebuilt_request->isProcessedParam(OAuth2Protocol::OAuth2Protocol_IDTokenHint), + 'id_token_hint processed flag must survive memento serialization round-trip' + ); + $this->assertTrue( + $rebuilt_request->isProcessedParam(OAuth2Protocol::OAuth2Protocol_LoginHint), + 'login_hint processed flag must survive memento serialization round-trip' + ); + } + + // ----------------------------------------------------------------------- + // Normal flow: consent accepted -> successful authorization + // ----------------------------------------------------------------------- + + /** + * When the user has already consented (AllowOnce), handle() should + * build a successful response and clear the memento. + */ + public function testHandleReturnsResponseWhenUserConsented(): void + { + $request = $this->buildOIDCRequest(); + + $client = $this->setupValidClient(); + $user = $this->setupLoggedInUser(); + $this->setupSecurityContext(); + + $this->token_service->shouldReceive('canCreateAccessToken') + ->with($user, $client) + ->andReturn(true); + + $this->auth_service->shouldReceive('getUserAuthorizationResponse') + ->andReturn(IAuthService::AuthorizationResponse_AllowOnce); + $this->auth_service->shouldReceive('registerRPLogin')->once(); + $this->auth_service->shouldReceive('clearUserAuthorizationResponse')->once(); + + $user->shouldReceive('findFirstConsentByClientAndScopes') + ->andReturn(null); + $this->user_consent_service->shouldReceive('addUserConsent')->once(); + + $this->memento_service->shouldReceive('forget')->once(); + + $result = $this->grant_type->publicHandle($request); + + $this->assertInstanceOf(OAuth2Response::class, $result); + } + + /** + * When the user is not logged in and no hints are provided, + * handle() should redirect to the login page. + */ + public function testHandleRedirectsToLoginWhenUserNotAuthenticated(): void + { + $request = $this->buildOIDCRequest(); + + $client = $this->setupValidClient(); + $this->setupSecurityContext(); + $this->allowCleanupCalls(); + + // User is NOT logged in + $this->auth_service->shouldReceive('isUserLogged')->andReturn(false); + $this->auth_service->shouldReceive('getUserAuthenticationResponse') + ->andReturn(IAuthService::AuthenticationResponse_None); + + $this->memento_service->shouldReceive('serialize')->once(); + + $login_redirect = 'login-redirect'; + $this->auth_strategy->shouldReceive('doLogin') + ->once() + ->andReturn($login_redirect); + + $result = $this->grant_type->publicHandle($request); + + $this->assertEquals($login_redirect, $result); + } + + /** + * When prompt=consent and user has no prior consent, handle() + * should redirect to the consent page. + */ + public function testHandleRedirectsToConsentWhenPromptConsentSet(): void + { + $request = $this->buildOIDCRequest([ + OAuth2Protocol::OAuth2Protocol_Prompt => OAuth2Protocol::OAuth2Protocol_Prompt_Consent, + ]); + + $client = $this->setupValidClient(); + $user = $this->setupLoggedInUser(); + $this->setupSecurityContext(); + $this->allowCleanupCalls(); + + $this->token_service->shouldReceive('canCreateAccessToken') + ->with($user, $client) + ->andReturn(true); + + $this->auth_service->shouldReceive('getUserAuthorizationResponse') + ->andReturn(IAuthService::AuthorizationResponse_None); + + $user->shouldReceive('findFirstConsentByClientAndScopes') + ->andReturn(null); + + $this->memento_service->shouldReceive('serialize')->once(); + + $consent_redirect = 'consent-redirect'; + $this->auth_strategy->shouldReceive('doConsent') + ->once() + ->andReturn($consent_redirect); + + $result = $this->grant_type->publicHandle($request); + + $this->assertEquals($consent_redirect, $result); + } +} diff --git a/tests/unit/OAuth2LoginStrategyTest.php b/tests/unit/OAuth2LoginStrategyTest.php new file mode 100644 index 00000000..701b9d11 --- /dev/null +++ b/tests/unit/OAuth2LoginStrategyTest.php @@ -0,0 +1,216 @@ +app = new Container(); + + $logger = Mockery::mock(LoggerInterface::class); + $logger->shouldReceive('debug', 'info', 'warning', 'error', 'critical', 'log') + ->zeroOrMoreTimes() + ->andReturnNull(); + $this->app->instance('log', $logger); + + Facade::setFacadeApplication($this->app); + + // Create service mocks + $this->auth_service = Mockery::mock(IAuthService::class); + $this->memento_service = Mockery::mock(IMementoOAuth2SerializerService::class); + $this->user_action_service = Mockery::mock(IUserActionService::class); + + // LoginHintProcessStrategy is final, so we create a real instance with mocked deps + $security_context_service = Mockery::mock(ISecurityContextService::class); + $security_context_service->shouldReceive('get')->andReturnNull()->byDefault(); + $this->login_hint_process_strategy = new LoginHintProcessStrategy( + $this->auth_service, + $security_context_service + ); + + $this->strategy = new OAuth2LoginStrategy( + $this->auth_service, + $this->memento_service, + $this->user_action_service, + $this->login_hint_process_strategy + ); + } + + protected function tearDown(): void + { + Mockery::close(); + Facade::clearResolvedInstances(); + Facade::setFacadeApplication(null); + parent::tearDown(); + } + + /** + * Set up the Auth facade mock. + */ + private function mockAuth(bool $isGuest): void + { + $authGuard = Mockery::mock(); + $authGuard->shouldReceive('guest')->andReturn($isGuest); + + $authManager = Mockery::mock(); + $authManager->shouldReceive('guard')->withAnyArgs()->andReturn($authGuard); + // Auth::guest() forwards to guard()->guest() via __call + $authManager->shouldReceive('guest')->andReturn($isGuest); + + $this->app->instance('auth', $authManager); + } + + /** + * Set up the Redirect facade mock and return it for assertions. + */ + private function mockRedirect(): Mockery\MockInterface + { + $redirector = Mockery::mock(); + $this->app->instance('redirect', $redirector); + return $redirector; + } + + // ----------------------------------------------------------------------- + // Fix B: authenticated user redirects to auth endpoint, not profile + // ----------------------------------------------------------------------- + + /** + * When the user is already authenticated during an OAuth2 flow, + * getLogin() MUST redirect to the OAuth2 authorization endpoint + * so the flow can continue. + */ + public function testGetLoginRedirectsToAuthEndpointWhenUserAuthenticated(): void + { + $this->mockAuth(isGuest: false); + + $expectedRedirect = Mockery::mock(RedirectResponse::class); + $redirector = $this->mockRedirect(); + $redirector->shouldReceive('action') + ->with("OAuth2\OAuth2ProviderController@auth") + ->once() + ->andReturn($expectedRedirect); + + $result = $this->strategy->getLogin(); + + $this->assertSame($expectedRedirect, $result); + } + + /** + * Regression test: the old code redirected authenticated users to + * UserController@getProfile, which broke the OAuth2 flow. + * Verify that getProfile is never the redirect target. + */ + public function testGetLoginDoesNotRedirectToProfileWhenUserAuthenticated(): void + { + $this->mockAuth(isGuest: false); + + $authEndpointRedirect = Mockery::mock(RedirectResponse::class); + $redirector = $this->mockRedirect(); + $redirector->shouldReceive('action') + ->with("OAuth2\OAuth2ProviderController@auth") + ->once() + ->andReturn($authEndpointRedirect); + + // This must NOT be called + $redirector->shouldNotReceive('action') + ->with("UserController@getProfile"); + + $result = $this->strategy->getLogin(); + + $this->assertSame($authEndpointRedirect, $result, + 'Authenticated user must be redirected to auth endpoint, not profile'); + } + + /** + * When the user is a guest (not authenticated), getLogin() should NOT + * return the early redirect — it should proceed past the guard check. + * We verify that neither the auth endpoint nor the profile redirect is + * returned, and that the method proceeds into the login form logic. + */ + public function testGetLoginProceedsToLoginFormWhenUserIsGuest(): void + { + $this->mockAuth(isGuest: true); + + $redirector = $this->mockRedirect(); + // Neither redirect should be called for guests + $redirector->shouldNotReceive('action') + ->with("OAuth2\OAuth2ProviderController@auth"); + $redirector->shouldNotReceive('action') + ->with("UserController@getProfile"); + + // The strategy proceeds past the guard check into login_hint processing + // and memento loading. Since we're using a real LoginHintProcessStrategy + // without a full request context, we need the Request facade set up. + $request = Mockery::mock(); + $request->shouldReceive('has')->andReturn(false); + $request->shouldReceive('query')->andReturn(null); + $this->app->instance('request', $request); + + // After login_hint processing, the strategy loads the memento. + // Return null to trigger an exception — we just need to prove + // the method got past the Auth::guest() check. + $this->memento_service->shouldReceive('load')->once()->andReturnNull(); + + $reachedMemento = false; + try { + $this->strategy->getLogin(); + } catch (\Throwable $e) { + // Expected: proceeds past Auth::guest() check, fails when + // trying to build from a null memento. + $reachedMemento = true; + } + + $this->assertTrue($reachedMemento, + 'Guest path must proceed past Auth::guest() check into memento loading'); + } +}