Merge pull request feast-dev#26 from dmartinol/feast-rbac

Added filter_only flag to assert_permissions

sdk/python/feast/permissions/enforcer.py

@@ -1,5 +1,5 @@
 import logging
-from typing import Union
+from typing import Optional, Union
 
 from feast.feast_object import FeastObject
 from feast.permissions.decision import DecisionEvaluator
@@ -18,7 +18,8 @@ def enforce_policy(
     user: str,
     resources: Union[list[FeastObject], FeastObject],
     actions: list[AuthzedAction],
-) -> tuple[bool, str]:
+    filter_only: bool = False,
+) -> Optional[Union[list[FeastObject], FeastObject]]:
     """
     Define the logic to apply the configured permissions when a given action is requested on
     a protected resource.
@@ -31,14 +32,21 @@ def enforce_policy(
         user: The current user.
         resources: The resources for which we need to enforce authorized permission.
         actions: The requested actions to be authorized.
+        filter_only: If `True`, it removes unauthorized resources from the returned value, otherwise it raises a `PermissionError` the
+        first unauthorized resource. Defaults to `False`.
+
     Returns:
-        tuple[bool, str]: a boolean with the result of the authorization check (`True` stands for allowed) and string to explain
-        the reason for denying execution.
+        Union[list[FeastObject], FeastObject]: A filtered list of the permitted resources or the original `resources`, if permitted
+        (otherwise it's `None`).
+
+    Raises:
+        PermissionError: If the current user is not authorized to eecute the requested actions on the given resources (and `filter_only` is `False`).
     """
+    _resources = resources if isinstance(resources, list) else [resources]
     if not permissions:
-        return (True, "")
+        return _resources
 
