Bug 31389 - Calculate user permissions in separate function
Summary: Calculate user permissions in separate function
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Cook
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 31380 32064
  Show dependency treegraph
 
Reported: 2022-08-18 00:32 UTC by David Cook
Modified: 2023-06-08 22:26 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00


Attachments
Bug 31389: Calculate user permissions in separate function (17.61 KB, patch)
2022-08-18 03:31 UTC, David Cook
Details | Diff | Splinter Review
Bug 31389: Calculate user permissions in separate function (18.45 KB, patch)
2022-08-18 06:22 UTC, David Cook
Details | Diff | Splinter Review
Bug 31389: Calculate user permissions in separate function (18.50 KB, patch)
2022-08-18 15:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 31389: Calculate user permissions in separate function (18.65 KB, patch)
2022-09-04 12:24 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 31389: (QA follow-up) Correct test message (1.08 KB, patch)
2022-09-04 12:24 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 31389: Calculate user permissions in separate function (18.71 KB, patch)
2022-09-13 10:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31389: (QA follow-up) Correct test message (1.14 KB, patch)
2022-09-13 10:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31389: (QA follow-up) Missing permissions in tests (23.48 KB, patch)
2022-09-22 11:57 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 David Cook 2022-08-18 00:32:43 UTC
C4::Auth::get_template_and_user() is an extremely long function. By decomposing it into functions, it becomes easier to test and re-use those functions elsewhere. 

The current calculation of "CAN_user_*" is also inefficient and contains some unused code, so this would help to clean it up.
Comment 1 David Cook 2022-08-18 03:31:56 UTC Comment hidden (obsolete)
Comment 2 David Cook 2022-08-18 06:22:08 UTC
Created attachment 139326 [details] [review]
Bug 31389: Calculate user permissions in separate function

This patch refactors the setting of user permissions for templates into
a new function, which can be easily unit tested and reduces the amount
of code in C4::Auth::get_template_and_user(). It also aids in the
re-usability of permission checking code.

Test plan:
0) Apply patch and koha-plack --restart kohadev
1) prove t/Koha/Auth/Permissions.t
2) As koha superlibrarian, go to
http://localhost:8081/cgi-bin/koha/tools/tools-home.pl
3) Go to http://localhost:8081/cgi-bin/koha/members/members-home.pl
4) Create new test user with "Staff access..." and "Remaining circulation permissions"
5) Logout of koha superlibrarian
6) Login as test user
7) Note you can only see a limited view of the staff interface
(i.e. no administration, no tools, no reports, etc.)
Comment 3 Kyle M Hall 2022-08-18 15:13:41 UTC
Created attachment 139398 [details] [review]
Bug 31389: Calculate user permissions in separate function

This patch refactors the setting of user permissions for templates into
a new function, which can be easily unit tested and reduces the amount
of code in C4::Auth::get_template_and_user(). It also aids in the
re-usability of permission checking code.

Test plan:
0) Apply patch and koha-plack --restart kohadev
1) prove t/Koha/Auth/Permissions.t
2) As koha superlibrarian, go to
http://localhost:8081/cgi-bin/koha/tools/tools-home.pl
3) Go to http://localhost:8081/cgi-bin/koha/members/members-home.pl
4) Create new test user with "Staff access..." and "Remaining circulation permissions"
5) Logout of koha superlibrarian
6) Login as test user
7) Note you can only see a limited view of the staff interface
(i.e. no administration, no tools, no reports, etc.)

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 4 Joonas Kylmälä 2022-09-04 12:24:04 UTC
Created attachment 140202 [details] [review]
Bug 31389: Calculate user permissions in separate function

This patch refactors the setting of user permissions for templates into
a new function, which can be easily unit tested and reduces the amount
of code in C4::Auth::get_template_and_user(). It also aids in the
re-usability of permission checking code.

Test plan:
0) Apply patch and koha-plack --restart kohadev
1) prove t/Koha/Auth/Permissions.t
2) As koha superlibrarian, go to
http://localhost:8081/cgi-bin/koha/tools/tools-home.pl
3) Go to http://localhost:8081/cgi-bin/koha/members/members-home.pl
4) Create new test user with "Staff access..." and "Remaining circulation permissions"
5) Logout of koha superlibrarian
6) Login as test user
7) Note you can only see a limited view of the staff interface
(i.e. no administration, no tools, no reports, etc.)

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 5 Joonas Kylmälä 2022-09-04 12:24:07 UTC
Created attachment 140203 [details] [review]
Bug 31389: (QA follow-up) Correct test message

The test in question concerns non-superlibrarian staff users.

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 6 Joonas Kylmälä 2022-09-04 12:25:08 UTC
Looks good to me but I want one more sign-off from the QA team since this change concerns authorization.
Comment 7 Martin Renvoize 2022-09-13 10:39:53 UTC
Created attachment 140563 [details] [review]
Bug 31389: Calculate user permissions in separate function

This patch refactors the setting of user permissions for templates into
a new function, which can be easily unit tested and reduces the amount
of code in C4::Auth::get_template_and_user(). It also aids in the
re-usability of permission checking code.

Test plan:
0) Apply patch and koha-plack --restart kohadev
1) prove t/Koha/Auth/Permissions.t
2) As koha superlibrarian, go to
http://localhost:8081/cgi-bin/koha/tools/tools-home.pl
3) Go to http://localhost:8081/cgi-bin/koha/members/members-home.pl
4) Create new test user with "Staff access..." and "Remaining circulation permissions"
5) Logout of koha superlibrarian
6) Login as test user
7) Note you can only see a limited view of the staff interface
(i.e. no administration, no tools, no reports, etc.)

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2022-09-13 10:40:02 UTC
Created attachment 140564 [details] [review]
Bug 31389: (QA follow-up) Correct test message

The test in question concerns non-superlibrarian staff users.

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2022-09-13 10:40:41 UTC
Second QA run done.. this is a nice little cleanup.

Passed.
Comment 10 Tomás Cohen Arazi 2022-09-22 11:57:28 UTC
Created attachment 140844 [details] [review]
Bug 31389: (QA follow-up) Missing permissions in tests

The following bugs refined permissions a bit, and the change was missing
on the tests:

* 23681
* 30335

Bonus: sorted permissions alphabetically for readability.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Tomás Cohen Arazi 2022-09-22 12:43:21 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 12 Lucas Gass 2022-10-31 22:57:35 UTC
Enhancement will not be backported to 22.05.x series
Comment 13 David Cook 2022-11-02 00:43:44 UTC
In hindsight, there is 1 test missing.

For "normal staff user test", I should've included "'reports' => 1" and then checked that it all the sub-permissions authorizations. 

I've opened bug 32064 to include that missing test.