Bug 28759 - Users with pretty basic staff interface permissions can see/add/remove API keys of any other user
Summary: Users with pretty basic staff interface permissions can see/add/remove API ke...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Peter Vashchuk
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-07-26 11:48 UTC by Peter Vashchuk
Modified: 2022-06-06 20:24 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.04, 20.11.10, 20.05.16, 19.11.22


Attachments
Bug 28759: limit accessibility for "Manage API keys" (3.03 KB, patch)
2021-08-10 15:12 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28759: limit accessibility for "Manage API keys" (3.08 KB, patch)
2021-08-11 11:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28759: limit accessibility for "Manage API keys" (3.15 KB, patch)
2021-09-03 15:21 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Vashchuk 2021-07-26 11:48:00 UTC
Any user that has permissions only to log in into the staff interface (intranet) and add, modify and view patron information, can freely look up any API key of any user and in the same way can add and delete api keys for users even if they did not have any before (even for admin!).

To reproduce the bug:
1) create/pick existing patron, set Staff access, allows viewing of catalogue in staff interface (catalogue)" and "Add, modify and view patron information (borrowers)" permissions on;
2) enable "RESTOAuth2ClientCredentials" in sysprefs;
3) login with that user into staff interface;
4) check any other patron, go to the "More"->"Manage API keys" and see that you can see, add and delete their API keys;
Comment 1 Peter Vashchuk 2021-07-26 11:48:18 UTC Comment hidden (obsolete)
Comment 2 Peter Vashchuk 2021-07-28 08:08:33 UTC
As it has no workaround, the proposed solution is: limit accessibility for "Manage API keys" section only to superlibrarian and the owner of the API key account.

Another open issue is that if user has permissions "Create SQL reports (create_reports)" user can create report that dumps API keys from the db with custom SQL report like "SELECT * FROM `api_keys`" 
(also reports allow to see hashed passwords but it's probably well known thing already)
Even though it leads to the similar problem being caused by a different issue, it has a workaround and it's better to create another ticket for it.
Comment 3 Jonathan Druart 2021-07-28 08:48:57 UTC
(In reply to Peter Vashchuk from comment #2)
> As it has no workaround, the proposed solution is: limit accessibility for
> "Manage API keys" section only to superlibrarian and the owner of the API
> key account.

Yes, agreed! I don't think we need a separate permission.

> Another open issue is that if user has permissions "Create SQL reports
> (create_reports)" user can create report that dumps API keys from the db
> with custom SQL report like "SELECT * FROM `api_keys`" 
> (also reports allow to see hashed passwords but it's probably well known
> thing already)
> Even though it leads to the similar problem being caused by a different
> issue, it has a workaround and it's better to create another ticket for it.

Yes, please a separate bug report for that one.
Comment 4 Jonathan Druart 2021-07-28 08:49:31 UTC
(In reply to Jonathan Druart from comment #3)
> (In reply to Peter Vashchuk from comment #2)
> > Another open issue is that if user has permissions "Create SQL reports
> > (create_reports)" user can create report that dumps API keys from the db
> > with custom SQL report like "SELECT * FROM `api_keys`" 
> > (also reports allow to see hashed passwords but it's probably well known
> > thing already)
> > Even though it leads to the similar problem being caused by a different
> > issue, it has a workaround and it's better to create another ticket for it.
> 
> Yes, please a separate bug report for that one.

Ha, you did already! Bug 28772.
Comment 5 Peter Vashchuk 2021-08-10 15:12:05 UTC
Created attachment 123701 [details] [review]
Bug 28759: limit accessibility for "Manage API keys"

This patch limits the accessibility for "Manage API keys" section only
to superlibrarians and the owner of that said API key account.

The way it does it is by checking if user is superlibrarian or if
logged-in user is the same as a patron id/borrower number is the same
as logged-in user number both in template and apikeys.pl and making sure
the link is inaccessible or redirects to the 403 page if user tries to
go there directly.

To reproduce:
1) create/pick existing patron, set Staff access, allows viewing
of catalogue in staff interface (catalogue)" and "Add, modify and
iew patron information (borrowers)" permissions on;
2) enable "RESTOAuth2ClientCredentials" in sysprefs;
3) login with that user into staff interface;
4) check any other patron, go to the "More"->"Manage API keys" and
check that you can see, add delete their API keys;
5) apply patch;
6) with that same user try to access "Manage API keys" page again.
Ensure that you can't access that page of other patrons but can
access your own page and manage your own API keys.
7) log in with superlibrarian now and ensure that you can access every
"Manage API keys" page of every patron and apply changes there.
Comment 6 Tomás Cohen Arazi 2021-08-11 11:40:30 UTC
Good job
Comment 7 Tomás Cohen Arazi 2021-08-11 11:40:58 UTC
Created attachment 123782 [details] [review]
Bug 28759: limit accessibility for "Manage API keys"

This patch limits the accessibility for "Manage API keys" section only
to superlibrarians and the owner of that said API key account.

The way it does it is by checking if user is superlibrarian or if
logged-in user is the same as a patron id/borrower number is the same
as logged-in user number both in template and apikeys.pl and making sure
the link is inaccessible or redirects to the 403 page if user tries to
go there directly.

To reproduce:
1) create/pick existing patron, set Staff access, allows viewing
of catalogue in staff interface (catalogue)" and "Add, modify and
iew patron information (borrowers)" permissions on;
2) enable "RESTOAuth2ClientCredentials" in sysprefs;
3) login with that user into staff interface;
4) check any other patron, go to the "More"->"Manage API keys" and
check that you can see, add delete their API keys;
5) apply patch;
6) with that same user try to access "Manage API keys" page again.
Ensure that you can't access that page of other patrons but can
access your own page and manage your own API keys.
7) log in with superlibrarian now and ensure that you can access every
"Manage API keys" page of every patron and apply changes there.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Martin Renvoize 2021-09-03 15:21:50 UTC
Created attachment 124494 [details] [review]
Bug 28759: limit accessibility for "Manage API keys"

This patch limits the accessibility for "Manage API keys" section only
to superlibrarians and the owner of that said API key account.

The way it does it is by checking if user is superlibrarian or if
logged-in user is the same as a patron id/borrower number is the same
as logged-in user number both in template and apikeys.pl and making sure
the link is inaccessible or redirects to the 403 page if user tries to
go there directly.

To reproduce:
1) create/pick existing patron, set Staff access, allows viewing
of catalogue in staff interface (catalogue)" and "Add, modify and
iew patron information (borrowers)" permissions on;
2) enable "RESTOAuth2ClientCredentials" in sysprefs;
3) login with that user into staff interface;
4) check any other patron, go to the "More"->"Manage API keys" and
check that you can see, add delete their API keys;
5) apply patch;
6) with that same user try to access "Manage API keys" page again.
Ensure that you can't access that page of other patrons but can
access your own page and manage your own API keys.
7) log in with superlibrarian now and ensure that you can access every
"Manage API keys" page of every patron and apply changes there.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2021-09-03 15:22:12 UTC
Good call, works as expected.. Signing off.
Comment 10 Martin Renvoize 2021-09-03 15:22:37 UTC
I meant PQA.. ;)
Comment 11 Victor Grousset/tuxayo 2021-09-17 01:29:00 UTC
Backported: Pushed to 20.05.x security branch for 20.05.16
Comment 12 Jonathan Druart 2021-09-30 07:23:07 UTC
Backported to 21.05.04, 20.11.10, 20.05.16, 19.11.22
Comment 13 Jonathan Druart 2021-09-30 07:51:43 UTC
Pushed to master for 21.11, thanks to everybody involved!