Add authentication support to REST views

This is really an interesting feature!

1. +++ b/core/modules/rest/lib/Drupal/rest/Plugin/views/display/RestExport.php
   @@ -121,7 +129,8 @@ public static function create(ContainerInterface $container, array $configuratio
   +      array_keys($container->get('authentication')->getSortedProviders())
   

   I would like to avoid executing any kind of code in the constructor. Wrap the logic in some additional method and we could be fine.

2. +++ b/core/modules/rest/lib/Drupal/rest/Plugin/views/display/RestExport.php
   @@ -217,6 +226,7 @@ protected function defineOptions() {
   +    $options['auth']['default'] = array();
   

   wait, you can select multiple?

3. +++ b/core/modules/rest/lib/Drupal/rest/Plugin/views/display/RestExport.php
   @@ -247,6 +257,21 @@ public function optionsSummary(&$categories, &$options) {
   +      $auth = t('No authentication is set');
   ...
   +      'title' => t('Authentication'),
   

   Just use $this->t() instead

4. +++ b/core/modules/rest/lib/Drupal/rest/Plugin/views/display/RestExport.php
   @@ -255,6 +280,35 @@ public function optionsSummary(&$categories, &$options) {
   +
   +    if ($form_state['section'] == 'auth') {
   +      $form['#title'] .= t('The supported authentication methods of this view');
   +      $form['auth'] = array(
   +        '#type' => 'checkboxes',
   +        '#title' => t('Autentication methods'),
   +        '#description' => t('These are the supported authentication providers for this view. When this view is requested, the client will be forced to authenticate himself with one of the selected providers.'),
   +        '#options' => array_combine($this->authenticationProviders, $this->authenticationProviders),
   +        '#default_value' => array_filter($this->getOption('auth')),
   +      );
   +    }
   +  }
   +
   

   I would argue that you might want to put that onto PathPluginBase instead ... this could be helpful on html ones as well.

5. +++ b/core/modules/rest/lib/Drupal/rest/Plugin/views/display/RestExport.php
   @@ -255,6 +280,35 @@ public function optionsSummary(&$categories, &$options) {
   +   * Overrides \Drupal\views\Plugin\views\display\DisplayPluginBase::submitOptionsForm().
   

   {@inheritdoc}

6. +++ b/core/modules/rest/lib/Drupal/rest/Plugin/views/display/RestExport.php
   @@ -270,6 +324,16 @@ public function collectRoutes(RouteCollection $collection) {
   +        $requirements['_user_is_logged_in'] = 'TRUE';
   

   What does that one do?

1. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/Feed.php
   @@ -152,6 +152,8 @@ protected function defineOptions() {
   +    // Relocate Authentication settings so they are visible.
   +    $options['auth']['category'] = 'page';
   
   +++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/Page.php
   @@ -112,6 +112,8 @@ public function execute() {
   +    // Relocate authentication settings so they are visible.
   +    $options['auth']['category'] = 'page';
   

   good idea but you can just move that into the common base class

2. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php
   @@ -217,6 +241,16 @@ public function collectRoutes(RouteCollection $collection) {
   +      $options = array(
   +        '_auth' => $auth,
   +      );
   +      $route->addOptions($options);
   

   Just use ->setOption as this makes it a little bit easier to read and (maybe) does not override other ones.

1. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
   @@ -112,7 +114,8 @@ public static function create(ContainerInterface $container, array $configuratio
   +      parent::getAuthProviders($container)
   

   It is always a bad sign to execute any code during the construction time of objects. Better just inject the service and be done with it. Otherwise problems like circular dependencies could occur, which are both hard to track and potentially fix.

2. +++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
   @@ -34,6 +34,13 @@
   +   * @var array
   

   In general you could use the array typehinting, see https://github.com/php-fig/fig-standards/pull/169/files#diff-9a10a11696d...

On top of that it would be great to expand the unit test coverage for adding authentication as well.

Any update here ? or there is other approaches to have auth in REST views?

Many thanks guys

No need to change status.

Patch rerolled.

core/modules/views/src/Plugin/views/display/PathPluginBase.php:433

        $authentication_providers = array_keys($this->authenticationManager->getSortedProviders());

To make this work I needed 'revert' #2456303: AuthenticationManager needs interface and ::getProviderKeys making the method public again. As suggested in #2456303-23: AuthenticationManager needs interface and ::getProviderKeys we need the code from Rest UI

Another hack is in core/modules/views/src/Plugin/views/display/PathPluginBase.php:67

  public function __construct(array $configuration, $plugin_id, $plugin_definition, RouteProviderInterface $route_provider, StateInterface $state, AuthenticationManager $authentication_manager = NULL) {

The unit test core/modules/rest/tests/src/Unit/CollectRoutesTest.php will still fail. What's wrong with the interdiff?

This is now a bug as views are not accessible through a Basic Auth rest call.

curl --user admin:admin --header 'Accept: application/hal+json' --request GET http://drupal.d8/node

gives 403 forbidden.

In #32 I made this a bug but I'm still not sure what the root cause is.

After following the 'How to test' steps requesting curl --header 'Accept: application/hal+json' --request GET http://drupal.d8/node shows a list of nodes but requesting as admin curl --user=admin:admin --header 'Accept: application/hal+json' --request GET http://drupal.d8/node gives 'access denied'.

Why is a anonymous allowed and admin not to view the result?

This is most likely because basic auth is not a global authentication provider while cookie auth is. Global authentication providers apply even when the route does not declare any authentication providers via _auth option, while non-global providers need to be explicitely set on the route.

If a resource is requested with basic auth credentials, then the basic auth provider claims that request. This happens before routing. After routing it is checked whether the authentication provider is allowed on the selected route. This will result in a 403 if that check fails.

@znerol thanks for the explanation.

As [edit] #2403307: RPC endpoints for user authentication: log in, check login status, log out #2228141: Add authentication support to REST views [/edit] is postponed to 8.1.x this means Rest export display are not accessible by authenticated users as the only option to authenticate is basic auth for 8.0.x

Setting to major as this otherwise renders headless Drupal basically useless.

@clemens.tolboom
This link points to this issue, do you mind updating the URL :)

@dawehner sorry for the wrong link. Is now fixed.

I meant to link to #2403307: RPC endpoints for user authentication: log in, check login status, log out

Yeah, the blocking issue is now fixed!

Rerolled and updated for AuthenticationCollector

Fixed the test fail.

+++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
@@ -228,6 +241,13 @@ public function collectRoutes(RouteCollection $collection) {
+    $auth = array_filter($this->getOption('auth'));

@@ -387,6 +407,21 @@ public function optionsSummary(&$categories, &$options) {
+    $auth = array_filter($this->getOption('auth'));

@@ -410,6 +445,17 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
+          '#default_value' => array_filter($this->getOption('auth')),

The test fail was due to array_filter() getting a NULL from the unit test. Seems brittle to me. Are there possible situations where ::getOption('auth') could be NULL?

1. +++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
   @@ -118,6 +130,7 @@ protected function defineOptions() {
   +    $options['auth']['default'] = array();
   

   Interesting, I would not have thought that we could simply also put auth onto the PathPluginBase level, this is great. Do you mind adapting the way how the code looks like to be more inline with the other default values?

2. +++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
   @@ -228,6 +241,13 @@ public function collectRoutes(RouteCollection $collection) {
   +    $auth = array_filter($this->getOption('auth'));
   
   @@ -387,6 +407,21 @@ public function optionsSummary(&$categories, &$options) {
   +    // Authentication.
   +    $auth = array_filter($this->getOption('auth'));
   
   @@ -439,6 +485,10 @@ public function submitOptionsForm(&$form, FormStateInterface $form_state) {
   +
   +    if ($form_state->get('section') == 'auth') {
   +      $this->setOption('auth', $form_state->getValue('auth'));
   +    }
   

   Do you think we really need to filter both on storage and on rendering? What about filtering on storage?

   + we certainly need some form of config schema

3. +++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
   @@ -410,6 +445,17 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +        $authentication_providers = array_keys($this->authenticationCollector->getSortedProviders());
   ...
   +          '#options' => array_combine($authentication_providers, $authentication_providers),
   

   We don't have any human readable names for them right? Well too bad

4. +++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
   @@ -410,6 +445,17 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +        $form['#title'] .= $this->t('The supported authentication methods of this view');
   

   Won't this cause some form of escaping?

#44:
1. Done :)
2. Done. Added config schema also.
3. Yeah. Too bad.
4. This was c/p'ed from code directly above. Not sure of the escaping.

Fixed test mocks to match changed PathPluginBase::__construct() method signature

@almaudoh Permissions for RESTEXport on view works fine for me.

+++ b/core/modules/rest/tests/src/Unit/CollectRoutesTest.php
@@ -78,6 +78,12 @@ protected function setUp() {
+    $authentication_collector = $this->getMock('\Drupal\Core\Authentication\AuthenticationCollectorInterface');
+    $container->set('authentication_collector', $authentication_collector);
+    $authentication_collector->expects($this->any())
+      ->method('getSortedProviders')
+      ->will($this->returnValue(array('basic_auth' => 'data', 'cookie' => 'data')));

Just in case you write another new mock somewhere, maybe consider using prophecy ... its just better to read.

We still need some proper tests ..

We still need some proper tests ..

In which area specifically do we need more tests?

Some form of integration test coverage would be great, so ensuring that basic auth for example works for such a configured view.

Re-roll

The latest patch works fine for view based RESTExport .
Authentication with basic_auth provider works for all 3 access type namely None, Role and Permission. The results are as expected.

Some form of integration test coverage would be great

@dawehner: I've struggled with this without much headway. My knowledge of Views internals is limited. Would appreciate some pointers.

As reported in [#2504433#5] using BASIC_AUTH seems to test for a token which seems completely wrong. I wonder whether this patch solves this. The issues summary suggest it works.

It would be still great if someone could write a proper test which uses an actual request to see whether the configuration got applied correctly.

Applying the patch is ok BUT the Authentication appears on every display which is not what we need.

@dawehner I agree we need integration tests. I'm similar puzzled as @almaudoh on where to start. Do you have some hints?

I've updated the step to test to using _format and can confirm the patch works so we can close #2504433: REST cookie auth overrides basic auth? as a duplicate of this issue.

This should not be as it confuses our users right?

This should not be as it confuses our users right?

Yeah I don't think it makes sense on page displays, oh well, maybe we could have a flag on views displays to include it, because well, it makes logically sense on PathPluginBase, given that this is the location which deals with the routing integration.

Yes, split on this. It definitely only makes sense on the REST export display, but code wise, the PathPluginBase is not a bad spot for it!

Working on this

- Moved the whole auth option thing to the rest display. Did not add a flag (as suggested in #67
- Added a simple test case for this.

Might maybe move this test to its own file. Comments on that?

Oops, took out a bit too much when removing some debug things from my test.

1. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
   @@ -230,6 +247,15 @@ protected function defineOptions() {
   +      $auth = $this->t('No authentication is set');
   ...
   +      $auth = implode(', ', $auth);
   

   This might as well just go on one line:

   $auth = $this->getOption('auth') ? implode(', ', $auth) : $this->t('No authentication is set')

2. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
   @@ -255,6 +286,35 @@ public function optionsSummary(&$categories, &$options) {
   +      $authentication_providers = array_keys($this->authenticationCollector->getSortedProviders());
   ...
   +        '#options' => array_combine($authentication_providers, $authentication_providers),
   

   IMO it's better to just have this in a small helper method, like getAuthOptions() or something. Not a major issue though.

1. +++ b/core/modules/rest/tests/modules/rest_test_views/test_views/views.view.test_serializer_node_display_field.yml
   @@ -149,3 +149,24 @@ display:
   +        - 'config:field.storage.node.body'
   \ No newline at end of file
   diff --git a/core/modules/rest/tests/src/Unit/CollectRoutesTest.php b/core/modules/rest/tests/src/Unit/CollectRoutesTest.php
   

   Nitpick

2. +++ b/core/modules/rest/tests/src/Unit/CollectRoutesTest.php
   @@ -81,6 +81,12 @@ protected function setUp() {
    
   +    $authentication_collector = $this->getMock('\Drupal\Core\Authentication\AuthenticationCollectorInterface');
   +    $container->set('authentication_collector', $authentication_collector);
   +    $authentication_collector->expects($this->any())
   +      ->method('getSortedProviders')
   +      ->will($this->returnValue(array('basic_auth' => 'data', 'cookie' => 'data')));
   +
        \Drupal::setContainer($container);
    
        $this->restExport = RestExport::create($container, array(), "test_routes", array());
   @@ -92,6 +98,9 @@ protected function setUp() {
   
   @@ -92,6 +98,9 @@ protected function setUp() {
        // Set the style option.
        $this->restExport->setOption('style', array('type' => 'serializer'));
    
   +    // Set the auth option.
   +    $this->restExport->setOption('auth', ['basic_auth']);
   +
   

   I don't see an added assertion in this change. Let's add one.

#75:

1 and 2: Fair enough. These were just copy-pasted from the earlier patch, but hard to disagree with that :)

#76

1. Fixed. Apparently my editor does not add new lines automatically on yaml files.
2. Added not only one, but 2 new assertions. Although this change stems from the earlier patch as well.

Thank you!

1. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
   @@ -205,11 +219,25 @@ public function getContentType() {
   +   * @return array
   

   Nit: string[].

2. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
   @@ -255,6 +291,34 @@ public function optionsSummary(&$categories, &$options) {
   +    if ($form_state->get('section') == 'auth') {
   ...
   +    if ($form_state->get('section') == 'auth') {
   

   Nit: why not ===?

3. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
   @@ -273,6 +337,11 @@ public function collectRoutes(RouteCollection $collection) {
   +      // Add authentication to the route if it was set.
   +      $auth = $this->getOption('auth');
   +      if (!empty($auth)) {
   +        $route->setOption('_auth', $auth);
   +      }
   

   What if no authentication is specified?

   Perhaps it's obvious to everyone else, but it's not obvious to me what the behavior is in that case. I think it'd be good to have that documented?

What if no authentication is specified?

Well, we fallback to the default routing behaviour, which is configurable by default.

Perhaps it is not obvious from reading the code, but from reading the patch it seems obvious to me that the route will be as before if auth is not added. That being said, it is not obvious from reading the code, I guess.

Just did your suggested changes and added one extra sentence about this.

Re-RTBC since #79 is addressed.

+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -337,7 +337,8 @@ public function collectRoutes(RouteCollection $collection) {
+      // set, no authentication will be required for the route.

This is wrong. We fallback to the default authentication mechanism, which is cookie based by default.

Just used your wording for the updated comment :)

thank you

This looks great but don't we need an update to re-save existing REST views so that the default option gets saved? Otherwise it'll show up in diffs etc.

This looks great but don't we need an update to re-save existing REST views so that the default option gets saved? Otherwise it'll show up in diffs etc.

Fair point. The behaviour wouldn't change, as we still have the defaults merging, but I agree that its a good idea for the diffs.

Just resaving them would not trigger any config diff. Just tried it. Should it?

The only way to trigger the diff would be to add the auth option (as empty) in the update hook. Should we do that? Attached patch with that solution.

Just resaving them would not trigger any config diff. Just tried it. Should it?

Mh, currently views doesn't? Maybe it should though

Maybe... Pretty sure that is a different issue though :p

doh

OH yeah, this is why some automatic checking could be pretty useful. Variables should always be defined :)

PS: We could get rid of the $view->set('display, $displays) call, but I cannot really care about it, to be honest.

Doesn't the hook_update_N implementation need a test?

Adding test case in views module for importing views related to rest configuration.

I have, rerolled agianst 8.1.x

Moved ViewsUpdateTest file as per namespace and fixed wrong include file path.

1. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
   @@ -19,6 +20,7 @@
   @@ -83,6 +85,13 @@ class RestExport extends PathPluginBase implements ResponseDisplayPluginInterfac
   

   We should adapt the calculate dependency method to include the auth providing module as well.

2. +++ b/core/modules/views/src/Tests/Update/ViewsUpdateTest.php
   @@ -0,0 +1,38 @@
   +    $module_data = \Drupal::config('core.extension')->get('module');
   +    $this->assertFalse(isset($module_data['views']), 'The views module was not installed.');
   

   Is this really what we want to test? Don't we want to test that 'auth' was added to the stored view?

Adding patch to make test case run.

Working on this.

Hm, these update tests never finishes on my computer.

Please feel free to take over, someone with the ability to run them.

There is something wrong with the test fixtures. This might still not be right, but something like this needs work.

Rerolled for 8.2

Good that got rid of the fatal error. The test view was not getting loaded before. But there are now 3 different test fails.

+++ b/core/modules/views/tests/fixtures/update/rest-export-with-authentication.php
@@ -0,0 +1,19 @@
+$connection->merge('config')
+  ->condition('name', 'views.view.rest_export')
+  ->condition('collection', '')
+  ->fields([
+    'data' => 'a:13:{s:4:"uuid";s:36:"24901f79-f99d-408f-a25c-92d0578117d2";s:8:"langcode";s:2:"en";s:6:"status";b:1;s:12:"dependencies";a:2:{s:6:"config";a:2:{
+  ])
+  ->execute();

hmmm, what is this supposed to be doing?

@alexpott, @xjm, @timplunkett discussed this issue.

We agreed that this is a serious blocker for people using REST views behind authentication.

In #2575453: Remove message claiming defaults or make it work like its claim raised a good point about usage of authentication providers, suppose description should point that user should expect when nothing checked

@lokapujya
Do you think you can work on this patch? It would be really nice to land this

Current implementation of REST resources skips routes without auth so why views implements fallback to cookie?

+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -273,6 +337,13 @@ public function collectRoutes(RouteCollection $collection) {
+      // Add authentication to the route if it was set. If no authentication was
+      // set, the default authentication will be used, which is cookie based by
+      // default.
+      $auth = $this->getOption('auth');
+      if (!empty($auth)) {
+        $route->setOption('_auth', $auth);
+      }

Why that different from rest module implementation?
\Drupal\rest\Routing\ResourceRoutes::alterRoutes()

@andypost
I see where you are coming from.

We certainly don't want to change the existing behaviour aka. no specific authentication is selected, as otherwise existing views out there would break.

Rerolled the patch

Just some additional reviews.

1. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
   @@ -69,6 +69,46 @@ protected function setUp() {
   +  public function testRestViewsPermissions() {
   

   This is a misleading function name, as it doesn't tell you that this is related to a custom auth method.

2. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
   @@ -69,6 +69,46 @@ protected function setUp() {
   +    $url = $this->buildUrl('test/serialize/auth_with_perm');
   +    $curl_options = array(
   +      CURLOPT_HTTPGET => TRUE,
   +      CURLOPT_CUSTOMREQUEST => 'GET',
   +      CURLOPT_URL => $url,
   +      CURLOPT_NOBODY => FALSE,
   +      CURLOPT_HTTPHEADER => [
   +        'Authorization: Basic ' . base64_encode($this->adminUser->getUsername() . ':' . $this->adminUser->pass_raw),
   +      ],
   +    );
   +    $this->responseBody = $this->curlExec($curl_options);
   

   I'm wondering whether we could leverage guzzle for much less cruft.

3. +++ b/core/modules/rest/tests/src/Unit/CollectRoutesTest.php
   @@ -76,6 +76,12 @@ protected function setUp() {
   +    $authentication_collector = $this->getMock('\Drupal\Core\Authentication\AuthenticationCollectorInterface');
   

   You could make this a little bit nicer by using AuthenticationCollectorInterface::class or even use prophecy for mocking.

4. +++ b/core/modules/views/tests/fixtures/update/rest-export-with-authentication.php
   @@ -0,0 +1,19 @@
   +    'data' => 'a:13:{s:4:"uuid";s:36:"24901f79-f99d-408f-a25c-92d0578117d2";s:8:"langcode";s:2:"en";s:6:"status";b:1;s:12:"dependencies";a:2:{s:6:"config";a:2:{i:0;s:33:"core.entity_view_mode.node.teaser";i:1;s:23:"user.role.authenticated";}s:6:"module";a:3:{i:0;s:4:"node";i:1;s:4:"rest";i:2;s:4:"user";}}s:2:"id";s:11:"rest_export";s:5:"label";s:11:"Rest Export";s:6:"module";s:5:"views";s:11:"description";s:0:"";s:3:"tag";s:0:"";s:10:"base_table";s:15:"node_field_data";s:10:"base_field";s:3:"nid";s:4:"core";s:3:"8.x";s:7:"display";a:2:{s:7:"default";a:6:{s:14:"display_plugin";s:7:"default";s:2:"id";s:7:"default";s:13:"display_title";s:6:"Master";s:8:"position";i:0;s:15:"display_options";a:17:{s:6:"access";a:2:{s:4:"type";s:4:"role";s:7:"options";a:1:{s:4:"role";a:1:{s:13:"authenticated";s:13:"authenticated";}}}s:5:"cache";a:2:{s:4:"type";s:3:"tag";s:7:"options";a:0:{}}s:5:"query";a:2:{s:4:"type";s:11:"views_query";s:7:"options";a:5:{s:19:"disable_sql_rewrite";b:0;s:8:"distinct";b:0;s:7:"replica";b:0;s:13:"query_comment";s:0:"";s:10:"query_tags";a:0:{}}}s:12:"exposed_form";a:2:{s:4:"type";s:5:"basic";s:7:"options";a:7:{s:13:"submit_button";s:5:"Apply";s:12:"reset_button";b:0;s:18:"reset_button_label";s:5:"Reset";s:19:"exposed_sorts_label";s:7:"Sort by";s:17:"expose_sort_order";b:1;s:14:"sort_asc_label";s:3:"Asc";s:15:"sort_desc_label";s:4:"Desc";}}s:5:"pager";a:2:{s:4:"type";s:4:"full";s:7:"options";a:7:{s:14:"items_per_page";i:10;s:6:"offset";i:0;s:2:"id";i:0;s:11:"total_pages";N;s:6:"expose";a:7:{s:14:"items_per_page";b:0;s:20:"items_per_page_label";s:14:"Items per page";s:22:"items_per_page_options";s:13:"5, 10, 25, 50";s:26:"items_per_page_options_all";b:0;s:32:"items_per_page_options_all_label";s:7:"- All -";s:6:"offset";b:0;s:12:"offset_label";s:6:"Offset";}s:4:"tags";a:4:{s:8:"previous";s:12:"‹ Previous";s:4:"next";s:8:"Next ›";s:5:"first";s:8:"« First";s:4:"last";s:7:"Last »";}s:8:"quantity";i:9;}}s:5:"style";a:1:{s:4:"type";s:7:"default";}s:3:"row";a:2:{s:4:"type";s:11:"entity:node";s:7:"options";a:1:{s:9:"view_mode";s:6:"teaser";}}s:6:"fields";a:1:{s:5:"title";a:37:{s:2:"id";s:5:"title";s:5:"table";s:15:"node_field_data";s:5:"field";s:5:"title";s:11:"entity_type";s:4:"node";s:12:"entity_field";s:5:"title";s:5:"label";s:0:"";s:5:"alter";a:8:{s:10:"alter_text";b:0;s:9:"make_link";b:0;s:8:"absolute";b:0;s:4:"trim";b:0;s:13:"word_boundary";b:0;s:8:"ellipsis";b:0;s:10:"strip_tags";b:0;s:4:"html";b:0;}s:10:"hide_empty";b:0;s:10:"empty_zero";b:0;s:8:"settings";a:1:{s:14:"link_to_entity";b:1;}s:9:"plugin_id";s:5:"field";s:12:"relationship";s:4:"none";s:10:"group_type";s:5:"group";s:11:"admin_label";s:0:"";s:7:"exclude";b:0;s:12:"element_type";s:0:"";s:13:"element_class";s:0:"";s:18:"element_label_type";s:0:"";s:19:"element_label_class";s:0:"";s:19:"element_label_colon";b:1;s:20:"element_wrapper_type";s:0:"";s:21:"element_wrapper_class";s:0:"";s:23:"element_default_classes";b:1;s:5:"empty";s:0:"";s:16:"hide_alter_empty";b:1;s:17:"click_sort_column";s:5:"value";s:4:"type";s:6:"string";s:12:"group_column";s:5:"value";s:13:"group_columns";a:0:{}s:10:"group_rows";b:1;s:11:"delta_limit";i:0;s:12:"delta_offset";i:0;s:14:"delta_reversed";b:0;s:16:"delta_first_last";b:0;s:10:"multi_type";s:9:"separator";s:9:"separator";s:2:", ";s:17:"field_api_classes";b:0;}}s:7:"filters";a:1:{s:6:"status";a:16:{s:2:"id";s:6:"status";s:5:"table";s:15:"node_field_data";s:5:"field";s:6:"status";s:12:"relationship";s:4:"none";s:10:"group_type";s:5:"group";s:11:"admin_label";s:0:"";s:8:"operator";s:1:"=";s:5:"value";b:0;s:5:"group";i:1;s:7:"exposed";b:0;s:6:"expose";a:10:{s:11:"operator_id";s:0:"";s:5:"label";s:0:"";s:11:"description";s:0:"";s:12:"use_operator";b:0;s:8:"operator";s:0:"";s:10:"identifier";s:0:"";s:8:"required";b:0;s:8:"remember";b:0;s:8:"multiple";b:0;s:14:"remember_roles";a:1:{s:13:"authenticated";s:13:"authenticated";}}s:10:"is_grouped";b:0;s:10:"group_info";a:10:{s:5:"label";s:0:"";s:11:"description";s:0:"";s:10:"identifier";s:0:"";s:8:"optional";b:1;s:6:"widget";s:6:"select";s:8:"multiple";b:0;s:8:"remember";b:0;s:13:"default_group";s:3:"All";s:22:"default_group_multiple";a:0:{}s:11:"group_items";a:0:{}}s:9:"plugin_id";s:7:"boolean";s:11:"entity_type";s:4:"node";s:12:"entity_field";s:6:"status";}}s:5:"sorts";a:1:{s:7:"created";a:13:{s:2:"id";s:7:"created";s:5:"table";s:15:"node_field_data";s:5:"field";s:7:"created";s:5:"order";s:4:"DESC";s:11:"entity_type";s:4:"node";s:12:"entity_field";s:7:"created";s:9:"plugin_id";s:4:"date";s:12:"relationship";s:4:"none";s:10:"group_type";s:5:"group";s:11:"admin_label";s:0:"";s:7:"exposed";b:0;s:6:"expose";a:1:{s:5:"label";s:0:"";}s:11:"granularity";s:6:"second";}}s:5:"title";s:11:"Rest Export";s:6:"header";a:0:{}s:6:"footer";a:0:{}s:5:"empty";a:0:{}s:13:"relationships";a:0:{}s:9:"arguments";a:0:{}s:17:"display_extenders";a:0:{}}s:14:"cache_metadata";a:3:{s:7:"max-age";i:-1;s:8:"contexts";a:5:{i:0;s:26:"languages:language_content";i:1;s:28:"languages:language_interface";i:2;s:14:"url.query_args";i:3;s:21:"user.node_grants:view";i:4;s:10:"user.roles";}s:4:"tags";a:0:{}}}s:13:"rest_export_1";a:6:{s:14:"display_plugin";s:11:"rest_export";s:2:"id";s:13:"rest_export_1";s:13:"display_title";s:11:"REST export";s:8:"position";i:2;s:15:"display_options";a:3:{s:17:"display_extenders";a:0:{}s:4:"path";s:19:"unpublished-content";s:4:"auth";a:1:{s:10:"basic_auth";s:10:"basic_auth";}}s:14:"cache_metadata";a:3:{s:7:"max-age";i:-1;s:8:"contexts";a:5:{i:0;s:26:"languages:language_content";i:1;s:28:"languages:language_interface";i:2;s:14:"request_format";i:3;s:21:"user.node_grants:view";i:4;s:10:"user.roles";}s:4:"tags";a:0:{}}}}}',
   

   I'm wondering whether we could provide the view as YML file instead, see core/modules/views/tests/fixtures/update/argument-placeholder.php as example. This would make it much more readable.

+++ b/core/modules/views/src/Tests/Update/ViewsUpdateTest.php
@@ -0,0 +1,41 @@
+  public function testUpdate() {
...
+    $this->assertIdentical($displays['rest_export_1']['display_options']['auth']['basic_auth'], 'basic_auth', 'Basic authentication is set as authentication method.');

+++ b/core/modules/views/tests/fixtures/update/rest-export-with-authentication.php
@@ -0,0 +1,19 @@
+    'data' => 'a:13:{s:4:"uuid";s:36:"24901f79-f99d-408f-a25c-92d0578117d2";s:8:"langcode";s:2:"en";s:6:"status";b:1;s:12:"dependencies";a:2:{s:6:"config";a:2:{i:0;s:33:"core.entity_view_mode.node.teaser";i:1;s:23:"user.role.authenticated";}s:6:"module";a:3:{i:0;s:4:"node";i:1;s:4:"rest";i:2;s:4:"user";}}s:2:"id";s:11:"rest_export";s:5:"label";s:11:"Rest Export";s:6:"module";s:5:"views";s:11:"description";s:0:"";s:3:"tag";s:0:"";s:10:"base_table";s:15:"node_field_data";s:10:"base_field";s:3:"nid";s:4:"core";s:3:"8.x";s:7:"display";a:2:{s:7:"default";a:6:{s:14:"display_plugin";s:7:"default";s:2:"id";s:7:"default";s:13:"display_title";s:6:"Master";s:8:"position";i:0;s:15:"display_options";a:17:{s:6:"access";a:2:{s:4:"type";s:4:"role";s:7:"options";a:1:{s:4:"role";a:1:{s:13:"authenticated";s:13:"authenticated";}}}s:5:"cache";a:2:{s:4:"type";s:3:"tag";s:7:"options";a:0:{}}s:5:"query";a:2:{s:4:"type";s:11:"views_query";s:7:"options";a:5:{s:19:"disable_sql_rewrite";b:0;s:8:"distinct";b:0;s:7:"replica";b:0;s:13:"query_comment";s:0:"";s:10:"query_tags";a:0:{}}}s:12:"exposed_form";a:2:{s:4:"type";s:5:"basic";s:7:"options";a:7:{s:13:"submit_button";s:5:"Apply";s:12:"reset_button";b:0;s:18:"reset_button_label";s:5:"Reset";s:19:"exposed_sorts_label";s:7:"Sort by";s:17:"expose_sort_order";b:1;s:14:"sort_asc_label";s:3:"Asc";s:15:"sort_desc_label";s:4:"Desc";}}s:5:"pager";a:2:{s:4:"type";s:4:"full";s:7:"options";a:7:{s:14:"items_per_page";i:10;s:6:"offset";i:0;s:2:"id";i:0;s:11:"total_pages";N;s:6:"expose";a:7:{s:14:"items_per_page";b:0;s:20:"items_per_page_label";s:14:"Items per page";s:22:"items_per_page_options";s:13:"5, 10, 25, 50";s:26:"items_per_page_options_all";b:0;s:32:"items_per_page_options_all_label";s:7:"- All -";s:6:"offset";b:0;s:12:"offset_label";s:6:"Offset";}s:4:"tags";a:4:{s:8:"previous";s:12:"‹ Previous";s:4:"next";s:8:"Next ›";s:5:"first";s:8:"« First";s:4:"last";s:7:"Last »";}s:8:"quantity";i:9;}}s:5:"style";a:1:{s:4:"type";s:7:"default";}s:3:"row";a:2:{s:4:"type";s:11:"entity:node";s:7:"options";a:1:{s:9:"view_mode";s:6:"teaser";}}s:6:"fields";a:1:{s:5:"title";a:37:{s:2:"id";s:5:"title";s:5:"table";s:15:"node_field_data";s:5:"field";s:5:"title";s:11:"entity_type";s:4:"node";s:12:"entity_field";s:5:"title";s:5:"label";s:0:"";s:5:"alter";a:8:{s:10:"alter_text";b:0;s:9:"make_link";b:0;s:8:"absolute";b:0;s:4:"trim";b:0;s:13:"word_boundary";b:0;s:8:"ellipsis";b:0;s:10:"strip_tags";b:0;s:4:"html";b:0;}s:10:"hide_empty";b:0;s:10:"empty_zero";b:0;s:8:"settings";a:1:{s:14:"link_to_entity";b:1;}s:9:"plugin_id";s:5:"field";s:12:"relationship";s:4:"none";s:10:"group_type";s:5:"group";s:11:"admin_label";s:0:"";s:7:"exclude";b:0;s:12:"element_type";s:0:"";s:13:"element_class";s:0:"";s:18:"element_label_type";s:0:"";s:19:"element_label_class";s:0:"";s:19:"element_label_colon";b:1;s:20:"element_wrapper_type";s:0:"";s:21:"element_wrapper_class";s:0:"";s:23:"element_default_classes";b:1;s:5:"empty";s:0:"";s:16:"hide_alter_empty";b:1;s:17:"click_sort_column";s:5:"value";s:4:"type";s:6:"string";s:12:"group_column";s:5:"value";s:13:"group_columns";a:0:{}s:10:"group_rows";b:1;s:11:"delta_limit";i:0;s:12:"delta_offset";i:0;s:14:"delta_reversed";b:0;s:16:"delta_first_last";b:0;s:10:"multi_type";s:9:"separator";s:9:"separator";s:2:", ";s:17:"field_api_classes";b:0;}}s:7:"filters";a:1:{s:6:"status";a:16:{s:2:"id";s:6:"status";s:5:"table";s:15:"node_field_data";s:5:"field";s:6:"status";s:12:"relationship";s:4:"none";s:10:"group_type";s:5:"group";s:11:"admin_label";s:0:"";s:8:"operator";s:1:"=";s:5:"value";b:0;s:5:"group";i:1;s:7:"exposed";b:0;s:6:"expose";a:10:{s:11:"operator_id";s:0:"";s:5:"label";s:0:"";s:11:"description";s:0:"";s:12:"use_operator";b:0;s:8:"operator";s:0:"";s:10:"identifier";s:0:"";s:8:"required";b:0;s:8:"remember";b:0;s:8:"multiple";b:0;s:14:"remember_roles";a:1:{s:13:"authenticated";s:13:"authenticated";}}s:10:"is_grouped";b:0;s:10:"group_info";a:10:{s:5:"label";s:0:"";s:11:"description";s:0:"";s:10:"identifier";s:0:"";s:8:"optional";b:1;s:6:"widget";s:6:"select";s:8:"multiple";b:0;s:8:"remember";b:0;s:13:"default_group";s:3:"All";s:22:"default_group_multiple";a:0:{}s:11:"group_items";a:0:{}}s:9:"plugin_id";s:7:"boolean";s:11:"entity_type";s:4:"node";s:12:"entity_field";s:6:"status";}}s:5:"sorts";a:1:{s:7:"created";a:13:{s:2:"id";s:7:"created";s:5:"table";s:15:"node_field_data";s:5:"field";s:7:"created";s:5:"order";s:4:"DESC";s:11:"entity_type";s:4:"node";s:12:"entity_field";s:7:"created";s:9:"plugin_id";s:4:"date";s:12:"relationship";s:4:"none";s:10:"group_type";s:5:"group";s:11:"admin_label";s:0:"";s:7:"exposed";b:0;s:6:"expose";a:1:{s:5:"label";s:0:"";}s:11:"granularity";s:6:"second";}}s:5:"title";s:11:"Rest Export";s:6:"header";a:0:{}s:6:"footer";a:0:{}s:5:"empty";a:0:{}s:13:"relationships";a:0:{}s:9:"arguments";a:0:{}s:17:"display_extenders";a:0:{}}s:14:"cache_metadata";a:3:{s:7:"max-age";i:-1;s:8:"contexts";a:5:{i:0;s:26:"languages:language_content";i:1;s:28:"languages:language_interface";i:2;s:14:"url.query_args";i:3;s:21:"user.node_grants:view";i:4;s:10:"user.roles";}s:4:"tags";a:0:{}}}s:13:"rest_export_1";a:6:{s:14:"display_plugin";s:11:"rest_export";s:2:"id";s:13:"rest_export_1";s:13:"display_title";s:11:"REST export";s:8:"position";i:2;s:15:"display_options";a:3:{s:17:"display_extenders";a:0:{}s:4:"path";s:19:"unpublished-content";s:4:"auth";a:1:{s:10:"basic_auth";s:10:"basic_auth";}}s:14:"cache_metadata";a:3:{s:7:"max-age";i:-1;s:8:"contexts";a:5:{i:0;s:26:"languages:language_content";i:1;s:28:"languages:language_interface";i:2;s:14:"request_format";i:3;s:21:"user.node_grants:view";i:4;s:10:"user.roles";}s:4:"tags";a:0:{}}}}}',

This test is weird, why is there a view which already has basic_auth enabled?

Addressed my own feedback. This feels RTBCable to be honest.

Reviewing.

I think the rest_update_8202() name is correct.

Let's fix things

The patch in #139 seems to include the patch from #2721595: Simplify REST configuration (but those changes are not in the interdiff). Are those changes needed to make this pass? Is this blocked by that issue?

Oh no, yeah that was not planned. The interdiff should be able to apply to #137 though.

This is #137 and the interdiff from #141 rolled into one. Only change I added was changing the name of the update test to something a little more descriptive then ViewsUpdateTest.

Thank you lendude!

This is a reference to the variable, so changes to $display end up in $displays.

Here is a more real life example:

<?php

// More kitten names can be found on http://www.findcatnames.com/cute-overload-100-super-cute-kitten-names/
$kittens = ['Peanut', 'Stewie', 'Yogi', 'Trudy'];

foreach ($kittens as $kitten) {
    $kitten .= ' about to kill';
}

print_r($kittens);

// Kittens killer!

foreach ($kittens as &$kitten) {
    $kitten .= ' about to kill';
}

print_r($kittens);

1. +++ b/core/modules/rest/rest.install
   @@ -21,3 +21,27 @@ function rest_requirements($phase) {
   +        if (!isset($display['display_options'])) {
   

   Shouldn't this be !isset($display['display_options']['auth'])

2. +++ b/core/modules/rest/rest.install
   @@ -21,3 +21,27 @@ function rest_requirements($phase) {
   +}
   \ No newline at end of file
   

   Missing new line at end of file.

3. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
   @@ -250,6 +286,34 @@ public function optionsSummary(&$categories, &$options) {
   +        '#options' => $this->getAuthOptions(),
   

   We need to ensure the plugin provides a dependency to the view. So that the view has a dependency on the module that provides the authentication method. This will need tests.

4. +++ b/core/modules/rest/rest.install
   @@ -21,3 +21,27 @@ function rest_requirements($phase) {
   +/**
   +  * Re-save all views with a REST display to add new auth defaults.
   +  */
   +function rest_update_8202() {
   

   Weird number... here's the views.install example

   /**
    * @addtogroup updates-8.2.0
    * @{
    */
   
   /**
    * Rebuild the container to add a new container parameter.
    */
   function views_update_8200() {
     // Empty update to cause a cache rebuild so that the container is rebuilt.
   }
   
   /**
    * @} End of "addtogroup updates-8.2.0".
    */
   

   We need the group thing and I think the update number should be 8200. @lokapujya why did you think it should be 8202?

We need the group thing and I think the update number should be 8200. @lokapujya why did you think it should be 8202?

I just meant that it should be 82xx. #2721595: [PP-1] Simplify REST configuration is going to add 8201 and 8202. So, you are right that it could be 8200.

#147: Whichever lands first gets the lowest number. So this should use 8200, the next one should use 8201, etc.

For now I'm just using the functionality which will be in #2308745: Remove rest.settings.yml, use rest_resource config entities

Just a reroll basically :)

Let's fix the failures.

Wanted to RTBC, but found a few too many nits.

1. +++ b/core/modules/rest/rest.install
   @@ -37,8 +37,33 @@ function rest_update_8201() {
        ->save();
   +  ¶
   +}
   +/**
   

   Trailing spaces. And actually, an unnecessary blank line before the closing brace. And missing a blank line after the closing brace.

2. +++ b/core/modules/rest/rest.install
   @@ -37,8 +37,33 @@ function rest_update_8201() {
   +
   

   I don't think this is necessary either?

3. +++ b/core/modules/views/src/Tests/Update/RestExportAuthUpdateTest.php
   @@ -0,0 +1,36 @@
   + * @group Update
   

   Should also have @group views?

4. +++ b/core/modules/views/tests/fixtures/update/rest-export-with-authentication.php
   @@ -0,0 +1,71 @@
   +    'value' => 'i:8000;',
   ...
   +    'value' => 'i:8000;',
   ...
   +    'value' => 'i:8000;',
   ...
   +$extensions['module']['rest'] = 0;
   +$extensions['module']['serialization'] = 0;
   +$extensions['module']['basic_auth'] = 0;
   

   8000 vs 0. Does not make sense.

5. +++ b/core/modules/views/tests/fixtures/update/rest-export-with-authentication.php
   @@ -0,0 +1,71 @@
   +  'resources' => [
   +  ],
   

   This should be removed?

8000 vs 0. Does not make sense.

It actually does 8000 is the schema version and 0 is the weight.

This should be removed?

Let's drop it.

Should also have @group views?

We can just have one group statement at the moment, so let's with what the majority of files in that folder have (4 against 2)

I don't think this is necessary either?

Oh yeah totally not. I hate that git doesn't figure out those problems automatically.

It actually does 8000 is the schema version and 0 is the weight.

Oh… oops, lots of update tests then have this wrong!

This can now also be removed:

   /**
+   * The collector of authentication providers.
+   *
+   * @var \Drupal\Core\Authentication\AuthenticationCollectorInterface
+   */
+  protected $authenticationCollector;

But that's a trivial thing that can happen on commit.

P.S.: wow, I thought the "-155.patch" was a typo, but it is not — gasp!

Oh… oops, lots of update tests then have this wrong!

Yeah, but I guess just no code really cares about it.

P.S.: wow, I thought the "-155.patch" was a typo, but it is not — gasp!

It is Drupal ...

Random fail:

Field.Drupal\field\Tests\Update\FieldUpdateTest
✗	
testFieldUpdate8001
fail: [Completion check] Line 47 of core/modules/field/src/Tests/Update/FieldUpdateTest.php:
The test did not complete due to a fatal error.

Re-testing.

Let's get a change record for the new key / value for auth in the view config.

Just wrote the change record. @alexpott ... feel free to commit it.

Committed 5987069 and pushed to 8.2.x. Thanks!

diff --git a/core/modules/rest/src/Plugin/views/display/RestExport.php b/core/modules/rest/src/Plugin/views/display/RestExport.php
index 6464ded..cf5e232 100644
--- a/core/modules/rest/src/Plugin/views/display/RestExport.php
+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -15,7 +15,6 @@
 use Drupal\views\Plugin\views\display\PathPluginBase;
 use Symfony\Component\DependencyInjection\ContainerInterface;
 use Symfony\Component\Routing\RouteCollection;
-use Drupal\Core\Authentication\AuthenticationCollectorInterface;
 
 /**
  * The plugin that handles Data response callbacks for REST resources.
diff --git a/core/modules/views/tests/fixtures/update/rest-export-with-authentication.php b/core/modules/views/tests/fixtures/update/rest-export-with-authentication.php
index 9e9aa30..8215cdc 100644
--- a/core/modules/views/tests/fixtures/update/rest-export-with-authentication.php
+++ b/core/modules/views/tests/fixtures/update/rest-export-with-authentication.php
@@ -1,8 +1,14 @@
 <?php
 
+/**
+ * @file
+ * Test fixture for \Drupal\views\Tests\Update\RestExportAuthUpdateTest.
+ */
+
 use Drupal\Component\Serialization\Yaml;
+use Drupal\Core\Database\Database;
 
-$connection = Drupal\Core\Database\Database::getConnection();
+$connection = Database::getConnection();
 $config = $connection;
 
 // Set the schema version.

Fixes on commit.

I only noticed now, after this landed, that this patch only makes changes to rest module, yet then adds the upgrade path test to views module. That does not seem right?

Nice catch!
Right, feel free to create a notice task to move the update path tests to rest module :)

i.e. this even has rest_update_8202() in rest.install, but then \Drupal\views\Tests\Update\RestExportAuthUpdateTest to test it…

It's confusing when seeing rest_update_8202 and then not finding the corresponding upgrade path test.

Right, feel free to create a notice task to move the update path tests to rest module :)

+1, done: #2756527: Move test coverage for rest_update_8202() from core/modules/views to core/modules/rest.