Attachment 74009 Details for Bug 20402 – Bug 20402: Tie client with patron, remove scopes, use mojo plugin, ...

[patch] Bug 20402: Tie client with patron, remove scopes, use mojo plugin, ...

Bug-20402-Tie-client-with-patron-remove-scopes-use.patch (text/plain), 18.62 KB, created by Julian Maurice on 2018-04-11 16:41:45 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Julian Maurice

Created: 2018-04-11 16:41:45 UTC

Size: 18.62 KB

patch

obsolete

>From bd3d91d2b88831ac85200bbb22ac0890de8f7056 Mon Sep 17 00:00:00 2001
>From: Julian Maurice <julian.maurice@biblibre.com>
>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__
>  
>  <api_secret_passphrase>CHANGEME</api_secret_passphrase>
> 
>+ 
>+ 
>+
>  
>  <ttf>
>     <font type="TR" >__FONT_DIR__/DejaVuSerif.ttf</font>
>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

Actions: View | Diff | Splinter Review

Attachments on bug 20402: 72865 | 72874 | 72886 | 72887 | 72888 | 72892 | 72893 | 72894 | 73086 | 73087 | 74009 | 74018 | 74050 | 74051 | 74069 | 74070 | 74088 | 74365 | 74366 | 74367 | 74368 | 74369 | 74380 | 74381 | 74382 | 74383 | 74384 | 74385 | 74386 | 74600 | 75026 | 75027 | 75028 | 75029 | 75030 | 75031 | 75032 | 75033 | 75189