Skip to content

fix: Call the decorated function in require_permissions - #6844

Open
David-Wu1119 wants to merge 1 commit into
feast-dev:masterfrom
David-Wu1119:fix/require-permissions-calls-function
Open

David-Wu1119 wants to merge 1 commit into
feast-dev:masterfrom
David-Wu1119:fix/require-permissions-calls-function

Conversation

@David-Wu1119

Copy link
Copy Markdown

What this PR does / why we need it:

require_permissions never calls the function it decorates.

def permission_checker(*args, **kwargs):
    ...
    return assert_permissions(        # <- returns here
        resource=resource,
        actions=actions,
    )
    logger.debug(...)                 # unreachable
    result = func(*args, **kwargs)    # unreachable
    return result                     # unreachable

assert_permissions returns the resource it was given, so every method decorated with @require_permissions skips its own body and returns the resource instead of its own return value. This has been the behaviour since the decorator was added in #4380.

The permission check itself still works — assert_permissions raises FeastPermissionError when access is denied — so this is not a security hole, it is a silent no-op for the protected method.

Demonstrated on master with the existing test fixtures (SecuredFeatureView.read_protected() is defined as return True):

read_protected() returned: SecuredFeatureView <SecuredFeatureView(name = secured, version = latest, ...)>

The fix drops the return in front of assert_permissions(...), which leaves it acting as a guard and makes the rest of the wrapper reachable again.

test_access_SecuredFeatureView passed before this change because it only asserted that permitted calls do not raise, never that the decorated method ran, so the two permitted branches now assert the returned value as well. Reverting the one-line source change makes 3 of its 4 parametrizations fail.

Which issue(s) this PR fixes:

None filed.

Checks

  • I've made sure the tests are passing.
  • My commits are signed off (git commit -s)
  • My PR title follows conventional commits format

Testing Strategy

  • Unit tests
pytest sdk/python/tests/unit/permissions --ignore=sdk/python/tests/unit/permissions/auth
239 passed, 40 warnings in 1.25s

(tests/unit/permissions/auth was skipped locally: it needs optional auth dependencies that are not installed in my environment.)

ruff check and ruff format --check are clean on both changed files.

Misc

The always-true early return was found with an AST scan for statements after an unconditional return/raise. The change and this description were written with AI assistance and reviewed line by line.

🤖 Generated with Claude Code

`permission_checker` returned the result of `assert_permissions(...)`, which
is the resource itself, so the code below it — including `func(*args, **kwargs)`
— was unreachable. Every method decorated with `@require_permissions` skipped
its own body and returned the resource instead.

The permission check itself still worked (`assert_permissions` raises
`FeastPermissionError` when access is denied), which is why the existing tests
passed: they only asserted that permitted calls do not raise, never that the
decorated method actually ran.

Drop the `return` so `assert_permissions` keeps acting as a guard and the
decorated function is called, and assert the returned value in
`test_access_SecuredFeatureView` so the regression cannot come back.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: David-Wu1119 <133224895+David-Wu1119@users.noreply.github.com>
@David-Wu1119
David-Wu1119 requested a review from a team as a code owner September 17, 2026 20:54
Copilot AI lite review requested due to automatic review settings September 17, 2026 20:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@codecov-commenter

codecov-commenter commented Sep 20, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' 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.42%. Comparing base (922db5c) to head (6898bd8).
⚠️ Report is 4 commits behind head on master.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6844      +/-   ##
==========================================
+ Coverage   47.11%   47.42%   +0.30%     
==========================================
  Files         419      421       +2     
  Lines       51886    52252     +366     
  Branches     7528     7582      +54     
==========================================
+ Hits        24448    24780     +332     
- Misses      25688    25708      +20     
- Partials     1750     1764      +14     
Flag Coverage Δ
go-feature-server 30.58% <ø> (ø)
python-unit 48.74% <100.00%> (+0.31%) ⬆️
Files with missing lines Coverage Δ
sdk/python/feast/permissions/decorator.py 100.00% <100.00%> (ø)

... and 13 files with indirect coverage changes


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 922db5c...6898bd8. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants