Bug 20568 - Add API key management interface for patrons
Summary: Add API key management interface for patrons
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Julian Maurice
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 20624 20627 20638 20612 20803
  Show dependency treegraph
 
Reported: 2018-04-11 22:08 UTC by Tomás Cohen Arazi
Modified: 2018-07-18 12:13 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Adds the ability to handle patron-level API keys to be used for authenticating the REST API.


Attachments
Bug 20568: API keys management in interface (22.99 KB, patch)
2018-04-13 20:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20568: Add mandatory description field for api keys (18.22 KB, patch)
2018-04-13 20:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20568: API keys management in interface (22.99 KB, patch)
2018-04-16 13:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20568: Add mandatory description field for api keys (19.80 KB, patch)
2018-04-16 13:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20568: Unit tests (4.15 KB, patch)
2018-04-16 13:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20568: API key management for OPAC users (14.89 KB, patch)
2018-04-16 13:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20568: API keys management in interface (22.99 KB, patch)
2018-04-16 13:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20568: Add mandatory description field for api keys (19.79 KB, patch)
2018-04-16 13:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20568: Unit tests (4.15 KB, patch)
2018-04-16 13:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20568: API key management for OPAC users (15.95 KB, patch)
2018-04-16 13:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20568 (follow-up) Interface and markup changes (19.89 KB, patch)
2018-04-16 15:58 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 20568: API keys management in interface (22.99 KB, patch)
2018-04-16 19:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20568: Add mandatory description field for api keys (20.56 KB, patch)
2018-04-16 19:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20568: Unit tests (3.37 KB, patch)
2018-04-16 19:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20568: API key management for OPAC users (15.95 KB, patch)
2018-04-16 19:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20568 (follow-up) Interface and markup changes (19.94 KB, patch)
2018-04-16 19:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20568: Move value => client_id + secret (12.20 KB, patch)
2018-04-16 19:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20568: API keys management in interface (23.13 KB, patch)
2018-04-17 14:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20568: Add mandatory description field for api keys (20.69 KB, patch)
2018-04-17 14:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20568: Unit tests (3.45 KB, patch)
2018-04-17 14:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20568: API key management for OPAC users (16.06 KB, patch)
2018-04-17 14:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20568 (follow-up) Interface and markup changes (20.03 KB, patch)
2018-04-17 14:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20568: Move value => client_id + secret (12.31 KB, patch)
2018-04-17 14:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20568: (follow-up) Interface and markup changes (20.05 KB, patch)
2018-04-17 19:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20568: Move value => client_id + secret (12.31 KB, patch)
2018-04-17 19:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20568: (follow-up) Atomic update fix (2.62 KB, patch)
2018-04-18 13:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20568: (follow-up) Atomic update fix (2.61 KB, patch)
2018-04-18 15:47 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20568: (QA follow-up) Test client_id and secret are not overwritten (1.62 KB, patch)
2018-04-18 16:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20568: (QA follow-up) Make sure client_id and secret are not overwritten on store (951 bytes, patch)
2018-04-18 16:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29568: (QA follow-up) Get rid of the id column (5.01 KB, patch)
2018-04-18 17:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20568: (QA follow-up) Get rid of the id column (5.01 KB, patch)
2018-04-18 17:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20568: (QA follow-up) Get rid of the id column (7.48 KB, patch)
2018-04-18 17:33 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20568: CSRF protection (7.24 KB, patch)
2018-04-18 17:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20568: CSRF protection (7.26 KB, patch)
2018-04-18 18:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20568: API keys management in interface (23.17 KB, patch)
2018-04-18 18:12 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20568: Add mandatory description field for api keys (20.73 KB, patch)
2018-04-18 18:13 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20568: Unit tests (3.49 KB, patch)
2018-04-18 18:13 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20568: API key management for OPAC users (16.11 KB, patch)
2018-04-18 18:13 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20568: (follow-up) Interface and markup changes (20.13 KB, patch)
2018-04-18 18:13 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20568: Move value => client_id + secret (12.40 KB, patch)
2018-04-18 18:13 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20568: (follow-up) Atomic update fix (2.67 KB, patch)
2018-04-18 18:13 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20568: (QA follow-up) Test client_id and secret are not overwritten (1.68 KB, patch)
2018-04-18 18:13 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20568: (QA follow-up) Make sure client_id and secret are not overwritten on store (1011 bytes, patch)
2018-04-18 18:13 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20568: (QA follow-up) Get rid of the id column (7.56 KB, patch)
2018-04-18 18:13 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20568: CSRF protection (7.34 KB, patch)
2018-04-18 18:13 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20568: (QA follow-up) Remove api-key management from OPAC (13.49 KB, patch)
2018-05-02 14:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20568: (QA follow-up) Remove api-key management from OPAC (14.30 KB, patch)
2018-05-02 14:13 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20568: (QA follow-up) Remove unused vars (740 bytes, patch)
2018-05-03 18:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20568: (follow-up) API keys management in interface (1.11 KB, patch)
2018-05-08 19:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20568: fix shebang (530 bytes, patch)
2018-05-08 19:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20568: Fix bad resolution conflict with bug 18403 (1.65 KB, patch)
2018-05-08 19:31 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 2018-04-11 22:08:09 UTC
Bug 20402 implements an Oauth2 server for the client credentials grant. It should be awesome if the patron management interface in Koha provided a way for staff members (and maybe for patron users later) to create and revoke API keys.

The work is actually done already, by Julian in the first commit for bug 13920. So we need to recover that patch, adapt it to our coding guidelines and make it work on top of bug 20402!
Comment 1 David Cook 2018-04-11 23:37:48 UTC
Looking forward to this. I've wanted this for a while.
Comment 2 Tomás Cohen Arazi 2018-04-13 20:18:15 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2018-04-13 20:18:20 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2018-04-13 20:20:33 UTC
I've attached my WIP stuff just in case anyone wants to comment on that.
I basically resurrected one patch from Julian from bug 13920 (API keys management in interface) and worked on adapting it to current practices.

The staff interface api management page is fully functional. OPAC is next.
Unit tests almost ready, not submitted.
Comment 5 Tomás Cohen Arazi 2018-04-16 13:06:06 UTC Comment hidden (obsolete)
Comment 6 Tomás Cohen Arazi 2018-04-16 13:06:09 UTC Comment hidden (obsolete)
Comment 7 Tomás Cohen Arazi 2018-04-16 13:06:12 UTC Comment hidden (obsolete)
Comment 8 Tomás Cohen Arazi 2018-04-16 13:06:15 UTC Comment hidden (obsolete)
Comment 9 Tomás Cohen Arazi 2018-04-16 13:21:00 UTC Comment hidden (obsolete)
Comment 10 Tomás Cohen Arazi 2018-04-16 13:21:07 UTC Comment hidden (obsolete)
Comment 11 Tomás Cohen Arazi 2018-04-16 13:21:13 UTC Comment hidden (obsolete)
Comment 12 Tomás Cohen Arazi 2018-04-16 13:21:18 UTC Comment hidden (obsolete)
Comment 13 Owen Leonard 2018-04-16 15:58:39 UTC Comment hidden (obsolete)
Comment 14 Tomás Cohen Arazi 2018-04-16 19:09:09 UTC Comment hidden (obsolete)
Comment 15 Tomás Cohen Arazi 2018-04-16 19:09:14 UTC Comment hidden (obsolete)
Comment 16 Tomás Cohen Arazi 2018-04-16 19:09:19 UTC Comment hidden (obsolete)
Comment 17 Tomás Cohen Arazi 2018-04-16 19:09:24 UTC Comment hidden (obsolete)
Comment 18 Tomás Cohen Arazi 2018-04-16 19:09:28 UTC Comment hidden (obsolete)
Comment 19 Tomás Cohen Arazi 2018-04-16 19:09:34 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2018-04-17 14:50:31 UTC
Created attachment 74343 [details] [review]
Bug 20568: API keys management in interface

This introduces the concept of API keys for use in the new REST API.
A key is a string of 32 alphanumerical characters (32 is purely
arbitrary, it can be changed easily).
A user can have multiple keys (unlimited at the moment)
Keys can be generated automatically, and then we have the possibility to
delete or revoke each one individually.

Test plan:
1/ Go to staff interface
2/ Go to a borrower page
3/ In toolbar, click on More -> Manage API keys
4/ Click on "Generate new key" multiple times, check that they are
   correctly displayed under the button, and they are active by default
5/ Revoke some keys, check that they are not active anymore
6/ Delete some keys, check that they disappear from table
7/ Go to opac interface, log in
8/ In your user account pages, you now have a new tab to the left "your
   API keys". Click on it.
9/ Repeat steps 4-6

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Kyle M Hall 2018-04-17 14:50:38 UTC
Created attachment 74344 [details] [review]
Bug 20568: Add mandatory description field for api keys

This patch changes the table structure adding fields usually found on
this kind of api management pages.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Kyle M Hall 2018-04-17 14:50:41 UTC
Created attachment 74345 [details] [review]
Bug 20568: Unit tests

This patch adds unit tests for the introduced classes: Koha::ApiKey(s).

To test:
- Apply this patch
- Run
  $ kshell
 k$ prove t/db_dependent/Koha/ApiKeys.t
=> FAIL: Tests fail because the feature is not implemented.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Kyle M Hall 2018-04-17 14:50:44 UTC
Created attachment 74346 [details] [review]
Bug 20568: API key management for OPAC users

This patch makes the OPAC interface for API keys management work
with the new lib. Verify all actions work for a logged user.

Users without login should be redirected to an error page.

The AllowPatronsManageAPIKeysInOPAC syspref is added to control if the
OPAC feature is enabled or not.

To test:
- Verify the syspref works
- Verify users can manage their API keys

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 24 Kyle M Hall 2018-04-17 14:50:47 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2018-04-17 14:50:50 UTC Comment hidden (obsolete)
Comment 26 Tomás Cohen Arazi 2018-04-17 19:40:08 UTC
Created attachment 74388 [details] [review]
Bug 20568: (follow-up) Interface and markup changes

This patch makes some interface changes to bring things better in line
with existing interface patterns. This patch also re-indents the
modified templates with 4 spaces instead of 2 and makes <input>s
self-closing.

Also changed: Corrected system preference check in opac-apikeys.pl.

To test, apply the patch and:

In the staff client:

 - Open a patron record and choose More -> Manage API keys.
   - There should be a standard message dialog containing a link to
     "Generate a new key."
     - Clicking the link should show the form for adding a new key.
     - Test that clicking the "Cancel" link hides the form.
     - Test that creating the new key works correctly.
   - You should now see a table showing existing keys and a "Generate a
     new key" button above it.
     - Test that the "Delete" button asks for confirmation, and that
       confirming and denying both work correctly.
     - Test that "Revoke" and "Activate" actions still work correctly.

In the OPAC:

 - Set the AllowPatronsManageAPIKeysInOPAC system preference to "Allow."
 - Log in to the OPAC and click the "your API keys" link in the sidebar.
   - Clicking the "Generate new key" button should display the form for
     adding a new key.
     - Clicking the "cancel" link should hide the form.
     - Submitting the form should add a new key.
   - You should now see a table showing existing keys.
     - Test that the "Delete" link asks for confirmation, and that
       confirming and denying both work correctly.
     - Test that "Revoke" and "Activate" actions still work correctly.
 - Set the AllowPatronsManageAPIKeysInOPAC system preference to "Don't
   allow."
   - Log in to the OPAC and confirm that the "your API keys" link in the
     sidebar is no longer visible.
     - Confirm that navigating directly to /cgi-bin/koha/opac-apikeys.pl
       results in a 404 error.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 27 Tomás Cohen Arazi 2018-04-17 19:40:19 UTC
Created attachment 74389 [details] [review]
Bug 20568: Move value => client_id + secret

This patch addresses the request from Julian that api keys are expected
to be client id/secret pairs.

It does so by
- Adding 'client_id' and 'secret' columns
- Removing 'value'

Tests got adjusted and so controller scripts and templates.
Both libs and tests changes have been squashed. This ones remain in
order to keep Owen's attribution on the template changes and avoid
rebase conflicts.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 28 Tomás Cohen Arazi 2018-04-18 13:56:07 UTC
Created attachment 74434 [details] [review]
Bug 20568: (follow-up) Atomic update fix

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 29 Julian Maurice 2018-04-18 15:47:17 UTC
Created attachment 74454 [details] [review]
Bug 20568: (follow-up) Atomic update fix

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 30 Julian Maurice 2018-04-18 16:23:54 UTC
QA comments:

1) The UUID generation happens in the store method. This means that every time I revoke/activate a key, client_id and secret are changed. I think that is not the desired behaviour.
2) A CSRF attack can remove and revoke keys. This can be prevented by using Koha::Token.
3) UUID::generate($uuid); UUID::unparse( $uuid, $uuidstring ); can be replaced by $uuidstring = uuid();
4) In intranet, if I have no key and click twice on "Generate a new client id/secret pair" link, the form appear then disappear. Not really a problem, but it's an odd behaviour. I suggest to just show the button even if I have no keys, like in OPAC.
5) I feel that api_keys.id is not useful as client_id can be used as a primary key
6) Since we're using UUID, why not store the binary representation instead of the string representation ? I don't know how that would affect performance but that will save storage space. If we stick with the string representation I suggest that we change the data type of client_id and secret to a fixed-width string type (CHAR(36) ?)

