Bug 4912: same-name notes being appended instead of replaced (#393)

When auth helper replies with clt_conn_tag=foo (or any KV pair) and
then later in future replaces it with clt_conn_tag=bar. The new pair
was getting appended instead of getting replaced.

i.e. notes would contain two KV pairs:
clt_conn_tag=foo
clt_conn_tag=bar

This fixes it and also fixes:
https://bugs.squid-cache.org/show_bug.cgi?id=4912

diff --git a/src/Notes.cc b/src/Notes.cc
index d7cd2740554aeba7dda818070fb0c7b2f6842fda..1e0232c17f0f951819e9b044a1de9926bcadb01f 100644 (file)
--- a/src/Notes.cc
+++ b/src/Notes.cc
@@ -354,6 +354,16 @@ NotePairs::appendNewOnly(const NotePairs *src)
     }
 }
 
+void
+NotePairs::replaceOrAddOrAppend(const NotePairs *src, const NotePairs::Names &appendables)
+{
+    for (const auto e: src->entries) {
+        if (std::find(appendables.begin(), appendables.end(), e->name()) == appendables.end())
+            remove(e->name());
+    }
+    append(src);
+}
+
 void
 NotePairs::replaceOrAdd(const NotePairs *src)
 {

diff --git a/src/Notes.h b/src/Notes.h
index 66d38cfd364154ea31056a2e238e45ed5d7dba7a..f80031a4aec165be13cb89e5433f49ee89c5e4a5 100644 (file)
--- a/src/Notes.h
+++ b/src/Notes.h
@@ -191,6 +191,7 @@ public:
         SBuf theValue;
     };
     typedef std::vector<Entry::Pointer> Entries;      ///< The key/value pair entries
+    typedef std::vector<SBuf> Names;
 
     NotePairs() {}
     NotePairs &operator=(NotePairs const &) = delete;
@@ -199,6 +200,11 @@ public:
     /// Append the entries of the src NotePairs list to our list.
     void append(const NotePairs *src);
 
+    /// Replace existing list entries with the src NotePairs entries.
+    /// Do not replace but append entries named in the appendables
+    /// Entries which do not exist in the destination set are added.
+    void replaceOrAddOrAppend(const NotePairs *src, const Names &appendables);
+
     /// Replace existing list entries with the src NotePairs entries.
     /// Entries which do not exist in the destination set are added.
     void replaceOrAdd(const NotePairs *src);

diff --git a/src/auth/basic/UserRequest.cc b/src/auth/basic/UserRequest.cc
index 71bb717ab7122de7f06bc04dda9c046d3c72186e..2b3fa53d78e215ed5b671f576581c5c07131c5b6 100644 (file)
--- a/src/auth/basic/UserRequest.cc
+++ b/src/auth/basic/UserRequest.cc
@@ -168,7 +168,8 @@ Auth::Basic::UserRequest::HandleReply(void *data, const Helper::Reply &reply)
 
     // 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);
+    static const NotePairs::Names appendables = { SBuf("group"), SBuf("tag") };
+    r->auth_user_request->user()->notes.replaceOrAddOrAppend(&reply.notes, appendables);
 
     /* this is okay since we only play with the Auth::Basic::User child fields below
      * and do not pass the pointer itself anywhere */

diff --git a/src/auth/digest/UserRequest.cc b/src/auth/digest/UserRequest.cc
index 7aac1970486c7eec863e39b46e33c2cb394af917..3d3151dcc40a45834c4bb2eb8decfe9c6dc6048f 100644 (file)
--- a/src/auth/digest/UserRequest.cc
+++ b/src/auth/digest/UserRequest.cc
@@ -327,7 +327,8 @@ Auth::Digest::UserRequest::HandleReply(void *data, const Helper::Reply &reply)
 
     // 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 const NotePairs::Names appendables = { SBuf("group"), SBuf("nonce"), SBuf("tag") };
+    auth_user_request->user()->notes.replaceOrAddOrAppend(&reply.notes, appendables);
     // remove any private credentials detail which got added.
     auth_user_request->user()->notes.remove("ha1");
 

diff --git a/src/auth/negotiate/UserRequest.cc b/src/auth/negotiate/UserRequest.cc
index 168d8cacd011f0e5f8d0dc4b7706631a4360fa6b..568fd33e5a0d6464b7b7a7adb20cb1c6465b7c09 100644 (file)
--- a/src/auth/negotiate/UserRequest.cc
+++ b/src/auth/negotiate/UserRequest.cc
@@ -278,7 +278,8 @@ Auth::Negotiate::UserRequest::HandleReply(void *data, const Helper::Reply &reply
 
     // 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 const NotePairs::Names appendables = { SBuf("group"), SBuf("tag") };
+    auth_user_request->user()->notes.replaceOrAddOrAppend(&reply.notes, appendables);
     // remove any private credentials detail which got added.
     auth_user_request->user()->notes.remove("token");
 

diff --git a/src/auth/ntlm/UserRequest.cc b/src/auth/ntlm/UserRequest.cc
index 2edbeb59b760f0caecc64efb6442111684db52ae..0115d70f0adbb5504be3999b5c81c4eba47bbf8d 100644 (file)
--- a/src/auth/ntlm/UserRequest.cc
+++ b/src/auth/ntlm/UserRequest.cc
@@ -272,7 +272,8 @@ Auth::Ntlm::UserRequest::HandleReply(void *data, const Helper::Reply &reply)
 
     // 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 const NotePairs::Names appendables = { SBuf("group"), SBuf("tag") };
+    auth_user_request->user()->notes.replaceOrAddOrAppend(&reply.notes, appendables);
     // remove any private credentials detail which got added.
     auth_user_request->user()->notes.remove("token");