Remove IAuthorizationPolicy from codebase

[tnytown]

Resolves #13690.

[tnytown]

I'm not sure if this is really a useful distinction, but it also didn't feel right to return Allowed() from security policies that didn't understand specific requests.

Our options are (a) keep raising this exception, (b) have a new exception/return type, or (c) simply return Allowed in this case

[dstufft]

I think this logic has a possible subtle security issue, though I suspect it isn't a security issue currently with our existing policies. I also suspect it doesn't work with the 2FA policy?

The issue I'm thinking of is something like a confused deputy attack.

We basically have a stack of security policies that we iterate over and try and find a current request.identity that is valid.

Once we have that, at some later point in time someone will ask if a particular request has permission to do something with a particular context, at which point we'll traverse back down through the stack and see if anyone has something to say about this.

However, we don't know if the security policy that thinks it knows if a request is permitted actually does know, in fact there's a good chance it does not.

For instance, if the TwoFactorAuthSecurityPolicy happened to get put first in our list of policies, it would allow anyone to do anything, as long as they had 2fa if the context required it, without checking any of the relevant ACLs.

So I think what we actually need to do here is one of two things:

1. Have the request somehow remember which security policy authenticated the request.identity, and only use that policy for permits.

   • I think this one is harder to implement in terms of what MultiSecurityPolicy has to do, but I think it has the clearest semantics AND the easiest to understand implementations for the underlying security policies-- each security policy is self contained, and you don't have to worry about any sort of interaction between the policies.

     It does also mean that TwoFactorSecurityPolicy doesn't work, but I think that's fine. It should probably be part of SessionSecurityPolicy and BasicAuthSecurityPolicy. Which I think makes Require API Tokens for upload if 2FA is enabled #7265 easier because the answer is "2FA doesn't make sense for API Tokens, so we don't implement it for MacaroonSecurityPolicy".

     I think the hardest thing is going to be how do we record what security policy returned the identity.

     We can't use a normal request property, because we run two of them at once, and that information is lost when request.identity gets set.

     We could maybe just manually set request._security_policy_for_identity in MultiSecurityPolicy? That feels kind of gross though.

     Maybe we can set an attribute on the identity itself, or create a wrapper identity, and have request.identity be a wrapper object, and request.user and request.oidc_publisher be our preferred mechanisms for accessing identity, where we can unwrap that wrapper.

     Also useful to note, we're already trying to differentiate in other places how a identity was found, so it's possible that we can align that to use the same method.

2. Check all of the policies, and use the combination of their answers to determine the final answer.

   • This one is probably easier to implement in terms of MultiSecurityPolicy, but I'm having a harder time working through the semantics in my head.

     ISecurityPolicy.permits can only return Allowed or Denied, so there's no way to have a "no comment" return value. If we have every underlying policy default to Denied, then MultiSecurityPolicy would have allow if any Allowed, which means that there's no way for something like TwoFactorSecurityPolicy to work. If we have everything return Allowed by default, then we can't tell if something is allowed because it's allowed, or because none of our policies have an idea how to permits it.

Personally, I think trying to get (1) working is a better, cleaner approach. It keeps each security policy self contained, which I think is a big win for security sensitive stuff like this. The cost is mostly going to be some minor duplication, primarily because we have both basic auth and sessions which are ways for users to directly log in, so we have to check 2FA support in both of them. Most of the non policy specific logic will end up getting deferred to the ACL framework anyways.

[dstufft]

Note: This sort of confused deputy like problem I think exists with our current solution. We have 3 different IAuthorizationPolicy that wrap each other, which means each of them may end up getting a request that they don't really understand (e.g. the Macaroon policy may be confused and allow something that it shouldn't when called with not a macaroon).

I think the existing solution works better than just having a list that returns the first value it can, because each of the wrapper policies is responsible for dispatching things they don't understand into the next level of the stack, which feels a lot clearer to me what's happening?

Anyways, I think (1) is a pretty good option, and think it'll be a nice improvement.

[tnytown]

This class doesn't look great, maybe we can completely inline its functionality into MultiSecurityPolicy or make it something other than a ISecurityPolicy?

[woodruffw]

Yeah, the absence of meaningful values here indicates that it should be inlined into MultiSecurityPolicy, IMO.

@dstufft may also have opinions here 🙂