Permissions management

Hi. Nice idea! Patch needs a few tweaks though:

+++ sites/all/modules/user_terms/user_terms.module	(working copy)
@@ -82,20 +90,34 @@
+         || $user->uid == 1

UID 1 always passes a user_access() check so there's no need for this condition.

+++ sites/all/modules/user_terms/user_terms.module	(working copy)
@@ -82,20 +90,34 @@
-      return $display ? user_terms_save_profile($edit, $account, $category) : '';

I'm guessing you're putting a check here to prevent that call deleting all the terms and not resetting them because there's nothing in the $edit array of form data.
Can you take a look at the patch at #728972: Terms are clobbered by user operations when terms are on account? I reckon the fix there might work for this too, meaning you don't need this check here.

+++ sites/all/modules/user_terms/user_terms.module	(working copy)
@@ -82,20 +90,34 @@
     case 'delete':

Deleting an account should surely always delete the terms, no matter who does the deleting or whether that user may edit terms.

I'm on crack. Are you, too?

This is a much needed patch - thanks for this, plazareff.

I've also added breaks after the conditions in the user hook.

Will post back if I have problems. Cheers.

> I've also added breaks after the conditions in the user hook.

Well spotted!

Add that to the list of things to fix with this patch... :)

See #784248: check 'access content' permission. That issue should get fixed before this one.

I've now committed #784248: check 'access content' permission, so if anyone here wants to work on this patch, I'll review it :)

New patch. Completed:

• UID==1 checkings were deleted
• Removed this part of patch:

  -      db_query('DELETE FROM {term_user} WHERE uid = %d', $account->uid);
  -  }
  +      if (user_access('edit any user terms', $account)
  +        || ($account->uid == $user->uid && user_access('edit own user terms', $account))
  +        || $user->uid == 1
  +      ) {
  +           db_query('DELETE FROM {term_user} WHERE uid = %d', $account->uid);
  +      }
  +    }
  

• removed user_access(..., $account) argument, because we need to check editor's permissions.
• Patch is updated to version 6.x-1.x-dev. So, patch's parts like

  -      return $display ? user_terms_form_profile($edit, $account, $category) : '';
  ...
  +        return $display ? user_terms_form_profile($edit, $account, $category) : '';
  

  were changed to:

  -      return user_terms_form_profile($edit, $account, $category);
  ...
  +        return user_terms_form_profile($edit, $account, $category);
  

Review it please.

Great work! That's looking perfect.

But... I had a thought.

We're adding permissions where there used to be none at all.

Logically, to prevent the upgrade breaking existing sites, we should:

- grant 'edit own user terms' to authenticated users, since any user can edit their own account
- grant 'edit any user terms' to roles with 'administer users', since hitherto they could edit terms anywhere.

OK, added user_terms_update_6003() routine.

Heh, you're cranking out patches faster than I can review them!!

I'm in the middle of moving house at the moment; will look at this later in the week. Thanks for tackling it!

Couple of things I spotted in a quick read:

