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;
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's better to create another bug for it.
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.
(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.
(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.
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.
Good job
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>
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>
Good call, works as expected.. Signing off.
I meant PQA.. ;)
Backported: Pushed to 20.05.x security branch for 20.05.16
Backported to 21.05.04, 20.11.10, 20.05.16, 19.11.22
Pushed to master for 21.11, thanks to everybody involved!