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

docstrings for permissions package

Author: redhatHameed

sdk/python/feast/permissions/action.py

@@ -3,7 +3,7 @@
 
 class AuthzedAction(enum.Enum):
     """
-    Identifies the type of action being secured by the permissions framework, according to the familiar CRUD and Feast terminology.
+    Identify the type of action being secured by the permissions framework, according to the familiar CRUD and Feast terminology.
     """
 
     ALL = "all"  # All actions

sdk/python/feast/permissions/decision.py

@@ -7,7 +7,7 @@
 
 class DecisionStrategy(enum.Enum):
     """
-    The strategy to be adopted in case multiple policies are defined.
+    The strategy to be adopted in case multiple permissions match an execution request.
     """
 
     UNANIMOUS = "unanimous"  # All policies must evaluate to a positive decision for the final decision to be also positive.
@@ -18,6 +18,23 @@ class DecisionStrategy(enum.Enum):
 
 
 class DecisionEvaluator:
+    """
+    A class to implement the decision logic, according to the selected strategy.
+
+    Args:
+        decision_strategy: The associated `DecisionStrategy`.
+        num_of_voters: The expected number of votes to complete the decision.
+
+    Examples:
+        Create the instance and specify the strategy and number of decisions:
+        `evaluator = DecisionEvaluator(DecisionStrategy.UNANIMOUS, 3)
+
+        For each vote that you receivem, add a decision grant: `evaluator.add_grant(vote, message)`
+        and check if the decision process ended: `if evaluator.is_decided():`
+        Once decided, get the result and the failure explanations using:
+        `grant, explanations = evaluator.grant()`
+    """
+
     def __init__(
         self,
         decision_strategy: DecisionStrategy,
@@ -49,15 +66,33 @@ def __init__(
         )
 
     def is_decided(self) -> bool:
+        """
+        Returns:
+            bool: `True` when the decision process completed (e.g. we added as many votes as specified in the `num_of_voters` creation argument).
+        """
         return self.grant_decision is not None
 
     def grant(self) -> tuple[bool, list[str]]:
+        """
+        Returns:
+            tuple[bool, list[str]]: The tuple of decision computation: a `bool` with the computation decision and a `list[str]` with the
+            denial explanations (possibly empty).
+        """
         logger.info(
             f"Decided grant is {self.grant_decision}, explanations={self.explanations}"
         )
         return bool(self.grant_decision), self.explanations
 
-    def add_grant(self, label, grant: bool, explanation: str):
+    def add_grant(self, grant: bool, explanation: str):
+        """
+        Add a single vote to the decision computation, with a possible denial reason.
+        If the evalluation process already ended, additional votes are discarded.
+
+        Args:
+            grant: `True` is the decision is accepted, `False` otherwise.
+            explanation: Denial reason (not considered when `vote` is `True`).
+        """
+
         if self.is_decided():
             logger.warning("Grant decision already decided, discarding vote")
             return
@@ -71,6 +106,6 @@ def add_grant(self, label, grant: bool, explanation: str):
             self.grant_decision = True
         if self.deny_count >= self.deny_quorum:
             self.grant_decision = False
-        logger.info(
-            f"After {label}: grants={self.grant_count}, deny_count={self.deny_count}, grant_decision={self.grant_decision}"
+        logger.debug(
+            f"After new grant: grants={self.grant_count}, deny_count={self.deny_count}, grant_decision={self.grant_decision}"
         )

sdk/python/feast/permissions/decorator.py

@@ -10,10 +10,12 @@
 
 def require_permissions(actions: Union[list[AuthzedAction], AuthzedAction]):
     """
-    A decorator to define the actions that are executed from within the current class method and that must be protected
+    A decorator to define the actions that are executed from the decorated class method and that must be protected
     against unauthorized access.
 
     The first parameter of the protected method must be `self`
+    Args:
+        actions: The list of actions that must be permitted to the current user.
     """
 
     def require_permissions_decorator(func):
@@ -26,7 +28,7 @@ def permission_checker(*args, **kwargs):
                 )
 
             return assert_permissions(
-                resource=resource,
+                resources=resource,
                 actions=actions,
             )
             logger.debug(

sdk/python/feast/permissions/enforcer.py

@@ -1,4 +1,5 @@
 import logging
+from typing import Union
 
 from feast.feast_object import FeastObject
 from feast.permissions.decision import DecisionEvaluator
@@ -15,48 +16,56 @@ def enforce_policy(
     role_manager: RoleManager,
     permissions: list[Permission],
     user: str,
-    resource: FeastObject,
+    resources: Union[list[FeastObject], FeastObject],
     actions: list[AuthzedAction],
 ) -> tuple[bool, str]:
     """
-    Defines the logic to apply the configured permissions when a given action is requested on
+    Define the logic to apply the configured permissions when a given action is requested on
     a protected resource.
 
+    If no permissions are defined, the result is to allow the execution.
+
     Args:
         role_manager: The `RoleManager` instance.
         permissions: The configured set of `Permission`.
         user: The current user.
-        resource: The resource for which we need to enforce authorized permission.
+        resources: The resources for which we need to enforce authorized permission.
         actions: The requested actions to be authorized.
+    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.
     """
-
     if not permissions:
         return (True, "")
 
-    matching_permissions = [
-        p
-        for p in permissions
-        if p.match_resource(resource) and p.match_actions(actions)
-    ]
-
-    if matching_permissions:
-        evaluator = DecisionEvaluator(
-            Permission.get_global_decision_strategy(), len(matching_permissions)
+    _resources = resources if isinstance(resources, list) else [resources]
+    for resource in _resources:
+        logger.debug(
+            f"Enforcing permission policies for {type(resource)}:{resource.name} to execute {actions}"
         )
-        for p in matching_permissions:
-            permission_grant, permission_explanation = p.policy.validate_user(
-                user=user, role_manager=role_manager
-            )
-            evaluator.add_grant(
-                f"Permission ({p.name})",
-                permission_grant,
-                f"Permission {p.name} denied access: {permission_explanation}",
+        matching_permissions = [
+            p
+            for p in permissions
+            if p.match_resource(resource) and p.match_actions(actions)
+        ]
+
+        if matching_permissions:
+            evaluator = DecisionEvaluator(
+                Permission.get_global_decision_strategy(), len(matching_permissions)
             )
+            for p in matching_permissions:
+                permission_grant, permission_explanation = p.policy.validate_user(
+                    user=user, role_manager=role_manager
+                )
+                evaluator.add_grant(
+                    permission_grant,
+                    f"Permission {p.name} denied access: {permission_explanation}",
+                )
 
-            if evaluator.is_decided():
-                grant, explanations = evaluator.grant()
-                return grant, ",".join(explanations)
+                if evaluator.is_decided():
+                    grant, explanations = evaluator.grant()
+                    return grant, ",".join(explanations)
     else:
-        message = f"No permissions defined to manage {actions} on {type(resource)}:{resource.name}."
+        message = f"No permissions defined to manage {actions} on {resources}."
         logger.info(f"**PERMISSION GRANTED**: {message}")
     return (True, "")

sdk/python/feast/permissions/matcher.py

@@ -1,3 +1,7 @@
+"""
+This module provides utility matching functions.
+"""
+
 import logging
 import re
 from typing import Any, Optional, get_args
@@ -10,6 +14,14 @@
 
 
 def is_a_feast_object(resource: Any):
+    """
+    A matcher to verify that a given object is one of the Feast objects defined in the `FeastObject` type.
+
+    Args:
+        resource: An object instance to verify.
+    Returns:
+        `True` if the given object is one of the types in the FeastObject alias or a subclass of one of them.
+    """
     for t in get_args(FeastObject):
         # Use isinstance to pass Mock validation
         if isinstance(resource, t):
@@ -36,6 +48,19 @@ def resource_match_config(
     name_pattern: Optional[str] = None,
     required_tags: Optional[dict[str, str]] = None,
 ) -> bool:
+    """
+    Match a given Feast object against the configured type, name and tags in a permission configuration.
+
+    Args:
+        resource: A FeastObject instance to match agains the permission.
+        expected_types: The list of object types configured in the permission.
+        with_subclasses: `True` if the type match includes sub-classes, `False` if the type match is exact.
+        name_pattern: The optional name pattern filter configured in the permission.
+        required_tags: The optional dicstionary of required tags configured in the permission.
+
+    Returns:
+        bool: `True` if the resource matches the configured permission filters.
+    """
     if resource is None:
         logger.warning(f"None passed to {resource_match_config.__name__}")
         return False
@@ -118,6 +143,17 @@ def actions_match_config(
     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.
+        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`
+    """
     if AuthzedAction.ALL in allowed_actions:
         return True
 

sdk/python/feast/permissions/permission.py

@@ -23,12 +23,14 @@ class Permission(ABC):
     requested on the matching resources.
 
     Attributes:
-        name: The permission name (can be duplicated, used for logging troubleshooting)
-        types: The list of protected resource  types as defined by the `FeastObject` type. Defaults to all managed types (e.g. the `ALL_RESOURCE_TYPES` constant)
-        with_subclasses: If `True`, it includes subclasses of the given types in the match, otherwise only precise type match is applied. Defaults to `True`.
-        name_pattern: a regex to match the resource name. Defaults to None, meaning that no name filtering is applied
-        required_tags: dictionary of key-value pairs that must match the resource tags. All these required_tags must be present as resource
-        tags with the given value. Defaults to None, meaning that no tags filtering is applied.
+        name: The permission name (can be duplicated, used for logging troubleshooting).
+        types: The list of protected resource  types as defined by the `FeastObject` type.
+        Defaults to all managed types (e.g. the `ALL_RESOURCE_TYPES` constant)
+        with_subclasses: If `True`, it includes sub-classes of the given types in the match, otherwise only exact type match is applied.
+        Defaults to `True`.
+        name_pattern: A regex to match the resource name. Defaults to None, meaning that no name filtering is applied
+        required_tags: Dictionary of key-value pairs that must match the resource tags. All these required_tags must
+        be present in a resource tags with the given value. Defaults to None, meaning that no tags filtering is applied.
         actions: The actions authorized by this permission. Defaults to `AuthzedAction.ALL`.
         policy: The policy to be applied to validate a client request.
     """
@@ -72,12 +74,15 @@ def __init__(
 
     @staticmethod
     def get_global_decision_strategy() -> DecisionStrategy:
+        """
+        The global decision strategy to be applied when multiple permissions match an execution request.
+        """
         return Permission._global_decision_strategy
 
     @staticmethod
     def set_global_decision_strategy(global_decision_strategy: DecisionStrategy):
         """
-        Defines the global decision strategy to be applied if multiple permissions match the same resource.
+        Define the global decision strategy to be applied when multiple permissions match an execution request.
         """
         Permission._global_decision_strategy = global_decision_strategy
 
@@ -110,6 +115,10 @@ def policy(self) -> Policy:
         return self._policy
 
     def match_resource(self, resource: FeastObject) -> bool:
+        """
+        Returns:
+            `True` when the given resource matches the type, name and tags filters defined in the permission.
+        """
         return resource_match_config(
             resource=resource,
             expected_types=self.types,
@@ -119,6 +128,10 @@ def match_resource(self, resource: FeastObject) -> bool:
         )
 
     def match_actions(self, 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,

sdk/python/feast/permissions/policy.py

@@ -11,26 +11,25 @@ class Policy(ABC):
     @abstractmethod
     def validate_user(self, user: str, **kwargs) -> tuple[bool, str]:
         """
-        Converts data source config in protobuf spec to a DataSource class object.
+        Validate the given user against the configured policy.
 
         Args:
-            data_source: A protobuf representation of a DataSource.
+            user: The current user.
+            kwargs: The list of keyword args to be passed to the actual implementation.
 
         Returns:
-            A DataSource class object.
-
-        Raises:
-            ValueError: The type of DataSource could not be identified.
+            bool: `True` if the user matches the policy criteria, `False` otherwise.
+            str: A possibly empty explanation of the reason for not matching the configured policy.
         """
         raise NotImplementedError
 
 
 class RoleBasedPolicy(Policy):
     """
-    An Policy class where the user roles must be enforced to grant access to the requested action.
-    All the configured roles must be granted to the current user in order to allow the execution.
+    A `Policy` implementation where the user roles must be enforced to grant access to the requested action.
+    At least one of the configured roles must be granted to the current user in order to allow the execution of the secured operation.
 
-    The `role_manager` keywork argument must be present in the `kwargs` optional key-value arguments.
+    E.g., if the policy enforces roles `a` and `b`, the user must have at least one of them in order to satisfy the policy.
     """
 
     def __init__(
@@ -43,6 +42,11 @@ def get_roles(self) -> list[str]:
         return self.roles
 
     def validate_user(self, user: str, **kwargs) -> tuple[bool, str]:
+        """
+        Validate the given `user` against the configured roles.
+
+        The `role_manager` keywork argument must be present in the `kwargs` optional key-value arguments.
+        """
         if "role_manager" not in kwargs:
             raise ValueError("Missing keywork argument 'role_manager'")
         if not isinstance(kwargs["role_manager"], RoleManager):
@@ -51,7 +55,7 @@ def validate_user(self, user: str, **kwargs) -> tuple[bool, str]:
             )
         rm = kwargs.get("role_manager")
         if isinstance(rm, RoleManager):
-            result = rm.has_roles_for_user(user, self.roles)
+            result = rm.user_has_matching_role(user, self.roles)
         explain = "" if result else f"Requires roles {self.roles}"
         return (result, explain)