-
Notifications
You must be signed in to change notification settings - Fork 308
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add authorization to AuthenticatedFileHandler #1021
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1021 +/- ##
==========================================
- Coverage 76.28% 75.44% -0.85%
==========================================
Files 63 63
Lines 8228 8235 +7
Branches 1637 1642 +5
==========================================
- Hits 6277 6213 -64
- Misses 1546 1618 +72
+ Partials 405 404 -1 ☔ View full report in Codecov by Sentry. |
@blink1073 Please take a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look right to me, but having not implemented authorization-related code, I'd prefer others take look.
One thing I do notice is that this needs to coordinate the resource name "contents"
with the same in ContentsManager
. I wonder if it would be beneficial to add an enumerated type (or equivalent) in auth/resources.py
(?) that provides canonical resource names that authorizing methods can set and authorizers can validate/reference against?
I'm not suggesting this be done in this PR, but, if others agree, is probably something that should be done fairly soon. The enum would also serve as the single location for identifying what resources are being authorized.
Thanks, @jiajunjie. This LGTM! |
jupyter_server/docs/source/operators/security.rst
Lines 251 to 255 in 31cdf4d
/files requests are handled by AuthenticatedFileHandler by default.