Bug 20624

Summary: Disable the OAuth2 client credentials grant by default
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: AuthenticationAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: dpavlin, jonathan.druart, julian.maurice, katrin.fischer, kyle.m.hall, martin.renvoize, nick, tomascohen
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 20402, 20568, 20612    
Bug Blocks: 20734, 25623    
Attachments: Bug 20624: Add RESTOAuth2ClientCredentials syspref
Bug 20624: Unit tests
Bug 20624: Make /api/v1/oauth/token respect RESTOAuth2ClientCredentials
Bug 20624: Make staff client respect RESTOAuth2ClientCredentials
Bug 20624: (QA follow-up) Unit tests for missing deps situation
Bug 20624: (QA follow-up) Handle missing deps gracefuly
Bug 20624: Add RESTOAuth2ClientCredentials syspref
Bug 20624: Unit tests
Bug 20624: Make /api/v1/oauth/token respect RESTOAuth2ClientCredentials
Bug 20624: Make staff client respect RESTOAuth2ClientCredentials
Bug 20624: (QA follow-up) Unit tests for missing deps situation
Bug 20624: (QA follow-up) Handle missing deps gracefuly
Bug 20624: (QA follow-up) Add miising POD in Koha::REST::V1::OAuth
Bug 20624: Net::OAuth2::AuthorizationServer is not a hard dependency
Bug 20624: Add an entry in About when deps are not present

Description Tomás Cohen Arazi 2018-04-19 18:15:52 UTC

    
Comment 1 Martin Renvoize 2018-04-20 11:34:09 UTC
Holistic review found here: https://github.com/PTFS-Europe/koha/pull/112#pullrequestreview-113933459
Comment 2 Tomás Cohen Arazi 2018-05-02 19:00:24 UTC
Created attachment 74986 [details] [review]
Bug 20624: Add RESTOAuth2ClientCredentials syspref

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Tomás Cohen Arazi 2018-05-02 19:00:29 UTC
Created attachment 74987 [details] [review]
Bug 20624: Unit tests

This patch adds tests to verify that disabling the RESTOAuth2ClientCredentials syspref
makes any request on the /api/v1/oauth/token using the
'client_credentials' grant fail with 'grant not implemented'.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/oauth.t
=> FAIL: Tests fail because the change is not implemented!
Comment 4 Tomás Cohen Arazi 2018-05-02 19:00:34 UTC
Created attachment 74988 [details] [review]
Bug 20624: Make /api/v1/oauth/token respect RESTOAuth2ClientCredentials

This patch makes the /api/v1/oauth/token enpoint respect the
RESTOAuth2ClientCredentials syspref. It will return 400 (with
'Unimplemented grant type' error message) on the event of the syspref
being disabled and the grant_type => 'client_credentials' value passed.

To test:
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/oauth.t
=> FAIL: It fails because the off-switch is not implemented
- Apply this patch
- Run:
  k$ prove t/db_dependent/api/v1/oauth.t
=> SUCCESS: Tests pass!
- Sign off :-D
Comment 5 Tomás Cohen Arazi 2018-05-02 19:00:39 UTC
Created attachment 74989 [details] [review]
Bug 20624: Make staff client respect RESTOAuth2ClientCredentials

This patch makes the staff client UI respect the
RESTOAuth2ClientCredentials syspref.

To test:
- Make sure RESTOAuth2ClientCredentials is "Don't enable"
- Go to a patron's detail page
=> SUCCESS: The 'More' dropdown doesn't show the API keys management
link.
- Enable RESTOAuth2ClientCredentials
- Reload
=> SUCCESS: The 'More' dropdown shows the API keys management link
- Click on the API keys management link
=> SUCCESS: You can edit the api keys
- Disable the syspref
- Reload
=> SUCCESS: You are presented an error 400 page.
- Sign off :-D
Comment 6 Katrin Fischer 2018-05-02 19:35:15 UTC
Applied 20402, then bug 20568, but the latest patch there doesn't apply:
Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 20568: API keys management in interface
Applying: Bug 20568: Add mandatory description field for api keys
Applying: Bug 20568: Unit tests
Applying: Bug 20568: API key management for OPAC users
error: sha1 information is lacking or useless (koha-tmpl/opac-tmpl/bootstrap/en/includes/usermenu.inc).
error: could not build fake ancestor
Patch failed at 0001 Bug 20568: API key management for OPAC users
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-20568-API-key-management-for-OPAC-users-Igx9Ny.patch

