-
Notifications
You must be signed in to change notification settings - Fork 98
Description
Opening a new issue since the described issue is broader than the one that was reported.
I think the behavior discussed in this thread is the cause of a more general problem that I have seen.
In our
validatefunction, we run a database query to see if the user still exists, as is recommended in the docs. Most of the time, it works fine. However, if there's a connection failure to the database, the database client throws an error, and errors invalidateseem to log the user out no matter what.I don't think it makes sense to log the user out just because the backend can't contact the database. Probably a 500 error should be returned, with the user still logged in so they can try again.
Originally posted by @sholladay in #245
The current validation handler assumes any error is caused by invalid credentials. Given that it is indeed common to need an external call to perform the validation, another class of validation errors are (temporary) internal validation errors. A third class (from #245) are policy errors, though these should generally be generated after validation. These errors should ideally be handled separately to avoid clearing, what could still be valid, credentials and return a code 500 or similar.
Not clearing the state on these kind of errors could probably be handled as a bug fix. Eg. by only clearing the cookie on a Boom.unauthorized() error.
Ideally, the original error would be returned as well, as suggested in #245. This could also be argued as a non-breaking change, especially if non-Boom errors are still treated as Boom.unauthorized().
It should be noted that system errors (like TypeError) already don't clear the cookie, and return a code 500 response (as demonstrated in my new tests from #252.
@sholladay FYI, your specific clearing issue can be mitigated by not using the clearInvalid option.