Claude/device flow auth a lt do#22
Conversation
- Add AuthenticationStrategy interface for injectable auth methods - Implement TokenAuthentication for personal access tokens - Implement GitHubAppAuthentication for GitHub App JWT authentication - Implement GitHubOAuth for OAuth access tokens - Update Connector to accept AuthenticationStrategy or string (backward compatible) - Add comprehensive tests for all authentication strategies Closes #10
Implement RFC 8628 Device Authorization Grant for CLI authentication: - Add DeviceFlowAuthentication strategy with full polling support - Add DeviceFlowCallback interface for handling flow events - Add DeviceFlowHttpClient interface for testable HTTP operations - Add DefaultDeviceFlowHttpClient as default implementation - Add DeviceFlowException for device flow errors - Handle authorization_pending and slow_down responses - Comprehensive tests with mock HTTP client Closes #20
|
Warning Rate limit exceeded@jordanpartridge has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 55 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/Unit/Auth/DefaultDeviceFlowHttpClientTest.php (1)
6-10: Consider adding tests for method behavior.While the interface implementation check is valid, consider adding tests that verify:
- The
post()method correctly builds and sends HTTP requests- Error handling when network requests fail
- The
sleep()method delegates to PHP's sleep functionThese tests would increase confidence in the implementation, though they may require mocking or integration test setup.
src/Auth/DefaultDeviceFlowHttpClient.php (1)
42-42: Consider validating JSON decode success.If the response is not valid JSON,
json_decodereturnsnull, which becomes an empty array via the null coalescing operator. This could mask invalid responses (e.g., HTML error pages). Consider checkingjson_last_error()to distinguish between valid empty responses and parsing failures.🔎 Suggested improvement
- /** @var array<string, mixed> */ - return json_decode($response, true) ?? []; + $decoded = json_decode($response, true); + + if (json_last_error() !== JSON_ERROR_NONE) { + throw new DeviceFlowException('invalid_response', 'Failed to parse JSON response from GitHub'); + } + + /** @var array<string, mixed> */ + return $decoded ?? [];tests/Unit/Auth/DeviceFlowAuthenticationTest.php (1)
277-347: Consider adding test for device code expiration scenario.The test suite comprehensively covers most scenarios but doesn't explicitly test the device code expiration path (when the polling loop exceeds
expires_inwithout getting a token). While the implementation handles this withDeviceFlowException::expired(), a test would increase confidence.🔎 Suggested test case
it('throws expired exception when polling exceeds expiration time', function () { $callback = new TestDeviceFlowCallback; $httpClient = new MockDeviceFlowHttpClient; $httpClient->addResponse([ 'device_code' => 'abc123', 'user_code' => 'ABCD-1234', 'verification_uri' => 'https://github.com/login/device', 'expires_in' => 0, // Immediate expiration 'interval' => 5, ]); $auth = new DeviceFlowAuthentication('client_id', $callback, null, $httpClient); try { $auth->authorize(); } catch (DeviceFlowException $e) { expect($e->getError())->toBe('expired_token') ->and($callback->error)->toBe('expired_token'); throw $e; } })->throws(DeviceFlowException::class, 'The device code has expired');src/Auth/DeviceFlowAuthentication.php (1)
107-131: Consider validating required response fields.The method assumes the response contains
verification_uri,user_code, andexpires_in. While GitHub's API is stable, defensive validation would improve robustness against API changes or network issues.🔎 Suggested validation
private function requestDeviceCode(): array { $params = ['client_id' => $this->clientId]; if ($this->scope !== null) { $params['scope'] = $this->scope; } $response = $this->httpClient->post(self::DEVICE_CODE_URL, $params); if (isset($response['error'])) { throw new DeviceFlowException( $response['error'], $response['error_description'] ?? 'Failed to request device code' ); } + + $requiredFields = ['verification_uri', 'user_code', 'expires_in', 'device_code', 'interval']; + foreach ($requiredFields as $field) { + if (!isset($response[$field])) { + throw new DeviceFlowException('invalid_response', "Missing required field: {$field}"); + } + } $this->callback->onCodeReady( $response['verification_uri'], $response['user_code'], $response['expires_in'] ); return $response; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/Auth/DefaultDeviceFlowHttpClient.php(1 hunks)src/Auth/DeviceFlowAuthentication.php(1 hunks)src/Auth/DeviceFlowCallback.php(1 hunks)src/Auth/DeviceFlowHttpClient.php(1 hunks)src/Exceptions/DeviceFlowException.php(1 hunks)tests/Unit/Auth/DefaultDeviceFlowHttpClientTest.php(1 hunks)tests/Unit/Auth/DeviceFlowAuthenticationTest.php(1 hunks)tests/Unit/Exceptions/DeviceFlowExceptionTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
src/Auth/DeviceFlowHttpClient.php (3)
src/Exceptions/DeviceFlowException.php (1)
DeviceFlowException(12-58)src/Auth/DefaultDeviceFlowHttpClient.php (2)
post(25-43)sleep(48-51)tests/Unit/Auth/DeviceFlowAuthenticationTest.php (2)
post(82-89)sleep(91-95)
src/Auth/DeviceFlowCallback.php (1)
tests/Unit/Auth/DeviceFlowAuthenticationTest.php (4)
onCodeReady(32-37)onPolling(39-42)onSuccess(44-49)onError(51-55)
tests/Unit/Exceptions/DeviceFlowExceptionTest.php (1)
src/Exceptions/DeviceFlowException.php (5)
DeviceFlowException(12-58)getError(30-33)getErrorDescription(38-41)expired(46-49)accessDenied(54-57)
src/Auth/DefaultDeviceFlowHttpClient.php (3)
src/Exceptions/DeviceFlowException.php (1)
DeviceFlowException(12-58)src/Auth/DeviceFlowHttpClient.php (2)
post(25-25)sleep(32-32)tests/Unit/Auth/DeviceFlowAuthenticationTest.php (2)
post(82-89)sleep(91-95)
tests/Unit/Auth/DeviceFlowAuthenticationTest.php (5)
src/Auth/DeviceFlowAuthentication.php (4)
DeviceFlowAuthentication(34-190)isAuthorized(78-81)getAuthenticator(88-98)authorize(69-73)src/Exceptions/DeviceFlowException.php (2)
DeviceFlowException(12-58)getError(30-33)src/Auth/DeviceFlowCallback.php (4)
onCodeReady(26-26)onPolling(33-33)onSuccess(42-42)onError(50-50)src/Auth/DefaultDeviceFlowHttpClient.php (2)
post(25-43)sleep(48-51)src/Auth/DeviceFlowHttpClient.php (2)
post(25-25)sleep(32-32)
src/Auth/DeviceFlowAuthentication.php (4)
src/Exceptions/DeviceFlowException.php (3)
DeviceFlowException(12-58)__construct(20-25)expired(46-49)src/Auth/DefaultDeviceFlowHttpClient.php (3)
DefaultDeviceFlowHttpClient(14-52)post(25-43)sleep(48-51)src/Auth/DeviceFlowHttpClient.php (2)
post(25-25)sleep(32-32)src/Auth/DeviceFlowCallback.php (4)
onCodeReady(26-26)onPolling(33-33)onSuccess(42-42)onError(50-50)
tests/Unit/Auth/DefaultDeviceFlowHttpClientTest.php (1)
src/Auth/DefaultDeviceFlowHttpClient.php (1)
DefaultDeviceFlowHttpClient(14-52)
🪛 PHPMD (2.15.0)
tests/Unit/Auth/DeviceFlowAuthenticationTest.php
82-82: Avoid unused parameters such as '$url'. (undefined)
(UnusedFormalParameter)
82-82: Avoid unused parameters such as '$params'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (12)
tests/Unit/Exceptions/DeviceFlowExceptionTest.php (1)
1-36: LGTM! Comprehensive test coverage for DeviceFlowException.The test suite thoroughly validates all aspects of the
DeviceFlowExceptionclass, including constructor behavior, accessors, and factory methods. The tests are clear and well-structured.src/Auth/DeviceFlowHttpClient.php (1)
14-33: LGTM! Well-designed interface with clear separation of concerns.The interface provides a clean abstraction for HTTP operations with proper type annotations and documentation. Separating
sleep()as a distinct method is a smart design choice that enables tests to skip actual delays.src/Auth/DefaultDeviceFlowHttpClient.php (1)
48-51: LGTM! Simple and effective delegation.The
sleep()method correctly delegates to PHP's built-insleep()function, enabling test doubles to override this behavior.src/Auth/DeviceFlowCallback.php (1)
15-51: LGTM! Well-designed callback interface.The interface provides comprehensive coverage of device flow lifecycle events with clear documentation. The nullable
$scopeparameter inonSuccess()appropriately handles cases where scope information may not be present.src/Exceptions/DeviceFlowException.php (1)
12-58: LGTM! Clean exception class with useful factory methods.The exception class is well-designed with:
- Clear separation between error code and description
- Appropriate use of readonly properties
- Convenient factory methods for common error cases
tests/Unit/Auth/DeviceFlowAuthenticationTest.php (5)
12-96: LGTM! Well-designed test helpers.The
TestDeviceFlowCallbackandMockDeviceFlowHttpClientclasses provide clean test doubles that enable comprehensive testing of the device flow. The static analysis warning about unused parameters inMockDeviceFlowHttpClient.post()is a false positive—mocks naturally ignore parameters when responses are pre-programmed.
98-165: LGTM! Thorough coverage of instantiation and happy path.The tests comprehensively validate instantiation patterns and the successful authorization flow, including proper callback state updates and final authorization status.
167-215: LGTM! Good coverage of authenticator retrieval and error cases.The tests properly validate:
- Authenticator type after successful authorization
- Exception handling for device code request failures
- Fallback behavior when error description is missing
217-275: LGTM! Excellent polling behavior coverage.The tests validate critical polling scenarios:
- Continuation on
authorization_pending- Proper interval adjustment on
slow_down- Sleep call tracking
277-347: LGTM! Comprehensive error handling coverage.The tests thoroughly validate:
- Access denied error propagation to callback
- Default token type handling when not provided
- Unknown error handling with fallback messages
src/Auth/DeviceFlowAuthentication.php (2)
140-189: LGTM! Correct implementation of RFC 8628 polling logic.The polling implementation properly handles:
- Time-based expiration checking
- Interval-based delays with
slow_downadjustment (+5 seconds per RFC 8628)- Proper state transitions (pending, success, error, expired)
- Callback notifications at appropriate lifecycle stages
The logic correctly treats responses without
access_tokenorerrorfields as pending, which provides resilience against unexpected API responses.
34-98: LGTM! Clean architecture with proper dependency injection.The class demonstrates good design principles:
- Dependency injection with sensible defaults
- Clear state management via
accessToken- Helpful error messages guiding users to call methods in the correct order
- Proper use of Saloon's
TokenAuthenticatorfor the final authenticator
| public function post(string $url, array $params): array | ||
| { | ||
| $context = stream_context_create([ | ||
| 'http' => [ | ||
| 'method' => 'POST', | ||
| 'header' => "Accept: application/json\r\nContent-Type: application/x-www-form-urlencoded\r\n", | ||
| 'content' => http_build_query($params), | ||
| ], | ||
| ]); | ||
|
|
||
| $response = @file_get_contents($url, false, $context); | ||
|
|
||
| if ($response === false) { | ||
| throw new DeviceFlowException('network_error', 'Failed to connect to GitHub'); | ||
| } | ||
|
|
||
| /** @var array<string, mixed> */ | ||
| return json_decode($response, true) ?? []; | ||
| } |
There was a problem hiding this comment.
Add timeout configuration to prevent indefinite hangs.
The HTTP request doesn't configure a timeout, which could cause the device flow to hang indefinitely if GitHub is unresponsive. Consider adding a timeout to the stream context.
🔎 Proposed fix
public function post(string $url, array $params): array
{
$context = stream_context_create([
'http' => [
'method' => 'POST',
'header' => "Accept: application/json\r\nContent-Type: application/x-www-form-urlencoded\r\n",
'content' => http_build_query($params),
+ 'timeout' => 30,
],
]);
$response = @file_get_contents($url, false, $context);
if ($response === false) {
throw new DeviceFlowException('network_error', 'Failed to connect to GitHub');
}
/** @var array<string, mixed> */
return json_decode($response, true) ?? [];
}🤖 Prompt for AI Agents
In src/Auth/DefaultDeviceFlowHttpClient.php around lines 25 to 43, the POST
request uses file_get_contents without any timeout which can hang indefinitely;
add a timeout parameter to the stream context (e.g. 'timeout' => 10) so the HTTP
wrapper will abort after a reasonable number of seconds, include this option in
the stream_context_create call and keep existing headers/content, and ensure the
code still throws DeviceFlowException on failure.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/Unit/Auth/DefaultDeviceFlowHttpClientTest.php (1)
20-27: Consider a more meaningful assertion for the sleep test.The assertion
expect(true)->toBeTrue()is a no-op that only proves the test ran. While coverage is the goal here, you could verify the method is callable by checking no exception was thrown more explicitly, or simply remove the dummy assertion since Pest will pass if no exception is thrown.🔎 Suggested simplification
it('sleep method executes without error', function () { $client = new DefaultDeviceFlowHttpClient; // Sleep for 0 seconds just to cover the method $client->sleep(0); - - expect(true)->toBeTrue(); + + // Test passes if no exception is thrown });tests/Unit/Auth/DeviceFlowAuthenticationTest.php (2)
82-89: Consider prefixing unused parameters with underscore to suppress static analysis warnings.The static analysis tool flags
$urland$paramsas unused. While this is intentional for a mock implementation, you can prefix them with underscores to signal they are deliberately ignored.🔎 Suggested fix
- public function post(string $url, array $params): array + public function post(string $_url, array $_params): array { if (! isset($this->responses[$this->callIndex])) { return []; } return $this->responses[$this->callIndex++]; }
112-136: Consider verifying the scope was actually passed in the request.The test comment says "includes scope in device code request when provided" but only verifies the response scope matches. Since
MockDeviceFlowHttpClient.post()ignores the$params, the test doesn't actually verify the scope was sent. Consider enhancing the mock to capture and expose the parameters for assertion.🔎 Suggested enhancement to MockDeviceFlowHttpClient
Add parameter capture to the mock:
/** @var array<array<string, mixed>> */ public array $capturedParams = []; public function post(string $url, array $params): array { $this->capturedParams[] = ['url' => $url, 'params' => $params]; if (! isset($this->responses[$this->callIndex])) { return []; } return $this->responses[$this->callIndex++]; }Then in the test, add:
expect($httpClient->capturedParams[0]['params'])->toHaveKey('scope', 'repo user');
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/Unit/Auth/DefaultDeviceFlowHttpClientTest.php(1 hunks)tests/Unit/Auth/DeviceFlowAuthenticationTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Unit/Auth/DeviceFlowAuthenticationTest.php (5)
src/Auth/DeviceFlowAuthentication.php (4)
DeviceFlowAuthentication(34-190)authorize(69-73)isAuthorized(78-81)getAuthenticator(88-98)src/Exceptions/DeviceFlowException.php (2)
DeviceFlowException(12-58)getError(30-33)src/Auth/DeviceFlowCallback.php (4)
onCodeReady(26-26)onPolling(33-33)onSuccess(42-42)onError(50-50)src/Auth/DeviceFlowHttpClient.php (2)
post(25-25)sleep(32-32)src/Auth/DefaultDeviceFlowHttpClient.php (2)
post(25-43)sleep(48-51)
🪛 PHPMD (2.15.0)
tests/Unit/Auth/DeviceFlowAuthenticationTest.php
82-82: Avoid unused parameters such as '$url'. (undefined)
(UnusedFormalParameter)
82-82: Avoid unused parameters such as '$params'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (13)
tests/Unit/Auth/DefaultDeviceFlowHttpClientTest.php (2)
7-11: LGTM!Clean interface conformance test that validates the class implements the expected contract.
13-18: LGTM!Good test for network error handling. Using an invalid URL with a high port number is a reliable way to trigger connection failures.
tests/Unit/Auth/DeviceFlowAuthenticationTest.php (11)
12-56: LGTM!Well-structured test callback implementation that captures all state for verification. All interface methods are properly implemented with appropriate property types.
98-158: LGTM!Good coverage of instantiation scenarios and pre-authorization state checks. The exception test for
getAuthenticator()before authorization properly validates the guard clause.
160-191: LGTM!Comprehensive test that validates the entire successful authorization flow, including all callback invocations and state updates.
193-216: LGTM!Good integration verification with Saloon's
TokenAuthenticator.
218-241: LGTM!Good coverage of error responses during device code request, including the edge case where
error_descriptionis missing.
243-271: LGTM!Good test for polling continuation when authorization is pending. Properly verifies the polling count increments.
273-301: LGTM!Good verification of the
slow_downerror handling, confirming the interval increases from 5 to 10 seconds.
303-330: LGTM!Good pattern using try-catch to verify callback state before re-throwing the exception. This ensures both the exception and callback are properly populated.
332-353: LGTM!Good edge case coverage for when
token_typeandscopeare not provided in the response.
355-373: LGTM!Covers the edge case of unknown errors without descriptions during polling.
375-404: Expiration test may be timing-sensitive.This test relies on the
expires_in=1andinterval=0to trigger expiration. While this should work in most cases, if the test environment is slow, the polling loop might exit before consuming all mock responses. Consider this a minor observation; the test is acceptable for unit testing purposes.
2 participants
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.