(WIP) CRM-20210: Improve authorization check for dynamic fks

[davialexandre]

Instead of actually calling the delegated action in order to check if it's authorized or not, we now
use Kernel->runAuthorize() to do it.

The way the check worked previously was: In order to be able to add an attachment to a entity X,
the user must have create permissions to X. DynamicFKAuthorization would then call X.create
passing the ID of the entity to which we want to add the attachment to. If the API call fails, it means
the authorization failed. Otherwise, the attachment creation would proceed.

The problem with this approach is that the API can fail for different reasons other than Authorization.
For example, a custom API can fail because it was expecting other mandatory fields (besides the id
given by DynamicFKAuthorization). By using Kernel->runAuthorize(), we avoid this problem, since now
the actual action will not be executed.

Since now we don't need to update the DB in order to check for Authorization, the transaction
isn't necessary anymore.

---

• CRM-20210: Improve permission check when adding attachments

[civicrm-builder]

Can one of the admins verify this patch?

[davialexandre]

This based on the comment @colemanw left here

[eileenmcnaughton]

Theory looks good, although I would want @colemanw to review this.

@ErichBSchulz fyi

[colemanw]

I think this is right, but would like @totten to review since he is the one who originally wrote this fn.

[colemanw]

@civicrm-builder add to whitelist

[ErichBSchulz]

thanks for the pointer @eileenmcnaughton - my question:answer ratio is gradually falling but it's quite a mountain :-/

[totten]

@davialexandre in a proper and consistent world, I would agree with this revision. But we're not in a proper and consistent world. :(

For example, consider the Contact API which begins:

function civicrm_api3_contact_create($params) {
  $contactID = CRM_Utils_Array::value('contact_id', $params, CRM_Utils_Array::value('id', $params));

  if ($contactID && !empty($params['check_permissions']) && !CRM_Contact_BAO_Contact_Permission::allow($contactID, CRM_Core_Permission::EDIT)) {
    throw new \Civi\API\Exception\UnauthorizedException('Permission denied to modify contact record');
  }
  ...
}

There's an authorization check in the body of the API method. This authorization-check is not fired as a listener in the AUTHORIZE phase. The only way this authorization check fires is if you actually run the create action.

The DynamicFKAuthorization was written with a skeptical view of the AUTHORIZE phase: we should have an AUTHORIZE phase, because that's easier to understand and it's what most frameworks do, but we haven't retroactively introduced it.

There are a few ways to address this:

• Keep DynamicFKAuthorization as-is (using a full call to run($entity, 'create', [id=>123])). Ensure that each API can be used to update based solely on id.
  • (My understanding is that this behavior is expected and enforced via SyntaxConformanceTest::testCreateSingleValueAlter() -- it tests each API to ensure that you can use id for an update. But there may be some grandfathered exceptions or external/nonconformant APIs?)
• Audit+patch the various API's to ensure that the AUTHORIZE phase is used in the expected way across the board. Then use this PR ((WIP) CRM-20210: Improve authorization check for dynamic fks #10010).
  • This is actually the ideal solution because it provides more consistent ways to extend/replace the authorization logic. It's also the most labor-intensive.
• Update DynamicFKAuthorization to support a transition where:
  • Some legacy API's do a full run($entity, 'create', [id=>123])
  • Some newer/audited API's do a faster runAuthorize($entity, 'create', [id=>123])

[totten]

If I were in your shoes, I'd go with #1 or #3 as a matter of expedience. But I think it's patchwelcome to pursue the ideal of #2.

Aside: There had been a question before about why Attachment API and DynamicFKAuthorization only applied to a few whitelisted entities -- and not to all entities. I think this was the reason: we couldn't assume that all existing API's met the tighter requirements necessary for DynamicFKAuthorization::authorizeDelegate() to be semantically correct.

(In effect, the story of the Civi API is that it started as the wild-west and gradually introduced law-and-order. DynamicFKAuthorization is another step in the introduction law-and-order -- it's a use-case where we need more structured authorization logic but couldn't count on that existing everywhere.)

[davialexandre]

@totten #3 looks like the best option for me. As you mentioned yourself, there is also this whitelist of entities to which we can add attachments to, and between #1 and #3, the latter seems to be the only one that addresses this issue, since it would work with both legacy and newer APIs (btw, there's this PR related to this #9875)

