Add VerifyPassword to API #1486
Conversation
|
I have corrected the PR from tylerclocke from previous comments and feedbacks from the team. |
|
I'd be curious about your use case -- how do you intend to make use of this? 😃 |
|
We are using it to check our user’s password (from a back-end service) before calling updatePassword. |
| @@ -254,6 +254,36 @@ func (d dexAPI) ListPasswords(ctx context.Context, req *api.ListPasswordReq) (*a | |||
|
|
|||
| } | |||
|
|
|||
| func (d dexAPI) VerifyPassword(ctx context.Context, req *api.VerifyPasswordReq) (*api.VerifyPasswordResp, error) { | |||
| if req.Email == "" { | |||
| return nil, errors.New("no email supplied") | |||
There was a problem hiding this comment.
This being GRPC, can we return status.Error(codes.InvalidArguments, "no email supplied") or something like that? errors.New(), or, any error would translate to codes.Internal or codes.Unknown, can't remember.
There was a problem hiding this comment.
I can change the error returned, but that will not be consistent with the rest of errors returned by the other functions. Do you want me to do it anyway ? and you will have some other tasks where you will change the other functions ?
There was a problem hiding this comment.
I'll deal with the others. 😃 You know what, you're correct, this is fine, let's keep them as-is and we can update all the errors in one go later.
| return nil, fmt.Errorf("verify password: %v", err) | ||
| } | ||
|
|
||
| if err := bcrypt.CompareHashAndPassword(password.Hash, []byte(req.Password)); err != nil { |
There was a problem hiding this comment.
Is the err interesting? If so, we might log it. If not, we could do
if bcrypt.CompareHashAndPassword(password.Hash, []byte(req.Password)) != nil {instead. Or even
verified := bcrypt.CompareHashAndPassword(password.Hash, []byte(req.Password)) == nil
return &api.VerifyPasswordResp{Verified: verified}, nilThere was a problem hiding this comment.
I will add some log
|
I have updated the branch in regards to some comments - adding some logs. |
|
Great ! Thanks |
|
@AlbanSeurat One small thing that kept me from merging this: you haven't added your name as author anywhere. I suppose it would make sense to amend this PR's commit and add so that github will pick it up as your contribution, too, properly. 😃 |
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. Co-authored-by: Alban Seurat <[email protected]>
Done |
…d-api Add VerifyPassword to API
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.
Re-open with fix to make it possible to merge