diff --git a/lib/Controller/ApiController.php b/lib/Controller/ApiController.php index fcfea413..416b6bc6 100644 --- a/lib/Controller/ApiController.php +++ b/lib/Controller/ApiController.php @@ -23,21 +23,13 @@ class ApiController extends Controller { public function __construct( IRequest $request, - private IRootFolder $root, - private UserMapper $userMapper, - private IUserManager $userManager, + private readonly IRootFolder $root, + private readonly UserMapper $userMapper, + private readonly IUserManager $userManager, ) { parent::__construct(Application::APP_ID, $request); } - /** - * @param int $providerId - * @param string $userId - * @param string|null $displayName - * @param string|null $email - * @param string|null $quota - * @return DataResponse - */ #[NoCSRFRequired] public function createUser(int $providerId, string $userId, ?string $displayName = null, ?string $email = null, ?string $quota = null): DataResponse { @@ -59,21 +51,18 @@ public function createUser(int $providerId, string $userId, ?string $displayName $user->setQuota($quota); } - $userFolder = $this->root->getUserFolder($user->getUID()); + $userId = $user->getUID(); + $userFolder = $this->root->getUserFolder($userId); try { // copy skeleton - \OC_Util::copySkeleton($user->getUID(), $userFolder); + \OC_Util::copySkeleton($userId, $userFolder); } catch (NotPermittedException $ex) { // read only uses } - return new DataResponse(['user_id' => $user->getUID()]); + return new DataResponse(['user_id' => $userId]); } - /** - * @param string $userId - * @return DataResponse - */ #[NoCSRFRequired] public function deleteUser(string $userId): DataResponse { $user = $this->userManager->get($userId); diff --git a/lib/Controller/BaseOidcController.php b/lib/Controller/BaseOidcController.php index 619e0e56..cc14d39e 100644 --- a/lib/Controller/BaseOidcController.php +++ b/lib/Controller/BaseOidcController.php @@ -20,8 +20,8 @@ class BaseOidcController extends Controller { public function __construct( IRequest $request, - private IConfig $config, - private IL10N $l, + private readonly IConfig $config, + private readonly IL10N $l, ) { parent::__construct(Application::APP_ID, $request); } @@ -33,14 +33,9 @@ protected function isDebugModeEnabled(): bool { return $this->config->getSystemValueBool('debug', false); } - /** - * @param string $message - * @param int $statusCode - * @param array $throttleMetadata - * @param bool|null $throttle - * @return TemplateResponse - */ - protected function buildErrorTemplateResponse(string $message, int $statusCode, array $throttleMetadata = [], ?bool $throttle = null): TemplateResponse { + protected function buildErrorTemplateResponse( + string $message, int $statusCode, array $throttleMetadata = [], ?bool $throttle = null + ): TemplateResponse { $params = [ 'message' => $message, 'title' => $this->l->t('Error'), @@ -48,14 +43,8 @@ protected function buildErrorTemplateResponse(string $message, int $statusCode, return $this->buildFailureTemplateResponse($params, $statusCode, $throttleMetadata, $throttle); } - /** - * @param string $message - * @param int $statusCode - * @param array $throttleMetadata - * @param bool|null $throttle - * @return TemplateResponse - */ - protected function build403TemplateResponse(string $message, int $statusCode, array $throttleMetadata = [], ?bool $throttle = null): TemplateResponse { + protected function build403TemplateResponse(string $message, int $statusCode, array $throttleMetadata = [], + ?bool $throttle = null): TemplateResponse { $params = [ 'message' => $message, 'title' => $this->l->t('Access forbidden'), @@ -63,16 +52,8 @@ protected function build403TemplateResponse(string $message, int $statusCode, ar return $this->buildFailureTemplateResponse($params, $statusCode, $throttleMetadata, $throttle); } - /** - * @param array $params - * @param int $statusCode - * @param array $throttleMetadata - * @param bool|null $throttle - * @return TemplateResponse - */ - protected function buildFailureTemplateResponse( - array $params, int $statusCode, array $throttleMetadata = [], ?bool $throttle = null, - ): TemplateResponse { + protected function buildFailureTemplateResponse(array $params, int $statusCode, array $throttleMetadata = [], + ?bool $throttle = null): TemplateResponse { $response = new TemplateResponse( Application::APP_ID, 'error', @@ -80,6 +61,7 @@ protected function buildFailureTemplateResponse( TemplateResponse::RENDER_AS_ERROR ); $response->setStatus($statusCode); + // if not specified, throttle if debug mode is off if (($throttle === null && !$this->isDebugModeEnabled()) || $throttle) { $response->throttle($throttleMetadata); @@ -89,15 +71,8 @@ protected function buildFailureTemplateResponse( // TODO: use the following methods only when 32 is the min supported version // as it includes the "back to NC" button - - /** - * @param string $message - * @param int $statusCode - * @param array $throttleMetadata - * @param bool|null $throttle - * @return TemplateResponse - */ - protected function buildCoreErrorTemplateResponse(string $message, int $statusCode, array $throttleMetadata = [], ?bool $throttle = null): TemplateResponse { + protected function buildCoreErrorTemplateResponse(string $message, int $statusCode, array $throttleMetadata = [], + ?bool $throttle = null): TemplateResponse { $params = [ 'errors' => [ ['error' => $message], @@ -106,27 +81,12 @@ protected function buildCoreErrorTemplateResponse(string $message, int $statusCo return $this->buildCoreFailureTemplateResponse('', 'error', $params, $statusCode, $throttleMetadata, $throttle); } - /** - * @param string $message - * @param int $statusCode - * @param array $throttleMetadata - * @param bool|null $throttle - * @return TemplateResponse - */ - protected function buildCore403TemplateResponse(string $message, int $statusCode, array $throttleMetadata = [], ?bool $throttle = null): TemplateResponse { + protected function buildCore403TemplateResponse(string $message, int $statusCode, array $throttleMetadata = [], + ?bool $throttle = null): TemplateResponse { $params = ['message' => $message]; return $this->buildCoreFailureTemplateResponse('core', '403', $params, $statusCode, $throttleMetadata, $throttle); } - /** - * @param string $appName - * @param string $templateName - * @param array $params - * @param int $statusCode - * @param array $throttleMetadata - * @param bool|null $throttle - * @return TemplateResponse - */ protected function buildCoreFailureTemplateResponse(string $appName, string $templateName, array $params, int $statusCode, array $throttleMetadata = [], ?bool $throttle = null): TemplateResponse { $response = new TemplateResponse( @@ -136,6 +96,7 @@ protected function buildCoreFailureTemplateResponse(string $appName, string $tem TemplateResponse::RENDER_AS_ERROR ); $response->setStatus($statusCode); + // if not specified, throttle if debug mode is off if (($throttle === null && !$this->isDebugModeEnabled()) || $throttle) { $response->throttle($throttleMetadata); diff --git a/lib/Controller/Id4meController.php b/lib/Controller/Id4meController.php index 72d6728c..f2d633fa 100644 --- a/lib/Controller/Id4meController.php +++ b/lib/Controller/Id4meController.php @@ -54,21 +54,21 @@ class Id4meController extends BaseOidcController { public function __construct( IRequest $request, - private ISecureRandom $random, - private ISession $session, + private readonly ISecureRandom $random, + private readonly ISession $session, IConfig $config, - private IL10N $l10n, - private ITimeFactory $timeFactory, - private IClientService $clientService, - private IURLGenerator $urlGenerator, - private UserMapper $userMapper, - private IUserSession $userSession, - private IUserManager $userManager, + private readonly IL10N $l10n, + private readonly ITimeFactory $timeFactory, + private readonly IClientService $clientService, + private readonly IURLGenerator $urlGenerator, + private readonly UserMapper $userMapper, + private readonly IUserSession $userSession, + private readonly IUserManager $userManager, HttpClientHelper $clientHelper, - private Id4MeMapper $id4MeMapper, - private ID4MeService $id4MeService, - private LoggerInterface $logger, - private ICrypto $crypto, + private readonly Id4MeMapper $id4MeMapper, + private readonly ID4MeService $id4MeService, + private readonly LoggerInterface $logger, + private readonly ICrypto $crypto, ) { parent::__construct($request, $config, $l10n); @@ -96,7 +96,6 @@ public function showLogin() { } /** - * @param string $domain * @return RedirectResponse|TemplateResponse */ #[PublicPage] @@ -165,9 +164,6 @@ private function registerClient(string $authorityName, OpenIdConfig $openIdConfi } /** - * @param string $state - * @param string $code - * @param string $scope * @return JSONResponse|RedirectResponse|TemplateResponse * @throws \Exception */ @@ -249,7 +245,7 @@ public function code(string $state = '', string $code = '', string $scope = '') $plainHeaders = json_decode(base64_decode($header), true); $plainPayload = json_decode(base64_decode($payload), true); - /** TODO: VALIATE SIGNATURE! */ + /** TODO: VALIDATE SIGNATURE! */ // Check expiration if ($plainPayload['exp'] < $this->timeFactory->getTime()) { diff --git a/lib/Controller/LoginController.php b/lib/Controller/LoginController.php index a9e4f951..8568b563 100644 --- a/lib/Controller/LoginController.php +++ b/lib/Controller/LoginController.php @@ -73,45 +73,38 @@ class LoginController extends BaseOidcController { public function __construct( IRequest $request, - private ProviderMapper $providerMapper, - private ProviderService $providerService, - private DiscoveryService $discoveryService, - private LdapService $ldapService, - private SettingsService $settingsService, - private ISecureRandom $random, - private ISession $session, - private HttpClientHelper $clientService, - private IURLGenerator $urlGenerator, - private IUserSession $userSession, - private IUserManager $userManager, - private ITimeFactory $timeFactory, - private IEventDispatcher $eventDispatcher, - private IConfig $config, - private IAppConfig $appConfig, - private IProvider $authTokenProvider, - private SessionMapper $sessionMapper, - private ProvisioningService $provisioningService, - private IL10N $l10n, - private LoggerInterface $logger, - private ICrypto $crypto, - private TokenService $tokenService, - private OidcService $oidcService, + private readonly ProviderMapper $providerMapper, + private readonly ProviderService $providerService, + private readonly DiscoveryService $discoveryService, + private readonly LdapService $ldapService, + private readonly SettingsService $settingsService, + private readonly ISecureRandom $random, + private readonly ISession $session, + private readonly HttpClientHelper $clientService, + private readonly IURLGenerator $urlGenerator, + private readonly IUserSession $userSession, + private readonly IUserManager $userManager, + private readonly ITimeFactory $timeFactory, + private readonly IEventDispatcher $eventDispatcher, + private readonly IConfig $config, + private readonly IAppConfig $appConfig, + private readonly IProvider $authTokenProvider, + private readonly SessionMapper $sessionMapper, + private readonly ProvisioningService $provisioningService, + private readonly IL10N $l10n, + private readonly LoggerInterface $logger, + private readonly ICrypto $crypto, + private readonly TokenService $tokenService, + private readonly OidcService $oidcService, ) { parent::__construct($request, $config, $l10n); } - /** - * @return bool - */ private function isSecure(): bool { // no restriction in debug mode return $this->isDebugModeEnabled() || $this->request->getServerProtocol() === 'https'; } - /** - * @param bool|null $throttle - * @return TemplateResponse - */ private function buildProtocolErrorResponse(?bool $throttle = null): TemplateResponse { $params = [ 'message' => $this->l10n->t('You must access Nextcloud with HTTPS to use OpenID Connect.'), @@ -120,29 +113,24 @@ private function buildProtocolErrorResponse(?bool $throttle = null): TemplateRes return $this->buildFailureTemplateResponse($params, Http::STATUS_NOT_FOUND, $throttleMetadata, $throttle); } - /** - * @param string|null $redirectUrl - * @return RedirectResponse - */ private function getRedirectResponse(?string $redirectUrl = null): RedirectResponse { + $baseUrl = $this->urlGenerator->getBaseUrl(); if ($redirectUrl === null) { - return new RedirectResponse($this->urlGenerator->getBaseUrl()); + return new RedirectResponse($baseUrl); } // Remove protocol and domain name $filtered = preg_replace('/^https?:\/\/[^\/]+/', '', $redirectUrl); // Additional check: ensure the result starts with a single / - if (!preg_match('/^\/[^\/]/', $filtered)) { - return new RedirectResponse($this->urlGenerator->getBaseUrl()); + if (!str_starts_with($filtered, '/') || str_starts_with($filtered, '//')) { + return new RedirectResponse($baseUrl); } return new RedirectResponse($filtered); } /** - * @param int $providerId - * @param string|null $redirectUrl * @return DataDisplayResponse|RedirectResponse|TemplateResponse */ #[PublicPage] @@ -281,7 +269,7 @@ public function login(int $providerId, ?string $redirectUrl = null) { 'response_type' => 'code', 'scope' => trim($provider->getScope()), 'redirect_uri' => $this->urlGenerator->linkToRouteAbsolute(Application::APP_ID . '.login.code'), - 'claims' => json_encode($claims), + 'claims' => json_encode($claims, JSON_THROW_ON_ERROR), 'state' => $state, 'nonce' => $nonce, ]; @@ -295,7 +283,6 @@ public function login(int $providerId, ?string $redirectUrl = null) { $data['code_challenge_method'] = 'S256'; } - $authorizationUrl = $this->discoveryService->buildAuthorizationUrl($discovery['authorization_endpoint'], $data); $this->logger->debug('Redirecting user to: ' . $authorizationUrl); @@ -310,11 +297,6 @@ public function login(int $providerId, ?string $redirectUrl = null) { } /** - * @param string $state - * @param string $code - * @param string $scope - * @param string $error - * @param string $error_description * @return JSONResponse|RedirectResponse|TemplateResponse * @throws DoesNotExistException * @throws MultipleObjectsReturnedException @@ -432,7 +414,7 @@ public function code(string $state = '', string $code = '', string $scope = '', } catch (ClientException|ServerException $e) { $response = $e->getResponse(); $body = (string)$response->getBody(); - $responseBodyArray = json_decode($body, true); + $responseBodyArray = json_decode($body, true, JSON_THROW_ON_ERROR); if ($responseBodyArray !== null && isset($responseBodyArray['error'], $responseBodyArray['error_description'])) { $this->logger->debug('Failed to contact the OIDC provider token endpoint', [ 'exception' => $e, @@ -451,7 +433,7 @@ public function code(string $state = '', string $code = '', string $scope = '', return $this->build403TemplateResponse($message, Http::STATUS_FORBIDDEN, [], false); } - $data = json_decode($body, true); + $data = json_decode($body, true, JSON_THROW_ON_ERROR); $this->logger->debug('Received code response: ' . json_encode($data, JSON_THROW_ON_ERROR)); $this->eventDispatcher->dispatchTyped(new TokenObtainedEvent($data, $provider, $discovery)); @@ -590,14 +572,15 @@ public function code(string $state = '', string $code = '', string $scope = '', $this->logger->debug('Logging user in'); $this->userSession->setUser($user); + $userUid = $user->getUID(); if ($this->userSession instanceof OC_UserSession) { // TODO server should/could be refactored so we don't need to manually create the user session and dispatch the login-related events // Warning! If GSS is used, it reacts to the BeforeUserLoggedInEvent and handles the redirection itself // So nothing after dispatching this event will be executed - $this->eventDispatcher->dispatchTyped(new BeforeUserLoggedInEvent($user->getUID(), null, \OCP\Server::get(Backend::class))); + $this->eventDispatcher->dispatchTyped(new BeforeUserLoggedInEvent($userUid, null, \OCP\Server::get(Backend::class))); - $this->userSession->completeLogin($user, ['loginName' => $user->getUID(), 'password' => '']); - $this->userSession->createSessionToken($this->request, $user->getUID(), $user->getUID()); + $this->userSession->completeLogin($user, ['loginName' => $userUid, 'password' => '']); + $this->userSession->createSessionToken($this->request, $userUid, $userUid); $this->userSession->createRememberMeToken($user); // prevent password confirmation @@ -609,7 +592,7 @@ public function code(string $state = '', string $code = '', string $scope = '', $this->authTokenProvider->updateToken($token); } - $this->eventDispatcher->dispatchTyped(new UserLoggedInEvent($user, $user->getUID(), null, false)); + $this->eventDispatcher->dispatchTyped(new UserLoggedInEvent($user, $userUid, null, false)); } $storeLoginTokenEnabled = $this->appConfig->getValueString(Application::APP_ID, 'store_login_token', '0', lazy: true) === '1'; @@ -621,7 +604,8 @@ public function code(string $state = '', string $code = '', string $scope = '', ); $this->tokenService->storeToken($tokenData); } - $this->config->setUserValue($user->getUID(), Application::APP_ID, 'had_token_once', '1'); + + $this->config->setUserValue($userUid, Application::APP_ID, 'had_token_once', '1'); // Set last password confirm to the future as we don't have passwords to confirm against with SSO $this->session->set('last-password-confirm', strtotime('+4 year', time())); @@ -636,7 +620,7 @@ public function code(string $state = '', string $code = '', string $scope = '', $authToken->getId(), $this->session->getId(), $idTokenRaw, - $user->getUID(), + $userUid, $providerId, ); } catch (InvalidTokenException $e) { @@ -748,9 +732,6 @@ public function singleLogoutService() { * which leads to the auth token that we can invalidate * Implemented according to https://openid.net/specs/openid-connect-backchannel-1_0.html * - * @param string $providerIdentifier - * @param string $logout_token - * @return JSONResponse * @throws Exception * @throws \JsonException */ @@ -889,11 +870,6 @@ public function backChannelLogout(string $providerIdentifier, string $logout_tok /** * Generate an error response according to the OIDC standard * Log the error - * - * @param string $error - * @param string $description - * @param array $throttleMetadata - * @return JSONResponse */ private function getBackchannelLogoutErrorResponse( string $error, diff --git a/lib/Controller/OcsApiController.php b/lib/Controller/OcsApiController.php index fbabc5ff..837a4fd7 100644 --- a/lib/Controller/OcsApiController.php +++ b/lib/Controller/OcsApiController.php @@ -22,21 +22,13 @@ class OcsApiController extends OCSController { public function __construct( IRequest $request, - private IRootFolder $root, - private UserMapper $userMapper, - private IUserManager $userManager, + private readonly IRootFolder $root, + private readonly UserMapper $userMapper, + private readonly IUserManager $userManager, ) { parent::__construct(Application::APP_ID, $request); } - /** - * @param int $providerId - * @param string $userId - * @param string|null $displayName - * @param string|null $email - * @param string|null $quota - * @return DataResponse - */ public function createUser(int $providerId, string $userId, ?string $displayName = null, ?string $email = null, ?string $quota = null): DataResponse { $backendUser = $this->userMapper->getOrCreate($providerId, $userId); @@ -57,21 +49,18 @@ public function createUser(int $providerId, string $userId, ?string $displayName $user->setQuota($quota); } - $userFolder = $this->root->getUserFolder($user->getUID()); + $userId = $user->getUID(); + $userFolder = $this->root->getUserFolder($userId); try { // copy skeleton - \OC_Util::copySkeleton($user->getUID(), $userFolder); + \OC_Util::copySkeleton($userId, $userFolder); } catch (NotPermittedException $ex) { // read only uses } - return new DataResponse(['user_id' => $user->getUID()]); + return new DataResponse(['user_id' => $userId]); } - /** - * @param string $userId - * @return DataResponse - */ public function deleteUser(string $userId): DataResponse { $user = $this->userManager->get($userId); if (is_null($user) || $user->getBackendClassName() !== Application::APP_ID) { diff --git a/lib/Controller/SettingsController.php b/lib/Controller/SettingsController.php index 004edc04..e4aba1fa 100644 --- a/lib/Controller/SettingsController.php +++ b/lib/Controller/SettingsController.php @@ -29,13 +29,13 @@ class SettingsController extends Controller { public function __construct( IRequest $request, - private IAppConfig $appConfig, - private ProviderMapper $providerMapper, - private ID4MeService $id4meService, - private ProviderService $providerService, - private ICrypto $crypto, - private IClientService $clientService, - private LoggerInterface $logger, + private readonly IAppConfig $appConfig, + private readonly ProviderMapper $providerMapper, + private readonly ID4MeService $id4meService, + private readonly ProviderService $providerService, + private readonly ICrypto $crypto, + private readonly IClientService $clientService, + private readonly LoggerInterface $logger, ) { parent::__construct(Application::APP_ID, $request); } @@ -55,7 +55,7 @@ public function isDiscoveryEndpointValid($url) { // Check if the request was successful if ($httpCode === Http::STATUS_OK && !empty($body)) { $result['isReachable'] = true; - $data = json_decode($body, true); + $data = json_decode($body, true, JSON_THROW_ON_ERROR); // Check for required fields as defined in: https://openid.net/specs/openid-connect-discovery-1_0.html $requiredFields = [ diff --git a/lib/Controller/TimezoneController.php b/lib/Controller/TimezoneController.php index f672cd94..4d9724e4 100644 --- a/lib/Controller/TimezoneController.php +++ b/lib/Controller/TimezoneController.php @@ -21,22 +21,19 @@ class TimezoneController extends Controller { public function __construct( string $appName, IRequest $request, - private IConfig $config, - private ISession $session, - private ?string $userId, + private readonly IConfig $config, + private readonly ISession $session, + private readonly ?string $userId, ) { parent::__construct($appName, $request); } /** - * @param string $timezone - * @param int $timezoneOffset - * @return JSONResponse * @throws \OCP\PreConditionNotMetException */ #[NoAdminRequired] #[UseSession] - public function setTimezone(string $timezone, int $timezoneOffset) { + public function setTimezone(string $timezone, int $timezoneOffset): JSONResponse { $this->config->setUserValue($this->userId, 'core', 'timezone', $timezone); $this->session->set('timezone', $timezoneOffset);