Summary: | Add support for Basic auth on the OAuth2 token endpoint | ||
---|---|---|---|
Product: | Koha | Reporter: | Tomás Cohen Arazi (tcohen) <tomascohen> |
Component: | Authentication | Assignee: | Tomás Cohen Arazi (tcohen) <tomascohen> |
Status: | CLOSED FIXED | QA Contact: | Martin Renvoize (ashimema) <martin.renvoize> |
Severity: | enhancement | ||
Priority: | P5 - low | CC: | arthur.suzuki, dpavlin, fridolin.somers, josef.moravec, julian.maurice, kyle, martin.renvoize |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
GIT URL: | 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
|
Circulation function: | |
Attachments: |
Bug 23146: Unit tests
Bug 23146: Add support for Basic auth on the OAuth2 token endpoint Bug 23146: Unit tests Bug 23146: Add support for Basic auth on the OAuth2 token endpoint Bug 23146: Unit tests Bug 23146: Add support for Basic auth on the OAuth2 token endpoint Bug 23146: (QA follow-up) Make sure we use the absolute path Bug 23145: Confirming transfer during checkin clears the table of previously checked-in items |
Description
Tomás Cohen Arazi (tcohen)
2019-06-18 13:53:42 UTC
Created attachment 90757 [details] [review] Bug 23146: Unit tests 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 Created attachment 90777 [details] [review] Bug 23146: Unit tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> 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> 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> 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> Tests pass, code looks sane and works. Passing QA 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> Nice work! Pushed to master for 19.11.00 Created attachment 91558 [details] [review] Bug 23145: Confirming transfer during checkin clears the table of previously checked-in items This patch corrects the checkin template so that checking in an item with an existing transfer does not clear the page of previous checkouts. To test, apply the patch and check in some items. - After each checkin the returned item should appear in the table of checkouts. - Check in an item which belongs to another branch. You will be asked to confirm that the items requires a transfer. When the page reloads you should see the same previous checkins in the table. - Check in that same item again. You will be again asked to confirm. Before this page, confirming the transfer would reload the page and clear the list of checkins. Now the page should reload and the table of checkins should still show the previously checked-in items. Enhancement not pushed to 19.05.x |