Skip to content

Field Error in Reauthentication Form #17681

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

Daksh2000
Copy link
Contributor

@Daksh2000 Daksh2000 commented Feb 28, 2025

Fixes: #16460

For proper error in password field while reauthenticating, I made some changes in reauthenticate function, adding a renderer
and returning HTTP status code 303 only in case if all form fields validation are successful , otherwise re render the form with appropriate error.

@Daksh2000 Daksh2000 requested a review from a team as a code owner February 28, 2025 18:58
@Daksh2000
Copy link
Contributor Author

Issue: #16460 (comment)

For proper error in password field while reauthenticating, I made some changes in reauthenticate function, adding a renderer and returning HTTP status code 303 only in case if all form fields validation are successful , otherwise re render the form with appropriate error.

Hi @di , Please share your views.

@Daksh2000
Copy link
Contributor Author

Issue: #16460 (comment)
For proper error in password field while reauthenticating, I made some changes in reauthenticate function, adding a renderer and returning HTTP status code 303 only in case if all form fields validation are successful , otherwise re render the form with appropriate error.

Hi @di , Please share your views.

Hi Team, could someone review this change

@Daksh2000 Daksh2000 changed the title Proper Error raise in Reauthentication Form Field Error in Reauthentication Form Mar 31, 2025
@Daksh2000
Copy link
Contributor Author

Issue: #16460 (comment)
For proper error in password field while reauthenticating, I made some changes in reauthenticate function, adding a renderer and returning HTTP status code 303 only in case if all form fields validation are successful , otherwise re render the form with appropriate error.

Hi @di , Please share your views.

Hi Team, could someone review this change

Hi @di , looking forward for updates on this PR

@di
Copy link
Member

di commented Apr 15, 2025

This mostly works but has the effect of changing the URL to /account/reauthenticate/ if the password is wrong, since in that case it renders the current view rather than redirecting.

As a result, visiting /account/reauthenticate/ directly always asks the user to re-authenticate (since the request is not a GET) and it never redirects elsewhere, even if the password is correct (because next_route is the same as the reauthentication route).

I think we might need to find another way to do this that doesn't require the view to render the page. Some options:

  • We could flash any errors instead: this wouldn't be great because we don't do this elsewhere
  • We could store any errors on the redirect_to route to make them accessible to the rendering page (like we do with next_route_matchdict and next_route_query)

@Daksh2000
Copy link
Contributor Author

Daksh2000 commented Apr 20, 2025

This mostly works but has the effect of changing the URL to /account/reauthenticate/ if the password is wrong, since in that case it renders the current view rather than redirecting.

As a result, visiting /account/reauthenticate/ directly always asks the user to re-authenticate (since the request is not a GET) and it never redirects elsewhere, even if the password is correct (because next_route is the same as the reauthentication route).

I think we might need to find another way to do this that doesn't require the view to render the page. Some options:

  • We could flash any errors instead: this wouldn't be great because we don't do this elsewhere
  • We could store any errors on the redirect_to route to make them accessible to the rendering page (like we do with next_route_matchdict and next_route_query)

Hi @di , Thank you for the insights and hints, I almost missed this case where user could visit the reauthenticate url directly and get stuck in a loop, I tried to pass a query param "errors" which has a "password" key and its corresponding error message, and also made some changes to "reauth_view" accordingly to incorporate for any query param,

Also , the views which have "require-reauth" set to True were facing this issue, so I made some changes in the wrapper function and they seem to work
Also made changes to few test cases accordingly,
Please share your views.

@Daksh2000 Daksh2000 marked this pull request as draft April 20, 2025 06:10
@Daksh2000 Daksh2000 marked this pull request as ready for review April 20, 2025 11:48
@Daksh2000 Daksh2000 marked this pull request as draft April 20, 2025 11:49
@Daksh2000
Copy link
Contributor Author

This mostly works but has the effect of changing the URL to /account/reauthenticate/ if the password is wrong, since in that case it renders the current view rather than redirecting.
As a result, visiting /account/reauthenticate/ directly always asks the user to re-authenticate (since the request is not a GET) and it never redirects elsewhere, even if the password is correct (because next_route is the same as the reauthentication route).
I think we might need to find another way to do this that doesn't require the view to render the page. Some options:

  • We could flash any errors instead: this wouldn't be great because we don't do this elsewhere
  • We could store any errors on the redirect_to route to make them accessible to the rendering page (like we do with next_route_matchdict and next_route_query)

Hi @di , Thank you for the insights and hints, I almost missed this case where user could visit the reauthenticate url directly and get stuck in a loop, I tried to pass a query param "errors" which has a "password" key and its corresponding error message, and also made some changes to "reauth_view" accordingly to incorporate for any query param,

Also , the views which have "require-reauth" set to True were facing this issue, so I made some changes in the wrapper function and they seem to work Also made changes to few test cases accordingly, Please share your views.

Hi @di , Also added some test cases to have more branch coverage.

@Daksh2000 Daksh2000 marked this pull request as ready for review April 20, 2025 16:45
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.

No error when password is incorrect when re-authenticating.
3 participants