Bug 10988 - Allow login via Google OAuth2 (OpenID Connect)
Summary: Allow login via Google OAuth2 (OpenID Connect)
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nicholas van Oudtshoorn
QA Contact: Testopia
URL:
Keywords:
Depends on: 5010
Blocks: 16892 18118 broke, openidconnect, persona, removal 29004 29006
  Show dependency treegraph
 
Reported: 2013-10-03 07:17 UTC by Nicholas van Oudtshoorn
Modified: 2022-11-14 22:19 UTC (History)
14 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
oauth login helper (4.16 KB, application/x-perl)
2013-10-03 07:17 UTC, Nicholas van Oudtshoorn
Details
Bug 10988 [ENH] Allow login via Google OAuth2 Adds googleoauth2 login to koha. Adds applicable system preferences Modifies templates to support oauth2 login failures changes the login links to use googleoauth2 when oauth2 is enabled. (14.56 KB, patch)
2013-10-10 08:40 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
Bug 10988 [ENH] Allow login via Google OAuth2 (15.66 KB, patch)
2013-10-10 09:08 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
Bug 10988 Fix some minor typos (2.66 KB, patch)
2013-10-10 09:13 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
Adds Google OAuth2 Support (20.18 KB, application/mbox)
2014-02-17 10:21 UTC, Nicholas van Oudtshoorn
Details
For updating an INSTALLED 3.14.03 to use GoogleOAuth2 (17.12 KB, patch)
2014-02-24 07:34 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
Bug 10988 [ENH] Allow login via Google OAuth2 (21.36 KB, patch)
2014-02-27 02:28 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
Allow Google OAuth2 Login (19.46 KB, patch)
2014-07-10 03:08 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
For updating an INSTALLED 3.14.03 to use GoogleOAuth2 (17.44 KB, patch)
2014-07-10 03:44 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
New version of the patch - changes how the domain restriction is handled (21.16 KB, application/mbox)
2014-07-24 09:00 UTC, Nicholas van Oudtshoorn
Details
Allow Google OAuth2 Login (21.16 KB, patch)
2014-07-24 09:02 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
Bug 10988 - Allow login via Google OAuth2 (18.57 KB, patch)
2015-03-02 23:50 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 10988 - Allow login via Google OAuth2 (18.58 KB, patch)
2015-03-02 23:51 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 10988 - Allow login via Google OAuth (18.73 KB, patch)
2015-06-11 05:13 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
Bug 10988 - Allow login via Google OAuth2 (16.90 KB, patch)
2015-06-18 03:03 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
Bug 10988: Allow login via Google OAuth2 (17.19 KB, patch)
2015-06-22 03:23 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 10988: Allow login via Google OAuth2 (17.19 KB, patch)
2015-07-14 05:12 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
Bug 10988: Fix the svc script path (4.59 KB, patch)
2015-08-19 13:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10988: Allow login via Google OAuth2 (17.18 KB, patch)
2015-10-22 23:31 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 10988: Fix the svc script path (4.59 KB, patch)
2015-10-22 23:31 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 10988: QA Follow-up - rewording suggestions (2.08 KB, patch)
2015-10-24 21:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 10988: Dealt with borrower flag issues (4.56 KB, patch)
2015-12-14 15:42 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 10988: Dealt with borrower flag issues (6.51 KB, patch)
2015-12-14 15:46 UTC, Mark Tompsett
Details | Diff | Splinter Review
Allow Google OAuth2 Login (16.98 KB, patch)
2015-12-22 03:14 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
Bug 10988 - Allow for Google OAuth2 logins Combined all of the patches above into one, making them apply to master again. (17.03 KB, patch)
2015-12-29 06:13 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 10988 - Rewrite and renamed to OpenIDConnect (19.38 KB, patch)
2016-01-11 08:52 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
Remove some outdated wording from googleopenidconnect (1.29 KB, patch)
2016-01-11 09:02 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10988 - Fixes for comments 57 and 58 (19.43 KB, patch)
2016-02-08 22:59 UTC, Mark Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10988 - Fix some wording Fix some outdated wording in googleopenidconnect (1.35 KB, patch)
2016-02-08 23:00 UTC, Mark Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10988: Tabs vs Spaces qa failure patch (2.70 KB, patch)
2016-02-08 23:00 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 10988 - Fixes for comments 57 and 58 (19.48 KB, patch)
2016-04-01 13:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 10988 - Fix some wording Fix some outdated wording in googleopenidconnect (1.40 KB, patch)
2016-04-01 13:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 10988: Tabs vs Spaces qa failure patch (2.75 KB, patch)
2016-04-01 13:57 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicholas van Oudtshoorn 2013-10-03 07:17:45 UTC Comment hidden (obsolete)
Comment 1 Nicholas van Oudtshoorn 2013-10-10 08:40:39 UTC Comment hidden (obsolete)
Comment 2 Nicholas van Oudtshoorn 2013-10-10 08:54:30 UTC
The latest patch is a fully functional version of GoogleOAuth2, being used in production here at http://library.pbc.wa.edu.au/

Test Plan:
1) Apply this patch
2) Update the database (this is the bit I'm not sure about - I've added the SQL into the atomicupdates directory - is this right?)
3) Create a web app in the google cloud console:
     - Go to https://cloud.google.com/console
     - Create a project, and give it some details
     - Open the Project by clicking on it
     - Under APIs & auth menu, open "Registered Apps" and click "Register App"
     - Give it a name and make sure you select "Web app", click ok
     - Under OAuth 2.0 Client ID,
         - under web origin, type <your_opac_address>
         - in the redirect uri enter <your_opac_address>/cgi-bin/koha/svc/googleoauth2
         - click Generate
