Follow-up to #2506345: Unpublished Nodes accessible

Problem/Motivation

When trying to save grants on a new, unpublished node, the checkboxes won't save.

Proposed resolution

Save grants on unpublished nodes too.

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#6 unpublished_nodes_grants_checkboxes_wont_save-2541588-6.patch538 bytesnick dewitte
#1 unpublished_nodes-2541588-1-tests-only.patch2.32 KBjoelpittet

Comments

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Active » Needs review
StatusFileSize
new unpublished_nodes-2541588-1-tests-only.patch2.32 KB

Here's some tests, they should fail:) I don't have a solution to the problem yet but this should help ensure it works going forward.

First time writing tests before I know the answer:)

Status: Needs review » Needs work

The last submitted patch, 1: unpublished_nodes-2541588-1-tests-only.patch, failed testing.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Needs review

Those two fails are expected;)

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Needs work

Actually I should leave this as needs-work because the fix needs to be added to these tests.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Priority: Normal » Major

Bump, there are tests to show this is broken, bumping priority.

nick dewitte’s picture

nick dewitte commented
Status: Needs work » Needs review
Issue tags: +Needs committer feedback
StatusFileSize
new unpublished_nodes_grants_checkboxes_wont_save-2541588-6.patch538 bytes

Attached is a patch that resolves this specific issue in a very naive way.

When this module saves grants, this is done via nodeaccess_set_grants().
This function calls node_access_acquire_grants(), which calls all implementations of hook_node_access_records().
The nodeaccess_node_access_records() implementation (which is the file I patched) checked if the node was unpublished,
and if so, it returned no grants (NULL).

After reading https://api.drupal.org/api/drupal/modules%21node%21node.module/group/nod...
it appears to me that the access based on the published state should be managed in hook_node_access().

Once grants are being checked, I don't think it's appropriate to flat out deny access to all unpublished nodes without any condition.
But perhaps ignoring the published state as this patch does isn't the way to go either?

Adding the "Needs committer feedback" tag since I think this is dependant on how the module should behave.

Status: Needs review » Needs work

The last submitted patch, 6: unpublished_nodes_grants_checkboxes_wont_save-2541588-6.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

I wrote tests in #1 see if they pass with that fix.

alison’s picture

alison
she/her
commented

Thank you @nick.dewitte and @joelpittet!

So, we need a version of the patch that has the fix and the tests, ya?

More broadly, though:
@nick.dewitte Are you saying that the patch you provided,
(1) allows the grants settings form to be saved (yay), and
(2) means that the grants settings from nodeaccess module will not have any impact on whether a user an see the node if it is unpublished, that determination is left up to whatever unpublished content perms exist on the site?

(Is that an accurate explanation, or?)

I think I'm fine with that concept. I'm ok with hearing other opinions, too, but it seeeeems logical to me that, unless additional nodeaccess-based permissions are added to account for viewing unpublished content (i.e. "view" + "edit" + "delete" + a new "view unpublished" perm), nodeaccess should not impact access to unpublished content. And, that feeeeeels like a safer setup -- I don't think people would want users with "view" access to automatically be able to view unpublished content -- right?

d.fisher’s picture

d.fisher commented
Status: Needs work » Postponed (maintainer needs more info)

Drupal 7 security support has ended as of 5 January 2025.

We are doing some housekeeping on the nodeaccess issue queue and moving all Drupal 7 issues to "postponed (maintainer needs more info)". See https://www.drupal.org/project/nodeaccess/issues/3516593.

If this issue persists on the latest dev branch of nodeaccess (2.0.x-dev) then please feel free to comment and we will change the version against this issue. If we do not hear any feedback within 2 weeks (9th October 2025) we will go ahead and close this issue as outdated.

Thank you.

d.fisher’s picture

d.fisher commented
Status: Postponed (maintainer needs more info) » Closed (outdated)

As per the above, as there has been no response we will now close this issue as outdated.

If this issue persists on the latest dev branch of nodeaccess (2.0.x-dev) then please feel free to update the version on this issue and reopen it.

Thank you.

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.