Skip to content

Commit 635a01b

Browse files
dmartinoldmartinol
authored
fix: Validating permission to update an existing request on both the new and the old instance (#4449)
* Validating permission to update an existing request on both the new and the old instance Signed-off-by: Daniele Martinoli <[email protected]> * Reviewed update permission logic as per comment, added UT Signed-off-by: Daniele Martinoli <[email protected]> --------- Signed-off-by: Daniele Martinoli <[email protected]> Signed-off-by: Daniele Martinoli <[email protected]>
1 parent 6b2f026 commit 635a01b

File tree

5 files changed

+226
-63
lines changed

5 files changed

+226
-63
lines changed

‎sdk/python/feast/permissions/enforcer.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ def enforce_policy(
2222
Define the logic to apply the configured permissions when a given action is requested on
2323
a protected resource.
2424
25-
If no permissions are defined, the result is to allow the execution.
25+
If no permissions are defined, the result is to deny the execution.
2626
2727
Args:
2828
permissions: The configured set of `Permission`.

‎sdk/python/feast/permissions/security_manager.py‎

Lines changed: 44 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
import logging
22
from contextvars import ContextVar
3-
from typing import List, Optional, Union
3+
from typing import Callable, List, Optional, Union
44

5+
from feast.errors import FeastObjectNotFoundException
56
from feast.feast_object import FeastObject
67
from feast.infra.registry.base_registry import BaseRegistry
78
from feast.permissions.action import AuthzedAction
@@ -59,9 +60,9 @@ def assert_permissions(
5960
filter_only: bool = False,
6061
) -> list[FeastObject]:
6162
"""
62-
Verify if the current user is authorized ro execute the requested actions on the given resources.
63+
Verify if the current user is authorized to execute the requested actions on the given resources.
6364
64-
If no permissions are defined, the result is to allow the execution.
65+
If no permissions are defined, the result is to deny the execution.
6566
6667
Args:
6768
resources: The resources for which we need to enforce authorized permission.
@@ -73,7 +74,7 @@ def assert_permissions(
7374
list[FeastObject]: A filtered list of the permitted resources, possibly empty.
7475
7576
Raises:
76-
PermissionError: If the current user is not authorized to eecute all the requested actions on the given resources.
77+
PermissionError: If the current user is not authorized to execute all the requested actions on the given resources.
7778
"""
7879
return enforce_policy(
7980
permissions=self.permissions,
@@ -84,6 +85,45 @@ def assert_permissions(
8485
)
8586

8687

88+
def assert_permissions_to_update(
89+
resource: FeastObject,
90+
getter: Callable[[str, str, bool], FeastObject],
91+
project: str,
92+
allow_cache: bool = True,
93+
) -> FeastObject:
94+
"""
95+
Verify if the current user is authorized to create or update the given resource.
96+
If the resource already exists, the user must be granted permission to execute DESCRIBE and UPDATE actions.
97+
If the resource does not exist, the user must be granted permission to execute the CREATE action.
98+
99+
If no permissions are defined, the result is to deny the execution.
100+
101+
Args:
102+
resource: The resources for which we need to enforce authorized permission.
103+
getter: The getter function used to retrieve the existing resource instance by name.
104+
The signature must be `get_permission(self, name: str, project: str, allow_cache: bool)`
105+
project: The project nane used in the getter function.
106+
allow_cache: Whether to use cached data. Defaults to `True`.
107+
Returns:
108+
FeastObject: The original `resource`, if permitted.
109+
110+
Raises:
111+
PermissionError: If the current user is not authorized to execute all the requested actions on the given resource or on the existing one.
112+
"""
113+
actions = [AuthzedAction.DESCRIBE, AuthzedAction.UPDATE]
114+
try:
115+
existing_resource = getter(
116+
name=resource.name,
117+
project=project,
118+
allow_cache=allow_cache,
119+
) # type: ignore[call-arg]
120+
assert_permissions(resource=existing_resource, actions=actions)
121+
except FeastObjectNotFoundException:
122+
actions = [AuthzedAction.CREATE]
123+
resource_to_update = assert_permissions(resource=resource, actions=actions)
124+
return resource_to_update
125+
126+
87127
def assert_permissions(
88128
resource: FeastObject,
89129
actions: Union[AuthzedAction, List[AuthzedAction]],

‎sdk/python/feast/registry_server.py‎

Lines changed: 70 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,13 @@
1616
from feast.infra.infra_object import Infra
1717
from feast.infra.registry.base_registry import BaseRegistry
1818
from feast.on_demand_feature_view import OnDemandFeatureView
19-
from feast.permissions.action import CRUD, AuthzedAction
19+
from feast.permissions.action import AuthzedAction
2020
from feast.permissions.permission import Permission
21-
from feast.permissions.security_manager import assert_permissions, permitted_resources
21+
from feast.permissions.security_manager import (
22+
assert_permissions,
23+
assert_permissions_to_update,
24+
permitted_resources,
25+
)
2226
from feast.permissions.server.grpc import grpc_interceptors
2327
from feast.permissions.server.utils import (
2428
ServerType,
@@ -37,14 +41,16 @@ def __init__(self, registry: BaseRegistry) -> None:
3741
self.proxied_registry = registry
3842

3943
def ApplyEntity(self, request: RegistryServer_pb2.ApplyEntityRequest, context):
40-
self.proxied_registry.apply_entity(
41-
entity=cast(
42-
Entity,
43-
assert_permissions(
44-
resource=Entity.from_proto(request.entity),
45-
actions=CRUD,
46-
),
44+
entity = cast(
45+
Entity,
46+
assert_permissions_to_update(
47+
resource=Entity.from_proto(request.entity),
48+
getter=self.proxied_registry.get_entity,
49+
project=request.project,
4750
),
51+
)
52+
self.proxied_registry.apply_entity(
53+
entity=entity,
4854
project=request.project,
4955
commit=request.commit,
5056
)
@@ -95,19 +101,19 @@ def DeleteEntity(self, request: RegistryServer_pb2.DeleteEntityRequest, context)
95101
def ApplyDataSource(
96102
self, request: RegistryServer_pb2.ApplyDataSourceRequest, context
97103
):
98-
(
99-
self.proxied_registry.apply_data_source(
100-
data_source=cast(
101-
DataSource,
102-
assert_permissions(
103-
resource=DataSource.from_proto(request.data_source),
104-
actions=CRUD,
105-
),
106-
),
104+
data_source = cast(
105+
DataSource,
106+
assert_permissions_to_update(
107+
resource=DataSource.from_proto(request.data_source),
108+
getter=self.proxied_registry.get_data_source,
107109
project=request.project,
108-
commit=request.commit,
109110
),
110111
)
112+
self.proxied_registry.apply_data_source(
113+
data_source=data_source,
114+
project=request.project,
115+
commit=request.commit,
116+
)
111117

112118
return Empty()
113119

@@ -182,12 +188,16 @@ def ApplyFeatureView(
182188
elif feature_view_type == "stream_feature_view":
183189
feature_view = StreamFeatureView.from_proto(request.stream_feature_view)
184190

191+
assert_permissions_to_update(
192+
resource=feature_view,
193+
# Will replace with the new get_any_feature_view method later
194+
getter=self.proxied_registry.get_feature_view,
195+
project=request.project,
196+
)
197+
185198
(
186199
self.proxied_registry.apply_feature_view(
187-
feature_view=cast(
188-
FeatureView,
189-
assert_permissions(resource=feature_view, actions=CRUD),
190-
),
200+
feature_view=feature_view,
191201
project=request.project,
192202
commit=request.commit,
193203
),
@@ -305,14 +315,16 @@ def ListOnDemandFeatureViews(
305315
def ApplyFeatureService(
306316
self, request: RegistryServer_pb2.ApplyFeatureServiceRequest, context
307317
):
308-
self.proxied_registry.apply_feature_service(
309-
feature_service=cast(
310-
FeatureService,
311-
assert_permissions(
312-
resource=FeatureService.from_proto(request.feature_service),
313-
actions=CRUD,
314-
),
318+
feature_service = cast(
319+
FeatureService,
320+
assert_permissions_to_update(
321+
resource=FeatureService.from_proto(request.feature_service),
322+
getter=self.proxied_registry.get_feature_service,
323+
project=request.project,
315324
),
325+
)
326+
self.proxied_registry.apply_feature_service(
327+
feature_service=feature_service,
316328
project=request.project,
317329
commit=request.commit,
318330
)
@@ -371,19 +383,19 @@ def DeleteFeatureService(
371383
def ApplySavedDataset(
372384
self, request: RegistryServer_pb2.ApplySavedDatasetRequest, context
373385
):
374-
(
375-
self.proxied_registry.apply_saved_dataset(
376-
saved_dataset=cast(
377-
SavedDataset,
378-
assert_permissions(
379-
resource=SavedDataset.from_proto(request.saved_dataset),
380-
actions=CRUD,
381-
),
382-
),
386+
saved_dataset = cast(
387+
SavedDataset,
388+
assert_permissions_to_update(
389+
resource=SavedDataset.from_proto(request.saved_dataset),
390+
getter=self.proxied_registry.get_saved_dataset,
383391
project=request.project,
384-
commit=request.commit,
385392
),
386393
)
394+
self.proxied_registry.apply_saved_dataset(
395+
saved_dataset=saved_dataset,
396+
project=request.project,
397+
commit=request.commit,
398+
)
387399

388400
return Empty()
389401

@@ -437,14 +449,16 @@ def DeleteSavedDataset(
437449
def ApplyValidationReference(
438450
self, request: RegistryServer_pb2.ApplyValidationReferenceRequest, context
439451
):
440-
self.proxied_registry.apply_validation_reference(
441-
validation_reference=cast(
442-
ValidationReference,
443-
assert_permissions(
444-
ValidationReference.from_proto(request.validation_reference),
445-
actions=CRUD,
446-
),
452+
validation_reference = cast(
453+
ValidationReference,
454+
assert_permissions_to_update(
455+
resource=ValidationReference.from_proto(request.validation_reference),
456+
getter=self.proxied_registry.get_validation_reference,
457+
project=request.project,
447458
),
459+
)
460+
self.proxied_registry.apply_validation_reference(
461+
validation_reference=validation_reference,
448462
project=request.project,
449463
commit=request.commit,
450464
)
@@ -547,13 +561,16 @@ def GetInfra(self, request: RegistryServer_pb2.GetInfraRequest, context):
547561
def ApplyPermission(
548562
self, request: RegistryServer_pb2.ApplyPermissionRequest, context
549563
):
550-
self.proxied_registry.apply_permission(
551-
permission=cast(
552-
Permission,
553-
assert_permissions(
554-
Permission.from_proto(request.permission), actions=CRUD
555-
),
564+
permission = cast(
565+
Permission,
566+
assert_permissions_to_update(
567+
resource=Permission.from_proto(request.permission),
568+
getter=self.proxied_registry.get_permission,
569+
project=request.project,
556570
),
571+
)
572+
self.proxied_registry.apply_permission(
573+
permission=permission,
557574
project=request.project,
558575
commit=request.commit,
559576
)

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

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import pytest
44

55
from feast import FeatureView
6+
from feast.entity import Entity
67
from feast.infra.registry.base_registry import BaseRegistry
78
from feast.permissions.decorator import require_permissions
89
from feast.permissions.permission import AuthzedAction, Permission
@@ -48,7 +49,10 @@ def users() -> list[User]:
4849
users.append(User("r", ["reader"]))
4950
users.append(User("w", ["writer"]))
5051
users.append(User("rw", ["reader", "writer"]))
51-
users.append(User("admin", ["reader", "writer", "admin"]))
52+
users.append(User("special", ["reader", "writer", "special-reader"]))
53+
users.append(User("updater", ["updater"]))
54+
users.append(User("creator", ["creator"]))
55+
users.append(User("admin", ["updater", "creator"]))
5256
return dict([(u.username, u) for u in users])
5357

5458

@@ -76,10 +80,26 @@ def security_manager() -> SecurityManager:
7680
name="special",
7781
types=FeatureView,
7882
name_pattern="special.*",
79-
policy=RoleBasedPolicy(roles=["admin", "special-reader"]),
83+
policy=RoleBasedPolicy(roles=["special-reader"]),
8084
actions=[AuthzedAction.DESCRIBE, AuthzedAction.UPDATE],
8185
)
8286
)
87+
permissions.append(
88+
Permission(
89+
name="entity_updater",
90+
types=Entity,
91+
policy=RoleBasedPolicy(roles=["updater"]),
92+
actions=[AuthzedAction.DESCRIBE, AuthzedAction.UPDATE],
93+
)
94+
)
95+
permissions.append(
96+
Permission(
97+
name="entity_creator",
98+
types=Entity,
99+
policy=RoleBasedPolicy(roles=["creator"]),
100+
actions=[AuthzedAction.CREATE],
101+
)
102+
)
83103

84104
registry = Mock(spec=BaseRegistry)
85105
registry.list_permissions = Mock(return_value=permissions)

0 commit comments

Comments
 (0)