From: Eduard Bagdasaryan Date: Mon, 30 Jan 2017 13:01:33 +0000 (+0200) Subject: Fix Auth::UserRequest::denyMessage() misuse. X-Git-Tag: M-staged-PR71~291 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=6f9a30f86fd6046d371dbdeab23a2781219f0a04;p=thirdparty%2Fsquid.git 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 1def969d34..52d75e4ec0 100644 --- 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 0711ee80aa..eafda066ed 100644 --- 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 0f675080ee..c7c3ab7363 100644 --- 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 4670a11908..891d110787 100644 --- 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 b1b07e7fa0..0ee6c1c236 100644 --- 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)