Improve /reset and /confirm w.r.t. GENERIC_RESPONSES and additional f…

…orm args.

Title says it all - very confusing (UX) results when adding a form arg (e.g. captcha) so the above forms and having an error.

closes #814

.pre-commit-config.yaml

@@ -14,12 +14,12 @@ repos:
     -   id: check-merge-conflict
     -   id: fix-byte-order-marker
 -   repo: https://github.com/asottile/pyupgrade
-    rev: v3.8.0
+    rev: v3.9.0
     hooks:
     -   id: pyupgrade
         args: [--py38-plus]
 -   repo: https://github.com/psf/black
-    rev: 23.3.0
+    rev: 23.7.0
     hooks:
     -   id: black
 -   repo: https://github.com/pycqa/flake8

CHANGES.rst

@@ -8,6 +8,9 @@ Version 5.3.0
 
 Released TBD
 
+This is a minor version bump due to the (possible) incompatibility w.r.t.
+WebAuthn.
+
 Fixes
 ++++++
 
@@ -18,6 +21,7 @@ Fixes
 - (:issue:`806`) Login no longer, by default, check for email deliverability.
 - (:issue:`791`) Token authentication is accepted on endpoints which only allow
   'session' as authentication-method. (N247S)
+- (:issue:`814`) /reset and /confirm and GENERIC_RESPONSES and addtional form args don't mix.
 
 Backwards Compatibility Concerns
 +++++++++++++++++++++++++++++++++

docs/patterns.rst

@@ -146,8 +146,12 @@ come with some usability concerns. The following endpoints are affected:
       message will be returned.
     * :py:data:`SECURITY_RESET_URL` - In all cases the `SECURITY_MSG_PASSWORD_RESET_REQUEST` message will be flashed. For JSON
       a 200 will always be returned (whether an email was sent or not).
+      ``Note``: If the application overrides the form and adds an additional field (e.g. `captcha`) and that field has
+      a validation error, a normal form error response will be returned (and JSON will return a 400).
     * :py:data:`SECURITY_CONFIRM_URL` - In all cases the `SECURITY_MSG_CONFIRMATION_REQUEST` message will be flashed. For JSON
       a 200 will always be returned (whether an email was sent or not).
+      ``Note``: If the application overrides the form and adds an additional field (e.g. `captcha`) and that field has
+      a validation error, a normal form error response will be returned (and JSON will return a 400).
     * :py:data:`SECURITY_US_SIGNIN_SEND_CODE_URL` - The `SECURITY_MSG_GENERIC_US_SIGNIN` message will be flashed in all cases -
       whether a selected method is setup for the user or not.
     * :py:data:`SECURITY_US_SIGNIN_URL` - For any errors (unknown username, inactive account, bad passcode) the `SECURITY_MSG_GENERIC_AUTHN_FAILED`
@@ -283,7 +287,7 @@ any changes to your UI and just need the following configuration::
 
 Angular's `httpClient`_ also supports this.
 
-For React based project you are free to choose your http client. It bundles fetch though. Retrieving the token is easy::
+For React based projects you are free to choose your http client (`fetch` is bundled by default). Retrieving the token is easy::
 
     fetch(url, {
       credentials: 'include',
@@ -294,7 +298,7 @@ For React based project you are free to choose your http client. It bundles fetc
       }
     });
 
