Problem/Motivation
Changing the access permissions for a newly created view from the default "Permission" to "Role" and clicking Apply causes an AJAX error message re getRoles() in Role.php (see below). The change to "Role" is applied, but the settings for "Role" can't be changed as clicking on the "Settings" link causes the same AJAX error. Tried in IE, FF and Chrome, same error and line 69 referenced in all browsers.
Error message in Chrome:
An AJAX HTTP error occurred.HTTP Result Code: 200Debugging information follows.Path: /drupal8/admin/structure/views/ajax/display/bug_test_view/default/access_optionsStatusText: OKResponseText: ( ! ) Fatal error: Call to undefined method Drupal\user\Plugin\views\access\Role::getRoles() in C:\wamp\www\drupal8\core\modules\user\lib\Drupal\user\Plugin\views\access\Role.php on line 69Call Stack#TimeMemoryFunctionLocation10.0011412328{main}( )..\index.php:020.00711052664drupal_handle_request( )..\index.php:1730.217319228392Symfony\Component\HttpKernel\Kernel->handle( )..\bootstrap.inc:220440.221919454056Drupal\Core\HttpKernel->handle( )..\Kernel.php:19750.221919454728Symfony\Component\HttpKernel\HttpKernel->handle( )..\HttpKernel.php:5260.221919454728Symfony\Component\HttpKernel\HttpKernel->handleRaw( )..\HttpKernel.php:7370.366622410704call_user_func_array( )..\HttpKernel.php:12980.366622411144Drupal\views_ui\Routing\ViewsUIController->ajaxForm( )..\HttpKernel.php:12990.381223937384views_ui_ajax_form( )..\ViewsUIController.php:377100.382824008192views_ajax_form_wrapper( )..\admin.inc:577110.382824008744drupal_build_form( )..\ajax.inc:104120.382824010528drupal_retrieve_form( )..\form.inc:368130.382924011712call_user_func_array( )..\form.inc:839140.382924011944views_ui_edit_display_form( )..\form.inc:839150.400825191872Drupal\views\Plugin\views\display\DisplayPluginBase->buildOptionsForm( )..\admin.inc:726160.404725250952Drupal\user\Plugin\views\access\Role->buildOptionsForm( )..\DisplayPluginBase.php:1496
Steps to get there:
- Install 8.x from scratch
- Menu -- Structure -- Views -- [create a new view]
- Click on Permission link next to access
- Choose Roles and click Apply
- AJAX error displayed
Proposed resolution
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#46 | interdiff-1912602-46.txt | 1.47 KB | damiankloip |
#46 | 1912602-46.patch | 12.56 KB | damiankloip |
#33 | interdiff.txt | 1.18 KB | dawehner |
#31 | vdc-1912602.patch | 12.84 KB | dawehner |
#31 | interdiff.txt | 665 bytes | dawehner |
Comments
Comment #1
dawehnerHere is a fix for that.
Comment #2
kerasai CreditAttribution: kerasai commented#1: drupal-1912602-1.patch queued for re-testing.
Comment #4
bdone CreditAttribution: bdone commentedi'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.
Comment #6
dawehnerAdded a test.
Comment #7
tim.plunkettThis seems to be missing an assert
Comment #8
dawehnerOh you and I seemed to have uploaded the wrong patch.
Comment #10
dagmarsetUp() already call to enableViewsTestModule() in the parent function. This reason test data is being created twice.
Comment #12
dawehnerHi dagmar!
Let's fix it.
Comment #13
dawehnerI kind of consider this more like a critical as the roles access plugin don't work with that.
Comment #14
s_gupta_14 CreditAttribution: s_gupta_14 commentedTested 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
Comment #15
s_gupta_14 CreditAttribution: s_gupta_14 commentedComment #16
marthinal CreditAttribution: marthinal commented12: vdc-1912602-12.patch queued for re-testing.
Comment #18
dawehnerGood point, let's actually fix all of it and write tests.
Wow, this shows a lot of problems all over the place.
Comment #20
dawehner18: vdc-1912602.patch queued for re-testing.
Comment #22
dawehnerSome 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.
Comment #24
dawehner22: vdc-1912602.patch queued for re-testing.
Comment #25
damiankloip CreditAttribution: damiankloip commentedI prefer the '\Drupal\Component\Utility\String::checkPlain' syntax, but that's up to you.
This inline commenting is weird. Is this a new thing?
Don't we inheritdoc this too now.
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.
Worth adding a ->with($this->isInstanceOf('Route'))?
Comment #26
dawehnerThank you for the review, sadly I cannot upload a new patch now.
Yes we indeed do that now, in quite some places. This is pretty cool for static code analyse tools as well as IDE users.
Comment #27
dawehnerReally great review!
Comment #28
tim.plunkettStray debug
Comment #29
dawehnerHacked the patch file.
Comment #31
dawehnerI don't often hack patch files, but when I do I fail completely!
Comment #32
damiankloip CreditAttribution: damiankloip commented"Finally, see if any dynamic access checkers apply."
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?
Comment #33
dawehnerThis 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.
Comment #34
damiankloip CreditAttribution: damiankloip commentedGreat, RTBC for me. I don't see why this would not come back green.
Comment #36
dawehnerHaha
Comment #37
dawehner.
Comment #38
vijaycs85Though 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)...
Comment #39
catchThis 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.
Comment #40
dawehnerWell, 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.
Comment #41
catchMaybe 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.
Comment #42
vijaycs85Here it is: #2157541: Views sets access to ANY on routes - could result in information disclosure
Comment #43
catchOK. Committed/pushed to 8.x, thanks!
Comment #44
catchHead broken:
Comment #45
catch36: vdc-1912602.patch queued for re-testing.
Comment #46
damiankloip CreditAttribution: damiankloip commentedThis 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?
Comment #47
catchThat'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.
Comment #49
catchCommitted/pushed to 8.x, thanks!