Bug 14321 adds tools/upload.pl This report adds a specific permission and adds it to the menu.
Let's get this one moving..
Created attachment 49257 [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.
Created attachment 49258 [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 1: If you have edit_catalogue, you can also use the form via the plugin in the cataloguing editor, but you can only search for uploads if you do not have upload_general_files. (This change of behavior triggered adding the permission for current users.) Note 2: 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.
Created attachment 49259 [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>
Created attachment 49260 [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.
Created attachment 49261 [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>
Created attachment 49552 [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>
Created attachment 49553 [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>
Created attachment 49554 [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>
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?
(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).
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.
(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?
Created attachment 49780 [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>
(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.
Created attachment 49835 [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>
Created attachment 49836 [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>
Rebased third patch.
(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.
(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!
Why does upload_manage is not set by default? Before this patch I was able to delete uploads, now I need the permission.
(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.
(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?
(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.
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>
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>
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>
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>
Created attachment 49855 [details] [review] Bug 14686: Remove unused use Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
(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!
Pushed to Master - Should be in the May 2016 Release. Thanks!
Fix for new installs on bug 17663