Bug 37711 - IdP auto-register should work on the staff interface
Summary: IdP auto-register should work on the staff interface
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-08-22 14:49 UTC by Tomás Cohen Arazi (tcohen)
Modified: 2024-11-06 22:53 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 37711: DB update (4.14 KB, patch)
2024-08-23 15:29 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 37711: DBIC schema (2.82 KB, patch)
2024-08-23 15:29 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 37711: API changes (3.02 KB, patch)
2024-08-23 15:29 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 37711: Domains CRUD update (9.71 KB, patch)
2024-08-23 15:29 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 37711: Add support for staff interface auto-register on IdP (10.37 KB, patch)
2024-08-23 15:29 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 37711: DB update (4.21 KB, patch)
2024-11-06 17:25 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 37711: DBIC schema (2.88 KB, patch)
2024-11-06 17:25 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 37711: API changes (3.08 KB, patch)
2024-11-06 17:25 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 37711: Domains CRUD update (9.78 KB, patch)
2024-11-06 17:25 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 37711: Add support for staff interface auto-register on IdP (10.44 KB, patch)
2024-11-06 17:25 UTC, Martin Renvoize (ashimema)
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 (tcohen) 2024-08-22 14:49:58 UTC
Right now it only works if the login happens in the OPAC.
Comment 1 Tomás Cohen Arazi (tcohen) 2024-08-23 15:29:18 UTC
Created attachment 170643 [details] [review]
Bug 37711: DB update
Comment 2 Tomás Cohen Arazi (tcohen) 2024-08-23 15:29:20 UTC
Created attachment 170644 [details] [review]
Bug 37711: DBIC schema
Comment 3 Tomás Cohen Arazi (tcohen) 2024-08-23 15:29:23 UTC
Created attachment 170645 [details] [review]
Bug 37711: API changes

This patch makes the API representation of the identity providers
domains match the new metadata.
Comment 4 Tomás Cohen Arazi (tcohen) 2024-08-23 15:29:25 UTC
Created attachment 170646 [details] [review]
Bug 37711: Domains CRUD update

This patch adapts the `Identity provider domains` CRUD page to handle
the new configuration options.

To test:
1. Have an IdP with some domains set. Make sure there's at least one
   domain with auto-register enabled and one with it disabled.
2. Apply this patches
3. Run:
   $ ktd --shell
  k$ updatedatabase
=> SUCCESS: Updates successfully
4. Run:
  k$ yarn build
5. Refresh the IdP domains page
=> SUCCESS: The domains table shows both OPAC and staff auto-register
   options.
=> SUCCESS: The staff auto-register is disabled for both domains
=> SUCCESS: The OPAC auto-register matches what was set for each domain
regarding auto-register (remember that before this patchset,
auto-register only works for the OPAC).
6. Play with editing/adding/removing domain configurations
=> SUCCESS: Things work as expected, values are persisted on the DB
correctly.
7. Sign off :-D
Comment 5 Tomás Cohen Arazi (tcohen) 2024-08-23 15:29:28 UTC
Created attachment 170647 [details] [review]
Bug 37711: Add support for staff interface auto-register on IdP

Before this patches, only OPAC auto-registration at login using
OAuth2/OIDC IdPs was allowed.

This patch makes the staff interface to be able to auto-register new
users.

On doing it, I had to track down the possible values for the `interface`
parameter in the case of the staff interface.
Koha::Template::Plugin::AuthClient uses 'staff' internally so I sticked
with it. The whole area needs more param validation and exceptions to
avoid mistakes for not knowing the valid values.

I added some safeguards on the Koha::REST::Plugin::Auth::IdP helpers.
They get tested on the added tests.

To test:
1. Apply this patch
2. Run:
   $ prove t/db_dependent/Koha/REST/Plugin/Auth/IdP.t
=> SUCCESS: Tests pass!
3. Have a valid OAuth...
4. Enable Staff auto-register
5. Login with an external IdP on the staff interface
=> SUCCESS: It gets registered!
Note: it will probably lack permissions to access the staff interface,
but you can check on the DB it got registered.
6. Sign off :-D

Sponsored-by: ByWater Solutions
Comment 6 Martin Renvoize (ashimema) 2024-11-06 17:25:45 UTC
Created attachment 174090 [details] [review]
Bug 37711: DB update

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize (ashimema) 2024-11-06 17:25:47 UTC
Created attachment 174091 [details] [review]
Bug 37711: DBIC schema

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize (ashimema) 2024-11-06 17:25:50 UTC
Created attachment 174092 [details] [review]
Bug 37711: API changes

This patch makes the API representation of the identity providers
domains match the new metadata.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize (ashimema) 2024-11-06 17:25:52 UTC
Created attachment 174093 [details] [review]
Bug 37711: Domains CRUD update

This patch adapts the `Identity provider domains` CRUD page to handle
the new configuration options.

To test:
1. Have an IdP with some domains set. Make sure there's at least one
   domain with auto-register enabled and one with it disabled.
2. Apply this patches
3. Run:
   $ ktd --shell
  k$ updatedatabase
=> SUCCESS: Updates successfully
4. Run:
  k$ yarn build
5. Refresh the IdP domains page
=> SUCCESS: The domains table shows both OPAC and staff auto-register
   options.
=> SUCCESS: The staff auto-register is disabled for both domains
=> SUCCESS: The OPAC auto-register matches what was set for each domain
regarding auto-register (remember that before this patchset,
auto-register only works for the OPAC).
6. Play with editing/adding/removing domain configurations
=> SUCCESS: Things work as expected, values are persisted on the DB
correctly.
7. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize (ashimema) 2024-11-06 17:25:55 UTC
Created attachment 174094 [details] [review]
Bug 37711: Add support for staff interface auto-register on IdP

Before this patches, only OPAC auto-registration at login using
OAuth2/OIDC IdPs was allowed.

This patch makes the staff interface to be able to auto-register new
users.

On doing it, I had to track down the possible values for the `interface`
parameter in the case of the staff interface.
Koha::Template::Plugin::AuthClient uses 'staff' internally so I sticked
with it. The whole area needs more param validation and exceptions to
avoid mistakes for not knowing the valid values.

I added some safeguards on the Koha::REST::Plugin::Auth::IdP helpers.
They get tested on the added tests.

To test:
1. Apply this patch
2. Run:
   $ prove t/db_dependent/Koha/REST/Plugin/Auth/IdP.t
=> SUCCESS: Tests pass!
3. Have a valid OAuth...
4. Enable Staff auto-register
5. Login with an external IdP on the staff interface
=> SUCCESS: It gets registered!
Note: it will probably lack permissions to access the staff interface,
but you can check on the DB it got registered.
6. Sign off :-D

Sponsored-by: ByWater Solutions
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize (ashimema) 2024-11-06 17:26:25 UTC
All working well in testing.. permissions mapping could be fun but is for a followup bug I reckon.
Comment 12 David Cook 2024-11-06 22:53:40 UTC
(In reply to Martin Renvoize (ashimema) from comment #11)
> All working well in testing.. permissions mapping could be fun but is for a
> followup bug I reckon.

Agreed. That one is scarier...