One of the items that came out of the Node Access sprint in Szeged was a consensus that the 'administer nodes' permission is far too broad. We envision a pluggable node access system that requires some elements of 'administer nodes' to be retained, while others a moved to the new 'bypass node access' permission.

This permission allows a role to View, Edit, or Delete all nodes at all times. This ability is taken away from users with 'Administer Nodes' who are now able to access and edit all information about a given node -- publication date, revisions, and so forth -- but must still follow node access rules. Doing so will allow for the sort of fine-grained editorial workflows that are common for node access modules like OG.

The attached patch introduces the new permission and includes a testing stub.

CommentFileSizeAuthor
#21 bypass_node_access.patch4.17 KBagentrickard
#12 bypass.patch5.66 KBagentrickard
#6 mw.patch4.58 KBmoshe weitzman
bypass_node_access.patch3.26 KBagentrickard
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented
Title: Split 'bypass node access' from 'administer nodes » Split 'bypass node access' from 'administer nodes'
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

I really like this, a couple of questions though:

You've not changed the permissions on admin/content/node - how do you see that working with this? There should also be an upgrade path to ensure users with administer nodes get both permissions.

The various node operations should be split into smaller permissions, but that's a follow-up patch and this one gets us a step closer to that.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented

Good point. That may just be an oversight, but I am pretty sure it is a deliberate omission from this patch. The problem with admin/content/node right now is that it does not obey node access rules (the old db_rewrite_sql). I think the proper solution is a new permission for that page, 'batch content editing' or similar.

Thoughts?

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented

@catch

See #305991: Administer nodes: split 'batch content editing' to new permission for the separate patch

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

Looks like a great patch to me. Code is good. If the unit tests pass, is RTBC.

Not really relevant to this patch, but that node.test section looks wrong to me. There is no assertion after the node_delete() call so we are not caring if the call fails or not. On top of that, node_delete is an API function and should not even have any access control check. We already check for access in node_menu('node/%node/delete'). We are better off doing a drupalGet("node/$nid/delete"). That does the needed checks.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
FileSize
mw.patch4.58 KB

I code and reviewed and tested this patch and it is RTBC. I changed the description and white space in the node.test because it didn't make much sense (that code predates Ken's patch). So here is a new patch with no new logic in it.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Needs review » Needs work

I forgot to mention that we should probably add an update function so we give this perm to all roles that have administer nodes. thats pretty easy now with the new role_permissions table.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented
Status: Needs work » Needs review

I think catch is right, we may also need an update function to give this permission to D6 users with the 'administer nodes' permission. Will try a re-roll later this week (unless it is too early in the patch cycle for that).

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented

We cross posted.

Let's worry about the update function later, then. After this gets in.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Needs review » Needs work

Er? Since when do we let patches in without an upgrade path?

We tried that with the menu system during the D6 cycle and it involved a weeks-long clean-up. Not eager to repeat that again. :P

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented

Not a problem.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented
Status: Needs work » Needs review
FileSize
bypass.patch5.66 KB

New patch, with DBTNG update function. Tests pass.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

Lovely use of db_insert() in update.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented

Crell wrote that after I handed him the original (D6) code. :-) So he gets partial patch credit!

Crell’s picture

Crell CreditAttribution: Crell commented

Subscribe. :-)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

I'm quite sure the permissions revamp thing I committed earlier broke this. Sorry. :\ Should be a pretty quick re-roll.

This looks like a nice change, and I'd like to get it into UNSTABLE-2. Can someone confirm they tested the upgrade path?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work
Crell’s picture

Crell CreditAttribution: Crell commented

Um, link to the permission revamp thing so we know what happened? :-)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

You mean you guys aren't following every last character on the core commit list with keen interest? ;)

#313213: Fix broken localization for permission names

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

#313213: Fix broken localization for permission names

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented
Status: Needs work » Needs review
FileSize
bypass_node_access.patch4.17 KB

Revised patch. I believe the test case is ok, but a testing ninja should take a look.

Automated testing passes -- http://testing.drupal.org/node/15172

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

Ken's change looks good, the node.test looks proper, and runs without failures. RTBC.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work

Awesome. :) Thanks!!

Please mention this in the upgrade docs.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented

The 'Create content' menu item no longer appears in the menu and as user 1 I have no longer access to /node/add.

//edit: Never mind, I didn't know the Content Types page was moved to Site building :P

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented

I put a comment here, but am not on the docs team:

http://drupal.org/node/224333#comment-1055014

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

Consider that another bug fixed. ;) You are now! ;)

Crell’s picture

Crell CreditAttribution: Crell commented

Silly agentrickard. Never tell someone on the docs team that you aren't on it. :-)

gpk’s picture

gpk CreditAttribution: gpk commented
Status: Needs work » Fixed

@26/7: LOL

I turned the comment into an entry: http://drupal.org/node/224333#bypass_node_access. Worth a quick look by s.o. to make sure I understood it correctly...

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented

Yes, you did. Thanks all!

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Fixed » Closed (fixed)

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

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy commented

Urgh... any chance of a backport to D6? This would be great for allowing users to create new revisions on access-controlled nodes without having access to any node on the site...