supported_formats and supported_auth should work in the same way

So the proposal that we came up on our last meeting:

* Make supported_formats and supported_auth explicit: every method on every resource has to specify both.
* No default behavior: if either supported_formats or supported_auth is missing the config is broken and we throw errors.
* We ship a default YAML file with HAL enabled as format and basic_auth enabled as authentication.

YAML config before:

# Enable all methods on nodes.
resources:
  entity:node:
    # what formats are possible on GET ??
    GET: { }
    # what authentication provider can I use on POST ??
    POST: { }
    PATCH: { }
    DELETE: { }

YAML config after:

# Enable all methods on nodes.
resources:
  entity:node:
    GET:
      supported_formats:
        - hal_json
      supported_auth:
        - http_basic
    POST:
      supported_formats:
        - hal_json
      supported_auth:
        - http_basic
    PATCH:
      supported_formats:
        - hal_json
      supported_auth:
        - http_basic
    DELETE:
      supported_formats:
        - hal_json
      supported_auth:
        - http_basic

The downside of this is that as soon as you have new formats or authentication providers you also have to update this config file, not wildcard possible anymore. But that will happen rarely.

Note that this also brings more consistency: enabled methods like GET, POST etc. also have to be explicitly specified and we have no wildcard there to allow all methods on a resource. So all configuration options in this YAML file work the same, work explicitly.

Here is a first pass at it. I have not reviewed tests yet.

Should we use watchdog() to register an error when a route is not added or silently ignore it?

Here is an improved version where I am verifying the resource structure.

While I let the testbot run all tests, I am about to review REST ones.

Re-rolling

Submitting to the test bot.

Here is another go at it. I am only making supported_formats available when there is a format requirement in the route.

Here I made both supported_auth and supported_formats mandatory. Error messages are logged if any of these is empty or not an array.

