From cc5114bdef1ed70589cffee8b2632faaf941efec Mon Sep 17 00:00:00 2001 From: Jim Date: Fri, 12 Jun 2020 15:22:23 -0400 Subject: [PATCH] refactor scopes to conform to public id usage and repo error msgs (#117) --- internal/cmd/base/servers.go | 4 +- internal/iam/public_ids.go | 11 +++++ internal/iam/public_ids_test.go | 16 +++++++ internal/iam/repository_scope.go | 44 ++++++++++++++---- internal/iam/repository_scope_test.go | 21 +++++++++ internal/iam/repository_test.go | 2 + internal/iam/scope.go | 65 ++++++++++----------------- internal/iam/scope_test.go | 43 +++++++++++++++++- 8 files changed, 152 insertions(+), 54 deletions(-) diff --git a/internal/cmd/base/servers.go b/internal/cmd/base/servers.go index 05785efcdb..a24a8bae8a 100644 --- a/internal/cmd/base/servers.go +++ b/internal/cmd/base/servers.go @@ -430,12 +430,12 @@ func (b *Server) CreateDevDatabase(dialect string) error { } } - scope, err = iam.NewOrganization(iam.WithPublicId(b.DefaultOrgId)) + scope, err = iam.NewOrganization() if err != nil { c() return fmt.Errorf("error creating new org scope: %w", err) } - scope, err = repo.CreateScope(ctx, scope) + scope, err = repo.CreateScope(ctx, scope, iam.WithPublicId(b.DefaultOrgId)) if err != nil { c() return fmt.Errorf("error persisting new org scope: %w", err) diff --git a/internal/iam/public_ids.go b/internal/iam/public_ids.go index 95ddfe4da2..9ce527cff1 100644 --- a/internal/iam/public_ids.go +++ b/internal/iam/public_ids.go @@ -26,3 +26,14 @@ func newGroupId() (string, error) { } return id, nil } + +func newScopeId(scopeType ScopeType) (string, error) { + if scopeType == UnknownScope { + return "", fmt.Errorf("new scope id: unknown is not supported %w", db.ErrInvalidParameter) + } + id, err := db.NewPublicId(scopeType.Prefix()) + if err != nil { + return "", fmt.Errorf("new %s id: %w", scopeType.String(), err) + } + return id, nil +} diff --git a/internal/iam/public_ids_test.go b/internal/iam/public_ids_test.go index 617dd9b99a..dac59b370a 100644 --- a/internal/iam/public_ids_test.go +++ b/internal/iam/public_ids_test.go @@ -1,9 +1,11 @@ package iam import ( + "errors" "strings" "testing" + "github.com/hashicorp/watchtower/internal/db" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -19,4 +21,18 @@ func Test_PublicIds(t *testing.T) { require.NoError(t, err) assert.True(t, strings.HasPrefix(id, GroupPrefix+"_")) }) + t.Run("scopes", func(t *testing.T) { + id, err := newScopeId(OrganizationScope) + require.NoError(t, err) + assert.True(t, strings.HasPrefix(id, OrganizationScope.Prefix())) + + id, err = newScopeId(ProjectScope) + require.NoError(t, err) + assert.True(t, strings.HasPrefix(id, ProjectScope.Prefix())) + + id, err = newScopeId(UnknownScope) + require.Error(t, err) + assert.Empty(t, id) + assert.True(t, errors.Is(err, db.ErrInvalidParameter)) + }) } diff --git a/internal/iam/repository_scope.go b/internal/iam/repository_scope.go index 09b457500e..357d0d9fab 100644 --- a/internal/iam/repository_scope.go +++ b/internal/iam/repository_scope.go @@ -4,18 +4,46 @@ import ( "context" "errors" "fmt" + "strings" "github.com/hashicorp/watchtower/internal/db" ) -// CreateScope will create a scope in the repository and return the written scope +// CreateScope will create a scope in the repository and return the written +// scope. Supported options include: WithPublicId. func (r *Repository) CreateScope(ctx context.Context, scope *Scope, opt ...Option) (*Scope, error) { if scope == nil { - return nil, errors.New("scope is nil for create") + return nil, fmt.Errorf("create scope: missing scope %w", db.ErrNilParameter) } - resource, err := r.create(ctx, scope) + if scope.Scope == nil { + return nil, fmt.Errorf("create scope: missing scope store %w", db.ErrNilParameter) + } + if scope.PublicId != "" { + return nil, fmt.Errorf("create scope: public id not empty: %w", db.ErrInvalidParameter) + } + opts := getOpts(opt...) + var publicId string + t := stringToScopeType(scope.Type) + if opts.withPublicId != "" { + if !strings.HasPrefix(opts.withPublicId, t.Prefix()+"_") { + return nil, fmt.Errorf("create scope: passed-in public ID %q has wrong prefix for type %q which uses prefix %q", opts.withPublicId, t.String(), t.Prefix()) + } + publicId = opts.withPublicId + } else { + var err error + publicId, err = newScopeId(t) + if err != nil { + return nil, fmt.Errorf("create scope: error generating public id %w for new scope", err) + } + } + s := scope.Clone().(*Scope) + s.PublicId = publicId + resource, err := r.create(ctx, s) if err != nil { - return nil, fmt.Errorf("failed to create scope: %w", err) + if db.IsUniqueError(err) { + return nil, fmt.Errorf("create scope: scope %s/%s already exists: %w", s.PublicId, s.Name, db.ErrNotUnique) + } + return nil, fmt.Errorf("create scope: %w for %s", err, s.PublicId) } return resource.(*Scope), nil } @@ -63,7 +91,7 @@ func (r *Repository) UpdateScope(ctx context.Context, scope *Scope, fieldMaskPat // found, it will return nil, nil. func (r *Repository) LookupScope(ctx context.Context, withPublicId string, opt ...Option) (*Scope, error) { if withPublicId == "" { - return nil, errors.New("cannot lookup a scope with an empty public id") + return nil, fmt.Errorf("lookup scope: missing public id %w", db.ErrInvalidParameter) } scope := allocScope() scope.PublicId = withPublicId @@ -71,7 +99,7 @@ func (r *Repository) LookupScope(ctx context.Context, withPublicId string, opt . if err == db.ErrRecordNotFound { return nil, nil } - return nil, fmt.Errorf("unable to lookup scope by public id %s: %w", withPublicId, err) + return nil, fmt.Errorf("lookup group: failed %w fo %s", err, withPublicId) } return &scope, nil } @@ -79,7 +107,7 @@ func (r *Repository) LookupScope(ctx context.Context, withPublicId string, opt . // DeleteScope will delete a scope from the repository func (r *Repository) DeleteScope(ctx context.Context, withPublicId string, opt ...Option) (int, error) { if withPublicId == "" { - return db.NoRowsAffected, errors.New("cannot delete a scope with an empty public id") + return db.NoRowsAffected, fmt.Errorf("delete scope: missing public id %w", db.ErrInvalidParameter) } scope := allocScope() scope.PublicId = withPublicId @@ -88,7 +116,7 @@ func (r *Repository) DeleteScope(ctx context.Context, withPublicId string, opt . if errors.Is(err, ErrMetadataScopeNotFound) { return 0, nil } - return db.NoRowsAffected, fmt.Errorf("unable to delete scope with public id %s: %w", withPublicId, err) + return db.NoRowsAffected, fmt.Errorf("delete scope: failed %w for %s", err, withPublicId) } return rowsDeleted, nil } diff --git a/internal/iam/repository_scope_test.go b/internal/iam/repository_scope_test.go index 1e33ab73e0..79702099b6 100644 --- a/internal/iam/repository_scope_test.go +++ b/internal/iam/repository_scope_test.go @@ -49,6 +49,27 @@ func Test_Repository_CreateScope(t *testing.T) { err = db.TestVerifyOplog(t, rw, s.PublicId, db.WithOperation(oplog.OpType_OP_TYPE_CREATE), db.WithCreateNotBefore(10*time.Second)) assert.NoError(err) }) + t.Run("valid-scope-withPublicId", func(t *testing.T) { + assert, require := assert.New(t), require.New(t) + rw := db.New(conn) + wrapper := db.TestWrapper(t) + repo, err := NewRepository(rw, rw, wrapper) + require.NoError(err) + publicId, err := newScopeId(OrganizationScope) + require.NoError(err) + s, err := NewOrganization() + require.NoError(err) + s, err = repo.CreateScope(context.Background(), s, WithPublicId(publicId)) + require.NoError(err) + require.NotNil(s) + assert.Equal(publicId, s.GetPublicId()) + foundScope, err := repo.LookupScope(context.Background(), s.PublicId) + require.NoError(err) + assert.True(proto.Equal(foundScope, s)) + + err = db.TestVerifyOplog(t, rw, s.PublicId, db.WithOperation(oplog.OpType_OP_TYPE_CREATE), db.WithCreateNotBefore(10*time.Second)) + assert.NoError(err) + }) t.Run("dup-org-names", func(t *testing.T) { assert, require := assert.New(t), require.New(t) rw := db.New(conn) diff --git a/internal/iam/repository_test.go b/internal/iam/repository_test.go index 8c9f7b1e1d..ea94e8a24c 100644 --- a/internal/iam/repository_test.go +++ b/internal/iam/repository_test.go @@ -122,6 +122,8 @@ func Test_Repository_create(t *testing.T) { s, err := NewOrganization(WithName("fname-" + id)) assert.NoError(err) + s.PublicId, err = newScopeId(OrganizationScope) + require.NoError(err) retScope, err := repo.create(context.Background(), s) require.NoError(err) require.NotNil(retScope) diff --git a/internal/iam/scope.go b/internal/iam/scope.go index 2e349037f9..43f57c1963 100644 --- a/internal/iam/scope.go +++ b/internal/iam/scope.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "strings" "github.com/hashicorp/watchtower/internal/db" "github.com/hashicorp/watchtower/internal/iam/store" @@ -36,6 +35,17 @@ func (s ScopeType) Prefix() string { }[s] } +func stringToScopeType(s string) ScopeType { + switch s { + case OrganizationScope.String(): + return OrganizationScope + case ProjectScope.String(): + return ProjectScope + default: + return UnknownScope + } +} + // Scope is used to create a hierarchy of "containers" that encompass the scope of // an IAM resource. Scopes are Organizations and Projects. type Scope struct { @@ -67,57 +77,28 @@ func NewProject(organizationPublicId string, opt ...Option) (*Scope, error) { } // newScope creates a new Scope of the specified ScopeType with options: -// WithName specifies the Scope's friendly name. -// WithScope specifies the Scope's parent +// WithName specifies the Scope's friendly name. WithDescription specifies the +// scope's description. WithScope specifies the Scope's parent func newScope(scopeType ScopeType, opt ...Option) (*Scope, error) { opts := getOpts(opt...) - withPublicId := opts.withPublicId - withName := opts.withName - withScope := opts.withScope - withDescription := opts.withDescription - if scopeType == UnknownScope { - return nil, errors.New("error unknown scope type for new scope") + return nil, fmt.Errorf("new scope: unknown scope type %w", db.ErrInvalidParameter) } - if scopeType == ProjectScope { - if withScope == nil { - return nil, errors.New("error project scope must be with a scope") - } - if withScope.PublicId == "" { - return nil, errors.New("error project scope parent id is unset") - } + if opts.withScope != nil && opts.withScope.PublicId == "" { + return nil, fmt.Errorf("new scope: with scope's parent id is missing %w", db.ErrInvalidParameter) } - var publicId string - if withPublicId != "" { - if !strings.HasPrefix(withPublicId, scopeType.Prefix()+"_") { - return nil, fmt.Errorf("passed-in public ID %q has wrong prefix for type %q which uses prefix %q", withPublicId, scopeType.String(), scopeType.Prefix()) - } - publicId = withPublicId - } else { - var err error - publicId, err = db.NewPublicId(scopeType.Prefix()) - if err != nil { - return nil, fmt.Errorf("error generating public id %w for new scope", err) - } + if scopeType == ProjectScope && opts.withScope == nil { + return nil, fmt.Errorf("new scope: project scope is missing its parent %w", db.ErrInvalidParameter) } - s := &Scope{ Scope: &store.Scope{ - PublicId: publicId, - Type: scopeType.String(), + Type: scopeType.String(), + Name: opts.withName, + Description: opts.withDescription, }, } - if withScope != nil { - if withScope.PublicId == "" { - return nil, errors.New("error assigning scope parent id to a scope with unset id") - } - s.ParentId = withScope.PublicId - } - if withName != "" { - s.Name = withName - } - if withDescription != "" { - s.Description = withDescription + if opts.withScope != nil { + s.ParentId = opts.withScope.PublicId } return s, nil } diff --git a/internal/iam/scope_test.go b/internal/iam/scope_test.go index 14d7358d8d..9b77b6ef28 100644 --- a/internal/iam/scope_test.go +++ b/internal/iam/scope_test.go @@ -25,6 +25,8 @@ func Test_NewScope(t *testing.T) { s, err := NewOrganization() require.NoError(err) require.NotNil(s.Scope) + s.PublicId, err = newScopeId(OrganizationScope) + require.NoError(err) err = w.Create(context.Background(), s) require.NoError(err) require.NotEmpty(s.PublicId) @@ -41,14 +43,14 @@ func Test_NewScope(t *testing.T) { s, err := newScope(UnknownScope) require.Error(err) require.Nil(s) - assert.Equal(err.Error(), "error unknown scope type for new scope") + assert.Equal(err.Error(), "new scope: unknown scope type invalid parameter") }) t.Run("proj-scope-with-no-org", func(t *testing.T) { assert, require := assert.New(t), require.New(t) s, err := NewProject("") require.Error(err) require.Nil(s) - assert.Equal(err.Error(), "error creating new project: error project scope parent id is unset") + assert.Equal(err.Error(), "error creating new project: new scope: with scope's parent id is missing invalid parameter") }) } func Test_ScopeCreate(t *testing.T) { @@ -66,6 +68,8 @@ func Test_ScopeCreate(t *testing.T) { s, err := NewOrganization() require.NoError(err) require.NotNil(s.Scope) + s.PublicId, err = newScopeId(OrganizationScope) + require.NoError(err) err = w.Create(context.Background(), s) require.NoError(err) assert.NotEmpty(s.PublicId) @@ -76,6 +80,8 @@ func Test_ScopeCreate(t *testing.T) { s, err := NewOrganization() require.NoError(err) require.NotNil(s.Scope) + s.PublicId, err = newScopeId(OrganizationScope) + require.NoError(err) err = w.Create(context.Background(), s) require.NoError(err) require.NotEmpty(s.PublicId) @@ -86,6 +92,8 @@ func Test_ScopeCreate(t *testing.T) { require.NotNil(project.Scope) assert.Equal(project.Scope.ParentId, s.PublicId) assert.Equal(project.GetDescription(), id) + project.PublicId, err = newScopeId(OrganizationScope) + require.NoError(err) err = w.Create(context.Background(), project) require.NoError(err) @@ -197,3 +205,34 @@ func TestScope_Clone(t *testing.T) { assert.True(!proto.Equal(cp.(*Scope).Scope, s2.Scope)) }) } + +func Test_stringToScopeType(t *testing.T) { + tests := []struct { + name string + s string + want ScopeType + }{ + { + name: "org", + s: "organization", + want: OrganizationScope, + }, + { + name: "proj", + s: "project", + want: ProjectScope, + }, + { + name: "unknown", + s: "org", + want: UnknownScope, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert := assert.New(t) + got := stringToScopeType(tt.s) + assert.Equal(tt.want, got) + }) + } +}