Bug 34631

Summary: Independentbranches - Non-superuser should not be able to write Additional contents for other branches
Product: Koha Reporter: Hammat wele <hammat.wele>
Component: ToolsAssignee: Hammat wele <hammat.wele>
Status: Signed Off --- QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: caroline.cyr-la-rose, lucas, martin.renvoize, matthias.le-gac, mkstephens, philippe.blouin
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches
Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches
Bug 34631: Independentbranches - rebase to master
Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches
Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches
Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches
Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches
Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches

Description Hammat wele 2023-08-28 17:10:56 UTC
When syspref Independentbranches is enabled Non-superuser should not be able to write Additional contents for other branches

To reproduce:
1- Connect to staff interface as superlibrarian
2- Enable IndependentBranches preference
3- Go to Tools -> HTML customizations
4- Crete an additional content for all libraries
5- Create or edit a borrower
    5-1- Set permission for the borrower 
        - catalogue
        - edit_additional_contents
4- Log in to the staff interface with the non-superlibrarian staff user
5- Go to Tools -> HTML customizations
    --> you can delete or edit the additional contents created by superlibrarian for All libraries 
6- Click on New entry 
    --> you can add additional content for other branches
Comment 1 Hammat wele 2023-08-28 17:17:21 UTC Comment hidden (obsolete)
Comment 2 Hammat wele 2023-08-28 17:23:00 UTC
Created attachment 154890 [details] [review]
Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches

When syspref Independentbranches is enabled Non-superuser should not be able to write Additional contents for other branches

To reproduce:
1- Connect to staff interface as superlibrarian
2- Enable IndependentBranches preference
3- Go to Tools -> HTML customizations
4- Crete an additional content for all libraries
5- Create or edit a borrower
    5-1- Set permission for the borrower
        - catalogue
        - edit_additional_contents
4- Log in to the staff interface with the non-superlibrarian staff user
5- Go to Tools -> HTML customizations
    --> you can delete or edit the additional contents created by superlibrarian for All libraries
6- Click on New entry
    --> you can add additional content for other branches
7- Apply the patch
8- Repeat step 5
    --> you cannot delete or edit the additional contents create by superlibrarian for All libraries
9- Repeat step 6
    --> you cannot add additional content for other branches
10- submit the form
    --> the branch of the created additional content is the current user branch
Comment 3 Myka Kennedy Stephens 2024-03-05 01:44:23 UTC
Attempted to test using git bz with Koha Testing Docker. Received following error:

Applying: Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches
Using index info to reconstruct a base tree...
M       koha-tmpl/intranet-tmpl/prog/en/modules/tools/additional-contents.tt
M       tools/additional-contents.pl
Falling back to patching base and 3-way merge...
Auto-merging tools/additional-contents.pl
CONFLICT (content): Merge conflict in tools/additional-contents.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/tools/additional-contents.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/tools/additional-contents.tt
error: Failed to merge in the changes.
Patch failed at 0001 Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches
Comment 4 Matthias Le Gac 2024-03-05 17:20:41 UTC
Created attachment 162787 [details] [review]
Bug 34631: Independentbranches - rebase to master

When syspref Independentbranches is enabled Non-superuser should not be able to write Additional contents for other branches

To reproduce:
1- Connect to staff interface as superlibrarian
2- Enable IndependentBranches preference
3- Go to Tools -> HTML customizations
4- Crete an additional content for all libraries
5- Create or edit a borrower
    5-1- Set permission for the borrower
        - catalogue
        - edit_additional_contents
4- Log in to the staff interface with the non-superlibrarian staff user
5- Go to Tools -> HTML customizations
    --> you can delete or edit the additional contents created by superlibrarian for All libraries
6- Click on New entry
    --> you can add additional content for other branches
7- Apply the patch
8- Repeat step 5
    --> you cannot delete or edit the additional contents create by superlibrarian for All libraries
9- Repeat step 6
    --> you cannot add additional content for other branches
10- submit the form
    --> the branch of the created additional content is the current user branch
Comment 5 Matthias Le Gac 2024-03-05 17:27:30 UTC
rebase to master
Comment 6 Myka Kennedy Stephens 2024-03-05 18:20:29 UTC
Applied patch and received an error when returning to the HTML Customizations tool in the non-superlibrarian login (additional-contents.pl) A PDF of the full error trace is available here: https://drive.google.com/file/d/1GNaNWN-sMIKQcYvdns5i5406qZ2iGw23/view?usp=sharing

The same error persists across News and Pages for both a superlibrarian login and a non-superlibrarian login.
Comment 7 Matthias Le Gac 2024-03-05 19:01:17 UTC
Created attachment 162788 [details] [review]
Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches

