feat(recoverable): Add support for username recovery via simple login flows #1041
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1041 +/- ##
==========================================
+ Coverage 98.36% 98.37% +0.01%
==========================================
Files 37 37
Lines 4764 4795 +31
==========================================
+ Hits 4686 4717 +31
Misses 78 78 ☔ View full report in Codecov by Sentry. |
|
Thanks for the review @jwag956 I'll try to address these changes tomorrow 👍🏽 |
@jwag956 Apologies this took me a bit longer to get back into. But I believe I addressed everything. Please take a look whenever you have a chance and enjoy your holidays! |
| @@ -127,6 +127,8 @@ def app(request: pytest.FixtureRequest) -> SecurityFixture: | |||
| # Make this hex_md5 for token tests | |||
| app.config["SECURITY_HASHING_SCHEMES"] = ["hex_md5"] | |||
| app.config["SECURITY_DEPRECATED_HASHING_SCHEMES"] = [] | |||
| # Enable username recovery for tests | |||
| app.config["SECURITY_USERNAME_RECOVERY"] = True | |||
There was a problem hiding this comment.
nit: do this in for loop below with all the other options
| @@ -256,6 +256,7 @@ Forms | |||
| .. autoclass:: flask_security.WebAuthnVerifyForm | |||
| .. autoclass:: flask_security.Form | |||
| .. autoclass:: flask_security.FormInfo | |||
| .. autoclass:: flask_security.UsernameRecoveryForm | |||
| description: render_template(SECURITY_USERNAME_RECOVERY_TEMPLATE) | ||
| example: render_template(SECURITY_USERNAME_RECOVERY_TEMPLATE) | ||
| post: | ||
| summary: Request username recovery |
There was a problem hiding this comment.
Not quite complete - both request and response should have both form and JSON
| 200: | ||
| description: Send username recovery email | ||
| content: | ||
| application/json: |
There was a problem hiding this comment.
not quite right - this looks like the response for forms - not JSON - and you need to add JSON API
| @@ -58,6 +58,7 @@ | |||
| TwoFactorSetupForm, | |||
| TwoFactorVerifyCodeForm, | |||
| VerifyForm, | |||
| UsernameRecoveryForm, | |||
| ) | ||
|
|
||
| if form.validate_on_submit(): | ||
| user = _datastore.find_user(email=form.email.data) |
There was a problem hiding this comment.
You don't need to do this - UserEmailFormMixin already looks it up and sets form.user if email exists. So if validation succeeds - form.user will be set.
| if _security._want_json(request): | ||
| return base_render_json(form, include_user=False) | ||
|
|
||
| if form.validate_on_submit(): |
There was a problem hiding this comment.
I am not comfortable calling validate_on_submit twice .... as with many other views I think that on GET or POST errors - the template should be rendered (as you do). Maybe what I am asking is - rather than this if/redirect here - do the redirect up in the validate_on_submit() part:
if validate_on_submit():
send_username_recovery_email()
if _security_want_json(request):
return base_render_json....
do_flash(xxx)
return redirect(get_url(".login")
| if form.validate_on_submit(): | ||
| return redirect(url_for_security("login")) | ||
|
|
||
| return _security.render_template( |
There was a problem hiding this comment.
Please add a context_processor like all other views!
PR adds support for requesting a forgotten username (closes #980). If the user requesting the username recovery enters an email that is valid, they will receive the associated username via the registered email, otherwise they will not.