REST views: authentication is broken

From what I can see, that test change would pass without the code change. This would need actual coverage to prove the bug.

Also, why doesn't this require a config schema change?

Okay so you're asserting that the resulting data structure is what you expect.
But this is tagged as a major bug.

What functionality is broken that fixes?
Let's test that!

You effectively wrote a unit test. I'm asking for an integration test.
Even if it's something clicking around the UI, if something is broken, we need to be able to expose that with a test to prevent regressions.

Just saying "the format was one way and now it's different" doesn't mean anything for a bug to be major.

To reproduce this, create a view with "rest_export" display type and try to set an authentication.

Then to test this, write a test that creates a view with "rest_export" display type and try to set an authentication.

That's unrelated. You would want to have a view that does not yet have an authorization/authentication, and then set one via the UI, thereby exposing the bug.

If you remove the bug fixes and run that test, it still passes. That means it's not actually testing anything.

The goal is to write a test that will fail, and then only pass once the fix is added. We ensure this by posting "test-only" patches alongside the full patch.

Just to make the title less scary

1. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
   @@ -227,7 +227,10 @@ public function getContentType() {
   +    // Variable contains conformity of authentication provider and module name.
   

   I have no idea what "variable contains conformity" means.

2. +++ b/core/modules/rest/src/Tests/Views/RestExportAuth.php
   @@ -0,0 +1,61 @@
   +   * Checks that correct authentication variants are available for choosing.
   

   s/variants/providers/

3. +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.rest_export_with_authorization.yml
   @@ -204,7 +204,7 @@ display:
          auth:
   -        basic_auth: basic_auth
   +        - basic_auth
   

   This looks logical. :)

Sorry for bumping it back, but after this it's definitely RTBC again! :)

do we need an upgrade path and/or upgrade path test, or am I to worries ?

swentel is asking whether we can provide an upgrade path that "fixes" broken configuration, I think.

I agree, let's fix what is possible. We deal with security here, so better be safe than sorry.

I think @dawehner meant this.

Yeah exactly :) Thank you @Wim Leers

An update path will ensure old configuration will work, yes.
An update path test is to ensure that the update path works.

This needs both.

Here is an update path + a corresponding test.

Note: This is not only a problem for REST but also for stuff like CSV exports.

Re-roll for 8.4.x.

Also, #39 didn't applied smoothly against 8.3.x.

Why do you have an interdiff for a reroll?

Well, then its obviously not a reroll ;)

Honestly, I don't get why you renamed files. This makes the interdiff harder to read than needed :(

Confirmed this problem, I've tested this manually by creating a view with user authentication and another using the contrib simple_oauth, both present the problem described (user defines a "cookie" authentication method and simple_oauth defines a "token_bearer" one).

After applying the patch on #41 and run the updates, for the user view, the problem is fixed, but for simple_oauth, the update is not performed.

I think this piece should be made more generic:

+  $process_auth = function ($auth_option) {
+    switch ($auth_option) {
+      case 'user':
+        return 'cookie';
+    }
+
+    return $auth_option;
+  };

Will this patch need to be backported to 8.3 and 8.2?

Did a plain re-roll of #39 for 8.4.x to try to facilitate the process.

Here are the changes from #41 with an interdiff after the reroll

And here's a proposal for solving the issue in #48 about contrib modules that might provide authentication services.

