[Bug 18880] New: Regression breaks local authentication fallback for all external authentications

classic Classic list List threaded Threaded
48 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] New: Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
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: [hidden email]
          Reporter: [hidden email]
        QA Contact: [hidden email]
                CC: [hidden email]

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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880

Oliver Bock <[hidden email]> 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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880

Zeno Tajoli <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[hidden email]
             Status|NEW                         |Needs Signoff

--
You are receiving this mail because:
You are the assignee for the bug.
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880

Katrin Fischer <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|critical                    |blocker
                 CC|                            |[hidden email]

--- Comment #1 from Katrin Fischer <[hidden email]> ---
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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880

--- Comment #2 from Oliver Bock <[hidden email]> ---
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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880

Jonathan Druart <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[hidden email]-c
                   |                            |ommunity.org

--- Comment #3 from Jonathan Druart <[hidden email]> ---
(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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880

--- Comment #4 from Oliver Bock <[hidden email]> ---
Right below the hunks I patched:
https://github.com/Koha-Community/Koha/commit/cfc484b173120dfe14616424c1ec279bb74cf2a9#diff-75b9144a15674b1f9c5c6acbf32e8ed9R1814

HTH

--
You are receiving this mail because:
You are watching all bug changes.
You are the assignee for the bug.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880

Jonathan Druart <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|Needs Signoff               |Failed QA

--- Comment #5 from Jonathan Druart <[hidden email]> ---
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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880

Jonathan Druart <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Version|17.05                       |master
           Assignee|[hidden email]-commun |[hidden email]
                   |ity.org                     |

--
You are receiving this mail because:
You are the assignee for the bug.
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880

--- Comment #6 from Oliver Bock <[hidden email]> ---
> 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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880

Oliver Bock <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|[hidden email]      |[hidden email]-c
                   |                            |ommunity.org

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880

Jonathan Druart <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|Failed QA                   |Needs Signoff
           Assignee|[hidden email]-c |[hidden email]
                   |ommunity.org                |

--- Comment #7 from Jonathan Druart <[hidden email]> ---
Indeed, you are right!

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880

--- Comment #8 from Katrin Fischer <[hidden email]> ---
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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880

M. Tompsett <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #64719|0                           |1
        is obsolete|                            |

--- Comment #9 from M. Tompsett <[hidden email]> ---
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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880

Oliver Bock <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #64862|0                           |1
        is obsolete|                            |

--- Comment #10 from Oliver Bock <[hidden email]> ---
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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880

Oliver Bock <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|[hidden email]      |[hidden email]-c
                   |                            |ommunity.org

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880

--- Comment #11 from Oliver Bock <[hidden email]> ---
Back to Jonathan, ready for sign-off.

Thanks

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880

Lee Jamison <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|Needs Signoff               |Signed Off

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880

Lee Jamison <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #64877|0                           |1
        is obsolete|                            |

--- Comment #12 from Lee Jamison <[hidden email]> ---
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 <[hidden email]>

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880

Lee Jamison <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[hidden email]

--- Comment #13 from Lee Jamison <[hidden email]> ---
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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880

--- Comment #14 from Lee Jamison <[hidden email]> ---
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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880

Marcel de Rooy <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[hidden email]

--- Comment #15 from Marcel de Rooy <[hidden email]> ---
Looking here now

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880

Marcel de Rooy <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|Signed Off                  |BLOCKED

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880

Marcel de Rooy <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|BLOCKED                     |Failed QA

--- Comment #16 from Marcel de Rooy <[hidden email]> ---
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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880

Marcel de Rooy <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         QA Contact|[hidden email]-communit |[hidden email]
                   |y.org                       |

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880

--- Comment #17 from Oliver Bock <[hidden email]> ---
> 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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880

--- Comment #18 from Marcel de Rooy <[hidden email]> ---
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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880

--- Comment #19 from Marcel de Rooy <[hidden email]> ---
(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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880

--- Comment #20 from Oliver Bock <[hidden email]> ---
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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 18880] Regression breaks local authentication fallback for all external authentications

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18880

--- Comment #21 from Jonathan Druart <[hidden email]> ---
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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
12
Loading...