Problem/Motivation

The parent issue #2856713: Authentication plugins and HTTP authentication has improved a lot the authentication mecanism.

We need to cover it in our tests:

  1. For each authentication plugins, add a test on a private file on authentication test to be sure to cover also the case of a HTTP client and not only a JSONAPI HTTP client
  2. Add a test on Oauth plugin
  3. For each authentication plugins with optional Key module integration, need to test it. So add the Key module in require-dev section in composer.json
  4. Also add the Key module in the suggest section.
CommentFileSizeAuthor
#12 entity_share-Tests_improve_authentication_plugins_tests-3167418-12.patch47.88 KBGrimreaper
#10 interdiff-3167418_9_10.txt22.22 KBivan.vujovic
#10 entity_share-Tests_improve_authentication_plugins_tests-3167418_10.patch46.08 KBivan.vujovic
#9 interdiff-3167418_6_9.txt10.64 KBivan.vujovic
#9 entity_share-Tests_improve_authentication_plugins_tests-3167418_9.patch44.09 KBivan.vujovic
#6 interdiff-3167418_5_6.txt27.92 KBivan.vujovic
#6 entity_share-Tests_improve_authentication_plugins_tests-3167418_6.patch45.33 KBivan.vujovic
#5 entity_share-Tests_improve_authentication_plugins_tests-3167418.patch23.08 KBivan.vujovic
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Grimreaper created an issue. See original summary.

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper at Smile commented
Issue summary: View changes
Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper at Smile commented
Title: Tests: Add test for the oauth authentication plugin » Tests: improve authentication plugins tests
Issue summary: View changes
ivan.vujovic’s picture

ivan.vujovic
Primary language Serbo-Croatian
CreditAttribution: ivan.vujovic at Smile commented
Assigned: Unassigned » ivan.vujovic
ivan.vujovic’s picture

ivan.vujovic
Primary language Serbo-Croatian
CreditAttribution: ivan.vujovic at Smile commented
Status: Active » Needs work
FileSize
entity_share-Tests_improve_authentication_plugins_tests-3167418.patch23.08 KB

Hello,
I am posting an incomplete (but functional) patch which does the following:

  • Creates a separate test class for Anonymous authorization plugin.
  • In this test, the fetch of a private file is tested.
  • Creates a separate test class for Basic auth authorization plugin.
  • In the latter, tests are performed as user with full access to nodes, then as user with no access to unpublished nodes.
  • Also in Basic auth test, credentials are first used from local State, and then using the Key module.

Still to do:

  • Create the test for Oauth plugin with the same design as the Basic auth test.
  • Add check for private file download in Basic auth and Oauth tests.
  • Refactor.
ivan.vujovic’s picture

ivan.vujovic
Primary language Serbo-Croatian
CreditAttribution: ivan.vujovic at Smile commented
FileSize
entity_share-Tests_improve_authentication_plugins_tests-3167418_6.patch45.33 KB
interdiff-3167418_5_6.txt27.92 KB

Hello, again I am uploading an incomplete patch, but this time the only remaining thing is testing of private file downloads.

So, the interdiff contains the testing of OAuth plugin.

There seems to be a problem with library League\OAuth2 and its method Client\Provider\AbstractProvider::getResponse() because it is calling GuzzleHTTP client's method send() without possibility to add $options parameter. Without altering default $options['allow_redirects'] of GuzzleHTTP request from within the test, Guzzle redirects again to the same URL (in our case it is /oauth/token) as GET, which causes a Route exception. However, this doesn't happen when the HTTP request to /oauth/token is issued from the back-office (submitting the form).
I couldn't determine why this happens, but apparently in case of test, response headers from /oauth/token are different from the response from the back-office call: we have 'Location', content type is 'text/html' and not 'application/json'. I saw that core REST and JSON:API tests always alter $options['allow_redirects'] so I did the same in my override.
One more thing is that I am not very satisfied with the way I overrode this library class, perhaps someone can give me some advice on it. But again, ideally the library class shouldn't be overridden at all and the root cause of the different behaviour in test vs. back-office context should be understood and fixed.

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper at Smile commented

Hello @ivan.vujovic,

Thanks a lot for your work!

I will begin to give a look, to see if I can give you hint on your blocking points.

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper at Smile commented

@ivan.vujovic, Really great work so far!!!

