Fixup review comments

Signed-off-by: Mark Anderson <[email protected]>

.changelog/12470.txt

@@ -1,3 +1,3 @@
 ```release-note:enhancement
-Provide fuller detail in the error messsage when an ACL denies access.
+acl: Provide fuller detail in the error messsage when an ACL denies access.
 ```

acl/authorizer.go

@@ -42,7 +42,6 @@ const (
 	ResourceEvent     Resource = "event"
 	ResourceIntention Resource = "intention"
 	ResourceKey       Resource = "key"
-	ResourceKeyPrefix Resource = "key prefix"
 	ResourceKeyring   Resource = "keyring"
 	ResourceNode      Resource = "node"
 	ResourceOperator  Resource = "operator"
@@ -229,7 +228,7 @@ func (a AllowAuthorizer) EventWriteAllowed(name string, ctx *AuthorizerContext)
 func (a AllowAuthorizer) IntentionDefaultAllowAllowed(ctx *AuthorizerContext) error {
 	if a.Authorizer.IntentionDefaultAllow(ctx) != Allow {
 		// This is a bit nuanced, in that this isn't set by a rule, but inherited globally
-		// TODO revisit when we have full accessor info
+		// TODO(acl-error-enhancements) revisit when we have full accessor info
 		return PermissionDeniedError{Cause: "Denied by intention default"}
 	}
 	return nil
@@ -281,7 +280,7 @@ func (a AllowAuthorizer) KeyWriteAllowed(name string, ctx *AuthorizerContext) er
 // that deny a write.
 func (a AllowAuthorizer) KeyWritePrefixAllowed(name string, ctx *AuthorizerContext) error {
 	if a.Authorizer.KeyWritePrefix(name, ctx) != Allow {
-		// TODO revisit this message; we may need to do some extra plumbing inside of KeyWritePrefix to
+		// TODO(acl-error-enhancements) revisit this message; we may need to do some extra plumbing inside of KeyWritePrefix to
 		// return properly detailed information.
 		return PermissionDeniedByACL(a, ctx, ResourceKey, AccessWrite, name)
 	}
@@ -334,8 +333,8 @@ func (a AllowAuthorizer) NodeReadAllowed(name string, ctx *AuthorizerContext) er
 // NodeReadAllAllowed checks for permission to read (discover) all nodes.
 func (a AllowAuthorizer) NodeReadAllAllowed(ctx *AuthorizerContext) error {
 	if a.Authorizer.NodeReadAll(ctx) != Allow {
-		// TODO Revisit this to see if we can return a more detailed answer
-		return PermissionDeniedByACLUnnamed(a, ctx, ResourceNode, AccessRead)
+		// This is only used to gate certain UI functions right now (e.g metrics)
+		return PermissionDeniedByACL(a, ctx, ResourceNode, AccessRead, "all nodes")
 	}
 	return nil
 }
@@ -396,10 +395,10 @@ func (a AllowAuthorizer) ServiceReadAllowed(name string, ctx *AuthorizerContext)
 // ServiceReadAllAllowed checks for permission to read all services
 func (a AllowAuthorizer) ServiceReadAllAllowed(ctx *AuthorizerContext) error {
 	if a.Authorizer.ServiceReadAll(ctx) != Allow {
-		return PermissionDeniedByACLUnnamed(a, ctx, ResourceService, AccessRead) // read
+		// This is only used to gate certain UI functions right now (e.g metrics)
+		return PermissionDeniedByACL(a, ctx, ResourceService, AccessRead, "all services") // read
 	}
 	return nil
-
 }
 
 // ServiceWriteAllowed checks for permission to create or update a given
@@ -409,7 +408,6 @@ func (a AllowAuthorizer) ServiceWriteAllowed(name string, ctx *AuthorizerContext
 		return PermissionDeniedByACL(a, ctx, ResourceService, AccessWrite, name)
 	}
 	return nil
-
 }
 
 // SessionReadAllowed checks for permission to read sessions for a given node.
@@ -432,7 +430,7 @@ func (a AllowAuthorizer) SessionWriteAllowed(name string, ctx *AuthorizerContext
 // SnapshotAllowed checks for permission to take and restore snapshots.
 func (a AllowAuthorizer) SnapshotAllowed(ctx *AuthorizerContext) error {
 	if a.Authorizer.Snapshot(ctx) != Allow {
-		// Implimetation of this currently just checks acl write
+		// Implementation of this currently just checks acl write
 		return PermissionDeniedByACLUnnamed(a, ctx, ResourceACL, AccessWrite)
 	}
 	return nil

acl/errors.go

@@ -3,9 +3,7 @@ package acl
 import (
 	"errors"
 	"fmt"
-	"github.com/stretchr/testify/require"
 	"strings"
-	"testing"
 )
 
 // These error constants define the standard ACL error types. The values
@@ -108,7 +106,7 @@ func (e PermissionDeniedError) Error() string {
 	fmt.Fprintf(&message, " lacks permission '%s:%s'", e.Resource, e.AccessLevel.String())
 
 	if e.ResourceID.Name != "" {
-		fmt.Fprintf(&message, " %s", e.ResourceID.ToString())
+		fmt.Fprintf(&message, " on %s", e.ResourceID.ToString())
 	}
 	return message.String()
 }
@@ -128,24 +126,3 @@ func PermissionDeniedByACLUnnamed(_ Authorizer, context *AuthorizerContext, reso
 	desc := NewResourceDescriptor("", context)
 	return PermissionDeniedError{Accessor: "", Resource: resource, AccessLevel: accessLevel, ResourceID: desc}
 }
-
-func RequirePermissionDeniedError(t testing.TB, err error, _ Authorizer, _ *AuthorizerContext, resource Resource, accessLevel AccessLevel, resourceID string) {
-	t.Helper()
-	if err == nil {
-		t.Fatal("An error is expected but got nil.")
-	}
-	if v, ok := err.(PermissionDeniedError); ok {
-		require.Equal(t, v.Resource, resource)
-		require.Equal(t, v.AccessLevel, accessLevel)
-		require.Equal(t, v.ResourceID.Name, resourceID)
-	} else {
-		t.Fatalf("Expected a permission denied error got %v", err)
-	}
-}
-
-func RequirePermissionDeniedMessage(t testing.TB, msg string, _ Authorizer, _ *AuthorizerContext, resource Resource, accessLevel AccessLevel, resourceID string) {
-	require.Contains(t, msg, errPermissionDenied)
-	require.Contains(t, msg, resource)
-	require.Contains(t, msg, accessLevel.String())
-	require.Contains(t, msg, resourceID)
-}

acl/errors_test.go

@@ -29,7 +29,7 @@ func TestPermissionDeniedError(t *testing.T) {
 		},
 		{
 			err:      PermissionDeniedByACL(&auth1, nil, ResourceService, AccessRead, "foobar"),
-			expected: "Permission denied: provided accessor lacks permission 'service:read' foobar",
+			expected: "Permission denied: provided accessor lacks permission 'service:read' on foobar",
 		},
 		{
 			err:      PermissionDeniedByACLUnnamed(&auth1, nil, ResourceService, AccessRead),

acl/testing.go

@@ -0,0 +1,47 @@
+package acl
+
+import (
+	"github.com/stretchr/testify/require"
+	"regexp"
+	"testing"
+)
+
+func RequirePermissionDeniedError(t testing.TB, err error, _ Authorizer, _ *AuthorizerContext, resource Resource, accessLevel AccessLevel, resourceID string) {
+	t.Helper()
+	if err == nil {
+		t.Fatal("An error is expected but got nil.")
+	}
+	if v, ok := err.(PermissionDeniedError); ok {
+		require.Equal(t, v.Resource, resource)
+		require.Equal(t, v.AccessLevel, accessLevel)
+		require.Equal(t, v.ResourceID.Name, resourceID)
+	} else {
+		t.Fatalf("Expected a permission denied error got %T %vp", err, err)
+	}
+}
+
+func RequirePermissionDeniedMessage(t testing.TB, msg string, auth Authorizer, _ *AuthorizerContext, resource Resource, accessLevel AccessLevel, resourceID string) {
+	require.NotEmpty(t, msg, "expected non-empty error message")
+
+	var resourceIDFound string
+	if auth == nil {
+		expr := "^Permission denied" + `: provided accessor lacks permission '(\S*):(\S*)' on (.*)\s*$`
+		re, _ := regexp.Compile(expr)
+		matched := re.FindStringSubmatch(msg)
+
+		require.Equal(t, string(resource), matched[1], "resource")
+		require.Equal(t, accessLevel.String(), matched[2], "access level")
+		resourceIDFound = matched[3]
+	} else {
+		expr := "^Permission denied" + `: accessor '(\S*)' lacks permission '(\S*):(\S*)' on (.*)\s*$`
+		re, _ := regexp.Compile(expr)
+		matched := re.FindStringSubmatch(msg)
+
+		require.Equal(t, auth, matched[1], "auth")
+		require.Equal(t, string(resource), matched[2], "resource")
+		require.Equal(t, accessLevel.String(), matched[3], "access level")
+		resourceIDFound = matched[4]
+	}
+	// AuthorizerContext information should be checked here
+	require.Contains(t, resourceIDFound, resourceID, "resource id")
+}

agent/acl.go

@@ -2,11 +2,13 @@ package agent
 
 import (
 	"fmt"
+
+	"github.com/hashicorp/serf/serf"
+
 	"github.com/hashicorp/consul/acl"
 	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/consul/api"
 	"github.com/hashicorp/consul/types"
-	"github.com/hashicorp/serf/serf"
 )
 
 // aclAccessorID is used to convert an ACLToken's secretID to its accessorID for non-

agent/consul/catalog_endpoint.go

@@ -650,7 +650,7 @@ func (c *Catalog) ServiceNodes(args *structs.ServiceSpecificRequest, reply *stru
 	// If we're doing a connect query, we need read access to the service
 	// we're trying to find proxies for, so check that.
 	if args.Connect {
-		// TODO: ACL Error improvements; can this be improved? What happens if we returned an error here?
+		// TODO(acl-error-enhancements) can this be improved? What happens if we returned an error here?
 		// Is this similar to filters where we might want to return a hint?
 		if authz.ServiceRead(args.ServiceName, &authzContext) != acl.Allow {
 			// Just return nil, which will return an empty response (tested)

agent/consul/config_endpoint.go

@@ -90,7 +90,7 @@ func (c *ConfigEntry) Apply(args *structs.ConfigEntryRequest, reply *bool) error
 	}
 
 	if !args.Entry.CanWrite(authz) {
-		return acl.ErrPermissionDenied // TODO: Better errors await refactoring of CanWrite above.
+		return acl.ErrPermissionDenied // TODO(acl-error-enhancements) Better errors await refactoring of CanWrite above.
 	}
 
 	if args.Op != structs.ConfigEntryUpsert && args.Op != structs.ConfigEntryUpsertCAS {

agent/consul/connect_ca_endpoint.go

@@ -65,7 +65,7 @@ func (s *ConnectCA) ConfigurationGet(
 	if err != nil {
 		return err
 	}
-	if err := authz.ToAllowAuthorizer().ToAllowAuthorizer().OperatorWriteAllowed(nil); err != nil {
+	if err := authz.ToAllowAuthorizer().OperatorWriteAllowed(nil); err != nil {
 		return err
 	}
 

agent/consul/health_endpoint.go

@@ -220,8 +220,7 @@ func (h *Health) ServiceNodes(args *structs.ServiceSpecificRequest, reply *struc
 	// If we're doing a connect or ingress query, we need read access to the service
 	// we're trying to find proxies for, so check that.
 	if args.Connect || args.Ingress {
-		// TODO: ACL Error improvements; can this be improved? What happens if we returned an error here?
-		// Is this similar to filters where we might want to return a hint?
+		// TODO(acl-error-enhancements) Look for ways to percolate this information up to give any feedback to the user.
 		if authz.ServiceRead(args.ServiceName, &authzContext) != acl.Allow {
 			// Just return nil, which will return an empty response (tested)
 			return nil

agent/http.go

@@ -273,7 +273,7 @@ func (s *HTTPHandlers) handler(enableDebug bool) http.Handler {
 			// If the token provided does not have the necessary permissions,
 			// write a forbidden response
 			// TODO(partitions): should this be possible in a partition?
-			// TODO(acl errors): can we return a better error here?
+			// TODO((acl-error-enhancements)): We should return error details somehow here.
 			if authz.OperatorRead(nil) != acl.Allow {
 				resp.WriteHeader(http.StatusForbidden)
 				return

agent/xds/delta_test.go

@@ -1156,6 +1156,9 @@ func TestServer_DeltaAggregatedResources_v3_ACLEnforcement(t *testing.T) {
 			case err := <-errCh:
 				if tt.wantDenied {
 					require.Error(t, err)
+					status, ok := status.FromError(err)
+					require.True(t, ok)
+					require.Equal(t, codes.PermissionDenied, status.Code())
 					require.Contains(t, err.Error(), "Permission denied")
 					mgr.AssertWatchCancelled(t, sid)
 				} else {

agent/xds/server.go

@@ -266,7 +266,7 @@ func (s *Server) authorize(ctx context.Context, cfgSnap *proxycfg.ConfigSnapshot
 	if acl.IsErrNotFound(err) {
 		return status.Errorf(codes.Unauthenticated, "unauthenticated: %v", err)
 	} else if acl.IsErrPermissionDenied(err) {
-		return status.Errorf(codes.PermissionDenied, "permission denied: %v", err)
+		return status.Error(codes.PermissionDenied, err.Error())
 	} else if err != nil {
 		return status.Errorf(codes.Internal, "error resolving acl token: %v", err)
 	}