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.

diff --git a/src/acl/Checklist.h b/src/acl/Checklist.h
index 51a2d4ae9a7e4cce16f3b0ee0ded03ed7f3ef26c..c368f04caac9fdd90f8ff5a54a43a5c7d92c508c 100644 (file)
--- 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 6e625f6acfca918fdd10018d17ffc3f4c0353461..7d19ed6629c11e1b78efc8468e5a4e228098afde 100644 (file)
--- 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 487ee7da5ede914555524dd13f57579e47598ad6..968bf2bd89291c8fe9773c9c7847e7d092f271b8 100644 (file)
--- 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);