Merge pull request #9213 from hashicorp/dnephin/resolve-tokens-take-2

acl: Remove some unused things and document delegate method

agent/acl.go

@@ -3,9 +3,10 @@ package agent
 import (
 	"fmt"
 
+	"github.com/hashicorp/serf/serf"
+
 	"github.com/hashicorp/consul/acl"
 	"github.com/hashicorp/consul/agent/structs"
-	"github.com/hashicorp/serf/serf"
 )
 
 // resolveToken is the primary interface used by ACL-checkers in the agent
@@ -36,16 +37,11 @@ func (a *Agent) resolveTokenAndDefaultMeta(id string, entMeta *structs.Enterpris
 	return a.delegate.ResolveTokenAndDefaultMeta(id, entMeta, authzContext)
 }
 
-// resolveIdentityFromToken is used to resolve an ACLToken's secretID to a structs.ACLIdentity
-func (a *Agent) resolveIdentityFromToken(secretID string) (structs.ACLIdentity, error) {
-	return a.delegate.ResolveTokenToIdentity(secretID)
-}
-
 // aclAccessorID is used to convert an ACLToken's secretID to its accessorID for non-
 // critical purposes, such as logging. Therefore we interpret all errors as empty-string
 // so we can safely log it without handling non-critical errors at the usage site.
 func (a *Agent) aclAccessorID(secretID string) string {
-	ident, err := a.resolveIdentityFromToken(secretID)
+	ident, err := a.delegate.ResolveTokenToIdentity(secretID)
 	if acl.IsErrNotFound(err) {
 		return ""
 	}

agent/acl_test.go

@@ -7,6 +7,9 @@ import (
 	"time"
 
 	"github.com/armon/go-metrics"
+	"github.com/hashicorp/go-hclog"
+	"github.com/hashicorp/serf/serf"
+
 	"github.com/hashicorp/consul/acl"
 	"github.com/hashicorp/consul/agent/config"
 	"github.com/hashicorp/consul/agent/consul"
@@ -15,8 +18,6 @@ import (
 	"github.com/hashicorp/consul/lib"
 	"github.com/hashicorp/consul/sdk/testutil"
 	"github.com/hashicorp/consul/types"
-	"github.com/hashicorp/go-hclog"
-	"github.com/hashicorp/serf/serf"
 
 	"github.com/stretchr/testify/require"
 )
@@ -519,6 +520,7 @@ func TestACL_filterChecks(t *testing.T) {
 	require.False(t, ok)
 }
 
+// TODO: remove?
 func TestACL_ResolveIdentity(t *testing.T) {
 	t.Parallel()
 	a := NewTestACLAgent(t, t.Name(), TestACLConfig(), nil, catalogIdent)
@@ -526,7 +528,7 @@ func TestACL_ResolveIdentity(t *testing.T) {
 	// this test is meant to ensure we are calling the correct function
 	// which is ResolveTokenToIdentity on the Agent delegate. Our
 	// nil authz resolver will cause it to emit an error if used
-	ident, err := a.resolveIdentityFromToken(nodeROSecret)
+	ident, err := a.delegate.ResolveTokenToIdentity(nodeROSecret)
 	require.NoError(t, err)
 	require.NotNil(t, ident)
 

agent/agent.go

@@ -130,9 +130,16 @@ type delegate interface {
 	LocalMember() serf.Member
 	JoinLAN(addrs []string) (n int, err error)
 	RemoveFailedNode(node string, prune bool) error
-	ResolveToken(secretID string) (acl.Authorizer, error)
-	ResolveTokenToIdentity(secretID string) (structs.ACLIdentity, error)
-	ResolveTokenAndDefaultMeta(secretID string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (acl.Authorizer, error)
+
+	// TODO: replace this method with consul.ACLResolver
+	ResolveTokenToIdentity(token string) (structs.ACLIdentity, error)
+
+	// ResolveTokenAndDefaultMeta returns an acl.Authorizer which authorizes
+	// actions based on the permissions granted to the token.
+	// If either entMeta or authzContext are non-nil they will be populated with the
+	// default namespace from the token.
+	ResolveTokenAndDefaultMeta(token string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (acl.Authorizer, error)
+
 	RPC(method string, args interface{}, reply interface{}) error
 	UseLegacyACLs() bool
 	SnapshotRPC(args *structs.SnapshotRequest, in io.Reader, out io.Writer, replyFn structs.SnapshotReplyFn) error

agent/consul/acl.go

@@ -8,12 +8,13 @@ import (
 
 	"github.com/armon/go-metrics"
 	"github.com/armon/go-metrics/prometheus"
-	"github.com/hashicorp/consul/acl"
-	"github.com/hashicorp/consul/agent/structs"
-	"github.com/hashicorp/consul/logging"
 	"github.com/hashicorp/go-hclog"
 	"golang.org/x/sync/singleflight"
 	"golang.org/x/time/rate"
+
+	"github.com/hashicorp/consul/acl"
+	"github.com/hashicorp/consul/agent/structs"
+	"github.com/hashicorp/consul/logging"
 )
 
 var ACLCounters = []prometheus.CounterDefinition{
@@ -1039,11 +1040,6 @@ func (r *ACLResolver) collectRolesForIdentity(identity structs.ACLIdentity, role
 	return roles, nil
 }
 
-func (r *ACLResolver) resolveTokenToPolicies(token string) (structs.ACLPolicies, error) {
-	_, policies, err := r.resolveTokenToIdentityAndPolicies(token)
-	return policies, err
-}
-
 func (r *ACLResolver) resolveTokenToIdentityAndPolicies(token string) (structs.ACLIdentity, structs.ACLPolicies, error) {
 	var lastErr error
 	var lastIdentity structs.ACLIdentity
@@ -1191,11 +1187,6 @@ func (r *ACLResolver) ResolveTokenToIdentityAndAuthorizer(token string) (structs
 	return identity, acl.NewChainedAuthorizer(chain), nil
 }
 
-func (r *ACLResolver) ResolveToken(token string) (acl.Authorizer, error) {
-	_, authz, err := r.ResolveTokenToIdentityAndAuthorizer(token)
-	return authz, err
-}
-
 func (r *ACLResolver) ResolveTokenToIdentity(token string) (structs.ACLIdentity, error) {
 	if !r.ACLsEnabled() {
 		return nil, nil
@@ -1979,7 +1970,7 @@ func (r *ACLResolver) filterACLWithAuthorizer(authorizer acl.Authorizer, subj in
 // rules configured for the provided token.
 func (r *ACLResolver) filterACL(token string, subj interface{}) error {
 	// Get the ACL from the token
-	authorizer, err := r.ResolveToken(token)
+	_, authorizer, err := r.ResolveTokenToIdentityAndAuthorizer(token)
 	if err != nil {
 		return err
 	}

agent/consul/acl_client.go

@@ -86,10 +86,6 @@ func (c *Client) ResolveRoleFromID(roleID string) (bool, *structs.ACLRole, error
 	return false, nil, nil
 }
 
-func (c *Client) ResolveToken(token string) (acl.Authorizer, error) {
-	return c.acls.ResolveToken(token)
-}
-
 func (c *Client) ResolveTokenToIdentity(token string) (structs.ACLIdentity, error) {
 	// not using ResolveTokenToIdentityAndAuthorizer because in this case we don't
 	// need to resolve the roles, policies and namespace but just want the identity
@@ -97,10 +93,6 @@ func (c *Client) ResolveTokenToIdentity(token string) (structs.ACLIdentity, erro
 	return c.acls.ResolveTokenToIdentity(token)
 }
 
-func (c *Client) ResolveTokenToIdentityAndAuthorizer(token string) (structs.ACLIdentity, acl.Authorizer, error) {
-	return c.acls.ResolveTokenToIdentityAndAuthorizer(token)
-}
-
 func (c *Client) ResolveTokenAndDefaultMeta(token string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (acl.Authorizer, error) {
 	identity, authz, err := c.acls.ResolveTokenToIdentityAndAuthorizer(token)
 	if err != nil {

agent/consul/acl_test.go

@@ -9,14 +9,15 @@ import (
 	"testing"
 	"time"
 
+	"github.com/mitchellh/copystructure"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
 	"github.com/hashicorp/consul/acl"
 	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/consul/api"
 	"github.com/hashicorp/consul/sdk/testutil"
 	"github.com/hashicorp/consul/sdk/testutil/retry"
-	"github.com/mitchellh/copystructure"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 )
 
 var testACLPolicy = `
@@ -61,10 +62,23 @@ func verifyAuthorizerChain(t *testing.T, expected acl.Authorizer, actual acl.Aut
 }
 
 func resolveTokenAsync(r *ACLResolver, token string, ch chan *asyncResolutionResult) {
-	authz, err := r.ResolveToken(token)
+	_, authz, err := r.ResolveTokenToIdentityAndAuthorizer(token)
 	ch <- &asyncResolutionResult{authz: authz, err: err}
 }
 
+// Deprecated: use resolveToken or ACLResolver.ResolveTokenToIdentityAndAuthorizer instead
+func (r *ACLResolver) ResolveToken(token string) (acl.Authorizer, error) {
+	_, authz, err := r.ResolveTokenToIdentityAndAuthorizer(token)
+	return authz, err
+}
+
+func resolveToken(t *testing.T, r *ACLResolver, token string) acl.Authorizer {
+	t.Helper()
+	_, authz, err := r.ResolveTokenToIdentityAndAuthorizer(token)
+	require.NoError(t, err)
+	return authz
+}
+
 func testIdentityForToken(token string) (bool, structs.ACLIdentity, error) {
 	switch token {
 	case "missing-policy":
@@ -1743,57 +1757,50 @@ func testACLResolver_variousTokens(t *testing.T, delegate *ACLResolverTestDelega
 	})
 
 	runTwiceAndReset("Missing Policy", func(t *testing.T) {
-		authz, err := r.ResolveToken("missing-policy")
-		require.NoError(t, err)
+		authz := resolveToken(t, r, "missing-policy")
 		require.NotNil(t, authz)
 		require.Equal(t, acl.Allow, authz.ACLRead(nil))
 		require.Equal(t, acl.Deny, authz.NodeWrite("foo", nil))
 	})
 
 	runTwiceAndReset("Missing Role", func(t *testing.T) {
-		authz, err := r.ResolveToken("missing-role")
-		require.NoError(t, err)
+		authz := resolveToken(t, r, "missing-role")
 		require.NotNil(t, authz)
 		require.Equal(t, acl.Allow, authz.ACLRead(nil))
 		require.Equal(t, acl.Deny, authz.NodeWrite("foo", nil))
 	})
 
 	runTwiceAndReset("Missing Policy on Role", func(t *testing.T) {
-		authz, err := r.ResolveToken("missing-policy-on-role")
-		require.NoError(t, err)
+		authz := resolveToken(t, r, "missing-policy-on-role")
 		require.NotNil(t, authz)
 		require.Equal(t, acl.Allow, authz.ACLRead(nil))
 		require.Equal(t, acl.Deny, authz.NodeWrite("foo", nil))
 	})
 
 	runTwiceAndReset("Normal with Policy", func(t *testing.T) {
-		authz, err := r.ResolveToken("found")
+		authz := resolveToken(t, r, "found")
 		require.NotNil(t, authz)
-		require.NoError(t, err)
 		require.Equal(t, acl.Deny, authz.ACLRead(nil))
 		require.Equal(t, acl.Allow, authz.NodeWrite("foo", nil))
 	})
 
 	runTwiceAndReset("Normal with Role", func(t *testing.T) {
-		authz, err := r.ResolveToken("found-role")
+		authz := resolveToken(t, r, "found-role")
 		require.NotNil(t, authz)
-		require.NoError(t, err)
 		require.Equal(t, acl.Deny, authz.ACLRead(nil))
 		require.Equal(t, acl.Allow, authz.NodeWrite("foo", nil))
 	})
 
 	runTwiceAndReset("Normal with Policy and Role", func(t *testing.T) {
-		authz, err := r.ResolveToken("found-policy-and-role")
+		authz := resolveToken(t, r, "found-policy-and-role")
 		require.NotNil(t, authz)
-		require.NoError(t, err)
 		require.Equal(t, acl.Deny, authz.ACLRead(nil))
 		require.Equal(t, acl.Allow, authz.NodeWrite("foo", nil))
 		require.Equal(t, acl.Allow, authz.ServiceRead("bar", nil))
 	})
 
 	runTwiceAndReset("Role With Node Identity", func(t *testing.T) {
-		authz, err := r.ResolveToken("found-role-node-identity")
-		require.NoError(t, err)
+		authz := resolveToken(t, r, "found-role-node-identity")
 		require.NotNil(t, authz)
 		require.Equal(t, acl.Allow, authz.NodeWrite("test-node", nil))
 		require.Equal(t, acl.Deny, authz.NodeWrite("test-node-dc2", nil))