Can't authenticate with REST Export [#2808583]

When fetching the array of authentication methods to populate the checkbox options the parameter "authentication_providers" from the container is returned in getAuthOptions() with array_combine. This alters array keys and causes provider_id's to not match what you would expect, causing 403 Forbidden on all requests to rest export routes with authentication enabled.

Example

With array_combine this is the returned value from getAuthOptions

array (size=3)
  'basic_auth' => string 'basic_auth' (length=10)
  'simple_oauth' => string 'simple_oauth' (length=12)
  'user' => string 'user' (length=4)

Without array_combine this is the returned value. As you can see the key for simple_oauth now matches the provider id specified in the simple_oauth.services.yml. This is also true with cookie/user auth

array (size=3)
  'basic_auth' => string 'basic_auth' (length=10)
  'token_bearer' => string 'simple_oauth' (length=12)
  'cookie' => string 'user' (length=4)

Then when AuthenticationManager->defaultFilter() is called and checks for the provider_id in the routes auth options they match and the views result is returned.

in_array($provider_id, $route->getOption('_auth'));

I cannot see any need for the array_combine so will submit a patch to remove it unless someone knows otherwise.

Comment	File	Size	Author
#6	restexport_authentication_options-2808583-6.patch	811 bytes	james.williams
#6	8.3.x: PHP 5.6 & MySQL 5.5 21,442 pass
#2	cant_authenticate_with_rest_export-2808583-1.patch	607 bytes	Robytfc
#2	8.2.x: PHP 5.5 & MySQL 5.5 20,960 pass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

29 September 2016 at 10:29

Robytfc created an issue. See original summary.

Comment #2

Robytfc CreditAttribution: Robytfc commented 29 September 2016 at 10:31

File	Size
cant_authenticate_with_rest_export-2808583-1.patch	607 bytes
8.2.x: PHP 5.5 & MySQL 5.5 20,960 pass

Comment #3

Robytfc CreditAttribution: Robytfc commented 29 September 2016 at 15:01

Status:

Active

» Needs review

Comment #4

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 1 December 2016 at 17:59

Version:	8.2.0-rc2	» 8.2.x-dev
Issue tags:		+Needs tests, +VDC

Thanks! Sorry that you didn't get feedback sooner, this fell off my radar.

I'm afraid this looks wrong, because:

  /**
   * Gets the auth options available.
   *
   * @return string[]
   *   An array to use as value for "#options" in the form element.
   */
  public function getAuthOptions() {
    return array_combine($this->authenticationProviders, $this->authenticationProviders);
  }

i.e. because An array to use as value for "#options" in the form element.. In that case, the keys are the values used in Form API, and the values are the labels presented to the user. Hence the use of array_combine() does make sense to me.

Comment #5

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 1 December 2016 at 18:00

Status:

Needs review

» Needs work

Comment #6

james.williams CreditAttribution: james.williams at ComputerMinds commented 24 January 2017 at 16:13

Version:

8.2.x-dev

» 8.3.x-dev

File	Size
restexport_authentication_options-2808583-6.patch	811 bytes
8.3.x: PHP 5.6 & MySQL 5.5 21,442 pass

The keys should indeed be kept untouched, which avoiding array_combine() achieves. However, labels for each option are needed, which aren't anywhere in the codebase at the moment. There are no annotations for authentication providers to provide a human-readable label, nor methods to provide them instead. So although values for the #options array are needed, at the moment even the keys are wrong, and labels would need adding somewhere new. I'm not sure of the best place for them?

Perhaps a new label() method needs adding to AuthenticationProviderInterface, or to preserve backwards-compatibilty, a new LabelledAuthenticationProviderInterface would be needed? This starts to get messy.

I suggest the best start-point would be to fix the keys, since otherwise this just doesn't work, plain and simple. Then a separate follow-up ticket could proceed with the architectural change to add labels. Here's my suggested patch to get the keys correct, and have a more useful label that does not require architectural change, based on the pattern used on the Field list report (/admin/reports/fields). This may even suffice rather than having any truly human-readable labels maybe?