Skip to content

Commit

Permalink
Minor change to how LoginForm is created/modified
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
  • Loading branch information
jwag956 committed Jan 3, 2025
1 parent 682eabe commit c72058d
Show file tree
Hide file tree
Showing 6 changed files with 109 additions and 60 deletions.
9 changes: 5 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 @@ -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
Expand Down
111 changes: 60 additions & 51 deletions flask_security/forms.py
Original file line number Diff line number Diff line change
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 @@ -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:
Expand All @@ -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.
Expand All @@ -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):
"""
Expand Down Expand Up @@ -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(

Check warning on line 836 in flask_security/forms.py

View check run for this annotation

Codecov / codecov/patch

flask_security/forms.py#L836

Added line #L836 was not covered by tests
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)

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
37 changes: 36 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,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="[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

0 comments on commit c72058d

Please sign in to comment.