Make "update access implies view access" optional [#2292451]

This module controls access to referenced data by checking access of the entity being referenced.

With #1909436: Access denied: Entity Reference should also allow on node edit permission, this module grants 'view' access on the basis of 'update' access to the referenced entity.

        // Check whether the user has access to the referenced entity.
        $has_view_access = (entity_access('view', $field['settings']['target_type'], $target_entities[$item['target_id']]) !== FALSE);
        $has_update_access = (entity_access('update', $field['settings']['target_type'], $target_entities[$item['target_id']]) !== FALSE);
        $items[$id][$delta]['access'] = ($has_view_access || $has_update_access);

There are a group of people who prefer this behaviour - see the comments on the related issue.

However this behaviour is counter-intuitive and it creates problems for other people.

As per this comment, a site may have removed 'view' permissions so that editors can see the site as a normal user would.
Check both permissions hurts performance. On my site, it crippled performance because 'update' access was controlled by a custom hook and much more expensive to calculate. This could be mitigated to a degree by only checking 'update' permission if 'view' permission failed.
Withholding 'view' permission should really mean exactly that. This is a borderline security issue, but I didn't raise it as one because it is pretty obscure. Suppose that on my site ordinary editor EE can edit content type CC. I use module field_permissions so that EE can't edit field XX (and hence can't see the existing value when editing) as XX contains data that needs to remain confidential whilst a node is unpublished. I carefully check that EE has no permission to view unpublished content of type CC. However my view VV will show field XX by means of an entity reference as soon as the entity is published. EE can now see the confidential field XX by means of view VV.

This behaviour needs to be a configurable option.

We can debate the details. I tend to feel that it should be off for new sites, but of course if so, it needs to be on as a migration step.

Comment	File	Size	Author
#29	entityreference.access.2292451-29.patch	5.31 KB	pokurek
#29	7.x-1.x: PHP 5.6 & MySQL 5.5, D7 10 pass
#26	entityreference.access.2292451-24.patch	5.29 KB	pokurek
#26	7.x-1.x: PHP 5.6 & MySQL 5.5, D7 Patch Failed to Apply
#24	entityreference.access.2292451-24.patch	5.54 KB	pokurek
#24	7.x-1.x: PHP 5.6 & MySQL 5.5, D7 Patch Failed to Apply PHP 7 & MySQL 5.5, D7 Patch Failed to Apply PHP 7.2 & MySQL 5.5, D7 Patch Failed to Apply
#16	entityreference.access.2292451-16.patch	5.32 KB	AdamPS
#16	7.x-1.x: PHP 5.3 & MySQL 5.5, D7 9 pass
#13	entityreference.access.2292451-13.patch	5.31 KB	AdamPS
#13	7.x-1.x: PHP 5.3 & MySQL 5.5, D7 9 pass
#1	entityreference.access.2292451-1.patch	902 bytes	AdamPS
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

AdamPS CreditAttribution: AdamPS commented 22 March 2015 at 10:15

File	Size
entityreference.access.2292451-1.patch	902 bytes

I came across the same situation as a performance problem. This is creating an extra call to hook_node_access 'update' for every row in a view, which generates unnecessary SQL queries and slows the site down. In my case, hook_node_access 'view' does nothing, so the difference is significant.

The documentation for hook_node_access does not make any mention that when checking to display content the 'update' permission should also be checked, and I've not come across that concept in other modules.

The situation where someone has deliberately set 'update' access but not 'view' isn't easy to imagine. Such a user can presumably see the existing values on the edit page so these permissions don't achieve much. So a developer may have decided that it's harmless to add the extra check. However the performance impact isn't harmless.

Possibly this code came about as a workaround for some module that sets 'update' access but accidentally forgets to set 'view' access?? If so, I would think it is better to fix that code. If anyone knows of such code, please add a comment.

The attached patch removes the unnecessary check.

Comment #2

AdamPS CreditAttribution: AdamPS commented 22 March 2015 at 10:15

Status:

Active

» Needs review

Comment #3

AdamPS CreditAttribution: AdamPS commented 13 July 2015 at 13:52

Parent issue:

» #2532152: Plan for EntityReference 7.x-1.2 release

Request that this one is in the next release please.

Comment #4

joachim CreditAttribution: joachim commented 10 May 2016 at 13:02

> This design decision seems weird! ;)

I agree! But maybe there's a reason we're not aware of.

Have you tried using git blame to find the issue that introduced this access check? Maybe that would give us some background on this.

Comment #5

MustangGB CreditAttribution: MustangGB commented 10 May 2016 at 14:08

This is the issue you're trying to rollback: #1909436: Access denied: Entity Reference should also allow on node edit permission

Comment #6

geek-merlin

they/omg

German

Freiburg, Germany

CreditAttribution: geek-merlin commented 10 May 2016 at 22:36

Title:	field formatter checks access to referenced entities with 'update' op	» Make "update access implies view access" optional
Status:	Needs review	» Needs work
Related issues:		+#1909436: Access denied: Entity Reference should also allow on node edit permission

Yes this was intentionally done in #1909436: Access denied: Entity Reference should also allow on node edit permission. I totally second Dave Reid who said in #1909436-17: Access denied: Entity Reference should also allow on node edit permission:

I really wish this was an optional behavior. I don't like this change since when I view a node that might refer to unpublished, but future-scheduled nodes in an entity reference field, this means I can see them now and doesn't match the expectation of what a normal user would see.

So as some gals need that and others don't this should be the way to go. Updating title accordingly.

Comment #7

AdamPS CreditAttribution: AdamPS commented 11 May 2016 at 09:09

Issue summary:

View changes

Comment #8

AdamPS CreditAttribution: AdamPS commented 11 May 2016 at 09:12

Thanks everyone - I should have thought of checking git blame. I have updated the issue summary.

I would welcome any opinions on the last para - what should the default behaviour be for "update access implies view access"?

A) Default to ON. This is simplest.
B) Default to OFF. Turn on as a migration step for existing sites.
C) As B, but also as a migration step turn on a warning inviting people to turn the setting off if they don't need it. Warning goes away when settings are saved for first time.

