Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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?
Comment | File | Size | Author |
---|---|---|---|
#37 | 3032702-36-increment.txt | 4.49 KB | pwolanin |
#37 | 3032702-36.patch | 15.83 KB | pwolanin |
| |||
#35 | 3032702-35-increment.txt | 2.75 KB | pwolanin |
#35 | 3032702-35.patch | 14.39 KB | pwolanin |
| |||
#34 | 3032702-34.patch | 12.85 KB | pwolanin |
|
Comments
Comment #2
hideaway CreditAttribution: hideaway commentedCrafted a patch to allow to set custom auth header, which is then checked against JWT auth
Comment #3
klausifeels 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?
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.
Comment #4
klausiComment #5
hideaway CreditAttribution: hideaway commentedIdea 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.
Comment #6
hideaway CreditAttribution: hideaway commentedRemoved that mediator class.
Comment #7
hideaway CreditAttribution: hideaway commentedHere 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.
Comment #8
lebster CreditAttribution: lebster commentedGuys, 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.
Comment #9
klausiThanks, some minor coding standard issues that should be fixed:
this should not be removed? we are still using the event dispatcher?
this is not a field test?
this is an assert, not a test.
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...
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.
Comment #10
hideaway CreditAttribution: hideaway commentedAttaching 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.
Comment #11
hideaway CreditAttribution: hideaway commentedForgot patch file :)
Comment #12
hideaway CreditAttribution: hideaway commentedComment #13
klausimakes 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.
this for example.
Comment #14
hideaway CreditAttribution: hideaway commentedRemoved all unrelated code style fixes.
Comment #15
klausiLooks good! Now we need to wait on #2920046: Schema file is missing and install config file is wrong..
Comment #16
hideaway CreditAttribution: hideaway commentedPatch reroll for beta-1.
Comment #17
klausidata types are missing for many parameters, can you add those?
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!
Comment #18
klausiOpened #3123528: Enable the testbot for the JWT project.
Comment #19
hideaway CreditAttribution: hideaway commented@klausi addressed remarks, here is the latest patch
Comment #20
hideaway CreditAttribution: hideaway commentedComment #21
hideaway CreditAttribution: hideaway commentedpatch - one last time, test was still failing
Comment #22
hideaway CreditAttribution: hideaway commentedreordered test file to have a test case at the top, and annotated types
Comment #23
hideaway CreditAttribution: hideaway commentedComment #24
klausiLooks good, some minor issues:
do we have to make this new config required?
Can we just use "Authorization" as default in JwtAuth if the config is empty?
why do we delete this file?
should be JWT?
Comment #25
gregglesJust bumping to 8.x-1.x since that's where development is happening.
Comment #26
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedComment #27
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedThis 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
Comment #28
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedI 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:
Comment #29
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedLooks like I'm wrong, that header is not allowed to be an array. see e.g.
https://stackoverflow.com/questions/29282578/multiple-http-authorization...
https://github.com/graphile/postgraphile/issues/799
Comment #30
klausiOur use case for the custom Authorization header is development and staging sites, so caching or proxies should not be an issue.
Comment #31
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedA 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?
Comment #32
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedNeeds test, but something like this?
Comment #33
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedAt 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).
Comment #34
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commented+ Functional test, need to check priority over basic_auth still
Comment #35
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedPlus the last bit of test coverage.
Comment #36
klausiLooks 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.
Comment #37
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedFrom that RFC, looks like the more correct option would be: 'JWT-Authorization'
Re-rolled with that instead.
Also added a README.
Comment #38
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedComment #40
Anas_maw CreditAttribution: Anas_maw at Coders Enterprise Web & Mobile Solutions commentedFor 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.
Comment #41
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commented@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.
Comment #42
Anas_maw CreditAttribution: Anas_maw at Coders Enterprise Web & Mobile Solutions commented@pwolanin I opened a followup issue #3154220: Problems with doubly tagged services in beta4