-
Notifications
You must be signed in to change notification settings - Fork 306
feat(auth): implement generateVerifyAndChangeEmailLink API #1195
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
e52381f
2aa3883
f52801f
23004e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,6 +47,7 @@ | |
| import com.google.firebase.auth.multitenancy.TenantAwareFirebaseAuth; | ||
| import com.google.firebase.auth.multitenancy.TenantManager; | ||
| import com.google.firebase.internal.ApiClientUtils; | ||
| import com.google.firebase.internal.CallableOperation; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, I saw this and I fixed it bro |
||
| import com.google.firebase.internal.FirebaseProcessEnvironment; | ||
| import com.google.firebase.internal.SdkUtils; | ||
| import com.google.firebase.testing.MultiRequestMockHttpTransport; | ||
|
|
@@ -1510,6 +1511,23 @@ public void testGenerateEmailVerificationLink() throws Exception { | |
| assertEquals("VERIFY_EMAIL", parsed.get("requestType")); | ||
| assertTrue((Boolean) parsed.get("returnOobLink")); | ||
| } | ||
|
|
||
| @Test | ||
| public void testGenerateVerifyAndChangeEmailLink() throws Exception { | ||
| TestResponseInterceptor interceptor = initializeAppForUserManagement( | ||
| TestUtils.loadResource("generateEmailLink.json")); | ||
| String link = FirebaseAuth.getInstance() | ||
| .generateVerifyAndChangeEmailLinkAsync("test@example.com", "new@example.com").get(); | ||
| assertEquals("https://mock-oob-link.for.auth.tests", link); | ||
| checkRequestHeaders(interceptor); | ||
|
|
||
| GenericJson parsed = parseRequestContent(interceptor); | ||
| assertEquals(4, parsed.size()); | ||
| assertEquals("test@example.com", parsed.get("email")); | ||
| assertEquals("new@example.com", parsed.get("newEmail")); | ||
| assertEquals("VERIFY_AND_CHANGE_EMAIL", parsed.get("requestType")); | ||
| assertTrue((Boolean) parsed.get("returnOobLink")); | ||
| } | ||
|
Comment on lines
+1515
to
+1530
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a good test for the success case. To make the tests more comprehensive, please consider adding another test case that calls
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gemini-code-assist Thank you for the review! I have addressed all the feedback in the latest commit: Refactored Logic: Updated AbstractFirebaseAuth.java to use the internal getEmailActionLink and getEmailActionLinkAsync helper methods. Validation & Javadoc: Added checkArgument to ensure emails are not null/empty, and included the requested Javadoc. Imports: Removed the unused CallableOperation import in the test file. Test Coverage: Added testGenerateVerifyAndChangeEmailLinkWithSettings to verify that ActionCodeSettings are correctly parsed and passed in the payload. Public API: Exposed the new methods in FirebaseAuth.java so they are accessible. Linting: Fixed all Checkstyle line-length and formatting requirements. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's great to see that you've added |
||
|
|
||
| @Test | ||
| public void testGenerateESignInWithEmailLinkNoEmail() throws Exception { | ||
|
|
@@ -1529,6 +1547,7 @@ public void testGenerateESignInWithEmailLinkNoEmail() throws Exception { | |
| } | ||
| } | ||
|
|
||
|
|
||
| @Test | ||
| public void testGenerateESignInWithEmailLinkNullSettings() throws Exception { | ||
| initializeAppForUserManagement(); | ||
|
|
@@ -2864,6 +2883,7 @@ public void testTenantAwareDeleteSamlProviderConfig() throws Exception { | |
| checkUrl(interceptor, "DELETE", expectedUrl); | ||
| } | ||
|
|
||
|
|
||
| @Test | ||
| public void testCreateOidcProviderFromEmulatorAuth() throws Exception { | ||
| FirebaseProcessEnvironment.setenv("FIREBASE_AUTH_EMULATOR_HOST", AUTH_EMULATOR); | ||
|
|
||
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.
The new
generateVerifyAndChangeEmailLinkmethods are a great addition. To align them with the existing patterns in this class and improve maintainability, I suggest adding Javadoc for the new public methods and refactoring to use a private...Ophelper method. This helper would centralize the logic and argument validation (e.g., usingcheckArgumentforemailandnewEmail), which is a common pattern in this file.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.
@gemini-code-assist Thanks for the detailed review! I've updated the PR to fully align with the existing patterns in this class:
Refactored Logic: Centralized the logic into a private CallableOperation helper method (generateVerifyAndChangeEmailLinkOp).
Validation: Added checkArgument to validate that email and newEmail are not null or empty.
Documentation: Added the requested Javadoc for the new public and async methods.
The build is fully passing locally. Ready for the next stage of review lets goo
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.
The changes look good and align with the suggested improvements. Thank you for addressing the feedback.