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.
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
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
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>
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').
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?
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.
(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..
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
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.
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.
(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 :)
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
I'm no longer able to work on this, so I'm setting assignee to default. Feel free to continue this work.