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): Add enableAnonymousUser param to CreateTenant and UpdateTenant #412

Merged
merged 7 commits into from
Apr 6, 2022
Merged

feat(auth): Add enableAnonymousUser param to CreateTenant and UpdateTenant #412

merged 7 commits into from
Apr 6, 2022

Conversation

sonatard
Copy link
Contributor

@sonatard sonatard commented Nov 9, 2020

#411
#372

"https://identitytoolkit.googleapis.com/v2beta1" does not support enableAnonymousUser param. use "https://identitytoolkit.googleapis.com/v2".

RELEASE NOTE: Allowed enabling of anonymous provider via tenant configuration

@google-cla
Copy link

google-cla bot commented Nov 9, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@sonatard
Copy link
Contributor Author

sonatard commented Nov 9, 2020

@googlebot I signed it!

@sonatard sonatard marked this pull request as ready for review November 9, 2020 16:33
@sonatard
Copy link
Contributor Author

#411 (comment)

@bojeil-google @rsgowman How is the progress of this plan?

@sonatard
Copy link
Contributor Author

@bojeil-google @rsgowman Is there any problem with this pull request? I'm having trouble because I need this feature.

@hiranya911
Copy link
Contributor

This was proposed and added to the Node.js SDK: https://firebase.google.com/docs/reference/admin/node/admin.auth.Tenant#anonymoussigninenabled

I don't think it's been approved for the Go SDK yet.

@sonatard
Copy link
Contributor Author

sonatard commented Oct 3, 2021

@lahirumaramba lahirumaramba self-assigned this Feb 7, 2022
@lahirumaramba lahirumaramba self-requested a review March 24, 2022 19:54
Copy link
Member

@lahirumaramba lahirumaramba left a comment

Choose a reason for hiding this comment

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

Thank you @sonatard for your contribution. I apologies for the delay. We have completed the required internal reviews for this API. There is a minor change requested, please check the review comments. Also, please change the base branch to dev instead of master, you might also have to rebase it to dev. Once that is done, we can merge this change.

auth/tenant_mgt.go Outdated Show resolved Hide resolved
auth/tenant_mgt.go Outdated Show resolved Hide resolved
auth/tenant_mgt.go Outdated Show resolved Hide resolved
@sonatard sonatard changed the base branch from master to dev March 25, 2022 01:52
@sonatard
Copy link
Contributor Author

@lahirumaramba Thank you for your review !! I fixed it.

@sonatard sonatard requested a review from lahirumaramba March 25, 2022 02:38
auth/tenant_mgt.go Outdated Show resolved Hide resolved
@lahirumaramba lahirumaramba requested a review from egilmorez March 25, 2022 23:25
@lahirumaramba
Copy link
Member

Hi @sonatard Thank you for updating the PR!
It looks like there are some lint issues. Please take a look at https://github.com/firebase/firebase-admin-go/runs/5699483758?check_suite_focus=true

I think your changes to go.sum cause the other CI failures. You should be able to fix them by running go mod download golang.org/x/lint or by just excluding the changes to go.sum from this PR.

@sonatard
Copy link
Contributor Author

@lahirumaramba I executed go mod download golang.org/x/lint.

@sonatard
Copy link
Contributor Author

@lahirumaramba I found CI failed. Is there anything that I need to do? Fix format?

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.

A couple of comment phrasing things to look at, thanks!

auth/tenant_mgt.go Outdated Show resolved Hide resolved
@@ -240,6 +242,11 @@ func (t *TenantToCreate) EnableEmailLinkSignIn(enable bool) *TenantToCreate {
return t.set(enableEmailLinkSignInKey, enable)
}

// EnableAnonymousUsers enables or disables anonymous user.

Choose a reason for hiding this comment

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

Looks like EnableAnonymousUsers is a literal that should be backticked for code font.

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't look like we back-tick literals in Go SDK. ex: TenantClient, AndroidNotification etc.

@@ -240,6 +242,11 @@ func (t *TenantToCreate) EnableEmailLinkSignIn(enable bool) *TenantToCreate {
return t.set(enableEmailLinkSignInKey, enable)
}

// EnableAnonymousUsers enables or disables anonymous user.

Choose a reason for hiding this comment

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

Our auth docs use the term "anonymous authentication." Should that be used here?

I'd suggest that phrase, or, alternatively, "anonymous user access."

