Displays are attached even when user does not have access

Fixed failing test, added extra test coverage and added access check to one additional place.

Okay, let's just fix the failing test...

... and expected test coverage only for the fixed code part.

@mxr576, thank you!

I've just tested the patch with a view of node with a Page display and a Feed display.

1. Page:
   • Path: /foo
   • Access: Role | Anonymous user
2. Feed:
   • Path: /bar
   • Attach to: Page
   • Access: Role | Administrator

Before:

Visited /foo as Anonymous user, I did SEE the feed icon, clicked the feed icon, lead me to /bar, and access denied got

After:

Applied the patch in #9, rebuilt the cache. visited /foo as Anonymous user, I DID NOT see the feed icon, visited /bar, and access denied got

Attached is the view exported for testing (uuid key removed manually, original file name: views.view.foo.yml)

+++ b/core/modules/views/tests/src/Unit/ViewExecutableTest.php
@@ -485,7 +497,13 @@
+    $cloned_view = $this->getMockBuilder('Drupal\views\ViewExecutable')
+      ->disableOriginalConstructor()
+      ->getMock();

The patch looks good to me, but I would promote mocking with Prophecy in PHPUnit https://www.drupal.org/docs/8/phpunit/using-prophecy

In all, RTBC+1

This looks really good already. Confirmed the bug, applied the patch and confirmed that this fixes the bug.

Since we are dealing with a borderline information disclosure issue here, I think it might be good to add functional test coverage for this as well, but lets see how the committers feel about adding additional coverage, or if they feel the unit tests are enough.

Thanks for the quick reviews!

Since we are dealing with a borderline information disclosure issue here, I think it might be good to add functional test coverage for this as well, but lets see how the committers feel about adding additional coverage, or if they feel the unit tests are enough.

Unless there is an existing functional test in core that could be easily altered to add this extra test coverage I would go with only the unit test. If a unit test was/is enough checking the "enabled" state of a display (is there a functional test for that?) which could also modify the result, a unit test should be also enough to verify the access.

The latest patch failed tests with:

Views.Drupal\Tests\views\Unit\ViewExecutableTest
✗	
Drupal\Tests\views\Unit\ViewExecutableTest
fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-765.xml:
PHPunit Test failed to complete; Error: PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\views\Unit\ViewExecutableTest
.................F...........                                     29 / 29 (100%)

Time: 471 ms, Memory: 12.00MB

There was 1 failure:

1) Drupal\Tests\views\Unit\ViewExecutableTest::testAttachDisplays with data set "enabled-revoked" (true, false, false)
Drupal\views\Plugin\views\display\DisplayPluginBase::attachTo(Mock_ViewExecutable_a97b86ea Object (...), 'default', Array (...)) was not expected to be called.

/var/www/html/core/modules/views/src/ViewExecutable.php:1728
/var/www/html/core/modules/views/tests/src/Unit/ViewExecutableTest.php:510

FAILURES!
Tests: 29, Assertions: 52, Failures: 1.

Also, as a minor access bypass, we probably should have filed it in the private tracker to start. Sometimes the sec team will approve public issues for low-risk access bypasses and information disclosures. I'll ask about this one.

@xjm, thanks for the review and the confirmation from the security team. (I also rolled my head around when I saw a possible security issue is in the public queue.) Are you sure the patch failed? I cannot reproduce this in my local, the "test-only.patch" failes of course.
Started new tests on the CI for #9 https://www.drupal.org/pift-ci-job/1675012 https://www.drupal.org/pift-ci-job/1675013

I was guessing @xjm tested the wrong/TEST-ONLY patch too. Let's set back to RTBC.

I was indeed looking at the test-only patch; was distracted by the fact that we had an access bypass issue in the public queue without Security Team approval and was focused on getting that. ;) Sorry!

A couple of thoughts:

• \Drupal\views\Plugin\views\display\Attachment::attachTo() handles access already. So can we remove that? Or should we be doing this in \Drupal\views\Plugin\views\display\Feed::attachTo() as well and not change \Drupal\views\ViewExecutable::attachDisplays()?
• What about caching - we happens if we cache a view with an attachment and another user views the same view - are we setting cache context's appropriately?

@alexpott, those are great deductions, I'll give you my 2¢ and try and push this issue forward.

1. Feed::attachTo() should probably not get this access check, it's an RSS feed, it's whole goal is to publicize (I don't think it would hurt but also not necessary, IMO), AND I'd not remove it from Attachment::attachTo() because that is our public API and who knows who's using that directly in contrib.
2. That's a big question, maybe too big for this issue, I'd say. For the Views displays in general, "are Views display permissions applying the appropriate caching context". The current Attachment::attachTo() access() check is before applyDisplayCacheabilityMetadata() already, so already looks to be short-cutting output in attachments.

I apologize if I misread/misinterpreted your point in #23

Hope all is well!👋

So... all of views' access API returns boolean instead of AccessResult. Until they return AccessResult, we can't add cacheability metadata. This would need to go up through ViewExecutable::access() and whatever calls it too.

Permission access (and to an extent role access) will be handled by the global permissions cache context which is applied to everything, but anything more esoteric must currently be buggy afaict.

Opened #3188952: Views access system should use AccessResult.

Committed and pushed 8fafcd342e to 9.2.x and c2bfd00955 to 9.1.x. Thanks!

@catch thanks for filing the follow-up for the bigger issue. I think that this issue represents a step in the correct direction so committing and we can improve on this in future issues.