When syspref Independentbranches is enabled Non-superuser should not be able to write Additional contents for other branches

To reproduce:
1- Connect to staff interface as superlibrarian
2- Enable IndependentBranches preference
3- Go to Tools -> HTML customizations
4- Crete an additional content for all libraries
5- Create or edit a borrower
    5-1- Set permission for the borrower
        - catalogue
        - edit_additional_contents
4- Log in to the staff interface with the non-superlibrarian staff user
5- Go to Tools -> HTML customizations
    --> you can delete or edit the additional contents created by superlibrarian for All libraries
6- Click on New entry
    --> you can add additional content for other branches
7- Apply the patch
8- Repeat step 5
    --> you cannot delete or edit the additional contents create by superlibrarian for All libraries
9- Repeat step 6
    --> you cannot add additional content for other branches
10- submit the form
    --> the branch of the created additional content is the current user branch
Comment 8 Myka Kennedy Stephens 2024-03-05 22:00:30 UTC
@Matthias, I'm still getting the same error when applying. Git bz applied both patches that are attached here. Either one of them is still producing the error or I'm doing something wrong with Koha Testing Docker. Please advise.
Comment 9 Lucas Gass 2024-03-05 22:07:49 UTC
Matthias, your patch sets now include merge markers. Please use the QA tool!
Comment 10 Matthias Le Gac 2024-03-06 20:56:04 UTC
Created attachment 162856 [details] [review]
Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches

When syspref Independentbranches is enabled Non-superuser should not be able to write Additional contents for other branches

To reproduce:
1- Connect to staff interface as superlibrarian
2- Enable IndependentBranches preference
3- Go to Tools -> HTML customizations
4- Crete an additional content for all libraries
5- Create or edit a borrower
    5-1- Set permission for the borrower
        - catalogue
        - edit_additional_contents
4- Log in to the staff interface with the non-superlibrarian staff user
5- Go to Tools -> HTML customizations
    --> you can delete or edit the additional contents created by superlibrarian for All libraries
6- Click on New entry
    --> you can add additional content for other branches
7- Apply the patch
8- Repeat step 5
    --> you cannot delete or edit the additional contents create by superlibrarian for All libraries
9- Repeat step 6
    --> you cannot add additional content for other branches
10- submit the form
    --> the branch of the created additional content is the current user branch
Comment 11 Matthias Le Gac 2024-03-06 20:58:17 UTC
I think it's good now to test

sorry for the trouble I've caused by trying to rebase
Comment 12 Myka Kennedy Stephens 2024-03-06 23:57:15 UTC
Different error, this time when attempting to apply the patch using git bz:

Applying: Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches
error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/tools/additional-contents.tt).
error: could not build fake ancestor
Patch failed at 0001 Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches
Comment 13 Matthias Le Gac 2024-03-07 18:35:57 UTC
Bug 34631 - Independentbranches - Non-superuser should not be able to write Additional contents for other branches

162856 - Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches

I have no problem to apply the patch, I don't understand your problem. Your master is up to date ?
Comment 14 Myka Kennedy Stephens 2024-03-07 18:49:15 UTC
Matthias: I don't understand it either. I tried again just now. Fresh boot of Koha Testing Docker, fresh git pull, and an extra git reset just for extra measure. When I did the hard reset it said:
    HEAD is now at fee4368cd8 Bug 35930: Add guards for plugins_enabled

Applying the patch with git bz, I get the same errors as yesterday:
    error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/tools/additional-contents.tt).
    error: could not build fake ancestor

I was able to test and sign off on another patch for another bug yesterday, so I'm fairly certain the issue is with this patch and not my setup. I'm sorry I don't have any more insights than that.
Comment 15 Blou 2024-03-07 18:55:15 UTC
Myka, Bug 35930 has been on the stack for a while.  My grep tells me it's number 486 on the master stack.

It's possible to test plenty of issue with an old master, but when it fails, it fails.  The issue right now seems with your setup, I'm sorry.
Comment 16 Myka Kennedy Stephens 2024-03-07 19:16:12 UTC
Created attachment 162929 [details] [review]
Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches

When syspref Independentbranches is enabled Non-superuser should not be able to write Additional contents for other branches

To reproduce:
1- Connect to staff interface as superlibrarian
2- Enable IndependentBranches preference
3- Go to Tools -> HTML customizations
4- Crete an additional content for all libraries
5- Create or edit a borrower
    5-1- Set permission for the borrower
        - catalogue
        - edit_additional_contents
4- Log in to the staff interface with the non-superlibrarian staff user
5- Go to Tools -> HTML customizations
    --> you can delete or edit the additional contents created by superlibrarian for All libraries
