Bug 22227 - Make GET /cities staff only
Summary: Make GET /cities staff only
Status: RESOLVED FIXED
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: release-notes-needed
Depends on: 22061
Blocks: 22218
  Show dependency treegraph
 
Reported: 2019-01-29 14:41 UTC by Tomás Cohen Arazi
Modified: 2019-02-25 13:34 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 22227: Make GET /cities staff only (2.38 KB, patch)
2019-01-29 14:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22227: Make GET /cities staff only (2.44 KB, patch)
2019-02-13 11:11 UTC, Martin Renvoize
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 2019-01-29 14:41:05 UTC
Now that we voted to have a /public namespace for unprivileged access endpoints, the existing /cities endpoint needs to be adapted.
Comment 1 Tomás Cohen Arazi 2019-01-29 14:41:44 UTC
Created attachment 84507 [details] [review]
Bug 22227: Make GET /cities staff only

This patch removes the possibility to access the city objects without
privileged access (minimum permissions == catalogue).

It does so by adding the required permissions to the spec. The tests are
adjusted.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/cities.t
=> SUCCESS: Tests pass!
- Sign off :-D
Comment 2 Jon Knight 2019-01-30 18:57:32 UTC
Do we need to do anything other than what is in the test plan on a kohadevbox? For example have some particular data loaded in our database? I've just tried to apply the patch with the following result:

vagrant@kohadevbox:kohaclone(master)$ git checkout -b bug22227
Switched to a new branch 'bug22227'
vagrant@kohadevbox:kohaclone(bug22227)$ git bz apply 22227
Bug 22227 - Make GET /cities staff only

84507 - Bug 22227: Make GET /cities staff only

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 22227: Make GET /cities staff only
vagrant@kohadevbox:kohaclone(bug22227)$ kshell
kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/api/v1/cities.t
t/db_dependent/api/v1/cities.t .. 
    #   Failed test '200 OK'
    #   at t/db_dependent/api/v1/cities.t line 87.
    #          got: '400'
    #     expected: '200'

    #   Failed test 'exact match for JSON Pointer ""'
    #   at t/db_dependent/api/v1/cities.t line 87.
    #     Structures begin differing at:
    #          $got->[0]{state} = Does not exist
    #     $expected->[0]{state} = 'O5v1BdpdGC68BA87LFM5W_sSkKGJLmnkPL4rz_34K8hlXzvLUtZsoeuJ8x6tv8zKdJ'

    #   Failed test '200 OK'
    #   at t/db_dependent/api/v1/cities.t line 96.
    #          got: '400'
    #     expected: '200'

    #   Failed test 'exact match for JSON Pointer ""'
    #   at t/db_dependent/api/v1/cities.t line 96.
    #     Structures begin differing at:
    #          $got->[0]{city_id} = Does not exist
    #     $expected->[0]{city_id} = '12'
    # Looks like you failed 4 tests of 18.
t/db_dependent/api/v1/cities.t .. 1/5 
#   Failed test 'list() tests'
#   at t/db_dependent/api/v1/cities.t line 109.

    #   Failed test '403 Forbidden'
    #   at t/db_dependent/api/v1/cities.t line 162.
    #          got: '200'
    #     expected: '403'
    # Looks like you failed 1 test of 17.

#   Failed test 'add() tests'
#   at t/db_dependent/api/v1/cities.t line 226.

    #   Failed test '403 Forbidden'
    #   at t/db_dependent/api/v1/cities.t line 246.
    #          got: '400'
    #     expected: '403'
    # Looks like you failed 1 test of 15.

#   Failed test 'update() tests'
#   at t/db_dependent/api/v1/cities.t line 324.

    #   Failed test '403 Forbidden'
    #   at t/db_dependent/api/v1/cities.t line 343.
    #          got: '200'
    #     expected: '403'

    #   Failed test '200 OK'
    #   at t/db_dependent/api/v1/cities.t line 349.
    #          got: '404'
    #     expected: '200'

    #   Failed test 'exact match for content'
    #   at t/db_dependent/api/v1/cities.t line 349.
    #          got: '{"error":"Object not found"}'
    #     expected: '""'
    # Looks like you failed 3 tests of 7.

#   Failed test 'delete() tests'
#   at t/db_dependent/api/v1/cities.t line 360.
# Looks like you failed 4 tests of 5.
t/db_dependent/api/v1/cities.t .. Dubious, test returned 4 (wstat 1024, 0x400)
Failed 4/5 subtests 

Test Summary Report
-------------------
t/db_dependent/api/v1/cities.t (Wstat: 1024 Tests: 5 Failed: 4)
  Failed tests:  1, 3-5
  Non-zero exit status: 4
Files=1, Tests=5,  3 wallclock secs ( 0.02 usr  0.00 sys +  2.37 cusr  0.24 csys =  2.63 CPU)
Result: FAIL
kohadev-koha@kohadevbox:/home/vagrant/kohaclone$
Comment 3 Tomás Cohen Arazi 2019-01-31 14:11:03 UTC
Can you retry?:

 $ git fetch
 $ git checkout origin/master -b bug22227
 $ kshell
k$ prove t/db_dependent/api/v1/cities.pl

In my case:

 $ kshell
k$ prove t/db_dependent/api/v1/cities.t
t/db_dependent/api/v1/cities.t .. ok
All tests successful.
Files=1, Tests=5,  4 wallclock secs ( 0.03 usr  0.01 sys +  3.21 cusr  0.37 csys =  3.62 CPU)
Result: PASS
Comment 4 Martin Renvoize 2019-02-13 11:11:44 UTC
Created attachment 85060 [details] [review]
Bug 22227: Make GET /cities staff only