Sorry, stuck :(
Comment 7 Tomás Cohen Arazi 2018-05-03 18:32:52 UTC
Created attachment 75020 [details] [review]
Bug 20624: (QA follow-up) Unit tests for missing deps situation

This patch tests the situation in which Net::OAuth2::AuthorizationServer
is missing. It mocks Module::Load::Conditional::can_load and expects the
/token endpoint answers 'Unimplemented grant type' to all requests, and
the 'authenticate_api_request' in 'under' exit with unauthorized (403)
to requests in which the Authorization header is passed containing a
Bearer token, but OAuth2 is not really available.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/oauth.t
=> FAIL: Tests fail because our REST endpoints don't support this
behaviour.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Tomás Cohen Arazi 2018-05-03 18:32:56 UTC
Created attachment 75021 [details] [review]
Bug 20624: (QA follow-up) Handle missing deps gracefuly

This patch makes the /token endpoint and the authenticate_api_request
method behave correctly in the event of missing deps for OAuth2.

To test:
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/oauth.t
=> FAIL: The behaviour is not implemented
- Apply this patch
- Run:
 k$ prove t/db_dependent/api/v1/oauth.t
=> SUCCESS: Tests pass!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Tomás Cohen Arazi 2018-05-03 19:13:37 UTC
(In reply to Katrin Fischer from comment #6)
> Applied 20402, then bug 20568, but the latest patch there doesn't apply:
> Apply? [(y)es, (n)o, (i)nteractive] y
> Applying: Bug 20568: API keys management in interface
> Applying: Bug 20568: Add mandatory description field for api keys
> Applying: Bug 20568: Unit tests
> Applying: Bug 20568: API key management for OPAC users
> error: sha1 information is lacking or useless
> (koha-tmpl/opac-tmpl/bootstrap/en/includes/usermenu.inc).
> error: could not build fake ancestor
> Patch failed at 0001 Bug 20568: API key management for OPAC users
> The copy of the patch that failed is found in: .git/rebase-apply/patch
> When you have resolved this problem run "git bz apply --continue".
> If you would prefer to skip this patch, instead run "git bz apply --skip".
> To restore the original branch and stop patching run "git bz apply --abort".
> Patch left in /tmp/Bug-20568-API-key-management-for-OPAC-users-Igx9Ny.patch
> 
> Sorry, stuck :(

Dependent bugs rebased!
Comment 10 Katrin Fischer 2018-05-04 06:01:04 UTC
bug 20402 is already a dependency of bug 20612
Comment 11 Katrin Fischer 2018-05-04 06:02:56 UTC
... trying to figure out the right sequence that will make git bz happy.
Comment 12 Katrin Fischer 2018-05-04 06:04:52 UTC
20402
20568
20612
20624 
... always tell git bz to ignore dependencies as it doesn't notice what's already applied.
Comment 13 Katrin Fischer 2018-05-04 06:09:17 UTC
Treating this as a false positive as it's not added by the patch set:

 FAIL	Koha/REST/V1/Auth.pm
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   FAIL	  pod coverage
		POD is missing for 'validate_query_parameters'

The link is gone, but the page remains accessible directly:
http://localhost:8081/cgi-bin/koha/members/apikeys.pl?patron_id=1

Tests fail for me:

t/db_dependent/api/v1/oauth.t .. 1/2 
#   Failed test '/oauth/token tests'
#   at t/db_dependent/api/v1/oauth.t line 116.
Can't call method "expires" on an undefined value at t/db_dependent/api/v1/oauth.t line 93.
# Looks like your test exited with 255 just after 1.
t/db_dependent/api/v1/oauth.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 2/2 subtests 

Test Summary Report
-------------------
t/db_dependent/api/v1/oauth.t (Wstat: 65280 Tests: 1 Failed: 1)
  Failed test:  1
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 2 tests but ran 1.
Comment 14 Kyle M Hall 2018-05-04 10:58:22 UTC
Created attachment 75060 [details] [review]
Bug 20624: Add RESTOAuth2ClientCredentials syspref

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Kyle M Hall 2018-05-04 10:58:29 UTC
Created attachment 75061 [details] [review]
Bug 20624: Unit tests

This patch adds tests to verify that disabling the RESTOAuth2ClientCredentials syspref
makes any request on the /api/v1/oauth/token using the
'client_credentials' grant fail with 'grant not implemented'.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/oauth.t
=> FAIL: Tests fail because the change is not implemented!

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Kyle M Hall 2018-05-04 10:58:33 UTC
Created attachment 75062 [details] [review]
Bug 20624: Make /api/v1/oauth/token respect RESTOAuth2ClientCredentials

This patch makes the /api/v1/oauth/token enpoint respect the
RESTOAuth2ClientCredentials syspref. It will return 400 (with
'Unimplemented grant type' error message) on the event of the syspref
being disabled and the grant_type => 'client_credentials' value passed.

To test:
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/oauth.t
=> FAIL: It fails because the off-switch is not implemented
- Apply this patch
- Run:
  k$ prove t/db_dependent/api/v1/oauth.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Kyle M Hall 2018-05-04 10:58:36 UTC
Created attachment 75063 [details] [review]
Bug 20624: Make staff client respect RESTOAuth2ClientCredentials

This patch makes the staff client UI respect the
RESTOAuth2ClientCredentials syspref.

To test:
- Make sure RESTOAuth2ClientCredentials is "Don't enable"
- Go to a patron's detail page
=> SUCCESS: The 'More' dropdown doesn't show the API keys management
link.
- Enable RESTOAuth2ClientCredentials
- Reload
=> SUCCESS: The 'More' dropdown shows the API keys management link
- Click on the API keys management link
=> SUCCESS: You can edit the api keys
- Disable the syspref
- Reload
=> SUCCESS: You are presented an error 400 page.
- Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Kyle M Hall 2018-05-04 10:58:39 UTC
Created attachment 75064 [details] [review]
Bug 20624: (QA follow-up) Unit tests for missing deps situation

This patch tests the situation in which Net::OAuth2::AuthorizationServer
is missing. It mocks Module::Load::Conditional::can_load and expects the
/token endpoint answers 'Unimplemented grant type' to all requests, and
the 'authenticate_api_request' in 'under' exit with unauthorized (403)
to requests in which the Authorization header is passed containing a
Bearer token, but OAuth2 is not really available.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/oauth.t
=> FAIL: Tests fail because our REST endpoints don't support this
behaviour.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Kyle M Hall 2018-05-04 10:58:43 UTC
Created attachment 75065 [details] [review]
Bug 20624: (QA follow-up) Handle missing deps gracefuly

This patch makes the /token endpoint and the authenticate_api_request
method behave correctly in the event of missing deps for OAuth2.

To test:
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/oauth.t
=> FAIL: The behaviour is not implemented
- Apply this patch
- Run:
 k$ prove t/db_dependent/api/v1/oauth.t
=> SUCCESS: Tests pass!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 20 Tomás Cohen Arazi 2018-05-07 14:58:17 UTC
Created attachment 75132 [details] [review]
Bug 20624: (QA follow-up) Add miising POD in Koha::REST::V1::OAuth

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 21 Tomás Cohen Arazi 2018-05-08 18:32:18 UTC
Created attachment 75184 [details] [review]
Bug 20624: Net::OAuth2::AuthorizationServer is not a hard dependency

While we get packaging sorted, Net::OAuth2::AuthorizationServer is not a
hard dependency for Koha and the feature requiring it is disabled by
default.

This patch:
- Makes the dependency optional
- Makes the unit tests for the OAuth2 client credentials flow skip if
  the dependency is not met.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 22 Jonathan Druart 2018-05-08 20:04:53 UTC
I was expecting to see rejected any requests made with a valid token when the pref is off.

Example:
Turn the pref on
Generate a token
Request something (or not)
Turn the pref on
Request something
=> 200
Should not we return 401 or whatever instead?
Comment 23 Tomás Cohen Arazi 2018-05-09 14:32:13 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2018-05-09 15:58:25 UTC
Pushed to master for 18.05, thanks to everybody involved!