Support OPTIONS request

This approach would only work for routes generated by REST module. And currently only for entities. That seems too narrow. We have all of the route data available. We should be able to just grab all the available routes at a path, check their legal methods, and return that. It should not be entity coupled.

That said, big +1 on supporting OPTIONS. It's just polite REST behavior.

Interesting! Do you have some paper/reference you can link to?

This approach would only work for routes generated by REST module. And currently only for entities. That seems too narrow. We have all of the route data available. We should be able to just grab all the available routes at a path, check their legal methods, and return that. It should not be entity coupled.

At least this example in code clearly shows that we need some logic, regarding access for example. Do you suggest to put that directly on top of the routing layer?

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/rest/resource/EntityResource.php
@@ -185,6 +185,32 @@ public function delete(EntityInterface $entity) {
+  public function options(EntityInterface $entity) {
+    if (!$entity->access('view')) {
+      throw new AccessDeniedHttpException();
+    }

I wonder whether we should also somehow expose that in views and just return GET ?

I don't think OPTIONS is user-access-sensitive. It's only whether or not there is code to respond to a given method. My understanding is that it's a shorthand for "so what's going to give me a non-405". Not "what's going to give me a 200". So relying on entity access would actually be wrong anyway.

I tried to find some supporting documentation for #4 but it's tricky to google. An argument in it's favor: CORS support brings in the "Access-Control-Max-Age" header as an option, which is a caching directive for OPTIONS responses (http://www.w3.org/TR/cors/#preflight-result-cache). Dynamic access control would probably mess with this cache.

Added links OPTIONS related docs.

Patch needed a reroll.

Enabling the OPTIONS for content then testing

$ curl --user admin:admin --header 'Accept: application/hal+json' --request OPTIONS http://drupal.d8/node/1 gives HTML output with an 'access denied'. Hal module was disabled.

curl --user admin:admin --header 'Accept: application/json' --request OPTIONS http://drupal.d8/node/1
{}

gives an empty json.

$ curl --user admin:admin --request OPTION http://drupal.d8/node/1
Method Not Allowed

I expected list of methods GET and authentication methods plus formats.

Hmmm ... my result comes from an exception.

Event dispatching takes core/lib/Drupal/Core/Routing/AccessAwareRouter.php before this core/modules/rest/src/Routing/OptionsRequestSubscriber.php (rest.options_subscriber)

There is iirc an issue about event ordering.

Event ordering is #2344645: The order of event subscribers should not be affected by definition order added to summary

Reading #2344645: The order of event subscribers should not be affected by definition order is about priority and this patch has a priority of -10000 which is in weight terms first but on priority last.

Changed that number to 1000 which gives at least a debug step into onKernelRequest with
curl --verbose --header 'Accept: application/json' --request OPTIONS http://drupal.d8/node/1?XDEBUG_SESSION_START=19350 but the request is not complete.

+++ b/core/modules/rest/src/Routing/OptionsRequestSubscriber.php
@@ -0,0 +1,102 @@
+  protected function getAllowedMethods(Request $request) {
+    $allow = array();
+    $route = $request->attributes->get(RouteObjectInterface::ROUTE_OBJECT);
+    if (isset($route)) {
+      foreach ($this->availableMethods as $method) {
+        $request->setMethod($method);
+        if ($this->accessManager->check($route, $request, $this->account)) {
+          $allow[] = $method;
+        }
+      }
+    }
+    return $allow;
+  }

This result keeps empty so we need some tunig for the priority number

array_keys($this->listeners['kernel.request'])

gives 9 values out of 12 listeners. The choosen '1000' in patch above is '200' and then '50' '35' '32' ... so what should it be ?!?

Fix missing schema. How could I miss that :-/

1. +++ b/core/modules/rest/src/Routing/OptionsRequestSubscriber.php
   @@ -0,0 +1,103 @@
   +/**
   + * Handles OPTIONS requests.
   + */
   +class OptionsRequestSubscriber implements EventSubscriberInterface {
   +
   

   It would be great to explain why we cannot implement this as ordinary route but need a request subscriber.

2. +++ b/core/modules/rest/src/Routing/OptionsRequestSubscriber.php
   @@ -0,0 +1,103 @@
   +    if ($request->getMethod() == 'OPTIONS') {
   

   you can use isMethod() here.

3. +++ b/core/modules/rest/src/Routing/OptionsRequestSubscriber.php
   @@ -0,0 +1,103 @@
   +      foreach ($this->availableMethods as $method) {
   

   I am confused, this property is not set anywhere.

#1 Added some documentation. Not sure whether that's enough. There is AFAIK no generic route as the OPTIONS methods matches any URI. See links to w3c in the summary.

#2 Done

#3 It's a class protected $availableMethods = array(

New issue:

When debugging with ie curl --verbose --header 'Accept: application/json' --request OPTIONS http://drupal.d8/node/1?XDEBUG_SESSION_START=16522

on a configured rest there still is no route found in

    $route = $request->attributes->get(RouteObjectInterface::ROUTE_OBJECT);

This needs a route expert.

+++ b/core/modules/rest/src/Routing/OptionsRequestSubscriber.php
@@ -18,6 +18,12 @@
  * Handles OPTIONS requests.
+ *
+ * Option request are allowed regardless of access permissions on
+ * the requested resource and can be requested on any path.
+ *
+ * For example requesting OPTIONS on /node/1 could result into list of methods
+ * depending permissions on other methods like GET, POST, PATCH, DELETE.
  */
 class OptionsRequestSubscriber implements EventSubscriberInterface {

Yeah this is somehow important information, maybe also link to the w3c page.

+ $events[KernelEvents::REQUEST][] = array('onKernelRequest', 1000);

In this case use priority < 32

@dawehner why that magic < 32 ? It should be one of the highest values as OPTIONS must run before access check.

I failed to find the Access :: onKernelRequest implementation. Guess I'm searching wrong :-/

Based on the list below I guess we need to be @ 32+/- 0.5 . Note OPTIONS does not have to do an access / permission check.

kernel.request
1000 rest.options_subscriber::onKernelRequest
200 path_subscriber::onKernelRequestConvertPath
50 ajax_subscriber::onRequest
50 ajax.subscriber::onKernelRequest
40 hal.subscriber::onKernelRequest
35 user_maintenance_mode_subscriber::onKernelRequestMaintenance
32 router_listener::onKernelRequest
31 route_content_controller_subscriber::onRequestDeriveFormat
30 route_content_controller_subscriber::onRequestDeriveContentWrapper
30 maintenance_mode_subscriber::onKernelRequestMaintenance
29 route_content_form_controller_subscriber::onRequestDeriveFormWrapper
0 router.route_preloader::onRequest
0 replica_database_ignore__subscriber::checkReplicaServer

Running

drush @drupal.d8 cache-rebuild
curl --verbose --header 'Accept: application/json' --request OPTIONS http://drupal.d8/node/1

gives either an empty Allow header field or {"error":"A fatal error occurred: No authentication credentials provided."}
depending on values above 33 (empty Allow) or 32 and below (error).

Adding authentication

curl --verbose --user admin:admin --header 'Accept: application/json' --request OPTIONS http://drupal.d8/node/1

gives an HTML exceptions response.

Recoverable fatal error: Argument 1 passed to Drupal\Core\Access\AccessManager::check() must implement interface Drupal\Core\Routing\RouteMatchInterface, instance of Symfony\Component\Routing\Route given, called in /Users/clemens/Sites/drupal/d8/www/core/modules/rest/src/Routing/OptionsRequestSubscriber.php on line 100 and defined in Drupal\Core\Access\AccessManager->check() (line 219 of core/lib/Drupal/Core/Access/AccessManager.php).

Drupal\Core\Access\AccessManager->check(Object, Object, Object)
Drupal\rest\Routing\OptionsRequestSubscriber->getAllowedMethods(Object)
Drupal\rest\Routing\OptionsRequestSubscriber->onKernelRequest(Object, 'kernel.request', Object)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.request', Object)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\PageCache->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1)
Stack\StackedHttpKernel->handle(Object, 1, 1)
Drupal\Core\DrupalKernel->handle(Object)

New patch
- adds puzzle to use the accept header but lacks to use this information. Mimetype is hardcoded to json. But that should use ie xml when requested. Not sure whether that is correct.
- lowers the priority from 1000 to 32 to see whether there are testbot failures too apart from mentioned exception.

Still learning about priorities. $ grep -r -A 2 " tags:" . | grep priority helps to find some but still no luck with level 35, 34 ... guess those are done through altering the prio?

Open puzzle is how to tweak the priority so it runs before access check. See Remaining tasks

Open puzzle is how to tweak the priority so it runs before access check. See Remaining tasks

The answer is, you cannot anymore. Access checking is hardcoded inside AccessAwareRouter.

Daniel, Kalpana, and I discussed this at BADCamp. We need to support all routes, not just rest.module's routes.

The way to do that is to have a request listener that fires before routing (probably right before). If the method is not OPTIONS, do nothing. If it is, then it looks up available routes in the RouteProvider, does a union of all of their legal methods, and returns a response listing those. Routing then never happens, controllers never happen, etc.

The RouteProvider will still only be called once, EITHER by this new listener or by normal routing. Never both. So the performance impact should be negligible. (It's just one more request listener that is usually a no-op.)

Can we get a status update for this? My understanding is that CORS requires preflight negotiation with the OPTIONS method and that this is not natively a part of core or the CORS bundle. I'm surprised there's no Issues tab on the github repo for the CORS module either. https://github.com/previousnext/drupal_cors

CORS seems like mere necessity for decoupled applications and I've yet to resolve this in Drupal 8.

Updated IS based on #29. This seems at least major, for the reasons outlined in #30.

Am I reading this right that drupal 8 doesn't support CORS requests? Thats... wow... That's a critical issue for the rest module isn't it?

Critical == "data loss, completely not working". CORS is not like that. The next priority level is "major". This issue is marked "major".

But I completely agree this is critical for REST. And that's also why this has the "rest-critical" issue tag. It's sad/unfortunate that D8's REST support doesn't yet have CORS. But this is open source, with people contributing in their free time.

If you'd like to help out with getting Drupal 8's REST to support CORS, that'd be great :) I'd be happy to provide reviews.

Looking into #29 now, yeah!

There we go.

Looks great :)

1. +++ b/core/core.services.yml
   @@ -1022,6 +1022,11 @@ services:
   +    class: Drupal\Core\EventSubscriber\OptionsRequestListener
   

   Why "listener" and not subscriber"?

2. +++ b/core/lib/Drupal/Core/EventSubscriber/OptionsRequestListener.php
   @@ -0,0 +1,71 @@
   +      // In case we don't have any routes, a 403 should be thrown by the normal
   +      // request handling.
   +      if (count($routes) > 0) {
   +        $methods = array_map(function (Route $route) {
   +          return $route->getMethods();
   +        }, $routes->all());
   +        // Flatten and unique the available methods.
   +        $methods = array_unique(call_user_func_array('array_merge', $methods));
   +        $response = new Response('', 200, ['Allow' => implode(', ', $methods)]);
   +        $event->setResponse($response);
   +      }
   

   Should this also do access checking on all those routes? That probably doesn't make sense because additional metadata may need to be sent with the request for access to be granted.

3. +++ b/core/lib/Drupal/Core/EventSubscriber/OptionsRequestListener.php
   @@ -0,0 +1,71 @@
   +    $events[KernelEvents::REQUEST][] = ['onRequest', 1000] ;
   

   Why 1000?

4. +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/OptionsRequestListenerTest.php
   @@ -0,0 +1,114 @@
   +  public function providerTestOnRequestWithOptionsRequest() {
   

   Nice :)

dawehner pointed out we allow GET + POST on almost any page which is done by #2019123: Use the same canonical URI paths as for HTML routes in \Drupal\Core\EventSubscriber\RouteMethodSubscriber::onRouteBuilding

[edit]WimLeers foundRelated[/edit] #2505339: Stop using getMainRequest() to build $form['#action'] as the offender for always GET + POST

It is weird to POST to a view until one realizes it could have filters etc. requiring a post.

curl --request OPTIONS --verbose "http://drupal.d8/node" 2>&1 | grep Allow 
< Allow: GET, POST

I've tested a few router.pattern_outlines of which node/%, user/% fail on node/add, user/login aka string argument variants of nid or uid.

curl --request OPTIONS --verbose "http://drupal.d8/node/add" 2>&1 | grep Allow
< Allow: GET, POST, DELETE, PATCH

curl --request OPTIONS --verbose "http://drupal.d8/user/login" 2>&1| grep Allow
< Allow: GET, POST, DELETE, PATCH

gives false results as one cannot DELETE, PATCH node/add or user/login. Similar goes for

curl --request OPTIONS --verbose "http://drupal.d8/rss.xml" 2>&1| grep Allow
< Allow: GET, POST
curl --request OPTIONS --verbose "http://drupal.d8/user/%/cancel/confirm" 2>&1| grep Allow
curl --request OPTIONS --verbose "http://drupal.d8/user/x/cancel/confirm" 2>&1| grep Allow 
< Allow: GET, POST
curl --request OPTIONS --verbose "http://drupal.d8/quickedit/entity/%/%" 2>&1| grep Allow
curl --request OPTIONS --verbose "http://drupal.d8/quickedit/entity/p/q" 2>&1| grep Allow 
< Allow: GET, POST

I guess this patch is good to go but building an API scan based on OPTIONS result is futile.

WimLeers found #2505339: Stop using getMasterRequest() to build $form['#action'] as the offender for always GET + POST

MH, I think we still need POST on most routes, otherwise people cannot just embed random forms in sidebars and what not.

I guess the reason for this strange behaviour is that /node/add is matched by /node/{node}, as long you don't apply the regex.

@WimLeers agreed : in #26 we had OptionsRequestListener (in a complete different namespace)

Should we anticipate in our tests for mentioned default POST result?

Attached patch contains the renaming.

Probably of topic aka ReST API related: I'm puzzled regarding _format. What if I configure POST only for hal+json? https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html#sec9.2 is unclear to me as they require a body when using 'Content-Type'

I didn't say #2505339: Stop using getMainRequest() to build $form['#action'] was the offender for GET+POST. That's a completely separate thing.

@Wim-leers: I've corrected in #40 ... sorry about that.

@dawehner I rather had the argument validated for a node/% rather then a match on node/add. But I guess we could RTBC this now right?

Regarding the naming, I couldn't care less. A listener is for me more about the concrete function which gets called, while a subscriber also specifies what it subscribes to. The RouterListener just came next in the core.services.yml file.

One remaining thing we could implement: Some form of configuration to enable options requests, but I guess given how fast they are responded there is no real a reason to make it toggleable.

@clemens.tolboom
I'm fine with some RTBC

RTBC as soon as Why 1000? is answered by a code comment.

Sure

+++ b/core/lib/Drupal/Core/EventSubscriber/OptionsRequestSubscriber.php
@@ -64,6 +64,7 @@ public function onRequest(GetResponseEvent $event) {
+    // Set a high priority so its executed before routing.

Ah, so that's the reason. Okay :)

Nit: s/its/it's/, can be fixed on commit.

Is this for 8.1.x or should it go for 8.2.x?

(+ a IS fix on curl command)

Well, ideally it is for both, but I fear at least CORS is certainly a feature so we missed the 8.1.x deadline.

This looks pretty much perfect, hope it gets in soon.

I'm +1 here as well, with the two on-commit-able tweaks. Thanks all!

This seems like a good candidate for 8.1.x-beta - it is new functionality this is isolated and should not break anything. Committed a4e5372 and pushed to 8.1.x and 8.2.x. Thanks!

diff --git a/core/lib/Drupal/Core/EventSubscriber/OptionsRequestSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/OptionsRequestSubscriber.php
index 622fe23..17ef946 100644
--- a/core/lib/Drupal/Core/EventSubscriber/OptionsRequestSubscriber.php
+++ b/core/lib/Drupal/Core/EventSubscriber/OptionsRequestSubscriber.php
@@ -23,6 +23,8 @@
 class OptionsRequestSubscriber implements EventSubscriberInterface {
 
   /**
+   * The route provider.
+   *
    * @var \Symfony\Cmf\Component\Routing\RouteProviderInterface
    */
   protected $routeProvider;
@@ -30,11 +32,11 @@ class OptionsRequestSubscriber implements EventSubscriberInterface {
   /**
    * Creates a new OptionsRequestSubscriber instance.
    *
-   * @param \Symfony\Cmf\Component\Routing\RouteProviderInterface $routeProvider
-   *   THe route provider.
+   * @param \Symfony\Cmf\Component\Routing\RouteProviderInterface $route_provider
+   *   The route provider.
    */
-  public function __construct(RouteProviderInterface $routeProvider) {
-    $this->routeProvider = $routeProvider;
+  public function __construct(RouteProviderInterface $route_provider) {
+    $this->routeProvider = $route_provider;
   }
 
   /**
@@ -64,8 +66,8 @@ public function onRequest(GetResponseEvent $event) {
    * {@inheritdoc}
    */
   public static function getSubscribedEvents() {
-    // Set a high priority so its executed before routing.
-    $events[KernelEvents::REQUEST][] = ['onRequest', 1000] ;
+    // Set a high priority so it is executed before routing.
+    $events[KernelEvents::REQUEST][] = ['onRequest', 1000];
     return $events;
   }
 
diff --git a/core/tests/Drupal/Tests/Core/EventSubscriber/OptionsRequestSubscriberTest.php b/core/tests/Drupal/Tests/Core/EventSubscriber/OptionsRequestSubscriberTest.php
index f1c6296..7c0e454 100644
--- a/core/tests/Drupal/Tests/Core/EventSubscriber/OptionsRequestSubscriberTest.php
+++ b/core/tests/Drupal/Tests/Core/EventSubscriber/OptionsRequestSubscriberTest.php
@@ -103,7 +103,7 @@ public function providerTestOnRequestWithOptionsRequest() {
           $collection->add('example.2', new Route('/example', [], [], [], '', [], [$method_a, $method_b]));
           $collection->add('example.3', new Route('/example', [], [], [], '', [], [$method_b, $method_c]));
           $methods = array_unique([$method_a, $method_b, $method_c]);
-          $data['multiple_routes_' . $method_a . '_' . $method_b . '_' . $method_c] = [$collection,  implode(', ', $methods)];
+          $data['multiple_routes_' . $method_a . '_' . $method_b . '_' . $method_c] = [$collection, implode(', ', $methods)];
         }
       }
     }

Nits fixed on commit.