From 00d9d9b19b828e81e1cd2f01257cd6820dee0d80 Mon Sep 17 00:00:00 2001 From: Chris Wagner Date: Tue, 9 Jul 2019 18:33:02 -0700 Subject: [PATCH] Fix - improve documentation and other small bug fixes. (#123) - Improve documentation for :meth:`.UserDatastore.create_user` to make clear that hashed password should be passed in. - Improve documentation for :class:`.UserDatastore` and :func:`.verify_and_update_password` to make clear that caller must commit changers to DB if using a session based datastore. - (:issue:`122`) Clarify when to use ``confirm_register_form`` rather than ``register_form``. - Fix bug in 2FA that didn't commit DB after using `verify_and_update_password`. - Fix bug(s) in UserDatastore where changes to user ``active`` flag weren't being added to DB. closes: #122 --- CHANGES.rst | 10 ++++++++- docs/conf.py | 4 ++-- flask_security/core.py | 29 +++++++++++++++++++++----- flask_security/datastore.py | 41 +++++++++++++++++++++++++++++-------- flask_security/utils.py | 10 ++++++++- flask_security/views.py | 2 ++ tests/test_datastore.py | 14 ++++++++++--- tests/test_hashing.py | 2 +- 8 files changed, 90 insertions(+), 22 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 845becbe..0e594350 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -10,10 +10,18 @@ Released TBD - (:pr:`120`) Native support for Permissions as part of Roles. Endpoints can be protected via permissions that are evaluated based on role(s) that the user has. +- Improve documentation for :meth:`.UserDatastore.create_user` to make clear that hashed password + should be passed in. +- Improve documentation for :class:`.UserDatastore` and :func:`.verify_and_update_password` + to make clear that caller must commit changers to DB if using a session based datastore. +- (:issue:`122`) Clarify when to use ``confirm_register_form`` rather than ``register_form``. +- Fix bug in 2FA that didn't commit DB after using `verify_and_update_password`. +- Fix bug(s) in UserDatastore where changes to user ``active`` flag weren't being + added to DB. Possible compatibility issues: -- (:pr:`120` RoleMixin now has a method ``get_permissions`` which is called as part +- (:pr:`120`) :class:`.RoleMixin` now has a method :meth:`.get_permissions` which is called as part each request to add Permissions to the authenticated user. It checks if the RoleModel has a property ``permissions`` and assumes it is a comma separated string of permissions. If your model already has such a property this will likely fail. You need to override ``get_permissions`` diff --git a/docs/conf.py b/docs/conf.py index 29eee934..96830a24 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -312,8 +312,8 @@ # Example configuration for intersphinx: refer to the Python standard library. -intersphinx_mapping = {"http://docs.python.org/": None} +intersphinx_mapping = {"https://docs.python.org/3": None} -# -- Options for spinx-issues --------------------------------------------- +# -- Options for sphinx-issues --------------------------------------------- # Github repo issues_github_path = "jwag956/flask-security" diff --git a/flask_security/core.py b/flask_security/core.py index 516da3cc..97d3c36b 100644 --- a/flask_security/core.py +++ b/flask_security/core.py @@ -171,8 +171,8 @@ "EMAIL_SUBJECT_PASSWORD_RESET": _("Password reset instructions"), "EMAIL_PLAINTEXT": True, "EMAIL_HTML": True, - "EMAIL_SUBJECT_TWO_FACTOR": "Two-factor Login", - "EMAIL_SUBJECT_TWO_FACTOR_RESCUE": "Two-factor Rescue", + "EMAIL_SUBJECT_TWO_FACTOR": _("Two-factor Login"), + "EMAIL_SUBJECT_TWO_FACTOR_RESCUE": _("Two-factor Rescue"), "USER_IDENTITY_ATTRIBUTES": ["email"], "HASHING_SCHEMES": ["sha256_crypt", "hex_md5"], "DEPRECATED_HASHING_SCHEMES": ["hex_md5"], @@ -589,7 +589,20 @@ def get_redirect_qparams(self, existing=None): return existing def verify_and_update_password(self, password): - """Verify and update user password using configured hash.""" + """Returns ``True`` if the password is valid for the specified user. + + Additionally, the hashed password in the database is updated if the + hashing algorithm happens to have changed. + + N.B. you MUST call DB commit if you are using a session-based datastore + (such as SqlAlchemy) since the user instance might have been altered + (i.e. ``app.security.datastore.commit()``). + This is usually handled in the view. + + .. versionadded:: 3.2.0 + + :param password: A plaintext password to verify + """ return verify_and_update_password(password, self) @@ -673,13 +686,19 @@ class Security(object): :param datastore: An instance of a user datastore. :param register_blueprint: to register the Security blueprint or not. :param login_form: set form for the login view - :param register_form: set form for the register view - :param confirm_register_form: set form for the confirm register view + :param register_form: set form for the register view when + SECURITY_CONFIRMABLE is false + :param confirm_register_form: set form for the register view when + SECURITY_CONFIRMABLE is true :param forgot_password_form: set form for the forgot password view :param reset_password_form: set form for the reset password view :param change_password_form: set form for the change password view :param send_confirmation_form: set form for the send confirmation view :param passwordless_login_form: set form for the passwordless login view + :param two_factor_setup_form: set form for the 2FA setup view + :param two_factor_verify_code_form: set form the the 2FA verify code view + :param two_factor_rescue_form: set form for the 2FA rescue view + :param two_factor_verify_password_form: set form for the 2FA verify password view :param anonymous_user: class to use for anonymous user :param render_template: function to use to render templates :param send_mail: function to use to send email diff --git a/flask_security/datastore.py b/flask_security/datastore.py index 6a0bd421..e2ad4f85 100644 --- a/flask_security/datastore.py +++ b/flask_security/datastore.py @@ -6,6 +6,7 @@ This module contains an user datastore classes. :copyright: (c) 2012 by Matt Wright. + :copyright: (c) 2019 by J. Christopher Wagner (jwag). :license: MIT, see LICENSE for more details. """ @@ -117,6 +118,11 @@ class UserDatastore(object): :param user_model: A user model class definition :param role_model: A role model class definition + + Be aware that for mutating operations, the user/role will be added to the + datastore (by calling self.put(). If the datastore is session based + (such as for SQLAlchemyDatastore) it is up to caller to actually + commit the transaction by calling datastore.commit(). """ def __init__(self, user_model, role_model): @@ -162,8 +168,9 @@ def find_role(self, *args, **kwargs): def add_role_to_user(self, user, role): """Adds a role to a user. - :param user: The user to manipulate - :param role: The role to add to the user + :param user: The user to manipulate. Can be an User object or email + :param role: The role to add to the user. Can be a Role object or + string role name """ user, role = self._prepare_role_modify_args(user, role) if role not in user.roles: @@ -175,8 +182,9 @@ def add_role_to_user(self, user, role): def remove_role_from_user(self, user, role): """Removes a role from a user. - :param user: The user to manipulate - :param role: The role to remove from the user + :param user: The user to manipulate. Can be an User object or email + :param role: The role to remove from the user. Can be a Role object or + string role name """ rv = False user, role = self._prepare_role_modify_args(user, role) @@ -189,6 +197,7 @@ def remove_role_from_user(self, user, role): def toggle_active(self, user): """Toggles a user's active status. Always returns True.""" user.active = not user.active + self.put(user) return True def deactivate_user(self, user): @@ -198,6 +207,7 @@ def deactivate_user(self, user): """ if user.active: user.active = False + self.put(user) return True return False @@ -208,6 +218,7 @@ def activate_user(self, user): """ if not user.active: user.active = True + self.put(user) return True return False @@ -216,10 +227,12 @@ def create_role(self, **kwargs): Creates and returns a new role from the given parameters. Supported params (depending on RoleModel): - :param name: Role name - :param permissions: a comma delimited list of permissions, a set or a list. - These are user-defined - strings that correspond to strings used with @permissions_required() + :kwparam name: Role name + :kwparam permissions: a comma delimited list of permissions, a set or a list. + These are user-defined strings that correspond to strings used with + @permissions_required() + + .. versionadded:: 3.3.0 """ @@ -242,7 +255,17 @@ def find_or_create_role(self, name, **kwargs): return self.find_role(name) or self.create_role(**kwargs) def create_user(self, **kwargs): - """Creates and returns a new user from the given parameters.""" + """Creates and returns a new user from the given parameters. + + :kwparam email: required. + :kwparam password: Hashed password. Be aware that whatever is passed in will + be stored directly in the DB. Do NOT pass in a plaintext password! + Best practice is to pass in hash_password(plaintext_password). + :kwparam roles: list of roles to be added to user. + Can be Role objects or strings + + The new user's ``active`` property will be set to true. + """ kwargs = self._prepare_create_user_args(**kwargs) user = self.user_model(**kwargs) return self.put(user) diff --git a/flask_security/utils.py b/flask_security/utils.py index 14ab0862..2a9e1fa4 100644 --- a/flask_security/utils.py +++ b/flask_security/utils.py @@ -109,9 +109,12 @@ def login_user(user, remember=None): def logout_user(): - """Logs out the current. + """Logs out the current user. This will also clean up the remember me cookie if it exists. + + This sends an ``identity_changed`` signal to note that the current + identity is now the `AnonymousIdentity` """ for key in ("identity.name", "identity.auth_type"): @@ -160,6 +163,11 @@ def verify_and_update_password(password, user): Additionally, the hashed password in the database is updated if the hashing algorithm happens to have changed. + N.B. you MUST call DB commit if you are using a session-based datastore + (such as SqlAlchemy) since the user instance might have been altered + (i.e. ``app.security.datastore.commit()``). + This is usually handled in the view. + :param password: A plaintext password to verify :param user: The user to verify against """ diff --git a/flask_security/views.py b/flask_security/views.py index 5e38269d..2ba0722a 100644 --- a/flask_security/views.py +++ b/flask_security/views.py @@ -838,6 +838,8 @@ def two_factor_verify_password(): form = form_class() if form.validate_on_submit(): + # form called verify_and_update_password() + after_this_request(_commit) session["tf_confirmed"] = True if not request.is_json: do_flash(*get_message("TWO_FACTOR_PASSWORD_CONFIRMATION_DONE")) diff --git a/tests/test_datastore.py b/tests/test_datastore.py index ae7b4f3d..99a613fc 100644 --- a/tests/test_datastore.py +++ b/tests/test_datastore.py @@ -22,6 +22,14 @@ class Role(RoleMixin): pass +class MockDatastore(UserDatastore): + def put(self, model): + pass + + def delete(self, model): + pass + + def test_unimplemented_datastore_methods(): datastore = Datastore(None) assert datastore.db is None @@ -43,7 +51,7 @@ def test_unimplemented_user_datastore_methods(): def test_toggle_active(): - datastore = UserDatastore(None, None) + datastore = MockDatastore(None, None) user = User() user.active = True assert datastore.toggle_active(user) is True @@ -53,7 +61,7 @@ def test_toggle_active(): def test_deactivate_user(): - datastore = UserDatastore(None, None) + datastore = MockDatastore(None, None) user = User() user.active = True assert datastore.deactivate_user(user) is True @@ -61,7 +69,7 @@ def test_deactivate_user(): def test_activate_user(): - datastore = UserDatastore(None, None) + datastore = MockDatastore(None, None) user = User() user.active = False assert datastore.activate_user(user) is True diff --git a/tests/test_hashing.py b/tests/test_hashing.py index b7e54a05..6b9b546f 100644 --- a/tests/test_hashing.py +++ b/tests/test_hashing.py @@ -94,7 +94,7 @@ def test_verify_password_bcrypt_rounds_too_low(app, sqlalchemy_datastore): "SECURITY_PASSWORD_HASH_OPTIONS": {"bcrypt": {"rounds": 3}}, } ) - assert all(s in str(exc_msg) for s in ["rounds", "too low"]) + assert all(s in str(exc_msg.value) for s in ["rounds", "too low"]) def test_login_with_bcrypt_enabled(app, sqlalchemy_datastore):