fix(oauth2): port Node SDK PR #11 — refresh_token, forceRefresh, 120s buffer, refreshEndpoint, re-entrance guard#94
Merged
Conversation
… buffer, refreshEndpoint, re-entrance guard
FIX 1 — 401 retry middleware calls forceRefresh() instead of exchangeCodeForToken()
AbstractApi::sendAuthenticatedRequest() intercepts 401, calls
OAuthProvider::forceRefresh() and retries once (RFC 6750 §3.1).
FIX 2 — refreshAccessToken() sends Authorization: Basic header
Required for confidential clients per RFC 6749 §3.2.1.
FIX 3 — Proactive refresh buffer raised from 60 s to 120 s
Mitigates clock skew between client and auth server.
FIX 4 — Configurable refreshEndpoint (distinct from tokenEndpoint)
OAuthProvider accepts an optional refreshEndpoint parameter (default =
tokenEndpoint). UpsunClient passes auth_url + refresh_endpoint.
FIX 5 — Re-entrance guard (thundering-herd protection)
acquiringToken bool prevents recursive acquisition in PHP Fiber
contexts. Multi-process FPM protection requires external lock (out of scope).
Also adds:
- doAcquireToken(): prefers refresh_token grant, falls back to api_token
- forceRefresh(): clears access token without touching refreshToken or acquiringToken
- storeTokenData() now persists refresh_token and token_type from response
- 9 new unit tests covering all 5 fixes (567 tests, 3395 assertions — all green)
- Minor CI workflow label fixes and composer clean script
Collaborator
🧾 API Coverage ReportLast updated: 3e5bb53 • Run #235
📋 Full JSON report |
Upsun SDK checker reportDisplay raw output |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
… remove backward-compat alias
Upsun SDK checker reportDisplay raw output |
Upsun SDK checker reportDisplay raw output |
Upsun SDK checker reportDisplay raw output |
…case tests, expand coverage to AbstractApi
Upsun SDK checker reportDisplay raw output |
doAcquireToken() already guards $this->refreshToken before calling refreshAccessToken(), making the inner check dead code (never reachable). Removing it achieves 100% line + method coverage on OAuthProvider. Also syncs template (oauth_provider.mustache) and commits phpunit.xml coverage-scope update (added src/Api/AbstractApi.php).
Upsun SDK checker reportDisplay raw output |
…double-retry Change 1 — Bearer-only mode + \Closure tokenProvider in AbstractApi - AbstractApi: replace OAuthProvider $oauthProvider with \Closure $tokenProvider - getAuthorizationHeader() → ($this->tokenProvider)() - refreshToken() → ($this->tokenProvider)() - All ~62 concrete API files: sed OAuthProvider → \Closure tokenProvider - api.mustache + abstract_api.mustache: updated to match - UpsunClient: $auth nullable (?OAuthProvider), only created when apiToken !== '' - new setBearerToken(string $token): void - getToken() three-way: auth → bearerToken → throw RuntimeException - $tokenProvider closure passed to all concrete APIs instead of $this->auth Change 2 — Fiber-based thundering-herd guard in OAuthProvider::ensureValidToken() - Replace bool $acquiringToken with ?\Fiber $acquiringFiber - Concurrent Fiber callers suspend (via \Fiber::getCurrent()?->suspend()) until the first acquisition completes, then return without a second HTTP call - In sync (FPM) contexts \Fiber::getCurrent() is null, so acquiringFiber is always null and the guard is a no-op — identical behaviour to the old bool flag Change 3 — $_retried guard in sendAuthenticatedRequest() prevents double forceRefresh - Add bool $_retried = false param; on 401 call tokenProvider(true) then recurse with $_retried=true so a second 401 goes straight to ApiException without a redundant force-refresh call Tests: - AbstractApiTest: rewritten to use closure tracking (tokenCallCount/forceRefreshCount) instead of OAuthProvider mock; 401-retry $_retried guard verified - UpsunClientTest: removed testGetTokenReturnsApiToken (now HTTP-backed); added testGetTokenDelegatesToOAuthProvider, testGetTokenReturnsBearerToken, testGetTokenThrowsWhenNoAuthMethodSet - OAuthProviderTest: replaced reflection-based acquiringToken test with proper Fiber concurrency test (testFiberGuardPreventsDoubleAcquisitionUnderConcurrency) - BaseTestCase + all ~25 TaskTests: OAuthProvider mock → \Closure tokenProvider Result: 584/584 tests pass, lint clean
Upsun SDK checker reportDisplay raw output |
Upsun SDK checker reportDisplay raw output |
…uisition Replaces bare Closure with a proper TokenProvider interface, aligning naming with the Node SDK's `type TokenProvider = (force?: boolean) => string`. - src/Core/TokenProvider.php: new interface with __invoke(bool $force=false): string - OAuthProvider: implements TokenProvider, adds __invoke() covering force-refresh path - AbstractApi: TokenProvider type hint replacing Closure - UpsunClient: $tokenProvider declared as named variable before $taskParams array (facade does not implement the interface — clean separation of concerns) - All 62 concrete API files + mustache templates updated - All tests updated: anonymous class implements TokenProvider (Closure != interface in PHP) - 25 task test files: multi-line anonymous class format (PHPCS compliant)
…d file
Alphabetical use-statement order (matching PHPCS rule) was broken after
introducing TokenProvider — `use {{invokerPackage}}\Core\TokenProvider;`
was sitting between JsonException and Http\ imports.
Now matches the order produced by phpcbf on AbstractApi.php.
Upsun SDK checker reportDisplay raw output |
…code fix:all (phpcbf + rector + php-cs-fixer) applied: - OAuthProvider: remove redundant `use Upsun\Core\TokenProvider;` (same namespace) - 25 task test files: FQNS `\Upsun\Core\TokenProvider` → `use` import + short name oauth_provider.mustache synced with OAuthProvider.php: - use Fiber; import (was \Fiber FQNS) - class OAuthProvider implements TokenProvider - short Fiber name throughout (not \Fiber) - __invoke(bool $force = false): string method added
Upsun SDK checker reportDisplay raw output |
1 similar comment
Upsun SDK checker reportDisplay raw output |
Upsun SDK checker reportDisplay raw output |
Contributor
There was a problem hiding this comment.
Pull request overview
Ports multiple OAuth2/token-lifecycle fixes from the Node SDK into the PHP SDK, introducing a TokenProvider abstraction so generated API clients can support OAuth2 refresh semantics, 401 retry, and bearer-only authentication.
Changes:
- Introduces
Core\TokenProviderand refactors generated API clients +AbstractApito depend on it (enabling force-refresh on 401 and bearer-only mode). - Enhances
OAuthProviderwith refresh-token grant support (Basic auth), configurable refresh endpoint, 120s proactive refresh buffer, and a Fiber re-entrance guard. - Adds/updates unit tests for the new token behaviors and updates CI workflows / coverage configuration.
Reviewed changes
Copilot reviewed 104 out of 105 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Api/AbstractApi.php | Switches auth dependency to TokenProvider and adds 401 retry logic (but currently has a constructor bug: undefined $plugins/$uriFactory). |
| templates/php/abstract_api.mustache | Template source for the AbstractApi changes (same constructor bug as generated output). |
| src/Core/OAuthProvider.php | Implements TokenProvider, adds refresh-token flow, 120s buffer, refresh endpoint, and Fiber guard (but currently calls Fiber suspend incorrectly). |
| templates/php/oauth_provider.mustache | Template source for OAuthProvider changes (same Fiber suspend issue). |
| src/Core/TokenProvider.php | New interface to supply Authorization header values and support force-refresh. |
| src/UpsunClient.php | Wires refreshEndpoint, supports bearer-only auth, and provides a TokenProvider adapter for generated APIs. |
| src/Api/AddOnsApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/AlertsApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/ApiConfiguration.php | Docstring capitalization update (ApiConfiguration). |
| src/Api/ApiTokensApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/ApiTokensApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/AutoscalingApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/BlackfireMonitoringApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/BlackfireProfilingApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/CertManagementApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/ConnectionsApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/ContinuousProfilingApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/DefaultApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/DeploymentApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/DeploymentTargetApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/DiffApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/DiscountsApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/DomainClaimApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/DomainManagementApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/EntrypointApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/EnvironmentActivityApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/EnvironmentApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/EnvironmentBackupsApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/EnvironmentTypeApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/EnvironmentVariablesApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/GrantsApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/HttpTrafficApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/InvoicesApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/MfaApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/OrdersApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/OrganizationInvitationsApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/OrganizationManagementApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/OrganizationMembersApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/OrganizationProjectsApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/OrganizationsApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/PhoneNumberApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/ProfilesApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/ProjectActivityApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/ProjectApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/ProjectInvitationsApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/ProjectsApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/ProjectSettingsApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/ProjectVariablesApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/RecordsApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/ReferencesApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/RegionsApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/RegistryCredentialApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/RepositoryApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/ResourcesApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/RoutingApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/RuntimeOperationsApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/SbomApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/SourceOperationsApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/SshKeysApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/SubscriptionsApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/SupportApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/SystemInformationApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/TaskApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/TeamAccessApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/TeamsApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/ThirdPartyIntegrationsApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/UserAccessApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/UserProfilesApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/UsersApi.php | Updates generated API constructor signature to accept TokenProvider. |
| src/Api/VouchersApi.php | Updates generated API constructor signature to accept TokenProvider. |
| templates/php/libraries/psr-18/api.mustache | Updates API generation template to accept TokenProvider and ApiConfiguration naming. |
| templates/php/Configuration.mustache | Docstring capitalization update (ApiConfiguration). |
| tests/Api/AbstractApiTest.php | New tests covering sendAuthenticatedRequest() 401 retry behavior via TokenProvider(force=true). |
| tests/Core/OAuthProviderTest.php | Adds tests for refresh-token grant behavior, refresh endpoint, buffer change, and Fiber guard (contains a Fiber API misuse). |
| tests/UpsunClientTest.php | Updates/extends auth token retrieval tests for OAuth + bearer-only mode. |
| tests/Core/Tasks/BaseTestCase.php | Refactors task test harness to use TokenProvider instead of OAuthProvider. |
| tests/Core/Tasks/ActivitiesTaskTest.php | Refactors to use TokenProvider in setup. |
| tests/Core/Tasks/ApplicationsTaskTest.php | Refactors to use TokenProvider in setup. |
| tests/Core/Tasks/BackupsTaskTest.php | Refactors to use TokenProvider in setup. |
| tests/Core/Tasks/CertificatesTaskTest.php | Refactors to use TokenProvider in setup. |
| tests/Core/Tasks/DomainsTaskTest.php | Refactors to use TokenProvider in setup. |
| tests/Core/Tasks/EnvironmentsTaskTest.php | Refactors to use TokenProvider in setup. |
| tests/Core/Tasks/IntegrationsTaskTest.php | Refactors to use TokenProvider in setup. |
| tests/Core/Tasks/InvitationsTaskTest.php | Refactors to use TokenProvider in setup. |
| tests/Core/Tasks/MountsTaskTest.php | Refactors to use TokenProvider in setup. |
| tests/Core/Tasks/OperationsTaskTest.php | Refactors to use TokenProvider in setup. |
| tests/Core/Tasks/OrganizationsTaskTest.php | Refactors to use TokenProvider in setup. |
| tests/Core/Tasks/ProjectsTaskTest.php | Refactors to use TokenProvider in setup. |
| tests/Core/Tasks/RegionsTaskTest.php | Refactors to use TokenProvider in setup. |
| tests/Core/Tasks/RepositoriesTaskTest.php | Refactors to use TokenProvider in setup. |
| tests/Core/Tasks/ResourcesTaskTest.php | Refactors to use TokenProvider in setup. |
| tests/Core/Tasks/RoutesTaskTest.php | Refactors to use TokenProvider in setup. |
| tests/Core/Tasks/ServicesTaskTest.php | Refactors to use TokenProvider in setup. |
| tests/Core/Tasks/SourceOperationsTaskTest.php | Refactors to use TokenProvider in setup. |
| tests/Core/Tasks/SshTaskTest.php | Refactors to use TokenProvider in setup. |
| tests/Core/Tasks/SupportTicketsTaskTest.php | Refactors to use TokenProvider in setup. |
| tests/Core/Tasks/TeamsTaskTest.php | Refactors to use TokenProvider in setup. |
| tests/Core/Tasks/UsersInvitationsTaskTest.php | Refactors to use TokenProvider in setup. |
| tests/Core/Tasks/UsersTaskTest.php | Refactors to use TokenProvider in setup. |
| tests/Core/Tasks/VariablesTaskTest.php | Refactors to use TokenProvider in setup. |
| tests/Core/Tasks/WorkersTaskTest.php | Refactors to use TokenProvider in setup. |
| phpunit.xml | Extends coverage inclusion to include src/Api/AbstractApi.php. |
| docs/php-alignment-node-prompt.md | Adds an internal alignment/implementation guide for porting Node SDK behavior. |
| .github/workflows/develop.yml | Workflow step naming tweak for clarity. |
| .github/workflows/publish.yml | Uses composer run install:ci for consistency with other workflows. |
Comments suppressed due to low confidence (2)
src/Api/AbstractApi.php:49
- AbstractApi::__construct() references $plugins and $uriFactory, but they are not defined in the constructor parameters anymore. This will trigger undefined variable errors and prevents customizing the plugin chain / URI factory as intended.
public function __construct(
private readonly TokenProvider $tokenProvider,
private ClientInterface $httpClient,
private readonly RequestFactoryInterface $requestFactory,
private readonly string $baseUri,
?StreamFactoryInterface $streamFactory = null,
) {
$plugins = $plugins ?? [
templates/php/abstract_api.mustache:57
- The AbstractApi template uses $plugins and $uriFactory inside __construct(), but they are not declared as parameters. The generated AbstractApi.php will therefore reference undefined variables.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ->method('sendRequest') | ||
| ->willReturnCallback(function () use (&$httpCallCount, $tokenResponse) { | ||
| $httpCallCount++; | ||
| \Fiber::getCurrent()?->suspend(); // simulate async I/O suspending the Fiber |
…lsafe instance call
Fiber::suspend() is a static method. Calling it via a nullsafe instance
reference ($fiber?->suspend()) is technically valid but a PHP anti-pattern
(calling static methods on instances). The correct form is:
if (Fiber::getCurrent() !== null) { Fiber::suspend(); }
This is explicit about the guard intent and avoids potential deprecation
warnings from static-via-instance call patterns.
Upsun SDK checker reportDisplay raw output |
flovntp
approved these changes
Jun 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Ports the 5 OAuth2 fixes from
upsun-sdk-nodePR #11 to the PHP SDK. All fixes are applied to the Mustache templates (source of truth for OpenAPI Generator) and to the generated files directly.Fixes
FIX 1 — 401 retry middleware calls
forceRefresh()instead ofexchangeCodeForToken()AbstractApi::sendAuthenticatedRequest()now intercepts HTTP 401 responses, callsOAuthProvider::forceRefresh()(which prefers therefresh_tokengrant), and retries the request once.FIX 2 —
refreshAccessToken()sendsAuthorization: BasicThe new
refreshAccessToken()method sendsAuthorization: Basic base64("platform-api-user:")as required for confidential clients.FIX 3 — Proactive refresh buffer raised from 60 s → 120 s
Mitigates clock skew between client and auth server.
FIX 4 — Configurable
refreshEndpoint(distinct fromtokenEndpoint)OAuthProvideraccepts an optionalrefreshEndpointconstructor parameter (defaults totokenEndpoint).UpsunClientpassesauth_url + "/" + refresh_endpointfromUpsunConfig.FIX 5 — Re-entrance guard (thundering-herd protection)
$acquiringToken: boolflag prevents recursive acquisition in PHP Fiber contexts. Multi-process FPM thundering-herd would require an external lock (Redis/APCu) — out of scope.New methods
forceRefresh()— public, clearsaccessToken/tokenExpirywithout touchingrefreshTokenoracquiringToken, then delegates toensureValidToken()refreshAccessToken()— private,grant_type=refresh_tokenviarefreshEndpointdoAcquireToken()— private, prefersrefresh_tokengrant, falls back toapi_tokenTests
9 new unit tests added (567 total, 3395 assertions — all green):
testRefreshAccessTokenSendsBasicAuthHeadertestRefreshAccessTokenUsesGrantTypeRefreshTokentestEnsureValidTokenPrefersRefreshTokenGranttestFallbackToApiTokenIfRefreshFailstestForceRefreshTriggersAcquisitionEvenWithValidTokentestForceRefreshPrefersRefreshTokenIfAvailabletestConcurrentForceRefreshCallsResultInSingleRequesttestRefreshEndpointUsedForRefreshTokenGranttestBuffer120SecondsTriggersProactiveRefreshSimilarity with Node SDK
~90% functional parity. The only intentional divergence is FIX 5: Node uses a shared
Promise(pendingToken) to deduplicate truly concurrent async callers; PHP uses a boolean re-entrance guard (PHP-FPM is single-threaded per process).Files changed
templates/php/oauth_provider.mustachetemplates/php/abstract_api.mustachesrc/Core/OAuthProvider.phpsrc/Api/AbstractApi.phpsrc/UpsunClient.phprefreshEndpointtoOAuthProvidertests/Core/OAuthProviderTest.php