From 495656f72b1244db748b255f5895520e64f4be46 Mon Sep 17 00:00:00 2001
From: Toby Allen <tobyallen@toflidium.com>
Date: Fri, 15 Feb 2019 18:50:30 +0000
Subject: [PATCH] basicauth: Log error when authentication fails (#2434)

* log basicauth errors

* Fixed tests

* Fix log include

* Remove some unneeded blank lines.
---
 caddyhttp/basicauth/basicauth.go      | 14 +++++++--
 caddyhttp/basicauth/basicauth_test.go | 45 ++++++++++++++++-----------
 2 files changed, 38 insertions(+), 21 deletions(-)

diff --git a/caddyhttp/basicauth/basicauth.go b/caddyhttp/basicauth/basicauth.go
index 59c65418c..9f9d4fc68 100644
--- a/caddyhttp/basicauth/basicauth.go
+++ b/caddyhttp/basicauth/basicauth.go
@@ -51,6 +51,9 @@ type BasicAuth struct {
 func (a BasicAuth) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) {
 	var protected, isAuthenticated bool
 	var realm string
+	var username string
+	var password string
+	var ok bool
 
 	// do not check for basic auth on OPTIONS call
 	if r.Method == http.MethodOptions {
@@ -69,7 +72,7 @@ func (a BasicAuth) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error
 			realm = rule.Realm
 
 			// parse auth header
-			username, password, ok := r.BasicAuth()
+			username, password, ok = r.BasicAuth()
 
 			// check credentials
 			if !ok ||
@@ -100,7 +103,14 @@ func (a BasicAuth) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error
 			realm = "Restricted"
 		}
 		w.Header().Set("WWW-Authenticate", "Basic realm=\""+realm+"\"")
-		return http.StatusUnauthorized, nil
+
+		// Get a replacer so we can provide basic info for the authentication error.
+		repl := httpserver.NewReplacer(r, nil, "-")
+		errstr := repl.Replace("BasicAuth: user \"%s\" was not found or password was incorrect. {remote} {host} {uri} {proto}")
+
+		// Username will not exist in Replacer so provide here.
+		err := fmt.Errorf(errstr, username)
+		return http.StatusUnauthorized, err
 	}
 
 	// Pass-through when no paths match
diff --git a/caddyhttp/basicauth/basicauth_test.go b/caddyhttp/basicauth/basicauth_test.go
index 1182b5226..0288f358f 100644
--- a/caddyhttp/basicauth/basicauth_test.go
+++ b/caddyhttp/basicauth/basicauth_test.go
@@ -22,6 +22,7 @@ import (
 	"net/http/httptest"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/mholt/caddy/caddyhttp/httpserver"
@@ -60,17 +61,18 @@ func TestBasicAuth(t *testing.T) {
 		result   int
 		user     string
 		password string
+		haserror bool
 	}
 
 	tests := []testType{
-		{"/testing", http.StatusOK, "okuser", "okpass"},
-		{"/testing", http.StatusUnauthorized, "baduser", "okpass"},
-		{"/testing", http.StatusUnauthorized, "okuser", "badpass"},
-		{"/testing", http.StatusUnauthorized, "OKuser", "okpass"},
-		{"/testing", http.StatusUnauthorized, "OKuser", "badPASS"},
-		{"/testing", http.StatusUnauthorized, "", "okpass"},
-		{"/testing", http.StatusUnauthorized, "okuser", ""},
-		{"/testing", http.StatusUnauthorized, "", ""},
+		{"/testing", http.StatusOK, "okuser", "okpass", false},
+		{"/testing", http.StatusUnauthorized, "baduser", "okpass", true},
+		{"/testing", http.StatusUnauthorized, "okuser", "badpass", true},
+		{"/testing", http.StatusUnauthorized, "OKuser", "okpass", true},
+		{"/testing", http.StatusUnauthorized, "OKuser", "badPASS", true},
+		{"/testing", http.StatusUnauthorized, "", "okpass", true},
+		{"/testing", http.StatusUnauthorized, "okuser", "", true},
+		{"/testing", http.StatusUnauthorized, "", "", true},
 	}
 
 	var test testType
@@ -89,7 +91,9 @@ func TestBasicAuth(t *testing.T) {
 			rec := httptest.NewRecorder()
 			result, err := rw.ServeHTTP(rec, req)
 			if err != nil {
-				t.Fatalf("Test %d: Could not ServeHTTP: %v", i, err)
+				if !test.haserror || !strings.HasPrefix(err.Error(), "BasicAuth: user") {
+					t.Fatalf("Test %d: Could not ServeHTTP: %v", i, err)
+				}
 			}
 			if result != test.result {
 				t.Errorf("Test %d: Expected status code %d but was %d",
@@ -124,16 +128,17 @@ func TestMultipleOverlappingRules(t *testing.T) {
 	}
 
 	tests := []struct {
-		from   string
-		result int
-		cred   string
+		from     string
+		result   int
+		cred     string
+		haserror bool
 	}{
-		{"/t", http.StatusOK, "t:p1"},
-		{"/t/t", http.StatusOK, "t:p1"},
-		{"/t/t", http.StatusOK, "t1:p2"},
-		{"/a", http.StatusOK, "t1:p2"},
-		{"/t/t", http.StatusUnauthorized, "t1:p3"},
-		{"/t", http.StatusUnauthorized, "t1:p2"},
+		{"/t", http.StatusOK, "t:p1", false},
+		{"/t/t", http.StatusOK, "t:p1", false},
+		{"/t/t", http.StatusOK, "t1:p2", false},
+		{"/a", http.StatusOK, "t1:p2", false},
+		{"/t/t", http.StatusUnauthorized, "t1:p3", true},
+		{"/t", http.StatusUnauthorized, "t1:p2", true},
 	}
 
 	for i, test := range tests {
@@ -148,7 +153,9 @@ func TestMultipleOverlappingRules(t *testing.T) {
 		rec := httptest.NewRecorder()
 		result, err := rw.ServeHTTP(rec, req)
 		if err != nil {
-			t.Fatalf("Test %d: Could not ServeHTTP %v", i, err)
+			if !test.haserror || !strings.HasPrefix(err.Error(), "BasicAuth: user") {
+				t.Fatalf("Test %d: Could not ServeHTTP %v", i, err)
+			}
 		}
 		if result != test.result {
 			t.Errorf("Test %d: Expected Header '%d' but was '%d'",