feat(auth): Add returnOobLink to the ActionCodeSettings #502

Dal-Papa · 2022-06-27T15:51:08Z

Discussion

#501

Testing

Make sure all existing tests in the repository pass after your change. ✅
If you fixed a bug or added a feature, add a new test to cover your code. ✅

API Changes

At this time we cannot accept changes that affect the public API. If you'd like to help
us make Firebase APIs better, please propose your change in an issue so that we
can discuss it together.

Dal-Papa · 2022-08-01T11:22:50Z

@lahirumaramba @hiranya911 👋

lahirumaramba · 2022-10-17T20:03:10Z

Hi @Dal-Papa Thank you for your patience on this. The Admin SDKs currently do not support sending email action links. Setting returnOobLink does not send the email, in-fact we already set it to true in the code below.

firebase-admin-go/auth/email_action_links.go

Line 119 in 23a1f17

"returnOobLink": true,

If you are looking to generate email action links the SDK already supports that and you can use your own email API to send out the emails.

Dal-Papa · 2022-10-25T07:30:21Z

@lahirumaramba : Either you read the code wrong or you used it wrong but I was able to make Firebase send me an email with my version of the code.

firebase-admin-go/auth/email_action_links.go

Line 119 in 23a1f17

"returnOobLink": true,

This is overwritten by the settings object so therefore it becomes false and hence it sends an email. Please consider this PR, we are using the fork in the meantime.

prameshj · 2022-11-10T19:24:57Z

Lahiru Maramba : Either you read the code wrong or you used it wrong but I was able to make Firebase send me an email with my version of the code.

firebase-admin-go/auth/email_action_links.go

Line 119 in 23a1f17

"returnOobLink": true,

This is overwritten by the settings object so therefore it becomes false and hence it sends an email. Please consider this PR, we are using the fork in the meantime.

Hi @Dal-Papa , just to clarify, you are exposing ReturnOObLink in ActionCodeSettings, so you can override to false, in case you want the email to be sent automatically, correct? That makes sense, however, I am curious about the use-case.. typically the email verification/password reset etc are triggered by the client, so maybe more suitable in the client SDKs? Can you explain how it is useful in admin SDKs? Happy to move forward on this, but would like to better understand the scenario. Thanks!

Dal-Papa · 2022-11-14T08:36:00Z

Lahiru Maramba : Either you read the code wrong or you used it wrong but I was able to make Firebase send me an email with my version of the code.

firebase-admin-go/auth/email_action_links.go

Line 119 in 23a1f17

"returnOobLink": true,

This is overwritten by the settings object so therefore it becomes false and hence it sends an email. Please consider this PR, we are using the fork in the meantime.

Hi @Dal-Papa , just to clarify, you are exposing ReturnOObLink in ActionCodeSettings, so you can override to false, in case you want the email to be sent automatically, correct? That makes sense, however, I am curious about the use-case.. typically the email verification/password reset etc are triggered by the client, so maybe more suitable in the client SDKs? Can you explain how it is useful in admin SDKs? Happy to move forward on this, but would like to better understand the scenario. Thanks!

That's correct ! We do this so we can make a few checks on the account in the backend before sending such link (i.e. if the account isn't banned or if their email has been verified in the past).

prameshj · 2022-11-14T21:51:01Z

auth/email_action_links_test.go

