Receive annotations from authentication and external ACL helpers

This saves the kv-pair from authentication and ACL helper responses as
annotations on the HttpRequest and pass on from there to logging.

Added a method appendNewOnly() to ensure duplicate-free addition
to a NotePairs list.

Also, fixes a bug in hasPair() accessor which was returning true
if either the key OR the value matched.
ie. hasPair("a","1") would match true for notes "a=2 b=1"

diff --git a/src/ExternalACLEntry.cc b/src/ExternalACLEntry.cc
index 9925320cdec68a0f274e3ffdc0397856bd2bdf64..a8a459cd079624fc6ae575c6a7ce612e28cf6998 100644 (file)
--- a/src/ExternalACLEntry.cc
+++ b/src/ExternalACLEntry.cc
@@ -49,7 +49,8 @@
 
 CBDATA_CLASS_INIT(ExternalACLEntry);
 
-ExternalACLEntry::ExternalACLEntry()
+ExternalACLEntry::ExternalACLEntry() :
+        notes()
 {
     lru.next = lru.prev = NULL;
     result = ACCESS_DENIED;
@@ -67,6 +68,11 @@ ExternalACLEntry::update(ExternalACLEntryData const &someData)
 {
     date = squid_curtime;
     result = someData.result;
+
+    // replace all notes. not combine
+    notes.entries.clean();
+    notes.append(&someData.notes);
+
 #if USE_AUTH
     user = someData.user;
     password = someData.password;

diff --git a/src/ExternalACLEntry.h b/src/ExternalACLEntry.h
index 47d713811076e71ed9cdeabb3f9f1f9716e5de0e..8181bdb56d181e8db08751f1240b4224500c2467 100644 (file)
--- a/src/ExternalACLEntry.h
+++ b/src/ExternalACLEntry.h
@@ -45,6 +45,7 @@
 #include "acl/Acl.h"
 #include "cbdata.h"
 #include "hash.h"
+#include "Notes.h"
 #include "SquidString.h"
 
 class external_acl;
@@ -62,6 +63,10 @@ public:
     ExternalACLEntryData() : result(ACCESS_DUNNO) {}
 
     allow_t result;
+
+    /// list of all kv-pairs returned by the helper
+    NotePairs notes;
+
 #if USE_AUTH
     // TODO use an AuthUser to hold this info
     String user;
@@ -88,6 +93,10 @@ public:
     dlink_node lru;
     allow_t result;
     time_t date;
+
+    /// list of all kv-pairs returned by the helper
+    NotePairs notes;
+
 #if USE_AUTH
     String user;
     String password;

diff --git a/src/Makefile.am b/src/Makefile.am
index 40a62cffc2fe08702238844c52d28c347fc540ef..c6f201c43e261fe82626e1fb385043b14a7fa30f 100644 (file)
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1261,6 +1261,8 @@ tests_testACLMaxUserIP_SOURCES= \
        int.cc \
        MasterXaction.cc \
        MasterXaction.h \
+       Notes.cc \
+       Notes.h \
        SquidList.h \
        SquidList.cc \
        mem_node.cc \

diff --git a/src/Notes.cc b/src/Notes.cc
index 1258da77d813379beffbf400b9a7381c89ff7388..a981ad2feeec3bc42a916abd5873f83afa097c72 100644 (file)
--- a/src/Notes.cc
+++ b/src/Notes.cc
@@ -206,7 +206,7 @@ bool
 NotePairs::hasPair(const char *key, const char *value) const
 {
     for (Vector<NotePairs::Entry *>::const_iterator  i = entries.begin(); i != entries.end(); ++i) {
-        if ((*i)->name.cmp(key) == 0 || (*i)->value.cmp(value) == 0)
+        if ((*i)->name.cmp(key) == 0 && (*i)->value.cmp(value) == 0)
             return true;
     }
     return false;
@@ -220,12 +220,25 @@ NotePairs::append(const NotePairs *src)
     }
 }
 
+void
+NotePairs::appendNewOnly(const NotePairs *src)
+{
+    for (Vector<NotePairs::Entry *>::const_iterator  i = src->entries.begin(); i != src->entries.end(); ++i) {
+        if (!hasPair((*i)->name.termedBuf(), (*i)->value.termedBuf()))
+            entries.push_back(new NotePairs::Entry((*i)->name.termedBuf(), (*i)->value.termedBuf()));
+    }
+}
+
 NotePairs &
 SyncNotes(AccessLogEntry &ale, HttpRequest &request)
 {
+    // XXX: auth code only has access to HttpRequest being authenticated
+    // so we must handle the case where HttpRequest is set without ALE being set.
+
     if (!ale.notes) {
-        assert(!request.notes);
-        ale.notes = request.notes = new NotePairs;
+        if (!request.notes)
+            request.notes = new NotePairs;
+        ale.notes = request.notes;
     } else {
         assert(ale.notes == request.notes);
     }

diff --git a/src/Notes.h b/src/Notes.h
index 0068bd233295ada980169fa359186f0fb99ffa5a..341b482716ef1c18df168a2e90e261cc9f2aba50 100644 (file)
--- a/src/Notes.h
+++ b/src/Notes.h
@@ -130,6 +130,12 @@ public:
      */
     void append(const NotePairs *src);
 
+    /**
+     * Append any new entries of the src NotePairs list to our list.
+     * Entries which already exist in the destination set are ignored.
+     */
+    void appendNewOnly(const NotePairs *src);
+
     /**
      * Returns a comma separated list of notes with key 'noteKey'.
      * Use findFirst instead when a unique kv-pair is needed.

diff --git a/src/auth/User.cc b/src/auth/User.cc
index 95fd1d49e206283c299901a51ac7f2ac5eb48a32..bc371b6c39790ff372abf50997ec7be2b47b4d15 100644 (file)
--- a/src/auth/User.cc
+++ b/src/auth/User.cc
@@ -58,6 +58,7 @@ Auth::User::User(Auth::Config *aConfig) :
         config(aConfig),
         ipcount(0),
         expiretime(0),
+        notes(),
         credentials_state(Auth::Unchecked),
         username_(NULL)
 {
@@ -99,6 +100,9 @@ Auth::User::absorb(Auth::User::Pointer from)
 
     debugs(29, 5, HERE << "auth_user '" << from << "' into auth_user '" << this << "'.");
 
+    // combine the helper response annotations. Ensuring no duplicates are copied.
+    notes.appendNewOnly(&from->notes);
+
     /* absorb the list of IP address sources (for max_user_ip controls) */
     AuthUserIP *new_ipdata;
     while (from->ip_list.head != NULL) {

diff --git a/src/auth/User.h b/src/auth/User.h
index 11d2e15078bc78eb0adad543bfc63a352c8de6ff..4e680c37c162d7ba8b403a2d0b6b7617dbe39ddc 100644 (file)
--- a/src/auth/User.h
+++ b/src/auth/User.h
@@ -39,6 +39,7 @@
 #include "base/RefCount.h"
 #include "dlink.h"
 #include "ip/Address.h"
+#include "Notes.h"
 
 class AuthUserHashPointer;
 class StoreEntry;
@@ -75,6 +76,9 @@ public:
     size_t ipcount;
     long expiretime;
 
+    /// list of key=value pairs the helper produced
+    NotePairs notes;
+
 public:
     static void cacheInit();
     static void CachedACLsReset();

diff --git a/src/auth/UserRequest.cc b/src/auth/UserRequest.cc
index 9d7761af8e381d6c2bccf14c0b8b54dfdd7beab0..b2a61995f4d65804fb28a871af5138fb094b2bfb 100644 (file)
--- a/src/auth/UserRequest.cc
+++ b/src/auth/UserRequest.cc
@@ -248,14 +248,27 @@ authenticateAuthenticateUser(Auth::UserRequest::Pointer auth_user_request, HttpR
 static Auth::UserRequest::Pointer
 authTryGetUser(Auth::UserRequest::Pointer auth_user_request, ConnStateData * conn, HttpRequest * request)
 {
+    Auth::UserRequest::Pointer res;
+
     if (auth_user_request != NULL)
-        return auth_user_request;
+        res = auth_user_request;
     else if (request != NULL && request->auth_user_request != NULL)
-        return request->auth_user_request;
+        res = request->auth_user_request;
     else if (conn != NULL)
-        return conn->getAuth();
-    else
-        return NULL;
+        res = conn->getAuth();
+
+    // attach the credential notes from helper to the transaction
+    if (request != NULL && res != NULL && res->user() != NULL) {
+        // XXX: we have no access to the transaction / AccessLogEntry so cant SyncNotes().
+        // workaround by using anything already set in HttpRequest
+        // OR use new and rely on a later Sync copying these to AccessLogEntry
+        if (!request->notes)
+            request->notes = new NotePairs;
+
+        request->notes->appendNewOnly(&res->user()->notes);
+    }
+
+    return res;
 }
 
 /* returns one of

diff --git a/src/auth/basic/UserRequest.cc b/src/auth/basic/UserRequest.cc
index c85830fc3d6d173195b0c4320b8201aa9d81349e..ef9dc6fa814d96b731e9a5e99250d32bd05a26d1 100644 (file)
--- a/src/auth/basic/UserRequest.cc
+++ b/src/auth/basic/UserRequest.cc
@@ -142,6 +142,10 @@ Auth::Basic::UserRequest::HandleReply(void *data, const HelperReply &reply)
     assert(r->auth_user_request != NULL);
     assert(r->auth_user_request->user()->auth_type == Auth::AUTH_BASIC);
 
+    // add new helper kv-pair notes to the credentials object
+    // so that any transaction using those credentials can access them
+    r->auth_user_request->user()->notes.appendNewOnly(&reply.notes);
+
     /* this is okay since we only play with the Auth::Basic::User child fields below
      * and dont pass the pointer itself anywhere */
     Auth::Basic::User *basic_auth = dynamic_cast<Auth::Basic::User *>(r->auth_user_request->user().getRaw());

diff --git a/src/auth/digest/UserRequest.cc b/src/auth/digest/UserRequest.cc
index 977169ba959de38d787c2551c455963935ce118d..161161f8065f72726f1d46dfddffe73e84b11a04 100644 (file)
--- a/src/auth/digest/UserRequest.cc
+++ b/src/auth/digest/UserRequest.cc
@@ -282,6 +282,10 @@ Auth::Digest::UserRequest::HandleReply(void *data, const HelperReply &reply)
     assert(replyData->auth_user_request != NULL);
     Auth::UserRequest::Pointer auth_user_request = replyData->auth_user_request;
 
+    // add new helper kv-pair notes to the credentials object
+    // so that any transaction using those credentials can access them
+    auth_user_request->user()->notes.appendNewOnly(&reply.notes);
+
     static bool oldHelperWarningDone = false;
     switch (reply.result) {
     case HelperReply::Unknown: {

diff --git a/src/auth/negotiate/UserRequest.cc b/src/auth/negotiate/UserRequest.cc
index 2115841adef954e600e64d467a14bf60e2ea7aba..086da9f7b9251ce777837443094f10333130f930 100644 (file)
--- a/src/auth/negotiate/UserRequest.cc
+++ b/src/auth/negotiate/UserRequest.cc
@@ -226,6 +226,10 @@ Auth::Negotiate::UserRequest::HandleReply(void *data, const HelperReply &reply)
     Auth::UserRequest::Pointer auth_user_request = r->auth_user_request;
     assert(auth_user_request != NULL);
 
+    // add new helper kv-pair notes to the credentials object
+    // so that any transaction using those credentials can access them
+    auth_user_request->user()->notes.appendNewOnly(&reply.notes);
+
     Auth::Negotiate::UserRequest *lm_request = dynamic_cast<Auth::Negotiate::UserRequest *>(auth_user_request.getRaw());
     assert(lm_request != NULL);
     assert(lm_request->waiting);

diff --git a/src/auth/ntlm/UserRequest.cc b/src/auth/ntlm/UserRequest.cc
index 01300609eca3a6aec21e37a40bc28efbfc91ffa7..4ce04ebf41f3d7a091ac32c6a1c3a9023e4a4986 100644 (file)
--- a/src/auth/ntlm/UserRequest.cc
+++ b/src/auth/ntlm/UserRequest.cc
@@ -220,6 +220,10 @@ Auth::Ntlm::UserRequest::HandleReply(void *data, const HelperReply &reply)
     Auth::UserRequest::Pointer auth_user_request = r->auth_user_request;
     assert(auth_user_request != NULL);
 
+    // add new helper kv-pair notes to the credentials object
+    // so that any transaction using those credentials can access them
+    auth_user_request->user()->notes.appendNewOnly(&reply.notes);
+
     Auth::Ntlm::UserRequest *lm_request = dynamic_cast<Auth::Ntlm::UserRequest *>(auth_user_request.getRaw());
     assert(lm_request != NULL);
     assert(lm_request->waiting);

diff --git a/src/client_side.cc b/src/client_side.cc
index 4afeea33f7dc16ceca672835a9754ac0ce352d7f..c1d13e6a381dd7e0e97dfe13feace6efab782d0e 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -670,8 +670,7 @@ ClientHttpRequest::logRequest()
 
     /*Add notes*/
     // The al->notes and request->notes must point to the same object.
-    // Enable the following assertion to check for possible bugs.
-    // assert(request->notes == al->notes);
+    (void)SyncNotes(*al, *request);
     typedef Notes::iterator ACAMLI;
     for (ACAMLI i = Config.notes.begin(); i != Config.notes.end(); ++i) {
         if (const char *value = (*i)->match(request, al->reply)) {

diff --git a/src/external_acl.cc b/src/external_acl.cc
index f4e04766f85e63cc8bf73e596a03189d51894fb4..88f40809d54bb77c1757903a330e3f67dc0c7ee2 100644 (file)
--- a/src/external_acl.cc
+++ b/src/external_acl.cc
@@ -1376,6 +1376,8 @@ externalAclHandleReply(void *data, const HelperReply &reply)
 
     // XXX: make entryData store a proper HelperReply object instead of copying.
 
+    entryData.notes.append(&reply.notes);
+
     const char *label = reply.notes.findFirst("tag");
     if (label != NULL && *label != '\0')
         entryData.tag = label;
@@ -1599,6 +1601,18 @@ ExternalACLLookup::LookupDone(void *data, void *result)
 {
     ACLFilledChecklist *checklist = Filled(static_cast<ACLChecklist*>(data));
     checklist->extacl_entry = cbdataReference((external_acl_entry *)result);
+
+    // attach the helper kv-pair to the transaction
+    if (HttpRequest * req = checklist->request) {
+        // XXX: we have no access to the transaction / AccessLogEntry so cant SyncNotes().
+        // workaround by using anything already set in HttpRequest
+        // OR use new and rely on a later Sync copying these to AccessLogEntry
+        if (!req->notes)
+            req->notes = new NotePairs;
+
+        req->notes->appendNewOnly(&checklist->extacl_entry->notes);
+    }
+
     checklist->resumeNonBlockingCheck(ExternalACLLookup::Instance());
 }