Bug 14540

Summary: Move member-flags.pl to PermissionsManager to better manage permissions for testing.
Product: Koha Reporter: Olli-Antti Kivilahti <olli-antti.kivilahti>
Component: AuthenticationAssignee: Olli-Antti Kivilahti <olli-antti.kivilahti>
Status: BLOCKED --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: abl, ablewicz, benjamin.rokseth, dcook, dpavlin, jonathan.druart
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13906
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20813
Change sponsored?: --- Patch complexity: Large patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 14495, 14536, 14539, 13995    
Bug Blocks: 7174    
Attachments: Bug 14540 - Move member-flags.pl to PermissionsManager to better manage permissions for testing.
Bug 14540 - Move member-flags.pl to PermissionsManager to better manage permissions for testing.
Bug 14540 - Move member-flags.pl to PermissionsManager to better manage permissions for testing.
A sketch of the permissions improvements.
Bug 14540 - Move member-flags.pl to PermissionsManager to better manage permissions for testing.
Bug 14540 - Move member-flags.pl to PermissionsManager to better manage permissions for testing.
Bug 14540 - Move member-flags.pl to PermissionsManager to better manage permissions for testing.
Bug 14540 - Move member-flags.pl to PermissionsManager to better manage permissions for testing.
Bug 14540 - Move member-flags.pl to PermissionsManager to better manage permissions for testing.
Bug 14540 - Move member-flags.pl to PermissionsManager to better manage permissions for testing.
Bug 14540 - Move member-flags.pl to PermissionsManager to better manage permissions for testing.
Bug 14540 - Move member-flags.pl to PermissionsManager to better manage permissions for testing.
Bug 14540 - Move member-flags.pl to PermissionsManager to better manage permissions for testing.
Bug 14540 - Move member-flags.pl to PermissionsManager to better manage permissions for testing.
Bug 14540 - PermissionManager - Followup fix sample_patrons.sql
Bug 14540 - Move member-flags.pl to PermissionsManager to better manage permissions for testing.
Bug 14540 - Followup regression test for bad login
Bug 14540 - Followup regression test for bad login
Bug 14540 - PermissionManager - Hotfix check_api_auth, remove flags.
Bug 14540 - Move member-flags.pl to PermissionsManager. Recover from changed userid mid-session.
Bug 14540 - Move member-flags.pl to PermissionsManager to better manage permissions for testing.
Bug 14540 - Move member-flags.pl to PermissionsManager to better manage permissions for testing.
Bug 14540 - Move member-flags.pl to PermissionsManager to better manage permissions for testing.

Description Olli-Antti Kivilahti 2015-07-16 14:16:08 UTC
Currently there is no nice way to grant/revoke permissions for Borrowers in Koha.
This is a big problem when doing integration testing and when working with permissions using the future REST API.

Currently we have one web-script, member-flags.pl, which deals with all permission-related things. Basically it always DELETEs all your permissions and then adds them again with some modifications. There is no internal API for creating permissions and this makes automated testing significantly harder.
To create permissions for a test borrower, we must decrypt the arcane way of creating koha.borrowers.flags and add user_permissions with brittle SQL-queries.
This makes for very messy test scripts, and quite frankly this is hardly ever done in Koha.
Also the koha.borrowers.flags-column is pretty horrible. It is 19-bit binary array where each bit tells of the borrower has all permissions for the respective permission module. This is very difficult to calculate and the mechanism is hard to understand. This feature is also unnecessary, since we are always interested in having the specific permissions. We don't need to know if one Borrower has all the permissions for one module. Also what happens if more subpermissions are added to the module? Existing all-permission-holders lose  their all-permissions-status?

Instead we could have a nice internal API to work with permissions.

My patch introduces a new database structure for permissions (not necessarily better), Koha::Object implementation of the DB structure,
and most importantly, a Koha::Auth::PermissionManager which is a gateway to all permissions-related actions.

This feature is well tested with unit tests and PageObject integration tests.
See the tests for usage examples.
Especially the t/db_dependent/Members/member-flags.t
I hope you like it :)

VOCABULARY:

Module (permission_modules)
    => a module which consists of specific Permissions.
       Typically covers one Module in Koha.
Permission (permission)
    => a specific Permission of a Module. Granting a
       Borrower the permission to do something.
Borrower permission (borrower_permissions)
    => A specific Permission a specific Borrower has, eg. the permission to
       edit bibliographic records.

TEST PLAN:

-2. Note the permissions a bunch of borrowers has.
-1. run updatedatabase.pl
0. Observe that those borrowers still have the same permissions. For modules which didn't have any subpermissions, a generic subpermission is added and the borrowers should have it if they had the existing module permission.

Basic login:

1. Go to Koha staff client
2. Make a password login.
3. Browse to any other staff client pages to confirm that the session is active and login persists on other pages.

Modify permissions:

1. Go to member-flags.pl (Home › Patrons › Set permissions)
2. Change your permissions. Save.
3. Go back to member-flags.pl
4. Observe that you see the same permissions you just set.

Modify superlibrarian permission:

1. Go to member-flags.pl (Home › Patrons › Set permissions)
2. Give the superlibrarian-permission. Observe all other permissions vanishing.
   Save.
3. Go back to member-flags.pl
4. Observe that you see only the superlibrarian-permission active.
5. Observe that the superlibrarian permission is the only top-level permission
   which doesn't have any subpermissions.
Comment 1 Olli-Antti Kivilahti 2015-07-16 14:19:17 UTC Comment hidden (obsolete)
Comment 2 Olli-Antti Kivilahti 2015-07-17 08:31:14 UTC Comment hidden (obsolete)
Comment 3 Olli-Antti Kivilahti 2015-07-17 09:19:41 UTC Comment hidden (obsolete)
Comment 4 Olli-Antti Kivilahti 2015-07-17 09:47:31 UTC
Created attachment 41056 [details]
A sketch of the permissions improvements.
Comment 5 Olli-Antti Kivilahti 2015-07-17 11:19:21 UTC Comment hidden (obsolete)
Comment 6 Olli-Antti Kivilahti 2015-07-17 12:02:37 UTC Comment hidden (obsolete)
Comment 7 Olli-Antti Kivilahti 2015-07-17 13:23:18 UTC Comment hidden (obsolete)
Comment 8 Benjamin Rokseth 2015-07-18 09:20:11 UTC
Great work, Olli! And cheers to this approach to a very annoying lack!  Will test as soon as possible.

One suggestion, though:
patching updatedatabase.pl is no longer neccesary:
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13068

Documented on wiki:
http://wiki.koha-community.org/wiki/Database_updates#New_workflow_since_bug_13068_has_been_pushed

We're already using it, as we need multiple patches with database updates, and it greatly simplifies testing.
Comment 9 Olli-Antti Kivilahti 2015-07-20 08:12:25 UTC
(In reply to Benjamin Rokseth from comment #8)
> Great work, Olli! And cheers to this approach to a very annoying lack!  Will
> test as soon as possible.
Thank you for the kind words Benjamin!
Sometimes it is very hard to guess how the community will react to things :)

I am also rewriting the authentication system. Or more like deprecating most of it and creating a new more modular system. There is already a lot of work in it and it works well. Integrating it to the legacy systems is really difficult, but you can see the code in Bug 7174 - Authentication rewriting.
That published code is not git bz apply:able yet, but I am working on it to support the most used legacy authentication mechanisms and work really well with the new REST API :)

I'll take a look at the atomicupdates!
Comment 10 Olli-Antti Kivilahti 2015-07-22 11:25:10 UTC Comment hidden (obsolete)
Comment 11 Olli-Antti Kivilahti 2015-07-24 14:03:48 UTC Comment hidden (obsolete)
Comment 12 Olli-Antti Kivilahti 2015-07-31 10:34:27 UTC Comment hidden (obsolete)
Comment 13 Olli-Antti Kivilahti 2015-07-31 10:36:59 UTC Comment hidden (obsolete)
Comment 14 Olli-Antti Kivilahti 2015-08-03 10:45:16 UTC Comment hidden (obsolete)
Comment 15 Olli-Antti Kivilahti 2015-08-06 10:25:54 UTC Comment hidden (obsolete)
Comment 16 Olli-Antti Kivilahti 2015-08-17 12:01:06 UTC Comment hidden (obsolete)
Comment 17 Olli-Antti Kivilahti 2015-09-04 11:52:39 UTC Comment hidden (obsolete)
Comment 18 Olli-Antti Kivilahti 2015-09-04 15:23:03 UTC
Created attachment 42410 [details] [review]
Bug 14540 - Move member-flags.pl to PermissionsManager to better manage permissions for testing.
Comment 19 Olli-Antti Kivilahti 2015-09-07 15:15:50 UTC Comment hidden (obsolete)
Comment 20 Olli-Antti Kivilahti 2015-09-07 15:28:18 UTC
Created attachment 42434 [details] [review]
Bug 14540 - Followup regression test for bad login
Comment 21 Olli-Antti Kivilahti 2015-09-15 09:14:47 UTC
Created attachment 42550 [details] [review]
Bug 14540 - PermissionManager - Hotfix check_api_auth, remove flags.
Comment 22 Olli-Antti Kivilahti 2015-11-05 12:06:03 UTC
Created attachment 44490 [details] [review]
Bug 14540 - Move member-flags.pl to PermissionsManager. Recover from changed userid mid-session.
Comment 23 Lari Taskula 2017-05-08 16:58:56 UTC
Created attachment 63252 [details] [review]
Bug 14540 - Move member-flags.pl to PermissionsManager to better manage permissions for testing.

Currently there is no nice way to grant/revoke permissions for Patrons in Koha.
This is a big problem when doing integration testing and when working with
permissions using the future REST API.

Currently we have one web-script, member-flags.pl, which deals with all
permission-related things. Basically it always DELETEs all your permissions and
then adds them again with some modifications. There is no internal API for
creating permissions and this makes automated testing significantly harder.
To create permissions for a test patron, we must decrypt the arcane way of
creating koha.borrowers.flags and add user_permissions with brittle SQL-queries.
This makes for very messy test scripts, and quite frankly this is hardly ever done
in Koha.
Also the koha.borrowers.flags-column is pretty horrible. It is 19-bit binary
array where each bit tells of the patron has all permissions for the respective
permission module. This is very difficult to calculate and the mechanism is hard
to understand. This feature is also unnecessary, since we are always interested in
having the specific permissions. We don't need to know if one Patron has all the
permissions for one module. Also what happens if more subpermissions are added to
the module? Existing all-permission-holders lose their all-permissions-status?

Instead we could have a nice internal API to work with permissions.

My patch introduces a new database structure for permissions
(not necessarily better), Koha::Object implementation of the DB structure,
and most importantly, a Koha::Auth::PermissionManager which is a gateway to all
permissions-related actions.

This feature is well tested with unit tests and PageObject integration tests.
See the tests for usage examples.
Especially the t/db_dependent/Members/member-flags.t
I hope you like it :)

SELENIUM TESTS:
Now that the new permission manager enables setting permissions easily in test
environments. This patch introduces more complex PageObject regressions tests.
-Logged in branch regression test for Intra.
-OPAC password login/logout
-OPAC anonymous search history preservation.

VOCABULARY:

Module (permission_modules)
    => a module which consists of specific Permissions.
       Typically covers one Module in Koha.
Permission (permission)
    => a specific Permission of a Module. Granting a
       Patron the permission to do something.
Patron permission (borrower_permissions)
    => A specific Permission a specific Patron has, eg. the permission to
       edit bibliographic records.

TEST PLAN:

-2. Note the permissions a bunch of patrons has.
-1. run updatedatabase.pl
0. Observe that those patrons still have the same permissions. For modules
   which didn't have any subpermissions, a generic subpermission is added and
   the patrons should have it if they had the existing module permission.

Basic login:

1. Go to Koha staff client
2. Make a password login.
3. Browse to any other staff client pages to confirm that the session is active
   and login persists on other pages.

Modify permissions:

1. Go to member-flags.pl (Home › Patrons › Set permissions)
2. Change your permissions. Save.
3. Go back to member-flags.pl
4. Observe that you see the same permissions you just set.

Modify superlibrarian permission:

1. Go to member-flags.pl (Home › Patrons › Set permissions)
2. Give the superlibrarian-permission. Observe all other permissions vanishing.
   Save.
3. Go back to member-flags.pl
4. Observe that you see only the superlibrarian-permission active.
5. Observe that the superlibrarian permission is the only top-level permission
   which doesn't have any subpermissions.
Comment 24 Lari Taskula 2017-05-08 17:01:12 UTC
Created attachment 63253 [details] [review]
Bug 14540 - Move member-flags.pl to PermissionsManager to better manage permissions for testing.

Currently there is no nice way to grant/revoke permissions for Patrons in Koha.
This is a big problem when doing integration testing and when working with
permissions using the future REST API.

Currently we have one web-script, member-flags.pl, which deals with all
permission-related things. Basically it always DELETEs all your permissions and
then adds them again with some modifications. There is no internal API for
creating permissions and this makes automated testing significantly harder.
To create permissions for a test patron, we must decrypt the arcane way of
creating koha.borrowers.flags and add user_permissions with brittle SQL-queries.
This makes for very messy test scripts, and quite frankly this is hardly ever done
in Koha.
Also the koha.borrowers.flags-column is pretty horrible. It is 19-bit binary
array where each bit tells of the patron has all permissions for the respective
permission module. This is very difficult to calculate and the mechanism is hard
to understand. This feature is also unnecessary, since we are always interested in
having the specific permissions. We don't need to know if one Patron has all the
permissions for one module. Also what happens if more subpermissions are added to
the module? Existing all-permission-holders lose their all-permissions-status?

Instead we could have a nice internal API to work with permissions.

My patch introduces a new database structure for permissions
(not necessarily better), Koha::Object implementation of the DB structure,
and most importantly, a Koha::Auth::PermissionManager which is a gateway to all
permissions-related actions.

This feature is well tested with unit tests and PageObject integration tests.
See the tests for usage examples.
Especially the t/db_dependent/Members/member-flags.t
I hope you like it :)

SELENIUM TESTS:
Now that the new permission manager enables setting permissions easily in test
environments. This patch introduces more complex PageObject regressions tests.
-Logged in branch regression test for Intra.
-OPAC password login/logout
-OPAC anonymous search history preservation.

VOCABULARY:

Module (permission_modules)
    => a module which consists of specific Permissions.
       Typically covers one Module in Koha.
Permission (permission)
    => a specific Permission of a Module. Granting a
       Patron the permission to do something.
Patron permission (borrower_permissions)
    => A specific Permission a specific Patron has, eg. the permission to
       edit bibliographic records.

TEST PLAN:

-2. Note the permissions a bunch of patrons has.
-1. run updatedatabase.pl
0. Observe that those patrons still have the same permissions. For modules
   which didn't have any subpermissions, a generic subpermission is added and
   the patrons should have it if they had the existing module permission.

Basic login:

1. Go to Koha staff client
2. Make a password login.
3. Browse to any other staff client pages to confirm that the session is active
   and login persists on other pages.

Modify permissions:

1. Go to member-flags.pl (Home › Patrons › Set permissions)
2. Change your permissions. Save.
3. Go back to member-flags.pl
4. Observe that you see the same permissions you just set.

Modify superlibrarian permission:

1. Go to member-flags.pl (Home › Patrons › Set permissions)
2. Give the superlibrarian-permission. Observe all other permissions vanishing.
   Save.
3. Go back to member-flags.pl
4. Observe that you see only the superlibrarian-permission active.
5. Observe that the superlibrarian permission is the only top-level permission
   which doesn't have any subpermissions.
Comment 25 Lari Taskula 2017-05-08 17:07:10 UTC
Created attachment 63254 [details] [review]
Bug 14540 - Move member-flags.pl to PermissionsManager to better manage permissions for testing.

Currently there is no nice way to grant/revoke permissions for Patrons in Koha.
This is a big problem when doing integration testing and when working with
permissions using the future REST API.

Currently we have one web-script, member-flags.pl, which deals with all
permission-related things. Basically it always DELETEs all your permissions and
then adds them again with some modifications. There is no internal API for
creating permissions and this makes automated testing significantly harder.
To create permissions for a test patron, we must decrypt the arcane way of
creating koha.borrowers.flags and add user_permissions with brittle SQL-queries.
This makes for very messy test scripts, and quite frankly this is hardly ever done
in Koha.
Also the koha.borrowers.flags-column is pretty horrible. It is 19-bit binary
array where each bit tells of the patron has all permissions for the respective
permission module. This is very difficult to calculate and the mechanism is hard
to understand. This feature is also unnecessary, since we are always interested in
having the specific permissions. We don't need to know if one Patron has all the
permissions for one module. Also what happens if more subpermissions are added to
the module? Existing all-permission-holders lose their all-permissions-status?

Instead we could have a nice internal API to work with permissions.

My patch introduces a new database structure for permissions
(not necessarily better), Koha::Object implementation of the DB structure,
and most importantly, a Koha::Auth::PermissionManager which is a gateway to all
permissions-related actions.

This feature is well tested with unit tests and PageObject integration tests.
See the tests for usage examples.
Especially the t/db_dependent/Members/member-flags.t
I hope you like it :)

SELENIUM TESTS:
Now that the new permission manager enables setting permissions easily in test
environments. This patch introduces more complex PageObject regressions tests.
-Logged in branch regression test for Intra.
-OPAC password login/logout
-OPAC anonymous search history preservation.

VOCABULARY:

Module (permission_modules)
    => a module which consists of specific Permissions.
       Typically covers one Module in Koha.
Permission (permission)
    => a specific Permission of a Module. Granting a
       Patron the permission to do something.
Patron permission (borrower_permissions)
    => A specific Permission a specific Patron has, eg. the permission to
       edit bibliographic records.

TEST PLAN:

-2. Note the permissions a bunch of patrons has.
-1. run updatedatabase.pl
0. Observe that those patrons still have the same permissions. For modules
   which didn't have any subpermissions, a generic subpermission is added and
   the patrons should have it if they had the existing module permission.

Basic login:

1. Go to Koha staff client
2. Make a password login.
3. Browse to any other staff client pages to confirm that the session is active
   and login persists on other pages.

Modify permissions:

1. Go to member-flags.pl (Home › Patrons › Set permissions)
2. Change your permissions. Save.
3. Go back to member-flags.pl
4. Observe that you see the same permissions you just set.

Modify superlibrarian permission:

1. Go to member-flags.pl (Home › Patrons › Set permissions)
2. Give the superlibrarian-permission. Observe all other permissions vanishing.
   Save.
3. Go back to member-flags.pl
4. Observe that you see only the superlibrarian-permission active.
5. Observe that the superlibrarian permission is the only top-level permission
   which doesn't have any subpermissions.