1. +++ b/core/modules/rest/rest.install
   @@ -92,5 +92,41 @@ function rest_update_8203() {
   +function rest_update_8204() {
   

   This now needs to be 8401, since this is going into Drupal 8.4.x.

2. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
   similarity index 72%
   rename from core/modules/rest/src/Tests/Update/RestExportAuthUpdateTest.php
   
   rename from core/modules/rest/src/Tests/Update/RestExportAuthUpdateTest.php
   rename to core/modules/rest/src/Tests/Update/RestExportAuthCorrectionUpdateTest.php
   

   Hm, why are we deleting the old update path test?

3. +++ b/core/modules/rest/src/Tests/Update/RestExportAuthCorrectionUpdateTest.php
   @@ -17,20 +17,20 @@ class RestExportAuthUpdateTest extends UpdatePathTestBase {
    
   

   Why are we deleting the old update path test? We still need that old update path test!

   It's fine to copy it, but we should also keep existing tests.

I was a bit lost on the deleted/moved tests, thanks @BR0kEN. I think comments on #53 are solved, moving this to RTBC.

Not sure what current policies are but we should maybe try to backport this to 8.3 and 8.2?

Back to RTBC

Thank you, that looks great!

I've read through this and I'm not entirely understanding how to tell if it is "fixed". I'm sure it is but some evidence might help a core committer to easily accept it. Can I suggest jumping up on stage at DevDays and asking for one of the 1st time contributors to run through it and create before and after screenshots etc?

If you can explain to them what the difference is and how to test, it might help improve the issue summary, too?

Cleaning tags :)

Yeah, lets try to provide screenshot after the fix.

+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -457,10 +461,12 @@ public function calculateDependencies() {
+      // During the update path the provider options might be wrong.
+      return isset($this->authenticationProviders[$provider])
+        ? $this->authenticationProviders[$provider]
+        : NULL;

This was introduced in #39, but not with a real explanation in the issue. The code comment also doesn't really help. Shouldn't calculateDependencies() only be called on save() at which point the option should have been fixed?

In my opinion this needs some further explanation at least. It seems as though this could leave config in a corrupt state due to missing dependencies, but I'm not sure.

This was introduced in #39, but not with a real explanation in the issue. The code comment also doesn't really help. Shouldn't calculateDependencies() only be called on  save() at which point the option should have been fixed?

It is indeed a good question. I never looked deep into it:

#1 /Users/dawehner/www/d8/core/modules/rest/src/Plugin/views/display/RestExport.php(466)
#2 [internal function]: Drupal\rest\Plugin\views\display\RestExport->Drupal\rest\Plugin\views\display\{closure}('user')
#3 /Users/dawehner/www/d8/core/modules/rest/src/Plugin/views/display/RestExport.php(470): array_map(Object(Closure), Array)
#4 /Users/dawehner/www/d8/core/lib/Drupal/Core/Plugin/PluginDependencyTrait.php(47): Drupal\rest\Plugin\views\display\RestExport->calculateDependencies()
#5 /Users/dawehner/www/d8/core/modules/views/src/Entity/View.php(283): Drupal\Core\Config\Entity\ConfigEntityBase->calculatePluginDependencies(Object(Drupal\rest\Plugin\views\display\RestExport))
#6 /Users/dawehner/www/d8/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php(346): Drupal\views\Entity\View->calculateDependencies()
#7 /Users/dawehner/www/d8/core/modules/views/src/Entity/View.php(293): Drupal\Core\Config\Entity\ConfigEntityBase->preSave(Object(Drupal\Core\Config\Entity\ConfigEntityStorage))
#8 /Users/dawehner/www/d8/core/lib/Drupal/Core/Entity/EntityStorageBase.php(434): Drupal\views\Entity\View->preSave(Object(Drupal\Core\Config\Entity\ConfigEntityStorage))
#9 /Users/dawehner/www/d8/core/lib/Drupal/Core/Entity/EntityStorageBase.php(389): Drupal\Core\Entity\EntityStorageBase->doPreSave(Object(Drupal\views\Entity\View))
#10 /Users/dawehner/www/d8/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php(259): Drupal\Core\Entity\EntityStorageBase->save(Object(Drupal\views\Entity\View))
#11 /Users/dawehner/www/d8/core/lib/Drupal/Core/Entity/Entity.php(364): Drupal\Core\Config\Entity\ConfigEntityStorage->save(Object(Drupal\views\Entity\View))
#12 /Users/dawehner/www/d8/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php(637): Drupal\Core\Entity\Entity->save()
#13 /Users/dawehner/www/d8/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php(195): Drupal\Core\Config\Entity\ConfigEntityBase->save()
#14 /Users/dawehner/www/d8/core/lib/Drupal/Core/Entity/EntityTypeEventSubscriberTrait.php(47): Drupal\views\EventSubscriber\ViewsEntitySchemaSubscriber->onEntityTypeUpdate(Object(Drupal\Core\Entity\ContentEntityType), Object(Drupal\Core\Entity\ContentEntityType))
#15 /Users/dawehner/www/d8/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php(111): Drupal\views\EventSubscriber\ViewsEntitySchemaSubscriber->onEntityTypeEvent(Object(Drupal\Core\Entity\EntityTypeEvent), 'entity_type.def...', Object(Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher))
#16 /Users/dawehner/www/d8/core/lib/Drupal/Core/Entity/EntityTypeListener.php(95): Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('entity_type.def...', Object(Drupal\Core\Entity\EntityTypeEvent))
#17 /Users/dawehner/www/d8/core/lib/Drupal/Core/Entity/EntityManager.php(393): Drupal\Core\Entity\EntityTypeListener->onEntityTypeUpdate(Object(Drupal\Core\Entity\ContentEntityType), Object(Drupal\Core\Entity\ContentEntityType))
#18 /Users/dawehner/www/d8/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManager.php(144): Drupal\Core\Entity\EntityManager->onEntityTypeUpdate(Object(Drupal\Core\Entity\ContentEntityType), Object(Drupal\Core\Entity\ContentEntityType))
#19 /Users/dawehner/www/d8/core/modules/block_content/block_content.install(72): Drupal\Core\Entity\EntityDefinitionUpdateManager->updateEntityType(Object(Drupal\Core\Entity\ContentEntityType))
#20 /Users/dawehner/www/d8/core/includes/update.inc(178): block_content_update_8300(Array)
#21 /Users/dawehner/www/d8/core/includes/batch.inc(252): update_do_one('block_content', 8300, Array, Array)
#22 /Users/dawehner/www/d8/core/includes/batch.inc(144): _batch_process(Array)
#23 /Users/dawehner/www/d8/core/includes/batch.inc(72): _batch_progress_page()
#24 /Users/dawehner/www/d8/core/modules/system/src/Controller/DbUpdateController.php(186): _batch_page(Object(Symfony\Component\HttpFoundation\Request))
#25 [internal function]: Drupal\system\Controller\DbUpdateController->handle('start', Object(Symfony\Component\HttpFoundation\Request))
#26 /Users/dawehner/www/d8/core/lib/Drupal/Core/Update/UpdateKernel.php(110): call_user_func_array(Array, Array)
#27 /Users/dawehner/www/d8/core/lib/Drupal/Core/Update/UpdateKernel.php(73): Drupal\Core\Update\UpdateKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request))
#28 /Users/dawehner/www/d8/update.php(28): Drupal\Core\Update\UpdateKernel->handle(Object(Symfony\Component\HttpFoundation\Request))
#29 {main}

Tl;DR

block_content_8300</block updates entity definitions, which triggers views to clean up its tables potentially: <code>\Drupal\views\EventSubscriber\ViewsEntitySchemaSubscriber::onEntityTypeUpdate resaves all the views. I think the current patch is alright ... you can never guarantee that not some other update functions run before your one.

To be honest I would have not expected view entities to be saved when entity schema changed, that for itself seems a bit fishy, but its how it is right now.

Regarding the screenshots, I hope they were not really required :)

you can never guarantee that not some other update functions run before your one.

But we have hook_update_dependencies() just for that. And I think it would be better to implement that and maybe even write a change notice if other modules are doing something similar, than to potentially have corrupt config? I'm still not 100% sure that you actually do get corrupt config, but if that is the case, I think we should try to avoid it if at all possible. AFAICT @alexpott was also very adament about this over in #2468045: When deleting a content type field, users do not realize the related View also is deleted.

But we have hook_update_dependencies() just for that.

This hook is really about just your local scoped .install file. You cannot know which all update hooks out there might run before yours.

Hmm... still not 100% sold, but let's see what committers think.

Meh, wanted to do this.

Discussed with @catch - we think we need to fix this using hook_ENTITY_TYPE_presave() so that default views supplied by contrib and custom modules can be updated too. This means the hook_update_N() needs to be moved to post update and just determine which views need to be saved again and save them.

While I totally agree that we should fix this problem, I'm not 100% convinced that this should block committing this patch, because, well, every module which ships with such a REST view at the moment is essentially broken, aka. has a bug. These views would have never worked for anyone, so these modules, from my point of view, in case anyone tested this piece of functionality, would have not shipped with the particular view in the first place.

This bug was reported again at #2874418: REST views: Authentication Error when using Simple OAuth authentication.

All other "REST views" issues are in the rest.module component, are tagged VDC, and all their titles start with REST views. Let's do the same here.

12:16:12 <WimLeers> alexpott: you un-RTBC'd a major REST views authentication bug: https://www.drupal.org/node/2825204#comment-12041776. dawehner replied, disagreeing. A reply to his comment is what's blocking further progress, it'd be splendid if you could post your thoughts :)
12:16:15 <drupalbot> https://www.drupal.org/node/2825204 => Views REST authentication is broken #2825204: REST views: authentication is broken => 77 comments, 1 IRC mention
12:17:23 <dawehner> alexpott: so yeah I would totally argue with some level of pragmatism here
12:18:09 <alexpott> dawehner, WimLeers: this doesn't feel fair - we've required this on other views fixes of this nature
12:18:40 <alexpott> dawehner, WimLeers: And the code is basically written - it's rest_update_8401()
12:23:04 <WimLeers> alexpott: dawehner I have no strong opinion or insight into updates to View entities. I worked on that sort of thing once or twice. I just care about REST stuff working correctly. I defer to dawehner.
12:23:47 <dawehner> alexpott: I just try to avoid executing too much random stuff on page load, as well, its like the update order problem, but even worse
12:24:38 <alexpott> dawehner: but this is on view save no - not page load
12:26:46 <dawehner> alexpott: oh you are right
13:22:24 <WimLeers> Looks like we can move forward again here  then :)

:)

