From 048117424eca476c36ab4e60febaf2737628e99f Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Sat, 15 Aug 2026 09:39:19 +0200 Subject: [PATCH 1/6] refactor: Port oauth2 app to Entity Signed-off-by: Carl Schwan --- .../CleanupExpiredAuthorizationCode.php | 3 +- .../lib/Command/ImportLegacyOcClient.php | 8 +- .../Controller/LoginRedirectorController.php | 10 +- .../lib/Controller/OauthApiController.php | 18 ++-- apps/oauth2/lib/Db/AccessToken.php | 65 +++++-------- apps/oauth2/lib/Db/AccessTokenMapper.php | 61 ++++-------- apps/oauth2/lib/Db/Client.php | 50 +++++----- apps/oauth2/lib/Db/ClientMapper.php | 55 ++++------- apps/oauth2/lib/Service/ClientService.php | 18 ++-- apps/oauth2/lib/Settings/Admin.php | 20 ++-- .../LoginRedirectorControllerTest.php | 23 +++-- .../Controller/OauthApiControllerTest.php | 94 +++++++++---------- .../Controller/SettingsControllerTest.php | 2 +- .../oauth2/tests/Db/AccessTokenMapperTest.php | 27 +++--- apps/oauth2/tests/Db/ClientMapperTest.php | 34 ++++--- .../tests/Service/ClientServiceTest.php | 40 ++++---- apps/oauth2/tests/Settings/AdminTest.php | 17 +--- 17 files changed, 233 insertions(+), 312 deletions(-) diff --git a/apps/oauth2/lib/BackgroundJob/CleanupExpiredAuthorizationCode.php b/apps/oauth2/lib/BackgroundJob/CleanupExpiredAuthorizationCode.php index 14d26c873810a..d2a03b4e60093 100644 --- a/apps/oauth2/lib/BackgroundJob/CleanupExpiredAuthorizationCode.php +++ b/apps/oauth2/lib/BackgroundJob/CleanupExpiredAuthorizationCode.php @@ -30,12 +30,11 @@ public function __construct( /** * @param mixed $argument - * @inheritDoc */ #[\Override] protected function run($argument): void { try { - $this->accessTokenMapper->cleanupExpiredAuthorizationCode(); + $this->accessTokenMapper->cleanupExpiredAuthorizationCode($this->time); } catch (Exception $e) { $this->logger->warning('Failed to cleanup tokens with expired authorization code', ['exception' => $e]); } diff --git a/apps/oauth2/lib/Command/ImportLegacyOcClient.php b/apps/oauth2/lib/Command/ImportLegacyOcClient.php index 93649fd4b1f62..ba237936fb619 100644 --- a/apps/oauth2/lib/Command/ImportLegacyOcClient.php +++ b/apps/oauth2/lib/Command/ImportLegacyOcClient.php @@ -67,10 +67,10 @@ protected function execute(InputInterface $input, OutputInterface $output): int $hashedClientSecret = bin2hex($this->crypto->calculateHMAC($clientSecret)); $client = new Client(); - $client->setName('ownCloud Desktop Client'); - $client->setRedirectUri('http://localhost:*'); - $client->setClientIdentifier($clientId); - $client->setSecret($hashedClientSecret); + $client->name = 'ownCloud Desktop Client'; + $client->redirectUri = 'http://localhost:*'; + $client->clientIdentifier = $clientId; + $client->secret = $hashedClientSecret; $this->clientMapper->insert($client); $output->writeln('Client imported successfully'); diff --git a/apps/oauth2/lib/Controller/LoginRedirectorController.php b/apps/oauth2/lib/Controller/LoginRedirectorController.php index fb64febd908e8..4252580fb315f 100644 --- a/apps/oauth2/lib/Controller/LoginRedirectorController.php +++ b/apps/oauth2/lib/Controller/LoginRedirectorController.php @@ -73,20 +73,20 @@ public function authorize( if ($response_type !== 'code') { //Fail - $url = $client->getRedirectUri() . '?error=unsupported_response_type&state=' . \urlencode($state); + $url = $client->redirectUri . '?error=unsupported_response_type&state=' . \urlencode($state); return new RedirectResponse($url); } $enableOcClients = $this->config->getSystemValueBool('oauth2.enable_oc_clients', false); $providedRedirectUri = ''; - if ($enableOcClients && $client->getRedirectUri() === 'http://localhost:*') { + if ($enableOcClients && $client->redirectUri === 'http://localhost:*') { $providedRedirectUri = $redirect_uri; } $this->session->set('oauth.state', $state); - if (in_array($client->getName(), $this->appConfig->getValueArray('oauth2', 'skipAuthPickerApplications', []))) { + if (in_array($client->name, $this->appConfig->getValueArray('oauth2', 'skipAuthPickerApplications', []))) { /** @see ClientFlowLoginController::showAuthPickerPage **/ $stateToken = $this->random->generate( 64, @@ -97,7 +97,7 @@ public function authorize( 'core.ClientFlowLogin.grantPage', [ 'stateToken' => $stateToken, - 'clientIdentifier' => $client->getClientIdentifier(), + 'clientIdentifier' => $client->clientIdentifier, 'providedRedirectUri' => $providedRedirectUri, ] ); @@ -105,7 +105,7 @@ public function authorize( $targetUrl = $this->urlGenerator->linkToRouteAbsolute( 'core.ClientFlowLogin.showAuthPickerPage', [ - 'clientIdentifier' => $client->getClientIdentifier(), + 'clientIdentifier' => $client->clientIdentifier, 'providedRedirectUri' => $providedRedirectUri, ] ); diff --git a/apps/oauth2/lib/Controller/OauthApiController.php b/apps/oauth2/lib/Controller/OauthApiController.php index 3a768bad71362..bddf1a5f349a8 100644 --- a/apps/oauth2/lib/Controller/OauthApiController.php +++ b/apps/oauth2/lib/Controller/OauthApiController.php @@ -113,7 +113,7 @@ public function getToken( if ($grant_type === 'authorization_code') { // check this token is in authorization code state - $deliveredTokenCount = $accessToken->getTokenCount(); + $deliveredTokenCount = $accessToken->tokenCount; if ($deliveredTokenCount > 0) { $response = new JSONResponse([ 'error' => 'invalid_request', @@ -124,7 +124,7 @@ public function getToken( // check authorization code expiration $now = $this->timeFactory->now()->getTimestamp(); - $codeCreatedAt = $accessToken->getCodeCreatedAt(); + $codeCreatedAt = $accessToken->codeCreatedAt; if ($codeCreatedAt < $now - self::AUTHORIZATION_CODE_EXPIRES_AFTER) { // we know this token is not useful anymore $this->accessTokenMapper->delete($accessToken); @@ -139,12 +139,12 @@ public function getToken( } try { - $client = $this->clientMapper->getByUid($accessToken->getClientId()); + $client = $this->clientMapper->getByUid($accessToken->clientId); } catch (ClientNotFoundException $e) { $response = new JSONResponse([ 'error' => 'invalid_request', ], Http::STATUS_BAD_REQUEST); - $response->throttle(['invalid_request' => 'client not found', 'client_id' => $accessToken->getClientId()]); + $response->throttle(['invalid_request' => 'client not found', 'client_id' => $accessToken->clientId]); return $response; } @@ -154,7 +154,7 @@ public function getToken( } try { - $storedClientSecretHash = $client->getSecret(); + $storedClientSecretHash = $client->secret; $clientSecretHash = bin2hex($this->crypto->calculateHMAC($client_secret)); } catch (\Exception $e) { $this->logger->error('OAuth client secret decryption error', ['exception' => $e]); @@ -164,7 +164,7 @@ public function getToken( ], Http::STATUS_BAD_REQUEST); } // The client id and secret must match. Else we don't provide an access token! - if ($client->getClientIdentifier() !== $client_id || $storedClientSecretHash !== $clientSecretHash) { + if ($client->clientIdentifier !== $client_id || $storedClientSecretHash !== $clientSecretHash) { $response = new JSONResponse([ 'error' => 'invalid_client', ], Http::STATUS_BAD_REQUEST); @@ -172,11 +172,11 @@ public function getToken( return $response; } - $decryptedToken = $this->crypto->decrypt($accessToken->getEncryptedToken(), $code); + $decryptedToken = $this->crypto->decrypt($accessToken->encryptedToken, $code); // Obtain the appToken associated try { - $appToken = $this->tokenProvider->getTokenById($accessToken->getTokenId()); + $appToken = $this->tokenProvider->getTokenById($accessToken->tokenId); } catch (ExpiredTokenException $e) { $appToken = $e->getToken(); } catch (InvalidTokenException $e) { @@ -200,7 +200,7 @@ public function getToken( $this->db->beginTransaction(); try { $updatedRows = $this->accessTokenMapper->rotateToken( - $accessToken->getId(), + $accessToken->id, $code, $newCode, $newEncryptedToken, diff --git a/apps/oauth2/lib/Db/AccessToken.php b/apps/oauth2/lib/Db/AccessToken.php index e0cadbc0a47e7..cb3170cc4b5b1 100644 --- a/apps/oauth2/lib/Db/AccessToken.php +++ b/apps/oauth2/lib/Db/AccessToken.php @@ -9,44 +9,31 @@ namespace OCA\OAuth2\Db; -use OCP\AppFramework\Db\Entity; -use OCP\DB\Types; +use OCP\AppFramework\ORM\Attribute\Column; +use OCP\AppFramework\ORM\Attribute\Entity; +use OCP\AppFramework\ORM\Attribute\Id; +use OCP\DB\Schema\ColumnType; -/** - * @method int getTokenId() - * @method void setTokenId(int $identifier) - * @method int getClientId() - * @method void setClientId(int $identifier) - * @method string getEncryptedToken() - * @method void setEncryptedToken(string $token) - * @method string getHashedCode() - * @method void setHashedCode(string $token) - * @method int getCodeCreatedAt() - * @method void setCodeCreatedAt(int $createdAt) - * @method int getTokenCount() - * @method void setTokenCount(int $tokenCount) - */ -class AccessToken extends Entity { - /** @var int */ - protected $tokenId; - /** @var int */ - protected $clientId; - /** @var string */ - protected $hashedCode; - /** @var string */ - protected $encryptedToken; - /** @var int */ - protected $codeCreatedAt; - /** @var int */ - protected $tokenCount; - - public function __construct() { - $this->addType('id', Types::INTEGER); - $this->addType('tokenId', Types::INTEGER); - $this->addType('clientId', Types::INTEGER); - $this->addType('hashedCode', 'string'); - $this->addType('encryptedToken', 'string'); - $this->addType('codeCreatedAt', Types::INTEGER); - $this->addType('tokenCount', Types::INTEGER); - } +#[Entity(name: 'oauth2_access_tokens')] +class AccessToken { + #[Id, Column(name: 'id', type: ColumnType::Integer)] + public int $id; + + #[Column(name: 'token_id', type: ColumnType::Integer)] + public int $tokenId; + + #[Column(name: 'client_id', type: ColumnType::Integer)] + public int $clientId; + + #[Column(name: 'hashed_code', type: ColumnType::String, length: 128)] + public string $hashedCode; + + #[Column(name: 'encrypted_token', type: ColumnType::String, length: 786)] + public string $encryptedToken; + + #[Column(name: 'code_created_at', type: ColumnType::Bigint, default: 0)] + public int $codeCreatedAt = 0; + + #[Column(name: 'token_count', type: ColumnType::Bigint, default: 0)] + public int $tokenCount = 0; } diff --git a/apps/oauth2/lib/Db/AccessTokenMapper.php b/apps/oauth2/lib/Db/AccessTokenMapper.php index fa13d4fd7d241..dae3922df4c9e 100644 --- a/apps/oauth2/lib/Db/AccessTokenMapper.php +++ b/apps/oauth2/lib/Db/AccessTokenMapper.php @@ -11,57 +11,41 @@ use OCA\OAuth2\Controller\OauthApiController; use OCA\OAuth2\Exceptions\AccessTokenNotFoundException; +use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Db\IMapperException; use OCP\AppFramework\Db\QBMapper; +use OCP\AppFramework\ORM\Repository; use OCP\AppFramework\Utility\ITimeFactory; use OCP\DB\Exception; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IDBConnection; /** - * @template-extends QBMapper + * @template-extends Repository */ -class AccessTokenMapper extends QBMapper { - - public function __construct( - IDBConnection $db, - private ITimeFactory $timeFactory, - ) { - parent::__construct($db, 'oauth2_access_tokens'); - } +class AccessTokenMapper extends Repository { + const string entityClass = AccessToken::class; /** - * @param string $code - * @return AccessToken * @throws AccessTokenNotFoundException */ public function getByCode(string $code): AccessToken { - $qb = $this->db->getQueryBuilder(); - $qb - ->select('*') - ->from($this->tableName) - ->where($qb->expr()->eq('hashed_code', $qb->createNamedParameter(hash('sha512', $code)))); - try { - $token = $this->findEntity($qb); - } catch (IMapperException $e) { + return $this->findOneBy([ + 'hashedCode' => hash('sha512', $code), + ]); + } catch (DoesNotExistException $e) { throw new AccessTokenNotFoundException('Could not find access token', 0, $e); } - - return $token; } /** - * delete all access token from a given client - * - * @param int $id + * Delete all access token from a given client */ - public function deleteByClientId(int $id) { - $qb = $this->db->getQueryBuilder(); - $qb - ->delete($this->tableName) - ->where($qb->expr()->eq('client_id', $qb->createNamedParameter($id, IQueryBuilder::PARAM_INT))); - $qb->executeStatement(); + public function deleteByClientId(int $id): void { + $this->deleteBy([ + 'clientId' => $id, + ]); } /** @@ -69,16 +53,15 @@ public function deleteByClientId(int $id) { * -> those that are old enough * and which never delivered any oauth token (still in authorization state) * - * @return void * @throws Exception */ - public function cleanupExpiredAuthorizationCode(): void { - $now = $this->timeFactory->now()->getTimestamp(); + public function cleanupExpiredAuthorizationCode(ITimeFactory $timeFactory): void { + $now = $timeFactory->now()->getTimestamp(); $maxTokenCreationTs = $now - OauthApiController::AUTHORIZATION_CODE_EXPIRES_AFTER; - $qb = $this->db->getQueryBuilder(); + $qb = $this->getDatabaseConnection()->getQueryBuilder(); $qb - ->delete($this->tableName) + ->delete($this->getTableName()) ->where($qb->expr()->eq('token_count', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT))) ->andWhere($qb->expr()->lt('code_created_at', $qb->createNamedParameter($maxTokenCreationTs, IQueryBuilder::PARAM_INT))); $qb->executeStatement(); @@ -87,17 +70,13 @@ public function cleanupExpiredAuthorizationCode(): void { /** * Rotate an access token only if it still matches the caller's previously-read state. * - * @param int $id - * @param string $oldCode - * @param string $newCode - * @param string $encryptedToken * @param bool $expectAuthorizationCodeState Require the token to still be unused * @return int Number of updated rows */ public function rotateToken(int $id, string $oldCode, string $newCode, string $encryptedToken, bool $expectAuthorizationCodeState): int { - $qb = $this->db->getQueryBuilder(); + $qb = $this->getDatabaseConnection()->getQueryBuilder(); $qb - ->update($this->tableName) + ->update($this->getTableName()) ->set('hashed_code', $qb->createNamedParameter(hash('sha512', $newCode))) ->set('encrypted_token', $qb->createNamedParameter($encryptedToken)) ->set('token_count', $qb->createFunction('token_count + 1')) diff --git a/apps/oauth2/lib/Db/Client.php b/apps/oauth2/lib/Db/Client.php index e87266e706685..ed74686919c4e 100644 --- a/apps/oauth2/lib/Db/Client.php +++ b/apps/oauth2/lib/Db/Client.php @@ -9,34 +9,26 @@ namespace OCA\OAuth2\Db; -use OCP\AppFramework\Db\Entity; -use OCP\DB\Types; -/** - * @method string getClientIdentifier() - * @method void setClientIdentifier(string $identifier) - * @method string getSecret() - * @method void setSecret(string $secret) - * @method string getRedirectUri() - * @method void setRedirectUri(string $redirectUri) - * @method string getName() - * @method void setName(string $name) - */ -class Client extends Entity { - /** @var string */ - protected $name; - /** @var string */ - protected $redirectUri; - /** @var string */ - protected $clientIdentifier; - /** @var string */ - protected $secret; - - public function __construct() { - $this->addType('id', Types::INTEGER); - $this->addType('name', 'string'); - $this->addType('redirectUri', 'string'); - $this->addType('clientIdentifier', 'string'); - $this->addType('secret', 'string'); - } +use OCP\AppFramework\ORM\Attribute\Column; +use OCP\AppFramework\ORM\Attribute\Entity; +use OCP\AppFramework\ORM\Attribute\Id; +use OCP\DB\Schema\ColumnType; + +#[Entity(name: 'oauth2_clients')] +class Client { + #[Id, Column(name: 'id', type: ColumnType::Integer)] + public int $id; + + #[Column(name: 'name', type: ColumnType::String, length: 64)] + public string $name; + + #[Column(name: 'redirect_uri', type: ColumnType::String, length: 2000)] + public string $redirectUri; + + #[Column(name: 'client_identifier', type: ColumnType::String, length: 64)] + public string $clientIdentifier; + + #[Column(name: 'secret', type: ColumnType::String, length: 512)] + public string $secret; } diff --git a/apps/oauth2/lib/Db/ClientMapper.php b/apps/oauth2/lib/Db/ClientMapper.php index c9c261aa1ee66..e6a0000d7972f 100644 --- a/apps/oauth2/lib/Db/ClientMapper.php +++ b/apps/oauth2/lib/Db/ClientMapper.php @@ -10,22 +10,18 @@ namespace OCA\OAuth2\Db; use OCA\OAuth2\Exceptions\ClientNotFoundException; +use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Db\IMapperException; use OCP\AppFramework\Db\QBMapper; +use OCP\AppFramework\ORM\Repository; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IDBConnection; /** - * @template-extends QBMapper + * @template-extends Repository */ -class ClientMapper extends QBMapper { - - /** - * @param IDBConnection $db - */ - public function __construct(IDBConnection $db) { - parent::__construct($db, 'oauth2_clients'); - } +class ClientMapper extends Repository { + public const string entityClass = Client::class; /** * @param string $clientIdentifier @@ -33,18 +29,13 @@ public function __construct(IDBConnection $db) { * @throws ClientNotFoundException */ public function getByIdentifier(string $clientIdentifier): Client { - $qb = $this->db->getQueryBuilder(); - $qb - ->select('*') - ->from($this->tableName) - ->where($qb->expr()->eq('client_identifier', $qb->createNamedParameter($clientIdentifier))); - try { - $client = $this->findEntity($qb); - } catch (IMapperException $e) { - throw new ClientNotFoundException('could not find client ' . $clientIdentifier, 0, $e); + return $this->findOneBy([ + 'clientIdentifier' => $clientIdentifier, + ]); + } catch (DoesNotExistException $e) { + throw new ClientNotFoundException('Could not find client ' . $clientIdentifier, previous: $e); } - return $client; } /** @@ -53,29 +44,19 @@ public function getByIdentifier(string $clientIdentifier): Client { * @throws ClientNotFoundException */ public function getByUid(int $id): Client { - $qb = $this->db->getQueryBuilder(); - $qb - ->select('*') - ->from($this->tableName) - ->where($qb->expr()->eq('id', $qb->createNamedParameter($id, IQueryBuilder::PARAM_INT))); - try { - $client = $this->findEntity($qb); - } catch (IMapperException $e) { - throw new ClientNotFoundException('could not find client with id ' . $id, 0, $e); + return $this->findOneBy([ + 'id' => $id, + ]); + } catch (DoesNotExistException $e) { + throw new ClientNotFoundException('could not find client with id ' . $id, previous: $e); } - return $client; } /** - * @return Client[] + * @return \Generator */ - public function getClients(): array { - $qb = $this->db->getQueryBuilder(); - $qb - ->select('*') - ->from($this->tableName); - - return $this->findEntities($qb); + public function getClients(): \Generator { + return $this->yieldAll(); } } diff --git a/apps/oauth2/lib/Service/ClientService.php b/apps/oauth2/lib/Service/ClientService.php index e47afa35bc8db..6cd6e5192a02e 100644 --- a/apps/oauth2/lib/Service/ClientService.php +++ b/apps/oauth2/lib/Service/ClientService.php @@ -49,19 +49,19 @@ public function __construct( */ public function addClient(string $name, string $redirectUri): array { $client = new Client(); - $client->setName($name); - $client->setRedirectUri($redirectUri); + $client->name = $name; + $client->redirectUri = $redirectUri; $secret = $this->secureRandom->generate(64, self::validChars); $hashedSecret = bin2hex($this->crypto->calculateHMAC($secret)); - $client->setSecret($hashedSecret); - $client->setClientIdentifier($this->secureRandom->generate(64, self::validChars)); + $client->secret = $hashedSecret; + $client->clientIdentifier = $this->secureRandom->generate(64, self::validChars); $client = $this->clientMapper->insert($client); return [ - 'id' => $client->getId(), - 'name' => $client->getName(), - 'redirectUri' => $client->getRedirectUri(), - 'clientId' => $client->getClientIdentifier(), + 'id' => $client->id, + 'name' => $client->name, + 'redirectUri' => $client->redirectUri, + 'clientId' => $client->clientIdentifier, 'clientSecret' => $secret, ]; } @@ -74,7 +74,7 @@ public function deleteClient(int $id): void { // OAuth2 client does not silently cancel a pending wipe. $tokens = $this->tokenProvider->getTokenByUser($user->getUID()); foreach ($tokens as $token) { - if ($token->getName() !== $client->getName()) { + if ($token->getName() !== $client->name) { continue; } try { diff --git a/apps/oauth2/lib/Settings/Admin.php b/apps/oauth2/lib/Settings/Admin.php index 68d580c79b2f9..b32164df03f80 100644 --- a/apps/oauth2/lib/Settings/Admin.php +++ b/apps/oauth2/lib/Settings/Admin.php @@ -15,7 +15,6 @@ use OCP\IURLGenerator; use OCP\Settings\ISettings; use OCP\Util; -use Psr\Log\LoggerInterface; class Admin implements ISettings { @@ -23,7 +22,6 @@ public function __construct( private IInitialState $initialState, private ClientMapper $clientMapper, private IURLGenerator $urlGenerator, - private LoggerInterface $logger, ) { } @@ -33,17 +31,13 @@ public function getForm(): TemplateResponse { $result = []; foreach ($clients as $client) { - try { - $result[] = [ - 'id' => $client->getId(), - 'name' => $client->getName(), - 'redirectUri' => $client->getRedirectUri(), - 'clientId' => $client->getClientIdentifier(), - 'clientSecret' => '', - ]; - } catch (\Exception $e) { - $this->logger->error('[Settings] OAuth client secret decryption error', ['exception' => $e]); - } + $result[] = [ + 'id' => $client->id, + 'name' => $client->name, + 'redirectUri' => $client->redirectUri, + 'clientId' => $client->clientIdentifier, + 'clientSecret' => '', + ]; } $this->initialState->provideInitialState('clients', $result); $this->initialState->provideInitialState('oauth2-doc-link', $this->urlGenerator->linkToDocs('admin-oauth2')); diff --git a/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php b/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php index d3fd059b5a701..ebbb0ca797079 100644 --- a/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php +++ b/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php @@ -25,7 +25,7 @@ use Test\TestCase; #[\PHPUnit\Framework\Attributes\Group(name: 'DB')] -class LoginRedirectorControllerTest extends TestCase { +final class LoginRedirectorControllerTest extends TestCase { private IRequest&MockObject $request; private IURLGenerator&MockObject $urlGenerator; private ClientMapper&MockObject $clientMapper; @@ -64,7 +64,8 @@ protected function setUp(): void { public function testAuthorize(): void { $client = new Client(); - $client->setClientIdentifier('MyClientIdentifier'); + $client->name = 'MyClientName'; + $client->clientIdentifier = 'MyClientIdentifier'; $this->clientMapper ->expects($this->once()) ->method('getByIdentifier') @@ -97,8 +98,8 @@ public function testAuthorize(): void { public function testAuthorizeSkipPicker(): void { $client = new Client(); - $client->setName('MyClientName'); - $client->setClientIdentifier('MyClientIdentifier'); + $client->name = 'MyClientName'; + $client->clientIdentifier = 'MyClientIdentifier'; $this->clientMapper ->expects($this->once()) ->method('getByIdentifier') @@ -114,7 +115,7 @@ public function testAuthorizeSkipPicker(): void { /* Expected */ break; default: - throw new LogicException(); + throw new \LogicException(); } }); $this->appConfig @@ -150,8 +151,8 @@ public function testAuthorizeSkipPicker(): void { public function testAuthorizeWrongResponseType(): void { $client = new Client(); - $client->setClientIdentifier('MyClientIdentifier'); - $client->setRedirectUri('http://foo.bar'); + $client->clientIdentifier = 'MyClientIdentifier'; + $client->redirectUri = 'http://foo.bar'; $this->clientMapper ->expects($this->once()) ->method('getByIdentifier') @@ -167,8 +168,9 @@ public function testAuthorizeWrongResponseType(): void { public function testAuthorizeWithLegacyOcClient(): void { $client = new Client(); - $client->setClientIdentifier('MyClientIdentifier'); - $client->setRedirectUri('http://localhost:*'); + $client->name = 'MyClientName'; + $client->clientIdentifier = 'MyClientIdentifier'; + $client->redirectUri = 'http://localhost:*'; $this->clientMapper ->expects($this->once()) ->method('getByIdentifier') @@ -201,7 +203,8 @@ public function testAuthorizeWithLegacyOcClient(): void { public function testAuthorizeNotForwardingUntrustedURIs(): void { $client = new Client(); - $client->setClientIdentifier('MyClientIdentifier'); + $client->name = 'MyClientName'; + $client->clientIdentifier = 'MyClientIdentifier'; $this->clientMapper ->expects($this->once()) ->method('getByIdentifier') diff --git a/apps/oauth2/tests/Controller/OauthApiControllerTest.php b/apps/oauth2/tests/Controller/OauthApiControllerTest.php index 89a7eab290f90..615d38bee2554 100644 --- a/apps/oauth2/tests/Controller/OauthApiControllerTest.php +++ b/apps/oauth2/tests/Controller/OauthApiControllerTest.php @@ -43,7 +43,7 @@ abstract class RequestMock implements IRequest { public array $server = []; } -class OauthApiControllerTest extends TestCase { +final class OauthApiControllerTest extends TestCase { private IRequest&MockObject $request; private ICrypto&MockObject $crypto; private AccessTokenMapper&MockObject $accessTokenMapper; @@ -132,8 +132,8 @@ public function testGetTokenExpiredCode(): void { $expected->throttle(['invalid_request' => 'authorization_code_expired', 'expired_since' => $expiredSince]); $accessToken = new AccessToken(); - $accessToken->setClientId(42); - $accessToken->setCodeCreatedAt($codeCreatedAt); + $accessToken->clientId = 42; + $accessToken->codeCreatedAt = $codeCreatedAt; $this->accessTokenMapper->method('getByCode') ->with('validcode') @@ -158,9 +158,9 @@ public function testGetTokenWithCodeForActiveToken(): void { $expected->throttle(['invalid_request' => 'authorization_code_received_for_active_token']); $accessToken = new AccessToken(); - $accessToken->setClientId(42); - $accessToken->setCodeCreatedAt($codeCreatedAt); - $accessToken->setTokenCount(1); + $accessToken->clientId = 42; + $accessToken->codeCreatedAt = $codeCreatedAt; + $accessToken->tokenCount = 1; $this->accessTokenMapper->method('getByCode') ->with('validcode') @@ -185,8 +185,8 @@ public function testGetTokenClientDoesNotExist(): void { $expected->throttle(['invalid_request' => 'client not found', 'client_id' => 42]); $accessToken = new AccessToken(); - $accessToken->setClientId(42); - $accessToken->setCodeCreatedAt($codeCreatedAt); + $accessToken->clientId = 42; + $accessToken->codeCreatedAt = $codeCreatedAt; $this->accessTokenMapper->method('getByCode') ->with('validcode') @@ -225,7 +225,7 @@ public function testRefreshTokenClientDoesNotExist(): void { $expected->throttle(['invalid_request' => 'client not found', 'client_id' => 42]); $accessToken = new AccessToken(); - $accessToken->setClientId(42); + $accessToken->clientId = 42; $this->accessTokenMapper->method('getByCode') ->with('validrefresh') @@ -259,7 +259,7 @@ public function testRefreshTokenInvalidClient($clientId, $clientSecret): void { $expected->throttle(['invalid_client' => 'client ID or secret does not match']); $accessToken = new AccessToken(); - $accessToken->setClientId(42); + $accessToken->clientId = 42; $this->accessTokenMapper->method('getByCode') ->with('validrefresh') @@ -277,8 +277,8 @@ public function testRefreshTokenInvalidClient($clientId, $clientSecret): void { }); $client = new Client(); - $client->setClientIdentifier('clientId'); - $client->setSecret(bin2hex('hashedClientSecret')); + $client->clientIdentifier = 'clientId'; + $client->secret = bin2hex('hashedClientSecret'); $this->clientMapper->method('getByUid') ->with(42) ->willReturn($client); @@ -293,17 +293,17 @@ public function testRefreshTokenInvalidAppToken(): void { $expected->throttle(['invalid_request' => 'token is invalid']); $accessToken = new AccessToken(); - $accessToken->setClientId(42); - $accessToken->setTokenId(1337); - $accessToken->setEncryptedToken('encryptedToken'); + $accessToken->clientId = 42; + $accessToken->tokenId = 1337; + $accessToken->encryptedToken = 'encryptedToken'; $this->accessTokenMapper->method('getByCode') ->with('validrefresh') ->willReturn($accessToken); $client = new Client(); - $client->setClientIdentifier('clientId'); - $client->setSecret(bin2hex('hashedClientSecret')); + $client->clientIdentifier = 'clientId'; + $client->secret = bin2hex('hashedClientSecret'); $this->clientMapper->method('getByUid') ->with(42) ->willReturn($client); @@ -331,18 +331,18 @@ public function testRefreshTokenInvalidAppToken(): void { public function testRefreshTokenValidAppToken(): void { $accessToken = new AccessToken(); - $accessToken->setId(21); - $accessToken->setClientId(42); - $accessToken->setTokenId(1337); - $accessToken->setEncryptedToken('encryptedToken'); + $accessToken->id = 21; + $accessToken->clientId = 42; + $accessToken->tokenId = 1337; + $accessToken->encryptedToken = 'encryptedToken'; $this->accessTokenMapper->method('getByCode') ->with('validrefresh') ->willReturn($accessToken); $client = new Client(); - $client->setClientIdentifier('clientId'); - $client->setSecret(bin2hex('hashedClientSecret')); + $client->clientIdentifier = 'clientId'; + $client->secret = bin2hex('hashedClientSecret'); $this->clientMapper->method('getByUid') ->with(42) ->willReturn($client); @@ -441,18 +441,18 @@ public function testRefreshTokenValidAppToken(): void { public function testRefreshTokenValidAppTokenBasicAuth(): void { $accessToken = new AccessToken(); - $accessToken->setId(21); - $accessToken->setClientId(42); - $accessToken->setTokenId(1337); - $accessToken->setEncryptedToken('encryptedToken'); + $accessToken->id = 21; + $accessToken->clientId = 42; + $accessToken->tokenId = 1337; + $accessToken->encryptedToken = 'encryptedToken'; $this->accessTokenMapper->method('getByCode') ->with('validrefresh') ->willReturn($accessToken); $client = new Client(); - $client->setClientIdentifier('clientId'); - $client->setSecret(bin2hex('hashedClientSecret')); + $client->clientIdentifier = 'clientId'; + $client->secret = bin2hex('hashedClientSecret'); $this->clientMapper->method('getByUid') ->with(42) ->willReturn($client); @@ -554,18 +554,18 @@ public function testRefreshTokenValidAppTokenBasicAuth(): void { public function testRefreshTokenExpiredAppToken(): void { $accessToken = new AccessToken(); - $accessToken->setId(21); - $accessToken->setClientId(42); - $accessToken->setTokenId(1337); - $accessToken->setEncryptedToken('encryptedToken'); + $accessToken->id = 21; + $accessToken->clientId = 42; + $accessToken->tokenId = 1337; + $accessToken->encryptedToken = 'encryptedToken'; $this->accessTokenMapper->method('getByCode') ->with('validrefresh') ->willReturn($accessToken); $client = new Client(); - $client->setClientIdentifier('clientId'); - $client->setSecret(bin2hex('hashedClientSecret')); + $client->clientIdentifier = 'clientId'; + $client->secret = bin2hex('hashedClientSecret'); $this->clientMapper->method('getByUid') ->with(42) ->willReturn($client); @@ -669,18 +669,18 @@ public function testRefreshTokenRedeemedConcurrently(): void { $expected->throttle(['invalid_request' => 'refresh_token_already_redeemed']); $accessToken = new AccessToken(); - $accessToken->setId(21); - $accessToken->setClientId(42); - $accessToken->setTokenId(1337); - $accessToken->setEncryptedToken('encryptedToken'); + $accessToken->id = 21; + $accessToken->clientId = 42; + $accessToken->tokenId = 1337; + $accessToken->encryptedToken = 'encryptedToken'; $this->accessTokenMapper->method('getByCode') ->with('validrefresh') ->willReturn($accessToken); $client = new Client(); - $client->setClientIdentifier('clientId'); - $client->setSecret(bin2hex('hashedClientSecret')); + $client->clientIdentifier = 'clientId'; + $client->secret = bin2hex('hashedClientSecret'); $this->clientMapper->method('getByUid') ->with(42) ->willReturn($client); @@ -753,18 +753,18 @@ public function testRefreshTokenRedeemedConcurrently(): void { */ private function arrangeSuccessfulTokenExchange(): PublicKeyToken { $accessToken = new AccessToken(); - $accessToken->setId(21); - $accessToken->setClientId(42); - $accessToken->setTokenId(1337); - $accessToken->setEncryptedToken('encryptedToken'); + $accessToken->id = 21; + $accessToken->clientId = 42; + $accessToken->tokenId = 1337; + $accessToken->encryptedToken = 'encryptedToken'; $this->accessTokenMapper->method('getByCode') ->with('validrefresh') ->willReturn($accessToken); $client = new Client(); - $client->setClientIdentifier('clientId'); - $client->setSecret(bin2hex('hashedClientSecret')); + $client->clientIdentifier = 'clientId'; + $client->secret = bin2hex('hashedClientSecret'); $this->clientMapper->method('getByUid') ->with(42) ->willReturn($client); diff --git a/apps/oauth2/tests/Controller/SettingsControllerTest.php b/apps/oauth2/tests/Controller/SettingsControllerTest.php index 316974c709cbf..56d875bd64233 100644 --- a/apps/oauth2/tests/Controller/SettingsControllerTest.php +++ b/apps/oauth2/tests/Controller/SettingsControllerTest.php @@ -16,7 +16,7 @@ use Test\TestCase; #[Group(name: 'DB')] -class SettingsControllerTest extends TestCase { +final class SettingsControllerTest extends TestCase { public function testInvalidRedirectUri(): void { $settingsController = Server::get(SettingsController::class); $result = $settingsController->addClient('test', 'invalidurl'); diff --git a/apps/oauth2/tests/Db/AccessTokenMapperTest.php b/apps/oauth2/tests/Db/AccessTokenMapperTest.php index 6156cab095eeb..ea0423d72c0f6 100644 --- a/apps/oauth2/tests/Db/AccessTokenMapperTest.php +++ b/apps/oauth2/tests/Db/AccessTokenMapperTest.php @@ -10,30 +10,26 @@ use OCA\OAuth2\Db\AccessToken; use OCA\OAuth2\Db\AccessTokenMapper; use OCA\OAuth2\Exceptions\AccessTokenNotFoundException; -use OCP\AppFramework\Utility\ITimeFactory; -use OCP\IDBConnection; use OCP\Server; use Test\TestCase; #[\PHPUnit\Framework\Attributes\Group(name: 'DB')] -class AccessTokenMapperTest extends TestCase { - /** @var AccessTokenMapper */ - private $accessTokenMapper; +final class AccessTokenMapperTest extends TestCase { + private AccessTokenMapper $accessTokenMapper; protected function setUp(): void { parent::setUp(); - $this->accessTokenMapper = new AccessTokenMapper(Server::get(IDBConnection::class), Server::get(ITimeFactory::class)); + $this->accessTokenMapper = Server::get(AccessTokenMapper::class); } public function testGetByCode(): void { $this->accessTokenMapper->deleteByClientId(1234); $token = new AccessToken(); - $token->setClientId(1234); - $token->setTokenId(time()); - $token->setEncryptedToken('MyEncryptedToken'); - $token->setHashedCode(hash('sha512', 'MyAwesomeToken')); + $token->clientId = 1234; + $token->tokenId = time(); + $token->encryptedToken = 'MyEncryptedToken'; + $token->hashedCode = hash('sha512', 'MyAwesomeToken'); $this->accessTokenMapper->insert($token); - $token->resetUpdatedFields(); $result = $this->accessTokenMapper->getByCode('MyAwesomeToken'); $this->assertEquals($token, $result); @@ -45,12 +41,11 @@ public function testDeleteByClientId(): void { $this->accessTokenMapper->deleteByClientId(1234); $token = new AccessToken(); - $token->setClientId(1234); - $token->setTokenId(time()); - $token->setEncryptedToken('MyEncryptedToken'); - $token->setHashedCode(hash('sha512', 'MyAwesomeToken')); + $token->clientId = 1234; + $token->tokenId = time(); + $token->encryptedToken = 'MyEncryptedToken'; + $token->hashedCode = hash('sha512', 'MyAwesomeToken'); $this->accessTokenMapper->insert($token); - $token->resetUpdatedFields(); $this->accessTokenMapper->deleteByClientId(1234); $this->accessTokenMapper->getByCode('MyAwesomeToken'); } diff --git a/apps/oauth2/tests/Db/ClientMapperTest.php b/apps/oauth2/tests/Db/ClientMapperTest.php index ce1e31867a2af..2122a5ca95a0f 100644 --- a/apps/oauth2/tests/Db/ClientMapperTest.php +++ b/apps/oauth2/tests/Db/ClientMapperTest.php @@ -15,13 +15,13 @@ use Test\TestCase; #[\PHPUnit\Framework\Attributes\Group(name: 'DB')] -class ClientMapperTest extends TestCase { +final class ClientMapperTest extends TestCase { /** @var ClientMapper */ private $clientMapper; protected function setUp(): void { parent::setUp(); - $this->clientMapper = new ClientMapper(Server::get(IDBConnection::class)); + $this->clientMapper = Server::get(ClientMapper::class); } protected function tearDown(): void { @@ -33,12 +33,11 @@ protected function tearDown(): void { public function testGetByIdentifier(): void { $client = new Client(); - $client->setClientIdentifier('MyAwesomeClientIdentifier'); - $client->setName('Client Name'); - $client->setRedirectUri('https://example.com/'); - $client->setSecret('TotallyNotSecret'); + $client->clientIdentifier = 'MyAwesomeClientIdentifier'; + $client->name = 'Client Name'; + $client->redirectUri = 'https://example.com/'; + $client->secret = 'TotallyNotSecret'; $this->clientMapper->insert($client); - $client->resetUpdatedFields(); $this->assertEquals($client, $this->clientMapper->getByIdentifier('MyAwesomeClientIdentifier')); } @@ -50,13 +49,12 @@ public function testGetByIdentifierNotExisting(): void { public function testGetByUid(): void { $client = new Client(); - $client->setClientIdentifier('MyNewClient'); - $client->setName('Client Name'); - $client->setRedirectUri('https://example.com/'); - $client->setSecret('TotallyNotSecret'); + $client->clientIdentifier = 'MyNewClient'; + $client->name = 'Client Name'; + $client->redirectUri = 'https://example.com/'; + $client->secret = 'TotallyNotSecret'; $this->clientMapper->insert($client); - $client->resetUpdatedFields(); - $this->assertEquals($client, $this->clientMapper->getByUid($client->getId())); + $this->assertEquals($client, $this->clientMapper->getByUid($client->id)); } public function testGetByUidNotExisting(): void { @@ -66,15 +64,15 @@ public function testGetByUidNotExisting(): void { } public function testGetClients(): void { - $this->assertSame('array', gettype($this->clientMapper->getClients())); + $this->assertInstanceOf(\Generator::class, $this->clientMapper->getClients()); } public function testInsertLongEncryptedSecret(): void { $client = new Client(); - $client->setClientIdentifier('MyNewClient'); - $client->setName('Client Name'); - $client->setRedirectUri('https://example.com/'); - $client->setSecret('b81dc8e2dc178817bf28ca7b37265aa96559ca02e6dcdeb74b42221d096ed5ef63681e836ae0ba1077b5fb5e6c2fa7748c78463f66fe0110c8dcb8dd7eb0305b16d0cd993e2ae275879994a2abf88c68|e466d9befa6b0102341458e45ecd551a|013af9e277374483123437f180a3b0371a411ad4f34c451547909769181a7d7cc191f0f5c2de78376d124dd7751b8c9660aabdd913f5e071fc6b819ba2e3d919|3'); + $client->clientIdentifier = 'MyNewClient'; + $client->name = 'Client Name'; + $client->redirectUri = 'https://example.com/'; + $client->secret = 'b81dc8e2dc178817bf28ca7b37265aa96559ca02e6dcdeb74b42221d096ed5ef63681e836ae0ba1077b5fb5e6c2fa7748c78463f66fe0110c8dcb8dd7eb0305b16d0cd993e2ae275879994a2abf88c68|e466d9befa6b0102341458e45ecd551a|013af9e277374483123437f180a3b0371a411ad4f34c451547909769181a7d7cc191f0f5c2de78376d124dd7751b8c9660aabdd913f5e071fc6b819ba2e3d919|3'; $this->clientMapper->insert($client); $this->assertTrue(true); } diff --git a/apps/oauth2/tests/Service/ClientServiceTest.php b/apps/oauth2/tests/Service/ClientServiceTest.php index bc9986e0b21f9..eeb35f7247d20 100644 --- a/apps/oauth2/tests/Service/ClientServiceTest.php +++ b/apps/oauth2/tests/Service/ClientServiceTest.php @@ -24,7 +24,7 @@ use Test\TestCase; #[\PHPUnit\Framework\Attributes\Group(name: 'DB')] -class ClientServiceTest extends TestCase { +final class ClientServiceTest extends TestCase { private ClientMapper&MockObject $clientMapper; private ISecureRandom&MockObject $secureRandom; private AccessTokenMapper&MockObject $accessTokenMapper; @@ -70,21 +70,21 @@ public function testAddClient(): void { ->willReturn('MyHashedSecret'); $client = new Client(); - $client->setName('My Client Name'); - $client->setRedirectUri('https://example.com/'); - $client->setSecret(bin2hex('MyHashedSecret')); - $client->setClientIdentifier('MyClientIdentifier'); + $client->name = 'My Client Name'; + $client->redirectUri = 'https://example.com/'; + $client->secret = bin2hex('MyHashedSecret'); + $client->clientIdentifier = 'MyClientIdentifier'; $this->clientMapper ->expects($this->once()) ->method('insert') ->with($this->callback(function (Client $c) { - return $c->getName() === 'My Client Name' - && $c->getRedirectUri() === 'https://example.com/' - && $c->getSecret() === bin2hex('MyHashedSecret') - && $c->getClientIdentifier() === 'MyClientIdentifier'; + return $c->name === 'My Client Name' + && $c->redirectUri === 'https://example.com/' + && $c->secret === bin2hex('MyHashedSecret') + && $c->clientIdentifier === 'MyClientIdentifier'; }))->willReturnCallback(function (Client $c) { - $c->setId(42); + $c->id = 42; return $c; }); @@ -125,11 +125,11 @@ public function testDeleteClient(): void { ->method('invalidateTokenById'); $client = new Client(); - $client->setId(123); - $client->setName('My Client Name'); - $client->setRedirectUri('https://example.com/'); - $client->setSecret(bin2hex('MyHashedSecret')); - $client->setClientIdentifier('MyClientIdentifier'); + $client->id = 123; + $client->name = 'My Client Name'; + $client->redirectUri = 'https://example.com/'; + $client->secret = bin2hex('MyHashedSecret'); + $client->clientIdentifier = 'MyClientIdentifier'; $this->clientMapper ->method('getByUid') @@ -164,11 +164,11 @@ public function testDeleteClientPreservesWipePendingToken(): void { $user->updateLastLoginTimestamp(); $client = new Client(); - $client->setId(456); - $client->setName('My Client Name'); - $client->setRedirectUri('https://example.com/'); - $client->setSecret(bin2hex('MyHashedSecret')); - $client->setClientIdentifier('MyClientIdentifier'); + $client->id = 456; + $client->name = 'My Client Name'; + $client->redirectUri = 'https://example.com/'; + $client->secret = bin2hex('MyHashedSecret'); + $client->clientIdentifier = 'MyClientIdentifier'; // Token marked for wipe with a matching client name: must NOT be invalidated. $wipeToken = $this->createMock(IToken::class); diff --git a/apps/oauth2/tests/Settings/AdminTest.php b/apps/oauth2/tests/Settings/AdminTest.php index 0060bc0dacf61..8839f76cde0d8 100644 --- a/apps/oauth2/tests/Settings/AdminTest.php +++ b/apps/oauth2/tests/Settings/AdminTest.php @@ -13,20 +13,14 @@ use OCP\AppFramework\Services\IInitialState; use OCP\IURLGenerator; use PHPUnit\Framework\MockObject\MockObject; -use Psr\Log\LoggerInterface; use Test\TestCase; -class AdminTest extends TestCase { - - /** @var Admin|MockObject */ - private $admin; - - /** @var IInitialState|MockObject */ - private $initialState; - - /** @var ClientMapper|MockObject */ - private $clientMapper; +final class AdminTest extends TestCase { + private Admin $admin; + private IInitialState&MockObject $initialState; + private ClientMapper&MockObject $clientMapper; + #[\Override] protected function setUp(): void { parent::setUp(); @@ -37,7 +31,6 @@ protected function setUp(): void { $this->initialState, $this->clientMapper, $this->createMock(IURLGenerator::class), - $this->createMock(LoggerInterface::class) ); } From 2978c51c726a16214169fee3372f27eb2948adb0 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Sat, 15 Aug 2026 10:03:01 +0200 Subject: [PATCH 2/6] refactor: Make oauth2 app pass psalm:strict Signed-off-by: Carl Schwan --- .../CleanupExpiredAuthorizationCode.php | 2 +- apps/oauth2/lib/Command/AddClient.php | 2 +- apps/oauth2/lib/Command/DeleteClient.php | 2 +- .../lib/Command/ImportLegacyOcClient.php | 2 +- .../Controller/LoginRedirectorController.php | 3 +- .../lib/Controller/OauthApiController.php | 35 ++++++++++++++++--- .../lib/Controller/SettingsController.php | 8 +++-- apps/oauth2/lib/Db/AccessToken.php | 5 ++- apps/oauth2/lib/Db/AccessTokenMapper.php | 1 + apps/oauth2/lib/Db/Client.php | 5 ++- apps/oauth2/lib/Db/ClientMapper.php | 1 + .../AccessTokenNotFoundException.php | 2 +- .../Exceptions/ClientNotFoundException.php | 2 +- .../lib/Migration/SetTokenExpiration.php | 13 ++++--- .../Version010401Date20181207190718.php | 12 ++----- .../Version010402Date20190107124745.php | 2 +- .../Version011601Date20230522143227.php | 10 +++--- .../Version011602Date20230613160650.php | 4 +-- .../Version011603Date20230620111039.php | 3 +- .../Version011901Date20240829164356.php | 6 ++-- apps/oauth2/lib/Service/ClientService.php | 6 ++-- apps/oauth2/lib/Settings/Admin.php | 2 +- .../LoginRedirectorControllerTest.php | 1 + .../Controller/OauthApiControllerTest.php | 19 +++++----- .../oauth2/tests/Db/AccessTokenMapperTest.php | 1 + apps/oauth2/tests/Db/ClientMapperTest.php | 5 +-- .../tests/Service/ClientServiceTest.php | 3 ++ psalm-strict.xml | 1 + 28 files changed, 96 insertions(+), 62 deletions(-) diff --git a/apps/oauth2/lib/BackgroundJob/CleanupExpiredAuthorizationCode.php b/apps/oauth2/lib/BackgroundJob/CleanupExpiredAuthorizationCode.php index d2a03b4e60093..3d7b26546c1ad 100644 --- a/apps/oauth2/lib/BackgroundJob/CleanupExpiredAuthorizationCode.php +++ b/apps/oauth2/lib/BackgroundJob/CleanupExpiredAuthorizationCode.php @@ -15,7 +15,7 @@ use OCP\DB\Exception; use Psr\Log\LoggerInterface; -class CleanupExpiredAuthorizationCode extends TimedJob { +final class CleanupExpiredAuthorizationCode extends TimedJob { public function __construct( ITimeFactory $timeFactory, diff --git a/apps/oauth2/lib/Command/AddClient.php b/apps/oauth2/lib/Command/AddClient.php index edf384b45372e..8962f1e4df4d5 100644 --- a/apps/oauth2/lib/Command/AddClient.php +++ b/apps/oauth2/lib/Command/AddClient.php @@ -17,7 +17,7 @@ use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Output\OutputInterface; -class AddClient extends Base { +final class AddClient extends Base { private const string ARGUMENT_CLIENT_NAME = 'client-name'; private const string ARGUMENT_CLIENT_REDIRECT_URI = 'client-redirect-uri'; diff --git a/apps/oauth2/lib/Command/DeleteClient.php b/apps/oauth2/lib/Command/DeleteClient.php index 6eb15421c88df..12440aeb47753 100644 --- a/apps/oauth2/lib/Command/DeleteClient.php +++ b/apps/oauth2/lib/Command/DeleteClient.php @@ -17,7 +17,7 @@ use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Output\OutputInterface; -class DeleteClient extends Base { +final class DeleteClient extends Base { private const string ARGUMENT_CLIENT_ID = 'client-id'; public function __construct( diff --git a/apps/oauth2/lib/Command/ImportLegacyOcClient.php b/apps/oauth2/lib/Command/ImportLegacyOcClient.php index ba237936fb619..5a369010d1ed1 100644 --- a/apps/oauth2/lib/Command/ImportLegacyOcClient.php +++ b/apps/oauth2/lib/Command/ImportLegacyOcClient.php @@ -18,7 +18,7 @@ use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Output\OutputInterface; -class ImportLegacyOcClient extends Command { +final class ImportLegacyOcClient extends Command { private const string ARGUMENT_CLIENT_ID = 'client-id'; private const string ARGUMENT_CLIENT_SECRET = 'client-secret'; diff --git a/apps/oauth2/lib/Controller/LoginRedirectorController.php b/apps/oauth2/lib/Controller/LoginRedirectorController.php index 4252580fb315f..e180f3b65f58c 100644 --- a/apps/oauth2/lib/Controller/LoginRedirectorController.php +++ b/apps/oauth2/lib/Controller/LoginRedirectorController.php @@ -29,7 +29,7 @@ use OCP\Security\ISecureRandom; #[OpenAPI(scope: OpenAPI::SCOPE_DEFAULT)] -class LoginRedirectorController extends Controller { +final class LoginRedirectorController extends Controller { public function __construct( string $appName, IRequest $request, @@ -88,6 +88,7 @@ public function authorize( if (in_array($client->name, $this->appConfig->getValueArray('oauth2', 'skipAuthPickerApplications', []))) { /** @see ClientFlowLoginController::showAuthPickerPage **/ + /** @psalm-suppress DeprecatedMethod No Randomizer-based replacement is mockable in tests yet. */ $stateToken = $this->random->generate( 64, ISecureRandom::CHAR_LOWER . ISecureRandom::CHAR_UPPER . ISecureRandom::CHAR_DIGITS diff --git a/apps/oauth2/lib/Controller/OauthApiController.php b/apps/oauth2/lib/Controller/OauthApiController.php index bddf1a5f349a8..a7a8b420f1405 100644 --- a/apps/oauth2/lib/Controller/OauthApiController.php +++ b/apps/oauth2/lib/Controller/OauthApiController.php @@ -40,9 +40,9 @@ use Psr\Log\LoggerInterface; #[OpenAPI(scope: OpenAPI::SCOPE_DEFAULT)] -class OauthApiController extends Controller { +final class OauthApiController extends Controller { // the authorization code expires after 10 minutes - public const AUTHORIZATION_CODE_EXPIRES_AFTER = 10 * 60; + public const int AUTHORIZATION_CODE_EXPIRES_AFTER = 10 * 60; public function __construct( string $appName, @@ -69,6 +69,7 @@ public function __construct( * Get a token * * @param 'authorization_code'|'refresh_token' $grant_type Token type that should be granted + * @psalm-param string $grant_type * @param ?string $code Code of the flow * @param ?string $refresh_token Refresh token * @param ?string $client_id Client ID @@ -101,6 +102,14 @@ public function getToken( $code = $refresh_token; } + if ($code === null) { + $response = new JSONResponse([ + 'error' => 'invalid_request', + ], Http::STATUS_BAD_REQUEST); + $response->throttle(['invalid_request' => 'token not found']); + return $response; + } + try { $accessToken = $this->accessTokenMapper->getByCode($code); } catch (AccessTokenNotFoundException $e) { @@ -148,9 +157,21 @@ public function getToken( return $response; } + /** + * @psalm-suppress NoInterfaceProperties, MixedArrayAccess + * IRequest exposes $server via a magic @property-read for the request's $_SERVER superglobal. + */ if (isset($this->request->server['PHP_AUTH_USER'])) { - $client_id = $this->request->server['PHP_AUTH_USER']; - $client_secret = $this->request->server['PHP_AUTH_PW']; + $client_id = (string)$this->request->server['PHP_AUTH_USER']; + $client_secret = (string)$this->request->server['PHP_AUTH_PW']; + } + + if ($client_secret === null) { + $response = new JSONResponse([ + 'error' => 'invalid_client', + ], Http::STATUS_BAD_REQUEST); + $response->throttle(['invalid_client' => 'client ID or secret does not match']); + return $response; } try { @@ -190,7 +211,9 @@ public function getToken( } // Rotate the apptoken (so the old one becomes invalid basically) + /** @psalm-suppress DeprecatedMethod No Randomizer-based replacement is mockable in tests yet. */ $newToken = $this->secureRandom->generate(72, ISecureRandom::CHAR_ALPHANUMERIC); + /** @psalm-suppress DeprecatedMethod No Randomizer-based replacement is mockable in tests yet. */ $newCode = $this->secureRandom->generate(128, ISecureRandom::CHAR_ALPHANUMERIC); $newEncryptedToken = $this->crypto->encrypt($newToken, $newCode); $redeemedThrottleReason = $grant_type === 'authorization_code' @@ -277,7 +300,9 @@ private function pushTokenToSecondary(IToken $appToken, string $newToken, ?int $ } try { - return $globalScaleService->sendToSecondary($user, $this->urlGenerator->linkToRoute('oauth2.OauthApi.pushToken'), [ + /** @var non-empty-string $pushRouteUrl */ + $pushRouteUrl = $this->urlGenerator->linkToRoute('oauth2.OauthApi.pushToken'); + return $globalScaleService->sendToSecondary($user, $pushRouteUrl, [ 'uid' => $appToken->getUID(), 'loginName' => $appToken->getLoginName(), 'name' => $appToken->getName(), diff --git a/apps/oauth2/lib/Controller/SettingsController.php b/apps/oauth2/lib/Controller/SettingsController.php index 0b0adbb1cb1b6..4a46f0f7c839a 100644 --- a/apps/oauth2/lib/Controller/SettingsController.php +++ b/apps/oauth2/lib/Controller/SettingsController.php @@ -17,7 +17,7 @@ use OCP\IL10N; use OCP\IRequest; -class SettingsController extends Controller { +final class SettingsController extends Controller { public function __construct( string $appName, IRequest $request, @@ -30,7 +30,11 @@ public function __construct( #[PasswordConfirmationRequired(strict: true)] public function addClient(string $name, string $redirectUri): JSONResponse { - if (filter_var($redirectUri, FILTER_VALIDATE_URL) === false) { + if ($name === '') { + return new JSONResponse(['message' => $this->l->t('Client name cannot be empty.')], Http::STATUS_BAD_REQUEST); + } + + if ($redirectUri === '' || filter_var($redirectUri, FILTER_VALIDATE_URL) === false) { return new JSONResponse(['message' => $this->l->t('Your redirect URL needs to be a full URL for example: https://yourdomain.com/path')], Http::STATUS_BAD_REQUEST); } diff --git a/apps/oauth2/lib/Db/AccessToken.php b/apps/oauth2/lib/Db/AccessToken.php index cb3170cc4b5b1..445e86d74214e 100644 --- a/apps/oauth2/lib/Db/AccessToken.php +++ b/apps/oauth2/lib/Db/AccessToken.php @@ -14,8 +14,11 @@ use OCP\AppFramework\ORM\Attribute\Id; use OCP\DB\Schema\ColumnType; +/** + * @psalm-suppress MissingConstructor ORM based hydration + */ #[Entity(name: 'oauth2_access_tokens')] -class AccessToken { +final class AccessToken { #[Id, Column(name: 'id', type: ColumnType::Integer)] public int $id; diff --git a/apps/oauth2/lib/Db/AccessTokenMapper.php b/apps/oauth2/lib/Db/AccessTokenMapper.php index dae3922df4c9e..50139a994dd14 100644 --- a/apps/oauth2/lib/Db/AccessTokenMapper.php +++ b/apps/oauth2/lib/Db/AccessTokenMapper.php @@ -22,6 +22,7 @@ /** * @template-extends Repository + * @psalm-suppress ClassMustBeFinal For unit tests */ class AccessTokenMapper extends Repository { const string entityClass = AccessToken::class; diff --git a/apps/oauth2/lib/Db/Client.php b/apps/oauth2/lib/Db/Client.php index ed74686919c4e..92d6269a6d002 100644 --- a/apps/oauth2/lib/Db/Client.php +++ b/apps/oauth2/lib/Db/Client.php @@ -15,8 +15,11 @@ use OCP\AppFramework\ORM\Attribute\Id; use OCP\DB\Schema\ColumnType; +/** + * @psalm-suppress MissingConstructor ORM based hydration + */ #[Entity(name: 'oauth2_clients')] -class Client { +final class Client { #[Id, Column(name: 'id', type: ColumnType::Integer)] public int $id; diff --git a/apps/oauth2/lib/Db/ClientMapper.php b/apps/oauth2/lib/Db/ClientMapper.php index e6a0000d7972f..b8bf91b46ae34 100644 --- a/apps/oauth2/lib/Db/ClientMapper.php +++ b/apps/oauth2/lib/Db/ClientMapper.php @@ -19,6 +19,7 @@ /** * @template-extends Repository + * @psalm-suppress ClassMustBeFinal For unit tests */ class ClientMapper extends Repository { public const string entityClass = Client::class; diff --git a/apps/oauth2/lib/Exceptions/AccessTokenNotFoundException.php b/apps/oauth2/lib/Exceptions/AccessTokenNotFoundException.php index 8dfa3e43d94b5..ce64d4949de49 100644 --- a/apps/oauth2/lib/Exceptions/AccessTokenNotFoundException.php +++ b/apps/oauth2/lib/Exceptions/AccessTokenNotFoundException.php @@ -9,5 +9,5 @@ namespace OCA\OAuth2\Exceptions; -class AccessTokenNotFoundException extends \Exception { +final class AccessTokenNotFoundException extends \Exception { } diff --git a/apps/oauth2/lib/Exceptions/ClientNotFoundException.php b/apps/oauth2/lib/Exceptions/ClientNotFoundException.php index 7d6c48eda91a4..4bc200f7e2be7 100644 --- a/apps/oauth2/lib/Exceptions/ClientNotFoundException.php +++ b/apps/oauth2/lib/Exceptions/ClientNotFoundException.php @@ -9,5 +9,5 @@ namespace OCA\OAuth2\Exceptions; -class ClientNotFoundException extends \Exception { +final class ClientNotFoundException extends \Exception { } diff --git a/apps/oauth2/lib/Migration/SetTokenExpiration.php b/apps/oauth2/lib/Migration/SetTokenExpiration.php index edf363313762b..cca900f42ea77 100644 --- a/apps/oauth2/lib/Migration/SetTokenExpiration.php +++ b/apps/oauth2/lib/Migration/SetTokenExpiration.php @@ -10,14 +10,13 @@ namespace OCA\OAuth2\Migration; use OC\Authentication\Token\IProvider as TokenProvider; -use OCA\OAuth2\Db\AccessToken; use OCP\AppFramework\Utility\ITimeFactory; use OCP\Authentication\Exceptions\InvalidTokenException; use OCP\IDBConnection; use OCP\Migration\IOutput; use OCP\Migration\IRepairStep; -class SetTokenExpiration implements IRepairStep { +final class SetTokenExpiration implements IRepairStep { public function __construct( private IDBConnection $connection, @@ -32,17 +31,17 @@ public function getName(): string { } #[\Override] - public function run(IOutput $output) { + public function run(IOutput $output): void { $qb = $this->connection->getQueryBuilder(); - $qb->select('*') + $qb->select('token_id') ->from('oauth2_access_tokens'); $cursor = $qb->executeQuery(); - while ($row = $cursor->fetchAssociative()) { - $token = AccessToken::fromRow($row); + while (($row = $cursor->fetchAssociative()) !== false) { + $tokenId = (int)$row['token_id']; try { - $appToken = $this->tokenProvider->getTokenById($token->getTokenId()); + $appToken = $this->tokenProvider->getTokenById($tokenId); $appToken->setExpires($this->time->getTime() + 3600); $this->tokenProvider->updateToken($appToken); } catch (InvalidTokenException $e) { diff --git a/apps/oauth2/lib/Migration/Version010401Date20181207190718.php b/apps/oauth2/lib/Migration/Version010401Date20181207190718.php index 46ac0a4034790..3d1a984f09a68 100644 --- a/apps/oauth2/lib/Migration/Version010401Date20181207190718.php +++ b/apps/oauth2/lib/Migration/Version010401Date20181207190718.php @@ -14,17 +14,9 @@ use OCP\Migration\IOutput; use OCP\Migration\SimpleMigrationStep; -class Version010401Date20181207190718 extends SimpleMigrationStep { - - /** - * @param IOutput $output - * @param Closure $schemaClosure The `\Closure` returns a `ISchemaWrapper` - * @param array $options - * @return null|ISchemaWrapper - */ +final class Version010401Date20181207190718 extends SimpleMigrationStep { #[\Override] - public function changeSchema(IOutput $output, Closure $schemaClosure, array $options) { - /** @var ISchemaWrapper $schema */ + public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper { $schema = $schemaClosure(); if (!$schema->hasTable('oauth2_clients')) { diff --git a/apps/oauth2/lib/Migration/Version010402Date20190107124745.php b/apps/oauth2/lib/Migration/Version010402Date20190107124745.php index ac203740ab587..09a2ae2403a34 100644 --- a/apps/oauth2/lib/Migration/Version010402Date20190107124745.php +++ b/apps/oauth2/lib/Migration/Version010402Date20190107124745.php @@ -14,7 +14,7 @@ use OCP\Migration\IOutput; use OCP\Migration\SimpleMigrationStep; -class Version010402Date20190107124745 extends SimpleMigrationStep { +final class Version010402Date20190107124745 extends SimpleMigrationStep { /** * @param IOutput $output diff --git a/apps/oauth2/lib/Migration/Version011601Date20230522143227.php b/apps/oauth2/lib/Migration/Version011601Date20230522143227.php index f667b23e6077e..08d706f6e23ce 100644 --- a/apps/oauth2/lib/Migration/Version011601Date20230522143227.php +++ b/apps/oauth2/lib/Migration/Version011601Date20230522143227.php @@ -10,14 +10,13 @@ namespace OCA\OAuth2\Migration; use Closure; -use OCP\DB\ISchemaWrapper; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IDBConnection; use OCP\Migration\IOutput; use OCP\Migration\SimpleMigrationStep; use OCP\Security\ICrypto; -class Version011601Date20230522143227 extends SimpleMigrationStep { +final class Version011601Date20230522143227 extends SimpleMigrationStep { public function __construct( private IDBConnection $connection, @@ -27,7 +26,6 @@ public function __construct( #[\Override] public function changeSchema(IOutput $output, Closure $schemaClosure, array $options) { - /** @var ISchemaWrapper $schema */ $schema = $schemaClosure(); if ($schema->hasTable('oauth2_clients')) { @@ -43,7 +41,7 @@ public function changeSchema(IOutput $output, Closure $schemaClosure, array $opt } #[\Override] - public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options) { + public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options): void { $qbUpdate = $this->connection->getQueryBuilder(); $qbUpdate->update('oauth2_clients') ->set('secret', $qbUpdate->createParameter('updateSecret')) @@ -56,8 +54,8 @@ public function postSchemaChange(IOutput $output, Closure $schemaClosure, array ->from('oauth2_clients'); $req = $qbSelect->executeQuery(); while ($row = $req->fetchAssociative()) { - $id = $row['id']; - $secret = $row['secret']; + $id = (int)$row['id']; + $secret = (string)$row['secret']; $encryptedSecret = $this->crypto->encrypt($secret); $qbUpdate->setParameter('updateSecret', $encryptedSecret, IQueryBuilder::PARAM_STR); $qbUpdate->setParameter('updateId', $id, IQueryBuilder::PARAM_INT); diff --git a/apps/oauth2/lib/Migration/Version011602Date20230613160650.php b/apps/oauth2/lib/Migration/Version011602Date20230613160650.php index 87233a09dfc2e..6b01a7505cd5c 100644 --- a/apps/oauth2/lib/Migration/Version011602Date20230613160650.php +++ b/apps/oauth2/lib/Migration/Version011602Date20230613160650.php @@ -10,11 +10,10 @@ namespace OCA\OAuth2\Migration; use Closure; -use OCP\DB\ISchemaWrapper; use OCP\Migration\IOutput; use OCP\Migration\SimpleMigrationStep; -class Version011602Date20230613160650 extends SimpleMigrationStep { +final class Version011602Date20230613160650 extends SimpleMigrationStep { public function __construct( ) { @@ -22,7 +21,6 @@ public function __construct( #[\Override] public function changeSchema(IOutput $output, Closure $schemaClosure, array $options) { - /** @var ISchemaWrapper $schema */ $schema = $schemaClosure(); if ($schema->hasTable('oauth2_clients')) { diff --git a/apps/oauth2/lib/Migration/Version011603Date20230620111039.php b/apps/oauth2/lib/Migration/Version011603Date20230620111039.php index c2d1f2cab013f..b23987c2573bd 100644 --- a/apps/oauth2/lib/Migration/Version011603Date20230620111039.php +++ b/apps/oauth2/lib/Migration/Version011603Date20230620111039.php @@ -17,7 +17,7 @@ use OCP\Migration\IOutput; use OCP\Migration\SimpleMigrationStep; -class Version011603Date20230620111039 extends SimpleMigrationStep { +final class Version011603Date20230620111039 extends SimpleMigrationStep { public function __construct( private IDBConnection $connection, @@ -26,7 +26,6 @@ public function __construct( #[\Override] public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper { - /** @var ISchemaWrapper $schema */ $schema = $schemaClosure(); if ($schema->hasTable('oauth2_access_tokens')) { diff --git a/apps/oauth2/lib/Migration/Version011901Date20240829164356.php b/apps/oauth2/lib/Migration/Version011901Date20240829164356.php index dd022c87451cc..1c0d8978b3968 100644 --- a/apps/oauth2/lib/Migration/Version011901Date20240829164356.php +++ b/apps/oauth2/lib/Migration/Version011901Date20240829164356.php @@ -16,7 +16,7 @@ use OCP\Migration\SimpleMigrationStep; use OCP\Security\ICrypto; -class Version011901Date20240829164356 extends SimpleMigrationStep { +final class Version011901Date20240829164356 extends SimpleMigrationStep { public function __construct( private IDBConnection $connection, @@ -38,8 +38,8 @@ public function postSchemaChange(IOutput $output, Closure $schemaClosure, array ->from('oauth2_clients'); $req = $qbSelect->executeQuery(); while ($row = $req->fetchAssociative()) { - $id = $row['id']; - $storedEncryptedSecret = $row['secret']; + $id = (int)$row['id']; + $storedEncryptedSecret = (string)$row['secret']; $secret = $this->crypto->decrypt($storedEncryptedSecret); $hashedSecret = bin2hex($this->crypto->calculateHMAC($secret)); $qbUpdate->setParameter('updateSecret', $hashedSecret, IQueryBuilder::PARAM_STR); diff --git a/apps/oauth2/lib/Service/ClientService.php b/apps/oauth2/lib/Service/ClientService.php index 6cd6e5192a02e..b8c41e845c32f 100644 --- a/apps/oauth2/lib/Service/ClientService.php +++ b/apps/oauth2/lib/Service/ClientService.php @@ -22,8 +22,8 @@ use OCP\Security\ISecureRandom; use Psr\Log\LoggerInterface; -class ClientService { - public const validChars = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789'; +final class ClientService { + public const string validChars = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789'; public function __construct( private readonly ISecureRandom $secureRandom, @@ -51,9 +51,11 @@ public function addClient(string $name, string $redirectUri): array { $client = new Client(); $client->name = $name; $client->redirectUri = $redirectUri; + /** @psalm-suppress DeprecatedMethod No Randomizer-based replacement is mockable in tests yet. */ $secret = $this->secureRandom->generate(64, self::validChars); $hashedSecret = bin2hex($this->crypto->calculateHMAC($secret)); $client->secret = $hashedSecret; + /** @psalm-suppress DeprecatedMethod No Randomizer-based replacement is mockable in tests yet. */ $client->clientIdentifier = $this->secureRandom->generate(64, self::validChars); $client = $this->clientMapper->insert($client); diff --git a/apps/oauth2/lib/Settings/Admin.php b/apps/oauth2/lib/Settings/Admin.php index b32164df03f80..3579160eaa871 100644 --- a/apps/oauth2/lib/Settings/Admin.php +++ b/apps/oauth2/lib/Settings/Admin.php @@ -16,7 +16,7 @@ use OCP\Settings\ISettings; use OCP\Util; -class Admin implements ISettings { +final class Admin implements ISettings { public function __construct( private IInitialState $initialState, diff --git a/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php b/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php index ebbb0ca797079..eae2b72ac3073 100644 --- a/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php +++ b/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php @@ -37,6 +37,7 @@ final class LoginRedirectorControllerTest extends TestCase { private LoginRedirectorController $loginRedirectorController; + #[\Override] protected function setUp(): void { parent::setUp(); diff --git a/apps/oauth2/tests/Controller/OauthApiControllerTest.php b/apps/oauth2/tests/Controller/OauthApiControllerTest.php index 615d38bee2554..afd54d48d459e 100644 --- a/apps/oauth2/tests/Controller/OauthApiControllerTest.php +++ b/apps/oauth2/tests/Controller/OauthApiControllerTest.php @@ -7,8 +7,6 @@ namespace OCA\OAuth2\Tests\Controller; -use OC\Authentication\Exceptions\ExpiredTokenException; -use OC\Authentication\Exceptions\InvalidTokenException; use OC\Authentication\Token\IProvider as TokenProvider; use OC\Authentication\Token\PublicKeyToken; use OCA\OAuth2\Controller\OauthApiController; @@ -21,6 +19,8 @@ use OCP\AppFramework\Http; use OCP\AppFramework\Http\JSONResponse; use OCP\AppFramework\Utility\ITimeFactory; +use OCP\Authentication\Exceptions\ExpiredTokenException; +use OCP\Authentication\Exceptions\InvalidTokenException; use OCP\Authentication\Token\IToken; use OCP\GlobalScale\IConfig as GlobalScaleConfig; use OCP\GlobalScale\IGlobalScaleService; @@ -44,7 +44,7 @@ abstract class RequestMock implements IRequest { } final class OauthApiControllerTest extends TestCase { - private IRequest&MockObject $request; + private RequestMock&MockObject $request; private ICrypto&MockObject $crypto; private AccessTokenMapper&MockObject $accessTokenMapper; private ClientMapper&MockObject $clientMapper; @@ -61,6 +61,7 @@ final class OauthApiControllerTest extends TestCase { private ContainerInterface&MockObject $container; private OauthApiController $oauthApiController; + #[\Override] protected function setUp(): void { parent::setUp(); @@ -238,7 +239,7 @@ public function testRefreshTokenClientDoesNotExist(): void { $this->assertEquals($expected, $this->oauthApiController->getToken('refresh_token', null, 'validrefresh', null, null)); } - public static function invalidClientProvider() { + public static function invalidClientProvider(): array { return [ ['invalidClientId', 'invalidClientSecret'], ['clientId', 'invalidClientSecret'], @@ -368,7 +369,7 @@ public function testRefreshTokenValidAppToken(): void { ->with($accessToken); $this->secureRandom->method('generate') - ->willReturnCallback(function ($len) { + ->willReturnCallback(function (int $len) { return 'random' . $len; }); @@ -478,7 +479,7 @@ public function testRefreshTokenValidAppTokenBasicAuth(): void { ->with($accessToken); $this->secureRandom->method('generate') - ->willReturnCallback(function ($len) { + ->willReturnCallback(function (int $len) { return 'random' . $len; }); @@ -591,7 +592,7 @@ public function testRefreshTokenExpiredAppToken(): void { ->with($accessToken); $this->secureRandom->method('generate') - ->willReturnCallback(function ($len) { + ->willReturnCallback(function (int $len) { return 'random' . $len; }); @@ -702,7 +703,7 @@ public function testRefreshTokenRedeemedConcurrently(): void { ->willReturn($appToken); $this->secureRandom->method('generate') - ->willReturnCallback(function ($len) { + ->willReturnCallback(function (int $len) { return 'random' . $len; }); @@ -792,7 +793,7 @@ private function arrangeSuccessfulTokenExchange(): PublicKeyToken { ->willReturn($appToken); $this->secureRandom->method('generate') - ->willReturnCallback(function ($len) { + ->willReturnCallback(function (int $len) { return 'random' . $len; }); $this->time->method('getTime')->willReturn(1000); diff --git a/apps/oauth2/tests/Db/AccessTokenMapperTest.php b/apps/oauth2/tests/Db/AccessTokenMapperTest.php index ea0423d72c0f6..2e137d601f04f 100644 --- a/apps/oauth2/tests/Db/AccessTokenMapperTest.php +++ b/apps/oauth2/tests/Db/AccessTokenMapperTest.php @@ -17,6 +17,7 @@ final class AccessTokenMapperTest extends TestCase { private AccessTokenMapper $accessTokenMapper; + #[\Override] protected function setUp(): void { parent::setUp(); $this->accessTokenMapper = Server::get(AccessTokenMapper::class); diff --git a/apps/oauth2/tests/Db/ClientMapperTest.php b/apps/oauth2/tests/Db/ClientMapperTest.php index 2122a5ca95a0f..9e9cb44a33235 100644 --- a/apps/oauth2/tests/Db/ClientMapperTest.php +++ b/apps/oauth2/tests/Db/ClientMapperTest.php @@ -16,14 +16,15 @@ #[\PHPUnit\Framework\Attributes\Group(name: 'DB')] final class ClientMapperTest extends TestCase { - /** @var ClientMapper */ - private $clientMapper; + private ClientMapper $clientMapper; + #[\Override] protected function setUp(): void { parent::setUp(); $this->clientMapper = Server::get(ClientMapper::class); } + #[\Override] protected function tearDown(): void { $query = Server::get(IDBConnection::class)->getQueryBuilder(); $query->delete('oauth2_clients')->executeStatement(); diff --git a/apps/oauth2/tests/Service/ClientServiceTest.php b/apps/oauth2/tests/Service/ClientServiceTest.php index eeb35f7247d20..991f31cf7064e 100644 --- a/apps/oauth2/tests/Service/ClientServiceTest.php +++ b/apps/oauth2/tests/Service/ClientServiceTest.php @@ -34,6 +34,7 @@ final class ClientServiceTest extends TestCase { private ICrypto&MockObject $crypto; private LoggerInterface&MockObject $logger; + #[\Override] protected function setUp(): void { parent::setUp(); @@ -111,6 +112,7 @@ public function testDeleteClient(): void { }; $userManager->callForAllUsers($function); $user1 = $userManager->createUser('test101', 'test101'); + $this->assertInstanceOf(IUser::class, $user1); $user1->updateLastLoginTimestamp(); $tokenProviderMock = $this->getMockBuilder(IAuthTokenProvider::class)->getMock(); @@ -161,6 +163,7 @@ public function testDeleteClient(): void { public function testDeleteClientPreservesWipePendingToken(): void { $userManager = Server::get(IUserManager::class); $user = $userManager->createUser('test_wipe_preserve', 'test_wipe_preserve'); + $this->assertInstanceOf(IUser::class, $user); $user->updateLastLoginTimestamp(); $client = new Client(); diff --git a/psalm-strict.xml b/psalm-strict.xml index 0f217d5b9bada..47b4de939a2b9 100644 --- a/psalm-strict.xml +++ b/psalm-strict.xml @@ -55,6 +55,7 @@ + From 4fe90082debf069f1352c4e13cbb03e310371936 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Sat, 15 Aug 2026 10:08:19 +0200 Subject: [PATCH 3/6] refactor(oauth2): Run rector:strict on apps/oauth2 Signed-off-by: Carl Schwan --- .../CleanupExpiredAuthorizationCode.php | 8 +-- apps/oauth2/lib/Command/AddClient.php | 1 + apps/oauth2/lib/Command/DeleteClient.php | 1 + .../lib/Command/ImportLegacyOcClient.php | 2 + .../Controller/LoginRedirectorController.php | 3 +- .../lib/Controller/OauthApiController.php | 36 +++++----- .../lib/Controller/SettingsController.php | 2 +- apps/oauth2/lib/Db/AccessTokenMapper.php | 9 +-- apps/oauth2/lib/Db/Client.php | 1 - apps/oauth2/lib/Db/ClientMapper.php | 15 ++--- .../lib/Migration/SetTokenExpiration.php | 5 +- .../Version010401Date20181207190718.php | 1 + .../Version010402Date20190107124745.php | 2 - .../Version011601Date20230522143227.php | 5 +- .../Version011602Date20230613160650.php | 4 -- .../Version011603Date20230620111039.php | 5 +- .../Version011901Date20240829164356.php | 5 +- apps/oauth2/lib/Service/ClientService.php | 18 ++--- apps/oauth2/lib/Settings/Admin.php | 3 +- .../LoginRedirectorControllerTest.php | 9 +++ .../Controller/OauthApiControllerTest.php | 65 +++++++++---------- .../oauth2/tests/Db/AccessTokenMapperTest.php | 4 ++ apps/oauth2/tests/Db/ClientMapperTest.php | 5 ++ .../tests/Service/ClientServiceTest.php | 37 ++++++----- apps/oauth2/tests/Settings/AdminTest.php | 4 ++ build/rector-strict.php | 1 + 26 files changed, 139 insertions(+), 112 deletions(-) diff --git a/apps/oauth2/lib/BackgroundJob/CleanupExpiredAuthorizationCode.php b/apps/oauth2/lib/BackgroundJob/CleanupExpiredAuthorizationCode.php index 3d7b26546c1ad..83735a0afdfd7 100644 --- a/apps/oauth2/lib/BackgroundJob/CleanupExpiredAuthorizationCode.php +++ b/apps/oauth2/lib/BackgroundJob/CleanupExpiredAuthorizationCode.php @@ -19,8 +19,8 @@ final class CleanupExpiredAuthorizationCode extends TimedJob { public function __construct( ITimeFactory $timeFactory, - private AccessTokenMapper $accessTokenMapper, - private LoggerInterface $logger, + private readonly AccessTokenMapper $accessTokenMapper, + private readonly LoggerInterface $logger, ) { parent::__construct($timeFactory); // 30 days @@ -35,8 +35,8 @@ public function __construct( protected function run($argument): void { try { $this->accessTokenMapper->cleanupExpiredAuthorizationCode($this->time); - } catch (Exception $e) { - $this->logger->warning('Failed to cleanup tokens with expired authorization code', ['exception' => $e]); + } catch (Exception $exception) { + $this->logger->warning('Failed to cleanup tokens with expired authorization code', ['exception' => $exception]); } } } diff --git a/apps/oauth2/lib/Command/AddClient.php b/apps/oauth2/lib/Command/AddClient.php index 8962f1e4df4d5..639d1bc58817f 100644 --- a/apps/oauth2/lib/Command/AddClient.php +++ b/apps/oauth2/lib/Command/AddClient.php @@ -19,6 +19,7 @@ final class AddClient extends Base { private const string ARGUMENT_CLIENT_NAME = 'client-name'; + private const string ARGUMENT_CLIENT_REDIRECT_URI = 'client-redirect-uri'; public function __construct( diff --git a/apps/oauth2/lib/Command/DeleteClient.php b/apps/oauth2/lib/Command/DeleteClient.php index 12440aeb47753..0a2f54c248bb5 100644 --- a/apps/oauth2/lib/Command/DeleteClient.php +++ b/apps/oauth2/lib/Command/DeleteClient.php @@ -53,6 +53,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int $output->writeln('' . $exception->getMessage() . ''); return Command::FAILURE; } + return Command::SUCCESS; } } diff --git a/apps/oauth2/lib/Command/ImportLegacyOcClient.php b/apps/oauth2/lib/Command/ImportLegacyOcClient.php index 5a369010d1ed1..7408a8925e8cd 100644 --- a/apps/oauth2/lib/Command/ImportLegacyOcClient.php +++ b/apps/oauth2/lib/Command/ImportLegacyOcClient.php @@ -20,6 +20,7 @@ final class ImportLegacyOcClient extends Command { private const string ARGUMENT_CLIENT_ID = 'client-id'; + private const string ARGUMENT_CLIENT_SECRET = 'client-secret'; public function __construct( @@ -71,6 +72,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int $client->redirectUri = 'http://localhost:*'; $client->clientIdentifier = $clientId; $client->secret = $hashedClientSecret; + $this->clientMapper->insert($client); $output->writeln('Client imported successfully'); diff --git a/apps/oauth2/lib/Controller/LoginRedirectorController.php b/apps/oauth2/lib/Controller/LoginRedirectorController.php index e180f3b65f58c..ab5a249066675 100644 --- a/apps/oauth2/lib/Controller/LoginRedirectorController.php +++ b/apps/oauth2/lib/Controller/LoginRedirectorController.php @@ -64,7 +64,7 @@ public function authorize( ): TemplateResponse|RedirectResponse { try { $client = $this->clientMapper->getByIdentifier($client_id); - } catch (ClientNotFoundException $e) { + } catch (ClientNotFoundException) { $params = [ 'content' => $this->l->t('Your client is not authorized to connect. Please inform the administrator of your client.'), ]; @@ -111,6 +111,7 @@ public function authorize( ] ); } + return new RedirectResponse($targetUrl); } } diff --git a/apps/oauth2/lib/Controller/OauthApiController.php b/apps/oauth2/lib/Controller/OauthApiController.php index a7a8b420f1405..94416464aa4d7 100644 --- a/apps/oauth2/lib/Controller/OauthApiController.php +++ b/apps/oauth2/lib/Controller/OauthApiController.php @@ -31,6 +31,7 @@ use OCP\IDBConnection; use OCP\IRequest; use OCP\IURLGenerator; +use OCP\IUser; use OCP\IUserManager; use OCP\Security\Bruteforce\IThrottler; use OCP\Security\ICrypto; @@ -112,7 +113,7 @@ public function getToken( try { $accessToken = $this->accessTokenMapper->getByCode($code); - } catch (AccessTokenNotFoundException $e) { + } catch (AccessTokenNotFoundException) { $response = new JSONResponse([ 'error' => 'invalid_request', ], Http::STATUS_BAD_REQUEST); @@ -149,7 +150,7 @@ public function getToken( try { $client = $this->clientMapper->getByUid($accessToken->clientId); - } catch (ClientNotFoundException $e) { + } catch (ClientNotFoundException) { $response = new JSONResponse([ 'error' => 'invalid_request', ], Http::STATUS_BAD_REQUEST); @@ -177,13 +178,14 @@ public function getToken( try { $storedClientSecretHash = $client->secret; $clientSecretHash = bin2hex($this->crypto->calculateHMAC($client_secret)); - } catch (\Exception $e) { - $this->logger->error('OAuth client secret decryption error', ['exception' => $e]); + } catch (\Exception $exception) { + $this->logger->error('OAuth client secret decryption error', ['exception' => $exception]); // we don't throttle here because it might not be a bruteforce attack return new JSONResponse([ 'error' => 'invalid_client', ], Http::STATUS_BAD_REQUEST); } + // The client id and secret must match. Else we don't provide an access token! if ($client->clientIdentifier !== $client_id || $storedClientSecretHash !== $clientSecretHash) { $response = new JSONResponse([ @@ -200,7 +202,7 @@ public function getToken( $appToken = $this->tokenProvider->getTokenById($accessToken->tokenId); } catch (ExpiredTokenException $e) { $appToken = $e->getToken(); - } catch (InvalidTokenException $e) { + } catch (InvalidTokenException) { //We can't do anything... $this->accessTokenMapper->delete($accessToken); $response = new JSONResponse([ @@ -251,15 +253,16 @@ public function getToken( $this->tokenProvider->updateToken($appToken); $this->db->commit(); - } catch (\Throwable $e) { + } catch (\Throwable $throwable) { if ($this->db->inTransaction()) { $this->db->rollBack(); } + // rotate() and updateToken() write the auth token to the cache, // so if we are past rotate() we must invalidate the new token $this->tokenProvider->invalidateToken($newToken); - throw $e; + throw $throwable; } $this->throttler->resetDelay($this->request->getRemoteAddress(), 'login', ['user' => $appToken->getUID()]); @@ -286,7 +289,7 @@ public function getToken( */ private function pushTokenToSecondary(IToken $appToken, string $newToken, ?int $expires): ?string { $user = $this->userManager->get($appToken->getUID()); - if ($user === null) { + if (!$user instanceof IUser) { $this->logger->warning('could not push oauth token to secondary: unknown user', ['uid' => $appToken->getUID()]); return null; } @@ -294,8 +297,8 @@ private function pushTokenToSecondary(IToken $appToken, string $newToken, ?int $ try { /** @var IGlobalScaleService $globalScaleService */ $globalScaleService = $this->container->get(IGlobalScaleService::class); - } catch (ContainerExceptionInterface $e) { - $this->logger->warning('could not push oauth token to secondary: globalsiteselector is not available', ['exception' => $e]); + } catch (ContainerExceptionInterface $containerException) { + $this->logger->warning('could not push oauth token to secondary: globalsiteselector is not available', ['exception' => $containerException]); return null; } @@ -312,9 +315,10 @@ private function pushTokenToSecondary(IToken $appToken, string $newToken, ?int $ 'expires' => $expires, 'token' => $newToken, ]); - } catch (\Exception $e) { - $this->logger->warning('could not push oauth token to secondary', ['exception' => $e]); + } catch (\Exception $exception) { + $this->logger->warning('could not push oauth token to secondary', ['exception' => $exception]); } + return null; } @@ -336,8 +340,8 @@ public function pushToken(string $jwt): JSONResponse { try { /** @var IGlobalScaleService $globalScaleService */ $globalScaleService = $this->container->get(IGlobalScaleService::class); - } catch (ContainerExceptionInterface $e) { - $this->logger->warning('could not receive oauth token from primary: globalsiteselector is not available', ['exception' => $e]); + } catch (ContainerExceptionInterface $containerException) { + $this->logger->warning('could not receive oauth token from primary: globalsiteselector is not available', ['exception' => $containerException]); $response = new JSONResponse([], Http::STATUS_BAD_REQUEST); $response->throttle(); return $response; @@ -362,8 +366,8 @@ public function pushToken(string $jwt): JSONResponse { (array)$decoded['scope'], $decoded['expires'] !== null ? (int)$decoded['expires'] : null, ); - } catch (\Exception $e) { - $this->logger->warning('could not create pushed oauth token', ['exception' => $e]); + } catch (\Exception $exception) { + $this->logger->warning('could not create pushed oauth token', ['exception' => $exception]); $response = new JSONResponse([], Http::STATUS_BAD_REQUEST); $response->throttle(); return $response; diff --git a/apps/oauth2/lib/Controller/SettingsController.php b/apps/oauth2/lib/Controller/SettingsController.php index 4a46f0f7c839a..2ab6457b1d3bf 100644 --- a/apps/oauth2/lib/Controller/SettingsController.php +++ b/apps/oauth2/lib/Controller/SettingsController.php @@ -21,7 +21,7 @@ final class SettingsController extends Controller { public function __construct( string $appName, IRequest $request, - private IL10N $l, + private readonly IL10N $l, private readonly ClientService $clientService, ) { parent::__construct($appName, $request); diff --git a/apps/oauth2/lib/Db/AccessTokenMapper.php b/apps/oauth2/lib/Db/AccessTokenMapper.php index 50139a994dd14..408aefdda9398 100644 --- a/apps/oauth2/lib/Db/AccessTokenMapper.php +++ b/apps/oauth2/lib/Db/AccessTokenMapper.php @@ -12,20 +12,17 @@ use OCA\OAuth2\Controller\OauthApiController; use OCA\OAuth2\Exceptions\AccessTokenNotFoundException; use OCP\AppFramework\Db\DoesNotExistException; -use OCP\AppFramework\Db\IMapperException; -use OCP\AppFramework\Db\QBMapper; use OCP\AppFramework\ORM\Repository; use OCP\AppFramework\Utility\ITimeFactory; use OCP\DB\Exception; use OCP\DB\QueryBuilder\IQueryBuilder; -use OCP\IDBConnection; /** * @template-extends Repository * @psalm-suppress ClassMustBeFinal For unit tests */ class AccessTokenMapper extends Repository { - const string entityClass = AccessToken::class; + public const string entityClass = AccessToken::class; /** * @throws AccessTokenNotFoundException @@ -35,8 +32,8 @@ public function getByCode(string $code): AccessToken { return $this->findOneBy([ 'hashedCode' => hash('sha512', $code), ]); - } catch (DoesNotExistException $e) { - throw new AccessTokenNotFoundException('Could not find access token', 0, $e); + } catch (DoesNotExistException $doesNotExistException) { + throw new AccessTokenNotFoundException('Could not find access token', 0, $doesNotExistException); } } diff --git a/apps/oauth2/lib/Db/Client.php b/apps/oauth2/lib/Db/Client.php index 92d6269a6d002..43c986b0ff9d5 100644 --- a/apps/oauth2/lib/Db/Client.php +++ b/apps/oauth2/lib/Db/Client.php @@ -9,7 +9,6 @@ namespace OCA\OAuth2\Db; - use OCP\AppFramework\ORM\Attribute\Column; use OCP\AppFramework\ORM\Attribute\Entity; use OCP\AppFramework\ORM\Attribute\Id; diff --git a/apps/oauth2/lib/Db/ClientMapper.php b/apps/oauth2/lib/Db/ClientMapper.php index b8bf91b46ae34..6c371a25337ef 100644 --- a/apps/oauth2/lib/Db/ClientMapper.php +++ b/apps/oauth2/lib/Db/ClientMapper.php @@ -11,11 +11,7 @@ use OCA\OAuth2\Exceptions\ClientNotFoundException; use OCP\AppFramework\Db\DoesNotExistException; -use OCP\AppFramework\Db\IMapperException; -use OCP\AppFramework\Db\QBMapper; use OCP\AppFramework\ORM\Repository; -use OCP\DB\QueryBuilder\IQueryBuilder; -use OCP\IDBConnection; /** * @template-extends Repository @@ -25,8 +21,6 @@ class ClientMapper extends Repository { public const string entityClass = Client::class; /** - * @param string $clientIdentifier - * @return Client * @throws ClientNotFoundException */ public function getByIdentifier(string $clientIdentifier): Client { @@ -34,14 +28,13 @@ public function getByIdentifier(string $clientIdentifier): Client { return $this->findOneBy([ 'clientIdentifier' => $clientIdentifier, ]); - } catch (DoesNotExistException $e) { - throw new ClientNotFoundException('Could not find client ' . $clientIdentifier, previous: $e); + } catch (DoesNotExistException $doesNotExistException) { + throw new ClientNotFoundException('Could not find client ' . $clientIdentifier, $doesNotExistException->getCode(), previous: $doesNotExistException); } } /** * @param int $id internal id of the client - * @return Client * @throws ClientNotFoundException */ public function getByUid(int $id): Client { @@ -49,8 +42,8 @@ public function getByUid(int $id): Client { return $this->findOneBy([ 'id' => $id, ]); - } catch (DoesNotExistException $e) { - throw new ClientNotFoundException('could not find client with id ' . $id, previous: $e); + } catch (DoesNotExistException $doesNotExistException) { + throw new ClientNotFoundException('could not find client with id ' . $id, $doesNotExistException->getCode(), previous: $doesNotExistException); } } diff --git a/apps/oauth2/lib/Migration/SetTokenExpiration.php b/apps/oauth2/lib/Migration/SetTokenExpiration.php index cca900f42ea77..b8f3491a901f7 100644 --- a/apps/oauth2/lib/Migration/SetTokenExpiration.php +++ b/apps/oauth2/lib/Migration/SetTokenExpiration.php @@ -16,7 +16,7 @@ use OCP\Migration\IOutput; use OCP\Migration\IRepairStep; -final class SetTokenExpiration implements IRepairStep { +final readonly class SetTokenExpiration implements IRepairStep { public function __construct( private IDBConnection $connection, @@ -44,10 +44,11 @@ public function run(IOutput $output): void { $appToken = $this->tokenProvider->getTokenById($tokenId); $appToken->setExpires($this->time->getTime() + 3600); $this->tokenProvider->updateToken($appToken); - } catch (InvalidTokenException $e) { + } catch (InvalidTokenException) { //Skip this token } } + $cursor->closeCursor(); } } diff --git a/apps/oauth2/lib/Migration/Version010401Date20181207190718.php b/apps/oauth2/lib/Migration/Version010401Date20181207190718.php index 3d1a984f09a68..2adb279c54889 100644 --- a/apps/oauth2/lib/Migration/Version010401Date20181207190718.php +++ b/apps/oauth2/lib/Migration/Version010401Date20181207190718.php @@ -71,6 +71,7 @@ public function changeSchema(IOutput $output, Closure $schemaClosure, array $opt $table->addUniqueIndex(['hashed_code'], 'oauth2_access_hash_idx'); $table->addIndex(['client_id'], 'oauth2_access_client_id_idx'); } + return $schema; } } diff --git a/apps/oauth2/lib/Migration/Version010402Date20190107124745.php b/apps/oauth2/lib/Migration/Version010402Date20190107124745.php index 09a2ae2403a34..096496adb3fa6 100644 --- a/apps/oauth2/lib/Migration/Version010402Date20190107124745.php +++ b/apps/oauth2/lib/Migration/Version010402Date20190107124745.php @@ -17,9 +17,7 @@ final class Version010402Date20190107124745 extends SimpleMigrationStep { /** - * @param IOutput $output * @param Closure $schemaClosure The `\Closure` returns a `ISchemaWrapper` - * @param array $options * @return null|ISchemaWrapper */ #[\Override] diff --git a/apps/oauth2/lib/Migration/Version011601Date20230522143227.php b/apps/oauth2/lib/Migration/Version011601Date20230522143227.php index 08d706f6e23ce..8e633ed42a2d8 100644 --- a/apps/oauth2/lib/Migration/Version011601Date20230522143227.php +++ b/apps/oauth2/lib/Migration/Version011601Date20230522143227.php @@ -19,8 +19,8 @@ final class Version011601Date20230522143227 extends SimpleMigrationStep { public function __construct( - private IDBConnection $connection, - private ICrypto $crypto, + private readonly IDBConnection $connection, + private readonly ICrypto $crypto, ) { } @@ -61,6 +61,7 @@ public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $qbUpdate->setParameter('updateId', $id, IQueryBuilder::PARAM_INT); $qbUpdate->executeStatement(); } + $req->closeCursor(); } } diff --git a/apps/oauth2/lib/Migration/Version011602Date20230613160650.php b/apps/oauth2/lib/Migration/Version011602Date20230613160650.php index 6b01a7505cd5c..fee395d8f451b 100644 --- a/apps/oauth2/lib/Migration/Version011602Date20230613160650.php +++ b/apps/oauth2/lib/Migration/Version011602Date20230613160650.php @@ -15,10 +15,6 @@ final class Version011602Date20230613160650 extends SimpleMigrationStep { - public function __construct( - ) { - } - #[\Override] public function changeSchema(IOutput $output, Closure $schemaClosure, array $options) { $schema = $schemaClosure(); diff --git a/apps/oauth2/lib/Migration/Version011603Date20230620111039.php b/apps/oauth2/lib/Migration/Version011603Date20230620111039.php index b23987c2573bd..4e2c308c92bcf 100644 --- a/apps/oauth2/lib/Migration/Version011603Date20230620111039.php +++ b/apps/oauth2/lib/Migration/Version011603Date20230620111039.php @@ -20,7 +20,7 @@ final class Version011603Date20230620111039 extends SimpleMigrationStep { public function __construct( - private IDBConnection $connection, + private readonly IDBConnection $connection, ) { } @@ -39,6 +39,7 @@ public function changeSchema(IOutput $output, Closure $schemaClosure, array $opt ]); $dbChanged = true; } + if (!$table->hasColumn('token_count')) { $table->addColumn('token_count', Types::BIGINT, [ 'notnull' => true, @@ -47,10 +48,12 @@ public function changeSchema(IOutput $output, Closure $schemaClosure, array $opt ]); $dbChanged = true; } + if (!$table->hasIndex('oauth2_tk_c_created_idx')) { $table->addIndex(['token_count', 'code_created_at'], 'oauth2_tk_c_created_idx'); $dbChanged = true; } + if ($dbChanged) { return $schema; } diff --git a/apps/oauth2/lib/Migration/Version011901Date20240829164356.php b/apps/oauth2/lib/Migration/Version011901Date20240829164356.php index 1c0d8978b3968..1c768c3a1bba0 100644 --- a/apps/oauth2/lib/Migration/Version011901Date20240829164356.php +++ b/apps/oauth2/lib/Migration/Version011901Date20240829164356.php @@ -19,8 +19,8 @@ final class Version011901Date20240829164356 extends SimpleMigrationStep { public function __construct( - private IDBConnection $connection, - private ICrypto $crypto, + private readonly IDBConnection $connection, + private readonly ICrypto $crypto, ) { } @@ -46,6 +46,7 @@ public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $qbUpdate->setParameter('updateId', $id, IQueryBuilder::PARAM_INT); $qbUpdate->executeStatement(); } + $req->closeCursor(); } } diff --git a/apps/oauth2/lib/Service/ClientService.php b/apps/oauth2/lib/Service/ClientService.php index b8c41e845c32f..fc393d829b78c 100644 --- a/apps/oauth2/lib/Service/ClientService.php +++ b/apps/oauth2/lib/Service/ClientService.php @@ -22,17 +22,17 @@ use OCP\Security\ISecureRandom; use Psr\Log\LoggerInterface; -final class ClientService { +final readonly class ClientService { public const string validChars = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789'; public function __construct( - private readonly ISecureRandom $secureRandom, - private readonly ICrypto $crypto, - private readonly ClientMapper $clientMapper, - private readonly IUserManager $userManager, - private readonly IAuthTokenProvider $tokenProvider, - private readonly LoggerInterface $logger, - private readonly AccessTokenMapper $accessTokenMapper, + private ISecureRandom $secureRandom, + private ICrypto $crypto, + private ClientMapper $clientMapper, + private IUserManager $userManager, + private IAuthTokenProvider $tokenProvider, + private LoggerInterface $logger, + private AccessTokenMapper $accessTokenMapper, ) { } @@ -79,6 +79,7 @@ public function deleteClient(int $id): void { if ($token->getName() !== $client->name) { continue; } + try { $this->tokenProvider->getTokenById($token->getId()); } catch (WipeTokenException) { @@ -90,6 +91,7 @@ public function deleteClient(int $id): void { } catch (InvalidTokenException) { // Token already invalid; let invalidateTokenById handle it. } + $this->tokenProvider->invalidateTokenById($user->getUID(), $token->getId()); } }); diff --git a/apps/oauth2/lib/Settings/Admin.php b/apps/oauth2/lib/Settings/Admin.php index 3579160eaa871..7644fea878eef 100644 --- a/apps/oauth2/lib/Settings/Admin.php +++ b/apps/oauth2/lib/Settings/Admin.php @@ -16,7 +16,7 @@ use OCP\Settings\ISettings; use OCP\Util; -final class Admin implements ISettings { +final readonly class Admin implements ISettings { public function __construct( private IInitialState $initialState, @@ -39,6 +39,7 @@ public function getForm(): TemplateResponse { 'clientSecret' => '', ]; } + $this->initialState->provideInitialState('clients', $result); $this->initialState->provideInitialState('oauth2-doc-link', $this->urlGenerator->linkToDocs('admin-oauth2')); diff --git a/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php b/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php index eae2b72ac3073..c87dc71f72054 100644 --- a/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php +++ b/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php @@ -1,5 +1,7 @@ crypto ->method('calculateHMAC') - ->with($this->callback(function (string $text) { - return $text === 'clientSecret' || $text === 'invalidClientSecret'; - })) - ->willReturnCallback(function (string $text) { - return $text === 'clientSecret' + ->with($this->callback(fn (string $text): bool => $text === 'clientSecret' || $text === 'invalidClientSecret')) + ->willReturnCallback(fn (string $text): string => $text === 'clientSecret' ? 'hashedClientSecret' - : 'hashedInvalidClientSecret'; - }); + : 'hashedInvalidClientSecret'); $client = new Client(); $client->clientIdentifier = 'clientId'; @@ -369,9 +382,7 @@ public function testRefreshTokenValidAppToken(): void { ->with($accessToken); $this->secureRandom->method('generate') - ->willReturnCallback(function (int $len) { - return 'random' . $len; - }); + ->willReturnCallback(fn (int $len): string => 'random' . $len); $this->tokenProvider->expects($this->once()) ->method('rotate') @@ -399,9 +410,7 @@ public function testRefreshTokenValidAppToken(): void { $this->tokenProvider->expects($this->once()) ->method('updateToken') ->with( - $this->callback(function (PublicKeyToken $token) { - return $token->getExpires() === 4600; - }) + $this->callback(fn (PublicKeyToken $token): bool => $token->getExpires() === 4600) ); $this->crypto->method('encrypt') @@ -479,9 +488,7 @@ public function testRefreshTokenValidAppTokenBasicAuth(): void { ->with($accessToken); $this->secureRandom->method('generate') - ->willReturnCallback(function (int $len) { - return 'random' . $len; - }); + ->willReturnCallback(fn (int $len): string => 'random' . $len); $this->tokenProvider->expects($this->once()) ->method('rotate') @@ -509,9 +516,7 @@ public function testRefreshTokenValidAppTokenBasicAuth(): void { $this->tokenProvider->expects($this->once()) ->method('updateToken') ->with( - $this->callback(function (PublicKeyToken $token) { - return $token->getExpires() === 4600; - }) + $this->callback(fn (PublicKeyToken $token): bool => $token->getExpires() === 4600) ); $this->crypto->method('encrypt') @@ -592,9 +597,7 @@ public function testRefreshTokenExpiredAppToken(): void { ->with($accessToken); $this->secureRandom->method('generate') - ->willReturnCallback(function (int $len) { - return 'random' . $len; - }); + ->willReturnCallback(fn (int $len): string => 'random' . $len); $this->tokenProvider->expects($this->once()) ->method('rotate') @@ -622,9 +625,7 @@ public function testRefreshTokenExpiredAppToken(): void { $this->tokenProvider->expects($this->once()) ->method('updateToken') ->with( - $this->callback(function (PublicKeyToken $token) { - return $token->getExpires() === 4600; - }) + $this->callback(fn (PublicKeyToken $token): bool => $token->getExpires() === 4600) ); $this->crypto->method('encrypt') @@ -703,9 +704,7 @@ public function testRefreshTokenRedeemedConcurrently(): void { ->willReturn($appToken); $this->secureRandom->method('generate') - ->willReturnCallback(function (int $len) { - return 'random' . $len; - }); + ->willReturnCallback(fn (int $len): string => 'random' . $len); $this->tokenProvider->expects($this->never()) ->method('rotate'); @@ -793,9 +792,7 @@ private function arrangeSuccessfulTokenExchange(): PublicKeyToken { ->willReturn($appToken); $this->secureRandom->method('generate') - ->willReturnCallback(function (int $len) { - return 'random' . $len; - }); + ->willReturnCallback(fn (int $len): string => 'random' . $len); $this->time->method('getTime')->willReturn(1000); $this->accessTokenMapper->method('rotateToken')->willReturn(1); $this->request->method('getRemoteAddress')->willReturn('1.2.3.4'); @@ -869,7 +866,7 @@ public function testGetTokenSkipsSecondaryPushWhenGlobalScaleServiceUnavailable( $this->globalScaleConfig->method('isGlobalScaleEnabled')->willReturn(true); $this->globalScaleConfig->method('isPrimary')->willReturn(true); - $user = $this->createMock(IUser::class); + $user = $this->createStub(IUser::class); $this->userManager->method('get')->with('userId')->willReturn($user); $this->container->method('get') @@ -888,7 +885,7 @@ public function testGetTokenPushesTokenToSecondaryWhenPrimary(): void { $this->globalScaleConfig->method('isGlobalScaleEnabled')->willReturn(true); $this->globalScaleConfig->method('isPrimary')->willReturn(true); - $user = $this->createMock(IUser::class); + $user = $this->createStub(IUser::class); $this->userManager->method('get')->with('userId')->willReturn($user); $this->urlGenerator->method('linkToRoute') @@ -928,7 +925,7 @@ public function testGetTokenSucceedsEvenIfPushToSecondaryFails(): void { $this->globalScaleConfig->method('isGlobalScaleEnabled')->willReturn(true); $this->globalScaleConfig->method('isPrimary')->willReturn(true); - $user = $this->createMock(IUser::class); + $user = $this->createStub(IUser::class); $this->userManager->method('get')->with('userId')->willReturn($user); $globalScaleService = $this->createMock(IGlobalScaleService::class); diff --git a/apps/oauth2/tests/Db/AccessTokenMapperTest.php b/apps/oauth2/tests/Db/AccessTokenMapperTest.php index 2e137d601f04f..8eecc597ba598 100644 --- a/apps/oauth2/tests/Db/AccessTokenMapperTest.php +++ b/apps/oauth2/tests/Db/AccessTokenMapperTest.php @@ -1,5 +1,7 @@ tokenId = time(); $token->encryptedToken = 'MyEncryptedToken'; $token->hashedCode = hash('sha512', 'MyAwesomeToken'); + $this->accessTokenMapper->insert($token); $result = $this->accessTokenMapper->getByCode('MyAwesomeToken'); @@ -46,6 +49,7 @@ public function testDeleteByClientId(): void { $token->tokenId = time(); $token->encryptedToken = 'MyEncryptedToken'; $token->hashedCode = hash('sha512', 'MyAwesomeToken'); + $this->accessTokenMapper->insert($token); $this->accessTokenMapper->deleteByClientId(1234); $this->accessTokenMapper->getByCode('MyAwesomeToken'); diff --git a/apps/oauth2/tests/Db/ClientMapperTest.php b/apps/oauth2/tests/Db/ClientMapperTest.php index 9e9cb44a33235..1b6c7ccdfabcb 100644 --- a/apps/oauth2/tests/Db/ClientMapperTest.php +++ b/apps/oauth2/tests/Db/ClientMapperTest.php @@ -1,5 +1,7 @@ name = 'Client Name'; $client->redirectUri = 'https://example.com/'; $client->secret = 'TotallyNotSecret'; + $this->clientMapper->insert($client); $this->assertEquals($client, $this->clientMapper->getByIdentifier('MyAwesomeClientIdentifier')); } @@ -54,6 +57,7 @@ public function testGetByUid(): void { $client->name = 'Client Name'; $client->redirectUri = 'https://example.com/'; $client->secret = 'TotallyNotSecret'; + $this->clientMapper->insert($client); $this->assertEquals($client, $this->clientMapper->getByUid($client->id)); } @@ -74,6 +78,7 @@ public function testInsertLongEncryptedSecret(): void { $client->name = 'Client Name'; $client->redirectUri = 'https://example.com/'; $client->secret = 'b81dc8e2dc178817bf28ca7b37265aa96559ca02e6dcdeb74b42221d096ed5ef63681e836ae0ba1077b5fb5e6c2fa7748c78463f66fe0110c8dcb8dd7eb0305b16d0cd993e2ae275879994a2abf88c68|e466d9befa6b0102341458e45ecd551a|013af9e277374483123437f180a3b0371a411ad4f34c451547909769181a7d7cc191f0f5c2de78376d124dd7751b8c9660aabdd913f5e071fc6b819ba2e3d919|3'; + $this->clientMapper->insert($client); $this->assertTrue(true); } diff --git a/apps/oauth2/tests/Service/ClientServiceTest.php b/apps/oauth2/tests/Service/ClientServiceTest.php index 991f31cf7064e..5cb4c90e507e4 100644 --- a/apps/oauth2/tests/Service/ClientServiceTest.php +++ b/apps/oauth2/tests/Service/ClientServiceTest.php @@ -1,5 +1,7 @@ clientMapper ->expects($this->once()) ->method('insert') - ->with($this->callback(function (Client $c) { - return $c->name === 'My Client Name' + ->with($this->callback(fn (Client $c): bool => $c->name === 'My Client Name' && $c->redirectUri === 'https://example.com/' && $c->secret === bin2hex('MyHashedSecret') - && $c->clientIdentifier === 'MyClientIdentifier'; - }))->willReturnCallback(function (Client $c) { - $c->id = 42; - return $c; - }); + && $c->clientIdentifier === 'MyClientIdentifier'))->willReturnCallback(function (Client $c): Client { + $c->id = 42; + return $c; + }); $result = $this->clientService->addClient('My Client Name', 'https://example.com/'); @@ -107,7 +114,7 @@ public function testDeleteClient(): void { $count = 0; $function = function (IUser $user) use (&$count): void { if ($user->getLastLogin() > 0) { - $count++; + ++$count; } }; $userManager->callForAllUsers($function); @@ -157,6 +164,7 @@ public function testDeleteClient(): void { ); $this->clientService->deleteClient(123); + $user1->delete(); } @@ -190,11 +198,9 @@ public function testDeleteClientPreservesWipePendingToken(): void { $this->authTokenProvider ->method('getTokenByUser') - ->willReturnCallback(function (string $uid) use ($wipeToken, $regularToken, $otherToken) { - return $uid === 'test_wipe_preserve' + ->willReturnCallback(fn (string $uid): array => $uid === 'test_wipe_preserve' ? [$wipeToken, $regularToken, $otherToken] - : []; - }); + : []); // Wipe state is signalled via WipeTokenException from getTokenById. $this->authTokenProvider ->method('getTokenById') @@ -202,6 +208,7 @@ public function testDeleteClientPreservesWipePendingToken(): void { if ($id === 11) { throw new WipeTokenException($wipeToken); } + return $regularToken; }); $this->authTokenProvider @@ -224,10 +231,8 @@ public function testDeleteClientPreservesWipePendingToken(): void { $this->logger->expects($this->atLeastOnce()) ->method('info') - ->with($this->stringContains('Preserving token'), $this->callback(function (array $context) { - return ($context['tokenId'] ?? null) === 11 - && ($context['uid'] ?? null) === 'test_wipe_preserve'; - })); + ->with($this->stringContains('Preserving token'), $this->callback(fn (array $context): bool => ($context['tokenId'] ?? null) === 11 + && ($context['uid'] ?? null) === 'test_wipe_preserve')); $clientService = new ClientService( $this->secureRandom, diff --git a/apps/oauth2/tests/Settings/AdminTest.php b/apps/oauth2/tests/Settings/AdminTest.php index 8839f76cde0d8..6ef24dc04f3ef 100644 --- a/apps/oauth2/tests/Settings/AdminTest.php +++ b/apps/oauth2/tests/Settings/AdminTest.php @@ -1,5 +1,7 @@ withAutoloadPaths([ // ensure rector properly autoload the public interfaces From 867f3f883fcb0c092afd30aa87cd6e4d503ddb5b Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Sat, 15 Aug 2026 10:16:17 +0200 Subject: [PATCH 4/6] fix: Use hash_equals to compare two hashes From a security point of view, this is more robust as this prevents side-channel timming attack. This is not really the case here as we already have brute force protection but this doesn't hurt. Signed-off-by: Carl Schwan --- apps/oauth2/lib/Controller/OauthApiController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/oauth2/lib/Controller/OauthApiController.php b/apps/oauth2/lib/Controller/OauthApiController.php index 94416464aa4d7..c085c790e2291 100644 --- a/apps/oauth2/lib/Controller/OauthApiController.php +++ b/apps/oauth2/lib/Controller/OauthApiController.php @@ -187,7 +187,7 @@ public function getToken( } // The client id and secret must match. Else we don't provide an access token! - if ($client->clientIdentifier !== $client_id || $storedClientSecretHash !== $clientSecretHash) { + if ($client->clientIdentifier !== $client_id || !hash_equals($storedClientSecretHash, $clientSecretHash)) { $response = new JSONResponse([ 'error' => 'invalid_client', ], Http::STATUS_BAD_REQUEST); From 80f412e8eb6b068af8cabb4c0b0dbbc56cede623 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Sat, 15 Aug 2026 10:31:52 +0200 Subject: [PATCH 5/6] fix: Finish porting oauth2 usage in other parts of the codebase Signed-off-by: Carl Schwan --- apps/oauth2/lib/Db/AccessTokenMapper.php | 4 ++-- core/Controller/ClientFlowLoginController.php | 22 +++++++++---------- .../Repair/Owncloud/MigrateOauthTables.php | 16 +++++++------- .../ClientFlowLoginControllerTest.php | 9 ++++---- 4 files changed, 26 insertions(+), 25 deletions(-) diff --git a/apps/oauth2/lib/Db/AccessTokenMapper.php b/apps/oauth2/lib/Db/AccessTokenMapper.php index 408aefdda9398..a5e407b3ed1ee 100644 --- a/apps/oauth2/lib/Db/AccessTokenMapper.php +++ b/apps/oauth2/lib/Db/AccessTokenMapper.php @@ -57,7 +57,7 @@ public function cleanupExpiredAuthorizationCode(ITimeFactory $timeFactory): void $now = $timeFactory->now()->getTimestamp(); $maxTokenCreationTs = $now - OauthApiController::AUTHORIZATION_CODE_EXPIRES_AFTER; - $qb = $this->getDatabaseConnection()->getQueryBuilder(); + $qb = $this->connection->getQueryBuilder(); $qb ->delete($this->getTableName()) ->where($qb->expr()->eq('token_count', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT))) @@ -72,7 +72,7 @@ public function cleanupExpiredAuthorizationCode(ITimeFactory $timeFactory): void * @return int Number of updated rows */ public function rotateToken(int $id, string $oldCode, string $newCode, string $encryptedToken, bool $expectAuthorizationCodeState): int { - $qb = $this->getDatabaseConnection()->getQueryBuilder(); + $qb = $this->connection->getQueryBuilder(); $qb ->update($this->getTableName()) ->set('hashed_code', $qb->createNamedParameter(hash('sha512', $newCode))) diff --git a/core/Controller/ClientFlowLoginController.php b/core/Controller/ClientFlowLoginController.php index 060091621bc0a..22c598c154b56 100644 --- a/core/Controller/ClientFlowLoginController.php +++ b/core/Controller/ClientFlowLoginController.php @@ -104,7 +104,7 @@ public function showAuthPickerPage(string $clientIdentifier = '', string $user = $client = null; if ($clientIdentifier !== '') { $client = $this->clientMapper->getByIdentifier($clientIdentifier); - $clientName = $client->getName(); + $clientName = $client->name; } // No valid clientIdentifier given and no valid API Request (APIRequest header not set) @@ -134,7 +134,7 @@ public function showAuthPickerPage(string $clientIdentifier = '', string $user = $csp = new ContentSecurityPolicy(); if ($client) { - $csp->addAllowedFormActionDomain($client->getRedirectUri()); + $csp->addAllowedFormActionDomain($client->redirectUri); } else { $csp->addAllowedFormActionDomain('nc://*'); } @@ -191,12 +191,12 @@ public function grantPage( $client = null; if ($clientIdentifier !== '') { $client = $this->clientMapper->getByIdentifier($clientIdentifier); - $clientName = $client->getName(); + $clientName = $client->name; } $csp = new ContentSecurityPolicy(); if ($client) { - $csp->addAllowedFormActionDomain($client->getRedirectUri()); + $csp->addAllowedFormActionDomain($client->redirectUri); } else { $csp->addAllowedFormActionDomain('nc://*'); } @@ -274,7 +274,7 @@ public function generateAppPassword( $client = false; if ($clientIdentifier !== '') { $client = $this->clientMapper->getByIdentifier($clientIdentifier); - $clientName = $client->getName(); + $clientName = $client->name; } $token = $this->random->generate(72, ISecureRandom::CHAR_UPPER . ISecureRandom::CHAR_LOWER . ISecureRandom::CHAR_DIGITS); @@ -292,16 +292,16 @@ public function generateAppPassword( if ($client) { $code = $this->random->generate(128, ISecureRandom::CHAR_UPPER . ISecureRandom::CHAR_LOWER . ISecureRandom::CHAR_DIGITS); $accessToken = new AccessToken(); - $accessToken->setClientId($client->getId()); - $accessToken->setEncryptedToken($this->crypto->encrypt($token, $code)); - $accessToken->setHashedCode(hash('sha512', $code)); - $accessToken->setTokenId($generatedToken->getId()); - $accessToken->setCodeCreatedAt($this->timeFactory->now()->getTimestamp()); + $accessToken->clientId = $client->id; + $accessToken->encryptedToken = $this->crypto->encrypt($token, $code); + $accessToken->hashedCode = hash('sha512', $code); + $accessToken->tokenId = $generatedToken->getId(); + $accessToken->codeCreatedAt = $this->timeFactory->now()->getTimestamp(); $this->accessTokenMapper->insert($accessToken); $enableOcClients = $this->config->getSystemValueBool('oauth2.enable_oc_clients', false); - $redirectUri = $client->getRedirectUri(); + $redirectUri = $client->redirectUri; if ($enableOcClients && $redirectUri === 'http://localhost:*') { // Sanity check untrusted redirect URI provided by the client first if (!preg_match('/^http:\/\/localhost:[0-9]+$/', $providedRedirectUri)) { diff --git a/lib/private/Repair/Owncloud/MigrateOauthTables.php b/lib/private/Repair/Owncloud/MigrateOauthTables.php index 7c371792bbcbf..f4122af517520 100644 --- a/lib/private/Repair/Owncloud/MigrateOauthTables.php +++ b/lib/private/Repair/Owncloud/MigrateOauthTables.php @@ -221,8 +221,8 @@ public function run(IOutput $output): void { $now = $this->timeFactory->now()->getTimestamp(); $index = 0; while ($row = $result->fetchAssociative()) { - $clientId = $row['client_id']; - $refreshToken = $row['token']; + $clientId = (int)$row['client_id']; + $refreshToken = (string)$row['token']; // Insert expired token so that it can be rotated on the next refresh $accessToken = $this->random->generate(72, ISecureRandom::CHAR_UPPER . ISecureRandom::CHAR_LOWER . ISecureRandom::CHAR_DIGITS); @@ -239,12 +239,12 @@ public function run(IOutput $output): void { $this->tokenProvider->updateToken($authToken); $accessTokenEntity = new AccessToken(); - $accessTokenEntity->setTokenId($authToken->getId()); - $accessTokenEntity->setClientId($clientId); - $accessTokenEntity->setHashedCode(hash('sha512', $refreshToken)); - $accessTokenEntity->setEncryptedToken($this->crypto->encrypt($accessToken, $refreshToken)); - $accessTokenEntity->setCodeCreatedAt($now); - $accessTokenEntity->setTokenCount(1); + $accessTokenEntity->tokenId = $authToken->getId(); + $accessTokenEntity->clientId = $clientId; + $accessTokenEntity->hashedCode = hash('sha512', $refreshToken); + $accessTokenEntity->encryptedToken = $this->crypto->encrypt($accessToken, $refreshToken); + $accessTokenEntity->codeCreatedAt = $now; + $accessTokenEntity->tokenCount = 1; $this->accessTokenMapper->insert($accessTokenEntity); $index++; diff --git a/tests/Core/Controller/ClientFlowLoginControllerTest.php b/tests/Core/Controller/ClientFlowLoginControllerTest.php index f77db8d79e72e..772562507429c 100644 --- a/tests/Core/Controller/ClientFlowLoginControllerTest.php +++ b/tests/Core/Controller/ClientFlowLoginControllerTest.php @@ -202,8 +202,8 @@ public function testShowAuthPickerPageWithOauth(): void { ['OCS-APIREQUEST', 'false'], ]); $client = new Client(); - $client->setName('My external service'); - $client->setRedirectUri('https://example.com/redirect.php'); + $client->name = 'My external service'; + $client->redirectUri = 'https://example.com/redirect.php'; $this->clientMapper ->expects($this->once()) ->method('getByIdentifier') @@ -491,8 +491,9 @@ public function testGeneratePasswordWithPasswordForOauthClient($redirectUri, $re ) ->willReturn($token); $client = new Client(); - $client->setName('My OAuth client'); - $client->setRedirectUri($redirectUri); + $client->id = 42; + $client->name = 'My OAuth client'; + $client->redirectUri = $redirectUri; $this->clientMapper ->expects($this->once()) ->method('getByIdentifier') From a22a2e72d38d75d9f7a124af86eeba2e8bc6966e Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Mon, 17 Aug 2026 14:56:55 +0200 Subject: [PATCH 6/6] fix(psalm): Remove entries from the baseline Signed-off-by: Carl Schwan --- .../lib/Controller/OauthApiController.php | 2 +- .../Controller/OauthApiControllerTest.php | 1 + build/psalm-baseline.xml | 20 ------------------- 3 files changed, 2 insertions(+), 21 deletions(-) diff --git a/apps/oauth2/lib/Controller/OauthApiController.php b/apps/oauth2/lib/Controller/OauthApiController.php index c085c790e2291..9f089ce2d424a 100644 --- a/apps/oauth2/lib/Controller/OauthApiController.php +++ b/apps/oauth2/lib/Controller/OauthApiController.php @@ -70,7 +70,6 @@ public function __construct( * Get a token * * @param 'authorization_code'|'refresh_token' $grant_type Token type that should be granted - * @psalm-param string $grant_type * @param ?string $code Code of the flow * @param ?string $refresh_token Refresh token * @param ?string $client_id Client ID @@ -90,6 +89,7 @@ public function getToken( ): JSONResponse { // We only handle two types + /** @psalm-suppress DocblockTypeContradiction We don't trust user input */ if ($grant_type !== 'authorization_code' && $grant_type !== 'refresh_token') { $response = new JSONResponse([ 'error' => 'invalid_grant', diff --git a/apps/oauth2/tests/Controller/OauthApiControllerTest.php b/apps/oauth2/tests/Controller/OauthApiControllerTest.php index 82c2c57ab442d..6efa02fe62b2e 100644 --- a/apps/oauth2/tests/Controller/OauthApiControllerTest.php +++ b/apps/oauth2/tests/Controller/OauthApiControllerTest.php @@ -124,6 +124,7 @@ public function testGetTokenInvalidGrantType(): void { ], Http::STATUS_BAD_REQUEST); $expected->throttle(['invalid_grant' => 'foo']); + /** @psalm-suppress InvalidArgument Test that we don't trust user input */ $this->assertEquals($expected, $this->oauthApiController->getToken('foo', null, null, null, null)); } diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index 035c0bf223d12..ea48a97dcc624 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -2226,26 +2226,6 @@ - - - - - - - - - - - - request->server]]> - - - - - - - - getUID())]]>