Closed impowski closed 7 years ago
You are right that there is no test for invalid login credentials. It's up for grabs if you are interested. But the User::find_by_uid
function does return NotFound
, here. The error will bubble up in verify
. Check out the ?
operator.
The register will fail because of the primary key restriction but you are again right that it's not explicit. You either have to match a uniqueness violation error from diesel or check for existence before. The tests are missing also.
Thanks for noticing :+1:
@mujx Yeah I know about ?
but it still return unauthorized
error instead of not_found
, I don't know why is that. With registration you say that there is no way to register because of UserId
I have a little bit different database scheme so I might check mine code too.
I don't think I understood correctly. What function will return unauthorized instead of not_found?
@mujx When we trying to Login
with non-existent user it should give not_found
.
It returns Forbidden
, because technically you submit invalid credentials. NotFound
could also work. It depends on whether or not you want to give info about the user's existence.
Although Forbidden
answers better the action you are trying to perform (gain access). You are not explicitly asking for the user's info. Of course you can customize the error message to include that info.
I agree with @mujx on this : in most cases, from a security point of view, it's better to not leak the information of whether an account exists or not, so return Forbidden
in all cases of unsuccessful login attempt.
@vberger I was trippin' cos of this error message inside of src/error.rs
not_found
error since we do a check inside ofapi/r0/profile.rs
, but there is no such thing forUser
because when we are trying to verify ourUser
withverify
inside ofmodel/user.rs
and we call another functionfind_by_uid
inside of it which doesn't returnDieselError::NotFound
and there is no test for invalid credentials inside of login too. And my question is does it even returnDieselError
because seems like no?In case this will be true I could implement those things.
P.S. Maybe I'm wrong in some cases because I use Ruma as an example to write my own thing.