-
Notifications
You must be signed in to change notification settings - Fork 0
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
Bump pyramid from 1.10.5 to 2.0 #237
Conversation
f8be294
to
eddcf74
Compare
eddcf74
to
74247b9
Compare
058553c
to
dd6d7de
Compare
<h2>Permissions</h2> | ||
<code>{{ request.identity.permissions }}</code> |
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's no such thing as principals anymore in Pyramid 2, so changed this to display permissions instead
@@ -13,15 +13,15 @@ def __init__(self, request): | |||
|
|||
@view_config( | |||
renderer="checkmate:templates/admin/pages.html.jinja2", | |||
effective_principals=[Principals.STAFF], | |||
permission=Permissions.ADMIN, |
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's no such thing as principals in Pyramid 2 anymore, so use a permission instead.
) | ||
def get(self): | ||
cookie = SimpleCookie() | ||
cookie.load(self.request.headers["Cookie"]) | ||
|
||
return {"session": cookie["session"].value} | ||
|
||
@view_config() | ||
@forbidden_view_config() |
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.
Previously this was an additional view without the effective_principals=[Principals.STAFF]
so it would be called when the STAFF
principal was missing. Now, with permissions, you need a forbidden view instead
@@ -41,7 +41,7 @@ class Meta: | |||
route_name="add_to_allow_list", | |||
request_method="POST", | |||
jsonapi={"schema": AllowRuleSchema()}, | |||
effective_principals=[Principals.STAFF], | |||
permission=Permissions.ADD_TO_ALLOW_LIST, |
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's no such thing as principals anymore in Pyramid 2 so use a permission instead. This also changes the 404 to a 403
dd6d7de
to
5e4d448
Compare
class Permissions(Enum): | ||
ADMIN = "admin" | ||
CHECK_URL = "check_url" | ||
ADD_TO_ALLOW_LIST = "add_to_allow_list" |
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.
Just one separate permission for each different security-protected view
class Identity(NamedTuple): | ||
userid: str | ||
permissions: List[str] |
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.
request.identity
is a NamedTuple
containing the authenticated user's ID and permissions
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.
Does NamedTuple
actually do anything with the type definitions here. I'm assuming it's just for documentation?
class CascadingSecurityPolicy: | ||
def __init__(self, subpolicies): | ||
self._subpolicies = subpolicies | ||
|
||
def identity(self, request): | ||
return self._effective_subpolicy(request).identity(request) | ||
|
||
def authenticated_userid(self, request): | ||
return self._effective_subpolicy(request).authenticated_userid(request) | ||
|
||
def permits(self, request, context, permission): | ||
return self._effective_subpolicy(request).permits(request, context, permission) | ||
|
||
def forget(self, request, **kwargs): | ||
return self._effective_subpolicy(request).forget(request, **kwargs) | ||
|
||
def remember(self, request, userid, iface, **kwargs): | ||
return self._get_specific_policy(iface).remember(request, userid, **kwargs) | ||
|
||
def _effective_subpolicy(self, request): | ||
for policy in self._subpolicies: | ||
if policy.authenticated_userid(request): | ||
return policy | ||
|
||
return self._subpolicies[-1] | ||
|
||
def _get_specific_policy(self, iface): | ||
for policy in self._subpolicies: | ||
if isinstance(policy, iface): | ||
return policy | ||
|
||
raise KeyError( | ||
f"Could not find a policy matching the requested interface: {iface}" | ||
) |
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.
This is just exactly the same thing as CascadingAuthenticationPolicy
was before
class SecurityPolicy(CascadingSecurityPolicy): | ||
def __init__(self): | ||
super().__init__( | ||
subpolicies=[HTTPBasicAuthSecurityPolicy(), GoogleSecurityPolicy()] | ||
) |
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.
This is just exactly the same thing as AuthenticationPolicy
was before
return Identity( | ||
userid, permissions=[Permissions.ADMIN, Permissions.ADD_TO_ALLOW_LIST] | ||
) |
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.
So GoogleSecurityPolicy
gives all Google-authenticated users (with a @hypothes.is
email address) the ADMIN
and ADD_TO_ALLOW_LIST
permissions. Google-authenticated users can access the admin pages and the add-to-allow-list API
if userid: | ||
return Identity(userid, permissions=[Permissions.CHECK_URL]) |
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.
HTTPBasicAuthSecurityPolicy
gives basic auth-authenticated users the CHECK_URL
permission so they can access the check API
5e4d448
to
bede20b
Compare
; Suppress warnings about an import of `imp` by Pyramid | ||
ignore:the imp module is deprecated in favour of importlib |
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.
Pyramid 2 fixed this deprecation warning
bede20b
to
c054c7f
Compare
# The subpolicy whose remember() method we expect to get called, | ||
# either subpolicy1 or subpolicy2. | ||
subpolicy = locals()[subpolicy] |
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.
Some cleverness here. You can't use a fixture in a parametrize
so instead I parametrized the test with the fixture's name and then retrieved the actual fixture by name from locals()
. The alternative would be to split this into two separate tests with some duplication
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 think this and some other things could be simplied by using an array as the fixture:
@pytest.fixture
def subpolicies(self):
return [
create_autospec(SubPolicy0, spec_set=True, instance=True),
create_autospec(SubPolicy1, spec_set=True, instance=True)
]
Then this test could be:
@pytest.mark.parametrize(
"iface,policy_index", [(SubPolicy0, 0), (SubPolicy1, 1)]
)
def test_remember(self, iface, policy_index, policy, subpolicies):
# The subpolicy whose remember() method we expect to get called,
# either subpolicy0 or subpolicy1.
subpolicy = locals()[policy_index]
You can achieve something similar with policy,_subpolicies
instead
@pytest.fixture(params=["subpolicy1", "subpolicy2", "both", None]) | ||
def effective_subpolicy( | ||
self, | ||
request, | ||
subpolicy1, | ||
subpolicy2, | ||
): | ||
"""Return the subpolicy that the test expects to be effective.""" | ||
|
||
# Any test that uses this parametrized `effective_subpolicy` fixture | ||
# will get run four times... | ||
|
||
if request.param == "subpolicy1": | ||
# The first time the test is run subpolicy1.authenticated_userid() | ||
# will return a userid but subpolicy2.authenticated_userid() won't. | ||
subpolicy1.authenticated_userid.return_value = sentinel.policy1_userid | ||
subpolicy2.authenticated_userid.return_value = None | ||
# CascadingSecurityPolicy should choose subpolicy1 as the effective policy. | ||
return subpolicy1 | ||
|
||
if request.param == "subpolicy2": | ||
# The next time the test is run subpolicy1.authenticated_userid() | ||
# *won't* return a userid but subpolicy2.authenticated_userid() will. | ||
subpolicy1.authenticated_userid.return_value = None | ||
subpolicy2.authenticated_userid.return_value = sentinel.policy2_userid | ||
# CascadingSecurityPolicy should choose subpolicy2 as the effective policy. | ||
return subpolicy2 | ||
|
||
if request.param == "both": | ||
# The next time the test is run both subpolicies will return a userid. | ||
subpolicy1.authenticated_userid.return_value = sentinel.policy1_userid | ||
subpolicy2.authenticated_userid.return_value = sentinel.policy2_userid | ||
# CascadingSecurityPolicy should choose subpolicy1 as the effective policy. | ||
return subpolicy1 | ||
|
||
# The final time the test is run neither subpolicy will return a userid. | ||
assert request.param is None | ||
subpolicy1.authenticated_userid.return_value = None | ||
subpolicy2.authenticated_userid.return_value = None | ||
# CascadingSecurityPolicy should choose subpolicy2 as the effective policy. | ||
return subpolicy2 |
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.
This is a very straightforward usage of parametrized fixtures. It enables identity()
, authenticated_userid()
, permits()
and forget()
to all have just one test each, but each to be tested in the four different authentication situations
Bumps [pyramid](https://github.com/Pylons/pyramid) from 1.10.5 to 2.0. - [Release notes](https://github.com/Pylons/pyramid/releases) - [Changelog](https://github.com/Pylons/pyramid/blob/2.0/CHANGES.rst) - [Commits](Pylons/pyramid@1.10.5...2.0) Co-authored-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]>
c054c7f
to
8acd77f
Compare
if permission in policy.identity(request).permissions: | ||
return Allowed("allowed") | ||
|
||
return Denied("denied") |
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.
This could be a nice thing to add to the identity object so the caller can instead do:
request.identity.allows(permissions)
I'm assuming from your summary that this should be available already on the request object by the time this gets called.
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.
Maybe. I'm not sure that's really the right thing to do though, as Pyramid already provides APIs for asking about permissions. There's the permission
view config arg but also things like request.has_permission()
and this goes through all the necessary Pyramid machinations to give the right answer. I think request.identity
is just meant to represent the user, not provide an API for asking about permissions. request.identity
is available to the whole app not just to the security policies
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.
This looks good, and honestly easier to understand than before. The less Pyramid all up in your auth the better as far as I'm concerned.
I think the only part I'd suggest doing some more work on is the cascading policy tests. I think these are swimming against the tide of pytest
a bit by insisting on using two separate fixtures for the sub-policies. I think this would be simplified by using an array of sub-policies, which could then be referenced by index. There are a few places this would likely help, or make it easier to script instead of repeating things.
This could either be explicit as a separate fixture as it is now, or it could be achieved by using policy._subpolicies
.
There's also a suggestion to use the class itself as the template for sub-policies as it implements the interface.
- It's nice because it cuts out some waffle
- It's slightly dodgy because it means the tests are coupled to the interface defined in the code... not sure if that's actually a problem or not
|
||
return Identity("", []) | ||
|
||
def authenticated_userid(self, request): # pylint:disable=no-self-use |
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.
Can you just declare this as a class method and ignore cls
?
pass | ||
|
||
def forget(self, request, **kw): | ||
pass |
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.
As CascadingSecurityPolicy
implements the interface, you could create the sub-policies from children if it to avoid making this class. There's nothing saying you can put cascading security policies inside each other.
# The subpolicy whose remember() method we expect to get called, | ||
# either subpolicy1 or subpolicy2. | ||
subpolicy = locals()[subpolicy] |
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 think this and some other things could be simplied by using an array as the fixture:
@pytest.fixture
def subpolicies(self):
return [
create_autospec(SubPolicy0, spec_set=True, instance=True),
create_autospec(SubPolicy1, spec_set=True, instance=True)
]
Then this test could be:
@pytest.mark.parametrize(
"iface,policy_index", [(SubPolicy0, 0), (SubPolicy1, 1)]
)
def test_remember(self, iface, policy_index, policy, subpolicies):
# The subpolicy whose remember() method we expect to get called,
# either subpolicy0 or subpolicy1.
subpolicy = locals()[policy_index]
You can achieve something similar with policy,_subpolicies
instead
"""Return the CascadingSecurityPolicy instance to be tested.""" | ||
return CascadingSecurityPolicy([subpolicy1, subpolicy2]) | ||
|
||
@pytest.fixture(params=["subpolicy1", "subpolicy2", "both", None]) |
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.
If we used an index based version as suggested above, this could be a list of active indices:
@pytest.fixture(params=[[0], [1], [0,1], []])
The body of this fixture could then probably be written as a single loop
Bumps pyramid from 1.10.5 to 2.0.
Changelog
Sourced from pyramid's changelog.
... (truncated)
Commits
7b18204
add pre-commented forward9a64c50
mark 2.0-branch as no longer in progressea6ac9c
Merge branch 'master' into 2.0-brancha05fd83
prep 2.0b2fd15e
Merge branch 'master' into 2.0-branchea30508
Merge pull request #3657 from luhn/reify-sphinx5b63fb8
prep 2.0b124ea786
Remove inspect.unwrap, not in Python 2.7 and it's a no-op.f510281
Write tests.7a055e7
Update changelog.Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting
@dependabot rebase
.Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR:
@dependabot rebase
will rebase this PR@dependabot recreate
will recreate this PR, overwriting any edits that have been made to it@dependabot merge
will merge this PR after your CI passes on it@dependabot squash and merge
will squash and merge this PR after your CI passes on it@dependabot cancel merge
will cancel a previously requested merge and block automerging@dependabot reopen
will reopen this PR if it is closed@dependabot close
will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually@dependabot ignore this major version
will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this minor version
will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this dependency
will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)What's changed
There are no such things as authentication policies and authorization policies anymore in Pyramid 2. Instead there are just security policies, which are like simplified authentication and authorization policies merged into one class. See Security and
ISecurityPolicy
.checkmate.auth.AuthenticationPolicy
is nowcheckmate.security.SecurityPolicy
.AuthenticationPolicy
was really just a subclass for configuringCascadingAuthenticationPolicy
though, and the newSecurityPolicy
is just the same thing. Before:After:
checkmate.auth.CascadingAuthenticationPolicy
is nowcheckmate.security.CascadingSecurityPolicy
. But this is just the same thing: it takes a list of subpolicies and consults theauthenticated_userid()
method of each subpolicy taking the first policy to return something as the "effective" subpolicy. As before theremember()
method takes aniface
argument to select the subpolicy.GoogleAuthenticationPolicy
is nowGoogleSecurityPolicy
. The class also no longer inherits from Pyramid'sSessionAuthenticationPolicy
. Pyramid 1 came with a bunch of builtin authentication policies that you could either use directly or subclass to make your own. Pyramid 2 doesn't come with any builtin security policies: you have to implement your own security policy (implementing all ofISecurityPolicy
's methods). Instead Pyramid 2 has a bunch of security policy helpers that correspond to the previously builtin authentication policies:SessionAuthenticationHelper
,AuthTktCookieHelper
, etc. Your custom security policy classes can use these helpers via composition instead of inheritance. So whereGoogleAuthenticationPolicy
subclassed the builtinSessionAuthenticationPolicy
,GoogleSecurityPolicy
now usesSessionAuthenticationHelper
.APIHTTPAuth
is nowHTTPBasicAuthSecurityPolicy
and instead of subclassingBasicAuthAuthenticationPolicy
it uses theextract_http_basic_credentials()
helper function.There are no such things as principals or access control lists anymore in Pyramid 2. Instead there are permissions (as before) and identities.
checkmate.auth.AuthorizationPolicy
(which made authorization decisions based on principals) is no more. Its functionality is now merged into the security policies. The way it works now is:You put a
@view_config(..., permission="foo")
on a viewPyramid calls your security policy's
authenticated_userid()
method to get the string value forrequest.authenticated_userid
Pyramid calls your security policy's
identity()
method to get the value forrequest.identity
.An identity is supposed to be an object representing the authenticated user. Whereas
authenticated_userid
must be a string,identity
can be any object (e.g. aUser
model object). It's a formalisation of a common informal pattern in Pyramid 1.x: apps would often setrequest.user
to aUser
model object.identity
doesn't have to be a model though, it can be a string,None
, or whatever you want. AFAIKidentity
isn't used at all by Pyramid itself. Pyramid just getsrequest.identity
from your security policy'sidentity()
method and makes it available to the rest of your app's code asrequest.identity
. In particular Pyramid passesrequest.identity
to your security policy'spermits()
method to make authorization decisions (see below).Pyramid calls your security policy's
permits(request, context, permission)
method which decides whetherrequest
haspermission
incontext
.SecurityPolicy.permits()
is the equivalent of whatAuthorizationPolicy.permits()
used to be: it's what makes the actual authorization decision (whether to call the view or not). Yourpermits()
method is just a normal Python method that does whatever it wants withrequest
,context
andpermission
to decide whether or not to allow the request. It doesn't have to have anything to do with ACLs. It can accessrequest.authenticated_userid
(which Pyramid set by calling your same security policy'sauthenticated_userid()
method) andrequest.identity
(which Pyramid set by calling your same security policy'sidentity()
method).