From fe49bebdb21824744fec0d0069026a707c586e92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Thu, 31 Oct 2024 15:53:36 +0100 Subject: [PATCH 1/6] Introduce ProtocolCache to repositories --- .../AbstractDatabaseRepository.php | 15 +++++------ src/Repositories/AccessTokenRepository.php | 6 ++++- src/Repositories/AuthCodeRepository.php | 6 ++++- src/Repositories/ClientRepository.php | 8 ++++-- src/Repositories/RefreshTokenRepository.php | 6 ++++- src/Repositories/ScopeRepository.php | 10 ++----- src/Repositories/UserRepository.php | 6 ++++- src/Services/Container.php | 27 +++++++++++++++---- .../RevokeTokenByAuthCodeIdTraitTest.php | 13 ++++++++- .../AccessTokenRepositoryTest.php | 5 ++++ .../AllowedOriginRepositoryTest.php | 8 +++++- .../Repositories/AuthCodeRepositoryTest.php | 5 ++++ .../src/Repositories/ClientRepositoryTest.php | 5 ++++ .../RefreshTokenRepositoryTest.php | 5 ++++ .../src/Repositories/UserRepositoryTest.php | 5 ++++ 15 files changed, 100 insertions(+), 30 deletions(-) diff --git a/src/Repositories/AbstractDatabaseRepository.php b/src/Repositories/AbstractDatabaseRepository.php index 832b5f05..61d8b416 100644 --- a/src/Repositories/AbstractDatabaseRepository.php +++ b/src/Repositories/AbstractDatabaseRepository.php @@ -15,24 +15,21 @@ */ namespace SimpleSAML\Module\oidc\Repositories; -use SimpleSAML\Configuration; use SimpleSAML\Database; use SimpleSAML\Module\oidc\ModuleConfig; +use SimpleSAML\Module\oidc\Utils\ProtocolCache; abstract class AbstractDatabaseRepository { - protected Configuration $config; - - protected Database $database; - /** * ClientRepository constructor. * @throws \Exception */ - public function __construct(protected ModuleConfig $moduleConfig) - { - $this->config = $this->moduleConfig->config(); - $this->database = Database::getInstance(); + public function __construct( + protected readonly ModuleConfig $moduleConfig, + protected readonly Database $database, + protected readonly ?ProtocolCache $protocolCache, + ) { } abstract public function getTableName(): ?string; diff --git a/src/Repositories/AccessTokenRepository.php b/src/Repositories/AccessTokenRepository.php index 357969b9..3e1ac577 100644 --- a/src/Repositories/AccessTokenRepository.php +++ b/src/Repositories/AccessTokenRepository.php @@ -20,6 +20,7 @@ use League\OAuth2\Server\Entities\AccessTokenEntityInterface as OAuth2AccessTokenEntityInterface; use League\OAuth2\Server\Entities\ClientEntityInterface as OAuth2ClientEntityInterface; use RuntimeException; +use SimpleSAML\Database; use SimpleSAML\Error\Error; use SimpleSAML\Module\oidc\Codebooks\DateFormatsEnum; use SimpleSAML\Module\oidc\Entities\AccessTokenEntity; @@ -30,6 +31,7 @@ use SimpleSAML\Module\oidc\Repositories\Interfaces\AccessTokenRepositoryInterface; use SimpleSAML\Module\oidc\Repositories\Traits\RevokeTokenByAuthCodeIdTrait; use SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException; +use SimpleSAML\Module\oidc\Utils\ProtocolCache; class AccessTokenRepository extends AbstractDatabaseRepository implements AccessTokenRepositoryInterface { @@ -39,11 +41,13 @@ class AccessTokenRepository extends AbstractDatabaseRepository implements Access public function __construct( ModuleConfig $moduleConfig, + Database $database, + ?ProtocolCache $protocolCache, protected readonly ClientRepository $clientRepository, protected readonly AccessTokenEntityFactory $accessTokenEntityFactory, protected readonly Helpers $helpers, ) { - parent::__construct($moduleConfig); + parent::__construct($moduleConfig, $database, $protocolCache); } public function getTableName(): string diff --git a/src/Repositories/AuthCodeRepository.php b/src/Repositories/AuthCodeRepository.php index d737d6af..f1b95fba 100644 --- a/src/Repositories/AuthCodeRepository.php +++ b/src/Repositories/AuthCodeRepository.php @@ -18,6 +18,7 @@ use League\OAuth2\Server\Entities\AuthCodeEntityInterface as OAuth2AuthCodeEntityInterface; use RuntimeException; +use SimpleSAML\Database; use SimpleSAML\Error\Error; use SimpleSAML\Module\oidc\Codebooks\DateFormatsEnum; use SimpleSAML\Module\oidc\Entities\AuthCodeEntity; @@ -26,16 +27,19 @@ use SimpleSAML\Module\oidc\Helpers; use SimpleSAML\Module\oidc\ModuleConfig; use SimpleSAML\Module\oidc\Repositories\Interfaces\AuthCodeRepositoryInterface; +use SimpleSAML\Module\oidc\Utils\ProtocolCache; class AuthCodeRepository extends AbstractDatabaseRepository implements AuthCodeRepositoryInterface { public function __construct( ModuleConfig $moduleConfig, + Database $database, + ?ProtocolCache $protocolCache, protected readonly ClientRepository $clientRepository, protected readonly AuthCodeEntityFactory $authCodeEntityFactory, protected readonly Helpers $helpers, ) { - parent::__construct($moduleConfig); + parent::__construct($moduleConfig, $database, $protocolCache); } final public const TABLE_NAME = 'oidc_auth_code'; diff --git a/src/Repositories/ClientRepository.php b/src/Repositories/ClientRepository.php index 6235fdda..6d6cef2f 100644 --- a/src/Repositories/ClientRepository.php +++ b/src/Repositories/ClientRepository.php @@ -17,18 +17,22 @@ use League\OAuth2\Server\Repositories\ClientRepositoryInterface; use PDO; +use SimpleSAML\Database; use SimpleSAML\Module\oidc\Entities\ClientEntity; use SimpleSAML\Module\oidc\Entities\Interfaces\ClientEntityInterface; use SimpleSAML\Module\oidc\Factories\Entities\ClientEntityFactory; use SimpleSAML\Module\oidc\ModuleConfig; +use SimpleSAML\Module\oidc\Utils\ProtocolCache; class ClientRepository extends AbstractDatabaseRepository implements ClientRepositoryInterface { public function __construct( ModuleConfig $moduleConfig, + Database $database, + ?ProtocolCache $protocolCache, protected readonly ClientEntityFactory $clientEntityFactory, ) { - parent::__construct($moduleConfig); + parent::__construct($moduleConfig, $database, $protocolCache); } final public const TABLE_NAME = 'oidc_client'; @@ -389,7 +393,7 @@ private function count(string $query, ?string $owner): int */ private function getItemsPerPage(): int { - return $this->config + return $this->moduleConfig->config() ->getOptionalIntegerRange(ModuleConfig::OPTION_ADMIN_UI_PAGINATION_ITEMS_PER_PAGE, 1, 100, 20); } diff --git a/src/Repositories/RefreshTokenRepository.php b/src/Repositories/RefreshTokenRepository.php index ee025083..e20bb23c 100644 --- a/src/Repositories/RefreshTokenRepository.php +++ b/src/Repositories/RefreshTokenRepository.php @@ -19,6 +19,7 @@ use League\OAuth2\Server\Entities\RefreshTokenEntityInterface as OAuth2RefreshTokenEntityInterface; use League\OAuth2\Server\Exception\OAuthServerException; use RuntimeException; +use SimpleSAML\Database; use SimpleSAML\Module\oidc\Codebooks\DateFormatsEnum; use SimpleSAML\Module\oidc\Entities\Interfaces\RefreshTokenEntityInterface; use SimpleSAML\Module\oidc\Entities\RefreshTokenEntity; @@ -27,6 +28,7 @@ use SimpleSAML\Module\oidc\ModuleConfig; use SimpleSAML\Module\oidc\Repositories\Interfaces\RefreshTokenRepositoryInterface; use SimpleSAML\Module\oidc\Repositories\Traits\RevokeTokenByAuthCodeIdTrait; +use SimpleSAML\Module\oidc\Utils\ProtocolCache; class RefreshTokenRepository extends AbstractDatabaseRepository implements RefreshTokenRepositoryInterface { @@ -36,11 +38,13 @@ class RefreshTokenRepository extends AbstractDatabaseRepository implements Refre public function __construct( ModuleConfig $moduleConfig, + Database $database, + ?ProtocolCache $protocolCache, protected readonly AccessTokenRepository $accessTokenRepository, protected readonly RefreshTokenEntityFactory $refreshTokenEntityFactory, protected readonly Helpers $helpers, ) { - parent::__construct($moduleConfig); + parent::__construct($moduleConfig, $database, $protocolCache); } /** diff --git a/src/Repositories/ScopeRepository.php b/src/Repositories/ScopeRepository.php index ea06d406..e623bc89 100644 --- a/src/Repositories/ScopeRepository.php +++ b/src/Repositories/ScopeRepository.php @@ -26,18 +26,12 @@ use function array_key_exists; use function in_array; -class ScopeRepository extends AbstractDatabaseRepository implements ScopeRepositoryInterface +class ScopeRepository implements ScopeRepositoryInterface { public function __construct( - ModuleConfig $moduleConfig, + protected readonly ModuleConfig $moduleConfig, protected readonly ScopeEntityFactory $scopeEntityFactory, ) { - parent::__construct($moduleConfig); - } - - public function getTableName(): ?string - { - return null; } /** diff --git a/src/Repositories/UserRepository.php b/src/Repositories/UserRepository.php index 4c2772a7..61b7fcba 100644 --- a/src/Repositories/UserRepository.php +++ b/src/Repositories/UserRepository.php @@ -21,11 +21,13 @@ use League\OAuth2\Server\Entities\ClientEntityInterface as OAuth2ClientEntityInterface; use League\OAuth2\Server\Entities\UserEntityInterface; use League\OAuth2\Server\Repositories\UserRepositoryInterface; +use SimpleSAML\Database; use SimpleSAML\Module\oidc\Entities\UserEntity; use SimpleSAML\Module\oidc\Factories\Entities\UserEntityFactory; use SimpleSAML\Module\oidc\Helpers; use SimpleSAML\Module\oidc\ModuleConfig; use SimpleSAML\Module\oidc\Repositories\Interfaces\IdentityProviderInterface; +use SimpleSAML\Module\oidc\Utils\ProtocolCache; class UserRepository extends AbstractDatabaseRepository implements UserRepositoryInterface, IdentityProviderInterface { @@ -33,10 +35,12 @@ class UserRepository extends AbstractDatabaseRepository implements UserRepositor public function __construct( ModuleConfig $moduleConfig, + Database $database, + ?ProtocolCache $protocolCache, protected readonly Helpers $helpers, protected readonly UserEntityFactory $userEntityFactory, ) { - parent::__construct($moduleConfig); + parent::__construct($moduleConfig, $database, $protocolCache); } public function getTableName(): string diff --git a/src/Services/Container.php b/src/Services/Container.php index 95b26640..8d13c20a 100644 --- a/src/Services/Container.php +++ b/src/Services/Container.php @@ -204,7 +204,15 @@ public function __construct() ); $this->services[ClientEntityFactory::class] = $clientEntityFactory; - $clientRepository = new ClientRepository($moduleConfig, $clientEntityFactory); + $database = Database::getInstance(); + $this->services[Database::class] = $database; + + $clientRepository = new ClientRepository( + $moduleConfig, + $database, + $protocolCache, + $clientEntityFactory, + ); $this->services[ClientRepository::class] = $clientRepository; $userEntityFactory = new UserEntityFactory($helpers); @@ -212,6 +220,8 @@ public function __construct() $userRepository = new UserRepository( $moduleConfig, + $database, + $protocolCache, $helpers, $userEntityFactory, ); @@ -228,6 +238,8 @@ public function __construct() $authCodeRepository = new AuthCodeRepository( $moduleConfig, + $database, + $protocolCache, $clientRepository, $authCodeEntityFactory, $helpers, @@ -252,6 +264,8 @@ public function __construct() $accessTokenRepository = new AccessTokenRepository( $moduleConfig, + $database, + $protocolCache, $clientRepository, $accessTokenEntityFactory, $helpers, @@ -263,6 +277,8 @@ public function __construct() $refreshTokenRepository = new RefreshTokenRepository( $moduleConfig, + $database, + $protocolCache, $accessTokenRepository, $refreshTokenEntityFactory, $helpers, @@ -272,12 +288,13 @@ public function __construct() $scopeRepository = new ScopeRepository($moduleConfig, $scopeEntityFactory); $this->services[ScopeRepository::class] = $scopeRepository; - $allowedOriginRepository = new AllowedOriginRepository($moduleConfig); + $allowedOriginRepository = new AllowedOriginRepository( + $moduleConfig, + $database, + $protocolCache, + ); $this->services[AllowedOriginRepository::class] = $allowedOriginRepository; - $database = Database::getInstance(); - $this->services[Database::class] = $database; - $databaseMigration = new DatabaseMigration($database); $this->services[DatabaseMigration::class] = $databaseMigration; diff --git a/tests/integration/src/Repositories/Traits/RevokeTokenByAuthCodeIdTraitTest.php b/tests/integration/src/Repositories/Traits/RevokeTokenByAuthCodeIdTraitTest.php index 12d4a27e..ddb89984 100644 --- a/tests/integration/src/Repositories/Traits/RevokeTokenByAuthCodeIdTraitTest.php +++ b/tests/integration/src/Repositories/Traits/RevokeTokenByAuthCodeIdTraitTest.php @@ -162,10 +162,19 @@ public function getDatabase(): Database $clientEntityFactoryMock = $this->createMock(ClientEntityFactory::class); $clientEntityFactoryMock->method('fromState')->willReturn($clientEntityMock); - $clientRepositoryMock = new ClientRepository($moduleConfig, $clientEntityFactoryMock); + $database = Database::getInstance(); + + $clientRepositoryMock = new ClientRepository( + $moduleConfig, + $database, + null, + $clientEntityFactoryMock + ); $this->accessTokenRepository = new AccessTokenRepository( $moduleConfig, + $database, + null, $clientRepositoryMock, $this->accessTokenEntityFactory, new Helpers(), @@ -180,6 +189,8 @@ public function getDatabase(): Database $user = new UserEntity(self::USER_ID, $createUpdatedAt, $createUpdatedAt, []); $userRepositoryMock = new UserRepository( $moduleConfig, + $database, + null, $helpers, new UserEntityFactory($helpers), ); diff --git a/tests/unit/src/Repositories/AccessTokenRepositoryTest.php b/tests/unit/src/Repositories/AccessTokenRepositoryTest.php index fab5514c..7da2cf77 100644 --- a/tests/unit/src/Repositories/AccessTokenRepositoryTest.php +++ b/tests/unit/src/Repositories/AccessTokenRepositoryTest.php @@ -20,6 +20,7 @@ use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use SimpleSAML\Configuration; +use SimpleSAML\Database; use SimpleSAML\Module\oidc\Codebooks\DateFormatsEnum; use SimpleSAML\Module\oidc\Entities\AccessTokenEntity; use SimpleSAML\Module\oidc\Entities\Interfaces\ClientEntityInterface; @@ -102,8 +103,12 @@ protected function setUp(): void $this->dateTimeHelperMock = $this->createMock(Helpers\DateTime::class); $this->helpersMock->method('dateTime')->willReturn($this->dateTimeHelperMock); + $database = Database::getInstance(); + $this->repository = new AccessTokenRepository( $this->moduleConfigMock, + $database, + null, $this->clientRepositoryMock, $this->accessTokenEntityFactoryMock, $this->helpersMock, diff --git a/tests/unit/src/Repositories/AllowedOriginRepositoryTest.php b/tests/unit/src/Repositories/AllowedOriginRepositoryTest.php index a5b1d25e..2c6f438c 100644 --- a/tests/unit/src/Repositories/AllowedOriginRepositoryTest.php +++ b/tests/unit/src/Repositories/AllowedOriginRepositoryTest.php @@ -6,6 +6,7 @@ use PHPUnit\Framework\TestCase; use SimpleSAML\Configuration; +use SimpleSAML\Database; use SimpleSAML\Module\oidc\ModuleConfig; use SimpleSAML\Module\oidc\Repositories\AllowedOriginRepository; use SimpleSAML\Module\oidc\Services\DatabaseMigration; @@ -45,7 +46,12 @@ public static function setUpBeforeClass(): void protected function setUp(): void { $moduleConfigMock = $this->createMock(ModuleConfig::class); - $this->repository = new AllowedOriginRepository($moduleConfigMock); + $database = Database::getInstance(); + $this->repository = new AllowedOriginRepository( + $moduleConfigMock, + $database, + null, + ); } public function tearDown(): void diff --git a/tests/unit/src/Repositories/AuthCodeRepositoryTest.php b/tests/unit/src/Repositories/AuthCodeRepositoryTest.php index 735b3f23..99cce538 100644 --- a/tests/unit/src/Repositories/AuthCodeRepositoryTest.php +++ b/tests/unit/src/Repositories/AuthCodeRepositoryTest.php @@ -21,6 +21,7 @@ use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use SimpleSAML\Configuration; +use SimpleSAML\Database; use SimpleSAML\Module\oidc\Codebooks\DateFormatsEnum; use SimpleSAML\Module\oidc\Entities\AuthCodeEntity; use SimpleSAML\Module\oidc\Entities\ClientEntity; @@ -84,8 +85,12 @@ protected function setUp(): void $this->dateTimeHelperMock = $this->createMock(Helpers\DateTime::class); $this->helpersMock->method('dateTime')->willReturn($this->dateTimeHelperMock); + $database = Database::getInstance(); + $this->repository = new AuthCodeRepository( $this->createMock(ModuleConfig::class), + $database, + null, $this->clientRepositoryMock, $this->authCodeEntityFactoryMock, $this->helpersMock, diff --git a/tests/unit/src/Repositories/ClientRepositoryTest.php b/tests/unit/src/Repositories/ClientRepositoryTest.php index 7a3d56e9..2d18cd8e 100644 --- a/tests/unit/src/Repositories/ClientRepositoryTest.php +++ b/tests/unit/src/Repositories/ClientRepositoryTest.php @@ -18,6 +18,7 @@ use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use SimpleSAML\Configuration; +use SimpleSAML\Database; use SimpleSAML\Module\oidc\Entities\ClientEntity; use SimpleSAML\Module\oidc\Entities\Interfaces\ClientEntityInterface; use SimpleSAML\Module\oidc\Factories\Entities\ClientEntityFactory; @@ -58,8 +59,12 @@ protected function setUp(): void $this->clientEntityMock = $this->createMock(ClientEntityInterface::class); $this->clientEntityFactoryMock = $this->createMock(ClientEntityFactory::class); + $database = Database::getInstance(); + $this->repository = new ClientRepository( new ModuleConfig(), + $database, + null, $this->clientEntityFactoryMock, ); } diff --git a/tests/unit/src/Repositories/RefreshTokenRepositoryTest.php b/tests/unit/src/Repositories/RefreshTokenRepositoryTest.php index 049b3086..ce04ccbd 100644 --- a/tests/unit/src/Repositories/RefreshTokenRepositoryTest.php +++ b/tests/unit/src/Repositories/RefreshTokenRepositoryTest.php @@ -21,6 +21,7 @@ use PHPUnit\Framework\TestCase; use RuntimeException; use SimpleSAML\Configuration; +use SimpleSAML\Database; use SimpleSAML\Module\oidc\Entities\AccessTokenEntity; use SimpleSAML\Module\oidc\Entities\RefreshTokenEntity; use SimpleSAML\Module\oidc\Factories\Entities\RefreshTokenEntityFactory; @@ -76,8 +77,12 @@ protected function setUp(): void $this->refreshTokenEntityMock = $this->createMock(RefreshTokenEntity::class); + $database = Database::getInstance(); + $this->repository = new RefreshTokenRepository( new ModuleConfig(), + $database, + null, $this->accessTokenRepositoryMock, $this->refreshTokenEntityFactoryMock, new Helpers(), diff --git a/tests/unit/src/Repositories/UserRepositoryTest.php b/tests/unit/src/Repositories/UserRepositoryTest.php index c1016ab1..9f8c47c5 100644 --- a/tests/unit/src/Repositories/UserRepositoryTest.php +++ b/tests/unit/src/Repositories/UserRepositoryTest.php @@ -20,6 +20,7 @@ use PHPUnit\Framework\MockObject\Stub; use PHPUnit\Framework\TestCase; use SimpleSAML\Configuration; +use SimpleSAML\Database; use SimpleSAML\Module\oidc\Entities\UserEntity; use SimpleSAML\Module\oidc\Factories\Entities\UserEntityFactory; use SimpleSAML\Module\oidc\Helpers; @@ -59,8 +60,12 @@ protected function setUp(): void $this->userEntityFactoryMock = $this->createMock(UserEntityFactory::class); $this->userEntityMock = $this->createMock(UserEntity::class); + $database = Database::getInstance(); + self::$repository = new UserRepository( $moduleConfig, + $database, + null, $this->helpersStub, $this->userEntityFactoryMock, ); From 55e0b344b59901018215e36056eecf37cb34813a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Mon, 4 Nov 2024 11:28:25 +0100 Subject: [PATCH 2/6] Add caching capabilities to UserRepository --- config-templates/module_oidc.php | 7 ++++ routing/services/services.yml | 6 +++- src/Controller/Federation/Test.php | 2 ++ src/ModuleConfig.php | 17 +++++++++ src/Repositories/UserRepository.php | 36 ++++++++++++++++--- .../RevokeTokenByAuthCodeIdTraitTest.php | 2 +- 6 files changed, 63 insertions(+), 7 deletions(-) diff --git a/config-templates/module_oidc.php b/config-templates/module_oidc.php index 0b915221..bd61a5de 100644 --- a/config-templates/module_oidc.php +++ b/config-templates/module_oidc.php @@ -288,6 +288,13 @@ // 60 * 60 * 6, // Default lifetime in seconds (used when particular cache item doesn't define its own lifetime) //], + // Cache duration for user entities (authenticated users data). If not set, cache duration will be the same as + // session duration. This is used to avoid fetching user data from database on every authentication event. + // This is only relevant if protocol cache adapter is set up. For duration format info, check + // https://www.php.net/manual/en/dateinterval.construct.php. +// ModuleConfig::OPTION_PROTOCOL_USER_ENTITY_CACHE_DURATION => 'PT1H', // 1 hour + ModuleConfig::OPTION_PROTOCOL_USER_ENTITY_CACHE_DURATION => null, // fallback to session duration + /** * Cron related options. */ diff --git a/routing/services/services.yml b/routing/services/services.yml index 0d5e8a48..0b4ea060 100644 --- a/routing/services/services.yml +++ b/routing/services/services.yml @@ -115,4 +115,8 @@ services: SimpleSAML\OpenID\Federation: factory: [ '@SimpleSAML\Module\oidc\Factories\FederationFactory', 'build' ] SimpleSAML\OpenID\Jwks: - factory: [ '@SimpleSAML\Module\oidc\Factories\JwksFactory', 'build' ] \ No newline at end of file + factory: [ '@SimpleSAML\Module\oidc\Factories\JwksFactory', 'build' ] + + # SSP + SimpleSAML\Database: + factory: [ 'SimpleSAML\Database', 'getInstance' ] diff --git a/src/Controller/Federation/Test.php b/src/Controller/Federation/Test.php index 64d579cf..0f4477b6 100644 --- a/src/Controller/Federation/Test.php +++ b/src/Controller/Federation/Test.php @@ -6,6 +6,7 @@ namespace SimpleSAML\Module\oidc\Controller\Federation; +use SimpleSAML\Database; use SimpleSAML\Module\oidc\Codebooks\RegistrationTypeEnum; use SimpleSAML\Module\oidc\Factories\CoreFactory; use SimpleSAML\Module\oidc\Factories\Entities\ClientEntityFactory; @@ -33,6 +34,7 @@ public function __construct( protected ?FederationCache $federationCache, protected LoggerService $loggerService, protected Jwks $jwks, + protected Database $database, protected ClientEntityFactory $clientEntityFactory, protected CoreFactory $coreFactory, protected \DateInterval $maxCacheDuration = new \DateInterval('PT30S'), diff --git a/src/ModuleConfig.php b/src/ModuleConfig.php index eec9669c..91331c01 100644 --- a/src/ModuleConfig.php +++ b/src/ModuleConfig.php @@ -80,6 +80,7 @@ class ModuleConfig final public const OPTION_FEDERATION_ENTITY_STATEMENT_CACHE_DURATION = 'federation_entity_statement_cache_duration'; final public const OPTION_PROTOCOL_CACHE_ADAPTER = 'protocol_cache_adapter'; final public const OPTION_PROTOCOL_CACHE_ADAPTER_ARGUMENTS = 'protocol_cache_adapter_arguments'; + final public const OPTION_PROTOCOL_USER_ENTITY_CACHE_DURATION = 'protocol_user_entity_cache_duration'; protected static array $standardScopes = [ ScopesEnum::OpenId->value => [ @@ -630,4 +631,20 @@ public function getProtocolCacheAdapterArguments(): array { return $this->config()->getOptionalArray(self::OPTION_PROTOCOL_CACHE_ADAPTER_ARGUMENTS, []); } + + /** + * Get cache duration for user entities (user data). If not set in configuration, it will fall back to SSP session + * duration. + * + * @throws \Exception + */ + public function getUserEntityCacheDuration(): DateInterval + { + return new DateInterval( + $this->config()->getOptionalString( + self::OPTION_PROTOCOL_USER_ENTITY_CACHE_DURATION, + null, + ) ?? "PT{$this->sspConfig()->getInteger('session.duration')}S", + ); + } } diff --git a/src/Repositories/UserRepository.php b/src/Repositories/UserRepository.php index 61b7fcba..ae0fb21f 100644 --- a/src/Repositories/UserRepository.php +++ b/src/Repositories/UserRepository.php @@ -48,6 +48,11 @@ public function getTableName(): string return $this->database->applyPrefix(self::TABLE_NAME); } + public function getCacheKey(string $identifier): string + { + return $this->getTableName() . '_' . $identifier; + } + /** * @param string $identifier * @@ -56,6 +61,13 @@ public function getTableName(): string */ public function getUserEntityByIdentifier(string $identifier): ?UserEntity { + /** @var ?array $cachedState */ + $cachedState = $this->protocolCache?->get(null, $this->getCacheKey($identifier)); + + if (is_array($cachedState)) { + return $this->userEntityFactory->fromState($cachedState); + } + $stmt = $this->database->read( "SELECT * FROM {$this->getTableName()} WHERE id = :id", [ @@ -99,21 +111,29 @@ public function add(UserEntity $userEntity): void $stmt, $userEntity->getState(), ); + + $this->protocolCache?->set( + $userEntity->getState(), + $this->moduleConfig->getUserEntityCacheDuration(), + $this->getCacheKey($userEntity->getIdentifier()), + ); } - public function delete(UserEntity $user): void + public function delete(UserEntity $userEntity): void { $this->database->write( "DELETE FROM {$this->getTableName()} WHERE id = :id", [ - 'id' => $user->getIdentifier(), + 'id' => $userEntity->getIdentifier(), ], ); + + $this->protocolCache?->delete($this->getCacheKey($userEntity->getIdentifier())); } - public function update(UserEntity $user, ?DateTimeImmutable $updatedAt = null): void + public function update(UserEntity $userEntity, ?DateTimeImmutable $updatedAt = null): void { - $user->setUpdatedAt($updatedAt ?? $this->helpers->dateTime()->getUtc()); + $userEntity->setUpdatedAt($updatedAt ?? $this->helpers->dateTime()->getUtc()); $stmt = sprintf( "UPDATE %s SET claims = :claims, updated_at = :updated_at, created_at = :created_at WHERE id = :id", @@ -122,7 +142,13 @@ public function update(UserEntity $user, ?DateTimeImmutable $updatedAt = null): $this->database->write( $stmt, - $user->getState(), + $userEntity->getState(), + ); + + $this->protocolCache?->set( + $userEntity->getState(), + $this->moduleConfig->getUserEntityCacheDuration(), + $this->getCacheKey($userEntity->getIdentifier()), ); } } diff --git a/tests/integration/src/Repositories/Traits/RevokeTokenByAuthCodeIdTraitTest.php b/tests/integration/src/Repositories/Traits/RevokeTokenByAuthCodeIdTraitTest.php index ddb89984..aa015884 100644 --- a/tests/integration/src/Repositories/Traits/RevokeTokenByAuthCodeIdTraitTest.php +++ b/tests/integration/src/Repositories/Traits/RevokeTokenByAuthCodeIdTraitTest.php @@ -168,7 +168,7 @@ public function getDatabase(): Database $moduleConfig, $database, null, - $clientEntityFactoryMock + $clientEntityFactoryMock, ); $this->accessTokenRepository = new AccessTokenRepository( From 328283d527864f329cb567751bdc2c43c07193f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Mon, 4 Nov 2024 13:40:21 +0100 Subject: [PATCH 3/6] Update test cases --- src/ModuleConfig.php | 2 +- src/Repositories/UserRepository.php | 4 +- .../RevokeTokenByAuthCodeIdTraitTest.php | 2 +- .../src/Repositories/UserRepositoryTest.php | 210 ++++++++++++++++-- 4 files changed, 195 insertions(+), 23 deletions(-) diff --git a/src/ModuleConfig.php b/src/ModuleConfig.php index 91331c01..f753bbc7 100644 --- a/src/ModuleConfig.php +++ b/src/ModuleConfig.php @@ -638,7 +638,7 @@ public function getProtocolCacheAdapterArguments(): array * * @throws \Exception */ - public function getUserEntityCacheDuration(): DateInterval + public function getProtocolUserEntityCacheDuration(): DateInterval { return new DateInterval( $this->config()->getOptionalString( diff --git a/src/Repositories/UserRepository.php b/src/Repositories/UserRepository.php index ae0fb21f..692bb197 100644 --- a/src/Repositories/UserRepository.php +++ b/src/Repositories/UserRepository.php @@ -114,7 +114,7 @@ public function add(UserEntity $userEntity): void $this->protocolCache?->set( $userEntity->getState(), - $this->moduleConfig->getUserEntityCacheDuration(), + $this->moduleConfig->getProtocolUserEntityCacheDuration(), $this->getCacheKey($userEntity->getIdentifier()), ); } @@ -147,7 +147,7 @@ public function update(UserEntity $userEntity, ?DateTimeImmutable $updatedAt = n $this->protocolCache?->set( $userEntity->getState(), - $this->moduleConfig->getUserEntityCacheDuration(), + $this->moduleConfig->getProtocolUserEntityCacheDuration(), $this->getCacheKey($userEntity->getIdentifier()), ); } diff --git a/tests/integration/src/Repositories/Traits/RevokeTokenByAuthCodeIdTraitTest.php b/tests/integration/src/Repositories/Traits/RevokeTokenByAuthCodeIdTraitTest.php index aa015884..c0354d28 100644 --- a/tests/integration/src/Repositories/Traits/RevokeTokenByAuthCodeIdTraitTest.php +++ b/tests/integration/src/Repositories/Traits/RevokeTokenByAuthCodeIdTraitTest.php @@ -138,7 +138,7 @@ public function useDatabase($config): void $moduleConfig = new ModuleConfig(); - $this->mock = new class ($moduleConfig) extends AbstractDatabaseRepository { + $this->mock = new class ($moduleConfig, $database, null) extends AbstractDatabaseRepository { use RevokeTokenByAuthCodeIdTrait; public function getTableName(): ?string diff --git a/tests/unit/src/Repositories/UserRepositoryTest.php b/tests/unit/src/Repositories/UserRepositoryTest.php index 9f8c47c5..fcac5681 100644 --- a/tests/unit/src/Repositories/UserRepositoryTest.php +++ b/tests/unit/src/Repositories/UserRepositoryTest.php @@ -16,6 +16,7 @@ namespace SimpleSAML\Test\Module\oidc\unit\Repositories; use DateTimeImmutable; +use PDOStatement; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\MockObject\Stub; use PHPUnit\Framework\TestCase; @@ -27,6 +28,8 @@ use SimpleSAML\Module\oidc\ModuleConfig; use SimpleSAML\Module\oidc\Repositories\UserRepository; use SimpleSAML\Module\oidc\Services\DatabaseMigration; +use SimpleSAML\Module\oidc\Utils\ProtocolCache; +use Symfony\Component\VarDumper\Cloner\Data; /** * @covers \SimpleSAML\Module\oidc\Repositories\UserRepository @@ -37,6 +40,17 @@ class UserRepositoryTest extends TestCase protected Stub $helpersStub; protected MockObject $userEntityFactoryMock; protected MockObject $userEntityMock; + protected MockObject $moduleConfigMock; + protected ?MockObject $protocolCacheMock; + protected MockObject $databaseMock; + protected MockObject $pdoStatementMock; + protected Database $database; + protected array $userEntityState = [ + 'id' => 'uniqueid', + 'claims' => '[]', + 'updated_at' => '2024-11-04 11:07:26', + 'created_at' => '2024-11-04 11:07:26', + ]; /** * @throws \Exception @@ -53,27 +67,44 @@ protected function setUp(): void ]; Configuration::loadFromArray($config, '', 'simplesaml'); - (new DatabaseMigration())->migrate(); + $this->database = Database::getInstance(); + (new DatabaseMigration($this->database))->migrate(); - $moduleConfig = new ModuleConfig(); + $this->databaseMock = $this->createMock(Database::class); + $this->pdoStatementMock = $this->createMock(PDOStatement::class); + $this->moduleConfigMock = $this->createMock(ModuleConfig::class); $this->helpersStub = $this->createStub(Helpers::class); $this->userEntityFactoryMock = $this->createMock(UserEntityFactory::class); $this->userEntityMock = $this->createMock(UserEntity::class); + $this->protocolCacheMock = $this->createMock(ProtocolCache::class); + } - $database = Database::getInstance(); + protected function mock( + ModuleConfig|MockObject $moduleConfig = null, + Database|MockObject $database = null, + ProtocolCache|MockObject $protocolCache = null, + Helpers|MockObject $helpers = null, + UserEntityFactory|MockObject $userEntityFactory = null + ): UserRepository + { + $moduleConfig ??= $this->moduleConfigMock; + $database ??= $this->database; // Let's use real database instance for tests by default. + $protocolCache ??= null; // Let's not use cache for tests by default. + $helpers ??= $this->helpersStub; + $userEntityFactory ??= $this->userEntityFactoryMock; - self::$repository = new UserRepository( + return new UserRepository( $moduleConfig, $database, - null, - $this->helpersStub, - $this->userEntityFactoryMock, + $protocolCache, + $helpers, + $userEntityFactory, ); } public function testGetTableName(): void { - $this->assertSame('phpunit_oidc_user', self::$repository->getTableName()); + $this->assertSame('phpunit_oidc_user', $this->mock()->getTableName()); } /** @@ -82,17 +113,21 @@ public function testGetTableName(): void */ public function testCanAddFindDelete(): void { + $repository = $this->mock(); + $createdUpdatedAt = new DateTimeImmutable(); - self::$repository->add(new UserEntity('uniqueid', $createdUpdatedAt, $createdUpdatedAt)); + $userEntity = new UserEntity('uniqueid', $createdUpdatedAt, $createdUpdatedAt); + + $repository->add($userEntity); - $this->userEntityMock->method('getIdentifier')->willReturn('uniqueid'); $this->userEntityFactoryMock->expects($this->once()) ->method('fromState') ->with($this->callback(function (array $state) { return $state['id'] === 'uniqueid'; })) - ->willReturn($this->userEntityMock); - $user = self::$repository->getUserEntityByIdentifier('uniqueid'); + ->willReturn($userEntity); + + $user = $repository->getUserEntityByIdentifier('uniqueid'); $this->assertNotNull($user); $this->assertSame($user->getIdentifier(), 'uniqueid'); @@ -103,7 +138,7 @@ public function testCanAddFindDelete(): void */ public function testNotFound(): void { - $user = self::$repository->getUserEntityByIdentifier('unknownid'); + $user = $this->mock()->getUserEntityByIdentifier('unknownid'); $this->assertNull($user); } @@ -114,19 +149,156 @@ public function testNotFound(): void */ public function testUpdate(): void { - $user = self::$repository->getUserEntityByIdentifier('uniqueid'); + $repository = $this->mock(); + + $user = $repository->getUserEntityByIdentifier('uniqueid'); $user->setClaims(['uid' => ['johndoe']]); - self::$repository->update($user); + $repository->update($user); - $user2 = self::$repository->getUserEntityByIdentifier('uniqueid'); + $user2 = $repository->getUserEntityByIdentifier('uniqueid'); $this->assertNotSame($user, $user2); } public function testCanDelete(): void { + $repository = $this->mock(); + $this->userEntityMock->method('getIdentifier')->willReturn('uniqueid'); - $this->assertNotNull(self::$repository->getUserEntityByIdentifier('uniqueid')); - self::$repository->delete($this->userEntityMock); - $this->assertNull(self::$repository->getUserEntityByIdentifier('uniqueid')); + $this->assertNotNull($repository->getUserEntityByIdentifier('uniqueid')); + $repository->delete($this->userEntityMock); + $this->assertNull($repository->getUserEntityByIdentifier('uniqueid')); + } + + public function testCanGetWhenUserEntityIsCached(): void + { + $this->protocolCacheMock->expects($this->once()) + ->method('get') + ->willReturn($this->userEntityState); + + $this->databaseMock->expects($this->never())->method('read'); + + $this->userEntityFactoryMock->expects($this->once()) + ->method('fromState') + ->with($this->callback(function (array $state) { + return $state['id'] === 'uniqueid'; + })) + ->willReturn($this->userEntityMock); + + $repository = $this->mock( + database: $this->databaseMock, + protocolCache: $this->protocolCacheMock, + ); + + $this->assertSame( + $this->userEntityMock, + $repository->getUserEntityByIdentifier('uniqueid'), + ); + } + + public function testCanGetWhenUserEntityIsNotCached(): void + { + $this->protocolCacheMock->expects($this->once()) + ->method('get') + ->willReturn(null); + + $this->pdoStatementMock->method('fetchAll')->willReturn([$this->userEntityState]); + + $this->databaseMock->expects($this->once()) + ->method('read') + ->willReturn($this->pdoStatementMock); + + $this->userEntityFactoryMock->expects($this->once()) + ->method('fromState') + ->with($this->callback(function (array $state) { + return $state['id'] === 'uniqueid'; + })) + ->willReturn($this->userEntityMock); + + $repository = $this->mock( + database: $this->databaseMock, + protocolCache: $this->protocolCacheMock, + ); + + $this->assertSame( + $this->userEntityMock, + $repository->getUserEntityByIdentifier('uniqueid'), + ); + } + + public function testWillAddToDatabaseAndCache(): void + { + $this->moduleConfigMock->method('getProtocolUserEntityCacheDuration') + ->willReturn(new \DateInterval('PT1H')); + + $this->userEntityMock->expects($this->exactly(2)) + ->method('getState') + ->willReturn($this->userEntityState); + + $this->protocolCacheMock->expects($this->once()) + ->method('set') + ->with($this->userEntityState); + + $this->databaseMock->expects($this->once()) + ->method('write') + ->with( + $this->isType('string'), + $this->userEntityState, + ); + + $this->mock( + database: $this->databaseMock, + protocolCache: $this->protocolCacheMock, + )->add($this->userEntityMock); + } + + public function testWillUpdateDatabaseAndCache(): void + { + $this->moduleConfigMock->method('getProtocolUserEntityCacheDuration') + ->willReturn(new \DateInterval('PT1H')); + + $this->userEntityMock->expects($this->exactly(2)) + ->method('getState') + ->willReturn($this->userEntityState); + + $this->protocolCacheMock->expects($this->once()) + ->method('set') + ->with($this->userEntityState); + + $this->databaseMock->expects($this->once()) + ->method('write') + ->with( + $this->isType('string'), + $this->userEntityState, + ); + + $this->mock( + database: $this->databaseMock, + protocolCache: $this->protocolCacheMock, + )->update($this->userEntityMock); + } + + public function testWillDeleteFromDatabaseAndCache(): void + { + $this->userEntityMock->expects($this->exactly(2)) + ->method('getIdentifier') + ->willReturn('uniqueid'); + + $this->protocolCacheMock->expects($this->once()) + ->method('delete') + ->with($this->stringContains('uniqueid')); + + $this->databaseMock->expects($this->once()) + ->method('write') + ->with( + $this->stringContains('DELETE'), + $this->callback(function (array $params) { + return $params['id'] === 'uniqueid'; + }) + ); + + $this->mock( + database: $this->databaseMock, + protocolCache: $this->protocolCacheMock, + )->delete($this->userEntityMock); } } From b483955073de6b37552f7a8406ea1355d808863f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Mon, 4 Nov 2024 14:26:38 +0100 Subject: [PATCH 4/6] Cache on first find --- src/Repositories/UserRepository.php | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/Repositories/UserRepository.php b/src/Repositories/UserRepository.php index 692bb197..420e02f5 100644 --- a/src/Repositories/UserRepository.php +++ b/src/Repositories/UserRepository.php @@ -85,7 +85,15 @@ public function getUserEntityByIdentifier(string $identifier): ?UserEntity return null; } - return $this->userEntityFactory->fromState($row); + $userEntity = $this->userEntityFactory->fromState($row); + + $this->protocolCache?->set( + $userEntity->getState(), + $this->moduleConfig->getProtocolUserEntityCacheDuration(), + $this->getCacheKey($userEntity->getIdentifier()), + ); + + return $userEntity; } /** From 3e945024eca9a547ba749da3ddebafc963c7f70d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Mon, 4 Nov 2024 14:27:24 +0100 Subject: [PATCH 5/6] Fix phpcbf --- .../unit/src/Repositories/UserRepositoryTest.php | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/tests/unit/src/Repositories/UserRepositoryTest.php b/tests/unit/src/Repositories/UserRepositoryTest.php index fcac5681..fc2e7270 100644 --- a/tests/unit/src/Repositories/UserRepositoryTest.php +++ b/tests/unit/src/Repositories/UserRepositoryTest.php @@ -29,7 +29,6 @@ use SimpleSAML\Module\oidc\Repositories\UserRepository; use SimpleSAML\Module\oidc\Services\DatabaseMigration; use SimpleSAML\Module\oidc\Utils\ProtocolCache; -use Symfony\Component\VarDumper\Cloner\Data; /** * @covers \SimpleSAML\Module\oidc\Repositories\UserRepository @@ -84,9 +83,8 @@ protected function mock( Database|MockObject $database = null, ProtocolCache|MockObject $protocolCache = null, Helpers|MockObject $helpers = null, - UserEntityFactory|MockObject $userEntityFactory = null - ): UserRepository - { + UserEntityFactory|MockObject $userEntityFactory = null, + ): UserRepository { $moduleConfig ??= $this->moduleConfigMock; $database ??= $this->database; // Let's use real database instance for tests by default. $protocolCache ??= null; // Let's not use cache for tests by default. @@ -201,12 +199,20 @@ public function testCanGetWhenUserEntityIsNotCached(): void ->method('get') ->willReturn(null); + $this->protocolCacheMock->expects($this->once()) + ->method('set') + ->with($this->userEntityState); + $this->pdoStatementMock->method('fetchAll')->willReturn([$this->userEntityState]); $this->databaseMock->expects($this->once()) ->method('read') ->willReturn($this->pdoStatementMock); + $this->userEntityMock->expects($this->once()) + ->method('getState') + ->willReturn($this->userEntityState); + $this->userEntityFactoryMock->expects($this->once()) ->method('fromState') ->with($this->callback(function (array $state) { @@ -293,7 +299,7 @@ public function testWillDeleteFromDatabaseAndCache(): void $this->stringContains('DELETE'), $this->callback(function (array $params) { return $params['id'] === 'uniqueid'; - }) + }), ); $this->mock( From 12a15243cd9bf06e2c5d44c25b1038138eb6fece Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Mon, 4 Nov 2024 14:27:53 +0100 Subject: [PATCH 6/6] Start using protocol cache for docker runs --- docker/ssp/module_oidc.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docker/ssp/module_oidc.php b/docker/ssp/module_oidc.php index a2f6a0b4..bd16a2d7 100644 --- a/docker/ssp/module_oidc.php +++ b/docker/ssp/module_oidc.php @@ -115,4 +115,9 @@ ModuleConfig::OPTION_AUTH_FORCED_ACR_VALUE_FOR_COOKIE_AUTHENTICATION => null, ModuleConfig::OPTION_CRON_TAG => 'hourly', + + ModuleConfig::OPTION_PROTOCOL_CACHE_ADAPTER => \Symfony\Component\Cache\Adapter\FilesystemAdapter::class, + ModuleConfig::OPTION_PROTOCOL_CACHE_ADAPTER_ARGUMENTS => [ + // Use defaults + ], ];