Merge pull request #3722 from hashicorp/b-3697

Fix HTTP code for permission denied errors

CHANGELOG.md

@@ -11,6 +11,7 @@ BUG FIXES:
    allocations could result in improper placement counts [[GH-3717](https://github.com/hashicorp/nomad/issues/3717)]
  * client: Migrated ephemeral_disk's maintain directory permissions [[GH-3723](https://github.com/hashicorp/nomad/issues/3723)]
  * config: Revert minimum CPU limit back to 20 from 100.
+ * ui: Fix ui on non-leaders when ACLs are enabled [[GH-3722](https://github.com/hashicorp/nomad/issues/3722)]
  * ui: Fix requests using client-side certificates in Firefox. [[GH-3728](https://github.com/hashicorp/nomad/pull/3728)]
 
 ## 0.7.1 (December 19, 2017)
@@ -663,7 +664,7 @@ BUG FIXES:
   * client: Killing an allocation doesn't cause allocation stats to block
     [[GH-1454](https://github.com/hashicorp/nomad/issues/1454)]
   * driver/docker: Disable swap on docker driver [[GH-1480](https://github.com/hashicorp/nomad/issues/1480)]
-  * driver/docker: Fix improper gating on privileged mode [[GH-1506](https://github.com/hashicorp/nomad/issues/1506)]
+  * driver/docker: Fix improper gating on priviledged mode [[GH-1506](https://github.com/hashicorp/nomad/issues/1506)]
   * driver/docker: Default network type is "nat" on Windows [[GH-1521](https://github.com/hashicorp/nomad/issues/1521)]
   * driver/docker: Cleanup created volume when destroying container [[GH-1519](https://github.com/hashicorp/nomad/issues/1519)]
   * driver/rkt: Set host environment variables [[GH-1581](https://github.com/hashicorp/nomad/issues/1581)]

command/agent/http.go

@@ -11,6 +11,7 @@ import (
 	"net/http/pprof"
 	"os"
 	"strconv"
+	"strings"
 	"time"
 
 	"github.com/NYTimes/gziphandler"
@@ -281,17 +282,22 @@ func (s *HTTPServer) wrap(handler func(resp http.ResponseWriter, req *http.Reque
 		if err != nil {
 			s.logger.Printf("[ERR] http: Request %v, error: %v", reqURL, err)
 			code := 500
+			errMsg := err.Error()
 			if http, ok := err.(HTTPCodedError); ok {
 				code = http.Code()
 			} else {
-				switch err.Error() {
-				case structs.ErrPermissionDenied.Error(), structs.ErrTokenNotFound.Error():
+				// RPC errors get wrapped, so manually unwrap by only looking at their suffix
+				if strings.HasSuffix(errMsg, structs.ErrPermissionDenied.Error()) {
+					errMsg = structs.ErrPermissionDenied.Error()
+					code = 403
+				} else if strings.HasSuffix(errMsg, structs.ErrTokenNotFound.Error()) {
+					errMsg = structs.ErrTokenNotFound.Error()
 					code = 403
 				}
 			}
 
 			resp.WriteHeader(code)
-			resp.Write([]byte(err.Error()))
+			resp.Write([]byte(errMsg))
 			return
 		}
 

command/agent/http_test.go

@@ -225,15 +225,28 @@ func TestPermissionDenied(t *testing.T) {
 	})
 	defer s.Shutdown()
 
-	resp := httptest.NewRecorder()
-	handler := func(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
-		return nil, structs.ErrPermissionDenied
+	{
+		resp := httptest.NewRecorder()
+		handler := func(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
+			return nil, structs.ErrPermissionDenied
+		}
+
+		req, _ := http.NewRequest("GET", "/v1/job/foo", nil)
+		s.Server.wrap(handler)(resp, req)
+		assert.Equal(t, resp.Code, 403)
 	}
 
-	urlStr := "/v1/job/foo"
-	req, _ := http.NewRequest("GET", urlStr, nil)
-	s.Server.wrap(handler)(resp, req)
-	assert.Equal(t, resp.Code, 403)
+	// When remote RPC is used the errors have "rpc error: " prependend
+	{
+		resp := httptest.NewRecorder()
+		handler := func(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
+			return nil, fmt.Errorf("rpc error: %v", structs.ErrPermissionDenied)
+		}
+
+		req, _ := http.NewRequest("GET", "/v1/job/foo", nil)
+		s.Server.wrap(handler)(resp, req)
+		assert.Equal(t, resp.Code, 403)
+	}
 }
 
 func TestTokenNotFound(t *testing.T) {