diff --git a/apps/oauth2/lib/BackgroundJob/CleanupExpiredAuthorizationCode.php b/apps/oauth2/lib/BackgroundJob/CleanupExpiredAuthorizationCode.php index 14d26c873810a..83735a0afdfd7 100644 --- a/apps/oauth2/lib/BackgroundJob/CleanupExpiredAuthorizationCode.php +++ b/apps/oauth2/lib/BackgroundJob/CleanupExpiredAuthorizationCode.php @@ -15,12 +15,12 @@ use OCP\DB\Exception; use Psr\Log\LoggerInterface; -class CleanupExpiredAuthorizationCode extends TimedJob { +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 @@ -30,14 +30,13 @@ public function __construct( /** * @param mixed $argument - * @inheritDoc */ #[\Override] protected function run($argument): void { try { - $this->accessTokenMapper->cleanupExpiredAuthorizationCode(); - } catch (Exception $e) { - $this->logger->warning('Failed to cleanup tokens with expired authorization code', ['exception' => $e]); + $this->accessTokenMapper->cleanupExpiredAuthorizationCode($this->time); + } 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 edf384b45372e..639d1bc58817f 100644 --- a/apps/oauth2/lib/Command/AddClient.php +++ b/apps/oauth2/lib/Command/AddClient.php @@ -17,8 +17,9 @@ 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'; public function __construct( diff --git a/apps/oauth2/lib/Command/DeleteClient.php b/apps/oauth2/lib/Command/DeleteClient.php index 6eb15421c88df..0a2f54c248bb5 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( @@ -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 93649fd4b1f62..7408a8925e8cd 100644 --- a/apps/oauth2/lib/Command/ImportLegacyOcClient.php +++ b/apps/oauth2/lib/Command/ImportLegacyOcClient.php @@ -18,8 +18,9 @@ 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'; public function __construct( @@ -67,10 +68,11 @@ 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..ab5a249066675 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, @@ -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.'), ]; @@ -73,21 +73,22 @@ 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 **/ + /** @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 @@ -97,7 +98,7 @@ public function authorize( 'core.ClientFlowLogin.grantPage', [ 'stateToken' => $stateToken, - 'clientIdentifier' => $client->getClientIdentifier(), + 'clientIdentifier' => $client->clientIdentifier, 'providedRedirectUri' => $providedRedirectUri, ] ); @@ -105,11 +106,12 @@ public function authorize( $targetUrl = $this->urlGenerator->linkToRouteAbsolute( 'core.ClientFlowLogin.showAuthPickerPage', [ - 'clientIdentifier' => $client->getClientIdentifier(), + 'clientIdentifier' => $client->clientIdentifier, 'providedRedirectUri' => $providedRedirectUri, ] ); } + return new RedirectResponse($targetUrl); } } diff --git a/apps/oauth2/lib/Controller/OauthApiController.php b/apps/oauth2/lib/Controller/OauthApiController.php index 3a768bad71362..9f089ce2d424a 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; @@ -40,9 +41,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, @@ -88,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', @@ -101,9 +103,17 @@ 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) { + } catch (AccessTokenNotFoundException) { $response = new JSONResponse([ 'error' => 'invalid_request', ], Http::STATUS_BAD_REQUEST); @@ -113,7 +123,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 +134,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,32 +149,45 @@ public function getToken( } try { - $client = $this->clientMapper->getByUid($accessToken->getClientId()); - } catch (ClientNotFoundException $e) { + $client = $this->clientMapper->getByUid($accessToken->clientId); + } catch (ClientNotFoundException) { $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; } + /** + * @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 { - $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]); + } 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->getClientIdentifier() !== $client_id || $storedClientSecretHash !== $clientSecretHash) { + if ($client->clientIdentifier !== $client_id || !hash_equals($storedClientSecretHash, $clientSecretHash)) { $response = new JSONResponse([ 'error' => 'invalid_client', ], Http::STATUS_BAD_REQUEST); @@ -172,14 +195,14 @@ 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) { + } catch (InvalidTokenException) { //We can't do anything... $this->accessTokenMapper->delete($accessToken); $response = new JSONResponse([ @@ -190,7 +213,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' @@ -200,7 +225,7 @@ public function getToken( $this->db->beginTransaction(); try { $updatedRows = $this->accessTokenMapper->rotateToken( - $accessToken->getId(), + $accessToken->id, $code, $newCode, $newEncryptedToken, @@ -228,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()]); @@ -263,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; } @@ -271,13 +297,15 @@ 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; } 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(), @@ -287,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; } @@ -311,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; @@ -337,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 0b0adbb1cb1b6..2ab6457b1d3bf 100644 --- a/apps/oauth2/lib/Controller/SettingsController.php +++ b/apps/oauth2/lib/Controller/SettingsController.php @@ -17,11 +17,11 @@ use OCP\IL10N; use OCP\IRequest; -class SettingsController extends Controller { +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); @@ -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 e0cadbc0a47e7..445e86d74214e 100644 --- a/apps/oauth2/lib/Db/AccessToken.php +++ b/apps/oauth2/lib/Db/AccessToken.php @@ -9,44 +9,34 @@ 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) + * @psalm-suppress MissingConstructor ORM based hydration */ -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')] +final 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..a5e407b3ed1ee 100644 --- a/apps/oauth2/lib/Db/AccessTokenMapper.php +++ b/apps/oauth2/lib/Db/AccessTokenMapper.php @@ -11,57 +11,39 @@ use OCA\OAuth2\Controller\OauthApiController; use OCA\OAuth2\Exceptions\AccessTokenNotFoundException; -use OCP\AppFramework\Db\IMapperException; -use OCP\AppFramework\Db\QBMapper; +use OCP\AppFramework\Db\DoesNotExistException; +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 + * @psalm-suppress ClassMustBeFinal For unit tests */ -class AccessTokenMapper extends QBMapper { - - public function __construct( - IDBConnection $db, - private ITimeFactory $timeFactory, - ) { - parent::__construct($db, 'oauth2_access_tokens'); - } +class AccessTokenMapper extends Repository { + public 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) { - throw new AccessTokenNotFoundException('Could not find access token', 0, $e); + return $this->findOneBy([ + 'hashedCode' => hash('sha512', $code), + ]); + } catch (DoesNotExistException $doesNotExistException) { + throw new AccessTokenNotFoundException('Could not find access token', 0, $doesNotExistException); } - - 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 +51,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->connection->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 +68,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->connection->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..43c986b0ff9d5 100644 --- a/apps/oauth2/lib/Db/Client.php +++ b/apps/oauth2/lib/Db/Client.php @@ -9,34 +9,28 @@ 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 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) + * @psalm-suppress MissingConstructor ORM based hydration */ -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'); - } +#[Entity(name: 'oauth2_clients')] +final 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..6c371a25337ef 100644 --- a/apps/oauth2/lib/Db/ClientMapper.php +++ b/apps/oauth2/lib/Db/ClientMapper.php @@ -10,72 +10,47 @@ namespace OCA\OAuth2\Db; use OCA\OAuth2\Exceptions\ClientNotFoundException; -use OCP\AppFramework\Db\IMapperException; -use OCP\AppFramework\Db\QBMapper; -use OCP\DB\QueryBuilder\IQueryBuilder; -use OCP\IDBConnection; +use OCP\AppFramework\Db\DoesNotExistException; +use OCP\AppFramework\ORM\Repository; /** - * @template-extends QBMapper + * @template-extends Repository + * @psalm-suppress ClassMustBeFinal For unit tests */ -class ClientMapper extends QBMapper { +class ClientMapper extends Repository { + public const string entityClass = Client::class; /** - * @param IDBConnection $db - */ - public function __construct(IDBConnection $db) { - parent::__construct($db, 'oauth2_clients'); - } - - /** - * @param string $clientIdentifier - * @return Client * @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 $doesNotExistException) { + throw new ClientNotFoundException('Could not find client ' . $clientIdentifier, $doesNotExistException->getCode(), previous: $doesNotExistException); } - return $client; } /** * @param int $id internal id of the client - * @return 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 $doesNotExistException) { + throw new ClientNotFoundException('could not find client with id ' . $id, $doesNotExistException->getCode(), previous: $doesNotExistException); } - 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/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..b8f3491a901f7 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 readonly class SetTokenExpiration implements IRepairStep { public function __construct( private IDBConnection $connection, @@ -32,23 +31,24 @@ 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) { + } catch (InvalidTokenException) { //Skip this token } } + $cursor->closeCursor(); } } diff --git a/apps/oauth2/lib/Migration/Version010401Date20181207190718.php b/apps/oauth2/lib/Migration/Version010401Date20181207190718.php index 46ac0a4034790..2adb279c54889 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')) { @@ -79,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 ac203740ab587..096496adb3fa6 100644 --- a/apps/oauth2/lib/Migration/Version010402Date20190107124745.php +++ b/apps/oauth2/lib/Migration/Version010402Date20190107124745.php @@ -14,12 +14,10 @@ use OCP\Migration\IOutput; use OCP\Migration\SimpleMigrationStep; -class Version010402Date20190107124745 extends SimpleMigrationStep { +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 f667b23e6077e..8e633ed42a2d8 100644 --- a/apps/oauth2/lib/Migration/Version011601Date20230522143227.php +++ b/apps/oauth2/lib/Migration/Version011601Date20230522143227.php @@ -10,24 +10,22 @@ 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, - private ICrypto $crypto, + private readonly IDBConnection $connection, + private readonly ICrypto $crypto, ) { } #[\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,13 +54,14 @@ 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); $qbUpdate->executeStatement(); } + $req->closeCursor(); } } diff --git a/apps/oauth2/lib/Migration/Version011602Date20230613160650.php b/apps/oauth2/lib/Migration/Version011602Date20230613160650.php index 87233a09dfc2e..fee395d8f451b 100644 --- a/apps/oauth2/lib/Migration/Version011602Date20230613160650.php +++ b/apps/oauth2/lib/Migration/Version011602Date20230613160650.php @@ -10,19 +10,13 @@ namespace OCA\OAuth2\Migration; use Closure; -use OCP\DB\ISchemaWrapper; use OCP\Migration\IOutput; use OCP\Migration\SimpleMigrationStep; -class Version011602Date20230613160650 extends SimpleMigrationStep { - - public function __construct( - ) { - } +final class Version011602Date20230613160650 extends SimpleMigrationStep { #[\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..4e2c308c92bcf 100644 --- a/apps/oauth2/lib/Migration/Version011603Date20230620111039.php +++ b/apps/oauth2/lib/Migration/Version011603Date20230620111039.php @@ -17,16 +17,15 @@ use OCP\Migration\IOutput; use OCP\Migration\SimpleMigrationStep; -class Version011603Date20230620111039 extends SimpleMigrationStep { +final class Version011603Date20230620111039 extends SimpleMigrationStep { public function __construct( - private IDBConnection $connection, + private readonly IDBConnection $connection, ) { } #[\Override] public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper { - /** @var ISchemaWrapper $schema */ $schema = $schemaClosure(); if ($schema->hasTable('oauth2_access_tokens')) { @@ -40,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, @@ -48,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 dd022c87451cc..1c768c3a1bba0 100644 --- a/apps/oauth2/lib/Migration/Version011901Date20240829164356.php +++ b/apps/oauth2/lib/Migration/Version011901Date20240829164356.php @@ -16,11 +16,11 @@ use OCP\Migration\SimpleMigrationStep; use OCP\Security\ICrypto; -class Version011901Date20240829164356 extends SimpleMigrationStep { +final class Version011901Date20240829164356 extends SimpleMigrationStep { public function __construct( - private IDBConnection $connection, - private ICrypto $crypto, + private readonly IDBConnection $connection, + private readonly ICrypto $crypto, ) { } @@ -38,14 +38,15 @@ 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); $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 e47afa35bc8db..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; -class ClientService { - public const validChars = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789'; +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, ) { } @@ -49,19 +49,21 @@ 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; + /** @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->setSecret($hashedSecret); - $client->setClientIdentifier($this->secureRandom->generate(64, self::validChars)); + $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); 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,9 +76,10 @@ 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 { $this->tokenProvider->getTokenById($token->getId()); } catch (WipeTokenException) { @@ -88,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 68d580c79b2f9..7644fea878eef 100644 --- a/apps/oauth2/lib/Settings/Admin.php +++ b/apps/oauth2/lib/Settings/Admin.php @@ -15,15 +15,13 @@ use OCP\IURLGenerator; use OCP\Settings\ISettings; use OCP\Util; -use Psr\Log\LoggerInterface; -class Admin implements ISettings { +final readonly class Admin implements ISettings { public function __construct( private IInitialState $initialState, private ClientMapper $clientMapper, private IURLGenerator $urlGenerator, - private LoggerInterface $logger, ) { } @@ -33,18 +31,15 @@ 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..c87dc71f72054 100644 --- a/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php +++ b/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php @@ -1,5 +1,7 @@ setClientIdentifier('MyClientIdentifier'); + $client->name = 'MyClientName'; + $client->clientIdentifier = 'MyClientIdentifier'; $this->clientMapper ->expects($this->once()) ->method('getByIdentifier') @@ -97,8 +108,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 +125,7 @@ public function testAuthorizeSkipPicker(): void { /* Expected */ break; default: - throw new LogicException(); + throw new \LogicException(); } }); $this->appConfig @@ -150,8 +161,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 +178,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 +213,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..6efa02fe62b2e 100644 --- a/apps/oauth2/tests/Controller/OauthApiControllerTest.php +++ b/apps/oauth2/tests/Controller/OauthApiControllerTest.php @@ -1,5 +1,7 @@ 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)); } @@ -132,8 +151,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 +177,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 +204,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 +244,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') @@ -238,7 +257,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'], @@ -259,7 +278,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') @@ -267,18 +286,14 @@ public function testRefreshTokenInvalidClient($clientId, $clientSecret): void { $this->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->setClientIdentifier('clientId'); - $client->setSecret(bin2hex('hashedClientSecret')); + $client->clientIdentifier = 'clientId'; + $client->secret = bin2hex('hashedClientSecret'); $this->clientMapper->method('getByUid') ->with(42) ->willReturn($client); @@ -293,17 +308,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 +346,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); @@ -368,9 +383,7 @@ public function testRefreshTokenValidAppToken(): void { ->with($accessToken); $this->secureRandom->method('generate') - ->willReturnCallback(function ($len) { - return 'random' . $len; - }); + ->willReturnCallback(fn (int $len): string => 'random' . $len); $this->tokenProvider->expects($this->once()) ->method('rotate') @@ -398,9 +411,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') @@ -441,18 +452,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); @@ -478,9 +489,7 @@ public function testRefreshTokenValidAppTokenBasicAuth(): void { ->with($accessToken); $this->secureRandom->method('generate') - ->willReturnCallback(function ($len) { - return 'random' . $len; - }); + ->willReturnCallback(fn (int $len): string => 'random' . $len); $this->tokenProvider->expects($this->once()) ->method('rotate') @@ -508,9 +517,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') @@ -554,18 +561,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); @@ -591,9 +598,7 @@ public function testRefreshTokenExpiredAppToken(): void { ->with($accessToken); $this->secureRandom->method('generate') - ->willReturnCallback(function ($len) { - return 'random' . $len; - }); + ->willReturnCallback(fn (int $len): string => 'random' . $len); $this->tokenProvider->expects($this->once()) ->method('rotate') @@ -621,9 +626,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') @@ -669,18 +672,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); @@ -702,9 +705,7 @@ public function testRefreshTokenRedeemedConcurrently(): void { ->willReturn($appToken); $this->secureRandom->method('generate') - ->willReturnCallback(function ($len) { - return 'random' . $len; - }); + ->willReturnCallback(fn (int $len): string => 'random' . $len); $this->tokenProvider->expects($this->never()) ->method('rotate'); @@ -753,18 +754,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); @@ -792,9 +793,7 @@ private function arrangeSuccessfulTokenExchange(): PublicKeyToken { ->willReturn($appToken); $this->secureRandom->method('generate') - ->willReturnCallback(function ($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'); @@ -868,7 +867,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') @@ -887,7 +886,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') @@ -927,7 +926,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/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..8eecc597ba598 100644 --- a/apps/oauth2/tests/Db/AccessTokenMapperTest.php +++ b/apps/oauth2/tests/Db/AccessTokenMapperTest.php @@ -1,5 +1,7 @@ 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 +45,12 @@ 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..1b6c7ccdfabcb 100644 --- a/apps/oauth2/tests/Db/ClientMapperTest.php +++ b/apps/oauth2/tests/Db/ClientMapperTest.php @@ -1,5 +1,7 @@ clientMapper = new ClientMapper(Server::get(IDBConnection::class)); + $this->clientMapper = Server::get(ClientMapper::class); } + #[\Override] protected function tearDown(): void { $query = Server::get(IDBConnection::class)->getQueryBuilder(); $query->delete('oauth2_clients')->executeStatement(); @@ -33,12 +36,12 @@ 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 +53,13 @@ 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 +69,16 @@ 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..5cb4c90e507e4 100644 --- a/apps/oauth2/tests/Service/ClientServiceTest.php +++ b/apps/oauth2/tests/Service/ClientServiceTest.php @@ -1,5 +1,7 @@ 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'; - }))->willReturnCallback(function (Client $c) { - $c->setId(42); - return $c; - }); + ->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): Client { + $c->id = 42; + return $c; + }); $result = $this->clientService->addClient('My Client Name', 'https://example.com/'); @@ -106,11 +114,12 @@ public function testDeleteClient(): void { $count = 0; $function = function (IUser $user) use (&$count): void { if ($user->getLastLogin() > 0) { - $count++; + ++$count; } }; $userManager->callForAllUsers($function); $user1 = $userManager->createUser('test101', 'test101'); + $this->assertInstanceOf(IUser::class, $user1); $user1->updateLastLoginTimestamp(); $tokenProviderMock = $this->getMockBuilder(IAuthTokenProvider::class)->getMock(); @@ -125,11 +134,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') @@ -155,20 +164,22 @@ public function testDeleteClient(): void { ); $this->clientService->deleteClient(123); + $user1->delete(); } 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(); - $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); @@ -187,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') @@ -199,6 +208,7 @@ public function testDeleteClientPreservesWipePendingToken(): void { if ($id === 11) { throw new WipeTokenException($wipeToken); } + return $regularToken; }); $this->authTokenProvider @@ -221,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 0060bc0dacf61..6ef24dc04f3ef 100644 --- a/apps/oauth2/tests/Settings/AdminTest.php +++ b/apps/oauth2/tests/Settings/AdminTest.php @@ -1,5 +1,7 @@ initialState, $this->clientMapper, $this->createMock(IURLGenerator::class), - $this->createMock(LoggerInterface::class) ); } 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())]]> diff --git a/build/rector-strict.php b/build/rector-strict.php index a0c89b6b04ddb..7566ca1956a45 100644 --- a/build/rector-strict.php +++ b/build/rector-strict.php @@ -49,6 +49,7 @@ $nextcloudDir . '/apps/files/tests/Sharing', $nextcloudDir . '/lib/public/AppFramework/ORM', $nextcloudDir . '/lib/private/AppFramework/ORM', + $nextcloudDir . '/apps/oauth2', ]) ->withAutoloadPaths([ // ensure rector properly autoload the public interfaces 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/psalm-strict.xml b/psalm-strict.xml index 0f217d5b9bada..47b4de939a2b9 100644 --- a/psalm-strict.xml +++ b/psalm-strict.xml @@ -55,6 +55,7 @@ + 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')