As the title says: copy over node permissions to the group level and implement the trumping of sitewide permissions as described in group_access().

Files: 
CommentFileSizeAuthor
#30 group-node_access-1976696-30.patch21.97 KBchaby
#30 interdiff-2032053-29-30.txt5.42 KBchaby
#38 group-node_access-1976696-38.patch9.27 KBchaby
#38 interdiff-1976696-37-38.txt2.53 KBchaby
#37 group-node_access-1976696-37.patch8.15 KBchaby
#34 group-node_access-1976696-34.patch17.88 KBkristiaanvandeneynde
#34 interdiff-1976696-27-34.txt4.96 KBkristiaanvandeneynde
#31 group-node_access-1976696-29-31.patch17.27 KBchaby
#31 interdiff-2032053-29-31.txt479 byteschaby
#31 group-node_access-1976696-30-31.patch21.97 KBchaby
#31 interdiff-2032053-30-31.txt479 byteschaby
#28 group-node_access-1976696-28.patch17.63 KBchaby
#28 interdiff-2032053-27-28.txt7.78 KBchaby
#29 group-node_access-1976696-29.patch17.27 KBchaby
#29 interdiff-2032053-27-29.txt4.97 KBchaby
#27 group-node_access-1976696-27.patch17.81 KBkristiaanvandeneynde
#27 interdiff-1976696-24-27.txt9.06 KBkristiaanvandeneynde
#24 group-node_access-1976696-24.patch13.42 KBkristiaanvandeneynde
#24 interdiff-1976696-21-24.txt2.35 KBkristiaanvandeneynde
#21 group-node_access-1976696-21.patch15.06 KBkristiaanvandeneynde
#21 interdiff-1976696-17-21.txt1.45 KBkristiaanvandeneynde
#18 group-node_access-1976696-18.patch11.97 KBchaby
#18 interdiff-2032053-17.txt2.03 KBchaby
#17 group-node_access-1976696-17.patch12.77 KBchaby
#17 interdiff-2032053-16.txt1.64 KBchaby
#16 group-node_access-1976696-16.patch12.63 KBchaby
#16 interdiff-2032053-15.txt4.28 KBchaby
#15 group-node_access-1976696-15.patch12.52 KBchaby
#15 interdiff-2032053-13.txt2.02 KBchaby
#13 group-node_access-1976696-13.patch13.08 KBkristiaanvandeneynde
#10 group-node_access-1976696-10.patch55.65 KBchaby
#10 interdiff-1976696-9.txt13.73 KBchaby
#9 group-node_access-1976696-7.patch51.33 KBchaby
#9 interdiff-1976696-7.txt17.92 KBchaby
#8 interdiff-1976696-6.txt4.81 KBchaby
#6 group-node_access-1976696-6.patch34.94 KBchaby
#5 group-node_access-1976696-5.patch32.87 KBchaby
#4 group-node_access-1976696-4.patch32.87 KBchaby

Comments

chaby’s picture

I'm working on it and want to clarify was i'm intented to do/having already done :

- to prevent other node access module, tag node access needs to be rebuild in hook_install

- implements hook group permission to provide CRUD permissions on all active node types

- implements hook_node_access :

for account which has at least one of the same membership of the entity (node)

=> allow view

=> allow create, update, delete based on group permission (including owner or all => own / any). There is an exception for "update any" : current account needs to have all same groups of the entities. For example, it would prevent an update of the node which remove a previous group selected by the owner because current user which could edit any content have not this membership but have same another membership (group A but not B).

- hook_node_grants : give the filter query arguments for conditions againsts node_access table with gids of the accounts provided, keyed by a realm relative to the group type associated to his groups.

- hook_node_access_records : while saving the node, insert/update/delete current gids of the entity into node_access , also keyed by realm group type.

These parts is already done but needs some testing.

What is missing at least :

- providing a way to the end-user to select his membership to associated with the node. Maybe also list it on the group view.

I was thinking about field extra as it takes power of the display fields (user could hide it, place it weight as it want). And we are using a "fake" field which aims at populating group_entity table.

I'm working on it and want to clarify was i'm intented to do/having already done :

- to prevent other node access module, tag node access needs to be rebuild in hook_install

- implements hook group permission to provide CRUD permissions on all active node types

- implements hook_node_access :

for account which has at least one of the same membership of the entity (node)

=> allow view

=> allow create, update, delete based on group permission (including owner or all => own / any). There is an exception for "update any" : current account needs to have all same groups of the entities. For example, it would prevent an update of the node which remove a previous group selected by the owner because current user which could edit any content have not this membership but have same another membership (group A but not B).

- hook_node_grants : give the filter query arguments for conditions againsts node_access table with gids of the accounts provided, keyed by a realm relative to the group type associated to his groups.

