New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
elgg-gitbot opened this Issue Feb 16, 2013 · 14 comments

Comments

Projects
None yet
5 participants
@elgg-gitbot

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().

@elgg-gitbot

This comment has been minimized.

Show comment
Hide comment
@elgg-gitbot

elgg-gitbot Feb 16, 2013

ewinslow wrote on 42363799-09-07

This ticket is pretty unclear.

ewinslow wrote on 42363799-09-07

This ticket is pretty unclear.

@elgg-gitbot

This comment has been minimized.

Show comment
Hide comment
@elgg-gitbot

elgg-gitbot Feb 16, 2013

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.

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

This comment has been minimized.

Show comment
Hide comment
@elgg-gitbot

elgg-gitbot Feb 16, 2013

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

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

@elgg-gitbot

This comment has been minimized.

Show comment
Hide comment
@elgg-gitbot

elgg-gitbot Feb 16, 2013

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

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

This comment has been minimized.

Show comment
Hide comment
@elgg-gitbot

elgg-gitbot Feb 16, 2013

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?

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

This comment has been minimized.

Show comment
Hide comment
@elgg-gitbot

elgg-gitbot Feb 16, 2013

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

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

@cash

This comment has been minimized.

Show comment
Hide comment
@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Jul 31, 2015

Member

More context in #8774.

Member

mrclay commented Jul 31, 2015

More context in #8774.

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

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Jul 31, 2015

Member

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

Member

mrclay commented Jul 31, 2015

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

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Jul 31, 2015

Member

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.

Member

mrclay commented Jul 31, 2015

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.

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Jul 31, 2015

fix(4231): Entity creation no longer needlessly checks user container
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 #4231

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Jul 31, 2015

fix(entities): Entity creation no longer needlessly checks user conta…
…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 #4231
@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Jul 31, 2015

Member

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

Member

mrclay commented Jul 31, 2015

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

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Jul 31, 2015

fix(entities): Entity creation no longer needlessly checks user conta…
…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 #4231

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Jul 31, 2015

fix(entities): Entity creation no longer needlessly checks user conta…
…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 #4231

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Jul 31, 2015

fix(entities): Entity creation no longer needlessly checks user conta…
…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 #4231

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Jul 31, 2015

fix(entities): Entity creation no longer needlessly checks owner cont…
…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 #4231

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Aug 1, 2015

fix(entities): Entity creation no longer needlessly checks owner cont…
…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 #4231
@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Aug 3, 2015

Member

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.

Member

mrclay commented Aug 3, 2015

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

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Aug 6, 2015

Member

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

Member

mrclay commented Aug 6, 2015

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

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Aug 6, 2015

fix(entities): Entity creation no longer needlessly checks owner cont…
…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 #4231

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Aug 6, 2015

fix(entities): Entity creation no longer needlessly checks owner cont…
…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 #4231

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Aug 15, 2015

fix(entities): Entity creation no longer needlessly checks owner cont…
…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 #4231

@mrclay mrclay closed this in #8778 Aug 15, 2015

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Dec 22, 2015

Contributor

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.

Contributor

hypeJunction commented Dec 22, 2015

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment