Changing view access from "Permission" to "Role" causes AJAX error message re getRoles()

Here is a fix for that.

#1: drupal-1912602-1.patch queued for re-testing.

i've manually tested and verified that the ajax error is removed using patch in #1. not sure why it failed, so here's a re-roll against head.

Added a test.

+++ b/core/modules/user/lib/Drupal/user/Tests/Views/AccessRoleUITest.phpundefined
@@ -0,0 +1,70 @@
+    $result = $entity_manager->getStorageController('view')->load(array('test_access_role'));
+    $view = reset($result);
+
+    $display = $view->getDisplay('default');
+    debug($display);
+    debug($display['display_options']['access_options']);

This seems to be missing an assert

Oh you and I seemed to have uploaded the wrong patch.

setUp() already call to enableViewsTestModule() in the parent function. This reason test data is being created twice.

Hi dagmar!

Let's fix it.

I kind of consider this more like a critical as the roles access plugin don't work with that.

Tested this patch #12 but not working completely. I am not getting any ajax error but after I save the view on selecting role access it gives following error

InvalidArgumentException: Routing requirement for "_role_id" must be a string. in Symfony\Component\Routing\Route->sanitizeRequirement() (line 570 of /usr/share/nginx/www/trial-d8/core/vendor/symfony/routing/Symfony/Component/Routing/Route.php).

12: vdc-1912602-12.patch queued for re-testing.

Good point, let's actually fix all of it and write tests.

Wow, this shows a lot of problems all over the place.

18: vdc-1912602.patch queued for re-testing.

Some of the ascci arts of http://www.ascii-art.de/ascii/def/finger.txt might apply here.

In simpletest we use a ContainerBuilder ... sadly a Container needs to be dumped
to have a working synchronized service: https://github.com/symfony/symfony/issues/9595

Additional I realized some bug in the access manager, which adds multiple access checkers
and calls applies() more than needed.

22: vdc-1912602.patch queued for re-testing.

1. +++ b/core/modules/user/lib/Drupal/user/Plugin/views/access/Role.php
   @@ -74,7 +77,7 @@ public function buildOptionsForm(&$form, &$form_state) {
   +      '#options' => array_map(array('\Drupal\Component\Utility\String', 'checkPlain'), user_role_names()),
   

   I prefer the '\Drupal\Component\Utility\String::checkPlain' syntax, but that's up to you.

2. +++ b/core/modules/user/lib/Drupal/user/Tests/Views/AccessRoleTest.php
   @@ -35,19 +38,43 @@ public static function getInfo() {
   +    /** @var \Drupal\views\ViewStorageInterface $view */
   ...
   +    /** @var \Symfony\Component\HttpKernel\HttpKernelInterface $kernel */
   

   This inline commenting is weird. Is this a new thing?

3. +++ b/core/modules/user/lib/Drupal/user/Tests/Views/AccessRoleUITest.php
   @@ -0,0 +1,68 @@
   +  public static function getInfo() {
   

   Don't we inheritdoc this too now.

4. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php
   @@ -185,6 +185,8 @@ protected function getRoute($view_id, $display_id) {
   +    $route->setOption('_access_mode', 'ANY');
   

   Does this change work fine for all cases? seems like a big change, maybe a comment is needed just so future me will know whats going on.

5. +++ b/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php
   @@ -149,6 +149,34 @@ public function testSetChecks() {
   +    $access_check->expects($this->exactly(2))
   +      ->method('applies')
   +      ->will($this->returnCallback(function (Route $route) {
   

   Worth adding a ->with($this->isInstanceOf('Route'))?

Thank you for the review, sadly I cannot upload a new patch now.

This inline commenting is weird. Is this a new thing?

Yes we indeed do that now, in quite some places. This is pretty cool for static code analyse tools as well as IDE users.

Really great review!

+++ b/core/modules/user/lib/Drupal/user/Plugin/views/access/Role.php
@@ -42,7 +42,10 @@ public function access(AccountInterface $account) {
+      debug($route->setRequirement('_role', (string) implode(',', $this->options['role'])));

Stray debug

Hacked the patch file.

I don't often hack patch files, but when I do I fail completely!

1. +++ b/core/lib/Drupal/Core/Access/AccessManager.php
   @@ -159,13 +159,11 @@ protected function applies(Route $route) {
   +    // Finally ask the dynamic access checkers whether they do apply.
   

   "Finally, see if any dynamic access checkers apply."

2. +++ b/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php
   @@ -149,6 +149,35 @@ public function testSetChecks() {
   +  public function testSetChecksWithDynamicAccessChecker() {
   

   Nice, this looks like some coverage that was missing originally - sorry about that!

   Worth asserting the expected _access_checks array on the route options too here?

Worth asserting the expected _access_checks array on the route options too here?

This is a good point! Let's do it.

In general views is one of the usecases where things gets complicated pretty fast, so we find bugs fast.

Great, RTBC for me. I don't see why this would not come back green.

Great, RTBC for me. I don't see why this would not come back green.

Haha

.

Though we are going back to procedural code, this is critical issue (as it throws fatal) and would be great to move forward, (i.e. discuss and update this patch or create follow up(s) etc)...

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php
@@ -185,6 +185,10 @@ protected function getRoute($view_id, $display_id) {
+    $route->setOption('_access_mode', 'ANY');

This looks like it could easily end up as an information disclosure vulnerability down the line, so I'm not really comfortable doing it in a follow-up - or at least that follow-up also needs to be critical.

This looks like it could easily end up as an information disclosure vulnerability down the line, so I'm not really comfortable doing it in a follow-up - or at least that follow-up also needs to be critical.

Well, this is the intended behavior ... what should we do? At least at the moment views does not allow you to configure multiple access plugins + the one for the "administer views" permission.

Maybe critical follow-up? I definitely don't have my head around what happens with overriding routes from the UI and how that interacts with multiple access checkers so it feels like its own discussion.

Here it is: #2157541: Views sets access to ANY on routes - could result in information disclosure

OK. Committed/pushed to 8.x, thanks!

Head broken:

* Drupal\user\Tests\Views\AccessRoleTest (20 pass(es), 0 fail(s), and 1 exception(s))
   - [exception] [Uncaught exception] "Symfony\Component\DependencyInjection\Exception\RuntimeException: You have requested a synthetic service ("request"). The DIC does not know how to construct this service. in Symfony\Component\DependencyInjection\ContainerBuilder->createService() (line 922 of /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php). Symfony\Component\DependencyInjection\ContainerBuilder->createService(Object, 'request')
Symfony\Component\DependencyInjection\ContainerBuilder->get('request')
Drupal::request()
Drupal\simpletest\WebTestBase->curlExec(Array)
Drupal\simpletest\WebTestBase->drupalGet('user/logout', Array)
Drupal\simpletest\WebTestBase->drupalLogout()
Drupal\simpletest\WebTestBase->drupalLogin(Object)
Drupal\user\Tests\Views\AccessRoleTest->testAccessRole()
Drupal\simpletest\TestBase->run()
simpletest_script_run_one_test('812', 'Drupal\user\Tests\Views\AccessRoleTest')
" in ContainerBuilder.php on line 922 of Symfony\Component\DependencyInjection\ContainerBuilder->createService().

36: vdc-1912602.patch queued for re-testing.

This is the calls to handle() on the http kernel in the tests.

So we could just use drupalGet() calls, as this is a web test anyway?

Unless someone wants to fix what handle() is doing with the container request service that is set?

That's worth fixing, but not here. I missed that we were doing kernel->handle() in the tests and probably would've pushed back against that anyway had I noticed.

RTBC pending bot coming back green.

Committed/pushed to 8.x, thanks!