Bug 21198 - authenticate_api_request should stash the reason access is granted
Summary: authenticate_api_request should stash the reason access is granted
Status: CLOSED INVALID
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Keywords:
Depends on: 22071
Blocks:
  Show dependency treegraph
 
Reported: 2018-08-09 19:11 UTC by Tomás Cohen Arazi
Modified: 2020-01-06 20:15 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21198: Unit tests (8.88 KB, patch)
2019-01-07 13:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21198: Make authenticate_api_request stash the reason access is granted (1.71 KB, patch)
2019-01-07 13:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2018-08-09 19:11:13 UTC
There's a pattern we will be facing in the controllers where overloading the behaviour depending on the logged user permissions. We have the information on the request on the permissions check step, and it should be carried down to the controllers for its use.
Comment 1 Tomás Cohen Arazi 2019-01-07 13:30:16 UTC
Created attachment 83683 [details] [review]
Bug 21198: Unit tests
Comment 2 Tomás Cohen Arazi 2019-01-07 13:30:25 UTC
Created attachment 83684 [details] [review]
Bug 21198: Make authenticate_api_request stash the reason access is granted

This patch makes the authenticate_api_request method stash the reason
the request or has been granted access to the resource.

Possible values are:
- permissions
- owner
- guarantor

This will allow the controllers to avoid querying on their own for this
info.

To test:
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t
=> SUCCESS: Tests pass!
- Sign off :-D
Comment 3 Tomás Cohen Arazi 2019-01-07 13:35:50 UTC
When I thought about this, I was writing the /patrons/:patron_id/password endpoint, which was an hybrid endpoint: it was intended for the user to change its own password, and also an admin user to change a patron's password. It required the controller to check what was the reason to grant access to the resource.

We decided to create a /public namespace for endpoints that would be used by unprivileged users instead.

I decided to still submit this, because
- We might need it
- People writing endpoints through plugins could take advantage of this

I hope you'll agree with this enhancement.
Comment 4 Josef Moravec 2019-02-25 21:59:31 UTC

#   Failed test '200 OK'
    #   at t/db_dependent/api/v1/auth_authenticate_api_request.t line 161.
    #          got: '403'
    #     expected: '200'
    not ok 9 - grant_reason is 'guarantor'

    #   Failed test 'grant_reason is 'guarantor''
    #   at t/db_dependent/api/v1/auth_authenticate_api_request.t line 164.
    #          got: undef
    #     expected: 'guarantor'
Comment 5 Martin Renvoize 2019-03-08 15:43:49 UTC
At this point I'm not sure this really serves any purpose.. I'm going to mark it as invalid for now.  We can always revisit it in the future should we choose to do so.
Comment 6 Martin Renvoize 2019-03-08 15:47:01 UTC
In reality, what I believe would be considerably more useful is for us to implement the FIXME in C4::Auth->haspermission `#FIXME - This fcn should return the failed permission so a suitable error msg can be delivered.`

Passing back the permission one is missing, perhaps as a catch-able Koha::Exception or something would be a distinctly more useful pattern.