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`.
Created attachment 86379 [details] [review] 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>
Created attachment 86380 [details] [review] Bug 22483: (follow-up) Improve POD as requested in bug 22031 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 86385 [details] [review] 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>
One step closer.. still another followup required to correct the logic for the /public endpoints where we are still inadvertently passing 'undef' to haspermission
Created attachment 86386 [details] [review] 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>
Created attachment 86388 [details] [review] Bug 22483: (follow-up) Fix wrong tests higlighted by bug Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86389 [details] [review] 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>
Created attachment 86390 [details] [review] 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>
Created attachment 86391 [details] [review] 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>
Created attachment 86392 [details] [review] 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>
Created attachment 86393 [details] [review] 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>
Created attachment 86394 [details] [review] 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>
Created attachment 86395 [details] [review] 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>
Patches pushed to master for 19.05 Thanks all!
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.
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.
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.
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.
(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.
Created attachment 86407 [details] [review] 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>
Created attachment 86408 [details] [review] 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>
Created attachment 86409 [details] [review] 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>
Created attachment 86410 [details] [review] 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>
Followup to enhancement not pushed to 18.11.x series