Bug 17479 - REST API: Save information on owner access
Summary: REST API: Save information on owner access
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Bugs List
QA Contact:
URL:
Keywords:
Depends on: 14868 18137
Blocks: 17565
  Show dependency treegraph
 
Reported: 2016-10-21 09:52 UTC by Lari Taskula
Modified: 2020-03-17 21:25 UTC (History)
5 users (show)

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


Attachments
Bug 17479: Store information on owner access into $c->stash (6.33 KB, patch)
2016-10-21 10:41 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17479: Store information on owner access into $c->stash (6.40 KB, patch)
2016-11-07 12:42 UTC, Lari Taskula
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17479: Store information on owner access into $c->stash (6.46 KB, patch)
2016-11-07 21:06 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17479: Store information on owner access into $c->stash (6.92 KB, patch)
2017-03-16 15:37 UTC, Lari Taskula
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lari Taskula 2016-10-21 09:52:52 UTC
There are two ways of accessing a resource via REST API; either:
- you have the required permission
- you do not have the permission but you are owner of the object, e.g. you want to GET your own patron information

In many cases we want to perform additional operations if the user is accessing his own object. Usually this additional operation is checking a system preference.

Example: Patron wants to update his own patron information via REST API. We have to check OPACPatronDetails system preference for this. If it is on, we should forward the changes for approval from a librarian.

Currently, in controller, we can check this opac-like access by checking that the user does not have permissions and that the patron he is accessing is himself. This would require another haspermission() call.

Instead, we could set a flag into $c->stash in Koha/REST/V1.pm in the case of ownership access. After this, in controller, we only need to check $c->stash for this flag.
Comment 1 Lari Taskula 2016-10-21 10:41:07 UTC
Created attachment 56720 [details] [review]
Bug 17479: Store information on owner access into $c->stash

There are two ways of accessing a resource via REST API; either:
- you have the required permission
- you do not have the permission but you are owner of the object, e.g. you want
  to GET your own patron information

In many cases we want to perform additional operations if the user is accessing his
own object. Usually this additional operation is checking a system preference.

Example: Patron wants to update his own patron information via REST API. We have to
check OPACPatronDetails system preference for this. If it is on, we should forward
the changes for approval from a librarian.

Currently, in controller, we can check this opac-like access by checking that the
user does not have permissions and that the patron he is accessing is himself. This
would require another haspermission() call.

Instead, we could set a flag into $c->stash in Koha/REST/V1.pm in the case of
ownership access. After this, in controller, we only need to check $c->stash for
this flag.

To test:
1. Apply patch
2. Run t/db_dependent/api/v1/ownerflag.t
3. Observe it pass
Comment 2 Lari Taskula 2016-11-07 12:42:47 UTC
Created attachment 57256 [details] [review]
Bug 17479: Store information on owner access into $c->stash

There are two ways of accessing a resource via REST API; either:
- you have the required permission
- you do not have the permission but you are owner of the object, e.g. you want
  to GET your own patron information

In many cases we want to perform additional operations if the user is accessing his
own object. Usually this additional operation is checking a system preference.

Example: Patron wants to update his own patron information via REST API. We have to
check OPACPatronDetails system preference for this. If it is on, we should forward
the changes for approval from a librarian.

Currently, in controller, we can check this opac-like access by checking that the
user does not have permissions and that the patron he is accessing is himself. This
would require another haspermission() call.

Instead, we could set a flag into $c->stash in Koha/REST/V1.pm in the case of
ownership access. After this, in controller, we only need to check $c->stash for
this flag.

To test:
1. Apply patch
2. Run t/db_dependent/api/v1/ownerflag.t
3. Observe it pass
Comment 3 Josef Moravec 2016-11-07 21:06:28 UTC
Created attachment 57286 [details] [review]
[SIGNED-OFF] Bug 17479: Store information on owner access into $c->stash

There are two ways of accessing a resource via REST API; either:
- you have the required permission
- you do not have the permission but you are owner of the object, e.g. you want
  to GET your own patron information

In many cases we want to perform additional operations if the user is accessing his
own object. Usually this additional operation is checking a system preference.

Example: Patron wants to update his own patron information via REST API. We have to
check OPACPatronDetails system preference for this. If it is on, we should forward
the changes for approval from a librarian.

Currently, in controller, we can check this opac-like access by checking that the
user does not have permissions and that the patron he is accessing is himself. This
would require another haspermission() call.

Instead, we could set a flag into $c->stash in Koha/REST/V1.pm in the case of
ownership access. After this, in controller, we only need to check $c->stash for
this flag.

To test:
1. Apply patch
2. Run t/db_dependent/api/v1/ownerflag.t
3. Observe it pass

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 4 Jiri Kozlovsky 2016-12-11 21:54:28 UTC
I like this flag approach! How about also adding the owned object to the stash? It is very common to perform duplicate search for that object (you may need some arbitrary column from it or to check if something has changes, etc ..)