4) Set the GoogleOAuth2ClientID and GoogleOAuth2ClientSecret according to the values generated above
5) Test that clicking login intiates an oauth2 login
6) Try logging in with an email that is not registered to a library user. It should fail and offer either to retry or to login manually.
7) Try logging in with an email that is registered to a library user. You should be logged in.
Comment 3 Nicholas van Oudtshoorn 2013-10-10 09:08:15 UTC Comment hidden (obsolete)
Comment 4 Nicholas van Oudtshoorn 2013-10-10 09:13:29 UTC Comment hidden (obsolete)
Comment 5 Nicholas van Oudtshoorn 2013-10-17 07:57:45 UTC
When using this in a public computer, there are some nice javascripts that can be injected to make this a whole lot more secure. Basically, the following userscripts disable the google account chooser, and log a person out of their google accounts when they log out of Koha. This is not very useful or polite outside of a public terminal, hence my thinking to do it with userscripts.

Just putting it here as a reference for anyone thinking to use this.

--------------------------------------------------------------------------------
// ==UserScript==
// @id             accounts.google.com-forcenocookie@scriptish
// @name           Disable persistent google login
// @version        1.0
// @namespace      
// @author         
// @description    
// @include        https://accounts.google.com/*
// @run-at         document-end
// ==/UserScript==
document.getElementsByName("PersistentCookie")[0].value='no';

--------------------------------------------------------------------------------
// ==UserScript==
// @id             koha-full-oauth2-logout@scriptish
// @name           Force Full Logout
// @version        1.0
// @namespace
// @author
// @description
// @include        http://OPACURLHERE/*
// @run-at         document-end
// ==/UserScript==
document.getElementById('logoutmenu').href="https://www.google.com/accounts/Logout?continue=https://appengine.google.com/_ah/logout?continue="+document.getElementById('logoutmenu').href;
document.getElementById('logout').href="https://www.google.com/accounts/Logout?continue=https://appengine.google.com/_ah/logout?continue="+document.getElementById('logoutmenu').href;
Comment 6 Nicholas van Oudtshoorn 2014-02-17 10:21:27 UTC Comment hidden (obsolete)
Comment 7 Nicholas van Oudtshoorn 2014-02-24 07:34:44 UTC Comment hidden (obsolete)
Comment 8 Nicholas van Oudtshoorn 2014-02-27 02:28:44 UTC Comment hidden (obsolete)
Comment 9 Nicholas van Oudtshoorn 2014-05-01 02:17:22 UTC
Ping on this. Anybody interested in  signing this one off?
Comment 10 Marc Véron 2014-06-30 09:44:52 UTC
Patch did not apply. Merge conflict in 
koha-tmpl/opac-tmpl/bootstrap/en/includes/masthead.inc
Comment 11 Nicholas van Oudtshoorn 2014-06-30 10:26:56 UTC
Oh no! I'm on holiday for the next week, but will fix it next Tuesday.
Comment 12 Samuel Martin 2014-07-04 07:28:40 UTC
Ran a test on this patch. And I had some issues. I'm no good at programming. But a friend of mine helped me. I thought I could post this to help out others.

I hope you'll understand what I mean. I'm new here and more of a librarian than a programmer.

So... Here's what we found out:

In the file: usr/share/koha/opac/cgi-bin/opac/svc/googleoauth2
there appeared to be some lines missing in the patch at the very beginning of the file. Though the code is available here in this bug. So that was easy to fix.

And then we still had some issues. At that time I still had an error 400 straight from google this time. We had to set up the opac URL in: /etc/apache2/conf.d/

I'm not sure if this was supposed to be done before or not. But I had to do it at that time.

Also: in the opac-main there is no hint where to log in with the google account. It would be a nice feature that once you use Google Oauth it would say which log in link to use to do that. Maybe even with a google logo?

Other than that the patch is working perfectly though I didn't test it extensively. It's only running on my test server.
Comment 13 Nicholas van Oudtshoorn 2014-07-10 03:08:18 UTC Comment hidden (obsolete)
Comment 14 Nicholas van Oudtshoorn 2014-07-10 03:08:58 UTC
(In reply to Marc Véron from comment #10)
> Patch did not apply. Merge conflict in 
> koha-tmpl/opac-tmpl/bootstrap/en/includes/masthead.inc

Should apply cleanly with the latest patch...
Comment 15 Nicholas van Oudtshoorn 2014-07-10 03:44:42 UTC Comment hidden (obsolete)
Comment 16 Nicholas van Oudtshoorn 2014-07-24 09:00:53 UTC Comment hidden (obsolete)
Comment 17 Nicholas van Oudtshoorn 2014-07-24 09:02:32 UTC Comment hidden (obsolete)
Comment 18 Marc Véron 2014-08-19 18:20:55 UTC
I tried to apply the second patch: Allow Google OAuth2 Login

It did not apply with:
error: patch failed: koha-tmpl/opac-tmpl/bootstrap/en/includes/masthead.inc:74
error: koha-tmpl/opac-tmpl/bootstrap/en/includes/masthead.inc: patch does not apply
Did you hand edit your patch?
It does not apply to blobs recorded in its index.
Cannot fall back to three-way merge.

--------------

Additionaly, I think in installer/data/mysql/updatedatabase.pl line 8573 should read:

$DBversion = "XXX";

See: http://wiki.koha-community.org/wiki/System_Preferences
Comment 19 Martin Renvoize 2014-12-17 09:06:47 UTC
Interesting work.. could we have this rebased for testing Nicholas?
Comment 20 Mark Tompsett 2015-03-02 23:50:15 UTC Comment hidden (obsolete)
Comment 21 Mark Tompsett 2015-03-02 23:51:40 UTC Comment hidden (obsolete)
Comment 22 Mark Tompsett 2015-03-02 23:53:33 UTC
It depends on 5010, because I cut out the hard coded 'http://' part of the code in the new googleoauth2 script. Technically, you can test it without 5010, but you need the protocol in your OPACBaseURL. :)
Comment 23 Nicholas van Oudtshoorn 2015-03-03 03:02:39 UTC
Thanks so much to M. Tompsett for doing this. I've been busy with other jobs (including upgrading to 3.18!), and just haven't got around to sending in our patch. :-)
(In reply to M. Tompsett from comment #22)
> It depends on 5010, because I cut out the hard coded 'http://' part of the
> code in the new googleoauth2 script. Technically, you can test it without
> 5010, but you need the protocol in your OPACBaseURL. :)
Comment 24 David Cook 2015-03-04 01:06:58 UTC
I'm just wrapping up a local project on using OpenID Connect, which is basically a wrapper around OAuth2.0 with a few extras thrown in on top, so I'd be interested in taking a look at this at some point.

