DEVXPT-35: Device code flow implementation - #2049
bansodejoyce wants to merge 13 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2049 +/- ##
============================================
+ Coverage 92.76% 92.97% +0.20%
- Complexity 2032 2123 +91
============================================
Files 126 128 +2
Lines 7337 7598 +261
============================================
+ Hits 6806 7064 +258
- Misses 531 534 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Multiple moderate issues remain in authentication precedence, request timeouts, error handling, and test isolation.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 5
Open (5)
What changed in this PR
Adds device-code OAuth authentication, token persistence and refresh, connector integration, logout handling, telemetry updates, and test coverage.
Changes:
- Implements device-code login with polling and legacy fallback.
- Adds silent device-token refresh and credential support.
- Updates service wiring, configuration, telemetry, and tests.
| File | Description |
|---|---|
tests/phpunit/src/Commands/Auth/AuthLoginCommandTest.php |
Device-flow and routing tests |
tests/phpunit/src/CloudApi/DeviceTokenRefresherTest.php |
Refresh behavior tests |
tests/phpunit/src/CloudApi/AccessTokenConnectorTest.php |
User-agent expectation updates |
src/Helpers/TelemetryHelper.php |
Environment provider detection |
src/Config/CloudDataConfig.php |
Device-token configuration schema |
src/Command/Auth/AuthLogoutCommand.php |
Device-session removal |
src/Command/Auth/AuthLoginCommand.php |
Device-code authentication and fallback |
src/CloudApi/DeviceTokenRefresher.php |
Silent token refresh |
src/CloudApi/ConnectorFactory.php |
Device-token connector selection |
src/CloudApi/CloudCredentials.php |
Device-token credential access |
src/CloudApi/ClientService.php |
Authentication detection and user-agent |
src/CloudApi/AccessTokenConnector.php |
Refreshed token request support |
src/ApiCredentialsInterface.php |
Credential interface extension |
src/AcsfApi/AcsfCredentials.php |
ACSF compatibility implementation |
config/prod/services.yml |
Dependency injection wiring |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (!empty($this->config['deviceAccessToken'])) { | ||
| return new AccessTokenConnector([ | ||
| 'access_token' => $this->config['deviceAccessToken'], | ||
| 'key' => null, | ||
| 'secret' => null, | ||
| ], $this->baseUri, $this->accountsUri, $this->deviceTokenRefresher); |
| } catch (ClientException $e) { | ||
| $output->writeln('<error>Failed to initiate device code flow: ' . $e->getMessage() . '</error>'); | ||
| return Command::FAILURE; | ||
| } |
| case '': | ||
| // Success — store token and exit. | ||
| $this->storeDeviceToken($token, $clientId); | ||
| $output->writeln(''); | ||
| $output->writeln('<info>✓ Authenticated successfully.</info>'); | ||
| return Command::SUCCESS; |
| use Prophecy\Argument; | ||
| use Prophecy\Prophecy\ObjectProphecy; | ||
|
|
||
| class DeviceTokenRefresherTest extends TestBase |
| /** | ||
| * @property AuthLoginCommand $command | ||
| */ | ||
| class AuthLoginCommandTest extends CommandTestBase |
There was a problem hiding this comment.
Reviewed at 71c8813 against the specifications for this feature.
CI is red on this head: testConnectorConfig fails on GitHub runners (User-Agent comes out acli/UNKNOWN (agent:github)), mutation testing is at 54% covered MSI, and the PR has no label. The first two are consequences of findings below.
Two of these will break production use, not just tests: the refresher can't reach Okta once the constants are compiled in (1), and the injected Guzzle client has no timeout (2). Details inline.
DeviceTokenRefresherreads Okta config from env vars only;AuthLoginCommanduses compiled constants. With real constants and no env vars, login works and every refresh fails.GuzzleHttp\Clientis a registered service, so autowiring injects it and thetimeoutdefaults in both constructors never apply.- User-Agent labels every existing provider (Acquia hosting, CI, DDEV) as
agent:and doesn't match the format product-specs#208 fixes. - Initiation only catches
ClientException; transport errors and 5xx escape as uncaught exceptions. - A 2xx token response without
access_tokenis stored and reported as success. - Key-holders now get a confirm gate before the chooser, and the key path never offers device code, contrary to what the #6 thread says this PR does.
- A lost refresh-token rotation race surfaces as "session expired" on every request.
auth:logoutchanged with no test changes.- Env-mutating test classes aren't in the
serialgroup. - Dead
deviceAccessTokenExpirywiring.
Not raising: Copilot's ConnectorFactory precedence comment (device token before ACLI_ACCESS_TOKEN). The order matches the spec's priority list (3 before 4). Decline it.
Spec follow-ups this PR exposes (for #6, not for this code):
- Empty constants + no env vars falls through to legacy auth silently. The spec scenario says error out and make no Okta request. The code's behavior is the right one for a phar shipped before the #850 values land, so the spec should change to match.
- Missing coverage against tasks.md acceptance 2.14: no tests for
slow_down, transport backoff in the poll loop,--no-interactionrouting, orConnectorFactorydevice-token routing (expired access token + refresh token builds the connector; key/secret precedence). The 54% MSI is the symptom. Add them here.
Reviewed with AI assistance (Claude); findings verified by the reviewer.
| // Access token expired — attempt a silent refresh. | ||
| $refreshToken = $stored['refresh_token'] ?? null; | ||
| $clientId = $stored['client_id'] ?? null; | ||
| $domain = getenv('ACLI_OKTA_DOMAIN'); |
There was a problem hiding this comment.
This reads ACLI_OKTA_DOMAIN and ACLI_OKTA_AUTH_SERVER_ID from the environment only. AuthLoginCommand::executeDeviceCodeFlow() resolves the same values as getenv(...) ?: self::CONSTANT. Once the real constants are compiled in and a user logs in without env vars set, login succeeds, $domain/$authServer are false here, this returns null, and after five minutes every command throws "Device token session expired".
Persist okta_domain and okta_auth_server_id into device_token at login next to client_id and read them here (add them to CloudDataConfig), or move the three-way resolution into one shared class both callers use. Add a test that refreshes with the env vars unset.
| public function __construct( | ||
| private CloudDataStore $datastore, | ||
| private LoggerInterface $logger, | ||
| private GuzzleClient $httpClient = new GuzzleClient(['timeout' => 15]), |
There was a problem hiding this comment.
GuzzleHttp\Client: ~ is a registered service in services.yml with autowire: true, so the container injects it here and in AuthLoginCommand (line 48). The default new GuzzleClient(['timeout' => 15]) only runs when the constructor is called by hand, i.e. in tests. In production both clients have no timeout: a hung /token blocks the poll loop, and a hung refresh blocks every API request.
Set the timeout explicitly: pass 'timeout' as a request option on each post() call, or define a dedicated Guzzle service with the timeout in services.yml and bind it. The #6 design decision ("GuzzleClient injected via PHP 8.1 default parameter expressions") rests on the same assumption, so the spec needs the same fix.
| ] | ||
| ); | ||
| $new = json_decode((string) $response->getBody(), true); | ||
| } catch (ClientException $e) { |
There was a problem hiding this comment.
With refresh_token_rotation = "ROTATE" (confirmed in idm-identity-service#850), two ACLI processes refreshing in the same window race. The loser gets HTTP 400 invalid_grant, this returns null, and AccessTokenConnector::createRequest() throws "session expired" while the session is fine and the winner has already written a fresh token to cloud_api.conf.
On 400, re-read device_token from the datastore before giving up: if the stored access token differs from the one this call started with and is not within the 60-second window, return it. That closes the common case without file locking. Log invalid_grant distinctly from other 400s.
| $userAgent = sprintf('acli/%s', $this->application->getVersion()); | ||
| $provider = TelemetryHelper::getEnvironmentProvider(); | ||
| if ($provider !== null) { | ||
| $userAgent .= sprintf(' (agent:%s)', $provider); |
There was a problem hiding this comment.
Two problems.
getEnvironmentProvider() returns every provider in getProviders(), so this labels Acquia hosting, GitHub Actions, CircleCI, DDEV and the rest as (agent:...). CI proves it: testConnectorConfig fails on GitHub runners with acli/UNKNOWN (agent:github).
product-specs#208 technical-design.md fixes the header as Acquia CLI (<version>, <telemetry ID>, <provider>), with the provider as the raw getProviders() key so API requests and telemetry events carry the same value. Emit that format, with the provider omitted when none is detected. Then fix the tests: AccessTokenConnectorTest.php:242 encodes (agent:acquia), and both User-Agent assertions need the provider env vars controlled inside the test so they pass regardless of the runner.
| 'scope' => 'openid profile email offline_access', | ||
| ], | ||
| ]); | ||
| } catch (ClientException $e) { |
There was a problem hiding this comment.
Only ClientException (4xx) is caught. DNS failure, connection timeout, and 5xx propagate as uncaught ConnectException/ServerException, so the command dies without the failure message and without reaching executeDeviceCodeFlowWithFallback(). The spec scenario at acli-auth/spec.md:13–18 requires transport failures to be caught. Catch GuzzleException here, as the poll loop already does.
| $error = $token['error'] ?? ''; | ||
|
|
||
| switch ($error) { | ||
| case '': |
There was a problem hiding this comment.
A 2xx response whose body has no error and no access_token lands here. storeDeviceToken() reads $token['access_token'] (undefined index) and writes access_token => null, then the command prints "Authenticated successfully". Same path if the body isn't JSON: $token is null, $token['error'] ?? '' is ''. Require a non-empty access_token before storing; otherwise print the unexpected-response error and return FAILURE.
| $deviceToken = $this->datastoreCloud->get('device_token'); | ||
|
|
||
| // Smart routing: existing API key → legacy, device token → device code re-auth. | ||
| if ($activeKey && $keys) { |
There was a problem hiding this comment.
Two things about the key-holder path.
The confirm gate is new. Until this PR, acli auth:login with saved keys went straight to the chooser, and the help text described switching accounts as the command's purpose. Now every switch costs an extra prompt (testAuthLoginInteractiveSelectsExistingEnvironmentKey had to grow a 'yes'). Drop the confirm and call executeLegacyAuth() directly; that method already prints the active key.
This path never reaches device code. The reply on acquia/acquia-cli#6 (spec.md:85 thread) says the PR gives key-holders "the legacy chooser with device code as a re-auth option". It doesn't. Either add a device-code entry to the chooser here, or correct the thread so the spec gets written to what the code does.
| throw new AcquiaCliException('There is no active Cloud Platform API key'); | ||
| $deviceToken = $this->datastoreCloud->get('device_token'); | ||
|
|
||
| if (!$activeKey && !$deviceToken) { |
There was a problem hiding this comment.
The command's behavior changed (device-only logout, key+device logout, new no-credentials message) and AuthLogoutCommandTest wasn't touched. Add the three cases.
| use Prophecy\Argument; | ||
| use Prophecy\Prophecy\ObjectProphecy; | ||
|
|
||
| class DeviceTokenRefresherTest extends TestBase |
There was a problem hiding this comment.
This class and AuthLoginCommandTest mutate process env (putenv) in setUp() and in tests. CI runs paratest --exclude-group serial for everything not marked serial, and the repo's convention for env-mutating tests is #[Group('serial')] (TelemetryHelperTest, ChecklistTest, and others). Add the attribute to both classes.
| accessToken: '@=service("cloud.credentials").getCloudAccessToken()' | ||
| accessTokenExpiry: '@=service("cloud.credentials").getCloudAccessTokenExpiry()' | ||
| deviceAccessToken: '@=service("cloud.credentials").getCloudDeviceAccessToken()' | ||
| deviceAccessTokenExpiry: '@=service("cloud.credentials").getCloudDeviceTokenExpiry()' |
There was a problem hiding this comment.
deviceAccessTokenExpiry is wired into both connector factories (here and line 122) and ConnectorFactory never reads it, correctly, since the connector is built on refresh-token presence and expiry is evaluated per request. Remove both lines and CloudCredentials::getCloudDeviceTokenExpiry() (nothing else calls it).
|
Try the dev build for this PR: https://acquia-cli.s3.amazonaws.com/build/pr/2049/acli.phar |
ndelrossi
left a comment
ndelrossi
left a comment
There was a problem hiding this comment.
Re-reviewed at 678be26. Most of the previous round is resolved, and the poll loop follows RFC 8628. Major issues remaining (details inline):
- Refresh race mitigation is dead code in production, and the race revokes the whole session.
CloudDataStoreloadscloud_api.confonce per process, so the "re-read" afterinvalid_grantnever sees another process's write. Okta reuse detection also means that by the timeinvalid_grantcomes back, the winner's tokens are already revoked. Fix: prevent the race rather than recover from it (lock plus reload from disk before refreshing). --environmentis ignored by the device code flow.auth:login --environment stagingwith no active key authenticates against the bundled Okta org, stores adevice_tokenwith no base URI, and every later command goes to the prod API.- Machine-readable output (spec G9, acceptance 2.16/2.17) is not implemented. There is no structured mode, no pending state during the wait, and no stable error values. Implement it here, or get #6 to explicitly defer it.
- CI is red. Mutation Testing is at 81% covered MSI (100% required),
require_labelis failing, and codecov/patch is failing.
| */ | ||
| private function tokenWrittenByAnotherProcess(array $stored): ?string | ||
| { | ||
| $current = $this->datastore->get('device_token'); |
There was a problem hiding this comment.
This re-read never sees another process's write. CloudDataStore (via JsonDataStore) parses cloud_api.conf once at construction, and Datastore::get() returns the in-memory Data. $current is therefore always identical to $stored, this returns null, and the lost-race path always ends in "session expired". testUsesTheWinnersTokenWhenAConcurrentInvocationRotatedFirst only passes because the prophecy returns two different values in sequence, which the real store never does.
Re-reading from disk wouldn't fix it either. Okta's refresh token reuse detection fires when a rotated refresh token is presented after the grace period (refresh_token_leeway = 30 in idm-identity-service#850). It then "invalidates the most recently issued refresh token and all access tokens issued since the user authenticated" (Okta docs). So when this invalid_grant arrives, the winner's token is already dead.
Realistic trigger: a command running longer than about 4 minutes (pull, env:create, notification polling) while any other acli call refreshes. That other call could be a second terminal or an agent running commands in parallel. The long-running process later refreshes with its stale in-memory refresh token and logs out every process.
This needs prevention, not recovery:
- take an exclusive
flockon a sidecar lock file around read → refresh → write; - reload
device_tokenfrom disk inside the lock, and only refresh if the reloaded token is still inside the 60s window.
There was a problem hiding this comment.
You were right, and the detail made it obvious rather than arguable. Reverted and replaced with prevention
| return null; | ||
| } | ||
|
|
||
| $this->datastore->set('device_token', [ |
There was a problem hiding this comment.
Related to the above: set() dumps the whole in-memory datastore, which was loaded when this process started. A refresh in a long-running command can therefore undo a concurrent auth:logout by bringing device_token back. It can also overwrite keys or a device_token written by another invocation since then. The write should happen inside the same lock, against freshly loaded data, touching only device_token.
| { | ||
| $env = $input->getOption('environment'); | ||
| // Explicit legacy flag or key/secret passed on the command line — always legacy. | ||
| if ($input->getOption('use-legacy-auth') || $input->getOption('key') || $input->getOption('secret')) { |
There was a problem hiding this comment.
--environment is ignored on the device code path. It is only read in executeLegacyAuth(). acli auth:login --environment staging with no active key falls through to executeDeviceCodeFlow(), which signs in against the single bundled Okta org and stores a device_token with no base URI. CloudCredentials::getBaseUri() then falls back to prod for every later command, so a user who thinks they're on staging is operating on prod.
With the other environments' Terraform landing soon, this needs a decision (and a spec update in acquia-cli#6). Either route or fail when --environment isn't prod, or map each environment to its own Okta config and API base URI and store it with the token.
|
|
||
| // Step 2 — surface the code and URL for the human. | ||
| $output->writeln(''); | ||
| $output->writeln('Sign in to Acquia ID in your browser:'); |
There was a problem hiding this comment.
Machine-readable output isn't implemented. The spec requirement "auth:login emits machine-readable output for agent-driven sign-in" and acceptance 2.16/2.17 call for three things:
- an opt-in mode that writes the verification challenge (
verification_uri,user_code, expiry) to stdout before polling; - a pending state while waiting;
- expiry, denial and initiation failure as distinguishable structured errors.
Today it's human-readable writeln only, and failures are plain <error> strings. Agent-driven sign-in is the stated motivation for this work, so either implement it here or get acquia-cli#6 to explicitly defer G9 to a follow-up.
This pull request introduces support for device-based OAuth authentication in the Cloud API client, enabling the use and automatic refresh of device access tokens. It adds a new
DeviceTokenRefresherservice, updates credential and connector classes to handle device tokens, and ensures the client can authenticate using device tokens as a fallback.Device token authentication and refresh support:
DeviceTokenRefresherclass to manage device access token retrieval and silent refresh, including handling of token expiry and refresh failures.