circ/offline.pl needs the perm circulate_remaining_permissions but tools/upload-file.pl needs tools => * That means a librarian cannot upload the .koc files generated offline
Created attachment 35661 [details] [review] Bug 13663: allow a librarian with circulate_remaining_permissions to upload koc files The tools/upload-file.pl script require one of the tools permissions to upload koc files. This script is called by offline_circ/process_koc.pl which requires circulate_remaining_permissions. It does not make sense to require another permission for this script. Test plan: Verify that the circulate_remaining_permissions perm is enough to upload .koc file.
Created attachment 35882 [details] [review] Bug 13663: allow a librarian with circulate_remaining_permissions to upload koc files The tools/upload-file.pl script require one of the tools permissions to upload koc files. This script is called by offline_circ/process_koc.pl which requires circulate_remaining_permissions. It does not make sense to require another permission for this script. Test plan: Verify that the circulate_remaining_permissions perm is enough to upload .koc file. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
If the patron doesn't have any tools permission, you have to log in again after uploading the file. I think this is because upload-file-progress.pl still requires the tool permission. Are we sure those files are only used for the offline circulation file upload?
(In reply to Katrin Fischer from comment #3) > If the patron doesn't have any tools permission, you have to log in again > after uploading the file. > > I think this is because > upload-file-progress.pl still requires the tool permission. > > Are we sure those files are only used for the offline circulation file > upload? Hum, I was sure I tested that, but actually you are right, the file is used by other scripts: upload-file.pl is used in file-upload.in which is used in - offline_circ/process_koc.tt - tools/stage-marc-import.tt - tools/upload-images.tt So this patch does not fix the problem correctly.
(In reply to Jonathan Druart from comment #4) > Hum, I was sure I tested that, but actually you are right, the file is used > by other scripts: upload-file.pl is used in file-upload.in which is used in > - offline_circ/process_koc.tt > - tools/stage-marc-import.tt > - tools/upload-images.tt > > So this patch does not fix the problem correctly. So, we don't need to check permissions again in upload-file.pl since we did it in offline_circ/process_koc.pl, tools/stage-marc-import.pl and tools/upload-cover-image.pl ?
I don't feel comfortable with 'not checking' - maybe just check that one of the needed permissions is given? - circulate_remaining_permissions - stage marc import - upload images
@Katrin: It's not about "not checking" because, if i'm not wrong, it's not possible to access/use upload-file.pl without a least one of these 3 permissions since they are checked before. The question is: Why checking them once again in upload-file.pl?
I guess it just doesn't sound very clean :) And someone without the proper permissions might be able to access the script then?
Oh yes, indeed, just seen that could be a security problem. So, why not check only catalogue permission (Which is the one required to access staff interface iirc)? I feel better with that because we won't need to update upload-file.pl and upload-file-progress.pl permissions each time we add a new page requiring these files with an other permission.
I don't think we will add more very often - I'd like it better if it was limited to the 3 permissions linked to the features.
Created attachment 40089 [details] [review] Bug 13663 - Fix permissions in upload-file.pl and upload-file-progress.pl
Created attachment 40123 [details] [review] Bug 13663 - Fix permissions in upload-file.pl and upload-file-progress.pl Signed-off-by: Indranil Das Gupta (L2C2 Technologies) <indradg@gmail.com>
Created attachment 40124 [details] [review] (followup) fixes tab char QA test fails Fixes the tab char qa test suite fails in the following files: 1/ tools/upload-file-progress.pl 2/ tools/upload-file.pl
I think Bug 6874 will impact this bug
Created attachment 41380 [details] [review] Bug 13663 - Fix permissions in upload-file.pl and upload-file-progress.pl Test plan: Verify that the circulate_remaining_permissions perm is enough to upload .koc file. Signed-off-by: Indranil Das Gupta (L2C2 Technologies) <indradg@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 41381 [details] [review] Bug 13663: (followup) fixes tab char QA test fails Fixes the tab char qa test suite fails in the following files: 1/ tools/upload-file-progress.pl 2/ tools/upload-file.pl Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Amended patch: tidy whitespaces
Patches pushed to master. Thanks Alex!
Pushed to 3.20.x will be in 3.20.3
(In reply to Fridolin SOMERS from comment #14) > I think Bug 6874 will impact this bug In the end no, the feature does not use upload-file.pl nor upload-file-progress.pl
Pushed to 3.16.x, will be in 3.16.14