This patch removes the possibility to access the city objects without
privileged access (minimum permissions == catalogue).

It does so by adding the required permissions to the spec. The tests are
adjusted.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/cities.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2019-02-13 11:12:13 UTC
Solid change, passes tests... going straight for PQA on this one.
Comment 6 Martin Renvoize 2019-02-13 11:12:45 UTC
Another api breaking change, can we get a note for the release notes.
Comment 7 Jon Knight 2019-02-13 11:57:21 UTC
Weird - it still fails on my kohadevbox:

vagrant@kohadevbox:kohaclone((66f3a72...))$ git fetch
vagrant@kohadevbox:kohaclone((66f3a72...))$ git checkout origin/master -b bug22227
Branch bug22227 set up to track remote branch master from origin.
Switched to a new branch 'bug22227'
vagrant@kohadevbox:kohaclone(bug22227)$ git bz apply 22227
Bug 22227 - Make GET /cities staff only

85060 - Bug 22227: Make GET /cities staff only

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 22227: Make GET /cities staff only
vagrant@kohadevbox:kohaclone(bug22227)$ kshell
kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/api/v1/cities.t 
t/db_dependent/api/v1/cities.t .. 
    #   Failed test '200 OK'
    #   at t/db_dependent/api/v1/cities.t line 87.
    #          got: '400'
    #     expected: '200'

    #   Failed test 'exact match for JSON Pointer ""'
    #   at t/db_dependent/api/v1/cities.t line 87.
    #     Structures begin differing at:
    #          $got->[0]{state} = Does not exist
    #     $expected->[0]{state} = 'VsCDNM9zaCm6qJBgAu2IdTVEi5Vxnw7unyoPbnQ1hgf8Yyfx02jJPwsjX6wzcG2zFN0o1OkAFt7zq_cGwi9piH'

    #   Failed test '200 OK'
    #   at t/db_dependent/api/v1/cities.t line 96.
    #          got: '400'
    #     expected: '200'

    #   Failed test 'exact match for JSON Pointer ""'
    #   at t/db_dependent/api/v1/cities.t line 96.
    #     Structures begin differing at:
    #          $got->[0]{postal_code} = Does not exist
    #     $expected->[0]{postal_code} = 'JAmMlEZoTch'
    # Looks like you failed 4 tests of 18.
t/db_dependent/api/v1/cities.t .. 1/5 
#   Failed test 'list() tests'
#   at t/db_dependent/api/v1/cities.t line 109.

    #   Failed test '403 Forbidden'
    #   at t/db_dependent/api/v1/cities.t line 162.
    #          got: '200'
    #     expected: '403'
    # Looks like you failed 1 test of 17.

#   Failed test 'add() tests'
#   at t/db_dependent/api/v1/cities.t line 226.

    #   Failed test '403 Forbidden'
    #   at t/db_dependent/api/v1/cities.t line 246.
    #          got: '400'
    #     expected: '403'
    # Looks like you failed 1 test of 15.

#   Failed test 'update() tests'
#   at t/db_dependent/api/v1/cities.t line 324.

    #   Failed test '403 Forbidden'
    #   at t/db_dependent/api/v1/cities.t line 343.
    #          got: '200'
    #     expected: '403'

    #   Failed test '200 OK'
    #   at t/db_dependent/api/v1/cities.t line 349.
    #          got: '404'
    #     expected: '200'

    #   Failed test 'exact match for content'
    #   at t/db_dependent/api/v1/cities.t line 349.
    #          got: '{"error":"Object not found"}'
    #     expected: '""'
    # Looks like you failed 3 tests of 7.

#   Failed test 'delete() tests'
#   at t/db_dependent/api/v1/cities.t line 360.
# Looks like you failed 4 tests of 5.
t/db_dependent/api/v1/cities.t .. Dubious, test returned 4 (wstat 1024, 0x400)
Failed 4/5 subtests 

Test Summary Report
-------------------
t/db_dependent/api/v1/cities.t (Wstat: 1024 Tests: 5 Failed: 4)
  Failed tests:  1, 3-5
  Non-zero exit status: 4
Files=1, Tests=5,  3 wallclock secs ( 0.02 usr  0.01 sys +  2.36 cusr  0.17 csys =  2.56 CPU)
Result: FAIL
kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ 


But if it works on the Martin and Tomas's machines, I think I'll have to put it down to something weird in my database it doesn't like (probably from testing other patches).
Comment 8 Martin Renvoize 2019-02-13 12:36:13 UTC
How odd.. I'll give it another test run through koha-testing-docker just in case I missed something. Thanks for pointing it out Jon
Comment 9 Martin Renvoize 2019-02-13 12:56:25 UTC
Yup, passes perfectly on a re-check in koha-testing-docker.. I wonder if it is data related, though the test looks like it shouldn't rely on (nor be affected by) any existing data.  Clutching at straws have you recently run the dbi updates and dbic schema update script?  Perhaps there's a missing dbic schema class file change crept into the code somewhere?
Comment 10 Jon Knight 2019-02-13 13:52:49 UTC
Yes, I ran dbic update at the start of the week (when working on another bug) to make sure my database schema was up to date. I will have different data in the database to a "clean" kohadevbox though as I've used it for testing over time.

Ho hum, don't worry about it if it works for both of you. ;-)
Comment 11 Nick Clemens 2019-02-15 18:50:56 UTC
Awesome work all!

Pushed to master for 19.05
Comment 12 Martin Renvoize 2019-02-25 13:34:48 UTC
Change of behaviour, won't be backported to 18.11.x series