Bug 20813 - Revamp user permissions system
Summary: Revamp user permissions system
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL: https://github.com/bywatersolutions/b...
Keywords:
Depends on: 20226
Blocks: 20956
  Show dependency treegraph
 
Reported: 2018-05-24 13:38 UTC by Kyle M Hall
Modified: 2022-11-02 14:28 UTC (History)
9 users (show)

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


Attachments
Bug 20813: Update db schema (9.18 KB, patch)
2018-05-24 13:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20813: Revamp user permissions system (44.10 KB, patch)
2018-05-24 13:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20813: Remove deleted table (2.18 KB, patch)
2018-05-24 13:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20813 [Do Not Push]: Update Schema Files (7.40 KB, patch)
2018-05-24 13:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20813: Update db schema (8.75 KB, patch)
2021-09-13 12:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20813: Revamp user permissions system (45.77 KB, patch)
2021-09-13 12:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20813: Remove deleted table (2.17 KB, patch)
2021-09-13 12:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20813: Remove flags references from Patron.pm, fix method has_permission (2.38 KB, patch)
2021-10-14 13:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20813: Update db schema (11.10 KB, patch)
2021-10-14 14:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20813: Revamp user permissions system (45.78 KB, patch)
2021-10-14 14:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20813: Remove deleted table (2.16 KB, patch)
2021-10-14 14:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20813: Update atomic update to new format (6.98 KB, patch)
2021-10-14 14:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20813: Remove flags references from Patron.pm, fix method has_permission (2.38 KB, patch)
2021-10-14 14:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20813: Update db schema (12.30 KB, patch)
2021-10-14 14:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20813: Revamp user permissions system (47.19 KB, patch)
2021-10-14 14:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20813: Remove deleted table (2.16 KB, patch)
2021-10-14 14:09 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2018-05-24 13:38:04 UTC
Koha's current user permissions system is a bit convoluted and limited due to it's legacy where the permissions system was a simple set of flags. A second layer of sub-permissions were later added complication and obfuscation to the system.

We should revamp the permissions system to be a more modern system and open the path to having unlimited nesting of sub-permissions instead of shoe-horning sub-permissions into the limited system we have.
Comment 1 Kyle M Hall 2018-05-24 13:45:24 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2018-05-24 13:45:31 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2018-05-24 13:45:33 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2018-05-24 13:45:36 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2018-05-24 13:46:17 UTC
Original pre-rebase code ( known to be working ): https://github.com/bywatersolutions/bywater-koha-devel/tree/user_permissions_revamp
Comment 6 Owen Leonard 2018-06-19 12:36:04 UTC
I'm changing this to "patch doesn't apply" because the attached patch didn't work correctly and Kyle said it was probably a rebase issue. I'm not comfortable signing off based on the original code because of how behind master it is.
Comment 7 Katrin Fischer 2018-06-19 19:34:37 UTC
Kyle, would you be ok to make this dependent on some patches in PQA when rebasing?

bug 11911 - new permission for managing suggestions
bug 7651 - new permission for managing currencies

