[Bug 18880] New: Regression breaks local authentication fallback for all external authentications
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Bug ID: 18880 Summary: Regression breaks local authentication fallback for all external authentications Change sponsored?: --- Product: Koha Version: 17.05 Hardware: All OS: All Status: NEW Severity: critical Priority: P5 - low Component: Authentication Assignee: koha-bugs@lists.koha-community.org Reporter: oliver.bock@aei.mpg.de QA Contact: testopia@bugs.koha-community.org CC: dpavlin@rot13.org Created attachment 64719 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64719&action=edit 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Oliver Bock <oliver.bock@aei.mpg.de> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=18314 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Zeno Tajoli <z.tajoli@cineca.it> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |z.tajoli@cineca.it Status|NEW |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|critical |blocker CC| |katrin.fischer@bsz-bw.de --- Comment #1 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 --- Comment #2 from Oliver Bock <oliver.bock@aei.mpg.de> --- 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #3 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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? -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 --- Comment #4 from Oliver Bock <oliver.bock@aei.mpg.de> --- Right below the hunks I patched: https://github.com/Koha-Community/Koha/commit/cfc484b173120dfe14616424c1ec27... HTH -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #5 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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 are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|17.05 |master Assignee|koha-bugs@lists.koha-commun |oliver.bock@aei.mpg.de |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 --- Comment #6 from Oliver Bock <oliver.bock@aei.mpg.de> ---
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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Oliver Bock <oliver.bock@aei.mpg.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|oliver.bock@aei.mpg.de |jonathan.druart@bugs.koha-c | |ommunity.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff Assignee|jonathan.druart@bugs.koha-c |oliver.bock@aei.mpg.de |ommunity.org | --- Comment #7 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Indeed, you are right! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 --- Comment #8 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #64719|0 |1 is obsolete| | --- Comment #9 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 64862 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64862&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Oliver Bock <oliver.bock@aei.mpg.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #64862|0 |1 is obsolete| | --- Comment #10 from Oliver Bock <oliver.bock@aei.mpg.de> --- Created attachment 64877 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64877&action=edit Bug 18880: Regression breaks local authentication fallback for all external authentications Thanks M, but I slightly trimmed the commit message. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Oliver Bock <oliver.bock@aei.mpg.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|oliver.bock@aei.mpg.de |jonathan.druart@bugs.koha-c | |ommunity.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 --- Comment #11 from Oliver Bock <oliver.bock@aei.mpg.de> --- Back to Jonathan, ready for sign-off. Thanks -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Lee Jamison <ldjamison@marywood.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Lee Jamison <ldjamison@marywood.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #64877|0 |1 is obsolete| | --- Comment #12 from Lee Jamison <ldjamison@marywood.edu> --- Created attachment 64909 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64909&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Lee Jamison <ldjamison@marywood.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |ldjamison@marywood.edu --- Comment #13 from Lee Jamison <ldjamison@marywood.edu> --- For posterity, my sign off is based on testing the patch an LDAP configuration. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 --- Comment #14 from Lee Jamison <ldjamison@marywood.edu> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl --- Comment #15 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Looking here now -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |BLOCKED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Failed QA --- Comment #16 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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.. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 --- Comment #17 from Oliver Bock <oliver.bock@aei.mpg.de> ---
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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 --- Comment #18 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 --- Comment #19 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 --- Comment #20 from Oliver Bock <oliver.bock@aei.mpg.de> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 --- Comment #21 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 65000 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65000&action=edit [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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 --- Comment #22 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Oliver, Marcel, could you take a look at this alternative patch please? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #65000|0 |1 is obsolete| | --- Comment #23 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 65001 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65001&action=edit [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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 --- Comment #24 from Oliver Bock <oliver.bock@aei.mpg.de> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 --- Comment #25 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com, tomascohen@gmail.com --- Comment #26 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #65001|0 |1 is obsolete| | --- Comment #27 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 65011 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65011&action=edit [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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #28 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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; -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Lee Jamison <ldjamison@marywood.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Lee Jamison <ldjamison@marywood.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #65011|0 |1 is obsolete| | --- Comment #29 from Lee Jamison <ldjamison@marywood.edu> --- Created attachment 65021 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65021&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #64909|0 |1 is obsolete| | Attachment #65021|0 |1 is obsolete| | --- Comment #30 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 65037 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65037&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 --- Comment #31 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 65038 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65038&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 --- Comment #32 from Oliver Bock <oliver.bock@aei.mpg.de> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #65037|0 |1 is obsolete| | Attachment #65038|0 |1 is obsolete| | --- Comment #33 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 65053 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65053&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 --- Comment #34 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 65054 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65054&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #35 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 17.11, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m Status|Pushed to Master |Pushed to Stable --- Comment #36 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- Pushed to 17.05.x, will be in 17.05.02 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|Pushed to Stable |RESOLVED --- Comment #37 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Bug 18314 is not in 16.11.x, so this is not relevant for this version. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=18953 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |philippe.blouin@inlibro.com --- Comment #38 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- *** Bug 18953 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also|https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=18314 | Depends on| |18314 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18314 [Bug 18314] Account lockout -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=20879 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org