Search code, repositories, users, issues, pull requests...


 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

Copy link

Author

Dal-Papa commented Aug 1, 2022

@lahirumaramba @hiranya911 👋


 Merge dev into master

23a1f17

@lahirumaramba lahirumaramba changed the base branch from master to dev

October 17, 2022 19:55

firebase-admin-go/auth/email_action_links.go

Copy link

Member

lahirumaramba commented Oct 17, 2022

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.

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.

@lahirumaramba lahirumaramba self-assigned this

Oct 17, 2022

firebase-admin-go/auth/email_action_links.go

Copy link

Author

Dal-Papa commented Oct 25, 2022

@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.

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

Copy link

prameshj commented Nov 10, 2022

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!


 Merge dev into master

1d24577

Copy link

Author

Dal-Papa commented Nov 14, 2022

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).


 Merge remote-tracking branch 'upstream/dev'

f9dc53b

prameshj

prameshj reviewed

Nov 14, 2022

auth/email_action_links_test.go Outdated

AndroidPackageName: "com.example.android",

AndroidInstallApp: true,

AndroidMinimumVersion: "6",

ReturnOobLink: true,

Copy link

@prameshj prameshj Nov 14, 2022

Choose a reason for hiding this comment

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

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!

auth/email_action_links_test.go Outdated

func TestEmailVerificationSendEmail(t *testing.T) {

s := echoServer(testActionLinkResponse, t)

defer s.Close()

Copy link

@prameshj prameshj Nov 14, 2022

Choose a reason for hiding this comment

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

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

Copy link

prameshj commented Nov 14, 2022

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.


 Fix all tests with both returnOobLink cases

12a4788

Copy link

Author

Dal-Papa commented Nov 15, 2022

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.

Copy link

Member

lahirumaramba commented Nov 15, 2022

@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!

@Dal-Papa Dal-Papa requested a review from prameshj

November 16, 2022 09:28

prameshj

prameshj approved these changes

Nov 21, 2022

Copy link

@prameshj prameshj left a comment

Choose a reason for hiding this comment

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

Thanks! We are working on the API review doc.

@Dal-Papa Dal-Papa requested review from lahirumaramba and pragatimodi and removed request for lahirumaramba and pragatimodi

November 23, 2022 16:08

prameshj

prameshj reviewed

Dec 1, 2022

Copy link

@prameshj prameshj left a comment

Choose a reason for hiding this comment

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

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

auth/email_action_links_test.go Outdated

if link != testActionLink {

t.Errorf("EmailVerificationLinkWithSettings() = %q; want = %q", link, testActionLink)

}

cases := []bool{true, false}

Copy link

@prameshj prameshj Dec 1, 2022

Choose a reason for hiding this comment

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

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)
}

Copy link

Author

@Dal-Papa Dal-Papa Oct 16, 2023

Choose a reason for hiding this comment

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

Done

auth/email_action_links.go

if err := json.Unmarshal(b, &result); err != nil {

return nil, err

}

result["returnOobLink"] = !settings.SendEmailLink

Copy link

@prameshj prameshj Dec 1, 2022

Choose a reason for hiding this comment

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

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?

Copy link

Author

@Dal-Papa Dal-Papa Oct 16, 2023

Choose a reason for hiding this comment

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

Fixed

Copy link

@prameshj prameshj Oct 26, 2023

Choose a reason for hiding this comment

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

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.


 Add an unset test case and a comment

981442d

@Dal-Papa 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

Copy link

Author

Dal-Papa commented Oct 13, 2023

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

prameshj approved these changes

Oct 26, 2023

Copy link

@prameshj prameshj left a comment

Choose a reason for hiding this comment

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

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

auth/email_action_links.go

if err := json.Unmarshal(b, &result); err != nil {

return nil, err

}

result["returnOobLink"] = !settings.SendEmailLink

Copy link

@prameshj prameshj Oct 26, 2023

Choose a reason for hiding this comment

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

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.


 Merge dev into master

b04387e

@drmaples

Copy link

drmaples commented Dec 11, 2023

any updates on when this will be merged?

Dal-Papa added 2 commits

March 4, 2024 10:21


 Merge remote-tracking branch 'upstream/master'

c348341


 Fix comment

da18d13

Copy link

Author

Dal-Papa commented Mar 4, 2024

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


 Merge branch 'dev'

77f51b8

Copy link

Author

Dal-Papa commented Mar 6, 2024

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

@lahirumaramba 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

Copy link

Author

Dal-Papa commented Mar 30, 2024 •

edited

Loading

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