diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 4873c528..08c7638b 100644 --- a/.pre-commit-config.yaml +++ b/.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 diff --git a/CHANGES.rst b/CHANGES.rst index 76414a23..546b0a8d 100644 --- a/CHANGES.rst +++ b/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 +++++++++++++++++++++++++++++++++ diff --git a/flask_security/forms.py b/flask_security/forms.py index 0ff197a0..a6f2b84e 100644 --- a/flask_security/forms.py +++ b/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 diff --git a/flask_security/views.py b/flask_security/views.py index faa4a25b..2d367ddd 100644 --- a/flask_security/views.py +++ b/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,8 +509,8 @@ 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) @@ -513,14 +518,17 @@ def forgot_password(): 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. diff --git a/pytest.ini b/pytest.ini index f38b6a6f..87360cbb 100644 --- a/pytest.ini +++ b/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 diff --git a/tests/templates/generic_confirm.html b/tests/templates/generic_confirm.html new file mode 100644 index 00000000..dbfa063f --- /dev/null +++ b/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) }} diff --git a/tests/templates/generic_reset.html b/tests/templates/generic_reset.html new file mode 100644 index 00000000..5841e00a --- /dev/null +++ b/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) }} diff --git a/tests/test_common.py b/tests/test_common.py index 94347036..3fe2c170 100644 --- a/tests/test_common.py +++ b/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="mattwho@lp.com", password="password"), - follow_redirects=False, - ) - assert response.status_code == 200 + with capture_flashes() as flashes: + response = client.post( + "/login", + data=dict(email="mattwho@lp.com", password="password"), + follow_redirects=False, + ) + assert response.status_code == 200 + assert len(flashes) == 0 @pytest.mark.registerable() diff --git a/tests/test_confirmable.py b/tests/test_confirmable.py index 9298076c..5af22ebb 100644 --- a/tests/test_confirmable.py +++ b/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="matt@lp.com")) 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="joe@lp.com", recaptcha="1234") + good_data = dict(email="joe@lp.com", 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 diff --git a/tests/test_recoverable.py b/tests/test_recoverable.py index 651cbbd7..d1e56f17 100644 --- a/tests/test_recoverable.py +++ b/tests/test_recoverable.py @@ -11,15 +11,18 @@ import pytest from flask import Flask +from wtforms.fields import StringField +from wtforms.validators import Length from tests.test_utils import ( authenticate, capture_flashes, capture_reset_password_requests, logout, + populate_data, ) -from flask_security.core import UserMixin -from flask_security.forms import LoginForm +from flask_security.core import Security, UserMixin +from flask_security.forms import ForgotPasswordForm, LoginForm from flask_security.signals import password_reset, reset_password_instructions_sent pytestmark = pytest.mark.recoverable() @@ -614,3 +617,64 @@ def test_generic_response(app, client, get_message): response = client.post("/reset", json=dict(email="whoami@test.com")) assert response.status_code == 200 assert not any(e in response.json["response"].keys() for e in ["error", "errors"]) + + +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 MyForgotPasswordForm(ForgotPasswordForm): + recaptcha = StringField("Recaptcha", validators=[Length(min=5)]) + + app.config["SECURITY_RETURN_GENERIC_RESPONSES"] = True + app.config["SECURITY_FORGOT_PASSWORD_TEMPLATE"] = "generic_reset.html" + app.security = Security( + app, + datastore=sqlalchemy_datastore, + forgot_password_form=MyForgotPasswordForm, + ) + + 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="joe@lp.com", recaptcha="1234") + good_data = dict(email="joe@lp.com", recaptcha="123456") + + with capture_flashes() as flashes: + response = client.post("/reset", 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("/reset", data=good_data) + assert len(flashes) == 1 + + # JSON + with capture_flashes() as flashes: + response = client.post("/reset", 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("/reset", 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("/reset", 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("/reset", 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