Skip to content
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

can_write_to_container() with a group ignores owner_guid (Trac #3722) #3722

Closed
elgg-gitbot opened this issue Feb 16, 2013 · 12 comments

Comments

@elgg-gitbot
Copy link

commented Feb 16, 2013

Original ticket http://trac.elgg.org/ticket/3722 on 41612506-02-01 by brettp, assigned to unknown.

Elgg version: 1.7

canEdit() is checked, but then if it's a group it checks membership. It shouldn't check membership if canEdit() is true.

@elgg-gitbot

This comment has been minimized.

Copy link
Author

commented Feb 16, 2013

trac user Brett Profitt wrote on 41696525-01-02

Fixes #3722. can_write_to_container() doesn't override canEdit() permissions for group entities. elgg_override_permissions_hook() checks permissions for passed user.
Changeset: 413e15c

@elgg-gitbot

This comment has been minimized.

Copy link
Author

commented Feb 16, 2013

cash wrote on 41758108-06-28

Looks like this has major issues.

@elgg-gitbot

This comment has been minimized.

Copy link
Author

commented Feb 16, 2013

Milestone changed to Elgg 1.7.13 by cash on 41758108-06-28

@elgg-gitbot

This comment has been minimized.

Copy link
Author

commented Feb 16, 2013

cash wrote on 41760435-05-12

Looks like [72a8ae0] also dealt with this. I'm going to take a look at the original problem and the current solution. Then I'll post some thoughts here.

@elgg-gitbot

This comment has been minimized.

Copy link
Author

commented Feb 16, 2013

cash wrote on 41760490-01-20

can_write_to_container() is called from

  • ElggEntity->canWriteToContainer() as a wrapper
  • create_entity() to check if an entity can be added for an owner/container
  • blog, bookmarks, file, and pages plugin to determine if an add menu item should be registered
  • pages plugin to check if a user can edit a page

For the plugin usage, a user may or may not be logged in, but in the cases of a logged out user, it is expected that false is returned.

For create_entity(), we allow logged out users to create entities so that they can register. Further, there may be additional cases for this from 3rd party plugins.

In addition to fixing the bug described in this ticket, the commit also fixed a bug where the user object passed to the plugin hook was ignored by elgg_override_permissions_hook(). This probably should have been its own ticket. This secondary fix introduced the bug that was fixed in [72a8ae0]. The primary fix introduced a bug of the same variety as that one into can_write_to_container().

The fix looks simple - just add a check that the $user variable is non-null in the check for the $container variable being non-null.

I recommend that we add something into our best practices about checking object variables before calling a method on the object.

@elgg-gitbot

This comment has been minimized.

Copy link
Author

commented Feb 16, 2013

trac user Brett Profitt wrote on 41763558-02-05

Fixes #3722. Checking for user when checking container.
Changeset: 66a4e75

@elgg-gitbot

This comment has been minimized.

Copy link
Author

commented Feb 16, 2013

cash wrote on 41777013-07-31

I don't believe these changes have been merged into master/1.8

@elgg-gitbot

This comment has been minimized.

Copy link
Author

commented Feb 16, 2013

cash wrote on 41801637-08-18

Reopening due to change in how access works. See #3979 for details.

@elgg-gitbot

This comment has been minimized.

Copy link
Author

commented Feb 16, 2013

Milestone changed to Elgg 1.7.14 by cash on 41801637-08-18

@elgg-gitbot

This comment has been minimized.

Copy link
Author

commented Feb 16, 2013

cash wrote on 41801694-09-13

Fixes #3722 reverted to previous can_write_to_container() plus fix for group check overriding previous return value. Passed unit tests.
Changeset: b0379f1

@elgg-gitbot

This comment has been minimized.

Copy link
Author

commented Feb 16, 2013

cash wrote on 41847727-11-08

Fixes #4018 Refs #3722 merged up to 1.7.14 into master
Changeset: f19305b

@elgg-gitbot

This comment has been minimized.

Copy link
Author

commented Feb 16, 2013

cash wrote on 41850786-03-02

Fixes #4018 Refs #3722 merged up to 1.7.14 into master
Changeset: f19305b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.