Problem/Motivation

While working on #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method, I also worked on ensuring that every format is also tested with every authentication provider, including cookie. But when using cookie, the user.login.http route would not ever accept the hal_json format, despite it automatically supporting every format. See #2737719-85: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method.

Turns out the reason is a tiny error in \Drupal\serialization\EventSubscriber\UserRouteAlterSubscriber::onRoutingAlterAddFormats()'s logic:

$formats = array_unique($formats + $this->serializerFormats);

should be

$formats = array_unique(array_merge($formats, $this->serializerFormats));

Because array_unique(['json'] + ['hal_json', 'json', 'xml']) === ['json', 'xml'], and array_unique(array_merge(['json'], ['hal_json', 'json', 'xml'])) === ['hal_json', 'json', 'xml']!

Note that it could very well be another format that is being ignored, it just depends on which format is listed first in $this->serializerFormats (which is the serializer.formats container parameter).

Proposed resolution

Fix the logic.

Remaining tasks

TBD

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#9 interdiff.txt1.58 KBlhangea
#9 2820888-add_hal_json_login_support-9.patch2.41 KBlhangea
#2 2820888-2.patch975 bytesWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Active » Needs review
FileSize
2820888-2.patch975 bytes

This is the fix. Still needs test coverage.

almaudoh’s picture

almaudoh CreditAttribution: almaudoh commented

:o Interesting bug array_unique(['json'] + ['hal_json', 'json', 'xml']) === ['json', 'xml'], I had to check again to confirm. I wonder how many places in core that bug exists?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

We're fairly good about only using array + array when it's an associative array. But yes, it's definitely happened before :)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

This indeed is quite a clusterfuck in php. There is this excellent phpstorm plugin: https://plugins.jetbrains.com/plugin/7622?pr=phpStorm which shows you those kind of problems.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: The user.login.http route never supports the 'hal_json' format or some other format, depending on module order » Cookie authentication: the user.login.http route never supports the 'hal_json' format or some other format, depending on module order
damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Not sure why \Drupal\serialization\EventSubscriber\UserRouteAlterSubscriber was added to serialization module to be honest. Looks like #2403307: RPC endpoints for user authentication: log in, check login status, log out added it under the guise of the REST module...

lhangea’s picture

lhangea CreditAttribution: lhangea commented
Assigned: Unassigned » lhangea
Status: Needs review » Needs work
lhangea’s picture

lhangea CreditAttribution: lhangea commented
Assigned: lhangea » Unassigned
Status: Needs work » Needs review
FileSize
2820888-add_hal_json_login_support-9.patch2.41 KB
interdiff.txt1.58 KB

We need to close this one in order to be able to close #2800873: Add XML GET REST test coverage, work around XML encoder quirks

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

@damiankloip: because it's the Serialization module which handles the "registered formats in the system". So once that module is installed, more formats become available for the User module's RPC routes too. Oh, but I see what you mean now, it still actually belongs in the user module… You're right! However, that really belongs in a separate issue. Want to file one?

#9: thanks! That looks perfect :)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

I just ran the test locally to prove it actually fails, and well, indeed it doesn.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Thanks Wim. I'll file an issue :)

I would say this needs a unit test for the event subscriber to be honest. Not sure how it got I without one ;)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed bf124f3 to 8.3.x and 29658ab to 8.2.x. Thanks!

  • alexpott committed bc7d4f1 on 8.3.x 
    Issue #2820888 by lhangea, Wim Leers: Cookie authentication: the user....

  • alexpott committed 29658ab on 8.2.x 
    Issue #2820888 by lhangea, Wim Leers: Cookie authentication: the user....
damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

That event subscriber should still have a unit test IMO. If it had that to start with this bug wouldn't have snuck in.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

@damiankloip: UserLoginHttpTest::testLogin() is a functional test. I think that's sufficient here. It's much more valuable to test the integration rather than the unit. The logic isn't complex. There aren't many edge cases. What matters most, is that all intended formats are supported. That's what the functional test ensures.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Not saying we should not have a functional test, but those kind of bugs are exactly why you have unit test coverage, no matter how simple the class you're testing is.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

You're right. But unit tests come with a cost too: we need to write, maintain and run them. All I'm saying is that I think in this case the value doesn't outweigh the cost. I think that in this case, the functional test coverage is sufficient.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

fair enough :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#2849474: \Drupal\Tests\rest\Functional\CookieResourceTestTrait::initAuthentication() should use the current test format, and should not send an Accept header

This forgot to also update \Drupal\Tests\rest\Functional\CookieResourceTestTrait::initAuthentication():

    // @todo Remove hardcoded use of the 'json' format, and use static::$format
    // + static::$mimeType instead in https://www.drupal.org/node/2820888.
    $user_login_url = Url::fromRoute('user.login.http')
      ->setRouteParameter('_format', 'json');

Opened #2849474: \Drupal\Tests\rest\Functional\CookieResourceTestTrait::initAuthentication() should use the current test format, and should not send an Accept header to fix that.