Bug 13663 - offline circ permission issue
Summary: offline circ permission issue
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Alex Arnaud
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-02-03 14:55 UTC by Jonathan Druart
Modified: 2016-12-05 21:22 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 13663: allow a librarian with circulate_remaining_permissions to upload koc files (1.28 KB, patch)
2015-02-03 15:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13663: allow a librarian with circulate_remaining_permissions to upload koc files (1.35 KB, patch)
2015-02-12 23:34 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 13663 - Fix permissions in upload-file.pl and upload-file-progress.pl (2.91 KB, patch)
2015-06-11 08:57 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 13663 - Fix permissions in upload-file.pl and upload-file-progress.pl (2.98 KB, patch)
2015-06-14 09:19 UTC, Indranil Das Gupta
Details | Diff | Splinter Review
(followup) fixes tab char QA test fails (2.81 KB, patch)
2015-06-14 09:24 UTC, Indranil Das Gupta
Details | Diff | Splinter Review
Bug 13663 - Fix permissions in upload-file.pl and upload-file-progress.pl (3.14 KB, patch)
2015-08-05 12:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13663: (followup) fixes tab char QA test fails (2.91 KB, patch)
2015-08-05 12:29 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2015-02-03 14:55:03 UTC
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
Comment 1 Jonathan Druart 2015-02-03 15:02:09 UTC Comment hidden (obsolete)
Comment 2 Brendan Gallagher 2015-02-12 23:34:22 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2015-02-14 17:45:58 UTC
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?
Comment 4 Jonathan Druart 2015-02-16 10:20:31 UTC
(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.
Comment 5 Alex Arnaud 2015-06-09 12:54:07 UTC
(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 ?
Comment 6 Katrin Fischer 2015-06-09 12:56:59 UTC
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
Comment 7 Alex Arnaud 2015-06-09 13:52:05 UTC
@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?
Comment 8 Katrin Fischer 2015-06-09 14:01:40 UTC
I guess it just doesn't sound very clean :) And someone without the proper permissions might be able to access the script then?
Comment 9 Alex Arnaud 2015-06-09 14:15:43 UTC
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.
Comment 10 Katrin Fischer 2015-06-09 19:47:41 UTC
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.
Comment 11 Alex Arnaud 2015-06-11 08:57:00 UTC Comment hidden (obsolete)
Comment 12 Indranil Das Gupta 2015-06-14 09:19:27 UTC Comment hidden (obsolete)
Comment 13 Indranil Das Gupta 2015-06-14 09:24:32 UTC Comment hidden (obsolete)
Comment 14 Fridolin Somers 2015-06-19 09:39:20 UTC
I think Bug 6874 will impact this bug
Comment 15 Jonathan Druart 2015-08-05 12:29:38 UTC
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>
Comment 16 Jonathan Druart 2015-08-05 12:29:49 UTC
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
Comment 17 Tomás Cohen Arazi 2015-08-11 16:00:15 UTC
Patches pushed to master.

Thanks Alex!
Comment 18 Chris Cormack 2015-08-11 20:24:22 UTC
Pushed to 3.20.x will be in 3.20.3
Comment 19 Fridolin Somers 2015-08-20 13:50:45 UTC
(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
Comment 20 Mason James 2015-08-28 06:07:15 UTC
Pushed to 3.16.x, will be in 3.16.14