Support for grants_via declarations using columns instead of strings (#…

…285)

.pre-commit-config.yaml

@@ -14,13 +14,13 @@ repos:
             '--remove-duplicate-keys',
           ]
   - repo: https://github.com/pre-commit/mirrors-isort
-    rev: v4.3.21
+    rev: v5.4.2
     hooks:
       - id: isort
         additional_dependencies:
           - toml
   - repo: https://github.com/ambv/black
-    rev: master
+    rev: 19.10b0
     hooks:
       - id: black
   - repo: https://github.com/PyCQA/bandit
@@ -29,7 +29,7 @@ repos:
       - id: bandit
         args: ["--ini", "setup.cfg", "-l", "-x", "tests, build/lib"]
   - repo: https://github.com/pre-commit/pre-commit-hooks
-    rev: v3.0.1
+    rev: v3.2.0
     hooks:
       - id: check-added-large-files
       - id: check-ast
@@ -55,7 +55,7 @@ repos:
       - id: requirements-txt-fixer
       - id: trailing-whitespace
   - repo: https://gitlab.com/pycqa/flake8
-    rev: 3.8.1
+    rev: 3.8.3
     hooks:
       - id: flake8
         additional_dependencies:

coaster/sqlalchemy/roles.py

@@ -118,9 +118,6 @@ def roles_for(self, actor=None, anchors=()):
                 roles.add('owner')
             return roles
 """
-# TODO: After moving to Py3 only, replace abc.Iterable with abc.Collection. Iterable
-# also applies to strings, which we do not want in the use case here. This will also
-# obsolete the `is_collection` function.
 
 from __future__ import absolute_import
 import six
@@ -130,6 +127,7 @@ def roles_for(self, actor=None, anchors=()):
 from copy import deepcopy
 from functools import wraps
 from itertools import chain
+import operator
 import warnings
 
 from sqlalchemy import event, inspect
@@ -163,6 +161,18 @@ def roles_for(self, actor=None, anchors=()):
 __cache__ = {}
 
 
+def _attrs_equal(lhs, rhs):
+    """
+    Helper function to compare two strings or two QueryableAttributes.
+    QueryableAttributes can't be compared with `==` to confirm both are same object.
+    But strings can't be compared with `is` to confirm they are the same string.
+    We have to change the operator based on types being compared.
+    """
+    if isinstance(lhs, six.string_types) and isinstance(rhs, six.string_types):
+        return lhs == rhs
+    return lhs is rhs
+
+
 def _actor_in_relationship(actor, relationship):
     """Test whether the given actor is present in the given attribute"""
     if actor == relationship:
@@ -199,10 +209,13 @@ def _roles_via_relationship(actor, relationship, actor_attr, roles, offer_map):
     # to the actor.
     if isinstance(relationship, (AppenderMixin, Query)):
         # Query-like relationship. Run a query.
-        relobj = relationship.filter_by(**{actor_attr: actor}).one_or_none()
-    elif isinstance(relationship, (abc.Sequence, abc.Set)):  # 'abc.Collection' in Py3
+        if isinstance(actor_attr, QueryableAttribute):
+            relobj = relationship.filter(operator.eq(actor_attr, actor)).one_or_none()
+        else:
+            relobj = relationship.filter_by(**{actor_attr: actor}).one_or_none()
+    elif isinstance(relationship, abc.Iterable):
         # List-like object. Scan through it looking for item related to actor.
-        # Note: strings are also collections. Checking for abc.Collection is only safe
+        # Note: strings are also collections. Checking for abc.Iterable is only safe
         # here because of the unlikeliness of a string relationship. If that becomes
         # necessary in future, add `and not isinstance(relationship, six.string_types)`
         for relitem in relationship:
@@ -294,8 +307,8 @@ def _role_is_present(self, role):
             # list or query relationship. _roles_via_relationship will check.
             # The related object may grant roles in one of three ways:
             # 1. By its mere existence (default).
-            # 2. By offering roles via an `offered_roles` property (see RoleGrantABC).
-            # 3. By being a RoleMixin instance that has a roles_for method.
+            # 2. By offering roles via an `offered_roles` property (see `RoleGrantABC`).
+            # 3. By being a `RoleMixin` instance that has a `roles_for` method.
             if 'granted_via' in self.obj.__roles__[role]:
                 for relattr, actor_attr in self.obj.__roles__[role][
                     'granted_via'
@@ -318,7 +331,9 @@ def _role_is_present(self, role):
                                     arole != role
                                     and 'granted_via' in actions
                                     and relattr in actions['granted_via']
-                                    and actions['granted_via'][relattr] == actor_attr
+                                    and _attrs_equal(
+                                        actions['granted_via'][relattr], actor_attr
+                                    )
                                 ):
                                     possible_roles.add(arole)
 
@@ -974,7 +989,7 @@ def _get_relationship(self, relattr):
             relationship = getattr(self, relattr)
         return relationship
 
-    def actors_with(self, roles):
+    def actors_with(self, roles, with_role=False):
         """
         Return actors who have the specified roles on this object, as an iterator.
 
