Bug 26322 - REST API plugin authorization is not checked anymore
Summary: REST API plugin authorization is not checked anymore
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 25411
Blocks:
  Show dependency treegraph
 
Reported: 2020-08-31 08:25 UTC by Joonas Kylmälä
Modified: 2021-06-14 21:31 UTC (History)
11 users (show)

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


Attachments
Bug 26322: Regression tests (3.02 KB, patch)
2020-08-31 12:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26322: Regression tests (3.02 KB, patch)
2020-08-31 12:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26322: Permissions not checked correctly for plugins (1.58 KB, patch)
2020-08-31 12:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26322: Regression tests (3.28 KB, patch)
2020-09-01 13:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26322: Permissions not checked correctly for plugins (1.58 KB, patch)
2020-09-01 13:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26322: Regression tests (3.54 KB, patch)
2020-09-02 12:33 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26322: Permissions not checked correctly for plugins (1.58 KB, patch)
2020-09-02 12:33 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26322: Regression tests (3.69 KB, patch)
2020-09-02 12:40 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 26322: Permissions not checked correctly for plugins (1.73 KB, patch)
2020-09-02 12:40 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 26322: Regression tests (3.75 KB, patch)
2020-09-02 14:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26322: Permissions not checked correctly for plugins (1.79 KB, patch)
2020-09-02 14:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26322: [19.11.x] Regression tests (3.62 KB, patch)
2020-09-21 22:16 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 26322: [19.11.x] Permissions not checked correctly for plugins (1.80 KB, patch)
2020-09-21 22:16 UTC, Aleisha Amohia
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Joonas Kylmälä 2020-08-31 08:25:55 UTC
Since May 22nd REST API plugin authorization has not been working at all, all authenticated users have gotten access to all REST API Plugin endpoints regardless of what permissions would have been required.

The following commit caused this issue due to misplaced parentheses by the looks of it:

Commit: 8d0c44578524dc2bd7b380dac56a53382b5aca7e
Bug 25411: Add special handling for public plugin routes
Comment 1 Tomás Cohen Arazi 2020-08-31 12:37:20 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2020-08-31 12:51:48 UTC
Created attachment 109366 [details] [review]
Bug 26322: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Tomás Cohen Arazi 2020-08-31 12:51:55 UTC
Created attachment 109367 [details] [review]
Bug 26322: Permissions not checked correctly for plugins

This patch fixes the logic in a condition to address the fact that
permissions are not checked for plugins. This was due to bad parenthesis
pairing and the lack of good tests for this.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/PluginRoutes.t
=> FAIL: Tests fail because of bad logic
3. Apply this patch
4. Repeat (2)
=> SUCCESS: Tests pass!
5. Verify the tests cover the use cases that are needed:
   - Anonymous access
   - Real user with wrong permissions (parameters => 1)
   - Real user with right permissions (borrowers => 1)
=> SUCCESS: Needed use cases so we catch any regression are found
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Joonas Kylmälä 2020-09-01 07:39:59 UTC
I need help reviewing this. I don't understand the line in the regression test patch:

+            value => { flags => 2**3 }    # borrowers flag = 4