- hook_node_access_records : while saving the node, insert/update/delete current gids of the entity into node_access , also keyed by realm group type.

These parts is already done but needs some testing.

What is missing at least :

- providing a way to the end-user to select his membership to associated with the node. Maybe also list it on the group view.

I was thinking about field extra as it takes power of the display fields (user could hide it, place it weight as it want). And we are using a "fake" field which aims at populating group_entity table.

-EDIT : new tasks :

- how to define if node access is enable and could we specify which node types are concern per group types ? I was thinking about adding these settings in the group type UI controller, adding links into the group type UI view (just after "permissions" columns). For storing these data, we need further things :

- add a new column in group_type, like 'node_types' which would be serialized to use entity API has it is already done + adding a new property to the entity, just like 'outsider' for example

- add a new method in the group entity class to get node types of the group type + adding an helper for this in helper/group.inc

Probably miss other things ? Are you ok with this ?

thanks

Probably miss other things ? Are you ok with this ?

thanks

kristiaanvandeneynde’s picture

I'm not going into the how (code) of it, but more the what (logic) of group node permissions.

I'd like to see something along the lines of:

  • User interface
    • A way to couple nodes to groups, perhaps as a vertical tab at the bottom of the node editing screen
    • This would only show the groups available to you as options.
  • Permissions
    • Attach content to this group
    • Detach content from this group
    • Create / Delete /Edit any content
    • Create / Delete /Edit content of type X
    • Create / Delete /Edit content of type Y
    • Create / Delete /Edit content of type Z

However, I see a few problems with that:

  • If we allow people to create content in a group and only that group, we are sure that content can never have multiple parents and we can simplify the logic behind all this.
  • If we allow people to create one node that is shared across multiple groups, things get complicated:
    • If content exists in group A and B and someone can delete it in group A, what happens to it for group B?
    • If content exists globally and someone can attach it to a private group, all of the sudden the content will no longer be available to anonymous visitors
    • ...

I think, before we go ahead with this, we need to figure out on how we want nodes to be attached to groups. Do we want one node to exist in one and only one group or do we want the ability to share nodes across multiple groups?

Or, do we still want entities (generally speaking) to follow a many-many relationship, but build a many-one relationship for nodes only. After all, user entities are 'special cases' too: they are handled by GroupMembership.

chaby’s picture

For the UI, it could be better to separate content management from node membership management (to avoid for e.g editing lock). We could achieve it by adding a new menu local tasks tab beside 'view' and 'edit' but the problem is that node will be first time public...

EDITnot really agree with permission attach/detach. If a user could create or edit the node, he could attach it to its group(s). If no group are attached, the node is public. Keep it stupid (no ?).
In fact, these permissions are needed. I mix content management and group management. This is not the same things so agree with this idea (one permission like 'manage node membership' per node type). Anyway, create without manage membership permission means that the first time the node is public...which is maybe not intented (and if unpublished, the "node membership manager" need to view unpublished node !)...

if we allow people to create content in a group and only that group, we are sure that content can never have multiple parents and we can simplify the logic behind all this.

sure, by doing this (allow only one group to be attach), it limits the system but could prevent a lot of bugs like :

  • group hierarchy access : what relations order ? parent have access on children and children don't have access to parents ?
    use case : We have a group A and a subgroup AA. If a member of AA create a node attach to AA, member of AA and member of A could have access on it. If a member of A edit the node, we must also list its children as group to be attached. Why not. But if group AA attached is removed, what happens for the member AA who is still the author ? Denied access for its original own content ? (why not after all !).
    Technically :
    • lookup recursively for every parents, could cost some additionnals query and complicate the code. e.g : node is attach to AAAA, member of A try to access it. It will make 3 queries : AAAA => AAA, AAA => AA, AA => A.
    • when node is attach to AA and is presave, we could merge in group_entities : A gid <> nid, AA gid <> nid. If we detach AA, how to programmatically know if parent A what implicitly inserted or explicitly selected (for example, select A first. Save the node. Then attach AA too.) ?

    EDIT : anyway, we could maybe have a simple workaround for this. When creating a subgroup (or attach it to the group parent), we could just copy membership from its parent. In that way, member of AAA copy membership from AA which have previously copy membership of A. Of course, when subgroup is detach from its parents, remove membership which are in its parent. But...we could removed a parent membership which was previously added (before it was a subgroup) :/

  • - an another use case about multiple group : member of AA only create the node. Then a member of A and B edit the node and attach it also to the group B. Then, member of AA edit the node. What happens for the group listing ? We cannot list group B because he is not membership of it...but we needs to include it as value. Indeed, when node is updated, we must don't forgot that group B was previously added...so we could maybe store these values on server side only to be "fixed".
  • and so on...we could have a lot of different use cases :-)

