Bug 20612

Summary: Make OAuth2 use patron's client_id/secret pairs
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: AuthenticationAssignee: Tomás Cohen Arazi <tomascohen>
Status: Pushed to Master --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: benjamin.rokseth, dpavlin, jonathan.druart, julian.maurice, martin.renvoize
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20734
Whiteboard:
Change sponsored?: Sponsored Patch complexity: ---
Bot Control: --- When did the bot last check this:
Who signed the patch off: Text to go in the release notes:
Bug Depends on: 20402, 20568    
Bug Blocks: 20624, 20627, 20733    
Attachments: Bug 20612: Unit tests
Bug 20612: Make OAuth2 use patron's client_id/secret pairs
Bug 20612: koha-conf.xml cleanup
Bug 20612: Unit tests
Bug 20612: Make OAuth2 use patron's client_id/secret pairs
Bug 20612: koha-conf.xml cleanup
Bug 20612: Unit tests
Bug 20612: Make OAuth2 use patron's client_id/secret pairs
Bug 20612: koha-conf.xml cleanup
Bug 20612: Unit tests
Bug 20612: Make OAuth2 use patron's client_id/secret pairs
Bug 20612: koha-conf.xml cleanup
Bug 20612: Unit tests
Bug 20612: Unit tests
Bug 20612: Make OAuth2 use patron's client_id/secret pairs
Bug 20612: koha-conf.xml cleanup
Bug 20612: Unit tests
Bug 20612: Make OAuth2 use patron's client_id/secret pairs
Bug 20612: koha-conf.xml cleanup
Bug 20612: (QA follow-up) Fix typo in method POD
Bug 20612: Unit tests
Bug 20612: Make OAuth2 use patron's client_id/secret pairs
Bug 20612: koha-conf.xml cleanup
Bug 20612: (QA follow-up) Fix typo in method POD

Description Tomás Cohen Arazi 2018-04-18 16:32:04 UTC
Bug 20568 implements a way to define client_id/secret API keys for patrons.
We should use those instead of the hardcoded ones in koha-conf.xml
Comment 1 Tomás Cohen Arazi 2018-04-18 16:42:30 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2018-04-18 16:42:38 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2018-04-18 16:42:44 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2018-04-18 18:07:05 UTC
Created attachment 74483 [details] [review]
Bug 20612: Unit tests

This patch makes the oauth.t tests leverage on the new Koha::ApiKey(s)
classes. It adds tests for expired tokens too.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/acquisitions_orders.t
=> FAIL: Tests should fail without the rest of the patches.

Sponsored-by: ByWater Solutions
Comment 5 Tomás Cohen Arazi 2018-04-18 18:07:11 UTC
Created attachment 74484 [details] [review]
Bug 20612: Make OAuth2 use patron's client_id/secret pairs

This patch wires the OAuth related code so it leverages on the new
Koha::ApiKey(s) classes and tools introduced by bug 20568 instead of the
hardcoded entries in koha-conf.xml originally proposed by bug 20402.

To test revisit the test plan for bug 20402, and verify that it works.
But create API key pairs instead of writing them down in koha-conf.xml.
Also:
- Run:
  $ prove t/db_dependent/api/v1/oauth.t
=> SUCCESS: Tests pass!
- Sign off :-D

Sponsored-by: ByWater Solutions
Comment 6 Tomás Cohen Arazi 2018-04-18 18:07:16 UTC
Created attachment 74485 [details] [review]
Bug 20612: koha-conf.xml cleanup
Comment 7 Julian Maurice 2018-04-18 18:27:11 UTC
Created attachment 74497 [details] [review]
Bug 20612: Unit tests

This patch makes the oauth.t tests leverage on the new Koha::ApiKey(s)
classes. It adds tests for expired tokens too.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/oauth.t
=> FAIL: Tests should fail without the rest of the patches.

Sponsored-by: ByWater Solutions
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 8 Julian Maurice 2018-04-18 18:27:15 UTC
Created attachment 74498 [details] [review]
Bug 20612: Make OAuth2 use patron's client_id/secret pairs

This patch wires the OAuth related code so it leverages on the new
Koha::ApiKey(s) classes and tools introduced by bug 20568 instead of the
hardcoded entries in koha-conf.xml originally proposed by bug 20402.

To test revisit the test plan for bug 20402, and verify that it works.
But create API key pairs instead of writing them down in koha-conf.xml.
Also:
- Run:
  $ prove t/db_dependent/api/v1/oauth.t
=> SUCCESS: Tests pass!
- Sign off :-D

