Add VerifyPassword to API #1272
Conversation
It takes in an email and plain text password to verify. If it fails to find a password stored for email, it returns not_found. If it finds the password hash stored but that hash doesn't match the password passed via the API, it returns verified = false, else it returns verified = true.
| NotFound: false, | ||
| } | ||
|
|
||
| if err := bcrypt.CompareHashAndPassword(password.Hash, []byte(req.Password)); err != nil { |
There was a problem hiding this comment.
What do you think, would it make sense to inspect the err return here? From the code, it'll return bcrypt.ErrMismatchedHashAndPassword if the passwords don't match; but there's other error returns that indicate different failures: here and here.
I'd propose something along the lines of
if err := bcrypt.CompareHashAndPassword(password.Hash, []byte(req.Password)); err != nil {
if err == bcrypt.ErrMismatchedHashAndPassword {
resp.Verified = false
return resp, nil
}
return nil, err
}There might be some further simplifications around those three returns, so, just illustrating what I had in mind.
| } | ||
|
|
||
| resp := &api.VerifyPasswordResp{ | ||
| NotFound: false, |
There was a problem hiding this comment.
[nit] It's ok to be explicit, but this is equivalent to
resp := &api.VerifyPasswordResp{}❓ It might be easier to reason about what's happening downward from here if each return declared its own &api.VerifyPasswordResp{}?
| } | ||
|
|
||
| if req.Password == "" { | ||
| return nil, errors.New("no passwored to verify supplied") |
There was a problem hiding this comment.
[typo] s/passwored/password/
|
Sorry for the delay. Not returning password hashes was an intentional design and I really don't like the idea of making them exportable. What's the use case? |
|
Woops, looking closer at this and realized it's not returning the password. I really don't want dex to add more user management features. v1 went crazy with this and I think it's a good lesson. User management is a product in itself, dex is just a shim. |
Same as with #1271; this addition would let us check our (local) users' passwords without screenscraping a web login 😅 -- but I'd still maintain that #1020, i.e. reversing the connecting, having our service provide the user mgmt, and dex providing the web login, would be better. |
|
#1486 has updated this, and has just been merged. 🎉 |
It takes in an email and plain text password to verify. If it fails to find a password stored for email, it returns not_found. If it finds the password hash stored but that hash doesn't match the password passed via the API, it returns verified = false, else it returns verified = true.
Will have some conflicts with #1271 so would prefer to get that one merged first.
Happy to chat about whether or not people think the Dex password API should be doing this but it gives a nice, programmatic way to build additional functionality on top of the API (like having a user verify their password before updating it) that would otherwise be challenging.