If content exists in group A and B and someone can delete it in group A, what happens to it for group B?

if content is deleted....it is just deleted ! whatever which member of which group do it...this is not a problem.

If content exists globally and someone can attach it to a private group, all of the sudden the content will no longer be available to anonymous visitors

not a problem too ! That's why node access exists...if user editor decide to do it, it is on its own risks...

Or, do we still want entities (generally speaking) to follow a many-many relationship, but build a many-one relationship for nodes only. After all, user entities are 'special cases' too: they are handled by GroupMembership.

sorry but I don't understand what you mean...! could you explain it in another way please ?

In one hand, only one group = keep it stupid, easy to maintains. And in other hand, we will loose the content sharing (with its complexity).

So I think that to begin, it is more easy to implements "only one group attach to a node". With this starting point, we would maybe have a better overview and see later if it is possible to implements it for multiple group support, including complex hierarchical group. But of course, this feature will need more reflection.

What is your point of view about it ?

thanks

chaby’s picture

Status:Active» Needs work
StatusFileSize
new32.87 KB

Hi,

Sorry to put the cart before the horse but for our project we need to go ahead quickly...

Of course, it doesn't only need review but needs work after all (finally what's we want ? All of this could be changed, no problem).

IMPORTANT : to work properly, this patch needs also this patch. It will be reroll ASAP.
And also don't forgot to enable simpletest module and clear the cache for file registry.

A summarize to what this patch aims to do/introduce :

  • only one group could be attach to a node.. So introduce node access per groups and nodes. A granted user could select only one of its group to be attached to the node. This is more easy to begin and prevent a lot of complex/unexpected behaviors.
  • new permissions for all content type (disabled or not). Permissions are :
    • create
    • update own
    • update any
    • delete own
    • delete any

    These permissions mean that :

    • there is no special content type private per group type. : to have a private content type, assign "create" and/or "update own/any" group permission per group type for this node type. If you don't want that this content type could be private, just not set these permissions.
    • no "manage node membership" group permission : to achieve it (attach/detach), user needs these group permissions (create and/or update). It could prevent some unexpected behavior such as : a user could create a node and admin forgot to set "manage node membership" to this user. So first time node is created by this user, the node is public (and this is not expected, published or not). Furthermore, this is more simple like this for now :-). What should be changed/improve is to :
      • allow attach/detach permission while creating a node only (not updating) ? doesn't really make sense...?
      • finally add this permission ''manage node membership' and group admin just have to don't forgot to set this permission for a member which could create a node of this type (of course, adding some doc about it !). Why not adding a validate handler on the group role permission tabs to avoid it...
    • no node strict access like OG : if node is a group entity, it is private. Otherwise (not a group entity), it is public. It means that a an outsider with the node permission 'create' could only create public node. And a member with the group permission 'create' could create both : public and/or private node attach to one of its group.
    • new test case classes under directory "/tests" (because we couldn't play with permissions without testing !) :
      • add a group helper test case : should be improve of course, but currently third methods are enough for node access test case.
      • a test case specific to node access (node permission and group permission) without UI. At the first look, we could have the feeling that it goes too far. But in fact, all test methods are needed : we want to be sure that each permissions in group and default node contexts act as expected. We could still review how they are done (executed). To save a lot of time (from 3 to 20 minutes on my desktop with VM), they are executed manually instead of using default simpletest "behavior". I will let's you take a look at ::testMain() comment ;-)
    • what still needs work at least :

      • an UI for the end-user to attach/detach node to group (and so be private or public). In one hand, seems that separate content management from membership management won't be easy even if better. But in other hand, with a "one group per node", it avoid conflict between concurrent node membership changes (however, not concurrent management content :s )...
        Whatever which approach would be kept, I would like to :
        • attach/detach node programmatically. For e.g :
          $node->gids = array(1, 2, 3);
          node_save($node);

          and the job is done without UI (whatever how but need to be done into a presave step to save grant records).

        • place element easily as I want
        • That why maybe using hook extra fields could be a solution : field attach, field display features are supported and we don't add new DB fields which must don't be used :p

      • needs work and a lot of review (sorry for spelling mistakes) :s

      Sorry again to boost this issue and thank you a lot to review it, giving feedbacks on what should be review, changed, and so on.

chaby’s picture

StatusFileSize
new32.87 KB

sorry reroll patch due to untracked file error.

chaby’s picture

StatusFileSize
new34.94 KB

reroll patch to add testing for unpublished node.

kristiaanvandeneynde’s picture

Hi there Yannick,

I'll look into this (and your other patches) as soon as possible.
I've got a busy schedule, which should clear up this afternoon or tomorrow.

In the mean time, you can go ahead and patch your own version of Group with these patches. If the code in there makes any sense and has something to offer to Group, it will go in one way or another.

Regards,
Kristiaan

P.S.: When creating multiple patches, this link could help :)

