Bug 18880 - Regression breaks local authentication fallback for all external authentications
Summary: Regression breaks local authentication fallback for all external authentications
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: master
Hardware: All All
: P5 - low blocker (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
: 18953 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-06-29 11:54 UTC by Oliver Bock
Modified: 2017-07-31 19:45 UTC (History)
10 users (show)

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


Attachments
Proposed patch (1.14 KB, patch)
2017-06-29 11:54 UTC, Oliver Bock
Details | Diff | Splinter Review
Bug 18880: Regression breaks local authentication fallback for all external authentications (2.20 KB, patch)
2017-07-06 18:39 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 18880: Regression breaks local authentication fallback for all external authentications (1.91 KB, patch)
2017-07-07 07:05 UTC, Oliver Bock
Details | Diff | Splinter Review
Bug 18880: Regression breaks local authentication fallback for all external authentications (1.94 KB, patch)
2017-07-07 12:02 UTC, Lee Jamison
Details | Diff | Splinter Review
[ALTERNATIVE-PATCH] Bug 18880: Fix authentication fallback for external authentications (3.74 KB, patch)
2017-07-11 15:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
[ALTERNATIVE-PATCH] Bug 18880: Fix authentication fallback for external authentications (3.86 KB, patch)
2017-07-11 15:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
[ALTERNATIVE-PATCH] Bug 18880: Fix authentication fallback for external authentications (3.86 KB, patch)
2017-07-12 18:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18880: Fix authentication fallback for external authentications (3.91 KB, patch)
2017-07-13 13:56 UTC, Lee Jamison
Details | Diff | Splinter Review
Bug 18880: Fix authentication fallback for external authentications (4.00 KB, patch)
2017-07-14 06:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18880: [QA Follow-up] Finishing touch (1.80 KB, patch)
2017-07-14 06:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18880: Fix authentication fallback for external authentications (3.90 KB, patch)
2017-07-14 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18880: [QA Follow-up] Finishing touch (1.76 KB, patch)
2017-07-14 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Bock 2017-06-29 11:54:42 UTC
Created attachment 64719 [details] [review]
Proposed patch

A regression in commit cfc484b17 / bug #18314 breaks the local authentication fallback for all external authentications like LDAP, CAS and Shibboleth. The severity is critical for us as we, while using LDAP authentication, use the self-service checkout which requires a local user account. Also non-LDAP staff can't log into the staff pages anymore.

The regression itself is a logical error as "@return = (0)" is considered to be "false" when checked with "unless". That's wrong as "unless" tests the number of elements in a list. Thus the "falsy" condition has to established with "@return = ()" instead.

Please find attached a proposed patch against "17.05.x". Also, please make sure the fallback workflow gets test coverage.

Thanks,
Oliver
Comment 1 Katrin Fischer 2017-07-04 06:01:39 UTC
Updating severity as this is a blocker issue for libraries using one of those external methods with no passwords in the db for their users.
Comment 2 Oliver Bock 2017-07-05 08:18:47 UTC
Given the relative urgency, can we assign this bug directly to the original author of the lines in question? He, Jonathan Druart, should be able to verify my assertions and fix the issue in no time.

Thanks
Comment 3 Jonathan Druart 2017-07-05 16:18:25 UTC
(In reply to Oliver Bock from comment #0)
> The regression itself is a logical error as "@return = (0)" is considered to
> be "false" when checked with "unless". That's wrong as "unless" tests the
> number of elements in a list. Thus the "falsy" condition has to established
> with "@return = ()" instead.

Where did you find this condition?
Comment 5 Jonathan Druart 2017-07-06 14:50:39 UTC
Ha yes I got it!

Unfortunately your patch does not look the way to fix it, if you modify the return value of this subroutine you will certainly introduce several side-effects as it is tested at different place.

I think you can patch it modifying the line with something like:
  @return = checkpw_internal( $dbh, $userid, $password, $no_set_userenv) unless $return[0];
or, if you want to be explicit:
  @return = checkpw_internal( $dbh, $userid, $password, $no_set_userenv) if $return[0] == 0;

At this point @return always contains at least one element.
Comment 6 Oliver Bock 2017-07-06 14:59:52 UTC
> you will certainly introduce several side-effects as it is tested at different place

Are you sure? Where? "My" empty list incarnation of @return is only used until line 1814. After that it's either containing the original returns of the external "checkpw_" calls (lines 1772, 1785, 1806) or the return of "checkpw_internal" (line 1814). So the final return value (line 1825) is completely unaffected by my patch.

Either way, you know what's wrong now and it's your call, being the original author, how to fix it.

Thanks,
Oliver
Comment 7 Jonathan Druart 2017-07-06 15:29:57 UTC
Indeed, you are right!
Comment 8 Katrin Fischer 2017-07-06 18:34:33 UTC
Oliver, could you resubmit as a proper patch? It's missing the "header" with your name and other information. In git you can use git format-patch to create one. Or you can take a look at git-bz on the wiki for a more automated workflow with bugzilla.
Comment 9 M. Tompsett 2017-07-06 18:39:42 UTC
Created attachment 64862 [details] [review]
Bug 18880: Regression breaks local authentication fallback for all external authentications

A regression in commit cfc484b17 / bug #18314 breaks the local
authentication fallback for all external authentications like LDAP, CAS
and Shibboleth. The severity is critical for us as we, while using LDAP
authentication, use the self-service checkout which requires a local
user account. Also non-LDAP staff can't log into the staff pages
anymore.

The regression itself is a logical error as "@return = (0)" is
considered to be "false" when checked with "unless". That's wrong as
"unless" tests the number of elements in a list. Thus the "falsy"
condition has to established with "@return = ()" instead.

Please find attached a proposed patch against "17.05.x". Also, please
make sure the fallback workflow gets test coverage.
Comment 10 Oliver Bock 2017-07-07 07:05:46 UTC
Created attachment 64877 [details] [review]
Bug 18880: Regression breaks local authentication fallback for all external authentications

Thanks M, but I slightly trimmed the commit message.
Comment 11 Oliver Bock 2017-07-07 07:10:20 UTC
Back to Jonathan, ready for sign-off.

Thanks
Comment 12 Lee Jamison 2017-07-07 12:02:03 UTC
Created attachment 64909 [details] [review]
Bug 18880: Regression breaks local authentication fallback for all external authentications

A regression in commit cfc484b17 / bug #18314 breaks the local
authentication fallback for all external authentications like LDAP, CAS
and Shibboleth.

The regression itself is a logical error as "@return = (0)" is
considered to be "false" when checked with "unless" (line 1814). That's
wrong as "unless" tests the number of elements in a list. Thus the
"falsy" condition has to established with "@return = ()" instead.

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>
Comment 13 Lee Jamison 2017-07-07 12:04:26 UTC
For posterity, my sign off is based on testing the patch an LDAP configuration.
Comment 14 Lee Jamison 2017-07-11 12:30:41 UTC
For testers not in an LDAP environment, see Bug 17215 Comment 4 which provides a LDAP stanza which can be used for testing this bug.
Comment 15 Marcel de Rooy 2017-07-11 13:45:07 UTC
Looking here now
Comment 16 Marcel de Rooy 2017-07-11 14:10:07 UTC
Just reading commit cfc484b17 and this patch, I think that we do not completely recover the regression described.
Formerly, a return 0; was issued in a number of cases. Now this patch assigns @return=() instead of (0). Both approaches are not bug free. 
Although we already know that we should return false, we are going to call checkpw_internal now since @return is empty! We should not.
After calling checkpw_internal, @return is changed and the actual results may vary. 

The problem of the regression is in the older commit is here:
return ( $retval, $retcard, $retuserid ) is not necessarily the same as return @return
If we evaluate in scalar context, return @return will return the number of list elements while return (...) returns the last element (here retuserid). Welcome to Perl :)
Evaluation in list context will be the same.
So when @return was (0), return @return did not return a 0 but did return a 1 in scalar context !!
Apparently, the external authentifications broken called this routine in scalar context. I would say that we also (at least theoretically) need to address these calls since they probably relied on $retuserid instead of $retval ! Which in practice might not be a problem btw..
Comment 17 Oliver Bock 2017-07-11 14:20:53 UTC
> Both approaches are not bug free.

What's buggy with my approach itself? The overall context notwithstanding, though.

> Although we already know that we should return false, we are going to call checkpw_internal now since @return is empty! We should not.

Why not? Isn't checkpw_internal meant as a fallback for the external auths? Jonathan seemed to have this in mind.

> So when @return was (0), return @return did not return a 0 but did return a 1 in scalar context !!

That's precisely my point, hence my fix.

Anyhow, I can't judge the overall implementation, I just fixed an obvious bug which the original author already agreed on. If there's a general issue besides the obvious bug I reported shouldn't it be fixed independently?

Thanks
Comment 18 Marcel de Rooy 2017-07-11 14:34:31 UTC
Another thing: The commit mentioned removed this for ldap:
 return 0 if $retval == -1;
by @return = ( $retval, $retcard, $retuserid ) since retval is true for -1.
Comment 19 Marcel de Rooy 2017-07-11 14:36:45 UTC
(In reply to Oliver Bock from comment #17)
> Why not? Isn't checkpw_internal meant as a fallback for the external auths?
> Jonathan seemed to have this in mind.

If the external auth formerly resulted in a return 0; in the old commit, why should we call checkpw_internal now ? We should return false.
Comment 20 Oliver Bock 2017-07-11 14:49:57 UTC
Ah, I see where you're coming from now.

However, I think in the LDAP-case you neglect that fact that "return 0" was *only* called for one specific case (checkpw_ldap returned -1). In the case it failed for other reasons (returning 0) checkpw_internal was indeed called as a fallback.

But you're right for CAS and Shibboleth, no fallback to internal auth there. It seems as you guys need to settle on the expected behavior first, then discuss the concrete implementation. Anyhow, that's out of my realm of expertise.

Thanks
Comment 21 Jonathan Druart 2017-07-11 15:27:06 UTC
Created attachment 65000 [details] [review]
[ALTERNATIVE-PATCH] Bug 18880: Fix authentication fallback for external authentications

A regression in commit cfc484b17 / bug #18314 breaks the local
authentication fallback for all external authentications like LDAP, CAS
and Shibboleth.

The regression itself is a logical error as "@return = (0)" is
considered to be "false" when checked with "unless" (line 1814). That's
wrong as "unless" tests the number of elements in a list. Thus the
"falsy" condition has to established with "@return = ()" instead.

This patch tries to simplify the logic by adding a $passwd_ok and
$check_internal_as_fallback flags to be more verbose and hopefully more
understandable.
The goal here is simply to restore back the same logic as before cfc484b17
Comment 22 Jonathan Druart 2017-07-11 15:27:41 UTC
Oliver, Marcel, could you take a look at this alternative patch please?
Comment 23 Jonathan Druart 2017-07-11 15:35:36 UTC
Created attachment 65001 [details] [review]
[ALTERNATIVE-PATCH] Bug 18880: Fix authentication fallback for external authentications

A regression in commit cfc484b17 / bug #18314 breaks the local
authentication fallback for all external authentications like LDAP, CAS
and Shibboleth.

The regression itself is a logical error as "@return = (0)" is
considered to be "false" when checked with "unless" (line 1814). That's
wrong as "unless" tests the number of elements in a list. Thus the
"falsy" condition has to established with "@return = ()" instead.

This patch tries to simplify the logic by adding a $passwd_ok and
$check_internal_as_fallback flags to be more verbose and hopefully more
understandable.
The goal here is simply to restore back the same logic as before cfc484b17
Comment 24 Oliver Bock 2017-07-11 15:42:50 UTC
Warning: I just looked at the patch and didn't apply it.

I think you lack a "$passwd_ok = 1" after checkpw_internal was called as a LDAP-fallback. IOW, the login attempt counter would be increased despite successful internal login after a LDAP bind error.

Also:
- is the "unless ( $passwd_ok ) {" ever closed?
- the "} elsif ( $patron ) {" doesn't seem to have a parent if-clause

Other than that your patch seems fine to me.
Comment 25 Jonathan Druart 2017-07-11 16:10:38 UTC
(In reply to Oliver Bock from comment #24)
> Warning: I just looked at the patch and didn't apply it.
> 
> I think you lack a "$passwd_ok = 1" after checkpw_internal was called as a
> LDAP-fallback. IOW, the login attempt counter would be increased despite
> successful internal login after a LDAP bind error.

Yes, I caught that and fixed it in the second version.

> Also:
> - is the "unless ( $passwd_ok ) {" ever closed?
> - the "} elsif ( $patron ) {" doesn't seem to have a parent if-clause
> 
> Other than that your patch seems fine to me.

  unless ($cond) {
    # do stuffs
  } elsif ($other_cond) {
    # do other stuff
  }
is a valid perl structure.
Comment 26 Marcel de Rooy 2017-07-12 10:54:34 UTC
LDAP behavior should be reverted in the sense of:
-        return 0 if $retval == -1;                                  # Incorrect password for LDAP login attempt
-        ($retval) and return ( $retval, $retcard, $retuserid );
-    }
Your patch falls back to internal for -1; it should NOT do that.
Internal fallback should be enabled only if retval == 0.

I am also wondering btw if (internal) patron lockout should overrule external authentification like cas and shibboleth. If not, the code could be simpler.
Just noting that this discussion was not touched on bug 18314. From this perspective you could even argue about allowing LDAP fallback.
Adding Martin and Tomas in CC for any feedback.
Comment 27 Jonathan Druart 2017-07-12 18:59:50 UTC
Created attachment 65011 [details] [review]
[ALTERNATIVE-PATCH] Bug 18880: Fix authentication fallback for external authentications

A regression in commit cfc484b17 / bug #18314 breaks the local
authentication fallback for all external authentications like LDAP, CAS
and Shibboleth.

The regression itself is a logical error as "@return = (0)" is
considered to be "false" when checked with "unless" (line 1814). That's
wrong as "unless" tests the number of elements in a list. Thus the
"falsy" condition has to established with "@return = ()" instead.

This patch tries to simplify the logic by adding a $passwd_ok and
$check_internal_as_fallback flags to be more verbose and hopefully more
understandable.
The goal here is simply to restore back the same logic as before cfc484b17
Comment 28 Jonathan Druart 2017-07-12 19:00:38 UTC
(In reply to Marcel de Rooy from comment #26)
> LDAP behavior should be reverted in the sense of:
> -        return 0 if $retval == -1;                                  #
> Incorrect password for LDAP login attempt
> -        ($retval) and return ( $retval, $retcard, $retuserid );
> -    }
> Your patch falls back to internal for -1; it should NOT do that.
> Internal fallback should be enabled only if retval == 0.

Right!

-        $check_internal_as_fallback = 1 if $retval == -1;
+        $check_internal_as_fallback = 1 if $retval == 0;
Comment 29 Lee Jamison 2017-07-13 13:56:47 UTC
Created attachment 65021 [details] [review]
Bug 18880: Fix authentication fallback for external authentications

A regression in commit cfc484b17 / bug #18314 breaks the local
authentication fallback for all external authentications like LDAP, CAS
and Shibboleth.

The regression itself is a logical error as "@return = (0)" is
considered to be "false" when checked with "unless" (line 1814). That's
wrong as "unless" tests the number of elements in a list. Thus the
"falsy" condition has to established with "@return = ()" instead.

This patch tries to simplify the logic by adding a $passwd_ok and
$check_internal_as_fallback flags to be more verbose and hopefully more
understandable.
The goal here is simply to restore back the same logic as before cfc484b17

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>
Comment 30 Marcel de Rooy 2017-07-14 06:31:43 UTC
Created attachment 65037 [details] [review]
Bug 18880: Fix authentication fallback for external authentications

A regression in commit cfc484b17 / bug #18314 breaks the local
authentication fallback for all external authentications like LDAP, CAS
and Shibboleth.

The regression itself is a logical error as "@return = (0)" is
considered to be "false" when checked with "unless" (line 1814). That's
wrong as "unless" tests the number of elements in a list. Thus the
"falsy" condition has to established with "@return = ()" instead.

This patch tries to simplify the logic by adding a $passwd_ok and
$check_internal_as_fallback flags to be more verbose and hopefully more
understandable.
The goal here is simply to restore back the same logic as before cfc484b17

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 31 Marcel de Rooy 2017-07-14 06:31:48 UTC
Created attachment 65038 [details] [review]
Bug 18880: [QA Follow-up] Finishing touch

Do not fill @return if retval == -1 for LDAP (see cfc484b17).
No need to call store after an DBIx update. Rearranged the if statement.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 32 Oliver Bock 2017-07-14 07:13:00 UTC
Hi guys,

Now that the implementation changed significantly you might want to adjust the commit message to reflect that. At least I'd remove the following part: "Thus the
"falsy" condition has to established with "@return = ()" instead."

Cheers
Comment 33 Jonathan Druart 2017-07-14 15:06:17 UTC
Created attachment 65053 [details] [review]
Bug 18880: Fix authentication fallback for external authentications

A regression in commit cfc484b17 / bug #18314 breaks the local
authentication fallback for all external authentications like LDAP, CAS
and Shibboleth.

The regression itself is a logical error as "@return = (0)" is
considered to be "false" when checked with "unless" (line 1814). That's
wrong as "unless" tests the number of elements in a list.

This patch tries to simplify the logic by adding a $passwd_ok and
$check_internal_as_fallback flags to be more verbose and hopefully more
understandable.
The goal here is simply to restore back the same logic as before cfc484b17

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 34 Jonathan Druart 2017-07-14 15:06:24 UTC
Created attachment 65054 [details] [review]
Bug 18880: [QA Follow-up] Finishing touch

Do not fill @return if retval == -1 for LDAP (see cfc484b17).
No need to call store after an DBIx update. Rearranged the if statement.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 35 Jonathan Druart 2017-07-14 15:33:25 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 36 Fridolin SOMERS 2017-07-18 13:46:36 UTC
Pushed to 17.05.x, will be in 17.05.02
Comment 37 Katrin Fischer 2017-07-19 21:20:16 UTC
Bug 18314 is not in 16.11.x, so this is not relevant for this version.
Comment 38 Jonathan Druart 2017-07-31 19:45:55 UTC
*** Bug 18953 has been marked as a duplicate of this bug. ***