Problem/Motivation

Often development/staging sites are protected with HTTP Basic authentication to not have sites publicly available on the internet. It also uses the "Authorization" HTTP header, so there is a clash between JWT and Basic Auth using the same header.

We want to protect our decoupled dev sites!

Proposed resolution

Add support for a second, different header name for development, for example "JWT-Authorization".

Remaining tasks

Change code, write tests.

API changes

None.

Release notes snippet

todo?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hideaway created an issue. See original summary.

hideaway’s picture

Crafted a patch to allow to set custom auth header, which is then checked against JWT auth

klausi’s picture

Status: Active » Needs review
  1. +++ b/jwt.services.yml
    @@ -1,11 +1,20 @@
    +    class: Drupal\jwt\Authentication\Header\JwtAuthHeader
    

    feels a bit over engineered, why do we need this as service? I think the others can just use the jwt.config service as parameter, then we don't need the extra JwtAuthHeader class.

    A class just for a default value?

  2. +++ b/src/Form/ConfigForm.php
    @@ -168,6 +176,10 @@ class ConfigForm extends ConfigFormBase {
    +      $this->config('jwt.config')->set('auth_header', $values['jwt_auth_header'])->save();
    

    so we also need to update the config schema?

    Oh, looks like the config schema is missing in the module?

    Oh, looks like the module doesn't have tests, that's probably why noone noticed the missing config schema so far? Found #2920046: Schema file is missing and install config file is wrong.

I think we should also write the first test case for this module to check if using a different auth header works. Leaving at needs review for other input.

klausi’s picture

Issue summary: View changes
Issue tags: +Needs tests
hideaway’s picture

Idea was that the auth header checking is done in multiple places (three places, two different classes). Wanted to have some wrapper which makes sure the proper auth header is checked. So when the header checking is required no config checking is required as the service already contains proper header.

But anyway can of course simplify this.

hideaway’s picture

Removed that mediator class.

hideaway’s picture

Here is the patch with the browser test covering the feature of setting up custom authorization header. This already includes the code changes from https://www.drupal.org/node/2920046 where config schema is added, because without config schema the tests cannot be run.

lebster’s picture

Guys, I have tested the patch and it is working well for me.

I had an issue with the oauth due to enable Basic Auth and JWT. I can obtain a token, but I can't be authorized with that token. I have applied the patch, changed the JWT authorization header and now the oauth working well.

klausi’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests

Thanks, some minor coding standard issues that should be fixed:

  1. +++ b/src/Authentication/Provider/JwtAuth.php
    @@ -34,13 +43,6 @@ class JwtAuth implements AuthenticationProviderInterface {
    -  /**
    -   * The event dispatcher.
    -   *
    -   * @var \Symfony\Component\EventDispatcher\EventDispatcherInterface
    -   */
    -  protected $eventDispatcher;
    

    this should not be removed? we are still using the event dispatcher?

  2. +++ b/tests/src/Functional/AuthHeaderTest.php
    @@ -0,0 +1,138 @@
    + * @group field
    

    this is not a field test?

  3. +++ b/tests/src/Functional/AuthHeaderTest.php
    @@ -0,0 +1,138 @@
    +   * Tests the user is JWT authorized.
    

    this is an assert, not a test.

  4. +++ b/tests/src/Functional/AuthHeaderTest.php
    @@ -0,0 +1,138 @@
    +   * @param $uid
    

    data type missing, should be int? Please add data types to all param docs https://www.drupal.org/docs/develop/standards/api-documentation-and-comm...

  5. +++ b/tests/src/Functional/AuthHeaderTest.php
    @@ -0,0 +1,138 @@
    +   * Tests the user is not JWT authorized.
    

    this is an assert, not a test, so should start with "Asserts that..."

Once this is ready again this is blocked on #2920046: Schema file is missing and install config file is wrong.

hideaway’s picture

FileSize
14.6 KB

Attaching the patch fixing the remarks. On top of that I fixed all the Code style complains, since I realized the Drupal CI won't run the tests when there are code style issues.

hideaway’s picture

Forgot patch file :)

hideaway’s picture

Status: Needs work » Needs review
klausi’s picture

Status: Needs review » Needs work
  1. +++ b/jwt.install
    @@ -1,8 +1,13 @@
     /**
    - *  * Implements hook_requirements().
    - *   */
    + * @file
    + * Install, update and uninstall functions for the JWT module.
    + */
    

    makes sense, but we should not mix random coding standard fixes into this patch.

    Only our code changes should follow coding standards, otherwise this patch gets too large.

    Please remove all changes that are not related to this issue.

  2. +++ b/jwt.links.menu.yml
    @@ -4,4 +4,3 @@ jwt.jwt_config_form:
       weight: 99
    -
    

    this for example.

hideaway’s picture

Status: Needs work » Needs review
FileSize
15.6 KB
3.47 KB

Removed all unrelated code style fixes.

klausi’s picture

Status: Needs review » Postponed
hideaway’s picture

Patch reroll for beta-1.

klausi’s picture

Status: Postponed » Needs work
  1. +++ b/tests/src/Functional/AuthHeaderTest.php
    @@ -0,0 +1,138 @@
    +   * @param $uid
    

    data types are missing for many parameters, can you add those?

  2. +++ b/tests/src/Functional/AuthHeaderTest.php
    @@ -0,0 +1,138 @@
    +  public function testAuthHeaders() {
    

    The actual test method should be further up after setUp() I think? Then it is easier to find the actual test case in this class.

It also looks like the changes to ConfigForm are missing?

We should enable testbot for this project, then we would see that this patch fails testing because it even tests the admin form!

klausi’s picture

hideaway’s picture

@klausi addressed remarks, here is the latest patch

hideaway’s picture

Status: Needs work » Needs review
hideaway’s picture

FileSize
9.46 KB

patch - one last time, test was still failing

hideaway’s picture

reordered test file to have a test case at the top, and annotated types

hideaway’s picture

Version: 8.x-1.0-alpha6 » 8.x-1.0-beta1
klausi’s picture

Status: Needs review » Needs work

Looks good, some minor issues:

  1. +++ b/config/install/jwt.config.yml
    @@ -0,0 +1 @@
    +auth_header: Authorization
    

    do we have to make this new config required?

    Can we just use "Authorization" as default in JwtAuth if the config is empty?

  2. +++ /dev/null
    @@ -1,2 +0,0 @@
    -algorithm: HS256
    -key_id: jwt_test_hmac
    

    why do we delete this file?

  3. +++ b/tests/src/Functional/AuthHeaderTest.php
    @@ -0,0 +1,145 @@
    + * @group field
    

    should be JWT?

greggles’s picture

Version: 8.x-1.0-beta1 » 8.x-1.x-dev

Just bumping to 8.x-1.x since that's where development is happening.

pwolanin’s picture

Title: Allow to set custom authorization header » Support setting custom authorization header
pwolanin’s picture

This post suggests reasons why this may not be a good idea (custom headers may be ignored, cached, or stripped).
https://stackoverflow.com/questions/42574022/custom-authorization-header

pwolanin’s picture

I wonder if a better approach (Assuming the basic auth still works with e.g. shield module) might be to look through an array of Authorization headers like:

  protected function getJwtFromRequest(Request $request) {
    $auth_headers = $request->headers->get('Authorization', NULL, FALSE);
    foreach ($auth_headers as $value) {
      if (preg_match('/^Bearer (.+)/', $value, $matches)) {
        return $matches[1];
      }
    }
    return FALSE;
  }

pwolanin’s picture

klausi’s picture

Our use case for the custom Authorization header is development and staging sites, so caching or proxies should not be an issue.

pwolanin’s picture

A better way to implement this given the use case would be to have an (optional?) fall-back header? e..g if there is a non-matching Authorization header, check the X-Authorization-JWT header instead?

pwolanin’s picture

Title: Support setting custom authorization header » Fallback header for use when basic authorization header is also sent
FileSize
1.66 KB

Needs test, but something like this?

pwolanin’s picture

Status: Needs work » Needs review
FileSize
6.33 KB

At least some minimal test coverage. Changed the service priority so it will be checked before basic_auth in case that's also enabled (should add Functional test for that).

pwolanin’s picture

+ Functional test, need to check priority over basic_auth still

pwolanin’s picture

Plus the last bit of test coverage.

klausi’s picture

Looks good to me, I think a standard fallback header works for us.

We should not use "X-Authorization-JWT" because the "X-" prefix should not be used for new API according to https://tools.ietf.org/html/rfc6648

So we can just use "Authorization-JWT".

Otherwise the change looks good and also the test coverage.

pwolanin’s picture

From that RFC, looks like the more correct option would be: 'JWT-Authorization'

Re-rolled with that instead.

Also added a README.

pwolanin’s picture

Issue summary: View changes
Status: Needs review » Fixed

  • pwolanin committed 6d663b3 on 8.x-1.x
    Issue #3032702 by hideaway, pwolanin, klausi: Fallback header for use...
Anas_maw’s picture

For some reasons, this commit causes an issue for me, I'm getting the following error: "Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException: The "paragraph" entity type did not specify a translation handler."

and this error is not caused by paragraph module as you can see here: #3031598-32: The "paragraph" entity type did not specify a translation handler.

Seems like this commit causes a bad cache build.

pwolanin’s picture

@Anas_maw does a cache rebuild resolve the problem? Please open a new issue to discuss. Perhaps this needs a post-update to force a cache rebuild.

Anas_maw’s picture

Status: Fixed » Closed (fixed)

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