6- Click on New entry
    --> you can add additional content for other branches
7- Apply the patch
8- Repeat step 5
    --> you cannot delete or edit the additional contents create by superlibrarian for All libraries
9- Repeat step 6
    --> you cannot add additional content for other branches
10- submit the form
    --> the branch of the created additional content is the current user branch

Signed-off-by: Myka Kennedy Stephens <mkstephens@fosgail.org>
Comment 17 Myka Kennedy Stephens 2024-03-07 19:38:12 UTC
I've been doing git pulls outside of Koha testing docker, which was making it look like my Koha repository was updating, but I guess it wasn't. I did a git pull from within the ktd shell, applied the patch, and then it worked!

Applying the patch has the desired effect that a non-superuser from another branch has the option to preview HTML content but the edit and delete buttons are hidden from content for other branches and all libraries.

After signing-off, I am having some trouble with steps 9 and 10 of the test plan. After trying to save new content with the limited-access user, I get the following error: Template process failed: undef error - The method Koha::AdditionalContent->idnew is not covered by tests!

This error persists to the superuser login. When I do a hard reset to master, the error goes away and I can see the content that I attempted to add and save before I got the error.

Hoping QA or a second tester can look into this further. I'm leaving it as signed off, but please change if there's a more appropriate status.
Comment 18 Blou 2024-03-07 19:51:19 UTC
Well, I'll fail it myself :-P
Matthias, the code for additional-contents.pl has been all reformated, like you passed it in perltidy.  It makes it very hard to see the actual modifications.

Please resend the patch with no extra formating, please (beside the lines with actual changes).
Comment 19 Matthias Le Gac 2024-03-08 15:37:28 UTC
Created attachment 162972 [details] [review]
Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches

When syspref Independentbranches is enabled Non-superuser should not be able to write Additional contents for other branches

To reproduce:
1- Connect to staff interface as superlibrarian
2- Enable IndependentBranches preference
3- Go to Tools -> HTML customizations
4- Crete an additional content for all libraries
5- Create or edit a borrower
    5-1- Set permission for the borrower
        - catalogue
        - edit_additional_contents
4- Log in to the staff interface with the non-superlibrarian staff user
5- Go to Tools -> HTML customizations
    --> you can delete or edit the additional contents created by superlibrarian for All libraries
6- Click on New entry
    --> you can add additional content for other branches
7- Apply the patch
8- Repeat step 5
    --> you cannot delete or edit the additional contents create by superlibrarian for All libraries
9- Repeat step 6
    --> you cannot add additional content for other branches
10- submit the form
    --> the branch of the created additional content is the current user branch
Comment 20 Matthias Le Gac 2024-03-08 15:42:12 UTC
Sorry for the confusion, I was working on an old master's DB, so I hadn't seen the change of a field in the DB that broke everything. Normally the problem is solved I managed to make the test plan.
Comment 21 Myka Kennedy Stephens 2024-03-09 01:05:56 UTC
Created attachment 162995 [details] [review]
Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches

When syspref Independentbranches is enabled Non-superuser should not be able to write Additional contents for other branches

To reproduce:
1- Connect to staff interface as superlibrarian
2- Enable IndependentBranches preference
3- Go to Tools -> HTML customizations
4- Crete an additional content for all libraries
5- Create or edit a borrower
    5-1- Set permission for the borrower
        - catalogue
        - edit_additional_contents
4- Log in to the staff interface with the non-superlibrarian staff user
5- Go to Tools -> HTML customizations
    --> you can delete or edit the additional contents created by superlibrarian for All libraries
6- Click on New entry
    --> you can add additional content for other branches
7- Apply the patch
8- Repeat step 5
    --> you cannot delete or edit the additional contents create by superlibrarian for All libraries
9- Repeat step 6
    --> you cannot add additional content for other branches
10- submit the form
    --> the branch of the created additional content is the current user branch

Signed-off-by: Myka Kennedy Stephens <mkstephens@fosgail.org>
Comment 22 Myka Kennedy Stephens 2024-03-09 01:12:35 UTC
This patch works beautifully! I completed the test plan and everything worked as expected this time, very smooth. With the patch applied, the non-superlibrarian staff user can preview HTML customizations for all libraries and for other branches -- the edit and delete buttons are suppressed. This user can also edit existing content for their branch. When creating new content, the drop-down to select all libraries or a particular branch is suppressed. The user can create new content successfully and when returned to the table of all HTML customizations, the new content appears under the user's branch and has the edit and delete buttons available.

Great work! Glad all the back-and-forth finally paid off and it sounds like we both learned a little in the process. :-)