Bug 16892 - Add automatic patron registration via OAuth2 login
Summary: Add automatic patron registration via OAuth2 login
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: M. Tompsett
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 10988
Blocks:
  Show dependency treegraph
 
Reported: 2016-07-11 04:39 UTC by M. Tompsett
Modified: 2017-07-31 17:08 UTC (History)
11 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 16892: Add automatic patron registration via OAuth2 login (5.26 KB, patch)
2016-07-11 04:50 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 16892: System Preferences (1.70 KB, patch)
2016-07-19 12:57 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 16892: Follow up with adding missing IGNORE (1.30 KB, patch)
2016-07-19 14:50 UTC, M. Tompsett
Details | Diff | Splinter Review
domain (12.97 KB, image/png)
2016-10-10 04:00 UTC, Jerwyn
Details
Bug 16892: Add automatic patron registration via OAuth2 login (5.29 KB, patch)
2016-11-03 03:23 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 16892: System Preferences (1.70 KB, patch)
2016-11-03 03:23 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 16892: Follow up with adding missing IGNORE (1.30 KB, patch)
2016-11-03 03:23 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 16892: Add automatic patron registration via OAuth2 login (5.34 KB, patch)
2016-12-22 05:07 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 16892: System Preferences (1.75 KB, patch)
2016-12-22 05:07 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 16892: Follow up with adding missing IGNORE (1.35 KB, patch)
2016-12-22 05:07 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 16892: Follow up use AddMember as per QA comment (1.78 KB, patch)
2017-04-03 17:07 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 16892: Add automatic patron registration via OAuth2 login (5.33 KB, patch)
2017-04-03 17:08 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 16892: System Preferences (1.70 KB, patch)
2017-04-03 17:08 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 16892: Follow up with adding missing IGNORE (1.30 KB, patch)
2017-04-03 17:08 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 16892: Follow up use AddMember as per QA comment (1.78 KB, patch)
2017-04-03 17:08 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 16892: Add automatic patron registration via OAuth2 login (5.38 KB, patch)
2017-04-05 14:00 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 16892: System Preferences (1.75 KB, patch)
2017-04-05 14:00 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 16892: Follow up with adding missing IGNORE (1.36 KB, patch)
2017-04-05 14:00 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 16892: Follow up use AddMember as per QA comment (1.84 KB, patch)
2017-04-05 14:00 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 16892: Add automatic patron registration via OAuth2 login (5.44 KB, patch)
2017-05-04 14:59 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16892: System Preferences (1.81 KB, patch)
2017-05-04 14:59 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16892: Follow up with adding missing IGNORE (1.42 KB, patch)
2017-05-04 14:59 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16892: Follow up use AddMember as per QA comment (1.90 KB, patch)
2017-05-04 14:59 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16892: Address error checking in comment #47 (4.21 KB, patch)
2017-07-28 15:53 UTC, M. Tompsett
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description M. Tompsett 2016-07-11 04:39:36 UTC
If a patron's email address is authenticated by Google, and is not registered as a primary email address, automatically register the patron to allow login access.
Comment 1 M. Tompsett 2016-07-11 04:50:01 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
Comment 2 M. Tompsett 2016-07-19 12:57:27 UTC
Created attachment 53485 [details] [review]
Bug 16892: System Preferences

This adds the missing system preferences for a fresh install.
Comment 3 M. Tompsett 2016-07-19 14:35:32 UTC
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.
Comment 4 M. Tompsett 2016-07-19 14:50:05 UTC
Created attachment 53488 [details] [review]
Bug 16892: Follow up with adding missing IGNORE
Comment 5 David Cook 2016-09-12 02:41:43 UTC
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?
Comment 6 Jerwyn 2016-10-09 06:48:16 UTC
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!
Comment 7 M. Tompsett 2016-10-09 12:42:00 UTC
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 8 M. Tompsett 2016-10-09 12:44:26 UTC
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?
Comment 9 Jerwyn 2016-10-10 04:00:21 UTC
Created attachment 56081 [details]
domain
Comment 10 Jerwyn 2016-10-10 04:01:00 UTC
Yeap good thing its already available.
Comment 11 David Cook 2016-10-11 06:17:45 UTC
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.
Comment 12 M. Tompsett 2016-10-11 10:41:54 UTC
(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. ;)
Comment 13 M. Tompsett 2016-11-03 03:23:43 UTC
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
Comment 14 M. Tompsett 2016-11-03 03:23:50 UTC
Created attachment 57112 [details] [review]
Bug 16892: System Preferences

