fix(entities): Entity creation no longer needlessly checks owner container

[mrclay]

BREAKING CHANGE:
When creating within a group, ElggEntity::create used to always separately check if the current user can use the owner's account as a container. This made sure that one group member could not post to the group using another member as owner. This separate check led to confusion, as handlers of the container_permissions_check hook were told that the owner was to be the container, when it was actually the group.

Here we bypass the separate owner container check if the desired owner_guid is the logged in user GUID. This eliminates the check under all normal circumstances but leaves it in place in case a poorly coded plugin allows the impersonation described above.

This also denies creation if the owner/container GUIDs are set but can't be loaded. Before, create() would simply bypass the permissions check if it couldn't load the owner/container.

Fixes #4231

(This is a modest change that eliminates the most common cases of #4231, but ultimately I think #8790 is the future)

• log cases where return false
• fix typo in docs

[mrclay]

The owner->canEdit() check blocks creating messages, because we create them under the recipient's owner. So checking the owner might need to go completely.

[mrclay]

This latest version no longer calls canEdit() on the owner.

[beck24]

LGTM - I don't think this ever affected anything I've worked on, but this does make more sense

[juho-jaakkola]

I haven't dug that deep into this, but could this make it possible to post something as another user?

E.g. Juho and Steve are members of a group. Juho is creating something to the group and changes the owner_guid in the a creation form to the GUID of Steve. Steve is allowed to write to the group, so nothing blocks Juho from saving the post, and then it looks like Steve posted it.

[juho-jaakkola]

A highly unlikely scenario, though. And it would probably mean that the particular content plugin isn't very secure in the first place.

[mrclay]

Yes, that sounds right. So we can kind of think of posting in a group as posting to your own container as well. Really the case of creating content owned by a different user needs a separate permission & hook. I.e. check $owner->canUseAsOwner() with a separate hook.

Messages would need to be updated (sender creates with recipient as owner) and likely this wouldn't be BC. So maybe all we can get away with for now is something like mrclay@f83763e (from #8776)

[ewinslow]

What kind of testing do we need to do here to make sure this doesn't break discussions or something else?

[mrclay]

In case I wasn't clear, @juho-jaakkola's is right that, with this change, a very poorly designed plugin could allow one to post in a group impersonating another member's GUID as owner. I personally don't think that's worth worrying about, but our inability to really test this is a bigger problem.

The solution in #8776 is crude but also IMO almost risk-free. The urge to get 2.0 out make me lean that way.

[mrclay]

I'm a dummy. Solution is to verify owner container only if owner doesn't match logged in user. I'll update this later today and if there are no strong objections I'm just going to pull it in and finish off the associated changes to discussions.

[mrclay]

Alright, please take another look (updated description) if you have time.

[hypeJunction]

Seems correct.

[ewinslow]

Can you articulate the new intended behaviors in the form of test cases? Permissions code is the most sensitive part of the system. It may be worth it to do whatever refactoring is needed to get real tests in.

"Since this causes confusion" is vague. Can you be more specific about what goes wrong in the case you describe?

"Can write to the owner's container" seems wrong to me. Usually owners are users and don't have containers. Do you mean "can write to the owner as a container?" Maybe this is what causes confusion?

[ewinslow]

What kind of documentation updates are merited here? Worth adding simmering to the upgrade notes?

[benwerd]

Is there scope for maybe writing best practices about how plugin authors should retrieve and store owner GUIDs? Sticking them in the form - with or without further checking - is obviously a terrible (and lazy) idea, but it might not be obvious to everyone how to better go about it.

[mrclay]

@benwerd yeah, good idea. I've updated upgrade docs and the docs for the hook. Still considering what to do for testing.

[juho-jaakkola]

I'll take a closer look later tomorrow.

[juho-jaakkola]

LGTM

[juho-jaakkola]

We should probably log each case when we return false.

User <guid> tried to create <type> <subtype> in the entity <container_guid> but the container could not be loaded.

[juho-jaakkola]

@mrclay Can you make the changes so we can merge this?

[mrclay]

Changes made.