Bug 24909 - Add unprivileged route to get a bibliographic record
Summary: Add unprivileged route to get a bibliographic record
Status: Pushed to oldstable
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: 23677
Blocks: 17371 25045
  Show dependency treegraph
 
Reported: 2020-03-19 15:29 UTC by Tomás Cohen Arazi
Modified: 2020-06-18 00:17 UTC (History)
9 users (show)

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


Attachments
Bug 24909: Unit tests (6.13 KB, patch)
2020-03-19 20:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24909: Route spec (2.81 KB, patch)
2020-03-19 20:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24909: Add unprivileged route to get a bibliographic record (5.11 KB, patch)
2020-03-19 20:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24909: Unit tests (6.19 KB, patch)
2020-03-31 22:24 UTC, David Nind
Details | Diff | Splinter Review
Bug 24909: Route spec (2.87 KB, patch)
2020-03-31 22:24 UTC, David Nind
Details | Diff | Splinter Review
Bug 24909: Add unprivileged route to get a bibliographic record (5.17 KB, patch)
2020-03-31 22:24 UTC, David Nind
Details | Diff | Splinter Review
Bug 24909: Unit tests (6.25 KB, patch)
2020-04-24 12:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24909: Route spec (2.93 KB, patch)
2020-04-24 12:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24909: Add unprivileged route to get a bibliographic record (5.23 KB, patch)
2020-04-24 12:53 UTC, Jonathan Druart
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 2020-03-19 15:29:51 UTC

    
Comment 1 Tomás Cohen Arazi 2020-03-19 15:30:50 UTC
It would be a handy addition so we can use JQuery to display more info on the biblios, and/or remove some old CGI scripts as well.
Comment 2 Jonathan Druart 2020-03-19 16:18:05 UTC
We have svc/records/preview already
Comment 3 Tomás Cohen Arazi 2020-03-19 17:40:31 UTC
This one is about the OPAC. I think you mean the other bug (24908).

But you are right about that. I want to introduce new things to the API and deprecate the old scripts.
Comment 4 Tomás Cohen Arazi 2020-03-19 20:20:01 UTC
Created attachment 101057 [details] [review]
Bug 24909: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Tomás Cohen Arazi 2020-03-19 20:20:08 UTC
Created attachment 101058 [details] [review]
Bug 24909: Route spec

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Tomás Cohen Arazi 2020-03-19 20:20:14 UTC
Created attachment 101059 [details] [review]
Bug 24909: Add unprivileged route to get a bibliographic record

This patch adds a route to get bibliographic records without privileged
access. This needs to match the OPAC expected behaviour.

Some things were considered on the implementation:
- The ViewPolicy filter that hides/shows things based on the frameworks
  needded to be used, as in the OPAC.
- OpacHiddenItems and OpacHiddenItemsExceptions need to be considered
  for hiding records the same way the OPAC is expected to.
- Avoid using OpacHiddenItemsExceptions, but rely on the patron category
  instead (use Koha::Patron::Category->override_hidden_items abstraction
  is used instead so it should keep working once 22547 is moved
  forward).
- Tests should cover all the use cases:
  * logged in user
  * anonymous user
  * logged in with category that overrides
  * logged in with category that doesn't override

This is all implemented on the tests.

To test:
1. Apply the tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/biblios.t
=> FAIL: Route not implemented
3. Apply the rest of the patches
4. Repeat 2
=> SUCCESS: Tests pass!
5. Try it with your favourite API tool (Postman?)
6. Sign off :-D

Note: please notice there isn't a default fallback behaviour for when
you don't specify the Accept header, so testing this on a regular
browser will just print the accepted mime types instead of the record
itself.

To test this with a tool (like Postman) you should enable
RESTBasicAuthe and make the tool use Basic authentication with valid
credentials. And you need to specify any of the following strings on the
Accept header:
- application/marcxml+xml
- application/marc-in-json
- application/marc
- text/plain
Comment 7 David Nind 2020-03-31 22:24:51 UTC
Created attachment 102163 [details] [review]
Bug 24909: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2020-03-31 22:24:54 UTC
Created attachment 102164 [details] [review]
Bug 24909: Route spec

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 David Nind 2020-03-31 22:24:58 UTC
Created attachment 102165 [details] [review]
Bug 24909: Add unprivileged route to get a bibliographic record

This patch adds a route to get bibliographic records without privileged
access. This needs to match the OPAC expected behaviour.

Some things were considered on the implementation:
- The ViewPolicy filter that hides/shows things based on the frameworks
  needded to be used, as in the OPAC.
- OpacHiddenItems and OpacHiddenItemsExceptions need to be considered
  for hiding records the same way the OPAC is expected to.
- Avoid using OpacHiddenItemsExceptions, but rely on the patron category
  instead (use Koha::Patron::Category->override_hidden_items abstraction
  is used instead so it should keep working once 22547 is moved
  forward).
- Tests should cover all the use cases:
  * logged in user
  * anonymous user
  * logged in with category that overrides
  * logged in with category that doesn't override

This is all implemented on the tests.

To test:
1. Apply the tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/biblios.t
=> FAIL: Route not implemented
3. Apply the rest of the patches
4. Repeat 2
=> SUCCESS: Tests pass!
5. Try it with your favourite API tool (Postman?)
6. Sign off :-D

Note: please notice there isn't a default fallback behaviour for when
you don't specify the Accept header, so testing this on a regular
browser will just print the accepted mime types instead of the record
itself.

