From 436ed1b4c15eecab04a87ff5e73610f1bc6b0e3f Mon Sep 17 00:00:00 2001 From: mesilov Date: Thu, 6 Aug 2026 01:28:25 +0600 Subject: [PATCH] Fix Core retry API version after token renewal (#544) --- .tasks/544/plan.md | 175 ++++++++++++++++++ CHANGELOG.md | 4 + rector.php | 3 +- src/Core/Core.php | 2 +- src/Core/Credentials/Scope.php | 4 +- tests/Unit/Core/CoreTest.php | 60 ++++++ .../Unit/Services/RemoteEventsFactoryTest.php | 1 + 7 files changed, 245 insertions(+), 4 deletions(-) create mode 100644 .tasks/544/plan.md diff --git a/.tasks/544/plan.md b/.tasks/544/plan.md new file mode 100644 index 00000000..ea6478e6 --- /dev/null +++ b/.tasks/544/plan.md @@ -0,0 +1,175 @@ +# Plan: Core::call() preserves API version after token renewal (issue #544) + +## Context + +Issue #544 reports a bug in `Bitrix24\SDK\Core\Core::call()`: when an OAuth request receives +`401 expired_token`, the method renews the token and repeats the request as +`$this->call($apiMethod, $parameters)`. Because the third argument is omitted, the recursive retry +uses the default `ApiVersion::v1`, even when the original request was made with `ApiVersion::v3`. + +The 302 domain-change retry in the same method already preserves the version with +`$this->call($apiMethod, $parameters, $apiVersion)`, so the expected fix is to make the +`expired_token` retry use the same argument forwarding pattern. + +The issue uses `tasks.task.get` as the v3 example. Current Bitrix24 documentation confirms +`tasks.task.get` is a tasks method with required task id parameters and a v3-style response envelope +containing `result.item`. This bug is in the SDK core retry layer, not in the Tasks service wrapper, +so no service/result-item generator is applicable. + +Baseline setup in the issue worktree: + +- Branch: `bugfix/544-preserve-api-version-on-token-renew`, based on `origin/v3-dev`. +- `make composer-install` completed because the fresh worktree had no `vendor/autoload.php`. +- `make oa-schema-build` completed successfully after copying the ignored local webhook env file. +- `make test-unit` baseline passed with `1221 tests, 3336 assertions`. + +### Brainstorming + +1. Fix only `src/Core/Core.php` without a regression test. + This is too weak because the one-line fix is easy to regress and the bug is hard to notice manually. + +2. Add a unit regression test around `Core::call()` and then apply the one-line retry fix. + This is the recommended approach: it tests the core retry contract directly, avoids live OAuth + expiration setup, and keeps the implementation scoped to the actual bug. + +3. Add an integration test with a real expired OAuth token. + This would be brittle and hard to run deterministically because it depends on live token state and + external refresh credentials. + +Use approach 2. + +--- + +## Files to Create + +No production files need to be created. + +The task plan file is created at `.tasks/544/plan.md`. + +--- + +## Files to Modify + +### 1. `tests/Unit/Core/CoreTest.php` + +Add a regression test before changing production code: + +```php +#[Test] +#[TestDox('call() preserves API version when repeating a request after expired_token renewal')] +public function testCallPreservesApiVersionAfterExpiredTokenRenewal(): void +{ + $capturedApiVersions = []; + + // First response: 401 expired_token. + // Second response: 200 OK. + // ApiClient::getResponse() callback records each ApiVersion argument. + // ApiClient::getNewAuthToken() returns a valid RenewedAuthToken DTO. + + $core->call('tasks.task.get', ['id' => 1], ApiVersion::v3); + + $this->assertSame([ApiVersion::v3, ApiVersion::v3], $capturedApiVersions); +} +``` + +The test must fail on the current code because the second captured version is `ApiVersion::v1`. + +### 2. `src/Core/Core.php` + +Change the `expired_token` recursive retry: + +```php +$response = $this->call($apiMethod, $parameters, $apiVersion); +``` + +No other retry semantics should change. + +### 3. `CHANGELOG.md` + +After the quality gate is green, add an entry under `## Unreleased` -> `### Fixed`: + +```markdown +- Fixed `Core::call()` preserving `ApiVersion::v3` when retrying a request after OAuth token renewal ([#544](https://github.com/bitrix24/b24phpsdk/issues/544)) +``` + +### 4. `rector.php` + +If `make lint-rector` fails before code analysis with `Unknown named parameter $strictBooleans`, +remove the `strictBooleans: false` argument from `withPreparedSets()`. Current `rector/rector` +2.6.1 no longer accepts that named argument, and the removed value is the default behavior. + +### 5. `tests/Unit/Services/RemoteEventsFactoryTest.php` + +If the updated Rector run reports `AllowMockObjectsForDataProviderRector`, apply the requested +class-level `#[\PHPUnit\Framework\Attributes\AllowMockObjectsWithoutExpectations]` attribute to keep +the repository's Rector gate green with the current dependency set. + +### 6. `src/Core/Credentials/Scope.php` + +If `make test-integration-core` fails because the live `scope` endpoint returns new available scope +codes that are absent from `Scope::$availableScope`, add only those missing codes to the canonical +scope list. In the current live portal response this is limited to `timemanmobile` and +`vibecodeconnector`. + +--- + +## Deptrac compliance + +The production change stays inside `src/Core/Core.php` and adds no new dependencies. + +The unit test will reuse existing test dependencies plus existing SDK DTOs: + +- `Bitrix24\SDK\Application\ApplicationStatus` +- `Bitrix24\SDK\Core\Credentials\AuthToken` +- `Bitrix24\SDK\Core\Response\DTO\RenewedAuthToken` + +No `deptrac.yaml` skip violations should be added. + +--- + +## Verification + +TDD cycle: + +```bash +docker compose run --rm php-cli php -d auto_prepend_file=tests/phpunit-preload-guard.php vendor/bin/phpunit --filter testCallPreservesApiVersionAfterExpiredTokenRenewal --testsuite unit_tests --display-warnings +``` + +Expected RED result before the production change: the captured API versions are +`[ApiVersion::v3, ApiVersion::v1]` instead of `[ApiVersion::v3, ApiVersion::v3]`. + +Phase 1 quality gate: + +```bash +make lint-cs-fixer +make lint-rector +make lint-phpstan +make lint-deptrac +make test-unit +``` + +Phase 2 heavy checks: + +No new integration suite is needed for this core retry bug. Use the existing core integration suite +as the heavy check: + +```bash +make test-integration-core +``` + +Current Phase 2 status: + +- `make test-integration-core` reached the full suite summary with `24 tests, 3174 assertions`. +- The only error is `BatchTraversableListTest::testSingleBatchWithDescSortingMore` failing in + `tearDown()` while deleting test CRM contacts because the live portal returned + `operation time limit exceeded method is blocked due to operation time limit`. +- The same run reached and executed the live `ScopeTest`; the `scope` endpoint response included + `timemanmobile` and `vibecodeconnector`, confirming the `Scope` list update against live metadata. +- After waiting for the portal operating window reset, the targeted retry + `make test-file path='tests/Integration/Core/BatchTraversableListTest.php --filter testSingleBatchWithDescSortingMore'` + failed the same way in `tearDown()` after `1 test, 2512 assertions`, confirming this is a + reproducible live cleanup limit in the existing heavy batch test rather than a regression from + issue #544. +- Leftover test contacts from the failed full-suite and targeted runs were cleaned up by filtering + `crm.contact.list` on the two failed-run `ORIGINATOR_ID` values and deleting only matching + contacts. The final cleanup count check returned `found=0` for both originator values. diff --git a/CHANGELOG.md b/CHANGELOG.md index 21d0a618..258a3ab2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -74,6 +74,10 @@ ### Fixed +- Fixed `Core::call()` preserving the requested API version when retrying a request after OAuth token + renewal ([#544](https://github.com/bitrix24/b24phpsdk/issues/544)) +- Added missing available scope codes `timemanmobile` and `vibecodeconnector` to `Scope` + ([#544](https://github.com/bitrix24/b24phpsdk/issues/544)) - Fixed `Services\CRM\Lead\Service\Batch::list()` returning `DealItemResult` instead of `LeadItemResult` items ([#470](https://github.com/bitrix24/b24phpsdk/pull/470)) - Fixed batch operations for `Services\Catalog\Product` using the wrong-case `ID` key instead of diff --git a/rector.php b/rector.php index 0c4f12f7..fcc6b368 100644 --- a/rector.php +++ b/rector.php @@ -120,8 +120,7 @@ privatization: true, naming: true, instanceOf: true, - earlyReturn: true, - strictBooleans: false + earlyReturn: true ) ->withSkip([ RenamePropertyToMatchTypeRector::class, diff --git a/src/Core/Core.php b/src/Core/Core.php index 77b4cdf8..ae53fb66 100644 --- a/src/Core/Core.php +++ b/src/Core/Core.php @@ -183,7 +183,7 @@ public function call(string $apiMethod, array $parameters = [], ApiVersion $apiV $this->eventDispatcher->dispatch(new AuthTokenRenewedEvent($renewedToken)); // repeat previous api-call with new auth token - $response = $this->call($apiMethod, $parameters); + $response = $this->call($apiMethod, $parameters, $apiVersion); $this->logger->debug( 'api call repeated', [ diff --git a/src/Core/Credentials/Scope.php b/src/Core/Credentials/Scope.php index fa0e82e5..95b3d7c4 100644 --- a/src/Core/Credentials/Scope.php +++ b/src/Core/Credentials/Scope.php @@ -81,12 +81,14 @@ class Scope 'tasksmobile', 'telephony', 'timeman', + 'timemanmobile', 'user', 'user.userfield', 'user_basic', 'user_brief', 'userconsent', 'userfieldconfig', + 'vibecodeconnector', 'vote', ]; @@ -154,4 +156,4 @@ public static function initFromString(string $scope): self { return new self(str_replace(' ', '', explode(',', $scope))); } -} \ No newline at end of file +} diff --git a/tests/Unit/Core/CoreTest.php b/tests/Unit/Core/CoreTest.php index 25d98f0b..f22a7619 100644 --- a/tests/Unit/Core/CoreTest.php +++ b/tests/Unit/Core/CoreTest.php @@ -13,14 +13,20 @@ namespace Bitrix24\SDK\Tests\Unit\Core; +use Bitrix24\SDK\Application\ApplicationStatus; use Bitrix24\SDK\Core\ApiLevelErrorHandler; use Bitrix24\SDK\Core\Contracts\ApiClientInterface; use Bitrix24\SDK\Core\Contracts\ApiVersion; use Bitrix24\SDK\Core\Core; +use Bitrix24\SDK\Core\Credentials\ApplicationProfile; +use Bitrix24\SDK\Core\Credentials\AuthToken; use Bitrix24\SDK\Core\Credentials\Credentials; +use Bitrix24\SDK\Core\Credentials\Endpoints; +use Bitrix24\SDK\Core\Credentials\Scope; use Bitrix24\SDK\Core\Credentials\WebhookUrl; use Bitrix24\SDK\Core\Exceptions\AuthForbiddenException; use Bitrix24\SDK\Core\Exceptions\PortalUnavailableException; +use Bitrix24\SDK\Core\Response\DTO\RenewedAuthToken; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\Attributes\TestDox; @@ -135,6 +141,60 @@ public function testCallRoutesV3UnauthorizedErrorArrayThroughApiLevelErrorHandle $core->call('documentation', apiVersion: ApiVersion::v3); } + #[Test] + #[TestDox('call() preserves API version when repeating a request after expired_token renewal')] + public function testCallPreservesApiVersionAfterExpiredTokenRenewal(): void + { + $expiredTokenResponse = $this->createStub(ResponseInterface::class); + $expiredTokenResponse->method('getStatusCode')->willReturn(401); + $expiredTokenResponse->method('toArray')->willReturn(['error' => 'expired_token']); + + $okResponse = $this->createStub(ResponseInterface::class); + $okResponse->method('getStatusCode')->willReturn(200); + + $credentials = Credentials::createFromOAuth( + new AuthToken('old-access-token', 'old-refresh-token', time() - 3600), + new ApplicationProfile('client-id', 'client-secret', new Scope(['tasks'])), + new Endpoints('https://myportal.example.com', 'https://oauth.bitrix.info/') + ); + $renewedAuthToken = new RenewedAuthToken( + new AuthToken('new-access-token', 'new-refresh-token', time() + 3600), + 'member-id', + 'https://myportal.example.com', + 'https://oauth.bitrix.info/', + ApplicationStatus::subscription(), + 'myportal.example.com' + ); + + $capturedApiVersions = []; + $responses = [$expiredTokenResponse, $okResponse]; + + $apiClient = $this->createMock(ApiClientInterface::class); + $apiClient->method('getCredentials')->willReturn($credentials); + $apiClient->expects($this->once())->method('getNewAuthToken')->willReturn($renewedAuthToken); + $apiClient + ->expects($this->exactly(2)) + ->method('getResponse') + ->willReturnCallback( + static function (string $apiMethod, array $parameters, ApiVersion $apiVersion) use (&$capturedApiVersions, &$responses): ResponseInterface { + $capturedApiVersions[] = $apiVersion; + + return array_shift($responses); + } + ); + + $core = new Core( + $apiClient, + new ApiLevelErrorHandler(new NullLogger()), + new EventDispatcher(), + new NullLogger() + ); + + $core->call('tasks.task.get', ['id' => 1], ApiVersion::v3); + + $this->assertSame([ApiVersion::v3, ApiVersion::v3], $capturedApiVersions); + } + #[Test] #[TestDox('call() injects auth_connector into request parameters when it is set')] public function testCallInjectsAuthConnectorWhenSet(): void diff --git a/tests/Unit/Services/RemoteEventsFactoryTest.php b/tests/Unit/Services/RemoteEventsFactoryTest.php index 2b5f1bf0..467d7a93 100644 --- a/tests/Unit/Services/RemoteEventsFactoryTest.php +++ b/tests/Unit/Services/RemoteEventsFactoryTest.php @@ -31,6 +31,7 @@ use Symfony\Component\HttpFoundation\Request; #[CoversClass(RemoteEventsFactory::class)] +#[\PHPUnit\Framework\Attributes\AllowMockObjectsWithoutExpectations] class RemoteEventsFactoryTest extends TestCase { private RemoteEventsFactory $factory;