-Sending the token on every, mutating, request is something that you should implement yourself. As an example an API call to an API
+Sending the token on every mutating request is something that you should implement yourself. As an example an API call to an API
 endpoint that does CSRF validation::
 
     function addUser(details) {

flask_security/forms.py

@@ -277,7 +277,7 @@ def form_errors_munge(form: Form, fields: t.Dict[str, t.Dict[str, str]]) -> None
     disclosing whether a user exists or not we need to return generic error messages.
     Furthermore, WTForms really likes to place errors on the field itself - which is
     a dead giveaway. We need to move errors from fields to the form.form_errors, and
-    replace then with generic msgs.
+    (optionally) replace then with generic msgs.
     """
     if not cv("RETURN_GENERIC_RESPONSES"):  # pragma: no cover
         return

flask_security/views.py

@@ -50,9 +50,12 @@
 from .decorators import anonymous_user_required, auth_required, unauth_csrf
 from .forms import (
     DummyForm,
+    ForgotPasswordForm,
+    LoginForm,
     build_form_from_request,
     build_form,
     form_errors_munge,
+    SendConfirmationForm,
     TwoFactorVerifyCodeForm,
     TwoFactorSetupForm,
     TwoFactorRescueForm,
@@ -153,8 +156,9 @@ def login() -> "ResponseValue":
     access user info and csrf-token.
     For POST - redirects to POST_LOGIN_VIEW (forms) or returns 400 (json).
     """
+    form = t.cast(LoginForm, build_form_from_request("login_form"))
 
-    if current_user.is_authenticated and request.method == "POST":
+    if current_user.is_authenticated:
         # Just redirect current_user to POST_LOGIN_VIEW.
         # While its tempting to try to logout the current user and login the
         # new requested user - that simply doesn't work with CSRF.
@@ -163,15 +167,17 @@ def login() -> "ResponseValue":
         # 'next' - which can cause infinite redirect loops
         # (see test_common::test_authenticated_loop)
         if _security._want_json(request):
-            payload = json_error_response(
-                errors=get_message("ANONYMOUS_USER_REQUIRED")[0]
-            )
-            return _security._render_json(payload, 400, None, None)
+            if request.method == "POST":
+                payload = json_error_response(
+                    errors=get_message("ANONYMOUS_USER_REQUIRED")[0]
+                )
+                return _security._render_json(payload, 400, None, None)
+            else:
+                form.user = current_user
+                return base_render_json(form)
         else:
             return redirect(get_url(cv("POST_LOGIN_VIEW")))
 
-    form = build_form_from_request("login_form")
-
     if form.validate_on_submit():
         assert form.user is not None
         remember_me = form.remember.data if "remember" in form else None
@@ -188,12 +194,8 @@ def login() -> "ResponseValue":
             return base_render_json(form, include_auth_token=True)
         return redirect(get_post_login_redirect())
 
-    if (
-        request.method == "POST"
-        and cv("RETURN_GENERIC_RESPONSES")
-        and not form.user_authenticated
-    ):
-        # Validation failed - make sure all error messages are generic
+    if request.method == "POST" and cv("RETURN_GENERIC_RESPONSES"):
+        # Validation failed - make sure PII error messages are generic
         fields_to_squash = dict(
             email=dict(replace_msg="GENERIC_AUTHN_FAILED"),
             password=dict(replace_msg="GENERIC_AUTHN_FAILED"),
@@ -202,19 +204,18 @@ def login() -> "ResponseValue":
             fields_to_squash["username"] = dict(replace_msg="GENERIC_AUTHN_FAILED")
         form_errors_munge(form, fields_to_squash)
 
-    if current_user.is_authenticated:
-        form.user = current_user
-        if _security._want_json(request):
-            return base_render_json(form)
-        return redirect(get_url(cv("POST_LOGIN_VIEW")))
-
     if _security._want_json(request):
         payload = {
             "identity_attributes": get_identity_attributes(),
         }
         return base_render_json(form, additional=payload)
 
-    if form.requires_confirmation and cv("REQUIRES_CONFIRMATION_ERROR_VIEW"):
+    if (
+        form.requires_confirmation
+        and cv("REQUIRES_CONFIRMATION_ERROR_VIEW")
+        and not cv("RETURN_GENERIC_RESPONSES")
+    ):
+        assert form.user_authenticated
         do_flash(*get_message("CONFIRMATION_REQUIRED"))
         return redirect(
             get_url(
@@ -398,8 +399,10 @@ def token_login(token):
 
 @unauth_csrf(fall_through=True)
 def send_confirmation():
-    """View function which sends confirmation instructions."""
-    form = build_form_from_request("send_confirmation_form")
+    """View function which sends confirmation instructions (/confirm)."""
+    form = t.cast(
+        SendConfirmationForm, build_form_from_request("send_confirmation_form")
+    )
 
     if form.validate_on_submit():
         send_confirmation_instructions(form.user)
@@ -410,10 +413,12 @@ def send_confirmation():
         # Here on GET or failed validate
         rinfo = dict(email=dict())
         form_errors_munge(form, rinfo)  # by suppressing errors JSON should return 200
-
-        # Make look exactly like successful (e.g. real user) request
-        if not _security._want_json(request):
-            do_flash(*get_message("CONFIRMATION_REQUEST", email=form.email.data))
+        # Check for other errors - for default form - there aren't additional fields
+        # but applications might add some (e.g. recaptcha)
+        if not form.errors:
+            # Make look exactly like successful (e.g. real user) request
+            if not _security._want_json(request):
+                do_flash(*get_message("CONFIRMATION_REQUEST", email=form.email.data))
 
     if _security._want_json(request):
         # Never include user info since this is an anonymous endpoint.
@@ -504,23 +509,26 @@ def confirm_email(token):
 @anonymous_user_required
 @unauth_csrf(fall_through=True)
 def forgot_password():
-    """View function that handles a forgotten password request."""
-    form = build_form_from_request("forgot_password_form")
+    """View function that handles a forgotten password request (/reset)."""
+    form = t.cast(ForgotPasswordForm, build_form_from_request("forgot_password_form"))
 
     if form.validate_on_submit():
         send_reset_password_instructions(form.user)
         if not _security._want_json(request):
             do_flash(*get_message("PASSWORD_RESET_REQUEST", email=form.email.data))
 
     elif request.method == "POST" and cv("RETURN_GENERIC_RESPONSES"):
-        # Here on GET or failed validate
+        # Here on failed validate (POST) and want generic responses
         rinfo = dict(email=dict())
         form_errors_munge(form, rinfo)  # by suppressing errors JSON should return 200
-
-        # Make look exactly like successful (e.g. real user) request
-        hash_password("not-a-password")  # reduce timing between successful and not.
-        if not _security._want_json(request):
-            do_flash(*get_message("PASSWORD_RESET_REQUEST", email=form.email.data))
+        # Check for other errors - for default form - there aren't additional fields
+        # but applications might add some (e.g. recaptcha)
+        if not form.errors:
+            # No OTHER errors on form.
+            # Make look exactly like successful (e.g. real user) request
+            hash_password("not-a-password")  # reduce timing between successful and not.
+            if not _security._want_json(request):
+                do_flash(*get_message("PASSWORD_RESET_REQUEST", email=form.email.data))
 
     if _security._want_json(request):
         # Never include user info since this is an anonymous endpoint.

pytest.ini

@@ -24,6 +24,6 @@ filterwarnings =
     ignore:.*passwordless feature.*:DeprecationWarning:flask_security:0
     ignore:.*passing settings to bcrypt.*:DeprecationWarning:passlib:0
     ignore:.*'crypt' is deprecated.*:DeprecationWarning:passlib:0
-    ignore:.*pkg_resources is deprecated.*:DeprecationWarning:pkg_resources:0
+    ignore:pkg_resources is deprecated:DeprecationWarning:passlib:0
     ignore:.*'sms' was enabled in SECURITY_US_ENABLED_METHODS;.*:UserWarning:flask_security:0
     ignore:.*'get_token_status' is deprecated.*:DeprecationWarning:flask_security:0

tests/templates/generic_confirm.html

@@ -0,0 +1,5 @@
+{% include "security/_messages.html" %}
+{% from "security/_macros.html" import render_field_with_errors, render_field, render_form_errors %}
+{{ render_field_with_errors(send_confirmation_form.email) }}
+{{ render_field_with_errors(send_confirmation_form.recaptcha) }}
+{{ render_form_errors(send_confirmation_form) }}

tests/templates/generic_reset.html

@@ -0,0 +1,5 @@
+{% include "security/_messages.html" %}
+{% from "security/_macros.html" import render_field_with_errors, render_field, render_form_errors %}
+{{ render_field_with_errors(forgot_password_form.email) }}
+{{ render_field_with_errors(forgot_password_form.recaptcha) }}
+{{ render_form_errors(forgot_password_form) }}

tests/test_common.py

@@ -21,6 +21,7 @@
 
 from tests.test_utils import (
     authenticate,
+    capture_flashes,
     get_auth_token_version_3x,
     get_form_action,
     get_num_queries,
@@ -218,12 +219,14 @@ def test_generic_response(app, client, get_message):
     )
 
     # make sure don't get confirmation required
-    response = client.post(
-        "/login",
-        data=dict(email="[email protected]", password="password"),
-        follow_redirects=False,
-    )
-    assert response.status_code == 200
+    with capture_flashes() as flashes:
+        response = client.post(
+            "/login",
+            data=dict(email="[email protected]", password="password"),
+            follow_redirects=False,
+        )
+        assert response.status_code == 200
+    assert len(flashes) == 0
 
 
 @pytest.mark.registerable()

tests/test_confirmable.py

@@ -11,16 +11,20 @@
 
 import pytest
 from flask import Flask
+from wtforms.fields import StringField
+from wtforms.validators import Length
 
-from flask_security.core import UserMixin
+from flask_security.core import Security, UserMixin
 from flask_security.confirmable import generate_confirmation_token
 from flask_security.signals import confirm_instructions_sent, user_confirmed
+from flask_security.forms import SendConfirmationForm
 
 from tests.test_utils import (
     authenticate,
     capture_flashes,
     capture_registrations,
     logout,
+    populate_data,
 )
 
 pytestmark = pytest.mark.confirmable()
@@ -573,3 +577,64 @@ def on_instructions_sent(app, **kwargs):
     response = client.post("/confirm", json=dict(email="[email protected]"))
     assert len(recorded_instructions_sent) == 2
     assert response.status_code == 200
+
+
+def test_generic_with_extra(app, sqlalchemy_datastore):
+    # If application adds a field, make sure we properly return errors
+    # even if 'RETURN_GENERIC_RESPONSES' is set.
+    class MySendConfirmationForm(SendConfirmationForm):
+        recaptcha = StringField("Recaptcha", validators=[Length(min=5)])
+
+    app.config["SECURITY_RETURN_GENERIC_RESPONSES"] = True
+    app.config["SECURITY_SEND_CONFIRMATION_TEMPLATE"] = "generic_confirm.html"
+    app.security = Security(
+        app,
+        datastore=sqlalchemy_datastore,
+        send_confirmation_form=MySendConfirmationForm,
+    )
+
+    populate_data(app)
+    client = app.test_client()
+
+    # Test valid user but invalid additional form field
+    # We should get a form error for the extra (invalid) field, no flash
+    bad_data = dict(email="[email protected]", recaptcha="1234")
+    good_data = dict(email="[email protected]", recaptcha="123456")
+
+    with capture_flashes() as flashes:
+        response = client.post("/confirm", data=bad_data)
+        assert b"Field must be at least 5" in response.data
+    assert len(flashes) == 0
+    with capture_flashes() as flashes:
+        response = client.post("/confirm", data=good_data)
+    assert len(flashes) == 1
+
+    # JSON
+    with capture_flashes() as flashes:
+        response = client.post("/confirm", json=bad_data)
+        assert response.status_code == 400
+        assert (
+            "Field must be at least 5"
+            in response.json["response"]["field_errors"]["recaptcha"][0]
+        )
+    assert len(flashes) == 0
+    with capture_flashes() as flashes:
+        response = client.post("/confirm", json=good_data)
+        assert response.status_code == 200
+    assert len(flashes) == 0
+
+    # Try bad email AND bad recaptcha
+    bad_data = dict(email="joe44-lp.com", recaptcha="1234")
+    with capture_flashes() as flashes:
+        response = client.post("/confirm", data=bad_data)
+        assert b"Field must be at least 5" in response.data
+    assert len(flashes) == 0
+    with capture_flashes() as flashes:
+        response = client.post("/confirm", json=bad_data)
+        assert response.status_code == 400
+        assert (
+            "Field must be at least 5"
+            in response.json["response"]["field_errors"]["recaptcha"][0]
+        )
+        assert len(response.json["response"]["errors"]) == 1
+    assert len(flashes) == 0