Sponsored-by: ByWater Solutions
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 9 Julian Maurice 2018-04-18 18:27:19 UTC
Created attachment 74499 [details] [review]
Bug 20612: koha-conf.xml cleanup

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 10 Benjamin Rokseth 2018-04-19 09:19:39 UTC
Created attachment 74521 [details] [review]
Bug 20612: Unit tests

This patch makes the oauth.t tests leverage on the new Koha::ApiKey(s)
classes. It adds tests for expired tokens too.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/oauth.t
=> FAIL: Tests should fail without the rest of the patches.

Sponsored-by: ByWater Solutions
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Comment 11 Benjamin Rokseth 2018-04-19 09:19:45 UTC
Created attachment 74522 [details] [review]
Bug 20612: Make OAuth2 use patron's client_id/secret pairs

This patch wires the OAuth related code so it leverages on the new
Koha::ApiKey(s) classes and tools introduced by bug 20568 instead of the
hardcoded entries in koha-conf.xml originally proposed by bug 20402.

To test revisit the test plan for bug 20402, and verify that it works.
But create API key pairs instead of writing them down in koha-conf.xml.
Also:
- Run:
  $ prove t/db_dependent/api/v1/oauth.t
=> SUCCESS: Tests pass!
- Sign off :-D

Sponsored-by: ByWater Solutions
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Comment 12 Benjamin Rokseth 2018-04-19 09:19:50 UTC
Created attachment 74523 [details] [review]
Bug 20612: koha-conf.xml cleanup

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Comment 13 Kyle M Hall 2018-04-19 10:08:18 UTC
Created attachment 74531 [details] [review]
Bug 20612: Unit tests

This patch makes the oauth.t tests leverage on the new Koha::ApiKey(s)
classes. It adds tests for expired tokens too.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/oauth.t
=> FAIL: Tests should fail without the rest of the patches.

Sponsored-by: ByWater Solutions
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 14 Kyle M Hall 2018-04-19 10:08:47 UTC
Created attachment 74532 [details] [review]
Bug 20612: Unit tests

This patch makes the oauth.t tests leverage on the new Koha::ApiKey(s)
classes. It adds tests for expired tokens too.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/oauth.t
=> FAIL: Tests should fail without the rest of the patches.

Sponsored-by: ByWater Solutions
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Kyle M Hall 2018-04-19 10:08:54 UTC
Created attachment 74533 [details] [review]
Bug 20612: Make OAuth2 use patron's client_id/secret pairs

This patch wires the OAuth related code so it leverages on the new
Koha::ApiKey(s) classes and tools introduced by bug 20568 instead of the
hardcoded entries in koha-conf.xml originally proposed by bug 20402.

To test revisit the test plan for bug 20402, and verify that it works.
But create API key pairs instead of writing them down in koha-conf.xml.
Also:
- Run:
  $ prove t/db_dependent/api/v1/oauth.t
=> SUCCESS: Tests pass!
- Sign off :-D

Sponsored-by: ByWater Solutions
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Kyle M Hall 2018-04-19 10:08:56 UTC
Created attachment 74534 [details] [review]
Bug 20612: koha-conf.xml cleanup

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Tomás Cohen Arazi 2018-04-19 13:08:59 UTC
Created attachment 74550 [details] [review]
Bug 20612: Unit tests

This patch makes the oauth.t tests leverage on the new Koha::ApiKey(s)
classes. It adds tests for expired tokens too.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/oauth.t
=> FAIL: Tests should fail without the rest of the patches.

Sponsored-by: ByWater Solutions

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Tomás Cohen Arazi 2018-04-19 13:09:04 UTC
Created attachment 74551 [details] [review]
Bug 20612: Make OAuth2 use patron's client_id/secret pairs

This patch wires the OAuth related code so it leverages on the new
Koha::ApiKey(s) classes and tools introduced by bug 20568 instead of the
hardcoded entries in koha-conf.xml originally proposed by bug 20402.

To test revisit the test plan for bug 20402, and verify that it works.
But create API key pairs instead of writing them down in koha-conf.xml.
Also:
- Run:
  $ prove t/db_dependent/api/v1/oauth.t
=> SUCCESS: Tests pass!
- Sign off :-D