Could we look at making this more extensible though? There are lots of other OAuth2.0 providers than just Google. 

Have yo looked at http://search.cpan.org/~kgrennan/Net-OAuth2-0.06/lib/Net/OAuth2.pm? I believe Chris Cormack took a look at that recently.

Anywho, I could be interested in testing this out down the road!
Comment 25 David Cook 2015-03-04 01:12:34 UTC
Hmm, it might be a better idea to add a parameter instead of using the $persona parameter in C4::Auth::checkauth().

C4::Auth needs a clean-up in general... which Martin (ashimema) is working on, I believe.

In any case, locally I added a $external_authen hashref as a parameter after $persona. I use $external_authen->{OpenIDConnect} to store the data I need to pass to checkauth(), add some handling in the appropriate spots, and that's it.

You might want to look at RFC 6749 (OAuth 2.0) http://tools.ietf.org/html/rfc6749 as well, as it might give you some more information.

OpenID Connect has a lot of validation steps, which may or may not also exist in OAuth2.0... not sure.
Comment 26 Mark Tompsett 2015-03-04 01:41:20 UTC
(In reply to David Cook from comment #25)
> Hmm, it might be a better idea to add a parameter instead of using the
> $persona parameter in C4::Auth::checkauth().

I would think the parameter would be better renamed to $AuthenticatedEmail, because persona and this OAuth patch both authenticate against an Email address (the primary email address is used to find the user).


> C4::Auth needs a clean-up in general... which Martin (ashimema) is working
> on, I believe.

True, it does need clean-up.


> In any case, locally I added a $external_authen hashref as a parameter after
> $persona. I use $external_authen->{OpenIDConnect} to store the data I need
> to pass to checkauth(), add some handling in the appropriate spots, and
> that's it.

NOOOOOO! Please hashref the parameter!
If there are more than 3 parameters, you probably should use a hashref.
Comment 27 Nicholas van Oudtshoorn 2015-03-04 02:43:11 UTC
(In reply to David Cook from comment #24)
> I'm just wrapping up a local project on using OpenID Connect, which is
> basically a wrapper around OAuth2.0 with a few extras thrown in on top, so
> I'd be interested in taking a look at this at some point.
> 
> Could we look at making this more extensible though? There are lots of other
> OAuth2.0 providers than just Google. 
> 
> Have yo looked at
> http://search.cpan.org/~kgrennan/Net-OAuth2-0.06/lib/Net/OAuth2.pm? I
> believe Chris Cormack took a look at that recently.
> 
> Anywho, I could be interested in testing this out down the road!

Hi David,

I like the idea of making it more extensible, and when I get a chance (probably in a few weeks from now) I'll see about checking out OAuth2.pm . Since we run Google Apps for Education for our students (at library.pbc.wa.edu.au), Google's oauth2 was our priority. It's been quite a while since I wrote this, but I think google has some google-specific functionality available - like the hd parameter. (Actually, the hd parameter isn't all that useful for us, seeing as it doesn't seem to like subdomains)
Comment 28 David Cook 2015-03-04 22:18:46 UTC
(In reply to M. Tompsett from comment #26)

> I would think the parameter would be better renamed to $AuthenticatedEmail,
> because persona and this OAuth patch both authenticate against an Email
> address (the primary email address is used to find the user).
> 

Yeah, I thought about that a bit, but it also sets a "persona" key in the session, which I think might be used for persona session management, so re-using it without careful checking could lead to weirdness with Persona. 

> NOOOOOO! Please hashref the parameter!
> If there are more than 3 parameters, you probably should use a hashref.

...

Yes, a single hashref would be nicer. If I were designing a new sub, I would use a single hashref. However, I'm personally not planning on refactoring C4::Auth::checkauth() and testing every use of it. Nor am I going to add checking to checkauth() so that it can use a single hashref OR a bunch of scalars. That just leads to more technical debt.

Of course, I'm not planning on upstreaming my OpenID Connect work at the moment anyway, so it doesn't really matter. I'm sure whoever refactors checkauth() someday will use a single hashref instead.
Comment 29 Mark Tompsett 2015-04-21 08:47:25 UTC
Comment on attachment 36355 [details] [review]
Bug 10988 - Allow login via Google OAuth2

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

::: koha-tmpl/opac-tmpl/bootstrap/en/includes/masthead.inc
@@ +58,4 @@
>                                                  <li role="presentation"><a href="#" tabindex="-1" class="menu-inactive" role="menuitem">No private lists</a></li>
>                                                  <li role="presentation"><a href="/cgi-bin/koha/opac-shelves.pl?display=privateshelves" tabindex="-1" role="menuitem" class="listmenulink">New list</a></li>
>                                              [% END %]
> +                                        [% ELSIF ( GoogleOAuth2 ) %]

I'd be surprised if there wasn't a [% USE Koha %] at the top of the file. Why pass preferences? Koha.Preference('GoogleOAuth2')
Comment 30 Nicholas van Oudtshoorn 2015-06-11 05:13:22 UTC Comment hidden (obsolete)
Comment 31 Mark Tompsett 2015-06-11 12:14:02 UTC
Comment on attachment 40087 [details] [review]
Bug 10988 - Allow login via Google OAuth

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

::: C4/Auth.pm
@@ +37,4 @@
>  use Encode qw( encode is_utf8);
>  
>  # use utf8;
> +use vars qw($VERSION @ISA @EXPORT @EXPORT_OK %EXPORT_TAGS $debug $googleoauth2 $ldap $cas $caslogout $shib $shib_login);

No need to add this.

@@ +55,4 @@
>        &get_all_subpermissions &get_user_subpermissions
>      );
>      %EXPORT_TAGS = ( EditPermissions => [qw(get_all_subpermissions get_user_subpermissions)] );
> +    $googleoauth2 = C4::Context->preference('GoogleOAuth2');

Or this.

@@ +471,4 @@
>              BranchesLoop                          => GetBranchesLoop($opac_name),
>              BranchCategoriesLoop                  => GetBranchCategories( 'searchdomain', 1, $opac_name ),
>              CalendarFirstDayOfWeek                => ( C4::Context->preference("CalendarFirstDayOfWeek") eq "Sunday" ) ? 0 : 1,
> +            GoogleOAuth2                          => (C4::Context->preference("GoogleOAuth2")),

Or even this.

@@ +1247,4 @@
>          );
>      }
>  
> +    if ($googleoauth2) {

Though you will have to grab the preference directly here.

::: koha-tmpl/opac-tmpl/bootstrap/en/includes/masthead.inc
@@ +58,4 @@
>                                                  <li role="presentation"><a href="#" tabindex="-1" class="menu-inactive" role="menuitem">No private lists</a></li>
>                                                  <li role="presentation"><a href="/cgi-bin/koha/opac-shelves.pl?display=privateshelves" tabindex="-1" role="menuitem" class="listmenulink">New list</a></li>
>                                              [% END %]
> +                                        [% ELSIF ( KOHA.Preference('GoogleOAuth2') == 1 ) %]

Haven't tested yet, but is Template Toolkit case sensitive? Why KOHA? The top has [% Use Koha %] right?

@@ +78,4 @@
>                                          [% IF Koha.Preference('casAuthentication') %]
>                                              [%# CAS authentication is too complicated for modal window %]
>                                              <li><a href="/cgi-bin/koha/opac-user.pl">Log in to your account</a></li>
> +                                        [% ELSIF ( GoogleOAuth2 ) %]

Wasn't just the one location. I was talking about all of them. That way you could minimize touching Auth.pm in the first place.

::: opac/svc/googleoauth2
@@ +114,5 @@
> +            my ( $userid, $cookie, $sessionID ) =
> +              checkauth( $query, 1, { borrow => 1 }, 'opac', $email );
> +            if ($userid) {    # A valid user has logged in
> +		if (   ($restricttodomain ne '' )
> +                    && (index($email, $restricttodomain) == -1) ) 

Pink spots at the end (at least that's the colour on my screen), usually indicate spaces at the end of a line. That fails koha qa test tools.
Comment 32 Nicholas van Oudtshoorn 2015-06-18 03:03:13 UTC Comment hidden (obsolete)
Comment 33 Mark Tompsett 2015-06-21 17:51:09 UTC
I have yet to test this, but I've been thinking about it, and the comments made in comment #24 in terms of Net::OAuth2 or something similar seem to be a concern for me.
Comment 34 Mark Tompsett 2015-06-22 02:49:06 UTC
Comment on attachment 40252 [details] [review]
Bug 10988 - Allow login via Google OAuth2

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

::: koha-tmpl/opac-tmpl/bootstrap/en/includes/masthead.inc
@@ +78,4 @@
>                                          [% IF Koha.Preference('casAuthentication') %]
>                                              [%# CAS authentication is too complicated for modal window %]
>                                              <li><a href="/cgi-bin/koha/opac-user.pl">Log in to your account</a></li>
> +                                        [% ELSIF ( Koha.Preference(' GoogleOAuth2') == 1 ) %]

Space! Oops.
Comment 35 Mark Tompsett 2015-06-22 03:23:16 UTC Comment hidden (obsolete)
Comment 36 Jonathan Druart 2015-07-10 09:46:05 UTC
Comment on attachment 40442 [details] [review]
Bug 10988: Allow login via Google OAuth2

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

I don't have an installation with a public ip, so I cannot test this patch.

Following a code review:
1/ Fix the patch's author (From: root <root@beta.pbcstudents.pbc.wa.edu.au>)
2/ perltidy the new file (opac/svc/googleoauth2)
3/ Maybe opac/svc/auth/googleoauth2 is more appropriate?
4/ the following:

::: opac/svc/googleoauth2
@@ +1,1 @@
> +#!/usr/bin/perl -w

Remove -w

@@ +36,5 @@
> +# means caching some information and updating it daily. But that
> +# would make things a lot faster
> +
> +use Modern::Perl;
> +use CGI qw/escape/;

and -utf8, even if not needed.

@@ +63,5 @@
> +    $query->delete('code');
> +    $query->param( 'OAuth2Failed' => $reason );
> +    my ( $template, $borrowernumber, $cookie ) = get_template_and_user(
> +        {
> +            template_name   => 'opac-user.tmpl',

Should be opac-user.tt

@@ +75,5 @@
> +    $template->param( 'loginprompt'        => 1 );
> +    output_html_with_http_headers $query, $cookie, $template->output;
> +}
> +
> +#die $query->param('code');

To remove.
Comment 37 Nicholas van Oudtshoorn 2015-07-14 05:12:53 UTC Comment hidden (obsolete)
Comment 38 Mark Tompsett 2015-07-14 05:16:24 UTC
(In reply to Jonathan Druart from comment #36)
> I don't have an installation with a public ip,
> so I cannot test this patch.

You don't need a public ip, as far as I recall, since the redirect is done at the browser level.
Comment 39 David Cook 2015-07-15 08:05:31 UTC
(In reply to M. Tompsett from comment #38)
> (In reply to Jonathan Druart from comment #36)
> > I don't have an installation with a public ip,
> > so I cannot test this patch.
> 
> You don't need a public ip, as far as I recall, since the redirect is done
> at the browser level.

I'm not familiar with this patch at all, but it's true about not needing a public IP. 

I have a local app where I send "localhost:8888/callback" as the return uri to the OAuth 2.0 server and it works well. Not sure if that's feasible within the context of this patch, but...
Comment 40 Jonathan Druart 2015-08-19 12:59:19 UTC
I can take a look again with a detailed test plan using a non-public IP.
Comment 41 Jonathan Druart 2015-08-19 13:00:01 UTC Comment hidden (obsolete)
Comment 42 Mason James 2015-08-24 01:30:35 UTC
FYI: CALYX came up with a technique to force a google-auth session logout when a user logs out of Koha. (note: this may be considered optional behaviour)

Unfortunately, it is a solution that exists within a Google account, therefore is outside of Koha

More info to come...
Comment 43 Katrin Fischer 2015-10-22 20:58:59 UTC
Patch does not apply - please rebase!

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 10988: Allow login via Google OAuth2
Using index info to reconstruct a base tree...
M	C4/Auth.pm
M	installer/data/mysql/sysprefs.sql
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref
M	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth.tt
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth.tt
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref
Auto-merging installer/data/mysql/sysprefs.sql
Auto-merging C4/Auth.pm
CONFLICT (content): Merge conflict in C4/Auth.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 10988: Allow login via Google OAuth2
The copy of the patch that failed is found in:
   /home/katrin/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-10988-Allow-login-via-Google-OAuth2-fFUW8a.patch
Comment 44 Mark Tompsett 2015-10-22 23:31:48 UTC Comment hidden (obsolete)
Comment 45 Mark Tompsett 2015-10-22 23:31:55 UTC Comment hidden (obsolete)
Comment 46 Mark Tompsett 2015-10-22 23:37:51 UTC
Setting back to Signed Off after this rebasing.
Comment 47 Katrin Fischer 2015-10-24 21:53:58 UTC
I have a follow-up with some suggestions for the wording - but can't test on my local Koha installation (localhost...)

Also: the svc script asks for the 'borrow' permission, that has since be removed. Please take a look and fix.
Bug 7976 - "Borrow Permission" is not used and can be removed
Comment 48 Katrin Fischer 2015-10-24 21:57:17 UTC Comment hidden (obsolete)
Comment 49 Mark Tompsett 2015-12-14 15:42:33 UTC Comment hidden (obsolete)
Comment 50 Mark Tompsett 2015-12-14 15:44:26 UTC
Test plan is the same:
Test Plan:
 0) Back up your database
 1) Apply this patch
 2) In your mysql client use your Koha database and execute:
    > DELETE FROM systempreferences;
    > SOURCE ~/kohaclone/installer/data/mysql/sysprefs.sql;
    -- Should be no errors.
    > SELECT * FROM systempreferences LIKE 'GoogleOAuth%';
    -- Should see 4 entries.
    > QUIT;
 3) Restore your database
 4) Run ./installer/data/mysql/updatedatabase.pl;
 5) In your mysql client use your Koha database and execute:
    > SELECT * FROM systempreferences LIKE 'GoogleOAuth%';
    -- Should see the same 4 entries.
 6) Log into the staff client
 7) Home -> Koha administration -> Global system preferences
 8) -> OPAC
    -- make sure your OPACBaseURL is set (e.g. https://opac.koha.ca)
 9) -> Administration
    -- There should be a 'Google OAuth2' section with the ability
       to set those 4 system preferences.
10) In a new tab, go to https://console.developers.google.com/project
11) Click 'Create Project'
12) Type in a project name that won't freak users out, like your
    library name (e.g. South Pole Library).
13) Click the 'Create' button.
14) Click the 'APIs & auth' in the left frame.
15) Click 'Credentials'
16) Click 'Create new Client ID'
17) Select 'Web application' and click 'Configure consent screen'.
18) Select the Email Address.
19) Put it a meaningful string into the Product Name
    (e.g. South Pole Library Authentication)
20) Fill in the other fields as desired (or not)
21) Click 'Save'
22) Change the 'AUTHORIZED JAVASCRIPT ORIGINS' to your OPACBaseURL.
    (http://library.yourDNS.org)
23) Change the 'AUTHORIZED REDIRECT URIS' to point to the new
    googleoauth2 script
    (http://library.yourDNS.org/cgi-bin/koha/svc/auth/googleoauth2)
24) Click 'Create Client ID'
25) Copy and paste the 'CLIENT ID' into the GoogleOAuth2ClientID
    system preference.
26) Copy and paste the 'CLIENT SECRET' into the GoogleOAuth2ClientSecret
    system preference.
27) Change the GoogleOAuth2 preference to 'Use'.
28) Click 'Save all Administration preferences'
29) In the OPAC, click 'Log in to your account'.
    -- You should get a confirmation request, if you are
        already logged in, OR a login screen if you are not.
    -- You need to have the primary email address set to one
       authenticated by Google in order to log in.
