Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix endpoint api/users/reset-password to set tenant ID. #742
Fix endpoint api/users/reset-password to set tenant ID. #742
Changes from 2 commits
db2dc58
8c9e93c
2d7d608
c6464ce
d1d5fa8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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... ;-)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.