Skip to content
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

Expose GetPassword via the GRPC API #1271

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tylercloke
Copy link
Contributor

GetPassword was already wired up to storage and everything, just took a bit more work to wire it through the API. This will be super helpful when building things on top of Dex as you can retrieve a password without enumerating the entire store via ListPasswords.

GetPassword was already wired up to storage and everything, just took a bit more work to wire it through the API. This will be super helpful when building things on top of Dex as you can retrieve a password without enumerating the entire store via ListPasswords.
Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

Similar to #1272, this doesn't seem to call for a general evaluation of the direction we want the gRPC API to take, but just adds one convenient method. I'm in favor of that 😄

@@ -48,6 +48,7 @@ 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. GetPassword
3. DeletePassword
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] I know it doesn't matter for presentation, but it's nicer to read this in raw form if the numbers are actually consecutive -- can you make DeletePassword a 4.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to maintain the style, but I'm just gonna make them all 1.

}

return &api.GetPasswordResp{
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] this line is superfluous, but being explicit is OK 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep was trying to be explicit.

@srenatus
Copy link
Contributor

@ericchiang What do you think about this one?

While #1020 would resolve our need for using dex for user storage, as long as that's the case, this addition would let us simplify all the place where we're currently going with

	resp, err := s.c.ListPasswords(ctx, &api.ListPasswordReq{})
	if err != nil {
		return nil, errors.Wrap(err, "get password")
	}

	// searches returned users for user
	for _, p := range resp.Passwords {
		if p.Email == email {
			usr := users.ShowUser{
				ID:    p.UserId,
				Name:  p.Username,
				Email: p.Email,
			}
			return &usr, nil
		}
	}

	return nil, &users.NotFoundError{}

...and, since it doesn't seem too big a deal for dex' GRPC API, I'd think it's a good addition 😉

@ericchiang
Copy link
Contributor

Can we do #1272 instead?

@srenatus
Copy link
Contributor

@ericchiang well, these two are rather complementary... 🤔 Or am I missing something? VerifyPassword wouldn't be of much use for simplifying this, would it?

string email = 1;
}

// GetPasswordResp a specific password by email.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is really oddly named. Can we either rename it "GetUserResp" and "GetUserReq"? Or can we strongly clarify that the password isn't returned?

@cippaciong
Copy link

@sagikazarmark @nabokihms I'm writing an internal component which relies on the gRPC API and I stumbled upon this issue while looking for a way the check if a certain Password/User exists in dex.

Is there any plan to include such an endpoint in the API?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants