Description
Julian Maurice
2015-09-22 09:02:48 UTC
Created attachment 42761 [details] [review] Bug 14868: Swagger2-driven Permission checking A hasty downgrade from 13920, utilizing new features implemented by the Mojolicious::Plugin::Swagger2-author, to make it possible to implement more complex authentication/authorization scenarios with the Plugin. Define 'x-koha-permission' for the Swagger2 Operation Object, to automatically authorize against the required permissions. This way we immediately tell the API consumer in the Swagger2-definition, which permissions are needed to access defined resources. Also we don't need to maintain permissions in multiple locations and we can build a smart testing framework to help a lot in creating tests for the new REST API. Created attachment 52577 [details] [review] Bug 14868: Swagger2-driven Permission checking A hasty downgrade from 13920, utilizing new features implemented by the Mojolicious::Plugin::Swagger2-author, to make it possible to implement more complex authentication/authorization scenarios with the Plugin. Define 'x-koha-permission' for the Swagger2 Operation Object, to automatically authorize against the required permissions. This way we immediately tell the API consumer in the Swagger2-definition, which permissions are needed to access defined resources. Also we don't need to maintain permissions in multiple locations and we can build a smart testing framework to help a lot in creating tests for the new REST API. Rebase notes from 2015-09-22 to 2016-06-20: - In patrons.t, change expected HTTP 403 to 401 when accessing the endpoint without authentication (401 = not authenticated, 403 = authenticated, but no permission). Created attachment 52578 [details] [review] Bug 14868: Give users possibility to request their own object If the user has no required permissions, but attempts to access his own object, allow this request in case "x-koha-permission" has defined "allow-owner": "1". As an example, the following resource can be accessed if user has borrowers-flag or if he is making the request to his own borrowernumber (in path or body): "/patrons/{borrowernumber}": { .. "x-koha-permission": { "allow-owner": "1", "permissions": { "borrowers": "1" } } } Created attachment 52579 [details] [review] Bug 14868: Use x-koha-permission in current routes Adds missing permission checking to holds. Adds 6 tests to patrons.t, adds 30 tests to holds.t. Created attachment 52592 [details] [review] Bug 14868: Swagger2-driven Permission checking A hasty downgrade from 13920, utilizing new features implemented by the Mojolicious::Plugin::Swagger2-author, to make it possible to implement more complex authentication/authorization scenarios with the Plugin. Define 'x-koha-permission' for the Swagger2 Operation Object, to automatically authorize against the required permissions. This way we immediately tell the API consumer in the Swagger2-definition, which permissions are needed to access defined resources. Also we don't need to maintain permissions in multiple locations and we can build a smart testing framework to help a lot in creating tests for the new REST API. Rebase notes from 2015-09-22 to 2016-06-20: - In patrons.t, change expected HTTP 403 to 401 when accessing the endpoint without authentication (401 = not authenticated, 403 = authenticated, but no permission). Created attachment 52593 [details] [review] Bug 14868: Give users possibility to request their own object If the user has no required permissions, but attempts to access his own object, allow this request in case "x-koha-permission" has defined "allow-owner": "1". As an example, the following resource can be accessed if user has borrowers-flag or if he is making the request to his own borrowernumber (in path or body): "/patrons/{borrowernumber}": { .. "x-koha-permission": { "allow-owner": "1", "permissions": { "borrowers": "1" } } } Created attachment 52594 [details] [review] Bug 14868: Use x-koha-permission in current routes Adds missing permission checking to holds. Adds 6 tests to patrons.t, adds 30 tests to holds.t. Created attachment 52602 [details] [review] Bug 14868: Swagger2-driven Permission checking A hasty downgrade from 13920, utilizing new features implemented by the Mojolicious::Plugin::Swagger2-author, to make it possible to implement more complex authentication/authorization scenarios with the Plugin. Define 'x-koha-permission' for the Swagger2 Operation Object, to automatically authorize against the required permissions. This way we immediately tell the API consumer in the Swagger2-definition, which permissions are needed to access defined resources. Also we don't need to maintain permissions in multiple locations and we can build a smart testing framework to help a lot in creating tests for the new REST API. Rebase notes from 2015-09-22 to 2016-06-20: - In patrons.t, change expected HTTP 403 to 401 when accessing the endpoint without authentication (401 = not authenticated, 403 = authenticated, but no permission). Created attachment 52603 [details] [review] Bug 14868: Give users possibility to request their own object If the user has no required permissions, but attempts to access his own object, allow this request in case "x-koha-permission" has defined "allow-owner": "1". As an example, the following resource can be accessed if user has borrowers-flag or if he is making the request to his own borrowernumber (in path or body): "/patrons/{borrowernumber}": { .. "x-koha-permission": { "allow-owner": "1", "permissions": { "borrowers": "1" } } } Created attachment 52604 [details] [review] Bug 14868: Use x-koha-permission in current routes Adds missing permission checking to holds. Adds 6 tests to patrons.t, adds 30 tests to holds.t. Created attachment 52963 [details] [review] Bug 14868: Rename 'x-koha-permission' to 'x-koha-authorization' Since we need to be able to define other authorization options than simply Koha permissions, for example "allow-owner", we should rename x-koha-permission into x-koha-authorization to better describe the object. This patch simply renames all occurences of x-koha-permission into x-koha-authorization. The new specification for authorization: "/patrons/{borrowernumber}": { .. "x-koha-authorization": { "allow-owner": "1", "pemissions": { "borrowers": "1" } } } Created attachment 52964 [details] [review] Bug 14868: Swagger2-driven Permission checking A hasty downgrade from 13920, utilizing new features implemented by the Mojolicious::Plugin::Swagger2-author, to make it possible to implement more complex authentication/authorization scenarios with the Plugin. Define 'x-koha-permission' for the Swagger2 Operation Object, to automatically authorize against the required permissions. This way we immediately tell the API consumer in the Swagger2-definition, which permissions are needed to access defined resources. Also we don't need to maintain permissions in multiple locations and we can build a smart testing framework to help a lot in creating tests for the new REST API. Rebase notes from 2015-09-22 to 2016-06-20: - In patrons.t, change expected HTTP 403 to 401 when accessing the endpoint without authentication (401 = not authenticated, 403 = authenticated, but no permission). Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> My name is Olli-Antti Kivilahti and I approve this commit. We have been using the Swagger2.0-driven REST API on Mojolicious for 1 year now in production and I am certain we have a pretty good idea on how to work with the limitations of Swagger2.0 We participated in the development of the Mojolicious::Plugin::Swagger and know it well. We have made an extension to the plugin to provide full CORS support and have been building all our in-house features on the new REST API. Created attachment 52965 [details] [review] Bug 14868: Give users possibility to request their own object If the user has no required permissions, but attempts to access his own object, allow this request in case "x-koha-permission" has defined "allow-owner": "1". As an example, the following resource can be accessed if user has borrowers-flag or if he is making the request to his own borrowernumber (in path or body): "/patrons/{borrowernumber}": { .. "x-koha-permission": { "allow-owner": "1", "permissions": { "borrowers": "1" } } } Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> My name is Olli-Antti Kivilahti and I approve this commit. We have been using the Swagger2.0-driven REST API on Mojolicious for 1 year now in production and I am certain we have a pretty good idea on how to work with the limitations of Swagger2.0 We participated in the development of the Mojolicious::Plugin::Swagger and know it well. We have made an extension to the plugin to provide full CORS support and have been building all our in-house features on the new REST API. Created attachment 52966 [details] [review] Bug 14868: Use x-koha-permission in current routes Adds missing permission checking to holds. Adds 6 tests to patrons.t, adds 30 tests to holds.t. Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> My name is Olli-Antti Kivilahti and I approve this commit. We have been using the Swagger2.0-driven REST API on Mojolicious for 1 year now in production and I am certain we have a pretty good idea on how to work with the limitations of Swagger2.0 We participated in the development of the Mojolicious::Plugin::Swagger and know it well. We have made an extension to the plugin to provide full CORS support and have been building all our in-house features on the new REST API. Created attachment 52967 [details] [review] Bug 14868: Rename 'x-koha-permission' to 'x-koha-authorization' Since we need to be able to define other authorization options than simply Koha permissions, for example "allow-owner", we should rename x-koha-permission into x-koha-authorization to better describe the object. This patch simply renames all occurences of x-koha-permission into x-koha-authorization. The new specification for authorization: "/patrons/{borrowernumber}": { .. "x-koha-authorization": { "allow-owner": "1", "pemissions": { "borrowers": "1" } } } Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> My name is Olli-Antti Kivilahti and I approve this commit. We have been using the Swagger2.0-driven REST API on Mojolicious for 1 year now in production and I am certain we have a pretty good idea on how to work with the limitations of Swagger2.0 We participated in the development of the Mojolicious::Plugin::Swagger and know it well. We have made an extension to the plugin to provide full CORS support and have been building all our in-house features on the new REST API. Created attachment 52981 [details] [review] [SIGNED-OFF] Bug 14868: Swagger2-driven Permission checking A hasty downgrade from 13920, utilizing new features implemented by the Mojolicious::Plugin::Swagger2-author, to make it possible to implement more complex authentication/authorization scenarios with the Plugin. Define 'x-koha-permission' for the Swagger2 Operation Object, to automatically authorize against the required permissions. This way we immediately tell the API consumer in the Swagger2-definition, which permissions are needed to access defined resources. Also we don't need to maintain permissions in multiple locations and we can build a smart testing framework to help a lot in creating tests for the new REST API. Rebase notes from 2015-09-22 to 2016-06-20: - In patrons.t, change expected HTTP 403 to 401 when accessing the endpoint without authentication (401 = not authenticated, 403 = authenticated, but no permission). Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> My name is Olli-Antti Kivilahti and I approve this commit. We have been using the Swagger2.0-driven REST API on Mojolicious for 1 year now in production and I am certain we have a pretty good idea on how to work with the limitations of Swagger2.0 We participated in the development of the Mojolicious::Plugin::Swagger and know it well. We have made an extension to the plugin to provide full CORS support and have been building all our in-house features on the new REST API. Signed-off-by: Johanna Raisa <johanna.raisa@gmail.com> My name is Johanna Räisä and I approve this commit. We have been using Swagger2.0-driven REST API in production successfully. Created attachment 52992 [details] [review] [SIGNED-OFF] Bug 14868: Swagger2-driven Permission checking A hasty downgrade from 13920, utilizing new features implemented by the Mojolicious::Plugin::Swagger2-author, to make it possible to implement more complex authentication/authorization scenarios with the Plugin. Define 'x-koha-permission' for the Swagger2 Operation Object, to automatically authorize against the required permissions. This way we immediately tell the API consumer in the Swagger2-definition, which permissions are needed to access defined resources. Also we don't need to maintain permissions in multiple locations and we can build a smart testing framework to help a lot in creating tests for the new REST API. Rebase notes from 2015-09-22 to 2016-06-20: - In patrons.t, change expected HTTP 403 to 401 when accessing the endpoint without authentication (401 = not authenticated, 403 = authenticated, but no permission). Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> My name is Olli-Antti Kivilahti and I approve this commit. We have been using the Swagger2.0-driven REST API on Mojolicious for 1 year now in production and I am certain we have a pretty good idea on how to work with the limitations of Swagger2.0 We participated in the development of the Mojolicious::Plugin::Swagger and know it well. We have made an extension to the plugin to provide full CORS support and have been building all our in-house features on the new REST API. Signed-off-by: Johanna Raisa <johanna.raisa@gmail.com> My name is Johanna Räisä and I approve this commit. We have been using Swagger2.0-driven REST API in production successfully. Created attachment 52993 [details] [review] [SIGNED-OFF] Bug 14868: Give users possibility to request their own object If the user has no required permissions, but attempts to access his own object, allow this request in case "x-koha-permission" has defined "allow-owner": "1". As an example, the following resource can be accessed if user has borrowers-flag or if he is making the request to his own borrowernumber (in path or body): "/patrons/{borrowernumber}": { .. "x-koha-permission": { "allow-owner": "1", "permissions": { "borrowers": "1" } } } Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> My name is Olli-Antti Kivilahti and I approve this commit. We have been using the Swagger2.0-driven REST API on Mojolicious for 1 year now in production and I am certain we have a pretty good idea on how to work with the limitations of Swagger2.0 We participated in the development of the Mojolicious::Plugin::Swagger and know it well. We have made an extension to the plugin to provide full CORS support and have been building all our in-house features on the new REST API. Signed-off-by: Johanna Raisa <johanna.raisa@gmail.com> My name is Johanna Räisä and I approve this commit. We have been using Swagger2.0-driven REST API in production successfully. Created attachment 52994 [details] [review] [SIGNED-OFF] Bug 14868: Use x-koha-permission in current routes Adds missing permission checking to holds. Adds 6 tests to patrons.t, adds 30 tests to holds.t. Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> My name is Olli-Antti Kivilahti and I approve this commit. We have been using the Swagger2.0-driven REST API on Mojolicious for 1 year now in production and I am certain we have a pretty good idea on how to work with the limitations of Swagger2.0 We participated in the development of the Mojolicious::Plugin::Swagger and know it well. We have made an extension to the plugin to provide full CORS support and have been building all our in-house features on the new REST API. Signed-off-by: Johanna Raisa <johanna.raisa@gmail.com> My name is Johanna Räisä and I approve this commit. We have been using Swagger2.0-driven REST API in production successfully. Created attachment 52995 [details] [review] [SIGNED-OFF] Bug 14868: Rename 'x-koha-permission' to 'x-koha-authorization' Since we need to be able to define other authorization options than simply Koha permissions, for example "allow-owner", we should rename x-koha-permission into x-koha-authorization to better describe the object. This patch simply renames all occurences of x-koha-permission into x-koha-authorization. The new specification for authorization: "/patrons/{borrowernumber}": { .. "x-koha-authorization": { "allow-owner": "1", "pemissions": { "borrowers": "1" } } } Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> My name is Olli-Antti Kivilahti and I approve this commit. We have been using the Swagger2.0-driven REST API on Mojolicious for 1 year now in production and I am certain we have a pretty good idea on how to work with the limitations of Swagger2.0 We participated in the development of the Mojolicious::Plugin::Swagger and know it well. We have made an extension to the plugin to provide full CORS support and have been building all our in-house features on the new REST API. Signed-off-by: Johanna Raisa <johanna.raisa@gmail.com> My name is Johanna Räisä and I approve this commit. We have been using Swagger2.0-driven REST API in production successfully. Created attachment 52996 [details] [review] [SIGNED-OFF] Bug 14868: Rename 'x-koha-permission' to 'x-koha-authorization' Since we need to be able to define other authorization options than simply Koha permissions, for example "allow-owner", we should rename x-koha-permission into x-koha-authorization to better describe the object. This patch simply renames all occurences of x-koha-permission into x-koha-authorization. The new specification for authorization: "/patrons/{borrowernumber}": { .. "x-koha-authorization": { "allow-owner": "1", "pemissions": { "borrowers": "1" } } } Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> My name is Olli-Antti Kivilahti and I approve this commit. We have been using the Swagger2.0-driven REST API on Mojolicious for 1 year now in production and I am certain we have a pretty good idea on how to work with the limitations of Swagger2.0 We participated in the development of the Mojolicious::Plugin::Swagger and know it well. We have made an extension to the plugin to provide full CORS support and have been building all our in-house features on the new REST API. Signed-off-by: Johanna Raisa <johanna.raisa@gmail.com> My name is Johanna Räisä and I approve this commit. We have been using Swagger2.0-driven REST API in production successfully. This a very good idea! If you reconsider on the automatic minifying of swagger made in bug 16212 I would be happy to sign off! This one needs a bit more work. For example "allow-owner" is now only checking against "borrowernumber", but we also need to be able to figure out ownership e.g. from a checkout_id. Also, these patches need to be rebased on top of master because there are already changes in API. Created attachment 54457 [details] [review] Bug 14868: Swagger2-driven Permission checking Define 'x-koha-permission' for the Swagger2 Operation Object, to automatically authorize against the required permissions. This way we immediately tell the API consumer in the Swagger2-definition, which permissions are needed to access defined resources. Also we don't need to maintain permissions in multiple locations and we can build a smart testing framework to help a lot in creating tests for the new REST API. Created attachment 54458 [details] [review] Bug 14868: Give users possibility to request their own object Allow access to user's own objects even if they do not have required permissions. This will be very useful in many cases where an user wants to request their own object, for example renewing their checkouts or placing a hold for themselves. First, this patch renames "x-koha-permission" to "x-koha-authorization" in order to describe the new functionality better. Second, we can now add two extra parameters under "x-koha-authorization": - "allow-owner"; Allows the owner of object to access it (without permission) - "allow-guarantor"; Allows guarantor of the owner of object to access it (without permission) Third, since permission checking is outside of actual controller, we need a way to find out ownership from different types of parameters, e.g. checkout_id from /checkouts/{checkout_id}, borrowernumber from /patrons/{borrowernumber} etc. A solution is to match the parameter with a subroutine that is designed to verify the ownership for that object. See the new subroutines in Koha::REST::V1. To use this functionality you will simply define it in Swagger: "/patrons/{borrowernumber}": { "get": { ..., "x-koha-authorization": { "allow-owner": true, "permissions": { "borrowers": "1" } } } } If a parameter that is not yet defined in Koha::REST::V1::check_object_ownership, you also need to define it and implement a subroutine that determines ownership. Tests are provided in a following patch that adds this functionality for current API operations. Created attachment 54459 [details] [review] Bug 14868: Use x-koha-authorization in current routes To test: 1. Run t/db_dependent/api/v1/holds.t 2. Run t/db_dependent/api/v1/patrons.t The patches went through heavy rebasing (now on top of master) and modifications. These patches fix wrong error messages, extend "allow-owner" functionality to be able to find out object owner from any parameter (not just borrowernumber like before), adds "allow-guarantor" that works similar to "allow-owner" but checks access for guarantor of the owner of the object instead, and adds some more tests that cover these changes. Meanwhile I also squashed "Rename 'x-koha-permission' to 'x-koha-authorization'" into "Give users possibility to request their own object". Because of heavy changes, I had to remove the sign-offs and would now be very interested to hear comments especially on the solution to find out object ownership from any parameter, and of course suggestions for a better solution if someone can come up with one. I think it's very useful to be able to define permissions in Swagger because it is now documented in your specification, and also permission checking is centralized into one place instead of duplicating it for nearly every operation in each controller. Created attachment 54631 [details] [review] Bug 14868: Swagger2-driven Permission checking Created attachment 54632 [details] [review] Bug 14868: Give users possibility to request their own object Created attachment 54633 [details] [review] Bug 14868: Use x-koha-authorization in current routes All I have to say, is that this patches are really awesome :-D Please rebase them on top of bug 17050, and port the fix so RestrictSessionByIP isn't broken by this patchset. Awesome job! Created attachment 54765 [details] [review] Bug 14868: Swagger2-driven Permission checking Define 'x-koha-permission' for the Swagger2 Operation Object, to automatically authorize against the required permissions. This way we immediately tell the API consumer in the Swagger2-definition, which permissions are needed to access defined resources. Also we don't need to maintain permissions in multiple locations and we can build a smart testing framework to help a lot in creating tests for the new REST API. Created attachment 54766 [details] [review] Bug 14868: Give users possibility to request their own object Allow access to user's own objects even if they do not have required permissions. This will be very useful in many cases where an user wants to request their own object, for example renewing their checkouts or placing a hold for themselves. First, this patch renames "x-koha-permission" to "x-koha-authorization" in order to describe the new functionality better. Second, we can now add two extra parameters under "x-koha-authorization": - "allow-owner"; Allows the owner of object to access it (without permission) - "allow-guarantor"; Allows guarantor of the owner of object to access it (without permission) Third, since permission checking is outside of actual controller, we need a way to find out ownership from different types of parameters, e.g. checkout_id from /checkouts/{checkout_id}, borrowernumber from /patrons/{borrowernumber} etc. A solution is to match the parameter with a subroutine that is designed to verify the ownership for that object. See the new subroutines in Koha::REST::V1. To use this functionality you will simply define it in Swagger: "/patrons/{borrowernumber}": { "get": { ..., "x-koha-authorization": { "allow-owner": true, "permissions": { "borrowers": "1" } } } } If a parameter that is not yet defined in Koha::REST::V1::check_object_ownership, you also need to define it and implement a subroutine that determines ownership. Tests are provided in a following patch that adds this functionality for current API operations. Created attachment 54767 [details] [review] Bug 14868: Use x-koha-authorization in current routes To test: 1. Run t/db_dependent/api/v1/holds.t 2. Run t/db_dependent/api/v1/patrons.t Created attachment 54770 [details] [review] Bug 14868: Display required permissions in permission error response When user does not have required permissions to use API operation, it would be useful to let him know which permissions he is missing. Since they are now defined in Swagger, we can easily render them into the response. To test: 1. Use a patron without any permissions 2. Make GET request to http://yourlib/api/v1/patrons 3. Observe permission error and see that required_permissions are displayed. 4. Run t/db_dependent/api/v1/patrons.t Created attachment 54771 [details] [review] Bug 14868: Display required permissions in permission error response When user does not have required permissions to use API operation, it would be useful to let them know which permissions he is missing. Since they are now defined in Swagger, we can easily render them into the response. To test: 1. Use a patron without any permissions 2. Make GET request to http://yourlib/api/v1/patrons 3. Observe permission error and see that required_permissions are displayed. 4. Run t/db_dependent/api/v1/patrons.t Created attachment 54894 [details] [review] Bug 14868: Swagger2-driven Permission checking Define 'x-koha-permission' for the Swagger2 Operation Object, to automatically authorize against the required permissions. This way we immediately tell the API consumer in the Swagger2-definition, which permissions are needed to access defined resources. Also we don't need to maintain permissions in multiple locations and we can build a smart testing framework to help a lot in creating tests for the new REST API. Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> Created attachment 54895 [details] [review] Bug 14868: Give users possibility to request their own object Allow access to user's own objects even if they do not have required permissions. This will be very useful in many cases where an user wants to request their own object, for example renewing their checkouts or placing a hold for themselves. First, this patch renames "x-koha-permission" to "x-koha-authorization" in order to describe the new functionality better. Second, we can now add two extra parameters under "x-koha-authorization": - "allow-owner"; Allows the owner of object to access it (without permission) - "allow-guarantor"; Allows guarantor of the owner of object to access it (without permission) Third, since permission checking is outside of actual controller, we need a way to find out ownership from different types of parameters, e.g. checkout_id from /checkouts/{checkout_id}, borrowernumber from /patrons/{borrowernumber} etc. A solution is to match the parameter with a subroutine that is designed to verify the ownership for that object. See the new subroutines in Koha::REST::V1. To use this functionality you will simply define it in Swagger: "/patrons/{borrowernumber}": { "get": { ..., "x-koha-authorization": { "allow-owner": true, "permissions": { "borrowers": "1" } } } } If a parameter that is not yet defined in Koha::REST::V1::check_object_ownership, you also need to define it and implement a subroutine that determines ownership. Tests are provided in a following patch that adds this functionality for current API operations. Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> Created attachment 54896 [details] [review] Bug 14868: Use x-koha-authorization in current routes To test: 1. Run t/db_dependent/api/v1/holds.t 2. Run t/db_dependent/api/v1/patrons.t Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> Created attachment 54897 [details] [review] Bug 14868: Display required permissions in permission error response When user does not have required permissions to use API operation, it would be useful to let them know which permissions he is missing. Since they are now defined in Swagger, we can easily render them into the response. To test: 1. Use a patron without any permissions 2. Make GET request to http://yourlib/api/v1/patrons 3. Observe permission error and see that required_permissions are displayed. 4. Run t/db_dependent/api/v1/patrons.t Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> Great Work, Lari! Tested thus : 1) minified swagger: perl misc/devel/minifySwagger.pl -s api/v1/swagger/swagger.json -d api/v1/swagger/swagger.min.json (minifySwagger should perhaps have execution permissions?) 2) restarted plack to pick up changes 3) loaded testdata and added circulation rule to allow holds 4) ran tests t/db_dependent/api/v1/patrons.t and t/db_dependent/api/v1/holds.t 5) ran some manual tests, without permissions, observed authentication failure 6) tested with patron session without borrower permissions, permission failure except on own patron info and own holds 7) tested with superlibrarian Should get an extra pair of eyes and testing, but I'm all OK. Now let's get Bug Bug 17004 moving as well (API auth), and we're really moving towards a great RESTful API Comment on attachment 54895 [details] [review] Bug 14868: Give users possibility to request their own object Review of attachment 54895 [details] [review]: ----------------------------------------------------------------- ::: Koha/REST/V1.pm @@ +203,5 @@ > + > +sub _object_ownership_by_borrowernumber { > + my ($c, $user, $borrowernumber) = @_; > + > + return $user->borrowernumber == $borrowernumber; I think it's not that easy. For instance at the OPAC, a patron is not allowed to update his own details, it will need to be approved by a staff member. Using the REST API he will be able to bypass the approval. Created attachment 55084 [details] [review] Bug 14868: Swagger2-driven Permission checking Define 'x-koha-permission' for the Swagger2 Operation Object, to automatically authorize against the required permissions. This way we immediately tell the API consumer in the Swagger2-definition, which permissions are needed to access defined resources. Also we don't need to maintain permissions in multiple locations and we can build a smart testing framework to help a lot in creating tests for the new REST API. Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 55085 [details] [review] Bug 14868: Give users possibility to request their own object Allow access to user's own objects even if they do not have required permissions. This will be very useful in many cases where an user wants to request their own object, for example renewing their checkouts or placing a hold for themselves. First, this patch renames "x-koha-permission" to "x-koha-authorization" in order to describe the new functionality better. Second, we can now add two extra parameters under "x-koha-authorization": - "allow-owner"; Allows the owner of object to access it (without permission) - "allow-guarantor"; Allows guarantor of the owner of object to access it (without permission) Third, since permission checking is outside of actual controller, we need a way to find out ownership from different types of parameters, e.g. checkout_id from /checkouts/{checkout_id}, borrowernumber from /patrons/{borrowernumber} etc. A solution is to match the parameter with a subroutine that is designed to verify the ownership for that object. See the new subroutines in Koha::REST::V1. To use this functionality you will simply define it in Swagger: "/patrons/{borrowernumber}": { "get": { ..., "x-koha-authorization": { "allow-owner": true, "permissions": { "borrowers": "1" } } } } If a parameter that is not yet defined in Koha::REST::V1::check_object_ownership, you also need to define it and implement a subroutine that determines ownership. Tests are provided in a following patch that adds this functionality for current API operations. Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 55086 [details] [review] Bug 14868: Use x-koha-authorization in current routes To test: 1. Run t/db_dependent/api/v1/holds.t 2. Run t/db_dependent/api/v1/patrons.t Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 55087 [details] [review] Bug 14868: Display required permissions in permission error response When user does not have required permissions to use API operation, it would be useful to let them know which permissions he is missing. Since they are now defined in Swagger, we can easily render them into the response. To test: 1. Use a patron without any permissions 2. Make GET request to http://yourlib/api/v1/patrons 3. Observe permission error and see that required_permissions are displayed. 4. Run t/db_dependent/api/v1/patrons.t Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 55088 [details] [review] Bug 14868: (QA followup) Change permission check order This patch changes the permission check order because haspermission is the smaller check, and going through the patron/user and its guaranteed before checking if it is (say) a staff member or even a superlibrarian doesn't seem right. Bonus: Remove unneeded C4::Auth import in Patron.pm Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 55089 [details] [review] Bug 14868: (QA followup) POD fixes To make the QA scripts happy, the POD needed a fix, and also keys applied to a hashref needs to be avoided. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 55099 [details] [review] Bug 14868: Display required permissions in permission error response When user does not have required permissions to use API operation, it would be useful to let them know which permissions he is missing. Since they are now defined in Swagger, we can easily render them into the response. To test: 1. Use a patron without any permissions 2. Make GET request to http://yourlib/api/v1/patrons 3. Observe permission error and see that required_permissions are displayed. 4. Run t/db_dependent/api/v1/patrons.t Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 55100 [details] [review] Bug 14868: Swagger2-driven Permission checking Define 'x-koha-permission' for the Swagger2 Operation Object, to automatically authorize against the required permissions. This way we immediately tell the API consumer in the Swagger2-definition, which permissions are needed to access defined resources. Also we don't need to maintain permissions in multiple locations and we can build a smart testing framework to help a lot in creating tests for the new REST API. Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 55101 [details] [review] Bug 14868: Give users possibility to request their own object Allow access to user's own objects even if they do not have required permissions. This will be very useful in many cases where an user wants to request their own object, for example renewing their checkouts or placing a hold for themselves. First, this patch renames "x-koha-permission" to "x-koha-authorization" in order to describe the new functionality better. Second, we can now add two extra parameters under "x-koha-authorization": - "allow-owner"; Allows the owner of object to access it (without permission) - "allow-guarantor"; Allows guarantor of the owner of object to access it (without permission) Third, since permission checking is outside of actual controller, we need a way to find out ownership from different types of parameters, e.g. checkout_id from /checkouts/{checkout_id}, borrowernumber from /patrons/{borrowernumber} etc. A solution is to match the parameter with a subroutine that is designed to verify the ownership for that object. See the new subroutines in Koha::REST::V1. To use this functionality you will simply define it in Swagger: "/patrons/{borrowernumber}": { "get": { ..., "x-koha-authorization": { "allow-owner": true, "permissions": { "borrowers": "1" } } } } If a parameter that is not yet defined in Koha::REST::V1::check_object_ownership, you also need to define it and implement a subroutine that determines ownership. Tests are provided in a following patch that adds this functionality for current API operations. Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 55102 [details] [review] Bug 14868: Use x-koha-authorization in current routes To test: 1. Run t/db_dependent/api/v1/holds.t 2. Run t/db_dependent/api/v1/patrons.t Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 55103 [details] [review] Bug 14868: Display required permissions in permission error response When user does not have required permissions to use API operation, it would be useful to let them know which permissions he is missing. Since they are now defined in Swagger, we can easily render them into the response. To test: 1. Use a patron without any permissions 2. Make GET request to http://yourlib/api/v1/patrons 3. Observe permission error and see that required_permissions are displayed. 4. Run t/db_dependent/api/v1/patrons.t Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 55104 [details] [review] Bug 14868: (QA followup) Change permission check order This patch changes the permission check order because haspermission is the smaller check, and going through the patron/user and its guaranteed before checking if it is (say) a staff member or even a superlibrarian doesn't seem right. Bonus: Remove unneeded C4::Auth import in Patron.pm Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> Created attachment 55105 [details] [review] Bug 14868: (QA followup) POD fixes To make the QA scripts happy, the POD needed a fix, and also keys applied to a hashref needs to be avoided. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> Pushed to master for 16.11, thanks Olli, Lari, Tomas! *** Bug 16815 has been marked as a duplicate of this bug. *** |