Bug 20402 - OAuth2 client credentials grant for REST API
Summary: OAuth2 client credentials grant for REST API
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: Julian Maurice
QA Contact: Tomás Cohen Arazi
URL:
Keywords: dependency
Depends on:
Blocks: 20612 20624
  Show dependency treegraph
 
Reported: 2018-03-14 13:55 UTC by Julian Maurice
Modified: 2020-06-18 03:36 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This development adds the OAuth2 client credentials grant support to Koha. This way securing the REST API for using it from other systems gets easier as it follows current standards.
Version(s) released in:


Attachments
Bug 20402: Implement OAuth2 authentication for REST API (14.62 KB, patch)
2018-03-14 13:56 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20402: Follow-up to correct uninitialized failures (904 bytes, patch)
2018-03-14 14:59 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 20402: Implement OAuth2 authentication for REST API (14.72 KB, patch)
2018-03-14 15:56 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20402: Add Net::OAuth2::AuthorizationServer to perl deps (826 bytes, patch)
2018-03-14 15:56 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20402: Follow-up to correct uninitialized failures (911 bytes, patch)
2018-03-14 15:56 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20402: Implement OAuth2 authentication for REST API (14.71 KB, patch)
2018-03-14 16:49 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 20402: Add Net::OAuth2::AuthorizationServer to perl deps (871 bytes, patch)
2018-03-14 16:49 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 20402: Follow-up to correct uninitialized failures (957 bytes, patch)
2018-03-14 16:49 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 20402: Add unit tests (5.80 KB, patch)
2018-03-19 09:47 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20402: Add cronjob script to delete expired tokens (1.67 KB, patch)
2018-03-19 09:47 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20402: Tie client with patron, remove scopes, use mojo plugin, ... (18.62 KB, patch)
2018-04-11 16:41 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20402: Implement OAuth2 authentication for REST API (21.25 KB, patch)
2018-04-11 17:03 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20402: Implement OAuth2 authentication for REST API (21.25 KB, patch)
2018-04-12 07:25 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20402: Fix oauth.t (2.05 KB, patch)
2018-04-12 07:25 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20402: Implement OAuth2 authentication for REST API (21.22 KB, patch)
2018-04-12 11:52 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20402: Fix oauth.t (2.10 KB, patch)
2018-04-12 11:52 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20402: Don't look at cookies if OAuth2 is attempted and has failed (2.97 KB, patch)
2018-04-12 12:51 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20402: Implement OAuth2 authentication for REST API (21.25 KB, patch)
2018-04-17 17:11 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20402: Fix oauth.t (2.11 KB, patch)
2018-04-17 17:11 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20402: Don't look at cookies if OAuth2 is attempted and has failed (2.97 KB, patch)
2018-04-17 17:11 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20402: Remove dependency on Mojo::Plugin::OAuth2::Server (2.78 KB, patch)
2018-04-17 17:11 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20402: Use TestBuilder->build_object in oauth.t (1012 bytes, patch)
2018-04-17 17:11 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20402: Implement OAuth2 authentication for REST API (21.23 KB, patch)
2018-04-17 19:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20402: Fix oauth.t (2.16 KB, patch)
2018-04-17 19:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20402: Don't look at cookies if OAuth2 is attempted and has failed (3.02 KB, patch)
2018-04-17 19:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20402: Remove dependency on Mojo::Plugin::OAuth2::Server (2.84 KB, patch)
2018-04-17 19:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20402: Use TestBuilder->build_object in oauth.t (1.04 KB, patch)
2018-04-17 19:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20402: Atomic update and kohastructure.sql fixes (2.74 KB, patch)
2018-04-17 19:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20402: Add missing POD (4.91 KB, patch)
2018-04-17 19:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20402: Remove useless call to Koha::OAuthAccessTokens->search (722 bytes, patch)
2018-04-20 08:38 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20402: Implement OAuth2 authentication for REST API (21.10 KB, patch)
2018-05-03 19:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20402: Fix oauth.t (2.16 KB, patch)
2018-05-03 19:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20402: Don't look at cookies if OAuth2 is attempted and has failed (3.02 KB, patch)
2018-05-03 19:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20402: Remove dependency on Mojo::Plugin::OAuth2::Server (2.84 KB, patch)
2018-05-03 19:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20402: Use TestBuilder->build_object in oauth.t (1.04 KB, patch)
2018-05-03 19:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20402: Atomic update and kohastructure.sql fixes (2.74 KB, patch)
2018-05-03 19:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20402: Add missing POD (4.91 KB, patch)
2018-05-03 19:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20402: Remove useless call to Koha::OAuthAccessTokens->search (722 bytes, patch)
2018-05-03 19:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20402: only output if verbose flag is set (838 bytes, patch)
2018-05-08 19:45 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2018-03-14 13:55:15 UTC

    
Comment 1 Julian Maurice 2018-03-14 13:56:22 UTC
Created attachment 72865 [details] [review]
Bug 20402: Implement OAuth2 authentication for REST API

It implements only the "client credentials" flow with basic scopes
support (only one is defined, "patrons.read").
API Clients are defined in $KOHA_CONF.

Test plan:
1. In $KOHA_CONF, add an <api_client> element under <config>:
     <api_client>
       <client_id>$CLIENT_ID</client_id>
       <client_secret>$CLIENT_SECRET</client_secret>
       <scope>patrons.read</scope>
     </api_client>
2. Apply patch, run updatedatabase.pl and reload starman
3. Install Firefox extension RESTer [1]
4. In RESTer, go to "Authorization" tab and create a new OAuth2
   configuration:
   - OAuth flow: Client credentials
   - Access Token Request Method: POST
   - Access Token Request Endpoint: http://koha/api/v1/oauth/token
   - Access Token Request Client Authentication: Credentials in request
     body
   - Client ID: $CLIENT_ID
   - Client Secret: $CLIENT_SECRET
   - Scopes: patrons.read
5. Click on the newly created configuration to generate a new token
   (which will be valid only for an hour)
6. Set method to GET and url to http://koha/api/v1/patrons
   It should return 200 OK with the list of patrons
7. Remove or change the <scope> from $KOHA_CONF (reload starman &
   memcached) and see that you cannot generate a new token.
   Then reset the scope to its initial value
8. Edit api/v1/swagger/paths/patrons.json, locate 'x-koha-scopes' (2
   occurences) and change the values to something else. Reload starman.
   Repeat step 6 and see that you receive a 403 Forbidden status
   Undo your changes in api/v1/swagger/paths/patrons.json and reload
   starman again.
