Skip to content

Commit

Permalink
Improve empty password support. (pallets-eco#627)
Browse files Browse the repository at this point in the history
Previously, if a user registered w/o a passwd (which was allowed if UNIFIED_SIGNIN was enabled) we put in a non-guessable password in the password User record. This meant that after registration, there was no way to tell if a user had a legitimate password or not. Not great.

Now - change User DB model to allow the password field to be nullable.
Change registration and change endpoints to properly handle an empty password.

Add a config variable - PASSWORD_REQUIRED (default True) that selects whether an empty password is allowed.

Enhance change template to properly show or hide the 'current' password. Note that for users that don't have passwords, the /change endpoint is protected via a freshness check.

Fix small bug that GET /change with json didn't work.
  • Loading branch information
jwag956 authored May 31, 2022
1 parent ebbc0e9 commit 12f6f9b
Show file tree
Hide file tree
Showing 20 changed files with 214 additions and 63 deletions.
22 changes: 17 additions & 5 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,15 @@ Fixes
- (:issue:`531`) Get rid of Flask-Mail. Flask-Mailman is now the default preferred email package.
Flask-Mail is still supported so there should be no backwards compatability issues.
- (:issue:`597`) A delete option has been added to us-setup (form and view).
- (:pr:`xxx`) Improve username support - the LoginForm now has a separate field for username if
- (:pr:`625`) Improve username support - the LoginForm now has a separate field for username if
``SECURITY_USERNAME_ENABLE`` is True, and properly displays input fields only if the associated
field is an identity attribute (as specified by :py:data:`SECURITY_USER_IDENTITY_ATTRIBUTES`)
- (:pr:`xxx`) Improve empty password handling. Prior, an unguessable password was set into the user
record when a user registered without a password - now, the DB user model has been changed to
allow nullable passwords. This provides a better user experience since Flask-Security now
knows if a user has an empty password or not. Since registering without a password is not
a mainstream feature, a new configuration variable :py:data:`SECURITY_PASSWORD_REQUIRED`
has been added (defaults to ``True``).

Backward Compatibility Concerns
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Expand All @@ -49,23 +55,25 @@ For unified signin:
- In ``us-verify`` the 'code_methods' item now lists just active/setup methods that generate a code
not ALL possible methods that generate a code.
- ``SECURITY_US_VERIFY_SEND_CODE_URL`` and ``SECURITY_US_SIGNIN_SEND_CODE_URL`` endpoints are now POST only.
- Empty passwords were always permitted when ``SECURITY_UNIFIED_SIGNIN`` was enabled - now an additional configuration
variable ``SECURITY_PASSWORD_REQUIRED`` must be set to False.

Login:

- Since the beginning of time, the flask-security login form has accepted any input in the
'email' field, and used that to check if it corresponds to any field in ``SECURITY_USER_IDENTITY_ATTRIBUTES``.
This has always been problematic and confusing - and with the addition of HTML attributes for various
form fields - having a field with multiple possible inputs is no longer a viable user experience.
This is no longer supported, and the LoginForm now declares the ``email`` field to be of type ``EmailFormMixin``
which requires a valid (after normalization) and existing email address. The most common usage of this legacy feature was to allow
an email or username - Flask-Security now has core support for a ``username`` option.
This is no longer supported, and the LoginForm now declares the ``email`` field to be of type ``EmailField``
which requires a valid (after normalization). The most common usage of this legacy feature was to allow
an email or username - Flask-Security now has core support for a ``username`` option - see :py:data:`SECURITY_USERNAME_ENABLE`.
Please see :ref:`custom_login_form` for an example of how to replicate the legacy behavior.
- Some error messages have changed - ``USER_DOES_NOT_EXIST`` is now returned for any identity error including an empty value.

Other:

- A very old piece of code in registrable, would immediately commit to the DB when a new user was created.
It now does what all over views due, and have the caller responsible for committing the transaction - usually by
It is now consistent with all other views, and has the caller responsible for committing the transaction - usually by
setting up a flask ``after_this_request`` action. This could affect an application that captured the registration signal
and stored the ``user`` object for later use - this user object would likely be invalid after the request is finished.
- Some fields have custom HTML attributes attached to them (e.g. autocomplete, type, etc). These are stored as part of the
Expand All @@ -90,6 +98,10 @@ If you are using Alembic the schema migration is easy::

op.add_column('user', sa.Column('fs_webauthn_user_handle', sa.String(length=64), nullable=True))


If you want to allow for empty passwords as part of registration then set :py:data:`SECURITY_PASSWORD_REQUIRED` to ``False``.
In addition you need to change your DB schema to allow the ``password`` field to be nullable.

Version 4.1.4
-------------

Expand Down
15 changes: 14 additions & 1 deletion docs/configuration.rst
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,18 @@ These configuration keys are used globally across all features.

.. _5.1.1.2 Memorized Secret Verifiers: https://pages.nist.gov/800-63-3/sp800-63b.html#sec5

.. py:data:: SECURITY_PASSWORD_REQUIRED
If set to ``False`` then a user can register with an empty password.
This requires :py:data:`SECURITY_UNIFIED_SIGNIN` to be enabled. By
default, the user will be able to authenticate using an email link.
Please note: this does not mean a user can sign in with an empty
password - it means that they must have some OTHER means of authenticating.

Default: ``True``

.. versionadded:: 5.0.0

.. py:data:: SECURITY_TOKEN_AUTHENTICATION_KEY
Specifies the query string parameter to read when using token authentication.
Expand Down Expand Up @@ -1275,6 +1287,7 @@ Unified Signin
Additional relevant configuration variables:

* :py:data:`SECURITY_USER_IDENTITY_ATTRIBUTES` - Defines the order and methods for parsing and validating identity.
* :py:data:`SECURITY_PASSWORD_REQUIRED` - Can a user register w/o a password?
* :py:data:`SECURITY_DEFAULT_REMEMBER_ME`
* :py:data:`SECURITY_SMS_SERVICE` - When SMS is enabled in :py:data:`SECURITY_US_ENABLED_METHODS`.
* :py:data:`SECURITY_SMS_SERVICE_CONFIG`
Expand Down Expand Up @@ -1597,7 +1610,7 @@ The default messages and error levels can be found in ``core.py``.
* ``SECURITY_MSG_PASSWORD_IS_THE_SAME``
* ``SECURITY_MSG_PASSWORD_MISMATCH``
* ``SECURITY_MSG_PASSWORD_NOT_PROVIDED``
* ``SECURITY_MSG_PASSWORD_NOT_SET``
* ``SECURITY_MSG_PASSWORD_REQUIRED``
* ``SECURITY_MSG_PASSWORD_RESET``
* ``SECURITY_MSG_PASSWORD_RESET_EXPIRED``
* ``SECURITY_MSG_PASSWORD_RESET_REQUEST``
Expand Down
2 changes: 1 addition & 1 deletion docs/models.rst
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ At the bare minimum your `User` and `Role` model should include the following fi

* primary key
* ``email`` (for most features - unique, non-nullable)
* ``password`` (non-nullable)
* ``password`` (string)
* ``active`` (boolean, non-nullable)
* ``fs_uniquifier`` (string, 64 bytes, unique, non-nullable)

Expand Down
12 changes: 12 additions & 0 deletions docs/openapi.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,18 @@ paths:
text/html:
schema:
example: render_template(SECURITY_CHANGE_PASSWORD_TEMPLATE)
application/json:
schema:
allOf:
- $ref: '#/components/schemas/DefaultJsonResponse'
- type: object
properties:
response:
type: object
properties:
active_password:
type: boolean
description: Does user already have a password?
post:
summary: Change password
parameters:
Expand Down
18 changes: 11 additions & 7 deletions flask_security/changeable.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@
flask_security.changeable
~~~~~~~~~~~~~~~~~~~~~~~~~
Flask-Security recoverable module
Flask-Security change password module
:copyright: (c) 2012 by Matt Wright.
:copyright: (c) 2019-2021 by J. Christopher Wagner (jwag).
:copyright: (c) 2019-2022 by J. Christopher Wagner (jwag).
:author: Eskil Heyn Olsen
:license: MIT, see LICENSE for more details.
"""
Expand All @@ -16,7 +16,7 @@

from .proxies import _datastore
from .signals import password_changed
from .utils import config_value, hash_password, login_user, send_mail
from .utils import config_value as cv, hash_password, login_user, send_mail

if t.TYPE_CHECKING: # pragma: no cover
from .datastore import User
Expand All @@ -27,13 +27,13 @@ def send_password_changed_notice(user):
:param user: The user to send the notice to
"""
if config_value("SEND_PASSWORD_CHANGE_EMAIL"):
subject = config_value("EMAIL_SUBJECT_PASSWORD_CHANGE_NOTICE")
if cv("SEND_PASSWORD_CHANGE_EMAIL"):
subject = cv("EMAIL_SUBJECT_PASSWORD_CHANGE_NOTICE")
send_mail(subject, user.email, "change_notice", user=user)


def change_user_password(
user: "User", password: str, notify: bool = True, autologin: bool = True
user: "User", password: t.Optional[str], notify: bool = True, autologin: bool = True
) -> None:
"""Change the specified user's password
Expand All @@ -42,7 +42,11 @@ def change_user_password(
:param notify: if True send notification (if configured) to user
:param autologin: if True, login user
"""
user.password = hash_password(password)

if password:
user.password = hash_password(password)
else:
user.password = None
# Change uniquifier - this will cause ALL sessions to be invalidated.
_datastore.set_uniquifier(user)
_datastore.put(user)
Expand Down
2 changes: 1 addition & 1 deletion flask_security/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@
"PASSWORD_CHECK_BREACHED": False,
"PASSWORD_BREACHED_COUNT": 1,
"PASSWORD_NORMALIZE_FORM": "NFKD",
"PASSWORD_REQUIRED": True,
"DEPRECATED_PASSWORD_SCHEMES": ["auto"],
"LOGIN_URL": "/login",
"LOGOUT_URL": "/logout",
Expand Down Expand Up @@ -412,7 +413,6 @@
"INVALID_EMAIL_ADDRESS": (_("Invalid email address"), "error"),
"INVALID_CODE": (_("Invalid code"), "error"),
"PASSWORD_NOT_PROVIDED": (_("Password not provided"), "error"),
"PASSWORD_NOT_SET": (_("No password is set for this user"), "error"),
"PASSWORD_INVALID_LENGTH": (
_("Password must be at least %(length)s characters"),
"error",
Expand Down
2 changes: 1 addition & 1 deletion flask_security/datastore.py
Original file line number Diff line number Diff line change
Expand Up @@ -1059,7 +1059,7 @@ class User(UserMixin):
id: int
email: str
username: t.Optional[str]
password: str
password: t.Optional[str]
active: bool
fs_uniquifier: str
fs_token_uniquifier: str
Expand Down
38 changes: 25 additions & 13 deletions flask_security/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
localize_callback,
url_for_security,
validate_redirect_url,
verify_password,
)

if t.TYPE_CHECKING: # pragma: no cover
Expand Down Expand Up @@ -502,8 +503,8 @@ def validate(self, **kwargs: t.Any) -> bool:
hash_password(self.password.data)
return False
if not self.user.password:
# Not sure this can ever happen
self.password.errors.append(get_message("PASSWORD_NOT_SET")[0])
# This is result of PASSWORD_REQUIRED=False and UNIFIED_SIGNIN
self.password.errors.append(get_message("INVALID_PASSWORD")[0])
# Reduce timing variation between existing and non-existing users
hash_password(self.password.data)
return False
Expand Down Expand Up @@ -558,9 +559,9 @@ def validate(self, **kwargs: t.Any) -> bool:
if not super().validate(**kwargs):
return False

# To support unified sign in - we permit registering with no password.
if not cv("UNIFIED_SIGNIN"):
# password required
# 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 not self.password.data or not self.password.data.strip():
self.password.errors.append(get_message("PASSWORD_NOT_PROVIDED")[0])
return False
Expand Down Expand Up @@ -633,9 +634,12 @@ def validate(self, **kwargs: t.Any) -> bool:
return True


class ChangePasswordForm(Form, PasswordFormMixin):
class ChangePasswordForm(Form):
"""The default change password form"""

password = PasswordField(
get_form_field_label("password"), render_kw={"autocomplete": "current-password"}
)
new_password = PasswordField(
get_form_field_label("new_password"),
render_kw={"autocomplete": "new-password"},
Expand All @@ -657,13 +661,21 @@ def validate(self, **kwargs: t.Any) -> bool:
if not super().validate(**kwargs):
return False

self.password.data = _security._password_util.normalize(self.password.data)
if not current_user.verify_and_update_password(self.password.data):
self.password.errors.append(get_message("INVALID_PASSWORD")[0])
return False
if self.password.data == self.new_password.data:
self.password.errors.append(get_message("PASSWORD_IS_THE_SAME")[0])
return False
# If user doesn't have a password then the caller (view) has already
# verified a current fresh session.
if current_user.password:
if not self.password.data or not self.password.data.strip():
self.password.errors.append(get_message("PASSWORD_NOT_PROVIDED")[0])
return False

self.password.data = _security._password_util.normalize(self.password.data)
if not verify_password(current_user.password, self.password.data):
self.password.errors.append(get_message("INVALID_PASSWORD")[0])
return False
if self.password.data == self.new_password.data:
self.password.errors.append(get_message("PASSWORD_IS_THE_SAME")[0])
return False

pbad, self.new_password.data = _security._password_util.validate(
self.new_password.data, False, user=current_user
)
Expand Down
4 changes: 4 additions & 0 deletions flask_security/models/fsqla_v3.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,10 @@ def webauthn(cls):
# 2FA - one time recovery codes - comma separated.
tf_recovery_codes = Column(AsaList(1024), nullable=True)

# Change password to nullable so we can tell after registration whether
# a user has a password or not.
password = Column(String(255), nullable=True)

# This is repeated since I couldn't figure out how to have it reference the
# new version of FsModels.
@declared_attr
Expand Down
25 changes: 9 additions & 16 deletions flask_security/registerable.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,16 @@
Flask-Security registerable module
:copyright: (c) 2012 by Matt Wright.
:copyright: (c) 2019-2021 by J. Christopher Wagner (jwag).
:copyright: (c) 2019-2022 by J. Christopher Wagner (jwag).
:license: MIT, see LICENSE for more details.
"""

import uuid

from flask import current_app as app

from .confirmable import generate_confirmation_link
from .proxies import _security, _datastore
from .signals import user_registered
from .utils import config_value, do_flash, get_message, hash_password, send_mail
from .utils import config_value as cv, do_flash, get_message, hash_password, send_mail


def register_user(registration_form):
Expand All @@ -28,19 +26,14 @@ def register_user(registration_form):
"""

user_model_kwargs = registration_form.to_dict(only_user=True)
blank_password = not user_model_kwargs["password"]

if blank_password:
# For no password - set an unguessable password.
# Since we still allow 'plaintext' as a password scheme - can't use a simple
# sentinel.
user_model_kwargs["password"] = "NoPassword-" + uuid.uuid4().hex

user_model_kwargs["password"] = hash_password(user_model_kwargs["password"])
# passwords are always required - with UNIFIED_SIGNIN
if user_model_kwargs["password"]:
user_model_kwargs["password"] = hash_password(user_model_kwargs["password"])
user = _datastore.create_user(**user_model_kwargs)

# if they didn't give a password - auto-setup email magic links.
if blank_password:
# if they didn't give a password - auto-setup email magic links (if UNIFIED SIGNIN)
if not user_model_kwargs["password"] and cv("UNIFIED_SIGNIN"):
_datastore.us_setup_email(user)

confirmation_link, token = None, None
Expand All @@ -56,9 +49,9 @@ def register_user(registration_form):
form_data=registration_form.to_dict(only_user=False),
)

if config_value("SEND_REGISTER_EMAIL"):
if cv("SEND_REGISTER_EMAIL"):
send_mail(
config_value("EMAIL_SUBJECT_REGISTER"),
cv("EMAIL_SUBJECT_REGISTER"),
user.email,
"welcome",
user=user,
Expand Down
6 changes: 5 additions & 1 deletion flask_security/templates/security/change_password.html
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@
<h1>{{ _fsdomain('Change password') }}</h1>
<form action="{{ url_for_security('change_password') }}" method="POST" name="change_password_form">
{{ change_password_form.hidden_tag() }}
{{ render_field_with_errors(change_password_form.password) }}
{% if active_password %}
{{ render_field_with_errors(change_password_form.password) }}
{% else %}
<h3>{{ _fsdomain('You do not currently have a password - this will add one.') }}</h3>
{% endif %}
{{ render_field_with_errors(change_password_form.new_password) }}
{{ render_field_with_errors(change_password_form.new_password_confirm) }}
{{ render_field(change_password_form.submit) }}
Expand Down
2 changes: 1 addition & 1 deletion flask_security/templates/security/email/change_notice.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{{ _fsdomain('Your password has been changed') }}
{{ _fsdomain('Your password has been changed.') }}
{% if security.recoverable %}
{{ _fsdomain('If you did not change your password, click the link below to reset it.') }}
{{ url_for_security('forgot_password', _external=True) }}
Expand Down
Loading

0 comments on commit 12f6f9b

Please sign in to comment.