Bug 14686 - New menu option and permission for file uploading
Summary: New menu option and permission for file uploading
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 16529
  Show dependency treegraph
 
Reported: 2015-08-17 12:22 UTC by Marcel de Rooy
Modified: 2017-12-07 22:15 UTC (History)
4 users (show)

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


Attachments
Bug 14686: Add allows_add_by to check permissions for uploading (5.86 KB, patch)
2016-03-17 12:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14686: Add two granular permission for upload under tools (3.77 KB, patch)
2016-03-17 12:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14686: Add Upload to Tools menu (6.67 KB, patch)
2016-03-17 12:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14686: Add two granular permission for upload under tools (3.51 KB, patch)
2016-03-17 12:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14686: Add Upload to Tools menu (6.67 KB, patch)
2016-03-17 12:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14686: Add allows_add_by to check permissions for uploading (5.87 KB, patch)
2016-03-24 19:44 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 14686: Add two granular permission for upload under tools (3.53 KB, patch)
2016-03-24 19:44 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 14686: Add Upload to Tools menu (6.69 KB, patch)
2016-03-24 19:44 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 14686: [QA Follow-up] Move allows_add_by to Upload.pm (5.23 KB, patch)
2016-04-01 09:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14686: Add Upload to Tools menu (6.74 KB, patch)
2016-04-04 06:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14686: [QA Follow-up] Move allows_add_by to Upload.pm (5.23 KB, patch)
2016-04-04 06:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14686: Add allows_add_by to check permissions for uploading (5.95 KB, patch)
2016-04-04 13:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14686: Add two granular permission for upload under tools (3.60 KB, patch)
2016-04-04 13:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14686: Add Upload to Tools menu (6.77 KB, patch)
2016-04-04 13:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14686: [QA Follow-up] Move allows_add_by to Upload.pm (5.25 KB, patch)
2016-04-04 13:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14686: Remove unused use (739 bytes, patch)
2016-04-04 13:23 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 Marcel de Rooy 2015-08-17 12:22:07 UTC
Bug 14321 adds tools/upload.pl
This report adds a specific permission and adds it to the menu.
Comment 1 Marcel de Rooy 2016-02-08 08:00:10 UTC
Let's get this one moving..
Comment 2 Marcel de Rooy 2016-03-17 12:32:22 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2016-03-17 12:32:29 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2016-03-17 12:32:33 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2016-03-17 12:38:18 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2016-03-17 12:38:21 UTC Comment hidden (obsolete)
Comment 7 Mirko Tietgen 2016-03-24 19:44:24 UTC Comment hidden (obsolete)
Comment 8 Mirko Tietgen 2016-03-24 19:44:31 UTC Comment hidden (obsolete)
Comment 9 Mirko Tietgen 2016-03-24 19:44:37 UTC Comment hidden (obsolete)
Comment 10 Mirko Tietgen 2016-03-24 19:47:26 UTC
Works as advertised.