Sponsored-by: ByWater Solutions

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Tomás Cohen Arazi 2018-04-19 13:09:10 UTC
Created attachment 74552 [details] [review]
Bug 20612: koha-conf.xml cleanup

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 20 Jonathan Druart 2018-04-19 16:43:17 UTC
Please write a wiki page about these changes.
Comment 21 Jonathan Druart 2018-04-19 17:29:09 UTC
Could we have a global switch to turn on/off the whole REST API stuffs?
Comment 22 Jonathan Druart 2018-04-19 17:32:33 UTC
Should we really remove a deleted key from the DB table?
I would flag it as inactive instead, it would permit to keep track of the different requests and associate it to a user.
Disclaimer: This may be a very stupid idea.
Comment 23 Tomás Cohen Arazi 2018-04-19 17:34:12 UTC
(In reply to Jonathan Druart from comment #22)
> Should we really remove a deleted key from the DB table?
> I would flag it as inactive instead, it would permit to keep track of the
> different requests and associate it to a user.
> Disclaimer: This may be a very stupid idea.

I thought about it too. That would mean an extra column: deleted TINYINT (1), and it makes sense.
Comment 24 Jonathan Druart 2018-04-19 18:05:34 UTC
(In reply to Jonathan Druart from comment #21)
> Could we have a global switch to turn on/off the whole REST API stuffs?

s/REST API/OAuth server
Comment 25 Tomás Cohen Arazi 2018-05-03 18:55:01 UTC
Created attachment 75024 [details] [review]
Bug 20612: (QA follow-up) Fix typo in method POD

As Martin correctly highlighted, the method name is not correctly
spelled in POD. This patch fixes it.

To test:
- Look carefully
=> FAIL: Method name is _verify_client_cb but POD says _verify_client_db
- Apply this patch
- Look carefully
=> SUCCESS: Notice the POD is fixed!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 26 Tomás Cohen Arazi 2018-05-03 19:10:04 UTC
Created attachment 75034 [details] [review]
Bug 20612: Unit tests

This patch makes the oauth.t tests leverage on the new Koha::ApiKey(s)
classes. It adds tests for expired tokens too.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/oauth.t
=> FAIL: Tests should fail without the rest of the patches.

Sponsored-by: ByWater Solutions

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 27 Tomás Cohen Arazi 2018-05-03 19:10:10 UTC
Created attachment 75035 [details] [review]
Bug 20612: Make OAuth2 use patron's client_id/secret pairs

This patch wires the OAuth related code so it leverages on the new
Koha::ApiKey(s) classes and tools introduced by bug 20568 instead of the
hardcoded entries in koha-conf.xml originally proposed by bug 20402.

To test revisit the test plan for bug 20402, and verify that it works.
But create API key pairs instead of writing them down in koha-conf.xml.
Also:
- Run:
  $ prove t/db_dependent/api/v1/oauth.t
=> SUCCESS: Tests pass!
- Sign off :-D

Sponsored-by: ByWater Solutions

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 28 Tomás Cohen Arazi 2018-05-03 19:10:16 UTC
Created attachment 75036 [details] [review]
Bug 20612: koha-conf.xml cleanup

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 29 Tomás Cohen Arazi 2018-05-03 19:10:22 UTC
Created attachment 75037 [details] [review]
Bug 20612: (QA follow-up) Fix typo in method POD

As Martin correctly highlighted, the method name is not correctly
spelled in POD. This patch fixes it.

To test:
- Look carefully
=> FAIL: Method name is _verify_client_cb but POD says _verify_client_db
- Apply this patch
- Look carefully
=> SUCCESS: Notice the POD is fixed!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 30 Jonathan Druart 2018-05-08 19:48:38 UTC
(In reply to Jonathan Druart from comment #20)
> Please write a wiki page about these changes.

See https://wiki.koha-community.org/wiki/APIs_and_protocols_supported_by_Koha#OAuth2
Comment 31 Jonathan Druart 2018-05-08 19:50:30 UTC
(In reply to Tomás Cohen Arazi from comment #23)
> (In reply to Jonathan Druart from comment #22)
> > Should we really remove a deleted key from the DB table?
> > I would flag it as inactive instead, it would permit to keep track of the
> > different requests and associate it to a user.
> > Disclaimer: This may be a very stupid idea.
> 
> I thought about it too. That would mean an extra column: deleted TINYINT
> (1), and it makes sense.

See bug 20733.
Comment 32 Jonathan Druart 2018-05-08 19:53:36 UTC
Please take care of bug 20734 before the end of the week.
Comment 33 Tomás Cohen Arazi 2018-05-09 15:11:54 UTC
(In reply to Jonathan Druart from comment #30)
> (In reply to Jonathan Druart from comment #20)
> > Please write a wiki page about these changes.
> 
> See
> https://wiki.koha-community.org/wiki/
> APIs_and_protocols_supported_by_Koha#OAuth2

Done.
Comment 34 Tomás Cohen Arazi 2018-05-09 15:12:08 UTC
(In reply to Jonathan Druart from comment #32)
> Please take care of bug 20734 before the end of the week.

Done!
Comment 35 Jonathan Druart 2018-05-09 15:57:43 UTC
Pushed to master for 18.05, thanks to everybody involved!