Fix - improve documentation and other small bug fixes. (pallets-eco#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: pallets-eco#122

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``

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"

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

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(<object>). 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)

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
     """

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"))

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,15 +61,15 @@ 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
     assert not user.active
 
 
 def test_activate_user():
-    datastore = UserDatastore(None, None)
+    datastore = MockDatastore(None, None)
     user = User()
     user.active = False
     assert datastore.activate_user(user) is True

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):