feat(security): Add PHP \Attribute for remaining security annotations by nickvergessen · Pull Request #37905 · nextcloud/server

nickvergessen · 2023-04-24T15:16:54Z

Summary

Allowing the remaining annotations of the security middlewares as Attributes

Sibling to feat(ratelimit): Add Attributes support to rate limit middleware #37864
And feat(middleware): Migrate BruteForceProtection annotation to PHP Attribute and allow multiple #36928

TODO

Add Attribute classes
Check for attribute OR annotation
Add Attribute additionally to OCP controllers
Add unit tests
Document and deprecate the old syntax feat(developer): Replace annotations with attributes documentation#10206

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included
Screenshots before/after for front-end changes
Documentation (manuals or wiki) has been updated or is not required feat(developer): Replace annotations with attributes documentation#10206
Backports requested where applicable (ex: critical bugfixes)

github-advanced-security

Psalm found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

Signed-off-by: Joas Schilling <coding@schilljs.com>

nickvergessen · 2023-04-25T13:17:31Z

Comment for #37039 on merge:

Added:

> # Backend
> 
> * `AuthorizedAdminSetting` controller access restriction can now be enabled using native PHP attributes https://github.com/nextcloud/server/pull/37905  https://docs.nextcloud.com//server/stable/developer_manual/basics/setting.html
> * `CORS` controller restriction can now be enabled using native PHP attributes https://github.com/nextcloud/server/pull/37905  https://docs.nextcloud.com//server/stable/developer_manual/digging_deeper/rest_apis.html
> * `NoAdminRequired` controller access restriction can now be enabled using native PHP attributes https://github.com/nextcloud/server/pull/37905  https://docs.nextcloud.com//server/stable/developer_manual/basics/controllers.html#authentication
> * `NoCSRFRequired` controller functionality can now be enabled using native PHP attributes https://github.com/nextcloud/server/pull/37905  https://docs.nextcloud.com//server/stable/developer_manual/prologue/security.html#cross-site-request-forgery
> * `PasswordConfirmationRequired` controller restriction can now be enabled using native PHP attributes https://github.com/nextcloud/server/pull/37905  https://docs.nextcloud.com//server/stable/developer_manual/digging_deeper/javascript-apis.html#nextcloud-password-confirmation
> * `PublicPage` controller access restriction can now be enabled using native PHP attributes https://github.com/nextcloud/server/pull/37905  https://docs.nextcloud.com//server/stable/developer_manual/basics/controllers.html#authentication
> * `SubAdminRequired` controller access restriction can now be enabled using native PHP attributes https://github.com/nextcloud/server/pull/37905  https://docs.nextcloud.com//server/stable/developer_manual/basics/controllers.html#authentication
> 
> ## Deprecations
> 
> * `@AuthorizedAdminSetting` annotation for controllers is now deprecated in favor of the `#[AuthorizedAdminSetting]` attribute https://docs.nextcloud.com//server/stable/developer_manual/basics/setting.html
> * `@CORS` annotation for controllers is now deprecated in favor of the `#[CORS]` attribute https://docs.nextcloud.com//server/stable/developer_manual/digging_deeper/rest_apis.html
> * `@NoAdminRequired` annotation for controllers is now deprecated in favor of the `#[NoAdminRequired]` attribute https://docs.nextcloud.com//server/stable/developer_manual/basics/controllers.html#authentication
> * `@NoCSRFRequired` annotation for controllers is now deprecated in favor of the `#[NoCSRFRequired]` attribute https://docs.nextcloud.com//server/stable/developer_manual/prologue/security.html#cross-site-request-forgery
> * `@PasswordConfirmationRequired` annotation for controllers is now deprecated in favor of the `#[PasswordConfirmationRequired]` attribute https://docs.nextcloud.com//server/stable/developer_manual/digging_deeper/javascript-apis.html#nextcloud-password-confirmation
> * `@PublicPage` annotation for controllers is now deprecated in favor of the `#[PublicPage]` attribute https://docs.nextcloud.com//server/stable/developer_manual/basics/controllers.html#authentication
> * `@SubAdminRequired` annotation for controllers is now deprecated in favor of the `#[SubAdminRequired]` attribute https://docs.nextcloud.com//server/stable/developer_manual/basics/controllers.html#authentication

vitormattos

Only suggestions, not blocking points.

vitormattos · 2023-04-25T14:12:05Z

+		if ($this->reflector->hasAnnotation($annotationName)) {
+			return true;
+		}
+


Suggested change

vitormattos · 2023-04-25T14:20:14Z

@@ -114,23 +141,25 @@ public function beforeController($controller, $methodName) {
 	 * @throws SecurityException
 	 */
 	public function afterController($controller, $methodName, Response $response) {


This will impact a lot of classes but would be good create a followup issue to do this fix return type and others from Middleware abstract class:

Suggested change

public function afterController($controller, $methodName, Response $response) {

public function afterController($controller, $methodName, Response $response): void {

vitormattos · 2023-04-25T14:32:48Z

+	 * @param class-string<T> $attributeClass
+	 * @return boolean
+	 */
+	protected function hasAnnotationOrAttribute(ReflectionMethod $reflectionMethod, string $annotationName, string $attributeClass): bool {


This method occur 3 times at this PR with the same signature.
Maybe will be good to move to parent class.

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

vitormattos · 2023-04-25T14:32:51Z

+	 * @return boolean
+	 */
+	protected function hasAnnotationOrAttribute(ReflectionMethod $reflectionMethod, string $annotationName, string $attributeClass): bool {
+		if ($this->reflector->hasAnnotation($annotationName)) {


At other implements of this method on this PR you used the follow code. Make any difference or only was about wrote more times the same method without doing a copy paste?

Suggested change

if ($this->reflector->hasAnnotation($annotationName)) {

if (!empty($reflectionMethod->getAttributes($attributeClass))) {

For AuthorizedAdminSetting we need the values, so i changed it there but then went for an independent approach, so that's why after that attempt some might diverge, but as mentioned above they are just 2 ifs so it doesn't really matter

nickvergessen added 2. developing Work in progress technical debt 🧱 🤔🚀 labels Apr 24, 2023

nickvergessen added this to the Nextcloud 27 milestone Apr 24, 2023

nickvergessen self-assigned this Apr 24, 2023

github-advanced-security AI found potential problems Apr 24, 2023

View reviewed changes

nickvergessen force-pushed the techdebt/noid/add-attributes-for-remaining-security-annotations branch from 4f53919 to a64daa3 Compare April 25, 2023 11:13

nickvergessen added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 25, 2023

nickvergessen marked this pull request as ready for review April 25, 2023 11:13

nickvergessen requested review from ChristophWurst, come-nc, susnux and vitormattos April 25, 2023 11:13

github-advanced-security AI found potential problems Apr 25, 2023

View reviewed changes

Comment thread lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php Fixed

ChristophWurst approved these changes Apr 25, 2023

View reviewed changes

Comment thread lib/public/AppFramework/Http/Attribute/AuthorizedAdminSetting.php

nickvergessen force-pushed the techdebt/noid/add-attributes-for-remaining-security-annotations branch from a64daa3 to 203f182 Compare April 25, 2023 11:53

nickvergessen added the pending documentation This pull request needs an associated documentation update label Apr 25, 2023

nickvergessen mentioned this pull request Apr 25, 2023

feat(developer): Replace annotations with attributes nextcloud/documentation#10206

Merged

feat(security): Add PHP \Attribute for remaining security annotations

ecb8b55

Signed-off-by: Joas Schilling <coding@schilljs.com>

nickvergessen force-pushed the techdebt/noid/add-attributes-for-remaining-security-annotations branch from 203f182 to ecb8b55 Compare April 25, 2023 12:50

nickvergessen mentioned this pull request Apr 25, 2023

feat(controllers): Migrate server annotations to attributes nextcloud/spreed#9377

Merged

4 tasks

vitormattos approved these changes Apr 25, 2023

View reviewed changes

nickvergessen merged commit 75f17b6 into master Apr 25, 2023

nickvergessen deleted the techdebt/noid/add-attributes-for-remaining-security-annotations branch April 25, 2023 15:13

nickvergessen removed the pending documentation This pull request needs an associated documentation update label May 30, 2023

Navigation Menu

Uh oh!

Uh oh!

feat(security): Add PHP \Attribute for remaining security annotations - #37905

feat(security): Add PHP \Attribute for remaining security annotations#37905
nickvergessen merged 1 commit into
masterfrom
techdebt/noid/add-attributes-for-remaining-security-annotations

nickvergessen commented Apr 24, 2023 •

edited

Loading

Uh oh!

github-advanced-security AI left a comment

Uh oh!

Uh oh!

Uh oh!

nickvergessen commented Apr 25, 2023

Uh oh!

vitormattos left a comment

Uh oh!

vitormattos Apr 25, 2023

Uh oh!

vitormattos Apr 25, 2023

Uh oh!

vitormattos Apr 25, 2023

Uh oh!

nickvergessen Apr 25, 2023

Uh oh!

vitormattos Apr 25, 2023

Uh oh!

nickvergessen Apr 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	public function afterController($controller, $methodName, Response $response) {
	public function afterController($controller, $methodName, Response $response): void {

	if ($this->reflector->hasAnnotation($annotationName)) {
	if (!empty($reflectionMethod->getAttributes($attributeClass))) {

Uh oh!

Uh oh!

Conversation

nickvergessen commented Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

TODO

Checklist

Uh oh!

github-advanced-security AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nickvergessen commented Apr 25, 2023

Uh oh!

vitormattos left a comment

Choose a reason for hiding this comment

Uh oh!

vitormattos Apr 25, 2023

Choose a reason for hiding this comment

Uh oh!

vitormattos Apr 25, 2023

Choose a reason for hiding this comment

Uh oh!

vitormattos Apr 25, 2023

Choose a reason for hiding this comment

Uh oh!

nickvergessen Apr 25, 2023

Choose a reason for hiding this comment

Uh oh!

vitormattos Apr 25, 2023

Choose a reason for hiding this comment

Uh oh!

nickvergessen Apr 25, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nickvergessen commented Apr 24, 2023 •

edited

Loading