This adds the missing system preferences for a fresh install.
Comment 15 M. Tompsett 2016-11-03 03:23:54 UTC
Created attachment 57113 [details] [review]
Bug 16892: Follow up with adding missing IGNORE
Comment 16 M. Tompsett 2016-11-03 03:54:01 UTC
Rebased.
Comment 17 PTFS Europe Sandboxes 2016-12-22 05:06:08 UTC
Patch tested with a sandbox, by Eugene Espinoza <eugenegf@yahoo.com>
Comment 18 PTFS Europe Sandboxes 2016-12-22 05:07:06 UTC
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>
Comment 19 PTFS Europe Sandboxes 2016-12-22 05:07:09 UTC
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>
Comment 20 PTFS Europe Sandboxes 2016-12-22 05:07:12 UTC
Created attachment 58375 [details] [review]
Bug 16892: Follow up with adding missing IGNORE

Signed-off-by: Eugene Espinoza <eugenegf@yahoo.com>
Comment 21 Jonathan Druart 2016-12-27 15:48:31 UTC
Koha::Patron->new should not be called directly yet.
Please use AddMember instead (unless there is a good reason?).
Comment 22 M. Tompsett 2017-04-03 16:41:03 UTC
(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.
Comment 23 M. Tompsett 2017-04-03 17:07:19 UTC
Created attachment 61800 [details] [review]
Bug 16892: Follow up use AddMember as per QA comment

Same Test plan as before.
Comment 24 M. Tompsett 2017-04-03 17:08:07 UTC
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
Comment 25 M. Tompsett 2017-04-03 17:08:13 UTC
Created attachment 61802 [details] [review]
Bug 16892: System Preferences

This adds the missing system preferences for a fresh install.
Comment 26 M. Tompsett 2017-04-03 17:08:18 UTC
Created attachment 61803 [details] [review]
Bug 16892: Follow up with adding missing IGNORE
Comment 27 M. Tompsett 2017-04-03 17:08:24 UTC
Created attachment 61804 [details] [review]
Bug 16892: Follow up use AddMember as per QA comment

Same Test plan as before.
Comment 28 sandboxes@biblibre.com 2017-04-05 13:59:12 UTC
Patch tested with a sandbox, by Eugene Jose Espinoza <eugenegf@yahoo.com>
Comment 29 sandboxes@biblibre.com 2017-04-05 14:00:08 UTC
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>
Comment 30 sandboxes@biblibre.com 2017-04-05 14:00:14 UTC
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>
Comment 31 sandboxes@biblibre.com 2017-04-05 14:00:19 UTC
Created attachment 61888 [details] [review]
Bug 16892: Follow up with adding missing IGNORE

Signed-off-by: Eugene Jose Espinoza <eugenegf@yahoo.com>
Comment 32 sandboxes@biblibre.com 2017-04-05 14:00:23 UTC
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>
Comment 33 M. Tompsett 2017-04-25 18:29:20 UTC
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.
Comment 34 Jonathan Druart 2017-05-03 20:03:06 UTC
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.
Comment 35 M. Tompsett 2017-05-03 22:39:39 UTC
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.
Comment 36 M. Tompsett 2017-05-03 23:00:53 UTC
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. :)
Comment 37 Nick Clemens 2017-05-04 14:59:10 UTC
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>
Comment 38 Nick Clemens 2017-05-04 14:59:15 UTC
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>
Comment 39 Nick Clemens 2017-05-04 14:59:19 UTC
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>
Comment 40 Nick Clemens 2017-05-04 14:59:23 UTC
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>
Comment 41 Nick Clemens 2017-05-04 15:02:18 UTC
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
Comment 42 M. Tompsett 2017-05-04 18:49:51 UTC Comment hidden (obsolete)
Comment 43 Martin Renvoize 2017-05-04 19:46:53 UTC Comment hidden (obsolete)
Comment 44 Nick Clemens 2017-05-05 01:28:46 UTC Comment hidden (obsolete)
Comment 45 M. Tompsett 2017-05-05 01:32:14 UTC Comment hidden (obsolete)
Comment 46 Nick Clemens 2017-07-21 13:16:52 UTC
Moving to PQA, Jonathan can comment at Rm level now :-)
Comment 47 Jonathan Druart 2017-07-24 18:56:13 UTC
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).
Comment 48 David Cook 2017-07-25 00:27:39 UTC
(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...
Comment 49 M. Tompsett 2017-07-25 02:27:46 UTC
(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.
Comment 50 Jonathan Druart 2017-07-25 12:50:33 UTC
(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.
Comment 51 M. Tompsett 2017-07-28 15:53:52 UTC
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