Intra server communication

- Code changes following review

Signed-off-by: Theodor Mihalache <tmihalac@redhat.com>

Author: tmihalac

sdk/python/feast/permissions/security_manager.py

@@ -103,14 +103,11 @@ def assert_permissions(
     Raises:
         PermissionError: If the current user is not authorized to execute the requested actions on the given resources.
     """
-    intra_communication_base64 = os.getenv("INTRA_COMMUNICATION_BASE64")
+
     sm = get_security_manager()
-    if sm is None or (
-        sm.current_user is not None
-        and sm.current_user.username == intra_communication_base64
-    ):
+    if not is_auth_necessary(sm):
         return resource
-    return sm.assert_permissions(
+    return sm.assert_permissions(  # type: ignore[union-attr]
         resources=[resource], actions=actions, filter_only=False
     )[0]
 
@@ -131,14 +128,10 @@ def permitted_resources(
         list[FeastObject]]: A filtered list of the permitted resources, possibly empty.
     """
 
-    intra_communication_base64 = os.getenv("INTRA_COMMUNICATION_BASE64")
     sm = get_security_manager()
-    if sm is None or (
-        sm.current_user is not None
-        and sm.current_user.username == intra_communication_base64
-    ):
+    if not is_auth_necessary(sm):
         return resources
-    return sm.assert_permissions(resources=resources, actions=actions, filter_only=True)
+    return sm.assert_permissions(resources=resources, actions=actions, filter_only=True)  # type: ignore[union-attr]
 
 
 """
@@ -171,3 +164,13 @@ def no_security_manager():
 
     global _sm
     _sm = None
+
+
+def is_auth_necessary(sm: Optional[SecurityManager]) -> bool:
+    intra_communication_base64 = os.getenv("INTRA_COMMUNICATION_BASE64")
+
+    return (
+        sm is not None
+        and sm.current_user is not None
+        and sm.current_user.username != intra_communication_base64
+    )

sdk/python/tests/unit/permissions/auth/test_token_parser.py

@@ -64,8 +64,16 @@ def test_oidc_token_validation_failure(mock_oauth2, oidc_config):
 
 
 @mock.patch.dict(os.environ, {"INTRA_COMMUNICATION_BASE64": "test1234"})
-@pytest.mark.parametrize("intra_communication_val", ("test1234", "my-name"))
-def test_oidc_inter_server_comm(intra_communication_val, oidc_config, monkeypatch):
+@pytest.mark.parametrize(
+    "intra_communication_val, is_intra_server",
+    [
+        ("test1234", True),
+        ("my-name", False),
+    ],
+)
+def test_oidc_inter_server_comm(
+    intra_communication_val, is_intra_server, oidc_config, monkeypatch
+):
     async def mock_oath2(self, request):
         return "OK"
 
@@ -84,7 +92,7 @@ async def mock_oath2(self, request):
         "preferred_username": f"{intra_communication_val}",
     }
 
-    if intra_communication_val != "test1234":
+    if not is_intra_server:
         user_data["resource_access"] = {_CLIENT_ID: {"roles": ["reader", "writer"]}}
 
     monkeypatch.setattr(
@@ -98,7 +106,7 @@ async def mock_oath2(self, request):
         token_parser.user_details_from_access_token(access_token=access_token)
     )
 
-    if intra_communication_val == "test1234":
+    if is_intra_server:
         assertpy.assert_that(user).is_not_none()
         assertpy.assert_that(user.username).is_equal_to(intra_communication_val)
         assertpy.assert_that(user.roles).is_equal_to([])
@@ -175,16 +183,23 @@ def test_k8s_token_validation_failure(mock_jwt, mock_config):
 
 
 @mock.patch.dict(os.environ, {"INTRA_COMMUNICATION_BASE64": "test1234"})
-@pytest.mark.parametrize("intra_communication_val", ("test1234", "my-name"))
+@pytest.mark.parametrize(
+    "intra_communication_val, is_intra_server",
+    [
+        ("test1234", True),
+        ("my-name", False),
+    ],
+)
 def test_k8s_inter_server_comm(
     intra_communication_val,
+    is_intra_server,
     oidc_config,
     request,
     rolebindings,
     clusterrolebindings,
     monkeypatch,
 ):
-    if intra_communication_val == "test1234":
+    if is_intra_server:
         subject = f":::{intra_communication_val}"
     else:
         sa_name = request.getfixturevalue("sa_name")
@@ -225,7 +240,7 @@ def test_k8s_inter_server_comm(
         token_parser.user_details_from_access_token(access_token=access_token)
     )
 
-    if intra_communication_val == "test1234":
+    if is_intra_server:
         assertpy.assert_that(user).is_not_none()
         assertpy.assert_that(user.username).is_equal_to(intra_communication_val)
         assertpy.assert_that(user.roles).is_equal_to([])

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

@@ -49,7 +49,6 @@ def users() -> list[User]:
     users.append(User("w", ["writer"]))
     users.append(User("rw", ["reader", "writer"]))
     users.append(User("admin", ["reader", "writer", "admin"]))
-    users.append(User("test1234", []))
     return dict([(u.username, u) for u in users])
 
 

sdk/python/tests/unit/permissions/test_decorator.py

@@ -5,7 +5,7 @@
 @pytest.mark.parametrize(
     "username, can_read, can_write",
     [
-        (None, False, False),
+        (None, True, True),
         ("r", True, False),
         ("w", False, True),
         ("rw", True, True),

sdk/python/tests/unit/permissions/test_security_manager.py

@@ -3,13 +3,14 @@
 
 from feast.permissions.action import READ, AuthzedAction
 from feast.permissions.security_manager import assert_permissions, permitted_resources
+from feast.permissions.user import User
 
 
 @pytest.mark.parametrize(
     "username, requested_actions, allowed, allowed_single, raise_error_in_assert, raise_error_in_permit, intra_communication_flag",
     [
-        (None, [], False, [False, False], [True, True], False, False),
-        (None, [], False, [False, False], [True, True], False, True),
+        (None, [], True, [True, True], [False, False], False, False),
+        (None, [], True, [True, True], [False, False], False, True),
         (
             "r",
             [AuthzedAction.DESCRIBE],
@@ -28,7 +29,7 @@
             False,
             True,
         ),
-        ("test1234", [], True, [True, True], [False, False], False, True),
+        ("server_intra_com_val", [], True, [True, True], [False, False], False, True),
         (
             "r",
             [AuthzedAction.UPDATE],
@@ -38,7 +39,7 @@
             False,
             False,
         ),
-        ("r", [AuthzedAction.UPDATE], False, [False, False], [True, True], False, True),
+        ("r", [AuthzedAction.UPDATE], True, [True, True], [False, False], False, True),
         (
             "w",
             [AuthzedAction.DESCRIBE],
@@ -51,8 +52,8 @@
         (
             "w",
             [AuthzedAction.DESCRIBE],
-            False,
-            [False, False],
+            True,
+            [True, True],
             [True, True],
             False,
             True,
@@ -115,10 +116,10 @@
         (
             "rw",
             [AuthzedAction.DESCRIBE, AuthzedAction.UPDATE],
-            False,
-            [False, False],
-            [True, True],
             True,
+            [True, True],
+            [False, False],
+            False,
             True,
         ),
         (
@@ -133,10 +134,10 @@
         (
             "admin",
             [AuthzedAction.DESCRIBE, AuthzedAction.UPDATE],
-            False,
-            [False, True],
-            [True, False],
             True,
+            [True, True],
+            [False, False],
+            False,
             True,
         ),
         (
@@ -151,10 +152,10 @@
         (
             "admin",
             READ + [AuthzedAction.UPDATE],
-            False,
-            [False, False],
-            [True, True],
             True,
+            [True, True],
+            [False, False],
+            False,
             True,
         ),
     ],
@@ -172,17 +173,18 @@ def test_access_SecuredFeatureView(
     intra_communication_flag,
     monkeypatch,
 ):
+    sm = security_manager
+    user = users.get(username)
+    sm.set_current_user(user)
+
     if intra_communication_flag:
-        monkeypatch.setenv("INTRA_COMMUNICATION_BASE64", "test1234")
+        monkeypatch.setenv("INTRA_COMMUNICATION_BASE64", "server_intra_com_val")
+        sm.set_current_user(User("server_intra_com_val", []))
     else:
         monkeypatch.delenv("INTRA_COMMUNICATION_BASE64", False)
 
-    sm = security_manager
     resources = feature_views
 
-    user = users.get(username)
-    sm.set_current_user(user)
-
     result = []
     if raise_error_in_permit:
         with pytest.raises(PermissionError):