9. Wait an hour (or run the following SQL query:
   UPDATE oauth_access_tokens SET expires = 0) and repeat step 6.
   You should have a 403 Forbidden status, and the token must have been
   removed from the database.

[1] https://addons.mozilla.org/en-US/firefox/addon/rester/
Comment 2 Mark Tompsett 2018-03-14 14:59:57 UTC
Created attachment 72874 [details] [review]
Bug 20402: Follow-up to correct uninitialized failures

Added type => 'access' to the token request line in
Koha/REST/V1/OAuth.pm based on my quick skim of
/usr/share/perl5/Net/OAuth2/AuthorizationServer/Defaults.pm
which was complaining, and seemed to allow valid values of
'auth', 'refresh', and 'access'.
Comment 3 Julian Maurice 2018-03-14 15:56:05 UTC
Created attachment 72886 [details] [review]
Bug 20402: Implement OAuth2 authentication for REST API

It implements only the "client credentials" flow with basic scopes
support (only one is defined, "patrons.read").
API Clients are defined in $KOHA_CONF.

Test plan:
0. Install Net::OAuth2::AuthorizationServer 0.16
1. In $KOHA_CONF, add an <api_client> element under <config>:
     <api_client>
       <client_id>$CLIENT_ID</client_id>
       <client_secret>$CLIENT_SECRET</client_secret>
       <scope>patrons.read</scope>
     </api_client>
2. Apply patch, run updatedatabase.pl and reload starman
3. Install Firefox extension RESTer [1]
4. In RESTer, go to "Authorization" tab and create a new OAuth2
   configuration:
   - OAuth flow: Client credentials
   - Access Token Request Method: POST
   - Access Token Request Endpoint: http://$KOHA_URL/api/v1/oauth/token
   - Access Token Request Client Authentication: Credentials in request
     body
   - Client ID: $CLIENT_ID
   - Client Secret: $CLIENT_SECRET
   - Scopes: patrons.read
5. Click on the newly created configuration to generate a new token
   (which will be valid only for an hour)
6. In RESTer, set HTTP method to GET and url to
   http://$KOHA_URL/api/v1/patrons then click on SEND
   It should return 200 OK with the list of patrons
7. Remove or change the <scope> from $KOHA_CONF (reload starman &
   memcached) and see that you cannot generate a new token.
   Then reset the scope to its initial value
8. Edit api/v1/swagger/paths/patrons.json, locate 'x-koha-scopes' (2
   occurences) and change the values to something else. Reload starman.
   Repeat step 6 and see that you receive a 403 Forbidden status
   Undo your changes in api/v1/swagger/paths/patrons.json and reload
   starman again.
9. Wait an hour (or run the following SQL query:
   UPDATE oauth_access_tokens SET expires = 0) and repeat step 6.
   You should have a 403 Forbidden status, and the token must have been
   removed from the database.

[1] https://addons.mozilla.org/en-US/firefox/addon/rester/
Comment 4 Julian Maurice 2018-03-14 15:56:09 UTC
Created attachment 72887 [details] [review]
Bug 20402: Add Net::OAuth2::AuthorizationServer to perl deps
Comment 5 Julian Maurice 2018-03-14 15:56:14 UTC
Created attachment 72888 [details] [review]
Bug 20402: Follow-up to correct uninitialized failures

Added type => 'access' to the token request line in
Koha/REST/V1/OAuth.pm based on my quick skim of
/usr/share/perl5/Net/OAuth2/AuthorizationServer/Defaults.pm
which was complaining, and seemed to allow valid values of
'auth', 'refresh', and 'access'.
Comment 6 Julian Maurice 2018-03-14 16:13:13 UTC
Note: you can add as many API clients as you want by adding additional <api_client> elements.

I'm sure it would be better for API clients to be stored in database with a nice interface to manage them in Koha. But I think that can come later.
Comment 7 Mark Tompsett 2018-03-14 16:49:41 UTC
Created attachment 72892 [details] [review]
Bug 20402: Implement OAuth2 authentication for REST API

It implements only the "client credentials" flow with basic scopes
support (only one is defined, "patrons.read").
API Clients are defined in $KOHA_CONF.

Test plan:
0. Install Net::OAuth2::AuthorizationServer 0.16
1. In $KOHA_CONF, add an <api_client> element under <config>:
     <api_client>
       <client_id>$CLIENT_ID</client_id>
       <client_secret>$CLIENT_SECRET</client_secret>
       <scope>patrons.read</scope>
     </api_client>
2. Apply patch, run updatedatabase.pl and reload starman
3. Install Firefox extension RESTer [1]
4. In RESTer, go to "Authorization" tab and create a new OAuth2
   configuration:
   - OAuth flow: Client credentials
   - Access Token Request Method: POST
   - Access Token Request Endpoint: http://$KOHA_URL/api/v1/oauth/token
   - Access Token Request Client Authentication: Credentials in request
     body
   - Client ID: $CLIENT_ID
   - Client Secret: $CLIENT_SECRET
   - Scopes: patrons.read
5. Click on the newly created configuration to generate a new token
   (which will be valid only for an hour)
6. In RESTer, set HTTP method to GET and url to
   http://$KOHA_URL/api/v1/patrons then click on SEND
   It should return 200 OK with the list of patrons
7. Remove or change the <scope> from $KOHA_CONF (reload starman &
   memcached) and see that you cannot generate a new token.
   Then reset the scope to its initial value
8. Edit api/v1/swagger/paths/patrons.json, locate 'x-koha-scopes' (2
   occurences) and change the values to something else. Reload starman.
   Repeat step 6 and see that you receive a 403 Forbidden status
   Undo your changes in api/v1/swagger/paths/patrons.json and reload
   starman again.
9. Wait an hour (or run the following SQL query:
   UPDATE oauth_access_tokens SET expires = 0) and repeat step 6.
   You should have a 403 Forbidden status, and the token must have been
   removed from the database.

[1] https://addons.mozilla.org/en-US/firefox/addon/rester/

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 8 Mark Tompsett 2018-03-14 16:49:44 UTC
Created attachment 72893 [details] [review]
Bug 20402: Add Net::OAuth2::AuthorizationServer to perl deps

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 9 Mark Tompsett 2018-03-14 16:49:47 UTC
Created attachment 72894 [details] [review]
Bug 20402: Follow-up to correct uninitialized failures

