|
Description
Mark Tompsett
2016-07-11 04:39:36 UTC
Created attachment 53257 [details] [review] Bug 16892: Add automatic patron registration via OAuth2 login 10988 added the ability to log into the OPAC authenticating with Google Open ID Connect. This extends it, by allowing an unregistered patron to have an account automatically created with default category code and branch. This is accomplished by adding 3 system preferences. - GoogleOpenIDConnectAutoRegister whether it will attempt to auto-register the patron. - GoogleOpenIDConnectDefaultCategory This category code will be used to create Google OpenID Connect patrons. - GoogleOpenIDConnectDefaultBranch' This branch code will be used to create Google OpenID Connect patrons. Sponsored-by: Tulong Aklatan Created attachment 53485 [details] [review] Bug 16892: System Preferences This adds the missing system preferences for a fresh install. TEST PLAN
---------
1) backup the database
2) run upgrade: ./installer/data/mysql/updatedatabase.pl
-- proves the atomic update.
3) run it again:
-- proves the INSERT IGNORE is done
4) drop the database and recreate it
mysql> drop database koha_library;
mysql> create database koha_library;
5) run the web installer
6) log into the staff client
7) Home -> Koha administration
-> Global system preferences
-> Administration
-- GoogleOpenIDConnectAutoRegister,
GoogleOpenIDConnectDefaultBranch, and
GoogleOpenIDConnectDefaultCategory should be visible
in the "Google OpenID Connect" section.
-- proves the admin.pref is correct
8) In a new tab, go to https://console.developers.google.com/project
and log in.
9) Click 'CREATE PROJECT'
10) Enter a project name which isn't going to scare the user off.
(e.g. {institution} Library Authentication)
11) Wait a bit, then click 'Credentials' on the left pane.
12) Click the 'Create credentials' drop down button, and
select 'OAuth client ID'
13) Click the 'Configure consent screen' button
14) Enter at least a product name which isn't going to scare the
user off.
(e.g. {institution} Library)
15) Click 'Save'
16) Select 'Web application'
17) Enter at least a memorable name whose purpose is clear.
(e.g. {institution} Library Credentials)
18) Enter the OPAC URL into the 'Authorized JavaScript origins' text box
19) Enter a URL of the form:
{OPACBaseURL}/cgi-bin/koha/svc/auth/googleopenidconnect
into the 'Authorized redirect URIs' text box.
20) Click the 'Create' button.
21) Copy the client ID into the 'GoogleOAuth2ClientID' system preference
22) Copy the client secret into the 'GoogleOAuth2ClientSecret' system preference
23) Change the 'GoogleOpenIDConnect' system preference to 'Use'.
24) Change the 'GoogleOpenIDConnectAutoRegister' system preference to 'Allow'
25) Enter default branch and category values.
(for all sample data: GoogleOpenIDConnectDefaultBranch=CPL,
GoogleOpenIDConnectDefaultCategory=PT)
26) Click 'Save all Administration preferences'
27) Click 'OPAC' in the left pane.
28) Make sure OPACBaseURL is set.
27) Open the OPAC and click 'Log in to your account'
28) Click 'Allow' to authorize Google.
-- should end up at opac-user page.
-- proves everything works. :)
NOTE: Because of the nature of OAuth, authenticating this way does
not force a Google log out and the user MUST make a secondary
effort to log out or close the browser. Otherwise, a person
from the same terminal could abuse that person's Google account.
Created attachment 53488 [details] [review] Bug 16892: Follow up with adding missing IGNORE This seems odd to me. I have a local Open ID Connect implementation for a Koha client, but they control the authentication server too. I auto-register anyone missing from Koha who already exists on the server, but I don't understand why you would if you didn't control the server. What library would want any Gmail user auto-enrolled? Hi Mark, Thanks a lot for your inputs! Dear David, This feature will be useful for those libraries that has multiple branches miles and miles away, that employees usually come and go often. The mother library or main library who registers the patron accounts in Koha is having a hard time tracking them down. With the centralized email registration in gmail they can limit all the automatic registration in Koha limiting it to its domain name. This would be a great feature for those libraries that has a single patron category however, it may have implications with libraries that has different circulation policies, for different patron categories. Looking forward to this. Thank you and best regards! Jerwyn, I think you missed David's point. This does not limit to a particular domain. Though, you do bring up a valid counter point, but would need further implementation. Google does provide domain hosting capabilities which those using their services do control the domain. There is currently no logic in this code to actually limit the domain, which is what David is complaining about, I think. Comment on attachment 53257 [details] [review] Bug 16892: Add automatic patron registration via OAuth2 login Review of attachment 53257 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref @@ +161,2 @@ > - Google OpenID Connect Restrict to domain (or subdomain of this domain) > - pref: GoogleOpenIDConnectDomain Oh wait. That is already implemented. Next problem? Created attachment 56081 [details]
domain
Yeap good thing its already available. Ahh, sounds good. I suppose I'm still not happy that this feature is Google-specific and wasn't made to be more general, but I had the opportunity to contribute my Open ID Connect code and didn't, so I can't complain too much :p. (In reply to David Cook from comment #11) > Ahh, sounds good. > > I suppose I'm still not happy that this feature is Google-specific and > wasn't made to be more general, but I had the opportunity to contribute my > Open ID Connect code and didn't, so I can't complain too much :p. Generalization could be a follow up bug report. ;) Created attachment 57111 [details] [review] Bug 16892: Add automatic patron registration via OAuth2 login 10988 added the ability to log into the OPAC authenticating with Google Open ID Connect. This extends it, by allowing an unregistered patron to have an account automatically created with default category code and branch. This is accomplished by adding 3 system preferences. - GoogleOpenIDConnectAutoRegister whether it will attempt to auto-register the patron. - GoogleOpenIDConnectDefaultCategory This category code will be used to create Google OpenID Connect patrons. - GoogleOpenIDConnectDefaultBranch' This branch code will be used to create Google OpenID Connect patrons. Sponsored-by: Tulong Aklatan Created attachment 57112 [details] [review] Bug 16892: System Preferences This adds the missing system preferences for a fresh install. Created attachment 57113 [details] [review] Bug 16892: Follow up with adding missing IGNORE Rebased. Patch tested with a sandbox, by Eugene Espinoza <eugenegf@yahoo.com> Created attachment 58373 [details] [review] Bug 16892: Add automatic patron registration via OAuth2 login 10988 added the ability to log into the OPAC authenticating with Google Open ID Connect. This extends it, by allowing an unregistered patron to have an account automatically created with default category code and branch. This is accomplished by adding 3 system preferences. - GoogleOpenIDConnectAutoRegister whether it will attempt to auto-register the patron. - GoogleOpenIDConnectDefaultCategory This category code will be used to create Google OpenID Connect patrons. - GoogleOpenIDConnectDefaultBranch' This branch code will be used to create Google OpenID Connect patrons. Sponsored-by: Tulong Aklatan Signed-off-by: Eugene Espinoza <eugenegf@yahoo.com> Created attachment 58374 [details] [review] Bug 16892: System Preferences This adds the missing system preferences for a fresh install. Signed-off-by: Eugene Espinoza <eugenegf@yahoo.com> Created attachment 58375 [details] [review] Bug 16892: Follow up with adding missing IGNORE Signed-off-by: Eugene Espinoza <eugenegf@yahoo.com> Koha::Patron->new should not be called directly yet. Please use AddMember instead (unless there is a good reason?). (In reply to Jonathan Druart from comment #21) > Koha::Patron->new should not be called directly yet. > Please use AddMember instead (unless there is a good reason?). Valid point, due to add_enrollment_fee_if_needed. Fixing and retesting. Created attachment 61800 [details] [review] Bug 16892: Follow up use AddMember as per QA comment Same Test plan as before. Created attachment 61801 [details] [review] Bug 16892: Add automatic patron registration via OAuth2 login 10988 added the ability to log into the OPAC authenticating with Google Open ID Connect. This extends it, by allowing an unregistered patron to have an account automatically created with default category code and branch. This is accomplished by adding 3 system preferences. - GoogleOpenIDConnectAutoRegister whether it will attempt to auto-register the patron. - GoogleOpenIDConnectDefaultCategory This category code will be used to create Google OpenID Connect patrons. - GoogleOpenIDConnectDefaultBranch' This branch code will be used to create Google OpenID Connect patrons. Sponsored-by: Tulong Aklatan Created attachment 61802 [details] [review] Bug 16892: System Preferences This adds the missing system preferences for a fresh install. Created attachment 61803 [details] [review] Bug 16892: Follow up with adding missing IGNORE Created attachment 61804 [details] [review] Bug 16892: Follow up use AddMember as per QA comment Same Test plan as before. Patch tested with a sandbox, by Eugene Jose Espinoza <eugenegf@yahoo.com> Created attachment 61886 [details] [review] Bug 16892: Add automatic patron registration via OAuth2 login 10988 added the ability to log into the OPAC authenticating with Google Open ID Connect. This extends it, by allowing an unregistered patron to have an account automatically created with default category code and branch. This is accomplished by adding 3 system preferences. - GoogleOpenIDConnectAutoRegister whether it will attempt to auto-register the patron. - GoogleOpenIDConnectDefaultCategory This category code will be used to create Google OpenID Connect patrons. - GoogleOpenIDConnectDefaultBranch' This branch code will be used to create Google OpenID Connect patrons. Sponsored-by: Tulong Aklatan Signed-off-by: Eugene Jose Espinoza <eugenegf@yahoo.com> Created attachment 61887 [details] [review] Bug 16892: System Preferences This adds the missing system preferences for a fresh install. Signed-off-by: Eugene Jose Espinoza <eugenegf@yahoo.com> Created attachment 61888 [details] [review] Bug 16892: Follow up with adding missing IGNORE Signed-off-by: Eugene Jose Espinoza <eugenegf@yahoo.com> Created attachment 61889 [details] [review] Bug 16892: Follow up use AddMember as per QA comment Same Test plan as before. Signed-off-by: Eugene Jose Espinoza <eugenegf@yahoo.com> Given that Google has changed their interface, I thought I'd post a newer update on how to link Google OAuth. Go to https://console.developers.google.com/project Log in Click '+ Create Project' Type in a project name and click 'CREATE' Click 'Credentials' in the left panel Click the 'Create credentials' drop-down button Click 'OAuth client ID' Click 'Configure consent screen' Put it a meaningful string into the Product Name (e.g. South Pole Library Authentication) Fill in the other fields as desired (or not) Click 'Save' Click the 'Web application' radio button Change the 'AUTHORIZED JAVASCRIPT ORIGINS' to your OPACBaseURL. (http://localhost:8080) Change the 'AUTHORIZED REDIRECT URIS' to point to the new googleoauth2 script (http://localhost:8080/cgi-bin/koha/svc/auth/googleopenidconnect) Click 'Create' Copy and paste the 'Client ID' into the GoogleOAuth2ClientID system preference. Copy and paste the 'client secret' into the GoogleOAuth2ClientSecret system preference. Change the GoogleOpenIDConnect system preference to 'Use'. Click 'Save all Administration preferences' 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. Can we get an updated test plan? It sounds like the Google APIs interface has changed. The last line you added to opac/svc/auth/googleopenidconnect (Koha::Patrons->find) is not necessary, $borrower is not used later. ARG! I really hate them constantly changing the interface. It makes it so much harder to help people set up. First you need to confirm the Google OAuth works normally: Go to https://console.developers.google.com/project Log in Click '+ Create Project' Type in a project name and click 'CREATE' -- the notification bell will highlight Select the creation notification with the project just created Click 'Go to APIs overview' Click 'Credentials' in the left panel Click the 'Create credentials' drop-down button Click 'OAuth client ID' Click 'Configure consent screen' Put it a meaningful string into the Product Name (e.g. South Pole Library Authentication) Fill in the other fields as desired (or not) Click 'Save' Click the 'Web application' radio button Change the 'AUTHORIZED JAVASCRIPT ORIGINS' to your OPACBaseURL. (http://localhost:8080) Change the 'AUTHORIZED REDIRECT URIS' to point to the new googleoauth2 script (http://localhost:8080/cgi-bin/koha/svc/auth/googleopenidconnect) Click 'Create' Copy and paste the 'Client ID' into the GoogleOAuth2ClientID system preference. Copy and paste the 'client secret' into the GoogleOAuth2ClientSecret system preference. Change the GoogleOpenIDConnect system preference to 'Use'. Click 'Save all Administration preferences' Click 'OK' to dismiss the client ID and client secret dialogue. 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. -- OPACBaseURL system preference needs to be set as well. This confirms you have the Google Authentication set up. Make sure your google email is not associated with an account now, or you have a second account to test with that has no patron matching. Apply the patch Upgrade the database Make sure you set the three new system preferences: GoogleOpenIDConnectAutoRegister - Allow GoogleOpenIDConnectDefaultBranch - Any valid existing branchcode. GoogleOpenIDConnectDefaultCategory - Any valid existing patron category. Attempt to log in with a Google account not associated with a Koha account. -- should succeed Searching for patron in Staff client should show they are that branch and category. Run koha qa test tools. I haven't tested it, but I think the find is a way of ensuring the member has been added. A cache flush? A pause? Remove it and test with a plack-based system, before deciding to remove it. Also, before the code was added and at the end means for debugging, I could assert that $borrower would be a valid patron. Which is conceptually beautiful. :) Created attachment 63117 [details] [review] Bug 16892: Add automatic patron registration via OAuth2 login 10988 added the ability to log into the OPAC authenticating with Google Open ID Connect. This extends it, by allowing an unregistered patron to have an account automatically created with default category code and branch. This is accomplished by adding 3 system preferences. - GoogleOpenIDConnectAutoRegister whether it will attempt to auto-register the patron. - GoogleOpenIDConnectDefaultCategory This category code will be used to create Google OpenID Connect patrons. - GoogleOpenIDConnectDefaultBranch' This branch code will be used to create Google OpenID Connect patrons. Sponsored-by: Tulong Aklatan Signed-off-by: Eugene Jose Espinoza <eugenegf@yahoo.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 63118 [details] [review] Bug 16892: System Preferences This adds the missing system preferences for a fresh install. Signed-off-by: Eugene Jose Espinoza <eugenegf@yahoo.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 63119 [details] [review] Bug 16892: Follow up with adding missing IGNORE Signed-off-by: Eugene Jose Espinoza <eugenegf@yahoo.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 63120 [details] [review] Bug 16892: Follow up use AddMember as per QA comment Same Test plan as before. Signed-off-by: Eugene Jose Espinoza <eugenegf@yahoo.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Tested with new instrcutions, works as advertised, the code looks good to me I tested under plack/memcached and saw no difference when I removed the find for borrower so think that can be safely removed. One question, out of scope: For CAS we have casLogout - can we have something similar here? I think disconcerting if a patron signs in via google, signs out of Koha and walks away leaving their google auth intact unintentionally Leaving at SO for final comments from Jonathan (In reply to Nick Clemens from comment #41) > I tested under plack/memcached and saw no difference when I removed the find > for borrower so think that can be safely removed. Thanks for testing that. > One question, out of scope: For CAS we have casLogout - can we have > something similar here? I think disconcerting if a patron signs in via > google, signs out of Koha and walks away leaving their google auth intact > unintentionally This is the problem. If you have multiple tabs, let's say gmail in one, and Koha in the other. If Koha does a forced logout, then your gmail tab will be affected. This is the problem that is widely known. I was thinking of actually doing another bug to drop-down choose what kind of log out. A federated logout is actually pretty rare in my experience, though it is possible. As you say Mark, it 'feels' strange to the user that logging out of seemingly unrelated apps can have the knock on effect of logging out apps in other tabs that happen to share an aouth2 openid connect identity provider. On a related note, I'm not sure I saw any code out end to support such a federated logout triggered by another service in our end either... but perhaps I missed it ;) Perhaps if OAuth is activated just a note added that when you log out of Koha you may still be logged in via google? I agree a user with many tabs open may not expect this, but a patron using a shared machine in the library who signs in to place a hold or renew may never visit their email or expect that they have signed in to that. I think a pop-up when logging out to say "Hey, you are logged in to google, and they know lots of stuff about you" wouldn't hurt, who can say if it would help :-) (In reply to Nick Clemens from comment #44) > Perhaps if OAuth is activated just a note added that when you log out of > Koha you may still be logged in via google? [SNIP] Perhaps this type of discussion should happen on another bug report, as this bug has nothing to do with logging out. :) Also, yes, some sort of warning should be displayed, somewhere, somehow. Moving to PQA, Jonathan can comment at Rm level now :-) Everything looks fine to me. However I think it would be good to handle a wrong or non-existent patron category or branchcode (GoogleOpenIDConnectDefaultCategory and GoogleOpenIDConnectDefaultBranch). Usually I am not in favour of over-checking everything but it sounds like it is needed here (in order to avoid the 500). (In reply to Jonathan Druart from comment #47) > Everything looks fine to me. > However I think it would be good to handle a wrong or non-existent patron > category or branchcode (GoogleOpenIDConnectDefaultCategory and > GoogleOpenIDConnectDefaultBranch). Usually I am not in favour of > over-checking everything but it sounds like it is needed here (in order to > avoid the 500). I still think it's a shame that we hard-coded "Google" into the system preferences, when OpenIDConnect can be implemented by other providers, but I think I'm flogging a dead horse on that one... (In reply to Jonathan Druart from comment #47) > However I think it would be good to handle a wrong or non-existent patron > category or branchcode (GoogleOpenIDConnectDefaultCategory and > GoogleOpenIDConnectDefaultBranch). I have no problem with that. That's reasonable. > Usually I am not in favour of > over-checking everything but it sounds like it is needed here (in order to > avoid the 500). I'm having the darnedest time with my $patron_category = Koha::Patron::Categories->find( $valid_category_code ); being undef. :( -- Working on it. (In reply to M. Tompsett from comment #49) > > Usually I am not in favour of > > over-checking everything but it sounds like it is needed here (in order to > > avoid the 500). > > I'm having the darnedest time with my $patron_category = > Koha::Patron::Categories->find( $valid_category_code ); being undef. :( -- > Working on it. Works for me: use Koha::Patron::Categories; my $pt = Koha::Patron::Categories->find('PT'); my $xxx = Koha::Patron::Categories->find('XXX'); say $pt; say $xxx; Koha::Patron::Category=HASH(0x28af7f8) Use of uninitialized value $xxx in say at t.pl line 5. Created attachment 65316 [details] [review] Bug 16892: Address error checking in comment #47 Tweaked logic to include error check for category and branch being valid. If not, the user is told about a configuration error to share with the library. Otherwise, it should proceed as normal. TEST PLAN is comment #35 still. Though include a run with an invalid category and/or branch code in the two system preferences: GoogleOpenIDConnectDefaultBranch and/or GoogleOpenIDConnectDefaultCategory (In reply to David Cook from comment #48) > (In reply to Jonathan Druart from comment #47) > > Everything looks fine to me. > > However I think it would be good to handle a wrong or non-existent patron > > category or branchcode (GoogleOpenIDConnectDefaultCategory and > > GoogleOpenIDConnectDefaultBranch). Usually I am not in favour of > > over-checking everything but it sounds like it is needed here (in order to > > avoid the 500). > > I still think it's a shame that we hard-coded "Google" into the system > preferences, when OpenIDConnect can be implemented by other providers, but I > think I'm flogging a dead horse on that one... Not introduced by this patch set, they use the existing pattern. Pushed to master for 17.11, thanks to everybody involved! Enhancement not pushed to 17.05.x We should've changed this title as it's actually really misleading in hindsight, but oh well! David earns the necromancer badge... (In reply to Martin Renvoize from comment #56) > David earns the necromancer badge... Guilty O_O. The worst part is that I have no idea why I was digging yesterday. Oh wait I was going through release notes for the past few major releases, and this one totally confused me until I realised it was referring to the Google OpenIDConnect functionality. (OAuth2 has to be one of the most misused words in AuthZ/AuthN discussions...but that just earns me the pedant badge >_>.) |