I was sure we would need something on page load level, but its actually just save time. I'll ensure we have that as well as a test.

Here is a fix together with a test.

1. +++ b/core/modules/rest/rest.module
   @@ -27,3 +28,28 @@ function rest_help($route_name, RouteMatchInterface $route_match) {
   +/**
   + * Implements hook_view_presave().
   + */
   +function rest_view_presave(ViewEntityInterface $view) {
   

   Nit: Let's add
   @see rest_update_8401()

   to the docblock?

2. +++ b/core/modules/rest/rest.module
   @@ -27,3 +28,28 @@ function rest_help($route_name, RouteMatchInterface $route_match) {
   +  $auth_providers = \Drupal::service('authentication_collector')->getSortedProviders();
   +  $process_auth = function ($auth_option) use ($auth_providers) {
   +    foreach ($auth_providers as $provider_id => $provider_data) {
   +      // The provider belongs to the module that declares it as a service.
   +      if (strtok($provider_data->_serviceId, '.') === $auth_option) {
   +        return $provider_id;
   +      }
   +    }
   +
   +    return $auth_option;
   +  };
   +
   +  foreach (array_keys($view->get('display')) as $display_id) {
   +    $display = &$view->getDisplay($display_id);
   +    if ($display['display_plugin'] === 'rest_export' && !empty($display['display_options']['auth'])) {
   +      $display['display_options']['auth'] = array_map($process_auth, $display['display_options']['auth']);
   +    }
   +  }
   

   I was going to say why duplicate all this from rest_update_8401()?, but then I realized there's a very good reason: update functions cannot depend on any other code. And other code cannot depend on update functions.

   So +1 for this duplication.

Thank you for your instant review @Wim Leers!

I forgot to mention that I found #2877981: \Drupal\rest\Tests\Views\RestExportAuth is missing the Test prefix along the way.

Nit: Let's add
@see rest_update_8401()

to the docblock?

Sure, why not.

+++ b/core/modules/rest/src/Tests/Views/RestExportAuth.php
@@ -0,0 +1,61 @@
+class RestExportAuth extends ViewTestBase {

Ehm this is an entirely new test… so let's fix it here instead of having the follow-up #2877981: \Drupal\rest\Tests\Views\RestExportAuth is missing the Test prefix :)

tAuth extends ViewTestBase {
Ehm this is an entirely new test… so let's fix it here instead of having the follow-up #2877981: \Drupal\rest\Tests\Views\RestExportAuth is missing the Test prefix :)

Oh nice, point! We can also ensure to not extend from the deprecated base class anymore.

Random fail in Drupal\aggregator\Tests\AggregatorCronTest.

1. +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.rest_export_with_authorization.yml
   @@ -204,7 +204,7 @@ display:
   -        basic_auth: basic_auth
   +        - basic_auth
   

   This is just clean-up AFAICT.

2. +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.rest_export_with_authorization_correction.yml
   @@ -204,7 +201,7 @@ display:
          auth:
   -        basic_auth: basic_auth
   +        - user
   

   Does user mean cookie?

   This is confusing.

@Wim Leers
There are test configurations to ensure that the update path is working.

1. +function rest_update_8401() {
   +function rest_view_presave(ViewEntityInterface $view) {
   

   There's duplicated code between these functions. Is there a reason not to do rest_update_8401() as a post_update instead that just resaves the views without duplicating the logic that's in the presave()? Similar to views_post_update_revision_metadata_fields()?

2. +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.rest_export_with_authorization.yml
   @@ -204,7 +204,7 @@ display:
   -        basic_auth: basic_auth
   +        - basic_auth
   

   The IS screenshot for how this is currently stored in config is different than what this line is in HEAD. Is the screenshot outdated, or is this test file incorrect in HEAD?

I'm asking #92.2, because if what's in HEAD already contains all the information we need (unlike the claim that's in the IS), then what's the motivation for changing the schema? Even if doing so simplifies the schema, seems like that should be a cleanup task that's separate from what's needed to fix the bug.

#91:

1. As a rule, hook_update_N() implementations cannot share/reuse any code with the "regular" codebase. Because they must forever continue to work in the same way. So this is correct.
2. The IS provides a screenshot of some View config entity YML. As you can clearly see in the screenshot, because it includes path: frontpage/list/articles, which certainly is nowhere in core, not even in tests. The code you're citing is a test that we're updating.
   I agree it's confusing though, that's why I asked about this in #90.1 too. But note that views.rest.schema specifies this is a sequence. Which means you can use arbitrary keys, they're ignored anyway. AFAICT this was just a small bug/bit of roughness that was introduced in #2228141: Add authentication support to REST views.

#92: this is not changing the schema. Config/config schemas are less strict than you'd think. Any map in config is also a sequence… I've been confused by this in the past as well.
However, I agree it's a cleanup task that does not belong here, because clearly we both find it super confusing.

So: NW for #93/#92.2/#90.1.

As a rule, hook_update_N() implementations cannot share/reuse any code with the "regular" codebase. Because they must forever continue to work in the same way. So this is correct.

I totally agree, code duplication is not as evil as we pretend it to be, especially when they are responsible for different part of the system (update vs. import).

The IS screenshot for how this is currently stored in config is different than what this line is in HEAD. Is the screenshot outdated, or is this test file incorrect in HEAD?

However, I agree it's a cleanup task that does not belong here, because clearly we both find it super confusing.

I made a
So we should certainly have an actual example in our tests, and well

      display_extenders: {  }
      auth:
        - user
      path: test-path

is how an actual real life example looks like. Using the module name here is the actual problem of the issue, so we should certainly have this as part of the test itself. We can drop the change for the basic_auth:basic_auth test case, so let's do that.

wow, this space matters :)

:D

#95 reverted the changes that confused @effulgentsia in #92.2 (and me in #90.1). Except a space was accidentally still being deleted, which was fixed in #97. Now, zero changes remain in core/modules/views/tests/modules/views_test_config/test_views/views.view.rest_export_with_authorization.yml, therefore removing an unnecessary/distracting change, and also proving that we're not changing the test coverage to make the tests pass.

A few points (sorry! forgot about this issue), nice work on test coverage by the way!

1. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
   @@ -227,7 +227,11 @@ public function getContentType() {
   +    $providers = array_keys($this->authenticationProviders);
   

   I wonder if this should be done in the constructor when we set it to the property?

2. +++ b/core/modules/rest/tests/src/Functional/Views/RestExportAuthTest.php
   @@ -0,0 +1,61 @@
   +  public function setUp($import_test_views = TRUE) {
   

   This can be removed.

3. +++ b/core/modules/rest/tests/src/Functional/Views/RestExportAuthTest.php
   @@ -0,0 +1,61 @@
   +    // Do not import test view.
   +    parent::setUp();
   

   This comment seems wrong, $import_test_views will default to TRUE in the parent constructor too. So test views are being imported? :)

4. +++ b/core/modules/rest/tests/src/Functional/Views/RestExportAuthTest.php
   @@ -0,0 +1,61 @@
   +    $this->drupalGet("admin/structure/views/nojs/display/$view_id/$view_display/auth");
   

   Shouldn't we test a POSTing of the form here too (after this hunk), then we can check the config options that were set from that?

5. +++ b/core/modules/rest/tests/src/Kernel/Views/RestExportAuthTest.php
   @@ -0,0 +1,60 @@
   +  protected function setUp($import_test_views = TRUE) {
   

   This param can be removed.

6. +++ b/core/modules/rest/tests/src/Kernel/Views/RestExportAuthTest.php
   @@ -0,0 +1,60 @@
   +    parent::setUp($import_test_views);
   

   We don't need to pass this in.

This bug was reported again: #2909757: RestExport views display authentication providers.

#101
1. Done
2. Note: You cannot remove parameters in a subclass if the parent class has them ...
3. He, fair
4. Good idea, let's do that.
5. See 2.
6. While we don't have to, removing the parameter is not possible so meh.

Thanks!

Ah, right. Missed/forgot that in ViewsKernelTestBase. There is some whitespace in the latest patch, I think this can be fixed on commit though.

Let's not the core committers bother with it

Sorry, that was the wrong patch.

+1

This probably no longer applies because both #2846614: Incorrect field name is used in views integration for multi-value base fields and #2449143: REST views specify HTML as a possible request format, so if there is a "regular" HTML view on the same path, it will serve JSON landed in the past 24 hours, this is a 3rd major Views bug affecting API-First.

There was only one conflict, with #2449143: REST views specify HTML as a possible request format, so if there is a "regular" HTML view on the same path, it will serve JSON:

$ git d
diff --cc core/modules/rest/src/Plugin/views/display/RestExport.php
index 81e0b7d,44d5e40..0000000
--- a/core/modules/rest/src/Plugin/views/display/RestExport.php
+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@@ -124,8 -115,10 +124,15 @@@ class RestExport extends PathPluginBas
      parent::__construct($configuration, $plugin_id, $plugin_definition, $route_provider, $state);
  
      $this->renderer = $renderer;
++<<<<<<< ours
 +    $this->authenticationProviders = $authentication_providers;
 +    $this->formatProviders = $serializer_format_providers;
++=======
+ 
+     // Values of "$this->authenticationProviders" - are module names, defining
+     // authentication providers. Only provider IDs should be used for choosing.
+     $this->authenticationProviders = array_keys($authentication_providers);
++>>>>>>> theirs
    }
  
    /**
wim.leers at acquia in ~/Work/d8 on 8.5.x*

Was trivial to resolve.

crediting reviewers

Fixed on commit

diff --git a/core/modules/rest/tests/src/Kernel/Views/RestExportAuthTest.php b/core/modules/rest/tests/src/Kernel/Views/RestExportAuthTest.php
index bb884f1..9d32f7f 100644
--- a/core/modules/rest/tests/src/Kernel/Views/RestExportAuthTest.php
+++ b/core/modules/rest/tests/src/Kernel/Views/RestExportAuthTest.php
@@ -3,7 +3,6 @@
 namespace Drupal\Tests\rest\Kernel\Views;
 
 use Drupal\Tests\views\Kernel\ViewsKernelTestBase;
-use Drupal\views\Tests\ViewTestData;
 
 /**
  * Tests the auth option of rest exports.

Committed as 40b143f and pushed to 8.5.x.

#111 was rolled to adjust the patch for a change committed to 8.5. The patch in #108 still cleanly applies to 8.4.

I've essentially reverted the change in #111 to get this patch.

Thanks -- I agree it would be much better to use the actual names of the authentication providers. Also glad to see that thought was given to fixing the config dependency calculation based on this as that was my first concern reading this issue.

1. +++ b/core/modules/rest/rest.module
   @@ -27,3 +28,30 @@ function rest_help($route_name, RouteMatchInterface $route_match) {
   +  // Fix the auth options on import, much like what rest_update_8401 does.
   

   Nit: this should have parens.

2. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
   @@ -115,7 +115,9 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
   +    // Values of "$this->authenticationProviders" - are module names, defining
   +    // authentication providers. Only provider IDs should be used for choosing.
   +    $this->authenticationProviders = array_keys($authentication_providers);
   

   This comment is a little weird. I think we can fix it by removing the hyphen (or is it an emdash? :P) and comma. Also it's not usual to use "choosing" without an object. I can't really parse out what it means to suggest an improvement, unfortunately. Is this true?

   $authentication_providers is an array of module names for modules that provide each authentication method,
   keyed by the authentication method.
   Use the method names,
   not the module names,
   in the $authenticationProviders property.

   But it seems really confusing to have the parameter and the property with almost-the-same-name but totally different values. Can we instead change or deprecate one or the other?

   Maybe it would be good to reference where the structure of authenticationProviders if it's documented elsewhere?

3. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
   @@ -457,10 +459,14 @@ public function calculateDependencies() {
   +    $dependencies['module'] = array_merge($dependencies['module'], array_filter(array_map(function ($provider) {
   +      // During the update path the provider options might be wrong. This can
   +      // happen when any update function, like block_update_8300() triggers a
   +      // view to be saved.
   +      return isset($this->authenticationProviders[$provider])
   +        ? $this->authenticationProviders[$provider]
   +        : NULL;
   +    }, $this->getOption('auth'))));
   

   This is similarly opaque. Can we make this clearer? I.e. "Previously... Now..." This might be easier if we do indeed change and deprecate the name of the property as I suggest above, rather than changing what the string values are.

...Hmmm just saw that this has already been committed to 8.5.x. Followup or revert? I do think point 2 above (edit: about the property and parameter having inverted values) is worth considering.

I'm inclined to say follow-up, please, but whichever you prefer is fine.

(I noticed point 2 too, but figured it wasn't worth it in the grand scheme of things — this is a major bug breaking a significant use case. Obviously I should've insisted on making the comments clearer.)

...Hmmm just saw that this has already been committed to 8.5.x. Followup or revert? I do think point 2 above (edit: about the property and parameter having inverted values) is worth considering.

I'm confused, for me a follow up makes much more sense. At least for me fixing the bug is super important, so iterating on top of that makes sense for me at least.

Feel free to disagree, but I opened up #2917527: Address post commit review from xjm in #2825204

👍

Per #118, back to RTBC.

The follow up is now ready to be reviewed.

The patch doesn't fix this: https://www.drupal.org/node/2874418

simple_oauth doesn't work with REST views export. Only basic_auth works fine.

I stand corrected, applying the patch fixes also: https://www.drupal.org/node/2874418

@adrianpintilie thanks for testing and confirming this also fixes #2874418: REST views: Authentication Error when using Simple OAuth authentication! (Which is already a related issue since May, see #77.)

I'm confused, for me a follow up makes much more sense. At least for me fixing the bug is super important, so iterating on top of that makes sense for me at least.

But now we're in a situation where both issues are in the RTBC queue, committed to different branches, and reviews are fragmented across two issues. Plus there's an internal BC break in this one that the other fixes, which is one thing for 8.5.x but quite another for backporting it. So let's revert and reroll it together.

Patches for both branches, with:

• The use statement from #114 cleaned up
• #2917527: Address post commit review from xjm in #2825204 included
• A comment over 80 chars from #2917527: Address post commit review from xjm in #2825204 rewrapped.

And then just one other thing to check:

+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -87,7 +87,17 @@ class RestExport extends PathPluginBase implements ResponseDisplayPluginInterfac
+   * The authentication providers, like 'cookie' and 'basic_auth'.

I think this is "Authentication provider modules, keyed by provider ID." ? Edit: It's the deprecated property's description.

Interdiffs.

Manually diffed #111 against @xjm's 8.5 patch and #115 against @xjm's 8.4 patch. Looks good.

RTBC++

But now we're in a situation where both issues are in the RTBC queue, committed to different branches, and reviews are fragmented across two issues. Plus there's an internal BC break in this one that the other fixes, which is one thing for 8.5.x but quite another for backporting it. So let's revert and reroll it together.

Fair, a review is easy. I just want to ensure we don't forget about this issue again for a couple of months :)

Attached addresses my last point from #127. (Because the docs for the two different properties were identical aside from the deprecation notice, which was also pretty confusing.) Setting NR to double-check the example.

:( d.o ate my patches.

I would have personally used an @see to avoid duplication, but I also don't care about deprecated docs that much :)

:( d.o ate my patches.

I'm glad someone ensures that d.o. is healthy on some level.

#132's improved description is correct, but there's a new typo:

+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -94,7 +94,13 @@ class RestExport extends PathPluginBase implements ResponseDisplayPluginInterfac
+   * Aunthentication providers like 'cookie' and 'basic_auth' are the array

Aunthentication 😀

I'm glad someone ensures that d.o. is healthy on some level.

🤣🤣🤣

Now envisioning a children's book about Aunt Hen and Uncle Rooster.

Alright, since all I did was combine two patches and add some docs, and especially since @larowlan already signed off and committed this once, I'm comfortable doing so again.

So, committed to 8.5.x and 8.4.x both. I also fixed "Aunthenitcation" on commit. Thanks everyone!

Drupal, the only system which ships with Aunthenitcation as well as Unclethenitcation.

I was able to apply this patch and get it working at my end, but it resurfaced for me. I wasn't able to fix it, even after clearing caches and re-uploading the patch, so I had to do a complete site restore. I figured out the trigger - this seemed to be happening after disabling caching and enabling twig debugging in settings.php and services.yml respectively. Even reverting those changes doesn't fix the problem, though - i still see 'user' instead of 'cookie'. I restored the site yet again and, this time, I'm leaving debugging and caching alone.

#140: It sounds like you perhaps did not run update.php?

#141: I did try update.php as well as rebuild.php, but it had no effect. That's why I had to restore a whole site backup.

I'd like to add that I only updated the rest module in the live 8.4.0 installation with the one from 8.4.x-dev. Maybe the issue will resolve itself when I upgrade to the next 8.4 release.

I'd like to add that I only updated the rest module in the live 8.4.0 installation with the one from 8.4.x-dev. Maybe the issue will resolve itself when I upgrade to the next 8.4 release.

😨 … that is guaranteed to cause problems, and most definitely unsupported. You must update all of Drupal, not copy/paste parts from newer versions. That's equivalent to hacking core!

#143: I like to live dangerously! :D

Thanks for your assistance. I'm now upgrading to 8.4.1!

Hah :D