Skip to content

Commit

Permalink
Minor change to how LoginForm is created/modified (#1053)
Browse files Browse the repository at this point in the history
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

Simplify the PASSWORD_REQUIRED logic - before we always checked for PASSWORD_REQUIRED or UNIFIED_SIGNIN - now - at init_app() time
we throw a ValueError if PASSWORD_REQUIRED==False and UNIFIED_SIGNIN feature not enabled (which is how it has always been documented).
  • Loading branch information
jwag956 authored Jan 4, 2025
1 parent 682eabe commit d24e376
Show file tree
Hide file tree
Showing 7 changed files with 162 additions and 66 deletions.
15 changes: 11 additions & 4 deletions flask_security/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -1562,6 +1562,12 @@ def init_app(
): # pragma: no cover
raise ValueError("User model must contain fs_uniquifier as of 4.0.0")

if not cv("PASSWORD_REQUIRED", app=app) and not cv("UNIFIED_SIGNIN", app=app):
raise ValueError(
"SECURITY_PASSWORD_REQUIRED can only be set to False if"
" the SECURITY_UNIFIED_SIGNIN feature is enabled"
)

# Check for pre-4.0 SECURITY_USER_IDENTITY_ATTRIBUTES format
for uia in cv("USER_IDENTITY_ATTRIBUTES", app=app): # pragma: no cover
if not isinstance(uia, dict):
Expand Down Expand Up @@ -1655,9 +1661,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
Expand Down
129 changes: 72 additions & 57 deletions flask_security/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
:copyright: (c) 2012 by Matt Wright.
:copyright: (c) 2017 by CERN.
:copyright: (c) 2019-2024 by J. Christopher Wagner (jwag).
:copyright: (c) 2019-2025 by J. Christopher Wagner (jwag).
:license: MIT, see LICENSE for more details.
"""

Expand Down Expand Up @@ -113,7 +113,7 @@ class ValidatorMixin:

def __init__(self, *args, **kwargs):
# If the message is available from config[MSG_xx] then it can be xlated.
# Otherwise it will be used as is.
# Otherwise, it will be used as is.
if "message" in kwargs:
self._original_message = kwargs["message"]
del kwargs["message"]
Expand Down Expand Up @@ -193,7 +193,7 @@ def _local_xlate(text):

def get_form_field_label(key):
"""This is called during import since form fields are declared as part of
class. Thus can't call 'localize_callback' until we need to actually
class. Thus, can't call 'localize_callback' until we need to actually
translate/render form.
"""
return make_lazy_string(_local_xlate, _default_field_labels.get(key, ""))
Expand Down Expand Up @@ -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()

Expand Down Expand Up @@ -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"))

Expand Down Expand Up @@ -526,26 +493,33 @@ 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
* username (based on :py:data:`SECURITY_USERNAME_ENABLE`)
* 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.
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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"""

Expand Down Expand Up @@ -685,7 +685,7 @@ def validate(self, **kwargs: t.Any) -> bool:

# whether a password is required is a config variable (PASSWORD_REQUIRED).
# For unified signin there are many other ways to authenticate
if cv("PASSWORD_REQUIRED") or not cv("UNIFIED_SIGNIN"):
if cv("PASSWORD_REQUIRED"):
if not self.password.data or not self.password.data.strip():
self.password.errors.append(get_message("PASSWORD_NOT_PROVIDED")[0])
failed = True
Expand Down Expand Up @@ -739,19 +739,24 @@ 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:
* email (required)
* password (required if :py:data:`SECURITY_PASSWORD_REQUIRED` is ``True``
OR :py:data:`SECURITY_UNIFIED_SIGNIN` is ``False``)
* password (required if :py:data:`SECURITY_PASSWORD_REQUIRED` is ``True``)
* password_confirm (based on :py:data:`SECURITY_PASSWORD_CONFIRM_REQUIRED`)
* username (based on :py:data:`SECURITY_USERNAME_ENABLE`)
* 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.
Expand All @@ -765,8 +770,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):
"""
Expand Down Expand Up @@ -826,11 +829,23 @@ 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):
# mark password field as Optional
fcls.password = PasswordField(
label=get_form_field_label("password"),
render_kw={"autocomplete": "new-password"},
validators=[Optional()],
)
fcls.password_confirm = PasswordField(
get_form_field_label("retype_password"),
render_kw={"autocomplete": "new-password"},
validators=[
EqualToLocalize("password", message="RETYPE_PASSWORD_MISMATCH"),
],
)
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)

Expand Down
5 changes: 3 additions & 2 deletions flask_security/templates/security/login_user.html
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@ <h1>{{ _fsdomain('Login') }}</h1>
<form action="{{ url_for_security('login') }}{{ prop_next() }}" method="post" name="login_user_form">
{{ 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 %}<h3>{{ _fsdomain("or") }}</h3>{% endif %}
{% if login_user_form.email and "email" in identity_attributes %}<h3>{{ _fsdomain("or") }}</h3>{% endif %}
{{ render_field_with_errors(login_user_form.username) }}
{% endif %}
<div class="fs-gap">{{ render_field_with_errors(login_user_form.password) }}</div>
Expand Down
5 changes: 4 additions & 1 deletion tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion tests/test_basic.py
Original file line number Diff line number Diff line change
Expand Up @@ -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="[email protected]"),
headers={"Content-Type": "application/json"},
)
assert get_message("USER_DOES_NOT_EXIST") in response.data
Expand Down
42 changes: 41 additions & 1 deletion tests/test_misc.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
logout,
populate_data,
reset_fresh,
get_form_input,
)
from tests.test_webauthn import HackWebauthnUtil, reg_2_keys

Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -1559,3 +1559,43 @@ 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 if app leave 'email' in as a user identity attribute we
# will ignore it
class MyLoginForm(LoginForm):
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="[email protected]", 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


@pytest.mark.settings(password_required=False)
def test_password_required_setting(app, sqlalchemy_datastore):
with pytest.raises(ValueError) as vex:
Security(app=app, datastore=sqlalchemy_datastore)
assert "SECURITY_PASSWORD_REQUIRED can only be" in str(vex.value)
Loading

0 comments on commit d24e376

Please sign in to comment.