Skip to content
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

Configure Token provider from the corresponding provider #15627

Merged
merged 33 commits into from
Apr 12, 2024

Conversation

MikeAlhayek
Copy link
Member

@MikeAlhayek MikeAlhayek commented Mar 29, 2024

Enhanced functionality has been introduced, empowering developers to manage the expiration time of various tokens, including password-reset, email-confirmation, change-email, and two-factor authentication, delivered via the email service. Below, you'll find a list of configurable options along with their default values:

Class Name Default Expiration Value
ChangeEmailTokenProviderOptions The token is valid by default for 15 minutes.
EmailConfirmationTokenProviderOptions The token is valid by default for 48 hours.
PasswordResetTokenProviderOptions The token is valid by default for 15 minutes.

You many change the default values of these options by using the services.Configure<> method. For instance, to change the EmailConfirmationTokenProviderOptions you can add the following code to your project

services.Configure<EmailConfirmationTokenProviderOptions>(options => options.TokenLifespan = TimeSpan.FromDays(7));

Security Upgrade

This PR also enhance the security of the app by increasing the length of the reset-password token. Currently the rest-password token is only 6 characters in length. With this update, it is a long token and can't be guessed as easy as 6 characters.

@lampersky
Copy link
Contributor

this PR looks ok to me, but it is not solving the issue mentioned here

@hishamco
Copy link
Member

hishamco commented Apr 7, 2024

@MikeAlhayek could you please address this issue #15585 please

MikeAlhayek and others added 2 commits April 6, 2024 21:08
Co-authored-by: Hisham Bin Ateya <hishamco_2007@yahoo.com>
Co-authored-by: Hisham Bin Ateya <hishamco_2007@yahoo.com>
@hishamco
Copy link
Member

hishamco commented Apr 7, 2024

LGTM if this will not address the @lampersky issue, it would be nice to address the issue in this PR while you're working on this

@MikeAlhayek MikeAlhayek requested a review from agriffard as a code owner April 7, 2024 23:06
@MikeAlhayek
Copy link
Member Author

@lampersky @hishamco as a bonus, I implemented a fix for #15585

@sebastienros @Piedone let me know your thoughts on these default values for OrchardCore. I think these as okay values to have by default. But, would like to know if you have a better fix default values that you like to change to.

New options were added for the email token-providers to give you control on configuring the expiration time on each token sent via email. Here is a list of the available options that you can configure:

Class Name Default Expiration Value
PasswordResetTokenProviderOptions The token is valid by default for 15 minutes.
EmailConfirmationTokenProviderOptions The token is valid by default for 48 hours.
ChangeEmailTokenProviderOptions The token is valid by default for 30 minutes.

@Piedone
Copy link
Member

Piedone commented Apr 8, 2024

I'd use the same 15 minutes for ChangeEmailTokenProviderOptions as for PasswordResetTokenProviderOptions. These open up access to the user account, so they should be as short as possible, while allowing reasonably enough time for an e-mail to arrive and the user to react. 15 minutes is enough for this.

EmailConfirmationTokenProviderOptions looks good.

@MikeAlhayek
Copy link
Member Author

I'd use the same 15 minutes for ChangeEmailTokenProviderOptions as for PasswordResetTokenProviderOptions

Not a bad idea. Changed

@MikeAlhayek MikeAlhayek requested review from sebastienros and removed request for lampersky April 8, 2024 22:53
@hishamco
Copy link
Member

hishamco commented Apr 9, 2024

@lampersky your review please before merge

@lampersky
Copy link
Contributor

is it finished? I see new commits each day 😜

@MikeAlhayek
Copy link
Member Author

not ready yet. I am trying to add some tests for the Rfc6238Authentication service. I am not comfortable with the logic.

@hishamco
Copy link
Member

hishamco commented Apr 9, 2024

Do you need any help?

@MikeAlhayek
Copy link
Member Author

@lampersky okay. It's ready now. Feel free to review the code.

Copy link
Contributor

This pull request has merge conflicts. Please resolve those before requesting a review.

@MikeAlhayek MikeAlhayek merged commit 191eedd into main Apr 12, 2024
5 checks passed
@MikeAlhayek MikeAlhayek deleted the ma/configure-token-providers branch April 12, 2024 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants