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
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.
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
(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?
Right below the hunks I patched: https://github.com/Koha-Community/Koha/commit/cfc484b173120dfe14616424c1ec279bb74cf2a9#diff-75b9144a15674b1f9c5c6acbf32e8ed9R1814 HTH
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.
> 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
Indeed, you are right!
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.
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.
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.
Back to Jonathan, ready for sign-off. Thanks
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>
For posterity, my sign off is based on testing the patch an LDAP configuration.
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.
Looking here now
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..
> 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
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.
(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.
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
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
Oliver, Marcel, could you take a look at this alternative patch please?
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
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.
(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.
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.
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
(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;
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>
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>
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>
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
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>
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>
Pushed to master for 17.11, thanks to everybody involved!
Pushed to 17.05.x, will be in 17.05.02
Bug 18314 is not in 16.11.x, so this is not relevant for this version.
*** Bug 18953 has been marked as a duplicate of this bug. ***