Fix Auth::UserRequest::denyMessage() misuse.

This method was improperly used in contexts where actually
Auth::UserRequest::setDenyMessage() expected. Probably the reason was
that both denyMessage() and getDenyMessage() were not constant,
provoking such 'misuse'.

Also placed some common code into UserRequest::denyMessageFromHelper(),
eliminating code duplication. Though there are still many places
where code is duplicated inside auth/ntlm/UserRequest.cc and
auth/negotiate/UserRequest.cc.

diff --git a/src/auth/UserRequest.cc b/src/auth/UserRequest.cc
index 1def969d340dfba337e39c855914f5388e08ea7d..52d75e4ec0e6c271c3ee43d2435017909b3b86fd 100644 (file)
--- a/src/auth/UserRequest.cc
+++ b/src/auth/UserRequest.cc
@@ -19,6 +19,8 @@
 #include "comm/Connection.h"
 #include "fatal.h"
 #include "format/Format.h"
+#include "helper.h"
+#include "helper/Reply.h"
 #include "http/Stream.h"
 #include "HttpReply.h"
 #include "HttpRequest.h"
@@ -116,13 +118,13 @@ Auth::UserRequest::setDenyMessage(char const *aString)
 }
 
 char const *
-Auth::UserRequest::getDenyMessage()
+Auth::UserRequest::getDenyMessage() const
 {
     return message;
 }
 
 char const *
