fix: Validating permission to update an existing request on both the new and the old instance (feast-dev#4449)

dmartinol · web-flow · commit 635a01b4c77d · 2024-08-27T14:51:21.000+04:00
* Validating permission to update an existing request on both the new and the old instance

Signed-off-by: Daniele Martinoli &lt;86618610+dmartinol@users.noreply.github.com&gt;

* Reviewed update permission logic as per comment, added UT

Signed-off-by: Daniele Martinoli &lt;dmartino@redhat.com&gt;

---------

Signed-off-by: Daniele Martinoli &lt;86618610+dmartinol@users.noreply.github.com&gt;
Signed-off-by: Daniele Martinoli &lt;dmartino@redhat.com&gt;
diff --git a/sdk/python/feast/permissions/enforcer.py b/sdk/python/feast/permissions/enforcer.py
@@ -22,7 +22,7 @@ def enforce_policy(
     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.
+    If no permissions are defined, the result is to deny the execution.
 
     Args:
         permissions: The configured set of `Permission`.
diff --git a/sdk/python/feast/permissions/security_manager.py b/sdk/python/feast/permissions/security_manager.py
@@ -1,7 +1,8 @@
 import logging
 from contextvars import ContextVar
-from typing import List, Optional, Union
+from typing import Callable, List, Optional, Union
 
+from feast.errors import FeastObjectNotFoundException
 from feast.feast_object import FeastObject
 from feast.infra.registry.base_registry import BaseRegistry
 from feast.permissions.action import AuthzedAction
@@ -59,9 +60,9 @@ def assert_permissions(
         filter_only: bool = False,
     ) -> list[FeastObject]:
         """
-        Verify if the current user is authorized ro execute the requested actions on the given resources.
+        Verify if the current user is authorized to execute the requested actions on the given resources.
 
-        If no permissions are defined, the result is to allow the execution.
+        If no permissions are defined, the result is to deny the execution.
 
         Args:
             resources: The resources for which we need to enforce authorized permission.
@@ -73,7 +74,7 @@ def assert_permissions(
             list[FeastObject]: A filtered list of the permitted resources, possibly empty.
 
         Raises:
-            PermissionError: If the current user is not authorized to eecute all the requested actions on the given resources.
+            PermissionError: If the current user is not authorized to execute all the requested actions on the given resources.
         """
         return enforce_policy(
             permissions=self.permissions,
@@ -84,6 +85,45 @@ def assert_permissions(
         )
 
 
+def assert_permissions_to_update(
+    resource: FeastObject,
+    getter: Callable[[str, str, bool], FeastObject],
+    project: str,
+    allow_cache: bool = True,
+) -> FeastObject:
+    """
+    Verify if the current user is authorized to create or update the given resource.
+    If the resource already exists, the user must be granted permission to execute DESCRIBE and UPDATE actions.
+    If the resource does not exist, the user must be granted permission to execute the CREATE action.
+
+    If no permissions are defined, the result is to deny the execution.
+
+    Args:
+        resource: The resources for which we need to enforce authorized permission.
+        getter: The getter function used to retrieve the existing resource instance by name.
+        The signature must be `get_permission(self, name: str, project: str, allow_cache: bool)`
+        project: The project nane used in the getter function.
+        allow_cache: Whether to use cached data. Defaults to `True`.
+    Returns:
+        FeastObject: The original `resource`, if permitted.
+
+    Raises:
+        PermissionError: If the current user is not authorized to execute all the requested actions on the given resource or on the existing one.
+    """
+    actions = [AuthzedAction.DESCRIBE, AuthzedAction.UPDATE]
+    try:
+        existing_resource = getter(
+            name=resource.name,
+            project=project,
+            allow_cache=allow_cache,
+        )  # type: ignore[call-arg]
+        assert_permissions(resource=existing_resource, actions=actions)
+    except FeastObjectNotFoundException:
+        actions = [AuthzedAction.CREATE]
+    resource_to_update = assert_permissions(resource=resource, actions=actions)
+    return resource_to_update
+
+
 def assert_permissions(
     resource: FeastObject,
     actions: Union[AuthzedAction, List[AuthzedAction]],
diff --git a/sdk/python/feast/registry_server.py b/sdk/python/feast/registry_server.py
@@ -16,9 +16,13 @@
 from feast.infra.infra_object import Infra
 from feast.infra.registry.base_registry import BaseRegistry
 from feast.on_demand_feature_view import OnDemandFeatureView
-from feast.permissions.action import CRUD, AuthzedAction
+from feast.permissions.action import AuthzedAction
 from feast.permissions.permission import Permission
-from feast.permissions.security_manager import assert_permissions, permitted_resources
+from feast.permissions.security_manager import (
+    assert_permissions,
+    assert_permissions_to_update,
+    permitted_resources,
+)
 from feast.permissions.server.grpc import grpc_interceptors
 from feast.permissions.server.utils import (
     ServerType,
@@ -37,14 +41,16 @@ def __init__(self, registry: BaseRegistry) -> None:
         self.proxied_registry = registry
 
     def ApplyEntity(self, request: RegistryServer_pb2.ApplyEntityRequest, context):
-        self.proxied_registry.apply_entity(
-            entity=cast(
-                Entity,
-                assert_permissions(
-                    resource=Entity.from_proto(request.entity),
-                    actions=CRUD,
-                ),
+        entity = cast(
+            Entity,
+            assert_permissions_to_update(
+                resource=Entity.from_proto(request.entity),
+                getter=self.proxied_registry.get_entity,
+                project=request.project,
             ),
+        )
+        self.proxied_registry.apply_entity(
+            entity=entity,
             project=request.project,
             commit=request.commit,
         )
@@ -95,19 +101,19 @@ def DeleteEntity(self, request: RegistryServer_pb2.DeleteEntityRequest, context)
     def ApplyDataSource(
         self, request: RegistryServer_pb2.ApplyDataSourceRequest, context
     ):
-        (
-            self.proxied_registry.apply_data_source(
-                data_source=cast(
-                    DataSource,
-                    assert_permissions(
-                        resource=DataSource.from_proto(request.data_source),
-                        actions=CRUD,
-                    ),
-                ),
+        data_source = cast(
+            DataSource,
+            assert_permissions_to_update(
+                resource=DataSource.from_proto(request.data_source),
+                getter=self.proxied_registry.get_data_source,
                 project=request.project,
-                commit=request.commit,
             ),
         )
+        self.proxied_registry.apply_data_source(
+            data_source=data_source,
+            project=request.project,
+            commit=request.commit,
+        )
 
         return Empty()
 
@@ -182,12 +188,16 @@ def ApplyFeatureView(
         elif feature_view_type == "stream_feature_view":
             feature_view = StreamFeatureView.from_proto(request.stream_feature_view)
 
+        assert_permissions_to_update(
+            resource=feature_view,
+            # Will replace with the new get_any_feature_view method later
+            getter=self.proxied_registry.get_feature_view,
+            project=request.project,
+        )
+
         (
             self.proxied_registry.apply_feature_view(
-                feature_view=cast(
-                    FeatureView,
-                    assert_permissions(resource=feature_view, actions=CRUD),
-                ),
+                feature_view=feature_view,
                 project=request.project,
                 commit=request.commit,
             ),
@@ -305,14 +315,16 @@ def ListOnDemandFeatureViews(
     def ApplyFeatureService(
         self, request: RegistryServer_pb2.ApplyFeatureServiceRequest, context
     ):
-        self.proxied_registry.apply_feature_service(
-            feature_service=cast(
-                FeatureService,
-                assert_permissions(
-                    resource=FeatureService.from_proto(request.feature_service),
-                    actions=CRUD,
-                ),
+        feature_service = cast(
+            FeatureService,
+            assert_permissions_to_update(
+                resource=FeatureService.from_proto(request.feature_service),
+                getter=self.proxied_registry.get_feature_service,
+                project=request.project,
             ),
+        )
+        self.proxied_registry.apply_feature_service(
+            feature_service=feature_service,
             project=request.project,
             commit=request.commit,
         )
@@ -371,19 +383,19 @@ def DeleteFeatureService(
     def ApplySavedDataset(
         self, request: RegistryServer_pb2.ApplySavedDatasetRequest, context
     ):
-        (
-            self.proxied_registry.apply_saved_dataset(
-                saved_dataset=cast(
-                    SavedDataset,
-                    assert_permissions(
-                        resource=SavedDataset.from_proto(request.saved_dataset),
-                        actions=CRUD,
-                    ),
-                ),
+        saved_dataset = cast(
+            SavedDataset,
+            assert_permissions_to_update(
+                resource=SavedDataset.from_proto(request.saved_dataset),
+                getter=self.proxied_registry.get_saved_dataset,
                 project=request.project,
-                commit=request.commit,
             ),
         )
+        self.proxied_registry.apply_saved_dataset(
+            saved_dataset=saved_dataset,
+            project=request.project,
+            commit=request.commit,
+        )
 
         return Empty()
 
@@ -437,14 +449,16 @@ def DeleteSavedDataset(
     def ApplyValidationReference(
         self, request: RegistryServer_pb2.ApplyValidationReferenceRequest, context
     ):
-        self.proxied_registry.apply_validation_reference(
-            validation_reference=cast(
-                ValidationReference,
-                assert_permissions(
-                    ValidationReference.from_proto(request.validation_reference),
-                    actions=CRUD,
-                ),
+        validation_reference = cast(
+            ValidationReference,
+            assert_permissions_to_update(
+                resource=ValidationReference.from_proto(request.validation_reference),
+                getter=self.proxied_registry.get_validation_reference,
+                project=request.project,
             ),
+        )
+        self.proxied_registry.apply_validation_reference(
+            validation_reference=validation_reference,
             project=request.project,
             commit=request.commit,
         )
@@ -547,13 +561,16 @@ def GetInfra(self, request: RegistryServer_pb2.GetInfraRequest, context):
     def ApplyPermission(
         self, request: RegistryServer_pb2.ApplyPermissionRequest, context
     ):
-        self.proxied_registry.apply_permission(
-            permission=cast(
-                Permission,
-                assert_permissions(
-                    Permission.from_proto(request.permission), actions=CRUD
-                ),
+        permission = cast(
+            Permission,
+            assert_permissions_to_update(
+                resource=Permission.from_proto(request.permission),
+                getter=self.proxied_registry.get_permission,
+                project=request.project,
             ),
+        )
+        self.proxied_registry.apply_permission(
+            permission=permission,
             project=request.project,
             commit=request.commit,
         )
diff --git a/sdk/python/tests/unit/permissions/conftest.py b/sdk/python/tests/unit/permissions/conftest.py
@@ -3,6 +3,7 @@
 import pytest
 
 from feast import FeatureView
+from feast.entity import Entity
 from feast.infra.registry.base_registry import BaseRegistry
 from feast.permissions.decorator import require_permissions
 from feast.permissions.permission import AuthzedAction, Permission
@@ -48,7 +49,10 @@ def users() -> list[User]:
     users.append(User("r", ["reader"]))
     users.append(User("w", ["writer"]))
     users.append(User("rw", ["reader", "writer"]))
-    users.append(User("admin", ["reader", "writer", "admin"]))
+    users.append(User("special", ["reader", "writer", "special-reader"]))
+    users.append(User("updater", ["updater"]))
+    users.append(User("creator", ["creator"]))
+    users.append(User("admin", ["updater", "creator"]))
     return dict([(u.username, u) for u in users])
 
 
@@ -76,10 +80,26 @@ def security_manager() -> SecurityManager:
             name="special",
             types=FeatureView,
             name_pattern="special.*",
-            policy=RoleBasedPolicy(roles=["admin", "special-reader"]),
+            policy=RoleBasedPolicy(roles=["special-reader"]),
             actions=[AuthzedAction.DESCRIBE, AuthzedAction.UPDATE],
         )
     )
+    permissions.append(
+        Permission(
+            name="entity_updater",
+            types=Entity,
+            policy=RoleBasedPolicy(roles=["updater"]),
+            actions=[AuthzedAction.DESCRIBE, AuthzedAction.UPDATE],
+        )
+    )
+    permissions.append(
+        Permission(
+            name="entity_creator",
+            types=Entity,
+            policy=RoleBasedPolicy(roles=["creator"]),
+            actions=[AuthzedAction.CREATE],
+        )
+    )
 
     registry = Mock(spec=BaseRegistry)
     registry.list_permissions = Mock(return_value=permissions)
diff --git a/sdk/python/tests/unit/permissions/test_security_manager.py b/sdk/python/tests/unit/permissions/test_security_manager.py
