[Koha-bugs] [Bug 30988] Adding a more generic version of googleopenidconnect

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30988

--- Comment #8 from David Cook <dcook at 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.