So, my plan would be:

• Prepare a list of legacy API's and implement #3
• Get (WIP) CRM-20160 - Remove restriction on which entities the Attachment api supports #9875 merged (Because now the authorization should be working for all core entities, and this will not be required anymore)

Does that sound like a good plan?

A note about how did we find this and updating only the ID in APIs

We found this problem while trying to add attachments to a LeaveRequest in CiviHR. We have some quite complex validations done when creating or updating a leave request. One of these (the very first one) is checking for required fields. We could probably not check for these during an update, but there are a few problems with this:

• The validation code would certainly get more complex, with a lot of if(!empty($params['field'])) checks in the other validations that relies upon the required fields.
• We would not be able to set these fields as required on the API specification, because there's no way, as far as I know, to have different specifications for create and update
• We would lose the concept of having a valid entity and instead would just have an array with a bunch of fields

So, even though it would not be dangerous to update a LeaveRequest passing only the ID (it will not change anything after all), the validation we have will prevent that

[totten]

OK, so drilling down on #3: The status quo with DynamicFKAuthorization is that it only delegates authorization checks for four entities (civicrm_activity, civicrm_mailing, civicrm_contact, civicrm_grant). Any other entity (whether in civicrm-core or contrib) is categorically disallowed/excluded/ignored.

The intent is to allow more entities in DynamicFKAuthorization, both in core and contrib. However, there are quite probably entities which are not ready/compatible. This leads to four scenarios to differentiate:

• Core entity, safe for DynamicFKAuthorization
• Core entity, not safe for DynamicFKAuthorization
• Contrib entity, safe for DynamicFKAuthorization
• Contrib entity, not safe for DynamicFKAuthorization

I guess I'm nervous about opening up to all entities in core+contrib. What would you think of keeping the whitelist -- but allowing contrib to alter the whitelist? For example, https://gist.github.com/totten/9e4cbc26ba33522c33e420728ef5bff4 provides pseudocode. I like that approach because it doesn't require any new hooks/events (but also willing to talk about other approaches if they seem clearer in your mind).

Some kind of API metadata approach might also be reasonable -- and possibly favored by @colemanw or @eileenmcnaughton -- but it's less clear in my mind where to put that API metadata. (I personally feel less design-ambiguity in the addAllowedDelegate() approach.)

[totten]

I think it's fine to continue putting discussion here, but I'm flagging this WIP because the discussion in #10010 indicates that we need major changes. If you'd rather close/reopen, that's also valid/appreciated.

[eileenmcnaughton]

Just as a general admin thing - I do think it's good to close PRs when they are not progressing & re-open then when ready to progress again. I'm pretty sure you can still comment & discuss on closed PRs.

[davialexandre]

@totten sorry the delay to reply to this.

I was just checking your gist and I really like the idea of opening DynamicFKAuthorization to be more flexible. I thought a bit about it and I propose to extend your idea a bit further. Instead of having extensions adding themselves to the DynamicFKAuthorization (by calling the addDelegate() method directly), I think we can use Service tags to make this more flexible and more OO. The plan is:

• Create a new DynamicFKAuthorizer (or something like that) interface. It should have just one method that will tell if the there's an authorization or not.
• Create a new Service tag (probably something like civi.dynamic_fk_authorizer), that will be used by services implementing the DynamicFKAuthorizer interface
• DynamicFKAuthorization will then have no authorization logic at all. It will just loop through all the registered authorizers and check if they all authorize the action or not.

We can have a default implementation that works as it is today (calling the delegated API) and another one to use runAuthorize.

Besides passing the entity and action to the authorizer, we can also pass the params. This would given even more flexibility and allow more complex logic like users being able to add attachments to an entities not created by them.

Does this sound good to you?

[davialexandre]

hey @totten, any thoughts about my last comment here?

[eileenmcnaughton]

I'm going to close this because it's a year old & not really moving / resolved / mergeable. Conversation can still proceed about this on a closed PR & re-opening a PR once it is ready will bring it back to the top of the queue.

[eileenmcnaughton]

(note it's still tracked from JIRA)