From d5cbce971f0761a3fc59074b5b1683c595b5caa9 Mon Sep 17 00:00:00 2001
From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Thu, 28 Jun 2012 12:26:44 -0600
Subject: [PATCH] Removed FilledChecklist::checkCallback() as harmful and not
 needed.

The method was resetting the authentication state (auth_user_request) of a
connection just before notifying the caller of the async ACL check result. The
reset restarts authentication sequence, creating a 407 "loop" for auth schemes
that require more than one step (e.g., NTLM).

The method is no longer needed because we do not need to explicitly "unlock"
auth_user_request anymore. It is refcounted now.

There are other cases where connection authentication state () is reset from
within the ACL code. Some of those cases are not needed and some might even
cause similar bugs, but I did not risk misclassifying them in this commit.
---
 src/acl/Checklist.h        |  5 +++--
 src/acl/FilledChecklist.cc | 25 -------------------------
 src/acl/FilledChecklist.h  |  3 ---
 3 files changed, 3 insertions(+), 30 deletions(-)

diff --git a/src/acl/Checklist.h b/src/acl/Checklist.h
index 51a2d4ae9a..c368f04caa 100644
--- a/src/acl/Checklist.h
+++ b/src/acl/Checklist.h
@@ -181,9 +181,10 @@ public:
     virtual bool hasRequest() const = 0;
     virtual bool hasReply() const = 0;
 
-protected:
-    virtual void checkCallback(allow_t answer);
 private:
+    /// Calls non-blocking check callback with the answer and destroys self.
+    void checkCallback(allow_t answer);
+
     void checkAccessList();
     void checkForAsync();
 
diff --git a/src/acl/FilledChecklist.cc b/src/acl/FilledChecklist.cc
index 6e625f6acf..7d19ed6629 100644
--- a/src/acl/FilledChecklist.cc
+++ b/src/acl/FilledChecklist.cc
@@ -12,31 +12,6 @@
 
 CBDATA_CLASS_INIT(ACLFilledChecklist);
 
-void
-ACLFilledChecklist::checkCallback(allow_t answer)
-{
-    debugs(28, 5, HERE << this << " answer=" << answer);
-
-#if USE_AUTH
-    /* During reconfigure, we can end up not finishing call
-     * sequences into the auth code */
-
-    if (auth_user_request != NULL) {
-        /* the filled_checklist lock */
-        auth_user_request = NULL;
-        // It might have been connection based
-        // In the case of sslBump we need to preserve authentication info
-        // XXX: need to re-evaluate this. ACL tests should not be playing with
-        // XXX: wider scoped TCP connection state, even if the helper lookup is stuck.
-        if (conn() && !conn()->switchedToHttps()) {
-            conn()->auth_user_request = NULL;
-        }
-    }
-#endif
-
-    ACLChecklist::checkCallback(answer); // may delete us
-}
-
 
 void *
 ACLFilledChecklist::operator new (size_t size)
diff --git a/src/acl/FilledChecklist.h b/src/acl/FilledChecklist.h
index 487ee7da5e..968bf2bd89 100644
--- a/src/acl/FilledChecklist.h
+++ b/src/acl/FilledChecklist.h
@@ -72,9 +72,6 @@ public:
 
     ExternalACLEntry *extacl_entry;
 
-private:
-    virtual void checkCallback(allow_t answer);
-
 private:
     CBDATA_CLASS(ACLFilledChecklist);
 
-- 
2.47.2