The 3 permissions 'hide display options', 'hide basic options', 'hide advanced options' are phrased and applied the wrong way - permissions in Drupal should always add privileges, not take them away. Apart from the significant user experience issues with the current setup this causes problem with sites using adminrole, which automatically grants all permissions to a certain role.

The attached patch renames these to 'use display options', 'use basic options', 'use advanced options', updates the logic to match, and also adds an update script to update permissions. I have tested and it all works for me :)

CommentFileSizeAuthor
fixperms.patch3.31 KBowen barton

Comments

Stuart Greenfield’s picture

Stuart Greenfield commented

You're right that the permissions are "back to front" but at the time it was a bit of a quick fix so that users automatically DID see the flash node options unless the administrator turned it off. I should probably have gone back and fixed it before now, but never did.

I read the patch quickly and it looks like it would work, but the update needs to be more comprehensive so as not to break existing sites. We would need to check and update permissions for users so that after the update they have the same permissions as they had before (if they are not currently set to "hide" a permission then we need to enable it so that they do have "view" permission!)

So the patch is the right thing to do, and I should have fixed it before flash node got popular :-) I'll use your patch as a start and see if I can complement it with an update!

owen barton’s picture

owen barton commented

Oh yeah - that's a good point - the update is back to front, we need to invert the permissions there too (not just rename!).

Not having a good permissions API makes this update annoying. The following 2 queries should do it (a little hacky, but probably less work than loading it all into arrays and back):

// Add the new perm for any roles that don't have the old perm
$sql = "UPDATE {permission} SET perm = CONCAT(perm, ', use display options') WHERE perm NOT LIKE '%hide display options%'";
// Remove the old perm throughout
$sql = "UPDATE {permission} SET perm = REPLACE(perm, 'hide display options, ', '');";
Stuart Greenfield’s picture

Stuart Greenfield commented

I thought about this some more on the train this morning! Maybe the best approach is to remove the 'hide display options' privilege where it exists, which puts all users unable to see anything.

Then admins manually grant privileges, on the assumption most sites won't have many roles.

If we grant the ''use display options" to all users not currently assigned the "hide" privilege then probably everyone gets all the privileges whether they need it or not, and that also seems a bit messy.

By resetting everyone to the same level we don't give the privilege to anyone who shouldn't have it, and it at least means end users will have to think about what privileges they are granting?

So the second SQL update is the only one we'd need?

What do you think?

I am almost ready to release a new flash node with the views support integrated, but I'll hold off on that as this update is almost ready to go and it means it's fixed once and for all!

owen barton’s picture

owen barton commented

That sounds like a reasonable solution to me. If this is the way we go, then we actually don't need any update at all - the permission will be completely ignored by Drupal, and will be removed as soon as the user saves the admin/user/permissions page. Generally modules (even core) don't remove permission configurations on uninstall, so this is an acceptable approach I think.

Stuart Greenfield’s picture

Stuart Greenfield commented

the permission will be completely ignored by Drupal, and will be removed as soon as the user saves the admin/user/permissions page

You have answered something I thought of on the train on the way home, and saved me having to look it up, or work it out for myself!

Stuart Greenfield’s picture

Stuart Greenfield commented
Status: Needs review » Fixed

Committed and included in latest releases.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.