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 8df7e9fdae929da3e614f3c3e6fd72a4dec92c5d..f8d273db20b9a7be26bee31987597543225d2ab7 100644 (file)
--- a/src/auth/UserRequest.cc
+++ b/src/auth/UserRequest.cc
@@ -21,6 +21,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"
@@ -118,13 +120,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;
@@ -564,3 +566,15 @@ Auth::UserRequest::helperRequestKeyExtras(HttpRequest *request, AccessLogEntry::
     return NULL;
 }
 
+void
+Auth::UserRequest::denyMessageFromHelper(const char *proto, const Helper::Reply &reply)
+{
+    static SBuf messageNote;
+    messageNote = SBuf(reply.notes.find("message"));
+    if (messageNote.isEmpty()) {
+        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 4256d33ad61cf7636721817b95c6668fba85f5d4..026baa02dc6484008c83ec8dd4442746dd3a6de4 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();
@@ -356,47 +356,37 @@ Auth::Negotiate::UserRequest::HandleReply(void *data, const Helper::Reply &reply
     }
     break;
 
-    case Helper::Error: {
-        const char *messageNote = reply.notes.find("message");
-        const char *tokenNote = reply.notes.findFirst("token");
-
+    case Helper::Error:
         /* authentication failure (wrong password, etc.) */
-        if (messageNote != NULL)
-            auth_user_request->denyMessage(messageNote);
-        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);
-    }
-    break;
+        break;
 
     case Helper::Unknown:
         debugs(29, DBG_IMPORTANT, "ERROR: Negotiate Authentication Helper '" << reply.whichServer << "' crashed!.");
     /* continue to the next case */
 
     case Helper::TimedOut:
-    case Helper::BrokenHelper: {
+    case Helper::BrokenHelper:
         /* TODO kick off a refresh process. This can occur after a YR or after
          * a KK. If after a YR release the helper and resubmit the request via
          * Authenticate Negotiate start.
          * If after a KK deny the user's request w/ 407 and mark the helper as
          * Needing YR. */
-        const char *errNote = reply.notes.find("message");
         if (reply.result == Helper::Unknown)
-            auth_user_request->denyMessage("Internal Error");
-        else if (errNote != NULL)
-            auth_user_request->denyMessage(errNote);
+            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();
         debugs(29, DBG_IMPORTANT, "ERROR: Negotiate Authentication validating user. Result: " << reply);
-    } // break;
+        break;
     }
 
     if (lm_request->request) {

diff --git a/src/auth/ntlm/UserRequest.cc b/src/auth/ntlm/UserRequest.cc
index 55464d167e3677566d7caaae9ec6cbc0b790ac05..d94c0f4bcfb60e8004ee980d73c06dca825fecfb 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();
 
@@ -351,44 +351,35 @@ Auth::Ntlm::UserRequest::HandleReply(void *data, const Helper::Reply &reply)
     }
     break;
 
-    case Helper::Error: {
+    case Helper::Error:
         /* authentication failure (wrong password, etc.) */
-        const char *errNote = reply.notes.find("message");
-        if (errNote != NULL)
-            auth_user_request->denyMessage(errNote);
-        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();
         debugs(29, 4, "Failed validating user via NTLM. Result: " << reply);
-    }
-    break;
+        break;
 
     case Helper::Unknown:
         debugs(29, DBG_IMPORTANT, "ERROR: NTLM Authentication Helper '" << reply.whichServer << "' crashed!.");
     /* continue to the next case */
 
     case Helper::TimedOut:
-    case Helper::BrokenHelper: {
+    case Helper::BrokenHelper:
         /* TODO kick off a refresh process. This can occur after a YR or after
          * a KK. If after a YR release the helper and resubmit the request via
          * Authenticate NTLM start.
          * If after a KK deny the user's request w/ 407 and mark the helper as
          * Needing YR. */
-        const char *errNote = reply.notes.find("message");
         if (reply.result == Helper::Unknown)
-            auth_user_request->denyMessage("Internal Error");
-        else if (errNote != NULL)
-            auth_user_request->denyMessage(errNote);
+            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();
         debugs(29, DBG_IMPORTANT, "ERROR: NTLM Authentication validating user. Result: " << reply);
-    }
-    break;
+        break;
     }
 
     if (lm_request->request) {

diff --git a/src/tests/stub_libauth.cc b/src/tests/stub_libauth.cc
index fe8b47c4770e0294e7c04013c0d2575290715bd4..b7215f4d18ebab9c039e59f72f6cbc43bab486fb 100644 (file)
--- a/src/tests/stub_libauth.cc
+++ b/src/tests/stub_libauth.cc
@@ -53,8 +53,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)