Bug 25411

Summary: Plugin routes cannot have anonymous access
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: REST APIAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Jonathan Druart <jonathan.druart+koha>
Severity: major    
Priority: P5 - low CC: aleisha, dcook, jonathan.druart+koha, josef.moravec, kyle, martin.renvoize, nick, victor
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.07
Bug Depends on: 25327    
Bug Blocks: 24003, 26322    
Attachments: Bug 25411: Regression tests
Bug 25411: Add special handling for public plugin routes
Bug 25411: Regression tests
Bug 25411: Add special handling for public plugin routes
Bug 25411: Regression tests
Bug 25411: Add special handling for public plugin routes
Bug 25411: Group conditions in if elsif
[19.11.x] Bug 25411: Regression tests
[19.11.x] Bug 25411: Add special handling for public plugin routes

Description Tomás Cohen Arazi 2020-05-07 11:52:05 UTC
This is a consequence of a big change introduced by bug 25045
Comment 1 Tomás Cohen Arazi 2020-05-15 18:49:01 UTC
*** Bug 25484 has been marked as a duplicate of this bug. ***
Comment 2 Tomás Cohen Arazi 2020-05-15 20:15:31 UTC
Created attachment 104968 [details] [review]
Bug 25411: Regression tests

This patch adds tests the new RESTPublicAnonymousRequests syspref
doesn't apply to routes added by plugins.

It is up to the plugin developer to handle those conditions.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/PluginRoutes.t
=> FAIL: Notice the tests fail
Comment 3 Tomás Cohen Arazi 2020-05-15 20:15:36 UTC
Created attachment 104969 [details] [review]
Bug 25411: Add special handling for public plugin routes

This patch implements the required logic in the API code so plugins are
not affected by the new RESTPublicAnonymousRequests system preference.
It is up to the plugin develpers to handle this

To test:
1. Apply the tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/PluginRoutes.t
=> FAIL: Notice the tests fail
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! Yay! Not bad for a friday evening!
5. Sign off :-D

Sponsored-by: ByWater Solutions
Comment 4 Victor Grousset/tuxayo 2020-05-16 04:52:28 UTC
Created attachment 104978 [details] [review]
Bug 25411: Regression tests

This patch adds tests the new RESTPublicAnonymousRequests syspref
doesn't apply to routes added by plugins.

It is up to the plugin developer to handle those conditions.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/PluginRoutes.t
=> FAIL: Notice the tests fail

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 5 Victor Grousset/tuxayo 2020-05-16 04:52:31 UTC
Created attachment 104979 [details] [review]
Bug 25411: Add special handling for public plugin routes

This patch implements the required logic in the API code so plugins are
not affected by the new RESTPublicAnonymousRequests system preference.
It is up to the plugin develpers to handle this

To test:
1. Apply the tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/PluginRoutes.t
=> FAIL: Notice the tests fail
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! Yay! Not bad for a friday evening!
5. Sign off :-D

Sponsored-by: ByWater Solutions
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 6 Victor Grousset/tuxayo 2020-05-16 04:53:20 UTC
It works, and hooray for tests first :)
Comment 7 Victor Grousset/tuxayo 2020-05-16 04:53:52 UTC
Whoops, wrong status.
Comment 8 Jonathan Druart 2020-05-20 15:14:45 UTC
Created attachment 105155 [details] [review]
Bug 25411: Regression tests

This patch adds tests the new RESTPublicAnonymousRequests syspref
doesn't apply to routes added by plugins.

It is up to the plugin developer to handle those conditions.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/PluginRoutes.t
=> FAIL: Notice the tests fail

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 9 Jonathan Druart 2020-05-20 15:14:49 UTC
Created attachment 105156 [details] [review]
Bug 25411: Add special handling for public plugin routes

This patch implements the required logic in the API code so plugins are
not affected by the new RESTPublicAnonymousRequests system preference.
It is up to the plugin develpers to handle this

To test:
1. Apply the tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/PluginRoutes.t
=> FAIL: Notice the tests fail
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! Yay! Not bad for a friday evening!
5. Sign off :-D

Sponsored-by: ByWater Solutions
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Jonathan Druart 2020-05-20 15:14:53 UTC
Created attachment 105157 [details] [review]
Bug 25411: Group conditions in if elsif

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Tomás Cohen Arazi 2020-05-20 17:14:45 UTC
(In reply to Jonathan Druart from comment #10)
> Created attachment 105157 [details] [review] [review]
> Bug 25411: Group conditions in if elsif
> 
> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Thanks, my initial implementation was a bit more complex (it involved forcing a new layout on the plugins side) and when I rolled it back I forgot this).
Comment 12 David Cook 2020-05-21 00:37:49 UTC
I've only skimmed the code, but it looks good at a glance.

For a second, I thought it was making all plugin API routes public, but then I noticed that's only if they don't have 'x-koha-authorization' defined. 

I wonder if there's anything we need or can do in the API documentation to make it more obvious what (plugin) routes require auth and which don't?
Comment 13 David Cook 2020-05-21 00:38:11 UTC
(In reply to David Cook from comment #12)
> I've only skimmed the code, but it looks good at a glance.
> 
> For a second, I thought it was making all plugin API routes public, but then
> I noticed that's only if they don't have 'x-koha-authorization' defined. 
> 
> I wonder if there's anything we need or can do in the API documentation to
> make it more obvious what (plugin) routes require auth and which don't?

Just a general question. Not directly related to this bug report per se...
Comment 14 Martin Renvoize 2020-05-22 08:33:54 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 15 Aleisha Amohia 2020-06-09 02:01:18 UTC
missing dependencies, not backported to 19.11.x
Comment 16 Tomás Cohen Arazi 2020-06-19 12:52:50 UTC
Created attachment 106084 [details] [review]
[19.11.x] Bug 25411: Regression tests

This patch adds tests the new RESTPublicAnonymousRequests syspref
doesn't apply to routes added by plugins.

It is up to the plugin developer to handle those conditions.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/PluginRoutes.t
=> FAIL: Notice the tests fail

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 17 Tomás Cohen Arazi 2020-06-19 12:52:56 UTC
Created attachment 106085 [details] [review]
[19.11.x] Bug 25411: Add special handling for public plugin routes

This patch implements the required logic in the API code so plugins are
not affected by the new RESTPublicAnonymousRequests system preference.
It is up to the plugin develpers to handle this

To test:
1. Apply the tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/PluginRoutes.t
=> FAIL: Notice the tests fail
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! Yay! Not bad for a friday evening!
5. Sign off :-D

Sponsored-by: ByWater Solutions
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 18 Tomás Cohen Arazi 2020-06-19 13:04:51 UTC
Aleisha, only the [19.11.x] patches here.
Comment 19 Aleisha Amohia 2020-06-21 23:52:51 UTC
backported to 19.11.x for 19.11.07
Comment 20 Victor Grousset/tuxayo 2020-07-02 22:34:04 UTC
Missing dependencies for 19.05.x, can't backport.