fix: Add checks for registry and feature server handlers#6455
Conversation
ntkathole
force-pushed
the
perms_check_all
branch
from
e28ca0d to
6f43e2d
Compare
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
jyejare
left a comment
jyejare
left a comment
There was a problem hiding this comment.
This PR adds important security and authorization checks to registry and feature server handlers, improving access control for projects and operations. The changes properly implement permission assertions and handle edge cases like missing projects, though there are some areas that need attention around error handling and code duplication.
| except Exception as e: | ||
| logger.warning(f"Failed to persist recent visits: {e}") | ||
| return response |
There was a problem hiding this comment.
[Critical] Unreachable code after return statement
There are three lines of unreachable code after the return statement. The response handling and call_next() execution will never be reached, which could break the middleware functionality.
Current code:
+ return response
response = await call_next(request)
return responseSuggested:
| except Exception as e: | |
| logger.warning(f"Failed to persist recent visits: {e}") | |
| return response | |
| return response | |
| # Remove the unreachable code below |
There was a problem hiding this comment.
The suggestion to remove those lines would break the middleware for all non-GET requests.
ntkathole
force-pushed
the
perms_check_all
branch
from
6f43e2d to
121819a
Compare
Signed-off-by: ntkathole <nikhilkathole2683@gmail.com>
ntkathole
force-pushed
the
perms_check_all
branch
from
121819a to
48f02b5
Compare
3 participants
What this PR does / why we need it:
For consistency across all registry and feature server endpoints, added missing inline auth checks that their sibling handlers already enforce.
FeastObjectNotFoundException -> Yes → pass -> return empty list
FeastPermissionError -> No → propagates → 403