Remove IdentityBasedPolicy base class
#8859
Conversation
seanh
changed the title
IdentityBasedPolicy base class
| @staticmethod | ||
| def authenticated_userid(identity: Self | None) -> str | None: | ||
| """Return the authenticated_userid from the given identity.""" | ||
| if identity and identity.user: | ||
| return identity.user.userid | ||
|
|
||
| return None |
There was a problem hiding this comment.
I moved the IdentityBasedPolicy.authenticated_userid() method to a static method on the Identity class itself. Alternatively this could just be a standalone helper function.
| def permits(self, request, context, permission) -> Allowed | Denied: | ||
| return identity_permits(self.identity(request), context, permission) |
There was a problem hiding this comment.
Each sub-policy class that previously inherited authenticated_userid() and permits() from IdentityBasedPolicy now instead needs to provide these two one-liner methods to delegate authenticated_userid() and permits() to the new helpers.
As is usual with composition-over-inheritance this leads to more duplication, but this is trivial boiler-plate code and the advantage is better code readability and decoupling.
| def permits(self, request, context, permission) -> Allowed | Denied: | ||
| return get_subpolicy(request).permits(request, context, permission) |
There was a problem hiding this comment.
Unlike with the various sub-policies, TopLevelPolicy's authenticated_userid() and permits() methods do not delegate to the new helpers. Instead, they delegate to the applicable sub-policy the same way that TopLevelPolicy's other methods do. I think this makes sense: TopLevelPolicy's only job is to choose the applicable sub-policy and delegate all calls to the sub-policy. It's the sub-policy's business how those methods are implemented (and whether or not they work by calling the new helpers). This also makes the behaviour of all of TopLevelPolicy's methods consistent with each other.
seanh
force-pushed
the
remove-IdentityBasedPolicy-base-class
branch
from
e61c731
to
7997448
Compare
Factor out the `IdentityBasedPolicy` base class, favouring helper methods that're used by composition instead. This base class is making future refactorings (see #8860 and #8861) more difficult to reason about by coupling together different security policy classes that I want to evolve in separate directions. The base class is also responsible for some disturbing unintended behaviours. For example, on `main`: * `IdentityBasedPolicy.authenticated_userid()` calls `identity()` (which must be implemented by `IdentityBasedPolicy` sub classes). * `CookiePolicy` has an `identity()` method and inherits from `IdentityBasedPolicy`, so `CookiePolicy` inherits `IdentityBasedPolicy`'s `authenticated_userid()` method that calls `CookiePolicy.identity()` * As with all h's security policies `CookiePolicy` is never used directly as a security policy, rather it's always delegated to by `TopLevelPolicy` * `TopLevelPolicy` has an `identity()` method that delegates to `CookiePolicy.identity()` for HTML pages, and `TopLevelPolicy` also inherits from `IdentityBasedPolicy`, so `TopLevelPolicy` inherits an `authenticated_userid()` method that calls `TopLevelPolicy.identity()`. * So the end result is that `CookiePolicy.authenticated_userid()` (which is inherited from `IdentityBasedPolicy`) is never actually used! Instead `TopLevelPolicy`'s copy of `authenticated_userid()` (also inherited from `TopLevelPolicy`) is used and calls `TopLevelPolicy.identity()` which delegates to `CookiePolicy.identity()`. The same thing happens when `APIPolicy` (which also inherits from `IdentityBasedPolicy`) delegates to API subpolicies. This is true for the `authenticated_userid()` and `permits()` methods of all h's security policies other than `TopLevelPolicy`: none of them are ever called. This hasn't resulted in any bugs because all the sub-policies and `TopLevelPolicy` inherit same `authenticated_userid()` and `permits()` methods from `IdentityBasedPolicy`, so `TopLevelPolicy`'s by-passing the `authenticated_userid()` and `permits()` methods of sub-policies makes no difference. In fact you could remove the `IdentityBasedPolicy` base class from all classes except `TopLevelPolicy` and it would make no difference, the methods that all those classes inherit from `IdentityBasedPolicy` are never called. But clearly this is very confusing and unintended, and would lead to bugs if there's ever a sub-policy in future that has a different `authenticated_userid()` or `permits()` method.
seanh
force-pushed
the
remove-IdentityBasedPolicy-base-class
branch
from
7997448
to
ad9b438
Compare
Factor out the
IdentityBasedPolicybase class, favouring helper methods that're used by composition instead.This base class is making future refactorings (see #8860 and #8861) more difficult to reason about by coupling together different security policy classes that I want to evolve in separate directions.
The base class is also responsible for some disturbing unintended behaviours. For example, on
main:IdentityBasedPolicy.authenticated_userid()callsidentity()(which must be implemented byIdentityBasedPolicysub classes).CookiePolicyhas anidentity()method and inherits fromIdentityBasedPolicy, soCookiePolicyinheritsIdentityBasedPolicy'sauthenticated_userid()method that callsCookiePolicy.identity()As with all h's security policies
CookiePolicyis never used directly as a security policy, rather it's always delegated to byTopLevelPolicyTopLevelPolicyhas anidentity()method that delegates toCookiePolicy.identity()for HTML pages, andTopLevelPolicyalso inherits fromIdentityBasedPolicy, soTopLevelPolicyinherits anauthenticated_userid()method that callsTopLevelPolicy.identity().So the end result is that
CookiePolicy.authenticated_userid()(which is inherited fromIdentityBasedPolicy) is never actually used! InsteadTopLevelPolicy's copy ofauthenticated_userid()(also inherited fromTopLevelPolicy) is used and callsTopLevelPolicy.identity()which delegates toCookiePolicy.identity().The same thing happens when
APIPolicy(which also inherits fromIdentityBasedPolicy) delegates to API subpolicies.This is true for the
authenticated_userid()andpermits()methods of all h's security policies other thanTopLevelPolicy: none of them are ever called. In fact you could remove theIdentityBasedPolicybase class from all classes exceptTopLevelPolicyand it would make no difference, the methods that all those classes inherit fromIdentityBasedPolicyare never called.This hasn't resulted in any bugs because all the sub-policies and
TopLevelPolicyinherit sameauthenticated_userid()andpermits()methods fromIdentityBasedPolicy, soTopLevelPolicy's by-passing theauthenticated_userid()andpermits()methods of sub-policies makes no difference.But clearly this is very confusing and unintended, and would lead to bugs if there's ever a sub-policy in future that has a different
authenticated_userid()orpermits()method.Testing
http POST 'http://localhost:5000/api/annotations' uri=foo Authorization:'Bearer 6879-***'devdata_admin, go to http://localhost:5000/admin/features and enable thepreact_create_group_formfeature flag, then go to http://localhost:5000/groups/new and create a new group.