Bug 30988 - Add generic OpenIDConnect client implementation
Summary: Add generic OpenIDConnect client implementation
Status: RESOLVED DUPLICATE of bug 31378
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Shi Yao Wang
QA Contact: Testopia
URL:
Keywords:
: 21586 (view as bug list)
Depends on:
Blocks:
 
Reported: 2022-06-17 19:16 UTC by Shi Yao Wang
Modified: 2022-11-14 07:54 UTC (History)
7 users (show)

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


Attachments
Bug 30988: Adding a more generic version of googleopenidconnect (23.02 KB, patch)
2022-06-17 19:18 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 30988: Adding a more generic version of googleopenidconnect (22.97 KB, patch)
2022-06-17 19:42 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 30988: Adding a more generic version of googleopenidconnect (23.13 KB, patch)
2022-06-20 21:08 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 30988: Add generic OpenIDConnect client implementation (26.16 KB, patch)
2022-06-21 15:16 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 30988: Moving id token validation code into a module (12.51 KB, patch)
2022-06-21 15:16 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 30988: Moving id token validation code into a module (12.64 KB, patch)
2022-06-21 15:39 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 30988: Add generic OpenIDConnect client implementation (26.79 KB, patch)
2022-06-22 21:35 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 30988: Moving id token validation code into a module (12.64 KB, patch)
2022-06-22 21:35 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 30988: Add generic OpenIDConnect client implementation (26.24 KB, patch)
2022-06-23 13:31 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 30988: Moving id token validation code into a module (12.64 KB, patch)
2022-06-23 13:31 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 30988: Add generic OpenIDConnect client implementation (26.30 KB, patch)
2022-06-24 00:33 UTC, David Cook
Details | Diff | Splinter Review
Bug 30988: Moving id token validation code into a module (12.69 KB, patch)
2022-06-24 00:33 UTC, David Cook
Details | Diff | Splinter Review
Bug 30988: Fix minor formatting issues (5.13 KB, patch)
2022-06-24 00:34 UTC, David Cook
Details | Diff | Splinter Review
Bug 30988: Add generic OpenIDConnect client implementation (26.29 KB, patch)
2022-07-06 15:41 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 30988: Moving id token validation code into a module (12.69 KB, patch)
2022-07-06 15:41 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 30988: Fix minor formatting issues (5.08 KB, patch)
2022-07-06 15:41 UTC, Shi Yao Wang
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Shi Yao Wang 2022-06-17 19:16:48 UTC
Using any OpenID provider's discovery document, we could allow openid connections from more providers.

There is Bug 21586 that already has a patch for generic OpenID implementation, but it uses a different approach from my proposed patch.
Comment 1 Shi Yao Wang 2022-06-17 19:18:32 UTC
Created attachment 136301 [details] [review]
Bug 30988: Adding a more generic version of googleopenidconnect

More generic OpenID Connect based off of googleopenidconnect