chaby’s picture

StatusFileSize
new4.81 KB

Hi Kristiaan,
Ok no problem.

P.S.: When creating multiple patches, this link could help :)

oh sorry ! indeed, strongly advice to attach also an interdiff for visibility :)

chaby’s picture

StatusFileSize
new17.92 KB
new51.33 KB

Hi, reroll patch with new features/bug fixes :

  • fix a bug in group_node_access.test while creating a group admin member account. Indeed, we needs to create a node type first, purge entities defaults to take into consideration new group type permissions for the role entity group_admin.
  • add an UI to end-user to attach/detach one of its groups to a node (with an extra field).
  • add a new test case classe for this UI integration.

What could be done in addition (except review :-)) :

  • find a way to require group member to publish private group content only. For the moment, the member could choice between public or private... Maybe this is not an expected behaviour.
    • At the beginning, I was thinking about adding a new group permission "publish public node". if user have not this permission, field group is required instead of optional. But this permission could be set to a role which is not global. So what we do ? Check if user has the permission into one of its group ?
    • or set a global variable config to force each node to be private if editor have not node permission but group permission ?
    • EDIT : an another simple approach would be to check if editor has node permission (like create or edit). if true, group select is optional and so group could be public. Otherwise, field is required and content private. I will work on this.

    TBH, i didn't really know how to deals with for the moment :s (but don't want an approach like OG where content type is public or private, we want both possible).

  • display group attach on the node view page with an extra field display ? Is that very necessary (as we have only one group attach to a node) ?

thanks

EDIT : sorry for the mistake with comment num into patch and interdiff. Patch should be -9 and interdiff -6

chaby’s picture

StatusFileSize
new13.73 KB
new55.65 KB

Reroll patch including :

  • new commit and so no need to this previous patch.
  • add a way to force member to post private node only. To force it, just not give a drupal role to this user with default node permission for this node type (not having at least one of permissions for this content type).
    Otherwise, author could also post public node.
  • And so, test have been reviewed.

thanks

kristiaanvandeneynde’s picture

Hi there Yannick,

I'm looking into your patch as we speak.
I like your suggestions in #4 a lot, after patch review I'll post my findings.

kristiaanvandeneynde’s picture

Hi there Yannick,

I'm almost done reviewing your patch. I've mixed it up quite a bit but am not entirely done yet. Seeing as I'm out of office tomorrow, I'll only be able to post a reviewed patch on Friday. You should tune in then.

Regards,
Kristiaan

kristiaanvandeneynde’s picture

Status:Needs work» Needs review
StatusFileSize
new13.08 KB

I've completely reviewed and reworked your patch.
I'm still looking into your test cases, but I thought you could already take a look at the actual code.

No interdiff because the code is completely different.

kristiaanvandeneynde’s picture

Small correction:

<?php
     
// Allow the user to edit own content of this type.
     
if ($op == 'update' && group_access("edit own $node_type->type", $group, $account)) {
       
$grants["group:$group->gid:$node_type:update"][] = $account->uid;
      }

     
// Allow the user to delete own content of this type.
     
if ($op == 'delete' && group_access("delete own $node_type->type", $group, $account)) {
       
$grants["group:$group->gid:$node_type:delete"][] = $account->uid;
      }
?>
chaby’s picture

StatusFileSize
new2.02 KB
new12.52 KB

