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
Created attachment 109358 [details] [review] Bug 26322: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 109366 [details] [review] Bug 26322: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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.
(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.
(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.
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
Created attachment 109428 [details] [review] Bug 26322: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
How bad is this?
(In reply to Jonathan Druart from comment #10) > How bad is this? Really bad
Which known plugins? Do we need to schedule a dedicated security release?
(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.
(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.....
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.
Created attachment 109512 [details] [review] Bug 26322: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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>
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>
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>
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>
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
I found those 3 plugins: https://gitlab.com/thekesolutions/plugins/koha-plugin-ort-tools/-/raw/master/Koha/Plugin/Com/Theke/ORT/openapi.json https://gitlab.com/thekesolutions/plugins/koha-plugin-bibliocommons/-/raw/master/Koha/Plugin/Com/Theke/BiblioCommons/openapi.json https://gitlab.com/thekesolutions/plugins/koha-plugin-innreach/-/raw/master/Koha/Plugin/Com/Theke/INNReach/openapi.json
IIUC 19.05.x dodged this bullet :)
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>
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>
backported to 20.05.x for 20.05.04
backported to 19.11.x for 19.11.10
Reminding that this should be pushed to master as well given it is in the released versions now.
Pushed to master for 20.11, thanks to everybody involved!