Merge pull request influxdata#6978 from influxdata/js-6959-403-forbid…

…den-on-auth-failure

Properly use the 401 and 403 HTTP status codes

CHANGELOG.md

@@ -34,6 +34,7 @@ With this release the systemd configuration files for InfluxDB will use the syst
 - [#6507](https://github.com/influxdata/influxdb/issues/6507): Refactor monitor service to avoid expvar and write monitor statistics on a truncated time interval.
 - [#6805](https://github.com/influxdata/influxdb/issues/6805): Allow any variant of the help option to trigger the help.
 - [#5499](https://github.com/influxdata/influxdb/issues/5499): Add stats and diagnostics to the TSM engine.
+- [#6959](https://github.com/influxdata/influxdb/issues/6959): Return 403 Forbidden when authentication succeeds but authorization fails.
 
 ### Bugfixes
 

etc/config.sample.toml

@@ -168,6 +168,7 @@ reporting-disabled = false
   ### Use a separate private key location.
   # https-private-key = ""
   max-row-limit = 10000
+  realm = "InfluxDB"
 
 ###
 ### [subsciber]

services/httpd/config.go

@@ -1,7 +1,12 @@
 package httpd
 
-// DefaultBindAddress is the default address to bind to.
-const DefaultBindAddress = ":8086"
+const (
+	// DefaultBindAddress is the default address to bind to.
+	DefaultBindAddress = ":8086"
+
+	// DefaultRealm is the default realm sent back when issuing a basic auth challenge.
+	DefaultRealm = "InfluxDB"
+)
 
 // Config represents a configuration for a HTTP service.
 type Config struct {
@@ -16,16 +21,18 @@ type Config struct {
 	MaxRowLimit        int    `toml:"max-row-limit"`
 	MaxConnectionLimit int    `toml:"max-connection-limit"`
 	SharedSecret       string `toml:"shared-secret"`
+	Realm              string `toml:"realm"`
 }
 
 // NewConfig returns a new Config with default settings.
 func NewConfig() Config {
 	return Config{
 		Enabled:          true,
-		BindAddress:      ":8086",
+		BindAddress:      DefaultBindAddress,
 		LogEnabled:       true,
 		HTTPSEnabled:     false,
 		HTTPSCertificate: "/etc/ssl/influxdb.pem",
 		MaxRowLimit:      DefaultChunkSize,
+		Realm:            DefaultRealm,
 	}
 }

services/httpd/handler.go

@@ -385,7 +385,7 @@ func (h *Handler) serveQuery(w http.ResponseWriter, r *http.Request, user *meta.
 			if err, ok := err.(meta.ErrAuthorize); ok {
 				h.Logger.Printf("Unauthorized request | user: %q | query: %q | database %q\n", err.User, err.Query.String(), err.Database)
 			}
-			h.httpError(w, "error authorizing query: "+err.Error(), pretty, http.StatusUnauthorized)
+			h.httpError(w, "error authorizing query: "+err.Error(), pretty, http.StatusForbidden)
 			return
 		}
 	}
@@ -539,13 +539,13 @@ func (h *Handler) serveWrite(w http.ResponseWriter, r *http.Request, user *meta.
 	}
 
 	if h.Config.AuthEnabled && user == nil {
-		h.resultError(w, influxql.Result{Err: fmt.Errorf("user is required to write to database %q", database)}, http.StatusUnauthorized)
+		h.resultError(w, influxql.Result{Err: fmt.Errorf("user is required to write to database %q", database)}, http.StatusForbidden)
 		return
 	}
 
 	if h.Config.AuthEnabled {
 		if err := h.WriteAuthorizer.AuthorizeWrite(user.Name, database); err != nil {
-			h.resultError(w, influxql.Result{Err: fmt.Errorf("%q user is not authorized to write to database %q", user.Name, database)}, http.StatusUnauthorized)
+			h.resultError(w, influxql.Result{Err: fmt.Errorf("%q user is not authorized to write to database %q", user.Name, database)}, http.StatusForbidden)
 			return
 		}
 	}
@@ -761,6 +761,11 @@ func (h *Handler) serveExpvar(w http.ResponseWriter, r *http.Request) {
 // h.httpError writes an error to the client in a standard format.
 func (h *Handler) httpError(w http.ResponseWriter, error string, pretty bool, code int) {
 	w.Header().Add("Content-Type", "application/json")
+	if code == http.StatusUnauthorized {
+		// If an unauthorized header will be sent back, add a WWW-Authenticate header
+		// as an authorization challenge.
+		w.Header().Set("WWW-Authenticate", fmt.Sprintf("Basic realm=\"%s\"", h.Config.Realm))
+	}
 	h.writeHeader(w, code)
 	response := Response{Err: errors.New(error)}
 	var b []byte

services/httpd/handler_test.go

@@ -274,19 +274,80 @@ func TestHandler_Query_ErrInvalidQuery(t *testing.T) {
 	}
 }
 
-// Ensure the handler returns a status 401 if the user is not authorized.
-// func TestHandler_Query_ErrUnauthorized(t *testing.T) {
-// 	h := NewHandler(false)
-// 	h.QueryExecutor.AuthorizeFn = func(u *meta.UserInfo, q *influxql.Query, db string) error {
-// 		return errors.New("marker")
-// 	}
-
-// 	w := httptest.NewRecorder()
-// 	h.ServeHTTP(w, MustNewJSONRequest("GET", "/query?u=bar&db=foo&q=SHOW+SERIES+FROM+bar", nil))
-// 	if w.Code != http.StatusUnauthorized {
-// 		t.Fatalf("unexpected status: %d", w.Code)
-// 	}
-// }
+// Ensure the handler returns an appropriate 401 or 403 status when authentication or authorization fails.
+func TestHandler_Query_ErrAuthorize(t *testing.T) {
+	h := NewHandler(true)
+	h.QueryAuthorizer.AuthorizeQueryFn = func(u *meta.UserInfo, q *influxql.Query, db string) error {
+		return errors.New("marker")
+	}
+	h.MetaClient.UsersFn = func() []meta.UserInfo {
+		return []meta.UserInfo{
+			{
+				Name:  "admin",
+				Hash:  "admin",
+				Admin: true,
+			},
+			{
+				Name: "user1",
+				Hash: "abcd",
+				Privileges: map[string]influxql.Privilege{
+					"db0": influxql.ReadPrivilege,
+				},
+			},
+		}
+	}
+	h.MetaClient.AuthenticateFn = func(u, p string) (*meta.UserInfo, error) {
+		for _, user := range h.MetaClient.Users() {
+			if u == user.Name {
+				if p == user.Hash {
+					return &user, nil
+				}
+				return nil, meta.ErrAuthenticate
+			}
+		}
+		return nil, meta.ErrUserNotFound
+	}
+
+	for i, tt := range []struct {
+		user     string
+		password string
+		query    string
+		code     int
+	}{
+		{
+			query: "/query?q=SHOW+DATABASES",
+			code:  http.StatusUnauthorized,
+		},
+		{
+			user:     "user1",
+			password: "abcd",
+			query:    "/query?q=SHOW+DATABASES",
+			code:     http.StatusForbidden,
+		},
+		{
+			user:     "user2",
+			password: "abcd",
+			query:    "/query?q=SHOW+DATABASES",
+			code:     http.StatusUnauthorized,
+		},
+	} {
+		w := httptest.NewRecorder()
+		r := MustNewJSONRequest("GET", tt.query, nil)
+		params := r.URL.Query()
+		if tt.user != "" {
+			params.Set("u", tt.user)
+		}
+		if tt.password != "" {
+			params.Set("p", tt.password)
+		}
+		r.URL.RawQuery = params.Encode()
+
+		h.ServeHTTP(w, r)
+		if w.Code != tt.code {
+			t.Errorf("%d. unexpected status: got=%d exp=%d\noutput: %s", i, w.Code, tt.code, w.Body.String())
+		}
+	}
+}
 
 // Ensure the handler returns a status 200 if an error is returned in the result.
 func TestHandler_Query_ErrResult(t *testing.T) {