Add brute force protection on all methods wrapped by PublicShareMiddleware

[julien-nc]

This adds a rate limit on all methods wrapped by PublicShareMiddleware if an invalid token is provided.

As the token check does not happen in the controller methods but in the middleware, we can't use the @AnonRateThrottle annotation on those methods.

Remaining questions:

• Does it make sense to use the brute force protection mechanism instead? If so, same issue, we can't use the annotation in the controller.
• Do the limit (10) and period (120) values make sense?
• Does it make sense to call registerAnonRequest only if the token check fails like it's done here?
  • It could be called in other cases
  • Where should it be called to apply the limit when the token is correct but the password is wrong?

[nickvergessen]

This adds a rate limit on all methods wrapped by PublicShareMiddleware if an invalid token is provided.

Sounds like you want to do brute force protection, not rate limiting.

• Rate limiting is for correct usage, but you want to prevent abuse, e.g. we could rate limit guests so they can only post 10 chat messages per 60 seconds.
• Brute force protection is to prevent guessing access, e.g. log every attempt of accessing a room that does not exist and then return without trying after X failed attempts

But seems like I should extend the docs with this a bit going forward.

[nickvergessen]

If so, same issue, we can't use the annotation in the controller.

Let's have a look together tomorrow

[julien-nc]

@nickvergessen Thanks for the explanation. Better like this?

It still only protects against incorrect token attempts but not against incorrect share password.

I'll squash the commits later if needed.

[julien-nc]

This now also protects against invalid password attempts.

[julien-nc]

Rebased and squashed.
@nickvergessen What do you think?

[nickvergessen]

Can we read if the controller itself has the annotation and then reuse that action or better not do anything, so it's not double slowed?

[julien-nc]

Technically we can, just like the BruteForceMiddleware does.
But as you said, what if the ShareController is handled by the BruteForceMiddleware in the future? sleepDelayOrThrowOnMax would be called twice.

Also it would probably bring some confusion. Why would the PublicShareMiddleware care about annotations that are bruteforce-related?
I think it's fine the way we do it. All methods having the token/password check are bruteforce protected.

Only weird aspect of this is when reading the controllers...nothing mentions bruteforce protection in OCA\Files_Sharing\Controller\PublicShareController and OCA\Files_Sharing\Controller\ShareController.
Should we add explicit comments pointing to the PublicShareMiddleware in those controllers? And/or in the AuthPublicShareController and PublicShareController?

[julien-nc]

@nickvergessen I kinda answered your question 😁. IMO it's fine and clearer like it is now.

[nickvergessen]

Good 👍🏽

But the one question is still up

[blizzz]

ℹ️ master is 26

[julien-nc]

/backport to stable25

[julien-nc]

/backport to stable24

[julien-nc]

/backport to stable23

[backportbot-nextcloud]

The backport to stable25 failed. Please do this backport manually.

[backportbot-nextcloud]

The backport to stable24 failed. Please do this backport manually.

[backportbot-nextcloud]

The backport to stable23 failed. Please do this backport manually.