Interactive Auth: Return 401 from for incorrect password #1160
Conversation
This requires a bit of fettling, because I want to return a helpful error message too but we don't want to distinguish between unknown user and invalid password. To avoid hardcoding the error message into 15 places in the code, I've had to refactor a few methods to return None instead of throwing. Fixes https://matrix.org/jira/browse/SYN-744
| raise LoginError( | ||
| 403, "Invalid password", | ||
| errcode=Codes.FORBIDDEN | ||
| ) |
There was a problem hiding this comment.
Much as I don't want to go further down this refactoring rabbit hole, it would be nice to move this up one more layer so this function returned None or raise a sane error, and contain the 403 code to validate_password_login which is used for login and login only. wdyt?
There was a problem hiding this comment.
I did try that. The thing is that, whatever error code _check_password_auth puts in its exception, check_auth is going to throw it away and turn it into a 401. So I'm not sure that's any clearer.
I started wondering what the point of LoginError actually is, and why it's different to SynapseError, and why it lets you set the http code rather than leaving that to a higher layer, and then I decided to stop thinking about it.
There was a problem hiding this comment.
Yeah, the fact that check_auth will clobber it with a 401 anyway is why I thought it might be clearer to use an exception without a specific code, so it's then catching an specific exception and turning it into one the upper layer understands, rather than catching the 403 exception and replacing the code. If you see it throwing the 403 error, it's surprising when that's re-caught, but less surprising if it's, say, AuthCheckException. I don't really hugely bothered though, either way this is an improvement over what was there before.
There was a problem hiding this comment.
OK, I see the problem: the different auth checkers all throw LoginError, so you'd have to change all of them. Yeah, ok - it's not worth it.
|
@matrixbot: retest this please |
This requires a bit of fettling, because I want to return a helpful error
message too but we don't want to distinguish between unknown user and invalid
password. To avoid hardcoding the error message into 15 places in the code,
I've had to refactor a few methods to return None instead of throwing.
Fixes https://matrix.org/jira/browse/SYN-744