|
Description
Tomás Cohen Arazi (tcohen)
2019-01-04 14:46:41 UTC
Created attachment 83638 [details] [review] Bug 22071: Regression tests The authenticate_api_request() behaviour needs more tests. This patch adds tests for the stashed Koha::Patron object. It highlights the bug in the OAuth authentication case. To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t => FAIL: Tests fail in the OAuth case, pass in the cookie-based auth case. Created attachment 83639 [details] [review] Bug 22071: Make authenticate_api_request stash koha.user in OAuth use case This patch makes the OAuth code stash the requestion Koha::Patron. To test: - Apply the tests patch: - Run: $ kshell k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t => FAIL: Tests fail :-( - Apply this patch - Run: k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t => SUCCESS: Tests pass :-D - Sign off =D Created attachment 83641 [details] [review] Bug 22071: Regression tests The authenticate_api_request() behaviour needs more tests. This patch adds tests for the stashed Koha::Patron object. It highlights the bug in the OAuth authentication case. To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t => FAIL: Tests fail in the OAuth case, pass in the cookie-based auth case. Created attachment 83642 [details] [review] Bug 22071: Make authenticate_api_request stash koha.user in OAuth use case This patch makes the OAuth code stash the requestion Koha::Patron. To test: - Apply the tests patch: - Run: $ kshell k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t => FAIL: Tests fail :-( - Apply this patch - Run: k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t => SUCCESS: Tests pass :-D - Sign off =D I was able to get the same behavior as described in the test plan: "not ok 1 - token-based tests" but "ok 2 - cookie-based tests" The following patch made all tests successful. Had to download Net::OAuth2::AuthorizationServer and activate the syspref RESTOAuth2ClientCredentials, if anybody needs additional instructions. Created attachment 83651 [details] [review] Bug 22071: Regression tests The authenticate_api_request() behaviour needs more tests. This patch adds tests for the stashed Koha::Patron object. It highlights the bug in the OAuth authentication case. To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t => FAIL: Tests fail in the OAuth case, pass in the cookie-based auth case. Signed-off-by: Charles Farmer <charles.farmer@inLibro.com> Created attachment 83652 [details] [review] Bug 22071: Make authenticate_api_request stash koha.user in OAuth use case This patch makes the OAuth code stash the requestion Koha::Patron. To test: - Apply the tests patch: - Run: $ kshell k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t => FAIL: Tests fail :-( - Apply this patch - Run: k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t => SUCCESS: Tests pass :-D - Sign off =D Signed-off-by: Charles Farmer <charles.farmer@inLibro.com> Created attachment 83676 [details] [review] Bug 22071: (follow-up) Simplify code In order to add features to this method, the current code would force us to do it for each authentication method. There's duplicated code that could be simplified. This patch makes the authentication code just set $user on each block (oauth and cookie authentication) and moves the final permissions check to the end of the authenticate_api_request method. Overall, the behaviour remains unchanged. To test: - Run: $ kshell k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t \ t/db_dependent/api/v1/oauth.t => SUCCESS: Tests pass! Nothing changed! - Sign off :-D Created attachment 83677 [details] [review] Bug 22071: (follow-up) Add POD for validate_query_parameters Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 83686 [details] [review] Bug 22071: Regression tests The authenticate_api_request() behaviour needs more tests. This patch adds tests for the stashed Koha::Patron object. It highlights the bug in the OAuth authentication case. To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t => FAIL: Tests fail in the OAuth case, pass in the cookie-based auth case. Signed-off-by: Charles Farmer <charles.farmer@inLibro.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 83687 [details] [review] Bug 22071: Make authenticate_api_request stash koha.user in OAuth use case This patch makes the OAuth code stash the requestion Koha::Patron. To test: - Apply the tests patch: - Run: $ kshell k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t => FAIL: Tests fail :-( - Apply this patch - Run: k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t => SUCCESS: Tests pass :-D - Sign off =D Signed-off-by: Charles Farmer <charles.farmer@inLibro.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 83688 [details] [review] Bug 22071: (follow-up) Simplify code In order to add features to this method, the current code would force us to do it for each authentication method. There's duplicated code that could be simplified. This patch makes the authentication code just set $user on each block (oauth and cookie authentication) and moves the final permissions check to the end of the authenticate_api_request method. Overall, the behaviour remains unchanged. To test: - Run: $ kshell k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t \ t/db_dependent/api/v1/oauth.t => SUCCESS: Tests pass! Nothing changed! - Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 83689 [details] [review] Bug 22071: (follow-up) Add POD for validate_query_parameters Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> A solid improvement to the existing code and good additional of tests. No regressions found and no qa failures found. Passing QA Awesome work all! Pushed to master for 19.05 Pushed to 18.11.x for 18.11.02 Pushed to 18.05.x for 18.05.08 Depends on Bug 20568 not in 17.11.x |