Fix peewee - backported from 4.0 (#364)

Closes: #347

Author: jwag956

CHANGES.rst

@@ -30,15 +30,18 @@ Fixed
   (which is set if ``basic`` @auth_required method is used), a 401 is returned. See below
   for backwards compatibility concerns.
 
-- (:pr:`xx`) As part of figuring out issue 359 - a redirect loop was found. In release 3.3.0 code was put
+- (:pr:`362`) As part of figuring out issue 359 - a redirect loop was found. In release 3.3.0 code was put
   in to redirect to :py:data:`SECURITY_POST_LOGIN_VIEW` when GET or POST was called and the caller was already authenticated. The
   method used would honor the request ``next`` query parameter. This could cause redirect loops. The pre-3.3.0 behavior
   of redirecting to :py:data:`SECURITY_POST_LOGIN_VIEW` and ignoring the ``next`` parameter has been restored.
 
+- (:issue:`347`) Fix peewee. Turns out - due to lack of unit tests - peewee hasn't worked since 'permissions' were added in 3.3.
+  Furthermore, changes in 3.4 around get_id and alternative tokens also didn't work since peewee defines its own get_id.
+
 Compatibility Concerns
 ++++++++++++++++++++++
 
-In 3.3.0, `.Security.auth_required` was changed to add a default argument if none was given. The default
+In 3.3.0, :func:`.auth_required` was changed to add a default argument if none was given. The default
 include all current methods - ``session``, ``token``, and ``basic``. However ``basic`` really isn't like the others
 and requires that we send back a ``WWW-Authenticate`` header if authentication fails (and return a 401 and not redirect).
 ``basic`` has been removed from the default set and must once again be explicitly requested.

docs/quickstart.rst

@@ -273,6 +273,7 @@ possible using MongoEngine:
     class Role(db.Document, RoleMixin):
         name = db.StringField(max_length=80, unique=True)
         description = db.StringField(max_length=255)
+        permissions = db.StringField(max_length=255)
 
     class User(db.Document, UserMixin):
         email = db.StringField(max_length=255)
@@ -348,15 +349,18 @@ possible using Peewee:
     # Create database connection object
     db = FlaskDB(app)
 
-    class Role(db.Model, RoleMixin):
+    class Role(RoleMixin, db.Model):
         name = CharField(unique=True)
         description = TextField(null=True)
+        permissions = TextField(null=True)
 
-    class User(db.Model, UserMixin):
+    # N.B. order is important since db.Model also contains a get_id() -
+    # we need the one from UserMixin.
+    class User(UserMixin, db.Model):
         email = TextField()
         password = TextField()
         active = BooleanField(default=True)
-        fs_uniquifier = TextField()
+        fs_uniquifier = TextField(unique=True, null=False)
         confirmed_at = DateTimeField(null=True)
 
     class UserRoles(db.Model):
@@ -368,6 +372,9 @@ possible using Peewee:
         name = property(lambda self: self.role.name)
         description = property(lambda self: self.role.description)
 
+        def get_permissions(self):
+            return self.role.get_permissions()
+
     # Setup Flask-Security
     user_datastore = PeeweeUserDatastore(db, User, Role, UserRoles)
     security = Security(app, user_datastore)

flask_security/core.py

@@ -678,6 +678,9 @@ def add_permissions(self, permissions):
         Caller must commit to DB.
 
         .. versionadded:: 3.3.0
+
+        .. deprecated:: 3.4.4
+            Use :meth:`.UserDatastore.remove_permissions_from_role`
         """
         if hasattr(self, "permissions"):
             current_perms = self.get_permissions()
@@ -688,7 +691,7 @@ def add_permissions(self, permissions):
             else:
                 perms = {permissions}
             self.permissions = ",".join(current_perms.union(perms))
-        else:
+        else:  # pragma: no cover
             raise NotImplementedError("Role model doesn't have permissions")
 
     def remove_permissions(self, permissions):
@@ -700,6 +703,9 @@ def remove_permissions(self, permissions):
         Caller must commit to DB.
 
         .. versionadded:: 3.3.0
+
+        .. deprecated:: 3.4.4
+            Use :meth:`.UserDatastore.remove_permissions_from_role`
         """
         if hasattr(self, "permissions"):
             current_perms = self.get_permissions()
@@ -710,7 +716,7 @@ def remove_permissions(self, permissions):
             else:
                 perms = {permissions}
             self.permissions = ",".join(current_perms.difference(perms))
-        else:
+        else:  # pragma: no cover
             raise NotImplementedError("Role model doesn't have permissions")
 
 
@@ -791,9 +797,8 @@ def has_permission(self, permission):
 
         """
         for role in self.roles:
-            if hasattr(role, "permissions"):
-                if permission in role.get_permissions():
-                    return True
+            if permission in role.get_permissions():
+                return True
         return False
 
     def get_security_payload(self):

flask_security/datastore.py

@@ -202,6 +202,48 @@ def remove_role_from_user(self, user, role):
             self.put(user)
         return rv
 
+    def add_permissions_to_role(self, role, permissions):
+        """Add one or more permissions to role.
+
+        :param role: The role to modify. Can be a Role object or
+            string role name
+        :param permissions: a set, list, or single string.
+        :return: True if permissions added, False if role doesn't exist.
+
+        Caller must commit to DB.
+
+        .. versionadded:: 3.4.4
+        """
+
+        rv = False
+        user, role = self._prepare_role_modify_args(None, role)
+        if role:
+            rv = True
+            role.add_permissions(permissions)
+            self.put(role)
+        return rv
+
+    def remove_permissions_from_role(self, role, permissions):
+        """Remove one or more permissions from a role.
+
+        :param role: The role to modify. Can be a Role object or
+            string role name
+        :param permissions: a set, list, or single string.
+        :return: True if permissions removed, False if role doesn't exist.
+
+        Caller must commit to DB.
+
+        .. versionadded:: 3.4.4
+        """
+
+        rv = False
+        user, role = self._prepare_role_modify_args(None, role)
+        if role:
+            rv = True
+            role.remove_permissions(permissions)
+            self.put(role)
+        return rv
+
     def toggle_active(self, user):
         """Toggles a user's active status. Always returns True."""
         user.active = not user.active

tests/conftest.py

@@ -235,6 +235,7 @@ def mongoengine_setup(request, app, tmpdir, realdburl):
     class Role(db.Document, RoleMixin):
         name = db.StringField(required=True, unique=True, max_length=80)
         description = db.StringField(max_length=255)
+        permissions = db.StringField(max_length=255)
         meta = {"db_alias": db_name}
 
     class User(db.Document, UserMixin):
@@ -347,6 +348,7 @@ class Role(Base, RoleMixin):
         id = Column(Integer(), primary_key=True)
         name = Column(String(80), unique=True)
         description = Column(String(255))
+        permissions = Column(String(255))
 
     class User(Base, UserMixin):
         __tablename__ = "user"
@@ -424,12 +426,14 @@ def peewee_setup(request, app, tmpdir, realdburl):
 
     db = FlaskDB(app)
 
-    class Role(db.Model, RoleMixin):
+    class Role(RoleMixin, db.Model):
         name = CharField(unique=True, max_length=80)
         description = TextField(null=True)
+        permissions = TextField(null=True)
 
-    class User(db.Model, UserMixin):
+    class User(UserMixin, db.Model):
         email = TextField()
+        fs_uniquifier = TextField(unique=True, null=False)
         username = TextField()
         security_number = IntegerField(null=True)
         password = TextField(null=True)
@@ -455,6 +459,9 @@ class UserRoles(db.Model):
         name = property(lambda self: self.role.name)
         description = property(lambda self: self.role.description)
 
+        def get_permissions(self):
+            return self.role.get_permissions()
+
     with app.app_context():
         for Model in (Role, User, UserRoles):
             Model.drop_table()
@@ -600,6 +607,27 @@ def client_nc(request, sqlalchemy_app):
     return app.test_client(use_cookies=False)
 
 
+@pytest.fixture(params=["cl-sqlalchemy", "c2", "cl-mongo", "cl-peewee"])
+def clients(request, app, tmpdir, realdburl):
+    if request.param == "cl-sqlalchemy":
+        ds = sqlalchemy_setup(request, app, tmpdir, realdburl)
+    elif request.param == "c2":
+        ds = sqlalchemy_session_setup(request, app, tmpdir, realdburl)
+    elif request.param == "cl-mongo":
+        ds = mongoengine_setup(request, app, tmpdir, realdburl)
+    elif request.param == "cl-peewee":
+        ds = peewee_setup(request, app, tmpdir, realdburl)
+    elif request.param == "cl-pony":
+        # Not working yet.
+        ds = pony_setup(request, app, tmpdir, realdburl)
+    app.security = Security(app, datastore=ds)
+    populate_data(app)
+    if request.param == "cl-peewee":
+        # peewee is insistent on a single connection?
+        ds.db.close_db(None)
+    return app.test_client()
+
+
 @pytest.yield_fixture()
 def in_app_context(request, sqlalchemy_app):
     app = sqlalchemy_app()

tests/test_common.py

@@ -269,42 +269,42 @@ def test_unauthorized_access_with_referrer(client, get_message):
 
 
 @pytest.mark.settings(unauthorized_view="/unauthz")
-def test_roles_accepted(client):
+def test_roles_accepted(clients):
     # This specificaly tests that we can pass a URL for unauthorized_view.
     for user in ("[email protected]", "[email protected]"):
-        authenticate(client, user)
-        response = client.get("/admin_or_editor")
+        authenticate(clients, user)
+        response = clients.get("/admin_or_editor")
         assert b"Admin or Editor Page" in response.data
-        logout(client)
+        logout(clients)
 
-    authenticate(client, "[email protected]")
-    response = client.get("/admin_or_editor", follow_redirects=True)
+    authenticate(clients, "[email protected]")
+    response = clients.get("/admin_or_editor", follow_redirects=True)
     assert b"Unauthorized" in response.data
 
 
 @pytest.mark.settings(unauthorized_view="unauthz")
-def test_permissions_accepted(client):
+def test_permissions_accepted(clients):
     for user in ("[email protected]", "[email protected]"):
-        authenticate(client, user)
-        response = client.get("/admin_perm")
+        authenticate(clients, user)
+        response = clients.get("/admin_perm")
         assert b"Admin Page with full-write or super" in response.data
-        logout(client)
+        logout(clients)
 
-    authenticate(client, "[email protected]")
-    response = client.get("/admin_perm", follow_redirects=True)
+    authenticate(clients, "[email protected]")
+    response = clients.get("/admin_perm", follow_redirects=True)
     assert b"Unauthorized" in response.data
 
 
 @pytest.mark.settings(unauthorized_view="unauthz")
-def test_permissions_required(client):
+def test_permissions_required(clients):
     for user in ["[email protected]"]:
-        authenticate(client, user)
-        response = client.get("/admin_perm_required")
+        authenticate(clients, user)
+        response = clients.get("/admin_perm_required")
         assert b"Admin Page required" in response.data
-        logout(client)
+        logout(clients)
 
-    authenticate(client, "[email protected]")
-    response = client.get("/admin_perm_required", follow_redirects=True)
+    authenticate(clients, "[email protected]")
+    response = clients.get("/admin_perm_required", follow_redirects=True)
     assert b"Unauthorized" in response.data
 
 
@@ -315,15 +315,15 @@ def test_unauthenticated_role_required(client, get_message):
 
 
 @pytest.mark.settings(unauthorized_view="unauthz")
-def test_multiple_role_required(client):
+def test_multiple_role_required(clients):
     for user in ("[email protected]", "[email protected]"):
-        authenticate(client, user)
-        response = client.get("/admin_and_editor", follow_redirects=True)
+        authenticate(clients, user)
+        response = clients.get("/admin_and_editor", follow_redirects=True)
         assert b"Unauthorized" in response.data
-        client.get("/logout")
+        clients.get("/logout")
 
-    authenticate(client, "[email protected]")
-    response = client.get("/admin_and_editor", follow_redirects=True)
+    authenticate(clients, "[email protected]")
+    response = clients.get("/admin_and_editor", follow_redirects=True)
     assert b"Admin and Editor Page" in response.data
 
 

tests/test_datastore.py

@@ -258,6 +258,7 @@ def test_create_user_with_roles_and_permissions(app, datastore):
         user = datastore.find_user(email="[email protected]")
         assert user.has_role("test1") is True
         assert user.has_permission("read") is True
+        assert user.has_permission("write") is False
 
 
 def test_permissions_strings(app, datastore):
@@ -305,20 +306,22 @@ def test_modify_permissions(app, datastore):
 
         t1 = ds.find_role("test1")
         assert perms == t1.get_permissions()
-        orig_update_time = t1.update_datetime
-        assert t1.update_datetime <= datetime.datetime.utcnow()
+        if hasattr(t1, "update_datetime"):
+            orig_update_time = t1.update_datetime
+            assert t1.update_datetime <= datetime.datetime.utcnow()
 
-        t1.add_permissions("execute")
+        ds.add_permissions_to_role(t1, "execute")
         ds.commit()
 
         t1 = ds.find_role("test1")
         assert perms.union({"execute"}) == t1.get_permissions()
 
-        t1.remove_permissions("read")
+        ds.remove_permissions_from_role(t1, "read")
         ds.commit()
         t1 = ds.find_role("test1")
         assert {"write", "execute"} == t1.get_permissions()
-        assert t1.update_datetime > orig_update_time
+        if hasattr(t1, "update_datetime"):
+            assert t1.update_datetime > orig_update_time
 
 
 def test_get_permissions(app, datastore):
@@ -350,13 +353,13 @@ def test_modify_permissions_multi(app, datastore):
         assert {"read", "write"} == t1.get_permissions()
 
         # send in a list
-        t1.add_permissions(["execute", "whatever"])
+        ds.add_permissions_to_role(t1, ["execute", "whatever"])
         ds.commit()
 
         t1 = ds.find_role("test1")
         assert {"read", "write", "execute", "whatever"} == t1.get_permissions()
 
-        t1.remove_permissions(["read", "whatever"])
+        ds.remove_permissions_from_role(t1, ["read", "whatever"])
         ds.commit()
         assert {"write", "execute"} == t1.get_permissions()
 
@@ -366,13 +369,13 @@ def test_modify_permissions_multi(app, datastore):
         ds.commit()
 
         t2 = ds.find_role("test2")
-        t2.add_permissions({"execute", "whatever"})
+        ds.add_permissions_to_role(t2, {"execute", "whatever"})
         ds.commit()
 
         t2 = ds.find_role("test2")
         assert {"read", "write", "execute", "whatever"} == t2.get_permissions()
 
-        t2.remove_permissions({"read", "whatever"})
+        ds.remove_permissions_from_role(t2, {"read", "whatever"})
         ds.commit()
         assert {"write", "execute"} == t2.get_permissions()