From 80fca740178540df5871a9cfc8e0906d76076177 Mon Sep 17 00:00:00 2001 From: Daniele Martinoli <86618610+dmartinol@users.noreply.github.com> Date: Tue, 2 Jul 2024 13:13:51 +0200 Subject: [PATCH 1/3] replaced aggregated actions with aliases for QUERY and WRITE and ALL Signed-off-by: Daniele Martinoli <86618610+dmartinol@users.noreply.github.com> --- sdk/python/feast/infra/registry/registry.py | 5 ++++- sdk/python/feast/permissions/action.py | 18 +++++++++++++++--- sdk/python/feast/permissions/matcher.py | 4 ---- sdk/python/feast/permissions/permission.py | 6 +++--- sdk/python/tests/unit/permissions/conftest.py | 6 +++--- .../tests/unit/permissions/test_permission.py | 10 +++++----- .../unit/permissions/test_security_manager.py | 14 +++++++------- 7 files changed, 37 insertions(+), 26 deletions(-) diff --git a/sdk/python/feast/infra/registry/registry.py b/sdk/python/feast/infra/registry/registry.py index def628d9e8..a0ee9645f9 100644 --- a/sdk/python/feast/infra/registry/registry.py +++ b/sdk/python/feast/infra/registry/registry.py @@ -928,7 +928,10 @@ def apply_permission( ): registry = self._prepare_registry_for_changes(project) for idx, existing_permission_proto in enumerate(registry.permissions): - if existing_permission_proto.name == permission.name and existing_permission_proto.project == project: + if ( + existing_permission_proto.name == permission.name + and existing_permission_proto.project == project + ): del registry.permissions[idx] permission_proto = permission.to_proto() permission_proto.project = project diff --git a/sdk/python/feast/permissions/action.py b/sdk/python/feast/permissions/action.py index 655bfcbdeb..82125848a3 100644 --- a/sdk/python/feast/permissions/action.py +++ b/sdk/python/feast/permissions/action.py @@ -6,14 +6,26 @@ class AuthzedAction(enum.Enum): Identify the type of action being secured by the permissions framework, according to the familiar CRUD and Feast terminology. """ - ALL = "all" # All actions CREATE = "create" # Create an instance READ = "read" # Access the instance state UPDATE = "update" # Update the instance state DELETE = "delete" # Deelete an instance - QUERY = "query" # Query both online and offline stores QUERY_ONLINE = "query_online" # Query the online store only QUERY_OFFLINE = "query_offline" # Query the offline store only - WRITE = "write" # Query on any store WRITE_ONLINE = "write_online" # Write to the online store only WRITE_OFFLINE = "write_offline" # Write to the offline store only + + +# Alias for all available actions +ALL_ACTIONS = [a for a in AuthzedAction.__members__.values()] + +# Alias for all query actions +QUERY = [ + AuthzedAction.QUERY_OFFLINE, + AuthzedAction.QUERY_ONLINE, +] +# Alias for all write actions +WRITE = [ + AuthzedAction.WRITE_OFFLINE, + AuthzedAction.WRITE_ONLINE, +] diff --git a/sdk/python/feast/permissions/matcher.py b/sdk/python/feast/permissions/matcher.py index 634cbf2f0e..00a820d641 100644 --- a/sdk/python/feast/permissions/matcher.py +++ b/sdk/python/feast/permissions/matcher.py @@ -156,9 +156,5 @@ def actions_match_config( Returns: 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 requested_actions) diff --git a/sdk/python/feast/permissions/permission.py b/sdk/python/feast/permissions/permission.py index 57aa0afd59..be8abacdd4 100644 --- a/sdk/python/feast/permissions/permission.py +++ b/sdk/python/feast/permissions/permission.py @@ -4,7 +4,7 @@ from typing import TYPE_CHECKING, Any, Dict, Optional, Union from feast.importer import import_class -from feast.permissions.action import AuthzedAction +from feast.permissions.action import ALL_ACTIONS, AuthzedAction from feast.permissions.decision import DecisionStrategy from feast.permissions.matcher import actions_match_config, resource_match_config from feast.permissions.policy import AllowAll, Policy @@ -34,7 +34,7 @@ class Permission(ABC): 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`. + actions: The actions authorized by this permission. Defaults to `ALL_ACTIONS`. policy: The policy to be applied to validate a client request. """ @@ -54,7 +54,7 @@ def __init__( with_subclasses: bool = True, name_pattern: Optional[str] = None, required_tags: Optional[dict[str, str]] = None, - actions: Union[list[AuthzedAction], AuthzedAction] = AuthzedAction.ALL, + actions: Union[list[AuthzedAction], AuthzedAction] = ALL_ACTIONS, policy: Policy = AllowAll, tags: Optional[Dict[str, str]] = None, ): diff --git a/sdk/python/tests/unit/permissions/conftest.py b/sdk/python/tests/unit/permissions/conftest.py index 19a84742b5..ee2e258981 100644 --- a/sdk/python/tests/unit/permissions/conftest.py +++ b/sdk/python/tests/unit/permissions/conftest.py @@ -25,7 +25,7 @@ def __init__(self, name, tags): def read_protected(self) -> bool: return True - @require_permissions(actions=[AuthzedAction.WRITE]) + @require_permissions(actions=[AuthzedAction.UPDATE]) def write_protected(self) -> bool: return True @@ -68,7 +68,7 @@ def security_manager() -> SecurityManager: types=FeatureView, with_subclasses=True, policy=RoleBasedPolicy(roles=["writer"]), - actions=[AuthzedAction.WRITE], + actions=[AuthzedAction.UPDATE], ) ) permissions.append( @@ -78,7 +78,7 @@ def security_manager() -> SecurityManager: with_subclasses=True, name_pattern="special.*", policy=RoleBasedPolicy(roles=["admin", "special-reader"]), - actions=[AuthzedAction.READ, AuthzedAction.WRITE], + actions=[AuthzedAction.READ, AuthzedAction.UPDATE], ) ) diff --git a/sdk/python/tests/unit/permissions/test_permission.py b/sdk/python/tests/unit/permissions/test_permission.py index 7f2a4431ef..86659d4ff9 100644 --- a/sdk/python/tests/unit/permissions/test_permission.py +++ b/sdk/python/tests/unit/permissions/test_permission.py @@ -10,8 +10,8 @@ from feast.feature_service import FeatureService from feast.feature_view import FeatureView from feast.on_demand_feature_view import OnDemandFeatureView +from feast.permissions.action import ALL_ACTIONS, AuthzedAction from feast.permissions.permission import ( - AuthzedAction, DecisionStrategy, Permission, ) @@ -40,7 +40,7 @@ def test_defaults(): assertpy.assert_that(p.name_pattern).is_none() assertpy.assert_that(p.required_tags).is_none() assertpy.assert_that(type(p.actions)).is_equal_to(list) - assertpy.assert_that(p.actions[0]).is_equal_to(AuthzedAction.ALL) + assertpy.assert_that(p.actions).is_equal_to(ALL_ACTIONS) assertpy.assert_that(type(p.actions)).is_equal_to(list) assertpy.assert_that(isinstance(p.policy, Policy)).is_true() assertpy.assert_that(p.policy).is_equal_to(AllowAll) @@ -55,8 +55,8 @@ def test_defaults(): ({"types": [FeatureView, FeatureService]}, True), ({"actions": None}, False), ({"actions": []}, False), - ({"actions": AuthzedAction.ALL}, True), - ({"actions": [AuthzedAction.ALL]}, True), + ({"actions": ALL_ACTIONS}, True), + ({"actions": ALL_ACTIONS}, True), ({"actions": [AuthzedAction.CREATE, AuthzedAction.DELETE]}, True), ({"policy": None}, False), ({"policy": []}, False), @@ -256,7 +256,7 @@ def test_resource_match_with_tags(required_tags, tags, result): @pytest.mark.parametrize( ("permitted_actions, requested_actions, result"), - [(AuthzedAction.ALL, a, True) for a in AuthzedAction.__members__.values()] + [(ALL_ACTIONS, [a], True) for a in AuthzedAction.__members__.values()] + [ ( [AuthzedAction.CREATE, AuthzedAction.DELETE], diff --git a/sdk/python/tests/unit/permissions/test_security_manager.py b/sdk/python/tests/unit/permissions/test_security_manager.py index c8a383a97f..9eb80260c3 100644 --- a/sdk/python/tests/unit/permissions/test_security_manager.py +++ b/sdk/python/tests/unit/permissions/test_security_manager.py @@ -1,7 +1,7 @@ import assertpy import pytest -from feast.permissions.action import AuthzedAction +from feast.permissions.action import QUERY, AuthzedAction from feast.permissions.decision import DecisionStrategy from feast.permissions.permission import Permission from feast.permissions.security_manager import assert_permissions, permitted_resources @@ -17,14 +17,14 @@ def setup_module(): [ (None, [], False, [False, False]), ("r", [AuthzedAction.READ], False, [True, False]), - ("r", [AuthzedAction.WRITE], False, [False, False]), + ("r", [AuthzedAction.UPDATE], False, [False, False]), ("w", [AuthzedAction.READ], False, [False, False]), - ("w", [AuthzedAction.WRITE], False, [True, False]), + ("w", [AuthzedAction.UPDATE], False, [True, False]), ("rw", [AuthzedAction.READ], False, [True, False]), - ("rw", [AuthzedAction.WRITE], False, [True, False]), - ("rw", [AuthzedAction.READ, AuthzedAction.WRITE], False, [True, False]), - ("admin", [AuthzedAction.READ, AuthzedAction.WRITE], True, [True, True]), - ("admin", [AuthzedAction.QUERY, AuthzedAction.WRITE], True, [True, True]), + ("rw", [AuthzedAction.UPDATE], False, [True, False]), + ("rw", [AuthzedAction.READ, AuthzedAction.UPDATE], False, [True, False]), + ("admin", [AuthzedAction.READ, AuthzedAction.UPDATE], True, [True, True]), + ("admin", QUERY + [AuthzedAction.UPDATE], True, [True, True]), ], ) def test_access_SecuredFeatureView( From 8a92b66a27d77cec1c7359d4939158fb41329737 Mon Sep 17 00:00:00 2001 From: Daniele Martinoli <86618610+dmartinol@users.noreply.github.com> Date: Tue, 2 Jul 2024 13:20:54 +0200 Subject: [PATCH 2/3] Updated user guide Signed-off-by: Daniele Martinoli <86618610+dmartinol@users.noreply.github.com> --- docs/getting-started/concepts/permission.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/getting-started/concepts/permission.md b/docs/getting-started/concepts/permission.md index 9ada6c162e..466d685b71 100644 --- a/docs/getting-started/concepts/permission.md +++ b/docs/getting-started/concepts/permission.md @@ -39,7 +39,7 @@ The `Permission` class identifies a single permission configured on the feature - `with_subclasses`: Specify if sub-classes are included in the resource match or not. 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. Defaults to `None`, meaning that no tags filtering is applied. -- `actions`: The actions authorized by this permission. Defaults to `AuthzedAction.ALL`. +- `actions`: The actions authorized by this permission. Defaults to `ALL_VALUES`, an alias defined in the `action` module. - `policy`: The policy to be applied to validate a client request. Given the above definitions, the feature store can be configured with granular control over each resource, enabling partitioned access by From 854981861c608741c6bb9efd3e85246ba4fcd9e7 Mon Sep 17 00:00:00 2001 From: Daniele Martinoli <86618610+dmartinol@users.noreply.github.com> Date: Tue, 2 Jul 2024 14:08:09 +0200 Subject: [PATCH 3/3] Updated enum in proto Signed-off-by: Daniele Martinoli <86618610+dmartinol@users.noreply.github.com> --- protos/feast/core/Permission.proto | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/protos/feast/core/Permission.proto b/protos/feast/core/Permission.proto index 60a153d1cb..aba9f0bb40 100644 --- a/protos/feast/core/Permission.proto +++ b/protos/feast/core/Permission.proto @@ -10,17 +10,14 @@ import "feast/core/Policy.proto"; message Permission { enum AuthzedAction { - ALL = 0; - CREATE = 1; - READ = 2; - UPDATE = 3; - DELETE = 4; - QUERY = 5; - QUERY_ONLINE = 6; - QUERY_OFFLINE = 7; - WRITE = 8; - WRITE_ONLINE = 9; - WRITE_OFFLINE = 10; + CREATE = 0; + READ = 1; + UPDATE = 2; + DELETE = 3; + QUERY_ONLINE = 4; + QUERY_OFFLINE = 5; + WRITE_ONLINE = 6; + WRITE_OFFLINE = 7; } // Name of the permission. Must be unique. Not updated.