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: Bugs List
QA Contact: Testopia
URL: https://github.com/bywatersolutions/b...
Whiteboard:
Keywords:
Depends on: 20226
Blocks: 20956
  Show dependency treegraph
 
Reported: 2018-05-24 13:38 UTC by Kyle M Hall
Modified: 2018-12-12 15:25 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


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

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
Created attachment 75542 [details] [review]
Bug 20813: Update db schema
Comment 2 Kyle M Hall 2018-05-24 13:45:31 UTC
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
Comment 3 Kyle M Hall 2018-05-24 13:45:33 UTC
Created attachment 75544 [details] [review]
Bug 20813: Remove deleted table
Comment 4 Kyle M Hall 2018-05-24 13:45:36 UTC
Created attachment 75545 [details] [review]
Bug 20813 [Do Not Push]: Update Schema Files
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!