https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30988 --- Comment #8 from David Cook <dcook@prosentient.com.au> --- Comment on attachment 136302 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=136302 Bug 30988: Adding a more generic version of googleopenidconnect Review of attachment 136302: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=30988&attachment=136302) ----------------------------------------------------------------- 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. -- You are receiving this mail because: You are watching all bug changes.