-    _resources = resources if isinstance(resources, list) else [resources]
+    _permitted_resources: list[FeastObject] = []
     for resource in _resources:
         logger.debug(
             f"Enforcing permission policies for {type(resource)}:{resource.name} to execute {actions}"
@@ -64,8 +72,19 @@ def enforce_policy(
 
                 if evaluator.is_decided():
                     grant, explanations = evaluator.grant()
-                    return grant, ",".join(explanations)
-    else:
-        message = f"No permissions defined to manage {actions} on {resources}."
-        logger.info(f"**PERMISSION GRANTED**: {message}")
-    return (True, "")
+                    if not grant and not filter_only:
+                        raise PermissionError(",".join(explanations))
+                    if grant:
+                        _permitted_resources.append(resource)
+                    break
+        else:
+            _permitted_resources.append(resource)
+            message = f"No permissions defined to manage {actions} on {type(resource)}/{resource.name}."
+            logger.info(f"**PERMISSION GRANTED**: {message}")
+    return (
+        _permitted_resources
+        if isinstance(resources, list)
+        else _permitted_resources[0]
+        if _permitted_resources
+        else None
+    )

sdk/python/feast/permissions/matcher.py

@@ -140,21 +140,21 @@ def resource_match_config(
 
 
 def actions_match_config(
-    actions: list[AuthzedAction],
+    requested_actions: list[AuthzedAction],
     allowed_actions: list[AuthzedAction],
 ) -> bool:
     """
     Match a list of actions against the actions defined in a permission configuration.
 
     Args:
-        actions: Alist of actions to be executed.
+        requested_actions: A list of actions to be executed.
         allowed_actions: The list of actions configured in the permission.
 
     Returns:
-        bool: `True` if all the given `actions` are defined in the `allowed_actions`.
-        Whatever the requested `actions`, it returns `True` if `allowed_actions` includes `AuthzedAction.ALL`
+        bool: `True` if all the given `requested_actions` are defined in the `allowed_actions`.
+        Whatever the `requested_actions`, it returns `True` if `allowed_actions` includes `AuthzedAction.ALL`
     """
     if AuthzedAction.ALL in allowed_actions:
         return True
 
-    return all(a in allowed_actions for a in actions)
+    return all(a in allowed_actions for a in requested_actions)

sdk/python/feast/permissions/permission.py

@@ -127,14 +127,14 @@ def match_resource(self, resource: FeastObject) -> bool:
             required_tags=self.required_tags,
         )
 
-    def match_actions(self, actions: list[AuthzedAction]) -> bool:
+    def match_actions(self, requested_actions: list[AuthzedAction]) -> bool:
         """
         Returns:
             `True` when the given actions are included in the permitted actions.
         """
         return actions_match_config(
             allowed_actions=self.actions,
-            actions=actions,
+            requested_actions=requested_actions,
         )
 
 

sdk/python/feast/permissions/security_manager.py

@@ -60,7 +60,8 @@ def assert_permissions(
         self,
         resources: Union[list[FeastObject], FeastObject],
         actions: Union[AuthzedAction, List[AuthzedAction]],
-    ):
+        filter_only: bool = False,
+    ) -> Optional[Union[list[FeastObject], FeastObject]]:
         """
         Verify if the current user is authorized ro execute the requested actions on the given resources.
 
@@ -69,34 +70,54 @@ def assert_permissions(
         Args:
             resources: The resources for which we need to enforce authorized permission.
             actions: The requested actions to be authorized.
+            filter_only: If `True`, it removes unauthorized resources from the returned value, otherwise it raises a `PermissionError` the
+            first unauthorized resource. Defaults to `False`.
+
+        Returns:
+            Union[list[FeastObject], FeastObject]: A filtered list of the permitted resources or the original `resources`, if permitted
+            (otherwise it's `None`).
 
         Raises:
             PermissionError: If the current user is not authorized to eecute all the requested actions on the given resources.
         """
-        result, explain = enforce_policy(
+        return enforce_policy(
             role_manager=self._role_manager,
             permissions=self._permissions,
             user=self.current_user if self.current_user is not None else "",
             resources=resources,
             actions=actions if isinstance(actions, list) else [actions],
+            filter_only=filter_only,
         )
-        if not result:
-            raise PermissionError(explain)
 
 
 def assert_permissions(
     resources: Union[list[FeastObject], FeastObject],
     actions: Union[AuthzedAction, List[AuthzedAction]],
-):
+    filter_only: bool = False,
+) -> Optional[Union[list[FeastObject], FeastObject]]:
     """
     A utility function to invoke the `assert_permissions` method on the global security manager.
 
     If no global `SecurityManager` is defined, the execution is permitted.
+
+    Args:
+        resources: The resources for which we need to enforce authorized permission.
+        actions: The requested actions to be authorized.
+        filter_only: If `True`, it removes unauthorized resources from the returned value, otherwise it raises a `PermissionError` the
+        first unauthorized resource. Defaults to `False`.
+    Returns:
+        Union[list[FeastObject], FeastObject]: A filtered list of the permitted resources or the original `resources`, if permitted
+        (otherwise it's `None`).
+
+    Raises:
+        PermissionError: If the current user is not authorized to eecute the requested actions on the given resources (and `filter_only` is `False`).
     """
     sm = get_security_manager()
     if sm is None:
-        return
-    return sm.assert_permissions(resources=resources, actions=actions)
+        return resources if isinstance(resources, list) else [resources]
+    return sm.assert_permissions(
+        resources=resources, actions=actions, filter_only=filter_only
+    )
 
 
 """

sdk/python/tests/unit/permissions/conftest.py

@@ -0,0 +1,92 @@
+from unittest.mock import Mock
+
+import pytest
+
+from feast import FeatureView
+from feast.permissions.decorator import require_permissions
+from feast.permissions.permission import AuthzedAction, Permission
+from feast.permissions.policy import RoleBasedPolicy
+from feast.permissions.role_manager import RoleManager
+from feast.permissions.security_manager import (
+    SecurityManager,
+    set_security_manager,
+)
+
+
+class SecuredFeatureView(FeatureView):
+    def __init__(self, name, tags):
+        super().__init__(
+            name=name,
+            source=Mock(),
+            tags=tags,
+        )
+
+    @require_permissions(actions=[AuthzedAction.READ])
+    def read_protected(self) -> bool:
+        return True
+
+    @require_permissions(actions=[AuthzedAction.WRITE])
+    def write_protected(self) -> bool:
+        return True
+
+    def unprotected(self) -> bool:
+        return True
+
+
+@pytest.fixture
+def feature_views() -> list[FeatureView]:
+    return [
+        SecuredFeatureView("secured", {}),
+        SecuredFeatureView("special-secured", {}),
+    ]
+
+
+@pytest.fixture
+def role_manager() -> RoleManager:
+    rm = RoleManager()
+    rm.add_roles_for_user("r", ["reader"])
+    rm.add_roles_for_user("w", ["writer"])
+    rm.add_roles_for_user("rw", ["reader", "writer"])
+    return rm
+
+
+@pytest.fixture
+def security_manager() -> SecurityManager:
+    permissions = []
+    permissions.append(
+        Permission(
+            name="reader",
+            types=FeatureView,
+            with_subclasses=True,
+            policy=RoleBasedPolicy(roles=["reader"]),
+            actions=[AuthzedAction.READ],
+        )
+    )
+    permissions.append(
+        Permission(
+            name="writer",
+            types=FeatureView,
+            with_subclasses=True,
+            policy=RoleBasedPolicy(roles=["writer"]),
+            actions=[AuthzedAction.WRITE],
+        )
+    )
+    permissions.append(
+        Permission(
+            name="special",
+            types=FeatureView,
+            with_subclasses=True,
+            name_pattern="special.*",
+            policy=RoleBasedPolicy(roles=["admin", "special-reader"]),
+            actions=[AuthzedAction.READ, AuthzedAction.WRITE],
+        )
+    )
+
+    rm = RoleManager()
+    rm.add_roles_for_user("r", ["reader"])
+    rm.add_roles_for_user("w", ["writer"])
+    rm.add_roles_for_user("rw", ["reader", "writer"])
+    rm.add_roles_for_user("admin", ["reader", "writer", "admin"])
+    sm = SecurityManager(role_manager=rm, permissions=permissions)
+    set_security_manager(sm)
+    return sm

sdk/python/tests/unit/permissions/test_decorator.py

@@ -1,74 +1,6 @@
-from unittest.mock import Mock
-
 import assertpy
 import pytest
 
-from feast import FeatureView
-from feast.permissions.decorator import require_permissions
-from feast.permissions.permission import AuthzedAction, Permission
-from feast.permissions.policy import RoleBasedPolicy
-from feast.permissions.role_manager import RoleManager
-from feast.permissions.security_manager import (
-    SecurityManager,
-    set_security_manager,
-)
-
-
-class SecuredFeatureView(FeatureView):
-    def __init__(self, name, tags):
-        super().__init__(
-            name=name,
-            source=Mock(),
-            tags=tags,
-        )
-
-    @require_permissions(actions=[AuthzedAction.READ])
-    def read_protected(self) -> bool:
-        return True
-
-    @require_permissions(actions=[AuthzedAction.WRITE])
-    def write_protected(self) -> bool:
-        return True
-
-    def unprotected(self) -> bool:
-        return True
-
-
-@pytest.fixture
-def security_manager() -> SecurityManager:
-    permissions = []
-    permissions.append(
-        Permission(
-            name="reader",
-            types=FeatureView,
-            with_subclasses=True,
-            policy=RoleBasedPolicy(roles=["reader"]),
-            actions=[AuthzedAction.READ],
-        )
-    )
-    permissions.append(
-        Permission(
-            name="writer",
-            types=FeatureView,
-            with_subclasses=True,
-            policy=RoleBasedPolicy(roles=["writer"]),
-            actions=[AuthzedAction.WRITE],
-        )
-    )
-
-    rm = RoleManager()
-    rm.add_roles_for_user("r", ["reader"])
-    rm.add_roles_for_user("w", ["writer"])
-    rm.add_roles_for_user("rw", ["reader", "writer"])
-    sm = SecurityManager(role_manager=rm, permissions=permissions)
-    set_security_manager(sm)
-    return sm
-
-
-@pytest.fixture
-def feature_view() -> FeatureView:
-    return SecuredFeatureView("secured", {})
-
 
 @pytest.mark.parametrize(
     "user, can_read, can_write",
@@ -80,10 +12,10 @@ def feature_view() -> FeatureView:
     ],
 )
 def test_access_SecuredFeatureView(
-    security_manager, feature_view, user, can_read, can_write
+    security_manager, feature_views, user, can_read, can_write
 ):
     sm = security_manager
-    fv = feature_view
+    fv = feature_views[0]
 
     sm.set_current_user(user)
     if can_read:

sdk/python/tests/unit/permissions/test_permission.py

@@ -251,7 +251,7 @@ def test_resource_match_with_tags(required_tags, tags, result):
 
 
 @pytest.mark.parametrize(
-    ("permitted_actions, actions, result"),
+    ("permitted_actions, requested_actions, result"),
     [(AuthzedAction.ALL, a, True) for a in AuthzedAction.__members__.values()]
     + [
         (
@@ -269,6 +269,8 @@ def test_resource_match_with_tags(required_tags, tags, result):
         ),
     ],
 )
-def test_match_actions(permitted_actions, actions, result):
+def test_match_actions(permitted_actions, requested_actions, result):
     p = Permission(name="test", actions=permitted_actions)
-    assertpy.assert_that(p.match_actions(actions=actions)).is_equal_to(result)
+    assertpy.assert_that(
+        p.match_actions(requested_actions=requested_actions)
+    ).is_equal_to(result)