From bd3d91d2b88831ac85200bbb22ac0890de8f7056 Mon Sep 17 00:00:00 2001 From: Julian Maurice Date: Wed, 11 Apr 2018 18:37:21 +0200 Subject: [PATCH] Bug 20402: Tie client with patron, remove scopes, use mojo plugin, ... ... and move cleanup code into misc/cronjobs/cleanup_database.pl --- C4/Installer/PerlDependencies.pm | 13 +++++--- Koha/OAuth.pm | 38 +++++----------------- Koha/OAuthAccessToken.pm | 8 ----- Koha/REST/V1.pm | 3 ++ Koha/REST/V1/Auth.pm | 36 +++++++++++--------- Koha/REST/V1/OAuth.pm | 30 ++++++++--------- Koha/Schema/Result/OauthAccessToken.pm | 11 ++----- api/v1/swagger/paths/oauth.json | 6 ---- api/v1/swagger/paths/patrons.json | 10 ++---- etc/koha-conf.xml | 15 +++++++++ .../data/mysql/atomicupdate/oauth_tokens.perl | 1 - misc/cronjobs/cleanup_database.pl | 11 +++++++ misc/cronjobs/delete_expired_oauth_tokens.pl | 22 ------------- t/db_dependent/api/v1/oauth.t | 30 ++++++++++++++--- 14 files changed, 112 insertions(+), 122 deletions(-) delete mode 100755 misc/cronjobs/delete_expired_oauth_tokens.pl diff --git a/C4/Installer/PerlDependencies.pm b/C4/Installer/PerlDependencies.pm index cb5cea2037..57658b2b25 100644 --- a/C4/Installer/PerlDependencies.pm +++ b/C4/Installer/PerlDependencies.pm @@ -874,10 +874,15 @@ our $PERL_DEPS = { # also required for Zebra installs: about page: bug 20061 }, 'Net::OAuth2::AuthorizationServer' => { - 'usage' => 'REST API', - 'required' => '1', - 'min_ver' => '0.16', - }, + usage => 'REST API', + required => '1', + min_ver => '0.16', + }, + 'Mojolicious::Plugin::OAuth2::Server' => { + usage => 'REST API', + required => '1', + min_ver => '0.40', + } }; diff --git a/Koha/OAuth.pm b/Koha/OAuth.pm index b984226b22..6966570502 100644 --- a/Koha/OAuth.pm +++ b/Koha/OAuth.pm @@ -1,25 +1,22 @@ package Koha::OAuth; use Modern::Perl; -use Net::OAuth2::AuthorizationServer::ClientCredentialsGrant; use Koha::OAuthAccessTokens; use Koha::OAuthAccessToken; -sub grant { - my $grant = Net::OAuth2::AuthorizationServer::ClientCredentialsGrant->new( +sub config { + return { verify_client_cb => \&_verify_client_cb, store_access_token_cb => \&_store_access_token_cb, verify_access_token_cb => \&_verify_access_token_cb - ); - - return $grant; + }; } sub _verify_client_cb { my (%args) = @_; - my ($client_id, $scopes_ref, $client_secret) - = @args{ qw/ client_id scopes client_secret / }; + my ($client_id, $client_secret) + = @args{ qw/ client_id client_secret / }; return (0, 'unauthorized_client') unless $client_id; @@ -30,28 +27,17 @@ sub _verify_client_cb { return (0, 'access_denied') unless $client_secret eq $client->{client_secret}; - $client->{scope} //= []; - $client->{scope} = [ $client->{scope} ] if ref $client->{scope} ne 'ARRAY'; - my $client_scopes = []; - foreach my $scope ( @{ $scopes_ref // [] } ) { - if (!grep { $_ eq $scope } @{ $client->{scope} }) { - return (0, 'invalid_scope'); - } - push @$client_scopes, $scope; - } - - return (1, undef, $client_scopes); + return (1, undef, []); } sub _store_access_token_cb { my ( %args ) = @_; - my ( $client_id, $access_token, $expires_in, $scopes_ref ) - = @args{ qw/ client_id access_token expires_in scopes / }; + my ( $client_id, $access_token, $expires_in ) + = @args{ qw/ client_id access_token expires_in / }; my $at = Koha::OAuthAccessToken->new({ access_token => $access_token, - scope => join (' ', @$scopes_ref), expires => time + $expires_in, client_id => $client_id, }); @@ -63,7 +49,7 @@ sub _store_access_token_cb { sub _verify_access_token_cb { my (%args) = @_; - my ($access_token, $scopes_ref) = @args{qw(access_token scopes)}; + my $access_token = $args{access_token}; my $at = Koha::OAuthAccessTokens->find($access_token); if ($at) { @@ -72,12 +58,6 @@ sub _verify_access_token_cb { $at->delete; return (0, 'invalid_grant') - } elsif ( $scopes_ref ) { - foreach my $scope ( @{ $scopes_ref // [] } ) { - unless ($at->has_scope($scope)) { - return (0, 'invalid_grant'); - } - } } return $at->unblessed; diff --git a/Koha/OAuthAccessToken.pm b/Koha/OAuthAccessToken.pm index 91fb7de1d5..c322ea645a 100644 --- a/Koha/OAuthAccessToken.pm +++ b/Koha/OAuthAccessToken.pm @@ -4,14 +4,6 @@ use Modern::Perl; use base qw(Koha::Object); -sub has_scope { - my ($self, $scope) = @_; - - my @scopes = split / /, $self->scope; - - return scalar grep { $_ eq $scope } @scopes; -} - sub _type { return 'OauthAccessToken'; } diff --git a/Koha/REST/V1.pm b/Koha/REST/V1.pm index 57ad113291..9059ea5069 100644 --- a/Koha/REST/V1.pm +++ b/Koha/REST/V1.pm @@ -19,6 +19,8 @@ use Modern::Perl; use Mojo::Base 'Mojolicious'; +use Koha::OAuth; + use C4::Context; =head1 NAME @@ -51,6 +53,7 @@ sub startup { $self->secrets([$secret_passphrase]); } + $self->plugin('OAuth2::Server' => Koha::OAuth::config); $self->plugin(OpenAPI => { url => $self->home->rel_file("api/v1/swagger/swagger.json"), route => $self->routes->under('/api/v1')->to('Auth#under'), diff --git a/Koha/REST/V1/Auth.pm b/Koha/REST/V1/Auth.pm index 23923e6a36..89c89bf281 100644 --- a/Koha/REST/V1/Auth.pm +++ b/Koha/REST/V1/Auth.pm @@ -22,6 +22,7 @@ use Modern::Perl; use Mojo::Base 'Mojolicious::Controller'; use C4::Auth qw( check_cookie_auth get_session haspermission ); +use C4::Context; use Koha::Account::Lines; use Koha::Checkouts; @@ -110,27 +111,32 @@ sub authenticate_api_request { my ( $c ) = @_; my $spec = $c->match->endpoint->pattern->defaults->{'openapi.op_spec'}; + my $authorization = $spec->{'x-koha-authorization'}; - my $authorization_header = $c->req->headers->authorization; - if ($authorization_header) { - my $grant = Koha::OAuth->grant; - my ($type, $token) = split / /, $authorization_header; - my ($is_valid, $error) = $grant->verify_access_token( - access_token => $token, - scopes => $spec->{'x-koha-scopes'} // [], - ); + 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; - if (!$is_valid) { - Koha::Exceptions::Authorization::Unauthorized->throw( - error => $error, - required_permissions => $spec->{'x-koha-scopes'}, - ); + my $patron = Koha::Patrons->find($client->{patron_id}); + my $permissions = $authorization->{'permissions'}; + # Check if the patron is authorized + if ( haspermission($patron->userid, $permissions) + or allow_owner($c, $authorization, $patron) + or allow_guarantor($c, $authorization, $patron) ) { + + validate_query_parameters( $c, $spec ); + + # Everything is ok + return 1; } - return 1; + Koha::Exceptions::Authorization::Unauthorized->throw( + error => "Authorization failure. Missing required permission(s).", + required_permissions => $permissions, + ); } - my $authorization = $spec->{'x-koha-authorization'}; my $cookie = $c->cookie('CGISESSID'); my ($session, $user); # Mojo doesn't use %ENV the way CGI apps do diff --git a/Koha/REST/V1/OAuth.pm b/Koha/REST/V1/OAuth.pm index 78a2d1e5b0..7be1a46e27 100644 --- a/Koha/REST/V1/OAuth.pm +++ b/Koha/REST/V1/OAuth.pm @@ -3,6 +3,8 @@ package Koha::REST::V1::OAuth; use Modern::Perl; use Mojo::Base 'Mojolicious::Controller'; + +use Net::OAuth2::AuthorizationServer; use Koha::OAuth; use C4::Context; @@ -17,15 +19,15 @@ sub token { my $client_id = $c->validation->param('client_id'); my $client_secret = $c->validation->param('client_secret'); - my $scope = [ split / /, ($c->validation->param('scope') // '') ]; - my $grant = Koha::OAuth->grant; + my $cb = "${grant_type}_grant"; + my $server = Net::OAuth2::AuthorizationServer->new; + my $grant = $server->$cb(Koha::OAuth::config); # verify a client against known clients - my ( $is_valid, $error, $scopes ) = $grant->verify_client( - client_id => $client_id, - client_secret => $client_secret, - scopes => $scope, + my ( $is_valid, $error ) = $grant->verify_client( + client_id => $client_id, + client_secret => $client_secret, ); unless ($is_valid) { @@ -34,23 +36,19 @@ sub token { # generate a token my $token = $grant->token( - client_id => $client_id, - scopes => $scopes, - type => 'access', + client_id => $client_id, + type => 'access', ); # store access token my $expires_in = 3600; $grant->store_access_token( - client_id => $client_id, - access_token => $token, - expires_in => $expires_in, - scopes => $scopes, + client_id => $client_id, + access_token => $token, + expires_in => $expires_in, ); - my $at = Koha::OAuthAccessTokens->search({ - access_token => $token, - })->next; + my $at = Koha::OAuthAccessTokens->search({ access_token => $token })->next; my $response = { access_token => $token, diff --git a/Koha/Schema/Result/OauthAccessToken.pm b/Koha/Schema/Result/OauthAccessToken.pm index ba70bc6d04..85b8a53fe1 100644 --- a/Koha/Schema/Result/OauthAccessToken.pm +++ b/Koha/Schema/Result/OauthAccessToken.pm @@ -35,11 +35,6 @@ __PACKAGE__->table("oauth_access_tokens"); is_nullable: 0 size: 255 -=head2 scope - - data_type: 'text' - is_nullable: 1 - =head2 expires data_type: 'integer' @@ -52,8 +47,6 @@ __PACKAGE__->add_columns( { data_type => "varchar", is_nullable => 0, size => 255 }, "client_id", { data_type => "varchar", is_nullable => 0, size => 255 }, - "scope", - { data_type => "text", is_nullable => 1 }, "expires", { data_type => "integer", is_nullable => 0 }, ); @@ -71,8 +64,8 @@ __PACKAGE__->add_columns( __PACKAGE__->set_primary_key("access_token"); -# Created by DBIx::Class::Schema::Loader v0.07046 @ 2018-03-14 12:13:59 -# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:VgSA5BIbeUR31WV1YwbfEQ +# Created by DBIx::Class::Schema::Loader v0.07046 @ 2018-04-11 17:44:30 +# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:u2e++Jrwln4Qhi3UPx2CQA # You can replace this text with custom code or comments, and it will be preserved on regeneration diff --git a/api/v1/swagger/paths/oauth.json b/api/v1/swagger/paths/oauth.json index f2c0c5a718..4b00db605a 100644 --- a/api/v1/swagger/paths/oauth.json +++ b/api/v1/swagger/paths/oauth.json @@ -26,12 +26,6 @@ "in": "formData", "description": "client secret", "type": "string" - }, - { - "name": "scope", - "in": "formData", - "description": "space-separated list of scopes", - "type": "string" } ], "responses": { diff --git a/api/v1/swagger/paths/patrons.json b/api/v1/swagger/paths/patrons.json index e253f6ac08..565d20c26e 100644 --- a/api/v1/swagger/paths/patrons.json +++ b/api/v1/swagger/paths/patrons.json @@ -46,10 +46,7 @@ "permissions": { "borrowers": "edit_borrowers" } - }, - "x-koha-scopes": [ - "patrons.read" - ] + } } }, "/patrons/{borrowernumber}": { @@ -108,10 +105,7 @@ "permissions": { "borrowers": "edit_borrowers" } - }, - "x-koha-scopes": [ - "patrons.read" - ] + } } } } diff --git a/etc/koha-conf.xml b/etc/koha-conf.xml index 268175dc67..6087e13318 100644 --- a/etc/koha-conf.xml +++ b/etc/koha-conf.xml @@ -127,6 +127,21 @@ __PAZPAR2_TOGGLE_XML_POST__ CHANGEME + + + __FONT_DIR__/DejaVuSerif.ttf diff --git a/installer/data/mysql/atomicupdate/oauth_tokens.perl b/installer/data/mysql/atomicupdate/oauth_tokens.perl index 10d5eae7f5..e22df6f9bd 100644 --- a/installer/data/mysql/atomicupdate/oauth_tokens.perl +++ b/installer/data/mysql/atomicupdate/oauth_tokens.perl @@ -5,7 +5,6 @@ if (CheckVersion($DBversion)) { CREATE TABLE oauth_access_tokens ( access_token VARCHAR(255) NOT NULL, client_id VARCHAR(255) NOT NULL, - scope TEXT, expires INT NOT NULL, PRIMARY KEY (access_token) ) ENGINE=InnoDB DEFAULT CHARSET=utf8 diff --git a/misc/cronjobs/cleanup_database.pl b/misc/cronjobs/cleanup_database.pl index 03c382d470..9fb2ffae2d 100755 --- a/misc/cronjobs/cleanup_database.pl +++ b/misc/cronjobs/cleanup_database.pl @@ -84,6 +84,7 @@ Usage: $0 [-h|--help] [--sessions] [--sessdays DAYS] [-v|--verbose] [--zebraqueu --temp-uploads Delete temporary uploads. --temp-uploads-days DAYS Override the corresponding preference value. --uploads-missing FLAG Delete upload records for missing files when FLAG is true, count them otherwise + --oauth-tokens Delete expired OAuth2 tokens USAGE exit $_[0]; } @@ -109,6 +110,7 @@ my $special_holidays_days; my $temp_uploads; my $temp_uploads_days; my $uploads_missing; +my $oauth_tokens; GetOptions( 'h|help' => \$help, @@ -132,6 +134,7 @@ GetOptions( 'temp-uploads' => \$temp_uploads, 'temp-uploads-days:i' => \$temp_uploads_days, 'uploads-missing:i' => \$uploads_missing, + 'oauth-tokens' => \$oauth_tokens, ) || usage(1); # Use default values @@ -165,6 +168,7 @@ unless ( $sessions || $special_holidays_days || $temp_uploads || defined $uploads_missing + || $oauth_tokens ) { print "You did not specify any cleanup work for the script to do.\n\n"; usage(1); @@ -336,6 +340,13 @@ if( defined $uploads_missing ) { } } +if ($oauth_tokens) { + require Koha::OAuthAccessTokens; + + my $count = int Koha::OAuthAccessTokens->search({ expires => { '<=', time } })->delete; + say "Removed $count expired OAuth2 tokens"; +} + exit(0); sub RemoveOldSessions { diff --git a/misc/cronjobs/delete_expired_oauth_tokens.pl b/misc/cronjobs/delete_expired_oauth_tokens.pl deleted file mode 100755 index 05317ee1ce..0000000000 --- a/misc/cronjobs/delete_expired_oauth_tokens.pl +++ /dev/null @@ -1,22 +0,0 @@ -#!/usr/bin/env perl - -# This file is part of Koha. -# -# Koha is free software; you can redistribute it and/or modify it under the -# terms of the GNU General Public License as published by the Free Software -# Foundation; either version 3 of the License, or (at your option) any later -# version. -# -# Koha is distributed in the hope that it will be useful, but WITHOUT ANY -# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR -# A PARTICULAR PURPOSE. See the GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License along -# with Koha; if not, write to the Free Software Foundation, Inc., -# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. - -use Modern::Perl; - -use Koha::OAuthAccessTokens; - -Koha::OAuthAccessTokens->search({ expires => { '<=', time } })->delete; diff --git a/t/db_dependent/api/v1/oauth.t b/t/db_dependent/api/v1/oauth.t index e81118d408..79d9eceb02 100755 --- a/t/db_dependent/api/v1/oauth.t +++ b/t/db_dependent/api/v1/oauth.t @@ -23,15 +23,25 @@ use Test::Mojo; use Koha::Database; use t::lib::Mocks; +use t::lib::TestBuilder; my $t = Test::Mojo->new('Koha::REST::V1'); my $schema = Koha::Database->new->schema; +my $builder = t::lib::TestBuilder->new(); subtest '/oauth/token tests' => sub { - plan tests => 17; + plan tests => 19; $schema->storage->txn_begin; + my $patron = $builder->build({ + source => 'Borrower', + value => { + surname => 'Test OAuth', + flags => 0, + }, + }); + # Missing parameter grant_type $t->post_ok('/api/v1/oauth/token') ->status_is(400); @@ -50,14 +60,13 @@ subtest '/oauth/token tests' => sub { t::lib::Mocks::mock_config('api_client', { 'client_id' => $client_id, 'client_secret' => $client_secret, - 'scope' => ['patrons.read'], + patron_id => $patron->{borrowernumber}, }); my $formData = { grant_type => 'client_credentials', client_id => $client_id, client_secret => $client_secret, - scope => 'patrons.read', }; $t->post_ok('/api/v1/oauth/token', form => $formData) ->status_is(200) @@ -70,9 +79,22 @@ subtest '/oauth/token tests' => sub { # Without access token, it returns 401 $t->get_ok('/api/v1/patrons')->status_is(401); - # With access token, it returns 200 + # With access token, but without permissions, it returns 403 my $tx = $t->ua->build_tx(GET => '/api/v1/patrons'); $tx->req->headers->authorization("Bearer $access_token"); + $t->request_ok($tx)->status_is(403); + + # With access token and permissions, it returns 200 + $builder->build({ + source => 'UserPermission', + value => { + borrowernumber => $patron->{borrowernumber}, + module_bit => 4, # borrowers + code => 'edit_borrowers', + }, + }); + $tx = $t->ua->build_tx(GET => '/api/v1/patrons'); + $tx->req->headers->authorization("Bearer $access_token"); $t->request_ok($tx)->status_is(200); $schema->storage->txn_rollback; -- 2.14.2