client: fix role permission issue with duplicate policies.

client/acl.go

@@ -7,6 +7,7 @@ import (
 	"time"
 
 	"github.com/armon/go-metrics"
+	"github.com/hashicorp/go-set"
 	"github.com/hashicorp/nomad/acl"
 	"github.com/hashicorp/nomad/nomad/structs"
 )
@@ -252,11 +253,6 @@ func (c *Client) resolvePolicies(secretID string, policies []string) ([]*structs
 func (c *Client) resolveTokenACLRoles(secretID string, roleLinks []*structs.ACLTokenRoleLink) ([]string, error) {
 
 	var (
-		// policyNames tracks the resolved ACL policies which are linked to the
-		// role. This is the output object and represents the authorisation
-		// this role provides token bearers.
-		policyNames []string
-
 		// missingRoleIDs are the roles linked which are not found within our
 		// cache. These must be looked up from the server via and RPC, so we
 		// can correctly identify the policy links.
@@ -268,6 +264,11 @@ func (c *Client) resolveTokenACLRoles(secretID string, roleLinks []*structs.ACLT
 		expiredRoleIDs []string
 	)
 
+	// policyNames tracks the resolved ACL policies which are linked to the
+	// role as a deduplicated list. This is the output object and represents
+	// the authorisation this role provides token bearers.
+	policyNames := set.New[string](0)
+
 	for _, roleLink := range roleLinks {
 
 		// Look within the cache to see if the role is already present. If we
@@ -284,7 +285,7 @@ func (c *Client) resolveTokenACLRoles(secretID string, roleLinks []*structs.ACLT
 		// each policy name to our return object tracking.
 		if entry.Age() <= c.GetConfig().ACLRoleTTL {
 			for _, policyLink := range entry.Get().Policies {
-				policyNames = append(policyNames, policyLink.Name)
+				policyNames.Insert(policyLink.Name)
 			}
 		} else {
 			expiredRoleIDs = append(expiredRoleIDs, entry.Get().ID)
@@ -295,7 +296,7 @@ func (c *Client) resolveTokenACLRoles(secretID string, roleLinks []*structs.ACLT
 	// generate a list of linked policy names. Therefore, we can avoid making
 	// any RPC calls.
 	if len(missingRoleIDs)+len(expiredRoleIDs) == 0 {
-		return policyNames, nil
+		return policyNames.Slice(), nil
 	}
 
 	// Created a combined list of role IDs that we need to lookup from server
@@ -325,10 +326,10 @@ func (c *Client) resolveTokenACLRoles(secretID string, roleLinks []*structs.ACLT
 			c.logger.Warn("failed to resolve ACL roles, using expired cached value", "error", err)
 			for _, aclRole := range roleByIDResp.ACLRoles {
 				for _, rolePolicyLink := range aclRole.Policies {
-					policyNames = append(policyNames, rolePolicyLink.Name)
+					policyNames.Insert(rolePolicyLink.Name)
 				}
 			}
-			return policyNames, nil
+			return policyNames.Slice(), nil
 		}
 		return nil, err
 	}
@@ -345,9 +346,9 @@ func (c *Client) resolveTokenACLRoles(secretID string, roleLinks []*structs.ACLT
 		// Iterate the role policy links, extracting the name and adding this
 		// to our return response tracking.
 		for _, rolePolicyLink := range aclRole.Policies {
-			policyNames = append(policyNames, rolePolicyLink.Name)
+			policyNames.Insert(rolePolicyLink.Name)
 		}
 	}
 
-	return policyNames, nil
+	return policyNames.Slice(), nil
 }

client/acl_test.go

@@ -131,13 +131,13 @@ func TestClient_resolveTokenACLRoles(t *testing.T) {
 	defer cleanup()
 
 	// Create an ACL Role and a client token which is linked to this.
-	mockACLRole := mock.ACLRole()
+	mockACLRole1 := mock.ACLRole()
 
 	mockACLToken := mock.ACLToken()
 	mockACLToken.Policies = []string{}
-	mockACLToken.Roles = []*structs.ACLTokenRoleLink{{ID: mockACLRole.ID}}
+	mockACLToken.Roles = []*structs.ACLTokenRoleLink{{ID: mockACLRole1.ID}}
 
-	err := testServer.State().UpsertACLRoles(structs.MsgTypeTestSetup, 10, []*structs.ACLRole{mockACLRole}, true)
+	err := testServer.State().UpsertACLRoles(structs.MsgTypeTestSetup, 10, []*structs.ACLRole{mockACLRole1}, true)
 	must.NoError(t, err)
 	err = testServer.State().UpsertACLTokens(structs.MsgTypeTestSetup, 20, []*structs.ACLToken{mockACLToken})
 	must.NoError(t, err)
@@ -150,12 +150,32 @@ func TestClient_resolveTokenACLRoles(t *testing.T) {
 	// Test the cache directly and check that the ACL role previously queried
 	// is now cached.
 	must.Eq(t, 1, testClient.roleCache.Len())
-	must.True(t, testClient.roleCache.Contains(mockACLRole.ID))
+	must.True(t, testClient.roleCache.Contains(mockACLRole1.ID))
 
 	// Resolve the roles again to check we get the same results.
 	resolvedRoles2, err := testClient.resolveTokenACLRoles(rootACLToken.SecretID, mockACLToken.Roles)
 	must.NoError(t, err)
 	must.SliceContainsAll(t, resolvedRoles1, resolvedRoles2)
+
+	// Create another ACL role which will have the same ACL policy links as the
+	// previous
+	mockACLRole2 := mock.ACLRole()
+	must.NoError(t,
+		testServer.State().UpsertACLRoles(
+			structs.MsgTypeTestSetup, 30, []*structs.ACLRole{mockACLRole2}, true))
+
+	// Update the ACL token so that it links to two ACL roles, which include
+	// duplicate ACL policies.
+	mockACLToken.Roles = append(mockACLToken.Roles, &structs.ACLTokenRoleLink{ID: mockACLRole2.ID})
+	must.NoError(t,
+		testServer.State().UpsertACLTokens(
+			structs.MsgTypeTestSetup, 40, []*structs.ACLToken{mockACLToken}))
+
+	// Ensure when resolving the ACL token, we are returned a deduplicated list
+	// of ACL policy names.
+	resolvedRoles3, err := testClient.resolveTokenACLRoles(rootACLToken.SecretID, mockACLToken.Roles)
+	must.NoError(t, err)
+	must.SliceContainsAll(t, []string{"mocked-test-policy-1", "mocked-test-policy-2"}, resolvedRoles3)
 }
 
 func TestClient_ACL_ResolveToken_Disabled(t *testing.T) {