more specifically I don't understand how the flags work and the comment here confuses me since 2**3 = 8 and not 4. To make sure the test tests the case where the borrower has and doesn't have the permission "borrowers" set I would need to know what is the userflags value for it I think. I got lost in the getuserflags subroutine in C4/Auth.pm trying to decipher the integer values. Any pointers to explain how userflags work would be greatly appreciated.
Comment 5 Jonathan Druart 2020-09-01 09:22:52 UTC
(In reply to Joonas Kylmälä from comment #4)
> I need help reviewing this. I don't understand the line in the regression
> test patch:
> 
> +            value => { flags => 2**3 }    # borrowers flag = 4
> 
> more specifically I don't understand how the flags work and the comment here
> confuses me since 2**3 = 8 and not 4. To make sure the test tests the case
> where the borrower has and doesn't have the permission "borrowers" set I
> would need to know what is the userflags value for it I think. I got lost in
> the getuserflags subroutine in C4/Auth.pm trying to decipher the integer
> values. Any pointers to explain how userflags work would be greatly
> appreciated.

Tomas's comment or code look wrong indeed.

How userflags work:
Have a look at the userflags table, there is a 'bit' and 'flag' columns, like:
(4, borrowers)
(9, editcatalogue)
If a patron a the "top permissions" borrowers and editcatalogue, borrowers.userflags will be 2**4+2**9
If they only have borrowers and some sub permissions of editcatalogue, borrowers.userflags will be 2**4 and sub permissions will be stored in a separate table: user_permissions.
This is described in C4::Auth::getuserflags.
Comment 6 Joonas Kylmälä 2020-09-01 09:35:24 UTC
(In reply to Jonathan Druart from comment #5)
> How userflags work:
> Have a look at the userflags table, there is a 'bit' and 'flag' columns,
> like:
> (4, borrowers)
> (9, editcatalogue)
> If a patron a the "top permissions" borrowers and editcatalogue,
> borrowers.userflags will be 2**4+2**9
> If they only have borrowers and some sub permissions of editcatalogue,
> borrowers.userflags will be 2**4 and sub permissions will be stored in a
> separate table: user_permissions.
> This is described in C4::Auth::getuserflags.

Thanks a lot, I didn't know 2**x was equivalent of logical shift left (1<<x) and that confused me. Now I understand this! :) I will have another go with reviewing the tests.
Comment 7 Tomás Cohen Arazi 2020-09-01 11:04:29 UTC
We use the sum of powers of two (the exponent being the permission bit) to store all permissions on a single integer column.

My comment could've been better but I say 'borrowers flag is 4, let's put 3', which is fairly correct.

I didn't invent the way to set permissions, and this way of doing it in the tests is everywhere
Comment 8 Tomás Cohen Arazi 2020-09-01 13:25:10 UTC
Created attachment 109428 [details] [review]
Bug 26322: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Tomás Cohen Arazi 2020-09-01 13:25:15 UTC
Created attachment 109429 [details] [review]
Bug 26322: Permissions not checked correctly for plugins

This patch fixes the logic in a condition to address the fact that
permissions are not checked for plugins. This was due to bad parenthesis
pairing and the lack of good tests for this.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/PluginRoutes.t
=> FAIL: Tests fail because of bad logic
3. Apply this patch
4. Repeat (2)
=> SUCCESS: Tests pass!
5. Verify the tests cover the use cases that are needed:
   - Anonymous access
   - Real user with wrong permissions (parameters => 1)
   - Real user with right permissions (borrowers => 1)
=> SUCCESS: Needed use cases so we catch any regression are found
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Jonathan Druart 2020-09-01 14:16:10 UTC
How bad is this?
Comment 11 Tomás Cohen Arazi 2020-09-01 14:16:58 UTC
(In reply to Jonathan Druart from comment #10)
> How bad is this?

Really bad
Comment 12 Jonathan Druart 2020-09-01 14:25:06 UTC
Which known plugins?

Do we need to schedule a dedicated security release?
Comment 13 Tomás Cohen Arazi 2020-09-01 14:42:17 UTC
(In reply to Jonathan Druart from comment #12)
> Which known plugins?
> 
> Do we need to schedule a dedicated security release?

Any plugin that implements a privileged access route is exposed. Most used plugins (like the coverflow one) are anonymous. But there are some others that aren't, and this deserves a dedicated security release.
Comment 14 Tomás Cohen Arazi 2020-09-01 15:40:59 UTC
(In reply to Tomás Cohen Arazi from comment #13)
> (In reply to Jonathan Druart from comment #12)
> > Which known plugins?
> > 
> > Do we need to schedule a dedicated security release?
> 
> Any plugin that implements a privileged access route is exposed. Most used
> plugins (like the coverflow one) are anonymous. But there are some others
> that aren't, and this deserves a dedicated security release.

Here's an example:

https://gitlab.com/thekesolutions/plugins/koha-plugin-innreach/-/blob/master/Koha/Plugin/Com/Theke/INNReach/openapi.json#L74

To my dismay, many plugins I looked for didn't contain permission checking and I already notified the authors.....
Comment 15 Joonas Kylmälä 2020-09-02 11:37:07 UTC
Tomás noticed that the regressions test were missing setting of RESTBasicAuth=1 syspref and thefore failing. The actual fix here however looks good and should be released ASAP, also to the versions of Koha that the patch introducing this bug was backported to.

I have not been able to be in touch with Tomás since yesterday so waiting a bit more whether I should send a follow-up for this or Tomás wants to sent a revised version.
Comment 16 Tomás Cohen Arazi 2020-09-02 12:33:28 UTC
Created attachment 109512 [details] [review]
Bug 26322: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 17 Tomás Cohen Arazi 2020-09-02 12:33:40 UTC
Created attachment 109513 [details] [review]
Bug 26322: Permissions not checked correctly for plugins

This patch fixes the logic in a condition to address the fact that
permissions are not checked for plugins. This was due to bad parenthesis
pairing and the lack of good tests for this.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/PluginRoutes.t
=> FAIL: Tests fail because of bad logic
3. Apply this patch
4. Repeat (2)
=> SUCCESS: Tests pass!
5. Verify the tests cover the use cases that are needed:
   - Anonymous access
   - Real user with wrong permissions (parameters => 1)
   - Real user with right permissions (borrowers => 1)
=> SUCCESS: Needed use cases so we catch any regression are found
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 18 Joonas Kylmälä 2020-09-02 12:40:09 UTC
Created attachment 109517 [details] [review]
Bug 26322: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 19 Joonas Kylmälä 2020-09-02 12:40:13 UTC
Created attachment 109518 [details] [review]
Bug 26322: Permissions not checked correctly for plugins

This patch fixes the logic in a condition to address the fact that
permissions are not checked for plugins. This was due to bad parenthesis
pairing and the lack of good tests for this.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/PluginRoutes.t
=> FAIL: Tests fail because of bad logic
3. Apply this patch
4. Repeat (2)
=> SUCCESS: Tests pass!
5. Verify the tests cover the use cases that are needed:
   - Anonymous access
   - Real user with wrong permissions (parameters => 1)
   - Real user with right permissions (borrowers => 1)
=> SUCCESS: Needed use cases so we catch any regression are found
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 20 Martin Renvoize 2020-09-02 14:08:53 UTC
Created attachment 109539 [details] [review]
Bug 26322: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2020-09-02 14:08:56 UTC
Created attachment 109540 [details] [review]
Bug 26322: Permissions not checked correctly for plugins

This patch fixes the logic in a condition to address the fact that
permissions are not checked for plugins. This was due to bad parenthesis
pairing and the lack of good tests for this.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/PluginRoutes.t
=> FAIL: Tests fail because of bad logic
3. Apply this patch
4. Repeat (2)
=> SUCCESS: Tests pass!
5. Verify the tests cover the use cases that are needed:
   - Anonymous access
   - Real user with wrong permissions (parameters => 1)
   - Real user with right permissions (borrowers => 1)
=> SUCCESS: Needed use cases so we catch any regression are found
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2020-09-02 14:10:34 UTC
Yikes, what a nasty little bug!

All works as expected and the test, once I'd got my head around those darn bit based permissions again, reads OK.

Passing QA
Comment 24 Victor Grousset/tuxayo 2020-09-20 20:23:49 UTC
IIUC 19.05.x dodged this bullet :)
Comment 25 Aleisha Amohia 2020-09-21 22:16:01 UTC
Created attachment 110519 [details] [review]
Bug 26322: [19.11.x] Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Aleisha Amohia 2020-09-21 22:16:07 UTC
Created attachment 110520 [details] [review]
Bug 26322: [19.11.x] Permissions not checked correctly for plugins

This patch fixes the logic in a condition to address the fact that
permissions are not checked for plugins. This was due to bad parenthesis
pairing and the lack of good tests for this.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/PluginRoutes.t
=> FAIL: Tests fail because of bad logic
3. Apply this patch
4. Repeat (2)
=> SUCCESS: Tests pass!
5. Verify the tests cover the use cases that are needed:
   - Anonymous access
   - Real user with wrong permissions (parameters => 1)
   - Real user with right permissions (borrowers => 1)
=> SUCCESS: Needed use cases so we catch any regression are found
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 27 Lucas Gass 2020-09-22 22:34:00 UTC
backported to 20.05.x for 20.05.04
Comment 28 Lucas Gass 2020-09-22 22:34:29 UTC
backported to 19.11.x for 19.11.10
Comment 29 Joonas Kylmälä 2020-09-29 09:16:50 UTC
Reminding that this should be pushed to master as well given it is in the released versions now.
Comment 30 Jonathan Druart 2020-09-29 12:32:10 UTC
Pushed to master for 20.11, thanks to everybody involved!