To test this with a tool (like Postman) you should enable
RESTBasicAuthe and make the tool use Basic authentication with valid
credentials. And you need to specify any of the following strings on the
Accept header:
- application/marcxml+xml
- application/marc-in-json
- application/marc
- text/plain

Signed-off-by: David Nind <david@davidnind.com>
Comment 10 David Nind 2020-03-31 22:27:38 UTC
Note: text/plain is not an available option for Accept header.
Comment 11 Tomás Cohen Arazi 2020-03-31 22:47:49 UTC
(In reply to David Nind from comment #10)
> Note: text/plain is not an available option for Accept header.

Right, I can write a follow-up for that. I wrote that for the privileged access API before writing this one!
Comment 12 Jonathan Druart 2020-04-02 09:10:18 UTC
What happens if OpacPublic is disabled?
Comment 13 David Nind 2020-04-02 09:40:50 UTC
For me: tests still pass, as basic authentication is enabled the requests with Accept headers still work as well.
Comment 14 Tomás Cohen Arazi 2020-04-02 11:55:24 UTC
(In reply to Jonathan Druart from comment #12)
> What happens if OpacPublic is disabled?

Nothing.

All routes that are in the /public namespace can be disabled using the RESTPublicAPI syspref. We are not going to add tests for that in this bug, as it is tested generally in t/db_dependent/api/auth.t:50 onwards (bug 22061).
Comment 15 Jonathan Druart 2020-04-24 10:19:52 UTC
(In reply to Tomás Cohen Arazi from comment #11)
> (In reply to David Nind from comment #10)
> > Note: text/plain is not an available option for Accept header.
> 
> Right, I can write a follow-up for that. I wrote that for the privileged
> access API before writing this one!

Waiting for the follow-up.
Comment 16 Tomás Cohen Arazi 2020-04-24 11:27:56 UTC
(In reply to Jonathan Druart from comment #15)
> (In reply to Tomás Cohen Arazi from comment #11)
> > (In reply to David Nind from comment #10)
> > > Note: text/plain is not an available option for Accept header.
> > 
> > Right, I can write a follow-up for that. I wrote that for the privileged
> > access API before writing this one!
> 
> Waiting for the follow-up.

This route correctly implements Accept: text/plain, already. So back to SO.
Comment 17 Jonathan Druart 2020-04-24 12:53:36 UTC
Created attachment 103672 [details] [review]
Bug 24909: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 18 Jonathan Druart 2020-04-24 12:53:40 UTC
Created attachment 103673 [details] [review]
Bug 24909: Route spec

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 19 Jonathan Druart 2020-04-24 12:53:43 UTC
Created attachment 103674 [details] [review]
Bug 24909: Add unprivileged route to get a bibliographic record

This patch adds a route to get bibliographic records without privileged
access. This needs to match the OPAC expected behaviour.

Some things were considered on the implementation:
- The ViewPolicy filter that hides/shows things based on the frameworks
  needded to be used, as in the OPAC.
- OpacHiddenItems and OpacHiddenItemsExceptions need to be considered
  for hiding records the same way the OPAC is expected to.
- Avoid using OpacHiddenItemsExceptions, but rely on the patron category
  instead (use Koha::Patron::Category->override_hidden_items abstraction
  is used instead so it should keep working once 22547 is moved
  forward).
- Tests should cover all the use cases:
  * logged in user
  * anonymous user
  * logged in with category that overrides
  * logged in with category that doesn't override

This is all implemented on the tests.

To test:
1. Apply the tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/biblios.t
=> FAIL: Route not implemented
3. Apply the rest of the patches
4. Repeat 2
=> SUCCESS: Tests pass!
5. Try it with your favourite API tool (Postman?)
6. Sign off :-D

Note: please notice there isn't a default fallback behaviour for when
you don't specify the Accept header, so testing this on a regular
browser will just print the accepted mime types instead of the record
itself.

To test this with a tool (like Postman) you should enable
RESTBasicAuthe and make the tool use Basic authentication with valid
credentials. And you need to specify any of the following strings on the
Accept header:
- application/marcxml+xml
- application/marc-in-json
- application/marc
- text/plain

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 20 Martin Renvoize 2020-04-27 10:18:29 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 21 Joy Nelson 2020-05-08 20:47:19 UTC
feature not backported to 19.11.x
Comment 22 Lucas Gass 2020-06-08 15:48:59 UTC
Is there any possibility that this could be backported to 19.11? Would be very helpful to have biblios endpoint accessible in the OPAC.
Comment 23 Aleisha Amohia 2020-06-14 23:49:04 UTC
backported to 19.11.x for 19.11.07
Comment 24 Tomás Cohen Arazi 2020-06-17 12:53:02 UTC
@aleisha: you should consider backporting bug 25045 and bug 25327 as well. It is your call, but we found out bug 25045 would be required during the QA round to allow disabling anonymous access.
Comment 25 Victor Grousset/tuxayo 2020-06-17 21:47:32 UTC
Enhancement not backported to oldoldstable (19.05.x)

Also missing dependency: bug 23677
Comment 26 Aleisha Amohia 2020-06-18 00:17:21 UTC
(In reply to Tomás Cohen Arazi from comment #24)
> @aleisha: you should consider backporting bug 25045 and bug 25327 as well.
> It is your call, but we found out bug 25045 would be required during the QA
> round to allow disabling anonymous access.

not backporting 25045 to 19.11.x due to the db update