search

silverstripe / silverstripe-mfa

MultiFactor Authentication for Silverstripe CMS
BSD 3-Clause "New" or "Revised" License
11 stars 25 forks source link

FIX Handle 429 rate limiting status code #401

Closed emteknetnz closed 4 years ago

emteknetnz commented 4 years ago

Fix for https://github.com/silverstripe/silverstripe-mfa/issues/378

I'm not too keen on the AC in the issue "All error responses are handled correctly (including 429s)", as this implies providing a generic message for a range of status codes from 400-499 (or 400-500). However there's already at least some logic that uses a 400, so I'd prefer to just solve this only for 429's.

If there's some other error codes we know about and what to handle in a similar fashion, pretty easy to update the switch statement

I've handled 500's on totp (which is what the issue is actually about) in this PR

Easy way to test for 429 is to temporarily add this to the top of SilverStripe\Control\RequestHandler::handleRequest() when you're at the screen where you enter 6 digits

$response = new HTTPResponse();
$response->setStatusCode(429);
return $response;
emteknetnz commented 4 years ago

@ScopeyNZ as suggested, I have updated the PR to treat 5** as a catch-all for unknown errors

ScopeyNZ commented 4 years ago

I think a >= 500 would've been slightly more elegant, but I'm not going to ask you to change it.