From 8c73f699c85228ac406f810372f9a9ac5fc7b65a Mon Sep 17 00:00:00 2001 From: Daniele Martinoli <86618610+dmartinol@users.noreply.github.com> Date: Mon, 26 Aug 2024 18:39:33 +0200 Subject: [PATCH 1/2] Validating permission to update an existing request on both the new and the old instance Signed-off-by: Daniele Martinoli <86618610+dmartinol@users.noreply.github.com> --- sdk/python/feast/permissions/enforcer.py | 2 +- .../feast/permissions/security_manager.py | 48 ++++++- sdk/python/feast/registry_server.py | 119 +++++++++++------- 3 files changed, 118 insertions(+), 51 deletions(-) diff --git a/sdk/python/feast/permissions/enforcer.py b/sdk/python/feast/permissions/enforcer.py index af41d12a2c..ae45b8a78b 100644 --- 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 index 178db6e6e9..fa37cea021 100644 --- 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, + actions: Union[AuthzedAction, List[AuthzedAction]], + getter: Callable[[str, str, bool], FeastObject], + project: str, + allow_cache: bool = True, +) -> FeastObject: + """ + Verify if the current user is authorized to execute the requested actions on the given resource and also checks the authorization on + the existing resource, if any. + + If no permissions are defined, the result is to deny the execution. + + Args: + resource: The resources for which we need to enforce authorized permission. + actions: The requested actions to be authorized. + 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. + """ + resource_to_update = assert_permissions(resource=resource, actions=actions) + try: + existing_resource = getter( + name=resource_to_update.name, + project=project, + allow_cache=allow_cache, + ) # type: ignore[call-arg] + assert_permissions(resource=existing_resource, actions=actions) + except FeastObjectNotFoundException: + pass + 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 index 6b37aba08d..7f4c623169 100644 --- a/sdk/python/feast/registry_server.py +++ b/sdk/python/feast/registry_server.py @@ -18,7 +18,11 @@ from feast.on_demand_feature_view import OnDemandFeatureView from feast.permissions.action import CRUD, 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,17 @@ 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), + actions=CRUD, + getter=self.proxied_registry.get_entity, + project=request.project, ), + ) + self.proxied_registry.apply_entity( + entity=entity, project=request.project, commit=request.commit, ) @@ -95,19 +102,20 @@ 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), + actions=CRUD, + 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 +190,17 @@ 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, + actions=CRUD, + # 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,11 +318,17 @@ def ListOnDemandFeatureViews( def ApplyFeatureService( self, request: RegistryServer_pb2.ApplyFeatureServiceRequest, context ): + feature_service = assert_permissions_to_update( + resource=FeatureService.from_proto(request.feature_service), + actions=CRUD, + getter=self.proxied_registry.get_feature_service, + project=request.project, + ) self.proxied_registry.apply_feature_service( feature_service=cast( FeatureService, assert_permissions( - resource=FeatureService.from_proto(request.feature_service), + resource=feature_service, actions=CRUD, ), ), @@ -371,19 +390,20 @@ 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), + actions=CRUD, + 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 +457,17 @@ 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), + actions=CRUD, + 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 +570,17 @@ 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), + actions=CRUD, + getter=self.proxied_registry.get_permission, + project=request.project, ), + ) + self.proxied_registry.apply_permission( + permission=permission, project=request.project, commit=request.commit, ) From 6a00545da9ae957a77c64fdabcc00a2728d917b1 Mon Sep 17 00:00:00 2001 From: Daniele Martinoli Date: Tue, 27 Aug 2024 11:50:30 +0200 Subject: [PATCH 2/2] Reviewed update permission logic as per comment, added UT Signed-off-by: Daniele Martinoli --- .../feast/permissions/security_manager.py | 14 +-- sdk/python/feast/registry_server.py | 28 ++---- sdk/python/tests/unit/permissions/conftest.py | 24 ++++- .../unit/permissions/test_security_manager.py | 92 ++++++++++++++++++- 4 files changed, 127 insertions(+), 31 deletions(-) diff --git a/sdk/python/feast/permissions/security_manager.py b/sdk/python/feast/permissions/security_manager.py index fa37cea021..2322602388 100644 --- a/sdk/python/feast/permissions/security_manager.py +++ b/sdk/python/feast/permissions/security_manager.py @@ -87,20 +87,19 @@ def assert_permissions( def assert_permissions_to_update( resource: FeastObject, - actions: Union[AuthzedAction, List[AuthzedAction]], getter: Callable[[str, str, bool], FeastObject], project: str, allow_cache: bool = True, ) -> FeastObject: """ - Verify if the current user is authorized to execute the requested actions on the given resource and also checks the authorization on - the existing resource, if any. + 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. - actions: The requested actions to be authorized. 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. @@ -111,16 +110,17 @@ def assert_permissions_to_update( Raises: PermissionError: If the current user is not authorized to execute all the requested actions on the given resource or on the existing one. """ - resource_to_update = assert_permissions(resource=resource, actions=actions) + actions = [AuthzedAction.DESCRIBE, AuthzedAction.UPDATE] try: existing_resource = getter( - name=resource_to_update.name, + name=resource.name, project=project, allow_cache=allow_cache, ) # type: ignore[call-arg] assert_permissions(resource=existing_resource, actions=actions) except FeastObjectNotFoundException: - pass + actions = [AuthzedAction.CREATE] + resource_to_update = assert_permissions(resource=resource, actions=actions) return resource_to_update diff --git a/sdk/python/feast/registry_server.py b/sdk/python/feast/registry_server.py index 7f4c623169..7b779e9f9e 100644 --- a/sdk/python/feast/registry_server.py +++ b/sdk/python/feast/registry_server.py @@ -16,7 +16,7 @@ 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, @@ -45,7 +45,6 @@ def ApplyEntity(self, request: RegistryServer_pb2.ApplyEntityRequest, context): Entity, assert_permissions_to_update( resource=Entity.from_proto(request.entity), - actions=CRUD, getter=self.proxied_registry.get_entity, project=request.project, ), @@ -106,7 +105,6 @@ def ApplyDataSource( DataSource, assert_permissions_to_update( resource=DataSource.from_proto(request.data_source), - actions=CRUD, getter=self.proxied_registry.get_data_source, project=request.project, ), @@ -192,7 +190,6 @@ def ApplyFeatureView( assert_permissions_to_update( resource=feature_view, - actions=CRUD, # Will replace with the new get_any_feature_view method later getter=self.proxied_registry.get_feature_view, project=request.project, @@ -318,20 +315,16 @@ def ListOnDemandFeatureViews( def ApplyFeatureService( self, request: RegistryServer_pb2.ApplyFeatureServiceRequest, context ): - feature_service = assert_permissions_to_update( - resource=FeatureService.from_proto(request.feature_service), - actions=CRUD, - getter=self.proxied_registry.get_feature_service, - project=request.project, + 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=cast( - FeatureService, - assert_permissions( - resource=feature_service, - actions=CRUD, - ), - ), + feature_service=feature_service, project=request.project, commit=request.commit, ) @@ -394,7 +387,6 @@ def ApplySavedDataset( SavedDataset, assert_permissions_to_update( resource=SavedDataset.from_proto(request.saved_dataset), - actions=CRUD, getter=self.proxied_registry.get_saved_dataset, project=request.project, ), @@ -461,7 +453,6 @@ def ApplyValidationReference( ValidationReference, assert_permissions_to_update( resource=ValidationReference.from_proto(request.validation_reference), - actions=CRUD, getter=self.proxied_registry.get_validation_reference, project=request.project, ), @@ -574,7 +565,6 @@ def ApplyPermission( Permission, assert_permissions_to_update( resource=Permission.from_proto(request.permission), - actions=CRUD, getter=self.proxied_registry.get_permission, project=request.project, ), diff --git a/sdk/python/tests/unit/permissions/conftest.py b/sdk/python/tests/unit/permissions/conftest.py index 7cd944fb47..6adbc6ec54 100644 --- 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 index 192542da78..228dddb01f 100644 --- a/sdk/python/tests/unit/permissions/test_security_manager.py +++ b/sdk/python/tests/unit/permissions/test_security_manager.py @@ -1,8 +1,14 @@ import assertpy import pytest +from feast.entity import Entity +from feast.errors import FeastObjectNotFoundException from feast.permissions.action import READ, AuthzedAction -from feast.permissions.security_manager import assert_permissions, permitted_resources +from feast.permissions.security_manager import ( + assert_permissions, + assert_permissions_to_update, + permitted_resources, +) @pytest.mark.parametrize( @@ -24,7 +30,7 @@ True, ), ( - "admin", + "special", [AuthzedAction.DESCRIBE, AuthzedAction.UPDATE], False, [False, True], @@ -32,7 +38,7 @@ True, ), ( - "admin", + "special", READ + [AuthzedAction.UPDATE], False, [False, False], @@ -81,3 +87,83 @@ def test_access_SecuredFeatureView( else: result = assert_permissions(resource=r, actions=requested_actions) assertpy.assert_that(result).is_none() + + +@pytest.mark.parametrize( + "username, allowed", + [ + (None, False), + ("r", False), + ("w", False), + ("rw", False), + ("special", False), + ("updater", False), + ("creator", True), + ("admin", True), + ], +) +def test_create_entity( + security_manager, + users, + username, + allowed, +): + sm = security_manager + entity = Entity( + name="", + ) + + user = users.get(username) + sm.set_current_user(user) + + def getter(name: str, project: str, allow_cache: bool): + raise FeastObjectNotFoundException() + + if allowed: + result = assert_permissions_to_update( + resource=entity, getter=getter, project="" + ) + assertpy.assert_that(result).is_equal_to(entity) + else: + with pytest.raises(PermissionError): + assert_permissions_to_update(resource=entity, getter=getter, project="") + + +@pytest.mark.parametrize( + "username, allowed", + [ + (None, False), + ("r", False), + ("w", False), + ("rw", False), + ("special", False), + ("updater", True), + ("creator", False), + ("admin", True), + ], +) +def test_update_entity( + security_manager, + users, + username, + allowed, +): + sm = security_manager + entity = Entity( + name="", + ) + + user = users.get(username) + sm.set_current_user(user) + + def getter(name: str, project: str, allow_cache: bool): + return entity + + if allowed: + result = assert_permissions_to_update( + resource=entity, getter=getter, project="" + ) + assertpy.assert_that(result).is_equal_to(entity) + else: + with pytest.raises(PermissionError): + assert_permissions_to_update(resource=entity, getter=getter, project="")