Bug 28303 - Having multiple pluginsdir causes plugin_upload to try to write to the opac-tmpl folder
Summary: Having multiple pluginsdir causes plugin_upload to try to write to the opac-t...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Plugin architecture (show other bugs)
Version: master
Hardware: All All
: P5 - low trivial (vote)
Assignee: Michael Hafen
QA Contact: Joonas Kylmälä
URL:
Keywords:
: 28143 (view as bug list)
Depends on:
Blocks:
 
Reported: 2021-05-07 20:15 UTC by Michael Hafen
Modified: 2021-11-06 16:41 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.05,20.11.11


Attachments
Proposed fix (1.27 KB, patch)
2021-05-07 20:17 UTC, Michael Hafen
Details | Diff | Splinter Review
Bug 28303 - Fix plugins system with multiple pluginsdir settings (1.27 KB, patch)
2021-08-03 20:04 UTC, David Nind
Details | Diff | Splinter Review
Bug 28303: Add unit test for badtemplatecheck (1.80 KB, patch)
2021-10-03 10:59 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28303: Fix plugins system with multiple pluginsdir settings (1.51 KB, patch)
2021-10-03 10:59 UTC, Joonas Kylmälä
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Hafen 2021-05-07 20:15:22 UTC
With multiple pluginsdir settings in koha-conf.xml, C4/Template::badtemplatecheck changes the config, causing plugins-upload to try to unpack plugins to the opac-tmpl folder which badtemplatecheck unshifts into the pluginsdir array in config.
Comment 1 Michael Hafen 2021-05-07 20:17:08 UTC
Created attachment 120722 [details] [review]
Proposed fix
Comment 2 David Nind 2021-07-30 19:06:10 UTC
Hi Michael. 

Could you create a test plan for this?

Then I will attempt to sign off.

Thanks.

David
Comment 3 Michael Hafen 2021-08-02 23:40:59 UTC
Test plan:

1. edit a koha instance's koha-conf.xml to set enable_plugins to 1
1.a   restart memcached service as necessary
2. upload plugin in Administration -> Manage plugins -> Upload plugin
3. plugin will be listed in the Manage plugins page
4. create a directory, for example `mkdir -p /var/lib/koha-common/plugins`
5. edit koha-conf.xml to add a pluginsdir tag pointing to the directory created in step 4
5.a   restart memcached service as necessary
6. uninstall plugin
7. try to upload plugin again.  Observe error message to verify that the Apache user can write to the plugins directory.
8. apply patch
9. try to upload plugin again.  It should succeed.
Comment 4 David Nind 2021-08-03 20:04:18 UTC
Created attachment 123444 [details] [review]
Bug 28303 - Fix plugins system with multiple pluginsdir settings

C4/Templates::badtemplatecheck mucks with the config('pluginsdir') array ref.
This makes sure it operates on a copy of the array.

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 David Nind 2021-08-03 20:06:30 UTC
Comment on attachment 120722 [details] [review]
Proposed fix

Not sure what happened here - obsoleting patch so there is only one (the signed off one).
Comment 6 David Nind 2021-08-03 20:20:27 UTC
Thanks Michael for the test plan!

I have now tested and signed off.

Testing notes (using koha-testing-docker):
- Step 7: 
  . I didn't see an error (maybe because I changed the user/group for the new directory to match the koha instance)
  . The plugin looked okay (I used ByWaterSolutions' dev-koha-plugin-kitchen-sink) but there were errors when i tried to do anything with it - it looks like it was uploaded to koha-tmpl/opac-tmpl
- After applying the patch and restarting the plugin showed as having errors. After reinstalling the plugin everything was back to working as expected and i could install other plugins. Manually deleted the plugin files added to koha-tmpl/opac-tmpl/Koha/

David
Comment 7 Joonas Kylmälä 2021-10-03 10:22:54 UTC
Looks good but missing unit test and fails QA script:

	* Commit title does not start with 'Bug XXXXX: ' - b73fd7c915
Comment 8 Joonas Kylmälä 2021-10-03 10:59:48 UTC
Created attachment 125667 [details] [review]
Bug 28303: Add unit test for badtemplatecheck

This tests that pluginsdir value is not modified by the call to
badtemplatecheck to make sure badtemplatecheck operates with its own
copy of pluginsdir configuration value.

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 9 Joonas Kylmälä 2021-10-03 10:59:52 UTC
Created attachment 125668 [details] [review]
Bug 28303: Fix plugins system with multiple pluginsdir settings

C4/Templates::badtemplatecheck mucks with the config('pluginsdir') array ref.
This makes sure it operates on a copy of the array.

To test:
   1) $ prove t/db_dependent/Templates.t

Signed-off-by: David Nind <david@davidnind.com>
JK: Fix commit message styling and add test plan

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 10 Joonas Kylmälä 2021-10-03 11:00:26 UTC
(In reply to Joonas Kylmälä from comment #7)
> Looks good but missing unit test and fails QA script:
> 
> 	* Commit title does not start with 'Bug XXXXX: ' - b73fd7c915

I added the unit test and fixed the commit title. Passing QA.
Comment 11 Katrin Fischer 2021-10-03 12:12:48 UTC
Thank you, Joonas!
Comment 12 Jonathan Druart 2021-10-06 08:37:22 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 13 Kyle M Hall 2021-10-08 14:25:19 UTC
Pushed to 21.05.x for 21.05.05
Comment 14 Fridolin Somers 2021-10-21 05:58:11 UTC
Pushed to 20.11.x for 20.11.11
Comment 15 Victor Grousset/tuxayo 2021-10-26 01:00:20 UTC
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.
Comment 16 Joonas Kylmälä 2021-11-06 16:41:34 UTC
*** Bug 28143 has been marked as a duplicate of this bug. ***