Added type => 'access' to the token request line in
Koha/REST/V1/OAuth.pm based on my quick skim of
/usr/share/perl5/Net/OAuth2/AuthorizationServer/Defaults.pm
which was complaining, and seemed to allow valid values of
'auth', 'refresh', and 'access'.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 10 Mark Tompsett 2018-03-14 16:51:49 UTC
Only concern is if the authorization isn't attempted after it expires, the oauth_access_tokens record will lay in the table indefinitely. But works nicely otherwise.
Comment 11 Marcel de Rooy 2018-03-15 07:49:51 UTC
Julian: Unit tests ?
Comment 12 Julian Maurice 2018-03-19 09:47:37 UTC
Created attachment 73086 [details] [review]
Bug 20402: Add unit tests

Also, avoid some Perl warnings about undefined variables, and check that
grant_type parameter is set correctly

Test plan:
1. prove t/db_dependent/api/v1/oauth.t
Comment 13 Julian Maurice 2018-03-19 09:47:40 UTC
Created attachment 73087 [details] [review]
Bug 20402: Add cronjob script to delete expired tokens

Test plan:
1. Generate X tokens (X > 1)
2. mysql> UPDATE oauth_access_tokens SET expires = 0
3. Generate Y tokens (Y > 1)
4. Run the script
5. Verify that only Y tokens remains
Comment 14 Tomás Cohen Arazi 2018-03-21 13:49:21 UTC
A couple remarks:

