⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 47.08%. Comparing base (81e1546) to head (bb18433).
⚠️ Report is 3 commits behind head on master.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@           Coverage Diff           @@
##           master    #6803   +/-   ##
=======================================
  Coverage   47.08%   47.08%           
=======================================
  Files         419      419           
  Lines       51877    51877           
  Branches     7525     7525           
=======================================
  Hits        24428    24428           
  Misses      25700    25700           
  Partials     1749     1749           

@hiyufan The Arrow Flight offline server never scopes SecurityManager to the requested project, so permission checks always load the server's home project's policy. This is not a critical issue in practice (offline servers are typically deployed one-per-project, and only get_historical_features even accepts a client-supplied project), but I would still consider this a valid defense-in-depth fix.

However, the current implementation has a bug that needs to be fixed before merging:

self.store.set_current_project() sets the wrong ContextVar. FeatureStore._current_project and SecurityManager._current_project are two independent ContextVar objects — setting the former has no effect on SecurityManager.permissions, which is where the permission list is loaded from. The fix needs to call get_security_manager().set_current_project() instead, which is the same pattern used in the three working registry paths:

• permissions/server/grpc.py:39 → token = sm.set_current_project(...)
• permissions/server/rest.py:60 → project_token = sm.set_current_project(project)
• api/registry/rest/rest_utils.py:35 → project_token = sm.set_current_project(...)

Something like this in both _call_api and do_get:

from feast.permissions.security_manager import get_security_manager

sm = get_security_manager()
sm_token = None
if sm is not None:
    sm_token = sm.set_current_project(command.get("project"))
try:
    # ... existing handler logic ...
finally:
    if sm is not None and sm_token is not None:
        sm.reset_current_project(sm_token)

You can keep self.store.set_current_project() alongside for data-access correctness (it affects FeatureStore.project used by methods like get_data_source), but it alone does not fix the permission scoping.

The tests should also be updated to assert that the SecurityManager's project was set, not just the FeatureStore's — currently they only verify store.set_current_project was called, which doesn't prove the fix works.

You are right, and thank you for reading the code rather than the description — the description said the right thing and the code did not do it. Fixed in 152c0936.

I confirmed it before changing anything:

feature_store.py:232   self._current_project: ContextVar[...] = ContextVar("current_project", ...)
security_manager.py:34 self._current_project: ContextVar[...] = ContextVar("current_project", ...)

security_manager.py:60  @property
                        def permissions(self) -> list[Permission]:
security_manager.py:65      project = self._current_project.get() or self._project
                            return self._registry.list_permissions(project=project)

Two independent ContextVars, and permissions reads the security manager's. self.store.set_current_project(...) never reached the lookup, so every check still ran against the server's home project — the PR fixed nothing it claimed to fix.

Both dispatchers now bind the security manager, following the three paths you pointed at, and keep the store binding for the reason you gave:

project = command.get("project")
sm = get_security_manager()
sm_token = sm.set_current_project(project) if sm is not None else None
project_token = self.store.set_current_project(project)
try:
    ...
finally:
    self.store.reset_current_project(project_token)
    if sm is not None and sm_token is not None:
        sm.reset_current_project(sm_token)

The tests were the real problem

You are right that asserting store.set_current_project was called proves nothing — it is a MagicMock, so it passes whichever variable the code sets. That is exactly how this got past me.

They now install a real SecurityManager whose registry records the project every permission lookup resolves to, and assert on that list:

registry.list_permissions.side_effect = lambda project=None, **kw: (seen.append(project) or [])
sm = SecurityManager(project=_SERVER_HOME_PROJECT, registry=registry)
...
    OfflineServer._call_api(server, command["api"], command, key)   # handler reads sm.permissions
    sm.permissions                                                  # after the dispatcher returns

assert seen == ["project_b", _SERVER_HOME_PROJECT]

One assertion covers both halves: inside the handler the check has to resolve to the requested project, and afterwards it has to be back to the server's own, so the reset is proved rather than assumed. Against the previous commit:

FAILED test_do_get_scopes_the_permission_lookup_to_the_requested_project
FAILED test_call_api_scopes_the_permission_lookup_to_the_requested_project
E   assert ['the_project_the_server_was_started_from'] == ['project_b', 'the_project_the_server_was_started_from']

Added test_dispatchers_work_without_a_security_manager as well, since an unauthenticated deployment has none and the dispatchers still have to serve.

15 passed on sdk/python/tests/unit/test_offline_server.py, ruff check and ruff format --check clean.

Unrelated: the red CI

All five failing jobs on the previous head died in dependency installation, before running anything:

error: Failed to download `torchvision==0.28.0+cpu`
make: *** [Makefile:110: install-python-dependencies-ci] Error 1

lint-python, the three unit-test jobs and smoke-test-python all have that same line. Nothing to do with this branch, but worth knowing before you read the red.

And on your framing — agreed that this is defense in depth rather than critical, given one-server-per-project deployments. I would rather it be described that way in the changelog than oversold.