Hi, here is just a quick patch including some bug fixes (also from #14).

I will review it Monday.

chaby’s picture

Status:Needs review» Needs work
StatusFileSize
new4.28 KB
new12.63 KB

Hi,
finally, got some time to review it completly and here is my feedback :

  • In fact, I was using in #10 grant system only for 'view' for further reasons :
    1. Probably that in 99% of use cases, we only need it for view. Indeed, seems that there is not a lot of query tag node access which use metadata to set an another op than 'view'.
    2. Mostly, I fear to decrease performance.
      • making additional queries for 'update' and 'delete' op which could be avoid with hook_node_access in most of use cases (not all). For e.g, when connected and viewing a node, system will check node access for 'update' op to know if it could display the contextual tab 'edit' near to tab 'view'.
      • this is maybe not the only module enable which could deals with grant system. An important website with 1000000 of group node published will have 7000000 rows in node_access table just for this module (instead of 1000000 if just add view grant). Fortunetly, we cannot have more than one group per node !! Anyway, I didn't make any profiling so I don't know if it could have a real impact on performance or not...

    But...

    1. For 1% of other cases, grant system is required for node op update + delete and realm bypass + administer ! For e.g, a select with tag 'node_access' and addMetaData 'op', 'update' to get a list of node that could be updated by the current user.
    2. not sure we will make a lot of additionals queries....(maybe only one or two ?)
    3. As say for node_access table size, decreased performance needs profiling and depends on a lot of others parameters !
  • Add a new patch to fix some little errors and logic review :
    • add a constant for "bypass group" user permission gid. In that way, other modules don't need to remind your birth date and I don't think it will change !! :p
    • I supposed that user permission 'bypass group' must be used only for group node (don't bypass other node grant access with !) ?
    • useless to check next statements if current match, no ? BTW, it could also remove one useless condition in query altered if op is update or delete and member could do any...
      But of course, any must be done before own group permission.
    • check if $node->group exists and > 0. Indeed, node could be saved programmatically. So hook node prepare might not be called...?
    • some minor mistakes...
  • not changing it but using $node->gid instead of $node->group could avoid confusions.
  • change status to this issue to 'needs work' because we are waiting test cases :-)

otherwise, thanks, good job !

chaby’s picture

StatusFileSize
new1.64 KB
new12.77 KB

Ooops...
BTW, allow administer user to create node.

chaby’s picture

StatusFileSize
new2.03 KB
new11.97 KB

Hi, an another patch to removed grant bypass group. It is in fact not needed because already checked in group_access(). In one hand, we multiply entry by 6 instead of 7. But in other hand, for account which have this permission (not a lot), we will make queries (node types and groups) and loop unnecessary.

Otherwise, I have further questions :

  • both patch do not deals with group permission which could be set for outsider throught UI but of course not use by node access / grant system. Maybe just disable checkboxes on UI and adding a new key in hook_group_permission like 'member_only' => TRUE/FALSE with FALSE by default...?
  • group permission 'view' disturb me... Member could not view private node (use an another content access system instead ) ? any role member could not view private node (except admin and bypass) ? is that really make sense ?
  • From #16, finally in other words, do we really need to use grant system for other op/use cases than 'view' ? Do we really need to pollute node access table and maybe doing additionnals queries (?) just for use cases which happen rarely ? Or what I have miss ?
  • thanks

kristiaanvandeneynde’s picture

From #16

Add a constant for "bypass group" user permission gid. In that way, other modules don't need to remind your birth date and I don't think it will change !! :p

This seems better indeed

I supposed that user permission 'bypass group' must be used only for group node (don't bypass other node grant access with !) ?

Good catch

Useless to check next statements if current match, no ? BTW, it could also remove one useless condition in query altered if op is update or delete and member could do any...

Correct, I'm going over the code to see if we can further optimize this.

Check if $node->group exists and > 0. Indeed, node could be saved programmatically. So hook node prepare might not be called...?

Again, good thinking

From #17

I believe "administer user" should be "administer group"?

From #18

We should not remove the bypass grant because it allows us to shortcircuit a lot of group_access() calls and because someone who has 'bypass group access' doesn't necessarily have the 'delete any page' checkbox checked (lazy admins...).

Both patch do not deals with group permission which could be set for outsider throught UI but of course not use by node access / grant system. Maybe just disable checkboxes on UI and adding a new key in hook_group_permission like 'member_only' => TRUE/FALSE with FALSE by default...?

The idea here is that you could allow outsiders to post some sort of content in a group, even if you're not a member. Think of a group 'Code experts' with a content type 'Code question' that outsiders could post to address the experts.

Group permission 'view' disturb me... Member could not view private node (use an another content access system instead ) ? any role member could not view private node (except admin and bypass) ? is that really make sense ?

Content type based view access could allow for some fine grained group displays. For instance, the pages could be public while a forum post could be members-only.

From #16, finally in other words, do we really need to use grant system for other op/use cases than 'view' ? Do we really need to pollute node access table and maybe doing additionnals queries (?) just for use cases which happen rarely ? Or what I have miss ?

See this blog post.
If we use hook_node_access(), we completely rule out other modules:

  • If we deny access, that's the end of the line.
  • If we allow access, only another module denying it could stop us from opening the gates to a specific node.
  • Node grants "play nice" with other modules -and- have the benefit of working with database queries.
chaby’s picture

I believe "administer user" should be "administer group"?

oops... yes !

We should not remove the bypass grant because it allows us to shortcircuit a lot of group_access() calls

yes, agree as say in #18...

because someone who has 'bypass group access' doesn't necessarily have the 'delete any page' checkbox checked (lazy admins...).

sorry but I don't really understand...We are talking about group permission ? if true, check will already be done with group_access() :

function group_access($permission, $group, $account = NULL) {
  global $user;

  if (!isset($account)) {
    $account = $user;
  }

  return user_access('bypass group access', $account)
    || $group->userHasPermission($account->uid, $permission);
}

however, why not adding an another "grant case" to prevent useless checks for this specify use case (not a lot of account are supposed to have this permission !).

The idea here is that you could allow outsiders to post some sort of content in a group, even if you're not a member.

oh I see...but :

  • why not just use default node permission ?
  • anyway, if outsider are allow to post node for groups, currently with UI he could not select one...no ?
  • furthermore as say in #18, hook_node_access and hook_node_grant needs to be completly review because we always check first if we are in a "context group" (groups of provided account, groups of provided node)...no ?

if we use hook_node_access(), we completely rule out other modules

indeed, I didn't think about this.

kristiaanvandeneynde’s picture

StatusFileSize
new1.45 KB
new15.06 KB

Small cleanup from #17, should be good now.
You can base further patches off of this one.

I'll look into the UI for outsiders now.

I also suggest we hold of tests until the code is in a somewhat 'frozen' state.
I've created a task for that: #2037881: Write tests

chaby’s picture

Hi,

I'll look into the UI for outsiders now.

Ok cool. But it mainly needs review in hook_node_access and hook_grant_access to allow outsiders account to use group permission without a group...

I also suggest we hold of tests until the code is in a somewhat 'frozen' state.
I've created a task for that: #2037881: Write tests

of course, good idea.

kristiaanvandeneynde’s picture

Status:Needs work» Needs review

Ok cool. But it mainly needs review in hook_node_access and hook_grant_access to allow outsiders account to use group permission without a group...

That's already covered in Group?

<?php
 
/**
   * Retrieve a user's permissions for a group.
   *
   * @param int $uid
   *   The uid of the user to retrieve the permissions for.
   *
   * @return array
   *   An array of group permission names.
   */
 
public function userPermissions($uid) {
    return (
$group_membership = $this->getMember($uid))
      ?
$group_membership->getPermissions()
      :
group_type_load($this->type)->outsider_permissions;
  }
?>
kristiaanvandeneynde’s picture

StatusFileSize
new2.35 KB
new13.42 KB

I've adjusted the JavaScript to only allow for one value in $node->group.
Furthermore I've added some documentation and clearer names.

This should be good to go.

chaby’s picture

That's already covered in Group?

yes but not in hooks.
Hook node access :

function group_node_access($node, $op, $account) {
  if (is_string($node)) {
    // Check for creation rights in the user's groups.
    if ($op == 'create' && isset($account->uid)) {
      foreach (group_load_by_member($account->uid) as $group) {

==> OUTSIDER NEVER ENTER IN THIS LOOPS, NO ??

        if (group_access('administer group', $group, $account) || group_access("create $node", $group, $account)) {
...

Hook node grant :

function group_node_grants($account, $op) {
  $grants = array();

  $node_types = node_type_get_types();
  foreach (group_load_by_member($account->uid) as $group) {

==> OUTSIDER NEVER ENTER IN THIS LOOP, NO ??

    // If the user is an admin, he requires no further specific grants.
    if (group_access('administer group', $group, $account)) {
...

did you understand now why I was talking about this in my previous comment ? Or I completly miss something...?

kristiaanvandeneynde’s picture

Status:Needs review» Needs work

Good catch, I'll look into this.

kristiaanvandeneynde’s picture

Status:Needs work» Needs review
StatusFileSize
new9.06 KB
new17.81 KB

I think I solved the issue mentioned in #25, but I'm not entirely sure whether the code isn't a bit too verbose right now. It looks good, but could be 'cleaner'?

chaby’s picture

StatusFileSize
new7.78 KB
new17.63 KB

Hi,
Not really something to clean to my mind but could be optimized.
Here is a patch with an horrible interdiff, sorry for that. Horrible because in fact, I just change some minor things (!) :

  • in group_node_access, use a static cache for op create only. Indeed, node_access() could be called more than once (to build links in menu like 'add this node', breadcrumb and so on...)...this is minor I agree :-).
    EDIT : also presume that we will always have an account object with uid attribute. No ?
    Mostly, static cache is wrong. Next checks are unnecessary executed. So needs work, sorry !
  • in group_node_grants, just check if $outsider_gids exists. If not, useless to execute all other next checks ?
  • in group_form_node_form_alter(), even if result are already statically cached, useless to call again API to retrieve member groups which are already here.
  • Otherwise, didn't change anything but could understand that we give priority to outsider before members. I don't think this use case would happen very often (allow some permissions to outsider). In that way, we could save a lot of useless query if user is a member and have permissions (not loading groups from group types, loading group roles, ...) ? If he don't have permission inside its own groups, check if it is outsider of other groups of groups types and check if outsider have permissions...no ?

thank you for the work done !
EDIT : of course, ignore this patch, see#29 :-)

chaby’s picture

StatusFileSize
new4.97 KB
new17.27 KB

ok reroll it from #27 to remove for the moment wrong static cache in group_node_access.

chaby’s picture

StatusFileSize
new5.42 KB
new21.97 KB

Hi Kristiaan,

As I think it begins to be more stable, I would suggest here some code to paste into a custom module if :

  • you know what you are doing : you don't/won't have any other modules which would interact with the node group access system.
  • want to save some disk space in node_acess table and prevent just some few queries.

It is really important to understand that this implementation have more drawbacks than advantages. Just read link posted by kristiaan in #19.
EDIT : fix some bugs

/**
* Implements hook_node_access().
*/
function MY_MODULE_node_access($node, $op, $account) {

  //Act only on group node for update and delete op.
  if (in_array($op, array('update', 'delete'), TRUE)) {

    // Node could not have more than one group.
    $node_groups = group_get_entity_groups('node', $node->nid);
    $node_group = array_shift($node_groups);

    //With this implementation, we don't ignore access to rely on group
    //grant system because we want to force it. That's why using this
    //implementation should be choice carefully. We didn't allow any other
    //modules except node module itself to interact with node access permission.
    if ($node_group) {

      //Deny access by default.
      $access = NODE_ACCESS_DENY;

      // If the user can bypass group access, he is allowed access.
      if (user_access('bypass group access', $account)) {
        $access = NODE_ACCESS_ALLOW;
      }
      else {

        //Check that account is author of the node.
        $owner = ((int)$node->uid === (int)$account->uid);

        //Get permissions of account if member or outsider.
        $permissions = $node_group->userPermissions($account->uid);

        //Administer account could edit or delete any content of this group.
        if (in_array('administer group', $permissions, TRUE)) {
          $access = NODE_ACCESS_ALLOW;
        }
        //Otherwise, check specific permissions based on op.
        elseif ($op === 'update' && (in_array("edit any $node->type content", $permissions, TRUE)
          || ($owner && in_array("edit own $node->type content", $permissions, TRUE)))) {
          $access = NODE_ACCESS_ALLOW;
        }
        elseif ($op === 'delete' && (in_array("delete any $node->type content", $permissions, TRUE)
          || ($owner && in_array("delete own $node->type content", $permissions, TRUE)))) {
          $access = NODE_ACCESS_ALLOW;
        }
      }

      //Force allow or denied access.
      return $access;
    }
  }

  return NODE_ACCESS_IGNORE;
}

/**
* Implements hook_node_access_records_alter().
*/
function MY_MODULE_node_access_records_alter(&$grants, $node) {

  //For node group, only insert 'view' and 'bypass' real.
  if (!empty($node->group)) {

    foreach ($grants as $key => $grant) {
      if (strpos($grant['realm'], 'group:') === 0
        && $grant['realm'] !== "group:$node->type:view"
        && $grant['realm'] !== 'group:bypass') {
        unset($grants[$key]);
      }
    }
  }
}

/**
* Implements hook_node_grants_alter().
*/
function MY_MODULE_node_grants_alter(&$grants, $account, $op) {

  // Gather the machine names of all node types.
  $node_types = array_keys(node_type_get_types());
  foreach (array_keys($grants) as $realm) {
    //Alter only group grants
    if (strpos($realm, 'group:') === 0) {

      //Just in case, be sure to be in an op 'view'.
      if ($op === 'view') {
        //Add view realm equivalent with all gids currently set and remove it.
        if ($realm === 'group:administer') {
          foreach ($node_types as $node_type) {
            $grants["group:$node_type:view"] = $grants[$realm];
          }
          unset($grants[$realm]);
        }
      }
      //Otherwise, be sure to remove all other grants except bypass to be
      //coherent if op isn't 'view'.
      //@see MY_MODULE_group_node_access_records_alter().
      elseif ($realm !== 'group:bypass') {
        unset($grants[$realm]);
      }
    }
  }
}

NOTE : it still need review but some functional tests have been executed successfully (for op update and delete only of course).
Finally, I was wondering if it couldn't be directly include with this patch ? :

  • copy/paste this code for every project having group module and don't care about using grant system would be very boring...
  • we could had a simple variable to switch between mode (of course, default to use grant system and advice the end-user that it must be sure to use "override grant system").
    But it would introduce a big deal : switching mode must not be allowed if at least one group node exists.
    Indeed, switching from node access to grant access means that new grants must be register into node_access for existings group nodes.
    On the contrary, not bad but more clean, would be to remove useless entries in node_access table (except view of course).
    So it needs at least prevent variable in UI to be disabled if a first group node exists (whatever which previous mode was). And let's developers doing their own migration (just update nodes, no ?) and forcing settings on their own risks.
  • To achieve it, merge current code with this is not simple and we would loose visibility.
    Here is a simple patch example which needs some improvements (merge hooks ?).
  • I doubt you would be agree with this and I could easily understand why :-). But just in case...! because it could be a good alternative for optimization ?

    thanks

    chaby’s picture

    Here is a patch for both #29 and #30 (just in case...) which fix an error with group gid attach to node (it could be set to 0 by default in hook_node_prepare if node is not already attach to a group).

    kristiaanvandeneynde’s picture

    EDIT : also presume that we will always have an account object with uid attribute. No ?

    We check for a UID to make sure the module doesn't throw warnings when an anonymous user wants to create a node.

    In group_node_grants, just check if $outsider_gids exists. If not, useless to execute all other next checks?

    Good point.

    In group_form_node_form_alter(), even if result are already statically cached, useless to call again API to retrieve member groups which are already here.

    Seems valid, will check to be sure.

    Otherwise, didn't change anything but could understand that we give priority to outsider before members. I don't think this use case would happen very often (allow some permissions to outsider). In that way, we could save a lot of useless query if user is a member and have permissions (not loading groups from group types, loading group roles, ...) ? If he don't have permission inside its own groups, check if it is outsider of other groups of groups types and check if outsider have permissions...no ?

    We always check for outsiders first because it is more likely that someone is member of some groups, but outsider to most other groups than the other way around. We therefore will always have to check someone's permissions in the groups that he isn't member of.

    Reviewing #30next.

    kristiaanvandeneynde’s picture

    Okay, I've given this some thought and I've come to the following decision regarding #30:

    • Adding this into Group as either a switch or part of the documentation will only confuse end users.
    • If, for some reason, Group's node access implementation fails to meet a module owner or site builder's expectations, they are most likely clever enough to write an implementation that suits their needs.

    For those reasons, I've chosen not to go ahead with #30.
    The patch in #29 and fix in #31 are both good, though.

    I'm doing a final review and then I'll submit a patch that should be ready to go in.

    P.S.: I've noticed you often add an empty line below a control statement (mostly 'if'). While it's not explicitly mentioned in https://drupal.org/coding-standards, we prefer to not add newlines after control statements.

    kristiaanvandeneynde’s picture

    StatusFileSize
    new4.96 KB
    new17.88 KB

    Final patch?

    Anonymous users get a UID of 0, so we'll have to user !empty() instead of isset().

    chaby’s picture

    Status:Needs review» Reviewed & tested by the community

    Seems that we can now go ahead !
    thanks

    kristiaanvandeneynde’s picture

    Status:Reviewed & tested by the community» Fixed

    Fixed as per a96101f

    chaby’s picture

    Status:Fixed» Needs review
    StatusFileSize
    new8.15 KB

    Hi,

    sorry to revive this issue but better to do it here instead of opening a new issue.

    After profiling a little our project, it appears that some useless queries are executed in group_node_grants().
    When a tag node access query is executed, this hook could be called further times by _node_query_node_access_alter() :

    • first time by : node_access_view_all_nodes (only if previously this function haven't been somewhere for that account)
    • then always by node_access_grants through module_invoke_all called

    A minimalist approach is to cache grants per account and op (changing the way how group_membership() is used could be a little complicated maybe not necessary).

    So this patch just wrap function body into :

      static $cache = array();

      if (!isset($cache[$account->uid][$op])) {
        $grants = array();
        $cache[$account->uid][$op] =& $grants;

        ...PREVIOUS CODE...

      }

      return $cache[$account->uid][$op];

    BTW, I didn't notice previously :

    foreach (group_types() as $type => $group_type) {
      $outsider_groups = group_load_by_type($type);

    In fact, we are just loading all groups of the site...I confess that it afraid me a bit ! It seems that it need s to be fixed too...

    chaby’s picture

    Ok, fixed temporary all group entities loaded by just loading all gids per group type the account is an outsider of.
    So here is an another patch in addition to #37.

    chaby’s picture

    Hi,

    previously, I was talking about performance issue by giving permissions to outsiders. Please correct me if I'm wrong but imagine that :

    • we have 200 groups of a group type which allow outsider to view group node
    • 10 node types
    • account provided is outsider and have no bypass group access/administer group permission

    For only one node access query tagged for view op, do we have 200*10 conditions added to the query ?
    If true, it needs a workaround or at least prevent integrator about it.

    kristiaanvandeneynde’s picture

    Currently there are three issues bundled here:

    • Cache the results of group_node_grants()
    • Load 'outsider' groups more efficiently
    • Check performance with outsider permissions

    Perhaps we should split the follow-up of this issue into three separate ones?
    The first and second point in the list could be easily solved. The last one, however, could require more work or be postponed until the code is more final.

    kristiaanvandeneynde’s picture

    • Commit a96101f on 7.x-1.x, ctools_plugins, inheritance by kristiaanvandeneynde:
      Issue #1976696 by chaby, kristiaanvandeneynde: Polyfill permissions for...