Skip to content

Commit fdf9ba9

Browse files
committed
Fixing unit tests
Signed-off-by: jyejare <[email protected]>
1 parent cafda55 commit fdf9ba9

File tree

6 files changed

+50
-20
lines changed

6 files changed

+50
-20
lines changed

infra/feast-operator/dist/install.yaml

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8211,6 +8211,12 @@ rules:
82118211
- list
82128212
- update
82138213
- watch
8214+
- apiGroups:
8215+
- authentication.k8s.io
8216+
resources:
8217+
- tokenreviews
8218+
verbs:
8219+
- create
82148220
- apiGroups:
82158221
- batch
82168222
resources:
@@ -8240,11 +8246,13 @@ rules:
82408246
- apiGroups:
82418247
- ""
82428248
resources:
8249+
- namespaces
82438250
- pods
82448251
- secrets
82458252
verbs:
82468253
- get
82478254
- list
8255+
- watch
82488256
- apiGroups:
82498257
- ""
82508258
resources:
@@ -8280,8 +8288,11 @@ rules:
82808288
- apiGroups:
82818289
- rbac.authorization.k8s.io
82828290
resources:
8291+
- clusterrolebindings
8292+
- clusterroles
82838293
- rolebindings
82848294
- roles
8295+
- subjectaccessreviews
82858296
verbs:
82868297
- create
82878298
- delete

infra/feast-operator/internal/controller/featurestore_controller_kubernetes_auth_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,7 @@ var _ = Describe("FeatureStore Controller-Kubernetes authorization", func() {
227227
feastRole)
228228
Expect(err).NotTo(HaveOccurred())
229229
Expect(feastRole.Rules).ToNot(BeEmpty())
230-
Expect(feastRole.Rules).To(HaveLen(1))
230+
Expect(feastRole.Rules).To(HaveLen(6))
231231
Expect(feastRole.Rules[0].APIGroups).To(HaveLen(1))
232232
Expect(feastRole.Rules[0].APIGroups[0]).To(Equal(rbacv1.GroupName))
233233
Expect(feastRole.Rules[0].Resources).To(HaveLen(2))

sdk/python/feast/permissions/security_manager.py

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,8 @@ def permitted_resources(
173173
A utility function to invoke the `assert_permissions` method on the global security manager.
174174
175175
If no global `SecurityManager` is defined (NoAuthConfig), all resources are permitted.
176-
If a SecurityManager exists but no user context, access is denied for security.
176+
If a SecurityManager exists but no user context and actions are requested, deny access for security.
177+
If a SecurityManager exists but user is intra-communication, allow access.
177178
178179
Args:
179180
resources: The resources for which we need to enforce authorized permission.
@@ -184,11 +185,15 @@ def permitted_resources(
184185

185186
sm = get_security_manager()
186187
if not is_auth_necessary(sm):
187-
# Check if this is NoAuthConfig (no security manager) vs missing user context
188+
# Check if this is NoAuthConfig (no security manager) vs missing user context vs intra-communication
188189
if sm is None:
189190
# NoAuthConfig: allow all resources
190191
logger.debug("NoAuthConfig enabled - allowing access to all resources")
191192
return resources
193+
elif sm.current_user is not None:
194+
# Intra-communication user: allow all resources
195+
logger.debug("Intra-communication user - allowing access to all resources")
196+
return resources
192197
else:
193198
# Security manager exists but no user context - deny access for security
194199
logger.warning(
@@ -233,8 +238,17 @@ def no_security_manager():
233238
def is_auth_necessary(sm: Optional[SecurityManager]) -> bool:
234239
intra_communication_base64 = os.getenv("INTRA_COMMUNICATION_BASE64")
235240

236-
return (
237-
sm is not None
238-
and sm.current_user is not None
239-
and sm.current_user.username != intra_communication_base64
240-
)
241+
# If no security manager, no auth is necessary
242+
if sm is None:
243+
return False
244+
245+
# If security manager exists but no user context, auth is necessary (security-first approach)
246+
if sm.current_user is None:
247+
return True
248+
249+
# If user is intra-communication, no auth is necessary
250+
if sm.current_user.username == intra_communication_base64:
251+
return False
252+
253+
# Otherwise, auth is necessary
254+
return True

sdk/python/tests/unit/permissions/auth/server/test_auth_registry_server.py

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,8 @@ def _test_list_entities(client_fs: FeatureStore, permissions: list[Permission]):
170170

171171

172172
def _no_permission_retrieved(permissions: list[Permission]) -> bool:
173-
return len(permissions) == 0
173+
# With security-first approach, no permissions means access should be denied
174+
return False
174175

175176

176177
def _test_list_permissions(
@@ -278,13 +279,17 @@ def _is_permission_enabled(
278279
permissions: list[Permission],
279280
permission: Permission,
280281
):
281-
return _is_auth_enabled(client_fs) and (
282-
_no_permission_retrieved(permissions)
283-
or (
284-
_permissions_exist_in_permission_list(
285-
[read_permissions_perm, permission], permissions
286-
)
287-
)
282+
# With security-first approach, if no permissions are defined, access should be denied
283+
if not _is_auth_enabled(client_fs):
284+
return True # No auth enabled, allow access
285+
286+
# If auth is enabled but no permissions are defined, deny access (security-first)
287+
if len(permissions) == 0:
288+
return False
289+
290+
# Check if the specific permission exists
291+
return _permissions_exist_in_permission_list(
292+
[read_permissions_perm, permission], permissions
288293
)
289294

290295

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
@pytest.mark.parametrize(
88
"username, can_read, can_write",
99
[
10-
(None, True, True),
10+
(None, False, False),
1111
("r", True, False),
1212
("w", False, True),
1313
("rw", True, True),

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
@pytest.mark.parametrize(
1616
"username, requested_actions, allowed, allowed_single, raise_error_in_assert, raise_error_in_permit, intra_communication_flag",
1717
[
18-
(None, [], True, [True, True], [False, False], False, False),
18+
(None, [], False, [False, False], [True, True], False, False),
1919
(None, [], True, [True, True], [False, False], False, True),
2020
(
2121
"r",
@@ -219,7 +219,7 @@ def test_access_SecuredFeatureView(
219219
@pytest.mark.parametrize(
220220
"username, allowed, intra_communication_flag",
221221
[
222-
(None, True, False),
222+
(None, False, False),
223223
(None, True, True),
224224
("r", False, False),
225225
("r", True, True),
@@ -275,7 +275,7 @@ def getter(name: str, project: str, allow_cache: bool):
275275
@pytest.mark.parametrize(
276276
"username, allowed, intra_communication_flag",
277277
[
278-
(None, True, False),
278+
(None, False, False),
279279
(None, True, True),
280280
("r", False, False),
281281
("r", True, True),

0 commit comments

Comments
 (0)