@@ -40,6 +40,7 @@ var testActionCodeSettings = &ActionCodeSettings{
 	AndroidPackageName:    "com.example.android",
 	AndroidInstallApp:     true,
 	AndroidMinimumVersion: "6",
+	ReturnOobLink:         true,


Can we remove this in the base testActionCodeSettings struct? We need a test case to verify that if this field is not specified, returnOobLink is still set to true, due to sdk implementation.

I suggest adding a new test case in the newly added test, see my other comment. Thanks!

prameshj · 2022-11-14T21:59:35Z

auth/email_action_links_test.go

+func TestEmailVerificationSendEmail(t *testing.T) {
+	s := echoServer(testActionLinkResponse, t)
+	defer s.Close()
+


Can we call this "TestEmailVerificationCustomOobLinkSettings" or similar?

Then, we can add 2 cases:

cases := []bool{true, false} testActionCodeSettingsCustom := map[string]interface{}{} // make a copy to avoid modifying the base maps. for k, v := range testActionCodeSettings { testActionCodeSettingsCustom[k] = v } testActionCodeSettingsMapCustom := map[string]interface{}{} for k, v := range testActionCodeSettingsMapCustom { testActionCodeSettingsMapCustom[k] = v } for _, returnOobLink := range cases { testActionCodeSettingsCustom.ReturnOobLink = returnOobLink testActionCodeSettingsCustomMap["returnOobLink"] = returnOobLink link, err := s.Client.EmailVerificationLink(..., testActionCodeSettingsCustom) ... ... }

It would be great if you can add a similar test for EmailSignInLink and PasswordReset too. Thanks!

prameshj · 2022-11-14T22:00:24Z

Lahiru Maramba : Either you read the code wrong or you used it wrong but I was able to make Firebase send me an email with my version of the code.

firebase-admin-go/auth/email_action_links.go

Line 119 in 23a1f17

"returnOobLink": true,

This is overwritten by the settings object so therefore it becomes false and hence it sends an email. Please consider this PR, we are using the fork in the meantime.

Hi Clement DAL PALU , just to clarify, you are exposing ReturnOObLink in ActionCodeSettings, so you can override to false, in case you want the email to be sent automatically, correct? That makes sense, however, I am curious about the use-case.. typically the email verification/password reset etc are triggered by the client, so maybe more suitable in the client SDKs? Can you explain how it is useful in admin SDKs? Happy to move forward on this, but would like to better understand the scenario. Thanks!

That's correct ! We do this so we can make a few checks on the account in the backend before sending such link (i.e. if the account isn't banned or if their email has been verified in the past).

Thanks! Left a couple of comments in the unit test, LGTM otherwise.

Dal-Papa · 2022-11-15T13:09:25Z

Lahiru Maramba : Either you read the code wrong or you used it wrong but I was able to make Firebase send me an email with my version of the code.

firebase-admin-go/auth/email_action_links.go

Line 119 in 23a1f17

"returnOobLink": true,

This is overwritten by the settings object so therefore it becomes false and hence it sends an email. Please consider this PR, we are using the fork in the meantime.

Hi Clement DAL PALU , just to clarify, you are exposing ReturnOObLink in ActionCodeSettings, so you can override to false, in case you want the email to be sent automatically, correct? That makes sense, however, I am curious about the use-case.. typically the email verification/password reset etc are triggered by the client, so maybe more suitable in the client SDKs? Can you explain how it is useful in admin SDKs? Happy to move forward on this, but would like to better understand the scenario. Thanks!

That's correct ! We do this so we can make a few checks on the account in the backend before sending such link (i.e. if the account isn't banned or if their email has been verified in the past).

Thanks! Left a couple of comments in the unit test, LGTM otherwise.

I went ahead and fixed all the tests that were using the settings, let me know if that's not what you had in mind.

lahirumaramba · 2022-11-15T22:11:09Z

@Dal-Papa Thank you for your contribution! I definitely did misunderstood the API in my initial comment, my apologies!

@prameshj I think we need an internal API proposal for the public API change before we can merge this. Thanks!

prameshj

Thanks! We are working on the API review doc.

prameshj

Thanks for making the changes! Left 2 more comments (sorry for the back and forth). Other than that, LGTM to me.

prameshj · 2022-12-01T19:24:08Z

auth/email_action_links_test.go

-	if link != testActionLink {
-		t.Errorf("EmailVerificationLinkWithSettings() = %q; want = %q", link, testActionLink)
-	}
+	cases := []bool{true, false}


I'm wondering if we can make this a list of testActionCodeSettings and testActionCodeSettingsMap instead, so we also test the "unset" case. Added some sample here, to illustrate what i mean.. (I did not verify that this compiles)

sendEmailLinkValues := []string{"true", "false", "unset"} testActionCodeSettingsCases := []ActionCodeSettings{} testActionCodeSettingsMapCases := []map[string]interface{}{} for _, val := range sendEmailLinkValues { settings, settingsMap:= getCopiesOfTestSettings(testActionCodeSettings, testActionCodeSettingsMap) switch(val) { case "true": settings.SendEmailLink = true; settingsMap["returnOobLink"] = false; case "false": settings.SendEmailLink = false; settingsMap["returnOobLink"] = true; case "unset": } testActionCodeSettingsCases = append(testActionCodeSettingsCases, settings) testActionCodeSettingsMapCases = append(testActionCodeSettingsMapCases, settingsMap) }

prameshj · 2022-12-01T19:25:22Z

auth/email_action_links.go