- I like it
- I think the way scopes are defined needs more thinking, also its relation to permissions. The patchset adds patrons.read but I'm sure we need a spec on how this are defined and documented.
- The cleanup script could just be replaced by an addition to the (already configured for everyone) cleanup_database.pl script.
Comment 15 Julian Maurice 2018-03-21 16:04:39 UTC
(In reply to Tomás Cohen Arazi from comment #14)
> - I think the way scopes are defined needs more thinking, also its relation
> to permissions. The patchset adds patrons.read but I'm sure we need a spec
> on how this are defined and documented.
I agree. It looks like OpenAPI spec covers that (https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#securityDefinitionsObject) but I'm not sure if we can and should use this.

About relation to permissions, do you think we should have 1:1 relationship between scopes and permissions ?
Comment 16 Tomás Cohen Arazi 2018-03-21 16:09:32 UTC
(In reply to Julian Maurice from comment #15)
> (In reply to Tomás Cohen Arazi from comment #14)
> > - I think the way scopes are defined needs more thinking, also its relation
> > to permissions. The patchset adds patrons.read but I'm sure we need a spec
> > on how this are defined and documented.
> I agree. It looks like OpenAPI spec covers that
> (https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.
> md#securityDefinitionsObject) but I'm not sure if we can and should use this.
> 
> About relation to permissions, do you think we should have 1:1 relationship
> between scopes and permissions ?

I think our permissions granularity is not enough for that. But I would like to see a spec to discuss about how scopes would be defined, having a catalog of them, etc.
Comment 17 Tomás Cohen Arazi 2018-04-03 19:16:30 UTC
I think we should tie api id and keys to patrons to take advantage of the current permission system. And introduce the use of scopes at a later stage.

Julian: can you do it? i.e. pick the code you once wrote for having patrons create api keys on the staff interface?
Comment 18 Julian Maurice 2018-04-04 07:11:41 UTC
(In reply to Tomás Cohen Arazi from comment #17)
> I think we should tie api id and keys to patrons to take advantage of the
> current permission system. And introduce the use of scopes at a later stage.
> 
> Julian: can you do it? i.e. pick the code you once wrote for having patrons
> create api keys on the staff interface?
I think it's not needed if you only want to tie an API client with a patron. We could add a <userid>xxx</userid> in $KOHA_CONF, and act as if that user was authenticated.
Comment 19 Julian Maurice 2018-04-11 15:02:18 UTC
It has been decided during the IRC meeting to make it possible to tie an API client with an existing patron and to use their permissions.
We give up the scopes for now, as it would require much more discussions and we want OAuth2 in 18.05
Comment 20 Julian Maurice 2018-04-11 16:41:45 UTC
Created attachment 74009 [details] [review]
Bug 20402: Tie client with patron, remove scopes, use mojo plugin, ...

... and move cleanup code into misc/cronjobs/cleanup_database.pl
Comment 21 Julian Maurice 2018-04-11 16:51:53 UTC
A few notes about the last patch:
- I attached it only as an easy way to see the last changes, beacause I will squash all patches into one clean patch.
- The use of Mojolicious::Plugin::OAuth2::Server was suggested on IRC, but it only helps removing a few lines of code. I'm not sure it's worth it (new non-packaged dependency)
- The test plan in comment 7 is not correct anymore. Before testing, please wait until I squash all patch and write a new test plan (or use your brain to guess a new test plan :))
Comment 22 Julian Maurice 2018-04-11 17:03:57 UTC
Created attachment 74018 [details] [review]
Bug 20402: Implement OAuth2 authentication for REST API

It implements only the "client credentials" flow with no scopes
support. API clients are tied to an existing patron and have the same
permissions as the patron they are tied to.
API Clients are defined in $KOHA_CONF.

Test plan:
0. Install Net::OAuth2::AuthorizationServer 0.16 and
   Mojolicious::Plugin::OAuth2::Server 0.40
1. In $KOHA_CONF, add an <api_client> element under <config>:
     <api_client>
       <client_id>$CLIENT_ID</client_id>
       <client_secret>$CLIENT_SECRET</client_secret>
       <patron_id>X</patron_id> <!-- X is an existing borrowernumber -->
     </api_client>
2. Apply patch, run updatedatabase.pl and reload starman
3. Install Firefox extension RESTer [1]
4. In RESTer, go to "Authorization" tab and create a new OAuth2
   configuration:
   - OAuth flow: Client credentials
   - Access Token Request Method: POST
   - Access Token Request Endpoint: http://$KOHA_URL/api/v1/oauth/token
   - Access Token Request Client Authentication: Credentials in request
     body
   - Client ID: $CLIENT_ID
   - Client Secret: $CLIENT_SECRET
5. Click on the newly created configuration to generate a new token
   (which will be valid only for an hour)
6. In RESTer, set HTTP method to GET and url to
   http://$KOHA_URL/api/v1/patrons then click on SEND
   If patron X has permission 'edit_borrowers', it should return 200 OK
   with the list of patrons
   Otherwise it should return 403 with the list of required permissions
   (Please test both cases)
7. Wait an hour (or run the following SQL query:
   UPDATE oauth_access_tokens SET expires = 0) and repeat step 6.
   You should have a 403 Forbidden status, and the token must have been
   removed from the database.
8. Create a bunch of tokens using RESTer, make some of them expires
   using the previous SQL query, and run the following command:
     misc/cronjobs/cleanup_database.pl --oauth-tokens
   Verify that expired tokens were removed, and that the others are
   still there
9. prove t/db_dependent/api/v1/oauth.t

[1] https://addons.mozilla.org/en-US/firefox/addon/rester/
Comment 23 Julian Maurice 2018-04-11 17:04:55 UTC
All patches squashed and rebased on master.

Please test !
Comment 24 Josef Moravec 2018-04-11 19:17:50 UTC
(In reply to Julian Maurice from comment #22)
> Created attachment 74018 [details] [review] [review]
> Bug 20402: Implement OAuth2 authentication for REST API
> 
> It implements only the "client credentials" flow with no scopes
> support. API clients are tied to an existing patron and have the same
> permissions as the patron they are tied to.
> API Clients are defined in $KOHA_CONF.
> 
> Test plan:
> 0. Install Net::OAuth2::AuthorizationServer 0.16 and
>    Mojolicious::Plugin::OAuth2::Server 0.40
> 1. In $KOHA_CONF, add an <api_client> element under <config>:
>      <api_client>
>        <client_id>$CLIENT_ID</client_id>
>        <client_secret>$CLIENT_SECRET</client_secret>
>        <patron_id>X</patron_id> <!-- X is an existing borrowernumber -->
>      </api_client>
> 2. Apply patch, run updatedatabase.pl and reload starman
> 3. Install Firefox extension RESTer [1]
> 4. In RESTer, go to "Authorization" tab and create a new OAuth2
>    configuration:
>    - OAuth flow: Client credentials
>    - Access Token Request Method: POST
>    - Access Token Request Endpoint: http://$KOHA_URL/api/v1/oauth/token
>    - Access Token Request Client Authentication: Credentials in request
>      body
>    - Client ID: $CLIENT_ID
>    - Client Secret: $CLIENT_SECRET
> 5. Click on the newly created configuration to generate a new token
>    (which will be valid only for an hour)
> 6. In RESTer, set HTTP method to GET and url to
>    http://$KOHA_URL/api/v1/patrons then click on SEND
>    If patron X has permission 'edit_borrowers', it should return 200 OK
>    with the list of patrons
>    Otherwise it should return 403 with the list of required permissions
>    (Please test both cases)
> 7. Wait an hour (or run the following SQL query:
>    UPDATE oauth_access_tokens SET expires = 0) and repeat step 6.
>    You should have a 403 Forbidden status, and the token must have been
>    removed from the database.

Token is removed, but i got status 500

> 8. Create a bunch of tokens using RESTer, make some of them expires
>    using the previous SQL query, and run the following command:
>      misc/cronjobs/cleanup_database.pl --oauth-tokens
>    Verify that expired tokens were removed, and that the others are
>    still there
> 9. prove t/db_dependent/api/v1/oauth.t

One test is failing for me: 

not ok 19 - 200 OK

    #   Failed test '200 OK'
    #   at t/db_dependent/api/v1/oauth.t line 98.
    #          got: '403'
    #     expected: '200'
    # Looks like you failed 1 test of 19.


> 
> [1] https://addons.mozilla.org/en-US/firefox/addon/rester/
Comment 25 Tomás Cohen Arazi 2018-04-11 22:05:00 UTC
(In reply to Julian Maurice from comment #21)
> A few notes about the last patch:
> - I attached it only as an easy way to see the last changes, beacause I will
> squash all patches into one clean patch.
> - The use of Mojolicious::Plugin::OAuth2::Server was suggested on IRC, but
> it only helps removing a few lines of code. I'm not sure it's worth it (new
> non-packaged dependency)
> - The test plan in comment 7 is not correct anymore. Before testing, please
> wait until I squash all patch and write a new test plan (or use your brain
> to guess a new test plan :))

When I tested using the Mojo plugin, I didn't find a clear way to implement several grant flows. And my guess was that sticking to using the core lib was easier.
Comment 26 Julian Maurice 2018-04-12 07:17:20 UTC
(In reply to Josef Moravec from comment #24)
> Token is removed, but i got status 500
Can you check the logs? I just tested and for me it returns 401 as expected

> One test is failing for me
I didn't launch the tests after the rebase and I missed that required permissions for /patrons have changed. I will fix that in a follow-up
Comment 27 Julian Maurice 2018-04-12 07:25:29 UTC
Created attachment 74050 [details] [review]
Bug 20402: Implement OAuth2 authentication for REST API

It implements only the "client credentials" flow with no scopes
support. API clients are tied to an existing patron and have the same
permissions as the patron they are tied to.
API Clients are defined in $KOHA_CONF.

Test plan:
0. Install Net::OAuth2::AuthorizationServer 0.16 and
   Mojolicious::Plugin::OAuth2::Server 0.40
1. In $KOHA_CONF, add an <api_client> element under <config>:
     <api_client>
       <client_id>$CLIENT_ID</client_id>
       <client_secret>$CLIENT_SECRET</client_secret>
       <patron_id>X</patron_id> <!-- X is an existing borrowernumber -->
     </api_client>
2. Apply patch, run updatedatabase.pl and reload starman
3. Install Firefox extension RESTer [1]
4. In RESTer, go to "Authorization" tab and create a new OAuth2
   configuration:
   - OAuth flow: Client credentials
   - Access Token Request Method: POST
   - Access Token Request Endpoint: http://$KOHA_URL/api/v1/oauth/token
   - Access Token Request Client Authentication: Credentials in request
     body
   - Client ID: $CLIENT_ID
   - Client Secret: $CLIENT_SECRET
5. Click on the newly created configuration to generate a new token
   (which will be valid only for an hour)
6. In RESTer, set HTTP method to GET and url to
   http://$KOHA_URL/api/v1/patrons then click on SEND
   If patron X has permission 'borrowers', it should return 200 OK
   with the list of patrons
   Otherwise it should return 403 with the list of required permissions
   (Please test both cases)
7. Wait an hour (or run the following SQL query:
   UPDATE oauth_access_tokens SET expires = 0) and repeat step 6.
   You should have a 403 Forbidden status, and the token must have been
   removed from the database.
8. Create a bunch of tokens using RESTer, make some of them expires
   using the previous SQL query, and run the following command:
     misc/cronjobs/cleanup_database.pl --oauth-tokens
   Verify that expired tokens were removed, and that the others are
   still there
9. prove t/db_dependent/api/v1/oauth.t

[1] https://addons.mozilla.org/en-US/firefox/addon/rester/
Comment 28 Julian Maurice 2018-04-12 07:25:33 UTC
Created attachment 74051 [details] [review]
Bug 20402: Fix oauth.t

GET /patrons now requires { "borrowers": 1 } instead of
{ "borrowers": "edit_borrowers" }
Comment 29 Josef Moravec 2018-04-12 11:50:41 UTC
Tested on another devbox, now I got 401 Unauthorized.

I have an idea of what is going on:

This only adds new authorization method, but the authorization with cookies (eg. when you are normally logged into koha) is still taken into account.

But yesterday I used the koha database user to change permission of api user - so token was expired and oauth say unauthorized, then cookie was found bud the logged in user was database user and that's the reason why it exploded
Comment 30 Josef Moravec 2018-04-12 11:52:09 UTC
Created attachment 74069 [details] [review]
Bug 20402: Implement OAuth2 authentication for REST API

It implements only the "client credentials" flow with no scopes
support. API clients are tied to an existing patron and have the same
permissions as the patron they are tied to.
API Clients are defined in $KOHA_CONF.

Test plan:
0. Install Net::OAuth2::AuthorizationServer 0.16 and
   Mojolicious::Plugin::OAuth2::Server 0.40
1. In $KOHA_CONF, add an <api_client> element under <config>:
     <api_client>
       <client_id>$CLIENT_ID</client_id>
       <client_secret>$CLIENT_SECRET</client_secret>
       <patron_id>X</patron_id> <!-- X is an existing borrowernumber -->
     </api_client>
2. Apply patch, run updatedatabase.pl and reload starman
3. Install Firefox extension RESTer [1]
4. In RESTer, go to "Authorization" tab and create a new OAuth2
   configuration:
   - OAuth flow: Client credentials
   - Access Token Request Method: POST
   - Access Token Request Endpoint: http://$KOHA_URL/api/v1/oauth/token
   - Access Token Request Client Authentication: Credentials in request
     body
   - Client ID: $CLIENT_ID
   - Client Secret: $CLIENT_SECRET
5. Click on the newly created configuration to generate a new token
   (which will be valid only for an hour)
6. In RESTer, set HTTP method to GET and url to
   http://$KOHA_URL/api/v1/patrons then click on SEND
   If patron X has permission 'borrowers', it should return 200 OK
   with the list of patrons
   Otherwise it should return 403 with the list of required permissions
   (Please test both cases)
7. Wait an hour (or run the following SQL query:
   UPDATE oauth_access_tokens SET expires = 0) and repeat step 6.
   You should have a 403 Forbidden status, and the token must have been
   removed from the database.
8. Create a bunch of tokens using RESTer, make some of them expires
   using the previous SQL query, and run the following command:
     misc/cronjobs/cleanup_database.pl --oauth-tokens
   Verify that expired tokens were removed, and that the others are
   still there
9. prove t/db_dependent/api/v1/oauth.t

[1] https://addons.mozilla.org/en-US/firefox/addon/rester/

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 31 Josef Moravec 2018-04-12 11:52:12 UTC
Created attachment 74070 [details] [review]
Bug 20402: Fix oauth.t

GET /patrons now requires { "borrowers": 1 } instead of
{ "borrowers": "edit_borrowers" }

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 32 Julian Maurice 2018-04-12 12:08:08 UTC
(In reply to Josef Moravec from comment #29)
> Tested on another devbox, now I got 401 Unauthorized.
> 
> I have an idea of what is going on:
> 
> This only adds new authorization method, but the authorization with cookies
> (eg. when you are normally logged into koha) is still taken into account.
> 
> But yesterday I used the koha database user to change permission of api user
> - so token was expired and oauth say unauthorized, then cookie was found bud
> the logged in user was database user and that's the reason why it exploded

Then maybe we should not try cookie authentication if oauth fail and we have an Authorization header containing 'Bearer' ?
Comment 33 Josef Moravec 2018-04-12 12:09:54 UTC
(In reply to Julian Maurice from comment #32)
> (In reply to Josef Moravec from comment #29)
> > Tested on another devbox, now I got 401 Unauthorized.
> > 
> > I have an idea of what is going on:
> > 
> > This only adds new authorization method, but the authorization with cookies
> > (eg. when you are normally logged into koha) is still taken into account.
> > 
> > But yesterday I used the koha database user to change permission of api user
> > - so token was expired and oauth say unauthorized, then cookie was found bud
> > the logged in user was database user and that's the reason why it exploded
> 
> Then maybe we should not try cookie authentication if oauth fail and we have
> an Authorization header containing 'Bearer' ?

+1
Comment 34 Julian Maurice 2018-04-12 12:51:32 UTC
Created attachment 74088 [details] [review]
Bug 20402: Don't look at cookies if OAuth2 is attempted and has failed
Comment 35 Julian Maurice 2018-04-17 17:11:16 UTC
Created attachment 74365 [details] [review]
Bug 20402: Implement OAuth2 authentication for REST API

It implements only the "client credentials" flow with no scopes
support. API clients are tied to an existing patron and have the same
permissions as the patron they are tied to.
API Clients are defined in $KOHA_CONF.

Test plan:
0. Install Net::OAuth2::AuthorizationServer 0.16
1. In $KOHA_CONF, add an <api_client> element under <config>:
     <api_client>
       <client_id>$CLIENT_ID</client_id>
       <client_secret>$CLIENT_SECRET</client_secret>
       <patron_id>X</patron_id> <!-- X is an existing borrowernumber -->
     </api_client>
2. Apply patch, run updatedatabase.pl and reload starman
3. Install Firefox extension RESTer [1]
4. In RESTer, go to "Authorization" tab and create a new OAuth2
   configuration:
   - OAuth flow: Client credentials
   - Access Token Request Method: POST
   - Access Token Request Endpoint: http://$KOHA_URL/api/v1/oauth/token
   - Access Token Request Client Authentication: Credentials in request
     body
   - Client ID: $CLIENT_ID
   - Client Secret: $CLIENT_SECRET
5. Click on the newly created configuration to generate a new token
   (which will be valid only for an hour)
6. In RESTer, set HTTP method to GET and url to
   http://$KOHA_URL/api/v1/patrons then click on SEND
   If patron X has permission 'borrowers', it should return 200 OK
   with the list of patrons
   Otherwise it should return 403 with the list of required permissions
   (Please test both cases)
7. Wait an hour (or run the following SQL query:
   UPDATE oauth_access_tokens SET expires = 0) and repeat step 6.
   You should have a 403 Forbidden status, and the token must have been
   removed from the database.
8. Create a bunch of tokens using RESTer, make some of them expires
   using the previous SQL query, and run the following command:
     misc/cronjobs/cleanup_database.pl --oauth-tokens
   Verify that expired tokens were removed, and that the others are
   still there
9. prove t/db_dependent/api/v1/oauth.t

[1] https://addons.mozilla.org/en-US/firefox/addon/rester/

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 36 Julian Maurice 2018-04-17 17:11:20 UTC
Created attachment 74366 [details] [review]
Bug 20402: Fix oauth.t

GET /patrons now requires { "borrowers": 1 } instead of
{ "borrowers": "edit_borrowers" }

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 37 Julian Maurice 2018-04-17 17:11:25 UTC
Created attachment 74367 [details] [review]
Bug 20402: Don't look at cookies if OAuth2 is attempted and has failed
Comment 38 Julian Maurice 2018-04-17 17:11:29 UTC
Created attachment 74368 [details] [review]
Bug 20402: Remove dependency on Mojo::Plugin::OAuth2::Server
Comment 39 Julian Maurice 2018-04-17 17:11:33 UTC
Created attachment 74369 [details] [review]
Bug 20402: Use TestBuilder->build_object in oauth.t
Comment 40 Tomás Cohen Arazi 2018-04-17 19:34:55 UTC
Created attachment 74380 [details] [review]
Bug 20402: Implement OAuth2 authentication for REST API

It implements only the "client credentials" flow with no scopes
support. API clients are tied to an existing patron and have the same
permissions as the patron they are tied to.
API Clients are defined in $KOHA_CONF.

Test plan:
0. Install Net::OAuth2::AuthorizationServer 0.16
1. In $KOHA_CONF, add an <api_client> element under <config>:
     <api_client>
       <client_id>$CLIENT_ID</client_id>
       <client_secret>$CLIENT_SECRET</client_secret>
       <patron_id>X</patron_id> <!-- X is an existing borrowernumber -->
     </api_client>
2. Apply patch, run updatedatabase.pl and reload starman
3. Install Firefox extension RESTer [1]
4. In RESTer, go to "Authorization" tab and create a new OAuth2
   configuration:
   - OAuth flow: Client credentials
   - Access Token Request Method: POST
   - Access Token Request Endpoint: http://$KOHA_URL/api/v1/oauth/token
   - Access Token Request Client Authentication: Credentials in request
     body
   - Client ID: $CLIENT_ID
   - Client Secret: $CLIENT_SECRET
5. Click on the newly created configuration to generate a new token
   (which will be valid only for an hour)
6. In RESTer, set HTTP method to GET and url to
   http://$KOHA_URL/api/v1/patrons then click on SEND
   If patron X has permission 'borrowers', it should return 200 OK
   with the list of patrons
   Otherwise it should return 403 with the list of required permissions
   (Please test both cases)
7. Wait an hour (or run the following SQL query:
   UPDATE oauth_access_tokens SET expires = 0) and repeat step 6.
   You should have a 403 Forbidden status, and the token must have been
   removed from the database.
8. Create a bunch of tokens using RESTer, make some of them expires
   using the previous SQL query, and run the following command:
     misc/cronjobs/cleanup_database.pl --oauth-tokens
   Verify that expired tokens were removed, and that the others are
   still there
9. prove t/db_dependent/api/v1/oauth.t

[1] https://addons.mozilla.org/en-US/firefox/addon/rester/

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 41 Tomás Cohen Arazi 2018-04-17 19:35:05 UTC
Created attachment 74381 [details] [review]
Bug 20402: Fix oauth.t

GET /patrons now requires { "borrowers": 1 } instead of
{ "borrowers": "edit_borrowers" }

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 42 Tomás Cohen Arazi 2018-04-17 19:35:11 UTC
Created attachment 74382 [details] [review]
Bug 20402: Don't look at cookies if OAuth2 is attempted and has failed

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 43 Tomás Cohen Arazi 2018-04-17 19:35:18 UTC
Created attachment 74383 [details] [review]
Bug 20402: Remove dependency on Mojo::Plugin::OAuth2::Server

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 44 Tomás Cohen Arazi 2018-04-17 19:35:26 UTC
Created attachment 74384 [details] [review]
Bug 20402: Use TestBuilder->build_object in oauth.t

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 45 Tomás Cohen Arazi 2018-04-17 19:35:34 UTC
Created attachment 74385 [details] [review]
Bug 20402: Atomic update and kohastructure.sql fixes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 46 Tomás Cohen Arazi 2018-04-17 19:35:41 UTC
Created attachment 74386 [details] [review]
Bug 20402: Add missing POD

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 47 Tomás Cohen Arazi 2018-04-17 19:38:08 UTC
Only minor fixes were needed. Works as expected.
Comment 48 Jonathan Druart 2018-04-19 16:59:53 UTC
How hard will it be to package Net::OAuth2::AuthorizationServer and deps?
Comment 49 Jonathan Druart 2018-04-19 17:13:23 UTC
It the key is revoked or deleted, the token is still valid, is it expected?
Comment 50 Martin Renvoize 2018-04-20 07:39:28 UTC
Comment on attachment 74380 [details] [review]
Bug 20402: Implement OAuth2 authentication for REST API

Review of attachment 74380 [details] [review]:
-----------------------------------------------------------------

Generally looks good.. moving onto the next patchset

::: Koha/OAuth.pm
@@ +21,5 @@
> +    return (0, 'unauthorized_client') unless $client_id;
> +
> +    my $clients = C4::Context->config('api_client');
> +    $clients = [ $clients ] unless ref $clients eq 'ARRAY';
> +    my ($client) = grep { $_->{client_id} eq $client_id } @$clients;

'any' would be more performant than 'grep' here: https://perldoc.perl.org/List/Util.html#any

@@ +50,5 @@
> +    my (%args) = @_;
> +
> +    my $access_token = $args{access_token};
> +
> +    my $at = Koha::OAuthAccessTokens->find($access_token);

Personally, I would wrap this in caching.. this will be called with literally every API request and as such will become a fairly heavy use of the DB.

::: Koha/REST/V1/Auth.pm
@@ +115,5 @@
> +
> +    if (my $oauth = $c->oauth) {
> +        my $clients = C4::Context->config('api_client');
> +        $clients = [ $clients ] unless ref $clients eq 'ARRAY';
> +        my ($client) = grep { $_->{client_id} eq $oauth->{client_id} } @$clients;

Another case where 'any' would be more performant than 'grep' ;)

::: Koha/REST/V1/OAuth.pm
@@ +47,5 @@
> +        access_token => $token,
> +        expires_in   => $expires_in,
> +    );
> +
> +    my $at = Koha::OAuthAccessTokens->search({ access_token => $token })->next;

search->next is generally a bad idiom when changed.. `->next` should really only be used inside a loop where you're expecting a set of results.. when chained it it raises warning signals to me that you really want to use either ->find or ->search( {},{order_by => 'something', rows => 1})->single to get THE explicit result you're after.
Comment 51 Martin Renvoize 2018-04-20 08:01:07 UTC
Comment on attachment 74380 [details] [review]
Bug 20402: Implement OAuth2 authentication for REST API

Review of attachment 74380 [details] [review]:
-----------------------------------------------------------------

::: Koha/REST/V1/Auth.pm
@@ +128,5 @@
> +            validate_query_parameters( $c, $spec );
> +
> +            # Everything is ok
> +            return 1;
> +        }

This 'scope' validation would normally be done within the _verify_access_token_cb for the usual case with the ::OAuth2::Server Plugin.
Comment 52 Martin Renvoize 2018-04-20 08:08:47 UTC
Comment on attachment 74380 [details] [review]
Bug 20402: Implement OAuth2 authentication for REST API

Review of attachment 74380 [details] [review]:
-----------------------------------------------------------------

::: Koha/REST/V1/Auth.pm
@@ +116,5 @@
> +    if (my $oauth = $c->oauth) {
> +        my $clients = C4::Context->config('api_client');
> +        $clients = [ $clients ] unless ref $clients eq 'ARRAY';
> +        my ($client) = grep { $_->{client_id} eq $oauth->{client_id} } @$clients;
> +

Actually.. I'm confused.. why do we check against the clients array for a second time.. We've already tested against this at the 'issue a token' stage.. or is this your idea of 'revoking a client'?
Comment 53 Martin Renvoize 2018-04-20 08:08:47 UTC
Comment on attachment 74380 [details] [review]
Bug 20402: Implement OAuth2 authentication for REST API

Review of attachment 74380 [details] [review]:
-----------------------------------------------------------------

::: Koha/REST/V1/Auth.pm
@@ +116,5 @@
> +    if (my $oauth = $c->oauth) {
> +        my $clients = C4::Context->config('api_client');
> +        $clients = [ $clients ] unless ref $clients eq 'ARRAY';
> +        my ($client) = grep { $_->{client_id} eq $oauth->{client_id} } @$clients;
> +

Actually.. I'm confused.. why do we check against the clients array for a second time.. We've already tested against this at the 'issue a token' stage.. or is this your idea of 'revoking a client'?
Comment 54 Martin Renvoize 2018-04-20 08:08:48 UTC
Comment on attachment 74380 [details] [review]
Bug 20402: Implement OAuth2 authentication for REST API

Review of attachment 74380 [details] [review]:
-----------------------------------------------------------------

::: Koha/REST/V1/Auth.pm
@@ +116,5 @@
> +    if (my $oauth = $c->oauth) {
> +        my $clients = C4::Context->config('api_client');
> +        $clients = [ $clients ] unless ref $clients eq 'ARRAY';
> +        my ($client) = grep { $_->{client_id} eq $oauth->{client_id} } @$clients;
> +

Actually.. I'm confused.. why do we check against the clients array for a second time.. We've already tested against this at the 'issue a token' stage.. or is this your idea of 'revoking a client'?
Comment 55 Julian Maurice 2018-04-20 08:29:28 UTC
(In reply to Martin Renvoize from comment #50)
> Comment on attachment 74380 [details] [review] [review]
> ::: Koha/OAuth.pm
> @@ +21,5 @@
> > +    return (0, 'unauthorized_client') unless $client_id;
> > +
> > +    my $clients = C4::Context->config('api_client');
> > +    $clients = [ $clients ] unless ref $clients eq 'ARRAY';
> > +    my ($client) = grep { $_->{client_id} eq $client_id } @$clients;
> 
> 'any' would be more performant than 'grep' here:
> https://perldoc.perl.org/List/Util.html#any
> 
'any' returns a boolean value, we're looking for the $client hashref here

> @@ +50,5 @@
> > +    my (%args) = @_;
> > +
> > +    my $access_token = $args{access_token};
> > +
> > +    my $at = Koha::OAuthAccessTokens->find($access_token);
> 
> Personally, I would wrap this in caching.. this will be called with
> literally every API request and as such will become a fairly heavy use of
> the DB.
I don't think we should cache the access token, because it can expire

> ::: Koha/REST/V1/OAuth.pm
> @@ +47,5 @@
> > +        access_token => $token,
> > +        expires_in   => $expires_in,
> > +    );
> > +
> > +    my $at = Koha::OAuthAccessTokens->search({ access_token => $token })->next;
> 
> search->next is generally a bad idiom when changed.. `->next` should really
> only be used inside a loop where you're expecting a set of results.. when
> chained it it raises warning signals to me that you really want to use
> either ->find or ->search( {},{order_by => 'something', rows => 1})->single
> to get THE explicit result you're after.

Actually, we should use ->find, since access_token is a primary key (->find is used at other places in the code, I forgot to change this one)
Comment 56 Julian Maurice 2018-04-20 08:32:08 UTC
(In reply to Martin Renvoize from comment #51)
> Comment on attachment 74380 [details] [review] [review]
> Bug 20402: Implement OAuth2 authentication for REST API
> 
> Review of attachment 74380 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: Koha/REST/V1/Auth.pm
> @@ +128,5 @@
> > +            validate_query_parameters( $c, $spec );
> > +
> > +            # Everything is ok
> > +            return 1;
> > +        }
> 
> This 'scope' validation would normally be done within the
> _verify_access_token_cb for the usual case with the ::OAuth2::Server Plugin.

scopes are not implemented, we'll use patron's permissions for now
Comment 57 Julian Maurice 2018-04-20 08:35:22 UTC
(In reply to Martin Renvoize from comment #54)
> Comment on attachment 74380 [details] [review] [review]
> Bug 20402: Implement OAuth2 authentication for REST API
> 
> Review of attachment 74380 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: Koha/REST/V1/Auth.pm
> @@ +116,5 @@
> > +    if (my $oauth = $c->oauth) {
> > +        my $clients = C4::Context->config('api_client');
> > +        $clients = [ $clients ] unless ref $clients eq 'ARRAY';
> > +        my ($client) = grep { $_->{client_id} eq $oauth->{client_id} } @$clients;
> > +
> 
> Actually.. I'm confused.. why do we check against the clients array for a
> second time.. We've already tested against this at the 'issue a token'
> stage.. or is this your idea of 'revoking a client'?

We need the $client hashref to get the patron_id defined in $KOHA_CONF.

Please note that this code is removed by bug 20612, which allows to define client_id/secret pairs in the interface instead of in $KOHA_CONF
Comment 58 Julian Maurice 2018-04-20 08:38:26 UTC
Created attachment 74600 [details] [review]
Bug 20402: Remove useless call to Koha::OAuthAccessTokens->search
Comment 59 Julian Maurice 2018-04-20 08:39:14 UTC
(In reply to Julian Maurice from comment #55)
> Actually, we should use ->find, since access_token is a primary key (->find
> is used at other places in the code, I forgot to change this one)
It was not even needed, so I removed it in the last patch
Comment 60 Martin Renvoize 2018-04-20 09:24:04 UTC
(In reply to Julian Maurice from comment #55)
> 'any' returns a boolean value, we're looking for the $client hashref here
> 
> > 'first' would be more appropriate, sorry, you are totally right, 'any' is not appropriate here at all.

> I don't think we should cache the access token, because it can expire
> 
> >Ah, you're using the 'random string' approach for the tokens so the expiry isn't embedded within the token. I've always used JWT's. Apologies, you can't cache it in that case then.
Comment 61 Tomás Cohen Arazi 2018-04-20 10:36:54 UTC
(In reply to Martin Renvoize from comment #60)
> (In reply to Julian Maurice from comment #55)
> > 'any' returns a boolean value, we're looking for the $client hashref here
> > 
> > > 'first' would be more appropriate, sorry, you are totally right, 'any' is not appropriate here at all.
> 
> > I don't think we should cache the access token, because it can expire
> > 
> > >Ah, you're using the 'random string' approach for the tokens so the expiry isn't embedded within the token. I've always used JWT's. Apologies, you can't cache it in that case then.

Martin, please review with all applied:
https://bugs.koha-community.org/bugzilla3/showdependencygraph.cgi?id=20612

Thanks!
Comment 62 Tomás Cohen Arazi 2018-05-03 19:06:00 UTC
Created attachment 75026 [details] [review]
Bug 20402: Implement OAuth2 authentication for REST API

It implements only the "client credentials" flow with no scopes
support. API clients are tied to an existing patron and have the same
permissions as the patron they are tied to.
API Clients are defined in $KOHA_CONF.

Test plan:
0. Install Net::OAuth2::AuthorizationServer 0.16
1. In $KOHA_CONF, add an <api_client> element under <config>:
     <api_client>
       <client_id>$CLIENT_ID</client_id>
       <client_secret>$CLIENT_SECRET</client_secret>
       <patron_id>X</patron_id> <!-- X is an existing borrowernumber -->
     </api_client>
2. Apply patch, run updatedatabase.pl and reload starman
3. Install Firefox extension RESTer [1]
4. In RESTer, go to "Authorization" tab and create a new OAuth2
   configuration:
   - OAuth flow: Client credentials
   - Access Token Request Method: POST
   - Access Token Request Endpoint: http://$KOHA_URL/api/v1/oauth/token
   - Access Token Request Client Authentication: Credentials in request
     body
   - Client ID: $CLIENT_ID
   - Client Secret: $CLIENT_SECRET
5. Click on the newly created configuration to generate a new token
   (which will be valid only for an hour)
6. In RESTer, set HTTP method to GET and url to
   http://$KOHA_URL/api/v1/patrons then click on SEND
   If patron X has permission 'borrowers', it should return 200 OK
   with the list of patrons
   Otherwise it should return 403 with the list of required permissions
   (Please test both cases)
7. Wait an hour (or run the following SQL query:
   UPDATE oauth_access_tokens SET expires = 0) and repeat step 6.
   You should have a 403 Forbidden status, and the token must have been
   removed from the database.
8. Create a bunch of tokens using RESTer, make some of them expires
   using the previous SQL query, and run the following command:
     misc/cronjobs/cleanup_database.pl --oauth-tokens
   Verify that expired tokens were removed, and that the others are
   still there
9. prove t/db_dependent/api/v1/oauth.t

[1] https://addons.mozilla.org/en-US/firefox/addon/rester/

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 63 Tomás Cohen Arazi 2018-05-03 19:06:07 UTC
Created attachment 75027 [details] [review]
Bug 20402: Fix oauth.t

GET /patrons now requires { "borrowers": 1 } instead of
{ "borrowers": "edit_borrowers" }

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 64 Tomás Cohen Arazi 2018-05-03 19:06:13 UTC
Created attachment 75028 [details] [review]
Bug 20402: Don't look at cookies if OAuth2 is attempted and has failed

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 65 Tomás Cohen Arazi 2018-05-03 19:06:20 UTC
Created attachment 75029 [details] [review]
Bug 20402: Remove dependency on Mojo::Plugin::OAuth2::Server

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 66 Tomás Cohen Arazi 2018-05-03 19:06:27 UTC
Created attachment 75030 [details] [review]
Bug 20402: Use TestBuilder->build_object in oauth.t

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 67 Tomás Cohen Arazi 2018-05-03 19:06:33 UTC
Created attachment 75031 [details] [review]
Bug 20402: Atomic update and kohastructure.sql fixes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 68 Tomás Cohen Arazi 2018-05-03 19:06:40 UTC
Created attachment 75032 [details] [review]
Bug 20402: Add missing POD

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 69 Tomás Cohen Arazi 2018-05-03 19:06:46 UTC
Created attachment 75033 [details] [review]
Bug 20402: Remove useless call to Koha::OAuthAccessTokens->search
Comment 70 Tomás Cohen Arazi 2018-05-03 19:07:20 UTC
Rebased.
Comment 71 Jonathan Druart 2018-05-08 19:45:17 UTC
Created attachment 75189 [details] [review]
Bug 20402: only output if verbose flag is set
Comment 72 Jonathan Druart 2018-05-09 15:57:22 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 73 David Cook 2018-07-03 06:50:20 UTC
I know this has already been pushed... but it looks like this allows for only 1 API client? What if you want to have multiple API clients?

BibLibre might have Coral talking to Koha's REST API, but maybe a client's Drupal website wants to talk to Koha's REST API too?

Right now, I suppose you could share the credentials amongst all the clients, but that doesn't seem like a great long-term option?
Comment 74 Julian Maurice 2018-07-03 07:01:51 UTC
(In reply to David Cook from comment #73)
> I know this has already been pushed... but it looks like this allows for
> only 1 API client? What if you want to have multiple API clients?

You could define as many API clients as you want in $KOHA_CONF.
But this has been replaced by bug 20568 and bug 20612. You can now define API keys in the interface.
Comment 75 David Cook 2018-07-03 09:10:16 UTC
(In reply to Julian Maurice from comment #74)
> You could define as many API clients as you want in $KOHA_CONF.
> But this has been replaced by bug 20568 and bug 20612. You can now define
> API keys in the interface.

Oooh awesome! I've wanted that feature for so long :D.