-Auth::UserRequest::denyMessage(char const * const default_message)
+Auth::UserRequest::denyMessage(char const * const default_message) const
 {
     if (getDenyMessage() == NULL)
         return default_message;
@@ -574,3 +576,14 @@ Auth::UserRequest::helperRequestKeyExtras(HttpRequest *request, AccessLogEntry::
     return NULL;
 }
 
+void
+Auth::UserRequest::denyMessageFromHelper(const char *proto, const Helper::Reply &reply)
+{
+    static SBuf messageNote;
+    if (!reply.notes.find(messageNote, "message")) {
+        messageNote.append(proto);
+        messageNote.append(" Authentication denied with no reason given");
+    }
+    setDenyMessage(messageNote.c_str());
+}
+

diff --git a/src/auth/UserRequest.h b/src/auth/UserRequest.h
index 0711ee80aa91f92388d088fdb5a003ec314db746..eafda066ed88b7bd08b0271ade303a8bb44a6f3c 100644 (file)
--- a/src/auth/UserRequest.h
+++ b/src/auth/UserRequest.h
@@ -179,13 +179,13 @@ public:
      */
     void start(HttpRequest *request, AccessLogEntry::Pointer &al, AUTHCB *handler, void *data);
 
-    char const * denyMessage(char const * const default_message = NULL);
+    char const * denyMessage(char const * const default_message = NULL) const;
 
     /** Possibly overrideable in future */
     void setDenyMessage(char const *);
 
     /** Possibly overrideable in future */
-    char const * getDenyMessage();
+    char const * getDenyMessage() const;
 
     /**
      * Squid does not make assumptions about where the username is stored.
@@ -208,6 +208,9 @@ public:
 
     const char *helperRequestKeyExtras(HttpRequest *, AccessLogEntry::Pointer &al);
 
+    /// Sets the reason of 'authentication denied' helper response.
+    void denyMessageFromHelper(char const *proto, const Helper::Reply &reply);
+
 protected:
     /**
      * The scheme-specific actions to be performed when sending helper lookup.

diff --git a/src/auth/negotiate/UserRequest.cc b/src/auth/negotiate/UserRequest.cc
index 0f675080eefba8756ac8b9b8425ea5d95b86e037..c7c3ab7363deb56d4946c45939c9166a2ccf6d2e 100644 (file)
--- a/src/auth/negotiate/UserRequest.cc
+++ b/src/auth/negotiate/UserRequest.cc
@@ -307,11 +307,11 @@ Auth::Negotiate::UserRequest::HandleReply(void *data, const Helper::Reply &reply
             const char *tokenNote = reply.notes.findFirst("token");
             lm_request->server_blob = xstrdup(tokenNote);
             auth_user_request->user()->credentials(Auth::Handshake);
-            auth_user_request->denyMessage("Authentication in progress");
+            auth_user_request->setDenyMessage("Authentication in progress");
             debugs(29, 4, HERE << "Need to challenge the client with a server token: '" << tokenNote << "'");
         } else {
             auth_user_request->user()->credentials(Auth::Failed);
-            auth_user_request->denyMessage("Negotiate authentication requires a persistent connection");
+            auth_user_request->setDenyMessage("Negotiate authentication requires a persistent connection");
         }
         break;
 
@@ -327,7 +327,7 @@ Auth::Negotiate::UserRequest::HandleReply(void *data, const Helper::Reply &reply
 
         /* we're finished, release the helper */
         auth_user_request->user()->username(userNote);
-        auth_user_request->denyMessage("Login successful");
+        auth_user_request->setDenyMessage("Login successful");
         safe_free(lm_request->server_blob);
         lm_request->server_blob = xstrdup(tokenNote);
         lm_request->releaseAuthServer();
@@ -357,17 +357,11 @@ Auth::Negotiate::UserRequest::HandleReply(void *data, const Helper::Reply &reply
     break;
 
     case Helper::Error: {
-        const char *tokenNote = reply.notes.findFirst("token");
-
-        SBuf messageNote;
         /* authentication failure (wrong password, etc.) */
-        if (reply.notes.find(messageNote, "message"))
-            auth_user_request->denyMessage(messageNote.c_str());
-        else
-            auth_user_request->denyMessage("Negotiate Authentication denied with no reason given");
+        auth_user_request->denyMessageFromHelper("Negotiate", reply);
         auth_user_request->user()->credentials(Auth::Failed);
         safe_free(lm_request->server_blob);
-        if (tokenNote != NULL)
+        if (const char *tokenNote = reply.notes.findFirst("token"))
             lm_request->server_blob = xstrdup(tokenNote);
         lm_request->releaseAuthServer();
         debugs(29, 4, "Failed validating user via Negotiate. Result: " << reply);
@@ -387,11 +381,9 @@ Auth::Negotiate::UserRequest::HandleReply(void *data, const Helper::Reply &reply
          * Needing YR. */
         SBuf errNote;
         if (reply.result == Helper::Unknown)
-            auth_user_request->denyMessage("Internal Error");
-        else if (reply.notes.find(errNote, "message"))
-            auth_user_request->denyMessage(errNote.c_str());
+            auth_user_request->setDenyMessage("Internal Error");
         else
-            auth_user_request->denyMessage("Negotiate Authentication failed with no reason given");
+            auth_user_request->denyMessageFromHelper("Negotiate", reply);
         auth_user_request->user()->credentials(Auth::Failed);
         safe_free(lm_request->server_blob);
         lm_request->releaseAuthServer();

diff --git a/src/auth/ntlm/UserRequest.cc b/src/auth/ntlm/UserRequest.cc
index 4670a11908c741a4778a3d35ce3423dd1938d7df..891d11078703ff4a57624d195f7ea4e723e82e00 100644 (file)
--- a/src/auth/ntlm/UserRequest.cc
+++ b/src/auth/ntlm/UserRequest.cc
@@ -302,11 +302,11 @@ Auth::Ntlm::UserRequest::HandleReply(void *data, const Helper::Reply &reply)
             const char *serverBlob = reply.notes.findFirst("token");
             lm_request->server_blob = xstrdup(serverBlob);
             auth_user_request->user()->credentials(Auth::Handshake);
-            auth_user_request->denyMessage("Authentication in progress");
+            auth_user_request->setDenyMessage("Authentication in progress");
             debugs(29, 4, HERE << "Need to challenge the client with a server token: '" << serverBlob << "'");
         } else {
             auth_user_request->user()->credentials(Auth::Failed);
-            auth_user_request->denyMessage("NTLM authentication requires a persistent connection");
+            auth_user_request->setDenyMessage("NTLM authentication requires a persistent connection");
         }
         break;
 
@@ -321,7 +321,7 @@ Auth::Ntlm::UserRequest::HandleReply(void *data, const Helper::Reply &reply)
             break;
         }
         auth_user_request->user()->username(userLabel);
-        auth_user_request->denyMessage("Login successful");
+        auth_user_request->setDenyMessage("Login successful");
         safe_free(lm_request->server_blob);
         lm_request->releaseAuthServer();
 
@@ -353,11 +353,7 @@ Auth::Ntlm::UserRequest::HandleReply(void *data, const Helper::Reply &reply)
 
     case Helper::Error: {
         /* authentication failure (wrong password, etc.) */
-        SBuf errNote;
-        if (reply.notes.find(errNote, "message"))
-            auth_user_request->denyMessage(errNote.c_str());
-        else
-            auth_user_request->denyMessage("NTLM Authentication denied with no reason given");
+        auth_user_request->denyMessageFromHelper("NTLM", reply);
         auth_user_request->user()->credentials(Auth::Failed);
         safe_free(lm_request->server_blob);
         lm_request->releaseAuthServer();
@@ -376,13 +372,10 @@ Auth::Ntlm::UserRequest::HandleReply(void *data, const Helper::Reply &reply)
          * Authenticate NTLM start.
          * If after a KK deny the user's request w/ 407 and mark the helper as
          * Needing YR. */
-        SBuf errNote;
         if (reply.result == Helper::Unknown)
-            auth_user_request->denyMessage("Internal Error");
-        else if (reply.notes.find(errNote, "message"))
-            auth_user_request->denyMessage(errNote.c_str());
+            auth_user_request->setDenyMessage("Internal Error");
         else
-            auth_user_request->denyMessage("NTLM Authentication failed with no reason given");
+            auth_user_request->denyMessageFromHelper("NTLM", reply);
         auth_user_request->user()->credentials(Auth::Failed);
         safe_free(lm_request->server_blob);
         lm_request->releaseAuthServer();

diff --git a/src/tests/stub_libauth.cc b/src/tests/stub_libauth.cc
index b1b07e7fa08073dc50f341ebacde98ffe3a1d11c..0ee6c1c236ad3fbc282eb7c820fb84eeaba2fdb3 100644 (file)
--- a/src/tests/stub_libauth.cc
+++ b/src/tests/stub_libauth.cc
@@ -59,8 +59,8 @@ void Auth::UserRequest::operator delete (void *) STUB
 Auth::UserRequest::UserRequest() STUB
 Auth::UserRequest::~UserRequest() STUB
 void Auth::UserRequest::setDenyMessage(char const *) STUB
-char const * Auth::UserRequest::getDenyMessage() STUB_RETVAL("stub")
-char const * Auth::UserRequest::denyMessage(char const * const) STUB_RETVAL("stub")
+char const * Auth::UserRequest::getDenyMessage() const STUB_RETVAL("stub")
+char const * Auth::UserRequest::denyMessage(char const * const) const STUB_RETVAL("stub")
 void authenticateAuthUserRequestRemoveIp(Auth::UserRequest::Pointer, Ip::Address const &) STUB
 void authenticateAuthUserRequestClearIp(Auth::UserRequest::Pointer) STUB
 int authenticateAuthUserRequestIPCount(Auth::UserRequest::Pointer) STUB_RETVAL(0)