From: Eduard Bagdasaryan Date: Sun, 5 Feb 2017 20:52:41 +0000 (+1300) Subject: Fix Auth::UserRequest::denyMessage() misuse. X-Git-Tag: SQUID_4_0_18~2 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=156df516a605b82f9db5af4705467474780d9895;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 8df7e9fdae..f8d273db20 100644 --- 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 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 4256d33ad6..026baa02dc 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(); @@ -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 55464d167e..d94c0f4bcf 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(); @@ -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 fe8b47c477..b7215f4d18 100644 --- 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)