Failing QA for 1 and 2.
3 and 4 are not required but they should be easy to do.
5 and 6 are just thoughts. Please comment.
Comment 31 Tomás Cohen Arazi 2018-04-18 16:57:44 UTC
Created attachment 74476 [details] [review]
Bug 20568: (QA follow-up) Test client_id and secret are not overwritten

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 32 Tomás Cohen Arazi 2018-04-18 16:57:50 UTC
Created attachment 74477 [details] [review]
Bug 20568: (QA follow-up) Make sure client_id and secret are not overwritten on store

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 33 Tomás Cohen Arazi 2018-04-18 17:10:03 UTC Comment hidden (obsolete)
Comment 34 Tomás Cohen Arazi 2018-04-18 17:11:12 UTC Comment hidden (obsolete)
Comment 35 Tomás Cohen Arazi 2018-04-18 17:33:54 UTC
Created attachment 74480 [details] [review]
Bug 20568: (QA follow-up) Get rid of the id column

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 36 Tomás Cohen Arazi 2018-04-18 17:39:26 UTC
Created attachment 74481 [details] [review]
Bug 20568: CSRF protection

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 37 Tomás Cohen Arazi 2018-04-18 17:58:26 UTC
(In reply to Julian Maurice from comment #30)
> QA comments:
> 
> 1) The UUID generation happens in the store method. This means that every
> time I revoke/activate a key, client_id and secret are changed. I think that
> is not the desired behaviour.

Done. I provided both regression tests for this situation and the fix.

> 2) A CSRF attack can remove and revoke keys. This can be prevented by using
> Koha::Token.

Done. Good catch :-D

> 3) UUID::generate($uuid); UUID::unparse( $uuid, $uuidstring ); can be
> replaced by $uuidstring = uuid();

I've had issues with Jessie's version of UUID (0.05). I get:

Error: "uuid" is not exported buy the UUID module

> 4) In intranet, if I have no key and click twice on "Generate a new client
> id/secret pair" link, the form appear then disappear. Not really a problem,
> but it's an odd behaviour. I suggest to just show the button even if I have
> no keys, like in OPAC.

Right, I will think about it. Maybe Owen can answer about this.

> 5) I feel that api_keys.id is not useful as client_id can be used as a
> primary key

You are right! Removed!

> 6) Since we're using UUID, why not store the binary representation instead
> of the string representation ? I don't know how that would affect
> performance but that will save storage space. If we stick with the string
> representation I suggest that we change the data type of client_id and
> secret to a fixed-width string type (CHAR(36) ?)

I don't think this is necessary. For this client_id/secret pairs as are not expected be be many. Also, VARCHAR only takes as much space as the size of the content.
Comment 38 Tomás Cohen Arazi 2018-04-18 18:00:26 UTC
Created attachment 74482 [details] [review]
Bug 20568: CSRF protection

Edit: fix warning introduced by this patch

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 39 Julian Maurice 2018-04-18 18:12:59 UTC
Created attachment 74486 [details] [review]
Bug 20568: API keys management in interface

This introduces the concept of API keys for use in the new REST API.
A key is a string of 32 alphanumerical characters (32 is purely
arbitrary, it can be changed easily).
A user can have multiple keys (unlimited at the moment)
Keys can be generated automatically, and then we have the possibility to
delete or revoke each one individually.

Test plan:
1/ Go to staff interface
2/ Go to a borrower page
3/ In toolbar, click on More -> Manage API keys
4/ Click on "Generate new key" multiple times, check that they are
   correctly displayed under the button, and they are active by default
5/ Revoke some keys, check that they are not active anymore
6/ Delete some keys, check that they disappear from table
7/ Go to opac interface, log in
8/ In your user account pages, you now have a new tab to the left "your
   API keys". Click on it.
9/ Repeat steps 4-6

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 40 Julian Maurice 2018-04-18 18:13:04 UTC
Created attachment 74487 [details] [review]
Bug 20568: Add mandatory description field for api keys

This patch changes the table structure adding fields usually found on
this kind of api management pages.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 41 Julian Maurice 2018-04-18 18:13:09 UTC
Created attachment 74488 [details] [review]
Bug 20568: Unit tests

This patch adds unit tests for the introduced classes: Koha::ApiKey(s).

To test:
- Apply this patch
- Run
  $ kshell
 k$ prove t/db_dependent/Koha/ApiKeys.t
=> FAIL: Tests fail because the feature is not implemented.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 42 Julian Maurice 2018-04-18 18:13:14 UTC
Created attachment 74489 [details] [review]
Bug 20568: API key management for OPAC users

This patch makes the OPAC interface for API keys management work
with the new lib. Verify all actions work for a logged user.

Users without login should be redirected to an error page.

The AllowPatronsManageAPIKeysInOPAC syspref is added to control if the
OPAC feature is enabled or not.

