fix(oauth2): prevent profile redirect during OIDC consent flow#120
fix(oauth2): prevent profile redirect during OIDC consent flow#120
Conversation
📝 WalkthroughWalkthroughRedirects authenticated users from app login to the OAuth2 provider authorization endpoint, adds detailed logging and stricter processing/marking of ID token and login/OTP hints in the interactive grant flow, broadens exception handling with logout on errors, and adds comprehensive unit tests plus CI env tweaks. Changes
Sequence DiagramsequenceDiagram
actor User
participant Client
participant OAuth2LoginStrategy
participant AuthService
participant OAuth2ProviderController
participant InteractiveGrantType
participant Logger
User->>Client: initiate /login with OAuth2 request
Client->>OAuth2LoginStrategy: getLogin(request)
OAuth2LoginStrategy->>AuthService: isAuthenticated?
AuthService-->>OAuth2LoginStrategy: authenticated
OAuth2LoginStrategy->>Logger: debug "user already authenticated, redirecting to auth endpoint"
OAuth2LoginStrategy->>OAuth2ProviderController: redirect to /oauth2/auth
OAuth2ProviderController->>InteractiveGrantType: processUserHint(request)
InteractiveGrantType->>Logger: debug "processing user hints"
InteractiveGrantType->>InteractiveGrantType: check IDTokenHint processed?
alt IDTokenHint not processed
InteractiveGrantType->>InteractiveGrantType: process IDTokenHint
InteractiveGrantType->>InteractiveGrantType: mark IDTokenHint processed
InteractiveGrantType->>Logger: debug "ID token processed"
else already processed
InteractiveGrantType->>Logger: warn "skip duplicate ID token processing"
end
InteractiveGrantType-->>OAuth2ProviderController: return outcome
OAuth2ProviderController-->>User: redirect to OAuth2 authorization endpoint
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-120/ This page is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/Strategies/OAuth2LoginStrategy.php (1)
64-67: Use the injectedIAuthServicefor consistency, not the LaravelAuthfacade.The class has
IAuthServiceavailable via the parent constructor, but line 64 checksAuth::guest()directly. While both are currently synchronized (they read/write the same Laravel Auth system, sologout(false)does clear the session), using the injected service—!$this->auth_service->isUserLogged()—reduces coupling and maintains consistency throughout the class. No behavior change needed, but prefer the injected dependency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Strategies/OAuth2LoginStrategy.php` around lines 64 - 67, In OAuth2LoginStrategy::getLogin replace the direct Laravel Auth facade check (Auth::guest()) with the injected auth service call so the condition reads using $this->auth_service->isUserLogged() (negated as needed); keep the existing Log::debug and Redirect::action behavior, ensuring you use !$this->auth_service->isUserLogged() in place of Auth::guest() to reduce coupling and maintain consistency with the class's injected IAuthService.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/libs/OAuth2/GrantTypes/InteractiveGrantType.php`:
- Around line 464-472: The new Log::debug call in
InteractiveGrant::processUserHint currently logs the entire OAuth2Request via
OAuth2Request::__toString(), which may leak sensitive fields (login_hint,
id_token_hint, OTPs); replace these full-request traces with a minimal debug
message that includes only non-sensitive info: the client ID (from
$client->getId()), boolean flags indicating which hints were present/handled
(e.g., hasLoginHint, hasIdTokenHint, processed flag), and any processing
outcome. Apply the same change to the other debug statements in processUserHint
(the block around the later Log::debug calls referenced) so no raw request
serialization is written to logs.
---
Nitpick comments:
In `@app/Strategies/OAuth2LoginStrategy.php`:
- Around line 64-67: In OAuth2LoginStrategy::getLogin replace the direct Laravel
Auth facade check (Auth::guest()) with the injected auth service call so the
condition reads using $this->auth_service->isUserLogged() (negated as needed);
keep the existing Log::debug and Redirect::action behavior, ensuring you use
!$this->auth_service->isUserLogged() in place of Auth::guest() to reduce
coupling and maintain consistency with the class's injected IAuthService.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: deaa76c5-7497-4c8f-beeb-6b50f65121c5
📒 Files selected for processing (4)
app/Strategies/OAuth2LoginStrategy.phpapp/libs/OAuth2/GrantTypes/InteractiveGrantType.phptests/unit/InteractiveGrantTypeTest.phptests/unit/OAuth2LoginStrategyTest.php
| Log::debug | ||
| ( | ||
| sprintf | ||
| ( | ||
| "InteractiveGrant::processUserHint request %s client %s", | ||
| $request->__toString(), | ||
| $client->getId() | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Don't log raw OIDC request payloads here.
OAuth2Request::__toString() serializes the full message, so these new debug lines will write login_hint, id_token_hint, OTP hints, and other auth parameters into logs. That's sensitive user/token material on a hot path; keep the trace to booleans, client ID, and processed-flag state instead.
Also applies to: 702-710
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/libs/OAuth2/GrantTypes/InteractiveGrantType.php` around lines 464 - 472,
The new Log::debug call in InteractiveGrant::processUserHint currently logs the
entire OAuth2Request via OAuth2Request::__toString(), which may leak sensitive
fields (login_hint, id_token_hint, OTPs); replace these full-request traces with
a minimal debug message that includes only non-sensitive info: the client ID
(from $client->getId()), boolean flags indicating which hints were
present/handled (e.g., hasLoginHint, hasIdTokenHint, processed flag), and any
processing outcome. Apply the same change to the other debug statements in
processUserHint (the block around the later Log::debug calls referenced) so no
raw request serialization is written to logs.
smarcet
force-pushed
the
hotfix/oauth2-consent-flow
branch
from
7392139 to
7473695
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-120/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unit/InteractiveGrantTypeTest.php (2)
156-220: Clean up$_SERVER['REMOTE_ADDR']intearDown().The superglobal is modified in
setUp()(line 161) but not restored intearDown(). This could cause test pollution if other tests rely on$_SERVERstate.♻️ Suggested fix
class InteractiveGrantTypeTest extends TestCase { private $auth_service; // ... other properties ... + private $original_remote_addr; protected function setUp(): void { parent::setUp(); // Set up a minimal facade root so Log:: calls don't crash. + $this->original_remote_addr = $_SERVER['REMOTE_ADDR'] ?? null; $_SERVER['REMOTE_ADDR'] = '127.0.0.1';protected function tearDown(): void { Mockery::close(); Facade::clearResolvedInstances(); Facade::setFacadeApplication(null); + if ($this->original_remote_addr !== null) { + $_SERVER['REMOTE_ADDR'] = $this->original_remote_addr; + } else { + unset($_SERVER['REMOTE_ADDR']); + } parent::tearDown(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/InteractiveGrantTypeTest.php` around lines 156 - 220, The test modifies $_SERVER['REMOTE_ADDR'] in setUp() and doesn't restore it; capture the original value at start of setUp (e.g. $this->originalRemoteAddr = $_SERVER['REMOTE_ADDR'] ?? null), continue to set $_SERVER['REMOTE_ADDR'] = '127.0.0.1' as now, and then restore that original value in tearDown() (if $this->originalRemoteAddr is null unset($_SERVER['REMOTE_ADDR']) else $_SERVER['REMOTE_ADDR'] = $this->originalRemoteAddr); add the $originalRemoteAddr property to the InteractiveGrantTypeTest class and update setUp() and tearDown() accordingly to avoid test pollution.
325-403: Consider suppressing the unused$clientvariable warning.The
$clientvariable fromsetupValidClient()is not directly used in these tests (lines 331, 379), though the method's side effects (setting up repository mocks) are required. You could prefix with underscore or discard the return value to clarify intent.♻️ Optional: Make intent explicit
- $client = $this->setupValidClient(); + $this->setupValidClient(); // Sets up client repository mockOr if you prefer to keep the variable for documentation:
- $client = $this->setupValidClient(); + /** `@noinspection` PhpUnusedLocalVariableInspection */ + $client = $this->setupValidClient();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/InteractiveGrantTypeTest.php` around lines 325 - 403, The local $client returned by setupValidClient() in the tests testHandleLogsOutUserWhenProcessUserHintThrowsException and testRegressionProcessUserHintExceptionWithoutLogoutCausedProfileRedirect is unused; to suppress warnings and make intent clear, stop assigning it to an unused variable by either calling $this->setupValidClient(); with no assignment or assign to a throwaway ($_ = $this->setupValidClient();). Update the two occurrences of "$client = $this->setupValidClient();" to one of these options so the method's side effects remain but no unused-variable warning is emitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/InteractiveGrantTypeTest.php`:
- Around line 156-220: The test modifies $_SERVER['REMOTE_ADDR'] in setUp() and
doesn't restore it; capture the original value at start of setUp (e.g.
$this->originalRemoteAddr = $_SERVER['REMOTE_ADDR'] ?? null), continue to set
$_SERVER['REMOTE_ADDR'] = '127.0.0.1' as now, and then restore that original
value in tearDown() (if $this->originalRemoteAddr is null
unset($_SERVER['REMOTE_ADDR']) else $_SERVER['REMOTE_ADDR'] =
$this->originalRemoteAddr); add the $originalRemoteAddr property to the
InteractiveGrantTypeTest class and update setUp() and tearDown() accordingly to
avoid test pollution.
- Around line 325-403: The local $client returned by setupValidClient() in the
tests testHandleLogsOutUserWhenProcessUserHintThrowsException and
testRegressionProcessUserHintExceptionWithoutLogoutCausedProfileRedirect is
unused; to suppress warnings and make intent clear, stop assigning it to an
unused variable by either calling $this->setupValidClient(); with no assignment
or assign to a throwaway ($_ = $this->setupValidClient();). Update the two
occurrences of "$client = $this->setupValidClient();" to one of these options so
the method's side effects remain but no unused-variable warning is emitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f056ed98-c41a-4bd0-80f4-f2a60db6f0ad
📒 Files selected for processing (5)
.github/workflows/pull_request_unit_tests.ymlapp/Strategies/OAuth2LoginStrategy.phpapp/libs/OAuth2/GrantTypes/InteractiveGrantType.phptests/unit/InteractiveGrantTypeTest.phptests/unit/OAuth2LoginStrategyTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- app/libs/OAuth2/GrantTypes/InteractiveGrantType.php
- tests/unit/OAuth2LoginStrategyTest.php
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.
smarcet
force-pushed
the
hotfix/oauth2-consent-flow
branch
from
7473695 to
8c2742a
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-120/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/OAuth2LoginStrategyTest.php (1)
180-215: Consider a cleaner assertion approach for the guest flow test.The test relies on catching an exception to prove the code path was reached, which makes the test somewhat fragile. If the implementation changes to not throw, the test might pass for the wrong reason.
💡 Alternative approach
Consider verifying the guest flow by checking that
memento_service->load()was called, which is a cleaner assertion than exception-based flow verification:- $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'); + try { + $this->strategy->getLogin(); + $this->fail('Expected exception when memento is null'); + } catch (\Throwable $e) { + // Expected behavior - verify load was called + } + + // The key assertion: memento_service->load() was invoked, + // proving we passed the Auth::guest() check + $this->memento_service->shouldHaveReceived('load')->once();Alternatively, mock
load()to return a valid memento and assert on the final response type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/OAuth2LoginStrategyTest.php` around lines 180 - 215, Replace the exception-based proof with a direct assertion that the memento loading was executed: in testGetLoginProceedsToLoginFormWhenUserIsGuest(), keep the mocked Request and the call to $this->strategy->getLogin(), but instead of wrapping it in a try/catch and asserting an exception, assert that $this->memento_service->load() was invoked (or alternatively mock load() to return a valid memento and assert the returned response from getLogin()), referencing the existing memento_service->shouldReceive('load') expectation and the getLogin() call to validate the guest flow without relying on thrown exceptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/libs/OAuth2/GrantTypes/InteractiveGrantType.php`:
- Around line 723-726: Add a narrow catch for the protocol exceptions thrown by
processUserHint so they propagate instead of triggering logout: inside
InteractiveGrant::mustAuthenticateUser, insert a catch block for
InvalidLoginHint | InteractionRequiredException that simply re-throws the
exception before the existing generic catch(Exception $ex) block; keep the
existing generic catch to Log::warning, call $this->auth_service->logout(false)
and return true for non-protocol exceptions so only unexpected errors cause
logout.
---
Nitpick comments:
In `@tests/unit/OAuth2LoginStrategyTest.php`:
- Around line 180-215: Replace the exception-based proof with a direct assertion
that the memento loading was executed: in
testGetLoginProceedsToLoginFormWhenUserIsGuest(), keep the mocked Request and
the call to $this->strategy->getLogin(), but instead of wrapping it in a
try/catch and asserting an exception, assert that $this->memento_service->load()
was invoked (or alternatively mock load() to return a valid memento and assert
the returned response from getLogin()), referencing the existing
memento_service->shouldReceive('load') expectation and the getLogin() call to
validate the guest flow without relying on thrown exceptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cc989bb9-983b-45d2-9373-23a1247feb94
📒 Files selected for processing (5)
.github/workflows/pull_request_unit_tests.ymlapp/Strategies/OAuth2LoginStrategy.phpapp/libs/OAuth2/GrantTypes/InteractiveGrantType.phptests/unit/InteractiveGrantTypeTest.phptests/unit/OAuth2LoginStrategyTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- app/Strategies/OAuth2LoginStrategy.php
- .github/workflows/pull_request_unit_tests.yml
| catch (Exception $ex){ | ||
| Log::warning($ex); | ||
| Log::warning("InteractiveGrant::mustAuthenticateUser processUserHint generic error", [ 'error' => $ex]); | ||
| $this->auth_service->logout(false); | ||
| return true; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's a narrow catch for protocol exceptions before the generic catch
ast-grep --pattern $'catch (InvalidLoginHint $$$) {
$$$
}'
# Also search for any catch blocks in mustAuthenticateUser
rg -n -A5 'catch.*Exception' app/libs/OAuth2/GrantTypes/InteractiveGrantType.php | head -40Repository: OpenStackweb/openstackid
Length of output: 1379
🏁 Script executed:
#!/bin/bash
# Find the processUserHint method to understand what exceptions it can throw
rg -n -B 20 'catch (JWSInvalidJWKException' app/libs/OAuth2/GrantTypes/InteractiveGrantType.php | head -80Repository: OpenStackweb/openstackid
Length of output: 154
🏁 Script executed:
#!/bin/bash
# Search for InvalidLoginHint and InteractionRequiredException in the codebase
rg -r 'InvalidLoginHint|InteractionRequiredException' --type php | head -20Repository: OpenStackweb/openstackid
Length of output: 464
🏁 Script executed:
#!/bin/bash
# Look for the processUserHint method definition to see what it can throw
rg -n -A 30 'function processUserHint' app/libs/OAuth2/GrantTypes/InteractiveGrantType.php | head -60Repository: OpenStackweb/openstackid
Length of output: 1802
🏁 Script executed:
#!/bin/bash
# Search for InvalidLoginHint and InteractionRequiredException definitions
rg 'InvalidLoginHint|InteractionRequiredException' --type php -nRepository: OpenStackweb/openstackid
Length of output: 1791
🏁 Script executed:
#!/bin/bash
# Get broader context of the try-catch block starting from line 458
sed -n '458,730p' app/libs/OAuth2/GrantTypes/InteractiveGrantType.php | cat -nRepository: OpenStackweb/openstackid
Length of output: 12696
🏁 Script executed:
#!/bin/bash
# Check the end of processUserHint method to see where try starts
rg -n 'try' app/libs/OAuth2/GrantTypes/InteractiveGrantType.php | grep -A2 -B2 "70[0-9]:"Repository: OpenStackweb/openstackid
Length of output: 50
Add narrow catch for protocol exceptions before the generic Exception catch.
The processUserHint() method throws InvalidLoginHint (for malformed hints) and InteractionRequiredException (for interaction requirements). Both are documented in the method's @throws declarations and thrown at multiple points within the method. These protocol exceptions should propagate to the client as OAuth2 errors, not trigger user logout.
The current code catches all Exception types generically at line 266-269, which incorrectly logs out users for protocol errors like bad auth URLs. Add a narrow catch for InvalidLoginHint | InteractionRequiredException before the generic Exception catch to re-throw these exceptions without logout:
Suggested structure
catch (InvalidLoginHint | InteractionRequiredException $ex) {
throw $ex;
}
catch (Exception $ex){
Log::warning("InteractiveGrant::mustAuthenticateUser processUserHint generic error", [ 'error' => $ex]);
$this->auth_service->logout(false);
return true;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/libs/OAuth2/GrantTypes/InteractiveGrantType.php` around lines 723 - 726,
Add a narrow catch for the protocol exceptions thrown by processUserHint so they
propagate instead of triggering logout: inside
InteractiveGrant::mustAuthenticateUser, insert a catch block for
InvalidLoginHint | InteractionRequiredException that simply re-throws the
exception before the existing generic catch(Exception $ex) block; keep the
existing generic catch to Log::warning, call $this->auth_service->logout(false)
and return true for non-protocol exceptions so only unexpected errors cause
logout.
1 participant
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.
Summary by CodeRabbit
Bug Fixes
Tests
Chores