fix: add missing parameters to Authentik OAuth2 token requests #9869
Conversation
This commit fixes the Authentik OAuth2 provider by: - Adding the missing 'code' parameter in getTokens() method - Adding the missing 'refresh_token' parameter in refreshTokens() method Without these parameters, authentication and token refresh would fail. Added unit tests to verify the fix and prevent regression.
WalkthroughThe update modifies the OAuth2 authorization endpoint in the Authentik provider by adding a trailing slash to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AuthentikProvider
participant OAuth2Server
User->>AuthentikProvider: Request login URL
AuthentikProvider->>OAuth2Server: Redirect to /application/o/authorize/? with params
OAuth2Server-->>User: OAuth2 consent screen
User->>OAuth2Server: Authorize
OAuth2Server-->>AuthentikProvider: Redirect with code
AuthentikProvider->>OAuth2Server: Exchange code for tokens
OAuth2Server-->>AuthentikProvider: Return access & refresh tokens
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed he 8000 lp? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/unit/Auth/OAuth2/AuthentikTest.php (2)
26-64: LGTM: Comprehensive test for code parameter inclusion.The test effectively verifies that the
codeparameter is included in the token request using appropriate mocking and reflection techniques. The callback validation ensures the parameter is correctly passed to the HTTP request.Consider enhancing the validation to check for all required parameters in the token request:
$this->callback(function ($queryString) { parse_str($queryString, $params); - return isset($params['code']) && $params['code'] === 'test-code'; + $requiredParams = ['code', 'client_id', 'client_secret', 'redirect_uri', 'grant_type']; + foreach ($requiredParams as $param) { + if (!isset($params[$param])) { + return false; + } + } + return $params['code'] === 'test-code'; })
136-146: LGTM: Validates URL generation with proper OAuth parameters.The test correctly verifies that the login URL includes all necessary OAuth parameters and confirms the trailing slash addition in the authorization endpoint.
Consider enhancing the URL validation for more robust parameter checking:
public function testGetLoginURL(): void { $authentik = new Authentik($this->appId, $this->appSecret, $this->callback); $loginUrl = $authentik->getLoginURL(); - // Check that the URL contains the necessary parameters - $this->assertStringContainsString('client_id=' . $this->appId, $loginUrl); - $this->assertStringContainsString('redirect_uri=' . urlencode($this->callback), $loginUrl); - $this->assertStringContainsString('response_type=code', $loginUrl); - $this->assertStringContainsString('https://authentik.example.com/application/o/authorize/', $loginUrl); + // Parse URL and validate parameters + $parsedUrl = parse_url($loginUrl); + parse_str($parsedUrl['query'], $params); + + $this->assertEquals('https://authentik.example.com/application/o/authorize/', + $parsedUrl['scheme'] . '://' . $parsedUrl['host'] . $parsedUrl['path']); + $this->assertEquals($this->appId, $params['client_id']); + $this->assertEquals($this->callback, $params['redirect_uri']); + $this->assertEquals('code', $params['response_type']); + $this->assertArrayHasKey('scope', $params); + $this->assertArrayHasKey('state', $params); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Appwrite/Auth/OAuth2/Authentik.php(1 hunks)tests/unit/Auth/OAuth2/AuthentikTest.php(1 hunks)
🔇 Additional comments (4)
src/Appwrite/Auth/OAuth2/Authentik.php (1)
45-45: LGTM: URL standardization improves API compatibility.The addition of the trailing slash to the authorization endpoint URL is a good practice that ensures compatibility with Authentik's OAuth2 implementation, as some providers require specific URL formatting.
Note: There's an inconsistency between the PR objectives and the visible changes. The PR objectives mention adding missing
codeandrefresh_tokenparameters to thegetTokens()andrefreshTokens()methods, but these parameters are already present in the code (lines 68 and 94 respectively). Only the trailing slash addition is marked as a change.Likely an incorrect or invalid review comment.
tests/unit/Auth/OAuth2/AuthentikTest.php (3)
10-21: LGTM: Well-structured test setup.The test setup correctly initializes the required configuration with proper JSON structure for the
appSecretthat includes bothclientSecretandauthentikDomainas expected by the Authentik OAuth2 provider.
69-103: LGTM: Proper validation of refresh token parameter inclusion.The test correctly verifies that the
refresh_tokenparameter is included in the refresh request. The mocking approach is consistent and the assertions validate the expected response structure.
108-131: LGTM: Critical edge case testing for token preservation.This test covers an important scenario where the OAuth provider doesn't return a new refresh token in the response. The verification that the original refresh token is preserved is crucial for maintaining authentication sessions and prevents token loss.
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
Description
This PR fixes a critical bug in the Authentik OAuth2 provider where required parameters were missing in token requests.
Fixes: #9567
Issues Fixed
getTokens()method was missing thecodeparameter in the HTTP request bodyrefreshTokens()method was missing therefresh_tokenparameter in the HTTP request bodyChanges Made
codeparameter to the token request ingetTokens()refresh_tokenparameter to the token request inrefreshTokens()Test Coverage
The new test cases verify:
codeparameter is correctly included in token requestsrefresh_tokenparameter is correctly included in refresh requestsChecklist
Summary by CodeRabbit
Bug Fixes
Tests