Receive annotations from authentication helpers

This saves the kv-pair from authentication helper responses as
annotations on the HttpRequest which was authenticated 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/Notes.cc b/src/Notes.cc
index b482f4eb2050a4b2e8af8bd15aaab8c9eb5b222b..9100e6cbfab5f98850c28e8f078c8387522e02f2 100644 (file)
--- a/src/Notes.cc
+++ b/src/Notes.cc
@@ -205,7 +205,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;
@@ -219,12 +219,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 29bac331ee3f969a1cd9a34d36c6b77a185a1d79..8583da4e93f7c96b7a3d986f3026ab9ac1a265db 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 83526f966eebbefe6739b91ea7b022dd7bf3decc..782196c7b7898193eec654972f8798ed729c6a41 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 3655db1516668729d4c70db08465d86f3b3949a6..d6b6bf1cab2bce6323d2208a6c59a54ae22354de 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 (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 e9fc63012b7b6d2084ebfa65675503f40ae06f27..af26ac532fd1a012bf154f80de2c53bf86723831 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 24c8147e37c86e5564dc12c32a526bb1b4590bc8..7277aca978cb716eafb7fd8362ef0433d4b5843f 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)) {