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

Closed
mrclay opened this Issue Jul 31, 2015 · 7 comments

Comments

Projects
None yet
2 participants
@mrclay
Member

mrclay commented Jul 31, 2015

If you create an object in a group, ElggEntity::create will call $owner->canWriteToContainer(0, $type, $subtype) even though the $owner (you) will not be the container. This results in a container_permissions_check hook in which you are $params['container'], which is not at all true.

This has worked so far because, up until now, handlers haven't really cared if the content was created under a user or a group. Instead, content placement was controlled upstream by UI code hiding forms from users, e.g.

For discussions, though, (and other types going forward) we want to use the container hook to determine whether that subtype can be created in the given container, a fairly reasonable usage. A few obvious solutions:

  1. Don't check container for the owner if (s)he is not the real container. (potential BC issues)
  2. Somehow let hook handlers know that this isn't the real container. (shouldn't affect existing code)

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

feature(hooks): Adds indication in container permissions hook of chec…
…king owner

For some reason ElggEntity::create checks the ability to write to the entity
owner, even if the owner will not be the entity’s container. In this case,
the container_permissions_check hook is called twice, and handlers cannot
tell which $params[‘container’] will actually be the entity container, and
which is really the owner being checked.

This introduces a flag $params[‘checking_owner’] to let handlers know that
$params[‘container’] is actually not the container of the entity being
created.

Fixes #8774
@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Jul 31, 2015

Member

@ewinslow PTAL and also at #8773. Obviously an alternative would be to remove the owner check from create(), but this seems safer for 2.0.

Member

mrclay commented Jul 31, 2015

@ewinslow PTAL and also at #8773. Obviously an alternative would be to remove the owner check from create(), but this seems safer for 2.0.

@ewinslow ewinslow closed this in #8773 Jul 31, 2015

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

feature(hooks): Adds indication in container permissions hook of chec…
…king owner

For some reason ElggEntity::create checks the ability to write to the entity
owner, even if the owner will not be the entity’s container. In this case,
the container_permissions_check hook is called twice, and handlers cannot
tell which $params[‘container’] will actually be the entity container, and
which is really the owner being checked.

This introduces a flag $params[‘checking_owner’] to let handlers know that
$params[‘container’] is actually not the container of the entity being
created.

Fixes #8774

@mrclay mrclay reopened this Jul 31, 2015

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Jul 31, 2015

Member

#8776 takes approach 2, introducing a checking_owner flag. It might be better to simply add $params['real_container'] to the hook!

Member

mrclay commented Jul 31, 2015

#8776 takes approach 2, introducing a checking_owner flag. It might be better to simply add $params['real_container'] to the hook!

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Jul 31, 2015

Member

I'm going to try to assess when/why that owner check was added.

Member

mrclay commented Jul 31, 2015

I'm going to try to assess when/why that owner check was added.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Jul 31, 2015

Contributor

I believe we have discussed this previously and I had opened a ticket a few
months back because I was running into this issue. You had some
reservations, so you may way to look back at that as well.
On Jul 31, 2015 8:53 PM, "Steve Clay" notifications@github.com wrote:

#8773 #8773 takes approach 2,
introducing a checking_owner flag. It might be better to simply add
$params['real_container'] to the hook!


Reply to this email directly or view it on GitHub
#8774 (comment).

Contributor

hypeJunction commented Jul 31, 2015

I believe we have discussed this previously and I had opened a ticket a few
months back because I was running into this issue. You had some
reservations, so you may way to look back at that as well.
On Jul 31, 2015 8:53 PM, "Steve Clay" notifications@github.com wrote:

#8773 #8773 takes approach 2,
introducing a checking_owner flag. It might be better to simply add
$params['real_container'] to the hook!


Reply to this email directly or view it on GitHub
#8774 (comment).

@mrclay

This comment has been minimized.

Show comment
Hide comment
Member

mrclay commented Jul 31, 2015

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Jul 31, 2015

Member

From the commit I assume this was done to affect widget permissions. @hypeJunction can you find that ticket?

Member

mrclay commented Jul 31, 2015

From the commit I assume this was done to affect widget permissions. @hypeJunction can you find that ticket?

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Jul 31, 2015

Member

Dupe of #4231

Member

mrclay commented Jul 31, 2015

Dupe of #4231

@mrclay mrclay closed this Jul 31, 2015

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

feature(hooks): Adds real_container in container permissions hook params
ElggEntity::create checks the ability to write to the entity owner, even
if the owner will not be the entity’s container. Because this results in
two hook triggerings, handlers may be confused about the real container
that will be used.

This adds `$params['real_container']` within the container_permissions_check
hook, which always indicates the real container to be used, even when the
owner is being checked.

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