30) Run koha qa test tools
Comment 51 Mark Tompsett 2015-12-14 15:46:56 UTC Comment hidden (obsolete)
Comment 52 Nicholas van Oudtshoorn 2015-12-22 03:14:29 UTC Comment hidden (obsolete)
Comment 53 Mark Tompsett 2015-12-29 06:10:40 UTC
There are no differences between the single commit and the multi-commit versions? What was the status before the commit? I'll retest anyways. *sigh* Seems like a waste of 15 minutes.
Comment 54 Mark Tompsett 2015-12-29 06:13:18 UTC Comment hidden (obsolete)
Comment 55 Nicholas van Oudtshoorn 2015-12-29 06:18:01 UTC
(In reply to M. Tompsett from comment #53)
> There are no differences between the single commit and the multi-commit
> versions? What was the status before the commit? I'll retest anyways. *sigh*
> Seems like a waste of 15 minutes.

Did this a while ago, but from memory the multiple patches didn't apply cleanly to master....
Comment 56 Martin Renvoize 2015-12-30 09:55:16 UTC
Just reviewing the code here.. one thing partially stands out.. What your actually implimenting here is OpenID Connect (Which is OpenID over Auth2.0) which is great, as I was a bit suspicious of using OAuth 2.0 for authentication as it's an api Authorization layer, not an Authentication medium out of the box (with OpenID Connect, one can use it for Authentication too).

Could we reword/rename the prefs to reflect that this is Google's implimentation of OpenID Connect, and not OAuth.. i think that will make it more clear that this is only for authentication and does not expand to using the range of api's available via OAuth.

I'll get back to reading the code now
Comment 57 Martin Renvoize 2015-12-30 10:31:15 UTC
Comment on attachment 46016 [details] [review]
Bug 10988 - Allow for Google OAuth2 logins Combined all of the patches above into one, making them apply to master again.

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

In general I feel this is a good start, but it's just that.. a start.  We need a more thorough handling of login fallbacks and we need to add state tokens into the mix to protect our users from CRSF attacks.

::: koha-tmpl/opac-tmpl/bootstrap/en/includes/masthead.inc
@@ +65,5 @@
>                                              [% IF some_private_shelves > 10 %]
>                                                  <li role="presentation"><a href="/cgi-bin/koha/opac-shelves.pl?op=list&amp;category=1" tabindex="-1" role="menuitem" class="listmenulink">View All</a></li>
>                                              [% END %]
> +                                        [% ELSIF ( Koha.Preference('GoogleOAuth2') == 1 ) %]
> +                                            <li role="presentation"><a href="/cgi-bin/koha/svc/auth/googleoauth2" tabindex="-1" class="menu-inactive" role="menuitem">Log in to create your own lists</a></li>

I disagree with this change. A) it looks to me like it won't do what your expecting (it looks like it's a level too deep in the nested IF's) but B) I don't feel adding a login link here is appropriate unless your going to add it for all available authentication mechanisms.. it's just make the interface inconsistent.

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth.tt
@@ +138,2 @@
>  
>                              [% END # / IF casAuthentication %]

There's not enough added to this file.  There should be a 'login with your google id' block somewhere which appears to be missing.

@@ +139,5 @@
>                              [% END # / IF casAuthentication %]
>  
> +                            [% IF ( invalidOAuth2Login ) %]
> +                                <h4>Automatic login</h4>
> +                                <p>Sorry, your automatic login failed. <span class="error">[% invalidOAuth2Login %]</span></p>

I think this needs rewording, it's a Google Login.. it's not automagic.. it's a shared login using the email claim from a google openid connect id token.  I feel the text is a little misleading.

::: opac/svc/auth/googleoauth2
@@ +147,5 @@
> +    }
> +
> +}
> +else {
> +    my $prompt = $query->param('reauthenticate') // q{};

I'm not seeing an state tokens in use anywhere in this Flow.. without them we are wide open to cross-site request forgery (CSRF) attacks.. we likely need to create a nice randomised string and store it between invocations of the script.
Comment 58 David Cook 2015-12-30 22:56:09 UTC
Comment on attachment 46016 [details] [review]
Bug 10988 - Allow for Google OAuth2 logins Combined all of the patches above into one, making them apply to master again.

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

I hadn't realized that this was OpenID Connect until Martin pointed it out and until I saw the "openid" value in the scope of the Authorization Request. 

I actually wrote an OpenID Connect feature for Koha for a client in 2014, but due to a lack of time and an incorrectly implemented third party OpenID Connect server, I never got around to upstreaming it to the community codebase. Perhaps I should try and remedy that one day. In any case, I've included some comments in the code review below based on my experience.

Nicholas, what documentation did you use for this patch? When I look at https://developers.google.com/identity/protocols/OpenIDConnect, it specifies different endpoints than you've used here. I'd recommend consulting that webpage and http://openid.net/specs/openid-connect-core-1_0.html. The latter is what I used for making an OpenID Connect compliant server app.

::: opac/svc/auth/googleoauth2
@@ +53,5 @@
> +# protocol is assumed in OPACBaseURL see bug 5010.
> +my $redirecturl  = $host . '/cgi-bin/koha/svc/auth/googleoauth2';
> +my $issuer       = 'accounts.google.com';
> +my $clientid     = C4::Context->preference('GoogleOAuth2ClientID');
> +my $clientsecret = C4::Context->preference('GoogleOAuth2ClientSecret');

Alternatively, you could put the ClientID and the ClientSecret in koha-conf.xml. While it would give less control to libraries, it would keep this information on a more "need to know" basis.

@@ +86,5 @@
> +elsif ( defined $query->param('code') ) {
> +    my $code    = $query->param('code');
> +    my $ua      = LWP::UserAgent->new();
> +    my $request = POST(
> +        'https://accounts.google.com/o/oauth2/token',

How did you choose this endpoint? Both https://developers.google.com/identity/protocols/OpenIDConnect?hl=en#discovery and https://accounts.google.com/.well-known/openid-configuration provide versioned endpoints.

@@ +99,5 @@
> +    );
> +    my $response = $ua->request($request)->decoded_content;
> +    my $json     = decode_json($response);
> +    if ( exists( $json->{'id_token'} ) ) {
> +        $request = POST( 'https://www.googleapis.com/oauth2/v1/tokeninfo',

tokeninfo isn't a standard OpenID Connect endpoint. It's a debugging tool that Google has available, but they discourage its use in production: https://developers.google.com/identity/protocols/OpenIDConnect?hl=en#validatinganidtoken

Use of tokeninfo also makes it harder for the rest of us to read the code since a decrypted token has a standard layout, but I haven't found what a tokeninfo response contains.

@@ +105,5 @@
> +        $response = $ua->request($request)->decoded_content;
> +        $json     = decode_json($response);
> +
> +# Confirm (as google suggests) that the issuer and audience are what we expect them to be
> +        if (   ( $json->{'issuer'} eq $issuer )

As noted above, a standard response should be $json->{'iss'}, but otherwise this is good as per #2 at http://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation.

@@ +106,5 @@
> +        $json     = decode_json($response);
> +
> +# Confirm (as google suggests) that the issuer and audience are what we expect them to be
> +        if (   ( $json->{'issuer'} eq $issuer )
> +            && ( $json->{'audience'} eq $clientid )

As noted in #3 at http://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation, $json->{'aud'} (the standard claim), "may" be an array. I'm not familiar with Google's responses... it might always return a string, but the spec points out that you should check for either a string or an array (and to reject the token if it doesn't contain the expected audience or if it includes untrusted audiences).

@@ +107,5 @@
> +
> +# Confirm (as google suggests) that the issuer and audience are what we expect them to be
> +        if (   ( $json->{'issuer'} eq $issuer )
> +            && ( $json->{'audience'} eq $clientid )
> +            && exists( $json->{'email'} ) )

This is bizarre... the token shouldn't contain the email. That must be for debugging purposes in tokeninfo. You should have to get it from https://www.googleapis.com/oauth2/v3/userinfo or a similar endpoint, which also need its "sub" claim to be validated.

@@ +108,5 @@
> +# Confirm (as google suggests) that the issuer and audience are what we expect them to be
> +        if (   ( $json->{'issuer'} eq $issuer )
> +            && ( $json->{'audience'} eq $clientid )
> +            && exists( $json->{'email'} ) )
> +        {

At this point, you must also verify that the token type is "Bearer" (case insensitive as per RFC 6749 Section 5.1).

At this point, you must also be verifying that the token isn't expired. 

You must also validate the "azp" claim if it's present in the response. 

For more on validation, consult http://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation and RFC 6749 (https://tools.ietf.org/html/rfc6749)

@@ +148,5 @@
> +
> +}
> +else {
> +    my $prompt = $query->param('reauthenticate') // q{};
> +

I agree with Martin. Even on Google's page, it states "You must protect the security of your users by preventing request forgery attacks." (https://developers.google.com/identity/protocols/OpenIDConnect?hl=en#createxsrftoken).

As developers, we have a responsibility to the users of Koha to ensure their security and their privacy.
Comment 59 Martin Renvoize 2015-12-31 08:40:13 UTC
All insightful stuff David, thanks :)

Just to make sure I wasn't coming accross too negative; I'm really pleased someone is working on adding this functionality.. nice start Nicholas.
Comment 60 Martin Renvoize 2015-12-31 08:44:08 UTC
https://developers.google.com/identity/protocols/OpenIDConnect is the documentation I would likely use to implement this authentication.
Comment 61 Nicholas van Oudtshoorn 2016-01-11 08:52:21 UTC Comment hidden (obsolete)
Comment 62 Nicholas van Oudtshoorn 2016-01-11 09:02:15 UTC Comment hidden (obsolete)
Comment 63 Martin Renvoize 2016-01-12 06:46:05 UTC
Nice work Nick! 

I will be eagerly looking at this, hopefully this afternoon.  Thanks for putting in all the hard work, so glad you followed through.

:-D
Comment 64 Mark Tompsett 2016-02-08 22:59:57 UTC Comment hidden (obsolete)
Comment 65 Mark Tompsett 2016-02-08 23:00:04 UTC Comment hidden (obsolete)
Comment 66 Mark Tompsett 2016-02-08 23:00:12 UTC Comment hidden (obsolete)
Comment 67 Mark Tompsett 2016-02-08 23:01:31 UTC
I wasn't attempting to gauge the prettiness of the UI for this sign off. :)
Comment 68 Martin Renvoize 2016-04-01 13:57:05 UTC
Created attachment 49785 [details] [review]
Bug 10988 - Fixes for comments 57 and 58

    Test Plan (remains the same):
     0) Back up your database
     1) Apply all these patches
     2) In your mysql client use your Koha database and execute:
        > DELETE FROM systempreferences;
        > SOURCE ~/kohaclone/installer/data/mysql/sysprefs.sql;
        -- Should be no errors.
        > SELECT * FROM systempreferences LIKE 'GoogleO%';
        -- Should see 4 entries.
        > QUIT;
     3) Restore your database
     4) Run ./installer/data/mysql/updatedatabase.pl;
     5) In your mysql client use your Koha database and execute:
        > SELECT * FROM systempreferences LIKE 'GoogleO%';
        -- Should see the same 4 entries.
     6) Log into the staff client
     7) Home -> Koha administration -> Global system preferences
     8) -> OPAC
        -- make sure your OPACBaseURL is set (e.g. https://opac.koha.ca)
     9) -> Administration
        -- There should be a 'Google OAuth2' section with the ability
           to set those 4 system preferences.
    10) In a new tab, go to https://console.developers.google.com/project
    11) Click 'Create Project'
    12) Type in a project name that won't freak users out, like your
        library name (e.g. South Pole Library).
    13) Click the 'Create' button.
    14) Click the 'APIs & auth' in the left frame.
    15) Click 'Credentials'
    16) Click 'Create new Client ID'
    17) Select 'Web application' and click 'Configure consent screen'.
    18) Select the Email Address.
    19) Put it a meaningful string into the Product Name
        (e.g. South Pole Library Authentication)
    20) Fill in the other fields as desired (or not)
    21) Click 'Save'
    22) Change the 'AUTHORIZED JAVASCRIPT ORIGINS' to your OPACBaseURL.
        (http://library.yourDNS.org)
    23) Change the 'AUTHORIZED REDIRECT URIS' to point to the new
        googleoauth2 script
        (http://library.yourDNS.org/cgi-bin/koha/svc/auth/googleopenidconnect)
    24) Click 'Create Client ID'
    25) Copy and paste the 'CLIENT ID' into the GoogleOAuth2ClientID
        system preference.
    26) Copy and paste the 'CLIENT SECRET' into the GoogleOAuth2ClientSecret
        system preference.
    27) Change the GoogleOpenIDConnect preference to 'Use'.
    28) Click 'Save all Administration preferences'
    29) In the OPAC, click 'Log in to your account'.
        -- You should get a confirmation request, if you are
            already logged in, OR a login screen if you are not.
        -- You need to have the primary email address set to one
           authenticated by Google in order to log in.
    30) Run koha qa test tools

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 69 Martin Renvoize 2016-04-01 13:57:09 UTC
Created attachment 49786 [details] [review]
Bug 10988 - Fix some wording Fix some outdated wording in googleopenidconnect

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 70 Martin Renvoize 2016-04-01 13:57:13 UTC
Created attachment 49787 [details] [review]
Bug 10988: Tabs vs Spaces qa failure patch

