[v14] Prevent duplicate Access List owners.

api/types/accesslist/accesslist.go

@@ -157,12 +157,6 @@ func (a *AccessList) CheckAndSetDefaults() error {
 		return trace.BadParameter("owners are missing")
 	}
 
-	for _, owner := range a.Spec.Owners {
-		if owner.Name == "" {
-			return trace.BadParameter("owner name is missing")
-		}
-	}
-
 	if a.Spec.Audit.Frequency == 0 {
 		return trace.BadParameter("audit frequency must be greater than 0")
 	}
@@ -173,6 +167,25 @@ func (a *AccessList) CheckAndSetDefaults() error {
 		return trace.BadParameter("grants must specify at least one role or trait")
 	}
 
+	// Deduplicate owners. The backend will currently prevent this, but it's possible that access lists
+	// were created with duplicated owners before the backend checked for duplicate owners. In order to
+	// ensure that these access lists are backwards compatible, we'll deduplicate them here.
+	ownerMap := make(map[string]struct{}, len(a.Spec.Owners))
+	deduplicatedOwners := []Owner{}
+	for _, owner := range a.Spec.Owners {
+		if owner.Name == "" {
+			return trace.BadParameter("owner name is missing")
+		}
+
+		if _, ok := ownerMap[owner.Name]; ok {
+			continue
+		}
+
+		ownerMap[owner.Name] = struct{}{}
+		deduplicatedOwners = append(deduplicatedOwners, owner)
+	}
+	a.Spec.Owners = deduplicatedOwners
+
 	return nil
 }
 

api/types/accesslist/accesslist_test.go

@@ -22,8 +22,67 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/require"
+
+	"github.com/gravitational/teleport/api/types/header"
 )
 
+func TestDeduplicateOwners(t *testing.T) {
+	accessList, err := NewAccessList(
+		header.Metadata{
+			Name: "duplicate test",
+		},
+		Spec{
+			Title:       "title",
+			Description: "test access list",
+			Owners: []Owner{
+				{
+					Name:        "test-user1",
+					Description: "test user 1",
+				},
+				{
+					Name:        "test-user2",
+					Description: "test user 2",
+				},
+				{
+					Name:        "test-user2",
+					Description: "duplicate",
+				},
+			},
+			Audit: Audit{
+				Frequency: time.Hour,
+			},
+			MembershipRequires: Requires{
+				Roles: []string{"mrole1", "mrole2"},
+				Traits: map[string][]string{
+					"mtrait1": {"mvalue1", "mvalue2"},
+					"mtrait2": {"mvalue3", "mvalue4"},
+				},
+			},
+			OwnershipRequires: Requires{
+				Roles: []string{"orole1", "orole2"},
+				Traits: map[string][]string{
+					"otrait1": {"ovalue1", "ovalue2"},
+					"otrait2": {"ovalue3", "ovalue4"},
+				},
+			},
+			Grants: Grants{
+				Roles: []string{"grole1", "grole2"},
+				Traits: map[string][]string{
+					"gtrait1": {"gvalue1", "gvalue2"},
+					"gtrait2": {"gvalue3", "gvalue4"},
+				},
+			},
+		},
+	)
+	require.NoError(t, err)
+
+	require.Len(t, accessList.Spec.Owners, 2)
+	require.Equal(t, "test-user1", accessList.Spec.Owners[0].Name)
+	require.Equal(t, "test user 1", accessList.Spec.Owners[0].Description)
+	require.Equal(t, "test-user2", accessList.Spec.Owners[1].Name)
+	require.Equal(t, "test user 2", accessList.Spec.Owners[1].Description)
+}
+
 func TestAuditMarshaling(t *testing.T) {
 	audit := Audit{
 		Frequency:     time.Hour,

lib/services/local/access_list.go

@@ -112,6 +112,13 @@ func (a *AccessListService) GetAccessList(ctx context.Context, name string) (*ac
 // UpsertAccessList creates or updates an access list resource.
 func (a *AccessListService) UpsertAccessList(ctx context.Context, accessList *accesslist.AccessList) (*accesslist.AccessList, error) {
 	err := a.service.RunWhileLocked(ctx, lockName(accessList.GetName()), accessListLockTTL, func(ctx context.Context, _ backend.Backend) error {
+		ownerMap := make(map[string]struct{}, len(accessList.Spec.Owners))
+		for _, owner := range accessList.Spec.Owners {
+			if _, ok := ownerMap[owner.Name]; ok {
+				return trace.AlreadyExists("owner %s already exists in the owner list", owner.Name)
+			}
+			ownerMap[owner.Name] = struct{}{}
+		}
 		return trace.Wrap(a.service.UpsertResource(ctx, accessList))
 	})
 	if err != nil {

lib/services/local/access_list_test.go

@@ -124,6 +124,42 @@ func TestAccessListCRUD(t *testing.T) {
 	out, err = service.GetAccessLists(ctx)
 	require.NoError(t, err)
 	require.Empty(t, out)
+
+	// Try to create an access list with duplicate owners.
+	accessListDuplicateOwners := newAccessList(t, "accessListDuplicateOwners")
+	accessListDuplicateOwners.Spec.Owners = append(accessListDuplicateOwners.Spec.Owners, accessListDuplicateOwners.Spec.Owners[0])
+
+	_, err = service.UpsertAccessList(ctx, accessListDuplicateOwners)
+	require.True(t, trace.IsAlreadyExists(err))
+}
+
+func TestAccessListDedupeOwnersBackwardsCompat(t *testing.T) {
+	ctx := context.Background()
+	clock := clockwork.NewFakeClock()
+
+	mem, err := memory.New(memory.Config{
+		Context: ctx,
+		Clock:   clock,
+	})
+	require.NoError(t, err)
+
+	service, err := NewAccessListService(backend.NewSanitizer(mem), clock)
+	require.NoError(t, err)
+
+	// Put an unduplicated owners access list in the backend.
+	accessListDuplicateOwners := newAccessList(t, "accessListDuplicateOwners")
+	accessListDuplicateOwners.Spec.Owners = append(accessListDuplicateOwners.Spec.Owners, accessListDuplicateOwners.Spec.Owners[0])
+	require.Len(t, accessListDuplicateOwners.Spec.Owners, 3)
+
+	item, err := service.service.MakeBackendItem(accessListDuplicateOwners, accessListDuplicateOwners.GetName())
+	require.NoError(t, err)
+	_, err = mem.Put(ctx, item)
+	require.NoError(t, err)
+
+	accessList, err := service.GetAccessList(ctx, accessListDuplicateOwners.GetName())
+	require.NoError(t, err)
+
+	require.Len(t, accessList.Spec.Owners, 2)
 }
 
 func TestAccessListUpsertWithMembers(t *testing.T) {