@@ -58,6 +59,7 @@ func (settings *ActionCodeSettings) toMap() (map[string]interface{}, error) {
 	if err := json.Unmarshal(b, &result); err != nil {
 		return nil, err
 	}
+	result["returnOobLink"] = !settings.SendEmailLink


Maybe add a comment here like : "Since SendEmailLink defaults to true, we will always set "returnOobLink" to false by default, this means no email is sent out in the defautl case".

And we can remove the "returnOobLink" : true setting in line 121?

Looking at this after several weeks - can you remind me how SendEmailLink defaults to true? I think the comment should be:

"Since SendEmailLink defaults to false, we will always set "returnOobLink" to true by default, this means no email is sent out in the default case".

Apologies for the mistake in my earlier comment.

- Release 4.12.0

This reverts commit 32af2b8.

Dal-Papa · 2023-10-13T14:57:00Z

Hey @prameshj & @lahirumaramba ! I've fixed your comments and I've recently merged the latest changes, would you mind taking a look so we can merge this old PR ? 🙂

prameshj

lgtm, except one comment about fixing a code comment. Thanks for your patience!

prameshj · 2023-10-26T02:03:24Z

auth/email_action_links.go

@@ -58,6 +59,7 @@ func (settings *ActionCodeSettings) toMap() (map[string]interface{}, error) {
 	if err := json.Unmarshal(b, &result); err != nil {
 		return nil, err
 	}
+	result["returnOobLink"] = !settings.SendEmailLink


Looking at this after several weeks - can you remind me how SendEmailLink defaults to true? I think the comment should be:

"Since SendEmailLink defaults to false, we will always set "returnOobLink" to true by default, this means no email is sent out in the default case".

Apologies for the mistake in my earlier comment.

drmaples · 2023-12-11T22:21:51Z

any updates on when this will be merged?

Dal-Papa · 2024-03-04T09:26:09Z

@prameshj : I've updated to the latest upstream branch and fixed the comment. Please take a look.

Dal-Papa · 2024-03-06T09:42:08Z

@lahirumaramba : It looks like @prameshj is AWOL, can you please take over ?

Dal-Papa · 2024-03-30T13:51:34Z

@jonathanedey : You seem like someone who's still active here, can you please take a look ?

google-oss-bot and others added 13 commits May 21, 2020 11:49

Merge dev into master

54b8114

Merge dev into master

cef91ac

Merge dev into master

77177c7

Merge dev into master

a957589

Merge dev into master

eb0d2a0

Merge dev into master

05378ef

Merge dev into master

4121c50

Merge dev into master

928b104

Merge dev into master

02cde4f

Merge dev into master

6b40682

Merge dev into master

e60757f

Merge dev into master

bb055ed

Add returnOobLink to the ActionCodeSettings

e8dfc32

Merge dev into master

23a1f17

lahirumaramba changed the base branch from master to dev October 17, 2022 19:55

lahirumaramba self-assigned this Oct 17, 2022

Merge dev into master

1d24577

Merge remote-tracking branch 'upstream/dev'

f9dc53b

prameshj reviewed Nov 14, 2022

View reviewed changes

Fix all tests with both returnOobLink cases

12a4788

Dal-Papa requested a review from prameshj November 16, 2022 09:28

prameshj approved these changes Nov 21, 2022

View reviewed changes

Dal-Papa requested review from pragatimodi and lahirumaramba and removed request for pragatimodi and lahirumaramba November 23, 2022 16:08

prameshj reviewed Dec 1, 2022

View reviewed changes

Add an unset test case and a comment

981442d

Dal-Papa requested review from prameshj and removed request for pragatimodi March 8, 2023 14:29

google-oss-bot and others added 7 commits April 6, 2023 11:31

Merge dev into master

61c6c04

[chore] Release 4.12.0 (firebase#561)

32af2b8

- Release 4.12.0

Revert "[chore] Release 4.12.0 (firebase#561)" (firebase#565)

02300a8

This reverts commit 32af2b8.

Merge dev into master

74c9bd5

Merge dev into master

37c7936

Merge remote-tracking branch 'fruitz-fork/master'

bef15cf

Merge remote-tracking branch 'upstream/dev'

b28ccfb

prameshj approved these changes Oct 26, 2023

View reviewed changes

Merge dev into master

b04387e

Dal-Papa added 2 commits March 4, 2024 10:21

Merge remote-tracking branch 'upstream/master'

c348341

Fix comment

da18d13

Merge branch 'dev'

77f51b8

lahirumaramba requested review from prameshj and removed request for prameshj March 12, 2024 20:11

Dal-Papa added 2 commits March 29, 2024 15:55

Merge branch 'dev'

147a277

Resolve conflicts on tests

e5a5134

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(auth): Add returnOobLink to the ActionCodeSettings #502

feat(auth): Add returnOobLink to the ActionCodeSettings #502

feat(auth): Add returnOobLink to the ActionCodeSettings #502

Are you sure you want to change the base?

feat(auth): Add returnOobLink to the ActionCodeSettings #502

Conversation

Discussion

Testing

API Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment