Problem/Motivation

The method ModerationInformation::getLatestRevisionId (and possibly ModerationInformation::getDefaultRevisionId) has varying results if modules with node access grants are enabled. This particularly reveals itself if the content in question has not yet been published. For non-admins that don't have access granted in the query alter (but may otherwise be granted access) this method returns NULL.

Proposed resolution

Since both of these are API methods, they shouldn't return different results depending on which user is logged in. As such, the accessCheck(FALSE) method should be called on the entity query.

Remaining tasks

Determine the best way to demonstrate this issue in a test.

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#6 2932154-06.patch7.66 KBjhedstrom
#6 2932154-06-TEST-ONLY.patch6.36 KBjhedstrom
#6 interdiff-2932154-03-06.txt1.75 KBjhedstrom
#3 2932154-03.patch8.26 KBjhedstrom
#3 2932154-03-TEST-ONLY.patch6.96 KBjhedstrom
#3 interdiff-2932154-02-03.txt6.96 KBjhedstrom
#2 2932154-02.patch1.3 KBjhedstrom
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhedstrom created an issue. See original summary.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
Status: Active » Needs review
FileSize
2932154-02.patch1.3 KB
jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
FileSize
interdiff-2932154-02-03.txt6.96 KB
2932154-03-TEST-ONLY.patch6.96 KB
2932154-03.patch8.26 KB

Here's 2 tests that demonstrate the issue. The functional one is a change to an existing test, but instead of using the node_access_test_empty module, it uses the one that actually provides some node grants. The second is a kernel test that demonstrates the issue at the API level.

Status: Needs review » Needs work

The last submitted patch, 3: 2932154-03.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented

@jhedstrom - Good find! The patch in #2 looks perfect. Once tests pass I'll give a proper review.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
Status: Needs work » Needs review
FileSize
interdiff-2932154-03-06.txt1.75 KB
2932154-06-TEST-ONLY.patch6.36 KB
2932154-06.patch7.66 KB

Those 2 fails were caused by the new logic in node_access_test_node_access() inadvertently granting the anonymous user access. Resolved by using a === operator. This also removes what was an unnessesary change to node_access_test_node_grants.

The last submitted patch, 6: 2932154-06-TEST-ONLY.patch, failed testing. View results

The last submitted patch, 6: 2932154-06-TEST-ONLY.patch, failed testing. View results

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community

Nice, looks good to me, thanks @jhedstrom!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Related issues: +#2931028: Separate entity query service into access checked and not access checked

Plug for #2931028: Separate entity query service into access checked and not access checked :)

  • larowlan committed f3b60ed on 8.5.x 
    Issue #2932154 by jhedstrom: ModerationInformation::getLatestRevisionId...

  • larowlan committed e5f7cc8 on 8.4.x 
    Issue #2932154 by jhedstrom: ModerationInformation::getLatestRevisionId...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan as a volunteer and at PreviousNext commented
Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed as f3b60ed and pushed to 8.5.x

Cherry-picked as e5f7cc8 and pushed to 8.4.x

Thanks

Status: Fixed » Closed (fixed)

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