Test plan:
1- Set up an OAuth2 provider that has a discovery doc (e.g. for google it's https://accounts.google.com/.well-known/openid-configuration) and note down the client id and client secret.
Docs to help setup:
google: https://developers.google.com/identity/protocols/oauth2/openid-connect
        https://koha-community.org/manual/20.11/en/html/administrationpreferences.html
gitlab: https://docs.gitlab.com/ee/integration/oauth_provider.html
2- Apply the patch
3- Run atomicupdate to update database
4- Configure system preferences:
    a- Set OpenIDConnect to Yes
    b- Enter the url to the discovery doc of your OAuth2 provider
    c- Fill OpenIDOAuth2ClientID with noted client id
    d- Fill OpenIDOAuth2ClientSecret with noted client secret

    e- Configure the other related system preferences as you wish. They
shoud work the same way as the similarly named GoogleOpenID* ones.
5- If OpenIDConnectAutoRegister is set to Don't allow, have a koha
account with the same email as the one used by your OAuth2 provider.
6- In OPAC, sign in using OpenID.
7- You may have to log into your OpenID provider account and accept conditions.
It should finally log you into the koha account.
Comment 2 Shi Yao Wang 2022-06-17 19:22:40 UTC
I tested this with google and gitlab.

If this works, it theoretically offers the same functionality as googleopenidconnect, but with the possibility of using other OAuth2 providers.
Comment 3 Shi Yao Wang 2022-06-17 19:42:07 UTC
Created attachment 136302 [details] [review]
Bug 30988: Adding a more generic version of googleopenidconnect

More generic OpenID Connect based off of googleopenidconnect

Test plan:
1- Set up an OAuth2 provider that has a discovery doc (e.g. for google it's https://accounts.google.com/.well-known/openid-configuration) and note down the client id and client secret.
Docs to help setup:
google: https://developers.google.com/identity/protocols/oauth2/openid-connect
        https://koha-community.org/manual/20.11/en/html/administrationpreferences.html
gitlab: https://docs.gitlab.com/ee/integration/oauth_provider.html
2- Apply the patch
3- Run atomicupdate to update database
4- Configure system preferences:
    a- Set OpenIDConnect to Yes
    b- Enter the url to the discovery doc of your OAuth2 provider
    c- Fill OpenIDOAuth2ClientID with noted client id
    d- Fill OpenIDOAuth2ClientSecret with noted client secret

    e- Configure the other related system preferences as you wish. They
shoud work the same way as the similarly named GoogleOpenID* ones.
5- If OpenIDConnectAutoRegister is set to Don't allow, have a koha
account with the same email as the one used by your OAuth2 provider.
6- In OPAC, sign in using OpenID.
7- You may have to log into your OpenID provider account and accept conditions.
It should finally log you into the koha account.
Comment 4 Shi Yao Wang 2022-06-17 20:10:06 UTC
The main goal of the patch is to allow our clients to use OpenID connect with more flexibility. For example, some schools that use Office 365 would want OpenID connection to their libraries for their students.

What do you think?
Comment 5 David Cook 2022-06-19 23:33:25 UTC
I'm glad to see someone working on this, since bug 21586 is out of date now. I have a more up-to-date version that I run locally, but I've been thinking about updating it to use a library[1] to handle the OpenID Connect side of things.

I'll make some comments using the review as well...

[1]
https://metacpan.org/pod/Mojolicious::Plugin::OAuth2
https://metacpan.org/pod/LWP::Authen::OAuth2
Comment 6 David Cook 2022-06-19 23:36:53 UTC
(In reply to David Cook from comment #5)
> https://metacpan.org/pod/LWP::Authen::OAuth2

Looks like the author/maintainer for this library is our very own Thomas Klausner (domm) so I'd be curious to hear more about how he uses this module...
Comment 7 David Cook 2022-06-19 23:37:08 UTC Comment hidden (obsolete)
Comment 8 David Cook 2022-06-19 23:58:52 UTC
Comment on attachment 136302 [details] [review]
Bug 30988: Adding a more generic version of googleopenidconnect

Review of attachment 136302 [details] [review]:
-----------------------------------------------------------------

I don't think system preferences are the right way to go, because they lock you into only using 1 identity provider. While some organisations only need to use 1, I've had multiple clients where they've needed to use at least 2 different identity providers. 

I seem to recall that Tomas was going to do some work on a web form to allow superlibrarians to define their own identity providers for Koha.

::: opac/svc/auth/openidconnect
@@ +39,5 @@
> +use HTTP::Request::Common qw{ POST };
> +use JSON;
> +use MIME::Base64 qw{ decode_base64url };
> +
> +my $discoveryDocURL  = C4::Context->preference('OpenIDConfigURL');

I notice you often use "OpenID" but I think it would be more appropriate to use the abbreviation "OIDC" if you're not going to write out OpenIDConnect fully.

@@ +54,5 @@
> +my $clientid     = C4::Context->preference('OpenIDOAuth2ClientID');
> +my $clientsecret = C4::Context->preference('OpenIDOAuth2ClientSecret');
> +
> +my $ua       = LWP::UserAgent->new();
> +my $response = $ua->get($discoveryDocURL);

I think this is great. I've been meaning to switch over to this for years...

@@ +131,5 @@
> +                'Authentication failed. Incorrect token type.' );
> +        }
> +        my $idtoken = $json->{'id_token'};
> +
> +        # need to validate the token here

I'd suggest putting this validation code into a function, and putting your functions into a module where they can be unit tested.

@@ +183,5 @@
> +            else {
> +                my $error_feedback =
> +'The email address you are trying to use is not associated with a borrower at this library.';
> +                my $auto_registration = C4::Context->preference('OpenIDConnectAutoRegister') // q{0};
> +                my $borrower = Koha::Patrons->find( { email => $email } );

This won't work properly for pre-existing patrons that might have their email saved into "emailpro". It would be wise to search both email and emailpro for borrowers.

@@ +188,5 @@
> +                if (! $borrower && $auto_registration==1) {
> +                    my $firstname = $claims_json->{'given_name'} // q{};
> +                    my $surname = $claims_json->{'family_name'} // q{};
> +                    my $delimiter = $firstname ? q{.} : q{};
> +                    my $userid = $firstname . $delimiter . $surname;

You'll need some error handling here in the unlikely event that there's no given_name or family_name claims.

@@ +196,5 @@
> +                    my $library = Koha::Libraries->find( $branchcode );
> +                    if (defined $patron_category && defined $library) {
> +                        my $password = undef;
> +                        # TODO errors handling!
> +                        my $borrower = Koha::Patron->new({

It looks like you're only using the standard claims of email, given_name, and family_name. I've noticed that it's common to have additional custom claims that you may need to map into Koha (e.g. sort1). So it would be good to have that mapping capacity here.

@@ +251,5 @@
> +    my $prompt = $query->param('reauthenticate') // q{};
> +    if ( $authendpoint eq q{} ) {
> +        loginfailed( $query, 'Unable to discover authorisation endpoint.' );
> +    }
> +    my $authorisationurl =

You might want to consider using the "query_form" method in the URI module. It makes for cleaner url building.
Comment 9 David Cook 2022-06-20 00:06:16 UTC
(In reply to David Cook from comment #8)
> @@ +183,5 @@
> > +            else {
> > +                my $error_feedback =
> > +'The email address you are trying to use is not associated with a borrower at this library.';
> > +                my $auto_registration = C4::Context->preference('OpenIDConnectAutoRegister') // q{0};
> > +                my $borrower = Koha::Patrons->find( { email => $email } );
> 
> This won't work properly for pre-existing patrons that might have their
> email saved into "emailpro". It would be wise to search both email and
> emailpro for borrowers.
> 

Actually, it looks like C4::Auth::checkauth() will only look for patrons using "email" anyway, so that shortcoming already exists in Koha. C4::Auth::checkauth() should probably be patched too..
Comment 10 Shi Yao Wang 2022-06-20 21:08:51 UTC
Created attachment 136382 [details] [review]
Bug 30988: Adding a more generic version of googleopenidconnect

More generic OpenID Connect based off of googleopenidconnect

Test plan:
1- Set up an OAuth2 provider that has a discovery doc (e.g. for google it's https://accounts.google.com/.well-known/openid-configuration) and note down the client id and client secret.
Docs to help setup:
google: https://developers.google.com/identity/protocols/oauth2/openid-connect
        https://koha-community.org/manual/20.11/en/html/administrationpreferences.html
gitlab: https://docs.gitlab.com/ee/integration/oauth_provider.html
2- Apply the patch
3- Run atomicupdate to update database
4- Configure system preferences:
    a- Set OpenIDConnect to Yes
    b- Enter the url to the discovery doc of your OAuth2 provider
    c- Fill OpenIDOAuth2ClientID with noted client id
    d- Fill OpenIDOAuth2ClientSecret with noted client secret

    e- Configure the other related system preferences as you wish. They
shoud work the same way as the similarly named GoogleOpenID* ones.
5- If OpenIDConnectAutoRegister is set to Don't allow, have a koha
account with the same email as the one used by your OAuth2 provider.
6- In OPAC, sign in using OpenID.
7- You may have to log into your OpenID provider account and accept conditions.
It should finally log you into the koha account.
Comment 11 David Cook 2022-06-20 23:30:56 UTC
Comment on attachment 136382 [details] [review]
Bug 30988: Adding a more generic version of googleopenidconnect

Review of attachment 136382 [details] [review]:
-----------------------------------------------------------------

::: opac/svc/auth/openidconnect
@@ +185,5 @@
> +            else {
> +                my $error_feedback =
> +'The email address you are trying to use is not associated with a borrower at this library.';
> +                my $auto_registration = C4::Context->preference('OIDCAutoRegister') // q{0};
> +                my $borrower = Koha::Patrons->find( { email => $email }, { emailpro => $email } );

I should've removed my earlier comment about "emailpro" since the C4::Auth::checkauth() only checks "email" for the email based authentication. Sorry!
Comment 12 David Cook 2022-06-21 00:19:43 UTC
My test plan:
0. Set up koha-testing-docker with a jboss/keycloak container as per https://hub.docker.com/r/jboss/keycloak/
0b. Create "test" realm with discovery doc: http://<my_ip>:8082/auth/realms/test/.well-known/openid-configuration
0c. Create condiential OIDC client "koha" in "test" realm
0d. Create "test" user with email "test@test.test" and password "test"
0e. Fix "OPACBaseURL" so that it resolves to localhost instead of a non-existent domain name
1. Apply patch
2. koha-plack --restart kohadev
3*. "koha-upgrade-schema kohadev" didn't work so had to manually apply DB update via: koha-mysql kohadev < installer/data/mysql/atomicupdate/openidconnect.sql
4. Set "OIDC" syspref to "Yes"
5. Set "OIDCAutoRegister" to "Allow"
6. Set "OIDCConfigURL" to "http://<my_ip>:8080/auth/realms/test/.well-known/openid-configuration" 
7. Set "OIDCDefaultBranch" to "CPL"
8. Set "OIDCDefaultCategory" to "Patron"
9. Set "OIDCOAuth2ClientID" to my Keycloak client id
10. Set "OIDCOAuth2ClientSecret" to my Keycloak client secret
11. Go to http://localhost:8080/cgi-bin/koha/opac-main.pl
12. Click "Log in to your account"
13. Fill out your username and password in Keycloak
14. Success! Returned to a logged in OPAC with new auto-registered borrower

However, at the moment, this patch would fail for a few reasons:

1. The atomic update doesn't look like it's set up correctly. It should be automatically detected by koha-upgrade-schema
2. "Log in with OpenID" button on login failure is misnamed (It's "OpenID Connect" and not "OpenID". "OpenID" is an older standard).
3. "Log in with OpenID" button is not readable. It is white text on a white background.

--

On a side note, it would probably be a good idea to add support for OpenID Connect logout as well, so that you're logged out of the SSO provider when you're logged out of Koha. While this might not be desirable at home, on public terminals it wouldn't be great if people logged out of Koha and then a stranger came along and was able to re-login as them...
Comment 13 David Cook 2022-06-21 00:29:23 UTC
Overall, it works pretty well! If you fix the atomic update and the text/styling, I think I'd be happy to sign this off. 

QA might knock it back because of the lack of unit tests though. It would be a good idea to move a lot of the code out of "opac/svc/auth/openidconnect" and into a Koha/Auth/Client/OIDC module. 

--

Also, I'm getting these warnings in the logs for failed logins (I had some issues initially setting up my Keycloak which caused some failures):

[2022/06/21 00:02:03] [WARN] Useless use of private variable in void context at /kohadevbox/koha/opac/svc/auth/openidconnect line 90.
[2022/06/21 00:02:03] [WARN] Useless use of anonymous hash ({}) in void context at /kohadevbox/koha/opac/svc/auth/openidconnect line 90.
Comment 14 David Cook 2022-06-21 00:32:01 UTC
I think you could probably rename this to something like "Add generic OpenIDConnect client implementation".

I'll mark bug 21586 as a duplicate.
Comment 15 David Cook 2022-06-21 00:32:20 UTC
*** Bug 21586 has been marked as a duplicate of this bug. ***
Comment 16 Shi Yao Wang 2022-06-21 15:16:51 UTC
Created attachment 136396 [details] [review]
Bug 30988: Add generic OpenIDConnect client implementation

A generic OpenID Connect implementation.

Test plan:
1- Set up an OAuth2 provider that has a discovery doc (e.g. for google it's https://accounts.google.com/.well-known/openid-configuration) and note down the client id and client secret.
Docs to help setup:
google: https://developers.google.com/identity/protocols/oauth2/openid-connect
        https://koha-community.org/manual/20.11/en/html/administrationpreferences.html
gitlab: https://docs.gitlab.com/ee/integration/oauth_provider.html
2- Apply the patch
3- Run atomicupdate to update database
4- Configure system preferences:
    a- Set OIDC to Yes
    b- Enter the url to the discovery doc of your OAuth2 provider
    c- Fill OIDCOAuth2ClientID with noted client id
    d- Fill OIDCOAuth2ClientSecret with noted client secret

    e- Configure the other related system preferences as you wish. They
shoud work the same way as the similar googleopenidconnect system
preferences.
5- If OIDCAutoRegister is set to Don't allow, have a koha
account with the same email as the one used by your OAuth2 provider.
6- In OPAC, sign in using OpenID Connect.
7- You may have to log into your OpenID provider account and accept conditions.
It should finally log you into the koha account.
Comment 17 Shi Yao Wang 2022-06-21 15:16:55 UTC
Created attachment 136397 [details] [review]
Bug 30988: Moving id token validation code into a module
Comment 18 Shi Yao Wang 2022-06-21 15:39:22 UTC
Created attachment 136403 [details] [review]
Bug 30988: Moving id token validation code into a module
Comment 19 Shi Yao Wang 2022-06-21 16:05:15 UTC
Fixed everything except mapping custom claims (comment #8), OIDC logout (comment #12) and warnings (comment #13).
Comment 20 Shi Yao Wang 2022-06-21 17:39:54 UTC
I will be looking into OIDC logout
Comment 21 Shi Yao Wang 2022-06-22 14:03:54 UTC
I don't see a simple and consistent way to do OIDC logout from my little research on the subject, so I will put this on Needs signoff.
Comment 22 Shi Yao Wang 2022-06-22 21:35:35 UTC
Created attachment 136430 [details] [review]
Bug 30988: Add generic OpenIDConnect client implementation

A generic OpenID Connect implementation.

Test plan:
1- Set up an OAuth2 provider that has a discovery doc (e.g. for google it's https://accounts.google.com/.well-known/openid-configuration) and note down the client id and client secret.
Docs to help setup:
google: https://developers.google.com/identity/protocols/oauth2/openid-connect
        https://koha-community.org/manual/20.11/en/html/administrationpreferences.html
gitlab: https://docs.gitlab.com/ee/integration/oauth_provider.html
2- Apply the patch
3- Run atomicupdate to update database
4- Configure system preferences:
    a- Set OIDC to Yes
    b- Enter the url to the discovery doc of your OAuth2 provider
    c- Fill OIDCOAuth2ClientID with noted client id
    d- Fill OIDCOAuth2ClientSecret with noted client secret
    e- Change OIDCProviderName to change the text displayed on OIDC login
buttons

    f- Configure the other related system preferences as you wish. They
shoud work the same way as the similar googleopenidconnect system
preferences.
5- If OIDCAutoRegister is set to Don't allow, have a koha
account with the same email as the one used by your OAuth2 provider.
6- In OPAC, sign in using OpenID Connect.
7- You may have to log into your OpenID provider account and accept conditions.
It should finally log you into the koha account.
Comment 23 Shi Yao Wang 2022-06-22 21:35:39 UTC
Created attachment 136431 [details] [review]
Bug 30988: Moving id token validation code into a module
Comment 24 David Cook 2022-06-22 23:47:48 UTC
(In reply to Shi Yao Wang from comment #21)
> I don't see a simple and consistent way to do OIDC logout from my little
> research on the subject, so I will put this on Needs signoff.

Perhaps not simple within the current confines of C4::Auth/your change. No worries.

I had a working example at https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=21586&attachment=80689
Comment 25 David Cook 2022-06-22 23:49:19 UTC
I'll look at testing this again in any case.
Comment 26 David Cook 2022-06-23 02:07:01 UTC
My test plan:
0. Set up koha-testing-docker with a jboss/keycloak container as per https://hub.docker.com/r/jboss/keycloak/
0b. Create "test" realm with discovery doc: http://<my_ip>:8082/auth/realms/test/.well-known/openid-configuration
0c. Create condiential OIDC client "koha" in "test" realm
0d. Create "test" user with email "test@test.test", first name "test1", last name "test2", password "test"
0e. Fix "OPACBaseURL" so that it resolves to localhost instead of a non-existent domain name
1. Apply patch
2. koha-plack --restart kohadev
3. koha-upgrade-schema kohadev
4. Set "OIDC" syspref to "Yes"
5. Set "OIDCAutoRegister" to "Allow"
6. Set "OIDCConfigURL" to "http://<my_ip>:8082/auth/realms/test/.well-known/openid-configuration" 
7. Set "OIDCDefaultBranch" to "CPL"
8. Set "OIDCDefaultCategory" to "Patron"
9. Set "OIDCOAuth2ClientID" to my Keycloak client id
10. Set "OIDCOAuth2ClientSecret" to my Keycloak client secret
11. Go to http://localhost:8080/cgi-bin/koha/opac-main.pl
12. Click "Log in to your account"
13. Fill out your username and password in Keycloak
14. Success! Returned to a logged in OPAC with new auto-registered borrower

--

Remaining issues:
1)
- installer/data/mysql/atomicupdate/bug_30988-add_oidc_syspref.pl
- installer/data/mysql/mandatory/sysprefs.sql
- koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref
"URL to identity provider's OpenID config" should be "URL to identity provider's OpenID Connect config"
("OIDCOAuth2ClientID" and "OIDCOAuth2ClientSecret" shouldn't include "OAuth2" as it's redundant but not really an issue I suppose.)

2)
- koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref
4 instances of "OpenID" instead of "OpenID Connect"

3)
- koha-tmpl/opac-tmpl/bootstrap/en/includes/masthead.inc
- koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-main.tt
- koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth.tt
I think the following text is totally unnecessary (and somewhat inaccurate since the protocol is OpenID Connect but the account isn't): 
"If you do not have an OpenID account from the provider specified in this library, but do have a local account, you can still log in:"

Personally, I envision a login area like the following: https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=97684. Without that text, we'd have something like that.
Comment 27 David Cook 2022-06-23 02:36:42 UTC
I think that this is very very close! 

--

By the way, when this is pushed and I adopt the pushed version, I'll move my existing libraries off the bug 21586 version. At that point, I'll port and upstream my work on the Logout endpoint, claims mapping, etc. 

If you do any OIDC work at all, feel free to add my email to the CC and I'll take a look. 

(I don't know if it's of interest to you, but we recently wrote and implemented a Keycloak extension to use Koha as the user/password storage for Keycloak. Keycloak acts as the SAML/OIDC IdP at the protocol level for Koha and other third-party systems, but Koha is the authoritative user database. Always happy to talk about that too.)
Comment 28 Shi Yao Wang 2022-06-23 13:31:09 UTC
Created attachment 136450 [details] [review]
Bug 30988: Add generic OpenIDConnect client implementation

A generic OpenID Connect implementation.

Test plan:
1- Set up an OAuth2 provider that has a discovery doc (e.g. for google it's https://accounts.google.com/.well-known/openid-configuration) and note down the client id and client secret.
Docs to help setup:
google: https://developers.google.com/identity/protocols/oauth2/openid-connect
        https://koha-community.org/manual/20.11/en/html/administrationpreferences.html
gitlab: https://docs.gitlab.com/ee/integration/oauth_provider.html
2- Apply the patch
3- Run atomicupdate to update database
4- Configure system preferences:
    a- Set OIDC to Yes
    b- Enter the url to the discovery doc of your OAuth2 provider
    c- Fill OIDCOAuth2ClientID with noted client id
    d- Fill OIDCOAuth2ClientSecret with noted client secret
    e- Change OIDCProviderName to change the text displayed on OIDC login
buttons

    f- Configure the other related system preferences as you wish. They
shoud work the same way as the similar googleopenidconnect system
preferences.
5- If OIDCAutoRegister is set to Don't allow, have a koha
account with the same email as the one used by your OAuth2 provider.
6- In OPAC, sign in using OpenID Connect.
7- You may have to log into your OpenID provider account and accept conditions.
It should finally log you into the koha account.
Comment 29 Shi Yao Wang 2022-06-23 13:31:13 UTC
Created attachment 136451 [details] [review]
Bug 30988: Moving id token validation code into a module
Comment 30 David Cook 2022-06-24 00:10:07 UTC
*** Bug 28420 has been marked as a duplicate of this bug. ***
Comment 31 David Cook 2022-06-24 00:33:54 UTC
Created attachment 136467 [details] [review]
Bug 30988: Add generic OpenIDConnect client implementation

A generic OpenID Connect implementation.

Test plan:
1- Set up an OAuth2 provider that has a discovery doc (e.g. for google it's https://accounts.google.com/.well-known/openid-configuration) and note down the client id and client secret.
Docs to help setup:
google: https://developers.google.com/identity/protocols/oauth2/openid-connect
        https://koha-community.org/manual/20.11/en/html/administrationpreferences.html
gitlab: https://docs.gitlab.com/ee/integration/oauth_provider.html
2- Apply the patch
3- Run atomicupdate to update database
4- Configure system preferences:
    a- Set OIDC to Yes
    b- Enter the url to the discovery doc of your OAuth2 provider
    c- Fill OIDCOAuth2ClientID with noted client id
    d- Fill OIDCOAuth2ClientSecret with noted client secret
    e- Change OIDCProviderName to change the text displayed on OIDC login
buttons

    f- Configure the other related system preferences as you wish. They
shoud work the same way as the similar googleopenidconnect system
preferences.
5- If OIDCAutoRegister is set to Don't allow, have a koha
account with the same email as the one used by your OAuth2 provider.
6- In OPAC, sign in using OpenID Connect.
7- You may have to log into your OpenID provider account and accept conditions.
It should finally log you into the koha account.

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 32 David Cook 2022-06-24 00:33:59 UTC
Created attachment 136468 [details] [review]
Bug 30988: Moving id token validation code into a module

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 33 David Cook 2022-06-24 00:34:04 UTC
Created attachment 136469 [details] [review]
Bug 30988: Fix minor formatting issues

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 34 David Cook 2022-06-24 00:37:27 UTC
Signed off!

Hopefully it'll get through QA soon and we'll have a generic OpenID Connect client in the main Koha codebase!
Comment 35 David Cook 2022-06-27 06:14:31 UTC
As an aside, I've been thinking a bit about how email address isn't necessarily the best matching point. 

Technically, the "sub" standard claim is the identifier for the user. (https://openid.net/specs/openid-connect-core-1_0.html#StandardClaims)

For a IdP like Keycloak, it will send a UUID as the "sub" claim, although you can override this in Keycloak to use whatever you want (e.g. email).

I was working on a SAML setup the other day and the Keycloak SAML IdP sent an identifier like 

"https://keycloak_idp_site...!https://client_site!<username>

Anyway, I don't think it's just a blocker. Just something that Koha could think about more in general.
Comment 36 David Cook 2022-06-27 06:22:40 UTC
The reason I was thinking about that was that we don't really need to use checkauth() here...

At the end of the day, all we really need to do is create a Koha session, create a CGISESSID cookie with the session ID as the value, and then redirect wherever. 

If we want to check if we're already authenticated, all we need to do is the same thing checkauth() is doing:

( $return, $session, $more_info ) = check_cookie_auth( $sessionID, $flags, { skip_version_check => 1 });

I've opened bug 31050 to make it easier to create a Koha session...
Comment 37 Shi Yao Wang 2022-07-06 15:41:10 UTC
Created attachment 137230 [details] [review]
Bug 30988: Add generic OpenIDConnect client implementation

A generic OpenID Connect implementation.

Test plan:
1- Set up an OAuth2 provider that has a discovery doc (e.g. for google it's https://accounts.google.com/.well-known/openid-configuration) and note down the client id and client secret.
Docs to help setup:
google: https://developers.google.com/identity/protocols/oauth2/openid-connect
        https://koha-community.org/manual/20.11/en/html/administrationpreferences.html
gitlab: https://docs.gitlab.com/ee/integration/oauth_provider.html
2- Apply the patch
3- Run atomicupdate to update database
4- Configure system preferences:
    a- Set OIDC to Yes
    b- Enter the url to the discovery doc of your OAuth2 provider
    c- Fill OIDCOAuth2ClientID with noted client id
    d- Fill OIDCOAuth2ClientSecret with noted client secret
    e- Change OIDCProviderName to change the text displayed on OIDC login
buttons

    f- Configure the other related system preferences as you wish. They
shoud work the same way as the similar googleopenidconnect system
preferences.
5- If OIDCAutoRegister is set to Don't allow, have a koha
account with the same email as the one used by your OAuth2 provider.
6- In OPAC, sign in using OpenID Connect.
7- You may have to log into your OpenID provider account and accept conditions.
It should finally log you into the koha account.

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 38 Shi Yao Wang 2022-07-06 15:41:13 UTC
Created attachment 137231 [details] [review]
Bug 30988: Moving id token validation code into a module

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 39 Shi Yao Wang 2022-07-06 15:41:17 UTC
Created attachment 137232 [details] [review]
Bug 30988: Fix minor formatting issues

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 40 Shi Yao Wang 2022-07-06 15:41:38 UTC
Rebase
Comment 41 David Cook 2022-07-22 04:32:25 UTC
I've asked folk to QA this a few times and just gotten silence, so I'm thinking we should look for someone else to sign off on this report, and then I'll join the QA team for Koha 22.11. 

I'd been on the QA team before but hadn't contributed heaps so I let myself slide off it, but I think maybe it would be good to get back onto it, especially to move along important changes like this one.
Comment 42 Martin Renvoize 2022-07-22 07:20:07 UTC
This is looking good.. a few comments of where I'd love to see it headed and one that is unfortunately a QA fail at the moment.

QA Fail
* We're missing Unit Tests for the new module you introduce, I'm afraid that's a hard fail for now.  It also looks like you have a note to do validation here which is missing... little confused as it looks like you are passing in a json structure rather than the token string.. so I'd have expected that to already be verified?  We could perhaps just use an existing library for this Mojo::JWT for instance?

Future
1. I'd love to see this moved out of preferences and into it's own management page.. I could see multiple OpenIDConnect providers being configured at the same time and allowing the end user to 'Pick their poison' at login.
2. If we went the above route, I'd love to see the Google stuff pulled out and the preferences it uses also migrated into the above management area.
3. The controller/svc script is pretty heavy right now.. I'd love to have seen more of this done 'in module' and have the controller lighter and perhaps in the REST side of things rather than a new /svc.. but I'm honestly not sure how it would fit in REST as yet so have no issue with it in /svc for now.
Comment 43 Katrin Fischer 2022-07-22 08:06:55 UTC
(In reply to David Cook from comment #41)
> I've asked folk to QA this a few times and just gotten silence, so I'm
> thinking we should look for someone else to sign off on this report, and
> then I'll join the QA team for Koha 22.11. 
> 
> I'd been on the QA team before but hadn't contributed heaps so I let myself
> slide off it, but I think maybe it would be good to get back onto it,
> especially to move along important changes like this one.

We are happy to have you for this cycle too if you are interested. In the summer time (Europe etc.) we are always struggling a bit more.
Comment 44 Shi Yao Wang 2022-07-22 16:19:46 UTC
(In reply to Martin Renvoize from comment #42)
> This is looking good.. a few comments of where I'd love to see it headed and
> one that is unfortunately a QA fail at the moment.
> 
> QA Fail
> * We're missing Unit Tests for the new module you introduce, I'm afraid
> that's a hard fail for now.  It also looks like you have a note to do
> validation here which is missing... little confused as it looks like you are
> passing in a json structure rather than the token string.. so I'd have
> expected that to already be verified?  We could perhaps just use an existing
> library for this Mojo::JWT for instance?

Sorry, I am not really knowledgeable on the subject of token validation. For the note, I put it there because I saw this note in the file opac/svc/auth/googleopenidconnect that I copied to opac/svc/auth/openidconnect when I started:
># Normally we'd have to validate the token - but google says not to worry here (Avoids another library!)
># See https://developers.google.com/identity/protocols/OpenIDConnect#obtainuserinfo for rationale

So I rewrote the note in the same spot just to give a heads up since I am not sure if it is needed and I couldn't figure out how to do it if it is needed. I looked into it a little and I think it has something to do with validating the token signature involving kid attribute (key id) and jwks_uri of the discovery document?

> little confused as it looks like you are
> passing in a json structure rather than the token string.. so I'd have
> expected that to already be verified?

The json passed in looks like this:
>{
>    'access_token' => '(encoded stuff)'
>    'id_token' => '(encoded stuff)'
>    'scope' => 'openid https://www.googleapis.com/auth/userinfo.profile https://www.googleapis.com/auth/userinfo.email'
>    'expires_in' => 3598
>    'token_type' => 'Bearer'
>}
Then 'id_token' is split (into 3 parts) by '.' and the middle part is decoded into another json that contains the claims. Hope this helps comprehension.
Comment 45 Shi Yao Wang 2022-07-22 17:29:42 UTC
I will wait for a reply on my previous comment before attempting to write unit tests. Should the test be at t/db_dependent/Koha/Auth/Client/OIDC.t?
Comment 46 David Cook 2022-07-25 01:04:06 UTC
(In reply to Martin Renvoize from comment #42)
> QA Fail
> * We're missing Unit Tests for the new module you introduce, I'm afraid
> that's a hard fail for now. 

I thought since the foundation Google OpenID Connect code (bug 10988) made it through without unit tests, this one would be able to as well, since it's just a slight modification on existing code in Koha.

> It also looks like you have a note to do
> validation here which is missing... little confused as it looks like you are
> passing in a json structure rather than the token string.. so I'd have
> expected that to already be verified?  We could perhaps just use an existing
> library for this Mojo::JWT for instance?

I think that the code comments are misleading. The Koha code does validate the "iss", "aud", and "exp" claims in the ID token. The only thing it doesn't check is the JWT signature. (See https://developers.google.com/identity/protocols/oauth2/openid-connect#validatinganidtoken). 

Mojo::JWT is probably the way to go, but I wonder if it's scope creep though, since the Google OpenID Connect code doesn't do it currently. 

> Future
> 1. I'd love to see this moved out of preferences and into it's own
> management page.. I could see multiple OpenIDConnect providers being
> configured at the same time and allowing the end user to 'Pick their poison'
> at login.
> 2. If we went the above route, I'd love to see the Google stuff pulled out
> and the preferences it uses also migrated into the above management area.

That's a goal of mine for sure. 

Locally, I have an endpoint called /cgi-bin/koha/svc/login_openidc/<provider_id>. The controller is located at "login_openidc" and it parses the remaining URL string to get the ID, and then looks up the provider in its configuration. It would be very easy to do. 

> 3. The controller/svc script is pretty heavy right now.. I'd love to have
> seen more of this done 'in module' and have the controller lighter and
> perhaps in the REST side of things rather than a new /svc.. but I'm honestly
> not sure how it would fit in REST as yet so have no issue with it in /svc
> for now.

This code is pretty much a copy/paste of bug 10988 so it inherits its problems. 

One could argue that it shouldn't be in /svc or the REST API, because it's not really an API. It's just an authentication endpoint. In terms of the controller, it's really just redirecting a browser (and doing some backend work which should be in a module for sure).
Comment 47 David Cook 2022-07-25 01:04:46 UTC
(In reply to Katrin Fischer from comment #43)
> We are happy to have you for this cycle too if you are interested. In the
> summer time (Europe etc.) we are always struggling a bit more.

Sounds good to me. Do we need to do anything for me to re-gain my QA genie powers?
Comment 48 David Cook 2022-07-25 01:06:32 UTC
(As a side note, Tomas has mentioned by email that he has an alternative implementation he's been working on. I recall that he and I talked about it a long time ago, and it sounded like a superior implementation. I'm going to ask him more about it by email.)
Comment 49 David Cook 2022-07-25 01:23:04 UTC
(In reply to Martin Renvoize from comment #42)
> 3. The controller/svc script is pretty heavy right now.. I'd love to have
> seen more of this done 'in module' and have the controller lighter and
> perhaps in the REST side of things rather than a new /svc.. but I'm honestly
> not sure how it would fit in REST as yet so have no issue with it in /svc
> for now.

(In reply to David Cook from comment #46)
> One could argue that it shouldn't be in /svc or the REST API, because it's
> not really an API. It's just an authentication endpoint. In terms of the
> controller, it's really just redirecting a browser (and doing some backend
> work which should be in a module for sure).

Since the only Mojolicious implementation is the REST API, that means that the controller really should stay in our bespoke Koha Plack/CGI scripts. 

With Bug 28325, I started working on a Mojolicious web UI base, but there was just too much work to do solo, and I ran out of steam.
Comment 50 Katrin Fischer 2022-07-25 07:36:57 UTC
(In reply to David Cook from comment #47)
> (In reply to Katrin Fischer from comment #43)
> > We are happy to have you for this cycle too if you are interested. In the
> > summer time (Europe etc.) we are always struggling a bit more.
> 
> Sounds good to me. Do we need to do anything for me to re-gain my QA genie
> powers?

I think adding yourself to the roles page will suffice ;)
https://wiki.koha-community.org/wiki/Roles_for_22.11
Comment 51 David Cook 2022-07-26 00:02:19 UTC
(In reply to Katrin Fischer from comment #50)
> (In reply to David Cook from comment #47)
> > (In reply to Katrin Fischer from comment #43)
> > > We are happy to have you for this cycle too if you are interested. In the
> > > summer time (Europe etc.) we are always struggling a bit more.
> > 
> > Sounds good to me. Do we need to do anything for me to re-gain my QA genie
> > powers?
> 
> I think adding yourself to the roles page will suffice ;)
> https://wiki.koha-community.org/wiki/Roles_for_22.11

Ah right. I think I meant to add myself for 23.05 but accidentally added myself to 22.11 previously. Happy accident!
Comment 52 Katrin Fischer 2022-11-14 07:54:46 UTC
The alternative OIDC implementation has been pushed and I hope it fulfills all requirements of this development. Marking as duplicate, also to make people aware.

*** This bug has been marked as a duplicate of bug 31378 ***