One question: without upload_manage permission users are able to delete the files they uploaded themselves. I suppose that is intended, but it was not explicitly mentioned in the test plan. Is it the intended behaviour?
Comment 11 Marcel de Rooy 2016-03-24 20:55:09 UTC
(In reply to Mirko Tietgen from comment #10)
> Works as advertised.
> 
> One question: without upload_manage permission users are able to delete the
> files they uploaded themselves. I suppose that is intended, but it was not
> explicitly mentioned in the test plan. Is it the intended behaviour?

Thx for testing.
Yes, it is intended. (I wrote it in the first commit message, maybe should have been more explicit).
Comment 12 Jesse Weaver 2016-03-24 22:44:29 UTC
I strongly believe that for consistency with the rest of Koha (and due to frustrations that changing the autogenerated schema files causes), that the sub allows_add_by should be moved to a Koha:: module. The code has no tie to the uploaded_files table anyway.

Koha::UploadedFiles might be a good name, as that could later easily become a Koha::Objects subclass, but that's just a suggestion.
Comment 13 Marcel de Rooy 2016-03-25 07:35:54 UTC
(In reply to Jesse Weaver from comment #12)
> I strongly believe that for consistency with the rest of Koha (and due to
> frustrations that changing the autogenerated schema files causes), that the
> sub allows_add_by should be moved to a Koha:: module. The code has no tie to
> the uploaded_files table anyway.

Hi Jesse,
I am all for consistency. But I moved this sub here instead of Upload.pm because it does not really fit into Upload's object design. Felt like polluting..
Not sure what frustrations you exactly mean, but imo they should not be a reason to block this patch. The custom section of the Results modules should be regenerated each update.
The tie to the table is indirect but clear: the sub functionally determines if you have permission to add to the table.

> Koha::UploadedFiles might be a good name, as that could later easily become
> a Koha::Objects subclass, but that's just a suggestion.
I referred to the Object[s] in the commit message, but doing the whole exercise was out of scope here. And just adding it for one sub did not attract me.

So I am not convinced that we should move it. But if you really insist, I'd rather move it to Upload.pm or so?
Comment 14 Marcel de Rooy 2016-04-01 09:26:39 UTC Comment hidden (obsolete)
Comment 15 Marcel de Rooy 2016-04-01 09:28:34 UTC
(In reply to Marcel de Rooy from comment #13)
> So I am not convinced that we should move it. But if you really insist, I'd
> rather move it to Upload.pm or so?

No reply is also an answer.
Jesse: I moved it. Please finish QA now.
Comment 16 Marcel de Rooy 2016-04-04 06:45:41 UTC Comment hidden (obsolete)
Comment 17 Marcel de Rooy 2016-04-04 06:45:47 UTC Comment hidden (obsolete)
Comment 18 Marcel de Rooy 2016-04-04 06:46:38 UTC
Rebased third patch.
Comment 19 Jonathan Druart 2016-04-04 11:11:17 UTC
(In reply to Jesse Weaver from comment #12)
> I strongly believe that for consistency with the rest of Koha (and due to
> frustrations that changing the autogenerated schema files causes), that the
> sub allows_add_by should be moved to a Koha:: module. The code has no tie to
> the uploaded_files table anyway.
> 
> Koha::UploadedFiles might be a good name, as that could later easily become
> a Koha::Objects subclass, but that's just a suggestion.

I agree with Jesse, you are adding a new way to do something where we already have one.
For instance, on Koha::Virtualshelf, there are several can_* methods taking a borrowernumber in parameter. It seems that it's exactly what you are achieving here. It would be better to stick on the current form or to suggest and move everything to another one.
Comment 20 Jonathan Druart 2016-04-04 11:12:13 UTC
(In reply to Marcel de Rooy from comment #15)
> (In reply to Marcel de Rooy from comment #13)
> > So I am not convinced that we should move it. But if you really insist, I'd
> > rather move it to Upload.pm or so?
> 
> No reply is also an answer.
> Jesse: I moved it. Please finish QA now.

ok seen!
Comment 21 Jonathan Druart 2016-04-04 11:36:44 UTC
Why does upload_manage is not set by default?
Before this patch I was able to delete uploads, now I need the permission.
Comment 22 Marcel de Rooy 2016-04-04 11:45:15 UTC
(In reply to Jonathan Druart from comment #21)
> Why does upload_manage is not set by default?
> Before this patch I was able to delete uploads, now I need the permission.

You should be able to delete your own uploads, not the other ones.
Comment 23 Jonathan Druart 2016-04-04 12:24:51 UTC
(In reply to Marcel de Rooy from comment #22)
> (In reply to Jonathan Druart from comment #21)
> > Why does upload_manage is not set by default?
> > Before this patch I was able to delete uploads, now I need the permission.
> 
> You should be able to delete your own uploads, not the other ones.

Yes but it's a change in the behavior, would not it better to set it for existing installs?
Comment 24 Marcel de Rooy 2016-04-04 12:29:11 UTC
(In reply to Jonathan Druart from comment #23)
> Yes but it's a change in the behavior, would not it better to set it for
> existing installs?

We add upload_general for active existing installs. I would rather not add the manage permission because it is meant to 'protect' against such deletes. Note that someone with all tools or superlibrarians can.
Comment 25 Jonathan Druart 2016-04-04 13:22:35 UTC
Created attachment 49851 [details] [review]
Bug 14686: Add allows_add_by to check permissions for uploading

The three permissions in tools/upload-file.pl are moved to a custom
routine in DBIx's UploadedFile.pm. An additional granular permission
upload_general_files is added. (The dbrev patch will contain it too.)
At some point in time this could be moved to a Koha::Object class.

The routine is tested in Upload.t.

Test plan:
[1] Run t/db_dependent/Upload.t
[2] If you only apply this patch, you can also test uploading with a
    user that has edit_catalogue but does not have tools or circulate.
    Upload status should say: Denied.

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 26 Jonathan Druart 2016-04-04 13:22:39 UTC
Created attachment 49852 [details] [review]
Bug 14686: Add two granular permission for upload under tools

[1] Upload_general_files
    This will allow you to access the tools/upload form via the Tools menu.
    And it will also allow you to add permanent uploads.

[2] Upload_manage
    It will allow you to delete uploads from other users.
    Note that anyone may delete his own uploads.
    It is not meant to include upload_general_files.

In order to not disturb existing users that now have edit_catalogue and are
using the plugin (read: added at least one record to uploaded_files), we
will add the first permission for them. New users will need to be authorized
in the usual way thereafter.

Note: If you only have one of the other permissions checked in allows_add_by,
e.g. stage_marc_import, you can add temporary uploads, but not permanent
uploads.

Test plan:
We are only testing the dbrev here, not if the perm works as advertised.
[1] Run the dbrev and check that you see the perms with a description on
    the Patron/Set permissions page.
[2] If you had records in uploaded_files and a user with edit_catalogue
    *only*, verify that this user now also has upload_general_files.

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 27 Jonathan Druart 2016-04-04 13:22:44 UTC
Created attachment 49853 [details] [review]
Bug 14686: Add Upload to Tools menu

This patch makes sure that the added granular permissions work as
advertised.

Note: The field owner was not included in the Koha::Upload->get response.
The code to verify if a user is allowed to delete an upload, is concentrated
in the template now. When get returns a Koha::Object, this check could be
relocated.

Test plan:
[1] Verify that the current user has permission for tools, or has
    at least upload_general_files.
[2] Do you see Upload in the Tools menu? Follow the link.
[3] Upload a permanent file (with a category).
[4] Do you see the Delete button in the results form?
[5] Make sure that another user has no permission to upload.
[6] Login as that user and check the Tools menu.
    Try the URL [yourserver]/cgi-bin/koha/tools/upload.pl
    You should have no access to the upload form.
[7] Enable upload_general_files for this user. Go to upload and search for
    the upload from step 3. You should not see a Delete button.
[8] Enable upload_manage for this user. Search for the upload again.
    Delete the upload.
[9] Go to upload via the Cataloguing editor (856$u plugin) or add
    parameter "plugin=1" to the URL. You should not see the Tools menu.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 28 Jonathan Druart 2016-04-04 13:22:49 UTC
Created attachment 49854 [details] [review]
Bug 14686: [QA Follow-up] Move allows_add_by to Upload.pm

As requested by QA, the sub should be moved.

Test plan:
See previous patches.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 29 Jonathan Druart 2016-04-04 13:23:02 UTC
Created attachment 49855 [details] [review]
Bug 14686: Remove unused use

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 30 Marcel de Rooy 2016-04-04 13:29:29 UTC
(In reply to Jonathan Druart from comment #29)
> Created attachment 49855 [details] [review] [review]
> Bug 14686: Remove unused use
> 
> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Thx for QAing!
Comment 31 Brendan Gallagher 2016-04-27 16:25:36 UTC
Pushed to Master - Should be in the May 2016 Release.  Thanks!
Comment 32 Marcel de Rooy 2016-11-20 15:48:27 UTC
Fix for new installs on bug 17663