[Bug 26383] New: Koha::Patron->is_superlibrarian is not optimal
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26383 Bug ID: 26383 Summary: Koha::Patron->is_superlibrarian is not optimal Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: jonathan.druart@bugs.koha-community.org QA Contact: testopia@bugs.koha-community.org Depends on: 23634 It must not call $self->has_permission which does a DB request. C4::Context::IsSuperLibrarian is what must be done: $self->flags % 2 It could be good to replace the occurrence of IsSuperLibrarian at the same time, it's a bad pattern to have twice the same method to check for this superpower. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23634 [Bug 23634] Privilege escalation vulnerability for staff users with 'edit_borrowers' permission and 'OpacResetPassword' enabled -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26383 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26383 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |martin.renvoize@ptfs-europe |ity.org |.com Status|NEW |ASSIGNED -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26383 --- Comment #1 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Just had another look at this one. The problem is that we are calling it from modules, and have to Koha::Patrons->find($userenv->{number}) when, before, we had just to return $flags % 2 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26383 --- Comment #2 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 122341 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=122341&action=edit Bug 26383: WIP Replace C4::Context->IsSuperLibrarian with Koha::Patron->is_superlibrarian -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26383 --- Comment #3 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- I am not convinced here, we may want to replace it from .pl but keep C4::Context->IsSuperLibrarian for modules. Then we are not able to remove it and still use the code in two places. Any suggestions? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26383 --- Comment #4 from David Cook <dcook@prosentient.com.au> --- Comment on attachment 122341 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=122341 Bug 26383: WIP Replace C4::Context->IsSuperLibrarian with Koha::Patron->is_superlibrarian Review of attachment 122341: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=26383&attachment=122341) ----------------------------------------------------------------- ::: C4/Circulation.pm @@ +1073,5 @@
+ + my $userenv = C4::Context->userenv; + if ( C4::Context->preference("IndependentBranches") && $userenv ) { + my $logged_in_user = Koha::Patrons->find($userenv->{number}); + unless ( $logged_in_user->is_superlibrarian ) {
Technically, the session stores the flags value, so we could just do the calculation off the session data. The only downside of doing that is that we probably only set the flags session data at login time, so if someone's permissions change while they're logged in, it won't be detected until they've logged out and logged back in. But... it would mean fewer database calls. If we're doing a multi-checkout, the number of database calls would increase by that factor... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26383 --- Comment #5 from David Cook <dcook@prosentient.com.au> --- Comment on attachment 122341 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=122341 Bug 26383: WIP Replace C4::Context->IsSuperLibrarian with Koha::Patron->is_superlibrarian Review of attachment 122341: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=26383&attachment=122341) ----------------------------------------------------------------- ::: C4/Circulation.pm @@ +1073,5 @@
+ + my $userenv = C4::Context->userenv; + if ( C4::Context->preference("IndependentBranches") && $userenv ) { + my $logged_in_user = Koha::Patrons->find($userenv->{number}); + unless ( $logged_in_user->is_superlibrarian ) {
I suppose a more optimal design would be to pass the issuer to CanBookBeIssued via a parameter, but that would probably be too dramatic a refactor for us to do at this point... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26383 --- Comment #6 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #3)
I am not convinced here, we may want to replace it from .pl but keep C4::Context->IsSuperLibrarian for modules. Then we are not able to remove it and still use the code in two places.
Any suggestions?
Another option could be something like this: my $is_superlibrarian = Koha::Auth->is_superlibrarian({ borrowernumber => $borrowernumber, }); my $is_superlibrarian = Koha::Auth->is_superlibrarian({ patron => $patron, }); The only downside of that would be needing to add "use Koha::Auth", although I suppose we could add that into C4::Auth, which is called in the .pl scripts because they need the "get_template_and_user" function. Alternatively, you could write Koha::Patron->is_superlibrarian to be used as either a class method or an object method. You just check whether $self is a string or a reference to an object. If it's a class method (ie $self is a string), you could require a "patron" or "borrowernumber" parameter. That way you're keeping the code in one place but you're allowing for 2 different ways of calling it. There's some ideas :D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26383 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|martin.renvoize@ptfs-europe |koha-bugs@lists.koha-commun |.com |ity.org -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26383 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com --- Comment #7 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- I think the approach is not bad at all, but why not focus on having `get_template_and_user` return the `Koha::Patron` object in the first place? -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26383 --- Comment #8 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (In reply to David Cook from comment #4)
Comment on attachment 122341 [details] [review] Bug 26383: WIP Replace C4::Context->IsSuperLibrarian with Koha::Patron->is_superlibrarian
Review of attachment 122341 [details] [review]: -----------------------------------------------------------------
::: C4/Circulation.pm @@ +1073,5 @@
+ + my $userenv = C4::Context->userenv; + if ( C4::Context->preference("IndependentBranches") && $userenv ) { + my $logged_in_user = Koha::Patrons->find($userenv->{number}); + unless ( $logged_in_user->is_superlibrarian ) {
Technically, the session stores the flags value, so we could just do the calculation off the session data. The only downside of doing that is that we probably only set the flags session data at login time, so if someone's permissions change while they're logged in, it won't be detected until they've logged out and logged back in. But... it would mean fewer database calls.
Can we drop open sessions in the event of permission changes? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26383 --- Comment #9 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to Tomás Cohen Arazi (tcohen) from comment #7)
I think the approach is not bad at all, but why not focus on having `get_template_and_user` return the `Koha::Patron` object in the first place?
Too many changes? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org