Improve error reporting #204
Conversation
MKodde
force-pushed
the
feature/improve-error-reporting
branch
from
7be57f8 to
136a243
Compare
The Assert statements would yield unusable error messages without a path to know what to go and fix. This change at least tells us what config item is not right. And what is expected.
Some additional bootstrapping was required to allow for this (in services.yaml).
This error status was previously not supported. It is now. The uncaught errors are caught, and the invalid-request is sent back to the JS app. That in turn displays the user facing error page.
MKodde
force-pushed
the
feature/improve-error-reporting
branch
from
136a243 to
4160478
Compare
| if (!$this->authenticationService->authenticationRequired()) { | ||
| $logger->error('There is no pending authentication request from SP'); | ||
|
|
||
| return new Response('No authentication required', Response::HTTP_BAD_REQUEST); |
There was a problem hiding this comment.
I've noticed that this code already existed before so it might be out of scope, but it seems to me that the status code of 400 is not correct here.
The request itself seems correct, however it can't be processed on the server. Not sure which status code is best here (because I lack the domain knowledge of when this happens in the grand picture), but it looks to me that the request itself seems valid.
There was a problem hiding this comment.
The request in this instance is not valid, the authentication/qr route is called without a pending authentication request present. This indicates a state issue, or someone is manually trying to open the route without having the correct preconditions (there must be a running authentication). So I think a 400 response is not incorrect.
Forbidden (403) also seems fitting. I love these kind of discussions. This one I tend to leave as-is.
There was a problem hiding this comment.
If the reason is that the server is in an unexpected state, it sounds like Precondition Failed (412) is a great fit.
400 should really be used if the request itself is incorrect (malformed, for example).
But I'm fine with leaving it as-is :).
At first I opted to handle the 'invalid-request' manually. But having a default switch-case to handle all unhandled stati as an error makes more sense. And before this commit, the invalid request was handled as a Push Notification failure. But that was not my intention. I wanted to render the error page, and for that, we need to call the switchToStatusRequestError method instead.
MKodde
force-pushed
the
feature/improve-error-reporting
branch
from
f929e11 to
aba1285
Compare
See: https://www.pivotaltracker.com/story/show/188205289