basicauth: Log error when authentication fails (#2434)

* log basicauth errors

* Fixed tests

* Fix log include

* Remove some unneeded blank lines.
This commit is contained in:
Toby Allen 2019-02-15 18:50:30 +00:00 committed by Matt Holt
parent c70d4a4cf6
commit 495656f72b
2 changed files with 38 additions and 21 deletions

View File

@ -51,6 +51,9 @@ type BasicAuth struct {
func (a BasicAuth) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) { func (a BasicAuth) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) {
var protected, isAuthenticated bool var protected, isAuthenticated bool
var realm string var realm string
var username string
var password string
var ok bool
// do not check for basic auth on OPTIONS call // do not check for basic auth on OPTIONS call
if r.Method == http.MethodOptions { if r.Method == http.MethodOptions {
@ -69,7 +72,7 @@ func (a BasicAuth) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error
realm = rule.Realm realm = rule.Realm
// parse auth header // parse auth header
username, password, ok := r.BasicAuth() username, password, ok = r.BasicAuth()
// check credentials // check credentials
if !ok || if !ok ||
@ -100,7 +103,14 @@ func (a BasicAuth) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error
realm = "Restricted" realm = "Restricted"
} }
w.Header().Set("WWW-Authenticate", "Basic realm=\""+realm+"\"") 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 // Pass-through when no paths match

View File

@ -22,6 +22,7 @@ import (
"net/http/httptest" "net/http/httptest"
"os" "os"
"path/filepath" "path/filepath"
"strings"
"testing" "testing"
"github.com/mholt/caddy/caddyhttp/httpserver" "github.com/mholt/caddy/caddyhttp/httpserver"
@ -60,17 +61,18 @@ func TestBasicAuth(t *testing.T) {
result int result int
user string user string
password string password string
haserror bool
} }
tests := []testType{ tests := []testType{
{"/testing", http.StatusOK, "okuser", "okpass"}, {"/testing", http.StatusOK, "okuser", "okpass", false},
{"/testing", http.StatusUnauthorized, "baduser", "okpass"}, {"/testing", http.StatusUnauthorized, "baduser", "okpass", true},
{"/testing", http.StatusUnauthorized, "okuser", "badpass"}, {"/testing", http.StatusUnauthorized, "okuser", "badpass", true},
{"/testing", http.StatusUnauthorized, "OKuser", "okpass"}, {"/testing", http.StatusUnauthorized, "OKuser", "okpass", true},
{"/testing", http.StatusUnauthorized, "OKuser", "badPASS"}, {"/testing", http.StatusUnauthorized, "OKuser", "badPASS", true},
{"/testing", http.StatusUnauthorized, "", "okpass"}, {"/testing", http.StatusUnauthorized, "", "okpass", true},
{"/testing", http.StatusUnauthorized, "okuser", ""}, {"/testing", http.StatusUnauthorized, "okuser", "", true},
{"/testing", http.StatusUnauthorized, "", ""}, {"/testing", http.StatusUnauthorized, "", "", true},
} }
var test testType var test testType
@ -89,7 +91,9 @@ func TestBasicAuth(t *testing.T) {
rec := httptest.NewRecorder() rec := httptest.NewRecorder()
result, err := rw.ServeHTTP(rec, req) result, err := rw.ServeHTTP(rec, req)
if err != nil { 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 { if result != test.result {
t.Errorf("Test %d: Expected status code %d but was %d", t.Errorf("Test %d: Expected status code %d but was %d",
@ -124,16 +128,17 @@ func TestMultipleOverlappingRules(t *testing.T) {
} }
tests := []struct { tests := []struct {
from string from string
result int result int
cred string cred string
haserror bool
}{ }{
{"/t", http.StatusOK, "t:p1"}, {"/t", http.StatusOK, "t:p1", false},
{"/t/t", http.StatusOK, "t:p1"}, {"/t/t", http.StatusOK, "t:p1", false},
{"/t/t", http.StatusOK, "t1:p2"}, {"/t/t", http.StatusOK, "t1:p2", false},
{"/a", http.StatusOK, "t1:p2"}, {"/a", http.StatusOK, "t1:p2", false},
{"/t/t", http.StatusUnauthorized, "t1:p3"}, {"/t/t", http.StatusUnauthorized, "t1:p3", true},
{"/t", http.StatusUnauthorized, "t1:p2"}, {"/t", http.StatusUnauthorized, "t1:p2", true},
} }
for i, test := range tests { for i, test := range tests {
@ -148,7 +153,9 @@ func TestMultipleOverlappingRules(t *testing.T) {
rec := httptest.NewRecorder() rec := httptest.NewRecorder()
result, err := rw.ServeHTTP(rec, req) result, err := rw.ServeHTTP(rec, req)
if err != nil { 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 { if result != test.result {
t.Errorf("Test %d: Expected Header '%d' but was '%d'", t.Errorf("Test %d: Expected Header '%d' but was '%d'",