We need a generic client implementation that allows setting different authentication services and configurations. It needs to: - work for the - work for the staff interface - have a CRUD page for dealing with authentication services - be designed so we eventually move other authentication mechanisms into this CRUD (e.g. CAS? LDAP?) - allow setting configurations per-service and per-domain (i.e. an OIDC service might allow authenticating several (campus?) domains and we should handle that). - provide a prettier integration with the login form than CAS and friends Follow-ups: - Prioritize IdP's when displaying login options - Icon / user-oriented presentation Not all of this needs to be implemented on this bug report. Should be splitted.
Forgot to mention (and is very important): - Implementation should use a library to handle OAuth2 (as opposed to the current googleopenidconnect script, which does things manually). Mojolicious::Plugin::OAuth2 is already packaged in the staging repo for this purpose (mtj++)
+1 Here for authoring, testing, or QAing.
Created attachment 141362 [details] [review] Bug 31378: DB changes
Created attachment 141363 [details] [review] Bug 31378: DBIC updates [DO NOT PUSH]
Created attachment 141364 [details] [review] Bug 31378: Add dependencies
Created attachment 141365 [details] [review] Bug 31378: Add Koha::Auth::Provider::Domain(s) class(es)
Created attachment 141366 [details] [review] Bug 31378: Add Koha::Auth::Provider(s) class(es)
Created attachment 141367 [details] [review] Bug 31378: Add Koha::Auth::Client* modules
Created attachment 141368 [details] [review] Bug 31378: Add Auth mojo plugin
Created attachment 141369 [details] [review] Bug 31378: Add API routes
Created attachment 141370 [details] [review] Bug 31378: Add auth providers template plugin
Created attachment 141371 [details] [review] Bug 31378: Add the API-based auth mechanism to C4::Auth::check_api_auth
Created attachment 141372 [details] [review] Bug 31378: Add OPAC support for authentication providers
Created attachment 141373 [details] [review] Bug 31378: Add STAFF support for authentication providers
Created attachment 141374 [details] [review] Bug 31378: Add accessor methods for JSON attributes
Created attachment 141375 [details] [review] Bug 31378: Add manage_authentication_providers permission Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 141376 [details] [review] Bug 31378: Add 'parameter' parameter to Koha::Exceptions::MissingParameter Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 141377 [details] [review] Bug 31378: Add authentication provider endpoints This patch adds routes for handling authentication providers to the REST API. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/auth_providers.t => SUCCESS: Tests pass! 3. Sign off :-D
Created attachment 141378 [details] [review] Bug 31378: Add an administration interface for authentication sources
Notes for testing: KTD (master) comes with the koha-staging repo already configured, so in order to test this patches you can just run: $ apt install libmojolicious-plugin-oauth2-perl After installing the dependency, you need to restart Plack (as usual): $ koha-plack --restart kohadev The OAuth2 Mojolicious plugin is feeded with the configured authentication providers at (plack) startup time, so in order for new providers to be picked, restart Plack, just to be sure. Enjoy, provide any constructive comments you can, early.
I just noticed that you need to run the database update for Koha to work. Not sure if this could cause problem with the web installer. Error while loading /etc/koha/sites/kohadev/plack.psgi: Can't load application from file "/kohadevbox/koha/api/v1/app.pl": DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Table 'koha_kohadev.auth_providers' doesn't exist at /kohadevbox/koha/Koha/Objects.pm line 312
Created attachment 141514 [details] [review] Bug 31378: Use base URLs for redirect uri
Are there test plans for the UIs? I'm keen to test this out (although might be worth saving my SO for the QA stage).
Created attachment 141771 [details] [review] Bug 31378: DB changes Signed-off-by: Katrin <me@myemail.com>
Created attachment 141772 [details] [review] Bug 31378: DBIC updates [DO NOT PUSH] Signed-off-by: Katrin <me@myemail.com>
Created attachment 141773 [details] [review] Bug 31378: Add dependencies Signed-off-by: Katrin <me@myemail.com>
Created attachment 141774 [details] [review] Bug 31378: Add Koha::Auth::Provider::Domain(s) class(es) Signed-off-by: Katrin <me@myemail.com>
Created attachment 141775 [details] [review] Bug 31378: Add Koha::Auth::Provider(s) class(es) Signed-off-by: Katrin <me@myemail.com>
Created attachment 141776 [details] [review] Bug 31378: Add Koha::Auth::Client* modules Signed-off-by: Katrin <me@myemail.com>
Created attachment 141777 [details] [review] Bug 31378: Add Auth mojo plugin Signed-off-by: Katrin <me@myemail.com>
Created attachment 141778 [details] [review] Bug 31378: Add API routes Signed-off-by: Katrin <me@myemail.com>
Created attachment 141779 [details] [review] Bug 31378: Add auth providers template plugin Signed-off-by: Katrin <me@myemail.com>
Created attachment 141780 [details] [review] Bug 31378: Add the API-based auth mechanism to C4::Auth::check_api_auth Signed-off-by: Katrin <me@myemail.com>
Created attachment 141781 [details] [review] Bug 31378: Add OPAC support for authentication providers Signed-off-by: Katrin <me@myemail.com>
Created attachment 141782 [details] [review] Bug 31378: Add STAFF support for authentication providers Signed-off-by: Katrin <me@myemail.com>
Created attachment 141783 [details] [review] Bug 31378: Add accessor methods for JSON attributes Signed-off-by: Katrin <me@myemail.com>
Created attachment 141784 [details] [review] Bug 31378: Add manage_authentication_providers permission Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Katrin <me@myemail.com>
Created attachment 141785 [details] [review] Bug 31378: Add 'parameter' parameter to Koha::Exceptions::MissingParameter Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Katrin <me@myemail.com>
Created attachment 141786 [details] [review] Bug 31378: Add authentication provider endpoints This patch adds routes for handling authentication providers to the REST API. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/auth_providers.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Katrin <me@myemail.com>
Created attachment 141787 [details] [review] Bug 31378: Add an administration interface for authentication sources Signed-off-by: Katrin <me@myemail.com>
Created attachment 141788 [details] [review] Bug 31378: Use base URLs for redirect uri Signed-off-by: Katrin <me@myemail.com>
Was not me, but the sandbox was created in my name. We'll fix sign-off lines.
Created attachment 141801 [details] [review] Bug 31378: DB changes Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu>
Created attachment 141802 [details] [review] Bug 31378: DBIC updates [DO NOT PUSH] Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu>
Created attachment 141803 [details] [review] Bug 31378: Add dependencies Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu>
Created attachment 141804 [details] [review] Bug 31378: Add Koha::Auth::Provider::Domain(s) class(es) Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu>
Created attachment 141805 [details] [review] Bug 31378: Add Koha::Auth::Provider(s) class(es) Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu>
Created attachment 141806 [details] [review] Bug 31378: Add Koha::Auth::Client* modules Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu>
Created attachment 141807 [details] [review] Bug 31378: Add Auth mojo plugin Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu>
Created attachment 141808 [details] [review] Bug 31378: Add API routes Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu>
Created attachment 141809 [details] [review] Bug 31378: Add auth providers template plugin Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu>
Created attachment 141810 [details] [review] Bug 31378: Add the API-based auth mechanism to C4::Auth::check_api_auth Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu>
Created attachment 141811 [details] [review] Bug 31378: Add OPAC support for authentication providers Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu>
Created attachment 141812 [details] [review] Bug 31378: Add STAFF support for authentication providers Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu>
Created attachment 141813 [details] [review] Bug 31378: Add accessor methods for JSON attributes Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu>
Created attachment 141814 [details] [review] Bug 31378: Add manage_authentication_providers permission Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu>
Created attachment 141815 [details] [review] Bug 31378: Add 'parameter' parameter to Koha::Exceptions::MissingParameter Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu>
Created attachment 141816 [details] [review] Bug 31378: Add authentication provider endpoints This patch adds routes for handling authentication providers to the REST API. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/auth_providers.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu>
Created attachment 141817 [details] [review] Bug 31378: Add an administration interface for authentication sources Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu>
Created attachment 141818 [details] [review] Bug 31378: Use base URLs for redirect uri Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu>
Just fixing up signoff lines.. I think David offered to QA this so I'll set him as assigned for it :)
(In reply to Katrin Fischer from comment #21) > I just noticed that you need to run the database update for Koha to work. > Not sure if this could cause problem with the web installer. > > Error while loading /etc/koha/sites/kohadev/plack.psgi: Can't load > application from file "/kohadevbox/koha/api/v1/app.pl": > DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st > execute failed: Table 'koha_kohadev.auth_providers' doesn't exist at > /kohadevbox/koha/Koha/Objects.pm line 312 This is certainly an issue. I'm looking at follow-ups...
(In reply to Martin Renvoize from comment #58) > To test: > 1. Apply this patch > 2. Run: > $ kshell > k$ prove t/db_dependent/api/v1/auth_providers.t > => SUCCESS: Tests pass! > 3. Sign off :-D Looks like the unit test file is t/db_dependent/Koha/Auth/Provider.t these days.
(In reply to David Cook from comment #62) > (In reply to Katrin Fischer from comment #21) > > I just noticed that you need to run the database update for Koha to work. > > Not sure if this could cause problem with the web installer. > > > > Error while loading /etc/koha/sites/kohadev/plack.psgi: Can't load > > application from file "/kohadevbox/koha/api/v1/app.pl": > > DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st > > execute failed: Table 'koha_kohadev.auth_providers' doesn't exist at > > /kohadevbox/koha/Koha/Objects.pm line 312 > > This is certainly an issue. I'm looking at follow-ups... I think the old GoogleOpenId... stuff removal is to blame, as we decided to defer it. Will look tomorrow morning
(In reply to David Cook from comment #63) > (In reply to Martin Renvoize from comment #58) > > To test: > > 1. Apply this patch > > 2. Run: > > $ kshell > > k$ prove t/db_dependent/api/v1/auth_providers.t > > => SUCCESS: Tests pass! > > 3. Sign off :-D > > Looks like the unit test file is t/db_dependent/Koha/Auth/Provider.t these > days. There should be tests for the providers CRUD in there as well. git diff origin/master --name-only
(In reply to David Cook from comment #62) > (In reply to Katrin Fischer from comment #21) > > I just noticed that you need to run the database update for Koha to work. > > Not sure if this could cause problem with the web installer. > > > > Error while loading /etc/koha/sites/kohadev/plack.psgi: Can't load > > application from file "/kohadevbox/koha/api/v1/app.pl": > > DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st > > execute failed: Table 'koha_kohadev.auth_providers' doesn't exist at > > /kohadevbox/koha/Koha/Objects.pm line 312 > > This is certainly an issue. I'm looking at follow-ups... Please comment about the code, propose things and stuff so this shines for the release. Follow-ups will come out of course. Thanks for your time!
(In reply to Tomás Cohen Arazi from comment #66) > Please comment about the code, propose things and stuff so this shines for > the release. Follow-ups will come out of course. Thanks for your time! I've spent the last few hours working on this one, and have a ton of feedback. Hopefully all pretty easy to fix I think...
I'll shortly add a follow-up to catch the error of the missing database update. In theory, with a package install, the database will be automatically upgraded before the web app is started up. But some people will still need to use the web installer, so we don't want it to fail. (Those people will need to restart their Koha before they're able to use the new auth functionality of course.) Test plan 1: 1. apt install libmojolicious-plugin-oauth2-perl 2. restart_all 3. koha-upgrade-schema kohadev 4. koha-plack --restart kohadev 5. prove t/db_dependent/Koha/Auth/Provider.t So far so good. Test plan 2: 1. Go to http://localhost:8081/cgi-bin/koha/admin/admin-home.pl 2. Click on "Authentication providers" (#NOTE: I would've preferred "Identity Providers" since that's a more common industry term.) 3. Click "New authentication provider" (#NOTE: The UI looks poorly formatted, but I think that's probably a symptom of the new staff interface styling. Not a blocker for me. This can be fixed later...) (#NOTE: There is a bug with the "Configuration" where "Add new X configuration" doesn't work if you've manually changed anything in the text box. Not a blocker for me, but will need to be fixed at some point. Same goes for "Add default OIDC mapping.) (#NOTE: There's not enough documentation/help text on how to use this UI. I'm figuring it out through trial and error, but a bit of help text for "Code", "Description", and "Icon URL" at a minimum would be good.) (#NOTE: With the new Staff Interface, there should be a "Help" link on the right hand side, but I don't see it for this new functionality. I think that's a blocker.) (#NOTE: I don't really like having to include raw JSON in this UI. This could be made more beautiful.) (#NOTE: "Code" doesn't appear to be restricted or validated in any way. We should stick to alphanumeric codes. This is borderline... but I think it's a blocker. We need to set the rule before people start using it.) (#NOTE: We need to add help text at the bottom of this page that says this auth provider won't be available until after a Koha restart.) 4. Click on "Manage Domains" 5. Click "Edit" for first and only line (#NOTE: It's not clear what a "Domain" is in this context. This needs more help text/documentation. I'd say that's a blocker. From past code review in Koha for OIDC, "Domain" referred to email domain. That really should spelled out clearly. ) 6. The breadcrumb doesn't show the auth provider code on the "Edit authentication provider domain" like it does on the "Authentication provider domains" page 4. koha-plack --restart kohadev (#NOTE: This is an unfortunate step but necessary because of the plugin being used. Auth providers are rarely set up, so not a big drama.) 4. In an Incognito tab or different browser, go to http://localhost:8081/ (#NOTE: In my opinion, we should *not* be allowing staff login by default. While "Auto register" is "Don't allow" by default, we should keep the staff interface as locked down as possible. Not a blocker but an observation...) 5. When I try to login with Keycloak, I get the following error in Koha: [{"message":"Malformed query string","path":"\/query\/session_state"}] (#NOTE: Newer OIDC providers will provide session_state in the authentication response. I'll turn this off in Keycloak. Folks can read more about session_state at https://openid.net/specs/openid-connect-session-1_0.html) 6. Now I'm getting this error on the Koha Staff Interface login page (since the user doesn't exist in Koha and I have auto-register turned off): There was an error authenticating to external identity provider Exception 'Koha::Exceptions::Auth::Unauthorized' thrown 'External auth user cannot access resource' with code => 401 (#NOTE: I don't think printing the exception on the staff interface is a good idea. Let's remove that.) 7. After adding my user to Koha and giving staff permissions, I'm able to log in. Very nice! 8. Go to http://localhost:8080/ and click "Log in with Keycloak IdP" 9. Since I already have a session in Keycloak, I'm logged into the OPAC with no login. Very good! 10. If I logout of Keycloak and try to log back into http://localhost:8080/ via Keycloak, I get the following: There was an error authenticating to external identity provider Can't call method "auto_register" on an undefined value at /kohadevbox/koha/Koha/REST/Plugin/Auth.pm line 66. (#NOTE: In the code I see "tranverse_hash" but it should be "traverse_hash" in English.) 11. Turn on auto register for all blank domain... 12. restart_all in ktd 13. Try to log into OPAC and Staff Interface 14. Neither works... (#NOTE: In Koha/REST/Plugin/Auth.pm, it looks like auto-register should only work for OPAC. In theory, I like that, although I suppose the workaround would be to auto-register for the OPAC, and then your account would exist for the Staff Interface anyway...) 15. I tried "Update on login" using the blank domain and a domain of "prosentient.com.au" and both failed to update my firstname and surname on login. That's a blocker...
Here is the TL;DR version: Question: 1. I noticed that the default "Mapping configuration" is just 1-to-1 for strings, but Oauth/OIDC claims can be complex data structures. How do you handle when the claim is an array or an object? The most common one I see in the real world is the "address" object, but there are others too. Looking at Koha/Auth/Client/OAuth.pm and Koha/Auth/Client.pm, I think that you have added code for this. Would it be "address.street_address" in the Mapping to get the "street_address" value out of the "address" claim? The code is uncommented and a bit obscure, so I can't tell if you can handle array values, although I'm less worried about them. Can you? TODO: 0. Add missing "Help" link to all new pages 1. Restrict "Code" to alphanumeric code 2. Add help text to "New authentication provider" UI 3. Add help text to "Edit authentication provider domain" UI 4. Fix breadcrumb on "Edit authentication provider domain" UI 5. Remove exception text from Koha staff interface login page 6. Remove exception text from Koha OPAC login page 7. Think about changing "tranverse_hash" to "traverse_hash" 8. Fix "Auto register" 9. Fix "Update on login"
Created attachment 141853 [details] [review] Bug 31378: (follow-up) catch startup error when missing Oauth provider table In theory, this shouldn't come up with a package install, but it could happen with other installations, so best to let it get to the web installer.
QA tool complaints: FAIL Koha/Auth/Client/OAuth.pm FAIL pod coverage POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666 FAIL Koha/Auth/Provider/Domain.pm FAIL pod coverage POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666 FAIL Koha/Exceptions/Auth.pm FAIL pod coverage POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666 FAIL Koha/REST/Plugin/Auth.pm FAIL pod coverage POD is missing for 'register' FAIL Koha/Schema/Result/AuthProvider.pm FAIL pod coverage POD is missing for koha_object_class POD is missing for koha_objects_class FAIL Koha/Schema/Result/AuthProviderDomain.pm FAIL pod coverage POD is missing for koha_object_class POD is missing for koha_objects_class FAIL admin/authentication_providers.pl FAIL file permissions File must have the exec flag FAIL koha-tmpl/intranet-tmpl/prog/en/modules/admin/authentication_providers.tt FAIL forbidden patterns forbidden pattern: simple-quote string (line 407) forbidden pattern: tab char (line 427) forbidden pattern: tab char (line 442) forbidden pattern: simple-quote string (line 449) forbidden pattern: simple-quote string (line 450) forbidden pattern: simple-quote string (line 456) FAIL koha-tmpl/intranet-tmpl/prog/en/modules/auth.tt FAIL valid_template DBIx::Class::Schema::source(): Can't find source for AuthProvider at /kohadevbox/koha/Koha/Objects.pm line 524 FAIL koha-tmpl/opac-tmpl/bootstrap/en/includes/masthead.inc FAIL valid_template DBIx::Class::Schema::source(): Can't find source for AuthProvider at /kohadevbox/koha/Koha/Objects.pm line 524 FAIL koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth.tt FAIL valid_template DBIx::Class::Schema::source(): Can't find source for AuthProvider at /kohadevbox/koha/Koha/Objects.pm line 524 FAIL koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-main.tt FAIL valid_template DBIx::Class::Schema::source(): Can't find source for AuthProvider at /kohadevbox/koha/Koha/Objects.pm line 524
At line 407 of koha-tmpl/intranet-tmpl/prog/en/modules/admin/authentication_providers.tt, "Not a valid JSON" should be "Not valid JSON" since JSON refers to the format and not the data itself.
(In reply to Tomás Cohen Arazi from comment #65) > There should be tests for the providers CRUD in there as well. > > git diff origin/master --name-only Not that I can see. Just t/db_dependent/Koha/Auth/Provider.t Maybe there are some missing patches
Another problem: If you have a "blank" domain and a specific domain, the "blank" domain is used instead of the specific domain. If you want to set different "Default library" and "Default category" based on email domain, it won't work if you have a "blank" domain. It also means the "Auto register" and "Update on login" wouldn't work right... if they were working... I think the only thing that does work is having "Allow staff: No" in "blank" domain and "Allow staff: Yes" in specific domain. ("Allow opac: no" for "blank" would also work but that would be an unlikely scenario.)
One last thing: the styling on the login buttons on the Staff Interface and OPAC have little to no contrast which is hard on the eyes. It also makes the Staff Interface buttons look strange when you have multiple providers with descriptions of different lengths.
One last last thing: It would be good to have a "Sequence" value for the providers, so that you could arrange them on the Staff Interface and OPAC in a desired order rather than the order that they were added to Koha.
(In reply to David Cook from comment #62) > (In reply to Katrin Fischer from comment #21) > > I just noticed that you need to run the database update for Koha to work. > > Not sure if this could cause problem with the web installer. > > > > Error while loading /etc/koha/sites/kohadev/plack.psgi: Can't load > > application from file "/kohadevbox/koha/api/v1/app.pl": > > DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st > > execute failed: Table 'koha_kohadev.auth_providers' doesn't exist at > > /kohadevbox/koha/Koha/Objects.pm line 312 > > This is certainly an issue. I'm looking at follow-ups... Thx David :) Read your comment below and I think it describes the problem well, package installations being probably ok, but others could get trapped with the web installer not coming up to tell them about the missing package.
Hi everyone, it's been a long time since I wrote to you! Thanks so much David for so extensive testing.. I'll try to address as much points as possible. (In reply to David Cook from comment #68) > (#NOTE: I would've preferred "Identity Providers" since that's a more common > industry term.) Nice point. I'll talk to Tomas if we change it > (#NOTE: The UI looks poorly formatted, but I think that's probably a symptom > of the new staff interface styling. Not a blocker for me. This can be fixed > later...) Sorry, it's been a long time since I've done anything related to Koha.. If you could point me to a well formated page, I'll try to stick to the style as much as possible > (#NOTE: There is a bug with the "Configuration" where "Add new X > configuration" doesn't work if you've manually changed anything in the text > box. Not a blocker for me, but will need to be fixed at some point. Same > goes for "Add default OIDC mapping.) I do not believe this patches are at the latest version.. I've added a jquery-validator to check if your jsons are valid before saving. I imagine this is a patch for now, and we need a way to edit a json in a proper way > (#NOTE: There's not enough documentation/help text on how to use this UI. Yes, I've been coding this between jobs so I had no time to document, but surely we will add some follow-ups. > I'm figuring it out through trial and error, but a bit of help text for > "Code", "Description", and "Icon URL" at a minimum would be good.) Code should be a unique name for your Identity Provider, the Description is the text that will appear in the login button, and Icon URL should be a URL where we can fetch an icon for login button > (#NOTE: With the new Staff Interface, there should be a "Help" link on the > right hand side, but I don't see it for this new functionality. I think > that's a blocker.) Ok, I'll ask Tomas for help about how this shoul be done > (#NOTE: I don't really like having to include raw JSON in this UI. This > could be made more beautiful.) Me neither, there should be a JSON editor or something.. does Koha have one? > (#NOTE: "Code" doesn't appear to be restricted or validated in any way. We > should stick to alphanumeric codes. This is borderline... but I think it's a > blocker. We need to set the rule before people start using it.) Good point. Will be done > (#NOTE: We need to add help text at the bottom of this page that says this > auth provider won't be available until after a Koha restart.) Yes, but we have plans to make it so that you wont need to restart Koha for the provider to be available (a PR to Mojolicious::Plugin::OAuth2 project) > 4. Click on "Manage Domains" > 5. Click "Edit" for first and only line > (#NOTE: It's not clear what a "Domain" is in this context. This needs more > help text/documentation. I'd say that's a blocker. From past code review in > Koha for OIDC, "Domain" referred to email domain. That really should spelled > out clearly. ) Agree. Domain in this case is, as you said, email domain. > 6. The breadcrumb doesn't show the auth provider code on the "Edit > authentication provider domain" like it does on the "Authentication provider > domains" page I'll check it > > > 4. koha-plack --restart kohadev > (#NOTE: This is an unfortunate step but necessary because of the plugin > being used. Auth providers are rarely set up, so not a big drama.) > > 4. In an Incognito tab or different browser, go to http://localhost:8081/ > (#NOTE: In my opinion, we should *not* be allowing staff login by default. > While "Auto register" is "Don't allow" by default, we should keep the staff > interface as locked down as possible. Not a blocker but an observation...) Ok, that's easily done > > 5. When I try to login with Keycloak, I get the following error in Koha: > [{"message":"Malformed query string","path":"\/query\/session_state"}] > (#NOTE: Newer OIDC providers will provide session_state in the > authentication response. I'll turn this off in Keycloak. Folks can read more > about session_state at > https://openid.net/specs/openid-connect-session-1_0.html) > > 6. Now I'm getting this error on the Koha Staff Interface login page (since > the user doesn't exist in Koha and I have auto-register turned off): > There was an error authenticating to external identity provider > Exception 'Koha::Exceptions::Auth::Unauthorized' thrown 'External auth user > cannot access resource' with code => 401 > (#NOTE: I don't think printing the exception on the staff interface is a > good idea. Let's remove that.) As I said, I do not believe these patches to be at the latest version.. now the user gets redirected to the login page (staff or opac) with error message > > 7. After adding my user to Koha and giving staff permissions, I'm able to > log in. Very nice! > > 8. Go to http://localhost:8080/ and click "Log in with Keycloak IdP" > 9. Since I already have a session in Keycloak, I'm logged into the OPAC with > no login. Very good! > > 10. If I logout of Keycloak and try to log back into http://localhost:8080/ > via Keycloak, I get the following: > There was an error authenticating to external identity provider > Can't call method "auto_register" on an undefined value at > /kohadevbox/koha/Koha/REST/Plugin/Auth.pm line 66. Strange, I'll have to check that out > > (#NOTE: In the code I see "tranverse_hash" but it should be "traverse_hash" > in English.) typo... sorry > > 11. Turn on auto register for all blank domain... > 12. restart_all in ktd > 13. Try to log into OPAC and Staff Interface > 14. Neither works... > (#NOTE: In Koha/REST/Plugin/Auth.pm, it looks like auto-register should only > work for OPAC. In theory, I like that, although I suppose the workaround > would be to auto-register for the OPAC, and then your account would exist > for the Staff Interface anyway...) > > 15. I tried "Update on login" using the blank domain and a domain of > "prosentient.com.au" and both failed to update my firstname and surname on > login. That's a blocker... Yep, I think I forgot to code that part Thanks!
(In reply to David Cook from comment #74) > Another problem: > > If you have a "blank" domain and a specific domain, the "blank" domain is > used instead of the specific domain. Strange, the behavior should be the opposite. The logic was that you could define multiple domains (even partially) and the domain with the longest matching text should win. If there was no matching domain, then go with the default. > > If you want to set different "Default library" and "Default category" based > on email domain, it won't work if you have a "blank" domain. Maybe I've done something wrong with the regexp, and if the email domain doesn't match, you won't see that "default library" and "default category" configured for the domain > > It also means the "Auto register" and "Update on login" wouldn't work > right... if they were working... I've fixed auto register in latest patches, but for it to work, you need to define userid or cardnumber in the mapper. > > I think the only thing that does work is having "Allow staff: No" in "blank" > domain and "Allow staff: Yes" in specific domain. ("Allow opac: no" for > "blank" would also work but that would be an unlikely scenario.)
(In reply to Agustín Moyano from comment #78) > > (#NOTE: The UI looks poorly formatted, but I think that's probably a symptom > > of the new staff interface styling. Not a blocker for me. This can be fixed > > later...) > > Sorry, it's been a long time since I've done anything related to Koha.. If > you could point me to a well formated page, I'll try to stick to the style > as much as possible I'm not familiar with the new staff UI myself, but I think you'll be able to see what I mean at a glance. On CRUD pages, the labels and fields aren't aligned correctly in all cases at the moment. > > (#NOTE: With the new Staff Interface, there should be a "Help" link on the > > right hand side, but I don't see it for this new functionality. I think > > that's a blocker.) > > Ok, I'll ask Tomas for help about how this shoul be done Sounds good. I think it might involve the Koha/Manual.pm file. (I just noticed it on a different issue report earlier today.) > > (#NOTE: I don't really like having to include raw JSON in this UI. This > > could be made more beautiful.) > > Me neither, there should be a JSON editor or something.. does Koha have one? Not that I know of. Rather, I meant it would make sense to just have a regular HTML form and translate that into JSON on the backend. > As I said, I do not believe these patches to be at the latest version.. now > the user gets redirected to the login page (staff or opac) with error message Looking forward to testing the next round! > > 10. If I logout of Keycloak and try to log back into http://localhost:8080/ > > via Keycloak, I get the following: > > There was an error authenticating to external identity provider > > Can't call method "auto_register" on an undefined value at > > /kohadevbox/koha/Koha/REST/Plugin/Auth.pm line 66. > > Strange, I'll have to check that out I wasn't able to reproduce it again so might've just been a bit glitchy due to restarts/Keycloak logouts/etc. Might not need to worry about it.
(In reply to Agustín Moyano from comment #79) > > If you have a "blank" domain and a specific domain, the "blank" domain is > > used instead of the specific domain. > > Strange, the behavior should be the opposite. The logic was that you could > define multiple domains (even partially) and the domain with the longest > matching text should win. If there was no matching domain, then go with the > default. Worth double-checking that one I reckon. > > If you want to set different "Default library" and "Default category" based > > on email domain, it won't work if you have a "blank" domain. > > Maybe I've done something wrong with the regexp, and if the email domain > doesn't match, you won't see that "default library" and "default category" > configured for the domain How is the domain supposed to be configured? Perhaps I wasn't inputting it as expected either? > I've fixed auto register in latest patches, but for it to work, you need to > define userid or cardnumber in the mapper. You'll definitely need to indicate that on the UI pages.
Looks like "soft" feature freeze is Oct 28, so if you can get the updates patches up ASAP, I'll make time for QA
For the missing help link the last patch on bug 31162 might be interesting. I also noticed the help missing there and it was due to changes to the page structure with the new staff design. For the JSON editor: Maybe CodeMirror that we use for SQL and YAML can do it?
(In reply to Katrin Fischer from comment #83) > For the missing help link the last patch on bug 31162 might be interesting. > I also noticed the help missing there and it was due to changes to the page > structure with the new staff design. > > For the JSON editor: Maybe CodeMirror that we use for SQL and YAML can do it? I felt like using the CodeMirror plugin would be enough. Maybe we should make the 'Create' button be a dropdown instead, for choosing the protocol, and make the form be already pre-populated, so we remove the buttons too.
I'm not too worried about the JSON editor at this point. More of a nice to have but not essential. Agustin, are we going to get an updated version before the end of this week? I could make time to QA this before the soft feature freeze this Friday.
*** Bug 31965 has been marked as a duplicate of this bug. ***
Comment on attachment 141807 [details] [review] Bug 31378: Add Auth mojo plugin Review of attachment 141807 [details] [review]: ----------------------------------------------------------------- ::: Koha/REST/Plugin/Auth.pm @@ +83,5 @@ > + 'auth.session' => sub { > + my ( $c, $patron ) = @_; > + my $userid = $patron->userid; > + my $cardnumber = $patron->cardnumber; > + my $cgi = CGI->new; I don't think that we should be using the CGI module in the Mojolicious REST API, since we're not using CGI. This is hacky. @@ +89,5 @@ > + $cgi->param( userid => $userid ); > + $cgi->param( cardnumber => $cardnumber ); > + $cgi->param( auth_client_login => 1 ); > + > + my ( $status, $cookie, $session_id ) = C4::Auth::check_api_auth($cgi); This is a suboptimal design for at least a couple of reasons: 1) it requires a CGI object; 2) it creates a session for the API interface rather than the OPAC/Staff interfaces; 3) it's not to purpose - we're not checking auth. We know that we're just creating a new session. I've been saying for years that we need to standardize session creation, and it's still true. In the meantime, a minimal session can be created as simply as this: $session->param( 'number', $patron->borrowernumber ); $session->param( 'id', $patron->userid ); $session->param( 'cardnumber', $patron->cardnumber ); $session->param( 'firstname', $patron->firstname ); $session->param( 'surname', $patron->surname ); $session->param( 'branch', $patron->branchcode ); $session->param( 'branchname', $patron->library->branchname ); $session->param( 'flags', $patron->flags ); $session->param( 'emailaddress', $patron->email ); $session->param( 'ip', $session->remote_addr() ); $session->param( 'lasttime', time() ); $session->param( 'interface', $type); Then you can either pass the $session object or the session ID as the return value.
(In reply to David Cook from comment #87) > I've been saying for years that we need to standardize session creation, and > it's still true. Recently, I did a little patch for that at Bug 31050, but I haven't had an urgent reason to continue on it. It would be perfect for a situation like this though. (Of course, I thought of it while working on Koha plugins, other auth solutions, etc.)
Hi all, a couple follow-ups have been pushed to the branch and are ready for testing. We couldn't make some new tests we attempted to write for login work. It would be great to have them, but not sure if required. Why help there is welcome. We ran out of fuel yesterday
The branch doesn't seem to be linked, can you please remind us where to find the updated patches?
Is this the link? https://gitlab.com/thekesolutions/Koha/-/commits/oauth_client
(In reply to Aleisha Amohia from comment #91) > Is this the link? > https://gitlab.com/thekesolutions/Koha/-/commits/oauth_client Hi Aleisha, Tomas added it to the URL field after my last comment, it matches with yours.
(In reply to Tomás Cohen Arazi from comment #89) > Hi all, a couple follow-ups have been pushed to the branch and are ready for > testing. We couldn't make some new tests we attempted to write for login > work. > > It would be great to have them, but not sure if required. Why help there is > welcome. We ran out of fuel yesterday Does this mean that the tests are failing and you'd like help to fix the tests?
(In reply to David Cook from comment #93) > (In reply to Tomás Cohen Arazi from comment #89) > > Hi all, a couple follow-ups have been pushed to the branch and are ready for > > testing. We couldn't make some new tests we attempted to write for login > > work. > > > > It would be great to have them, but not sure if required. Why help there is > > welcome. We ran out of fuel yesterday > > Does this mean that the tests are failing and you'd like help to fix the > tests? It means an attempt from Agustín to write login tests didn't go well. We're can live without those. I'd like you to test that branch, which solves things that were reported, including a rename. That branch probably needs a tidy, I haven't checked that last commit (before the tests).
(In reply to Tomás Cohen Arazi from comment #94) > It means an attempt from Agustín to write login tests didn't go well. We're > can live without those. Ok I think I understand. I won't worry too much about failing tests for this go around then. > I'd like you to test that branch, which solves things that were reported, > including a rename. That branch probably needs a tidy, I haven't checked > that last commit (before the tests). The follow up commits seem to be a mix of the tests and core code changes, so those would need to be re-organised before QAing and pushing I think. -- I suppose Aleisha or myself could QA, so I suppose either of us could sign off. I'll have a look in any case.
Just skip the last commit (wip trying to add tests). I noticed we need to redo Nick's and your patches
(In reply to Tomás Cohen Arazi from comment #96) > Just skip the last commit (wip trying to add tests). > > I noticed we need to redo Nick's and your patches It looks like the last commit includes some updates besides tests though despite the commit name?
(In reply to David Cook from comment #97) > > It looks like the last commit includes some updates besides tests though > despite the commit name? Skip the tests themselves, unless you really want to look at them
Ok. Starting testing now...
Looks like we're 547 commits behind master... and for some reason "qa" think the "qa-prev-commit" is 1 commit behind. Getting a number of failures: testing 1 commit(s) (applied to 46c299f 'd6 Bug 31378: Rename Auth Provider to') Processing files before patches Configure idp routes=====>| 10 / 10 (100.00%) Processing files after patches |========================>| 10 / 10 (100.00%) OK Koha/Auth/Client.pm OK Koha/REST/V1/Auth/Identity/Provider/Domains.pm OK api/v1/swagger/definitions/identity_provider.yaml OK api/v1/swagger/definitions/identity_provider_domain.yaml OK api/v1/swagger/paths/auth.yaml OK installer/data/mysql/atomicupdate/bug_31378.pl OK installer/data/mysql/kohastructure.sql FAIL t/db_dependent/Koha/REST/Plugin/Auth/IdP.t FAIL file permissions File must have the exec flag FAIL forbidden patterns forbidden pattern: trailing space char (line 55) forbidden pattern: trailing space char (line 123) FAIL t/db_dependent/api/v1/idp.t FAIL file permissions File must have the exec flag FAIL forbidden patterns forbidden pattern: trailing space char (line 195) forbidden pattern: trailing space char (line 258) forbidden pattern: trailing space char (line 301) forbidden pattern: Use of Data::Printer (line 310) forbidden pattern: Use of Data::Printer, should only be used for devlopment purpose (line 310) FAIL spelling wiht ==> with wihtout ==> without FAIL valid Can't locate Crypt/OpenSSL/RSA.pm in @INC (you may need to install the Crypt::OpenSSL::RSA module) (@INC contains: /kohadevbox/koha /kohadevbox/koha/lib /kohadevbox/qa-test-tools /etc/perl /usr/local/lib/x86_64-linux-gnu/ perl/5.32.1 /usr/local/share/perl/5.32.1 /usr/lib/x86_64-linux-gnu/perl5/5.32 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl-base /usr/lib/x86_64-linux-gnu/perl/5.32 /usr/share/perl/5.32 /usr/local/lib/site_perl) FAIL t/lib/IdP/ExternalIdP.pm FAIL pod coverage POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666 Processing additional checks * Commit title does not start with 'Bug XXXXX: ' - c649ebc111
Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc Auto-merging installer/data/mysql/mandatory/userpermissions.sql CONFLICT (content): Merge conflict in installer/data/mysql/mandatory/userpermissions.sql error: could not apply b466baa415... Bug 31378: Add manage_authentication_providers permission Resolve all conflicts manually, mark them as resolved with "git add/rm <conflicted_files>", then run "git rebase --continue". You can instead skip this commit: run "git rebase --skip". To abort and get back to the state before "git rebase", run "git rebase --abort". Could not apply b466baa415... Bug 31378: Add manage_authentication_providers permission
Started trying to fix the merge conflicts but looks like there's a fair number. This is something that should've been done before moving it to "Needs Signoff" I reckon.
I'll still keep testing to see what I can see though...
Test plan 1: 1. apt install libmojolicious-plugin-oauth2-perl 2. koha-upgrade-schema kohadev 3. restart_all <Note that Nick and David's patches from Bugzilla are still needed for Gitlab branch...> Test plan 2: 1. Go to http://localhost:8081/cgi-bin/koha/admin/admin-home.pl 2. Click on "Identity providers" 3. Click "New identity provider" (#NOTE: UI formatting looks OK on Koha 22.06, but I wonder what it'll look like on Koha 22.11...) (#FIXME: "Add default OAuth..." buttons still don't work if you make any changes in the text box before pressing the button.) (#FIXME: There's still not enough help text on the UI to explain how to fill it all in.) (#NOTE: It probably makes more sense to default to "OIDC" than "OAuth" since the former is more common with common identity providers) 4. Enter relevant details and click "Submit" (#FIXME: "Code" shouldn't be able to contain non-alphanumeric characters) (#FIXME: Putting in garbage values causes the page to return to /cgi-bin/koha/admin/identity_providers.pl with no error messages) (#FIXME: There needs to be a warning that integration won't work until after an application restart...) 5. Click on "Manage Domains" (#FIXME: It's not clear what "domains" means in this context. It should say something like "Identity provider email domains") 6. Click "Edit" on the default domains (#FIXME: The "Default library" and "Default category" are initially set to empty, but you can't set them to empty in the "Edit" interface) (#FIXME: The breadcrumb says "Domains for" instead of "Domains for Test") 7. koha-plack --restart kohadev 8. In an Incognito window or different browser, go to http://localhost:8080, and try logging in with a user that doesn't exist in Koha (#FIXME: When authentication fails, the end user sees the following message: There was an error authenticating to external identity provider Can't call method "auto_register" on an undefined value at /kohadevbox/koha/Koha/REST/Plugin/Auth/IdP.pm line 66. ) 9. Change "Identity provider" default domain to "Auto register" 10. In an Incognito window or different browser, go to http://localhost:8080, and try logging in with a user that doesn't exist in Koha (#FIXME: When authentication fails, the end user sees the following message: There was an error authenticating to external identity provider Can't call method "auto_register" on an undefined value at /kohadevbox/koha/Koha/REST/Plugin/Auth/IdP.pm line 66. ) (#FIXME: The auto register feature doens't work. It needs to be fixed.) 11. Create a Koha user to match your IdP user 12. In an Incognito window or different browser, go to http://localhost:8080, and try logging in with a user that does exist in Koha 13. Login succeeds 14. Kill IdP session and logout of Koha 15. Change default domain to "Update on login" 16. In an Incognito window or different browser, go to http://localhost:8080, and try logging in with a user that does exist in Koha (#FIXME: The update on login feature doesn't work. It needs to be fixed.) 17. Change default domain to "Allow staff" logins 18. In an Incognito window or different browser, go to http://localhost:8080, and try logging in with a user that does exist in Koha 19. Note that you get a normal "Error: You do not have permission to access this page" message. That's good. 20. Add "catalogue" permission to the Koha user 21. In an Incognito window or different browser, go to http://localhost:8080, and try logging in with a user that does exist in Koha 22. Login succeeds 23. In an Incognito window or different browser, go to http://localhost:8081, and try logging in with a user that doesn't exist in Koha (#FIXME: When authentication fails, the end user sees the following message: Error: Session timed out. Please log in again There was an error authenticating to external identity provider Exception 'Koha::Exceptions::Auth::Unauthorized' thrown 'External auth user cannot access resource' with code => 401 ) 24. Add a new domain for that matches the email domain of your IdP/Koha user. 25. Mark "Allow opac" and "Allow staff" as "No" 26. In an Incognito window or different browser, go to http://localhost:8080, and try logging in with a user that does exist in Koha (#FIXME: Koha login happens even though the specific domain says it shouldn't be allowed) 27. In an Incognito window or different browser, go to http://localhost:8081, and try logging in with a user that does exist in Koha (#FIXME: Koha login happens even though the specific domain says it shouldn't be allowed) -- I imagine that there are other problems, but I think that's a fairly thorough analysis of the core functionality.
I appreciate the replacement of "Authentication provider" being changed to "Identity provider" and "tranverse_hash" being changed to "traverse_hash", but it looks like every other issue I described last time is still present in the Gitlab version... I have to fail this again, because several of the features don't work. Before I test this again, I'd like someone to confirm that they've looked at my "FIXME" comments, and fixed them (or at least provide a reason for not fixing them).
Overall, I think that we're on the right track, and I'm so excited for this feature. Thanks for all the work, Theke.
Created attachment 143056 [details] [review] Bug 31378: DB changes
(In reply to David Cook from comment #105) > I appreciate the replacement of "Authentication provider" being changed to > "Identity provider" and "tranverse_hash" being changed to "traverse_hash", > but it looks like every other issue I described last time is still present > in the Gitlab version... > > I have to fail this again, because several of the features don't work. > > Before I test this again, I'd like someone to confirm that they've looked at > my "FIXME" comments, and fixed them (or at least provide a reason for not > fixing them). Hi David, could you please try again? I've attended to all your valuable FIXME comments in the last patch. Please let me know what you think. Thanks so much!
Created attachment 143087 [details] [review] Bug 31378: DB changes
Created attachment 143088 [details] [review] Bug 31378: DBIC updates [DO NOT PUSH]
Created attachment 143089 [details] [review] Bug 31378: Add dependencies
Created attachment 143090 [details] [review] Bug 31378: Add Koha::Auth::Provider::Domain(s) class(es)
Created attachment 143091 [details] [review] Bug 31378: Add Koha::Auth::Provider(s) class(es)
Created attachment 143092 [details] [review] Bug 31378: Add Koha::Auth::Client* modules
Created attachment 143093 [details] [review] Bug 31378: Add Auth mojo plugin
Created attachment 143094 [details] [review] Bug 31378: Add API routes
Created attachment 143095 [details] [review] Bug 31378: Add auth providers template plugin
Created attachment 143096 [details] [review] Bug 31378: Add the API-based auth mechanism to C4::Auth::check_api_auth
Created attachment 143097 [details] [review] Bug 31378: Add OPAC support for authentication providers
Created attachment 143098 [details] [review] Bug 31378: Add STAFF support for authentication providers
Created attachment 143099 [details] [review] Bug 31378: Add accessor methods for JSON attributes
Created attachment 143100 [details] [review] Bug 31378: Add manage_authentication_providers permission Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 143101 [details] [review] Bug 31378: Add 'parameter' parameter to Koha::Exceptions::MissingParameter Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 143102 [details] [review] Bug 31378: Add authentication provider endpoints This patch adds routes for handling authentication providers to the REST API. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/auth_providers.t => SUCCESS: Tests pass! 3. Sign off :-D
Created attachment 143103 [details] [review] Bug 31378: Add an administration interface for authentication sources
Created attachment 143104 [details] [review] Bug 31378: Rename Auth Provider to Identity Provider and add Client.t tests
Created attachment 143105 [details] [review] Bug 31378: (follow-up) do all FIXME pointed by David
Created attachment 143106 [details] [review] Bug 31378: Use base URLs for redirect uri Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu>
Created attachment 143107 [details] [review] Bug 31378: (follow-up) catch startup error when missing Oauth provider table In theory, this shouldn't come up with a package install, but it could happen with other installations, so best to let it get to the web installer.
(In reply to Agustín Moyano from comment #108) > Hi David, could you please try again? > > I've attended to all your valuable FIXME comments in the last patch. > > Please let me know what you think. > > Thanks so much! Thanks, Agustín. I don't have time to thoroughly test it today, and I'm away on Monday, but I'll do a quick glance over it now.
First things first...staying in Failed QA due to qa tool failures: -- testing 21 commit(s) (applied to ba01d76 '78 Bug 31713: Fix permissions') Processing files before patches Configure idp routes=====>| 48 / 48 (100.00%) Processing files after patches |========================>| 48 / 48 (100.00%) OK C4/Auth.pm OK Koha/Auth/Client.pm FAIL Koha/Auth/Client/OAuth.pm FAIL pod coverage POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666 OK Koha/Auth/Provider.pm OK Koha/Auth/Provider/Domain.pm OK Koha/Auth/Provider/Domains.pm OK Koha/Auth/Provider/OAuth.pm OK Koha/Auth/Provider/OIDC.pm OK Koha/Auth/Providers.pm OK Koha/Exceptions.pm FAIL Koha/Exceptions/Auth.pm FAIL pod coverage POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666 OK Koha/REST/Plugin/Auth.pm OK Koha/REST/V1.pm OK Koha/REST/V1/Auth/Provider/Domains.pm OK Koha/REST/V1/Auth/Providers.pm OK Koha/REST/V1/OAuth/Client.pm OK Koha/Schema/Result/AuthProvider.pm OK Koha/Schema/Result/AuthProviderDomain.pm OK Koha/Schema/Result/Branch.pm OK Koha/Schema/Result/Category.pm OK Koha/Schema/Result/IdentityProviderDomain.pm OK Koha/Template/Plugin/AuthClient.pm OK admin/authentication_providers.pl OK admin/identity_providers.pl OK api/v1/swagger/definitions/auth_provider.yaml OK api/v1/swagger/definitions/auth_provider_domain.yaml OK api/v1/swagger/paths/auth.yaml OK api/v1/swagger/paths/oauth.yaml OK api/v1/swagger/paths/public_oauth.yaml OK api/v1/swagger/swagger.yaml OK installer/data/mysql/atomicupdate/bug_31378.pl OK installer/data/mysql/kohastructure.sql OK koha-tmpl/intranet-tmpl/prog/en/includes/admin-menu.inc OK koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc OK koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt OK koha-tmpl/intranet-tmpl/prog/en/modules/admin/authentication_provider_domains.tt OK koha-tmpl/intranet-tmpl/prog/en/modules/admin/authentication_providers.tt OK koha-tmpl/intranet-tmpl/prog/en/modules/admin/identity_provider_domains.tt OK koha-tmpl/intranet-tmpl/prog/en/modules/admin/identity_providers.tt FAIL koha-tmpl/intranet-tmpl/prog/en/modules/auth.tt FAIL valid_template DBIx::Class::Schema::source(): Can't find source for IdentityProvider at /kohadevbox/koha/Koha/Objects.pm line 524 FAIL koha-tmpl/opac-tmpl/bootstrap/en/includes/masthead.inc FAIL valid_template DBIx::Class::Schema::source(): Can't find source for IdentityProvider at /kohadevbox/koha/Koha/Objects.pm line 524 FAIL koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth.tt FAIL valid_template DBIx::Class::Schema::source(): Can't find source for IdentityProvider at /kohadevbox/koha/Koha/Objects.pm line 524 FAIL koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-main.tt FAIL valid_template DBIx::Class::Schema::source(): Can't find source for IdentityProvider at /kohadevbox/koha/Koha/Objects.pm line 524 OK t/db_dependent/Koha/Auth/Client.t OK t/db_dependent/Koha/Auth/Provider.t OK t/db_dependent/Koha/REST/Plugin/Auth/IdP.t OK t/db_dependent/api/v1/idp.t FAIL t/lib/IdP/ExternalIdP.pm FAIL pod coverage POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666 Processing additional checks OK!
Looks like Koha/Template/Plugin/AuthClient.pm might be having an issue with Koha/Auth/Identity/Providers.pm... -- Test plan 1: 0. Apply patches 1. apt install libmojolicious-plugin-oauth2-perl 2. koha-upgrade-schema kohadev 3. restart_all Test plan 2: 1. Go to http://localhost:8081/cgi-bin/koha/admin/admin-home.pl 2. Click on "Identity providers" 3. Click "New identity provider" 4. Enter relevant details and click "Submit" 5. Click on "Manage Domains" 6. Click "Edit" on the default domains 7. Set "Allow staff" and allow update on login and autoregister and click "Submit" 8. koha-plack --restart kohadev 9. In an Incognito window or different browser, go to http://localhost:8080, and try logging in with a user that doesn't exist in Koha -- I'm getting this error: "There was an error authenticating to external identity provider No configuration found for your provider" I'm seeing the following error in the logs on Starman startup: [2022/11/04 06:16:26] [WARN] Warning: Failed to fetch oauth configuration: Can't locate object method "search" via package "Koha::Auth::Providers" (perhaps you forgot to load "Koha::Auth::Providers"?) at /kohadevbox/koha/Koha/REST/V1.pm line 143. [2022/11/04 06:16:26] [WARN] Warning: Failed to fetch oauth configuration: Can't locate object method "search" via package "Koha::Auth::Providers" (perhaps you forgot to load "Koha::Auth::Providers"?) at /kohadevbox/koha/Koha/REST/V1.pm line 143. -- Looks like a typo.
Agustín, how are you testing this during development? In my docker-compose.yml, I use the following: keycloak: image: jboss/keycloak networks: - kohanet environment: KEYCLOAK_USER: <username> KEYCLOAK_PASSWORD: <password> ports: - "8082:8080" If you are interested in using Keycloak for testing, I can give a few more instructions on how to set up the realm, the client, the user, and where to find the details for the Koha configuration. -- That actually reminds me that I don't have a test plan for the "OAuth" Protocol option. Does it still work with the OIDC option since OIDC is just a wrapper around OAuth? -- Anyway, that's all for me for now!
Created attachment 143186 [details] [review] Bug 31378: DB changes
Created attachment 143187 [details] [review] Bug 31378: DBIC updates [DO NOT PUSH]
Created attachment 143188 [details] [review] Bug 31378: Add dependencies
Created attachment 143189 [details] [review] Bug 31378: Add Koha::Auth::Provider::Domain(s) class(es)
Created attachment 143190 [details] [review] Bug 31378: Add Koha::Auth::Provider(s) class(es)
Created attachment 143191 [details] [review] Bug 31378: Add Koha::Auth::Client* modules
Created attachment 143192 [details] [review] Bug 31378: Add Auth mojo plugin
Created attachment 143193 [details] [review] Bug 31378: Add API routes
Created attachment 143194 [details] [review] Bug 31378: Add auth providers template plugin
Created attachment 143195 [details] [review] Bug 31378: Add the API-based auth mechanism to C4::Auth::check_api_auth
Created attachment 143196 [details] [review] Bug 31378: Add OPAC support for authentication providers
Created attachment 143197 [details] [review] Bug 31378: Add STAFF support for authentication providers
Created attachment 143198 [details] [review] Bug 31378: Add accessor methods for JSON attributes
Created attachment 143199 [details] [review] Bug 31378: Add manage_authentication_providers permission Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 143200 [details] [review] Bug 31378: Add 'parameter' parameter to Koha::Exceptions::MissingParameter Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 143202 [details] [review] Bug 31378: Add authentication provider endpoints This patch adds routes for handling authentication providers to the REST API. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/auth_providers.t => SUCCESS: Tests pass! 3. Sign off :-D
Created attachment 143204 [details] [review] Bug 31378: Add an administration interface for authentication sources
Created attachment 143205 [details] [review] Bug 31378: Rename Auth Provider to Identity Provider and add Client.t tests
Created attachment 143206 [details] [review] Bug 31378: (follow-up) do all FIXME pointed by David
Created attachment 143207 [details] [review] Bug 31378: Use base URLs for redirect uri Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu>
Created attachment 143208 [details] [review] Bug 31378: (follow-up) catch startup error when missing Oauth provider table In theory, this shouldn't come up with a package install, but it could happen with other installations, so best to let it get to the web installer.
(In reply to David Cook from comment #132) > Looks like Koha/Template/Plugin/AuthClient.pm might be having an issue with > Koha/Auth/Identity/Providers.pm... > > -- > > Test plan 1: > 0. Apply patches > 1. apt install libmojolicious-plugin-oauth2-perl > 2. koha-upgrade-schema kohadev > 3. restart_all > > Test plan 2: > 1. Go to http://localhost:8081/cgi-bin/koha/admin/admin-home.pl > 2. Click on "Identity providers" > 3. Click "New identity provider" > 4. Enter relevant details and click "Submit" > 5. Click on "Manage Domains" > 6. Click "Edit" on the default domains > 7. Set "Allow staff" and allow update on login and autoregister and click > "Submit" > > 8. koha-plack --restart kohadev > > 9. In an Incognito window or different browser, go to http://localhost:8080, > and try logging in with a user that doesn't exist in Koha > > -- > > I'm getting this error: > > "There was an error authenticating to external identity provider > > No configuration found for your provider" > > I'm seeing the following error in the logs on Starman startup: > > [2022/11/04 06:16:26] [WARN] Warning: Failed to fetch oauth configuration: > Can't locate object method "search" via package "Koha::Auth::Providers" > (perhaps you forgot to load "Koha::Auth::Providers"?) at > /kohadevbox/koha/Koha/REST/V1.pm > line 143. > [2022/11/04 06:16:26] [WARN] Warning: Failed to fetch oauth configuration: > Can't locate object method "search" via package "Koha::Auth::Providers" > (perhaps you forgot to load "Koha::Auth::Providers"?) at > /kohadevbox/koha/Koha/REST/V1.pm > line 143. > > -- > > Looks like a typo. Sorry, I applied your patch but didn't update Koha::Auth::Providers to Koha::Auth::Identity::Providers Now it should work Thanks!
Created attachment 143215 [details] [review] Bug 31378: DB changes Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 143216 [details] [review] Bug 31378: DBIC updates [DO NOT PUSH] Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 143217 [details] [review] Bug 31378: Add dependencies Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 143218 [details] [review] Bug 31378: Add Koha::Auth::Provider::Domain(s) class(es) Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 143219 [details] [review] Bug 31378: Add Koha::Auth::Provider(s) class(es) Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 143220 [details] [review] Bug 31378: Add Koha::Auth::Client* modules Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 143221 [details] [review] Bug 31378: Add Auth mojo plugin Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 143222 [details] [review] Bug 31378: Add API routes Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 143223 [details] [review] Bug 31378: Add auth providers template plugin Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 143224 [details] [review] Bug 31378: Add the API-based auth mechanism to C4::Auth::check_api_auth Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 143225 [details] [review] Bug 31378: Add OPAC support for authentication providers Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 143226 [details] [review] Bug 31378: Add STAFF support for authentication providers Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 143227 [details] [review] Bug 31378: Add accessor methods for JSON attributes Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 143228 [details] [review] Bug 31378: Add manage_authentication_providers permission Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 143229 [details] [review] Bug 31378: Add 'parameter' parameter to Koha::Exceptions::MissingParameter Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 143230 [details] [review] Bug 31378: Add authentication provider endpoints This patch adds routes for handling authentication providers to the REST API. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/auth_providers.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu>
Created attachment 143231 [details] [review] Bug 31378: Add an administration interface for authentication sources Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 143232 [details] [review] Bug 31378: Rename Auth Provider to Identity Provider and add Client.t tests Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 143233 [details] [review] Bug 31378: (follow-up) Fix QA concerns Several FIXME comments added on the report addressed here. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 143234 [details] [review] Bug 31378: Use base URLs for redirect uri Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 143235 [details] [review] Bug 31378: (follow-up) catch startup error when missing Oauth provider table In theory, this shouldn't come up with a package install, but it could happen with other installations, so best to let it get to the web installer. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 143236 [details] [review] Bug 31378: (QA follow-up) Tidy tests This patch perltidys the tests added lately. It also removes some commented debugging lines. Note: There's an attempt to write tests on the full auth workflow that is commented out but left there on purpose for now.
I restored Lucasz signature that was lost at some point.
Created attachment 143266 [details] [review] Bug 31378: DB changes Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143267 [details] [review] Bug 31378: DBIC updates [DO NOT PUSH] Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143268 [details] [review] Bug 31378: Add dependencies Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143269 [details] [review] Bug 31378: Add Koha::Auth::Provider::Domain(s) class(es) Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143270 [details] [review] Bug 31378: Add Koha::Auth::Provider(s) class(es) Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143271 [details] [review] Bug 31378: Add Koha::Auth::Client* modules Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143272 [details] [review] Bug 31378: Add Auth mojo plugin Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143273 [details] [review] Bug 31378: Add API routes Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143274 [details] [review] Bug 31378: Add auth providers template plugin Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143275 [details] [review] Bug 31378: Add the API-based auth mechanism to C4::Auth::check_api_auth Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143277 [details] [review] Bug 31378: Add OPAC support for authentication providers Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143278 [details] [review] Bug 31378: Add STAFF support for authentication providers Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143279 [details] [review] Bug 31378: Add accessor methods for JSON attributes Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143280 [details] [review] Bug 31378: Add manage_authentication_providers permission Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143281 [details] [review] Bug 31378: Add 'parameter' parameter to Koha::Exceptions::MissingParameter Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143282 [details] [review] Bug 31378: Add authentication provider endpoints This patch adds routes for handling authentication providers to the REST API. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/auth_providers.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143283 [details] [review] Bug 31378: Add an administration interface for authentication sources Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143284 [details] [review] Bug 31378: Rename Auth Provider to Identity Provider and add Client.t tests Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143285 [details] [review] Bug 31378: (follow-up) Fix QA concerns Several FIXME comments added on the report addressed here. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143286 [details] [review] Bug 31378: Use base URLs for redirect uri Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143287 [details] [review] Bug 31378: (follow-up) catch startup error when missing Oauth provider table In theory, this shouldn't come up with a package install, but it could happen with other installations, so best to let it get to the web installer. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143288 [details] [review] Bug 31378: (QA follow-up) Tidy tests This patch perltidys the tests added lately. It also removes some commented debugging lines. Note: There's an attempt to write tests on the full auth workflow that is commented out but left there on purpose for now. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
(In reply to David Cook from comment #133) > Agustín, how are you testing this during development? > > In my docker-compose.yml, I use the following: > > keycloak: > image: jboss/keycloak > networks: > - kohanet > environment: > KEYCLOAK_USER: <username> > KEYCLOAK_PASSWORD: <password> > ports: > - "8082:8080" > > If you are interested in using Keycloak for testing, I can give a few more > instructions on how to set up the realm, the client, the user, and where to > find the details for the Koha configuration. I've just added an --sso option switch to the 'ktd' command, using the examples you provided (with minor tweaks). Thanks!
(In reply to Tomás Cohen Arazi from comment #201) > I've just added an --sso option switch to the 'ktd' command, using the > examples you provided (with minor tweaks). I use a bit of a custom setup, but I'll include your docker-compose.sso.yml :D
(In reply to David Cook from comment #202) > (In reply to Tomás Cohen Arazi from comment #201) > > I've just added an --sso option switch to the 'ktd' command, using the > > examples you provided (with minor tweaks). > > I use a bit of a custom setup, but I'll include your docker-compose.sso.yml > :D Looks like the docker-compose.sso.yml still needs some work, but I've emailed about fixing that.
Test plan 1: 0. Apply patches 1. apt install libmojolicious-plugin-oauth2-perl 2. koha-upgrade-schema kohadev 3. restart_all Test plan 2a (Quarkus Keycloak): 1. Go to http://<workstation_ip>:8082/ 2. Click "Administration Console" 3. Login to master realm 4. Click on dropdown menu that says "master" 5. Click "Create Realm" 6. Type "test" into "Realm name" and click "Create" 7. Click "Clients" 8. Click "Create client" 9. Type "test" into "Client ID" 10. Click "Next" 11. Set "Client authentication" to "On" 12. Click "Save" 13. Add "Valid redirect URIs" of "http://localhost:8080/*" and "http://localhost:8081/*" 14. Click "Credentials" tab and note the "Client secret" 15. Click "Advanced" tab and turn "Exclude Session State From Authentication Response" to "On" then click "Save" 16. Click "Users" on left nav 17. Click "Create new user" 18. Type "kohadev" into "Username" 19. Type "kohadev@koha-community.org" into Email" 20. Type "Firstname" into "First name" 21. Type "Lastname" into "Last name" 22. Click "Create" 23. Click "Credentials" tab and click "Set password" 24. Choose a password, turn off "Temporary", and click "Save" 25. Click "Realm settings" from the left nav 26. Click "OpenID Endpoint Configuration" at the bottom of the page Test plan 2b: 1. Go to http://localhost:8081/cgi-bin/koha/admin/admin-home.pl 2. Click on "Identity providers" 3. Click "New identity provider" 4a. Type "test" into "Code" 4b. Type "Test" into "Description" 4c. Choose "OIDC" for "Protocol" 4d. Click "Add default OIDC configuration" 4e. Click "Add default OIDC mapping" 4f. Update "Configuration" to match Keycloak details for "key", "secret", and "well_known_url" 5. Click on "Manage Domains" 6. Click "Edit" on the default domains 7. Set "Allow staff" and allow update on login and autoregister and click "Submit" 8. koha-plack --restart kohadev 9. Update OPACBaseURL and staffClientBaseURL to http://localhost:8080 and http://localhost:8081 respectively Test plan 3: 1. In an Incognito window or different browser, go to http://localhost:8080, and try logging in with a user that doesn't exist in Koha -- I got the following error: There was an error authenticating to external identity provider DBIC result _type isn't of the _type Category at /usr/share/perl5/DBIx/Class/Storage/BlockRunner.pm line 130. Looks like auto register doesn't work still... I changed the email address for the "koha" user to "kohadev@koha-community.org" at http://localhost:8081/cgi-bin/koha/members/memberentry.pl?op=modify&destination=circ&borrowernumber=51 Login works, but it doesn't update the user details on login either. Going to have to fail this again.
When I add a Domain rule that forbids login for staff interface or OPAC, it says "No configuration found for your email domain". I think you could just leave it as "There was an error authenticating to external identity provider". But that's just a comment and not a blocker.
(In reply to David Cook from comment #204) > I got the following error: > > There was an error authenticating to external identity provider > > DBIC result _type isn't of the _type Category at > /usr/share/perl5/DBIx/Class/Storage/BlockRunner.pm line 130. > > Looks like auto register doesn't work still... It looks like I had forgotten to provide "Default library" and "Default category". When I do provide "Default library" and "Default category", it does auto register a new patron! Yay! If I provide a "Default category" but no "Default library", then I get this error: "There was an error authenticating to external identity provider Broken FK constraint" -- I think for these two scenarios, you just shouldn't show the error on the Web UI. Log it to the log files and leave it at that. It's enough to tell the end user that "There was an error authentication to external identity provider.
I've double-checked that my ID token and UserInfo responses include "given_name" and "family_name" and they do. I've noticed with the auto register that it isn't populating the "firstname" and "surname" fields. That's probably why the update on login isn't working either. -- So I think fixes required are: 1) Hide overly revealing error messages in Web UI 2) Fix mapping so that auto-register "update on login" works and auto register includes mappings beyond just email.
(In reply to David Cook from comment #207) > So I think fixes required are: > > 1) Hide overly revealing error messages in Web UI > 2) Fix mapping so that auto-register "update on login" works and auto > register includes mappings beyond just email. I notice in the help text that it says "If you plan to use auto register feature, either userid or cardnumber must be present in this mapping". That doesn't appear to be true in my testing.
(In reply to David Cook from comment #208) > (In reply to David Cook from comment #207) > > So I think fixes required are: > > > > 1) Hide overly revealing error messages in Web UI > > 2) Fix mapping so that auto-register "update on login" works and auto > > register includes mappings beyond just email. > > I notice in the help text that it says "If you plan to use auto register > feature, either userid or cardnumber must be present in this mapping". > > That doesn't appear to be true in my testing. Any NOT NULL attribute on the borrowers table needs to be mapped for auto-register. I don't really understand this one.
Created attachment 143396 [details] [review] Bug 31378: DB changes Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143397 [details] [review] Bug 31378: DBIC updates [DO NOT PUSH] Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143398 [details] [review] Bug 31378: Add dependencies Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143399 [details] [review] Bug 31378: Add Koha::Auth::Provider::Domain(s) class(es) Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143400 [details] [review] Bug 31378: Add Koha::Auth::Provider(s) class(es) Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143401 [details] [review] Bug 31378: Add Koha::Auth::Client* modules Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143402 [details] [review] Bug 31378: Add Auth mojo plugin Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143403 [details] [review] Bug 31378: Add API routes Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143404 [details] [review] Bug 31378: Add auth providers template plugin Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143405 [details] [review] Bug 31378: Add the API-based auth mechanism to C4::Auth::check_api_auth Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143406 [details] [review] Bug 31378: Add OPAC support for authentication providers Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143407 [details] [review] Bug 31378: Add STAFF support for authentication providers Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143408 [details] [review] Bug 31378: Add accessor methods for JSON attributes Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143409 [details] [review] Bug 31378: Add manage_authentication_providers permission Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143410 [details] [review] Bug 31378: Add 'parameter' parameter to Koha::Exceptions::MissingParameter Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143411 [details] [review] Bug 31378: Add authentication provider endpoints This patch adds routes for handling authentication providers to the REST API. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/auth_providers.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143412 [details] [review] Bug 31378: Add an administration interface for authentication sources Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143413 [details] [review] Bug 31378: Rename Auth Provider to Identity Provider and add Client.t tests Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143414 [details] [review] Bug 31378: (follow-up) Fix QA concerns Several FIXME comments added on the report addressed here. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143415 [details] [review] Bug 31378: Use base URLs for redirect uri Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143416 [details] [review] Bug 31378: (follow-up) catch startup error when missing Oauth provider table In theory, this shouldn't come up with a package install, but it could happen with other installations, so best to let it get to the web installer. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143417 [details] [review] Bug 31378: (QA follow-up) Tidy tests This patch perltidys the tests added lately. It also removes some commented debugging lines. Note: There's an attempt to write tests on the full auth workflow that is commented out but left there on purpose for now. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 143418 [details] [review] Bug 31378: Add domain settings to IdP add page This patch introduces a small form for an initial domain configuration to the 'New identity provider' page, to avoid confusion on testers about required settings for things to work. To test: 1. Apply this patch 2. Verify you are now offered a configuration section for domain settings. Fields like 'Default category' and 'Default library' are mandatory. 3. Choosing some values makes them get stored on 'Submit' Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 143420 [details] [review] Bug 31378: DB changes Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 143421 [details] [review] Bug 31378: DBIC updates [DO NOT PUSH] Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 143422 [details] [review] Bug 31378: Add dependencies Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 143423 [details] [review] Bug 31378: Add Koha::Auth::Provider::Domain(s) class(es) Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 143424 [details] [review] Bug 31378: Add Koha::Auth::Provider(s) class(es) Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 143425 [details] [review] Bug 31378: Add Koha::Auth::Client* modules Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 143426 [details] [review] Bug 31378: Add Auth mojo plugin Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 143427 [details] [review] Bug 31378: Add API routes Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 143428 [details] [review] Bug 31378: Add auth providers template plugin Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 143429 [details] [review] Bug 31378: Add the API-based auth mechanism to C4::Auth::check_api_auth Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 143430 [details] [review] Bug 31378: Add OPAC support for authentication providers Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 143431 [details] [review] Bug 31378: Add STAFF support for authentication providers Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 143432 [details] [review] Bug 31378: Add accessor methods for JSON attributes Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 143433 [details] [review] Bug 31378: Add manage_authentication_providers permission Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 143434 [details] [review] Bug 31378: Add 'parameter' parameter to Koha::Exceptions::MissingParameter Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 143435 [details] [review] Bug 31378: Add authentication provider endpoints This patch adds routes for handling authentication providers to the REST API. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/auth_providers.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 143436 [details] [review] Bug 31378: Add an administration interface for authentication sources Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 143437 [details] [review] Bug 31378: Rename Auth Provider to Identity Provider and add Client.t tests Signed-off-by: Lukasz Koszyk <lukasz.koszyk@kit.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 143438 [details] [review] Bug 31378: (follow-up) Fix QA concerns Several FIXME comments added on the report addressed here. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 143439 [details] [review] Bug 31378: Use base URLs for redirect uri Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 143440 [details] [review] Bug 31378: (follow-up) catch startup error when missing Oauth provider table In theory, this shouldn't come up with a package install, but it could happen with other installations, so best to let it get to the web installer. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 143441 [details] [review] Bug 31378: (QA follow-up) Tidy tests This patch perltidys the tests added lately. It also removes some commented debugging lines. Note: There's an attempt to write tests on the full auth workflow that is commented out but left there on purpose for now. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 143442 [details] [review] Bug 31378: Add domain settings to IdP add page This patch introduces a small form for an initial domain configuration to the 'New identity provider' page, to avoid confusion on testers about required settings for things to work. To test: 1. Apply this patch 2. Verify you are now offered a configuration section for domain settings. Fields like 'Default category' and 'Default library' are mandatory. 3. Choosing some values makes them get stored on 'Submit' Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 143443 [details] [review] Bug 31378: DBIC Build Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Pushed to master for 22.11. Nice work everyone, thanks!
Thanks everyone. Any issues you might find, please file follow-up bugs. Fixes/enhancements are welcome of course.
Created attachment 143450 [details] [review] Bug 31378: (follow-up) MySQL 8 doens't allow default for longtext Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Tomás Cohen Arazi from comment #209) > > I notice in the help text that it says "If you plan to use auto register > > feature, either userid or cardnumber must be present in this mapping". > > > > That doesn't appear to be true in my testing. > > Any NOT NULL attribute on the borrowers table needs to be mapped for > auto-register. I don't really understand this one. Both userid and cardnumber can be null though. Auto-register works without them being mapped, although perhaps it shouldn't.
(In reply to Tomás Cohen Arazi from comment #258) > Thanks everyone. Any issues you might find, please file follow-up bugs. > Fixes/enhancements are welcome of course. Sounds good. I'll give it another test this morning, and open follow-up bugs for any issues I find. Glad to see this one in master in any case!
I've raised bug 32138 and bug 32139 as follow-ups, but they just relate to some configuration-related fixes. They should be easy to fix, so I'll write patches for them shortly. When they're fixed, I think that I'll be happy enough with this feature. (I'll probably raise 1 more follow-up soon but it's a minor UI tweak.) I also raised bug 32140 because I'm a bit concerned that the "interface" is set to "api" rather than the relevant "intranet" or "opac", but I haven't observed any obvious problems with that yet, so... I'm not making it a priority right now. -- Thanks again to Agustin and Tomas and everyone else for making this happen. I've carried a local customization to enable OIDC auth in Koha since 2014, so I'll be glad to put that to rest, and just focus on helping make this core OIDC client the best it can be.
Error while loading /etc/koha/plack.psgi: Can't load application from file "/usr/share/koha/api/v1/app.pl": Can't locate Mojolicious/Plugin/OAuth2.pm in @INC Should we mark this as dependency for Mason please? apt-get install libmojolicious-plugin-oauth2-perl apt-get install libmojolicious-plugin-renderfile-perl
Hola, Marcel! (In reply to Marcel de Rooy from comment #263) > Error while loading /etc/koha/plack.psgi: Can't load application from file > "/usr/share/koha/api/v1/app.pl": Can't locate Mojolicious/Plugin/OAuth2.pm > in @INC > > Should we mark this as dependency for Mason please? > > apt-get install libmojolicious-plugin-oauth2-perl > apt-get install libmojolicious-plugin-renderfile-perl Dependencies are packaged (by Mason) and already present on the latest KTD images. Thanks for reporting, I will mention it on the list for other devs to be aware!
(In reply to Marcel de Rooy from comment #263) > Error while loading /etc/koha/plack.psgi: Can't load application from file > "/usr/share/koha/api/v1/app.pl": Can't locate Mojolicious/Plugin/OAuth2.pm > in @INC > > Should we mark this as dependency for Mason please? > > apt-get install libmojolicious-plugin-oauth2-perl > apt-get install libmojolicious-plugin-renderfile-perl I can see 'Mojolicious::Plugin::RenderFile' in cpanfile, but I can't see 'Mojolicious::Plugin::OAuth2'. I think cpanfile might need an update? I think debian/control gets updated at release time...
(In reply to David Cook from comment #265) > (In reply to Marcel de Rooy from comment #263) > > Error while loading /etc/koha/plack.psgi: Can't load application from file > > "/usr/share/koha/api/v1/app.pl": Can't locate Mojolicious/Plugin/OAuth2.pm > > in @INC > > > > Should we mark this as dependency for Mason please? > > > > apt-get install libmojolicious-plugin-oauth2-perl > > apt-get install libmojolicious-plugin-renderfile-perl > > I can see 'Mojolicious::Plugin::RenderFile' in cpanfile, but I can't see > 'Mojolicious::Plugin::OAuth2'. The third patch introduced it.
(In reply to Tomás Cohen Arazi from comment #266) > The third patch introduced it. Ahhh, I see it now. I expected it to be in the usual alphabetical order, but I do see it now. All good.
*** Bug 30988 has been marked as a duplicate of this bug. ***
*** Bug 28420 has been marked as a duplicate of this bug. ***
Created attachment 144071 [details] [review] Bug 31378: (QA follow-up) Defer store until JSON fields are populated Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 144175 [details] [review] Bug 31378: Tests shouldn't remove all patrons Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Hello everyone! This is a bit technical for me, but I attempted to add the information to the manual. Would any of you who worked on this have the time to read what I wrote and let me know if I mangled anything? (It's not very long) https://gitlab.com/koha-community/koha-manual/-/merge_requests/670 Thanks! Caroline