Bug 31389

Summary: Calculate user permissions in separate function
Product: Koha Reporter: David Cook <dcook>
Component: Architecture, internals, and plumbingAssignee: David Cook <dcook>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: joonas.kylmala, lucas, martin.renvoize
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27342
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00
Bug Depends on:    
Bug Blocks: 31380, 32064    
Attachments: Bug 31389: Calculate user permissions in separate function
Bug 31389: Calculate user permissions in separate function
Bug 31389: Calculate user permissions in separate function
Bug 31389: Calculate user permissions in separate function
Bug 31389: (QA follow-up) Correct test message
Bug 31389: Calculate user permissions in separate function
Bug 31389: (QA follow-up) Correct test message
Bug 31389: (QA follow-up) Missing permissions in tests

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.