Move floodgate to middleware, add extender + integration tests #2170
Conversation
askvortsov1
requested review from
franzliedke,
tankerkiller125,
luceos and
dsevillamartin
[ci skip] [skip ci]
[ci skip] [skip ci]
…n controllers" This reverts commit 02597ab.
…ated, don't remove the code, so that old style still works for one release.
… This will become unnecessary when we move to testing within transactions
- Floodgate is now called ThrottleApi for clarity - Each throttler is now responsible for checking route / method information on its own - Throttlers are now provided as simple callbacks.
askvortsov1
force-pushed
the
as/floodgate_to_middleware_plus_extender
branch
from
8d467e6
to
16657ec
Compare
[ci skip] [skip ci]
|
@luceos @datitisev I updated this to drastically simplify it: throttlers are now just regular old callbacks, and are responsible for deciding which routes they do / don't want to be called on themselves (this way, wildcard routes are supported in the rare instances where needed without us having to explicitly support it. |
|
@clarkwinkelmann wanted to ping you on this one as well given your thoughts on #2438: This one has all variables contained as properties (or public getters) of the request, but do we want to pass them in as individual arguments for additional safety (since, for instance, we might change how Actor is registered with the request object)? |
There was a problem hiding this comment.
Looking at the code, this looks good.
I don't think we should worry about splitting the actor and such into additional parameters at this time. We can always make the change later. If we make changes to request attributes, don't forget we also break user-created middlewares and controllers, so it would have a huge impact independently from what we do here.
Not for this PR - but do we want to add a default throttle to the API, with maybe a default rate for GET/HEAD and a different one for the other methods?
Should we make this feature also available to the forum frontend? It would be great for restricting login and password reset.
Hmm that's bordering on extension territory IMO
Login and Password Reset are both implemented by the Forum views using our simulated API client to send requests to backend views. This isn't the case for email confirmations, although IMO we should make it so. Then, we could have the ThrottleAPI middleware run on ApiClient requests (which shouldn't be that difficult, we just need to simulate a middleware pipe in the client), and voila, we have throttling! @clarkwinkelmann is this something you'd like to see included here before we merge, or would a separate PR be fine? I think I'd prefer the separate PR route, especially since aren't removing / impacting functionality, just adding new features. |
**Part of #2091, #1891 **
Depends on #2425
Changes proposed in this pull request:
Reviewers should focus on:
NOTE: The changes to MailTest are here to prevent this PR breaking those tests due to DB contamination. We really need to wipe the DB between every test class...
Note followup: if #2172 is merged before this, I'll remove those changes.
Confirmed
composer test).