From c72058ded8f73bbe131aa2dfff2df3810e71893b Mon Sep 17 00:00:00 2001 From: Chris Wagner Date: Thu, 2 Jan 2025 17:37:42 -0800 Subject: [PATCH] Minor change to how LoginForm is created/modified The default LoginForm now reflects the default configuration - email is required. In init_app() build_login_form() is called that if USERNAME_ENABLE is set will add the username field (as before) and change the email field to be Optional(). This is a small semantic change - prior the email field was not marked as required. Change the new RegisterFormV2 construction - now the default form reflects the default configuration - new_password and confirm_password are required. From init_app build_register_form() is called and it will: 1) remove password_confirm field if PASSWORD_CONFIRM_REQUIRED is False 2) add username field if USERNAME_ENABLE is True 3) mark the password field as optional if PASSWORD_REQUIRED is False or UNIFIED_SIGNING is True --- flask_security/core.py | 9 +- flask_security/forms.py | 111 ++++++++++-------- .../templates/security/login_user.html | 5 +- tests/conftest.py | 5 +- tests/test_basic.py | 2 +- tests/test_misc.py | 37 +++++- 6 files changed, 109 insertions(+), 60 deletions(-) diff --git a/flask_security/core.py b/flask_security/core.py index 5739b4f6..18f130f8 100644 --- a/flask_security/core.py +++ b/flask_security/core.py @@ -56,7 +56,7 @@ VerifyForm, build_register_username_field, build_register_form, - login_username_field, + build_login_form, ) from .json import setup_json from .mail_util import MailUtil @@ -1655,9 +1655,10 @@ def init_app( fcls = self.forms["confirm_register_form"].cls if fcls and issubclass(fcls, RegisterFormMixin): fcls.username = build_register_username_field(app) - fcls = self.forms["login_form"].cls - if fcls and issubclass(fcls, LoginForm): - fcls.username = login_username_field + + fcls = self.forms["login_form"].cls + if fcls and issubclass(fcls, LoginForm): + build_login_form(app=app, fcls=fcls) # new unified RegisterForm fcls = self.forms["register_form"].cls diff --git a/flask_security/forms.py b/flask_security/forms.py index 24d36946..9b466bd5 100644 --- a/flask_security/forms.py +++ b/flask_security/forms.py @@ -354,32 +354,6 @@ class PasswordConfirmFormMixin: ) -def build_password_field(is_confirm=False, autocomplete="new-password", app=None): - # Based on configuration return PasswordField with appropriate validators - # Note that length and breached validators are done as part of form. - validators = list() - render_kw = {"autocomplete": autocomplete} - if cv("PASSWORD_REQUIRED", app) or not cv("UNIFIED_SIGNIN", app): - validators.append(password_required) - - if is_confirm: - validators.append( - EqualToLocalize("password", message="RETYPE_PASSWORD_MISMATCH") - ) - return PasswordField( - label=get_form_field_label("retype_password"), - render_kw=render_kw, - validators=validators, - ) - - # normal password field - return PasswordField( - label=get_form_field_label("password"), - render_kw=render_kw, - validators=validators, - ) - - class NextFormMixin: next = HiddenField() @@ -418,13 +392,6 @@ def build_register_username_field(app): ) -login_username_field = StringField( - get_form_field_label("username"), - render_kw={"autocomplete": "username"}, - validators=[username_validator], -) - - class RegisterFormMixin: submit = SubmitField(get_form_field_label("register")) @@ -526,7 +493,7 @@ def validate(self, **kwargs: t.Any) -> bool: class LoginForm(Form, PasswordFormMixin, NextFormMixin): - """The default login form + """The default login form. The following fields are defined: * email @@ -534,18 +501,25 @@ class LoginForm(Form, PasswordFormMixin, NextFormMixin): * password * remember (checkbox) * next - - If a subclass wants to handle identity, it can set self.ifield to the - form field that it validated. That will cause the validation logic here around - identity to be skipped. The subclass must also set self.user to the found User. + * submit + + The following attributes might be useful for subclasses: + * ifield - If a subclass wants to handle identity, it can set self.ifield to the + form field that it validated. That will cause the validation logic here + around identity to be skipped. + The subclass must also set self.user to the found User. + * user - as stated above - if subclass does identity check it must set this + field. """ - # email field - we don't use valid_user_email since for login - # with username feature it is potentially optional. + # email field - we don't use valid_user_email since for login we must check + # user_identity_attributes to ensure 'email' is listed. + # If USERNAME_ENABLE is set - this field will be replaced to be Optional() + # see build_login_form() email = EmailField( get_form_field_label("email"), render_kw={"autocomplete": "email"}, - validators=[Optional(), EmailValidation(verify=False)], + validators=[email_required, EmailValidation(verify=False)], ) # username is added dynamically based on USERNAME_ENABLED. @@ -582,7 +556,9 @@ def validate(self, **kwargs: t.Any) -> bool: # responsible to deal with USER_IDENTITY_ATTRIBUTES if it cares. if not self.ifield: uia_email = get_identity_attribute("email") - if uia_email and self.email.data: + # pedantic checks - if app subclasses form, removes email but forgets to + # remove "email" from identity_attributes + if uia_email and hasattr(self, "email") and self.email and self.email.data: self.ifield = self.email self.user = _datastore.find_user( case_insensitive=uia_email.get("case_insensitive", False), @@ -635,6 +611,30 @@ def validate(self, **kwargs: t.Any) -> bool: return True +def build_login_form(app, fcls): + # Based on app configuration, add optional/configurable fields to the login form + # Allow app to override the field using mixins. + # Note this is only called (from init_app()) if form is subclassed from ours. + # We really don't want to touch the form unless there is a specific option + # requested - so that subclasses can change/do what they want (including deleting + # email for example). + if cv("USERNAME_ENABLE", app): + fcls.username = StringField( + get_form_field_label("username"), + render_kw={"autocomplete": "username"}, + validators=[username_validator], + ) + if hasattr(fcls, "email") and fcls.email: + # Make field Optional() + # let subclass easily get rid of this + # Note that WTForms 'del' operator actually sets this to None + fcls.email = EmailField( + get_form_field_label("email"), + render_kw={"autocomplete": "email"}, + validators=[Optional(), EmailValidation(verify=False)], + ) + + class VerifyForm(Form, PasswordFormMixin): """The verify authentication form""" @@ -739,7 +739,13 @@ def __init__(self, *args, **kwargs): self.next.data = request.args.get("next", "") -class RegisterFormV2(Form, UniqueEmailFormMixin, NextFormMixin): +class RegisterFormV2( + Form, + UniqueEmailFormMixin, + NextFormMixin, + NewPasswordFormMixin, + PasswordConfirmFormMixin, +): """This form is used for registering. The following fields are defined: @@ -751,7 +757,7 @@ class RegisterFormV2(Form, UniqueEmailFormMixin, NextFormMixin): * next Since there are many configuration options that alter which fields and - which validators (e.g. Required), some fields are added dynamically at + which validators (e.g. Required), some fields are added or changed at Security init_app() time by calling the internal method build_register_form(). We want to support OWASP best-practice around mitigating user enumeration. @@ -765,8 +771,6 @@ class RegisterFormV2(Form, UniqueEmailFormMixin, NextFormMixin): submit = SubmitField(get_form_field_label("register")) username: t.ClassVar[Field] - password: t.ClassVar[Field] - password_confirm: t.ClassVar[Field] def to_dict(self, only_user): """ @@ -826,11 +830,16 @@ def __init__(self, *args, **kwargs): def build_register_form(app, fcls): # Based on app configuration, add optional/configurable fields to the register form # Allow app to override the field using mixins - if not (hasattr(fcls, "password") and fcls.password): - fcls.password = build_password_field(app=app) - if cv("PASSWORD_CONFIRM_REQUIRED", app=app): - if not (hasattr(fcls, "password_confirm") and fcls.password_confirm): - fcls.password_confirm = build_password_field(app=app, is_confirm=True) + # Note that this occurs AFTER app might have sub-classed the form + if not cv("PASSWORD_REQUIRED", app) or cv("UNIFIED_SIGNIN", app): + # mark password field as Optional + fcls.password = PasswordField( + label=get_form_field_label("password"), + render_kw={"autocomplete": "new-password"}, + validators=[Optional()], + ) + if not cv("PASSWORD_CONFIRM_REQUIRED", app=app): + fcls.password_confirm = None if cv("USERNAME_ENABLE", app): fcls.username = build_register_username_field(app=app) diff --git a/flask_security/templates/security/login_user.html b/flask_security/templates/security/login_user.html index 2d254921..42a4dbac 100644 --- a/flask_security/templates/security/login_user.html +++ b/flask_security/templates/security/login_user.html @@ -7,9 +7,10 @@

{{ _fsdomain('Login') }}

{{ login_user_form.hidden_tag() }} {{ render_form_errors(login_user_form) }} - {% if "email" in identity_attributes %}{{ render_field_with_errors(login_user_form.email) }}{% endif %} + {% if login_user_form.email and "email" in identity_attributes %} + {{ render_field_with_errors(login_user_form.email) }}{% endif %} {% if login_user_form.username and "username" in identity_attributes %} - {% if "email" in identity_attributes %}

{{ _fsdomain("or") }}

{% endif %} + {% if login_user_form.email and "email" in identity_attributes %}

{{ _fsdomain("or") }}

{% endif %} {{ render_field_with_errors(login_user_form.username) }} {% endif %}
{{ render_field_with_errors(login_user_form.password) }}
diff --git a/tests/conftest.py b/tests/conftest.py index 406a763d..3f8d1107 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -340,10 +340,13 @@ def revert_forms(): del app.security.forms[form_name].cls.username from flask_security import RegisterFormV2 + from flask_security.forms import PasswordConfirmFormMixin, NewPasswordFormMixin - for attr in ["username", "password", "password_confirm"]: + for attr in ["username"]: if hasattr(RegisterFormV2, attr): delattr(RegisterFormV2, attr) + RegisterFormV2.password_confirm = PasswordConfirmFormMixin.password_confirm + RegisterFormV2.password = NewPasswordFormMixin.password request.addfinalizer(revert_forms) yield app diff --git a/tests/test_basic.py b/tests/test_basic.py index a386bb00..ce6217ac 100644 --- a/tests/test_basic.py +++ b/tests/test_basic.py @@ -228,7 +228,7 @@ def test_authenticate_case_insensitive_email(app, client): def test_authenticate_with_invalid_input(client, get_message): response = client.post( "/login", - json=dict(password="password"), + json=dict(password="password", email="mememe@test.com"), headers={"Content-Type": "application/json"}, ) assert get_message("USER_DOES_NOT_EXIST") in response.data diff --git a/tests/test_misc.py b/tests/test_misc.py index 1045081f..f8fd56ca 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -34,6 +34,7 @@ logout, populate_data, reset_fresh, + get_form_input, ) from tests.test_webauthn import HackWebauthnUtil, reg_2_keys @@ -375,7 +376,6 @@ class MyRegisterForm(RegisterFormV2): security.init_app(app) client = app.test_client() - response = client.get("/login") assert b"My Login Email Address Field" in response.data @@ -1559,3 +1559,38 @@ def test_secret_key_fallbacks(app, verify_secret_key, verify_fallbacks, should_p else: with pytest.raises(BadTimeSignature): serializer.loads(token) + + +@pytest.mark.settings(username_enable=True) +def test_custom_login_form(app, sqlalchemy_datastore, get_message): + # Test custom login form that deletes email and uses username only + # Also test that is app leave 'email' in as a user identity attribute we + # will ignore it + class MyLoginForm(LoginForm): + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + del self.email # note that WTForms ends up setting self.email=None + + app.security = Security( + app, + datastore=sqlalchemy_datastore, + login_form=MyLoginForm, + ) + + populate_data(app) + client = app.test_client() + + response = client.get("/login", follow_redirects=False) + assert not get_form_input(response, "email") + + response = client.post( + "/login", json=dict(email="jill@lp.com", password="password") + ) + assert response.status_code == 400 + assert ( + get_message("USER_DOES_NOT_EXIST") + == response.json["response"]["field_errors"][""][0].encode() + ) + + response = client.post("/login", json=dict(username="jill", password="password")) + assert response.status_code == 200