@@ -984,6 +999,12 @@ def actors_with(self, roles):
 
         Subclasses of :class:`RoleMixin` that have custom role granting logic in
         :meth:`roles_for` must provide a matching :meth:`actors_with` implementation.
+
+        :param set roles: Iterable specifying roles to find actors with. May be an
+            ordered type if ordering is important
+        :param bool with_role: If True, yields a tuple of the actor and the role they
+            were found with. The actor may have more roles, but only the first match
+            is returned
         """
         if not is_collection(roles):
             raise ValueError("`roles` parameter must be a set")
@@ -1014,9 +1035,9 @@ def is_new(actor):
                 if isinstance(relationship, (AppenderMixin, Query, abc.Iterable)):
                     for actor in relationship:
                         if is_new(actor):
-                            yield actor
+                            yield (actor, role) if with_role else actor
                 elif is_new(relationship):
-                    yield relationship
+                    yield (relationship, role) if with_role else relationship
             # Scan granted_via declarations
             for relattr, actor_attr in (
                 self.__roles__.get(role, {}).get('granted_via', {}).items()
@@ -1052,14 +1073,24 @@ def is_new(actor):
                                         obj.offered_roles
                                     )
                                 ):
-                                    actor = getattr(obj, actor_attr)
+                                    actor = getattr(
+                                        obj,
+                                        actor_attr.key
+                                        if isinstance(actor_attr, QueryableAttribute)
+                                        else actor_attr,
+                                    )
                                     if is_new(actor):
-                                        yield actor
+                                        yield (actor, role) if with_role else actor
                             # 1b. Doesn't have offered_roles? Accept it as is
                             else:
-                                actor = getattr(obj, actor_attr)
+                                actor = getattr(
+                                    obj,
+                                    actor_attr.key
+                                    if isinstance(actor_attr, QueryableAttribute)
+                                    else actor_attr,
+                                )
                                 if is_new(actor):
-                                    yield actor
+                                    yield (actor, role) if with_role else actor
                         # 2. No actor attribute? If it's a RoleMixin, we can call its
                         #    actors_with method and pass on whatever we get
                         elif isinstance(obj, RoleMixin):
@@ -1073,7 +1104,7 @@ def is_new(actor):
                             if rel_roles:
                                 for actor in obj.actors_with(rel_roles):
                                     if is_new(actor):
-                                        yield actor
+                                        yield (actor, role) if with_role else actor
                         # 3. No actor attribute and it's not a RoleMixin. This must be
                         #    an error
                         else:
@@ -1224,10 +1255,25 @@ def _configure_roles(mapper_, cls):
                             for role in rhs:
                                 reverse_offer_map.setdefault(role, set()).add(lhs)
                         roles = set(reverse_offer_map.keys())
+                    elif isinstance(roles, six.string_types):
+                        raise TypeError(
+                            "grants_via declaration {{{actor_attr!r}: {roles!r}}} on"
+                            " {cls}.{name} is using a string but needs to be a set or"
+                            " dict".format(
+                                actor_attr=actor_attr,
+                                roles=roles,
+                                cls=cls.__name__,
+                                name=name,
+                            )
+                        )
                     else:
                         offer_map = None
                         reverse_offer_map = None
-                    if actor_attr and '.' in actor_attr:
+                    if (
+                        actor_attr
+                        and isinstance(actor_attr, six.string_types)
+                        and '.' in actor_attr
+                    ):
                         parts = actor_attr.split('.')
                         dotted_name = '.'.join([name] + parts[:-1])
                         actor_attr = parts[-1]

pyproject.toml

@@ -23,3 +23,28 @@ exclude = '''
   | funnel/assets
 )/
 '''
+
+[tool.isort]
+multi_line_output = 3
+include_trailing_comma = true
+line_length = 88
+order_by_type = true
+use_parentheses = true
+from_first = true
+known_future_library = ['__future__', 'six']
+known_first_party = ['coaster']
+known_sqlalchemy = ['alembic', 'sqlalchemy', 'sqlalchemy_utils', 'flask_sqlalchemy', 'psycopg2']
+known_flask = [
+  'flask',
+  'werkzeug',
+  'itsdangerous',
+  'wtforms',
+  'webassets',
+  'flask_assets',
+  'flask_flatpages',
+  'flask_mail',
+  'flask_migrate',
+  'flask_rq2'
+]
+default_section = 'THIRDPARTY'
+sections = ['FUTURE', 'STDLIB', 'SQLALCHEMY', 'FLASK', 'THIRDPARTY', 'FIRSTPARTY', 'LOCALFOLDER']

setup.cfg

@@ -10,20 +10,6 @@ max-line-length = 88
 [pytest]
 doctest_optionflags = ALLOW_UNICODE
 
-[isort]
-multi_line_output = 3
-include_trailing_comma = true
-line_length = 88
-order_by_type = true
-use_parentheses = true
-from_first = true
-known_future_library = six,future
-known_first_party = coaster
-known_sqlalchemy = alembic, sqlalchemy, sqlalchemy_utils, flask_sqlalchemy, psycopg2
-known_flask = flask, werkzeug, itsdangerous, wtforms, webassets, flask_assets, flask_flatpages, flask_mail, flask_migrate, flask_rq2
-default_section = THIRDPARTY
-sections = FUTURE, STDLIB, SQLALCHEMY, FLASK, THIRDPARTY, FIRSTPARTY, LOCALFOLDER
-
 # Bandit config for flake8-bandit. There's another copy in .pre-commit-config.yaml
 [bandit]
 exclude = tests, build/lib

tests/test_sqlalchemy_roles.py

@@ -302,20 +302,22 @@ class MultiroleDocument(BaseMixin, db.Model):
         'parent_role': {'granted_via': {'parent': 'user'}},
         'parent_other_role': {'granted_via': {'parent': 'user'}},
         'role1': {'granted_via': {'rel_lazy': 'user', 'rel_list': 'user'}},
-        'role2': {'granted_via': {'rel_lazy': 'user'}},
         'incorrectly_specified_role': {'granted_via': {'rel_list': None}},
         'also_incorrect_role': {'granted_via': {'not_a_relationship': None}},
     }
 
     # Grant via a query relationship
-    rel_lazy = db.relationship(RoleMembership, lazy='dynamic')
+    rel_lazy = with_roles(
+        db.relationship(RoleMembership, lazy='dynamic'),
+        grants_via={RoleMembership.user: {'role2'}},
+    )
     # Grant via a list-like relationship
     rel_list = with_roles(
         db.relationship(RoleMembership), grants_via={'user': {'role3'}}
     )
 
     # Role grants can be specified via:
-    # 1. with_roles(grants_via={actor_attr_name: {role,} or {offered_role: role}})
+    # 1. with_roles(grants_via={actor_attr_or_name: {role} or {offered_role: role}})
     # 2. __roles__[role]['granted_via'] = {'rel_name': 'actor_attr_name'}
     # Offered role maps are specified in an internal __relationship_role_offer_map__.
     # The only way to make an entry there is via with_roles.
@@ -775,6 +777,28 @@ def test_actors_with(self):
         assert set(m1.actors_with({'primary_role', 'secondary_role'})) == {u1, u2}
         assert set(m2.actors_with({'primary_role', 'secondary_role'})) == {u2}
 
+        # Ask for role when returning a user
+        assert set(o1.actors_with(['creator'], with_role=True)) == {(u1, 'creator')}
+        assert set(o2.actors_with(['creator'], with_role=True)) == {(u2, 'creator')}
+
+        assert set(m1.actors_with(['primary_role'], with_role=True)) == {
+            (u1, 'primary_role')
+        }
+        assert set(m2.actors_with(['primary_role'], with_role=True)) == {
+            (u2, 'primary_role')
+        }
+        assert set(m1.actors_with(['secondary_role'], with_role=True)) == {
+            (u1, 'secondary_role'),
+            (u2, 'secondary_role'),
+        }
+        assert set(m2.actors_with(['secondary_role'], with_role=True)) == set()
+        assert set(
+            m1.actors_with(['primary_role', 'secondary_role'], with_role=True)
+        ) == {(u1, 'primary_role'), (u2, 'secondary_role')}
+        assert set(
+            m2.actors_with(['primary_role', 'secondary_role'], with_role=True)
+        ) == {(u2, 'primary_role')}
+
     def test_actors_with_invalid(self):
         m1 = RoleGrantMany()
         with pytest.raises(ValueError):