There are actually two interleaved issues here.

The current node_access_grants() code still assumes procedural style, taking $account = NULL as an argument. If NULL, the $account is assumed to be the (deprecated) global user.

Methods that call node_access_grants() pass an AccountInterface $account, which, it seems, is an instance of Drupal\Core\Session\UserSession, which is not a fully-loaded $account entity, which is what an API consumer would expect, since hook_node_grants() has always passed a user entity. We need to ensure that behavior is consistent.

So things that need deciding / fixing:

  • Should the caller always pass an $account?
  • What format should that $account be in?
  • When should $account be transformed into a user entity?
CommentFileSizeAuthor
#15 interdiff.txt897 bytestim.plunkett
#15 node-2147291-15.patch6.53 KBtim.plunkett
#13 node-2147291-13.patch5.66 KBtim.plunkett
#10 interdiff.txt1.07 KBmartin107
#10 node_access_grant-2147291-8.patch1.79 KBmartin107
#7 node_access_grant-2147291-7.patch1.49 KBtkuldeep17
#1 2147291-nodeAccessGrants.patch1.21 KBagentrickard

Comments

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Active » Needs review
StatusFileSize
new 2147291-nodeAccessGrants.patch1.21 KB

And a patch to get things started.

Status: Needs review » Needs work

The last submitted patch, 1: 2147291-nodeAccessGrants.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented

I doubt that we should ever load the user. The code uses the AccountInterface and not the UserInterface, so if anything relies on the user, it should load the user for themself.

agentrickard’s picture

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

No. That's a bad assumption. Access checks like this may depend on account information that is only accessible via a UserInterface object.

Why this ever loaded an AccountInterface is the real question.

The API expects to pass a $user object. Always has. $account has been a stand-in to remind people not to assume the global $user object when making an access check.

At the very least, node_access_grants should supply a proper $user object to the hook implementation.

agentrickard’s picture

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

Oh, and the facepalm of the year. Look at the parent access method, which calls EntityAccessController::prepareUser().

That method proves that we are meant to have a full user object. It's just being loaded improperly from the deprecated GLOBAL $user.

tkuldeep17’s picture

tkuldeep17 commented

Hi agentrickard
I am agree with you, that we have to pass AccountInterface object to node_access_grants,
But Access check mechanism requires only uid, which is provided by AccountInterface object.
So we don't need to explicitly load user through entity_load method.

And also we should use \Drupal::moduleHandler()->alter('node_grants', $grants, $account, $op); instead of drupal_alter('node_grants', $grants, $account, $op);

tkuldeep17’s picture

tkuldeep17 commented
StatusFileSize
new node_access_grant-2147291-7.patch1.49 KB

Submitting my patch..

tkuldeep17’s picture

tkuldeep17 commented
Status: Needs work » Needs review

changes status...

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Needs review » Needs work 
+ *  The user object for the user performing the operation.

Would then be:

+ *  The account object for the user performing the operation.

There are places in node.api.php that need updating as well.

martin107’s picture

martin107 commented
StatusFileSize
new node_access_grant-2147291-8.patch1.79 KB
new interdiff.txt1.07 KB

Comment text changed as suggested in the previous comment.

NB this patch needed a reroll because another patch has corrected the following code.

- if (!isset($account)) {
- $account = $GLOBALS['user'];
- }

This issue is still relevant because of the 'alter' line of code and the other patch was not careful enough to update the function parameters of the funciton, or update the comments.

finally ( minor house keeping ) I have removed unused use statement from top of the code.

martin107’s picture

martin107 commented
Status: Needs work » Needs review
dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/node/node.module
@@ -1632,15 +1632,15 @@ function node_permissions_get_configured_types() {
+ * ¶
...
+ *  The account object for the user performing the operation.

Please move the whitespace from 1636 to the beginning of 1638 :p

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Title: node_access_grants call GLOBAL $user » node_access_grants should always be passed an account
StatusFileSize
new node-2147291-13.patch5.66 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 13: node-2147291-13.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review
StatusFileSize
new node-2147291-15.patch6.53 KB
new interdiff.txt897 bytes

Hah! node_test_node_grants() does not use either parameter, which let NodeAccessRecordsTest call it directly with the params in the wrong order. Now that we're typehinting, it broke.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Needs review » Reviewed & tested by the community

Looks good, tests green.

  • Commit f0677c1 on 8.x by webchick: 
    Issue #2147291 by tim.plunkett, martin107, tkuldeep17, agentrickard:...
webchick’s picture

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

Wow, nice catches!

+++ b/core/modules/node/node.module
@@ -1433,22 +1433,16 @@ function node_permissions_get_configured_types() {
+function node_access_grants($op, AccountInterface $account) {

Side note: It's funny that everywhere else we use $account, $op except here. Can see why the tests got confused.

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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