Now you create DB query when checking for ownership and then, the same query when the object itself is required from within the controller.

After this implemented, there would always be only one DB query for the owned object. In the controller then, it'd be called $c->stash('owned_object').
Comment 5 Marcel de Rooy 2017-03-10 09:03:59 UTC
Just a dumb [hypothetical] question, since I am not that deep into Mojolicious etc.
You only set the flag; you do not clear the flag.
How do you make sure that setting this owner flag is not misused/abused later on? Is it possible that it is still on thru persistence?
I could imagine that you would clear this flag in the else branch of the haspermission if?
Comment 6 Lari Taskula 2017-03-10 11:19:44 UTC
In Mojolicious, the stash, which we use for this flag, is a non-persistent storage only for the current request. So it will be cleared for the following requests and then set again.
Comment 7 Marcel de Rooy 2017-03-10 11:33:03 UTC
(In reply to Lari Taskula from comment #6)
> In Mojolicious, the stash, which we use for this flag, is a non-persistent
> storage only for the current request. So it will be cleared for the
> following requests and then set again.

OK I suspected that. Altough clearing it in the else still seems safe..
Comment 8 Lari Taskula 2017-03-16 15:37:14 UTC
Created attachment 61174 [details] [review]
Bug 17479: Store information on owner access into $c->stash

There are two ways of accessing a resource via REST API; either:
- you have the required permission
- you do not have the permission but you are owner of the object, e.g. you want
  to GET your own patron information

In many cases we want to perform additional operations if the user is accessing his
own object. Usually this additional operation is checking a system preference.

Example: Patron wants to update his own patron information via REST API. We have to
check OPACPatronDetails system preference for this. If it is on, we should forward
the changes for approval from a librarian.

Currently, in controller, we can check this opac-like access by checking that the
user does not have permissions and that the patron he is accessing is himself. This
would require another haspermission() call.

Instead, we could set a flag into $c->stash in Koha/REST/V1.pm in the case of
ownership access. After this, in controller, we only need to check $c->stash for
this flag.

To test:
1. Apply patch
2. Run t/db_dependent/api/v1/ownerflag.t
3. Observe it pass
Comment 9 Lari Taskula 2017-03-16 15:39:10 UTC
Rebased on top of Bug 18137. To test, first apply Bug 18137, then this one, and run tests. Removed sign-offs as the test file was heavily rewritten.
Comment 10 Jiri Kozlovsky 2017-03-17 17:07:29 UTC
Lari, what do you think about storing owned object also?

I think sparing one DB request for each owner / guarantor request is a good step forward.
Comment 11 Lari Taskula 2017-03-17 17:13:03 UTC
(In reply to Jiri Kozlovsky from comment #10)
> Lari, what do you think about storing owned object also?
> 
> I think sparing one DB request for each owner / guarantor request is a good
> step forward.

True. I don't see why we should not also stash the owned object. Feel free to provide a follow-up :)
Comment 12 Josef Moravec 2017-06-14 08:31:36 UTC
The test is failing:

t/db_dependent/api/v1/ownerflag.t .. 
    #   Failed test 'exact match for JSON Pointer "/error"'
    #   at t/db_dependent/api/v1/ownerflag.t line 61.
    #          got: 'librarian_access'
    #     expected: 'is_owner_access'
    # Looks like you failed 1 test of 3.
t/db_dependent/api/v1/ownerflag.t .. 1/3 
#   Failed test 'without permission, owner of object tests'
#   at t/db_dependent/api/v1/ownerflag.t line 64.

    #   Failed test 'exact match for JSON Pointer "/error"'
    #   at t/db_dependent/api/v1/ownerflag.t line 73.
    #          got: 'librarian_access'
    #     expected: 'is_guarantor_access'
    # Looks like you failed 1 test of 3.

#   Failed test 'without permissions, guarantor of the owner of the object tests'
#   at t/db_dependent/api/v1/ownerflag.t line 76.
# Looks like you failed 2 tests of 3.
t/db_dependent/api/v1/ownerflag.t .. Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/3 subtests 

Test Summary Report
-------------------
t/db_dependent/api/v1/ownerflag.t (Wstat: 512 Tests: 3 Failed: 2)
  Failed tests:  1-2
  Non-zero exit status: 2
Files=1, Tests=3,  2 wallclock secs ( 0.02 usr  0.00 sys +  1.19 cusr  0.20 csys =  1.41 CPU)
Result: FAIL
Comment 13 Lari Taskula 2020-03-17 21:25:53 UTC
I'm no longer able to work on this, so I'm setting assignee to default. Feel free to continue this work.