[Bug 22483] New: haspermissions used to support passing 'undef' for $flagsrequired
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 Bug ID: 22483 Summary: haspermissions used to support passing 'undef' for $flagsrequired Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: martin.renvoize@ptfs-europe.com QA Contact: testopia@bugs.koha-community.org Target Milestone: --- Bug 22031 inadvertently prevents passing 'undef' (or not passing $flagsrequired at all) to mean "pass if I have ANY permission on anything". This functionality was in use in two places. /svc/members/search and /Koha/REST/V1/Auth.pm. We do support passing `*` to mean the same thing (and this can also be used at arbitrary depth in the structure to mean `any subpermission`. -- 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=22483 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |major -- 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=22483 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |22031 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22031 [Bug 22031] C4::Auth->haspermission should allow checking for more than one subpermission -- 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=22483 --- Comment #1 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 86379 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86379&action=edit Bug 22483: Explicitly ban 'undef' as a valid $flagsrequired Before bug 22031 the haspermission subroutine signature allowed for passing 'undef' to mean 'any permission' in $flagsrequired. This feels like a mistake and was only in practical use in two places in the codebase. This patch explicitly forbids this practice (`*` may be used to the same result and is more explicit in it's nature) and replaces the two instances of it's use. Test Plan 1. Before this patch, the API tests are all failing with authentication errors 2. After this patch the API tests should now all pass. 3. t/db_dependent/Auth/haspermission.t should continue to pass (with one addition subtest added herin) 3. /svc/members/search is not unit tested. Please check that patron searching still yields results in the UI after this patch. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- 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=22483 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |martin.renvoize@ptfs-europe |ity.org |.com -- 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=22483 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org, | |nick@bywatersolutions.com, | |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=22484 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 --- Comment #2 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 86380 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86380&action=edit Bug 22483: (follow-up) Improve POD as requested in bug 22031 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 --- Comment #3 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 86385 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86385&action=edit Bug 22483: (QA follow-up) Corrections to logic in check_cookie_auth check_cookie_auth needs to allow for cases where we wish to check for ANY permission and cases where we wish to skip the permissions check entirely and just authenticate the session. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |ASSIGNED --- Comment #4 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- One step closer.. still another followup required to correct the logic for the /public endpoints where we are still inadvertently passing 'undef' to haspermission -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 --- Comment #5 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 86386 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86386&action=edit Bug 22483: (QA follow-up) Tweaking call to haspermission This patch tweaks the logic to check the permissions is defined before passing to haspermission. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 --- Comment #6 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 86388 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86388&action=edit Bug 22483: (follow-up) Fix wrong tests higlighted by bug Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 --- Comment #7 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 86389 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86389&action=edit Bug 22483: Restore undef behaviour Turns out that we rely heavily on the side effect that passing undef to haspermission would always return true no matter what permissions or lack of permissions you had. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #86379|0 |1 is obsolete| | Attachment #86380|0 |1 is obsolete| | Attachment #86385|0 |1 is obsolete| | Attachment #86386|0 |1 is obsolete| | Attachment #86388|0 |1 is obsolete| | Attachment #86389|0 |1 is obsolete| | --- Comment #8 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 86390 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86390&action=edit Bug 22483: Explicitly ban 'undef' as a valid $flagsrequired Before bug 22031 the haspermission subroutine signature allowed for passing 'undef' to mean 'any permission' in $flagsrequired. This feels like a mistake and was only in practical use in two places in the codebase. This patch explicitly forbids this practice (`*` may be used to the same result and is more explicit in it's nature) and replaces the two instances of it's use. Test Plan 1. Before this patch, the API tests are all failing with authentication errors 2. After this patch the API tests should now all pass. 3. t/db_dependent/Auth/haspermission.t should continue to pass (with one addition subtest added herin) 3. /svc/members/search is not unit tested. Please check that patron searching still yields results in the UI after this patch. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 --- Comment #9 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 86391 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86391&action=edit Bug 22483: (follow-up) Improve POD as requested in bug 22031 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 --- Comment #10 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 86392 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86392&action=edit Bug 22483: (QA follow-up) Corrections to logic in check_cookie_auth check_cookie_auth needs to allow for cases where we wish to check for ANY permission and cases where we wish to skip the permissions check entirely and just authenticate the session. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 --- Comment #11 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 86393 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86393&action=edit Bug 22483: (QA follow-up) Tweaking call to haspermission This patch tweaks the logic to check the permissions is defined before passing to haspermission. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 --- Comment #12 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 86394 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86394&action=edit Bug 22483: (follow-up) Fix wrong tests higlighted by bug Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 --- Comment #13 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 86395 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86395&action=edit Bug 22483: Restore undef behaviour Turns out that we rely heavily on the side effect that passing undef to haspermission would always return true no matter what permissions or lack of permissions you had. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #86390|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #86391|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #86392|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #86393|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #86394|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #86395|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #14 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Patches pushed to master for 19.05 Thanks all! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |tomascohen@gmail.com |y.org | Target Milestone|--- |19.05 --- Comment #15 from Tomás Cohen Arazi <tomascohen@gmail.com> --- I've pushed this follow-up bug that restores the previous behavior for haspermission regarding undef. It highlighted "interesting" bugs and flawed design in our mixed up authentication-authorization methods. Specially how the rest of the code adapts to those side effects. We will certainly file new bugs for things that where raised. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |additional_work_needed --- Comment #16 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Login at the OPAC is impossible: Can't use string ("1") as a HASH ref while "strict refs" in use at /home/vagrant/kohaclone/C4/Auth.pm line 202. at /home/vagrant/kohaclone/C4/Auth.pm line 192 Caught by t/db_dependent/selenium/authentication.t, always good to run those tests to make sure there are no obvious regressions. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 Pierre-Marc Thibault <pierre-marc.thibault@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |pierre-marc.thibault@inlibr | |o.com --- Comment #17 from Pierre-Marc Thibault <pierre-marc.thibault@inlibro.com> --- I found a bug related to this bugzilla. When one tries to create a course reserve, a software error occurs with the following message : Can't use string ("1") as a HASH ref while "strict refs" in use at /C4/Auth.pm line 302. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 --- Comment #18 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 2087 return 1 unless defined($flagsrequired); # This is horrifying but restores behaviour prior to bug 22031 Are you sure about that? I do not see any situations where is returned 1 before 22031. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 Pierre-Marc Thibault <pierre-marc.thibault@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC|pierre-marc.thibault@inlibr | |o.com | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 --- Comment #19 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Martin Renvoize from comment #0)
This functionality was in use in two places. /svc/members/search and /Koha/REST/V1/Auth.pm.
About /svc/members/search: at first glance I think we should rewrite this code to adapt it, not the other way around. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 --- Comment #20 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 86407 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86407&action=edit Bug 22483: Restore exact behaviour of undef Passing undef (or nothing) as $flagsrequired to haspermission simply returned the return from fetchrow prior to this patch. Restoring that behaviour. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 --- Comment #21 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 86408 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86408&action=edit Bug 22483: Restore svc/members/search This script used to pass 'undef' to haspermission, this patch restores that behaviour. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #86407|0 |1 is obsolete| | --- Comment #22 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 86409 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86409&action=edit Bug 22483: Restore exact behaviour of undef Passing undef (or nothing) as $flagsrequired to haspermission simply returned the return from fetchrow prior to this patch. Restoring that behaviour. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #86408|0 |1 is obsolete| | --- Comment #23 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 86410 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86410&action=edit Bug 22483: Restore svc/members/search This script used to pass 'undef' to haspermission, this patch restores that behaviour. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|haspermissions used to |haspermissions previously |support passing 'undef' for |supported passing 'undef' |$flagsrequired |for $flagsrequired -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 Bug 22483 depends on bug 22031, which changed state. Bug 22031 Summary: C4::Auth->haspermission should allow checking for more than one subpermission https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22031 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|Pushed to Master |RESOLVED --- Comment #24 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Followup to enhancement not pushed to 18.11.x series -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |22529 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22529 [Bug 22529] /svc/members/search relies on quirks of haspermission -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=22529 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22483 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|additional_work_needed | -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org