Skip to content

Add VerifyPassword to API #1272

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
194 changes: 142 additions & 52 deletions api/api.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 12 additions & 0 deletions api/api.proto
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,16 @@ message RevokeRefreshResp {
bool not_found = 1;
}

message VerifyPasswordReq {
string email = 1;
string password = 2;
}

message VerifyPasswordResp {
bool verified = 1;
bool not_found = 2;
}

// Dex represents the dex gRPC service.
service Dex {
// CreateClient creates a client.
Expand All @@ -156,4 +166,6 @@ service Dex {
//
// Note that each user-client pair can have only one refresh token at a time.
rpc RevokeRefresh(RevokeRefreshReq) returns (RevokeRefreshResp) {};
// VerifyPassword returns whether a password matches a hash for a specific email or not.
rpc VerifyPassword(VerifyPasswordReq) returns (VerifyPasswordResp) {};
}
3 changes: 2 additions & 1 deletion examples/grpc-client/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ Finally run the Dex client providing the CA certificate, client certificate and
Running the gRPC client will cause the following API calls to be made to the server
1. CreatePassword
2. ListPasswords
3. DeletePassword
3. VerifyPassword
4. DeletePassword

## Cleaning up

Expand Down
33 changes: 33 additions & 0 deletions examples/grpc-client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,39 @@ func createPassword(cli api.DexClient) error {
log.Printf("%+v", pass)
}

// Verifying correct and incorrect passwords
log.Print("Verifying Password:\n")
verifyReq := &api.VerifyPasswordReq{
Email: "[email protected]",
Password: "test1",
}
verifyResp, err := cli.VerifyPassword(context.TODO(), verifyReq)
if err != nil {
return fmt.Errorf("failed to run VerifyPassword for correct password: %v", err)
}
if !verifyResp.Verified {
return fmt.Errorf("failed to verify correct password: %v", verifyResp)
}
log.Printf("properly verified correct password: %t\n", verifyResp.Verified)

badVerifyReq := &api.VerifyPasswordReq{
Email: "[email protected]",
Password: "wrong_password",
}
badVerifyResp, err := cli.VerifyPassword(context.TODO(), badVerifyReq)
if err != nil {
return fmt.Errorf("failed to run VerifyPassword for incorrect password: %v", err)
}
if badVerifyResp.Verified {
return fmt.Errorf("verify returned true for incorrect password: %v", badVerifyResp)
}
log.Printf("properly failed to verify incorrect password: %t\n", badVerifyResp.Verified)

log.Print("Listing Passwords:\n")
for _, pass := range resp.Passwords {
log.Printf("%+v", pass)
}

deleteReq := &api.DeletePasswordReq{
Email: p.Email,
}
Expand Down
34 changes: 34 additions & 0 deletions server/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,40 @@ 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")
}

if req.Password == "" {
return nil, errors.New("no passwored to verify supplied")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[typo] s/passwored/password/

}

// TODO we can dry this up if https://github.com/coreos/dex/pull/1271/files merges
password, err := d.s.GetPassword(req.Email)
if err != nil {
if err == storage.ErrNotFound {
return &api.VerifyPasswordResp{
NotFound: true,
Verified: false,
}, nil
}
d.logger.Errorf("api: there was an error retrieving the password: %v", err)
return nil, fmt.Errorf("verify password: %v", err)
}

resp := &api.VerifyPasswordResp{
NotFound: false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 err := bcrypt.CompareHashAndPassword(password.Hash, []byte(req.Password)); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.Verified = false
return resp, nil
}
resp.Verified = true
return resp, nil
}

func (d dexAPI) ListRefresh(ctx context.Context, req *api.ListRefreshReq) (*api.ListRefreshResp, error) {
id := new(internal.IDTokenSubject)
if err := internal.Unmarshal(req.UserId, id); err != nil {
Expand Down
Loading