Bug 14868

Summary: REST API: Swagger2-driven permission checking
Product: Koha Reporter: Julian Maurice <julian.maurice>
Component: Web servicesAssignee: Lari Taskula <lari.taskula>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: abl, benjamin.rokseth, f.demians, gitbot, kyle, lari.taskula, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16815
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 16699, 17050    
Bug Blocks: 17479, 8030, 13895, 16330, 16652, 17190, 19405    
Attachments: Bug 14868: Swagger2-driven Permission checking
Bug 14868: Swagger2-driven Permission checking
Bug 14868: Give users possibility to request their own object
Bug 14868: Use x-koha-permission in current routes
Bug 14868: Swagger2-driven Permission checking
Bug 14868: Give users possibility to request their own object
Bug 14868: Use x-koha-permission in current routes
Bug 14868: Swagger2-driven Permission checking
Bug 14868: Give users possibility to request their own object
Bug 14868: Use x-koha-permission in current routes
Bug 14868: Rename 'x-koha-permission' to 'x-koha-authorization'
Bug 14868: Swagger2-driven Permission checking
Bug 14868: Give users possibility to request their own object
Bug 14868: Use x-koha-permission in current routes
Bug 14868: Rename 'x-koha-permission' to 'x-koha-authorization'
[SIGNED-OFF] Bug 14868: Swagger2-driven Permission checking
[SIGNED-OFF] Bug 14868: Swagger2-driven Permission checking
[SIGNED-OFF] Bug 14868: Give users possibility to request their own object
[SIGNED-OFF] Bug 14868: Use x-koha-permission in current routes
[SIGNED-OFF] Bug 14868: Rename 'x-koha-permission' to 'x-koha-authorization'
[SIGNED-OFF] Bug 14868: Rename 'x-koha-permission' to 'x-koha-authorization'
Bug 14868: Swagger2-driven Permission checking
Bug 14868: Give users possibility to request their own object
Bug 14868: Use x-koha-authorization in current routes
Bug 14868: Swagger2-driven Permission checking
Bug 14868: Give users possibility to request their own object
Bug 14868: Use x-koha-authorization in current routes
Bug 14868: Swagger2-driven Permission checking
Bug 14868: Give users possibility to request their own object
Bug 14868: Use x-koha-authorization in current routes
Bug 14868: Display required permissions in permission error response
Bug 14868: Display required permissions in permission error response
Bug 14868: Swagger2-driven Permission checking
Bug 14868: Give users possibility to request their own object
Bug 14868: Use x-koha-authorization in current routes
Bug 14868: Display required permissions in permission error response
Bug 14868: Swagger2-driven Permission checking
Bug 14868: Give users possibility to request their own object
Bug 14868: Use x-koha-authorization in current routes
Bug 14868: Display required permissions in permission error response
Bug 14868: (QA followup) Change permission check order
Bug 14868: (QA followup) POD fixes
Bug 14868: Display required permissions in permission error response
Bug 14868: Swagger2-driven Permission checking
Bug 14868: Give users possibility to request their own object
Bug 14868: Use x-koha-authorization in current routes
Bug 14868: Display required permissions in permission error response
Bug 14868: (QA followup) Change permission check order
Bug 14868: (QA followup) POD fixes