@@ -275,6 +282,11 @@ func (t *TenantToUpdate) EnableEmailLinkSignIn(enable bool) *TenantToUpdate {
return t.set(enableEmailLinkSignInKey, enable)
}

// EnableAnonymousUsers enables or disables anonymous user.

Choose a reason for hiding this comment

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

Looks like EnableAnonymousUsers is a literal that should be backticked for code font.

@@ -275,6 +282,11 @@ func (t *TenantToUpdate) EnableEmailLinkSignIn(enable bool) *TenantToUpdate {
return t.set(enableEmailLinkSignInKey, enable)
}

// EnableAnonymousUsers enables or disables anonymous user.

Choose a reason for hiding this comment

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

Our auth docs use the term "anonymous authentication." Should that be used here?

I'd suggest that phrase, or, alternatively, "anonymous user access."

Copy link
Member

Choose a reason for hiding this comment

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

This makes sense, but in Go SDK we normally mention the function name followed by a description. Ex: AllowPasswordSignUp enables or disables email sign-in provider. or EnableEmailLinkSignIn enables or disables email link sign-in.

Choose a reason for hiding this comment

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

Hm, OK, no backticks I could live with. But just "anonymous user" doesn't sound to me like a thing that can be enabled or disabled. Does it to you Lahiru?

I'd say that "email link sign-in" does sound like it can be disabled/enabled. FWIW at this point, "email sign-in provider" does not; "email sign-in provider support" or "email sign-in provider functionality" might.

This sounds like I'm splitting hairs, but one of the few things I bring to the table is an ear for written English. :) Let's get Kevin's input as well!

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed.

"EnableAnonymousUsers enables or disables anonymous authentication."

(And FWIW, "AllowPasswordSignUp enables or disables the email sign-in provider.")

Choose a reason for hiding this comment

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

+1 Yup, "the" solves that latter one!

Copy link
Member

Choose a reason for hiding this comment

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

Oh I misread your previous comment, sorry! I understand what we are referring to now.
I am good with // EnableAnonymousUsers enables or disables anonymous user authentication.
Thank you!

@lahirumaramba
Copy link
Member

Hi, @sonatard Thank you for addressing the PR feedback!
Could you fix the formatting issues, please? You should be able to see them locally if you run gofmt -l -s .. Thank you!

@sonatard
Copy link
Contributor Author

@lahirumaramba I fixed format 👍

Copy link
Member

@lahirumaramba lahirumaramba left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

Copy link
Member

@lahirumaramba lahirumaramba left a comment

Choose a reason for hiding this comment

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

Hi @sonatard Thank you again for being patient with all the PR changes. Just one more change to reference docs and we can merge this PR to include in the upcoming release.

Please change the comment // EnableAnonymousUsers enables or disables user. to // EnableAnonymousUsers enables or disables anonymous authentication.

Thank you!

auth/tenant_mgt.go Outdated Show resolved Hide resolved
auth/tenant_mgt.go Outdated Show resolved Hide resolved
sonatard and others added 2 commits April 1, 2022 12:59
Co-authored-by: Lahiru Maramba <[email protected]>
Co-authored-by: Lahiru Maramba <[email protected]>
@sonatard
Copy link
Contributor Author

sonatard commented Apr 1, 2022

@lahirumaramba Thank you for your kind communication. I have had a very pleasant experience since the review began.
I’m so excited to release it!

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.

If Kevin's OK, I'm OK!

Thanks for sending!

@lahirumaramba lahirumaramba merged commit 6f9551f into firebase:dev Apr 6, 2022
@lahirumaramba lahirumaramba changed the title Add enableAnonymousUser param to CreateTenant and UpdateTenant feat(auth): Add enableAnonymousUser param to CreateTenant and UpdateTenant Apr 6, 2022
@sonatard sonatard deleted the enable-anonymous-user branch April 6, 2022 14:00
lahirumaramba added a commit that referenced this pull request Apr 6, 2022
- Bumped version to 4.8.0
- Includes #412 #485 #492
lahirumaramba added a commit that referenced this pull request Apr 6, 2022
- Bumped version to 4.8.0
- Includes #412 #485 #492
@lahirumaramba
Copy link
Member

Thank you @sonatard for your contribution! This feature is now released in v4.8.0

@sonatard
Copy link
Contributor Author

sonatard commented Apr 7, 2022

Thanks!!

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.

5 participants