Bug 18118

Summary: Unexpected behaviour with 'GoogleOpenIDConnect' and 'OpacPublic' syspref combination
Product: Koha Reporter: Mason James <mtj>
Component: OPACAssignee: Mason James <mtj>
Status: CLOSED FIXED QA Contact: Kyle M Hall <kyle>
Severity: normal    
Priority: P5 - low CC: katrin.fischer, kyle, mtompset
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 10988, 18144    
Bug Blocks:    
Attachments: Bug 18118 - Unexpected behaviour with 'GoogleOpenIDConnect' and 'OpacPublic' syspref combination.
Bug 18118 - Unexpected behaviour with 'GoogleOpenIDConnect' and 'OpacPublic' syspref combination.
Bug 18118 - Unexpected behaviour with 'GoogleOpenIDConnect' and 'OpacPublic' syspref combination.

Description Mason James 2017-02-15 04:53:21 UTC
When 'OpacPublic' is set to NO, a successful login will load the main OPAC page (opac-main.pl), that's good, correct and expected

The bug is that with 'OpacPublic' set to OFF and 'GoogleOpenIDConnect' set ON - a successful login redirects the user to their account page (opac-user.pl) not the main OPAC page, that's unexpected and bad
Comment 1 Mason James 2017-02-15 04:54:27 UTC
(In reply to Mason James from comment #0)
> When 'OpacPublic' is set to NO, a successful login will load the main OPAC
> page (opac-main.pl), that's good, correct and expected
> 
> The bug is that with 'OpacPublic' set to OFF and 'GoogleOpenIDConnect' set
> ON - a successful login redirects the user to their account page
> (opac-user.pl) not the main OPAC page, that's unexpected and bad

oops, s/NO/OFF/
Comment 2 Mason James 2017-02-15 06:45:47 UTC
Created attachment 60236 [details] [review]
Bug 18118 - Unexpected behaviour with 'GoogleOpenIDConnect' and 'OpacPublic' syspref combination.

to test this patch...
1/ configure a working 'GoogleOpenIDConnect' account

2/ set 'OpacPublic' syspref to OFF

3/ log in user successfully via google-auth, observe redirect to opac-user.pl (bad)

4/ apply patch

5/ log in user successfully via google-auth, observe expected redirect to opac-main.pl (good)
Comment 3 Mark Tompsett 2017-02-20 07:25:22 UTC
Google Open ID Connect authentication broke when bug 17486 was pushed. Need to fix it before testing this. See bug 18144.
Comment 4 Katrin Fischer 2017-10-15 11:34:13 UTC
Dependencies have been resolved and this should now be testable again even with a sandbox.

Sadly I have no idea about how to set up GoogleOpenIDConnect. Can you provide a more detailed test plan? Maybe a list to the mailing list would also help find a tester using this feature.
Comment 5 Mark Tompsett 2017-10-15 16:09:26 UTC
The instructions are generally on https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16892#c3

The difficulty is Google keeps changing their API so it is a pain.

Between steps 10 and 11:
Once you've created the project, and you get the nice notification in the top right (bell should have a pretty red 1)

click the notification and you will be taken to a massive information overload page where it makes no sense to a normal person with many boxes.

In the top left, you will notice that the top left area will have a nice three line square beside "Google APIs" and then the project you just created in a drop down. Click the three line square.

This will give you a drop down left pane that has "API & Services" at the top.

Hover over it, and then you will get a submenu with 'Credentials' at the bottom.

Step 11: Click credentials
Comment 6 Mark Tompsett 2017-10-15 16:19:33 UTC
(In reply to M. Tompsett from comment #5)
> The difficulty is Google keeps changing their API so it is a pain.

I meant UI, sorry. Still, you would hope they would stop re-arranging things.
Comment 7 Mark Tompsett 2017-10-17 23:20:52 UTC
Okay... So currently:
OpacPublic: Disabled, GoogleOpenIDConnect: No -> General search page
OpacPublic: Enabled, GoogleOpenIDConnect: No -> User Page
OpacPublic: Disabled, GoogleOpenIDConnect: Yes -> User Page
OpacPublic: Enabled, GoogleOpenIDConnect: Yes -> User Page

Hence the test plan where GoogleOpenIDConnect is Yes, and OpacPublic is Disabled.

> 1/ configure a working 'GoogleOpenIDConnect' account

See comment #5 which also links back to https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16892#c3

> 2/ set 'OpacPublic' (under OPAC) to 'Disabled' and 'GoogleOpenIDConnect' (under Administration) to 'Yes'.


> 3/ log in user successfully via google-auth, observe redirect to
> opac-user.pl (bad)

As noted above in my preliminary testing.


> 4/ apply patch

-- on kohadevbox remember to restart all! Plack is unforgiving. :)


> 5/ log in user successfully via google-auth, observe expected redirect to
> opac-main.pl (good)

-- which matches the behaviour regardless of GoogleOpenIDConnect, as it should.
Comment 8 Mark Tompsett 2017-10-17 23:27:31 UTC
Created attachment 68237 [details] [review]
Bug 18118 - Unexpected behaviour with 'GoogleOpenIDConnect' and 'OpacPublic' syspref combination.

TEST PLAN
----------
1/ configure a working 'GoogleOpenIDConnect' account

See comment #5 which also links back to
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16892#c3

2/ set 'OpacPublic' (under OPAC) to 'Disabled' and
   'GoogleOpenIDConnect' (under Administration) to 'Yes'.

3/ log in user successfully via google-auth, observe redirect to
   opac-user.pl (bad)

4/ apply patch
   -- on kohadevbox remember to restart all! Plack is unforgiving. :)

5/ log in user successfully via google-auth, observe expected
   redirect to opac-main.pl (good)

While I would normally suggest running koha qa test tools, because
this file doesn't end in .pl, it doesn't get picked up by them.

6/ perlcritic -4 opac/svc/auth/googleopenidconnect
   -- notice this is a level better than required. :)

This also eyeballs easily well.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 9 Kyle M Hall 2017-10-27 13:15:50 UTC
Created attachment 68694 [details] [review]
Bug 18118 - Unexpected behaviour with 'GoogleOpenIDConnect' and 'OpacPublic' syspref combination.

TEST PLAN
----------
1/ configure a working 'GoogleOpenIDConnect' account

See comment #5 which also links back to
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16892#c3

2/ set 'OpacPublic' (under OPAC) to 'Disabled' and
   'GoogleOpenIDConnect' (under Administration) to 'Yes'.

3/ log in user successfully via google-auth, observe redirect to
   opac-user.pl (bad)

4/ apply patch
   -- on kohadevbox remember to restart all! Plack is unforgiving. :)

5/ log in user successfully via google-auth, observe expected
   redirect to opac-main.pl (good)

While I would normally suggest running koha qa test tools, because
this file doesn't end in .pl, it doesn't get picked up by them.

6/ perlcritic -4 opac/svc/auth/googleopenidconnect
   -- notice this is a level better than required. :)

This also eyeballs easily well.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Jonathan Druart 2017-10-27 17:13:53 UTC
Pushed to master for 17.11, thanks to everybody involved!