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.
Created attachment 75542 [details] [review] Bug 20813: Update db schema
Created attachment 75543 [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
Created attachment 75544 [details] [review] Bug 20813: Remove deleted table
Created attachment 75545 [details] [review] Bug 20813 [Do Not Push]: Update Schema Files
Original pre-rebase code ( known to be working ): https://github.com/bywatersolutions/bywater-koha-devel/tree/user_permissions_revamp
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.
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.
(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.
Working on a rebase, small conflict caused by http://git.koha-community.org/gitweb/?p=koha.git;a=blobdiff;f=C4/Auth.pm;h=10929d887f2c6cf20f4ff2cced45ad577f29ebc8;hp=60c8c87911faeaf4459c5f1dc6154f726cbab710;hb=421746dd6337814f96adc6d79a773a2a23f50556;hpb=3d68ab447eda3eb5a25444b1ceaeea96b446c64b Will see how far I get.
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).
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).
:( But thx for the status update, Kyle. Maybe someone will be able to pick this up!
(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!
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
(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.
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.
(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
This bug is now even more relevant for security reasons
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).
Created attachment 124834 [details] [review] Bug 20813: Update db schema
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
Created attachment 124836 [details] [review] Bug 20813: Remove deleted table
(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!
These apply ok, but then I just get the web installer on my testing docker.
Created attachment 126280 [details] [review] Bug 20813: Remove flags references from Patron.pm, fix method has_permission
Created attachment 126281 [details] [review] Bug 20813: Update db schema
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
Created attachment 126283 [details] [review] Bug 20813: Remove deleted table
Created attachment 126284 [details] [review] Bug 20813: Update atomic update to new format
Created attachment 126285 [details] [review] Bug 20813: Remove flags references from Patron.pm, fix method has_permission
Created attachment 126287 [details] [review] Bug 20813: Update db schema
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
Created attachment 126289 [details] [review] Bug 20813: Remove deleted table
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
(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.
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.
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.
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.
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.
(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...
(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...
I like a lot of things from these patches, but I reckon they could probably be broken up into a number of smaller issues.
(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.
Want to work on this since it's fresh in the mind... but better work on OIDC instead...
(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 :)