From a80899dddccfe1e8f9e977344aec652860639dc7 Mon Sep 17 00:00:00 2001 From: Olli-Antti Kivilahti Date: Tue, 23 Jun 2015 12:17:04 +0300 Subject: [PATCH] Bug 14437 - Refactor C4::Auth::haspermission() to Koha::Object and return better errors. Refactoring haspermission() and improving documentation. Also added a return value unintrusively (in the old context still RETURNS the same result) to tell which permission failed. Modification should have no adverse effects, other than killing the process (by throwing uncaught exceptions) if no valid parameters are given. Depends on Buugg 13995 - Proper Exception handling so apply it first. Cannot create unit tests because there is no CRUD interface for permissions. Not even with TestBuilder. TEST PLAN: 1. Find a borrower with only the catalogue-permission. 2. try opening this page cgi-bin/koha/tools/manage-marc-import.pl 3. You get "Permission denied" --- C4/Auth.pm | 145 +++++++++++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 118 insertions(+), 27 deletions(-) diff --git a/C4/Auth.pm b/C4/Auth.pm index 0d21575..d7feb99 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -23,6 +23,7 @@ use Digest::MD5 qw(md5_base64); use JSON qw/encode_json/; use URI::Escape; use CGI::Session; +use Scalar::Util qw(blessed); require Exporter; use C4::Context; @@ -31,11 +32,15 @@ use C4::Languages; use C4::Branch; # GetBranches use C4::Search::History; use Koha; +use Koha::Borrowers; use Koha::AuthUtils qw(hash_password); use POSIX qw/strftime/; use List::MoreUtils qw/ any /; use Encode qw( encode is_utf8); +use Koha::Exception::BadParameter; +use Koha::Exception::UnknownObject; + # use utf8; use vars qw($VERSION @ISA @EXPORT @EXPORT_OK %EXPORT_TAGS $debug $ldap $cas $caslogout $shib $shib_login); @@ -1915,22 +1920,75 @@ sub get_all_subpermissions { } =head2 haspermission +PUBLIC + + my ($failedPermission, $borrowerPermissions) = C4::Auth::haspermission($borrowerOrIdentifier, $flagsrequired); + #or + #If you don't care about the reason why the permission check failed + my $borrowerPermissions = C4::Auth::haspermission($borrowerOrIdentifier, $flagsrequired); + #or + #Just get the borrower permissions + my $borrowerPermissions = C4::Auth::haspermission($borrowerOrIdentifier); + +Finds the borrower's permission flags or checks if the borrower has the proper +permissions if $flagsrequired is given. + +C<$borrowerOrIdentifier> MANDATORY, either the userid of the borrower + or a Koha::Borrower-object +C<$flagsrequired> OPTIONAL, is a hashref of required flags + { borrowers => 1, #Must have all borrowers permission + circulate => *, #Must have any circulate permission + tools => 'stage_marc_import', #Must have the specific permission + } - $flags = ($userid, $flagsrequired); - -C<$userid> the userid of the member -C<$flags> is a hashref of required flags like C<$borrower-<{authflags}> - -Returns member's flags or 0 if a permission is not met. - +C A List of String and a Reference to a HASH of HASHes, member's flags. Eg. + C + ( + undef, ##No failed permission + { tools => 1, ##borrower permissions + catalogue => 1, #All catalogue permissions + circulate => { override_renewals => 1, + force_checkout => 1, + }, + lists => 0, #No lists permissions + ... + } + ) + C + ( + "circulate => force_checkout", ##The failed permission, see. C4::Auth::check_permissions() + 0 ##Notify that the check failed. + ) + +C Koha::Exception::UnknownObject, if no borrower is found. + Koha::Exception::BadParameter, if parameters are not properly given. =cut sub haspermission { - my ( $userid, $flagsrequired ) = @_; - my $sth = C4::Context->dbh->prepare("SELECT flags FROM borrowers WHERE userid=?"); - $sth->execute($userid); - my $row = $sth->fetchrow(); - my $flags = getuserflags( $row, $userid ); + my ( $borrOrId, $flagsrequired ) = @_; + my ($borrower); + + ##Validate parameters since this is a public subroutine + #The best thing would be to get a honest reference to the Borrower-object, + #but currently we can only prepare that some day somebody will use me properly. + if (blessed($borrOrId) && $borrOrId->isa('Koha::Borrower')) { + $borrower = $borrOrId; + } + #We can get back to the DBIx to refetch the Borrower-object from the given borrower identifier, again. + elsif ($borrOrId && length($borrOrId) > 0) { + my @b = Koha::Borrowers->search( {userid => $borrOrId} ); + unless (scalar(@b)) { + Koha::Exception::UnknownObject->throw(error => __PACKAGE__."::haspermission():> No borrower found with the given borrower identifier '$borrOrId'."); + } + $borrower = $b[0]; + } + else { + Koha::Exception::BadParameter->throw(error => __PACKAGE__."::haspermission():> No borrower or borrowerIdentifier given."); + } + + ##Parameters validated, time to get the flags! + my $userid = $borrower->userid(); + my $flags = getuserflags( $borrower->flags(), $userid ); if ( $userid eq C4::Context->config('user') ) { # Super User Account from /etc/koha.conf @@ -1942,26 +2000,59 @@ sub haspermission { $flags->{'superlibrarian'} = 1; } - return $flags if $flags->{superlibrarian}; + #This fcn returns the failed permission so a suitable error msg can be delivered. + my $failedPermission = ($flagsrequired) ? check_permissions($flagsrequired, $flags) : undef; + if ($failedPermission) { + return ($failedPermission, 0); + } + return ($failedPermission, $flags); +} + +=head check_permissions +PUBLIC + + my $failedPermission = check_permissions($permissionsRequired, $permissions); + +Check if the given $permissions satisfy the $permissionsRequired. +@PARAM1 Reference to HASH of HASHes, see. C4::Auth::haspermission() $flagsrequired +@PARAM2 Reference to HASH of HASHes, see. C4::Auth::haspermission() @RETURNS +@RETURNS String, the failed permission, eg: + "circulate == 1", or "circulate == *", or "circulate => force_checkout" + or + undef if everything went ok. +@THROWS Koha::Exception::BadParameter, if all parameters are not properly given. +=cut + +sub check_permissions { + my ($permissionsRequired, $permissions) = @_; - foreach my $module ( keys %$flagsrequired ) { - my $subperm = $flagsrequired->{$module}; + ##Validate parameters, since this is a public subroutine + unless ($permissionsRequired && ref($permissionsRequired) eq 'HASH' && scalar(%$permissionsRequired)) { + Koha::Exception::BadParameter->throw(error => __PACKAGE__."::check_permission():> Mandatory parameter \$permissionsRequired is not given or is not a reference to HASH, or the HASH is empty."); + } + unless ($permissions && ref($permissions) eq 'HASH' && scalar(%$permissions)) { + Koha::Exception::BadParameter->throw(error => __PACKAGE__."::check_permission():> Mandatory parameter \$permissions is not given or is not a reference to HASH, or the HASH is empty."); + } + + ##Start checking permissions + return undef if $permissions->{superlibrarian}; + + my $failedPermission; + foreach my $module ( keys %$permissionsRequired ) { + my $subperm = $permissionsRequired->{$module}; if ( $subperm eq '*' ) { - return 0 unless ( $flags->{$module} == 1 or ref( $flags->{$module} ) ); + $failedPermission = {$module => '*'} unless ( $permissions->{$module} == 1 or + ref( $permissions->{$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 ) - ); + $failedPermission = {$module => '1'} unless ( defined($permissions->{$module}) and + $permissions->{$module} == 1 ); + $failedPermission = {$module => $subperm} unless ( ref($permissions->{$module}) and + exists($permissions->{$module}->{$subperm}) and + $permissions->{$module}->{$subperm} == 1 ); } + last() if $failedPermission; } - return $flags; - - #FIXME - This fcn should return the failed permission so a suitable error msg can be delivered. + return $failedPermission || undef; } sub getborrowernumber { -- 1.7.9.5