-
Notifications
You must be signed in to change notification settings - Fork 1k
Refactor Authorization #13849
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
Refactor Authorization #13849
Conversation
Signed-off-by: Andrew Pan <[email protected]>
Signed-off-by: Andrew Pan <[email protected]>
Signed-off-by: Andrew Pan <[email protected]>
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.
Grossest diff ever, but this makes sense after four cups of coffee and a bit too much time switching between diffs and the files as they sit.
from pyramid.httpexceptions import HTTPUnauthorized | ||
from pyramid.interfaces import IAuthorizationPolicy, ISecurityPolicy | ||
from pyramid.threadlocal import get_current_request |
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.
I bet this made you so happy @dstufft
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.
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.
I'm not a Pyramid security policy expert, but from what I could follow, this makes sense and simplifies the policy stack.
Should these changes (and the ones that follow) be represented somewhere in the dev docs? https://github.com/pypi/warehouse/blob/main/docs/dev/security.rst
Overall security approach/strategy for internal authN/authZ might be helpful to understand how the thing is meant to work. I think you wrote a fair amount already in Slack messages, and have some more ideas up your sleeve.
(Allow, "group:psf_staff", "psf_staff"), | ||
(Allow, "group:psf_staff", "admin_dashboard_access"), | ||
(Allow, Authenticated, "manage:user"), | ||
] |
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.
praise: love that the assertion is now equivalent with the object, so any extra keys would be sniffed out
def _check_for_mfa(request, context) -> WarehouseDenied | None: | ||
# It should only be possible for request.identity to be a User object | ||
# at this point, and we only a User in these policies. | ||
assert isinstance(request.identity, User) |
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.
thought: I thought using assert
could be tricky, if the runtime ever chose to run under -O
https://docs.python.org/3.11/using/cmdline.html#cmdoption-O
I don't think we're doing that today, but on the off chance we ever do run the warehouse with the flag (or envvar PYTHONOPTIMIZE
), this would always return None
. Wouldn't running under optimize mode effectively bypass this check?
assert
is used another couple of times here, as well as other existing cases in warehouse/macaroons/caveats/__init__.py
and warehouse/http.py
so this isn't a new thing, but If You See Something, Say Something.
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.
It would technically bypass this check, but the outcome of doing that is we would just fail later with a more inscrutable error message. For example in this method we would fail trying to access `request.identity.has_two_factor``.
In all the places I used asserts, it should be fine to remove them because the structure of the code means that any other condition should not be possible, and would likely error out due to an assumption later.
So why do I have the assertions at all?
- They serve as verifiable documentation for these functions that they are only valid to be called in cases where the assertion is true.
- They teach type checkers like MyPy what type is being used, so it can do better linting than assuming
request.identity
isAny
. - If we somehow get into a state where a non user is passed in, we'll just try to fail early on with a more reasonable error message.
(3) is the only one really affected by PYTHONOPTIMIZE
, and it's the weakest reason of the bunch. It would also likely only be something we do in prod so if we started violating these assumptions we'd likely notice it in development first.
The alternatives here would be to do nothing and not assert (which is what most Python code bases do, they just assume that you've passed in a type that looks like what they expect, e.g. duck typing) or explicitly do type checking with a real error. The latter has all of the same benefits really, but we end up having to test impossible conditions then, for something that shouldn't ever happen.
Co-authored-by: Mike Fiedler <[email protected]> Co-authored-by: Ee Durbin <[email protected]>
This refactors our use of the legacy
IAuthorizationPolicy
to migrate fully ontoISecurityPolicy
.Important things to note:
MultiSecurityPolicy
remembers what policy provided an identity, and will only dispatch to that policy to callpermits()
.RequestLocalCache
which will store a weak reference to the request and return the same results each time, this keeps the logic encapsulated insideMultiSecurityPolicy
.request.identity
now knows how to generate it's own principals, this keeps ACLs and Principals both "near" the objects that implement them.Removed somegroup:with_admin_dashboard_access
group and related permission that, as far as I can tell, were not used for anything and were being granted to every logged in user, which felt wrong and confusing to me.group:with_admin_dashboard_access
and granted theadmin_dashboard_access
permission to our 3 admin groups.Making this a draft because I want to do some manual testing tomorrow when I'm more awake.
Thanks to @tnytown for a great start with #13754!
Closes #13754
Fixed #13690