From d8f31bf2e4783f910cb9bfa9bdea54459255ba4f Mon Sep 17 00:00:00 2001 From: Lorenzo Affetti Date: Thu, 2 Apr 2020 11:01:44 +0200 Subject: [PATCH] fix(tenant): make storage pass tenant tests --- tenant/error.go | 47 ++++ tenant/service_bucket.go | 2 + tenant/service_test.go | 71 ++++++ tenant/service_urm_test.go | 36 ++- tenant/storage_bucket.go | 2 +- tenant/storage_urm.go | 8 + tenant/storage_urm_test.go | 48 +++- tenant/storage_user.go | 20 +- testing/tenant.go | 278 +++++++++++++++-------- testing/user_resource_mapping_service.go | 131 +++++++---- 10 files changed, 488 insertions(+), 155 deletions(-) diff --git a/tenant/error.go b/tenant/error.go index cf54cf8da4..6732481f31 100644 --- a/tenant/error.go +++ b/tenant/error.go @@ -1,6 +1,9 @@ package tenant import ( + "fmt" + "strings" + "github.com/influxdata/influxdb/v2" ) @@ -42,3 +45,47 @@ func ErrInternalServiceError(err error) *influxdb.Error { Err: err, } } + +type errSlice []error + +func (e errSlice) Error() string { + l := len(e) + sb := strings.Builder{} + for i, err := range e { + if i > 0 { + sb.WriteRune('\n') + } + sb.WriteString(fmt.Sprintf("error %d/%d: %s", i+1, l, err.Error())) + } + return sb.String() +} + +// AggregateError enables composing multiple errors. +// This is ideal in the case that you are applying functions with side effects to a slice of elements. +// E.g., deleting/updating a slice of resources. +type AggregateError struct { + errs errSlice +} + +// NewAggregateError returns a new AggregateError. +func NewAggregateError() *AggregateError { + return &AggregateError{ + errs: make([]error, 0), + } +} + +// Add adds an error to the aggregate. +func (e *AggregateError) Add(err error) { + if err == nil { + return + } + e.errs = append(e.errs, err) +} + +// Err returns a proper error from this aggregate error. +func (e *AggregateError) Err() error { + if len(e.errs) > 0 { + return e.errs + } + return nil +} diff --git a/tenant/service_bucket.go b/tenant/service_bucket.go index e6cc2c7f9a..1e411b1813 100644 --- a/tenant/service_bucket.go +++ b/tenant/service_bucket.go @@ -108,6 +108,8 @@ func (s *Service) FindBuckets(ctx context.Context, filter influxdb.BucketFilter, return nil, 0, err } + // NOTE: this is a remnant of the old system. + // There are org that do not have system buckets stored, but still need to be displayed. needsSystemBuckets := true for _, b := range buckets { if b.Type == influxdb.BucketTypeSystem { diff --git a/tenant/service_test.go b/tenant/service_test.go index a712ecf486..e31fd5f22e 100644 --- a/tenant/service_test.go +++ b/tenant/service_test.go @@ -7,9 +7,12 @@ import ( "os" "testing" + "github.com/influxdata/influxdb/v2" "github.com/influxdata/influxdb/v2/bolt" "github.com/influxdata/influxdb/v2/inmem" "github.com/influxdata/influxdb/v2/kv" + "github.com/influxdata/influxdb/v2/tenant" + influxdbtesting "github.com/influxdata/influxdb/v2/testing" "go.uber.org/zap/zaptest" ) @@ -37,3 +40,71 @@ func NewTestBoltStore(t *testing.T) (kv.Store, func(), error) { func NewTestInmemStore(t *testing.T) (kv.Store, func(), error) { return inmem.NewKVStore(), func() {}, nil } + +func TestBoltTenantService(t *testing.T) { + influxdbtesting.TenantService(t, initBoltTenantService) +} + +func initBoltTenantService(t *testing.T, f influxdbtesting.TenantFields) (influxdb.TenantService, func()) { + s, closeBolt, err := NewTestBoltStore(t) + if err != nil { + t.Fatalf("failed to create new kv store: %v", err) + } + storage, err := tenant.NewStore(s) + if err != nil { + t.Fatal(err) + } + svc := tenant.NewService(storage) + + for _, u := range f.Users { + if err := svc.CreateUser(context.Background(), u); err != nil { + t.Fatalf("error populating users: %v", err) + } + } + + for i := range f.Passwords { + if err := svc.SetPassword(context.Background(), f.Users[i].ID, f.Passwords[i]); err != nil { + t.Fatalf("error setting passsword user, %s %s: %v", f.Users[i].Name, f.Passwords[i], err) + } + } + + for _, o := range f.Organizations { + if err := svc.CreateOrganization(context.Background(), o); err != nil { + t.Fatalf("failed to populate organizations: %s", err) + } + } + + for _, b := range f.Buckets { + if err := svc.CreateBucket(context.Background(), b); err != nil { + t.Fatalf("failed to populate buckets: %s", err) + } + } + + for _, m := range f.UserResourceMappings { + if err := svc.CreateUserResourceMapping(context.Background(), m); err != nil { + t.Fatalf("failed to populate mappings: %v", err) + } + } + + return svc, func() { + for _, u := range f.Users { + if err := svc.DeleteUser(context.Background(), u.ID); err != nil { + t.Logf("error removing users: %v", err) + } + } + + for _, m := range f.UserResourceMappings { + if err := svc.DeleteUserResourceMapping(context.Background(), m.ResourceID, m.UserID); err != nil { + t.Logf("failed to remove user resource mapping: %v", err) + } + } + + for _, o := range f.Organizations { + if err := svc.DeleteOrganization(context.Background(), o.ID); err != nil { + t.Logf("failed to remove organization: %v", err) + } + } + + closeBolt() + } +} diff --git a/tenant/service_urm_test.go b/tenant/service_urm_test.go index 2361c8029b..72a0c9fa93 100644 --- a/tenant/service_urm_test.go +++ b/tenant/service_urm_test.go @@ -34,17 +34,51 @@ func initUserResourceMappingService(s kv.Store, f influxdbtesting.UserResourceFi } svc := tenant.NewService(storage) + // Create resources before mappings. + + for _, u := range f.Users { + if err := svc.CreateUser(context.Background(), u); err != nil { + t.Fatalf("error populating users: %v", err) + } + } + + for _, o := range f.Organizations { + if err := svc.CreateOrganization(context.Background(), o); err != nil { + t.Fatalf("failed to populate organizations: %s", err) + } + } + + for _, b := range f.Buckets { + if err := svc.CreateBucket(context.Background(), b); err != nil { + t.Fatalf("failed to populate buckets: %s", err) + } + } + + // Now create mappings. + for _, m := range f.UserResourceMappings { if err := svc.CreateUserResourceMapping(context.Background(), m); err != nil { - t.Fatalf("failed to populate mappings") + t.Fatalf("failed to populate mappings: %v", err) } } return svc, func() { + for _, u := range f.Users { + if err := svc.DeleteUser(context.Background(), u.ID); err != nil { + t.Logf("error removing users: %v", err) + } + } + for _, m := range f.UserResourceMappings { if err := svc.DeleteUserResourceMapping(context.Background(), m.ResourceID, m.UserID); err != nil { t.Logf("failed to remove user resource mapping: %v", err) } } + + for _, o := range f.Organizations { + if err := svc.DeleteOrganization(context.Background(), o.ID); err != nil { + t.Logf("failed to remove organization: %v", err) + } + } } } diff --git a/tenant/storage_bucket.go b/tenant/storage_bucket.go index 93292bad41..d0170975c9 100644 --- a/tenant/storage_bucket.go +++ b/tenant/storage_bucket.go @@ -25,7 +25,7 @@ func bucketIndexKey(o influxdb.ID, name string) ([]byte, error) { } k := make([]byte, influxdb.IDLength+len(name)) copy(k, orgID) - copy(k[influxdb.IDLength:], []byte(name)) + copy(k[influxdb.IDLength:], name) return k, nil } diff --git a/tenant/storage_urm.go b/tenant/storage_urm.go index 4c725b589c..c50eba18ba 100644 --- a/tenant/storage_urm.go +++ b/tenant/storage_urm.go @@ -13,7 +13,15 @@ var ( urmByUserIndexBucket = []byte("userresourcemappingsbyuserindexv1") ) +// NOTE(affo): On URM creation, we check that the user exists. +// We do not check that the resource it is pointing to exists. +// This decision takes into account that different resources could not be in the same store. +// To perform that kind of check, we must rely on the service layer. +// However, we do not want having the storage layer depend on the service layer above. func (s *Store) CreateURM(ctx context.Context, tx kv.Tx, urm *influxdb.UserResourceMapping) error { + if _, err := s.GetUser(ctx, tx, urm.UserID); err != nil { + return err + } if err := s.uniqueUserResourceMapping(ctx, tx, urm); err != nil { return err } diff --git a/tenant/storage_urm_test.go b/tenant/storage_urm_test.go index 622ba6547e..fce70cd9dc 100644 --- a/tenant/storage_urm_test.go +++ b/tenant/storage_urm_test.go @@ -2,6 +2,7 @@ package tenant_test import ( "context" + "fmt" "reflect" "sort" "testing" @@ -20,8 +21,17 @@ func TestURM(t *testing.T) { simpleSetup := func(t *testing.T, store *tenant.Store, tx kv.Tx) { for i := 1; i <= 10; i++ { - err := store.CreateURM(context.Background(), tx, &influxdb.UserResourceMapping{ - UserID: influxdb.ID(i + 1), + // User must exist to create urm. + uid := influxdb.ID(i + 1) + err := store.CreateUser(context.Background(), tx, &influxdb.User{ + ID: uid, + Name: fmt.Sprintf("user%d", i), + }) + if err != nil { + t.Fatal(err) + } + err = store.CreateURM(context.Background(), tx, &influxdb.UserResourceMapping{ + UserID: uid, UserType: influxdb.Owner, MappingType: influxdb.UserMappingType, ResourceType: influxdb.OrgsResourceType, @@ -49,7 +59,7 @@ func TestURM(t *testing.T) { } if len(urms) != 10 { - t.Fatalf("ten records are created and we recieved %d", len(urms)) + t.Fatalf("ten records are created and we received %d", len(urms)) } var expected []*influxdb.UserResourceMapping for i := 1; i <= 10; i++ { @@ -74,6 +84,36 @@ func TestURM(t *testing.T) { } }, }, + { + name: "create - user not found", + setup: simpleSetup, + results: func(t *testing.T, store *tenant.Store, tx kv.Tx) { + users, err := store.ListUsers(context.Background(), tx) + if err != nil { + t.Fatal(err) + } + + maxID := influxdb.ID(0) + for _, u := range users { + if u.ID > maxID { + maxID = u.ID + } + } + + err = store.CreateURM(context.Background(), tx, &influxdb.UserResourceMapping{ + UserID: maxID + 1, + UserType: influxdb.Owner, + MappingType: influxdb.UserMappingType, + ResourceType: influxdb.OrgsResourceType, + ResourceID: influxdb.ID(1), + }) + if err == nil { + t.Fatal("expected error got none") + } else if influxdb.ErrorCode(err) != influxdb.ENotFound { + t.Fatalf("expected not found error got: %v", err) + } + }, + }, { name: "get", setup: simpleSetup, @@ -110,7 +150,7 @@ func TestURM(t *testing.T) { } if len(urms) != 10 { - t.Fatalf("ten records are created and we recieved %d", len(urms)) + t.Fatalf("ten records are created and we received %d", len(urms)) } var expected []*influxdb.UserResourceMapping for i := 1; i <= 10; i++ { diff --git a/tenant/storage_user.go b/tenant/storage_user.go index d3f3a430c6..b64357ac2a 100644 --- a/tenant/storage_user.go +++ b/tenant/storage_user.go @@ -270,13 +270,29 @@ func (s *Store) DeleteUser(ctx context.Context, tx kv.Tx, id influxdb.ID) error return ErrInternalServiceError(err) } - // clean up users password + // Clean up users password. ub, err := tx.Bucket(userpasswordBucket) if err != nil { return UnavailablePasswordServiceError(err) } + if err := ub.Delete(encodedID); err != nil { + return err + } - return ub.Delete(encodedID) + // Clean up user URMs. + urms, err := s.ListURMs(ctx, tx, influxdb.UserResourceMappingFilter{UserID: id}) + if err != nil { + return err + } + // Do not fail fast on error. + // Try to avoid as much as possible the effects of partial deletion. + aggErr := NewAggregateError() + for _, urm := range urms { + if err := s.DeleteURM(ctx, tx, urm.ResourceID, urm.UserID); err != nil { + aggErr.Add(err) + } + } + return aggErr.Err() } func (s *Store) GetPassword(ctx context.Context, tx kv.Tx, id influxdb.ID) (string, error) { diff --git a/testing/tenant.go b/testing/tenant.go index 9f5c5f7586..8350599aa3 100644 --- a/testing/tenant.go +++ b/testing/tenant.go @@ -158,6 +158,13 @@ func Create(t *testing.T, init func(*testing.T, TenantFields) (influxdb.TenantSe defer done() ctx := context.Background() + // Number of buckets prior to user creation. + // This is because, for now, system buckets always get returned for compatibility with the old system. + _, nbs, err := s.FindBuckets(ctx, influxdb.BucketFilter{}) + if err != nil { + t.Fatal(err) + } + u := &influxdb.User{ ID: 1, Name: "user1", @@ -188,25 +195,76 @@ func Create(t *testing.T, init func(*testing.T, TenantFields) (influxdb.TenantSe if nurms > 0 { t.Errorf("expected no urm, got: %v", urms) } - bs, nbs, err := s.FindBuckets(ctx, influxdb.BucketFilter{}) + bs, nnbs, err := s.FindBuckets(ctx, influxdb.BucketFilter{}) if err != nil { t.Fatal(err) } - if nbs != 0 { - t.Errorf("expected no buckets, got: %+v", bs) + // Compare new number of buckets with the one prior to user creation. + if nnbs != nbs { + t.Errorf("expected no bucket created, got: %+v", bs) } }) // NOTE(affo)(*kv.Service): nope, it does create a useless URM, no existence check. // Apparently, system buckets are created too :thinking. - t.Run("creating urm pointing to non existing user/org fails", func(t *testing.T) { + t.Run("creating urm pointing to non existing user fails", func(t *testing.T) { data := fields() s, done := init(t, data) defer done() ctx := context.Background() + // First create an org and a user. + u := &influxdb.User{ + ID: 1, + Name: "user1", + } + if err := s.CreateUser(ctx, u); err != nil { + t.Fatal(err) + } + o := &influxdb.Organization{ + ID: 1, + Name: "org1", + } + if err := s.CreateOrganization(ctx, o); err != nil { + t.Fatal(err) + } + + checkInvariance := func(nurms int) { + orgs, norgs, err := s.FindOrganizations(ctx, influxdb.OrganizationFilter{}) + if err != nil { + t.Fatal(err) + } + if norgs != 1 { + t.Errorf("expected 1 org, got: %v", orgs) + } + usrs, nusrs, err := s.FindUsers(ctx, influxdb.UserFilter{}) + if err != nil { + t.Fatal(err) + } + if nusrs != 1 { + t.Errorf("expected 1 user, got: %v", usrs) + } + urms, nnurms, err := s.FindUserResourceMappings(ctx, influxdb.UserResourceMappingFilter{}) + if err != nil { + t.Fatal(err) + } + if nnurms != nurms { + t.Errorf("expected %d urms got %d: %+v", nurms, nnurms, urms) + } + bs, nbs, err := s.FindBuckets(ctx, influxdb.BucketFilter{}) + if err != nil { + t.Fatal(err) + } + if nbs != 2 { + t.Errorf("expected 2 buckets, got: %v", bs) + } + } + + checkInvariance(0) + + // Wrong userID. urm := &influxdb.UserResourceMapping{ - UserID: 1, + UserID: 2, UserType: influxdb.Owner, MappingType: influxdb.UserMappingType, ResourceType: influxdb.OrgsResourceType, @@ -216,35 +274,21 @@ func Create(t *testing.T, init func(*testing.T, TenantFields) (influxdb.TenantSe t.Errorf("expected error got none") } - // Check existence - orgs, norgs, err := s.FindOrganizations(ctx, influxdb.OrganizationFilter{}) - if err != nil { - t.Fatal(err) + checkInvariance(0) + + // Wrong orgID. The URM gets created successfully. + urm = &influxdb.UserResourceMapping{ + UserID: 1, + UserType: influxdb.Owner, + MappingType: influxdb.UserMappingType, + ResourceType: influxdb.OrgsResourceType, + ResourceID: 2, } - if norgs != 0 { - t.Errorf("expected no org, got: %v", orgs) - } - usrs, nusrs, err := s.FindUsers(ctx, influxdb.UserFilter{}) - if err != nil { - t.Fatal(err) - } - if nusrs != 0 { - t.Errorf("expected no user, got: %v", usrs) - } - urms, nurms, err := s.FindUserResourceMappings(ctx, influxdb.UserResourceMappingFilter{}) - if err != nil { - t.Fatal(err) - } - if nurms > 0 { - t.Errorf("expected no urm, got: %v", urms) - } - bs, nbs, err := s.FindBuckets(ctx, influxdb.BucketFilter{}) - if err != nil { - t.Fatal(err) - } - if nbs != 0 { - t.Errorf("expected no buckets, got: %v", bs) + if err := s.CreateUserResourceMapping(ctx, urm); err != nil { + t.Errorf("unexpected error: %v", err) } + + checkInvariance(1) }) // NOTE(affo)(*kv.Service): errors on bucket creation. @@ -255,6 +299,13 @@ func Create(t *testing.T, init func(*testing.T, TenantFields) (influxdb.TenantSe defer done() ctx := context.Background() + // Number of buckets prior to bucket creation. + // This is because, for now, system buckets always get returned for compatibility with the old system. + _, nbs, err := s.FindBuckets(ctx, influxdb.BucketFilter{}) + if err != nil { + t.Fatal(err) + } + b := &influxdb.Bucket{ ID: 1, OrgID: 1, @@ -286,16 +337,17 @@ func Create(t *testing.T, init func(*testing.T, TenantFields) (influxdb.TenantSe if nurms > 0 { t.Errorf("expected no urm, got: %v", urms) } - bs, nbs, err := s.FindBuckets(ctx, influxdb.BucketFilter{}) + bs, nnbs, err := s.FindBuckets(ctx, influxdb.BucketFilter{}) if err != nil { t.Fatal(err) } - if nbs != 0 { - t.Errorf("expected no buckets, got: %v", bs) + // Compare new number of buckets with the one prior to bucket creation. + if nnbs != nbs { + t.Errorf("expected bucket created, got: %+v", bs) } }) - t.Run("making user part of org creates mappings to buckets", func(t *testing.T) { + t.Run("making user part of org creates mapping to org only", func(t *testing.T) { for _, userType := range []influxdb.UserType{influxdb.Owner, influxdb.Member} { t.Run(string(userType), func(t *testing.T) { data := fields() @@ -371,20 +423,6 @@ func Create(t *testing.T, init func(*testing.T, TenantFields) (influxdb.TenantSe ResourceType: influxdb.OrgsResourceType, ResourceID: o.ID, }, - { - UserID: u.ID, - UserType: userType, - MappingType: influxdb.UserMappingType, - ResourceType: influxdb.BucketsResourceType, - ResourceID: bs[0].ID, - }, - { - UserID: u.ID, - UserType: userType, - MappingType: influxdb.UserMappingType, - ResourceType: influxdb.BucketsResourceType, - ResourceID: bs[1].ID, - }, } sort.Sort(urmByResourceID(want)) sort.Sort(urmByResourceID(urms)) @@ -394,7 +432,7 @@ func Create(t *testing.T, init func(*testing.T, TenantFields) (influxdb.TenantSe // Now add a new bucket and check the URMs. b := &influxdb.Bucket{ - ID: 1, + ID: 1000, OrgID: o.ID, Name: "bucket1", } @@ -405,6 +443,8 @@ func Create(t *testing.T, init func(*testing.T, TenantFields) (influxdb.TenantSe if err != nil { t.Fatal(err) } + // TODO(affo): this shouldn't be needed. + // This URM should not be created. want = append(want, &influxdb.UserResourceMapping{ UserID: u.ID, UserType: userType, @@ -461,6 +501,7 @@ func Delete(t *testing.T, init func(*testing.T, TenantFields) (influxdb.TenantSe }, }, UserResourceMappings: []*influxdb.UserResourceMapping{ + // NOTE(affo): bucket URMs should not be here, create them only for deletion purposes. // user 1 owns org1 (and so bucket1) { UserID: 1, @@ -469,6 +510,13 @@ func Delete(t *testing.T, init func(*testing.T, TenantFields) (influxdb.TenantSe ResourceType: influxdb.OrgsResourceType, ResourceID: 10, }, + { + UserID: 1, + UserType: influxdb.Owner, + MappingType: influxdb.UserMappingType, + ResourceType: influxdb.BucketsResourceType, + ResourceID: 100, + }, // user 1 is member of org2 (and so bucket2) { UserID: 1, @@ -477,6 +525,13 @@ func Delete(t *testing.T, init func(*testing.T, TenantFields) (influxdb.TenantSe ResourceType: influxdb.OrgsResourceType, ResourceID: 20, }, + { + UserID: 1, + UserType: influxdb.Member, + MappingType: influxdb.UserMappingType, + ResourceType: influxdb.BucketsResourceType, + ResourceID: 200, + }, // user 2 owns org2 (and so bucket2) { UserID: 2, @@ -485,7 +540,13 @@ func Delete(t *testing.T, init func(*testing.T, TenantFields) (influxdb.TenantSe ResourceType: influxdb.OrgsResourceType, ResourceID: 20, }, - // NOTE(affo): URMs to buckets are automatically created as for the tests for creation. + { + UserID: 2, + UserType: influxdb.Owner, + MappingType: influxdb.UserMappingType, + ResourceType: influxdb.BucketsResourceType, + ResourceID: 200, + }, }, } } @@ -523,8 +584,9 @@ func Delete(t *testing.T, init func(*testing.T, TenantFields) (influxdb.TenantSe } }) - // NOTE(affo)(*kv.Service): it does create dangling resources. - t.Run("deleting bucket urm does not create dangling resources", func(t *testing.T) { + // NOTE(affo): those resources could not be dangling (URM could be inferred from an user being in the owner org). + // We do not want to automatically propagate this kind of delete because an resource will always have an owner org. + t.Run("deleting bucket urm does create dangling bucket", func(t *testing.T) { data := fields() s, done := init(t, data) defer done() @@ -599,7 +661,7 @@ func Delete(t *testing.T, init func(*testing.T, TenantFields) (influxdb.TenantSe t.Errorf("expected 1 buckets, got: %v", bs) } // Now delete user1 -> bucket2. - // bucket2 should be deleted (nobody points to it). + // Still expect bucket2 to exist (nobody points to it). if err := s.DeleteUserResourceMapping(ctx, data.Buckets[1].ID, data.Users[0].ID); err != nil { t.Fatal(err) } @@ -607,19 +669,21 @@ func Delete(t *testing.T, init func(*testing.T, TenantFields) (influxdb.TenantSe if err != nil { t.Fatal(err) } - if nbs > 0 { - t.Errorf("expected no buckets, got: %v", bs) - urms, _, err := s.FindUserResourceMappings(ctx, influxdb.UserResourceMappingFilter{ - ResourceType: influxdb.BucketsResourceType, - ResourceID: data.Buckets[1].ID, - }) - if err != nil { - t.Fatal(err) - } - t.Logf("bucket2 should be dangling at this point. These are its urms: %+v", urms) + if nbs != 1 { + t.Errorf("expected 1 buckets, got: %v", bs) + } + urms, nurms, err := s.FindUserResourceMappings(ctx, influxdb.UserResourceMappingFilter{ + ResourceType: influxdb.BucketsResourceType, + ResourceID: data.Buckets[1].ID, + }) + if err != nil { + t.Fatal(err) + } + if nurms != 0 { + t.Errorf("expected bucket2, to be dangling, got: %+v", urms) } // Now delete user1 -> bucket1. - // bucket1 should be deleted (nobody points to it). + // Still expect bucket1 to exist (nobody points to it). if err := s.DeleteUserResourceMapping(ctx, data.Buckets[0].ID, data.Users[0].ID); err != nil { t.Fatal(err) } @@ -627,21 +691,22 @@ func Delete(t *testing.T, init func(*testing.T, TenantFields) (influxdb.TenantSe if err != nil { t.Fatal(err) } - if nbs > 0 { - t.Errorf("expected no buckets, got: %v", bs) - urms, _, err := s.FindUserResourceMappings(ctx, influxdb.UserResourceMappingFilter{ - ResourceType: influxdb.BucketsResourceType, - ResourceID: data.Buckets[1].ID, - }) - if err != nil { - t.Fatal(err) - } - t.Logf("bucket1 should be dangling at this point. These are its urms: %+v", urms) + if nbs != 1 { + t.Errorf("expected 1 buckets, got: %v", bs) + } + urms, nurms, err = s.FindUserResourceMappings(ctx, influxdb.UserResourceMappingFilter{ + ResourceType: influxdb.BucketsResourceType, + ResourceID: data.Buckets[0].ID, + }) + if err != nil { + t.Fatal(err) + } + if nurms != 0 { + t.Errorf("expected bucket1, to be dangling, got: %+v", urms) } }) - // NOTE(affo)(*kv.Service): it deletes urms, but not dangling resources. - t.Run("deleting a user deletes every urm and so for dangling resources", func(t *testing.T) { + t.Run("deleting a user deletes every related urm and nothing else", func(t *testing.T) { data := fields() s, done := init(t, data) defer done() @@ -672,7 +737,7 @@ func Delete(t *testing.T, init func(*testing.T, TenantFields) (influxdb.TenantSe } // Delete user1. - // We expect his urms deleted and bucket1 deleted (he was the only one pointing at it). + // We expect his urms deleted but not bucket1. if err := s.DeleteUser(ctx, data.Users[0].ID); err != nil { t.Fatal(err) } @@ -689,16 +754,31 @@ func Delete(t *testing.T, init func(*testing.T, TenantFields) (influxdb.TenantSe if err != nil { t.Fatal(err) } - if nbs > 0 { - t.Errorf("expected no buckets, got: %v", bs) - urms, _, err := s.FindUserResourceMappings(ctx, influxdb.UserResourceMappingFilter{ - ResourceType: influxdb.BucketsResourceType, - ResourceID: data.Buckets[1].ID, - }) - if err != nil { - t.Fatal(err) - } - t.Logf("bucket1 should be dangling at this point. These are its urms: %+v", urms) + if nbs != 1 { + t.Errorf("expected 1 buckets, got: %v", bs) + } + }) + + t.Run("deleting a bucket deletes every related urm", func(t *testing.T) { + data := fields() + s, done := init(t, data) + defer done() + ctx := context.Background() + + // Delete bucket2. + // We expect its urms deleted. + if err := s.DeleteBucket(ctx, data.Buckets[1].ID); err != nil { + t.Fatal(err) + } + urms, nurms, err := s.FindUserResourceMappings(ctx, influxdb.UserResourceMappingFilter{ + ResourceType: influxdb.BucketsResourceType, + ResourceID: data.Buckets[1].ID, + }) + if err != nil { + t.Fatal(err) + } + if nurms > 0 { + t.Errorf("expected that bucket deletion would remove dangling urms, got: %+v", urms) } }) @@ -709,10 +789,14 @@ func Delete(t *testing.T, init func(*testing.T, TenantFields) (influxdb.TenantSe defer done() ctx := context.Background() + // Number of base buckets return by a find operation. + // This is because, for now, system buckets always get returned for compatibility with the old system. + const baseNBuckets = 2 + // Delete org1. // We expect its buckets to be deleted. - // We expect urms to those buckets to be deleted too - // user1 should not be deleted because he is member of org2. + // We expect urms to those buckets to be deleted too. + // No user should be deleted. preDeletionBuckets, _, err := s.FindBuckets(ctx, influxdb.BucketFilter{OrganizationID: &data.Organizations[0].ID}) if err != nil { t.Fatal(err) @@ -724,8 +808,8 @@ func Delete(t *testing.T, init func(*testing.T, TenantFields) (influxdb.TenantSe if err != nil { t.Fatal(err) } - if nbs > 0 { - t.Errorf("expected no buckets, got: %v", bs) + if nbs != baseNBuckets { + t.Errorf("expected org buckets to be deleted, got: %+v", bs) } urms, _, err := s.FindUserResourceMappings(ctx, influxdb.UserResourceMappingFilter{ @@ -762,8 +846,8 @@ func Delete(t *testing.T, init func(*testing.T, TenantFields) (influxdb.TenantSe if err != nil { t.Fatal(err) } - if nusrs != 0 { - t.Errorf("expected no user, got: %v", usrs) + if nusrs != 2 { + t.Errorf("expected 2 users, got: %v", usrs) } urms, nurms, err := s.FindUserResourceMappings(ctx, influxdb.UserResourceMappingFilter{}) if err != nil { @@ -776,8 +860,8 @@ func Delete(t *testing.T, init func(*testing.T, TenantFields) (influxdb.TenantSe if err != nil { t.Fatal(err) } - if nbs != 0 { - t.Errorf("expected no buckets, got: %v", bs) + if nbs != baseNBuckets { + t.Errorf("expected buckets to be deleted, got: %+v", bs) } }) } diff --git a/testing/user_resource_mapping_service.go b/testing/user_resource_mapping_service.go index b4a243b016..21b208c901 100644 --- a/testing/user_resource_mapping_service.go +++ b/testing/user_resource_mapping_service.go @@ -79,6 +79,23 @@ func UserResourceMappingService( } } +// baseUserResourceFields creates base fields to create URMs. +// Users for URMs must exist in order not to fail on creation. +func baseUserResourceFields() UserResourceFields { + return UserResourceFields{ + Users: []*platform.User{ + { + Name: "user1", + ID: MustIDBase16(userOneID), + }, + { + Name: "user2", + ID: MustIDBase16(userTwoID), + }, + }, + } +} + func CreateUserResourceMapping( init func(UserResourceFields, *testing.T) (platform.UserResourceMappingService, func()), t *testing.T, @@ -99,16 +116,18 @@ func CreateUserResourceMapping( }{ { name: "basic create user resource mapping", - fields: UserResourceFields{ - UserResourceMappings: []*platform.UserResourceMapping{ + fields: func() UserResourceFields { + f := baseUserResourceFields() + f.UserResourceMappings = []*platform.UserResourceMapping{ { ResourceID: MustIDBase16(bucketOneID), UserID: MustIDBase16(userOneID), UserType: platform.Member, ResourceType: platform.BucketsResourceType, }, - }, - }, + } + return f + }(), args: args{ mapping: &platform.UserResourceMapping{ ResourceID: MustIDBase16(bucketTwoID), @@ -136,28 +155,33 @@ func CreateUserResourceMapping( }, { name: "duplicate mappings are not allowed", - fields: UserResourceFields{ - UserResourceMappings: []*platform.UserResourceMapping{ + fields: func() UserResourceFields { + f := baseUserResourceFields() + f.UserResourceMappings = []*platform.UserResourceMapping{ { - ResourceID: MustIDBase16(bucketOneID), - UserID: MustIDBase16(userOneID), - UserType: platform.Member, + ResourceID: MustIDBase16(bucketOneID), + UserID: MustIDBase16(userOneID), + UserType: platform.Member, + ResourceType: platform.BucketsResourceType, }, - }, - }, + } + return f + }(), args: args{ mapping: &platform.UserResourceMapping{ - ResourceID: MustIDBase16(bucketOneID), - UserID: MustIDBase16(userOneID), - UserType: platform.Member, + ResourceID: MustIDBase16(bucketOneID), + UserID: MustIDBase16(userOneID), + UserType: platform.Member, + ResourceType: platform.BucketsResourceType, }, }, wants: wants{ mappings: []*platform.UserResourceMapping{ { - ResourceID: MustIDBase16(bucketOneID), - UserID: MustIDBase16(userOneID), - UserType: platform.Member, + ResourceID: MustIDBase16(bucketOneID), + UserID: MustIDBase16(userOneID), + UserType: platform.Member, + ResourceType: platform.BucketsResourceType, }, }, //lint:ignore ST1005 Error is capitalized in the tested code. @@ -215,15 +239,18 @@ func DeleteUserResourceMapping( }{ { name: "basic delete user resource mapping", - fields: UserResourceFields{ - UserResourceMappings: []*platform.UserResourceMapping{ + fields: func() UserResourceFields { + f := baseUserResourceFields() + f.UserResourceMappings = []*platform.UserResourceMapping{ { - ResourceID: MustIDBase16(bucketOneID), - UserID: MustIDBase16(userOneID), - UserType: platform.Member, + ResourceID: MustIDBase16(bucketOneID), + UserID: MustIDBase16(userOneID), + UserType: platform.Member, + ResourceType: platform.BucketsResourceType, }, - }, - }, + } + return f + }(), args: args{ resourceID: MustIDBase16(bucketOneID), userID: MustIDBase16(userOneID), @@ -335,8 +362,9 @@ func FindUserResourceMappings( }{ { name: "basic find mappings", - fields: UserResourceFields{ - UserResourceMappings: []*platform.UserResourceMapping{ + fields: func() UserResourceFields { + f := baseUserResourceFields() + f.UserResourceMappings = []*platform.UserResourceMapping{ { ResourceID: MustIDBase16(bucketOneID), UserID: MustIDBase16(userOneID), @@ -349,8 +377,9 @@ func FindUserResourceMappings( UserType: platform.Member, ResourceType: platform.BucketsResourceType, }, - }, - }, + } + return f + }(), args: args{ filter: platform.UserResourceMappingFilter{}, }, @@ -373,8 +402,9 @@ func FindUserResourceMappings( }, { name: "find mappings filtered by user", - fields: UserResourceFields{ - UserResourceMappings: []*platform.UserResourceMapping{ + fields: func() UserResourceFields { + f := baseUserResourceFields() + f.UserResourceMappings = []*platform.UserResourceMapping{ { ResourceID: MustIDBase16(bucketOneID), UserID: MustIDBase16(userOneID), @@ -387,8 +417,9 @@ func FindUserResourceMappings( UserType: platform.Member, ResourceType: platform.BucketsResourceType, }, - }, - }, + } + return f + }(), args: args{ filter: platform.UserResourceMappingFilter{ UserID: MustIDBase16(userOneID), @@ -407,8 +438,9 @@ func FindUserResourceMappings( }, { name: "find mappings filtered by resource", - fields: UserResourceFields{ - UserResourceMappings: []*platform.UserResourceMapping{ + fields: func() UserResourceFields { + f := baseUserResourceFields() + f.UserResourceMappings = []*platform.UserResourceMapping{ { ResourceID: MustIDBase16(bucketOneID), UserID: MustIDBase16(userOneID), @@ -421,8 +453,9 @@ func FindUserResourceMappings( UserType: platform.Member, ResourceType: platform.BucketsResourceType, }, - }, - }, + } + return f + }(), args: args{ filter: platform.UserResourceMappingFilter{ ResourceID: MustIDBase16(bucketOneID), @@ -441,8 +474,9 @@ func FindUserResourceMappings( }, { name: "find mappings filtered by user type", - fields: UserResourceFields{ - UserResourceMappings: []*platform.UserResourceMapping{ + fields: func() UserResourceFields { + f := baseUserResourceFields() + f.UserResourceMappings = []*platform.UserResourceMapping{ { ResourceID: MustIDBase16(bucketOneID), UserID: MustIDBase16(userOneID), @@ -455,8 +489,9 @@ func FindUserResourceMappings( UserType: platform.Owner, ResourceType: platform.BucketsResourceType, }, - }, - }, + } + return f + }(), args: args{ filter: platform.UserResourceMappingFilter{ UserType: platform.Owner, @@ -475,22 +510,18 @@ func FindUserResourceMappings( }, { name: "find mappings filtered by resource type", - fields: UserResourceFields{ - UserResourceMappings: []*platform.UserResourceMapping{ - { - ResourceID: MustIDBase16(bucketOneID), - UserID: MustIDBase16(userOneID), - UserType: platform.Member, - ResourceType: platform.DashboardsResourceType, - }, + fields: func() UserResourceFields { + f := baseUserResourceFields() + f.UserResourceMappings = []*platform.UserResourceMapping{ { ResourceID: MustIDBase16(bucketTwoID), UserID: MustIDBase16(userTwoID), UserType: platform.Member, ResourceType: platform.BucketsResourceType, }, - }, - }, + } + return f + }(), args: args{ filter: platform.UserResourceMappingFilter{ ResourceType: platform.BucketsResourceType,