https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30988
--- Comment #8 from David Cook <[email protected]> --- 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. _______________________________________________ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
