Skip to content

feat(auth): GetUserByProviderID() #339

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

Merged
merged 24 commits into from
Mar 23, 2021
Merged

Conversation

rsgowman
Copy link
Member

No description provided.

@rsgowman rsgowman self-assigned this Feb 28, 2020
@rsgowman
Copy link
Member Author

rsgowman commented Mar 5, 2020

Note the diffbase. I'll merge+retarget to dev once the bulk_get stuff lands.

@rsgowman rsgowman requested a review from hiranya911 March 5, 2020 14:49
@rsgowman rsgowman assigned hiranya911 and unassigned rsgowman Mar 5, 2020
Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Looks good. Just a couple of suggestions on unit tests.

@hiranya911 hiranya911 assigned rsgowman and unassigned hiranya911 Mar 5, 2020
@rsgowman rsgowman assigned hiranya911 and unassigned rsgowman Mar 5, 2020
Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

LGTM with a suggestion.

defer s.Close()
for _, tc := range cases {
t.Run(tc.providerID+":"+tc.providerUID, func(t *testing.T) {
// The resulting user isn't parsed, so it just needs to exist (even if it's empty).
Copy link
Contributor

Choose a reason for hiding this comment

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

Move server initialization and defer close out of the main loop.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@hiranya911 hiranya911 assigned rsgowman and unassigned hiranya911 Mar 5, 2020
Base automatically changed from rsgowman/bulk_get to dev May 20, 2020 20:48
@rsgowman rsgowman assigned hiranya911 and unassigned rsgowman Mar 7, 2021
Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Implementation and tests look pretty good. Lets remove the redundant CI config bits, and we should be good to go.

@@ -0,0 +1,105 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

This file no longer exists in source. Can be dropped from this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, that's what caused this bad merge. (I mean, I should've caught it anyways, but this is making more sense now.)

@@ -92,6 +92,8 @@ fi
echo_info "Tag v${RELEASE_VERSION} does not exist."


<<<<<<< HEAD
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@@ -0,0 +1,64 @@
# Copyright 2020 Google Inc.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@@ -0,0 +1,74 @@
# Copyright 2020 Google Inc.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.


userRecord, err := s.Client.GetUserByProviderID(context.Background(), "google.com", "google_uid1")
want := "cannot find user from providerID: { google.com, google_uid1 }"
if userRecord != nil || err == nil || err.Error() != want {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to also check for IsUserNotFoundError(err) here

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@hiranya911 hiranya911 assigned rsgowman and unassigned hiranya911 Mar 8, 2021
@rsgowman rsgowman assigned hiranya911 and unassigned rsgowman Mar 16, 2021
Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM 👍

@hiranya911 hiranya911 assigned rsgowman and unassigned hiranya911 Mar 16, 2021
@hiranya911 hiranya911 changed the title GetUserByProviderID() feat(auth): GetUserByProviderID() Mar 16, 2021
@rsgowman rsgowman requested a review from egilmorez March 17, 2021 13:24
@rsgowman rsgowman assigned egilmorez and unassigned rsgowman Mar 17, 2021
Copy link

@egilmorez egilmorez left a comment

Choose a reason for hiding this comment

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

Just some minor nits. See what you think, and I'm happy to approve in any case. Thanks!

auth/user_mgt.go Outdated
@@ -498,6 +499,47 @@ func (c *baseClient) GetUserByPhoneNumber(ctx context.Context, phone string) (*U
})
}

// GetUserByProviderID gets the user data for the user corresponding to a given provider id.

Choose a reason for hiding this comment

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

Unless this is a literal, capped "ID" would probably be preferred.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

auth/user_mgt.go Outdated
// [Retrieve user data](https://firebase.google.com/docs/auth/admin/manage-users#retrieve_user_data)
// for code samples and detailed documentation.
//
// `providerID` indicates the provider, e.g. 'google.com' for the Google provider.

Choose a reason for hiding this comment

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

Suggest "such as" instead of "e.g."

My sense is that if we want to say "For example," it should come after a hard stop like a period or semicolon after "provider." Internal style guide: https://developers.google.com/style/abbreviations#dont-use

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

auth/user_mgt.go Outdated
// `providerUID` is the user identifier for the given provider.
func (c *baseClient) GetUserByProviderID(ctx context.Context, providerID string, providerUID string) (*UserRecord, error) {
// Although we don't really advertise it, we want to also handle non-federated
// idps with this call. So if we detect one of them, we'll reroute this

Choose a reason for hiding this comment

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

Should this be IDPs, with the acronym in caps?

Choose a reason for hiding this comment

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

Actually, this may not be a public-facing comment I guess :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Not a public facing comment; but still a valid point, so done anyways.

Copy link

@egilmorez egilmorez left a comment

Choose a reason for hiding this comment

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

LG, thanks!

@rsgowman rsgowman merged commit 9f28a3b into dev Mar 23, 2021
@rsgowman rsgowman deleted the rsgowman/getAccountByProviderId branch March 23, 2021 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants