Description
Pedro Amorim
2023-02-15 10:17:06 UTC
Comment hidden (obsolete)
Currently, there is only one top-level permission for ERM on the system. There should be granular permissions, example: Allow a staff member to create agreements but not delete them. Created attachment 146698 [details] [review] Bug 32968: (WIP) Create granular permissions for ERM This is just a work in progress but open for discussion if anyone has any input, ideally would like to have a wildcard for erm subpermissions on x-koha-authorization paths but as it stands the way I found that works is listing all sub-permissions (idea is to give read access to agreements even if user only has 'manage_licenses' permission What are the real-life use cases we are trying to solve here? Maybe we are reproducing a wrong pattern with this approach (having all the different permissions) and we should prefer something else. We could have permissions that represent jobs in a library. erm-admin - allow read/write access to everything erm-show - only read erm-licensing - read/write to license and read on related resources (we will face the problem of the patron search permission) erm-agreement - read/write agreements, and read licenses erm-eholdings - read/write eholdings, and read agreements and a special one for ebsco? Does it answer the different needs? (In reply to Jonathan Druart from comment #3) > What are the real-life use cases we are trying to solve here? > > Maybe we are reproducing a wrong pattern with this approach (having all the > different permissions) and we should prefer something else. We could have > permissions that represent jobs in a library. I've long wanted to impliment a roles based access control system.. problem is I don't think a developer can define what permissions a role requires.. different libraries see different roles meaning different things. In my rebus:list days, we had very granular flags for each operation that could take place and then we had a role builder interface that allowed admins to create their roles and assign those granular permissions to them.. then end users were granted one or more roles on the system. Created attachment 147466 [details] [review] Bug 32968: (WIP) Create granular permissions for ERM POC for permissions implementation in agreements. Opinions welcome! Thanks! Yes, this looks good to me. One suggestion is to move the "OR" logic in the template Pass permission.agreement = permissions.admin || permissions.agreement ? 1 : 0 And simply test permission.agreement later you could also have + permissions.admin || + permissions.agreement || + permissions.show in show_agreement? Or... ... generate a hash like (pseudo-code): { agreement => { read => { permissions.admin || permissions.agreement || permissions.show } write => { permissions.admin || permissions.agreement } } } Then simply test for agreement.read and agreement.write? Created attachment 147510 [details] [review] Bug 32968: ERM granular permissions - Preparation Created attachment 147511 [details] [review] Bug 32968: ERM granular permissions - DB specific Created attachment 147513 [details] [review] Bug 32968: ERM granular permissions - API specs Created attachment 147514 [details] [review] Bug 32968: ERM granular permissions - Licenses Created attachment 147515 [details] [review] Bug 32968: ERM granular permissions - Agreements Created attachment 147516 [details] [review] Bug 32968: ERM granular permissions - EHoldings Packages Created attachment 147517 [details] [review] Bug 32968: ERM granular permissions - EHoldings Titles Created attachment 147518 [details] [review] Bug 32968: ERM granular permissions - Navigation guards Created attachment 147559 [details] [review] Bug 32968: ERM granular permissions - DB specific Preparation: * Apply patch and reset_all * Enable ERMModule sys pref * Create a new patron John (staff) with only catalogue, acquisitions and borrowers permissions (these last 2 are required for ERM, for now, see bug 32971) Test plan - show: * Add only the erm_show permission to John * Login as John * Verify that John has read-only access to all ERM, but cannot create edit or delete anything Test plan - admin: * Add only the erm_admin permission to John * Login as John * Verify that all ERM CRUD is available Test plan - licenses: * Add only the erm_licensing permission to John * Login as John * Verify that only licenses are available in the menu and that accessing directly to /cgi-bin/koha/erm/agreements will redirect to erm homepage Test plan - agreements: * Add only the erm_agreement permission to John * Login as John * Verify that side menu has all content available * Verify that John can create edit and delete agreements * Verify that John can only read licenses and accessing /cgi-bin/koha/erm/licenses/add will redirect to erm homepage * Verify that John can only read eHoldings and accessing /cgi-bin/koha/erm/eholdings/local/titles/add will redirect to erm homepage Test plan - eholdings: * Add only the erm_eholdings permission to John * Login as John * Verify that side menu has all content available except licenses * Verify that John can create edit and delete eholdings titles and packages * Verify that John can only read agreements and accessing /cgi-bin/koha/erm/agreements/add will redirect to erm homepage This could use some cypress tests to go along with, we would have to create a patron with the necessary permissions for testing beforeEach test, and delete said patron afterEach test. Discussing with Jonathan 2 options seem reasonable: 1) By POSTing and DELETEIng the patron - this would require the addition of a REST endpoint for permissions 2) Use cypress to click through the UI to create the patron, do the same thing to remove. This does not apply, please rebase. Created attachment 148797 [details] [review] Bug 32968: ERM granular permissions - Preparation Created attachment 148798 [details] [review] Bug 32968: ERM granular permissions - API specs Created attachment 148799 [details] [review] Bug 32968: ERM granular permissions - Licenses Created attachment 148800 [details] [review] Bug 32968: ERM granular permissions - Agreements Created attachment 148801 [details] [review] Bug 32968: ERM granular permissions - EHoldings Packages Created attachment 148802 [details] [review] Bug 32968: ERM granular permissions - EHoldings Titles Created attachment 148803 [details] [review] Bug 32968: ERM granular permissions - Navigation guards Created attachment 148804 [details] [review] Bug 32968: ERM granular permissions - DB specific Created attachment 148959 [details] [review] Bug 32968: ERM granular permissions - Preparation Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com> Created attachment 148960 [details] [review] Bug 32968: ERM granular permissions - API specs Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com> Created attachment 148961 [details] [review] Bug 32968: ERM granular permissions - Licenses Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com> Created attachment 148962 [details] [review] Bug 32968: ERM granular permissions - Agreements Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com> Created attachment 148963 [details] [review] Bug 32968: ERM granular permissions - EHoldings Packages Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com> Created attachment 148964 [details] [review] Bug 32968: ERM granular permissions - EHoldings Titles Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com> Created attachment 148965 [details] [review] Bug 32968: ERM granular permissions - Navigation guards Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com> Created attachment 148966 [details] [review] Bug 32968: ERM granular permissions - DB specific Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com> Hi Pedro, can you please clean these up? OK api/v1/swagger/paths/erm_agreements.yaml OK api/v1/swagger/paths/erm_documents.yaml OK api/v1/swagger/paths/erm_eholdings_packages.yaml OK api/v1/swagger/paths/erm_eholdings_packages_resources.yaml OK api/v1/swagger/paths/erm_eholdings_resources.yaml OK api/v1/swagger/paths/erm_eholdings_titles.yaml OK api/v1/swagger/paths/erm_eholdings_titles_resources.yaml OK api/v1/swagger/paths/erm_licenses.yaml OK api/v1/swagger/paths/erm_users.yaml OK erm/erm.pl FAIL installer/data/mysql/atomicupdate/bug_32968.pl FAIL file permissions File must have the exec flag OK koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc FAIL koha-tmpl/intranet-tmpl/prog/en/modules/erm/erm.tt FAIL filters missing_filter at line 49 ( write: [% CAN_user_erm_erm_licensing || CAN_user_erm_erm_admin ? 1 : 0 %],) missing_filter at line 50 ( read: [% CAN_user_erm_erm_licensing || CAN_user_erm_erm_admin || CAN_user_erm_erm_show || CAN_user_erm_erm_agreement ? 1 : 0 %],) missing_filter at line 53 ( write: [% CAN_user_erm_erm_agreement || CAN_user_erm_erm_admin ? 1 : 0 %],) missing_filter at line 54 ( read: [% CAN_user_erm_erm_agreement || CAN_user_erm_erm_admin || CAN_user_erm_erm_show || CAN_user_erm_erm_eholdings ? 1 : 0 %],) missing_filter at line 57 ( write: [% CAN_user_erm_erm_eholdings || CAN_user_erm_erm_admin ? 1 : 0 %],) missing_filter at line 58 ( read: [% CAN_user_erm_erm_eholdings || CAN_user_erm_erm_admin || CAN_user_erm_erm_show || CAN_user_erm_erm_agreement ? 1 : 0 %],) OK koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/AgreementsList.vue FAIL koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/AgreementsShow.vue FAIL Tidiness File is not tidy, please run `yarn run prettier --trailing-comma es5 --semi false --arrow-parens avoid --write /kohadevbox/koha/koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/AgreementsShow.vue` OK koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/AgreementsToolbar.vue OK koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/EHoldingsLocalPackagesList.vue FAIL koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/EHoldingsLocalPackagesShow.vue FAIL Tidiness File is not tidy, please run `yarn run prettier --trailing-comma es5 --semi false --arrow-parens avoid --write /kohadevbox/koha/koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/EHoldingsLocalPackagesShow.vue` OK koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/EHoldingsLocalPackagesToolbar.vue OK koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/EHoldingsLocalTitlesList.vue FAIL koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/EHoldingsLocalTitlesShow.vue FAIL Tidiness File is not tidy, please run `yarn run prettier --trailing-comma es5 --semi false --arrow-parens avoid --write /kohadevbox/koha/koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/EHoldingsLocalTitlesShow.vue` OK koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/EHoldingsLocalTitlesToolbar.vue OK koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/LicensesList.vue FAIL koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/LicensesShow.vue FAIL Tidiness File is not tidy, please run `yarn run prettier --trailing-comma es5 --semi false --arrow-parens avoid --write /kohadevbox/koha/koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/LicensesShow.vue` OK koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/LicensesToolbar.vue OK koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/Main.vue OK koha-tmpl/intranet-tmpl/prog/js/vue/routes/erm.js Created attachment 149423 [details] [review] Bug 32968: ERM granular permissions - Preparation Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com> Created attachment 149424 [details] [review] Bug 32968: ERM granular permissions - API specs Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com> Created attachment 149425 [details] [review] Bug 32968: ERM granular permissions - DB specific Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com> Created attachment 149426 [details] [review] Bug 32968: ERM granular permissions - Licenses Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com> Created attachment 149427 [details] [review] Bug 32968: ERM granular permissions - Agreements Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com> Created attachment 149428 [details] [review] Bug 32968: ERM granular permissions - EHoldings Packages Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com> Created attachment 149429 [details] [review] Bug 32968: ERM granular permissions - EHoldings Titles Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com> Created attachment 149430 [details] [review] Bug 32968: ERM granular permissions - Navigation guards Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com> Thank you Katrin. Rebased this as it no longer applied nicely. QA script seems happy now, please confirm. Test plan remains the same. 1. Should we restrict access to the whole ERM module is vendor_manage is not set? 2. I can list agreements even if I have only erm_eholdings. It's confusing as we don't have access to "Licenses". 3. EBSCO Add to/Remove from holdings buttons should be removed with only erm_show 4. Same for "Add new agreement" on EBSCO pkg show view 5. I don't get an error when I access /cgi-bin/koha/erm/licenses/edit/1 with only erm_show (same for agreement) Note that you should certain rebase on top of bug 33483, here is how I fixed the conflict: 130 actions: { 131 0: ["show"], 132 ...((permissions.agreement.write || this.embedded) && { 133 "-1": this.embedded 134 ? [ 135 { 136 select: { 137 text: this.$__("Select"), 138 icon: "fa fa-check", 139 }, 140 }, 141 ] 142 : ["edit", "delete"], 143 }), 144 }, (In reply to Jonathan Druart from comment #46) > 5. I don't get an error when I access /cgi-bin/koha/erm/licenses/edit/1 with > only erm_show (same for agreement) Not sure we should consider this blocker, does not seem trivial to fix. This wasn't applying. Rebased. It's even worse now. After bug 33408, the staff user also requires the permission parameters in order to be able to fetch the ERMModule syspref. Created attachment 150060 [details] [review] Bug 32968: ERM granular permissions - Preparation Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com> Created attachment 150061 [details] [review] Bug 32968: ERM granular permissions - API specs Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com> Created attachment 150062 [details] [review] Bug 32968: ERM granular permissions - DB specific Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com> Created attachment 150063 [details] [review] Bug 32968: ERM granular permissions - Licenses Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com> Created attachment 150064 [details] [review] Bug 32968: ERM granular permissions - Agreements Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com> Created attachment 150065 [details] [review] Bug 32968: ERM granular permissions - EHoldings Packages Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com> Created attachment 150066 [details] [review] Bug 32968: ERM granular permissions - EHoldings Titles Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com> Created attachment 150067 [details] [review] Bug 32968: ERM granular permissions - Navigation guards Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com> (In reply to Jonathan Druart from comment #46) > 1. Should we restrict access to the whole ERM module is vendor_manage is not > set? I don't know :) Also, see comment #49 > 2. I can list agreements even if I have only erm_eholdings. It's confusing > as we don't have access to "Licenses". It's working as intented. I followed your suggestion on comment #3, to me it makes sense because eHoldings may be related to agreements, but not licenses. Either way, I'm open to suggestions about this, should eHoldings write permission give read-only permission to both agreements+licenses? > 3. EBSCO Add to/Remove from holdings buttons should be removed with only > erm_show > 4. Same for "Add new agreement" on EBSCO pkg show view Yes, thanks! I missed EBSCO permissions in these patches. Will provide patches for this when I find the time. > 5. I don't get an error when I access /cgi-bin/koha/erm/licenses/edit/1 with > only erm_show (same for agreement) You don't get an error, you are redirected to /cgi-bin/koha/erm/erm.pl. If this is what you are experiencing, do you think it would be preferable to show an error page instead? (I don't think this is important, this is just a permission double-check in case the user attempts to access content that he doesn't have permission to, directly through the URL). Error or not, user doesn't have access to the content he's not permitted to. (In reply to Pedro Amorim from comment #49) > This wasn't applying. Rebased. It's even worse now. > After bug 33408, the staff user also requires the permission parameters in > order to be able to fetch the ERMModule syspref. Yes, we noticed that with Tomas last week. We need to fix that before anything else. (In reply to Jonathan Druart from comment #59) > (In reply to Pedro Amorim from comment #49) > > This wasn't applying. Rebased. It's even worse now. > > After bug 33408, the staff user also requires the permission parameters in > > order to be able to fetch the ERMModule syspref. > > Yes, we noticed that with Tomas last week. We need to fix that before > anything else. Reported on bug 33606. |