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

feat(auth): ability to delete provider in auth #579

Merged
merged 10 commits into from
Oct 5, 2021

Conversation

hardikns
Copy link
Contributor

This address #578,
Adds the feature in the python admin sdk (which has been present in the nodejs sdk) that will allow users to delete providers (like google.com, facebook.com). Basically add delete_providers as input to update_user method in auth.

@hardikns hardikns marked this pull request as ready for review September 20, 2021 08:13
@hiranya911
Copy link
Contributor

Thank for the PR @hardikns. There's an open PR #383 that implements this feature, but I'm not sure if the original authors are still available to finish up the work.

@rsgowman @bojeil-google @nrsim are any of you available to finish and merge #383? If not we can accept this PR by @hardikns (it will likely need some minor changes to meet the approved API design).

@bojeil-google
Copy link
Contributor

If @rsgowman or @nrsim are unable to finish the pending PR, perhaps, we can accept @hardikns PR (adjusted to meet the approved API design) especially if it only needs minor changes?

@rsgowman
Copy link
Member

Ah, sorry; I forgot this was still outstanding. I don't think @nrsim will be available to finish #383, at least not for some time.

@hardikns's PR has the advantage of not having to resolve 1y+ worth of merge conflicts. Though #383 also does linking (whereas this one just deletes).

Since this PR likely only needs minor revision, we can probably get this one submitted quicker than #383. (And I assume @hardikns is waiting on this feature.) My vote is to proceed with this, and then rebase 383 ontop of this one.

My only hesitation is that the provider_to_link parameter should ideally appear before the providers_to_delete parameter in the update_user function. Or at least, I think that's the ordering on the other platforms.

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 everybody for the comments. I've reviewed the PR with the expectation that we will accept and merge this before #383.

@hardikns PR looks pretty good. I've pointed out a few things that can be improved.

firebase_admin/_user_mgt.py Outdated Show resolved Hide resolved
firebase_admin/_auth_client.py Outdated Show resolved Hide resolved
firebase_admin/_user_mgt.py Outdated Show resolved Hide resolved
integration/test_auth.py Outdated Show resolved Hide resolved
integration/test_auth.py Outdated Show resolved Hide resolved
tests/test_user_mgt.py Show resolved Hide resolved
@hiranya911 hiranya911 assigned hardikns and unassigned rsgowman and bojeil-google Sep 24, 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 @hardikns. LGTM with a few nits. I can merge once those comments are addressed.

firebase_admin/_auth_utils.py Outdated Show resolved Hide resolved
tests/test_user_mgt.py Outdated Show resolved Hide resolved
tests/test_user_mgt.py Show resolved Hide resolved
@hiranya911 hiranya911 changed the title [add] ability to delete provider in auth feat(auth): ability to delete provider in auth Sep 28, 2021
@hardikns
Copy link
Contributor Author

hardikns commented Oct 5, 2021

@hiranya911 I have addressed your comments and also updated my branch. Let me know if anything else is pending.

@hiranya911 hiranya911 merged commit 0a11d07 into firebase:master Oct 5, 2021
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.

None yet

4 participants