feat(middleware): Migrate BruteForceProtection annotation to PHP Attribute and allow multiple by nickvergessen · Pull Request #36928 · nextcloud/server

nickvergessen · 2023-02-28T21:53:05Z

Previously (and still supported)

	/**
	 * @BruteForceProtection(action=login)
	 */
	public function testMethodWithAnnotation() {

New syntax

	#[BruteForceProtection(action: 'login')]
	public function testMethodWithAttribute() {

Multiple attributes now supported

By specifying an 'action' in the throttle($metadata) only the dedicated attribute will trigger it's BFP, if none is specified all will throttle.
This is useful e.g. in controllers that need to protect different data, e.g. Talk has the tokens and the signaling secret in the same method and needs to do a lot of manual work instead of just calling throttle on the response:
https://github.com/nextcloud/spreed/blob/29a442cf86d49b426ecd47776c86ab6f6de5da58/lib/Controller/SignalingController.php#L154-L179

	#[BruteForceProtection(action: 'token')]
	#[BruteForceProtection(action: 'password')]
	public function testMethodWithMultipleAttributes() {

🚧 ToDo

Allow as attribute
Add multi support
Log debug when the request was throttled, but there was no Annotation nor matching Attribute
Add a header to the response when the request is throttled(?)

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(bruteforce): Document the new BruteForceProtection attribute documentation#9742
Backports requested where applicable (ex: critical bugfixes)

…ibute and allow multiple Signed-off-by: Joas Schilling <coding@schilljs.com>

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

nickvergessen · 2023-03-08T11:10:05Z

Documentation update at nextcloud/documentation#9742

come-nc · 2023-03-09T15:43:16Z

 		if ($this->reflector->hasAnnotation('BruteForceProtection')) {
 			$action = $this->reflector->getAnnotationParameter('BruteForceProtection', 'action');
 			$this->throttler->sleepDelayOrThrowOnMax($this->request->getRemoteAddress(), $action);
+		} else {


Would it make sense to add hasAttribute / getAttributes to ControllerMethodReflector ?

THought about that too. I'm still up for it as a follow up, to combine the logic our for UseSession and this one.
Just thought it's easier to first get this in and then do more rework.

I’m always afraid the follow-up never comes in these cases :-)

We should try to make the new way as easy to use as the previous one if we want the code to get migrated at some point.

I’m always afraid the follow-up never comes in these cases

I guess you have a point, at the same time there is the RateLimit annotation waiting for me as a task this and next month, so will come.

vitormattos · 2023-03-16T18:22:47Z

+		if ($response->isThrottled()) {
+			if ($this->reflector->hasAnnotation('BruteForceProtection')) {
+				$action = $this->reflector->getAnnotationParameter('BruteForceProtection', 'action');
+				$ip = $this->request->getRemoteAddress();
+				$this->throttler->sleepDelay($ip, $action);
+				$this->throttler->registerAttempt($action, $ip, $response->getThrottleMetadata());
+			} else {
+				$reflectionMethod = new ReflectionMethod($controller, $methodName);
+				$attributes = $reflectionMethod->getAttributes(BruteForceProtection::class);
+
+				if (!empty($attributes)) {
+					$ip = $this->request->getRemoteAddress();
+					$metaData = $response->getThrottleMetadata();
+
+					foreach ($attributes as $attribute) {
+						/** @var BruteForceProtection $protection */
+						$protection = $attribute->newInstance();
+						$action = $protection->getAction();
+
+						if (!isset($metaData['action']) || $metaData['action'] === $action) {
+							$this->throttler->sleepDelay($ip, $action);
+							$this->throttler->registerAttempt($action, $ip, $metaData);
+						}
+					}
+				} else {
+					$this->logger->debug('Response for ' . get_class($controller) . '::' . $methodName . ' got bruteforce throttled but has no annotation nor attribute defined.');
+				}
+			}


Is a good piece of code to refactor and split the responsibility at separated method to make the reading more easy.

Maybe anything like:

->parseBruteForceAnnotation ->parseBruteForceAttributes

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

nickvergessen added this to the Nextcloud 27 milestone Feb 28, 2023

nickvergessen requested a review from ChristophWurst February 28, 2023 21:53

nickvergessen self-assigned this Feb 28, 2023

nickvergessen added the pending documentation This pull request needs an associated documentation update label Feb 28, 2023

nickvergessen added 2 commits March 8, 2023 12:09

feat(middleware): Migrate BruteForceProtection annotation to PHP Attr…

e839eb9

…ibute and allow multiple Signed-off-by: Joas Schilling <coding@schilljs.com>

Add a debug message when throttling without defining

2b49861

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

nickvergessen force-pushed the techdebt/noid/bruteforce-protection-attribute branch from c26dc84 to 2b49861 Compare March 8, 2023 11:09

nickvergessen requested review from come-nc and danxuliu March 8, 2023 11:09

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

nickvergessen marked this pull request as ready for review March 8, 2023 11:09

nickvergessen added enhancement security labels Mar 8, 2023

come-nc approved these changes Mar 9, 2023

View reviewed changes

nickvergessen requested a review from vitormattos March 16, 2023 17:45

vitormattos reviewed Mar 16, 2023

View reviewed changes

vitormattos approved these changes Mar 16, 2023

View reviewed changes

nickvergessen merged commit bfc37af into master Mar 16, 2023

nickvergessen deleted the techdebt/noid/bruteforce-protection-attribute branch March 16, 2023 18:33

nickvergessen mentioned this pull request Mar 16, 2023

Add a getAttributes to IControllerMethodReflector #37279

Closed

4 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

feat(middleware): Migrate BruteForceProtection annotation to PHP Attribute and allow multiple#36928

feat(middleware): Migrate BruteForceProtection annotation to PHP Attribute and allow multiple#36928
nickvergessen merged 2 commits into
masterfrom
techdebt/noid/bruteforce-protection-attribute

nickvergessen commented Feb 28, 2023 •

edited

Loading

Uh oh!

nickvergessen commented Mar 8, 2023

Uh oh!

come-nc Mar 9, 2023

Uh oh!

nickvergessen Mar 10, 2023

Uh oh!

come-nc Mar 13, 2023

Uh oh!

nickvergessen Mar 13, 2023

Uh oh!

vitormattos Mar 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Conversation

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

Uh oh!

Previously (and still supported)

New syntax

Multiple attributes now supported

🚧 ToDo

Checklist

Uh oh!

nickvergessen commented Mar 8, 2023

Uh oh!

come-nc Mar 9, 2023

Choose a reason for hiding this comment

Uh oh!

nickvergessen Mar 10, 2023

Choose a reason for hiding this comment

Uh oh!

come-nc Mar 13, 2023

Choose a reason for hiding this comment

Uh oh!

nickvergessen Mar 13, 2023

Choose a reason for hiding this comment

Uh oh!

vitormattos Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nickvergessen commented Feb 28, 2023 •

edited

Loading