From 14f9fc917318e3348df1484528a6c97a0b59ef87 Mon Sep 17 00:00:00 2001 From: Todd Knight Date: Fri, 25 Sep 2020 13:41:20 -0700 Subject: [PATCH] Removing status from all handlers, repo errors are passed on directly for the most part to be intercepted and translated by the API error handler. (#447) --- .../handlers/accounts/account_service.go | 30 +++++----- .../authmethods/authmethod_service.go | 20 +++---- .../handlers/authtokens/authtoken_service.go | 6 +- .../handlers/groups/group_service.go | 40 +++++++------ .../host_catalogs/host_catalog_service.go | 15 +++-- .../handlers/host_sets/host_set_service.go | 42 ++++++++------ .../controller/handlers/hosts/host_service.go | 27 +++++---- .../controller/handlers/roles/role_service.go | 58 ++++++++++--------- .../handlers/roles/role_service_test.go | 27 +++++++++ .../handlers/scopes/scope_service.go | 32 +++++----- .../handlers/sessions/session_service.go | 4 +- .../handlers/targets/target_service.go | 13 +++-- .../handlers/targets/target_service_test.go | 37 ++++++++++++ .../controller/handlers/users/user_service.go | 15 +++-- 14 files changed, 223 insertions(+), 143 deletions(-) diff --git a/internal/servers/controller/handlers/accounts/account_service.go b/internal/servers/controller/handlers/accounts/account_service.go index dbd69f087..94ab5468e 100644 --- a/internal/servers/controller/handlers/accounts/account_service.go +++ b/internal/servers/controller/handlers/accounts/account_service.go @@ -16,7 +16,6 @@ import ( "github.com/hashicorp/boundary/internal/types/action" "github.com/hashicorp/boundary/internal/types/resource" "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" "google.golang.org/protobuf/types/known/wrapperspb" ) @@ -187,7 +186,8 @@ func (s Service) getFromRepo(ctx context.Context, id string) (*pb.Account, error func (s Service) createInRepo(ctx context.Context, authMethodId, scopeId string, item *pb.Account) (*pb.Account, error) { pwAttrs := &pb.PasswordAccountAttributes{} if err := handlers.StructToProto(item.GetAttributes(), pwAttrs); err != nil { - return nil, status.Errorf(codes.InvalidArgument, "Provided attributes don't match expected format.") + return nil, handlers.InvalidArgumentErrorf("Error in provided request.", + map[string]string{"attributes": "Attribute fields do not match the expected format."}) } opts := []password.Option{password.WithLoginName(pwAttrs.GetLoginName())} if item.GetName() != nil { @@ -198,7 +198,7 @@ func (s Service) createInRepo(ctx context.Context, authMethodId, scopeId string, } a, err := password.NewAccount(authMethodId, opts...) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to build user for creation: %v.", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to build user for creation: %v.", err) } repo, err := s.repoFn() if err != nil { @@ -211,10 +211,10 @@ func (s Service) createInRepo(ctx context.Context, authMethodId, scopeId string, } out, err := repo.CreateAccount(ctx, scopeId, a, createOpts...) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to create user: %v.", err) + return nil, fmt.Errorf("unable to create user: %w", err) } if out == nil { - return nil, status.Error(codes.Internal, "Unable to create user but no error returned from repository.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to create user but no error returned from repository.") } return toProto(out) } @@ -229,13 +229,13 @@ func (s Service) updateInRepo(ctx context.Context, scopeId, authMethId, id strin } u, err := password.NewAccount(authMethId, opts...) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to build auth method for update: %v.", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to build auth method for update: %v.", err) } u.PublicId = id pwAttrs := &pb.PasswordAccountAttributes{} if err := handlers.StructToProto(item.GetAttributes(), pwAttrs); err != nil { - return nil, status.Errorf(codes.InvalidArgument, "Provided attributes don't match expected format.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.InvalidArgument, "Provided attributes don't match expected format.") } if pwAttrs.GetLoginName() != "" { u.LoginName = pwAttrs.GetLoginName() @@ -254,13 +254,13 @@ func (s Service) updateInRepo(ctx context.Context, scopeId, authMethId, id strin if err != nil { switch { case errors.Is(err, password.ErrTooShort): - return nil, handlers.InvalidArgumentErrorf("Error in provided request", + return nil, handlers.InvalidArgumentErrorf("Error in provided request.", map[string]string{"attributes.login_name": "Length too short."}) } - return nil, status.Errorf(codes.Internal, "Unable to update auth method: %v.", err) + return nil, fmt.Errorf("unable to update auth method: %w", err) } if rowsUpdated == 0 { - return nil, handlers.NotFoundErrorf("AuthMethod %q doesn't exist.", id) + return nil, handlers.NotFoundErrorf("AuthMethod %q doesn't exist or incorrect version provided.", id) } return toProto(out) } @@ -275,7 +275,7 @@ func (s Service) deleteFromRepo(ctx context.Context, scopeId, id string) (bool, if errors.Is(err, db.ErrRecordNotFound) { return false, nil } - return false, status.Errorf(codes.Internal, "Unable to delete account: %v.", err) + return false, fmt.Errorf("unable to delete account: %w", err) } return rows > 0, nil } @@ -317,10 +317,10 @@ func (s Service) changePasswordInRepo(ctx context.Context, scopeId, id string, v return nil, handlers.InvalidArgumentErrorf("Error in provided request.", map[string]string{"new_password": "New password equal to current password."}) } - return nil, status.Errorf(codes.Internal, "Unable to change password: %v.", err) + return nil, fmt.Errorf( "unable to change password: %w", err) } if out == nil { - return nil, status.Errorf(codes.PermissionDenied, "Failed to change password.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.PermissionDenied, "Failed to change password.") } return toProto(out) } @@ -339,7 +339,7 @@ func (s Service) setPasswordInRepo(ctx context.Context, scopeId, id string, vers return nil, handlers.InvalidArgumentErrorf("Error in provided request.", map[string]string{"password": "Password is too short."}) } - return nil, status.Errorf(codes.Internal, "Unable to set password: %v.", err) + return nil, fmt.Errorf("unable to set password: %w", err) } return toProto(out) } @@ -403,7 +403,7 @@ func toProto(in *password.Account) (*pb.Account, error) { if st, err := handlers.ProtoToStruct(&pb.PasswordAccountAttributes{LoginName: in.GetLoginName()}); err == nil { out.Attributes = st } else { - return nil, status.Errorf(codes.Internal, "failed building password attribute struct: %v", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "failed building password attribute struct: %v", err) } return &out, nil } diff --git a/internal/servers/controller/handlers/authmethods/authmethod_service.go b/internal/servers/controller/handlers/authmethods/authmethod_service.go index 2215d10d4..93ffa7ead 100644 --- a/internal/servers/controller/handlers/authmethods/authmethod_service.go +++ b/internal/servers/controller/handlers/authmethods/authmethod_service.go @@ -17,7 +17,6 @@ import ( "github.com/hashicorp/boundary/internal/types/resource" "github.com/hashicorp/boundary/internal/types/scope" "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" "google.golang.org/protobuf/types/known/wrapperspb" ) @@ -185,7 +184,7 @@ func (s Service) createInRepo(ctx context.Context, scopeId string, item *pb.Auth } u, err := password.NewAuthMethod(scopeId, opts...) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to build auth method for creation: %v.", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to build auth method for creation: %v.", err) } repo, err := s.repoFn() if err != nil { @@ -193,10 +192,10 @@ func (s Service) createInRepo(ctx context.Context, scopeId string, item *pb.Auth } out, err := repo.CreateAuthMethod(ctx, u) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to create auth method: %v.", err) + return nil,fmt.Errorf("unable to create auth method: %w", err) } if out == nil { - return nil, status.Error(codes.Internal, "Unable to create auth method but no error returned from repository.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to create auth method but no error returned from repository.") } return toProto(out) } @@ -211,12 +210,13 @@ func (s Service) updateInRepo(ctx context.Context, scopeId, id string, mask []st } u, err := password.NewAuthMethod(scopeId, opts...) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to build auth method for update: %v.", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to build auth method for update: %v.", err) } pwAttrs := &pb.PasswordAuthMethodAttributes{} if err := handlers.StructToProto(item.GetAttributes(), pwAttrs); err != nil { - return nil, status.Errorf(codes.InvalidArgument, "Provided attributes don't match expected format.") + return nil, handlers.InvalidArgumentErrorf("Error in provided request.", + map[string]string{"attributes": "Attribute fields do not match the expected format."}) } if pwAttrs.GetMinLoginNameLength() != 0 { u.MinLoginNameLength = pwAttrs.GetMinLoginNameLength() @@ -237,10 +237,10 @@ func (s Service) updateInRepo(ctx context.Context, scopeId, id string, mask []st } out, rowsUpdated, err := repo.UpdateAuthMethod(ctx, u, version, dbMask) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to update auth method: %v.", err) + return nil, fmt.Errorf("unable to update auth method: %w", err) } if rowsUpdated == 0 { - return nil, handlers.NotFoundErrorf("AuthMethod %q doesn't exist.", id) + return nil, handlers.NotFoundErrorf("AuthMethod %q doesn't exist or incorrect version provided.", id) } return toProto(out) } @@ -255,7 +255,7 @@ func (s Service) deleteFromRepo(ctx context.Context, scopeId, id string) (bool, if errors.Is(err, db.ErrRecordNotFound) { return false, nil } - return false, status.Errorf(codes.Internal, "Unable to delete auth method: %v.", err) + return false, fmt.Errorf("unable to delete auth method: %w", err) } return rows > 0, nil } @@ -324,7 +324,7 @@ func toProto(in *password.AuthMethod) (*pb.AuthMethod, error) { MinPasswordLength: in.GetMinPasswordLength(), }) if err != nil { - return nil, status.Errorf(codes.Internal, "failed building password attribute struct: %v", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "failed building password attribute struct: %v", err) } out.Attributes = st return &out, nil diff --git a/internal/servers/controller/handlers/authtokens/authtoken_service.go b/internal/servers/controller/handlers/authtokens/authtoken_service.go index 730d0fb4d..17b56e0cd 100644 --- a/internal/servers/controller/handlers/authtokens/authtoken_service.go +++ b/internal/servers/controller/handlers/authtokens/authtoken_service.go @@ -15,8 +15,6 @@ import ( "github.com/hashicorp/boundary/internal/types/action" "github.com/hashicorp/boundary/internal/types/resource" "github.com/hashicorp/boundary/internal/types/scope" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" ) // Service handles request as described by the pbs.AuthTokenServiceServer interface. @@ -100,7 +98,7 @@ func (s Service) getFromRepo(ctx context.Context, id string) (*pb.AuthToken, err if errors.Is(err, db.ErrRecordNotFound) { return nil, handlers.NotFoundErrorf("AuthToken %q doesn't exist.", id) } - return nil, err + return nil, fmt.Errorf("unable to list auth tokens: %w", err) } if u == nil { return nil, handlers.NotFoundErrorf("AuthToken %q doesn't exist.", id) @@ -118,7 +116,7 @@ func (s Service) deleteFromRepo(ctx context.Context, id string) (bool, error) { if errors.Is(err, db.ErrRecordNotFound) { return false, nil } - return false, status.Errorf(codes.Internal, "Unable to delete user: %v.", err) + return false, fmt.Errorf("unable to delete user: %w", err) } return rows > 0, nil } diff --git a/internal/servers/controller/handlers/groups/group_service.go b/internal/servers/controller/handlers/groups/group_service.go index 235e63663..61adc7008 100644 --- a/internal/servers/controller/handlers/groups/group_service.go +++ b/internal/servers/controller/handlers/groups/group_service.go @@ -17,7 +17,6 @@ import ( "github.com/hashicorp/boundary/internal/types/resource" "github.com/hashicorp/boundary/internal/types/scope" "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" "google.golang.org/protobuf/types/known/wrapperspb" ) @@ -194,7 +193,7 @@ func (s Service) getFromRepo(ctx context.Context, id string) (*pb.Group, error) if errors.Is(err, db.ErrRecordNotFound) { return nil, handlers.NotFoundErrorf("Group %q doesn't exist.", id) } - return nil, err + return nil, fmt.Errorf("unable to get group: %w", err) } if g == nil { return nil, handlers.NotFoundErrorf("Group %q doesn't exist.", id) @@ -212,7 +211,7 @@ func (s Service) createInRepo(ctx context.Context, scopeId string, item *pb.Grou } u, err := iam.NewGroup(scopeId, opts...) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to build group for creation: %v.", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to build group for creation: %v.", err) } repo, err := s.repoFn() if err != nil { @@ -220,10 +219,10 @@ func (s Service) createInRepo(ctx context.Context, scopeId string, item *pb.Grou } out, err := repo.CreateGroup(ctx, u) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to create group: %v.", err) + return nil, fmt.Errorf("unable to create group: %w", err) } if out == nil { - return nil, status.Error(codes.Internal, "Unable to create group but no error returned from repository.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to create group but no error returned from repository.") } return toProto(out, nil), nil } @@ -239,7 +238,7 @@ func (s Service) updateInRepo(ctx context.Context, scopeId, id string, mask []st version := item.GetVersion() g, err := iam.NewGroup(scopeId, opts...) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to build group for update: %v.", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to build group for update: %v.", err) } g.PublicId = id dbMask := maskManager.Translate(mask) @@ -252,10 +251,10 @@ func (s Service) updateInRepo(ctx context.Context, scopeId, id string, mask []st } out, m, rowsUpdated, err := repo.UpdateGroup(ctx, g, version, dbMask) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to update group: %v.", err) + return nil, fmt.Errorf("unable to update group: %w", err) } if rowsUpdated == 0 { - return nil, handlers.NotFoundErrorf("Group %q doesn't exist.", id) + return nil, handlers.NotFoundErrorf("Group %q doesn't exist or incorrect version provided.", id) } return toProto(out, m), nil } @@ -270,7 +269,7 @@ func (s Service) deleteFromRepo(ctx context.Context, id string) (bool, error) { if errors.Is(err, db.ErrRecordNotFound) { return false, nil } - return false, status.Errorf(codes.Internal, "Unable to delete group: %v.", err) + return false, fmt.Errorf("unable to delete group: %w", err) } return rows > 0, nil } @@ -282,7 +281,7 @@ func (s Service) listFromRepo(ctx context.Context, scopeId string) ([]*pb.Group, } gl, err := repo.ListGroups(ctx, scopeId) if err != nil { - return nil, err + return nil, fmt.Errorf("unable to list groups: %w", err) } var outGl []*pb.Group for _, g := range gl { @@ -298,14 +297,15 @@ func (s Service) addMembersInRepo(ctx context.Context, groupId string, userIds [ } _, err = repo.AddGroupMembers(ctx, groupId, version, userIds) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to add members to group: %v.", err) + // TODO: Figure out a way to surface more helpful error info beyond the Internal error. + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to add members to group: %v.", err) } out, m, err := repo.LookupGroup(ctx, groupId) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to look up group: %v.", err) + return nil, fmt.Errorf("unable to look up group after adding members: %w", err) } if out == nil { - return nil, status.Error(codes.Internal, "Unable to lookup group after adding member to it.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to lookup group after adding member to it.") } return toProto(out, m), nil } @@ -317,14 +317,15 @@ func (s Service) setMembersInRepo(ctx context.Context, groupId string, userIds [ } _, _, err = repo.SetGroupMembers(ctx, groupId, version, userIds) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to set members on group: %v.", err) + // TODO: Figure out a way to surface more helpful error info beyond the Internal error. + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to set members on group: %v.", err) } out, m, err := repo.LookupGroup(ctx, groupId) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to look up group: %v.", err) + return nil, fmt.Errorf("unable to look up group after setting members: %w", err) } if out == nil { - return nil, status.Error(codes.Internal, "Unable to lookup group after setting members for it.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to lookup group after setting members for it.") } return toProto(out, m), nil } @@ -336,14 +337,15 @@ func (s Service) removeMembersInRepo(ctx context.Context, groupId string, userId } _, err = repo.DeleteGroupMembers(ctx, groupId, version, userIds) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to remove members from group: %v.", err) + // TODO: Figure out a way to surface more helpful error info beyond the Internal error. + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to remove members from group: %v.", err) } out, m, err := repo.LookupGroup(ctx, groupId) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to look up group: %v.", err) + return nil, fmt.Errorf("unable to look up group after removing members: %w", err) } if out == nil { - return nil, status.Error(codes.Internal, "Unable to lookup group after removing members from it.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to lookup group after removing members from it.") } return toProto(out, m), nil } diff --git a/internal/servers/controller/handlers/host_catalogs/host_catalog_service.go b/internal/servers/controller/handlers/host_catalogs/host_catalog_service.go index bef9edbb0..8fb71fd93 100644 --- a/internal/servers/controller/handlers/host_catalogs/host_catalog_service.go +++ b/internal/servers/controller/handlers/host_catalogs/host_catalog_service.go @@ -16,7 +16,6 @@ import ( "github.com/hashicorp/boundary/internal/types/resource" "github.com/hashicorp/boundary/internal/types/scope" "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" "google.golang.org/protobuf/types/known/wrapperspb" ) @@ -179,7 +178,7 @@ func (s Service) createInRepo(ctx context.Context, projId string, item *pb.HostC } h, err := static.NewHostCatalog(projId, opts...) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to build host catalog for creation: %v.", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to build host catalog for creation: %v.", err) } repo, err := s.staticRepoFn() if err != nil { @@ -187,10 +186,10 @@ func (s Service) createInRepo(ctx context.Context, projId string, item *pb.HostC } out, err := repo.CreateCatalog(ctx, h) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to create host catalog: %v.", err) + return nil, fmt.Errorf("unable to create host catalog: %w", err) } if out == nil { - return nil, status.Error(codes.Internal, "Unable to create host catalog but no error returned from repository.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to create host catalog but no error returned from repository.") } return toProto(out), nil } @@ -206,7 +205,7 @@ func (s Service) updateInRepo(ctx context.Context, projId, id string, mask []str version := item.GetVersion() h, err := static.NewHostCatalog(projId, opts...) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to build host catalog for update: %v.", err) + return nil, fmt.Errorf("unable to build host catalog for update: %w", err) } h.PublicId = id dbMask := maskManager.Translate(mask) @@ -219,10 +218,10 @@ func (s Service) updateInRepo(ctx context.Context, projId, id string, mask []str } out, rowsUpdated, err := repo.UpdateCatalog(ctx, h, version, dbMask) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to update host catalog: %v.", err) + return nil, fmt.Errorf("unable to update host catalog: %w", err) } if rowsUpdated == 0 { - return nil, handlers.NotFoundErrorf("Host catalog %q doesn't exist.", id) + return nil, handlers.NotFoundErrorf("Host catalog %q doesn't exist or incorrect version provided.", id) } return toProto(out), nil } @@ -234,7 +233,7 @@ func (s Service) deleteFromRepo(ctx context.Context, id string) (bool, error) { } rows, err := repo.DeleteCatalog(ctx, id) if err != nil { - return false, status.Errorf(codes.Internal, "Unable to delete host: %v.", err) + return false, fmt.Errorf("unable to delete host: %w", err) } return rows > 0, nil } diff --git a/internal/servers/controller/handlers/host_sets/host_set_service.go b/internal/servers/controller/handlers/host_sets/host_set_service.go index cd33fe629..893d61957 100644 --- a/internal/servers/controller/handlers/host_sets/host_set_service.go +++ b/internal/servers/controller/handlers/host_sets/host_set_service.go @@ -2,9 +2,11 @@ package host_sets import ( "context" + "errors" "fmt" "github.com/hashicorp/boundary/internal/auth" + "github.com/hashicorp/boundary/internal/db" pb "github.com/hashicorp/boundary/internal/gen/controller/api/resources/hostsets" pbs "github.com/hashicorp/boundary/internal/gen/controller/api/services" "github.com/hashicorp/boundary/internal/host" @@ -15,7 +17,6 @@ import ( "github.com/hashicorp/boundary/internal/types/action" "github.com/hashicorp/boundary/internal/types/resource" "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" "google.golang.org/protobuf/types/known/wrapperspb" ) @@ -206,7 +207,7 @@ func (s Service) createInRepo(ctx context.Context, scopeId, catalogId string, it } h, err := static.NewHostSet(catalogId, opts...) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to build host set for creation: %v.", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to build host set for creation: %v.", err) } repo, err := s.staticRepoFn() if err != nil { @@ -214,10 +215,14 @@ func (s Service) createInRepo(ctx context.Context, scopeId, catalogId string, it } out, err := repo.CreateSet(ctx, scopeId, h) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to create host set: %v.", err) + if db.IsUniqueError(err) || errors.Is(err, db.ErrNotUnique) { + // Push this error through so the error interceptor can interpret it correctly. + return nil, err + } + return nil, fmt.Errorf("unable to create host set: %w", err) } if out == nil { - return nil, status.Error(codes.Internal, "Unable to create host set but no error returned from repository.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to create host set but no error returned from repository.") } return toProto(out, nil), nil } @@ -232,7 +237,7 @@ func (s Service) updateInRepo(ctx context.Context, scopeId, catalogId, id string } h, err := static.NewHostSet(catalogId, opts...) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to build host set for update: %v.", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to build host set for update: %v.", err) } h.PublicId = id dbMask := maskManager.Translate(mask) @@ -245,10 +250,10 @@ func (s Service) updateInRepo(ctx context.Context, scopeId, catalogId, id string } out, m, rowsUpdated, err := repo.UpdateSet(ctx, scopeId, h, item.GetVersion(), dbMask) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to update host set: %v.", err) + return nil, fmt.Errorf("unable to update host set: %w.", err) } if rowsUpdated == 0 { - return nil, handlers.NotFoundErrorf("Host set %q doesn't exist.", id) + return nil, handlers.NotFoundErrorf("Host set %q doesn't exist or incorrect version provided.", id) } return toProto(out, m), nil } @@ -260,7 +265,7 @@ func (s Service) deleteFromRepo(ctx context.Context, scopeId, id string) (bool, } rows, err := repo.DeleteSet(ctx, scopeId, id) if err != nil { - return false, status.Errorf(codes.Internal, "Unable to delete host: %v.", err) + return false, fmt.Errorf("unable to delete host: %w", err) } return rows > 0, nil } @@ -288,14 +293,15 @@ func (s Service) addInRepo(ctx context.Context, scopeId, setId string, hostIds [ } _, err = repo.AddSetMembers(ctx, scopeId, setId, version, hostIds) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to add hosts to host set: %v.", err) + // TODO: Figure out a way to surface more helpful error info beyond the Internal error. + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to add hosts to host set: %v.", err) } out, m, err := repo.LookupSet(ctx, setId) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to look up host set: %v.", err) + return nil, fmt.Errorf("unable to look up host set after adding hosts: %w", err) } if out == nil { - return nil, status.Error(codes.Internal, "Unable to lookup host set after adding hosts to it.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to lookup host set after adding hosts to it.") } return toProto(out, m), nil } @@ -307,15 +313,16 @@ func (s Service) setInRepo(ctx context.Context, scopeId, setId string, hostIds [ } _, _, err = repo.SetSetMembers(ctx, scopeId, setId, version, hostIds) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to set hosts in host set: %v.", err) + // TODO: Figure out a way to surface more helpful error info beyond the Internal error. + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to set hosts in host set: %v.", err) } out, m, err := repo.LookupSet(ctx, setId) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to look up host set: %v.", err) + return nil, fmt.Errorf("unable to look up host set after setting hosts: %w", err) } if out == nil { - return nil, status.Error(codes.Internal, "Unable to lookup host set after setting hosts for it.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to lookup host set after setting hosts for it.") } return toProto(out, m), nil } @@ -327,14 +334,15 @@ func (s Service) removeInRepo(ctx context.Context, scopeId, setId string, hostId } _, err = repo.DeleteSetMembers(ctx, scopeId, setId, version, hostIds) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to remove hosts from host set: %v.", err) + // TODO: Figure out a way to surface more helpful error info beyond the Internal error. + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to remove hosts from host set: %v.", err) } out, m, err := repo.LookupSet(ctx, setId) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to look up host set: %v.", err) + return nil, fmt.Errorf("unable to look up host set: %w", err) } if out == nil { - return nil, status.Error(codes.Internal, "Unable to lookup host set after removing hosts from it.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to lookup host set after removing hosts from it.") } return toProto(out, m), nil } diff --git a/internal/servers/controller/handlers/hosts/host_service.go b/internal/servers/controller/handlers/hosts/host_service.go index f4d32d4ad..5de84cf8c 100644 --- a/internal/servers/controller/handlers/hosts/host_service.go +++ b/internal/servers/controller/handlers/hosts/host_service.go @@ -2,11 +2,13 @@ package hosts import ( "context" + "errors" "fmt" "net" "strings" "github.com/hashicorp/boundary/internal/auth" + "github.com/hashicorp/boundary/internal/db" pb "github.com/hashicorp/boundary/internal/gen/controller/api/resources/hosts" pbs "github.com/hashicorp/boundary/internal/gen/controller/api/services" "github.com/hashicorp/boundary/internal/host" @@ -17,7 +19,6 @@ import ( "github.com/hashicorp/boundary/internal/types/action" "github.com/hashicorp/boundary/internal/types/resource" "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" "google.golang.org/protobuf/types/known/wrapperspb" ) @@ -153,7 +154,7 @@ func (s Service) getFromRepo(ctx context.Context, id string) (*pb.Host, error) { func (s Service) createInRepo(ctx context.Context, scopeId, catalogId string, item *pb.Host) (*pb.Host, error) { ha := &pb.StaticHostAttributes{} if err := handlers.StructToProto(item.GetAttributes(), ha); err != nil { - return nil, status.Errorf(codes.Internal, "Failed converting attributes to subtype proto: %s", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Failed converting attributes to subtype proto: %s", err) } var opts []static.Option if ha.GetAddress() != nil { @@ -167,7 +168,7 @@ func (s Service) createInRepo(ctx context.Context, scopeId, catalogId string, it } h, err := static.NewHost(catalogId, opts...) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to build host for creation: %v.", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to build host for creation: %v.", err) } repo, err := s.staticRepoFn() @@ -176,10 +177,14 @@ func (s Service) createInRepo(ctx context.Context, scopeId, catalogId string, it } out, err := repo.CreateHost(ctx, scopeId, h) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to create host: %v.", err) + if db.IsUniqueError(err) || errors.Is(err, db.ErrNotUnique) { + // Push this error through so the error interceptor can interpret it correctly. + return nil, err + } + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to create host: %v.", err) } if out == nil { - return nil, status.Error(codes.Internal, "Unable to create host but no error returned from repository.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to create host but no error returned from repository.") } return toProto(out, nil) } @@ -187,7 +192,7 @@ func (s Service) createInRepo(ctx context.Context, scopeId, catalogId string, it func (s Service) updateInRepo(ctx context.Context, scopeId, catalogId, id string, mask []string, item *pb.Host) (*pb.Host, error) { ha := &pb.StaticHostAttributes{} if err := handlers.StructToProto(item.GetAttributes(), ha); err != nil { - return nil, status.Errorf(codes.Internal, "Failed converting attributes to subtype proto: %s", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Failed converting attributes to subtype proto: %s", err) } var opts []static.Option if desc := item.GetDescription(); desc != nil { @@ -201,7 +206,7 @@ func (s Service) updateInRepo(ctx context.Context, scopeId, catalogId, id string } h, err := static.NewHost(catalogId, opts...) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to build host for update: %v.", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to build host for update: %v.", err) } h.PublicId = id dbMask := maskManager.Translate(mask) @@ -214,10 +219,10 @@ func (s Service) updateInRepo(ctx context.Context, scopeId, catalogId, id string } out, rowsUpdated, err := repo.UpdateHost(ctx, scopeId, h, item.GetVersion(), dbMask) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to update host: %v.", err) + return nil, fmt.Errorf("unable to update host: %w", err) } if rowsUpdated == 0 { - return nil, handlers.NotFoundErrorf("Host %q doesn't exist.", id) + return nil, handlers.NotFoundErrorf("Host %q doesn't exist or incorrect version provided.", id) } return toProto(out, nil) } @@ -229,7 +234,7 @@ func (s Service) deleteFromRepo(ctx context.Context, scopeId, id string) (bool, } rows, err := repo.DeleteHost(ctx, scopeId, id) if err != nil { - return false, status.Errorf(codes.Internal, "Unable to delete host: %v.", err) + return false, fmt.Errorf("unable to delete host: %w", err) } return rows > 0, nil } @@ -314,7 +319,7 @@ func toProto(in *static.Host, members []*static.HostSet) (*pb.Host, error) { } st, err := handlers.ProtoToStruct(&pb.StaticHostAttributes{Address: wrapperspb.String(in.GetAddress())}) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to convert static attribute to struct: %s", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to convert static attribute to struct: %s", err) } out.Attributes = st return &out, nil diff --git a/internal/servers/controller/handlers/roles/role_service.go b/internal/servers/controller/handlers/roles/role_service.go index b65f34927..1a195204a 100644 --- a/internal/servers/controller/handlers/roles/role_service.go +++ b/internal/servers/controller/handlers/roles/role_service.go @@ -18,7 +18,6 @@ import ( "github.com/hashicorp/boundary/internal/types/resource" "github.com/hashicorp/boundary/internal/types/scope" "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" "google.golang.org/protobuf/types/known/wrapperspb" ) @@ -267,7 +266,7 @@ func (s Service) createInRepo(ctx context.Context, scopeId string, item *pb.Role } u, err := iam.NewRole(scopeId, opts...) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to build role for creation: %v.", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to build role for creation: %v.", err) } repo, err := s.repoFn() if err != nil { @@ -275,10 +274,10 @@ func (s Service) createInRepo(ctx context.Context, scopeId string, item *pb.Role } out, err := repo.CreateRole(ctx, u) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to create role: %v.", err) + return nil, fmt.Errorf("unable to create role: %w", err) } if out == nil { - return nil, status.Error(codes.Internal, "Unable to create role but no error returned from repository.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to create role but no error returned from repository.") } return toProto(out, nil, nil), nil } @@ -298,7 +297,7 @@ func (s Service) updateInRepo(ctx context.Context, scopeId, id string, mask []st u, err := iam.NewRole(scopeId, opts...) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to build role for update: %v.", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to build role for update: %v.", err) } u.PublicId = id dbMask := maskManager.Translate(mask) @@ -311,12 +310,11 @@ func (s Service) updateInRepo(ctx context.Context, scopeId, id string, mask []st } out, pr, gr, rowsUpdated, err := repo.UpdateRole(ctx, u, version, dbMask) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to update role: %v.", err) + return nil, fmt.Errorf("unable to update role: %w", err) } if rowsUpdated == 0 { - return nil, handlers.NotFoundErrorf("Role %q doesn't exist.", id) + return nil, handlers.NotFoundErrorf("Role %q doesn't exist or incorrect version provided.", id) } - // TODO: Attach principals and grants to UpdateRole response return toProto(out, pr, gr), nil } @@ -330,7 +328,7 @@ func (s Service) deleteFromRepo(ctx context.Context, id string) (bool, error) { if errors.Is(err, db.ErrRecordNotFound) { return false, nil } - return false, status.Errorf(codes.Internal, "Unable to delete role: %v.", err) + return false, fmt.Errorf("unable to delete role: %w", err) } return rows > 0, nil } @@ -359,14 +357,15 @@ func (s Service) addPrinciplesInRepo(ctx context.Context, roleId string, princip } _, err = repo.AddPrincipalRoles(ctx, roleId, version, principalIds) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to add principals to role: %v.", err) + // TODO: Figure out a way to surface more helpful error info beyond the Internal error. + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to add principals to role: %v.", err) } out, pr, roleGrants, err := repo.LookupRole(ctx, roleId) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to look up role: %v.", err) + return nil, fmt.Errorf("unable to look up role after adding principals: %w", err) } if out == nil { - return nil, status.Error(codes.Internal, "Unable to lookup role after adding principals to it.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to lookup role after adding principals to it.") } return toProto(out, pr, roleGrants), nil } @@ -378,14 +377,15 @@ func (s Service) setPrinciplesInRepo(ctx context.Context, roleId string, princip } _, _, err = repo.SetPrincipalRoles(ctx, roleId, version, principalIds) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to set principals on role: %v.", err) + // TODO: Figure out a way to surface more helpful error info beyond the Internal error. + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to set principals on role: %v.", err) } out, pr, roleGrants, err := repo.LookupRole(ctx, roleId) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to look up role: %v.", err) + return nil, fmt.Errorf("unable to look up role after setting principals: %w", err) } if out == nil { - return nil, status.Error(codes.Internal, "Unable to lookup role after setting principals for it.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to lookup role after setting principals for it.") } return toProto(out, pr, roleGrants), nil } @@ -397,14 +397,15 @@ func (s Service) removePrinciplesInRepo(ctx context.Context, roleId string, prin } _, err = repo.DeletePrincipalRoles(ctx, roleId, version, principalIds) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to remove principals from role: %v.", err) + // TODO: Figure out a way to surface more helpful error info beyond the Internal error. + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to remove principals from role: %v.", err) } out, pr, roleGrants, err := repo.LookupRole(ctx, roleId) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to look up role: %v.", err) + return nil, fmt.Errorf("unable to look up role after removing principals: %w", err) } if out == nil { - return nil, status.Error(codes.Internal, "Unable to lookup role after removing principals from it.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to lookup role after removing principals from it.") } return toProto(out, pr, roleGrants), nil } @@ -416,14 +417,15 @@ func (s Service) addGrantsInRepo(ctx context.Context, roleId string, grants []st } _, err = repo.AddRoleGrants(ctx, roleId, version, grants) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to add grants to role: %v.", err) + // TODO: Figure out a way to surface more helpful error info beyond the Internal error. + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to add grants to role: %v.", err) } out, pr, roleGrants, err := repo.LookupRole(ctx, roleId) if err != nil { - return nil, status.Error(codes.Internal, "Error looking up role after adding grants to it.") + return nil, fmt.Errorf("unable to look up role after adding grants: %w", err) } if out == nil { - return nil, status.Error(codes.Internal, "Unable to lookup role after adding grants to it.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to lookup role after adding grants to it.") } return toProto(out, pr, roleGrants), nil } @@ -439,14 +441,15 @@ func (s Service) setGrantsInRepo(ctx context.Context, roleId string, grants []st } _, _, err = repo.SetRoleGrants(ctx, roleId, version, grants) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to set grants on role: %v.", err) + // TODO: Figure out a way to surface more helpful error info beyond the Internal error. + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to set grants on role: %v.", err) } out, pr, roleGrants, err := repo.LookupRole(ctx, roleId) if err != nil { - return nil, status.Error(codes.Internal, "Error looking up role after setting grants on it.") + return nil, fmt.Errorf("unable to look up role after setting grants: %w", err) } if out == nil { - return nil, status.Error(codes.Internal, "Unable to lookup role after setting grants on it.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to lookup role after setting grants on it.") } return toProto(out, pr, roleGrants), nil } @@ -458,14 +461,15 @@ func (s Service) removeGrantsInRepo(ctx context.Context, roleId string, grants [ } _, err = repo.DeleteRoleGrants(ctx, roleId, version, grants) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to remove grants from role: %v.", err) + // TODO: Figure out a way to surface more helpful error info beyond the Internal error. + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to remove grants from role: %v", err) } out, pr, roleGrants, err := repo.LookupRole(ctx, roleId) if err != nil { - return nil, status.Error(codes.Internal, "Error looking up role after removing grants from it.") + return nil, fmt.Errorf("unable to look up role after removing grants: %w", err) } if out == nil { - return nil, status.Error(codes.Internal, "Unable to lookup role after removing grants from it.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to lookup role after removing grants from it.") } return toProto(out, pr, roleGrants), nil } diff --git a/internal/servers/controller/handlers/roles/role_service_test.go b/internal/servers/controller/handlers/roles/role_service_test.go index ab9a22286..a66a99328 100644 --- a/internal/servers/controller/handlers/roles/role_service_test.go +++ b/internal/servers/controller/handlers/roles/role_service_test.go @@ -1425,6 +1425,15 @@ func TestAddGrants(t *testing.T) { req *pbs.AddRoleGrantsRequest err error }{ + { + name: "Bad Version", + req: &pbs.AddRoleGrantsRequest{ + Id: role.GetPublicId(), + GrantStrings: []string{"id=*;actions=create"}, + Version: role.GetVersion() + 2, + }, + err: handlers.ApiErrorWithCode(codes.Internal), + }, { name: "Bad Role Id", req: &pbs.AddRoleGrantsRequest{ @@ -1528,6 +1537,15 @@ func TestSetGrants(t *testing.T) { req *pbs.SetRoleGrantsRequest err error }{ + { + name: "Bad Version", + req: &pbs.SetRoleGrantsRequest{ + Id: role.GetPublicId(), + GrantStrings: []string{"id=*;actions=create"}, + Version: role.GetVersion() + 2, + }, + err: handlers.ApiErrorWithCode(codes.Internal), + }, { name: "Bad Role Id", req: &pbs.SetRoleGrantsRequest{ @@ -1631,6 +1649,15 @@ func TestRemoveGrants(t *testing.T) { err error }{ + { + name: "Bad Version", + req: &pbs.RemoveRoleGrantsRequest{ + Id: role.GetPublicId(), + GrantStrings: []string{"id=*;actions=create"}, + Version: role.GetVersion() + 2, + }, + err: handlers.ApiErrorWithCode(codes.Internal), + }, { name: "Bad Role Id", req: &pbs.RemoveRoleGrantsRequest{ diff --git a/internal/servers/controller/handlers/scopes/scope_service.go b/internal/servers/controller/handlers/scopes/scope_service.go index ae8bfb870..6e901fe32 100644 --- a/internal/servers/controller/handlers/scopes/scope_service.go +++ b/internal/servers/controller/handlers/scopes/scope_service.go @@ -18,7 +18,6 @@ import ( "github.com/hashicorp/boundary/internal/types/resource" "github.com/hashicorp/boundary/internal/types/scope" "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" "google.golang.org/protobuf/types/known/wrapperspb" ) @@ -175,7 +174,7 @@ func (s Service) createInRepo(ctx context.Context, authResults auth.VerifyResult iamScope, err = iam.NewProject(parentScope.GetId(), opts...) } if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to build new scope for creation: %v.", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to build new scope for creation: %v.", err) } repo, err := s.repoFn() if err != nil { @@ -183,10 +182,10 @@ func (s Service) createInRepo(ctx context.Context, authResults auth.VerifyResult } out, err := repo.CreateScope(ctx, iamScope, authResults.UserId, opts...) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to create scope: %v.", err) + return nil, fmt.Errorf("unable to create scope: %w", err) } if out == nil { - return nil, status.Error(codes.Internal, "Unable to create scope but no error returned from repository.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to create scope but no error returned from repository.") } return ToProto(out), nil } @@ -210,7 +209,7 @@ func (s Service) updateInRepo(ctx context.Context, parentScope *scopes.ScopeInfo iamScope, err = iam.NewProject(parentScope.GetId(), opts...) } if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to build scope for update: %v.", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to build scope for update: %v.", err) } iamScope.PublicId = scopeId dbMask := maskManager.Translate(mask) @@ -223,10 +222,10 @@ func (s Service) updateInRepo(ctx context.Context, parentScope *scopes.ScopeInfo } out, rowsUpdated, err := repo.UpdateScope(ctx, iamScope, version, dbMask) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to update project: %v.", err) + return nil, fmt.Errorf("unable to update project: %w", err) } if rowsUpdated == 0 { - return nil, handlers.NotFoundErrorf("Scope %q doesn't exist.", scopeId) + return nil, handlers.NotFoundErrorf("Scope %q doesn't exist or incorrect version provided.", scopeId) } return ToProto(out), nil } @@ -238,7 +237,7 @@ func (s Service) deleteFromRepo(ctx context.Context, scopeId string) (bool, erro } rows, err := repo.DeleteScope(ctx, scopeId) if err != nil { - return false, status.Errorf(codes.Internal, "Unable to delete scope: %v.", err) + return false, fmt.Errorf("unable to delete scope: %w", err) } return rows > 0, nil } @@ -264,10 +263,11 @@ func (s Service) listFromRepo(ctx context.Context, scopeId string) ([]*pb.Scope, case strings.HasPrefix(scopeId, scope.Org.Prefix()): scps, err = repo.ListProjects(ctx, scopeId) default: - return nil, status.Errorf(codes.InvalidArgument, "Invalid scope ID given for listing") + return nil, handlers.InvalidArgumentErrorf("Error in provided request.", + map[string]string{"scope_id": "Invalid id for listing."}) } if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to list scopes: %v", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to list scopes: %v", err) } var outPl []*pb.Scope @@ -357,7 +357,7 @@ func validateGetRequest(req *pbs.GetScopeRequest) error { badFields["id"] = "Invalidly formatted scope id." } if len(badFields) > 0 { - return handlers.InvalidArgumentErrorf("Improperly formatted identifier.", badFields) + return handlers.InvalidArgumentErrorf("Error in provided request.", badFields) } return nil } @@ -393,7 +393,7 @@ func validateCreateRequest(req *pbs.CreateScopeRequest) error { badFields["version"] = "This cannot be specified at create time." } if len(badFields) > 0 { - return handlers.InvalidArgumentErrorf("Argument errors found in the request.", badFields) + return handlers.InvalidArgumentErrorf("Error in provided request.", badFields) } return nil } @@ -427,7 +427,7 @@ func validateUpdateRequest(req *pbs.UpdateScopeRequest) error { item := req.GetItem() if item == nil { if len(badFields) > 0 { - return handlers.InvalidArgumentErrorf("Errors in provided fields.", badFields) + return handlers.InvalidArgumentErrorf("Error in provided request.", badFields) } // It is legitimate for no item to be specified in an update request as it indicates all fields provided in // the mask will be marked as unset. @@ -446,7 +446,7 @@ func validateUpdateRequest(req *pbs.UpdateScopeRequest) error { badFields["updated_time"] = "This is a read only field and cannot be specified in an update request." } if len(badFields) > 0 { - return handlers.InvalidArgumentErrorf("Errors in provided fields.", badFields) + return handlers.InvalidArgumentErrorf("Error in provided request.", badFields) } return nil @@ -470,7 +470,7 @@ func validateDeleteRequest(req *pbs.DeleteScopeRequest) error { badFields["id"] = "Invalidly formatted scope id." } if len(badFields) > 0 { - return handlers.InvalidArgumentErrorf("Errors in provided fields.", badFields) + return handlers.InvalidArgumentErrorf("Error in provided request.", badFields) } return nil } @@ -478,7 +478,7 @@ func validateDeleteRequest(req *pbs.DeleteScopeRequest) error { func validateListRequest(req *pbs.ListScopesRequest) error { badFields := map[string]string{} if len(badFields) > 0 { - return handlers.InvalidArgumentErrorf("Improperly formatted identifier.", badFields) + return handlers.InvalidArgumentErrorf("Error in provided request.", badFields) } return nil } diff --git a/internal/servers/controller/handlers/sessions/session_service.go b/internal/servers/controller/handlers/sessions/session_service.go index f4a29d649..e216a6e0d 100644 --- a/internal/servers/controller/handlers/sessions/session_service.go +++ b/internal/servers/controller/handlers/sessions/session_service.go @@ -15,8 +15,6 @@ import ( "github.com/hashicorp/boundary/internal/types/action" "github.com/hashicorp/boundary/internal/types/resource" "github.com/hashicorp/boundary/internal/types/scope" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" ) // Service handles request as described by the pbs.SessionServiceServer interface. @@ -132,7 +130,7 @@ func (s Service) cancelInRepo(ctx context.Context, id string, version uint32) (* } out, err := repo.CancelSession(ctx, id, version) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to update session: %v.", err) + return nil, fmt.Errorf("unable to update session: %w", err) } return toProto(out), nil } diff --git a/internal/servers/controller/handlers/targets/target_service.go b/internal/servers/controller/handlers/targets/target_service.go index ed2e9ba01..b6397a3e5 100644 --- a/internal/servers/controller/handlers/targets/target_service.go +++ b/internal/servers/controller/handlers/targets/target_service.go @@ -461,7 +461,7 @@ func (s Service) createInRepo(ctx context.Context, item *pb.Target) (*pb.Target, } out, m, err := repo.CreateTcpTarget(ctx, u) if err != nil { - return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to create target: %v.", err) + return nil, fmt.Errorf("unable to create target: %w", err) } if out == nil { return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to create target but no error returned from repository.") @@ -506,7 +506,7 @@ func (s Service) updateInRepo(ctx context.Context, scopeId, id string, mask []st } out, m, rowsUpdated, err := repo.UpdateTcpTarget(ctx, u, version, dbMask) if err != nil { - return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to update target: %v.", err) + return nil, fmt.Errorf("unable to update target: %w", err) } if rowsUpdated == 0 { return nil, handlers.NotFoundErrorf("Target %q not found or incorrect version provided.", id) @@ -524,7 +524,7 @@ func (s Service) deleteFromRepo(ctx context.Context, id string) (bool, error) { if errors.Is(err, db.ErrRecordNotFound) { return false, nil } - return false, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to delete target: %v.", err) + return false, fmt.Errorf("unable to delete target: %w", err) } return rows > 0, nil } @@ -556,6 +556,7 @@ func (s Service) addInRepo(ctx context.Context, targetId string, hostSetId []str } out, m, err := repo.AddTargetHostSets(ctx, targetId, version, hostSetId) if err != nil { + // TODO: Figure out a way to surface more helpful error info beyond the Internal error. return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to add host sets to target: %v.", err) } if out == nil { @@ -571,12 +572,13 @@ func (s Service) setInRepo(ctx context.Context, targetId string, hostSetIds []st } _, _, err = repo.SetTargetHostSets(ctx, targetId, version, hostSetIds) if err != nil { + // TODO: Figure out a way to surface more helpful error info beyond the Internal error. return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to set host sets in target: %v.", err) } out, m, err := repo.LookupTarget(ctx, targetId) if err != nil { - return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to look up target: %v.", err) + return nil, fmt.Errorf("unable to look up target after setting host sets: %w", err) } if out == nil { return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to lookup target after setting host sets for it.") @@ -591,11 +593,12 @@ func (s Service) removeInRepo(ctx context.Context, targetId string, hostSetIds [ } _, err = repo.DeleteTargeHostSets(ctx, targetId, version, hostSetIds) if err != nil { + // TODO: Figure out a way to surface more helpful error info beyond the Internal error. return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to remove host sets from target: %v.", err) } out, m, err := repo.LookupTarget(ctx, targetId) if err != nil { - return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to look up target: %v.", err) + return nil, fmt.Errorf("unable to look up target after removing host sets: %w", err) } if out == nil { return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to lookup target after removing host sets from it.") diff --git a/internal/servers/controller/handlers/targets/target_service_test.go b/internal/servers/controller/handlers/targets/target_service_test.go index 4b8f3d422..0ae31b0ad 100644 --- a/internal/servers/controller/handlers/targets/target_service_test.go +++ b/internal/servers/controller/handlers/targets/target_service_test.go @@ -761,6 +761,43 @@ func TestUpdate(t *testing.T) { } } +func TestUpdate_BadVersion(t *testing.T) { + t.Parallel() + conn, _ := db.TestSetup(t, "postgres") + wrapper := db.TestWrapper(t) + kms := kms.TestKms(t, conn, wrapper) + + _, proj := iam.TestScopes(t, iam.TestRepo(t, conn, wrapper)) + + rw := db.New(conn) + repoFn := func() (*target.Repository, error) { + return target.NewRepository(rw, rw, kms) + } + repo, err := repoFn() + require.NoError(t, err, "Couldn't create new target repo.") + + tar, err := target.NewTcpTarget(proj.GetPublicId(), target.WithName("default"), target.WithDescription("default")) + tar.DefaultPort = 2 + require.NoError(t, err) + gtar, _, err := repo.CreateTcpTarget(context.Background(), tar) + require.NoError(t, err) + + tested, err := testService(t, conn, kms, wrapper) + require.NoError(t, err, "Failed to create a new host set service.") + + upTar, err := tested.UpdateTarget(auth.DisabledAuthTestContext(auth.WithScopeId(proj.GetPublicId())), &pbs.UpdateTargetRequest{ + Id: gtar.GetPublicId(), + Item: &pb.Target{ + Description: wrapperspb.String("updated"), + Version: 72, + }, + UpdateMask: &field_mask.FieldMask{Paths: []string{"description"}}, + }) + assert.Nil(t, upTar) + assert.Error(t, err) + assert.True(t, errors.Is(err, handlers.NotFoundError()), "Got %v, wanted not found error.", err) +} + func TestAddTargetHostSets(t *testing.T) { conn, _ := db.TestSetup(t, "postgres") wrapper := db.TestWrapper(t) diff --git a/internal/servers/controller/handlers/users/user_service.go b/internal/servers/controller/handlers/users/user_service.go index 5ce00c68c..ca83a0ab9 100644 --- a/internal/servers/controller/handlers/users/user_service.go +++ b/internal/servers/controller/handlers/users/user_service.go @@ -17,7 +17,6 @@ import ( "github.com/hashicorp/boundary/internal/types/resource" "github.com/hashicorp/boundary/internal/types/scope" "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" "google.golang.org/protobuf/types/known/wrapperspb" ) @@ -161,7 +160,7 @@ func (s Service) createInRepo(ctx context.Context, orgId string, item *pb.User) } u, err := iam.NewUser(orgId, opts...) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to build user for creation: %v.", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to build user for creation: %v.", err) } repo, err := s.repoFn() if err != nil { @@ -169,10 +168,10 @@ func (s Service) createInRepo(ctx context.Context, orgId string, item *pb.User) } out, err := repo.CreateUser(ctx, u) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to create user: %v.", err) + return nil, fmt.Errorf("unable to create user: %w", err) } if out == nil { - return nil, status.Error(codes.Internal, "Unable to create user but no error returned from repository.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to create user but no error returned from repository.") } return toProto(out), nil } @@ -188,7 +187,7 @@ func (s Service) updateInRepo(ctx context.Context, orgId, id string, mask []stri version := item.GetVersion() u, err := iam.NewUser(orgId, opts...) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to build user for update: %v.", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to build user for update: %v.", err) } u.PublicId = id dbMask := maskManager.Translate(mask) @@ -201,10 +200,10 @@ func (s Service) updateInRepo(ctx context.Context, orgId, id string, mask []stri } out, _, rowsUpdated, err := repo.UpdateUser(ctx, u, version, dbMask) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to update user: %v.", err) + return nil, fmt.Errorf("unable to update user: %w", err) } if rowsUpdated == 0 { - return nil, handlers.NotFoundErrorf("User %q doesn't exist.", id) + return nil, handlers.NotFoundErrorf("User %q doesn't exist or incorrect version provided.", id) } return toProto(out), nil } @@ -219,7 +218,7 @@ func (s Service) deleteFromRepo(ctx context.Context, id string) (bool, error) { if errors.Is(err, db.ErrRecordNotFound) { return false, nil } - return false, status.Errorf(codes.Internal, "Unable to delete user: %v.", err) + return false, fmt.Errorf("unable to delete user: %w", err) } return rows > 0, nil }