Description Julian Maurice 2015-09-22 09:02:48 UTC

    
Comment 1 Julian Maurice 2015-09-22 09:03:18 UTC Comment hidden (obsolete)
Comment 2 Lari Taskula 2016-06-20 12:29:02 UTC Comment hidden (obsolete)
Comment 3 Lari Taskula 2016-06-20 12:29:25 UTC Comment hidden (obsolete)
Comment 4 Lari Taskula 2016-06-20 12:30:04 UTC Comment hidden (obsolete)
Comment 5 Lari Taskula 2016-06-20 14:03:41 UTC Comment hidden (obsolete)
Comment 6 Lari Taskula 2016-06-20 14:04:02 UTC Comment hidden (obsolete)
Comment 7 Lari Taskula 2016-06-20 14:04:18 UTC Comment hidden (obsolete)
Comment 8 Lari Taskula 2016-06-20 15:18:34 UTC Comment hidden (obsolete)
Comment 9 Lari Taskula 2016-06-20 15:19:18 UTC Comment hidden (obsolete)
Comment 10 Lari Taskula 2016-06-20 15:19:40 UTC Comment hidden (obsolete)
Comment 11 Lari Taskula 2016-06-30 11:45:17 UTC Comment hidden (obsolete)
Comment 12 Olli-Antti Kivilahti 2016-06-30 12:27:17 UTC Comment hidden (obsolete)
Comment 13 Olli-Antti Kivilahti 2016-06-30 12:27:54 UTC Comment hidden (obsolete)
Comment 14 Olli-Antti Kivilahti 2016-06-30 12:28:21 UTC Comment hidden (obsolete)
Comment 15 Olli-Antti Kivilahti 2016-06-30 12:28:53 UTC Comment hidden (obsolete)
Comment 16 Johanna Räisä 2016-06-30 13:40:42 UTC Comment hidden (obsolete)
Comment 17 Johanna Räisä 2016-06-30 13:53:19 UTC Comment hidden (obsolete)
Comment 18 Johanna Räisä 2016-06-30 13:53:33 UTC Comment hidden (obsolete)
Comment 19 Johanna Räisä 2016-06-30 13:53:48 UTC Comment hidden (obsolete)
Comment 20 Johanna Räisä 2016-06-30 13:54:09 UTC Comment hidden (obsolete)
Comment 21 Johanna Räisä 2016-06-30 13:54:22 UTC Comment hidden (obsolete)
Comment 22 Benjamin Rokseth 2016-07-08 09:04:40 UTC
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!
Comment 23 Lari Taskula 2016-08-10 16:20:20 UTC
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.
Comment 24 Lari Taskula 2016-08-15 09:59:58 UTC Comment hidden (obsolete)
Comment 25 Lari Taskula 2016-08-15 10:00:28 UTC Comment hidden (obsolete)
Comment 26 Lari Taskula 2016-08-15 10:01:01 UTC Comment hidden (obsolete)
Comment 27 Lari Taskula 2016-08-15 10:15:30 UTC
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.
Comment 28 Lari Taskula 2016-08-19 13:06:24 UTC Comment hidden (obsolete)
Comment 29 Lari Taskula 2016-08-19 13:06:48 UTC Comment hidden (obsolete)
Comment 30 Lari Taskula 2016-08-19 13:07:10 UTC Comment hidden (obsolete)
Comment 31 Tomás Cohen Arazi 2016-08-22 18:44:44 UTC
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!
Comment 32 Lari Taskula 2016-08-23 11:51:07 UTC Comment hidden (obsolete)
Comment 33 Lari Taskula 2016-08-23 11:51:32 UTC Comment hidden (obsolete)
Comment 34 Lari Taskula 2016-08-23 11:51:52 UTC Comment hidden (obsolete)
Comment 35 Lari Taskula 2016-08-23 12:47:30 UTC Comment hidden (obsolete)
Comment 36 Lari Taskula 2016-08-23 12:48:30 UTC Comment hidden (obsolete)
Comment 37 Benjamin Rokseth 2016-08-25 22:26:26 UTC Comment hidden (obsolete)
Comment 38 Benjamin Rokseth 2016-08-25 22:26:34 UTC Comment hidden (obsolete)
Comment 39 Benjamin Rokseth 2016-08-25 22:26:42 UTC Comment hidden (obsolete)
Comment 40 Benjamin Rokseth 2016-08-25 22:26:48 UTC Comment hidden (obsolete)
Comment 41 Benjamin Rokseth 2016-08-25 22:36:37 UTC
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 42 Jonathan Druart 2016-08-29 09:23:22 UTC
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.
Comment 43 Tomás Cohen Arazi 2016-08-31 23:46:01 UTC Comment hidden (obsolete)
Comment 44 Tomás Cohen Arazi 2016-08-31 23:46:08 UTC Comment hidden (obsolete)
Comment 45 Tomás Cohen Arazi 2016-08-31 23:46:16 UTC Comment hidden (obsolete)
Comment 46 Tomás Cohen Arazi 2016-08-31 23:46:24 UTC Comment hidden (obsolete)
Comment 47 Tomás Cohen Arazi 2016-08-31 23:46:29 UTC Comment hidden (obsolete)
Comment 48 Tomás Cohen Arazi 2016-08-31 23:46:34 UTC Comment hidden (obsolete)
Comment 49 Benjamin Rokseth 2016-09-01 20:59:31 UTC Comment hidden (obsolete)
Comment 50 Benjamin Rokseth 2016-09-01 21:01:18 UTC
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>
Comment 51 Benjamin Rokseth 2016-09-01 21:01:28 UTC
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>
Comment 52 Benjamin Rokseth 2016-09-01 21:01:35 UTC
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>
Comment 53 Benjamin Rokseth 2016-09-01 21:01:51 UTC
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>
Comment 54 Benjamin Rokseth 2016-09-01 21:01:59 UTC
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>
Comment 55 Benjamin Rokseth 2016-09-01 21:02:05 UTC
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>
Comment 56 Kyle M Hall 2016-09-02 13:26:34 UTC
Pushed to master for 16.11, thanks Olli, Lari, Tomas!
Comment 57 Tomás Cohen Arazi 2016-12-14 14:05:38 UTC
*** Bug 16815 has been marked as a duplicate of this bug. ***