Both are adding new permissions. I am keen on resolving as many permission bugs as possible this release and would be happy to help with this one to have a better foundation.
Comment 8 Kyle M Hall 2018-07-11 16:10:39 UTC
(In reply to Katrin Fischer from comment #7)
> Kyle, would you be ok to make this dependent on some patches in PQA when
> rebasing?
> 
> bug 11911 - new permission for managing suggestions
> bug 7651 - new permission for managing currencies
> 
> Both are adding new permissions. I am keen on resolving as many permission
> bugs as possible this release and would be happy to help with this one to
> have a better foundation.

I'm a bit late replying, but yes.
Comment 10 Katrin Fischer 2018-07-31 20:06:09 UTC
Hi Kyle,

I think with Koha's code having moved since this was originally written, this needs more of a rebase than I can handle :(

Do you think you will have time to work on this? I'd really like to help move this along and will make time for it.

Some things I noticed so far:

- CATCODE_MULTI will be removed by bug 20226
- We have the patron object available in the templates now, so won't need the template variables (bug 18789)
- We already got a has_permission in Patron.pm, so this gets doubled up by the patch
- There is also a is_child that could simplify some of the code
- Do we still need the description column in the permission table? We could think about removing it as the descriptions are in the templates.
- Database update appears to be missing
- Why the change to member in members-toolbar.inc?
- Got confused by having both Koha::Patron::Permission(s) and Koha::Permission(s)
- Owen is trying to get rid of the tree view on bug 11375 - maybe something to talk about before moving forward here or there.
- I feel like we should keep part of the copyright on top of member-flags.pl

I am thinking about moving on with my patch for making granular administration permissions that I had started as it might not be too hard to adapt this patch set (bug 14391).
Comment 11 Kyle M Hall 2018-12-10 12:21:03 UTC
I do not plan on continuing to develop this code. If anyone wished to pick it up and work on it, or use it as inspiration for a re-write, they are most welcome to it!

I will remove myself as the assignee.

(In reply to Katrin Fischer from comment #10)
> Hi Kyle,
> 
> I think with Koha's code having moved since this was originally written,
> this needs more of a rebase than I can handle :(
> 
> Do you think you will have time to work on this? I'd really like to help
> move this along and will make time for it.
> 
> Some things I noticed so far:
> 
> - CATCODE_MULTI will be removed by bug 20226
> - We have the patron object available in the templates now, so won't need
> the template variables (bug 18789)
> - We already got a has_permission in Patron.pm, so this gets doubled up by
> the patch
> - There is also a is_child that could simplify some of the code
> - Do we still need the description column in the permission table? We could
> think about removing it as the descriptions are in the templates.
> - Database update appears to be missing
> - Why the change to member in members-toolbar.inc?
> - Got confused by having both Koha::Patron::Permission(s) and
> Koha::Permission(s)
> - Owen is trying to get rid of the tree view on bug 11375 - maybe something
> to talk about before moving forward here or there.
> - I feel like we should keep part of the copyright on top of member-flags.pl
> 
> I am thinking about moving on with my patch for making granular
> administration permissions that I had started as it might not be too hard to
> adapt this patch set (bug 14391).
Comment 12 Katrin Fischer 2018-12-11 16:23:04 UTC
:(

But thx for the status update, Kyle. Maybe someone will be able to pick this up!
Comment 13 Kyle M Hall 2018-12-12 15:25:43 UTC
(In reply to Katrin Fischer from comment #12)
> :(
> 
> But thx for the status update, Kyle. Maybe someone will be able to pick this
> up!

Maybe post it to the developers list? Perhaps another Koha developer would be interested in picking it up!
Comment 14 Lari Taskula 2020-07-29 18:15:11 UTC
This would be really really really useful. The permission bits are one big headache because features introducing new ones will conflict with one another.

And what about Koha plugins? Can they currently even add custom permission modules without eventually conflicting the bit value?

+1 to this Bug
Comment 15 Kyle M Hall 2020-07-30 13:49:23 UTC
(In reply to Lari Taskula from comment #14)
> This would be really really really useful. The permission bits are one big
> headache because features introducing new ones will conflict with one
> another.
> 
> And what about Koha plugins? Can they currently even add custom permission
> modules without eventually conflicting the bit value?
> 
> +1 to this Bug

I ran out of time to work on this, so if you want to pick it up please do! I wouldn't worry about plugins. I'm not aware of any plugins creating top level permissions.
Comment 16 Jonathan Druart 2020-07-30 14:08:52 UTC
I've tried to rebase the remote branch this morning but the original commits embed too many unrelated changes, which makes the work non trivial.
Comment 17 Lari Taskula 2020-07-30 14:13:47 UTC
(In reply to Kyle M Hall from comment #15)
> I wouldn't worry about plugins. I'm not aware of any plugins creating top
> level permissions.

I see it likely that someone at some point will need it.

Also, as discussed on #koha IRC [1], we noticed we will soon hit the top limit of borrowers.flags, making this Bug relevant and something that we should definitely look into.

Hopefully we can find funding to continue this Bug.

[1] http://irc.koha-community.org/koha/2020-07-29#i_2272965
Comment 18 Fridolin Somers 2021-09-10 20:29:14 UTC
This bug is now even more relevant for security reasons
Comment 19 David Cook 2021-09-13 04:56:42 UTC
For a while, I've been fantasizing about an RBAC or ABAC permission system, but I think modernizing Koha's AuthZ might be an insurmountable task. 

I like the idea of groups too. Administrators, Cataloguers, and Circulation as three out-of-the-box groups with default permissions set. Rather than setting permissions for individual users, the average library might just assign users to groups and manage permissions at the group level. 

I think someone needs to do something (although it's not going to be me).
Comment 20 Kyle M Hall 2021-09-13 12:59:34 UTC
Created attachment 124834 [details] [review]
Bug 20813: Update db schema
Comment 21 Kyle M Hall 2021-09-13 12:59:47 UTC
Created attachment 124835 [details] [review]
Bug 20813: Revamp user permissions system

Koha's current user permissions system is a bit convoluted and limited
due to it's legacy where the permissions system was a simple set of
flags. A second layer of sub-permissions were later added complication
and obfuscation to the system.

We should revamp the permissions system to be a more modern system and
open the path to having unlimited nesting of sub-permissions instead of
shoe-horning sub-permissions into the limited system we have.

Test Plan:
1) Apply this patch set
2) Run updatedatabase.pl
3) Restart all the things!
4) Koha should still enforce user permissions as before
Comment 22 Kyle M Hall 2021-09-13 12:59:51 UTC
Created attachment 124836 [details] [review]
Bug 20813: Remove deleted table
Comment 23 Kyle M Hall 2021-09-13 13:00:56 UTC
(In reply to Fridolin Somers from comment #18)
> This bug is now even more relevant for security reasons

Given that, I've rebased this patch set. I haven't tested it at all yet. Please let me know if it any issue! Given the age of the patches, I think a fresh sign-off is in order!
Comment 24 Andrew Fuerste-Henry 2021-10-13 21:41:13 UTC
These apply ok, but then I just get the web installer on my testing docker.
Comment 25 Kyle M Hall 2021-10-14 13:57:28 UTC
Created attachment 126280 [details] [review]
Bug 20813: Remove flags references from Patron.pm, fix method has_permission
Comment 26 Kyle M Hall 2021-10-14 14:05:03 UTC
Created attachment 126281 [details] [review]
Bug 20813: Update db schema
Comment 27 Kyle M Hall 2021-10-14 14:05:15 UTC
Created attachment 126282 [details] [review]
Bug 20813: Revamp user permissions system

Koha's current user permissions system is a bit convoluted and limited
due to it's legacy where the permissions system was a simple set of
flags. A second layer of sub-permissions were later added complication
and obfuscation to the system.

We should revamp the permissions system to be a more modern system and
open the path to having unlimited nesting of sub-permissions instead of
shoe-horning sub-permissions into the limited system we have.

Test Plan:
1) Apply this patch set
2) Run updatedatabase.pl
3) Restart all the things!
4) Koha should still enforce user permissions as before
Comment 28 Kyle M Hall 2021-10-14 14:05:19 UTC
Created attachment 126283 [details] [review]
Bug 20813: Remove deleted table
Comment 29 Kyle M Hall 2021-10-14 14:05:22 UTC
Created attachment 126284 [details] [review]
Bug 20813: Update atomic update to new format
Comment 30 Kyle M Hall 2021-10-14 14:05:26 UTC
Created attachment 126285 [details] [review]
Bug 20813: Remove flags references from Patron.pm, fix method has_permission
Comment 31 Kyle M Hall 2021-10-14 14:09:12 UTC
Created attachment 126287 [details] [review]
Bug 20813: Update db schema
Comment 32 Kyle M Hall 2021-10-14 14:09:22 UTC
Created attachment 126288 [details] [review]
Bug 20813: Revamp user permissions system

Koha's current user permissions system is a bit convoluted and limited
due to it's legacy where the permissions system was a simple set of
flags. A second layer of sub-permissions were later added complication
and obfuscation to the system.

We should revamp the permissions system to be a more modern system and
open the path to having unlimited nesting of sub-permissions instead of
shoe-horning sub-permissions into the limited system we have.

Test Plan:
1) Apply this patch set
2) Run updatedatabase.pl
3) Restart all the things!
4) Koha should still enforce user permissions as before
Comment 33 Kyle M Hall 2021-10-14 14:09:26 UTC
Created attachment 126289 [details] [review]
Bug 20813: Remove deleted table
Comment 34 Marcel de Rooy 2021-10-27 06:39:29 UTC
ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Cannot add or update a child row: a foreign key constraint fails (`koha_myclone`.`user_permissions`, CONSTRAINT `user_permissions_ibfk_2` FOREIGN KEY (`code`) REFERENCES `permissions` (`code`)) at /usr/share/koha/C4/Installer.pm line 736
Comment 35 Marcel de Rooy 2021-10-27 06:50:11 UTC
(In reply to Marcel de Rooy from comment #34)
> ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Cannot add or
> update a child row: a foreign key constraint fails
> (`koha_myclone`.`user_permissions`, CONSTRAINT `user_permissions_ibfk_2`
> FOREIGN KEY (`code`) REFERENCES `permissions` (`code`)) at
> /usr/share/koha/C4/Installer.pm line 736

Repeated the install on a fresh system without this warning. Benefit of the doubt.
Comment 36 Marcel de Rooy 2021-10-27 06:51:23 UTC
But couldnt get thru onboarding with your patches. Borrowers from the optional data were not inserted too. There must be references to flags still somewhere.
Comment 37 Marcel de Rooy 2021-10-27 06:55:28 UTC
C4/Acquisition.pm:        my $sth = $dbh->prepare("SELECT flags FROM borrowers WHERE borrowernumber = ?");

C4/Auth.pm:"select borrowernumber, firstname, surname, flags, borrowers.branchcode, branches.branchname as branchname, email from borrowers left join branches
 on borrowers.branchcode=branches.branchcode where userid=?"

C4/Members.pm:        AND ( borrowers.flags IS NULL OR borrowers.flags = 0 )

t/db_dependent/Serials_2.t:            UPDATE borrowers SET flags=? WHERE borrowernumber=?
t/db_dependent/api/v1/holds.t:            flags        => 80, #borrowers and reserveforothers flags
t/db_dependent/api/v1/patrons_extended_attributes.t:            value => { flags => 2**4 }    # 'borrowers' flag == 4
t/db_dependent/api/v1/patrons_holds.t:        value => { flags => 2 ** 4 } # 'borrowers' flag == 4

This is just top of the iceberg. Looks like this development is far from ready.
Comment 38 David Cook 2021-11-18 01:25:54 UTC
This is just an aside but "unlimited nesting of sub-permissions" sounds like it would be difficult to maintain and process, but maybe I'm misunderstanding the proposal.
Comment 39 Martin Renvoize 2021-12-06 11:34:02 UTC
I've discussed this a little offline with a few people but never found a moment to put it down as a comment here.

I think this bug starts going in the right direction.. but..

I don't think we should continue down the route of 'nesting' permissions in a hierarchy.

I'd rather see 'a bag of permissions' and then ways to arbitrarily group them.. be that for 'roles' or 'permissions groups' or whatever.
Comment 40 David Cook 2022-11-02 01:05:33 UTC
(In reply to Martin Renvoize from comment #39)
> I've discussed this a little offline with a few people but never found a
> moment to put it down as a comment here.
> 
> I think this bug starts going in the right direction.. but..
> 
> I don't think we should continue down the route of 'nesting' permissions in
> a hierarchy.
> 
> I'd rather see 'a bag of permissions' and then ways to arbitrarily group
> them.. be that for 'roles' or 'permissions groups' or whatever.

Agreed.

I think a good first step would be to have a single coherent way of fetching and checking permissions (ie authorizations) across Koha, and then we can incrementally improve the creation and organisation of permissions.

Right now, we use "C4::Auth::haspermission($userid,$flagsrequired)" in order to get our permission data structure, when really we should be doing something like "Koha::Auth->is_authorized({ flags => $flags, flagsrequired => $flagsrequired })"

Bug 31389 takes the $flags data structure and translates it into something that can be used for the template authorizations. 

Koha::Auth->is_authorized could actually leverage Koha::Auth::Permissions->get_authz_from_flags(). 

Or something new could be made that fits the different scenarios.

--

Overall, we're not doing anything too complicated with permissions/authorizations.

Maybe I could have another look at this sometime...
Comment 41 David Cook 2022-11-02 01:30:41 UTC
(In reply to Marcel de Rooy from comment #37)
> This is just top of the iceberg. Looks like this development is far from
> ready.

Yikes... yeah maybe it'll be a while before we get rid of flags...
Comment 42 David Cook 2022-11-02 01:32:05 UTC
I like a lot of things from these patches, but I reckon they could probably be broken up into a number of smaller issues.
Comment 43 David Cook 2022-11-02 02:30:01 UTC
(In reply to David Cook from comment #42)
> I like a lot of things from these patches, but I reckon they could probably
> be broken up into a number of smaller issues.

- Refactor C4::Auth::getuserflags() and move to Koha::Auth::Permissions->get_flags() with unit tests
- Refactor Koha::Auth::Permissions->get_authz_from_flags to build a fully populated authz hash, and then create Koha::Auth::Permissions->get_template_authz_from_authz() 
- Refactor C4::Auth::haspermission() (this is actually used more extensively through Koha than I originally thought...)
- Update Acquisition.pm, Budgets.pm, about.pl, and other users of authorization data checks outside of the initial AuthZ check on page load...

The goal of the above is to centralize the business logic of permission/authorization checks in Koha, so we're not repeating complex code structures throughout the app.

- Finally, move "flags" column into "user_permissions" table (that said, this could have unexpected consequences for borrower_modifications and deletedborrowers...)(this also will have consequences with manual SQL and tests)

The goal of the above is to free us from the bit-packed "flags" integer and separate sub-permission table, so that it's easier to create and organise flags/permissions.
Comment 44 David Cook 2022-11-02 02:34:08 UTC
Want to work on this since it's fresh in the mind... but better work on OIDC instead...
Comment 45 Kyle M Hall 2022-11-02 14:28:02 UTC
(In reply to David Cook from comment #44)
> Want to work on this since it's fresh in the mind... but better work on OIDC
> instead...

If you do decide to, I'll be happy to sign/qa anything you do :)