- Notifications
You must be signed in to change notification settings - Fork 269
feat(auth): Update ActionCodeSettings to support LinkDomain and deprecate DynamicLinkDomain #703
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): Update ActionCodeSettings to support LinkDomain and deprecate DynamicLinkDomain #703
Conversation
- Release 4.12.0
This reverts commit 32af2b8.
Xiaoshouzi-gh left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting this together!
| Thanks for the contribution! The base branch for PRs should be pointed to |
| I guess you'll want to close either this PR or #682. I had originally created a PR targeting dev, but was recently asked to make one targeting master |
jonathanedey left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @graeme-verticalscope, thanks for getting started on this.
Can you add some error logic similar for what we have for dynamicLinkDomain:
firebase-admin-go/auth/user_mgt.go
Line 524 in d515faf
| invalidDynamicLinkDomain = "INVALID_DYNAMIC_LINK_DOMAIN" |
For invalidHostingLinkDomain the backend error should be "INVALID_HOSTING_LINK_DOMAIN"
firebase-admin-go/auth/user_mgt.go
Lines 554 to 557 in d515faf
| // IsInvalidDynamicLinkDomain checks if the given error was due to an invalid dynamic link domain. | |
| func IsInvalidDynamicLinkDomain(err error) bool { | |
| return hasAuthErrorCode(err, invalidDynamicLinkDomain) | |
| } |
firebase-admin-go/auth/user_mgt.go
Lines 1445 to 1449 in d515faf
| "INVALID_DYNAMIC_LINK_DOMAIN": { | |
| code: internal.InvalidArgument, | |
| message: "the provided dynamic link domain is not configured or authorized for the current project", | |
| authCode: invalidDynamicLinkDomain, | |
| }, |
Also ensure these are included in our tests:
firebase-admin-go/auth/user_mgt_test.go
Lines 2149 to 2153 in d515faf
| "INVALID_DYNAMIC_LINK_DOMAIN": { | |
| IsInvalidDynamicLinkDomain, | |
| errorutils.IsInvalidArgument, | |
| "the provided dynamic link domain is not configured or authorized for the current project", | |
| }, |
firebase-admin-go/auth/email_action_links_test.go
Lines 293 to 296 in d515faf
| cases := map[string]func(error) bool{ | |
| "UNAUTHORIZED_DOMAIN": IsUnauthorizedContinueURI, | |
| "INVALID_DYNAMIC_LINK_DOMAIN": IsInvalidDynamicLinkDomain, | |
| } |
Thanks!
| @jonathanedey, thanks I added the error code you mentioned! You may want to check that the code and message are correct here: I used the same |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one change, thanks!
auth/user_mgt.go Outdated
| }, | ||
| "INVALID_HOSTING_LINK_DOMAIN": { | ||
| code: internal.InvalidArgument, | ||
| message: "the provided hosting link domain is not configured or authorized for the current project", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call out, lets go with the following to match the node sdk:
the provided hosting link domain is not configured in Firebase Hosting or is not owned by the current project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! I updated the error message.
egilmorez left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG with possible nit, thanks!
| Let me know if there's anything else I need to do, otherwise I'm assuming this can be approved and merged by a repository maintainer :) |
ActionCodeSettings to support LinkDomain and deprecate DynamicLinkDomain ActionCodeSettings to support LinkDomain and deprecate DynamicLinkDomainActionCodeSettings to support LinkDomain and deprecate DynamicLinkDomain | That's everything, thanks again @graeme-verticalscope for putting this together! |
Discussion
Add the
LinkDomainfield to theActionCodeSettingsstruct, this new field enables migrating off of firebase dynamic links, which is deprecated and will be unsupported soon.See #681, I was asked to create a second PR targeting master branch. Original PR is #682