Bug 23146 - Add support for Basic auth on the OAuth2 token endpoint
Summary: Add support for Basic auth on the OAuth2 token endpoint
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-06-18 13:53 UTC by Tomás Cohen Arazi
Modified: 2021-06-14 21:28 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This patchset adds flexibility to the OAuth2 implementation regarding how the parameters are passed on the request. The original implementation of OAuth2 only contemplated the option to pass the client_id and client_secret parameters on the request body. It is very common that clients expect to be able to pass them as a Basic authorization header.
Version(s) released in:
19.11.00


Attachments
Bug 23146: Unit tests (6.94 KB, patch)
2019-06-18 19:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23146: Add support for Basic auth on the OAuth2 token endpoint (3.45 KB, patch)
2019-06-18 19:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23146: Unit tests (7.01 KB, patch)
2019-06-19 11:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23146: Add support for Basic auth on the OAuth2 token endpoint (3.52 KB, patch)
2019-06-19 11:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23146: Unit tests (7.06 KB, patch)
2019-06-19 11:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23146: Add support for Basic auth on the OAuth2 token endpoint (3.57 KB, patch)
2019-06-19 11:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23146: (QA follow-up) Make sure we use the absolute path (2.09 KB, patch)
2019-06-19 15:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23145: Confirming transfer during checkin clears the table of previously checked-in items (2.42 KB, patch)
2019-07-16 18:13 UTC, Owen Leonard
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2019-06-18 13:53:42 UTC
The current implementation restricts it to only allow the parameters to be passed on the request body. This is an unnecessary limitation that (probably) prevents the integration of systems not capable of sending the parameters in the request body.
Comment 1 Tomás Cohen Arazi 2019-06-18 19:18:00 UTC
Created attachment 90757 [details] [review]
Bug 23146: Unit tests
Comment 2 Tomás Cohen Arazi 2019-06-18 19:18:04 UTC
Created attachment 90758 [details] [review]
Bug 23146: Add support for Basic auth on the OAuth2 token endpoint

The original implementation only contemplated the option to pass the
client_id and client_secret on the request body. It is very common that
clients expect to be able to pass them as a Basic authorization header:

Authorization: Basic encode_base64(client_id:client_secret)

This patch introduces support for this, by:
- Adding a check for the presence of the Authorization header in the
OAuth token request handling code and making that case extract the
client_id and client_secret from the header instead of the original
implementation. No behaviour changes.
- The Auth#under sub is changed so it doesn't go through the
authenticate_api_request chain step, as it would be in conflict with
general Basic authentication.
- Original tests are generalized so they are run in both ways, with the
same expected results.

To test:
- Apply the unit tests patch
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/oauth.t
=> FAIL: Tests fail because the current API doesn't support the feature
- Apply this patch
- Run:
 k$ prove t/db_dependent/api/v1/oauth.t
=> SUCCESS: Tests pass!
- Sign off :-D

Sponsored-by: ByWater Solutions
Comment 3 Kyle M Hall 2019-06-19 11:44:08 UTC
Created attachment 90777 [details] [review]
Bug 23146: Unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 4 Kyle M Hall 2019-06-19 11:44:18 UTC
Created attachment 90778 [details] [review]
Bug 23146: Add support for Basic auth on the OAuth2 token endpoint

The original implementation only contemplated the option to pass the
client_id and client_secret on the request body. It is very common that
clients expect to be able to pass them as a Basic authorization header:

Authorization: Basic encode_base64(client_id:client_secret)

This patch introduces support for this, by:
- Adding a check for the presence of the Authorization header in the
OAuth token request handling code and making that case extract the
client_id and client_secret from the header instead of the original
implementation. No behaviour changes.
- The Auth#under sub is changed so it doesn't go through the
authenticate_api_request chain step, as it would be in conflict with
general Basic authentication.
- Original tests are generalized so they are run in both ways, with the
same expected results.

To test:
- Apply the unit tests patch
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/oauth.t
=> FAIL: Tests fail because the current API doesn't support the feature
- Apply this patch
- Run:
 k$ prove t/db_dependent/api/v1/oauth.t
=> SUCCESS: Tests pass!
- Sign off :-D

Sponsored-by: ByWater Solutions

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 5 Martin Renvoize 2019-06-19 11:48:29 UTC
Created attachment 90779 [details] [review]
Bug 23146: Unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2019-06-19 11:48:32 UTC
Created attachment 90780 [details] [review]
Bug 23146: Add support for Basic auth on the OAuth2 token endpoint

The original implementation only contemplated the option to pass the
client_id and client_secret on the request body. It is very common that
clients expect to be able to pass them as a Basic authorization header:

Authorization: Basic encode_base64(client_id:client_secret)

This patch introduces support for this, by:
- Adding a check for the presence of the Authorization header in the
OAuth token request handling code and making that case extract the
client_id and client_secret from the header instead of the original
implementation. No behaviour changes.
- The Auth#under sub is changed so it doesn't go through the
authenticate_api_request chain step, as it would be in conflict with
general Basic authentication.
- Original tests are generalized so they are run in both ways, with the
same expected results.

To test:
- Apply the unit tests patch
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/oauth.t
=> FAIL: Tests fail because the current API doesn't support the feature
- Apply this patch
- Run:
 k$ prove t/db_dependent/api/v1/oauth.t
=> SUCCESS: Tests pass!
- Sign off :-D

Sponsored-by: ByWater Solutions
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2019-06-19 11:48:55 UTC
Tests pass, code looks sane and works.

Passing QA
Comment 8 Tomás Cohen Arazi 2019-06-19 15:43:48 UTC
Created attachment 90801 [details] [review]
Bug 23146: (QA follow-up) Make sure we use the absolute path

When making a request using any tool (like cUrl or Postman) you get a
'Basic authentication disabled' error (if it is actually disabled) or an
'invalid password' error if it is disabled. This is because the
comparisson of the path we do passes on oauth.t but fails on external
tools. This is probably related to our stack including Apache URL
mappings and then in the plack.psgi file. The safest way is to just ask
Mojo::URL the absolute path to be sure.

To test:
- Having the rest of the patches applied and plack restarted, run: [1]
  $ curl -X POST -H 'Authorization: Basic ZGQ2NjlmNGUtZmI1NS00Y2YzLWE4ZmYtYmFiYzJiNDIwNWY1OmM0ZDJmYmYzLWYwOWMtNGJkZi1iNWE4LTgxMDJmNjcwYTI1Mw' -i 'http://kohadev.myDNSname.org:8081/api/v1/oauth/token' --data grant_type=client_credentials
=> FAIL: It fails saying Basic auth is disabled
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/oauth.t
=> SUCCESS: Tests pass
- Apply this patch
- Replicate your curl/postman test
=> SUCCESS: It now works as expected
- Run:
 k$ prove t/db_dependent/api/v1/oauth.t
=> SUCCESS: Tests still pass!
- Sign off :-D

[1] You need to generate a client_id and client_secret, and encode them
using: encode_base64url( "$client_id:$client_secret" );

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Martin Renvoize 2019-06-24 16:03:39 UTC
Nice work!

Pushed to master for 19.11.00
Comment 10 Owen Leonard 2019-07-16 18:13:02 UTC Comment hidden (obsolete)
Comment 11 Fridolin Somers 2019-07-24 07:27:49 UTC
Enhancement not pushed to 19.05.x