Skip to content

feat(auth): Implemented GeneratePasswordResetLinkAsync() API #162

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Apr 2, 2020
Merged

feat(auth): Implemented GeneratePasswordResetLinkAsync() API #162

merged 5 commits into from
Apr 2, 2020

Conversation

@hiranya911
Copy link
Contributor

@hiranya911 hiranya911 commented Mar 30, 2020 •
edited
Loading

API for email action link generation. API proposal go/firebase-dotnet-mt

class ActionCodeSettings
  string Url
  boolean HandleCodeInApp
  string DynamicLinkDomain
  string IosBundleId
  string AndroidPackageName
  string AndroidMinimumVersion
  boolean AndroidInstallApp
class FirebaseAuth
  Task<string> GeneratePasswordResetLinkAsync(string email)
  Task<string> GeneratePasswordResetLinkAsync(string email, ActionCodeSettings settings)

RELEASE NOTE: Added GeneratePasswordResetLinkAsync() method for generating links for password reset flows.

lahirumaramba
lahirumaramba approved these changes Apr 1, 2020
View reviewed changes
Copy link
Member

@lahirumaramba lahirumaramba left a comment

Choose a reason for hiding this comment

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

LGTM!

@hiranya911 hiranya911 assigned egilmorez and unassigned hiranya911 Apr 1, 2020
@hiranya911 hiranya911 requested a review from egilmorez April 1, 2020 19:13
@hiranya911
Copy link
Contributor Author

hiranya911 commented Apr 1, 2020

@egilmorez please review the API docs in ActionCodeSettings and FirebaseAuth classes.

egilmorez
egilmorez reviewed Apr 2, 2020
View reviewed changes
FirebaseAdmin/FirebaseAdmin/Auth/FirebaseAuth.cs Outdated
/// </summary>
/// <exception cref="FirebaseAuthException">If an error occurs while setting custom claims.</exception>
/// <param name="email">The email of the user whose password is to be reset.</param>
/// <param name="settings">The action code settings object which defines whether
Copy link
Contributor

@egilmorez egilmorez Apr 2, 2020

Choose a reason for hiding this comment

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

I think this technically should be "that" -- you're referring to a particular object that has these attributes, and not just adding information about the object.

Copy link
Contributor Author

@hiranya911 hiranya911 Apr 2, 2020

Choose a reason for hiding this comment

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

Done

egilmorez
egilmorez reviewed Apr 2, 2020
View reviewed changes
FirebaseAdmin/FirebaseAdmin/Auth/FirebaseAuth.cs Outdated
/// <param name="email">The email of the user whose password is to be reset.</param>
/// <param name="settings">The action code settings object which defines whether
/// the link is to be handled by a mobile app and the additional state information to be
/// passed in the deep link..</param>
Copy link
Contributor

@egilmorez egilmorez Apr 2, 2020

Choose a reason for hiding this comment

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

Extra period here.

Copy link
Contributor Author

@hiranya911 hiranya911 Apr 2, 2020

Choose a reason for hiding this comment

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

Done

egilmorez
egilmorez reviewed Apr 2, 2020
View reviewed changes
FirebaseAdmin/FirebaseAdmin/Auth/FirebaseAuth.cs Outdated
/// </summary>
/// <exception cref="FirebaseAuthException">If an error occurs while setting custom claims.</exception>
/// <param name="email">The email of the user whose password is to be reset.</param>
/// <param name="settings">The action code settings object which defines whether
Copy link
Contributor

@egilmorez egilmorez Apr 2, 2020

Choose a reason for hiding this comment

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

Suggest "that," as above.

Copy link
Contributor Author

@hiranya911 hiranya911 Apr 2, 2020

Choose a reason for hiding this comment

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

Done

egilmorez
egilmorez reviewed Apr 2, 2020
View reviewed changes
FirebaseAdmin/FirebaseAdmin/Auth/FirebaseAuth.cs Outdated
/// <param name="email">The email of the user whose password is to be reset.</param>
/// <param name="settings">The action code settings object which defines whether
/// the link is to be handled by a mobile app and the additional state information to be
/// passed in the deep link..</param>
Copy link
Contributor

@egilmorez egilmorez Apr 2, 2020

Choose a reason for hiding this comment

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

Extra period.

Copy link
Contributor Author

@hiranya911 hiranya911 Apr 2, 2020

Choose a reason for hiding this comment

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

Done

egilmorez
egilmorez reviewed Apr 2, 2020
View reviewed changes
FirebaseAdmin/FirebaseAdmin/Auth/ActionCodeSettings.cs
/// <summary>
/// Gets or sets a value indicating whether to open the link via a mobile app or a browser.
/// The default is false. When set to true, the action code link is sent as a Universal
/// Link or an Android App Link and is opened by the app if installed. In the false case,
Copy link
Contributor

@egilmorez egilmorez Apr 2, 2020

Choose a reason for hiding this comment

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

Is any of this relevant when the app isn't installed? I wonder if we need this phrase.

Copy link
Contributor Author

@hiranya911 hiranya911 Apr 2, 2020

Choose a reason for hiding this comment

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

I think those are orthogonal issues. If the app isn't installed, you will still get a dynamic link that will offer to install the app.

egilmorez
egilmorez reviewed Apr 2, 2020
View reviewed changes
FirebaseAdmin/FirebaseAdmin/Auth/ActionCodeSettings.cs Outdated
public string AndroidPackageName { get; set; }

/// <summary>
/// Gets or sets the minimum version for Android app. If the installed app is an older
Copy link
Contributor

@egilmorez egilmorez Apr 2, 2020

Choose a reason for hiding this comment

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

Suggest "an Android app," or "the" if we mean the specific one being targeted.

Copy link
Contributor Author

@hiranya911 hiranya911 Apr 2, 2020

Choose a reason for hiding this comment

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

Done

egilmorez
egilmorez reviewed Apr 2, 2020
View reviewed changes
Copy link
Contributor

@egilmorez egilmorez left a comment

Choose a reason for hiding this comment

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

A few things to check, thanks!!

egilmorez
egilmorez approved these changes Apr 2, 2020
View reviewed changes
Copy link
Contributor

@egilmorez egilmorez left a comment

Choose a reason for hiding this comment

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

Thanks!

@hiranya911 hiranya911 merged commit d51d33c into master Apr 2, 2020
@hiranya911 hiranya911 deleted the hkj-password-reset branch April 2, 2020 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lahirumaramba lahirumaramba lahirumaramba approved these changes

@egilmorez egilmorez egilmorez approved these changes

Assignees

@egilmorez egilmorez

Labels

release-note

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hiranya911 @lahirumaramba @egilmorez