feat(auth): Add returnOobLink to the ActionCodeSettings

Dal-Papa commented 2 years ago

Discussion

https://github.com/firebase/firebase-admin-go/issues/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 commented 1 year ago

@lahirumaramba @hiranya911 👋

lahirumaramba commented 1 year ago

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. https://github.com/firebase/firebase-admin-go/blob/23a1f17959238d989add41d562602b8bf04e71d6/auth/email_action_links.go#L119

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 commented 1 year ago

@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. https://github.com/firebase/firebase-admin-go/blob/23a1f17959238d989add41d562602b8bf04e71d6/auth/email_action_links.go#L119 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 commented 1 year ago

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.

https://github.com/firebase/firebase-admin-go/blob/23a1f17959238d989add41d562602b8bf04e71d6/auth/email_action_links.go#L119

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 commented 1 year ago

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. https://github.com/firebase/firebase-admin-go/blob/23a1f17959238d989add41d562602b8bf04e71d6/auth/email_action_links.go#L119

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 commented 1 year ago

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. https://github.com/firebase/firebase-admin-go/blob/23a1f17959238d989add41d562602b8bf04e71d6/auth/email_action_links.go#L119

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 commented 1 year ago

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. https://github.com/firebase/firebase-admin-go/blob/23a1f17959238d989add41d562602b8bf04e71d6/auth/email_action_links.go#L119

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 commented 1 year ago

@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 commented 9 months ago

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 ? 🙂

drmaples commented 7 months ago

any updates on when this will be merged?

Dal-Papa commented 4 months ago

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

Dal-Papa commented 4 months ago

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

Dal-Papa commented 3 months ago

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

firebase / firebase-admin-go