[Koha-bugs] [Bug 10988] Allow login via Google OAuth2

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

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10988

--- Comment #57 from Martin Renvoize <martin.renvoize at ptfs-europe.com> ---
Comment on attachment 46016
  --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=46016
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:
 --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=10988&attachment=46016)
-----------------------------------------------------------------

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&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.

-- 
You are receiving this mail because:
You are watching all bug changes.