create_entity() calls can_write_to_container() on both owner and container (Trac #4231) #4231

elgg-gitbot · 2013-02-16T19:33:53Z

Original ticket http://trac.elgg.org/ticket/4231 on 41973581-03-26 by trac user ismayil.khayredinov, assigned to unknown.

Elgg version: 1.8

The whole container permissions issues is starting to become a pain.

First of all, I don't understand why this check halts entity create in create_entity:

    if (!can_write_to_container($user_guid, $owner_guid, $type, $subtype)) {
        return false;
    }

What does owner have to do with this???

Secondly, I think as we are moving to an ElggComment class, the entire can write to container will become a big pain. I think by default, any ElggObject should be able to serve as a container. ElggUsers and ElggGroups is a different thing, and I understand why the permissions checks are in place. With ElggObjects there is no reason to restrict writing to ElggObject container - this is in a way equivalent to canAnnotate().

The text was updated successfully, but these errors were encountered:

elgg-gitbot · 2013-02-16T19:33:54Z

ewinslow wrote on 42363799-09-07

This ticket is pretty unclear.

elgg-gitbot · 2013-02-16T19:33:54Z

brettp wrote on 43029992-03-09

I think the bug he's trying to report is that in create_entity() the owner is checked as the container even if a different container_guid is set:

if (!can_write_to_container($user_guid, $owner_guid, $type, $subtype)) {
    return false;
}
if ($owner_guid != $container_guid) {
    if (!can_write_to_container($user_guid, $container_guid, $type, $subtype)) {
        return false;
    }
}

This does seem like a bug to me, so I'm updating the ticket to reflect that.

elgg-gitbot · 2013-02-16T19:33:55Z

Milestone changed to Elgg 1.8.13 by brettp on 43029992-03-09

elgg-gitbot · 2013-02-16T19:33:56Z

Title changed from can_write_to_container() - bugs and suggestions to create_entity() calls can_write_to_container() on both owner and container by brettp on 43029992-03-09

elgg-gitbot · 2013-02-16T19:33:56Z

cash wrote on 43060112-09-18

The access system is brittle so I tend to avoid changing it in a minor release. Do we have a test case for when the current access doesn't work?

elgg-gitbot · 2013-02-16T19:33:57Z

Milestone changed to Elgg 1.8.14 by cash on 43074389-05-22

cash · 2013-07-04T14:38:21Z

See https://gist.github.com/cash/5928150

mrclay · 2015-07-31T19:31:19Z

More context in #8774.

mrclay · 2015-07-31T19:33:45Z

I'm leaning towards removing this check, which was added by @benwerd in 1.5.

mrclay · 2015-07-31T19:37:00Z

Hmm, maybe the intention was to catch creating an entity with a different owner than the current user. I see no explicit check for that. ... Maybe it's supposed to be an ordinary canEdit() check on owner.

ElggEntity::create was checking if the current user could write to the owner's container, even if it was creating an object in a different container like a group. create() now makes sure the user canEdit() the owner (not a container check) instead. This also denies creation if either the owner or container entities can't be loaded. Before, create() would simply bypass a permissions check if it couldn't load the owner/container. Fixes Elgg#4231

…iner ElggEntity::create was checking if the current user could write to the owner's container, even if it was creating an object in a different container like a group. create() now makes sure the user canEdit() the owner (not a container check) instead. This also denies creation if either the owner or container entities can't be loaded. Before, create() would simply bypass a permissions check if it couldn't load the owner/container. Fixes Elgg#4231

mrclay · 2015-07-31T19:58:25Z

PR #8778. I also noticed we just bypass perms checks if owner/container can't be loaded. Was that intentional?

…iner ElggEntity::create was checking if the current user could write to the owner's container, even if it was creating an object in a different container like a group. create() now makes sure the user canEdit() the owner (not a container check) instead. This also denies creation if either the owner or container entities can't be loaded. Before, create() would simply bypass a permissions check if it couldn't load the owner/container. Sites have no owner/container so these checks are bypassed for them. Fixes Elgg#4231

…iner ElggEntity::create was checking if the current user could write to the owner's container, even if it was creating an object in a different container like a group. create() now makes sure the user canEdit() the owner (not a container check) instead. This also denies creation if either the owner or container entities can't be loaded. Before, create() would simply bypass a permissions check if it couldn't load the owner/container. Sites and user have no owner/container so these checks are bypassed for them. Fixes Elgg#4231

…iner ElggEntity::create was checking if the current user could write to the owner's container, even if it was creating an object in a different container like a group. create() now makes sure the user canEdit() the owner (not a container check) instead. 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 Elgg#4231

…ainer ElggEntity::create was checking if the current user could write to the owner's container, even if it was creating an object in a different container like a group. create() now makes sure the user canEdit() the owner (not a container check) instead. 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 Elgg#4231

…ainer ElggEntity::create was checking if the current user could write to the owner's container, even if it was creating an object in a different container like a group. create() no longer makes this check. 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 Elgg#4231

mrclay · 2015-08-03T15:31:58Z

I think longer term #8790 might be a solution. To get 2.0 out the door something simple like the first commit in ~~#8776~~ #8815, which marks the hack container hook.

mrclay · 2015-08-06T03:59:37Z

I think #8815 is the short term solution for 2.0.

…ainer When creating within a group, ElggEntity::create checks if the current user can write to the owner's container. Since this causes confusion, we don't do this if the owner is the current user. 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 Elgg#4231

…ainer 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 Elgg#4231

hypeJunction · 2015-12-22T15:46:56Z

This is biting me again. I am using container hooks to only allow certain subtypes contain other subtypes. The container check on the owner causes trouble, because I don't want users to contain that subtype.

ewinslow removed this from the Elgg 1.10.0 milestone Jun 13, 2014

ewinslow mentioned this issue Jul 31, 2015

chore(discussions): Always allow users to be owners of discussions #8769

Closed

mrclay mentioned this issue Jul 31, 2015

ElggEntity::create verifies the owner writable as container, even if that's not the container #8774

Closed

ewinslow added this to the Elgg 2.0.x milestone Jul 31, 2015

mrclay mentioned this issue Jul 31, 2015

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

Merged

2 tasks

mrclay mentioned this issue Aug 3, 2015

Add ElggUser::canUseAsOwner #8790

Closed

mrclay mentioned this issue Aug 6, 2015

master: Bad UX for user-centered discussions #8757

Closed

mrclay closed this as completed in #8778 Aug 15, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create_entity() calls can_write_to_container() on both owner and container (Trac #4231) #4231

create_entity() calls can_write_to_container() on both owner and container (Trac #4231) #4231

elgg-gitbot commented Feb 16, 2013

elgg-gitbot commented Feb 16, 2013

elgg-gitbot commented Feb 16, 2013

elgg-gitbot commented Feb 16, 2013

elgg-gitbot commented Feb 16, 2013

elgg-gitbot commented Feb 16, 2013

elgg-gitbot commented Feb 16, 2013

cash commented Jul 4, 2013

mrclay commented Jul 31, 2015

mrclay commented Jul 31, 2015

mrclay commented Jul 31, 2015

mrclay commented Jul 31, 2015

mrclay commented Aug 3, 2015

mrclay commented Aug 6, 2015

hypeJunction commented Dec 22, 2015

create_entity() calls can_write_to_container() on both owner and container (Trac #4231) #4231

create_entity() calls can_write_to_container() on both owner and container (Trac #4231) #4231

Comments

elgg-gitbot commented Feb 16, 2013

elgg-gitbot commented Feb 16, 2013

elgg-gitbot commented Feb 16, 2013

elgg-gitbot commented Feb 16, 2013

elgg-gitbot commented Feb 16, 2013

elgg-gitbot commented Feb 16, 2013

elgg-gitbot commented Feb 16, 2013

cash commented Jul 4, 2013

mrclay commented Jul 31, 2015

mrclay commented Jul 31, 2015

mrclay commented Jul 31, 2015

mrclay commented Jul 31, 2015

mrclay commented Aug 3, 2015

mrclay commented Aug 6, 2015

hypeJunction commented Dec 22, 2015