+++ user_terms.install	20 Jul 2010 10:19:33 -0000
@@ -44,12 +44,12 @@ function user_terms_schema() {
+  user_terms_update_6003();

Update_N shouldn't get called here.

+++ user_terms.install	20 Jul 2010 10:19:33 -0000
@@ -106,3 +106,40 @@ function user_terms_update_6002() {
+  // (perm LIKE "%administer users%") is not applicable,
+  // because we can detect permission like 'not administer users' :)

Does core have this kind of pattern too?
Or does it just assume that module developers must be careful to choose permission strings that are not substrings of others?

Powered by Dreditor.

1. OK
2. The second one. I think that if I can avoid hidden and obliviousness bug in future (even if probability is 5%), I should do it. Especially if fix is 3 lines.

Let's follow this pattern: http://api.drupal.org/api/function/system_update_6034
If you really want to guard the string limits, see http://api.drupal.org/api/function/system_update_6039

Thank you for usable link. Regexps is a great idea.

Quick eyeball review:

+++ user_terms.install	21 Jul 2010 09:04:01 -0000
@@ -106,3 +105,34 @@ function user_terms_update_6002() {
+  $rid = db_result(db_query('SELECT rid FROM {role} WHERE (name="authenticated user")'));

Heh. You can use http://api.drupal.org/api/constant/DRUPAL_AUTHENTICATED_RID/6 :)

+++ user_terms.install	21 Jul 2010 09:04:01 -0000
@@ -106,3 +105,34 @@ function user_terms_update_6002() {
+       LEFT JOIN {permission} p ON r.rid = p.rid');

Why do we need to join to {role}? We only need rid and perm string; those are both in the {permission} table.

+++ user_terms.install	21 Jul 2010 09:04:01 -0000
@@ -106,3 +105,34 @@ function user_terms_update_6002() {
+  if (!empty($perms)) $perms .= ', ';

No single line ifs please; always open up a {} block.

+++ user_terms.install	21 Jul 2010 09:04:01 -0000
@@ -106,3 +105,34 @@ function user_terms_update_6002() {
+  $sql = 'UPDATE {permission} SET perm="' . db_escape_string($perms) . '" WHERE (rid=' . (int)$rid . ')';
+  $result[] = update_sql($sql);

No need to use db_escape_string, since we're putting in a string we just took out of the database -- system_update_6039() doesn't.
Same with the $rid: no need to cast it.

+++ user_terms.install	21 Jul 2010 09:04:01 -0000
@@ -106,3 +105,34 @@ function user_terms_update_6002() {
+  $rids = db_query(

More conventional to always call this $result. Again, follow the system_update_6039() pattern more closely.

Powered by Dreditor.

I know you're still reviewing the coding of the patch, but I just wanted to chime in and say that the functionality appears to be sound. I appreciate how removing the "edit own" permission from users (thoughtful defaults are fine) makes it quite similar to a hidden Profile select option -- but that I can use the same taxonomy for nodes AND users.

Thanks!

I've rolled again this patch, merging changes from #976618: Add permissions for setting and viewing user terms: two new permissions for viewing own user terms and view any user terms taken.

As viewing was always allowed (as along as user can access user profiles) I've granted those new permissions to all user in the updating function.

Patch improvements:

• DRUPAL_AUTHENTICATED_RID is now used.
• Not join with {role}
• Fixed single line if
• db_escape_string not used for database retrieved input.
• $result renamed to $ret, and db_query result renamed to $result, as http://api.drupal.org/api/function/system_update_6039 provided example
• Removed empty check on permissions readed from roles with 'administer users' permissions (because if role has 'administer users' permission it for sure has at least one permission, so 'edit any user terms' must be appended with a comma

Permissions checks have been moved from user_terms_user hook to each delegated function (user_terms_view_profile and user_terms_form_profile) where empty terms check is made. I think this way code is cleaner.

Thanks for getting stuck in with this!

It's looking good, just a few hiccups:

+++ user_terms.install	21 Nov 2010 10:59:00 -0000
@@ -106,3 +106,37 @@ function user_terms_update_6002() {
+  // Frist step.

This should describe what's being done. Eg: 'Grant permissions to authenticated users that prior to this update the module implicitly gave them.'

Same for the second step.

+++ user_terms.install	21 Nov 2010 10:59:00 -0000
@@ -106,3 +106,37 @@ function user_terms_update_6002() {
+  $sql = 'UPDATE {permission} SET perm="' . $perms . '" WHERE (rid=' . DRUPAL_AUTHENTICATED_RID . ')';

We can simplify this even further -- system_update_6039 just uses "" on the whole query so variables can be put inside the string. Obv the constant has to stay outside (though IIRC there's a way to do that in PHP... {} possibly).

+++ user_terms.install	21 Nov 2010 10:59:00 -0000
@@ -106,3 +106,37 @@ function user_terms_update_6002() {
+  while ($row = db_fetch_object($rids)) {

Should be $result I think.

> Permissions checks have been moved from user_terms_user hook to each delegated function (user_terms_view_profile and user_terms_form_profile) where empty terms check is made. I think this way code is cleaner.

I'll have to ponder that one... I can see arguments for both. The earlier way saves us a function call.

Powered by Dreditor.

Rolled again.

• Added meaningful comments.
• Changed ' by " in queries to include inline varaibles.

$row I think is right becasue is a db_fetch_object result, so it has its own meaning (it's really a row data), not a resultset object (as db_query results are). For example module_list() function in module.inc:

        $result = db_query("SELECT name, filename, throttle FROM {system} WHERE type = 'module' AND status = 1 ORDER BY weight ASC, filename ASC");
      }
      while ($module = db_fetch_object($result)) {
        if (file_exists($module->filename)) {
   
 

I think is better to move permissions check code inside helper functions becasue code is cleaner to me. If check is done outside helper functions check code must be written at least twice inside user_terms_user functions, one time for each helper function that now makes this permissions check inside. So, with new view permissions, user_terms_user gets cluttered with permissions check code. Also, I think a single function call has a very little overhead, as long as it's not called inside a loop or gets called hundred of times in a process.

Anyway, I have moved opermissions check inside user_terms_form_profile to the begining to avoid useless procesing, and to avoid bypass permissions by using a user_terms_override_selector.

Patch file seems to change too much things, but this is only because cvs diff gets confused.

+++ user_terms.install	22 Nov 2010 10:01:34 -0000
@@ -106,3 +106,39 @@ function user_terms_update_6002() {
+
+  while ($row = db_fetch_object($rids)) {

I meant that $rids should be $result - $rids is not defined at this point and we're not doing anything with $result ;)
$row is indeed a fine variable name.

Powered by Dreditor.

Ooops! You are right about $result, you have a good eye... it sounded strange to me why use $result for the $row variable and now I get it.. :D

Attached a new patch with fixed variable name.

Could anybody test this last patch to have it RTBC?

I have a problem applying the patch. I was using User Terms module version 6.x - 1.1 before, replaced that one with the 6.x - 1.x-dev version, and then applied the patch using

# patch < 734666-user_terms.edit_permissions.8.patch
patching file user_terms.install
Hunk #1 succeeded at 105 (offset -1 lines).
patching file user_terms.module
Hunk #1 succeeded at 23 (offset -1 lines).
Hunk #2 succeeded at 136 (offset -1 lines).
Hunk #3 FAILED at 243.
1 out of 3 hunks FAILED -- saving rejects to file user_terms.module.rej

Am I using the right version for patching? I attached the user_terms.module.rej

I suppose it's due to changes made on dev after patch was rolled.

If this is the case patch must be rerolled, sorry. I may reroll it, but I've to be sure that patch it's going to be tested, don't want to work in vain... I guess we do need more testers for this feature request to get commited.

I'm snowed under I'm afraid -- up to this module's users to step up and test it.

No success for my part. Used patch from #19, applied to 6.x - 1.x-dev version. Blank page. Would love a way to set permissions on this module. Thanks for your effort.

This is still not implemented in dev or current version?

As the docs page on status values at http://drupal.org/node/156119 explains, the status of this issue will change to 'fixed' when that happens.

> up to this module's users to step up and test it.

This remains in effect -- if you want this feature, please contribute work towards it.

Ok but this than needs more work, not review as it's not working.

Well, it is working somewhat if you apply the patch in #19 to user_terms-6.x-1.3, which was the then-current, recommended version of user_terms when that patch was rolled. I just mistakenly updated to an unpatched 6.x-1.4 and had to roll back when I realized that we were running this custom patch on 6.x-1.3.

I haven't fully tested it, as we only needed a couple specific aspects of this patch to work. And I think we have run into a few minor issues. But the added functionality we need is generally working for us as expected, using the versions indicated.

One "minor issue" that I haven't attempted to track down in full detail (and which may not in fact be the fault of the patch) has to do with users editing their profile when they have not been given permission to edit their terms. This seems to cause us some problems, and I end up manually fixing it by editing the user's profile again through an admin account. For us this is not a dealbreaker, and the amount of time I might need to figure out the problem is much greater than the amount that I will spend manually fixing the errors as they arise.

Phil.

#19 didn't apply any longer so I re-rolled it then tried to simplify it a bit further. Sorry about the whitespace clean-up, it 'dirties' up the patch a bit... though I did make the two permission checks look the same and stopped the large wrapping IF block and the complications added to the other if block for view/edit forms.

Also, from my cursory testing this seems to do the trick, thanks all!

FYI, masquerade module is helpful for testing permissions.
https://drupal.org/project/masquerade