1. +++ b/core/modules/rest/lib/Drupal/rest/EventSubscriber/RouteSubscriber.php
   @@ -66,23 +66,28 @@ public function dynamicRoutes(RouteBuildEvent $event) {
   +          // Check that formats are defined
   

   Punctuation missing at the end.

2. +++ b/core/modules/rest/lib/Drupal/rest/EventSubscriber/RouteSubscriber.php
   @@ -66,23 +66,28 @@ public function dynamicRoutes(RouteBuildEvent $event) {
   +          // Add the resource.
   

   Vague comment, should be something like "The configuration seems legit at this point, so we set the authentication provider and add the route."

3. +++ b/core/modules/rest/lib/Drupal/rest/Tests/RESTTestBase.php
   @@ -33,6 +33,7 @@ protected function setUp() {
   +    $this->defaultAuth = array('cookie');
        // Create a test content type for node testing.
   

   The class property is not defined?

Otherwise this looks pretty good.

Thanks! Here it is.

Discussed documentation of rest.settings.yml with @klausi. Here it is.

Reviewed config with @klausi. Here it is.

Forgot to remove a phrase.

What could I do without @klausi's surgeon eye? Nothing mate. Nothing.

I'm happy if the testbot is.

We might not need all those watchdog() calls, see #2107687: Provide configuration schema for Rest module. Anyone familiar with the config schema system? Could we use that for config validation?

#18: drupal-rest-explicit-formats-auth-2065193-18.patch queued for re-testing.

If tests pass now, I would rather commit this as it is and create a follow up so when #2107687: Provide configuration schema for Rest module gets in, we implement configuration schema validation. Looks like a fantastic way to validate resources.

Unfortunately I have been told that configuration schema validation does not exist (yet?), so will have to leave this in place anyway.

The other issue got resolved, so this is still good to go.

Patch no longer applies.

Rerolled. I also changed http_basic to basic_auth in the configuration because of #2107483: Make basic_auth module and http_basic auth setting consistent.

REST module doesn't depend on basic_auth, so why enabling that by default for HAL? Also with the logic change doesn't this mean cookie auth is disabled when only basic_auth is specified, or am I reading wrong?

This is going to fall foul of #2090115: Don't install a module when its default configuration has unmet dependencies because the configuration depends on an optional module - which is fine but just flagging that this is the case.

REST module doesn't depend on basic_auth, so why enabling that by default for HAL?

I do not see any relationship between the question and this patch.

Also with the logic change doesn't this mean cookie auth is disabled when only basic_auth is specified, or am I reading wrong?

Yes, if a REST resource just supports basic_auth, then the generated route will only accept this authentication system.

This is going to fall foul of #2090115: Don't install configuration when it has a soft-dependency on a module that's not installed because the configuration depends on an optional module - which is fine but just flagging that this is the case.

The sample configuration does not enable hal module, only defines REST resources. If hal module is not enabled, some of them (the ones for POST and PATCH methods) will simply not work.

If hal module is not enabled, some of them (the ones for POST and PATCH methods) will simply not work.

Right this is what I mean - is that a problem? Or will they just work/not work automatically when the module is installed/uninstalled?

If HAL module is not available then the routes will not be exposed, so requests for nodes will not work. If basic_auth module is not enabled then only requests that are allowed for anonymous users will work.

We think that HAL and basic_auth are the most useful defaults for REST module users, while REST module does not necessarily depend on them.

So we should extend the comment at the beginning of the config file that people must enable HAL module for this to work and basic_auth if they want privileged access.

If we're going to advise people to enable basic auth, we should make sure to mention SSL for it, too. That comment is a good place for it. (And probably mention that other options are probably better; basic auth is in core to make sure we can do that sort of thing, not because basic auth is good. )

Here it is.

Is it kosher to reference contrib modules from core documentation specifically? If so, I'm fine with #43.

1. +++ b/core/modules/rest/config/rest.settings.yml
   @@ -1,4 +1,9 @@
   +# You must enable hal and basic_auth modules for this to work. Also, if you are
   

   Module names in comments should be capitalised

2. +++ b/core/modules/rest/config/rest.settings.yml
   @@ -1,4 +1,9 @@
   +# An alternative to basic_auth out of Drupal core is OAuth module:
   +# https://drupal.org/project/oauth.
   

   How about? An alternative to Basic_auth in contrib is OAuth module https://drupal.org/project/oauth

3. +++ b/core/modules/rest/lib/Drupal/rest/EventSubscriber/RouteSubscriber.php
   @@ -60,23 +60,29 @@ protected function routes(RouteCollection $collection) {
   +          // Check that authentication providers are defined.
   +          if (empty($enabled_methods[$method]['supported_auth']) || !is_array($enabled_methods[$method]['supported_auth'])) {
   +            watchdog('rest', 'At least one authentication provider must be defined for resource @id', array(':id' => $id), WATCHDOG_ERROR);
                continue;
              }
   ...
   +          // Check that formats are defined.
   +          if (empty($enabled_methods[$method]['supported_formats']) || !is_array($enabled_methods[$method]['supported_formats'])) {
   +            watchdog('rest', 'At least one format must be defined for resource @id', array(':id' => $id), WATCHDOG_ERROR);
   +            continue;
              }
   

   Do we have test coverage for this?

Addressed the three suggestions given by @alexpott.

This is ready for review.w

Let's do.

Sorry I thought I'd followed up on this, but apparently I never did.

I don't think we should link directly to the oauth module - there might be a different oauth module that's better supported in three years and little chance the comment will get updated. Can reference oauth and then let google find the URL.

Otherwise this looks good so hopefully quick re-roll.

Re-rolled since #2099439: REST's CSRF check is triggered even when using Basic Auth got committed and then removed link to OAuth module.

Reasonable.

Committed 43a4ef2 and pushed to 8.x. Thanks!

Writing change notification.

Done https://drupal.org/node/2147705