Nice usage of traits. Nice slicing of methods. etc.

Here is my review:

  1. +++ b/composer.json
@@ -14,12 +14,14 @@
+        "drupal/simple_oauth": "Allows to setup server for OAuth plugin.",

    simple_oauth also needs to be added in require-dev. 

  2. +++ b/composer.json
@@ -14,12 +14,14 @@
+        "drupal/key": "Provides ability to manage keys for Authentication plugins"

    Add a dot at the end. 

  3. +++ b/modules/entity_share_client/tests/src/Functional/AuthenticationBasicAuthTest.php
@@ -0,0 +1,237 @@
+    // to authenticate.

    Nice to have a comment, but I think it needs a little completion. Because I am thinking why the channel user could not have been used. 

  4. +++ b/modules/entity_share_client/tests/src/Functional/AuthenticationOAuthTest.php
@@ -0,0 +1,434 @@
+    $request_options = [
+      RequestOptions::HTTP_ERRORS => FALSE,
+      RequestOptions::ALLOW_REDIRECTS => [
+        'strict' => TRUE,
+      ],
+    ];

    Ok, I guess this is for that that you had to introduce the TestableGenericProvider.

    As we can control Guzzle settings with the settings.php and the client factory service.

    I had given a look at core tests implicating a $settings in settings.php. And it seems that you only have to create a new Settings object.

    Here are some examples that I found:
    - app/core/tests/Drupal/Tests/Core/Http/ClientFactoryTest.php
    - app/core/tests/Drupal/Tests/Core/Database/UrlConversionTest.php
    - app/modules/contrib/redis/tests/src/Functional/WebTest.php

    I think that way you can avoid the testableGeneric provider and simplify the code.

    Also as Authentication tests are in separated classes let's take that as an advantage :) 

  5. +++ b/modules/entity_share_client/tests/src/Functional/EntityShareClientFunctionalTestBase.php
@@ -281,30 +282,50 @@ abstract class EntityShareClientFunctionalTestBase extends BrowserTestBase {
+    // By default, create "Basic Auth" plugin for autorization.

    Small typo: authorization.

ivan.vujovic’s picture

ivan.vujovic
Primary language Serbo-Croatian
CreditAttribution: ivan.vujovic at Smile commented
FileSize
entity_share-Tests_improve_authentication_plugins_tests-3167418_9.patch44.09 KB
interdiff-3167418_6_9.txt10.64 KB

Hello @grimreaper,

Many thanks for your advice on using Settings to override GuzzleHTTP client options!

I fixed the issues from your previous comment.

ivan.vujovic’s picture

ivan.vujovic
Primary language Serbo-Croatian
CreditAttribution: ivan.vujovic at Smile commented
Assigned: ivan.vujovic » Grimreaper
Status: Needs work » Needs review
FileSize
entity_share-Tests_improve_authentication_plugins_tests-3167418_10.patch46.08 KB
interdiff-3167418_9_10.txt22.22 KB

Hello again,

I added the private file test and refactored the test classes so that they all inherit an abstract base class.

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper at Smile commented

Hello,

Thanks @ivan.vujovic for your very good work!

  1. +++ b/modules/entity_share_client/src/Plugin/ClientAuthorization/Oauth.php
@@ -284,25 +271,23 @@ class Oauth extends ClientAuthorizationPluginBase {
+   * @return \Drupal\entity_share_client\ClientAuthorization\TestableGenericProvider

    PHP Doc to update. 

  2. +++ b/modules/entity_share_client/tests/src/Functional/AuthenticationBasicAuthTest.php
@@ -0,0 +1,212 @@
+    // to authenticate. We first test as administative user because they have

    Small typo: administrative

2 small fix that I will handle. So it will be the occasion to post a new patch to retest against recent merge.

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper at Smile commented
FileSize
entity_share-Tests_improve_authentication_plugins_tests-3167418-12.patch47.88 KB 
+++ b/composer.json
@@ -14,12 +14,15 @@
+        "drupal/key": "^1.14",

~1.14 to be similar to the other require-dev Drupal modules.

  • ivan.vujovic authored cf4024d on 8.x-3.x 
    Issue #3167418 by ivan.vujovic, Grimreaper: Tests: improve...
Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper at Smile commented
Assigned: Grimreaper » Unassigned
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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