From 28219e00e7c26f295d564e7e57d5f98f23ed4721 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Tue, 27 Jan 2026 10:30:22 +0100 Subject: [PATCH 1/6] feat(astrolabe): add background token refresh job Prevents users from having to re-authorize Astrolabe after periods of inactivity by proactively refreshing OAuth tokens before they expire. Changes: - Add RefreshUserTokens background job that runs every 15 minutes - Add on-demand token refresh in SemanticSearchProvider (Unified Search) - Store issued_at timestamp for accurate token lifetime calculation - Add getAllUsersWithTokens() to query users needing refresh The job dynamically calculates refresh threshold based on actual token lifetime (50% remaining), working with any IdP (Nextcloud OIDC, Keycloak, etc.) rather than relying on IdP-specific configuration. Closes #510 Co-Authored-By: Claude Opus 4.5 --- third_party/astrolabe/appinfo/info.xml | 3 + .../lib/BackgroundJob/RefreshUserTokens.php | 155 ++++++++++++++++++ .../lib/Search/SemanticSearchProvider.php | 26 ++- .../astrolabe/lib/Service/McpTokenStorage.php | 36 ++++ 4 files changed, 217 insertions(+), 3 deletions(-) create mode 100644 third_party/astrolabe/lib/BackgroundJob/RefreshUserTokens.php diff --git a/third_party/astrolabe/appinfo/info.xml b/third_party/astrolabe/appinfo/info.xml index 1992b8f..31209a6 100644 --- a/third_party/astrolabe/appinfo/info.xml +++ b/third_party/astrolabe/appinfo/info.xml @@ -57,4 +57,7 @@ See [documentation](https://github.com/cbcoutinho/nextcloud-mcp-server) for conf link + + OCA\Astrolabe\BackgroundJob\RefreshUserTokens + diff --git a/third_party/astrolabe/lib/BackgroundJob/RefreshUserTokens.php b/third_party/astrolabe/lib/BackgroundJob/RefreshUserTokens.php new file mode 100644 index 0000000..c79add2 --- /dev/null +++ b/third_party/astrolabe/lib/BackgroundJob/RefreshUserTokens.php @@ -0,0 +1,155 @@ +setInterval(self::JOB_INTERVAL_SECONDS); + $this->setTimeSensitivity(IJob::TIME_INSENSITIVE); + } + + protected function run(mixed $argument): void { + $this->logger->info('RefreshUserTokens: Starting background token refresh'); + + $userIds = $this->tokenStorage->getAllUsersWithTokens(); + $this->logger->debug('RefreshUserTokens: Found ' . count($userIds) . ' users with tokens'); + + $refreshed = 0; + $failed = 0; + $skipped = 0; + + foreach ($userIds as $userId) { + $result = $this->refreshUserTokenIfNeeded($userId); + match ($result) { + 'refreshed' => $refreshed++, + 'failed' => $failed++, + 'skipped' => $skipped++, + }; + } + + $this->logger->info("RefreshUserTokens: Complete - refreshed=$refreshed, failed=$failed, skipped=$skipped"); + } + + /** + * Refresh a user's token if it's nearing expiration. + * + * Calculates the refresh threshold based on the token's actual lifetime, + * refreshing when less than 50% of the lifetime remains. + * + * @return string 'refreshed', 'failed', or 'skipped' + */ + private function refreshUserTokenIfNeeded(string $userId): string { + $token = $this->tokenStorage->getUserToken($userId); + + if ($token === null) { + return 'skipped'; + } + + $expiresAt = (int)($token['expires_at'] ?? 0); + $issuedAt = isset($token['issued_at']) ? (int)$token['issued_at'] : null; + $timeRemaining = $expiresAt - time(); + + // Calculate token lifetime from stored data or use default + if ($issuedAt !== null) { + $tokenLifetime = $expiresAt - $issuedAt; + } else { + // Fallback: use default lifetime assumption + $tokenLifetime = self::DEFAULT_TOKEN_LIFETIME_SECONDS; + } + + // Calculate threshold: refresh when 50% of lifetime remains + $threshold = max( + (int)($tokenLifetime * self::REFRESH_AT_REMAINING_PERCENT), + self::MIN_THRESHOLD_SECONDS + ); + + if ($timeRemaining > $threshold) { + // Token still has plenty of time, skip + return 'skipped'; + } + + // Token is expiring soon, attempt refresh + if (!isset($token['refresh_token'])) { + $this->logger->warning("RefreshUserTokens: User $userId has no refresh token"); + return 'failed'; + } + + $this->logger->debug("RefreshUserTokens: Refreshing token for user $userId (remaining={$timeRemaining}s, threshold={$threshold}s)"); + + try { + /** @var string $refreshToken */ + $refreshToken = $token['refresh_token']; + $newTokenData = $this->tokenRefresher->refreshAccessToken($refreshToken); + + if ($newTokenData === null) { + $this->logger->warning("RefreshUserTokens: Refresh returned null for user $userId"); + // Don't delete token here - let on-demand refresh handle cleanup + return 'failed'; + } + + // Calculate new expiration and store issued_at for future calculations + $expiresIn = (int)($newTokenData['expires_in'] ?? self::DEFAULT_TOKEN_LIFETIME_SECONDS); + $now = time(); + + /** @var string $accessToken */ + $accessToken = $newTokenData['access_token']; + /** @var string $newRefreshToken */ + $newRefreshToken = $newTokenData['refresh_token'] ?? $refreshToken; + + $this->tokenStorage->storeUserToken( + $userId, + $accessToken, + $newRefreshToken, + $now + $expiresIn, + $now // issued_at + ); + + $this->logger->debug("RefreshUserTokens: Successfully refreshed token for user $userId"); + return 'refreshed'; + + } catch (\Exception $e) { + $this->logger->error("RefreshUserTokens: Failed to refresh for user $userId: " . $e->getMessage()); + return 'failed'; + } + } +} diff --git a/third_party/astrolabe/lib/Search/SemanticSearchProvider.php b/third_party/astrolabe/lib/Search/SemanticSearchProvider.php index 8da0cb4..bdcb2f0 100644 --- a/third_party/astrolabe/lib/Search/SemanticSearchProvider.php +++ b/third_party/astrolabe/lib/Search/SemanticSearchProvider.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace OCA\Astrolabe\Search; use OCA\Astrolabe\AppInfo\Application; +use OCA\Astrolabe\Service\IdpTokenRefresher; use OCA\Astrolabe\Service\McpServerClient; use OCA\Astrolabe\Service\McpTokenStorage; use OCA\Astrolabe\Settings\Admin as AdminSettings; @@ -35,6 +36,7 @@ class SemanticSearchProvider implements IProvider { public function __construct( private McpServerClient $client, private McpTokenStorage $tokenStorage, + private IdpTokenRefresher $tokenRefresher, private IConfig $config, private IL10N $l10n, private IURLGenerator $urlGenerator, @@ -85,12 +87,30 @@ class SemanticSearchProvider implements IProvider { return SearchResult::complete($this->getName(), []); } - // Get OAuth token for user - $accessToken = $this->tokenStorage->getAccessToken($user->getUID()); + $userId = $user->getUID(); + + // Create refresh callback matching ApiController pattern + /** @return array{access_token: string, refresh_token: string, expires_in: int}|null */ + $refreshCallback = function (string $refreshToken): ?array { + $newTokenData = $this->tokenRefresher->refreshAccessToken($refreshToken); + + if ($newTokenData === null) { + return null; + } + + return [ + 'access_token' => $newTokenData['access_token'], + 'refresh_token' => $newTokenData['refresh_token'] ?? $refreshToken, + 'expires_in' => $newTokenData['expires_in'] ?? 3600, + ]; + }; + + // Get OAuth token for user with automatic refresh + $accessToken = $this->tokenStorage->getAccessToken($userId, $refreshCallback); if ($accessToken === null) { // User hasn't authorized the app yet - return empty results $this->logger->debug('No OAuth token for user in semantic search', [ - 'user_id' => $user->getUID(), + 'user_id' => $userId, ]); return SearchResult::complete($this->getName(), []); } diff --git a/third_party/astrolabe/lib/Service/McpTokenStorage.php b/third_party/astrolabe/lib/Service/McpTokenStorage.php index 5d499fa..8c36211 100644 --- a/third_party/astrolabe/lib/Service/McpTokenStorage.php +++ b/third_party/astrolabe/lib/Service/McpTokenStorage.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace OCA\Astrolabe\Service; use OCP\IConfig; +use OCP\IDBConnection; use OCP\Security\ICrypto; use Psr\Log\LoggerInterface; @@ -20,15 +21,18 @@ class McpTokenStorage { private $config; private $crypto; + private $db; private $logger; public function __construct( IConfig $config, ICrypto $crypto, + IDBConnection $db, LoggerInterface $logger, ) { $this->config = $config; $this->crypto = $crypto; + $this->db = $db; $this->logger = $logger; } @@ -41,18 +45,21 @@ class McpTokenStorage { * @param string $accessToken OAuth access token * @param string $refreshToken OAuth refresh token * @param int $expiresAt Unix timestamp when token expires + * @param int|null $issuedAt Unix timestamp when token was issued (for lifetime calculation) */ public function storeUserToken( string $userId, string $accessToken, string $refreshToken, int $expiresAt, + ?int $issuedAt = null, ): void { try { $tokenData = [ 'access_token' => $accessToken, 'refresh_token' => $refreshToken, 'expires_at' => $expiresAt, + 'issued_at' => $issuedAt ?? time(), ]; // Encrypt token data before storage @@ -153,6 +160,35 @@ class McpTokenStorage { } } + /** + * Get all user IDs that have OAuth tokens stored. + * + * Queries oc_preferences directly since IConfig doesn't support + * listing all users with a specific key set. + * + * @return list Array of user IDs + */ + public function getAllUsersWithTokens(): array { + $qb = $this->db->getQueryBuilder(); + $qb->select('userid') + ->from('preferences') + ->where($qb->expr()->eq('appid', $qb->createNamedParameter('astrolabe'))) + ->andWhere($qb->expr()->eq('configkey', $qb->createNamedParameter('oauth_tokens'))); + + $result = $qb->executeQuery(); + /** @var list $userIds */ + $userIds = []; + /** @psalm-suppress MixedAssignment - IResult::fetch() returns mixed */ + while (($row = $result->fetch()) !== false) { + if (is_array($row) && isset($row['userid']) && is_string($row['userid'])) { + $userIds[] = $row['userid']; + } + } + $result->closeCursor(); + + return $userIds; + } + /** * Get the access token for a user, handling expiration and refresh. * From c46f9eb212102734fa5a77a0503f22ab8ee79a04 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Tue, 27 Jan 2026 11:25:43 +0100 Subject: [PATCH 2/6] fix(astrolabe): add issued_at to on-demand token refresh Fixes missing issued_at parameter when storing tokens refreshed via getAccessToken() callback, ensuring accurate token lifetime calculation for the background refresh job. Co-Authored-By: Claude Opus 4.5 --- .../astrolabe/lib/Service/McpTokenStorage.php | 16 ++++++++++++---- third_party/astrolabe/psalm-baseline.xml | 7 ------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/third_party/astrolabe/lib/Service/McpTokenStorage.php b/third_party/astrolabe/lib/Service/McpTokenStorage.php index 8c36211..6e71eb3 100644 --- a/third_party/astrolabe/lib/Service/McpTokenStorage.php +++ b/third_party/astrolabe/lib/Service/McpTokenStorage.php @@ -217,14 +217,22 @@ class McpTokenStorage { if ($newTokenData && isset($newTokenData['access_token'])) { // Store refreshed token // Use new refresh token if provided (rotation), otherwise keep old one + $now = time(); + /** @var string $accessToken */ + $accessToken = $newTokenData['access_token']; + /** @var string $refreshToken */ + $refreshToken = $newTokenData['refresh_token'] ?? $token['refresh_token']; + $expiresIn = (int)($newTokenData['expires_in'] ?? 3600); + $this->storeUserToken( $userId, - $newTokenData['access_token'], - $newTokenData['refresh_token'] ?? $token['refresh_token'], - time() + ($newTokenData['expires_in'] ?? 3600) + $accessToken, + $refreshToken, + $now + $expiresIn, + $now // issued_at for accurate lifetime calculation ); - return $newTokenData['access_token']; + return $accessToken; } } catch (\Exception $e) { $this->logger->error("Failed to refresh token for user $userId", [ diff --git a/third_party/astrolabe/psalm-baseline.xml b/third_party/astrolabe/psalm-baseline.xml index 626e059..b4c0207 100644 --- a/third_party/astrolabe/psalm-baseline.xml +++ b/third_party/astrolabe/psalm-baseline.xml @@ -388,11 +388,6 @@ - - - - - @@ -400,11 +395,9 @@ - - From 815a09be3418d256af718fe0a5ea82492913583e Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Tue, 27 Jan 2026 12:23:06 +0100 Subject: [PATCH 3/6] test(astrolabe): add unit tests for background token refresh - Fix McpTokenStorageTest: add IDBConnection mock for new constructor parameter - Add doctrine/dbal dev dependency for IQueryBuilder mock support - Add tests for getAllUsersWithTokens() database query method - Create RefreshUserTokensTest with comprehensive coverage: - Job interval configuration (15 min) - Token refresh threshold logic (50% lifetime) - issued_at tracking for accurate lifetime calculation - Fallback to default lifetime when issued_at missing - Token rotation handling - Error handling and logging Co-Authored-By: Claude Opus 4.5 --- third_party/astrolabe/composer.json | 1 + third_party/astrolabe/composer.lock | 304 +++++++++++- .../BackgroundJob/RefreshUserTokensTest.php | 435 ++++++++++++++++++ .../unit/Service/McpTokenStorageTest.php | 86 +++- 4 files changed, 818 insertions(+), 8 deletions(-) create mode 100644 third_party/astrolabe/tests/unit/BackgroundJob/RefreshUserTokensTest.php diff --git a/third_party/astrolabe/composer.json b/third_party/astrolabe/composer.json index d153cc0..91969c1 100644 --- a/third_party/astrolabe/composer.json +++ b/third_party/astrolabe/composer.json @@ -39,6 +39,7 @@ "php": "^8.1" }, "require-dev": { + "doctrine/dbal": "^3.8", "nextcloud/ocp": "dev-stable30", "phpunit/phpunit": "^10.0", "roave/security-advisories": "dev-latest" diff --git a/third_party/astrolabe/composer.lock b/third_party/astrolabe/composer.lock index 59c26e1..d45cb51 100644 --- a/third_party/astrolabe/composer.lock +++ b/third_party/astrolabe/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "94a9d7f7619235ef2a310deec2ce14f0", + "content-hash": "e6ea5a770c578a5d7694602bb2618cef", "packages": [ { "name": "bamarni/composer-bin-plugin", @@ -65,6 +65,259 @@ } ], "packages-dev": [ + { + "name": "doctrine/dbal", + "version": "3.10.4", + "source": { + "type": "git", + "url": "https://github.com/doctrine/dbal.git", + "reference": "63a46cb5aa6f60991186cc98c1d1b50c09311868" + }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/doctrine/dbal/zipball/63a46cb5aa6f60991186cc98c1d1b50c09311868", + "reference": "63a46cb5aa6f60991186cc98c1d1b50c09311868", + "shasum": "" + }, + "require": { + "composer-runtime-api": "^2", + "doctrine/deprecations": "^0.5.3|^1", + "doctrine/event-manager": "^1|^2", + "php": "^7.4 || ^8.0", + "psr/cache": "^1|^2|^3", + "psr/log": "^1|^2|^3" + }, + "conflict": { + "doctrine/cache": "< 1.11" + }, + "require-dev": { + "doctrine/cache": "^1.11|^2.0", + "doctrine/coding-standard": "14.0.0", + "fig/log-test": "^1", + "jetbrains/phpstorm-stubs": "2023.1", + "phpstan/phpstan": "2.1.30", + "phpstan/phpstan-strict-rules": "^2", + "phpunit/phpunit": "9.6.29", + "slevomat/coding-standard": "8.24.0", + "squizlabs/php_codesniffer": "4.0.0", + "symfony/cache": "^5.4|^6.0|^7.0|^8.0", + "symfony/console": "^4.4|^5.4|^6.0|^7.0|^8.0" + }, + "suggest": { + "symfony/console": "For helpful console commands such as SQL execution and import of files." + }, + "bin": [ + "bin/doctrine-dbal" + ], + "type": "library", + "autoload": { + "psr-4": { + "Doctrine\\DBAL\\": "src" + } + }, + "notification-url": "https://packagist.org/downloads/", + "license": [ + "MIT" + ], + "authors": [ + { + "name": "Guilherme Blanco", + "email": "guilhermeblanco@gmail.com" + }, + { + "name": "Roman Borschel", + "email": "roman@code-factory.org" + }, + { + "name": "Benjamin Eberlei", + "email": "kontakt@beberlei.de" + }, + { + "name": "Jonathan Wage", + "email": "jonwage@gmail.com" + } + ], + "description": "Powerful PHP database abstraction layer (DBAL) with many features for database schema introspection and management.", + "homepage": "https://www.doctrine-project.org/projects/dbal.html", + "keywords": [ + "abstraction", + "database", + "db2", + "dbal", + "mariadb", + "mssql", + "mysql", + "oci8", + "oracle", + "pdo", + "pgsql", + "postgresql", + "queryobject", + "sasql", + "sql", + "sqlite", + "sqlserver", + "sqlsrv" + ], + "support": { + "issues": "https://github.com/doctrine/dbal/issues", + "source": "https://github.com/doctrine/dbal/tree/3.10.4" + }, + "funding": [ + { + "url": "https://www.doctrine-project.org/sponsorship.html", + "type": "custom" + }, + { + "url": "https://www.patreon.com/phpdoctrine", + "type": "patreon" + }, + { + "url": "https://tidelift.com/funding/github/packagist/doctrine%2Fdbal", + "type": "tidelift" + } + ], + "time": "2025-11-29T10:46:08+00:00" + }, + { + "name": "doctrine/deprecations", + "version": "1.1.5", + "source": { + "type": "git", + "url": "https://github.com/doctrine/deprecations.git", + "reference": "459c2f5dd3d6a4633d3b5f46ee2b1c40f57d3f38" + }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/doctrine/deprecations/zipball/459c2f5dd3d6a4633d3b5f46ee2b1c40f57d3f38", + "reference": "459c2f5dd3d6a4633d3b5f46ee2b1c40f57d3f38", + "shasum": "" + }, + "require": { + "php": "^7.1 || ^8.0" + }, + "conflict": { + "phpunit/phpunit": "<=7.5 || >=13" + }, + "require-dev": { + "doctrine/coding-standard": "^9 || ^12 || ^13", + "phpstan/phpstan": "1.4.10 || 2.1.11", + "phpstan/phpstan-phpunit": "^1.0 || ^2", + "phpunit/phpunit": "^7.5 || ^8.5 || ^9.6 || ^10.5 || ^11.5 || ^12", + "psr/log": "^1 || ^2 || ^3" + }, + "suggest": { + "psr/log": "Allows logging deprecations via PSR-3 logger implementation" + }, + "type": "library", + "autoload": { + "psr-4": { + "Doctrine\\Deprecations\\": "src" + } + }, + "notification-url": "https://packagist.org/downloads/", + "license": [ + "MIT" + ], + "description": "A small layer on top of trigger_error(E_USER_DEPRECATED) or PSR-3 logging with options to disable all deprecations or selectively for packages.", + "homepage": "https://www.doctrine-project.org/", + "support": { + "issues": "https://github.com/doctrine/deprecations/issues", + "source": "https://github.com/doctrine/deprecations/tree/1.1.5" + }, + "time": "2025-04-07T20:06:18+00:00" + }, + { + "name": "doctrine/event-manager", + "version": "2.1.0", + "source": { + "type": "git", + "url": "https://github.com/doctrine/event-manager.git", + "reference": "c07799fcf5ad362050960a0fd068dded40b1e312" + }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/doctrine/event-manager/zipball/c07799fcf5ad362050960a0fd068dded40b1e312", + "reference": "c07799fcf5ad362050960a0fd068dded40b1e312", + "shasum": "" + }, + "require": { + "php": "^8.1" + }, + "conflict": { + "doctrine/common": "<2.9" + }, + "require-dev": { + "doctrine/coding-standard": "^14", + "phpdocumentor/guides-cli": "^1.4", + "phpstan/phpstan": "^2.1.32", + "phpunit/phpunit": "^10.5.58" + }, + "type": "library", + "autoload": { + "psr-4": { + "Doctrine\\Common\\": "src" + } + }, + "notification-url": "https://packagist.org/downloads/", + "license": [ + "MIT" + ], + "authors": [ + { + "name": "Guilherme Blanco", + "email": "guilhermeblanco@gmail.com" + }, + { + "name": "Roman Borschel", + "email": "roman@code-factory.org" + }, + { + "name": "Benjamin Eberlei", + "email": "kontakt@beberlei.de" + }, + { + "name": "Jonathan Wage", + "email": "jonwage@gmail.com" + }, + { + "name": "Johannes Schmitt", + "email": "schmittjoh@gmail.com" + }, + { + "name": "Marco Pivetta", + "email": "ocramius@gmail.com" + } + ], + "description": "The Doctrine Event Manager is a simple PHP event system that was built to be used with the various Doctrine projects.", + "homepage": "https://www.doctrine-project.org/projects/event-manager.html", + "keywords": [ + "event", + "event dispatcher", + "event manager", + "event system", + "events" + ], + "support": { + "issues": "https://github.com/doctrine/event-manager/issues", + "source": "https://github.com/doctrine/event-manager/tree/2.1.0" + }, + "funding": [ + { + "url": "https://www.doctrine-project.org/sponsorship.html", + "type": "custom" + }, + { + "url": "https://www.patreon.com/phpdoctrine", + "type": "patreon" + }, + { + "url": "https://tidelift.com/funding/github/packagist/doctrine%2Fevent-manager", + "type": "tidelift" + } + ], + "time": "2026-01-17T22:40:21+00:00" + }, { "name": "myclabs/deep-copy", "version": "1.13.4", @@ -775,6 +1028,55 @@ ], "time": "2025-12-06T07:50:42+00:00" }, + { + "name": "psr/cache", + "version": "3.0.0", + "source": { + "type": "git", + "url": "https://github.com/php-fig/cache.git", + "reference": "aa5030cfa5405eccfdcb1083ce040c2cb8d253bf" + }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/php-fig/cache/zipball/aa5030cfa5405eccfdcb1083ce040c2cb8d253bf", + "reference": "aa5030cfa5405eccfdcb1083ce040c2cb8d253bf", + "shasum": "" + }, + "require": { + "php": ">=8.0.0" + }, + "type": "library", + "extra": { + "branch-alias": { + "dev-master": "1.0.x-dev" + } + }, + "autoload": { + "psr-4": { + "Psr\\Cache\\": "src/" + } + }, + "notification-url": "https://packagist.org/downloads/", + "license": [ + "MIT" + ], + "authors": [ + { + "name": "PHP-FIG", + "homepage": "https://www.php-fig.org/" + } + ], + "description": "Common interface for caching libraries", + "keywords": [ + "cache", + "psr", + "psr-6" + ], + "support": { + "source": "https://github.com/php-fig/cache/tree/3.0.0" + }, + "time": "2021-02-03T23:26:27+00:00" + }, { "name": "psr/clock", "version": "1.0.0", diff --git a/third_party/astrolabe/tests/unit/BackgroundJob/RefreshUserTokensTest.php b/third_party/astrolabe/tests/unit/BackgroundJob/RefreshUserTokensTest.php new file mode 100644 index 0000000..62bd32a --- /dev/null +++ b/third_party/astrolabe/tests/unit/BackgroundJob/RefreshUserTokensTest.php @@ -0,0 +1,435 @@ +timeFactory = $this->createMock(ITimeFactory::class); + $this->tokenStorage = $this->createMock(McpTokenStorage::class); + $this->tokenRefresher = $this->createMock(IdpTokenRefresher::class); + $this->logger = $this->createMock(LoggerInterface::class); + + $this->job = new RefreshUserTokens( + $this->timeFactory, + $this->tokenStorage, + $this->tokenRefresher, + $this->logger + ); + } + + // ========================================================================= + // Constructor Tests + // ========================================================================= + + public function testConstructorSetsInterval(): void { + // Use reflection to access the protected interval property + $reflection = new \ReflectionClass($this->job); + $property = $reflection->getProperty('interval'); + $property->setAccessible(true); + + $this->assertEquals(900, $property->getValue($this->job)); + } + + // ========================================================================= + // run() Method Tests + // ========================================================================= + + public function testRunWithNoUsers(): void { + $this->tokenStorage->method('getAllUsersWithTokens') + ->willReturn([]); + + $this->logger->expects($this->exactly(2)) + ->method('info') + ->willReturnCallback(function (string $message) { + static $callCount = 0; + $callCount++; + if ($callCount === 1) { + $this->assertStringContainsString('Starting', $message); + } else { + $this->assertStringContainsString('refreshed=0, failed=0, skipped=0', $message); + } + }); + + $this->logger->expects($this->once()) + ->method('debug') + ->with($this->stringContains('Found 0 users')); + + // Call run() via reflection since it's protected + $this->invokeRun(); + } + + public function testRunWithMultipleUsersAndMixedResults(): void { + $this->tokenStorage->method('getAllUsersWithTokens') + ->willReturn(['alice', 'bob', 'charlie']); + + // Alice: token with plenty of time (skipped) + // Bob: token near expiry with refresh token (refreshed) + // Charlie: token near expiry without refresh token (failed) + $this->tokenStorage->method('getUserToken') + ->willReturnCallback(function (string $userId) { + $now = time(); + return match ($userId) { + 'alice' => [ + 'access_token' => 'alice-token', + 'refresh_token' => 'alice-refresh', + 'expires_at' => $now + 3600, // 1 hour remaining (>50% of default lifetime) + 'issued_at' => $now, + ], + 'bob' => [ + 'access_token' => 'bob-token', + 'refresh_token' => 'bob-refresh', + 'expires_at' => $now + 100, // ~100s remaining (<50% of default lifetime) + 'issued_at' => $now - 3500, + ], + 'charlie' => [ + 'access_token' => 'charlie-token', + // No refresh_token + 'expires_at' => $now + 100, + 'issued_at' => $now - 3500, + ], + default => null, + }; + }); + + // Bob's refresh should succeed + $this->tokenRefresher->method('refreshAccessToken') + ->with('bob-refresh') + ->willReturn([ + 'access_token' => 'bob-new-token', + 'refresh_token' => 'bob-new-refresh', + 'expires_in' => 3600, + ]); + + $this->tokenStorage->expects($this->once()) + ->method('storeUserToken') + ->with( + 'bob', + 'bob-new-token', + 'bob-new-refresh', + $this->anything(), + $this->anything() + ); + + $this->logger->expects($this->exactly(2)) + ->method('info') + ->willReturnCallback(function (string $message) { + static $callCount = 0; + $callCount++; + if ($callCount === 2) { + $this->assertStringContainsString('refreshed=1, failed=1, skipped=1', $message); + } + }); + + $this->invokeRun(); + } + + // ========================================================================= + // refreshUserTokenIfNeeded() Tests + // ========================================================================= + + public function testRefreshSkippedWhenTokenHasPlentyOfTime(): void { + $now = time(); + $this->tokenStorage->method('getUserToken') + ->with('testuser') + ->willReturn([ + 'access_token' => 'valid-token', + 'refresh_token' => 'refresh-token', + 'expires_at' => $now + 3600, // 1 hour remaining + 'issued_at' => $now, + ]); + + $this->tokenRefresher->expects($this->never()) + ->method('refreshAccessToken'); + + $result = $this->invokeRefreshUserTokenIfNeeded('testuser'); + + $this->assertEquals('skipped', $result); + } + + public function testRefreshTriggeredWhenTokenNearExpiry(): void { + $now = time(); + $this->tokenStorage->method('getUserToken') + ->with('testuser') + ->willReturn([ + 'access_token' => 'expiring-token', + 'refresh_token' => 'refresh-token', + 'expires_at' => $now + 300, // 5 min remaining (< 50% of 3600s) + 'issued_at' => $now - 3300, // Issued 55 min ago + ]); + + $this->tokenRefresher->expects($this->once()) + ->method('refreshAccessToken') + ->with('refresh-token') + ->willReturn([ + 'access_token' => 'new-token', + 'refresh_token' => 'new-refresh-token', + 'expires_in' => 3600, + ]); + + $this->tokenStorage->expects($this->once()) + ->method('storeUserToken'); + + $result = $this->invokeRefreshUserTokenIfNeeded('testuser'); + + $this->assertEquals('refreshed', $result); + } + + public function testRefreshFailsWhenNoRefreshToken(): void { + $now = time(); + $this->tokenStorage->method('getUserToken') + ->with('testuser') + ->willReturn([ + 'access_token' => 'expiring-token', + // No refresh_token + 'expires_at' => $now + 100, + 'issued_at' => $now - 3500, + ]); + + $this->logger->expects($this->once()) + ->method('warning') + ->with($this->stringContains('no refresh token')); + + $this->tokenRefresher->expects($this->never()) + ->method('refreshAccessToken'); + + $result = $this->invokeRefreshUserTokenIfNeeded('testuser'); + + $this->assertEquals('failed', $result); + } + + public function testRefreshFailsWhenRefresherReturnsNull(): void { + $now = time(); + $this->tokenStorage->method('getUserToken') + ->with('testuser') + ->willReturn([ + 'access_token' => 'expiring-token', + 'refresh_token' => 'invalid-refresh', + 'expires_at' => $now + 100, + 'issued_at' => $now - 3500, + ]); + + $this->tokenRefresher->expects($this->once()) + ->method('refreshAccessToken') + ->with('invalid-refresh') + ->willReturn(null); + + $this->logger->expects($this->once()) + ->method('warning') + ->with($this->stringContains('Refresh returned null')); + + // Should NOT delete token - let on-demand refresh handle cleanup + $this->tokenStorage->expects($this->never()) + ->method('deleteUserToken'); + + $result = $this->invokeRefreshUserTokenIfNeeded('testuser'); + + $this->assertEquals('failed', $result); + } + + public function testRefreshUsesIssuedAtForLifetimeCalculation(): void { + $now = time(); + // Token with custom lifetime: issued 50 min ago, expires in 10 min (total 60 min) + // 10/60 = 16.7% remaining, which is < 50%, so should refresh + $this->tokenStorage->method('getUserToken') + ->with('testuser') + ->willReturn([ + 'access_token' => 'token', + 'refresh_token' => 'refresh', + 'expires_at' => $now + 600, // 10 min remaining + 'issued_at' => $now - 3000, // 50 min ago, total lifetime 60 min + ]); + + $this->tokenRefresher->expects($this->once()) + ->method('refreshAccessToken') + ->willReturn([ + 'access_token' => 'new-token', + 'refresh_token' => 'new-refresh', + 'expires_in' => 3600, + ]); + + $result = $this->invokeRefreshUserTokenIfNeeded('testuser'); + + $this->assertEquals('refreshed', $result); + } + + public function testRefreshUsesDefaultLifetimeWhenNoIssuedAt(): void { + $now = time(); + // Token without issued_at, uses default 3600s lifetime + // 300s remaining / 3600s = 8.3% remaining, which is < 50%, so should refresh + $this->tokenStorage->method('getUserToken') + ->with('testuser') + ->willReturn([ + 'access_token' => 'token', + 'refresh_token' => 'refresh', + 'expires_at' => $now + 300, // 5 min remaining + // No issued_at + ]); + + $this->tokenRefresher->expects($this->once()) + ->method('refreshAccessToken') + ->willReturn([ + 'access_token' => 'new-token', + 'refresh_token' => 'new-refresh', + 'expires_in' => 3600, + ]); + + $result = $this->invokeRefreshUserTokenIfNeeded('testuser'); + + $this->assertEquals('refreshed', $result); + } + + public function testRefreshStoresNewTokenWithIssuedAt(): void { + $now = time(); + $this->tokenStorage->method('getUserToken') + ->with('testuser') + ->willReturn([ + 'access_token' => 'old-token', + 'refresh_token' => 'old-refresh', + 'expires_at' => $now + 100, + 'issued_at' => $now - 3500, + ]); + + $this->tokenRefresher->expects($this->once()) + ->method('refreshAccessToken') + ->willReturn([ + 'access_token' => 'new-token', + 'refresh_token' => 'new-refresh', + 'expires_in' => 3600, + ]); + + // Verify storeUserToken is called with issued_at parameter + $this->tokenStorage->expects($this->once()) + ->method('storeUserToken') + ->with( + 'testuser', + 'new-token', + 'new-refresh', + $this->greaterThan($now), // expires_at = now + 3600 + $this->greaterThanOrEqual($now) // issued_at = now + ); + + $result = $this->invokeRefreshUserTokenIfNeeded('testuser'); + + $this->assertEquals('refreshed', $result); + } + + public function testRefreshKeepsOldRefreshTokenIfNotRotated(): void { + $now = time(); + $this->tokenStorage->method('getUserToken') + ->with('testuser') + ->willReturn([ + 'access_token' => 'old-token', + 'refresh_token' => 'original-refresh', + 'expires_at' => $now + 100, + 'issued_at' => $now - 3500, + ]); + + // IdP returns new access token but no new refresh token (no rotation) + $this->tokenRefresher->expects($this->once()) + ->method('refreshAccessToken') + ->willReturn([ + 'access_token' => 'new-token', + // No refresh_token in response + 'expires_in' => 3600, + ]); + + // Should use the original refresh token + $this->tokenStorage->expects($this->once()) + ->method('storeUserToken') + ->with( + 'testuser', + 'new-token', + 'original-refresh', // Original refresh token preserved + $this->anything(), + $this->anything() + ); + + $result = $this->invokeRefreshUserTokenIfNeeded('testuser'); + + $this->assertEquals('refreshed', $result); + } + + public function testRefreshHandlesException(): void { + $now = time(); + $this->tokenStorage->method('getUserToken') + ->with('testuser') + ->willReturn([ + 'access_token' => 'token', + 'refresh_token' => 'refresh', + 'expires_at' => $now + 100, + 'issued_at' => $now - 3500, + ]); + + $this->tokenRefresher->expects($this->once()) + ->method('refreshAccessToken') + ->willThrowException(new \Exception('Network error')); + + $this->logger->expects($this->once()) + ->method('error') + ->with($this->stringContains('Failed to refresh')); + + $result = $this->invokeRefreshUserTokenIfNeeded('testuser'); + + $this->assertEquals('failed', $result); + } + + public function testRefreshSkippedWhenNoToken(): void { + $this->tokenStorage->method('getUserToken') + ->with('testuser') + ->willReturn(null); + + $this->tokenRefresher->expects($this->never()) + ->method('refreshAccessToken'); + + $result = $this->invokeRefreshUserTokenIfNeeded('testuser'); + + $this->assertEquals('skipped', $result); + } + + // ========================================================================= + // Helper Methods + // ========================================================================= + + /** + * Invoke the protected run() method. + */ + private function invokeRun(): void { + $reflection = new \ReflectionClass($this->job); + $method = $reflection->getMethod('run'); + $method->setAccessible(true); + $method->invoke($this->job, null); + } + + /** + * Invoke the private refreshUserTokenIfNeeded() method. + */ + private function invokeRefreshUserTokenIfNeeded(string $userId): string { + $reflection = new \ReflectionClass($this->job); + $method = $reflection->getMethod('refreshUserTokenIfNeeded'); + $method->setAccessible(true); + return $method->invoke($this->job, $userId); + } +} diff --git a/third_party/astrolabe/tests/unit/Service/McpTokenStorageTest.php b/third_party/astrolabe/tests/unit/Service/McpTokenStorageTest.php index 6137241..9688516 100644 --- a/third_party/astrolabe/tests/unit/Service/McpTokenStorageTest.php +++ b/third_party/astrolabe/tests/unit/Service/McpTokenStorageTest.php @@ -5,7 +5,11 @@ declare(strict_types=1); namespace OCA\Astrolabe\Tests\Unit\Service; use OCA\Astrolabe\Service\McpTokenStorage; +use OCP\DB\IResult; +use OCP\DB\QueryBuilder\IExpressionBuilder; +use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IConfig; +use OCP\IDBConnection; use OCP\Security\ICrypto; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; @@ -19,6 +23,7 @@ use Psr\Log\LoggerInterface; final class McpTokenStorageTest extends TestCase { private IConfig&MockObject $config; private ICrypto&MockObject $crypto; + private IDBConnection&MockObject $db; private LoggerInterface&MockObject $logger; private McpTokenStorage $storage; @@ -27,11 +32,13 @@ final class McpTokenStorageTest extends TestCase { $this->config = $this->createMock(IConfig::class); $this->crypto = $this->createMock(ICrypto::class); + $this->db = $this->createMock(IDBConnection::class); $this->logger = $this->createMock(LoggerInterface::class); $this->storage = new McpTokenStorage( $this->config, $this->crypto, + $this->db, $this->logger ); } @@ -46,15 +53,15 @@ final class McpTokenStorageTest extends TestCase { $refreshToken = 'refresh-token-456'; $expiresAt = time() + 3600; - $expectedTokenData = [ - 'access_token' => $accessToken, - 'refresh_token' => $refreshToken, - 'expires_at' => $expiresAt, - ]; - $this->crypto->expects($this->once()) ->method('encrypt') - ->with(json_encode($expectedTokenData)) + ->with($this->callback(function (string $json) use ($accessToken, $refreshToken, $expiresAt) { + $data = json_decode($json, true); + return $data['access_token'] === $accessToken + && $data['refresh_token'] === $refreshToken + && $data['expires_at'] === $expiresAt + && isset($data['issued_at']); // issued_at should be set (defaults to time()) + })) ->willReturn('encrypted-data'); $this->config->expects($this->once()) @@ -524,4 +531,69 @@ final class McpTokenStorageTest extends TestCase { $this->assertNull($result); } + + // ========================================================================= + // getAllUsersWithTokens Tests + // ========================================================================= + + public function testGetAllUsersWithTokensReturnsUserIds(): void { + $qb = $this->createMock(IQueryBuilder::class); + $expr = $this->createMock(IExpressionBuilder::class); + $result = $this->createMock(IResult::class); + + // Chain builder methods + $qb->method('select')->willReturnSelf(); + $qb->method('from')->willReturnSelf(); + $qb->method('where')->willReturnSelf(); + $qb->method('andWhere')->willReturnSelf(); + $qb->method('expr')->willReturn($expr); + $qb->method('createNamedParameter')->willReturnArgument(0); + $qb->method('executeQuery')->willReturn($result); + + // Mock expression builder + $expr->method('eq')->willReturn('mocked_condition'); + + // Mock result set with multiple users + $result->method('fetch')->willReturnOnConsecutiveCalls( + ['userid' => 'admin'], + ['userid' => 'alice'], + ['userid' => 'bob'], + false // End of results + ); + $result->expects($this->once())->method('closeCursor'); + + $this->db->method('getQueryBuilder')->willReturn($qb); + + $userIds = $this->storage->getAllUsersWithTokens(); + + $this->assertEquals(['admin', 'alice', 'bob'], $userIds); + } + + public function testGetAllUsersWithTokensReturnsEmptyArrayWhenNoTokens(): void { + $qb = $this->createMock(IQueryBuilder::class); + $expr = $this->createMock(IExpressionBuilder::class); + $result = $this->createMock(IResult::class); + + // Chain builder methods + $qb->method('select')->willReturnSelf(); + $qb->method('from')->willReturnSelf(); + $qb->method('where')->willReturnSelf(); + $qb->method('andWhere')->willReturnSelf(); + $qb->method('expr')->willReturn($expr); + $qb->method('createNamedParameter')->willReturnArgument(0); + $qb->method('executeQuery')->willReturn($result); + + // Mock expression builder + $expr->method('eq')->willReturn('mocked_condition'); + + // Mock empty result set + $result->method('fetch')->willReturn(false); + $result->expects($this->once())->method('closeCursor'); + + $this->db->method('getQueryBuilder')->willReturn($qb); + + $userIds = $this->storage->getAllUsersWithTokens(); + + $this->assertEquals([], $userIds); + } } From 5b71ac3251402adea741ca15386ae655dcb7fd0b Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Tue, 27 Jan 2026 22:23:42 +0100 Subject: [PATCH 4/6] fix(astrolabe): add locking to prevent token refresh race condition Adds distributed locking using Nextcloud's ILockingProvider to prevent race conditions between background job and on-demand token refresh. Uses double-check locking pattern: 1. Quick check without lock - return immediately if token is valid 2. Acquire exclusive lock if token needs refresh 3. Re-check after lock - another process may have refreshed 4. Refresh only if still needed 5. Graceful degradation on LockedException Changes: - McpTokenStorage: add ILockingProvider, withTokenLock() method - McpTokenStorage: update getAccessToken() with locking pattern - RefreshUserTokens: wrap refresh in withTokenLock(), catch LockedException - Add comprehensive unit tests for locking behavior Co-Authored-By: Claude Opus 4.5 --- .../lib/BackgroundJob/RefreshUserTokens.php | 105 ++++++++---- .../astrolabe/lib/Service/McpTokenStorage.php | 150 ++++++++++++----- .../BackgroundJob/RefreshUserTokensTest.php | 145 ++++++++++++++++ .../unit/Service/McpTokenStorageTest.php | 156 +++++++++++++++++- 4 files changed, 482 insertions(+), 74 deletions(-) diff --git a/third_party/astrolabe/lib/BackgroundJob/RefreshUserTokens.php b/third_party/astrolabe/lib/BackgroundJob/RefreshUserTokens.php index c79add2..a74ec65 100644 --- a/third_party/astrolabe/lib/BackgroundJob/RefreshUserTokens.php +++ b/third_party/astrolabe/lib/BackgroundJob/RefreshUserTokens.php @@ -9,6 +9,7 @@ use OCA\Astrolabe\Service\McpTokenStorage; use OCP\AppFramework\Utility\ITimeFactory; use OCP\BackgroundJob\IJob; use OCP\BackgroundJob\TimedJob; +use OCP\Lock\LockedException; use Psr\Log\LoggerInterface; /** @@ -76,6 +77,10 @@ class RefreshUserTokens extends TimedJob { * Calculates the refresh threshold based on the token's actual lifetime, * refreshing when less than 50% of the lifetime remains. * + * Uses locking to prevent race conditions with on-demand refresh in + * getAccessToken(). If lock cannot be acquired, skips this user since + * on-demand refresh is already handling it. + * * @return string 'refreshed', 'failed', or 'skipped' */ private function refreshUserTokenIfNeeded(string $userId): string { @@ -108,45 +113,81 @@ class RefreshUserTokens extends TimedJob { return 'skipped'; } - // Token is expiring soon, attempt refresh - if (!isset($token['refresh_token'])) { - $this->logger->warning("RefreshUserTokens: User $userId has no refresh token"); - return 'failed'; - } - - $this->logger->debug("RefreshUserTokens: Refreshing token for user $userId (remaining={$timeRemaining}s, threshold={$threshold}s)"); - + // Token is expiring soon, attempt refresh with lock try { - /** @var string $refreshToken */ - $refreshToken = $token['refresh_token']; - $newTokenData = $this->tokenRefresher->refreshAccessToken($refreshToken); + return $this->tokenStorage->withTokenLock($userId, function () use ($userId) { + // Re-check token after acquiring lock (double-check pattern) + // Another process may have refreshed while we waited for lock + $currentToken = $this->tokenStorage->getUserToken($userId); - if ($newTokenData === null) { - $this->logger->warning("RefreshUserTokens: Refresh returned null for user $userId"); - // Don't delete token here - let on-demand refresh handle cleanup - return 'failed'; - } + if ($currentToken === null) { + return 'skipped'; + } - // Calculate new expiration and store issued_at for future calculations - $expiresIn = (int)($newTokenData['expires_in'] ?? self::DEFAULT_TOKEN_LIFETIME_SECONDS); - $now = time(); + // Recalculate threshold with current token data + $currentExpiresAt = (int)($currentToken['expires_at'] ?? 0); + $currentIssuedAt = isset($currentToken['issued_at']) ? (int)$currentToken['issued_at'] : null; + $currentTimeRemaining = $currentExpiresAt - time(); - /** @var string $accessToken */ - $accessToken = $newTokenData['access_token']; - /** @var string $newRefreshToken */ - $newRefreshToken = $newTokenData['refresh_token'] ?? $refreshToken; + if ($currentIssuedAt !== null) { + $currentTokenLifetime = $currentExpiresAt - $currentIssuedAt; + } else { + $currentTokenLifetime = self::DEFAULT_TOKEN_LIFETIME_SECONDS; + } - $this->tokenStorage->storeUserToken( - $userId, - $accessToken, - $newRefreshToken, - $now + $expiresIn, - $now // issued_at - ); + $currentThreshold = max( + (int)($currentTokenLifetime * self::REFRESH_AT_REMAINING_PERCENT), + self::MIN_THRESHOLD_SECONDS + ); - $this->logger->debug("RefreshUserTokens: Successfully refreshed token for user $userId"); - return 'refreshed'; + if ($currentTimeRemaining > $currentThreshold) { + // Token was refreshed by another process while we waited + $this->logger->debug("RefreshUserTokens: Token already refreshed for user $userId while waiting for lock"); + return 'skipped'; + } + // Still needs refresh, proceed + if (!isset($currentToken['refresh_token'])) { + $this->logger->warning("RefreshUserTokens: User $userId has no refresh token"); + return 'failed'; + } + + $this->logger->debug("RefreshUserTokens: Refreshing token for user $userId (remaining={$currentTimeRemaining}s, threshold={$currentThreshold}s)"); + + /** @var string $refreshToken */ + $refreshToken = $currentToken['refresh_token']; + $newTokenData = $this->tokenRefresher->refreshAccessToken($refreshToken); + + if ($newTokenData === null) { + $this->logger->warning("RefreshUserTokens: Refresh returned null for user $userId"); + // Don't delete token here - let on-demand refresh handle cleanup + return 'failed'; + } + + // Calculate new expiration and store issued_at for future calculations + $expiresIn = (int)($newTokenData['expires_in'] ?? self::DEFAULT_TOKEN_LIFETIME_SECONDS); + $now = time(); + + /** @var string $accessToken */ + $accessToken = $newTokenData['access_token']; + /** @var string $newRefreshToken */ + $newRefreshToken = $newTokenData['refresh_token'] ?? $refreshToken; + + $this->tokenStorage->storeUserToken( + $userId, + $accessToken, + $newRefreshToken, + $now + $expiresIn, + $now // issued_at + ); + + $this->logger->debug("RefreshUserTokens: Successfully refreshed token for user $userId"); + return 'refreshed'; + }); + } catch (LockedException $e) { + // Lock held by on-demand refresh - expected, not an error + $this->logger->debug("RefreshUserTokens: Lock held for user $userId, skipping"); + return 'skipped'; } catch (\Exception $e) { $this->logger->error("RefreshUserTokens: Failed to refresh for user $userId: " . $e->getMessage()); return 'failed'; diff --git a/third_party/astrolabe/lib/Service/McpTokenStorage.php b/third_party/astrolabe/lib/Service/McpTokenStorage.php index 6e71eb3..6ceefc6 100644 --- a/third_party/astrolabe/lib/Service/McpTokenStorage.php +++ b/third_party/astrolabe/lib/Service/McpTokenStorage.php @@ -6,6 +6,8 @@ namespace OCA\Astrolabe\Service; use OCP\IConfig; use OCP\IDBConnection; +use OCP\Lock\ILockingProvider; +use OCP\Lock\LockedException; use OCP\Security\ICrypto; use Psr\Log\LoggerInterface; @@ -23,17 +25,20 @@ class McpTokenStorage { private $crypto; private $db; private $logger; + private ILockingProvider $lockingProvider; public function __construct( IConfig $config, ICrypto $crypto, IDBConnection $db, LoggerInterface $logger, + ILockingProvider $lockingProvider, ) { $this->config = $config; $this->crypto = $crypto; $this->db = $db; $this->logger = $logger; + $this->lockingProvider = $lockingProvider; } /** @@ -136,6 +141,38 @@ class McpTokenStorage { return time() >= ($token['expires_at'] - self::TOKEN_EXPIRY_BUFFER_SECONDS); } + /** + * Get the lock path for a user's token refresh operation. + * + * @param string $userId User ID + * @return string Lock path + */ + private function getTokenRefreshLockPath(string $userId): string { + return 'astrolabe/oauth/tokens/' . $userId; + } + + /** + * Execute callback while holding exclusive lock on user's token. + * + * Prevents race conditions between background job and on-demand token refresh. + * + * @template T + * @param string $userId User ID + * @param callable(): T $callback + * @return T + * @throws LockedException If lock cannot be acquired + */ + public function withTokenLock(string $userId, callable $callback): mixed { + $lockPath = $this->getTokenRefreshLockPath($userId); + + $this->lockingProvider->acquireLock($lockPath, ILockingProvider::LOCK_EXCLUSIVE); + try { + return $callback(); + } finally { + $this->lockingProvider->releaseLock($lockPath, ILockingProvider::LOCK_EXCLUSIVE); + } + } + /** * Delete stored tokens for a user. * @@ -195,67 +232,98 @@ class McpTokenStorage { * This is a convenience method that combines token retrieval, * expiration checking, and automatic refresh if needed. * + * Uses double-check locking pattern to prevent race conditions between + * background job and on-demand refresh while minimizing lock contention. + * * @param string $userId User ID * @param callable|null $refreshCallback Callback to refresh token if expired * Should accept (refreshToken) and return new token data * @return string|null Access token, or null if not available */ public function getAccessToken(string $userId, ?callable $refreshCallback = null): ?string { + // Quick check without lock (optimization) $token = $this->getUserToken($userId); if (!$token) { return null; } - // Check if token is expired - if ($this->isExpired($token)) { - // Try to refresh if callback provided - if ($refreshCallback && isset($token['refresh_token'])) { - try { - $newTokenData = $refreshCallback($token['refresh_token']); + // If not expired, return immediately without lock + if (!$this->isExpired($token)) { + return $token['access_token']; + } - if ($newTokenData && isset($newTokenData['access_token'])) { - // Store refreshed token - // Use new refresh token if provided (rotation), otherwise keep old one - $now = time(); - /** @var string $accessToken */ - $accessToken = $newTokenData['access_token']; - /** @var string $refreshToken */ - $refreshToken = $newTokenData['refresh_token'] ?? $token['refresh_token']; - $expiresIn = (int)($newTokenData['expires_in'] ?? 3600); + // Token expired - acquire lock for refresh + try { + /** @var string|null */ + return $this->withTokenLock($userId, function () use ($userId, $refreshCallback): ?string { + // Re-check after acquiring lock (double-check pattern) + // Another process may have refreshed while we waited for the lock + $currentToken = $this->getUserToken($userId); - $this->storeUserToken( - $userId, - $accessToken, - $refreshToken, - $now + $expiresIn, - $now // issued_at for accurate lifetime calculation - ); - - return $accessToken; - } - } catch (\Exception $e) { - $this->logger->error("Failed to refresh token for user $userId", [ - 'error' => $e->getMessage() - ]); - // Delete stale token to prevent repeated refresh attempts - $this->deleteUserToken($userId); + if (!$currentToken) { return null; } - // Refresh callback returned null or invalid data - delete stale token + // Check if another process already refreshed the token + if (!$this->isExpired($currentToken)) { + $this->logger->debug("Token already refreshed for user $userId while waiting for lock"); + return $currentToken['access_token']; + } + + // Still expired, perform refresh + if ($refreshCallback && isset($currentToken['refresh_token'])) { + try { + /** @var string $refreshToken */ + $refreshToken = $currentToken['refresh_token']; + $newTokenData = $refreshCallback($refreshToken); + + if ($newTokenData && isset($newTokenData['access_token'])) { + // Store refreshed token + // Use new refresh token if provided (rotation), otherwise keep old one + $now = time(); + /** @var string $accessToken */ + $accessToken = $newTokenData['access_token']; + /** @var string $newRefreshToken */ + $newRefreshToken = $newTokenData['refresh_token'] ?? $refreshToken; + $expiresIn = (int)($newTokenData['expires_in'] ?? 3600); + + $this->storeUserToken( + $userId, + $accessToken, + $newRefreshToken, + $now + $expiresIn, + $now // issued_at for accurate lifetime calculation + ); + + return $accessToken; + } + } catch (\Exception $e) { + $this->logger->error("Failed to refresh token for user $userId", [ + 'error' => $e->getMessage() + ]); + // Delete stale token to prevent repeated refresh attempts + $this->deleteUserToken($userId); + return null; + } + + // Refresh callback returned null or invalid data - delete stale token + $this->deleteUserToken($userId); + $this->logger->info("Deleted stale token for user $userId after refresh failure"); + return null; + } + + // Token expired and no refresh callback available - delete stale token $this->deleteUserToken($userId); - $this->logger->info("Deleted stale token for user $userId after refresh failure"); + $this->logger->info("Token expired for user $userId, no refresh available"); return null; - } - - // Token expired and no refresh callback available - delete stale token - $this->deleteUserToken($userId); - $this->logger->info("Token expired for user $userId, no refresh available"); - return null; + }); + } catch (LockedException $e) { + // Could not acquire lock - another process is refreshing + // Return stale token rather than failing - caller can retry if needed + $this->logger->warning("Could not acquire token lock for user $userId, returning stale token"); + return $token['access_token'] ?? null; } - - return $token['access_token']; } /** diff --git a/third_party/astrolabe/tests/unit/BackgroundJob/RefreshUserTokensTest.php b/third_party/astrolabe/tests/unit/BackgroundJob/RefreshUserTokensTest.php index 62bd32a..3ebe654 100644 --- a/third_party/astrolabe/tests/unit/BackgroundJob/RefreshUserTokensTest.php +++ b/third_party/astrolabe/tests/unit/BackgroundJob/RefreshUserTokensTest.php @@ -8,6 +8,7 @@ use OCA\Astrolabe\BackgroundJob\RefreshUserTokens; use OCA\Astrolabe\Service\IdpTokenRefresher; use OCA\Astrolabe\Service\McpTokenStorage; use OCP\AppFramework\Utility\ITimeFactory; +use OCP\Lock\LockedException; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; @@ -40,6 +41,15 @@ final class RefreshUserTokensTest extends TestCase { ); } + /** + * Set up default withTokenLock behavior that executes the callback. + * Call this in tests that need the lock to succeed. + */ + private function setupDefaultLockBehavior(): void { + $this->tokenStorage->method('withTokenLock') + ->willReturnCallback(fn ($userId, $callback) => $callback()); + } + // ========================================================================= // Constructor Tests // ========================================================================= @@ -82,6 +92,8 @@ final class RefreshUserTokensTest extends TestCase { } public function testRunWithMultipleUsersAndMixedResults(): void { + $this->setupDefaultLockBehavior(); + $this->tokenStorage->method('getAllUsersWithTokens') ->willReturn(['alice', 'bob', 'charlie']); @@ -170,6 +182,8 @@ final class RefreshUserTokensTest extends TestCase { } public function testRefreshTriggeredWhenTokenNearExpiry(): void { + $this->setupDefaultLockBehavior(); + $now = time(); $this->tokenStorage->method('getUserToken') ->with('testuser') @@ -198,6 +212,8 @@ final class RefreshUserTokensTest extends TestCase { } public function testRefreshFailsWhenNoRefreshToken(): void { + $this->setupDefaultLockBehavior(); + $now = time(); $this->tokenStorage->method('getUserToken') ->with('testuser') @@ -221,6 +237,8 @@ final class RefreshUserTokensTest extends TestCase { } public function testRefreshFailsWhenRefresherReturnsNull(): void { + $this->setupDefaultLockBehavior(); + $now = time(); $this->tokenStorage->method('getUserToken') ->with('testuser') @@ -250,6 +268,8 @@ final class RefreshUserTokensTest extends TestCase { } public function testRefreshUsesIssuedAtForLifetimeCalculation(): void { + $this->setupDefaultLockBehavior(); + $now = time(); // Token with custom lifetime: issued 50 min ago, expires in 10 min (total 60 min) // 10/60 = 16.7% remaining, which is < 50%, so should refresh @@ -276,6 +296,8 @@ final class RefreshUserTokensTest extends TestCase { } public function testRefreshUsesDefaultLifetimeWhenNoIssuedAt(): void { + $this->setupDefaultLockBehavior(); + $now = time(); // Token without issued_at, uses default 3600s lifetime // 300s remaining / 3600s = 8.3% remaining, which is < 50%, so should refresh @@ -302,6 +324,8 @@ final class RefreshUserTokensTest extends TestCase { } public function testRefreshStoresNewTokenWithIssuedAt(): void { + $this->setupDefaultLockBehavior(); + $now = time(); $this->tokenStorage->method('getUserToken') ->with('testuser') @@ -337,6 +361,8 @@ final class RefreshUserTokensTest extends TestCase { } public function testRefreshKeepsOldRefreshTokenIfNotRotated(): void { + $this->setupDefaultLockBehavior(); + $now = time(); $this->tokenStorage->method('getUserToken') ->with('testuser') @@ -373,6 +399,8 @@ final class RefreshUserTokensTest extends TestCase { } public function testRefreshHandlesException(): void { + $this->setupDefaultLockBehavior(); + $now = time(); $this->tokenStorage->method('getUserToken') ->with('testuser') @@ -409,6 +437,123 @@ final class RefreshUserTokensTest extends TestCase { $this->assertEquals('skipped', $result); } + // ========================================================================= + // Locking Tests + // ========================================================================= + + public function testRefreshSkippedWhenLockCannotBeAcquired(): void { + $now = time(); + $this->tokenStorage->method('getUserToken') + ->with('testuser') + ->willReturn([ + 'access_token' => 'expiring-token', + 'refresh_token' => 'refresh-token', + 'expires_at' => $now + 100, // ~100s remaining (< 50% of default) + 'issued_at' => $now - 3500, + ]); + + // Lock acquisition fails (on-demand refresh is holding it) + $this->tokenStorage->expects($this->once()) + ->method('withTokenLock') + ->willThrowException(new LockedException('astrolabe/oauth/tokens/testuser')); + + // Token refresher should NOT be called when lock fails + $this->tokenRefresher->expects($this->never()) + ->method('refreshAccessToken'); + + $this->logger->expects($this->once()) + ->method('debug') + ->with($this->stringContains('Lock held for user testuser')); + + $result = $this->invokeRefreshUserTokenIfNeeded('testuser'); + + $this->assertEquals('skipped', $result); + } + + public function testRefreshUsesLockForTokenRefresh(): void { + $now = time(); + $this->tokenStorage->method('getUserToken') + ->with('testuser') + ->willReturn([ + 'access_token' => 'expiring-token', + 'refresh_token' => 'refresh-token', + 'expires_at' => $now + 100, + 'issued_at' => $now - 3500, + ]); + + // withTokenLock is called and executes the callback + $this->tokenStorage->expects($this->once()) + ->method('withTokenLock') + ->with('testuser', $this->isInstanceOf(\Closure::class)) + ->willReturnCallback(function ($userId, $callback) { + return $callback(); + }); + + $this->tokenRefresher->expects($this->once()) + ->method('refreshAccessToken') + ->with('refresh-token') + ->willReturn([ + 'access_token' => 'new-token', + 'refresh_token' => 'new-refresh-token', + 'expires_in' => 3600, + ]); + + $this->tokenStorage->expects($this->once()) + ->method('storeUserToken'); + + $result = $this->invokeRefreshUserTokenIfNeeded('testuser'); + + $this->assertEquals('refreshed', $result); + } + + public function testRefreshSkippedWhenTokenAlreadyRefreshedWhileWaitingForLock(): void { + $now = time(); + + // First call (before lock): token is expiring + // Calls inside lock callback: token is now fresh + $callCount = 0; + $this->tokenStorage->method('getUserToken') + ->with('testuser') + ->willReturnCallback(function () use (&$callCount, $now) { + $callCount++; + if ($callCount === 1) { + // First check: token is expiring + return [ + 'access_token' => 'expiring-token', + 'refresh_token' => 'refresh-token', + 'expires_at' => $now + 100, + 'issued_at' => $now - 3500, + ]; + } + // Inside lock: token was already refreshed + return [ + 'access_token' => 'already-refreshed-token', + 'refresh_token' => 'new-refresh-token', + 'expires_at' => $now + 3600, // Fresh token + 'issued_at' => $now, + ]; + }); + + // withTokenLock is called and executes the callback + $this->tokenStorage->expects($this->once()) + ->method('withTokenLock') + ->willReturnCallback(function ($userId, $callback) { + return $callback(); + }); + + // Token refresher should NOT be called since token is already fresh + $this->tokenRefresher->expects($this->never()) + ->method('refreshAccessToken'); + + $this->logger->expects($this->once()) + ->method('debug') + ->with($this->stringContains('already refreshed')); + + $result = $this->invokeRefreshUserTokenIfNeeded('testuser'); + + $this->assertEquals('skipped', $result); + } + // ========================================================================= // Helper Methods // ========================================================================= diff --git a/third_party/astrolabe/tests/unit/Service/McpTokenStorageTest.php b/third_party/astrolabe/tests/unit/Service/McpTokenStorageTest.php index 9688516..bb24924 100644 --- a/third_party/astrolabe/tests/unit/Service/McpTokenStorageTest.php +++ b/third_party/astrolabe/tests/unit/Service/McpTokenStorageTest.php @@ -10,6 +10,8 @@ use OCP\DB\QueryBuilder\IExpressionBuilder; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IConfig; use OCP\IDBConnection; +use OCP\Lock\ILockingProvider; +use OCP\Lock\LockedException; use OCP\Security\ICrypto; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; @@ -25,6 +27,7 @@ final class McpTokenStorageTest extends TestCase { private ICrypto&MockObject $crypto; private IDBConnection&MockObject $db; private LoggerInterface&MockObject $logger; + private ILockingProvider&MockObject $lockingProvider; private McpTokenStorage $storage; protected function setUp(): void { @@ -34,12 +37,14 @@ final class McpTokenStorageTest extends TestCase { $this->crypto = $this->createMock(ICrypto::class); $this->db = $this->createMock(IDBConnection::class); $this->logger = $this->createMock(LoggerInterface::class); + $this->lockingProvider = $this->createMock(ILockingProvider::class); $this->storage = new McpTokenStorage( $this->config, $this->crypto, $this->db, - $this->logger + $this->logger, + $this->lockingProvider ); } @@ -291,6 +296,155 @@ final class McpTokenStorageTest extends TestCase { $this->assertNull($result); } + // ========================================================================= + // Token Refresh Locking Tests + // ========================================================================= + + public function testGetAccessTokenAcquiresLockWhenRefreshing(): void { + $userId = 'testuser'; + $expiredTokenData = [ + 'access_token' => 'expired-access-token', + 'refresh_token' => 'old-refresh-token', + 'expires_at' => time() - 100, // Expired + ]; + + $newTokenData = [ + 'access_token' => 'new-access-token', + 'refresh_token' => 'new-refresh-token', + 'expires_in' => 3600, + ]; + + $this->config->method('getUserValue') + ->willReturn('encrypted-data'); + + $this->crypto->method('decrypt') + ->willReturn(json_encode($expiredTokenData)); + + $this->crypto->method('encrypt') + ->willReturn('new-encrypted-data'); + + // Verify lock is acquired and released + $this->lockingProvider->expects($this->once()) + ->method('acquireLock') + ->with('astrolabe/oauth/tokens/testuser', ILockingProvider::LOCK_EXCLUSIVE); + + $this->lockingProvider->expects($this->once()) + ->method('releaseLock') + ->with('astrolabe/oauth/tokens/testuser', ILockingProvider::LOCK_EXCLUSIVE); + + $refreshCallback = fn (string $refreshToken) => $newTokenData; + + $result = $this->storage->getAccessToken($userId, $refreshCallback); + + $this->assertEquals('new-access-token', $result); + } + + public function testGetAccessTokenReturnsStaleTokenOnLockedException(): void { + $userId = 'testuser'; + $expiredTokenData = [ + 'access_token' => 'expired-access-token', + 'refresh_token' => 'old-refresh-token', + 'expires_at' => time() - 100, // Expired + ]; + + $this->config->method('getUserValue') + ->willReturn('encrypted-data'); + + $this->crypto->method('decrypt') + ->willReturn(json_encode($expiredTokenData)); + + // Lock acquisition fails + $this->lockingProvider->expects($this->once()) + ->method('acquireLock') + ->willThrowException(new LockedException('astrolabe/oauth/tokens/testuser')); + + // Refresh callback should NOT be called when lock fails + $refreshCallbackCalled = false; + $refreshCallback = function (string $refreshToken) use (&$refreshCallbackCalled) { + $refreshCallbackCalled = true; + return ['access_token' => 'new-token', 'expires_in' => 3600]; + }; + + $result = $this->storage->getAccessToken($userId, $refreshCallback); + + // Should return stale token instead of failing + $this->assertEquals('expired-access-token', $result); + $this->assertFalse($refreshCallbackCalled); + } + + public function testGetAccessTokenSkipsRefreshWhenTokenAlreadyRefreshedWhileWaitingForLock(): void { + $userId = 'testuser'; + $expiredTokenData = [ + 'access_token' => 'expired-access-token', + 'refresh_token' => 'old-refresh-token', + 'expires_at' => time() - 100, // Expired + ]; + + // After lock is acquired, token appears fresh (another process refreshed it) + $freshTokenData = [ + 'access_token' => 'fresh-access-token', + 'refresh_token' => 'fresh-refresh-token', + 'expires_at' => time() + 3600, // Valid for 1 hour + ]; + + $callCount = 0; + $this->config->method('getUserValue') + ->willReturn('encrypted-data'); + + // First call returns expired, subsequent calls return fresh + $this->crypto->method('decrypt') + ->willReturnCallback(function () use (&$callCount, $expiredTokenData, $freshTokenData) { + $callCount++; + return $callCount === 1 + ? json_encode($expiredTokenData) + : json_encode($freshTokenData); + }); + + $this->lockingProvider->expects($this->once()) + ->method('acquireLock'); + + $this->lockingProvider->expects($this->once()) + ->method('releaseLock'); + + // Refresh callback should NOT be called since token is already fresh + $refreshCallbackCalled = false; + $refreshCallback = function (string $refreshToken) use (&$refreshCallbackCalled) { + $refreshCallbackCalled = true; + return ['access_token' => 'new-token', 'expires_in' => 3600]; + }; + + $result = $this->storage->getAccessToken($userId, $refreshCallback); + + $this->assertEquals('fresh-access-token', $result); + $this->assertFalse($refreshCallbackCalled); + } + + public function testGetAccessTokenNoLockRequiredWhenNotExpired(): void { + $userId = 'testuser'; + $validTokenData = [ + 'access_token' => 'valid-access-token', + 'refresh_token' => 'refresh-token', + 'expires_at' => time() + 3600, // Valid for 1 hour + ]; + + $this->config->method('getUserValue') + ->willReturn('encrypted-data'); + + $this->crypto->method('decrypt') + ->willReturn(json_encode($validTokenData)); + + // Lock should NOT be acquired for valid tokens + $this->lockingProvider->expects($this->never()) + ->method('acquireLock'); + + $this->lockingProvider->expects($this->never()) + ->method('releaseLock'); + + $result = $this->storage->getAccessToken($userId); + + $this->assertEquals('valid-access-token', $result); + } + // ========================================================================= // App Password Storage Tests (Multi-User Basic Auth) // ========================================================================= From 9491d698e832c2f372852419fb9cb9ec9f911763 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 28 Jan 2026 08:13:22 +0100 Subject: [PATCH 5/6] fix(astrolabe): add pagination and psalm fixes for token refresh - Add pagination to getAllUsersWithTokens() with limit/offset params - Update RefreshUserTokens to process users in batches of 100 - Add lock TTL documentation to withTokenLock() docstring - Fix psalm type errors in getAccessToken() method - Add unit tests for pagination and batched processing Co-Authored-By: Claude Opus 4.5 --- .../lib/BackgroundJob/RefreshUserTokens.php | 35 ++++++--- .../astrolabe/lib/Service/McpTokenStorage.php | 29 +++++-- .../BackgroundJob/RefreshUserTokensTest.php | 63 ++++++++++++++- .../unit/Service/McpTokenStorageTest.php | 76 +++++++++++++++++++ 4 files changed, 182 insertions(+), 21 deletions(-) diff --git a/third_party/astrolabe/lib/BackgroundJob/RefreshUserTokens.php b/third_party/astrolabe/lib/BackgroundJob/RefreshUserTokens.php index a74ec65..7c3c3a0 100644 --- a/third_party/astrolabe/lib/BackgroundJob/RefreshUserTokens.php +++ b/third_party/astrolabe/lib/BackgroundJob/RefreshUserTokens.php @@ -38,6 +38,9 @@ class RefreshUserTokens extends TimedJob { /** Default assumed token lifetime if we can't determine it (1 hour) */ private const DEFAULT_TOKEN_LIFETIME_SECONDS = 3600; + /** Batch size for processing users (prevents memory issues on large installations) */ + private const BATCH_SIZE = 100; + public function __construct( ITimeFactory $time, private McpTokenStorage $tokenStorage, @@ -52,23 +55,31 @@ class RefreshUserTokens extends TimedJob { protected function run(mixed $argument): void { $this->logger->info('RefreshUserTokens: Starting background token refresh'); - $userIds = $this->tokenStorage->getAllUsersWithTokens(); - $this->logger->debug('RefreshUserTokens: Found ' . count($userIds) . ' users with tokens'); - $refreshed = 0; $failed = 0; $skipped = 0; + $offset = 0; + $totalUsers = 0; - foreach ($userIds as $userId) { - $result = $this->refreshUserTokenIfNeeded($userId); - match ($result) { - 'refreshed' => $refreshed++, - 'failed' => $failed++, - 'skipped' => $skipped++, - }; - } + // Process users in batches to prevent memory issues on large installations + do { + $userIds = $this->tokenStorage->getAllUsersWithTokens(self::BATCH_SIZE, $offset); + $batchCount = count($userIds); + $totalUsers += $batchCount; - $this->logger->info("RefreshUserTokens: Complete - refreshed=$refreshed, failed=$failed, skipped=$skipped"); + foreach ($userIds as $userId) { + $result = $this->refreshUserTokenIfNeeded($userId); + match ($result) { + 'refreshed' => $refreshed++, + 'failed' => $failed++, + 'skipped' => $skipped++, + }; + } + + $offset += self::BATCH_SIZE; + } while ($batchCount === self::BATCH_SIZE); + + $this->logger->info("RefreshUserTokens: Complete - total=$totalUsers, refreshed=$refreshed, failed=$failed, skipped=$skipped"); } /** diff --git a/third_party/astrolabe/lib/Service/McpTokenStorage.php b/third_party/astrolabe/lib/Service/McpTokenStorage.php index 6ceefc6..15d0973 100644 --- a/third_party/astrolabe/lib/Service/McpTokenStorage.php +++ b/third_party/astrolabe/lib/Service/McpTokenStorage.php @@ -156,6 +156,10 @@ class McpTokenStorage { * * Prevents race conditions between background job and on-demand token refresh. * + * Note: Lock TTL is configured at the Nextcloud server level (default: 3600s). + * If a process crashes while holding the lock, it will auto-expire after the TTL. + * The ILockingProvider interface does not support per-call timeouts. + * * @template T * @param string $userId User ID * @param callable(): T $callback @@ -198,20 +202,29 @@ class McpTokenStorage { } /** - * Get all user IDs that have OAuth tokens stored. + * Get user IDs that have OAuth tokens stored. * * Queries oc_preferences directly since IConfig doesn't support * listing all users with a specific key set. * + * @param int $limit Maximum users to return (0 = no limit, for backward compatibility) + * @param int $offset Starting offset for pagination * @return list Array of user IDs */ - public function getAllUsersWithTokens(): array { + public function getAllUsersWithTokens(int $limit = 0, int $offset = 0): array { $qb = $this->db->getQueryBuilder(); $qb->select('userid') ->from('preferences') ->where($qb->expr()->eq('appid', $qb->createNamedParameter('astrolabe'))) ->andWhere($qb->expr()->eq('configkey', $qb->createNamedParameter('oauth_tokens'))); + if ($limit > 0) { + $qb->setMaxResults($limit); + } + if ($offset > 0) { + $qb->setFirstResult($offset); + } + $result = $qb->executeQuery(); /** @var list $userIds */ $userIds = []; @@ -255,19 +268,23 @@ class McpTokenStorage { // Token expired - acquire lock for refresh try { - /** @var string|null */ + /** + * @return string|null + * @psalm-suppress MixedInferredReturnType + */ return $this->withTokenLock($userId, function () use ($userId, $refreshCallback): ?string { // Re-check after acquiring lock (double-check pattern) // Another process may have refreshed while we waited for the lock $currentToken = $this->getUserToken($userId); - if (!$currentToken) { + if ($currentToken === null) { return null; } // Check if another process already refreshed the token if (!$this->isExpired($currentToken)) { $this->logger->debug("Token already refreshed for user $userId while waiting for lock"); + /** @var string */ return $currentToken['access_token']; } @@ -322,7 +339,9 @@ class McpTokenStorage { // Could not acquire lock - another process is refreshing // Return stale token rather than failing - caller can retry if needed $this->logger->warning("Could not acquire token lock for user $userId, returning stale token"); - return $token['access_token'] ?? null; + /** @var string|null $staleToken */ + $staleToken = $token['access_token'] ?? null; + return $staleToken; } } diff --git a/third_party/astrolabe/tests/unit/BackgroundJob/RefreshUserTokensTest.php b/third_party/astrolabe/tests/unit/BackgroundJob/RefreshUserTokensTest.php index 3ebe654..d467841 100644 --- a/third_party/astrolabe/tests/unit/BackgroundJob/RefreshUserTokensTest.php +++ b/third_party/astrolabe/tests/unit/BackgroundJob/RefreshUserTokensTest.php @@ -79,14 +79,11 @@ final class RefreshUserTokensTest extends TestCase { if ($callCount === 1) { $this->assertStringContainsString('Starting', $message); } else { + $this->assertStringContainsString('total=0', $message); $this->assertStringContainsString('refreshed=0, failed=0, skipped=0', $message); } }); - $this->logger->expects($this->once()) - ->method('debug') - ->with($this->stringContains('Found 0 users')); - // Call run() via reflection since it's protected $this->invokeRun(); } @@ -151,6 +148,7 @@ final class RefreshUserTokensTest extends TestCase { static $callCount = 0; $callCount++; if ($callCount === 2) { + $this->assertStringContainsString('total=3', $message); $this->assertStringContainsString('refreshed=1, failed=1, skipped=1', $message); } }); @@ -158,6 +156,63 @@ final class RefreshUserTokensTest extends TestCase { $this->invokeRun(); } + public function testRunProcessesUsersInBatches(): void { + $this->setupDefaultLockBehavior(); + + // Simulate 150 users processed in 2 batches (100 + 50) + $batch1 = array_map(fn ($i) => "user{$i}", range(1, 100)); + $batch2 = array_map(fn ($i) => "user{$i}", range(101, 150)); + + $callCount = 0; + $this->tokenStorage->method('getAllUsersWithTokens') + ->willReturnCallback(function (int $limit, int $offset) use (&$callCount, $batch1, $batch2) { + $callCount++; + // First call: offset 0, return 100 users (full batch) + if ($offset === 0) { + $this->assertEquals(100, $limit); + return $batch1; + } + // Second call: offset 100, return 50 users (partial batch = last) + if ($offset === 100) { + $this->assertEquals(100, $limit); + return $batch2; + } + // Should not be called again + $this->fail("Unexpected getAllUsersWithTokens call with offset $offset"); + }); + + // All tokens have plenty of time (all skipped) + $this->tokenStorage->method('getUserToken') + ->willReturnCallback(function (string $userId) { + $now = time(); + return [ + 'access_token' => "{$userId}-token", + 'refresh_token' => "{$userId}-refresh", + 'expires_at' => $now + 3600, + 'issued_at' => $now, + ]; + }); + + $this->tokenRefresher->expects($this->never()) + ->method('refreshAccessToken'); + + $this->logger->expects($this->exactly(2)) + ->method('info') + ->willReturnCallback(function (string $message) { + static $infoCallCount = 0; + $infoCallCount++; + if ($infoCallCount === 2) { + $this->assertStringContainsString('total=150', $message); + $this->assertStringContainsString('refreshed=0, failed=0, skipped=150', $message); + } + }); + + $this->invokeRun(); + + // Verify getAllUsersWithTokens was called exactly twice (2 batches) + $this->assertEquals(2, $callCount); + } + // ========================================================================= // refreshUserTokenIfNeeded() Tests // ========================================================================= diff --git a/third_party/astrolabe/tests/unit/Service/McpTokenStorageTest.php b/third_party/astrolabe/tests/unit/Service/McpTokenStorageTest.php index bb24924..d2bc761 100644 --- a/third_party/astrolabe/tests/unit/Service/McpTokenStorageTest.php +++ b/third_party/astrolabe/tests/unit/Service/McpTokenStorageTest.php @@ -750,4 +750,80 @@ final class McpTokenStorageTest extends TestCase { $this->assertEquals([], $userIds); } + + public function testGetAllUsersWithTokensWithLimitAndOffset(): void { + $qb = $this->createMock(IQueryBuilder::class); + $expr = $this->createMock(IExpressionBuilder::class); + $result = $this->createMock(IResult::class); + + // Chain builder methods + $qb->method('select')->willReturnSelf(); + $qb->method('from')->willReturnSelf(); + $qb->method('where')->willReturnSelf(); + $qb->method('andWhere')->willReturnSelf(); + $qb->method('expr')->willReturn($expr); + $qb->method('createNamedParameter')->willReturnArgument(0); + $qb->method('executeQuery')->willReturn($result); + + // Verify setMaxResults and setFirstResult are called with correct values + $qb->expects($this->once()) + ->method('setMaxResults') + ->with(50) + ->willReturnSelf(); + $qb->expects($this->once()) + ->method('setFirstResult') + ->with(100) + ->willReturnSelf(); + + // Mock expression builder + $expr->method('eq')->willReturn('mocked_condition'); + + // Mock result set + $result->method('fetch')->willReturnOnConsecutiveCalls( + ['userid' => 'user1'], + ['userid' => 'user2'], + false + ); + $result->expects($this->once())->method('closeCursor'); + + $this->db->method('getQueryBuilder')->willReturn($qb); + + $userIds = $this->storage->getAllUsersWithTokens(50, 100); + + $this->assertEquals(['user1', 'user2'], $userIds); + } + + public function testGetAllUsersWithTokensWithZeroLimitDoesNotSetMaxResults(): void { + $qb = $this->createMock(IQueryBuilder::class); + $expr = $this->createMock(IExpressionBuilder::class); + $result = $this->createMock(IResult::class); + + // Chain builder methods + $qb->method('select')->willReturnSelf(); + $qb->method('from')->willReturnSelf(); + $qb->method('where')->willReturnSelf(); + $qb->method('andWhere')->willReturnSelf(); + $qb->method('expr')->willReturn($expr); + $qb->method('createNamedParameter')->willReturnArgument(0); + $qb->method('executeQuery')->willReturn($result); + + // setMaxResults should NOT be called when limit is 0 + $qb->expects($this->never()) + ->method('setMaxResults'); + + // setFirstResult should NOT be called when offset is 0 + $qb->expects($this->never()) + ->method('setFirstResult'); + + // Mock expression builder + $expr->method('eq')->willReturn('mocked_condition'); + + // Mock result set + $result->method('fetch')->willReturn(false); + $result->expects($this->once())->method('closeCursor'); + + $this->db->method('getQueryBuilder')->willReturn($qb); + + $this->storage->getAllUsersWithTokens(0, 0); + } } From c7882adb24d9c952bc210a9d7edf187dd84b2293 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 28 Jan 2026 08:38:29 +0100 Subject: [PATCH 6/6] docs: add authentication flows reference by deployment mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Create unified documentation covering authentication flows across all five deployment modes. Documents three communication patterns (MCP Client → MCP Server → Nextcloud, background sync, Astrolabe → MCP Server) with ASCII sequence diagrams and implementation references. Co-Authored-By: Claude Opus 4.5 --- docs/auth-flows.md | 461 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 461 insertions(+) create mode 100644 docs/auth-flows.md diff --git a/docs/auth-flows.md b/docs/auth-flows.md new file mode 100644 index 0000000..051bd8d --- /dev/null +++ b/docs/auth-flows.md @@ -0,0 +1,461 @@ +# Authentication Flows by Deployment Mode + +This document provides a unified reference for authentication flows across all deployment modes. For configuration details, see [Authentication](authentication.md). For OAuth protocol details, see [OAuth Architecture](oauth-architecture.md). + +## Quick Reference Matrix + +| Mode | Client → MCP → NC | Background Sync | Astrolabe → MCP | +|------|-------------------|-----------------|-----------------| +| [Single-User BasicAuth](#1-single-user-basicauth) | Embedded credentials | Same credentials | N/A | +| [Multi-User BasicAuth](#2-multi-user-basicauth) | Header pass-through | App password (optional) | Bearer token | +| [OAuth Single-Audience](#3-oauth-single-audience-default) | Multi-audience token | Refresh token exchange | Bearer token | +| [OAuth Token Exchange](#4-oauth-token-exchange-rfc-8693) | RFC 8693 exchange | Refresh token exchange | Bearer token | +| [Smithery Stateless](#5-smithery-stateless) | Session parameters | Not supported | N/A | + +## Communication Patterns + +This document covers three distinct communication patterns: + +1. **MCP Client → MCP Server → Nextcloud**: Interactive tool calls initiated by users through MCP clients (Claude Desktop, etc.) +2. **MCP Server → Nextcloud**: Background operations like vector sync that run without user interaction +3. **Astrolabe → MCP Server**: Nextcloud app backend communication for settings UI and unified search + +--- + +## Deployment Modes + +### 1. Single-User BasicAuth + +**Use Case:** Personal Nextcloud instance, local development, single-user deployments. + +#### MCP Client → MCP Server → Nextcloud + +``` +MCP Client MCP Server Nextcloud + │ │ │ + │── MCP Request ─────────────▶│ │ + │ (no auth required) │ │ + │ │── HTTP + BasicAuth ───────▶│ + │ │ Authorization: Basic │ + │ │ (embedded credentials) │ + │ │◀── API Response ───────────│ + │◀── Tool Result ─────────────│ │ +``` + +**Key characteristics:** +- Credentials embedded in server configuration (`NEXTCLOUD_USERNAME`, `NEXTCLOUD_PASSWORD`) +- Single shared `NextcloudClient` created at startup +- No MCP-level authentication required (server trusts local clients) +- All requests use the same Nextcloud user + +**Implementation:** `context.py:78-79` - Returns shared client from lifespan context + +#### Background Sync + +Uses the same embedded credentials as interactive requests. The background job accesses Nextcloud with the configured username/password. + +**Implementation:** Background jobs use `get_settings()` to access credentials + +#### Astrolabe Integration + +Not applicable - Astrolabe is only used in multi-user deployments where users need personal settings and token management. + +--- + +### 2. Multi-User BasicAuth + +**Use Case:** Internal deployment where users provide their own credentials via HTTP headers. + +#### MCP Client → MCP Server → Nextcloud + +``` +MCP Client MCP Server Nextcloud + │ │ │ + │── MCP Request ─────────────▶│ │ + │ Authorization: Basic │ │ + │ (user credentials) │ │ + │ │── BasicAuthMiddleware ────▶│ + │ │ Extracts credentials │ + │ │ │ + │ │── HTTP + BasicAuth ───────▶│ + │ │ (pass-through) │ + │ │◀── API Response ───────────│ + │◀── Tool Result ─────────────│ │ +``` + +**Key characteristics:** +- `BasicAuthMiddleware` extracts credentials from `Authorization: Basic` header +- Credentials passed through to Nextcloud (not stored) +- Client created per-request from extracted credentials +- Stateless - no credential storage between requests + +**Implementation:** `context.py:187-248` - `_get_client_from_basic_auth()` extracts credentials from request state + +#### Background Sync (Optional) + +Requires `ENABLE_OFFLINE_ACCESS=true`. Users can store app passwords via Astrolabe for background operations. + +``` +Astrolabe MCP Server Nextcloud + │ │ │ + │── Store App Password ──────▶│ │ + │ (via management API) │ │ + │ │── Store in SQLite ────────▶│ + │ │ (encrypted) │ + │◀── Confirmation ────────────│ │ + │ │ │ + │ [Background Job] │ │ + │ │── Retrieve app password ──▶│ + │ │ (from encrypted storage) │ + │ │── HTTP + BasicAuth ───────▶│ + │ │ (stored app password) │ + │ │◀── API Response ───────────│ +``` + +**Requirements:** +- `ENABLE_OFFLINE_ACCESS=true` +- `TOKEN_ENCRYPTION_KEY` for credential encryption +- `TOKEN_STORAGE_DB` for SQLite storage path + +#### Astrolabe → MCP Server + +``` +Astrolabe MCP Server Nextcloud OIDC + │ │ │ + │── OAuth Flow ──────────────▶│◀── Token from IdP ────────▶│ + │ (user initiates) │ │ + │ │ │ + │── Bearer Token ────────────▶│ │ + │ (management API calls) │ │ + │ │── Validate via JWKS ──────▶│ + │ │ (or introspection) │ + │◀── API Response ────────────│ │ +``` + +**Key characteristics:** +- Astrolabe has its own OAuth client (`astrolabe_client_id` in Nextcloud config) +- Tokens are validated by MCP server using Nextcloud OIDC JWKS +- Authorization check: `token.sub == requested_resource_owner` +- Any valid Nextcloud OIDC token accepted (relaxed audience validation per ADR-018) + +**Implementation:** `unified_verifier.py:120-183` - `verify_token_for_management_api()` validates without strict audience check + +--- + +### 3. OAuth Single-Audience (Default) + +**Use Case:** Multi-user deployment with OAuth authentication. Tokens work for both MCP and Nextcloud. + +This is the default mode when `NEXTCLOUD_USERNAME`/`NEXTCLOUD_PASSWORD` are not set. + +#### MCP Client → MCP Server → Nextcloud + +``` +MCP Client MCP Server Nextcloud + │ │ │ + │── Bearer Token ────────────▶│ │ + │ aud: ["mcp-server", │ │ + │ "nextcloud"] │ │ + │ │── Validate MCP audience ──▶│ + │ │ (UnifiedTokenVerifier) │ + │ │ │ + │ │── HTTP + Same Token ──────▶│ + │ │ Authorization: Bearer │ + │ │ (multi-audience token) │ + │ │ │ + │ │ NC validates its own aud │ + │ │◀── API Response ───────────│ + │◀── Tool Result ─────────────│ │ +``` + +**Key characteristics:** +- Token contains both audiences: `aud: ["mcp-server", "nextcloud"]` +- MCP server validates only MCP audience (per RFC 7519) +- Nextcloud independently validates its own audience +- No token exchange needed - same token used throughout +- Stateless operation for interactive requests + +**Token validation flow:** +1. `UnifiedTokenVerifier.verify_token()` validates MCP audience +2. Token passed directly to Nextcloud via `get_client_from_context()` +3. Nextcloud validates its own audience when receiving API calls + +**Implementation:** +- `unified_verifier.py:185-252` - `_verify_mcp_audience()` validates MCP audience only +- `context.py:96-99` - Uses token directly in multi-audience mode + +#### Background Sync + +Requires `ENABLE_OFFLINE_ACCESS=true`. Uses stored refresh tokens to obtain access tokens for background operations. + +``` + MCP Server Nextcloud OIDC + │ │ + [Background Job starts] │ │ + │── Get refresh token ──────▶│ + │ (from encrypted storage) │ + │ │ + │── Token refresh request ──▶│ + │ grant_type=refresh_token │ + │ scope=openid profile ... │ + │◀── New access + refresh ───│ + │ (rotation) │ + │ │ + │── Store rotated refresh ──▶│ + │ (encrypted) │ + │ │ + │── HTTP + Access Token ────▶│ + │ Authorization: Bearer │ + │◀── API Response ───────────│ +``` + +**Key characteristics:** +- Refresh tokens stored encrypted in SQLite (`TOKEN_STORAGE_DB`) +- Nextcloud OIDC rotates refresh tokens on every use (one-time use) +- `TokenBrokerService` handles token lifecycle +- Per-user locking prevents race conditions during concurrent refresh + +**Implementation:** +- `token_broker.py:269-362` - `get_background_token()` handles refresh with locking +- `token_broker.py:428-509` - `_refresh_access_token_with_scopes()` exchanges refresh token + +#### Astrolabe → MCP Server + +Same as Multi-User BasicAuth. See [Astrolabe → MCP Server](#astrolabe--mcp-server) above. + +--- + +### 4. OAuth Token Exchange (RFC 8693) + +**Use Case:** Multi-user deployment where MCP tokens are separate from Nextcloud tokens. Provides stronger security boundaries. + +Enabled by `ENABLE_TOKEN_EXCHANGE=true`. + +#### MCP Client → MCP Server → Nextcloud + +``` +MCP Client MCP Server Nextcloud OIDC + │ │ │ + │── Bearer Token ────────────▶│ │ + │ aud: "mcp-server" │ │ + │ (MCP audience only) │ │ + │ │── Validate MCP audience ──▶│ + │ │ │ + │ │── RFC 8693 Exchange ──────▶│ + │ │ grant_type= │ + │ │ urn:ietf:params:oauth: │ + │ │ grant-type:token-exchange + │ │ subject_token=│ + │ │ requested_audience= │ + │ │ "nextcloud" │ + │ │◀── Delegated Token ────────│ + │ │ aud: "nextcloud" │ + │ │ │ + │ │── HTTP + Delegated Token ─▶│ + │ │ Authorization: Bearer │ + │ │◀── API Response ───────────│ + │◀── Tool Result ─────────────│ │ +``` + +**Key characteristics:** +- Strict audience separation: MCP token has `aud: "mcp-server"` only +- Server exchanges for Nextcloud-audience token on each request +- Ephemeral delegated tokens (not cached by default) +- Strongest security boundary between MCP and Nextcloud access + +**Token exchange details:** +- Uses RFC 8693 "urn:ietf:params:oauth:grant-type:token-exchange" +- Subject token: MCP access token +- Requested audience: Nextcloud resource URI +- Result: Short-lived token scoped for Nextcloud + +**Implementation:** +- `token_broker.py:220-267` - `get_session_token()` performs on-demand exchange +- `token_exchange.py` - `exchange_token_for_delegation()` implements RFC 8693 +- `context.py:88-94` - Routes to session client in exchange mode + +#### Background Sync + +Same as OAuth Single-Audience. Uses stored refresh tokens from Flow 2 provisioning. + +``` + MCP Server Nextcloud OIDC + │ │ + [User provisions access] │ │ + │── Flow 2 OAuth ───────────▶│ + │ client_id="mcp-server" │ + │ scope=offline_access ... │ + │◀── Refresh Token ──────────│ + │ (stored encrypted) │ + │ │ + [Background Job runs later] │ │ + │── Refresh for background ─▶│ + │ (same as single-audience)│ +``` + +**Key difference from interactive:** +- Interactive: On-demand token exchange per request +- Background: Uses pre-provisioned refresh tokens (Flow 2) + +#### Astrolabe → MCP Server + +Same as Multi-User BasicAuth. See [Astrolabe → MCP Server](#astrolabe--mcp-server) above. + +--- + +### 5. Smithery Stateless + +**Use Case:** Multi-tenant SaaS deployment via Smithery platform. Fully stateless. + +Enabled by `SMITHERY_DEPLOYMENT=true`. + +#### MCP Client → MCP Server → Nextcloud + +``` +MCP Client MCP Server Nextcloud + │ │ │ + │── SSE Connect ─────────────▶│ │ + │ ?nextcloud_url=... │ │ + │ &username=... │ │ + │ &app_password=... │ │ + │ │── SmitheryConfigMiddleware │ + │ │ Extract URL params │ + │ │ │ + │── MCP Request ─────────────▶│ │ + │ (no Authorization header) │ │ + │ │── Create per-request ─────▶│ + │ │ NextcloudClient │ + │ │ │ + │ │── HTTP + BasicAuth ───────▶│ + │ │ (from session params) │ + │ │◀── API Response ───────────│ + │◀── Tool Result ─────────────│ │ +``` + +**Key characteristics:** +- Configuration passed via URL query parameters (Smithery `configSchema`) +- No persistent state - client created fresh per request +- No OAuth infrastructure +- No background sync support (stateless) +- No admin UI available + +**Required session parameters:** +- `nextcloud_url`: Nextcloud instance URL +- `username`: Nextcloud username +- `app_password`: Nextcloud app password + +**Implementation:** `context.py:108-184` - `_get_client_from_session_config()` creates client from session params + +#### Background Sync + +Not supported. Smithery mode is fully stateless with no credential storage. + +#### Astrolabe Integration + +Not applicable. Smithery deployments don't integrate with Astrolabe. + +--- + +## Astrolabe Background Token Refresh + +The Astrolabe Nextcloud app includes a background job that proactively refreshes OAuth tokens before expiration. + +``` +Nextcloud Cron Astrolabe MCP Server IdP + │ │ │ + │── Run RefreshUserTokens ───▶│ │ + │ (every 15 minutes) │ │ + │ │── Get all user tokens ────▶│ + │ │ (from preferences) │ + │ │ │ + │ [For each user] │ │ + │ │── Check expiry ───────────▶│ + │ │ refresh if <50% lifetime │ + │ │ │ + │ │── Acquire user lock ──────▶│ + │ │ (prevent race condition) │ + │ │ │ + │ │── Token refresh request ──▶│ + │ │ grant_type=refresh_token │ + │ │◀── New tokens ─────────────│ + │ │ │ + │ │── Store new tokens ───────▶│ + │ │ (with issued_at) │ + │◀── Job complete ────────────│ │ +``` + +**Key characteristics:** +- Runs every 15 minutes via Nextcloud cron +- Refreshes when <50% of token lifetime remains +- Uses locking to prevent race conditions with on-demand refresh +- Stores `issued_at` timestamp for accurate lifetime calculation +- Batch processing (100 users at a time) for memory efficiency + +**Implementation:** `third_party/astrolabe/lib/BackgroundJob/RefreshUserTokens.php` + +--- + +## Configuration Quick Reference + +### Single-User BasicAuth +```bash +NEXTCLOUD_HOST=http://localhost:8080 +NEXTCLOUD_USERNAME=admin +NEXTCLOUD_PASSWORD=password +``` + +### Multi-User BasicAuth +```bash +NEXTCLOUD_HOST=http://nextcloud.example.com +ENABLE_MULTI_USER_BASIC_AUTH=true + +# Optional: For background sync +ENABLE_OFFLINE_ACCESS=true +TOKEN_ENCRYPTION_KEY=<32-byte-key> +TOKEN_STORAGE_DB=/data/tokens.db +``` + +### OAuth Single-Audience (Default) +```bash +NEXTCLOUD_HOST=http://nextcloud.example.com +# No username/password triggers OAuth mode + +# Optional: Static client credentials (instead of DCR) +NEXTCLOUD_OIDC_CLIENT_ID= +NEXTCLOUD_OIDC_CLIENT_SECRET= + +# Optional: For background sync +ENABLE_OFFLINE_ACCESS=true +TOKEN_ENCRYPTION_KEY=<32-byte-key> +TOKEN_STORAGE_DB=/data/tokens.db +``` + +### OAuth Token Exchange +```bash +NEXTCLOUD_HOST=http://nextcloud.example.com +ENABLE_TOKEN_EXCHANGE=true +NEXTCLOUD_OIDC_CLIENT_ID= +NEXTCLOUD_OIDC_CLIENT_SECRET= + +# Optional: For background sync +ENABLE_OFFLINE_ACCESS=true +TOKEN_ENCRYPTION_KEY=<32-byte-key> +TOKEN_STORAGE_DB=/data/tokens.db +``` + +### Smithery Stateless +```bash +SMITHERY_DEPLOYMENT=true +# All other config comes from session URL parameters +``` + +--- + +## Related Documentation + +- [Authentication](authentication.md) - Configuration details and setup guides +- [OAuth Architecture](oauth-architecture.md) - Deep OAuth protocol details +- [ADR-004: Progressive Consent](ADR-004-mcp-application-oauth.md) - Dual OAuth flow architecture +- [ADR-005: Token Audience Validation](ADR-005-token-audience-validation.md) - Audience validation strategy +- [ADR-018: Nextcloud PHP App](ADR-018-nextcloud-php-app-for-settings-ui.md) - Astrolabe integration +- [ADR-020: Deployment Modes](ADR-020-deployment-modes-and-configuration-validation.md) - Mode detection and validation