Bug 22483 - haspermissions previously supported passing 'undef' for $flagsrequired
Summary: haspermissions previously supported passing 'undef' for $flagsrequired
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Martin Renvoize
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 22031
Blocks: 22529
  Show dependency treegraph
 
Reported: 2019-03-08 12:23 UTC by Martin Renvoize
Modified: 2019-06-27 09:24 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 22483: Explicitly ban 'undef' as a valid $flagsrequired (3.87 KB, patch)
2019-03-08 12:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22483: (follow-up) Improve POD as requested in bug 22031 (1.64 KB, patch)
2019-03-08 12:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22483: (QA follow-up) Corrections to logic in check_cookie_auth (2.46 KB, patch)
2019-03-08 14:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22483: (QA follow-up) Tweaking call to haspermission (1.01 KB, patch)
2019-03-08 15:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22483: (follow-up) Fix wrong tests higlighted by bug (1.36 KB, patch)
2019-03-08 16:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22483: Restore undef behaviour (2.36 KB, patch)
2019-03-08 16:54 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22483: Explicitly ban 'undef' as a valid $flagsrequired (3.94 KB, patch)
2019-03-08 18:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22483: (follow-up) Improve POD as requested in bug 22031 (1.71 KB, patch)
2019-03-08 18:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22483: (QA follow-up) Corrections to logic in check_cookie_auth (2.53 KB, patch)
2019-03-08 18:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22483: (QA follow-up) Tweaking call to haspermission (1.08 KB, patch)
2019-03-08 18:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22483: (follow-up) Fix wrong tests higlighted by bug (1.43 KB, patch)
2019-03-08 18:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22483: Restore undef behaviour (2.43 KB, patch)
2019-03-08 18:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22483: Restore exact behaviour of undef (1.21 KB, patch)
2019-03-09 08:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22483: Restore svc/members/search (1.00 KB, patch)
2019-03-09 08:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22483: Restore exact behaviour of undef (1.27 KB, patch)
2019-03-09 08:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22483: Restore svc/members/search (1.06 KB, patch)
2019-03-09 08:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2019-03-08 12:23:40 UTC
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`.
Comment 1 Martin Renvoize 2019-03-08 12:31:24 UTC
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>
Comment 2 Martin Renvoize 2019-03-08 12:51:49 UTC
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>
Comment 3 Martin Renvoize 2019-03-08 14:22:27 UTC
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>
Comment 4 Martin Renvoize 2019-03-08 14:23:46 UTC
One step closer.. still another followup required to correct the logic for the /public endpoints where we are still inadvertently passing 'undef' to haspermission
Comment 5 Martin Renvoize 2019-03-08 15:11:51 UTC
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>
Comment 6 Tomás Cohen Arazi 2019-03-08 16:46:32 UTC
Created attachment 86388 [details] [review]
Bug 22483: (follow-up) Fix wrong tests higlighted by bug

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Martin Renvoize 2019-03-08 16:54:14 UTC
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>
Comment 8 Kyle M Hall 2019-03-08 18:15:04 UTC
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>
Comment 9 Kyle M Hall 2019-03-08 18:15:14 UTC
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>
Comment 10 Kyle M Hall 2019-03-08 18:15:17 UTC
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>
Comment 11 Kyle M Hall 2019-03-08 18:15:19 UTC
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>
Comment 12 Kyle M Hall 2019-03-08 18:15:22 UTC
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>
Comment 13 Kyle M Hall 2019-03-08 18:15:25 UTC
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>
Comment 14 Tomás Cohen Arazi 2019-03-08 18:55:22 UTC
Patches pushed to master for 19.05

Thanks all!
Comment 15 Tomás Cohen Arazi 2019-03-08 18:57:43 UTC
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.
Comment 16 Jonathan Druart 2019-03-08 21:38:33 UTC
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.
Comment 17 Pierre-Marc Thibault 2019-03-08 21:44:47 UTC
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.
Comment 18 Jonathan Druart 2019-03-08 21:49:40 UTC
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.
Comment 19 Jonathan Druart 2019-03-08 21:53:07 UTC
(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.
Comment 20 Martin Renvoize 2019-03-09 08:14:37 UTC
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>
Comment 21 Martin Renvoize 2019-03-09 08:16:13 UTC
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>
Comment 22 Tomás Cohen Arazi 2019-03-09 08:30:08 UTC
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>
Comment 23 Tomás Cohen Arazi 2019-03-09 08:30:14 UTC
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>
Comment 24 Martin Renvoize 2019-03-12 08:48:40 UTC
Followup to enhancement not pushed to 18.11.x series