Bug 36056 - Clarify subpermissions check behavior in C4::Auth
Summary: Clarify subpermissions check behavior in C4::Auth
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-02-09 02:13 UTC by Tomás Cohen Arazi
Modified: 2024-07-25 11:11 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.04,23.05.10


Attachments
Bug 36056: Clarify subpermissions AND behavior (3.37 KB, patch)
2024-02-09 02:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36056: Clarify subpermissions AND behavior (3.43 KB, patch)
2024-02-09 07:30 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2024-02-09 02:13:13 UTC
Looking at C4::Auth::haspermission, it looks like it allows checking:

{ perm => { subperm_a => 1, subperm_b => 1 } }

treating it like subperm_a AND subperm_b.

But this use case is not documented. We should add tests and add it to the POD.
Comment 1 Tomás Cohen Arazi 2024-02-09 02:25:22 UTC
Created attachment 161957 [details] [review]
Bug 36056: Clarify subpermissions AND behavior

Working on bug 31791, I found myself wondering if our current recursive
code in C4::Auth::haspermission() would allow checking AND on
subpermissions.

As it is not documented in the POD or tested, I decided to write some
unit tests for it.

It turned out it was well supported, so I decided to submit the tests,
and a small tweak in the POD to reflect that.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Auth/haspermission.t
=> SUCCESS: Tests pass! The code supports AND on subpermissions!
3. Sign off :-D
Comment 2 Martin Renvoize 2024-02-09 07:30:19 UTC
Created attachment 161958 [details] [review]
Bug 36056: Clarify subpermissions AND behavior

Working on bug 31791, I found myself wondering if our current recursive
code in C4::Auth::haspermission() would allow checking AND on
subpermissions.

As it is not documented in the POD or tested, I decided to write some
unit tests for it.

It turned out it was well supported, so I decided to submit the tests,
and a small tweak in the POD to reflect that.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Auth/haspermission.t
=> SUCCESS: Tests pass! The code supports AND on subpermissions!
3. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 3 Martin Renvoize 2024-02-09 07:31:08 UTC
Straight QA here, simple improvements to clarify and enhance testing of existing functionality.

Thanks Tomas
Comment 4 Katrin Fischer 2024-03-07 16:37:25 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 5 Fridolin Somers 2024-03-11 10:25:01 UTC
Pushed to 23.11.x for 23.11.04
Comment 6 Lucas Gass 2024-03-19 20:42:01 UTC
Backported to 23.05.x for upcoming 23.05.10.