diff --git a/api/types/accesslist/accesslist.go b/api/types/accesslist/accesslist.go index 690643d2481a0..ace9f67a0c8f6 100644 --- a/api/types/accesslist/accesslist.go +++ b/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 } diff --git a/api/types/accesslist/accesslist_test.go b/api/types/accesslist/accesslist_test.go index b0f58300c8338..4f2e92594a139 100644 --- a/api/types/accesslist/accesslist_test.go +++ b/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, diff --git a/lib/services/local/access_list.go b/lib/services/local/access_list.go index 312dabe45ae55..6a73bab7fe3a0 100644 --- a/lib/services/local/access_list.go +++ b/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 { diff --git a/lib/services/local/access_list_test.go b/lib/services/local/access_list_test.go index ff39a128bf5c8..aaee4487e76a2 100644 --- a/lib/services/local/access_list_test.go +++ b/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) {