refactor scopes to conform to public id usage and repo error msgs (#117)

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)

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
+}

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))
+	})
 }

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
 }

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)

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)

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
 }

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)
+		})
+	}
+}