feat(security): Add PHP \Attribute for remaining security annotations #37905
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,11 +26,13 @@ | |
| use OC\AppFramework\Middleware\Security\Exceptions\NotConfirmedException; | ||
| use OC\AppFramework\Utility\ControllerMethodReflector; | ||
| use OCP\AppFramework\Controller; | ||
| use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired; | ||
| use OCP\AppFramework\Middleware; | ||
| use OCP\AppFramework\Utility\ITimeFactory; | ||
| use OCP\ISession; | ||
| use OCP\IUserSession; | ||
| use OCP\User\Backend\IPasswordConfirmationBackend; | ||
| use ReflectionMethod; | ||
|
|
||
| class PasswordConfirmationMiddleware extends Middleware { | ||
| /** @var ControllerMethodReflector */ | ||
|
|
@@ -68,7 +70,9 @@ public function __construct(ControllerMethodReflector $reflector, | |
| * @throws NotConfirmedException | ||
| */ | ||
| public function beforeController($controller, $methodName) { | ||
| $reflectionMethod = new ReflectionMethod($controller, $methodName); | ||
|
|
||
| if ($this->hasAnnotationOrAttribute($reflectionMethod, 'PasswordConfirmationRequired', PasswordConfirmationRequired::class)) { | ||
| $user = $this->userSession->getUser(); | ||
| $backendClassName = ''; | ||
| if ($user !== null) { | ||
|
|
@@ -89,4 +93,24 @@ public function beforeController($controller, $methodName) { | |
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * @template T | ||
| * | ||
| * @param ReflectionMethod $reflectionMethod | ||
| * @param string $annotationName | ||
| * @param class-string<T> $attributeClass | ||
| * @return boolean | ||
| */ | ||
| protected function hasAnnotationOrAttribute(ReflectionMethod $reflectionMethod, string $annotationName, string $attributeClass): bool { | ||
| if (!empty($reflectionMethod->getAttributes($attributeClass))) { | ||
| return true; | ||
| } | ||
|
|
||
| if ($this->reflector->hasAnnotation($annotationName)) { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,7 +58,7 @@ public function afterException($controller, $methodName, \Exception $exception) | |
|
|
||
| return new RedirectResponse($this->urlGenerator->linkToRouteAbsolute( | ||
| 'core.login.showLoginForm', | ||
| ['clear' => true] // this param the code in login.js may be removed when the "Clear-Site-Data" is working in the browsers | ||
| )); | ||
| } | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,6 +48,12 @@ | |
| use OCP\App\AppPathNotFoundException; | ||
| use OCP\App\IAppManager; | ||
| use OCP\AppFramework\Controller; | ||
| use OCP\AppFramework\Http\Attribute\AuthorizedAdminSetting; | ||
| use OCP\AppFramework\Http\Attribute\NoAdminRequired; | ||
| use OCP\AppFramework\Http\Attribute\NoCSRFRequired; | ||
| use OCP\AppFramework\Http\Attribute\PublicPage; | ||
| use OCP\AppFramework\Http\Attribute\StrictCookiesRequired; | ||
| use OCP\AppFramework\Http\Attribute\SubAdminRequired; | ||
| use OCP\AppFramework\Http\JSONResponse; | ||
| use OCP\AppFramework\Http\RedirectResponse; | ||
| use OCP\AppFramework\Http\Response; | ||
|
|
@@ -61,6 +67,7 @@ | |
| use OCP\IUserSession; | ||
| use OCP\Util; | ||
| use Psr\Log\LoggerInterface; | ||
| use ReflectionMethod; | ||
|
|
||
| /** | ||
| * Used to do all the authentication and checking stuff for a controller method | ||
|
|
@@ -145,22 +152,24 @@ public function beforeController($controller, $methodName) { | |
| $this->navigationManager->setActiveEntry('spreed'); | ||
| } | ||
|
|
||
| $reflectionMethod = new ReflectionMethod($controller, $methodName); | ||
|
|
||
| // security checks | ||
| $isPublicPage = $this->hasAnnotationOrAttribute($reflectionMethod, 'PublicPage', PublicPage::class); | ||
| if (!$isPublicPage) { | ||
| if (!$this->isLoggedIn) { | ||
| throw new NotLoggedInException(); | ||
| } | ||
| $authorized = false; | ||
| if ($this->hasAnnotationOrAttribute($reflectionMethod, 'AuthorizedAdminSetting', AuthorizedAdminSetting::class)) { | ||
| $authorized = $this->isAdminUser; | ||
|
|
||
| if (!$authorized && $this->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class)) { | ||
| $authorized = $this->isSubAdmin; | ||
| } | ||
|
|
||
| if (!$authorized) { | ||
| $settingClasses = $this->getAuthorizedAdminSettingClasses($reflectionMethod); | ||
| $authorizedClasses = $this->groupAuthorizationMapper->findAllClassesForUser($this->userSession->getUser()); | ||
| foreach ($settingClasses as $settingClass) { | ||
| $authorized = in_array($settingClass, $authorizedClasses, true); | ||
|
|
@@ -174,29 +183,30 @@ public function beforeController($controller, $methodName) { | |
| throw new NotAdminException($this->l10n->t('Logged in user must be an admin, a sub admin or gotten special right to access this setting')); | ||
| } | ||
| } | ||
| if ($this->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class) | ||
| && !$this->isSubAdmin | ||
| && !$this->isAdminUser | ||
| && !$authorized) { | ||
| throw new NotAdminException($this->l10n->t('Logged in user must be an admin or sub admin')); | ||
| } | ||
| if (!$this->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class) | ||
| && !$this->hasAnnotationOrAttribute($reflectionMethod, 'NoAdminRequired', NoAdminRequired::class) | ||
| && !$this->isAdminUser | ||
| && !$authorized) { | ||
| throw new NotAdminException($this->l10n->t('Logged in user must be an admin')); | ||
| } | ||
| } | ||
|
|
||
| // Check for strict cookie requirement | ||
| if ($this->hasAnnotationOrAttribute($reflectionMethod, 'StrictCookieRequired', StrictCookiesRequired::class) || | ||
| !$this->hasAnnotationOrAttribute($reflectionMethod, 'NoCSRFRequired', NoCSRFRequired::class)) { | ||
| if (!$this->request->passesStrictCookieCheck()) { | ||
| throw new StrictCookieMissingException(); | ||
| } | ||
| } | ||
| // CSRF check - also registers the CSRF token since the session may be closed later | ||
| Util::callRegister(); | ||
| if (!$this->hasAnnotationOrAttribute($reflectionMethod, 'NoCSRFRequired', NoCSRFRequired::class)) { | ||
| /* | ||
| * 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 | ||
|
|
@@ -232,6 +242,48 @@ public function beforeController($controller, $methodName) { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * @template T | ||
| * | ||
| * @param ReflectionMethod $reflectionMethod | ||
| * @param string $annotationName | ||
| * @param class-string<T> $attributeClass | ||
| * @return boolean | ||
| */ | ||
| protected function hasAnnotationOrAttribute(ReflectionMethod $reflectionMethod, string $annotationName, string $attributeClass): bool { | ||
| if (!empty($reflectionMethod->getAttributes($attributeClass))) { | ||
| return true; | ||
| } | ||
|
|
||
| if ($this->reflector->hasAnnotation($annotationName)) { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * @param ReflectionMethod $reflectionMethod | ||
| * @return string[] | ||
| */ | ||
| protected 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; | ||
| } | ||
|
|
||
| /** | ||
| * If an SecurityException is being caught, ajax requests return a JSON error | ||
| * response and non ajax requests redirect to the index | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,8 @@ | |
| */ | ||
| namespace OCP\AppFramework; | ||
|
|
||
| use OCP\AppFramework\Http\Attribute\NoCSRFRequired; | ||
| use OCP\AppFramework\Http\Attribute\PublicPage; | ||
| use OCP\AppFramework\Http\Response; | ||
| use OCP\IRequest; | ||
|
|
||
|
|
@@ -70,6 +72,8 @@ public function __construct($appName, | |
| * @PublicPage | ||
| * @since 7.0.0 | ||
| */ | ||
| #[NoCSRFRequired] | ||
| #[PublicPage] | ||
| public function preflightedCors() { | ||
| if (isset($this->request->server['HTTP_ORIGIN'])) { | ||
| $origin = $this->request->server['HTTP_ORIGIN']; | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,10 @@ | |
| */ | ||
| namespace OCP\AppFramework; | ||
|
|
||
| use OCP\AppFramework\Http\Attribute\BruteForceProtection; | ||
| use OCP\AppFramework\Http\Attribute\NoCSRFRequired; | ||
| use OCP\AppFramework\Http\Attribute\PublicPage; | ||
| use OCP\AppFramework\Http\Attribute\UseSession; | ||
| use OCP\AppFramework\Http\RedirectResponse; | ||
| use OCP\AppFramework\Http\TemplateResponse; | ||
| use OCP\IRequest; | ||
|
|
@@ -70,6 +74,8 @@ public function __construct(string $appName, | |
| * | ||
| * @since 14.0.0 | ||
| */ | ||
| #[NoCSRFRequired] | ||
| #[PublicPage] | ||
| public function showAuthenticate(): TemplateResponse { | ||
| return new TemplateResponse('core', 'publicshareauth', [], 'guest'); | ||
| } | ||
|
|
@@ -129,7 +135,7 @@ protected function authFailed() { | |
| } | ||
|
|
||
| /** | ||
| * Function called after successful authentication | ||
| * | ||
| * You can use this to do some logging for example | ||
| * | ||
|
|
@@ -147,6 +153,9 @@ protected function authSucceeded() { | |
| * | ||
| * @since 14.0.0 | ||
| */ | ||
| #[BruteForceProtection(action: 'publicLinkAuth')] | ||
| #[PublicPage] | ||
| #[UseSession] | ||
| final public function authenticate(string $password = '', string $passwordRequest = 'no', string $identityToken = '') { | ||
| // Already authenticated | ||
| if ($this->isAuthenticated()) { | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method occur 3 times at this PR with the same signature.
Maybe will be good to move to parent class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ia only temporary until we drop the annotations and its 2 if statements. I don't really feel like creating an abstract parent class for 4 operational lines