Description
Tomás Cohen Arazi (tcohen)
2018-04-19 18:15:52 UTC
Holistic review found here: https://github.com/PTFS-Europe/koha/pull/112#pullrequestreview-113933459 Created attachment 74986 [details] [review] Bug 20624: Add RESTOAuth2ClientCredentials syspref Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 74987 [details] [review] Bug 20624: Unit tests This patch adds tests to verify that disabling the RESTOAuth2ClientCredentials syspref makes any request on the /api/v1/oauth/token using the 'client_credentials' grant fail with 'grant not implemented'. To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/api/v1/oauth.t => FAIL: Tests fail because the change is not implemented! Created attachment 74988 [details] [review] Bug 20624: Make /api/v1/oauth/token respect RESTOAuth2ClientCredentials This patch makes the /api/v1/oauth/token enpoint respect the RESTOAuth2ClientCredentials syspref. It will return 400 (with 'Unimplemented grant type' error message) on the event of the syspref being disabled and the grant_type => 'client_credentials' value passed. To test: - Run: $ kshell k$ prove t/db_dependent/api/v1/oauth.t => FAIL: It fails because the off-switch is not implemented - Apply this patch - Run: k$ prove t/db_dependent/api/v1/oauth.t => SUCCESS: Tests pass! - Sign off :-D Created attachment 74989 [details] [review] Bug 20624: Make staff client respect RESTOAuth2ClientCredentials This patch makes the staff client UI respect the RESTOAuth2ClientCredentials syspref. To test: - Make sure RESTOAuth2ClientCredentials is "Don't enable" - Go to a patron's detail page => SUCCESS: The 'More' dropdown doesn't show the API keys management link. - Enable RESTOAuth2ClientCredentials - Reload => SUCCESS: The 'More' dropdown shows the API keys management link - Click on the API keys management link => SUCCESS: You can edit the api keys - Disable the syspref - Reload => SUCCESS: You are presented an error 400 page. - Sign off :-D Applied 20402, then bug 20568, but the latest patch there doesn't apply: Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 20568: API keys management in interface Applying: Bug 20568: Add mandatory description field for api keys Applying: Bug 20568: Unit tests Applying: Bug 20568: API key management for OPAC users error: sha1 information is lacking or useless (koha-tmpl/opac-tmpl/bootstrap/en/includes/usermenu.inc). error: could not build fake ancestor Patch failed at 0001 Bug 20568: API key management for OPAC users The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-20568-API-key-management-for-OPAC-users-Igx9Ny.patch Sorry, stuck :( Created attachment 75020 [details] [review] Bug 20624: (QA follow-up) Unit tests for missing deps situation This patch tests the situation in which Net::OAuth2::AuthorizationServer is missing. It mocks Module::Load::Conditional::can_load and expects the /token endpoint answers 'Unimplemented grant type' to all requests, and the 'authenticate_api_request' in 'under' exit with unauthorized (403) to requests in which the Authorization header is passed containing a Bearer token, but OAuth2 is not really available. To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/api/v1/oauth.t => FAIL: Tests fail because our REST endpoints don't support this behaviour. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 75021 [details] [review] Bug 20624: (QA follow-up) Handle missing deps gracefuly This patch makes the /token endpoint and the authenticate_api_request method behave correctly in the event of missing deps for OAuth2. To test: - Run: $ kshell k$ prove t/db_dependent/api/v1/oauth.t => FAIL: The behaviour is not implemented - Apply this patch - Run: k$ prove t/db_dependent/api/v1/oauth.t => SUCCESS: Tests pass! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> (In reply to Katrin Fischer from comment #6) > Applied 20402, then bug 20568, but the latest patch there doesn't apply: > Apply? [(y)es, (n)o, (i)nteractive] y > Applying: Bug 20568: API keys management in interface > Applying: Bug 20568: Add mandatory description field for api keys > Applying: Bug 20568: Unit tests > Applying: Bug 20568: API key management for OPAC users > error: sha1 information is lacking or useless > (koha-tmpl/opac-tmpl/bootstrap/en/includes/usermenu.inc). > error: could not build fake ancestor > Patch failed at 0001 Bug 20568: API key management for OPAC users > The copy of the patch that failed is found in: .git/rebase-apply/patch > When you have resolved this problem run "git bz apply --continue". > If you would prefer to skip this patch, instead run "git bz apply --skip". > To restore the original branch and stop patching run "git bz apply --abort". > Patch left in /tmp/Bug-20568-API-key-management-for-OPAC-users-Igx9Ny.patch > > Sorry, stuck :( Dependent bugs rebased! ... trying to figure out the right sequence that will make git bz happy. 20402 20568 20612 20624 ... always tell git bz to ignore dependencies as it doesn't notice what's already applied. Treating this as a false positive as it's not added by the patch set: FAIL Koha/REST/V1/Auth.pm OK critic OK forbidden patterns OK git manipulation OK pod FAIL pod coverage POD is missing for 'validate_query_parameters' The link is gone, but the page remains accessible directly: http://localhost:8081/cgi-bin/koha/members/apikeys.pl?patron_id=1 Tests fail for me: t/db_dependent/api/v1/oauth.t .. 1/2 # Failed test '/oauth/token tests' # at t/db_dependent/api/v1/oauth.t line 116. Can't call method "expires" on an undefined value at t/db_dependent/api/v1/oauth.t line 93. # Looks like your test exited with 255 just after 1. t/db_dependent/api/v1/oauth.t .. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 2/2 subtests Test Summary Report ------------------- t/db_dependent/api/v1/oauth.t (Wstat: 65280 Tests: 1 Failed: 1) Failed test: 1 Non-zero exit status: 255 Parse errors: Bad plan. You planned 2 tests but ran 1. Created attachment 75060 [details] [review] Bug 20624: Add RESTOAuth2ClientCredentials syspref Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 75061 [details] [review] Bug 20624: Unit tests This patch adds tests to verify that disabling the RESTOAuth2ClientCredentials syspref makes any request on the /api/v1/oauth/token using the 'client_credentials' grant fail with 'grant not implemented'. To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/api/v1/oauth.t => FAIL: Tests fail because the change is not implemented! Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 75062 [details] [review] Bug 20624: Make /api/v1/oauth/token respect RESTOAuth2ClientCredentials This patch makes the /api/v1/oauth/token enpoint respect the RESTOAuth2ClientCredentials syspref. It will return 400 (with 'Unimplemented grant type' error message) on the event of the syspref being disabled and the grant_type => 'client_credentials' value passed. To test: - Run: $ kshell k$ prove t/db_dependent/api/v1/oauth.t => FAIL: It fails because the off-switch is not implemented - Apply this patch - Run: k$ prove t/db_dependent/api/v1/oauth.t => SUCCESS: Tests pass! - Sign off :-D Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 75063 [details] [review] Bug 20624: Make staff client respect RESTOAuth2ClientCredentials This patch makes the staff client UI respect the RESTOAuth2ClientCredentials syspref. To test: - Make sure RESTOAuth2ClientCredentials is "Don't enable" - Go to a patron's detail page => SUCCESS: The 'More' dropdown doesn't show the API keys management link. - Enable RESTOAuth2ClientCredentials - Reload => SUCCESS: The 'More' dropdown shows the API keys management link - Click on the API keys management link => SUCCESS: You can edit the api keys - Disable the syspref - Reload => SUCCESS: You are presented an error 400 page. - Sign off :-D Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 75064 [details] [review] Bug 20624: (QA follow-up) Unit tests for missing deps situation This patch tests the situation in which Net::OAuth2::AuthorizationServer is missing. It mocks Module::Load::Conditional::can_load and expects the /token endpoint answers 'Unimplemented grant type' to all requests, and the 'authenticate_api_request' in 'under' exit with unauthorized (403) to requests in which the Authorization header is passed containing a Bearer token, but OAuth2 is not really available. To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/api/v1/oauth.t => FAIL: Tests fail because our REST endpoints don't support this behaviour. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 75065 [details] [review] Bug 20624: (QA follow-up) Handle missing deps gracefuly This patch makes the /token endpoint and the authenticate_api_request method behave correctly in the event of missing deps for OAuth2. To test: - Run: $ kshell k$ prove t/db_dependent/api/v1/oauth.t => FAIL: The behaviour is not implemented - Apply this patch - Run: k$ prove t/db_dependent/api/v1/oauth.t => SUCCESS: Tests pass! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 75132 [details] [review] Bug 20624: (QA follow-up) Add miising POD in Koha::REST::V1::OAuth Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 75184 [details] [review] Bug 20624: Net::OAuth2::AuthorizationServer is not a hard dependency While we get packaging sorted, Net::OAuth2::AuthorizationServer is not a hard dependency for Koha and the feature requiring it is disabled by default. This patch: - Makes the dependency optional - Makes the unit tests for the OAuth2 client credentials flow skip if the dependency is not met. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> I was expecting to see rejected any requests made with a valid token when the pref is off. Example: Turn the pref on Generate a token Request something (or not) Turn the pref on Request something => 200 Should not we return 401 or whatever instead? Created attachment 75206 [details] [review] Bug 20624: Add an entry in About when deps are not present Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Pushed to master for 18.05, thanks to everybody involved! |