Problem/Motivation

While working on #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method, I encountered 403 responses when I expected to get 406 responses. Let me quote #2737719-38: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method, which is where this is being reported:

The struggle is because of the weird interaction/design of the routing + authentication system, which causes things to fail miserably in an unfriendly way when the user forgets to specify ?_format=nonsense, or has a typo (for example ?_format=haljson on a URL that already has a HTML route.

It's very very very confusing how Symfony's routing system first grants access because a user is authenticated via basic auth, and so AccessAwareRouter says everything is okay, and then afterwards, during AuthenticationSubscriber::onKenrelRequestFilterProvider(), we choose to deny access after all because basic_auth isn't a globally allowed access provider, and the matched route (which matches the HTML, non-REST route due to the missing ?format=) is lacking an _auth route option. You can either get a 406 (which is what we want), or a 403 with HTML (in case of non-specified or invalid format) or a 403 in the expected format (in case of non-specified or invalid format plus Accept header, even though we don't actually support Accept headers… thanks to DefaultExceptionSubscriber calling Request::getAcceptableContentTypes()).
Worse, it's even possible to get the appropriate 406 response in case of an invalid format, but for that response to be sent with Content-Type: text/html! (In case of anonymous.)

More edge cases than I can count.

Seems like the authentication system and routing system are integrated in a brittle, confusing, backward manner. And the content negotiation system makes it that tiny bit extra unpredictable. This is no one's fault. This is complex software with (too) many layers, and without comprehensive integration tests, this sort of thing is to be expected.

Proposed resolution

TBD

Remaining tasks

  1. Determine root cause.
  2. Propose possible solutions
  3. TBD

User interface changes

None.

API changes

TBD, hopefully none.

Data model changes

TBD, hopefully none.

CommentFileSizeAuthor
#24 2805279-24.patch7.7 KBwim leers
#24 interdiff.txt821 byteswim leers
#23 2805279-23.patch7.7 KBwim leers
#18 2805279-18-interdiff.txt3.43 KBcburschka
#18 2805279-18.patch7.74 KBcburschka
#15 2805279-15.patch4.31 KBcburschka
#12 2805279-12.patch4.29 KBdawehner
#7 2805279-7-test-only-FAIL.patch3.55 KBwim leers
#6 2805279-6.patch3.2 KBdawehner

Comments

Wim Leers created an issue. See original summary.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

This is the pretty crazy assertion that is called 4 times by EntityResourceTestBase:

  /**
   * Asserts a 406 response… or in some cases a 403 response, because weirdness.
   *
   * Asserting a 406 response should be easy, but it's not, due to bugs.
   *
   * Drupal returns a 403 response instead of a 406 response when:
   * - there is a canonical route, i.e. one that serves HTML
   * - unless the user is logged in with any non-global authentication provider,
   *   because then they tried to access a route that requires the user to be
   *   authenticated, but they used an authentication provider that is only
   *   accepted for specific routes, and HTML routes never have such specific
   *   authentication providers specified. (By default, only 'cookie' is a
   *   global authentication provider.)
   *
   * @todo Remove this in https://www.drupal.org/node/2805279.
   *
   * @param \Psr\Http\Message\ResponseInterface $response
   *   The response to assert.
   */
  protected function assert406Response(ResponseInterface $response) {
    if ($this->entity->hasLinkTemplate('canonical') && ($this->account && static::$auth !== 'cookie')) {
      $this->assertSame(403, $response->getStatusCode());
    }
    else {
      // This is the desired response.
      $this->assertSame(406, $response->getStatusCode());
    }
  }

It was added in #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method.

That's the assertion that this issue should remove.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2817727: Add test coverage to prove controller is called *after* authentication validation

Somewhat related: #2817727: Add test coverage to prove controller is called *after* authentication validation.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new 2805279-6.patch3.2 KB

I started to experiment with something like this, but its certainly not a proper solution yet.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new 2805279-7-test-only-FAIL.patch3.55 KB

Thanks for getting this started!

I'll provide a patch from the other direction: per #3, this is the test simplification we should be able to make, which should fail against HEAD, and which the patch we develop here should allow to pass tests.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Version: 8.4.x-dev » 8.5.x-dev
dawehner’s picture

dawehner
Primary language German
commented

Yeah, collaboration: 🎆

The last submitted patch, 6: 2805279-6.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 7: 2805279-7-test-only-FAIL.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new 2805279-12.patch4.29 KB

#6 had a missing file ...

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review

And #12 was never queued for testing :P Very high probability this no longer applies.

Status: Needs review » Needs work

The last submitted patch, 12: 2805279-12.patch, failed testing. View results

cburschka’s picture

cburschka
they
commented
Status: Needs work » Needs review
StatusFileSize
new 2805279-15.patch4.31 KB

Conflict resolved...

Status: Needs review » Needs work

The last submitted patch, 15: 2805279-15.patch, failed testing. View results

cburschka’s picture

cburschka
they
commented

It looks like all of the test failures result from the test class still expecting 403 instead of 406, so that should be fine once it is updated.

cburschka’s picture

cburschka
they
commented
Status: Needs work » Needs review
StatusFileSize
new 2805279-18.patch7.74 KB
new 2805279-18-interdiff.txt3.43 KB

Removing the @todo-marked function and replacing the calls.

Status: Needs review » Needs work

The last submitted patch, 18: 2805279-18.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Thanks for pushing this forward, @cburschka!

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2854543: NegotiationMiddleware calls $request->setRequestFormat('html') when there is no _format request parameter, but shouldn't

See #2854543-28: NegotiationMiddleware calls $request->setRequestFormat('html') when there is no _format request parameter, but shouldn't and #2854543-30: NegotiationMiddleware calls $request->setRequestFormat('html') when there is no _format request parameter, but shouldn't — that issue is starting to cross over into the territory/scope of this issue.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new 2805279-23.patch7.7 KB

#2854543: NegotiationMiddleware calls $request->setRequestFormat('html') when there is no _format request parameter, but shouldn't landed. Let's see what happens if we run tests again the latest patch (#18) from almost 7 months ago…

This needed to be rebased, because #2854543: NegotiationMiddleware calls $request->setRequestFormat('html') when there is no _format request parameter, but shouldn't shifted some of the test assertions around. diffstat is nearly identical.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new interdiff.txt821 bytes
new 2805279-24.patch7.7 KB

It seems this change is necessary in the assertions for tests to pass.

The last submitted patch, 23: 2805279-23.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 24: 2805279-24.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

g-brodiei’s picture

g-brodiei
Primary language Chinese, Traditional
commented
Issue tags: +Needs steps to reproduce, +Needs issue summary update, +Bug Smash Initiative

Adding new tags for steps to reproduce and requires summary update for new contributors to step in easier.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.