To test:
- Verify the syspref works
- Verify users can manage their API keys

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 43 Julian Maurice 2018-04-18 18:13:20 UTC
Created attachment 74490 [details] [review]
Bug 20568: (follow-up) Interface and markup changes

This patch makes some interface changes to bring things better in line
with existing interface patterns. This patch also re-indents the
modified templates with 4 spaces instead of 2 and makes <input>s
self-closing.

Also changed: Corrected system preference check in opac-apikeys.pl.

To test, apply the patch and:

In the staff client:

 - Open a patron record and choose More -> Manage API keys.
   - There should be a standard message dialog containing a link to
     "Generate a new key."
     - Clicking the link should show the form for adding a new key.
     - Test that clicking the "Cancel" link hides the form.
     - Test that creating the new key works correctly.
   - You should now see a table showing existing keys and a "Generate a
     new key" button above it.
     - Test that the "Delete" button asks for confirmation, and that
       confirming and denying both work correctly.
     - Test that "Revoke" and "Activate" actions still work correctly.

In the OPAC:

 - Set the AllowPatronsManageAPIKeysInOPAC system preference to "Allow."
 - Log in to the OPAC and click the "your API keys" link in the sidebar.
   - Clicking the "Generate new key" button should display the form for
     adding a new key.
     - Clicking the "cancel" link should hide the form.
     - Submitting the form should add a new key.
   - You should now see a table showing existing keys.
     - Test that the "Delete" link asks for confirmation, and that
       confirming and denying both work correctly.
     - Test that "Revoke" and "Activate" actions still work correctly.
 - Set the AllowPatronsManageAPIKeysInOPAC system preference to "Don't
   allow."
   - Log in to the OPAC and confirm that the "your API keys" link in the
     sidebar is no longer visible.
     - Confirm that navigating directly to /cgi-bin/koha/opac-apikeys.pl
       results in a 404 error.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 44 Julian Maurice 2018-04-18 18:13:25 UTC
Created attachment 74491 [details] [review]
Bug 20568: Move value => client_id + secret

This patch addresses the request from Julian that api keys are expected
to be client id/secret pairs.

It does so by
- Adding 'client_id' and 'secret' columns
- Removing 'value'

Tests got adjusted and so controller scripts and templates.
Both libs and tests changes have been squashed. This ones remain in
order to keep Owen's attribution on the template changes and avoid
rebase conflicts.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 45 Julian Maurice 2018-04-18 18:13:30 UTC
Created attachment 74492 [details] [review]
Bug 20568: (follow-up) Atomic update fix

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 46 Julian Maurice 2018-04-18 18:13:35 UTC
Created attachment 74493 [details] [review]
Bug 20568: (QA follow-up) Test client_id and secret are not overwritten

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 47 Julian Maurice 2018-04-18 18:13:39 UTC
Created attachment 74494 [details] [review]
Bug 20568: (QA follow-up) Make sure client_id and secret are not overwritten on store

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 48 Julian Maurice 2018-04-18 18:13:44 UTC
Created attachment 74495 [details] [review]
Bug 20568: (QA follow-up) Get rid of the id column

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 49 Julian Maurice 2018-04-18 18:13:49 UTC
Created attachment 74496 [details] [review]
Bug 20568: CSRF protection

Edit: fix warning introduced by this patch

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 50 Tomás Cohen Arazi 2018-05-02 14:06:41 UTC
Created attachment 74980 [details] [review]
Bug 20568: (QA follow-up) Remove api-key management from OPAC

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 51 Tomás Cohen Arazi 2018-05-02 14:13:12 UTC
Created attachment 74981 [details] [review]
Bug 20568: (QA follow-up) Remove api-key management from OPAC

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 52 Tomás Cohen Arazi 2018-05-03 18:52:10 UTC
Created attachment 75023 [details] [review]
Bug 20568: (QA follow-up) Remove unused vars

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 53 Jonathan Druart 2018-05-08 19:16:25 UTC
Created attachment 75186 [details] [review]
Bug 20568: (follow-up) API keys management in interface

Fix bad resolution conflict.
Comment 54 Jonathan Druart 2018-05-08 19:28:09 UTC
Created attachment 75187 [details] [review]
Bug 20568: fix shebang
Comment 55 Jonathan Druart 2018-05-08 19:31:25 UTC
Created attachment 75188 [details] [review]
Bug 20568: Fix bad resolution conflict with bug 18403

borrowers module permission has now several subpermissions
Comment 56 Jonathan Druart 2018-05-09 15:57:37 UTC
Pushed to master for 18.05, thanks to everybody involved!