Bugzilla – Attachment 40513 Details for
Bug 14437
Refactor C4::Auth::haspermission() to Koha::Object and return better errors.
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 14437 - Refactor C4::Auth::haspermission() to Koha::Object and return better errors.
Bug-14437---Refactor-C4Authhaspermission-to-KohaOb.patch (text/plain), 9.08 KB, created by
Olli-Antti Kivilahti
on 2015-06-23 09:18:19 UTC
(
hide
)
Description:
Bug 14437 - Refactor C4::Auth::haspermission() to Koha::Object and return better errors.
Filename:
MIME Type:
Creator:
Olli-Antti Kivilahti
Created:
2015-06-23 09:18:19 UTC
Size:
9.08 KB
patch
obsolete
>From d00c8af42bba6ba883feaf0aa00abd4084df8e5a Mon Sep 17 00:00:00 2001 >From: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> >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..5e86991 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<RETURNS> A List of String and a Reference to a HASH of HASHes, member's flags. Eg. >+ C<A successful permissions check> >+ ( >+ 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<A failed permissions check> >+ ( >+ "circulate => force_checkout", ##The failed permission, see. C4::Auth::check_permissions() >+ 0 ##Notify that the check failed. >+ ) >+ >+C<THROWS> 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
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 14437
:
40513
|
40571
|
40718
|
40719