From 083ffeb18d8326618575504b530ff18594b16bcf Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Tue, 28 Jul 2026 14:36:42 +0200 Subject: [PATCH 1/2] feat: Add missing NoSubAdminRequired attribute Signed-off-by: Carl Schwan --- .../lib/Controller/AppConfigController.php | 4 +- .../lib/Controller/GroupsController.php | 4 +- .../lib/Controller/PreferencesController.php | 13 ++--- .../lib/Controller/UsersController.php | 31 +++++------- .../lib/Controller/VerificationController.php | 9 ++-- .../Middleware/ProvisioningApiMiddleware.php | 4 +- .../ProvisioningApiMiddlewareTest.php | 8 +-- .../Controller/AdminSettingsController.php | 3 +- .../lib/Controller/AuthSettingsController.php | 49 +++++-------------- .../Controller/ChangePasswordController.php | 5 +- .../DeclarativeSettingsController.php | 3 +- .../lib/Controller/HelpController.php | 7 +-- .../Controller/PersonalSettingsController.php | 5 +- .../lib/Controller/ReasonsController.php | 8 ++- .../lib/Controller/UsersController.php | 34 ++----------- .../lib/Controller/WebAuthnController.php | 13 ++--- .../lib/Middleware/SubadminMiddleware.php | 5 +- .../Controller/AuthSettingsControllerTest.php | 20 ++++---- .../Middleware/SubadminMiddlewareTest.php | 25 +++------- core/Controller/ProfileApiController.php | 4 +- .../Http/Attribute/NoSubAdminRequired.php | 21 ++++++++ 21 files changed, 104 insertions(+), 171 deletions(-) create mode 100644 lib/public/AppFramework/Http/Attribute/NoSubAdminRequired.php diff --git a/apps/provisioning_api/lib/Controller/AppConfigController.php b/apps/provisioning_api/lib/Controller/AppConfigController.php index ade9e8a73c84a..edee8f28695c7 100644 --- a/apps/provisioning_api/lib/Controller/AppConfigController.php +++ b/apps/provisioning_api/lib/Controller/AppConfigController.php @@ -15,6 +15,7 @@ use OCP\App\IAppManager; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\NoAdminRequired; +use OCP\AppFramework\Http\Attribute\NoSubAdminRequired; use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired; use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\OCSController; @@ -102,8 +103,6 @@ public function getValue(string $app, string $key, string $defaultValue = ''): D } /** - * @NoSubAdminRequired - * * Update the config value of an app * * @param string $app ID of the app @@ -116,6 +115,7 @@ public function getValue(string $app, string $key, string $defaultValue = ''): D */ #[PasswordConfirmationRequired] #[NoAdminRequired] + #[NoSubAdminRequired] public function setValue(string $app, string $key, string $value): DataResponse { $user = $this->userSession->getUser(); if ($user === null) { diff --git a/apps/provisioning_api/lib/Controller/GroupsController.php b/apps/provisioning_api/lib/Controller/GroupsController.php index bcae81099c381..1c1be04c88ded 100644 --- a/apps/provisioning_api/lib/Controller/GroupsController.php +++ b/apps/provisioning_api/lib/Controller/GroupsController.php @@ -17,6 +17,7 @@ use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\AuthorizedAdminSetting; use OCP\AppFramework\Http\Attribute\NoAdminRequired; +use OCP\AppFramework\Http\Attribute\NoSubAdminRequired; use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired; use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\OCS\OCSException; @@ -138,8 +139,6 @@ public function getGroup(string $groupId): DataResponse { } /** - * @NoSubAdminRequired - * * Get a list of users in the specified group * * @param string $groupId ID of the group @@ -151,6 +150,7 @@ public function getGroup(string $groupId): DataResponse { * 200: User IDs returned */ #[NoAdminRequired] + #[NoSubAdminRequired] public function getGroupUsers(string $groupId): DataResponse { $groupId = urldecode($groupId); diff --git a/apps/provisioning_api/lib/Controller/PreferencesController.php b/apps/provisioning_api/lib/Controller/PreferencesController.php index 8ae64e65b81d9..ee272d810b07d 100644 --- a/apps/provisioning_api/lib/Controller/PreferencesController.php +++ b/apps/provisioning_api/lib/Controller/PreferencesController.php @@ -11,6 +11,7 @@ use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\NoAdminRequired; +use OCP\AppFramework\Http\Attribute\NoSubAdminRequired; use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\OCSController; use OCP\Config\BeforePreferenceDeletedEvent; @@ -33,8 +34,6 @@ public function __construct( } /** - * @NoSubAdminRequired - * * Update multiple preference values of an app * * @param string $appId ID of the app @@ -46,6 +45,7 @@ public function __construct( * 400: Preference invalid */ #[NoAdminRequired] + #[NoSubAdminRequired] public function setMultiplePreferences(string $appId, array $configs): DataResponse { $userId = $this->userSession->getUser()->getUID(); @@ -78,8 +78,6 @@ public function setMultiplePreferences(string $appId, array $configs): DataRespo } /** - * @NoSubAdminRequired - * * Update a preference value of an app * * @param string $appId ID of the app @@ -91,6 +89,7 @@ public function setMultiplePreferences(string $appId, array $configs): DataRespo * 400: Preference invalid */ #[NoAdminRequired] + #[NoSubAdminRequired] public function setPreference(string $appId, string $configKey, string $configValue): DataResponse { $userId = $this->userSession->getUser()->getUID(); @@ -119,8 +118,6 @@ public function setPreference(string $appId, string $configKey, string $configVa } /** - * @NoSubAdminRequired - * * Delete multiple preferences for an app * * @param string $appId ID of the app @@ -132,6 +129,7 @@ public function setPreference(string $appId, string $configKey, string $configVa * 400: Preference invalid */ #[NoAdminRequired] + #[NoSubAdminRequired] public function deleteMultiplePreference(string $appId, array $configKeys): DataResponse { $userId = $this->userSession->getUser()->getUID(); @@ -162,8 +160,6 @@ public function deleteMultiplePreference(string $appId, array $configKeys): Data } /** - * @NoSubAdminRequired - * * Delete a preference for an app * * @param string $appId ID of the app @@ -174,6 +170,7 @@ public function deleteMultiplePreference(string $appId, array $configKeys): Data * 400: Preference invalid */ #[NoAdminRequired] + #[NoSubAdminRequired] public function deletePreference(string $appId, string $configKey): DataResponse { $userId = $this->userSession->getUser()->getUID(); diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index 370528af6d909..391c205238bea 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -26,6 +26,7 @@ use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\AuthorizedAdminSetting; use OCP\AppFramework\Http\Attribute\NoAdminRequired; +use OCP\AppFramework\Http\Attribute\NoSubAdminRequired; use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired; use OCP\AppFramework\Http\Attribute\UserRateLimit; use OCP\AppFramework\Http\DataResponse; @@ -349,8 +350,6 @@ public function getLastLoggedInUsers( } /** - * @NoSubAdminRequired - * * Search users by their phone numbers * * @param string $location Location of the phone number (for country code) @@ -360,6 +359,7 @@ public function getLastLoggedInUsers( * 200: Users returned * 400: Invalid location */ + #[NoSubAdminRequired] #[NoAdminRequired] public function searchByPhoneNumbers(string $location, array $search): DataResponse { if ($this->phoneNumberUtil->getCountryCodeForRegion($location) === null) { @@ -656,8 +656,6 @@ public function addUser( } /** - * @NoSubAdminRequired - * * Get the details of a user * * @param string $userId ID of the user @@ -667,6 +665,7 @@ public function addUser( * 200: User returned */ #[NoAdminRequired] + #[NoSubAdminRequired] public function getUser(string $userId): DataResponse { $includeScopes = false; $currentUser = $this->userSession->getUser(); @@ -683,8 +682,6 @@ public function getUser(string $userId): DataResponse { } /** - * @NoSubAdminRequired - * * Get the details of the current user * * @return DataResponse @@ -693,6 +690,7 @@ public function getUser(string $userId): DataResponse { * 200: Current user returned */ #[NoAdminRequired] + #[NoSubAdminRequired] public function getCurrentUser(): DataResponse { $user = $this->userSession->getUser(); if ($user) { @@ -705,8 +703,6 @@ public function getCurrentUser(): DataResponse { } /** - * @NoSubAdminRequired - * * Get a list of fields that are editable for the current user * * @return DataResponse, array{}> @@ -715,6 +711,7 @@ public function getCurrentUser(): DataResponse { * 200: Editable fields returned */ #[NoAdminRequired] + #[NoSubAdminRequired] public function getEditableFields(): DataResponse { $currentLoggedInUser = $this->userSession->getUser(); if (!$currentLoggedInUser instanceof IUser) { @@ -738,8 +735,6 @@ public function getEnabledApps(): DataResponse { } /** - * @NoSubAdminRequired - * * Get a list of fields that are editable for a user * * @param string $userId ID of the user @@ -749,6 +744,7 @@ public function getEnabledApps(): DataResponse { * 200: Editable fields for user returned */ #[NoAdminRequired] + #[NoSubAdminRequired] public function getEditableFieldsForUser(string $userId): DataResponse { $currentLoggedInUser = $this->userSession->getUser(); if (!$currentLoggedInUser instanceof IUser) { @@ -794,8 +790,6 @@ public function getEditableFieldsForUser(string $userId): DataResponse { } /** - * @NoSubAdminRequired - * * Update multiple values of the user's details * * @param string $userId ID of the user @@ -809,6 +803,7 @@ public function getEditableFieldsForUser(string $userId): DataResponse { */ #[PasswordConfirmationRequired] #[NoAdminRequired] + #[NoSubAdminRequired] #[UserRateLimit(limit: 5, period: 60)] public function editUserMultiValue( string $userId, @@ -1173,8 +1168,6 @@ private function validatePasswordChange(IUser $targetUser, string $password): ?a } /** - * @NoSubAdminRequired - * * Update a value of the user's details * * @param string $userId ID of the user @@ -1187,6 +1180,7 @@ private function validatePasswordChange(IUser $targetUser, string $password): ?a */ #[PasswordConfirmationRequired] #[NoAdminRequired] + #[NoSubAdminRequired] #[UserRateLimit(limit: 50, period: 600)] public function editUser(string $userId, string $key, string $value): DataResponse { $currentLoggedInUser = $this->userSession->getUser(); @@ -1611,8 +1605,6 @@ private function setEnabled(string $userId, bool $value): DataResponse { } /** - * @NoSubAdminRequired - * * Get a list of groups the user belongs to * * @param string $userId ID of the user @@ -1622,6 +1614,7 @@ private function setEnabled(string $userId, bool $value): DataResponse { * 200: Users groups returned */ #[NoAdminRequired] + #[NoSubAdminRequired] public function getUsersGroups(string $userId): DataResponse { $loggedInUser = $this->userSession->getUser(); @@ -1656,8 +1649,6 @@ public function getUsersGroups(string $userId): DataResponse { } /** - * @NoSubAdminRequired - * * Get a list of groups with details * * @param string $userId ID of the user @@ -1667,6 +1658,7 @@ public function getUsersGroups(string $userId): DataResponse { * 200: Users groups returned */ #[NoAdminRequired] + #[NoSubAdminRequired] public function getUsersGroupsDetails(string $userId): DataResponse { $loggedInUser = $this->userSession->getUser(); @@ -1733,8 +1725,6 @@ function (string $gid) { } /** - * @NoSubAdminRequired - * * Get a list of the groups the user is a subadmin of, with details * * @param string $userId ID of the user @@ -1744,6 +1734,7 @@ function (string $gid) { * 200: Users subadmin groups returned */ #[NoAdminRequired] + #[NoSubAdminRequired] public function getUserSubAdminGroupsDetails(string $userId): DataResponse { $loggedInUser = $this->userSession->getUser(); diff --git a/apps/provisioning_api/lib/Controller/VerificationController.php b/apps/provisioning_api/lib/Controller/VerificationController.php index d5a51a95ad07f..9472038d9b15b 100644 --- a/apps/provisioning_api/lib/Controller/VerificationController.php +++ b/apps/provisioning_api/lib/Controller/VerificationController.php @@ -15,6 +15,7 @@ use OCP\AppFramework\Http\Attribute\BruteForceProtection; use OCP\AppFramework\Http\Attribute\NoAdminRequired; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\Attribute\NoSubAdminRequired; use OCP\AppFramework\Http\Attribute\OpenAPI; use OCP\AppFramework\Http\TemplateResponse; use OCP\HintException; @@ -41,9 +42,7 @@ public function __construct( parent::__construct($appName, $request); } - /** - * @NoSubAdminRequired - */ + #[NoSubAdminRequired] #[NoAdminRequired] #[NoCSRFRequired] public function showVerifyMail(string $token, string $userId, string $key): TemplateResponse { @@ -71,9 +70,7 @@ public function showVerifyMail(string $token, string $userId, string $key): Temp ], TemplateResponse::RENDER_AS_GUEST); } - /** - * @NoSubAdminRequired - */ + #[NoSubAdminRequired] #[NoAdminRequired] #[BruteForceProtection(action: 'emailVerification')] public function verifyMail(string $token, string $userId, string $key): TemplateResponse { diff --git a/apps/provisioning_api/lib/Middleware/ProvisioningApiMiddleware.php b/apps/provisioning_api/lib/Middleware/ProvisioningApiMiddleware.php index 9f8f28d9197c6..1ad470b46d891 100644 --- a/apps/provisioning_api/lib/Middleware/ProvisioningApiMiddleware.php +++ b/apps/provisioning_api/lib/Middleware/ProvisioningApiMiddleware.php @@ -13,6 +13,7 @@ use OCP\AppFramework\Controller; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\AuthorizedAdminSetting; +use OCP\AppFramework\Http\Attribute\NoSubAdminRequired; use OCP\AppFramework\Http\Response; use OCP\AppFramework\Middleware; use OCP\AppFramework\OCS\OCSException; @@ -43,7 +44,8 @@ public function __construct( #[\Override] public function beforeController(Controller $controller, string $methodName): void { // If AuthorizedAdminSetting, the check will be done in the SecurityMiddleware - if (!$this->isAdmin && !$this->reflector->hasAnnotation('NoSubAdminRequired') && !$this->isSubAdmin && !$this->reflector->hasAnnotationOrAttribute('AuthorizedAdminSetting', AuthorizedAdminSetting::class)) { + if (!$this->isAdmin && !$this->reflector->hasAnnotationOrAttribute('NoSubAdminRequired', NoSubAdminRequired::class) + && !$this->isSubAdmin && !$this->reflector->hasAnnotationOrAttribute('AuthorizedAdminSetting', AuthorizedAdminSetting::class)) { throw new NotSubAdminException(); } } diff --git a/apps/provisioning_api/tests/Middleware/ProvisioningApiMiddlewareTest.php b/apps/provisioning_api/tests/Middleware/ProvisioningApiMiddlewareTest.php index 15760f21e16d0..0d1df6f8c9790 100644 --- a/apps/provisioning_api/tests/Middleware/ProvisioningApiMiddlewareTest.php +++ b/apps/provisioning_api/tests/Middleware/ProvisioningApiMiddlewareTest.php @@ -53,15 +53,11 @@ public function testBeforeController(bool $subadminRequired, bool $isAdmin, bool $isSubAdmin ); - $this->reflector->method('hasAnnotation') - ->willReturnCallback(function ($annotation) use ($subadminRequired) { + $this->reflector->method('hasAnnotationOrAttribute') + ->willReturnCallback(function ($annotation, $attribute) use ($subadminRequired, $hasSettingAuthorizationAnnotation) { if ($annotation === 'NoSubAdminRequired') { return !$subadminRequired; } - return false; - }); - $this->reflector->method('hasAnnotationOrAttribute') - ->willReturnCallback(function ($annotation, $attribute) use ($hasSettingAuthorizationAnnotation) { if ($annotation === 'AuthorizedAdminSetting') { return $hasSettingAuthorizationAnnotation; } diff --git a/apps/settings/lib/Controller/AdminSettingsController.php b/apps/settings/lib/Controller/AdminSettingsController.php index cd02d68ec3800..c6cfe2a2b7cfc 100644 --- a/apps/settings/lib/Controller/AdminSettingsController.php +++ b/apps/settings/lib/Controller/AdminSettingsController.php @@ -10,6 +10,7 @@ use OCP\AppFramework\Controller; use OCP\AppFramework\Http\Attribute\NoAdminRequired; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\Attribute\NoSubAdminRequired; use OCP\AppFramework\Http\Attribute\OpenAPI; use OCP\AppFramework\Http\TemplateResponse; use OCP\AppFramework\Services\IInitialState; @@ -47,10 +48,10 @@ public function __construct( } /** - * @NoSubAdminRequired * We are checking the permissions in the getSettings method. If there is no allowed * settings for the given section. The user will be greeted by an error message. */ + #[NoSubAdminRequired] #[NoAdminRequired] #[NoCSRFRequired] public function index(string $section): TemplateResponse { diff --git a/apps/settings/lib/Controller/AuthSettingsController.php b/apps/settings/lib/Controller/AuthSettingsController.php index afdc13b76253f..a0e82ed98d1e8 100644 --- a/apps/settings/lib/Controller/AuthSettingsController.php +++ b/apps/settings/lib/Controller/AuthSettingsController.php @@ -20,6 +20,7 @@ use OCP\AppFramework\Controller; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\NoAdminRequired; +use OCP\AppFramework\Http\Attribute\NoSubAdminRequired; use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired; use OCP\AppFramework\Http\JSONResponse; use OCP\AppFramework\Services\IAppConfig; @@ -56,10 +57,9 @@ public function __construct( } /** - * @NoSubAdminRequired - * * @param bool $qrcodeLogin If set to true, the returned token could be (depending on server settings) a onetime password, that can only be used to get the actual app password a single time */ + #[NoSubAdminRequired] #[NoAdminRequired] #[PasswordConfirmationRequired(strict: true)] public function create(string $name = '', bool $qrcodeLogin = false): JSONResponse { @@ -139,10 +139,7 @@ public function create(string $name = '', bool $qrcodeLogin = false): JSONRespon ]); } - /** - * @return JSONResponse - */ - private function getServiceNotAvailableResponse() { + private function getServiceNotAvailableResponse(): JSONResponse { $resp = new JSONResponse(); $resp->setStatus(Http::STATUS_SERVICE_UNAVAILABLE); return $resp; @@ -152,10 +149,8 @@ private function getServiceNotAvailableResponse() { * Return a 25 digit device password * * Example: AbCdE-fGhJk-MnPqR-sTwXy-23456 - * - * @return string */ - private function generateRandomDeviceToken() { + private function generateRandomDeviceToken(): string { $groups = []; for ($i = 0; $i < 5; $i++) { $groups[] = $this->random->generate(5, ISecureRandom::CHAR_HUMAN_READABLE); @@ -167,15 +162,10 @@ private function checkAppToken(): bool { return $this->session->exists('app_password'); } - /** - * @NoSubAdminRequired - * - * @param int $id - * @return array|JSONResponse - */ + #[NoSubAdminRequired] #[NoAdminRequired] #[PasswordConfirmationRequired(strict: true)] - public function destroy($id) { + public function destroy(int $id): JSONResponse { if ($this->checkAppToken()) { return new JSONResponse([], Http::STATUS_BAD_REQUEST); } @@ -195,20 +185,13 @@ public function destroy($id) { $this->tokenProvider->invalidateTokenById($this->userId, $token->getId()); $this->publishActivity($subject, $token->getId(), ['name' => $token->getName()]); - return []; + return new JSONResponse([]); } - /** - * @NoSubAdminRequired - * - * @param int $id - * @param array $scope - * @param string $name - * @return array|JSONResponse - */ + #[NoSubAdminRequired] #[NoAdminRequired] #[PasswordConfirmationRequired(strict: true)] - public function update($id, array $scope, string $name) { + public function update(int $id, array $scope, string $name): JSONResponse { if ($this->checkAppToken()) { return new JSONResponse([], Http::STATUS_BAD_REQUEST); } @@ -236,14 +219,9 @@ public function update($id, array $scope, string $name) { } $this->tokenProvider->updateToken($token); - return []; + return new JSONResponse([]); } - /** - * @param string $subject - * @param int $id - * @param array $parameters - */ private function publishActivity(string $subject, int $id, array $parameters = []): void { $event = $this->activityManager->generateEvent(); $event->setApp('settings') @@ -263,8 +241,6 @@ private function publishActivity(string $subject, int $id, array $parameters = [ /** * Find a token by given id and check if uid for current session belongs to this token * - * @param int $id - * @return IToken * @throws InvalidTokenException */ private function findTokenByIdAndUser(int $id): IToken { @@ -281,13 +257,10 @@ private function findTokenByIdAndUser(int $id): IToken { } /** - * @NoSubAdminRequired - * - * @param int $id - * @return JSONResponse * @throws InvalidTokenException * @throws ExpiredTokenException */ + #[NoSubAdminRequired] #[NoAdminRequired] #[PasswordConfirmationRequired] public function wipe(int $id): JSONResponse { diff --git a/apps/settings/lib/Controller/ChangePasswordController.php b/apps/settings/lib/Controller/ChangePasswordController.php index 3c1fcd0a9998a..b8e658db83ab9 100644 --- a/apps/settings/lib/Controller/ChangePasswordController.php +++ b/apps/settings/lib/Controller/ChangePasswordController.php @@ -18,6 +18,7 @@ use OCP\AppFramework\Controller; use OCP\AppFramework\Http\Attribute\BruteForceProtection; use OCP\AppFramework\Http\Attribute\NoAdminRequired; +use OCP\AppFramework\Http\Attribute\NoSubAdminRequired; use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired; use OCP\AppFramework\Http\JSONResponse; use OCP\HintException; @@ -41,9 +42,7 @@ public function __construct( parent::__construct($appName, $request); } - /** - * @NoSubAdminRequired - */ + #[NoSubAdminRequired] #[NoAdminRequired] #[BruteForceProtection(action: 'changePersonalPassword')] public function changePersonalPassword(string $oldpassword = '', ?string $newpassword = null): JSONResponse { diff --git a/apps/settings/lib/Controller/DeclarativeSettingsController.php b/apps/settings/lib/Controller/DeclarativeSettingsController.php index 4e4bee4043ca0..f233a5d7bbf6a 100644 --- a/apps/settings/lib/Controller/DeclarativeSettingsController.php +++ b/apps/settings/lib/Controller/DeclarativeSettingsController.php @@ -15,6 +15,7 @@ use OCA\Settings\ResponseDefinitions; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\NoAdminRequired; +use OCP\AppFramework\Http\Attribute\NoSubAdminRequired; use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired; use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\OCS\OCSBadRequestException; @@ -115,10 +116,10 @@ private function saveValue(string $app, string $formId, string $fieldId, mixed $ * * @return DataResponse, array{}> * @throws NotLoggedInException - * @NoSubAdminRequired * * 200: Forms returned */ + #[NoSubAdminRequired] #[NoAdminRequired] public function getForms(): DataResponse { $user = $this->userSession->getUser(); diff --git a/apps/settings/lib/Controller/HelpController.php b/apps/settings/lib/Controller/HelpController.php index e5e70998cd680..a0bf69e6de572 100644 --- a/apps/settings/lib/Controller/HelpController.php +++ b/apps/settings/lib/Controller/HelpController.php @@ -12,6 +12,7 @@ use OCP\AppFramework\Controller; use OCP\AppFramework\Http\Attribute\NoAdminRequired; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\Attribute\NoSubAdminRequired; use OCP\AppFramework\Http\Attribute\OpenAPI; use OCP\AppFramework\Http\ContentSecurityPolicy; use OCP\AppFramework\Http\TemplateResponse; @@ -41,13 +42,9 @@ public function __construct( parent::__construct($appName, $request); } - /** - * @return TemplateResponse - * - * @NoSubAdminRequired - */ #[NoCSRFRequired] #[NoAdminRequired] + #[NoSubAdminRequired] public function help(string $mode = 'user'): TemplateResponse { $this->navigationManager->setActiveEntry('help'); $pageTitle = $this->l10n->t('Administrator documentation'); diff --git a/apps/settings/lib/Controller/PersonalSettingsController.php b/apps/settings/lib/Controller/PersonalSettingsController.php index be74b2ed0c7a8..99eafc3a5ad9a 100644 --- a/apps/settings/lib/Controller/PersonalSettingsController.php +++ b/apps/settings/lib/Controller/PersonalSettingsController.php @@ -10,6 +10,7 @@ use OCP\AppFramework\Controller; use OCP\AppFramework\Http\Attribute\NoAdminRequired; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\Attribute\NoSubAdminRequired; use OCP\AppFramework\Http\Attribute\OpenAPI; use OCP\AppFramework\Http\TemplateResponse; use OCP\AppFramework\Services\IInitialState; @@ -46,10 +47,8 @@ public function __construct( $this->initialState = $initialState; } - /** - * @NoSubAdminRequired - */ #[NoAdminRequired] + #[NoSubAdminRequired] #[NoCSRFRequired] public function index(string $section): TemplateResponse { return $this->getIndexResponse( diff --git a/apps/settings/lib/Controller/ReasonsController.php b/apps/settings/lib/Controller/ReasonsController.php index f96797695a005..7975f0a360762 100644 --- a/apps/settings/lib/Controller/ReasonsController.php +++ b/apps/settings/lib/Controller/ReasonsController.php @@ -12,18 +12,16 @@ use OCP\AppFramework\Controller; use OCP\AppFramework\Http\Attribute\NoAdminRequired; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\Attribute\NoSubAdminRequired; use OCP\AppFramework\Http\Attribute\OpenAPI; use OCP\AppFramework\Http\DataDisplayResponse; #[OpenAPI(scope: OpenAPI::SCOPE_IGNORE)] class ReasonsController extends Controller { - - /** - * @NoSubAdminRequired - */ + #[NoSubAdminRequired] #[NoAdminRequired] #[NoCSRFRequired] - public function getPdf() { + public function getPdf(): DataDisplayResponse { $data = file_get_contents(__DIR__ . '/../../data/Reasons to use Nextcloud.pdf'); $resp = new DataDisplayResponse($data); diff --git a/apps/settings/lib/Controller/UsersController.php b/apps/settings/lib/Controller/UsersController.php index 19da07e854b49..82e501262d0d3 100644 --- a/apps/settings/lib/Controller/UsersController.php +++ b/apps/settings/lib/Controller/UsersController.php @@ -31,6 +31,7 @@ use OCP\AppFramework\Http\Attribute\AuthorizedAdminSetting; use OCP\AppFramework\Http\Attribute\NoAdminRequired; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\Attribute\NoSubAdminRequired; use OCP\AppFramework\Http\Attribute\OpenAPI; use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired; use OCP\AppFramework\Http\Attribute\UserRateLimit; @@ -329,32 +330,8 @@ protected function canAdminChangeUserPasswords(): bool { return $canChangePassword; } - /** - * @NoSubAdminRequired - * - * @param string|null $avatarScope - * @param string|null $displayname - * @param string|null $displaynameScope - * @param string|null $phone - * @param string|null $phoneScope - * @param string|null $email - * @param string|null $emailScope - * @param string|null $website - * @param string|null $websiteScope - * @param string|null $address - * @param string|null $addressScope - * @param string|null $twitter - * @param string|null $twitterScope - * @param string|null $bluesky - * @param string|null $blueskyScope - * @param string|null $fediverse - * @param string|null $fediverseScope - * @param string|null $birthdate - * @param string|null $birthdateScope - * - * @return DataResponse - */ #[NoAdminRequired] + #[NoSubAdminRequired] #[PasswordConfirmationRequired] #[UserRateLimit(limit: 5, period: 60)] public function setUserSettings(?string $avatarScope = null, @@ -378,7 +355,7 @@ public function setUserSettings(?string $avatarScope = null, ?string $birthdateScope = null, ?string $pronouns = null, ?string $pronounsScope = null, - ) { + ): DataResponse { $user = $this->userSession->getUser(); if (!$user instanceof IUser) { return new DataResponse( @@ -523,12 +500,9 @@ protected function saveUserSettings(IAccount $userAccount): void { /** * Set the mail address of a user * - * @NoSubAdminRequired - * - * @param string $account * @param bool $onlyVerificationCode only return verification code without updating the data - * @return DataResponse */ + #[NoSubAdminRequired] #[NoAdminRequired] #[PasswordConfirmationRequired] public function getVerificationCode(string $account, bool $onlyVerificationCode): DataResponse { diff --git a/apps/settings/lib/Controller/WebAuthnController.php b/apps/settings/lib/Controller/WebAuthnController.php index 1f2f1e0ceb228..d61a2a8904328 100644 --- a/apps/settings/lib/Controller/WebAuthnController.php +++ b/apps/settings/lib/Controller/WebAuthnController.php @@ -15,6 +15,7 @@ use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\NoAdminRequired; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\Attribute\NoSubAdminRequired; use OCP\AppFramework\Http\Attribute\OpenAPI; use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired; use OCP\AppFramework\Http\Attribute\UseSession; @@ -39,9 +40,7 @@ public function __construct( parent::__construct(Application::APP_ID, $request); } - /** - * @NoSubAdminRequired - */ + #[NoSubAdminRequired] #[NoAdminRequired] #[PasswordConfirmationRequired] #[UseSession] @@ -57,9 +56,7 @@ public function startRegistration(): JSONResponse { return new JSONResponse($credentialOptions); } - /** - * @NoSubAdminRequired - */ + #[NoSubAdminRequired] #[NoAdminRequired] #[PasswordConfirmationRequired] #[UseSession] @@ -79,9 +76,7 @@ public function finishRegistration(string $name, string $data): JSONResponse { return new JSONResponse($this->manager->finishRegister($publicKeyCredentialCreationOptions, $name, $data)); } - /** - * @NoSubAdminRequired - */ + #[NoSubAdminRequired] #[NoAdminRequired] #[PasswordConfirmationRequired] public function deleteRegistration(int $id): JSONResponse { diff --git a/apps/settings/lib/Middleware/SubadminMiddleware.php b/apps/settings/lib/Middleware/SubadminMiddleware.php index 24d443a55d2f6..7074bb8e4788b 100644 --- a/apps/settings/lib/Middleware/SubadminMiddleware.php +++ b/apps/settings/lib/Middleware/SubadminMiddleware.php @@ -15,6 +15,7 @@ use OC\AppFramework\Utility\ControllerMethodReflector; use OCP\AppFramework\Controller; use OCP\AppFramework\Http\Attribute\AuthorizedAdminSetting; +use OCP\AppFramework\Http\Attribute\NoSubAdminRequired; use OCP\AppFramework\Http\TemplateResponse; use OCP\AppFramework\Middleware; use OCP\Group\ISubAdmin; @@ -24,7 +25,7 @@ /** * Verifies whether a user has at least sub-admin rights. - * To bypass use the `@NoSubAdminRequired` annotation + * To bypass use the `#[NoSubAdminRequired]` annotation */ class SubadminMiddleware extends Middleware { public function __construct( @@ -45,7 +46,7 @@ private function isSubAdmin(): bool { #[Override] public function beforeController(Controller $controller, string $methodName): void { - if (!$this->reflector->hasAnnotation('NoSubAdminRequired') && !$this->reflector->hasAnnotationOrAttribute('AuthorizedAdminSetting', AuthorizedAdminSetting::class)) { + if (!$this->reflector->hasAnnotationOrAttribute('NoSubAdminRequired', NoSubAdminRequired::class) && !$this->reflector->hasAnnotationOrAttribute('AuthorizedAdminSetting', AuthorizedAdminSetting::class)) { if (!$this->isSubAdmin()) { throw new NotAdminException($this->l10n->t('Logged in account must be a sub admin')); } diff --git a/apps/settings/tests/Controller/AuthSettingsControllerTest.php b/apps/settings/tests/Controller/AuthSettingsControllerTest.php index f3503e3c2d7f3..cf05df6a45d73 100644 --- a/apps/settings/tests/Controller/AuthSettingsControllerTest.php +++ b/apps/settings/tests/Controller/AuthSettingsControllerTest.php @@ -152,7 +152,7 @@ public function testCreateDisabledBySystemConfig(): void { $expected = new JSONResponse(); $expected->setStatus(Http::STATUS_SERVICE_UNAVAILABLE); - $this->assertEquals($expected, $this->controller->create($name)); + $this->assertEquals($expected, $this->controller->create($name)->getData()); } public function testCreateSessionNotAvailable(): void { @@ -165,7 +165,7 @@ public function testCreateSessionNotAvailable(): void { $expected = new JSONResponse(); $expected->setStatus(Http::STATUS_SERVICE_UNAVAILABLE); - $this->assertEquals($expected, $this->controller->create($name)); + $this->assertEquals($expected, $this->controller->create($name)->getData()); } public function testCreateInvalidToken(): void { @@ -185,7 +185,7 @@ public function testCreateInvalidToken(): void { $expected = new JSONResponse(); $expected->setStatus(Http::STATUS_SERVICE_UNAVAILABLE); - $this->assertEquals($expected, $this->controller->create($name)); + $this->assertEquals($expected, $this->controller->create($name)->getData()); } public function testDestroy(): void { @@ -207,7 +207,7 @@ public function testDestroy(): void { ->method('invalidateTokenById') ->with($this->uid, $tokenId); - $this->assertEquals([], $this->controller->destroy($tokenId)); + $this->assertEquals([], $this->controller->destroy($tokenId)->getData()); } public function testDestroyExpired(): void { @@ -231,7 +231,7 @@ public function testDestroyExpired(): void { ->method('invalidateTokenById') ->with($this->uid, $tokenId); - $this->assertSame([], $this->controller->destroy($tokenId)); + $this->assertSame([], $this->controller->destroy($tokenId)->getData()); } public function testDestroyWipePendingEmitsCancelledSubject(): void { @@ -268,7 +268,7 @@ public function testDestroyWipePendingEmitsCancelledSubject(): void { $this->activityManager->expects($this->once()) ->method('publish'); - $this->assertEquals([], $this->controller->destroy($tokenId)); + $this->assertEquals([], $this->controller->destroy($tokenId)->getData()); } public function testDestroyWrongUser(): void { @@ -321,7 +321,7 @@ public function testUpdateRename(string $name, string $newName): void { ->method('updateToken') ->with($this->equalTo($token)); - $this->assertSame([], $this->controller->update($tokenId, [IToken::SCOPE_FILESYSTEM => true], $newName)); + $this->assertSame([], $this->controller->update($tokenId, [IToken::SCOPE_FILESYSTEM => true], $newName)->getData()); } public static function dataUpdateFilesystemScope(): array { @@ -359,7 +359,7 @@ public function testUpdateFilesystemScope(bool $filesystem, bool $newFilesystem) ->method('updateToken') ->with($this->equalTo($token)); - $this->assertSame([], $this->controller->update($tokenId, [IToken::SCOPE_FILESYSTEM => $newFilesystem], 'App password')); + $this->assertSame([], $this->controller->update($tokenId, [IToken::SCOPE_FILESYSTEM => $newFilesystem], 'App password')->getData()); } public function testUpdateNoChange(): void { @@ -390,7 +390,7 @@ public function testUpdateNoChange(): void { ->method('updateToken') ->with($this->equalTo($token)); - $this->assertSame([], $this->controller->update($tokenId, [IToken::SCOPE_FILESYSTEM => true], 'App password')); + $this->assertSame([], $this->controller->update($tokenId, [IToken::SCOPE_FILESYSTEM => true], 'App password')->getData()); } public function testUpdateExpired(): void { @@ -410,7 +410,7 @@ public function testUpdateExpired(): void { ->method('updateToken') ->with($this->equalTo($token)); - $this->assertSame([], $this->controller->update($tokenId, [IToken::SCOPE_FILESYSTEM => true], 'App password')); + $this->assertSame([], $this->controller->update($tokenId, [IToken::SCOPE_FILESYSTEM => true], 'App password')->getData()); } public function testUpdateTokenWrongUser(): void { diff --git a/apps/settings/tests/Middleware/SubadminMiddlewareTest.php b/apps/settings/tests/Middleware/SubadminMiddlewareTest.php index 371af67734025..12fb31ce0c1b0 100644 --- a/apps/settings/tests/Middleware/SubadminMiddlewareTest.php +++ b/apps/settings/tests/Middleware/SubadminMiddlewareTest.php @@ -15,6 +15,7 @@ use OCA\Settings\Middleware\SubadminMiddleware; use OCP\AppFramework\Controller; use OCP\AppFramework\Http\Attribute\AuthorizedAdminSetting; +use OCP\AppFramework\Http\Attribute\NoSubAdminRequired; use OCP\AppFramework\Http\TemplateResponse; use OCP\Group\ISubAdmin; use OCP\IL10N; @@ -24,7 +25,7 @@ /** * Verifies whether an user has at least subadmin rights. - * To bypass use the `@NoSubAdminRequired` annotation + * To bypass use the `#[NoSubAdminRequired]` attribute * * @package Tests\Settings\Middleware */ @@ -62,15 +63,10 @@ public function testBeforeControllerAsUserWithoutAnnotation(): void { $this->expectException(NotAdminException::class); $this->reflector - ->expects($this->exactly(1)) - ->method('hasAnnotation') - ->willReturnMap([ - ['NoSubAdminRequired', false], - ]); - $this->reflector - ->expects($this->exactly(1)) + ->expects($this->exactly(2)) ->method('hasAnnotationOrAttribute') ->willReturnMap([ + ['NoSubAdminRequired', NoSubAdminRequired::class, false], ['AuthorizedAdminSetting', AuthorizedAdminSetting::class, false], ]); @@ -85,8 +81,8 @@ public function testBeforeControllerAsUserWithoutAnnotation(): void { public function testBeforeControllerWithAnnotation(): void { $this->reflector ->expects($this->once()) - ->method('hasAnnotation') - ->with('NoSubAdminRequired') + ->method('hasAnnotationOrAttribute') + ->with('NoSubAdminRequired', NoSubAdminRequired::class) ->willReturn(true); $this->subAdminManager @@ -98,15 +94,10 @@ public function testBeforeControllerWithAnnotation(): void { public function testBeforeControllerAsSubAdminWithoutAnnotation(): void { $this->reflector - ->expects($this->exactly(1)) - ->method('hasAnnotation') - ->willReturnMap([ - ['NoSubAdminRequired', false], - ]); - $this->reflector - ->expects($this->exactly(1)) + ->expects($this->exactly(2)) ->method('hasAnnotationOrAttribute') ->willReturnMap([ + ['NoSubAdminRequired', NoSubAdminRequired::class, false], ['AuthorizedAdminSetting', AuthorizedAdminSetting::class, false], ]); diff --git a/core/Controller/ProfileApiController.php b/core/Controller/ProfileApiController.php index 02979cb164979..97c5682b1b70e 100644 --- a/core/Controller/ProfileApiController.php +++ b/core/Controller/ProfileApiController.php @@ -16,6 +16,7 @@ use OCP\AppFramework\Http\Attribute\ApiRoute; use OCP\AppFramework\Http\Attribute\BruteForceProtection; use OCP\AppFramework\Http\Attribute\NoAdminRequired; +use OCP\AppFramework\Http\Attribute\NoSubAdminRequired; use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired; use OCP\AppFramework\Http\Attribute\UserRateLimit; use OCP\AppFramework\Http\DataResponse; @@ -49,8 +50,6 @@ public function __construct( } /** - * @NoSubAdminRequired - * * Update the visibility of a parameter * * @param string $targetUserId ID of the user @@ -63,6 +62,7 @@ public function __construct( * * 200: Visibility updated successfully */ + #[NoSubAdminRequired] #[NoAdminRequired] #[PasswordConfirmationRequired] #[UserRateLimit(limit: 40, period: 600)] diff --git a/lib/public/AppFramework/Http/Attribute/NoSubAdminRequired.php b/lib/public/AppFramework/Http/Attribute/NoSubAdminRequired.php new file mode 100644 index 0000000000000..ea5bc047ce267 --- /dev/null +++ b/lib/public/AppFramework/Http/Attribute/NoSubAdminRequired.php @@ -0,0 +1,21 @@ + Date: Tue, 28 Jul 2026 15:06:08 +0200 Subject: [PATCH 2/2] perf: Remove MiddlewareUtils overhead Avoid doing some reflection Signed-off-by: Carl Schwan --- lib/composer/composer/autoload_classmap.php | 1 - lib/composer/composer/autoload_static.php | 1 - .../DependencyInjection/DIContainer.php | 3 +- .../Middleware/MiddlewareUtils.php | 60 ------------------- .../Middleware/Security/CORSMiddleware.php | 12 ++-- .../Security/SameSiteCookieMiddleware.php | 8 +-- .../Security/SecurityMiddleware.php | 60 ++++++++++++------- .../Utility/ControllerMethodReflector.php | 13 ++++ .../Security/CORSMiddlewareTest.php | 25 ++++---- .../Security/SameSiteCookieMiddlewareTest.php | 5 +- .../Security/SecurityMiddlewareTest.php | 5 +- 11 files changed, 76 insertions(+), 117 deletions(-) delete mode 100644 lib/private/AppFramework/Middleware/MiddlewareUtils.php diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index beafb4700c69d..2aa14317081a3 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1177,7 +1177,6 @@ 'OC\\AppFramework\\Middleware\\AdditionalScriptsMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/AdditionalScriptsMiddleware.php', 'OC\\AppFramework\\Middleware\\CompressionMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/CompressionMiddleware.php', 'OC\\AppFramework\\Middleware\\MiddlewareDispatcher' => $baseDir . '/lib/private/AppFramework/Middleware/MiddlewareDispatcher.php', - 'OC\\AppFramework\\Middleware\\MiddlewareUtils' => $baseDir . '/lib/private/AppFramework/Middleware/MiddlewareUtils.php', 'OC\\AppFramework\\Middleware\\NotModifiedMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/NotModifiedMiddleware.php', 'OC\\AppFramework\\Middleware\\OCSMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/OCSMiddleware.php', 'OC\\AppFramework\\Middleware\\PublicShare\\Exceptions\\NeedAuthenticationException' => $baseDir . '/lib/private/AppFramework/Middleware/PublicShare/Exceptions/NeedAuthenticationException.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index e024c1fb44b9f..1b1bbb346597d 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1218,7 +1218,6 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\AppFramework\\Middleware\\AdditionalScriptsMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/AdditionalScriptsMiddleware.php', 'OC\\AppFramework\\Middleware\\CompressionMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/CompressionMiddleware.php', 'OC\\AppFramework\\Middleware\\MiddlewareDispatcher' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/MiddlewareDispatcher.php', - 'OC\\AppFramework\\Middleware\\MiddlewareUtils' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/MiddlewareUtils.php', 'OC\\AppFramework\\Middleware\\NotModifiedMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/NotModifiedMiddleware.php', 'OC\\AppFramework\\Middleware\\OCSMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/OCSMiddleware.php', 'OC\\AppFramework\\Middleware\\PublicShare\\Exceptions\\NeedAuthenticationException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/PublicShare/Exceptions/NeedAuthenticationException.php', diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php index 15a1aff97ea66..28580065dfdde 100644 --- a/lib/private/AppFramework/DependencyInjection/DIContainer.php +++ b/lib/private/AppFramework/DependencyInjection/DIContainer.php @@ -17,7 +17,6 @@ use OC\AppFramework\Middleware\AdditionalScriptsMiddleware; use OC\AppFramework\Middleware\CompressionMiddleware; use OC\AppFramework\Middleware\MiddlewareDispatcher; -use OC\AppFramework\Middleware\MiddlewareUtils; use OC\AppFramework\Middleware\NotModifiedMiddleware; use OC\AppFramework\Middleware\OCSMiddleware; use OC\AppFramework\Middleware\PublicShare\PublicShareMiddleware; @@ -205,7 +204,7 @@ public function __construct( $securityMiddleware = new SecurityMiddleware( $c->get(IRequest::class), - $c->get(MiddlewareUtils::class), + $c->get(ControllerMethodReflector::class), $c->get(INavigationManager::class), $c->get(IURLGenerator::class), $c->get(LoggerInterface::class), diff --git a/lib/private/AppFramework/Middleware/MiddlewareUtils.php b/lib/private/AppFramework/Middleware/MiddlewareUtils.php deleted file mode 100644 index 020d259e6322e..0000000000000 --- a/lib/private/AppFramework/Middleware/MiddlewareUtils.php +++ /dev/null @@ -1,60 +0,0 @@ - $attributeClass - * @deprecated 34.0.0 call directly on the reflector - */ - public function hasAnnotationOrAttribute(ReflectionMethod $reflectionMethod, ?string $annotationName, string $attributeClass): bool { - return $this->reflector->hasAnnotationOrAttribute($annotationName, $attributeClass); - } - - /** - * @param ReflectionMethod $reflectionMethod - * @return string[] - */ - public function getAuthorizedAdminSettingClasses(ReflectionMethod $reflectionMethod): array { - $classes = []; - if ($this->reflector->hasAnnotation('AuthorizedAdminSetting')) { - $classes = explode(';', $this->reflector->getAnnotationParameter('AuthorizedAdminSetting', 'settings')); - } - - $attributes = $reflectionMethod->getAttributes(AuthorizedAdminSetting::class); - if (!empty($attributes)) { - foreach ($attributes as $attribute) { - /** @var AuthorizedAdminSetting $setting */ - $setting = $attribute->newInstance(); - $classes[] = $setting->getSettings(); - } - } - - return $classes; - } -} diff --git a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php index a894fe9a4a1e3..9fbdf5ffc5c33 100644 --- a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php @@ -8,8 +8,8 @@ namespace OC\AppFramework\Middleware\Security; -use OC\AppFramework\Middleware\MiddlewareUtils; use OC\AppFramework\Middleware\Security\Exceptions\SecurityException; +use OC\AppFramework\Utility\ControllerMethodReflector; use OC\Authentication\Exceptions\PasswordLoginForbiddenException; use OC\User\Session; use OCP\AppFramework\Controller; @@ -35,7 +35,7 @@ class CORSMiddleware extends Middleware { public function __construct( private readonly IRequest $request, - private readonly MiddlewareUtils $middlewareUtils, + private readonly ControllerMethodReflector $reflector, private readonly Session $session, private readonly IThrottler $throttler, ) { @@ -43,12 +43,10 @@ public function __construct( #[Override] public function beforeController(Controller $controller, string $methodName): void { - $reflectionMethod = new ReflectionMethod($controller, $methodName); - // ensure that @CORS annotated API routes are not used in conjunction // with session authentication since this enables CSRF attack vectors - if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'CORS', CORS::class) - && (!$this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'PublicPage', PublicPage::class) || $this->session->isLoggedIn())) { + if ($this->reflector->hasAnnotationOrAttribute('CORS', CORS::class) + && (!$this->reflector->hasAnnotationOrAttribute('PublicPage', PublicPage::class) || $this->session->isLoggedIn())) { $user = array_key_exists('PHP_AUTH_USER', $this->request->server) ? $this->request->server['PHP_AUTH_USER'] : null; $pass = array_key_exists('PHP_AUTH_PW', $this->request->server) ? $this->request->server['PHP_AUTH_PW'] : null; @@ -77,7 +75,7 @@ public function afterController(Controller $controller, string $methodName, Resp if (isset($this->request->server['HTTP_ORIGIN'])) { $reflectionMethod = new ReflectionMethod($controller, $methodName); - if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'CORS', CORS::class)) { + if ($this->reflector->hasAnnotationOrAttribute('CORS', CORS::class)) { // allow credentials headers must not be true or CSRF is possible // otherwise foreach ($response->getHeaders() as $header => $value) { diff --git a/lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php b/lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php index d6420607ee6b7..a1b5cc299b010 100644 --- a/lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php @@ -10,19 +10,18 @@ namespace OC\AppFramework\Middleware\Security; use OC\AppFramework\Http\Request; -use OC\AppFramework\Middleware\MiddlewareUtils; use OC\AppFramework\Middleware\Security\Exceptions\LaxSameSiteCookieFailedException; +use OC\AppFramework\Utility\ControllerMethodReflector; use OCP\AppFramework\Controller; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\NoSameSiteCookieRequired; use OCP\AppFramework\Http\Response; use OCP\AppFramework\Middleware; -use ReflectionMethod; class SameSiteCookieMiddleware extends Middleware { public function __construct( private readonly Request $request, - private readonly MiddlewareUtils $middlewareUtils, + private readonly ControllerMethodReflector $reflector, ) { } @@ -36,8 +35,7 @@ public function beforeController(Controller $controller, string $methodName): vo return; } - $reflectionMethod = new ReflectionMethod($controller, $methodName); - $noSSC = $this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'NoSameSiteCookieRequired', NoSameSiteCookieRequired::class); + $noSSC = $this->reflector->hasAnnotationOrAttribute('NoSameSiteCookieRequired', NoSameSiteCookieRequired::class); if ($noSSC) { return; } diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php index 5456d2ab940cc..0c31a9d76751e 100644 --- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php @@ -9,7 +9,6 @@ namespace OC\AppFramework\Middleware\Security; -use OC\AppFramework\Middleware\MiddlewareUtils; use OC\AppFramework\Middleware\Security\Exceptions\AdminIpNotAllowedException; use OC\AppFramework\Middleware\Security\Exceptions\AppNotEnabledException; use OC\AppFramework\Middleware\Security\Exceptions\CrossSiteRequestForgeryException; @@ -19,6 +18,7 @@ use OC\AppFramework\Middleware\Security\Exceptions\NotLoggedInException; use OC\AppFramework\Middleware\Security\Exceptions\SecurityException; use OC\AppFramework\Middleware\Security\Exceptions\StrictCookieMissingException; +use OC\AppFramework\Utility\ControllerMethodReflector; use OC\Security\CSRF\CsrfTokenManager; use OC\Settings\AuthorizedGroupMapper; use OC\User\Session; @@ -64,7 +64,7 @@ class SecurityMiddleware extends Middleware { public function __construct( private readonly IRequest $request, - private readonly MiddlewareUtils $middlewareUtils, + private readonly ControllerMethodReflector $reflector, private readonly INavigationManager $navigationManager, private readonly IURLGenerator $urlGenerator, private readonly LoggerInterface $logger, @@ -118,18 +118,16 @@ public function beforeController(Controller $controller, string $methodName): vo $this->navigationManager->setActiveEntry('spreed'); } - $reflectionMethod = new ReflectionMethod($controller, $methodName); - // security checks - $isPublicPage = $this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'PublicPage', PublicPage::class); + $isPublicPage = $this->reflector->hasAnnotationOrAttribute('PublicPage', PublicPage::class); - if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'ExAppRequired', ExAppRequired::class)) { + if ($this->reflector->hasAnnotationOrAttribute('ExAppRequired', ExAppRequired::class)) { if (!$this->userSession instanceof Session || $this->userSession->getSession()->get('app_api') !== true) { throw new ExAppRequiredException(); } } elseif (!$isPublicPage) { $authorized = false; - if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, null, AppApiAdminAccessWithoutUser::class)) { + if ($this->reflector->hasAnnotationOrAttribute(null, AppApiAdminAccessWithoutUser::class)) { // this attribute allows ExApp to access admin endpoints only if "userId" is "null" if ($this->userSession instanceof Session && $this->userSession->getSession()->get('app_api') === true && $this->userSession->getUser() === null) { $authorized = true; @@ -140,15 +138,15 @@ public function beforeController(Controller $controller, string $methodName): vo throw new NotLoggedInException(); } - if (!$authorized && $this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'AuthorizedAdminSetting', AuthorizedAdminSetting::class)) { + if (!$authorized && $this->reflector->hasAnnotationOrAttribute('AuthorizedAdminSetting', AuthorizedAdminSetting::class)) { $authorized = $this->isAdminUser(); - if (!$authorized && $this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class)) { + if (!$authorized && $this->reflector->hasAnnotationOrAttribute('SubAdminRequired', SubAdminRequired::class)) { $authorized = $this->isSubAdmin(); } if (!$authorized) { - $settingClasses = $this->middlewareUtils->getAuthorizedAdminSettingClasses($reflectionMethod); + $settingClasses = $this->getAuthorizedAdminSettingClasses(); $authorizedClasses = $this->groupAuthorizationMapper->findAllClassesForUser($this->userSession->getUser()); foreach ($settingClasses as $settingClass) { $authorized = in_array($settingClass, $authorizedClasses, true); @@ -165,24 +163,24 @@ public function beforeController(Controller $controller, string $methodName): vo throw new AdminIpNotAllowedException($this->l10n->t('Your current IP address doesn\'t allow you to perform admin actions')); } } - if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class) + if ($this->reflector->hasAnnotationOrAttribute('SubAdminRequired', SubAdminRequired::class) && !$this->isSubAdmin() && !$this->isAdminUser() && !$authorized) { throw new NotAdminException($this->l10n->t('Logged in account must be an admin or sub admin')); } - if (!$this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class) - && !$this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'NoAdminRequired', NoAdminRequired::class) + if (!$this->reflector->hasAnnotationOrAttribute('SubAdminRequired', SubAdminRequired::class) + && !$this->reflector->hasAnnotationOrAttribute('NoAdminRequired', NoAdminRequired::class) && !$this->isAdminUser() && !$authorized) { throw new NotAdminException($this->l10n->t('Logged in account must be an admin')); } - if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class) + if ($this->reflector->hasAnnotationOrAttribute('SubAdminRequired', SubAdminRequired::class) && !$this->remoteAddress->allowsAdminActions()) { throw new AdminIpNotAllowedException($this->l10n->t('Your current IP address doesn\'t allow you to perform admin actions')); } - if (!$this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class) - && !$this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'NoAdminRequired', NoAdminRequired::class) + if (!$this->reflector->hasAnnotationOrAttribute('SubAdminRequired', SubAdminRequired::class) + && !$this->reflector->hasAnnotationOrAttribute('NoAdminRequired', NoAdminRequired::class) && !$this->remoteAddress->allowsAdminActions()) { throw new AdminIpNotAllowedException($this->l10n->t('Your current IP address doesn\'t allow you to perform admin actions')); } @@ -190,15 +188,15 @@ public function beforeController(Controller $controller, string $methodName): vo } // Check for strict cookie requirement - if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'StrictCookieRequired', StrictCookiesRequired::class) - || !$this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'NoCSRFRequired', NoCSRFRequired::class)) { + if ($this->reflector->hasAnnotationOrAttribute('StrictCookieRequired', StrictCookiesRequired::class) + || !$this->reflector->hasAnnotationOrAttribute('NoCSRFRequired', NoCSRFRequired::class)) { if (!$this->request->passesStrictCookieCheck()) { throw new StrictCookieMissingException(); } } // CSRF check - also registers the CSRF token since the session may be closed later Server::get(CsrfTokenManager::class)->generateSessionToken(); - if ($this->isInvalidCSRFRequired($reflectionMethod)) { + if ($this->isInvalidCSRFRequired()) { /* * Only allow the CSRF check to fail on OCS Requests. This kind of * hacks around that we have no full token auth in place yet and we @@ -229,8 +227,8 @@ public function beforeController(Controller $controller, string $methodName): vo } } - private function isInvalidCSRFRequired(ReflectionMethod $reflectionMethod): bool { - if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'NoCSRFRequired', NoCSRFRequired::class)) { + private function isInvalidCSRFRequired(): bool { + if ($this->reflector->hasAnnotationOrAttribute('NoCSRFRequired', NoCSRFRequired::class)) { return false; } @@ -296,4 +294,24 @@ public function afterException(Controller $controller, string $methodName, \Exce throw $exception; } + + /** + * @param ReflectionMethod $reflectionMethod + * @return string[] + */ + public function getAuthorizedAdminSettingClasses(): array { + $classes = []; + if ($this->reflector->hasAnnotation('AuthorizedAdminSetting')) { + $classes = explode(';', $this->reflector->getAnnotationParameter('AuthorizedAdminSetting', 'settings')); + } + + $attribute = $this->reflector->getAttribute(AuthorizedAdminSetting::class); + if ($attribute !== null) { + /** @var AuthorizedAdminSetting $setting */ + $setting = $attribute->newInstance(); + $classes[] = $setting->getSettings(); + } + + return $classes; + } } diff --git a/lib/private/AppFramework/Utility/ControllerMethodReflector.php b/lib/private/AppFramework/Utility/ControllerMethodReflector.php index b064dfc2bb38b..efe1aba282144 100644 --- a/lib/private/AppFramework/Utility/ControllerMethodReflector.php +++ b/lib/private/AppFramework/Utility/ControllerMethodReflector.php @@ -147,6 +147,19 @@ public function hasAnnotationOrAttribute(?string $annotationName, string $attrib return false; } + /** + * @template T + * @param class-string $attributeClass + * @return ?T + */ + public function getAttribute(string $attributeClass): ?object { + $attributes = $this->reflectionMethod->getAttributes($attributeClass); + if (!empty($attributes)) { + return $attributes[0]; + } + return null; + } + /** * Check if a method contains an annotation * @param string $name the name of the annotation diff --git a/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php index 702b0c3260775..5695923ac3dc4 100644 --- a/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php @@ -9,7 +9,6 @@ namespace Test\AppFramework\Middleware\Security; use OC\AppFramework\Http\Request; -use OC\AppFramework\Middleware\MiddlewareUtils; use OC\AppFramework\Middleware\Security\CORSMiddleware; use OC\AppFramework\Middleware\Security\Exceptions\SecurityException; use OC\AppFramework\Utility\ControllerMethodReflector; @@ -65,7 +64,7 @@ public function testSetCORSAPIHeader(string $method): void { $this->createMock(IConfig::class) ); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, new MiddlewareUtils($this->reflector, $this->logger), $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); $response = $middleware->afterController($this->controller, $method, new Response()); $headers = $response->getHeaders(); @@ -83,7 +82,7 @@ public function testNoAnnotationNoCORSHEADER(): void { $this->createMock(IConfig::class) ); $this->reflector->reflect($this->controller, __FUNCTION__); - $middleware = new CORSMiddleware($request, new MiddlewareUtils($this->reflector, $this->logger), $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); $response = $middleware->afterController($this->controller, __FUNCTION__, new Response()); $headers = $response->getHeaders(); @@ -105,7 +104,7 @@ public function testNoOriginHeaderNoCORSHEADER(string $method): void { $this->createMock(IConfig::class) ); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, new MiddlewareUtils($this->reflector, $this->logger), $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); $response = $middleware->afterController($this->controller, $method, new Response()); $headers = $response->getHeaders(); @@ -133,7 +132,7 @@ public function testCorsIgnoredIfWithCredentialsHeaderPresent(string $method): v $this->createMock(IConfig::class) ); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, new MiddlewareUtils($this->reflector, $this->logger), $this->session, $this->throttler, $this->logger); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->logger); $response = new Response(); $response->addHeader('AcCess-control-Allow-Credentials ', 'TRUE'); @@ -157,7 +156,7 @@ public function testNoCORSOnAnonymousPublicPage(string $method): void { $this->createMock(IConfig::class) ); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, new MiddlewareUtils($this->reflector, $this->logger), $this->session, $this->throttler, $this->logger); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->logger); $this->session->expects($this->once()) ->method('isLoggedIn') ->willReturn(false); @@ -189,7 +188,7 @@ public function testCORSShouldNeverAllowCookieAuth(string $method): void { $this->createMock(IConfig::class) ); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, new MiddlewareUtils($this->reflector, $this->logger), $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); $this->session->expects($this->once()) ->method('isLoggedIn') ->willReturn(true); @@ -228,7 +227,7 @@ public function testCORSShouldRelogin(string $method): void { ->with($this->equalTo('user'), $this->equalTo('pass')) ->willReturn(true); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, new MiddlewareUtils($this->reflector, $this->logger), $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); $middleware->beforeController($this->controller, $method); } @@ -259,7 +258,7 @@ public function testCORSShouldFailIfPasswordLoginIsForbidden(string $method): vo ->with($this->equalTo('user'), $this->equalTo('pass')) ->willThrowException(new PasswordLoginForbiddenException); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, new MiddlewareUtils($this->reflector, $this->logger), $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); $middleware->beforeController($this->controller, $method); } @@ -290,7 +289,7 @@ public function testCORSShouldNotAllowCookieAuth(string $method): void { ->with($this->equalTo('user'), $this->equalTo('pass')) ->willReturn(false); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, new MiddlewareUtils($this->reflector, $this->logger), $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); $middleware->beforeController($this->controller, $method); } @@ -305,7 +304,7 @@ public function testAfterExceptionWithSecurityExceptionNoStatus(): void { $this->createMock(IConfig::class) ); $this->reflector->reflect($this->controller, __FUNCTION__); - $middleware = new CORSMiddleware($request, new MiddlewareUtils($this->reflector, $this->logger), $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); $response = $middleware->afterException($this->controller, __FUNCTION__, new SecurityException('A security exception')); $expected = new JSONResponse(['message' => 'A security exception'], 500); @@ -322,7 +321,7 @@ public function testAfterExceptionWithSecurityExceptionWithStatus(): void { $this->createMock(IConfig::class) ); $this->reflector->reflect($this->controller, __FUNCTION__); - $middleware = new CORSMiddleware($request, new MiddlewareUtils($this->reflector, $this->logger), $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); $response = $middleware->afterException($this->controller, __FUNCTION__, new SecurityException('A security exception', 501)); $expected = new JSONResponse(['message' => 'A security exception'], 501); @@ -342,7 +341,7 @@ public function testAfterExceptionWithRegularException(): void { $this->createMock(IConfig::class) ); $this->reflector->reflect($this->controller, __FUNCTION__); - $middleware = new CORSMiddleware($request, new MiddlewareUtils($this->reflector, $this->logger), $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); $middleware->afterException($this->controller, __FUNCTION__, new \Exception('A regular exception')); } } diff --git a/tests/lib/AppFramework/Middleware/Security/SameSiteCookieMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/SameSiteCookieMiddlewareTest.php index f7ec3a94a6ed3..08691f6b4b27c 100644 --- a/tests/lib/AppFramework/Middleware/Security/SameSiteCookieMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/SameSiteCookieMiddlewareTest.php @@ -8,7 +8,6 @@ namespace Test\AppFramework\Middleware\Security; use OC\AppFramework\Http\Request; -use OC\AppFramework\Middleware\MiddlewareUtils; use OC\AppFramework\Middleware\Security\Exceptions\LaxSameSiteCookieFailedException; use OC\AppFramework\Middleware\Security\Exceptions\SecurityException; use OC\AppFramework\Middleware\Security\SameSiteCookieMiddleware; @@ -47,7 +46,7 @@ protected function setUp(): void { $this->request = $this->createMock(Request::class); $this->logger = $this->createMock(LoggerInterface::class); $this->reflector = $this->createMock(ControllerMethodReflector::class); - $this->middleware = new SameSiteCookieMiddleware($this->request, new MiddlewareUtils($this->reflector, $this->logger)); + $this->middleware = new SameSiteCookieMiddleware($this->request, $this->reflector); } #[\PHPUnit\Framework\Attributes\DoesNotPerformAssertions] @@ -120,7 +119,7 @@ public function testAfterExceptionLaxCookie(): void { ->willReturn('/myrequri'); $middleware = $this->getMockBuilder(SameSiteCookieMiddleware::class) - ->setConstructorArgs([$this->request, new MiddlewareUtils($this->reflector, $this->logger)]) + ->setConstructorArgs([$this->request, $this->reflector]) ->onlyMethods(['setSameSiteCookie']) ->getMock(); diff --git a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php index c34248411fb92..1f2a092de64da 100644 --- a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php @@ -10,7 +10,6 @@ use OC\AppFramework\Http; use OC\AppFramework\Http\Request; -use OC\AppFramework\Middleware\MiddlewareUtils; use OC\AppFramework\Middleware\Security\Exceptions\AppNotEnabledException; use OC\AppFramework\Middleware\Security\Exceptions\CrossSiteRequestForgeryException; use OC\AppFramework\Middleware\Security\Exceptions\ExAppRequiredException; @@ -51,7 +50,6 @@ class SecurityMiddlewareTest extends \Test\TestCase { private SecurityMiddlewareController $controller; private SecurityException $secAjaxException; private IRequest|MockObject $request; - private MiddlewareUtils $middlewareUtils; private LoggerInterface&MockObject $logger; private INavigationManager&MockObject $navigationManager; private IURLGenerator&MockObject $urlGenerator; @@ -79,7 +77,6 @@ protected function setUp(): void { $this->navigationManager = $this->createMock(INavigationManager::class); $this->urlGenerator = $this->createMock(IURLGenerator::class); $this->l10n = $this->createMock(IL10N::class); - $this->middlewareUtils = new MiddlewareUtils($this->reader, $this->logger); $this->middleware = $this->getMiddleware(true, true, false); $this->secAjaxException = new SecurityException('hey', true); } @@ -101,7 +98,7 @@ private function getMiddleware(bool $isLoggedIn, bool $isAdminUser, bool $isSubA return new SecurityMiddleware( $this->request, - $this->middlewareUtils, + $this->reader, $this->navigationManager, $this->urlGenerator, $this->logger,