fix: Scope offline server permission checks to the requested project - #6803
Open
hiyufan wants to merge 1 commit into
Open
fix: Scope offline server permission checks to the requested project#6803hiyufan wants to merge 1 commit into
hiyufan wants to merge 1 commit into
Conversation
The registry server's REST and gRPC paths bind the SecurityManager to the project each request names, so its permission list is loaded for that project. The Arrow Flight offline server never did: both of its dispatchers ran the handlers, and therefore `assert_permissions`, with whatever project the SecurityManager was constructed from — the project in the server's own `feature_store.yaml`. An offline server that serves more than one project consequently enforced its home project's `Permission` list against every request, so a role granted only in that project reached the other projects' data sources and feature views, while the policies those projects defined for themselves were never consulted. `get_historical_features` already requires a `project` in its command, so the per-request project was available all along. Bind the SecurityManager to `command["project"]` around both dispatchers and reset it afterwards, following the interceptor in permissions/server/grpc.py. A command that carries no project passes `None`, which the SecurityManager falls back from to the server's own project, leaving those paths unchanged. Fixes feast-dev#6784 Signed-off-by: Chen Yufan <yufan_ai@outlook.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
SecurityManager.permissionsis loaded per project, and every request-facing path issupposed to bind it to the project the request names before any permission check runs.
The registry server does this —
api/registry/rest/rest_utils.py,permissions/server/rest.py,permissions/server/grpc.pyandregistry_server.pyallcall
set_current_project(...)and reset it afterwards.The Arrow Flight offline server does not. Neither of its two dispatchers —
OfflineServer.do_getandOfflineServer._call_api— touches the current project, soevery
assert_permissionscall in the handlers below them is evaluated against thePermissionlist of whatever project theSecurityManagerwas constructed from, whichis the project in the server's own
feature_store.yaml.An offline server that serves more than one project therefore enforces its home
project's policy on every request. A role granted only in that project reaches the other
projects' data sources, feature views and saved datasets, and the policies those projects
defined for themselves are never loaded.
get_historical_featuresalready asserts thatprojectis mandatory in its command and uses it to resolve feature views, so theper-request project was available at the dispatcher the whole time — it just never
reached the permission layer.
The fix
Bind the
SecurityManagertocommand["project"]around both dispatchers and reset itin a
finally, following the interceptor inpermissions/server/grpc.py:A command that carries no project passes
None.SecurityManager.permissionsalreadyfalls back from
Noneto the server's own project, so those paths —offline_write_batchand
write_logged_features, which readself.store.config.project— behave exactly asbefore.
Which issue(s) this PR fixes:
Relates to #6784 (advisory
GHSA-5px7-7gwg-6g93).That issue is written against the registry server, and the registry paths already scope
the lookup on
master. This is the same defect on the Arrow Flight offline server, whichwas not covered. I have left it as "relates to" rather than "fixes" so you can decide
whether it closes the issue or belongs as a follow-up.
Checks
git commit -s)Testing Strategy
Four tests in
sdk/python/tests/unit/test_offline_server.py:test_do_get_scopes_permissions_to_the_requested_projecttest_call_api_scopes_permissions_to_the_requested_projecttest_call_api_resets_the_project_when_the_handler_raises— a failed request must notleave its project bound for the next one
test_call_api_without_a_project_leaves_the_lookup_unchanged— pins theNonefallback, so the paths that do not send a project keep their current behaviour
All four fail on the base revision and pass with the change:
pre-commitpasses on both changed files.Misc
The scope here is deliberately narrow: it moves the offline server onto the same
per-request project binding the registry server already uses, and changes nothing about
how permissions themselves are defined or evaluated. If you would rather see the binding
lifted into a shared Flight middleware — the way
permissions/server/grpc.pydoes it forgRPC, instead of at each dispatcher — I am happy to rework it that way.
This change was developed with AI assistance. The test runs and the base-revision
comparison reported above were executed locally against this branch as submitted.