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.

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

Fix endpoint api/users/reset-password to set tenant ID. #742

Closed
wants to merge 5 commits into from
Closed

Fix endpoint api/users/reset-password to set tenant ID. #742

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
db2dc58
Fix endpoint api/users/reset-password to set tenant ID via param.
jay-cascade Jul 13, 2022
8c9e93c
Add allow anonymous attribute.
jay-cascade Jul 13, 2022
2d7d608
Remove tenant param from ResetPasswordAsync interface; Add TennantIdH…
jay-cascade Jul 14, 2022
c6464ce
The email message now contains the API endpoint for password reset;
jay-cascade Jul 14, 2022
d1d5fa8
merge main branch
jay-cascade Jul 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Fix endpoint api/users/reset-password to set tenant ID via param.
  • Loading branch information
@jay-cascade
jay-cascade committed Jul 13, 2022
commit db2dc58338783931f58d6fa29aa5b3f92bf8ca20
2 changes: 1 addition & 1 deletion src/Core/Application/Identity/Users/IUserService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,6 @@ public interface IUserService : ITransientService
Task<string> ConfirmPhoneNumberAsync(string userId, string code);

Task<string> ForgotPasswordAsync(ForgotPasswordRequest request, string origin);
Task<string> ResetPasswordAsync(ResetPasswordRequest request);
Task<string> ResetPasswordAsync(ResetPasswordRequest request, string tenant);
Task ChangePasswordAsync(ChangePasswordRequest request, string userId);
}
4 changes: 2 additions & 2 deletions src/Host/Controllers/Identity/UsersController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,9 +112,9 @@ public Task<string> ForgotPasswordAsync(ForgotPasswordRequest request)
[HttpPost("reset-password")]
[OpenApiOperation("Reset a user's password.", "")]
[ApiConventionMethod(typeof(FSHApiConventions), nameof(FSHApiConventions.Register))]
public Task<string> ResetPasswordAsync(ResetPasswordRequest request)
public Task<string> ResetPasswordAsync(ResetPasswordRequest request, [FromQuery] string tenant)
{
return _userService.ResetPasswordAsync(request);
return _userService.ResetPasswordAsync(request, tenant);
jay-cascade marked this conversation as resolved.
Show resolved Hide resolved
}

private string GetOriginFromRequest() => $"{Request.Scheme}://{Request.Host.Value}{Request.PathBase.Value}";
Expand Down
6 changes: 4 additions & 2 deletions src/Infrastructure/Identity/UserService.Password.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ public async Task<string> ForgotPasswordAsync(ForgotPasswordRequest request, str
// For more information on how to enable account confirmation and password reset please
// visit https://go.microsoft.com/fwlink/?LinkID=532713
string code = await _userManager.GeneratePasswordResetTokenAsync(user);
const string route = "account/reset-password";
string route = $"account/reset-password?token={code}";
Copy link
Contributor

Choose a reason for hiding this comment

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

The token (code) is already added a few lines lower using the AddQueryString method...

This should probably be something like ?tenant={_currentTenant.Id} instead (or also use the AddQueryString method).

Have you tested this? Not 100% sure how this is supposed to work... but I suppose this url that is sent out is pointing to the front-end UI, where the user can input a new password... then that UI in turn issues a call to the backend again... so in that last call, the TenantId can actually be added as a header (and the controller method should then just have the [TenantIdHeader] attribute?

Like I said... not 100% sure... just my 2 cents ;-) (I personally am using AAD, so no need for password resets or the like)

Copy link
Author

Choose a reason for hiding this comment

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

I tested that the endpoint works., but I am still not sure this is exactly what we want. Yes, comments are helpful thanks... let me rework and come back to you.

Copy link
Author

@jay-cascade jay-cascade Jul 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@fretje , I have made further changes to this taking into account your suggestions. The tenancy is now pulled from the header using the attribute. But my point of confusion now is what to say in that email message? I've currently updated this to show the API endpoint and not a 'passwordResetUrl', as this is probably a frontend routing decision? Any suggestions? The confirm email route is using a GET request, but I do not suppose we can do that here as the url would contain sensitive information such as password!

Copy link
Contributor

@fretje fretje Jul 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

No, you need to go to your frontend from the link in the mail, where the user is then presented with a form to fill in their new password. From there, the front-end then issues a request to the "reset-password" api (in the backend) providing the actual new password which has been input by the user, and also the tenant and code which have been parsed from the url (querystring).

I mean... you can't hand over an url to the back-end and let the user figure out how to issue a post request to that url with the right json payload, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

The confirm email url is going straight to the backend, as that one doesn't need any other input from the user. I do think it should actually redirect back to the front-end again to be able to show a more user-friendly "your email has been confirmed" message, with an extra button to go back to the login page... but that's something else... ;-)

Copy link
Author

@jay-cascade jay-cascade Jul 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

"I mean... you can't hand over an url to the back-end and let the user figure out how to issue a post request to that url with the right json payload, right?"

I agree. But this project is a backend / api boilerplate, should we put some documentation in explaining how this should be setup, maybe the frontend route could come from config? Or are we looking to actually create a page for this in the boilerplate?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is https://github.com/fullstackhero/blazor-wasm-boilerplate which is a blazor front-end for this api... there's also an angular one, but that one hasn't been updated in a while.

Copy link
Author

Choose a reason for hiding this comment

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

@fretje So that's another issue, updating those. I might be able to look at the Angular one at some point in the future! For now then I think I will draw a line under this. I think it's better than how I found it, the endpoint now works at least.

Thanks for your hard work on an amazing project.

var endpointUri = new Uri(string.Concat($"{origin}/", route));
string passwordResetUrl = QueryHelpers.AddQueryString(endpointUri.ToString(), "Token", code);
var mailRequest = new MailRequest(
Expand All @@ -33,8 +33,10 @@ public async Task<string> ForgotPasswordAsync(ForgotPasswordRequest request, str
return _t["Password Reset Mail has been sent to your authorized Email."];
}

public async Task<string> ResetPasswordAsync(ResetPasswordRequest request)
public async Task<string> ResetPasswordAsync(ResetPasswordRequest request, string tenant)
{
EnsureValidTenant();

var user = await _userManager.FindByEmailAsync(request.Email?.Normalize());

// Don't reveal that the user does not exist
Expand Down