Description
Hammat wele
2023-08-28 17:10:56 UTC
Created attachment 154889 [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 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 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 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 rebase to master 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. 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 @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. Matthias, your patch sets now include merge markers. Please use the QA tool! 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 I think it's good now to test sorry for the trouble I've caused by trying to rebase 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 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 ? 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. 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. 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> 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. 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). 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 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. 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> 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. :-) Signed-off-by: John Doe <you@example.com> - my $branchcode = $cgi->param('branchcode') || undef; + my $branchcode = $my_branch ? $my_branch : $cgi->param('branchcode') || undef; This looks odd at least. I am posting on branchcode and we will write to current branch? + [% UNLESS ( my_branch && my_branch != c.branchcode ) %] <input type="checkbox" name="ids" value="[% c.id | html %]" /> + [% END %] Hiding an id based on an unclear condition (my_branch does not represent what we intuitively think that it is). Note that I found one counterargument: tools/letter.pl:our $my_branch = C4::Context->preference("IndependentBranches") && !$staffflags->{'superlibrarian'} This still needs a bit more attention at both server side and client side. Created attachment 172231 [details] [review] Bug 34631: (Follow up) Changed variable name Same test plan as before just changed variable name for clarity. The logic for the branchcode has changed slightly, and it seems to be the right approach. You can access the staff interface with two roles: superlibrarian or staff. Previously, there were no checks, allowing unrestricted editing and deleting. The main issue was with variable names, which weren’t intuitive. To clarify, the branchcode is linked to the library field (Tools -> HTML customization -> new entry). Now, superlibrarians have access to all libraries, while staff can only access their own. The variable my_branch indicates whether the user is a staff, adjusting the interface accordingly. I've renamed it for clarity, but that appears to be the only necessary change at this point. Regarding the hidden ID of the checkbox, it works similarly: if you're staff and your library is different, there’s no checkbox, as it allows deletion of selected items. Created attachment 175973 [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: John Doe <you@example.com> Signed-off-by: David Nind <david@davidnind.com> Created attachment 175974 [details] [review] Bug 34631: (Follow up) Changed variable name Same test plan as before just changed variable name for clarity. Signed-off-by: David Nind <david@davidnind.com> Created attachment 175975 [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> Signed-off-by: David Nind <david@davidnind.com> Created attachment 175976 [details] [review] Bug 34631: (Follow up) Changed variable name Same test plan as before just changed variable name for clarity. Signed-off-by: David Nind <david@davidnind.com> I fixed the original commit message sign off signature (changed John Doe to Myka Kennedy Stephens). This feels like a workaround to me. Harder to maintain code. There should be another way to do this better. I wont pass QA but leave room for another QA point of view. No coding guideline to FQA either. Some comments from another IndependentBranches related bug that may be relevant for this bug - bug 38040 comment #13: "... The trend has been to move parts of Independent Branches to more granular system preferences ( IndependentBranchesPatronModifications , IndependentBranchesTransfers ) so maybe we need 'IndependentBranchesReserves'? ...." (In reply to David Nind from comment #35) > Some comments from another IndependentBranches related bug that may be > relevant for this bug - bug 38040 comment #13: > > "... > The trend has been to move parts of Independent Branches to more granular > system preferences ( IndependentBranchesPatronModifications , > IndependentBranchesTransfers ) so maybe we need > 'IndependentBranchesReserves'? > ...." And there are also library groups that already does a lot of what independentbranches did, but in a better and more flexible way. It would be nice to be able to get rid of the preference at some point. Agreeing with others here - we need a new pref or another way of determining access - this would be a large change of behavior. Additionally, I can still edit additional contents through URL manipulation: http://localhost:8081/cgi-bin/koha/tools/additional-contents.pl?op=add_form&id=7&editmode=wysiwyg It doesn't matter if the id in question belongs to my branch, it will after the edit, but I can still affect them Adding the delete button in manually as well: <input type="checkbox" name="ids" value="2"> Allows me to delete any entry Created attachment 179859 [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> Signed-off-by: David Nind <david@davidnind.com> Created attachment 179860 [details] [review] Bug 34631: (Follow up) Changed variable name Same test plan as before just changed variable name for clarity. Signed-off-by: David Nind <david@davidnind.com> Created attachment 180602 [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> Signed-off-by: David Nind <david@davidnind.com> Created attachment 180603 [details] [review] Bug 34631: (Follow up) Changed variable name Same test plan as before just changed variable name for clarity. Signed-off-by: David Nind <david@davidnind.com> Created attachment 180604 [details] [review] Bug 34631: Add IndependentBranchesAdditionalcontents preference and some checks into additional-contents.pl 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 and run updatedatabase 8- Enable IndependentBranchesAdditionalcontents preference 9- Repeat step 5 --> you cannot delete or edit the additional contents create by superlibrarian for All libraries even by url manipulation 10- Repeat step 6 --> you cannot add additional content for other branches 11- submit the form --> the branch of the created additional content is the current user branch Not sure I understand the state of this bug. There are two signed off patches and one that is not signed off. The one that is not signed off has a very similar test plan to the main patch that is signed off. Should the two signed off patches be obsoleted? (In reply to Magnus Enger from comment #43) > Not sure I understand the state of this bug. There are two signed off > patches and one that is not signed off. The one that is not signed off has a > very similar test plan to the main patch that is signed off. > > Should the two signed off patches be obsoleted? Hi Magnus, the one that is not signed off add corrections that is suggested in comment 37, that need to be signed off. I attached a test plan because there is a new preference IndependentBranchesAdditionalcontents, the two first patches should not be obsoleted This only seems to partially work, if I have tested correctly. I Failed QA because of 4.4 and 4.5 - I can't edit existing items for my specific library, but I can delete it. 1. I only tested with news. 2. Using the superlibrarian (koha), I created two news items: 2.1 One for all libraries 2.2 One for Midway only 3. I changed the permissions for Henry Acevedo. 4. With the patch applied, IndependentBranchesAdditionalcontents = Yes, and logged in as Henry: 4.1 I can't edit or delete the news item for all libraries (as expected) 4.2 I have options to edit and delete the news item for Midway (as expected) 4.3 I can delete the news item for Midway (as expected) 4.4 When I go to edit the news item for Midway, the edit form is not pre-populated with the news item details - the form title is "New news item": . For the Library field, the "All libraries" option is selected . There is no publication date . There are no news item details (title and content) 4.5 If I create a news item: . For the library field, I can choose any library - when I save, it changes the library to Midway . When I edit the news item, the same as 4.4 . If I fill in the fields and save, a new news item is created Suggestion for the system preference description: - Current: Prevent staff (but not superlibrarians) from editing Additional contents in other libraries: - Suggested: Prevent staff (but not superlibrarians) from editing additional contents for other libraries: |