Run Koha QA Test tools and discovered this failed because of tabs.
Rather than fail this and wait forever for it to get fixed, this
patches it, and I'll mark it as signed off anyways.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 71 Martin Renvoize 2016-04-01 13:58:36 UTC
Looks great, works great, good enhancement.. well done!
Comment 72 Brendan Gallagher 2016-04-01 19:51:18 UTC
Pushed to Master - Should be in the May 2016 release.  Thanks!
Comment 73 Nicholas van Oudtshoorn 2016-04-04 01:39:31 UTC
Thanks Martin, Mark and Brendan! It feels great to have this done! 😃
Comment 74 David Cook 2016-04-10 23:51:46 UTC
(In reply to Brendan Gallagher from comment #72)
> Pushed to Master - Should be in the May 2016 release.  Thanks!

Neato! I should take a look at this again. I haven't had time/money to upstream my generic OpenID Connect stuff, but maybe I can use this to replace some of my homespun code...
Comment 75 Marian 2017-09-21 08:43:08 UTC
Does this solution is still working on Koha 17, I have tried to apply the patches but I receive the fallowing error
 Hunk #1 FAILED at 137.
1 out of 1 hunk FAILED -- saving rejects to file /usr/share/koha/opac/htdocs/opac-tmpl/bootstrap/en/modules/opac-auth.tt.rej
patching file opac/svc/auth/googleopenidconnect
patch unexpectedly ends in middle of line
Comment 76 Mark Tompsett 2017-09-21 10:48:30 UTC
(In reply to Marian from comment #75)
> Does this solution is still working on Koha 17, I have tried to apply the
> patches but I receive the fallowing error
>  Hunk #1 FAILED at 137.
> 1 out of 1 hunk FAILED -- saving rejects to file
> /usr/share/koha/opac/htdocs/opac-tmpl/bootstrap/en/modules/opac-auth.tt.rej
> patching file opac/svc/auth/googleopenidconnect
> patch unexpectedly ends in middle of line

It's already in Koha 17.05, so no need to apply. It was pushed (see comment #72). Changing status.
Comment 77 David Cook 2022-06-23 02:21:40 UTC
When bug 30988 is ready, which will be very soon, I think that it could replace this Google-specific functionality. 

Then we could just maintain the 1 generic OpenID Connect client.

In the past, I've had libraries that need multiple Open ID Connect clients supported and I used bug 21586 for that, but I could develop that functionality later for bug 30988 if necessary. 

Actually, if I did that, then we could seamlessly migrate the googleopenidconnect to the generic model just by adding it as a generic provider. 

Something to think about at some point...
Comment 78 David Cook 2022-06-24 00:36:35 UTC
Just noting that I've signed off Bug 30988 so it would be great to get some QA eyes on it.
Comment 79 David Cook 2022-11-14 22:19:25 UTC
I suspect eventually this will be deprecated and removed now that we have bug 31378 in master.