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/
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'.
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/
Created attachment 72887 [details] [review] Bug 20402: Add Net::OAuth2::AuthorizationServer to perl deps
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'.
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.
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>
Created attachment 72893 [details] [review] Bug 20402: Add Net::OAuth2::AuthorizationServer to perl deps Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
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>
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.
Julian: Unit tests ?
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
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
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.
(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 ?
(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.
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?
(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.
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
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
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 :))
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/
All patches squashed and rebased on master. Please test !
(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/
(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.
(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
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/
Created attachment 74051 [details] [review] Bug 20402: Fix oauth.t GET /patrons now requires { "borrowers": 1 } instead of { "borrowers": "edit_borrowers" }
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
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>
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>
(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' ?
(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
Created attachment 74088 [details] [review] Bug 20402: Don't look at cookies if OAuth2 is attempted and has failed
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>
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>
Created attachment 74367 [details] [review] Bug 20402: Don't look at cookies if OAuth2 is attempted and has failed
Created attachment 74368 [details] [review] Bug 20402: Remove dependency on Mojo::Plugin::OAuth2::Server
Created attachment 74369 [details] [review] Bug 20402: Use TestBuilder->build_object in oauth.t
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>
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>
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>
Created attachment 74383 [details] [review] Bug 20402: Remove dependency on Mojo::Plugin::OAuth2::Server Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 74384 [details] [review] Bug 20402: Use TestBuilder->build_object in oauth.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 74385 [details] [review] Bug 20402: Atomic update and kohastructure.sql fixes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 74386 [details] [review] Bug 20402: Add missing POD Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Only minor fixes were needed. Works as expected.
How hard will it be to package Net::OAuth2::AuthorizationServer and deps?
It the key is revoked or deleted, the token is still valid, is it expected?
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 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 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'?
(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)
(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
(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
Created attachment 74600 [details] [review] Bug 20402: Remove useless call to Koha::OAuthAccessTokens->search
(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
(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.
(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!
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>
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>
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>
Created attachment 75029 [details] [review] Bug 20402: Remove dependency on Mojo::Plugin::OAuth2::Server Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 75030 [details] [review] Bug 20402: Use TestBuilder->build_object in oauth.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 75031 [details] [review] Bug 20402: Atomic update and kohastructure.sql fixes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 75032 [details] [review] Bug 20402: Add missing POD Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 75033 [details] [review] Bug 20402: Remove useless call to Koha::OAuthAccessTokens->search
Rebased.
Created attachment 75189 [details] [review] Bug 20402: only output if verbose flag is set
Pushed to master for 18.05, thanks to everybody involved!
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?
(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.
(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.