From bf748654132787cb78b4ee2baab2b5cb5495ba1f Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Sat, 22 Dec 2018 13:55:23 +0000 Subject: [PATCH] Bug 22031: Add SQL::Abstract like syntax to haspermission This patch adds an SQL::Abstract inspired query syntax to the haspermission method in C4::Auth. One can now pass Arrayrefs to denote an OR list of flags, a Hashref to denote a AND list of flags. Structures can be nested at arbitrary depth. --- C4/Auth.pm | 59 +++++-- t/db_dependent/Auth/haspermission.t | 248 +++++++++++++++++++--------- 2 files changed, 214 insertions(+), 93 deletions(-) diff --git a/C4/Auth.pm b/C4/Auth.pm index b19197b35b..8932066d48 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -19,6 +19,8 @@ package C4::Auth; use strict; use warnings; +use Carp qw/croak/; + use Digest::MD5 qw(md5_base64); use JSON qw/encode_json/; use URI::Escape; @@ -2025,12 +2027,49 @@ sub get_all_subpermissions { $flags = ($userid, $flagsrequired); C<$userid> the userid of the member -C<$flags> is a hashref of required flags like C<$borrower-<{authflags}> +C<$flags> is a query structure similar to that used by SQL::Abstract that +denotes the combination of flags required. + +The main logic of this method is that things in arrays are OR'ed, and things +in hashes are AND'ed. Returns member's flags or 0 if a permission is not met. =cut +sub _dispatch { + my ($required, $flags) = @_; + + my $ref = ref($required); + if ($ref eq '') { + if ($required eq '*') { + return 0 unless ( $flags or ref( $flags ) ); + } else { + return 0 unless ( $flags and (!ref( $flags ) || $flags->{$required} )); + } + } elsif ($ref eq 'HASH') { + foreach my $key (keys %{$required}) { + my $require = $required->{$key}; + my $rflags = $flags->{$key}; + return 0 unless _dispatch($require, $rflags); + } + } elsif ($ref eq 'ARRAY') { + my $satisfied = 0; + foreach my $require ( @{$required} ) { + my $rflags = + ( ref($flags) && !ref($require) && ( $require ne '*' ) ) + ? $flags->{$require} + : $flags; + $satisfied++ if _dispatch( $require, $rflags ); + } + return 0 unless $satisfied; + } else { + croak "Unexpected structure found: $ref"; + } + + return $flags; +}; + sub haspermission { my ( $userid, $flagsrequired ) = @_; my $sth = C4::Context->dbh->prepare("SELECT flags FROM borrowers WHERE userid=?"); @@ -2039,23 +2078,7 @@ sub haspermission { my $flags = getuserflags( $row, $userid ); return $flags if $flags->{superlibrarian}; - - foreach my $module ( keys %$flagsrequired ) { - my $subperm = $flagsrequired->{$module}; - if ( $subperm eq '*' ) { - return 0 unless ( $flags->{$module} == 1 or ref( $flags->{$module} ) ); - } else { - return 0 unless ( - ( defined $flags->{$module} and - $flags->{$module} == 1 ) - or - ( ref( $flags->{$module} ) and - exists $flags->{$module}->{$subperm} and - $flags->{$module}->{$subperm} == 1 ) - ); - } - } - return $flags; + return _dispatch($flagsrequired, $flags); #FIXME - This fcn should return the failed permission so a suitable error msg can be delivered. } diff --git a/t/db_dependent/Auth/haspermission.t b/t/db_dependent/Auth/haspermission.t index bf0290d6be..672832b9ee 100644 --- a/t/db_dependent/Auth/haspermission.t +++ b/t/db_dependent/Auth/haspermission.t @@ -20,7 +20,7 @@ # along with Koha; if not, see . use Modern::Perl; -use Test::More tests => 13; +use Test::More tests => 3; use Koha::Database; use t::lib::TestBuilder; @@ -31,78 +31,176 @@ $schema->storage->txn_begin; # Adding two borrowers and granular permissions for the second borrower my $builder = t::lib::TestBuilder->new(); -my $borr1 = $builder->build({ - source => 'Borrower', - value => { - surname => 'Superlib', - flags => 1, - }, -}); -my $borr2 = $builder->build({ - source => 'Borrower', - value => { - surname => 'Bor2', - flags => 2 + 4 + 2**11, # circulate, catalogue, acquisition - }, -}); -$builder->build({ - source => 'UserPermission', - value => { - borrowernumber => $borr2->{borrowernumber}, - module_bit => 13, # tools - code => 'upload_local_cover_images', - }, -}); -$builder->build({ - source => 'UserPermission', - value => { - borrowernumber => $borr2->{borrowernumber}, - module_bit => 13, # tools - code => 'batch_upload_patron_images', - }, -}); - -# Check top level permission for superlibrarian -my $r = haspermission( $borr1->{userid}, - { circulate => 1, editcatalogue => 1 } ); -is( ref($r), 'HASH', 'Superlibrarian/circulate' ); - -# Check specific top level permission(s) for borr2 -$r = haspermission( $borr2->{userid}, - { circulate => 1, catalogue => 1 } ); -is( ref($r), 'HASH', 'Borrower2/circulate' ); -$r = haspermission( $borr2->{userid}, { updatecharges => 1 } ); -is( $r, 0, 'Borrower2/updatecharges should fail' ); - -# Check granular permission with 1: means all subpermissions -$r = haspermission( $borr1->{userid}, { tools => 1 } ); -is( ref($r), 'HASH', 'Superlibrarian/tools granular all' ); -$r = haspermission( $borr2->{userid}, { tools => 1 } ); -is( $r, 0, 'Borrower2/tools granular all should fail' ); - -# Check granular permission with *: means at least one subpermission -$r = haspermission( $borr1->{userid}, { tools => '*' } ); -is( ref($r), 'HASH', 'Superlibrarian/tools granular *' ); -$r = haspermission( $borr2->{userid}, { acquisition => '*' } ); -is( ref($r), 'HASH', 'Borrower2/acq granular *' ); -$r = haspermission( $borr2->{userid}, { tools => '*' } ); -is( ref($r), 'HASH', 'Borrower2/tools granular *' ); -$r = haspermission( $borr2->{userid}, { serials => '*' } ); -is( $r, 0, 'Borrower2/serials granular * should fail' ); - -# Check granular permission with one or more specific subperms -$r = haspermission( $borr1->{userid}, { tools => 'edit_news' } ); -is( ref($r), 'HASH', 'Superlibrarian/tools edit_news' ); -$r = haspermission( $borr2->{userid}, { acquisition => 'budget_manage' } ); -is( ref($r), 'HASH', 'Borrower2/acq budget_manage' ); -$r = haspermission( $borr2->{userid}, - { acquisition => 'budget_manage', tools => 'edit_news' } ); -is( $r, 0, 'Borrower2/two granular should fail' ); -$r = haspermission( $borr2->{userid}, { - tools => 'upload_local_cover_images', - tools => 'batch_upload_patron_images', -}); -is( ref($r), 'HASH', 'Borrower2/tools granular two upload subperms' ); - -# End +my $borr1 = $builder->build( + { + source => 'Borrower', + value => { + surname => 'Superlib', + flags => 1, + }, + } +); +my $borr2 = $builder->build( + { + source => 'Borrower', + value => { + surname => 'Bor2', + flags => 2 + 4 + 2**11, # circulate, catalogue, acquisition + }, + } +); +$builder->build( + { + source => 'UserPermission', + value => { + borrowernumber => $borr2->{borrowernumber}, + module_bit => 13, # tools + code => 'upload_local_cover_images', + }, + } +); +$builder->build( + { + source => 'UserPermission', + value => { + borrowernumber => $borr2->{borrowernumber}, + module_bit => 13, # tools + code => 'batch_upload_patron_images', + }, + } +); + +subtest 'scalar top level tests' => sub { + + plan tests => 3; + + # Check top level permission for superlibrarian + my $r = haspermission( $borr1->{userid}, 'circulate' ); + is( ref($r), 'HASH', 'Superlibrarian/circulate' ); + + # Check specific top level permission(s) for borr2 + $r = haspermission( $borr2->{userid}, 'circulate' ); + is( ref($r), 'HASH', 'Borrower2/circulate' ); + $r = haspermission( $borr2->{userid}, 'updatecharges' ); + is( $r, 0, 'Borrower2/updatecharges should fail' ); +}; + +subtest 'hashref top level AND tests' => sub { + + plan tests => 15; + + # Check top level permission for superlibrarian + my $r = + haspermission( $borr1->{userid}, { circulate => 1 } ); + is( ref($r), 'HASH', 'Superlibrarian/circulate' ); + + # Check specific top level permission(s) for borr2 + $r = haspermission( $borr2->{userid}, { circulate => 1, catalogue => 1 } ); + is( ref($r), 'HASH', 'Borrower2/circulate' ); + $r = haspermission( $borr2->{userid}, { updatecharges => 1 } ); + is( $r, 0, 'Borrower2/updatecharges should fail' ); + + # Check granular permission with 1: means all subpermissions + $r = haspermission( $borr1->{userid}, { tools => 1 } ); + is( ref($r), 'HASH', 'Superlibrarian/tools granular all' ); + $r = haspermission( $borr2->{userid}, { tools => 1 } ); + is( $r, 0, 'Borrower2/tools granular all should fail' ); + + # Check granular permission with *: means at least one subpermission + $r = haspermission( $borr1->{userid}, { tools => '*' } ); + is( ref($r), 'HASH', 'Superlibrarian/tools granular *' ); + $r = haspermission( $borr2->{userid}, { acquisition => '*' } ); + is( ref($r), 'HASH', 'Borrower2/acq granular *' ); + $r = haspermission( $borr2->{userid}, { tools => '*' } ); + is( ref($r), 'HASH', 'Borrower2/tools granular *' ); + $r = haspermission( $borr2->{userid}, { serials => '*' } ); + is( $r, 0, 'Borrower2/serials granular * should fail' ); + + # Check granular permission with one or more specific subperms + $r = haspermission( $borr1->{userid}, { tools => 'edit_news' } ); + is( ref($r), 'HASH', 'Superlibrarian/tools edit_news' ); + $r = haspermission( $borr2->{userid}, { acquisition => 'budget_manage' } ); + is( ref($r), 'HASH', 'Borrower2/acq budget_manage' ); + $r = haspermission( $borr2->{userid}, + { acquisition => 'budget_manage', tools => 'edit_news' } ); + is( $r, 0, 'Borrower2 (/acquisition|budget_manage AND /tools|edit_news) should fail' ); + $r = haspermission( + $borr2->{userid}, + { + tools => { + 'upload_local_cover_images' => 1, + 'batch_upload_patron_images' => 1 + }, + } + ); + is( ref($r), 'HASH', 'Borrower2 (/tools|upload_local_cover_image AND /tools|batch_upload_patron_images) granular' ); + $r = haspermission( + $borr2->{userid}, + { + tools => { + 'upload_local_cover_images' => 1, + 'edit_news' => 1 + }, + } + ); + is( $r, 0, 'Borrower2 (/tools|upload_local_cover_image AND /tools|edit_news) granular' ); + $r = haspermission( + $borr2->{userid}, + { + tools => [ 'upload_local_cover_images', 'edit_news'], + } + ); + is( ref($r), 'HASH', 'Borrower2 (/tools|upload_local_cover_image OR /tools|edit_news) granular' ); +}; + +subtest 'arrayref top level OR tests' => sub { + + plan tests => 13; + + # Check top level permission for superlibrarian + my $r = + haspermission( $borr1->{userid}, [ 'circulate', 'editcatalogue' ] ); + is( ref($r), 'HASH', 'Superlibrarian/circulate' ); + + # Check specific top level permission(s) for borr2 + $r = haspermission( $borr2->{userid}, [ 'circulate', 'updatecharges' ] ); + is( ref($r), 'HASH', 'Borrower2/circulate OR Borrower2/updatecharges' ); + $r = haspermission( $borr2->{userid}, ['updatecharges', 'serials' ] ); + is( $r, 0, 'Borrower2/updatecharges OR Borrower2/serials should fail' ); + + # Check granular permission with 1: means all subpermissions + $r = haspermission( $borr1->{userid}, [ 'tools' ] ); + is( ref($r), 'HASH', 'Superlibrarian/tools granular all' ); + $r = haspermission( $borr2->{userid}, [ 'tools' ] ); + is( $r, 0, 'Borrower2/tools granular all should fail' ); + + # Check granular permission with *: means at least one subpermission + $r = haspermission( $borr1->{userid}, [ { tools => '*' } ] ); + is( ref($r), 'HASH', 'Superlibrarian/tools granular *' ); + $r = haspermission( $borr2->{userid}, [ { acquisition => '*' } ] ); + is( ref($r), 'HASH', 'Borrower2/acq granular *' ); + $r = haspermission( $borr2->{userid}, [ { tools => '*' } ] ); + is( ref($r), 'HASH', 'Borrower2/tools granular *' ); + $r = haspermission( $borr2->{userid}, [ { serials => '*' } ] ); + is( $r, 0, 'Borrower2/serials granular * should fail' ); + + # Check granular permission with one or more specific subperms + $r = haspermission( $borr1->{userid}, [ { tools => 'edit_news' } ] ); + is( ref($r), 'HASH', 'Superlibrarian/tools edit_news' ); + $r = + haspermission( $borr2->{userid}, [ { acquisition => 'budget_manage' } ] ); + is( ref($r), 'HASH', 'Borrower2/acq budget_manage' ); + $r = haspermission( $borr2->{userid}, + [ { acquisition => 'budget_manage'}, { tools => 'edit_news' } ] ); + is( ref($r), 'HASH', 'Borrower2/two granular OR should pass' ); + $r = haspermission( + $borr2->{userid}, + [ + { tools => ['upload_local_cover_images'] }, + { tools => ['edit_news'] } + ] + ); + is( ref($r), 'HASH', 'Borrower2/tools granular OR subperms' ); +}; + $schema->storage->txn_rollback; -- 2.19.2