Comment #9

joachim CreditAttribution: joachim commented 11 May 2016 at 10:14

> Default to ON

This is good for existing sites, but not ideal for new sites, as it's not the default we want to encourage.

> Default to OFF

This is good for new installations, as it helps performance.

However, it means it breaks existing sites. At least it breaks them in the direction of hiding things that should be visible, rather than exposing things that should be hidden. But it's still a pain to sites that rely on this.

I would suggest default to OFF, and a hook_update_N() which automatically sets existing fields to ON to preserve the existing behaviour (and has an explanation of why it's doing that).

Comment #10

MustangGB CreditAttribution: MustangGB commented 11 May 2016 at 10:20

I'd say B is enough (default off + hook_update_N() + documentation), no need to go the extra step of forcing/encouraging people to switch.

Comment #11

AdamPS CreditAttribution: AdamPS commented 11 May 2016 at 11:01

Assigned:

Unassigned

» AdamPS

OK, thanks, that's two votes for B, so I'll make a new patch based on that option.

Comment #12

geek-merlin

they/omg

German

Freiburg, Germany

CreditAttribution: geek-merlin commented 11 May 2016 at 19:14

Thanks a lot Adam PS!

Comment #13

AdamPS CreditAttribution: AdamPS at AlbanyWeb commented 13 May 2016 at 20:57

Status:

Needs work

» Needs review

File	Size
entityreference.access.2292451-13.patch	5.31 KB
7.x-1.x: PHP 5.3 & MySQL 5.5, D7 9 pass

1 file was hidden/shown/deleted

File	Size
entityreference.access.2292451-1.patch	902 bytes

OK, ready for review please.

Comment #14

geek-merlin

they/omg

German

Freiburg, Germany

CreditAttribution: geek-merlin commented 16 May 2016 at 18:28

The code looks straightforward and solid to me. Did not test though.
(I wish that more code be so well-commented and readable ;-)!

Comment #15

scotwith1t

Birmingham, AL

CreditAttribution: scotwith1t commented 16 August 2016 at 17:05

While well-formatted and well-commented, "entityreference" is spelled "entityreferece" in many places...can't imagine that's intended...

Comment #16

AdamPS CreditAttribution: AdamPS at AlbanyWeb commented 16 August 2016 at 17:28

File	Size
entityreference.access.2292451-16.patch	5.32 KB
7.x-1.x: PHP 5.3 & MySQL 5.5, D7 9 pass

Thanks - done a search and replace of many entityreferece with entityreference

Comment #17

16 August 2016 at 17:31

Status:

Needs review

» Needs work

The last submitted patch, 16: entityreference.access.2292451-16.patch, failed testing.

Comment #18

AdamPS CreditAttribution: AdamPS at AlbanyWeb commented 19 August 2016 at 16:00

Status:

Needs work

» Needs review

Baffled as to why the trivial search and replace has caused the tests to fail. Advice welcome! Setting back to needs review to give the tests another try in case it was a temporary glitch.

Comment #19

19 August 2016 at 16:03

Status:

Needs review

» Needs work

The last submitted patch, 16: entityreference.access.2292451-16.patch, failed testing.

Comment #20

geek-merlin

they/omg

German

Freiburg, Germany

CreditAttribution: geek-merlin commented 20 August 2016 at 19:34

@AsanPS:
note that the branch tests are very old and the failing tests migh come from the module itself, not your patch. (note that the number of tests changed, and the last branch test is from 2015)
i did not find how to re-trigger the branch test as non-maintainer, but in any case, you can upload an empty (trivial, comment-only) patch here to check the branch test integrity.

if that does not uncover the cause, please make and upload an interdiff.

Comment #21

MustangGB CreditAttribution: MustangGB commented 7 September 2016 at 11:58

Looks like branch tests are passing as of August 24th (https://www.drupal.org/pift-ci-job/431579), so I tried queuing a re-test.

Comment #22

MustangGB CreditAttribution: MustangGB commented 7 September 2016 at 11:59

Status:

Needs work

» Needs review

Looks like it passes fine now.

Comment #23

Chris Matthews CreditAttribution: Chris Matthews as a volunteer commented 30 January 2019 at 00:59

Status:	Needs review	» Needs work
Issue tags:		+Needs reroll

The 2 year old patch in #16 does not apply to the latest 7.x-1.x-dev and if still relevant needs a reroll.

Comment #24

pokurek CreditAttribution: pokurek commented 19 March 2019 at 21:33

File	Size
entityreference.access.2292451-24.patch	5.54 KB
7.x-1.x: PHP 5.6 & MySQL 5.5, D7 Patch Failed to Apply PHP 7 & MySQL 5.5, D7 Patch Failed to Apply PHP 7.2 & MySQL 5.5, D7 Patch Failed to Apply

Hi, I tried to reroll patch #16 to the latest 7.x-1.x-dev.

Comment #25

MustangGB CreditAttribution: MustangGB commented 19 March 2019 at 21:43

Status:

Needs work

» Needs review

Lets trigger some tests then.

Comment #26

pokurek CreditAttribution: pokurek commented 19 March 2019 at 21:45

File	Size
entityreference.access.2292451-24.patch	5.29 KB
7.x-1.x: PHP 5.6 & MySQL 5.5, D7 Patch Failed to Apply

1 file was hidden/shown/deleted

File	Size
entityreference.access.2292451-24.patch	5.54 KB
7.x-1.x: PHP 5.6 & MySQL 5.5, D7 Patch Failed to Apply PHP 7 & MySQL 5.5, D7 Patch Failed to Apply PHP 7.2 & MySQL 5.5, D7 Patch Failed to Apply