diff --git a/api/error_details.gen.go b/api/error_details.gen.go index fe64c3339..401cc75f0 100644 --- a/api/error_details.gen.go +++ b/api/error_details.gen.go @@ -4,5 +4,6 @@ package api type ErrorDetails struct { TraceId string `json:"TraceId,omitempty"` RequestId string `json:"request_id,omitempty"` + ErrorId string `json:"error_id,omitempty"` RequestFields []*FieldError `json:"request_fields,omitempty"` } diff --git a/internal/auth/password/repository_password.go b/internal/auth/password/repository_password.go index 6dad3f183..459b1c6ae 100644 --- a/internal/auth/password/repository_password.go +++ b/internal/auth/password/repository_password.go @@ -101,8 +101,9 @@ func (r *Repository) Authenticate(ctx context.Context, scopeId, authMethodId, lo // the stored password. The account for the accountId is returned with a // new CredentialId if password is successfully changed. // -// Returns nil if old does not match the stored password for accountId. -// Returns ErrPasswordsEqual if old and new are equal. +// Returns nil, db.ErrorRecordNotFound if the account doesn't exist. +// Returns nil, nil if old does not match the stored password for accountId. +// Returns nil, ErrPasswordsEqual if old and new are equal. func (r *Repository) ChangePassword(ctx context.Context, scopeId, accountId, old, new string, version uint32) (*Account, error) { if accountId == "" { return nil, fmt.Errorf("change password: no account id: %w", db.ErrInvalidParameter) diff --git a/internal/gen/controller/api/error.pb.go b/internal/gen/controller/api/error.pb.go index 140103d2a..16febd98e 100644 --- a/internal/gen/controller/api/error.pb.go +++ b/internal/gen/controller/api/error.pb.go @@ -32,7 +32,8 @@ type ErrorDetails struct { TraceId string `protobuf:"bytes,1,opt,name=TraceId,proto3" json:"TraceId,omitempty"` RequestId string `protobuf:"bytes,2,opt,name=request_id,proto3" json:"request_id,omitempty"` - RequestFields []*FieldError `protobuf:"bytes,3,rep,name=request_fields,proto3" json:"request_fields,omitempty"` + ErrorId string `protobuf:"bytes,3,opt,name=error_id,proto3" json:"error_id,omitempty"` + RequestFields []*FieldError `protobuf:"bytes,4,rep,name=request_fields,proto3" json:"request_fields,omitempty"` } func (x *ErrorDetails) Reset() { @@ -81,6 +82,13 @@ func (x *ErrorDetails) GetRequestId() string { return "" } +func (x *ErrorDetails) GetErrorId() string { + if x != nil { + return x.ErrorId + } + return "" +} + func (x *ErrorDetails) GetRequestFields() []*FieldError { if x != nil { return x.RequestFields @@ -227,33 +235,35 @@ var file_controller_api_v1_error_proto_rawDesc = []byte{ 0x0a, 0x1d, 0x63, 0x6f, 0x6e, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x65, 0x72, 0x2f, 0x61, 0x70, 0x69, 0x2f, 0x76, 0x31, 0x2f, 0x65, 0x72, 0x72, 0x6f, 0x72, 0x2e, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x12, 0x11, 0x63, 0x6f, 0x6e, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x65, 0x72, 0x2e, 0x61, 0x70, 0x69, 0x2e, - 0x76, 0x31, 0x22, 0x8f, 0x01, 0x0a, 0x0c, 0x45, 0x72, 0x72, 0x6f, 0x72, 0x44, 0x65, 0x74, 0x61, + 0x76, 0x31, 0x22, 0xab, 0x01, 0x0a, 0x0c, 0x45, 0x72, 0x72, 0x6f, 0x72, 0x44, 0x65, 0x74, 0x61, 0x69, 0x6c, 0x73, 0x12, 0x18, 0x0a, 0x07, 0x54, 0x72, 0x61, 0x63, 0x65, 0x49, 0x64, 0x18, 0x01, 0x20, 0x01, 0x28, 0x09, 0x52, 0x07, 0x54, 0x72, 0x61, 0x63, 0x65, 0x49, 0x64, 0x12, 0x1e, 0x0a, 0x0a, 0x72, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x5f, 0x69, 0x64, 0x18, 0x02, 0x20, 0x01, 0x28, - 0x09, 0x52, 0x0a, 0x72, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x5f, 0x69, 0x64, 0x12, 0x45, 0x0a, - 0x0e, 0x72, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x5f, 0x66, 0x69, 0x65, 0x6c, 0x64, 0x73, 0x18, - 0x03, 0x20, 0x03, 0x28, 0x0b, 0x32, 0x1d, 0x2e, 0x63, 0x6f, 0x6e, 0x74, 0x72, 0x6f, 0x6c, 0x6c, - 0x65, 0x72, 0x2e, 0x61, 0x70, 0x69, 0x2e, 0x76, 0x31, 0x2e, 0x46, 0x69, 0x65, 0x6c, 0x64, 0x45, - 0x72, 0x72, 0x6f, 0x72, 0x52, 0x0e, 0x72, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x5f, 0x66, 0x69, - 0x65, 0x6c, 0x64, 0x73, 0x22, 0x42, 0x0a, 0x0a, 0x46, 0x69, 0x65, 0x6c, 0x64, 0x45, 0x72, 0x72, - 0x6f, 0x72, 0x12, 0x12, 0x0a, 0x04, 0x6e, 0x61, 0x6d, 0x65, 0x18, 0x01, 0x20, 0x01, 0x28, 0x09, - 0x52, 0x04, 0x6e, 0x61, 0x6d, 0x65, 0x12, 0x20, 0x0a, 0x0b, 0x64, 0x65, 0x73, 0x63, 0x72, 0x69, - 0x70, 0x74, 0x69, 0x6f, 0x6e, 0x18, 0x02, 0x20, 0x01, 0x28, 0x09, 0x52, 0x0b, 0x64, 0x65, 0x73, - 0x63, 0x72, 0x69, 0x70, 0x74, 0x69, 0x6f, 0x6e, 0x22, 0x88, 0x01, 0x0a, 0x05, 0x45, 0x72, 0x72, - 0x6f, 0x72, 0x12, 0x16, 0x0a, 0x06, 0x73, 0x74, 0x61, 0x74, 0x75, 0x73, 0x18, 0x01, 0x20, 0x01, - 0x28, 0x05, 0x52, 0x06, 0x73, 0x74, 0x61, 0x74, 0x75, 0x73, 0x12, 0x12, 0x0a, 0x04, 0x63, 0x6f, - 0x64, 0x65, 0x18, 0x02, 0x20, 0x01, 0x28, 0x09, 0x52, 0x04, 0x63, 0x6f, 0x64, 0x65, 0x12, 0x18, - 0x0a, 0x07, 0x6d, 0x65, 0x73, 0x73, 0x61, 0x67, 0x65, 0x18, 0x03, 0x20, 0x01, 0x28, 0x09, 0x52, - 0x07, 0x6d, 0x65, 0x73, 0x73, 0x61, 0x67, 0x65, 0x12, 0x39, 0x0a, 0x07, 0x64, 0x65, 0x74, 0x61, - 0x69, 0x6c, 0x73, 0x18, 0x04, 0x20, 0x01, 0x28, 0x0b, 0x32, 0x1f, 0x2e, 0x63, 0x6f, 0x6e, 0x74, - 0x72, 0x6f, 0x6c, 0x6c, 0x65, 0x72, 0x2e, 0x61, 0x70, 0x69, 0x2e, 0x76, 0x31, 0x2e, 0x45, 0x72, - 0x72, 0x6f, 0x72, 0x44, 0x65, 0x74, 0x61, 0x69, 0x6c, 0x73, 0x52, 0x07, 0x64, 0x65, 0x74, 0x61, - 0x69, 0x6c, 0x73, 0x42, 0x3f, 0x5a, 0x3d, 0x67, 0x69, 0x74, 0x68, 0x75, 0x62, 0x2e, 0x63, 0x6f, - 0x6d, 0x2f, 0x68, 0x61, 0x73, 0x68, 0x69, 0x63, 0x6f, 0x72, 0x70, 0x2f, 0x62, 0x6f, 0x75, 0x6e, - 0x64, 0x61, 0x72, 0x79, 0x2f, 0x69, 0x6e, 0x74, 0x65, 0x72, 0x6e, 0x61, 0x6c, 0x2f, 0x67, 0x65, - 0x6e, 0x2f, 0x63, 0x6f, 0x6e, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x65, 0x72, 0x2f, 0x61, 0x70, 0x69, - 0x3b, 0x61, 0x70, 0x69, 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33, + 0x09, 0x52, 0x0a, 0x72, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x5f, 0x69, 0x64, 0x12, 0x1a, 0x0a, + 0x08, 0x65, 0x72, 0x72, 0x6f, 0x72, 0x5f, 0x69, 0x64, 0x18, 0x03, 0x20, 0x01, 0x28, 0x09, 0x52, + 0x08, 0x65, 0x72, 0x72, 0x6f, 0x72, 0x5f, 0x69, 0x64, 0x12, 0x45, 0x0a, 0x0e, 0x72, 0x65, 0x71, + 0x75, 0x65, 0x73, 0x74, 0x5f, 0x66, 0x69, 0x65, 0x6c, 0x64, 0x73, 0x18, 0x04, 0x20, 0x03, 0x28, + 0x0b, 0x32, 0x1d, 0x2e, 0x63, 0x6f, 0x6e, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x65, 0x72, 0x2e, 0x61, + 0x70, 0x69, 0x2e, 0x76, 0x31, 0x2e, 0x46, 0x69, 0x65, 0x6c, 0x64, 0x45, 0x72, 0x72, 0x6f, 0x72, + 0x52, 0x0e, 0x72, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x5f, 0x66, 0x69, 0x65, 0x6c, 0x64, 0x73, + 0x22, 0x42, 0x0a, 0x0a, 0x46, 0x69, 0x65, 0x6c, 0x64, 0x45, 0x72, 0x72, 0x6f, 0x72, 0x12, 0x12, + 0x0a, 0x04, 0x6e, 0x61, 0x6d, 0x65, 0x18, 0x01, 0x20, 0x01, 0x28, 0x09, 0x52, 0x04, 0x6e, 0x61, + 0x6d, 0x65, 0x12, 0x20, 0x0a, 0x0b, 0x64, 0x65, 0x73, 0x63, 0x72, 0x69, 0x70, 0x74, 0x69, 0x6f, + 0x6e, 0x18, 0x02, 0x20, 0x01, 0x28, 0x09, 0x52, 0x0b, 0x64, 0x65, 0x73, 0x63, 0x72, 0x69, 0x70, + 0x74, 0x69, 0x6f, 0x6e, 0x22, 0x88, 0x01, 0x0a, 0x05, 0x45, 0x72, 0x72, 0x6f, 0x72, 0x12, 0x16, + 0x0a, 0x06, 0x73, 0x74, 0x61, 0x74, 0x75, 0x73, 0x18, 0x01, 0x20, 0x01, 0x28, 0x05, 0x52, 0x06, + 0x73, 0x74, 0x61, 0x74, 0x75, 0x73, 0x12, 0x12, 0x0a, 0x04, 0x63, 0x6f, 0x64, 0x65, 0x18, 0x02, + 0x20, 0x01, 0x28, 0x09, 0x52, 0x04, 0x63, 0x6f, 0x64, 0x65, 0x12, 0x18, 0x0a, 0x07, 0x6d, 0x65, + 0x73, 0x73, 0x61, 0x67, 0x65, 0x18, 0x03, 0x20, 0x01, 0x28, 0x09, 0x52, 0x07, 0x6d, 0x65, 0x73, + 0x73, 0x61, 0x67, 0x65, 0x12, 0x39, 0x0a, 0x07, 0x64, 0x65, 0x74, 0x61, 0x69, 0x6c, 0x73, 0x18, + 0x04, 0x20, 0x01, 0x28, 0x0b, 0x32, 0x1f, 0x2e, 0x63, 0x6f, 0x6e, 0x74, 0x72, 0x6f, 0x6c, 0x6c, + 0x65, 0x72, 0x2e, 0x61, 0x70, 0x69, 0x2e, 0x76, 0x31, 0x2e, 0x45, 0x72, 0x72, 0x6f, 0x72, 0x44, + 0x65, 0x74, 0x61, 0x69, 0x6c, 0x73, 0x52, 0x07, 0x64, 0x65, 0x74, 0x61, 0x69, 0x6c, 0x73, 0x42, + 0x3f, 0x5a, 0x3d, 0x67, 0x69, 0x74, 0x68, 0x75, 0x62, 0x2e, 0x63, 0x6f, 0x6d, 0x2f, 0x68, 0x61, + 0x73, 0x68, 0x69, 0x63, 0x6f, 0x72, 0x70, 0x2f, 0x62, 0x6f, 0x75, 0x6e, 0x64, 0x61, 0x72, 0x79, + 0x2f, 0x69, 0x6e, 0x74, 0x65, 0x72, 0x6e, 0x61, 0x6c, 0x2f, 0x67, 0x65, 0x6e, 0x2f, 0x63, 0x6f, + 0x6e, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x65, 0x72, 0x2f, 0x61, 0x70, 0x69, 0x3b, 0x61, 0x70, 0x69, + 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33, } var ( diff --git a/internal/iam/repository_user.go b/internal/iam/repository_user.go index e20caa322..1b22bdafc 100644 --- a/internal/iam/repository_user.go +++ b/internal/iam/repository_user.go @@ -30,7 +30,7 @@ func (r *Repository) CreateUser(ctx context.Context, user *User, opt ...Option) resource, err := r.create(ctx, u.(*User)) if err != nil { if db.IsUniqueError(err) { - return nil, fmt.Errorf("create user: user %s already exists in org %s", user.Name, user.ScopeId) + return nil, fmt.Errorf("create user: user %s already exists in org %s: %w", user.Name, user.ScopeId, err) } return nil, fmt.Errorf("create user: %w for %s", err, u.(*User).PublicId) } diff --git a/internal/iam/repository_user_test.go b/internal/iam/repository_user_test.go index 797278eb7..e9d33630e 100644 --- a/internal/iam/repository_user_test.go +++ b/internal/iam/repository_user_test.go @@ -87,7 +87,7 @@ func TestRepository_CreateUser(t *testing.T) { assert.Nil(u) switch tt.name { case "dup-name": - assert.Equal(fmt.Sprintf(tt.wantErrMsg, "dup-name"+id, org.PublicId), err.Error()) + assert.Contains(err.Error(), fmt.Sprintf(tt.wantErrMsg, "dup-name"+id, org.PublicId)) default: assert.True(strings.HasPrefix(err.Error(), tt.wantErrMsg)) } diff --git a/internal/proto/local/controller/api/v1/error.proto b/internal/proto/local/controller/api/v1/error.proto index e42f520c4..e2fe61140 100644 --- a/internal/proto/local/controller/api/v1/error.proto +++ b/internal/proto/local/controller/api/v1/error.proto @@ -7,7 +7,8 @@ option go_package = "github.com/hashicorp/boundary/internal/gen/controller/api;a message ErrorDetails { string TraceId = 1 [json_name="TraceId"]; string request_id = 2 [json_name="request_id"]; - repeated FieldError request_fields = 3 [json_name="request_fields"]; + string error_id = 3 [json_name="error_id"]; + repeated FieldError request_fields = 4 [json_name="request_fields"]; } // FieldErrors contains error information on a per field basis. diff --git a/internal/servers/controller/handlers/accounts/account_service.go b/internal/servers/controller/handlers/accounts/account_service.go index ded13e3be..b22bb20d4 100644 --- a/internal/servers/controller/handlers/accounts/account_service.go +++ b/internal/servers/controller/handlers/accounts/account_service.go @@ -252,6 +252,11 @@ func (s Service) updateInRepo(ctx context.Context, scopeId, authMethId, id strin } out, rowsUpdated, err := repo.UpdateAccount(ctx, scopeId, u, version, dbMask) if err != nil { + switch { + case errors.Is(err, password.ErrTooShort): + 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) } if rowsUpdated == 0 { @@ -302,6 +307,16 @@ func (s Service) changePasswordInRepo(ctx context.Context, scopeId, id string, v } out, err := repo.ChangePassword(ctx, scopeId, id, currentPassword, newPassword, version) if err != nil { + switch { + case errors.Is(err, db.ErrRecordNotFound): + return nil, handlers.NotFoundErrorf("Account not found.") + case errors.Is(err, password.ErrTooShort): + return nil, handlers.InvalidArgumentErrorf("Error in provided request.", + map[string]string{"new_password": "Password is too short."}) + case errors.Is(err, password.ErrPasswordsEqual): + 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) } if out == nil { @@ -310,13 +325,20 @@ func (s Service) changePasswordInRepo(ctx context.Context, scopeId, id string, v return toProto(out) } -func (s Service) setPasswordInRepo(ctx context.Context, scopeId, id string, version uint32, password string) (*pb.Account, error) { +func (s Service) setPasswordInRepo(ctx context.Context, scopeId, id string, version uint32, pw string) (*pb.Account, error) { repo, err := s.repoFn() if err != nil { return nil, err } - out, err := repo.SetPassword(ctx, scopeId, id, password, version) + out, err := repo.SetPassword(ctx, scopeId, id, pw, version) if err != nil { + switch { + case errors.Is(err, db.ErrRecordNotFound): + return nil, handlers.NotFoundErrorf("Account not found.") + case errors.Is(err, password.ErrTooShort): + 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 toProto(out) @@ -447,7 +469,7 @@ func validateListRequest(req *pbs.ListAccountsRequest) error { badFields["auth_method_id"] = "Invalid formatted identifier." } if len(badFields) > 0 { - return handlers.InvalidArgumentErrorf("Improperly formatted identifier.", badFields) + return handlers.InvalidArgumentErrorf("Error in provided request.", badFields) } return nil } @@ -467,7 +489,7 @@ func validateChangePasswordRequest(req *pbs.ChangePasswordRequest) error { badFields["current_password"] = "This is a required field." } if len(badFields) > 0 { - return handlers.InvalidArgumentErrorf("Improperly formatted identifier.", badFields) + return handlers.InvalidArgumentErrorf("Error in provided request.", badFields) } return nil } @@ -481,7 +503,7 @@ func validateSetPasswordRequest(req *pbs.SetPasswordRequest) error { badFields["version"] = "Existing resource version is required for an update." } 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/accounts/account_service_test.go b/internal/servers/controller/handlers/accounts/account_service_test.go index 57f37e41a..f46a701b3 100644 --- a/internal/servers/controller/handlers/accounts/account_service_test.go +++ b/internal/servers/controller/handlers/accounts/account_service_test.go @@ -1,6 +1,7 @@ package accounts_test import ( + "errors" "fmt" "strings" "testing" @@ -23,7 +24,6 @@ import ( "github.com/stretchr/testify/require" "google.golang.org/genproto/protobuf/field_mask" "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" "google.golang.org/protobuf/testing/protocmp" "google.golang.org/protobuf/types/known/structpb" "google.golang.org/protobuf/types/known/wrapperspb" @@ -57,41 +57,43 @@ func TestGet(t *testing.T) { } cases := []struct { - name string - req *pbs.GetAccountRequest - res *pbs.GetAccountResponse - errCode codes.Code + name string + req *pbs.GetAccountRequest + res *pbs.GetAccountResponse + err error }{ { - name: "Get an existing account", - req: &pbs.GetAccountRequest{Id: wireAccount.GetId()}, - res: &pbs.GetAccountResponse{Item: &wireAccount}, - errCode: codes.OK, + name: "Get an existing account", + req: &pbs.GetAccountRequest{Id: wireAccount.GetId()}, + res: &pbs.GetAccountResponse{Item: &wireAccount}, }, { - name: "Get a non existing account", - req: &pbs.GetAccountRequest{Id: password.AccountPrefix + "_DoesntExis"}, - res: nil, - errCode: codes.NotFound, + name: "Get a non existing account", + req: &pbs.GetAccountRequest{Id: password.AccountPrefix + "_DoesntExis"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { - name: "Wrong id prefix", - req: &pbs.GetAccountRequest{Id: "j_1234567890"}, - res: nil, - errCode: codes.InvalidArgument, + name: "Wrong id prefix", + req: &pbs.GetAccountRequest{Id: "j_1234567890"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { - name: "space in id", - req: &pbs.GetAccountRequest{Id: authtoken.AuthTokenPrefix + "_1 23456789"}, - res: nil, - errCode: codes.InvalidArgument, + name: "space in id", + req: &pbs.GetAccountRequest{Id: authtoken.AuthTokenPrefix + "_1 23456789"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) got, gErr := s.GetAccount(auth.DisabledAuthTestContext(auth.WithScopeId(org.GetPublicId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "GetAccount(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "GetAccount(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } assert.Empty(cmp.Diff(got, tc.res, protocmp.Transform()), "GetAccount(%q) got response %q, wanted %q", tc.req, got, tc.res) }) } @@ -142,40 +144,41 @@ func TestList(t *testing.T) { name string authMethod string res *pbs.ListAccountsResponse - errCode codes.Code + err error }{ { name: "List Some Accounts", authMethod: amSomeAccounts.GetPublicId(), res: &pbs.ListAccountsResponse{Items: wantSomeAccounts}, - errCode: codes.OK, }, { name: "List Other Accounts", authMethod: amOtherAccounts.GetPublicId(), res: &pbs.ListAccountsResponse{Items: wantOtherAccounts}, - errCode: codes.OK, }, { name: "List No Accounts", authMethod: amNoAccounts.GetPublicId(), res: &pbs.ListAccountsResponse{}, - errCode: codes.OK, }, { name: "Unfound Auth Method", authMethod: password.AuthMethodPrefix + "_DoesntExis", - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { + assert, require := assert.New(t), require.New(t) s, err := accounts.NewService(repoFn) - require.NoError(t, err, "Couldn't create new user service.") + require.NoError(err, "Couldn't create new user service.") got, gErr := s.ListAccounts(auth.DisabledAuthTestContext(auth.WithScopeId(o.GetPublicId())), &pbs.ListAccountsRequest{AuthMethodId: tc.authMethod}) - assert.Equal(t, tc.errCode, status.Code(gErr), "ListAccounts() with auth method %q got error %v, wanted %v", tc.authMethod, gErr, tc.errCode) - assert.Empty(t, cmp.Diff(got, tc.res, protocmp.Transform(), protocmp.SortRepeatedFields(got)), "ListUsers() with scope %q got response %q, wanted %q", tc.authMethod, got, tc.res) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "ListAccounts() with auth method %q got error %v, wanted %v", tc.authMethod, gErr, tc.err) + } + assert.Empty(cmp.Diff(got, tc.res, protocmp.Transform(), protocmp.SortRepeatedFields(got)), "ListUsers() with scope %q got response %q, wanted %q", tc.authMethod, got, tc.res) }) } } @@ -197,47 +200,49 @@ func TestDelete(t *testing.T) { require.NoError(t, err, "Error when getting new user service.") cases := []struct { - name string - scope string - req *pbs.DeleteAccountRequest - res *pbs.DeleteAccountResponse - errCode codes.Code + name string + scope string + req *pbs.DeleteAccountRequest + res *pbs.DeleteAccountResponse + err error }{ { name: "Delete an existing token", req: &pbs.DeleteAccountRequest{ Id: ac.GetPublicId(), }, - res: &pbs.DeleteAccountResponse{}, - errCode: codes.OK, + res: &pbs.DeleteAccountResponse{}, }, { name: "Delete bad account id", req: &pbs.DeleteAccountRequest{ Id: password.AccountPrefix + "_doesntexis", }, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { name: "Bad account id formatting", req: &pbs.DeleteAccountRequest{ Id: "bad_format", }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) got, gErr := s.DeleteAccount(auth.DisabledAuthTestContext(auth.WithScopeId(o.GetPublicId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "DeleteAccount(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "DeleteAccount(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } assert.EqualValuesf(tc.res, got, "DeleteAccount(%q) got response %q, wanted %q", tc.req, got, tc.res) }) } } func TestDelete_twice(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) conn, _ := db.TestSetup(t, "postgres") wrap := db.TestWrapper(t) rw := db.New(conn) @@ -251,7 +256,7 @@ func TestDelete_twice(t *testing.T) { ac := password.TestAccounts(t, conn, am.GetPublicId(), 1)[0] s, err := accounts.NewService(repoFn) - require.NoError(t, err, "Error when getting new user service") + require.NoError(err, "Error when getting new user service") req := &pbs.DeleteAccountRequest{ Id: ac.GetPublicId(), } @@ -259,7 +264,7 @@ func TestDelete_twice(t *testing.T) { assert.NoError(gErr, "First attempt") _, gErr = s.DeleteAccount(auth.DisabledAuthTestContext(auth.WithScopeId(o.GetPublicId())), req) assert.Error(gErr, "Second attempt") - assert.Equal(codes.NotFound, status.Code(gErr), "Expected permission denied for the second delete.") + assert.True(errors.Is(gErr, handlers.ApiErrorWithCode(codes.NotFound)), "Expected permission denied for the second delete.") } func TestCreate(t *testing.T) { @@ -291,10 +296,10 @@ func TestCreate(t *testing.T) { } cases := []struct { - name string - req *pbs.CreateAccountRequest - res *pbs.CreateAccountResponse - errCode codes.Code + name string + req *pbs.CreateAccountRequest + res *pbs.CreateAccountResponse + err error }{ { name: "Create a valid Account", @@ -319,7 +324,6 @@ func TestCreate(t *testing.T) { Attributes: createAttr("validaccount", ""), }, }, - errCode: codes.OK, }, { name: "Create a valid Account without type defined", @@ -339,7 +343,6 @@ func TestCreate(t *testing.T) { Attributes: createAttr("notypedefined", ""), }, }, - errCode: codes.OK, }, { name: "Create a valid Account with password defined", @@ -363,7 +366,6 @@ func TestCreate(t *testing.T) { Attributes: createAttr("haspassword", ""), }, }, - errCode: codes.OK, }, { name: "Cant specify mismatching type", @@ -374,8 +376,8 @@ func TestCreate(t *testing.T) { Attributes: createAttr("nopwprovided", ""), }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Can't specify Id", @@ -387,8 +389,8 @@ func TestCreate(t *testing.T) { Attributes: createAttr("cantprovideid", ""), }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Can't specify Created Time", @@ -400,8 +402,8 @@ func TestCreate(t *testing.T) { Attributes: createAttr("nocreatedtime", ""), }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Can't specify Update Time", @@ -413,8 +415,8 @@ func TestCreate(t *testing.T) { Attributes: createAttr("noupdatetime", ""), }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Must specify login name for password type", @@ -424,15 +426,18 @@ func TestCreate(t *testing.T) { Type: "password", }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { assert, require := assert.New(t), require.New(t) got, gErr := s.CreateAccount(auth.DisabledAuthTestContext(auth.WithScopeId(o.GetPublicId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "CreateAccount(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "CreateAccount(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } if got != nil { assert.Contains(got.GetUri(), tc.res.Uri) assert.True(strings.HasPrefix(got.GetItem().GetId(), password.AccountPrefix+"_")) @@ -500,10 +505,10 @@ func TestUpdate(t *testing.T) { } cases := []struct { - name string - req *pbs.UpdateAccountRequest - res *pbs.UpdateAccountResponse - errCode codes.Code + name string + req *pbs.UpdateAccountRequest + res *pbs.UpdateAccountResponse + err error }{ { name: "Update an Existing AuthMethod", @@ -527,7 +532,6 @@ func TestUpdate(t *testing.T) { Scope: defaultScopeInfo, }, }, - errCode: codes.OK, }, { name: "Multiple Paths in single string", @@ -551,7 +555,6 @@ func TestUpdate(t *testing.T) { Scope: defaultScopeInfo, }, }, - errCode: codes.OK, }, { name: "No Update Mask", @@ -562,7 +565,7 @@ func TestUpdate(t *testing.T) { Attributes: modifiedAttributes, }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant change type", @@ -575,7 +578,7 @@ func TestUpdate(t *testing.T) { Type: "oidc", }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "No Paths in Mask", @@ -587,7 +590,7 @@ func TestUpdate(t *testing.T) { Attributes: modifiedAttributes, }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Only non-existant paths in Mask", @@ -599,7 +602,7 @@ func TestUpdate(t *testing.T) { Attributes: modifiedAttributes, }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Unset Name", @@ -621,7 +624,6 @@ func TestUpdate(t *testing.T) { Scope: defaultScopeInfo, }, }, - errCode: codes.OK, }, { name: "Update Only Name", @@ -645,7 +647,6 @@ func TestUpdate(t *testing.T) { Scope: defaultScopeInfo, }, }, - errCode: codes.OK, }, { name: "Update Only Description", @@ -669,7 +670,6 @@ func TestUpdate(t *testing.T) { Scope: defaultScopeInfo, }, }, - errCode: codes.OK, }, { name: "Update Only LoginName", @@ -693,7 +693,6 @@ func TestUpdate(t *testing.T) { Scope: defaultScopeInfo, }, }, - errCode: codes.OK, }, { name: "Update a Non Existing Account", @@ -707,7 +706,7 @@ func TestUpdate(t *testing.T) { Description: &wrapperspb.StringValue{Value: "desc"}, }, }, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { name: "Cant change Id", @@ -720,8 +719,8 @@ func TestUpdate(t *testing.T) { Name: &wrapperspb.StringValue{Value: "new"}, Description: &wrapperspb.StringValue{Value: "new desc"}, }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant specify Created Time", @@ -733,8 +732,8 @@ func TestUpdate(t *testing.T) { CreatedTime: ptypes.TimestampNow(), }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant specify Updated Time", @@ -746,8 +745,8 @@ func TestUpdate(t *testing.T) { UpdatedTime: ptypes.TimestampNow(), }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant specify Type", @@ -759,8 +758,8 @@ func TestUpdate(t *testing.T) { Type: "oidc", }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { @@ -781,7 +780,10 @@ func TestUpdate(t *testing.T) { } got, gErr := tested.UpdateAccount(auth.DisabledAuthTestContext(auth.WithScopeId(o.GetPublicId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "UpdateAccount(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "UpdateAccount(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } if tc.res == nil { require.Nil(got) diff --git a/internal/servers/controller/handlers/authenticate/authenticate_service.go b/internal/servers/controller/handlers/authenticate/authenticate_service.go index 9ab7152ed..127f0fb68 100644 --- a/internal/servers/controller/handlers/authenticate/authenticate_service.go +++ b/internal/servers/controller/handlers/authenticate/authenticate_service.go @@ -18,7 +18,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" ) const ( @@ -77,7 +76,7 @@ func (s Service) Authenticate(ctx context.Context, req *pbs.AuthenticateRequest) // Deauthenticate implements the interface pbs.AuthenticationServiceServer. func (s Service) Deauthenticate(ctx context.Context, req *pbs.DeauthenticateRequest) (*pbs.DeauthenticateResponse, error) { - return nil, status.Error(codes.Unimplemented, "Requested method is unimplemented.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Unimplemented, "Requested method is unimplemented.") } func (s Service) authenticateWithRepo(ctx context.Context, scopeId, authMethodId, loginName, pw string) (*pba.AuthToken, error) { @@ -99,7 +98,7 @@ func (s Service) authenticateWithRepo(ctx context.Context, scopeId, authMethodId return nil, err } if acct == nil { - return nil, status.Error(codes.Unauthenticated, "Unable to authenticate.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Unauthenticated, "Unable to authenticate.") } u, err := iamRepo.LookupUserWithLogin(ctx, acct.GetPublicId(), iam.WithAutoVivify(true)) diff --git a/internal/servers/controller/handlers/authenticate/authenticate_service_test.go b/internal/servers/controller/handlers/authenticate/authenticate_service_test.go index 2b352f599..7c9116cd8 100644 --- a/internal/servers/controller/handlers/authenticate/authenticate_service_test.go +++ b/internal/servers/controller/handlers/authenticate/authenticate_service_test.go @@ -2,6 +2,7 @@ package authenticate import ( "context" + "errors" "strings" "testing" @@ -14,10 +15,10 @@ import ( pbs "github.com/hashicorp/boundary/internal/gen/controller/api/services" "github.com/hashicorp/boundary/internal/iam" "github.com/hashicorp/boundary/internal/kms" + "github.com/hashicorp/boundary/internal/servers/controller/handlers" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" ) const ( @@ -98,7 +99,7 @@ func TestAuthenticate(t *testing.T) { return &structpb.Struct{Fields: creds} }(), }, - wantErr: status.Error(codes.InvalidArgument, "invalid argument"), + wantErr: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "no-authmethod", @@ -111,7 +112,7 @@ func TestAuthenticate(t *testing.T) { return &structpb.Struct{Fields: creds} }(), }, - wantErr: status.Error(codes.InvalidArgument, "invalid argument"), + wantErr: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "wrong-password", @@ -126,7 +127,7 @@ func TestAuthenticate(t *testing.T) { return &structpb.Struct{Fields: creds} }(), }, - wantErr: status.Error(codes.Unauthenticated, "unauthenticated"), + wantErr: handlers.ApiErrorWithCode(codes.Unauthenticated), }, { name: "wrong-login-name", @@ -141,7 +142,7 @@ func TestAuthenticate(t *testing.T) { return &structpb.Struct{Fields: creds} }(), }, - wantErr: status.Error(codes.Unauthenticated, "unauthenticated"), + wantErr: handlers.ApiErrorWithCode(codes.Unauthenticated), }, } @@ -154,7 +155,7 @@ func TestAuthenticate(t *testing.T) { resp, err := s.Authenticate(auth.DisabledAuthTestContext(auth.WithScopeId(o.GetPublicId())), tc.request) if tc.wantErr != nil { assert.Error(err) - assert.Equal(status.Code(tc.wantErr), status.Code(err)) + assert.Truef(errors.Is(err, tc.wantErr), "Got %#v, wanted %#v", err, tc.wantErr) return } require.NoError(err) diff --git a/internal/servers/controller/handlers/authmethods/authmethod_service_test.go b/internal/servers/controller/handlers/authmethods/authmethod_service_test.go index 8662b4fc4..a70b8b419 100644 --- a/internal/servers/controller/handlers/authmethods/authmethod_service_test.go +++ b/internal/servers/controller/handlers/authmethods/authmethod_service_test.go @@ -1,6 +1,7 @@ package authmethods_test import ( + "errors" "fmt" "strings" "testing" @@ -15,11 +16,11 @@ import ( pbs "github.com/hashicorp/boundary/internal/gen/controller/api/services" "github.com/hashicorp/boundary/internal/iam" "github.com/hashicorp/boundary/internal/kms" + "github.com/hashicorp/boundary/internal/servers/controller/handlers" "github.com/hashicorp/boundary/internal/servers/controller/handlers/authmethods" "github.com/hashicorp/boundary/internal/types/scope" "google.golang.org/genproto/protobuf/field_mask" "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" "google.golang.org/protobuf/testing/protocmp" "google.golang.org/protobuf/types/known/structpb" "google.golang.org/protobuf/types/known/wrapperspb" @@ -66,35 +67,34 @@ func TestGet(t *testing.T) { scopeId string req *pbs.GetAuthMethodRequest res *pbs.GetAuthMethodResponse - errCode codes.Code + err error }{ { name: "Get an Existing AuthMethod", scopeId: o.GetPublicId(), req: &pbs.GetAuthMethodRequest{Id: am.GetPublicId()}, res: &pbs.GetAuthMethodResponse{Item: wantU}, - errCode: codes.OK, }, { name: "Get a non existant AuthMethod", scopeId: o.GetPublicId(), req: &pbs.GetAuthMethodRequest{Id: password.AuthMethodPrefix + "_DoesntExis"}, res: nil, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { name: "Wrong id prefix", scopeId: o.GetPublicId(), req: &pbs.GetAuthMethodRequest{Id: "j_1234567890"}, res: nil, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "space in id", scopeId: o.GetPublicId(), req: &pbs.GetAuthMethodRequest{Id: password.AuthMethodPrefix + "_1 23456789"}, res: nil, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { @@ -105,7 +105,10 @@ func TestGet(t *testing.T) { require.NoError(err, "Couldn't create new auth_method service.") got, gErr := s.GetAuthMethod(auth.DisabledAuthTestContext(auth.WithScopeId(tc.scopeId)), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "GetAuthMethod(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "GetAuthMethod(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } assert.Empty(cmp.Diff(got, tc.res, protocmp.Transform()), "GetAuthMethod(%q) got response %q, wanted %q", tc.req, got, tc.res) }) } @@ -166,30 +169,27 @@ func TestList(t *testing.T) { name string scopeId string res *pbs.ListAuthMethodsResponse - errCode codes.Code + err error }{ { name: "List Some Auth Methods", scopeId: oWithAuthMethods.GetPublicId(), res: &pbs.ListAuthMethodsResponse{Items: wantSomeAuthMethods}, - errCode: codes.OK, }, { name: "List Other Auth Methods", scopeId: oWithOtherAuthMethods.GetPublicId(), res: &pbs.ListAuthMethodsResponse{Items: wantOtherAuthMethods}, - errCode: codes.OK, }, { name: "List No Auth Methods", scopeId: oNoAuthMethods.GetPublicId(), res: &pbs.ListAuthMethodsResponse{}, - errCode: codes.OK, }, { name: "Unfound Auth Method", scopeId: "o_DoesntExis", - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, } for _, tc := range cases { @@ -199,7 +199,10 @@ func TestList(t *testing.T) { require.NoError(err, "Couldn't create new auth_method service.") got, gErr := s.ListAuthMethods(auth.DisabledAuthTestContext(auth.WithScopeId(tc.scopeId)), &pbs.ListAuthMethodsRequest{ScopeId: tc.scopeId}) - assert.Equal(tc.errCode, status.Code(gErr), "ListAuthMethods() for scope %q got error %v, wanted %v", tc.scopeId, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "ListAuthMethods() for scope %q got error %v, wanted %v", tc.scopeId, gErr, tc.err) + } assert.Empty(cmp.Diff(got, tc.res, protocmp.Transform()), "ListAuthMethods() for scope %q got response %q, wanted %q", tc.scopeId, got, tc.res) }) } @@ -225,39 +228,41 @@ func TestDelete(t *testing.T) { require.NoError(t, err, "Error when getting new auth_method service.") cases := []struct { - name string - req *pbs.DeleteAuthMethodRequest - res *pbs.DeleteAuthMethodResponse - errCode codes.Code + name string + req *pbs.DeleteAuthMethodRequest + res *pbs.DeleteAuthMethodResponse + err error }{ { name: "Delete an Existing AuthMethod", req: &pbs.DeleteAuthMethodRequest{ Id: am.GetPublicId(), }, - res: &pbs.DeleteAuthMethodResponse{}, - errCode: codes.OK, + res: &pbs.DeleteAuthMethodResponse{}, }, { name: "Delete bad auth_method id", req: &pbs.DeleteAuthMethodRequest{ Id: password.AuthMethodPrefix + "_doesntexis", }, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { name: "Bad AuthMethod Id formatting", req: &pbs.DeleteAuthMethodRequest{ Id: "bad_format", }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) got, gErr := s.DeleteAuthMethod(auth.DisabledAuthTestContext(auth.WithScopeId(o.GetPublicId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "DeleteAuthMethod(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "DeleteAuthMethod(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } assert.EqualValuesf(tc.res, got, "DeleteAuthMethod(%q) got response %q, wanted %q", tc.req, got, tc.res) }) } @@ -290,7 +295,7 @@ func TestDelete_twice(t *testing.T) { assert.NoError(gErr, "First attempt") _, gErr = s.DeleteAuthMethod(auth.DisabledAuthTestContext(auth.WithScopeId(o.GetPublicId())), req) assert.Error(gErr, "Second attempt") - assert.Equal(codes.NotFound, status.Code(gErr), "Expected permission denied for the second delete.") + assert.True(errors.Is(gErr, handlers.ApiErrorWithCode(codes.NotFound)), "Expected permission denied for the second delete.") } func TestCreate(t *testing.T) { @@ -312,10 +317,10 @@ func TestCreate(t *testing.T) { require.NoError(t, err, "Error converting proto to timestamp.") cases := []struct { - name string - req *pbs.CreateAuthMethodRequest - res *pbs.CreateAuthMethodResponse - errCode codes.Code + name string + req *pbs.CreateAuthMethodRequest + res *pbs.CreateAuthMethodResponse + err error }{ { name: "Create a valid AuthMethod", @@ -343,7 +348,6 @@ func TestCreate(t *testing.T) { }}, }, }, - errCode: codes.OK, }, { name: "Create a global AuthMethod", @@ -371,7 +375,6 @@ func TestCreate(t *testing.T) { }}, }, }, - errCode: codes.OK, }, { name: "Can't specify Id", @@ -380,8 +383,8 @@ func TestCreate(t *testing.T) { Id: password.AuthMethodPrefix + "_notallowed", Type: "password", }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Can't specify Created Time", @@ -389,8 +392,8 @@ func TestCreate(t *testing.T) { ScopeId: o.GetPublicId(), CreatedTime: ptypes.TimestampNow(), }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Can't specify Update Time", @@ -399,8 +402,8 @@ func TestCreate(t *testing.T) { UpdatedTime: ptypes.TimestampNow(), Type: "password", }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Can't specify Update Time", @@ -409,8 +412,8 @@ func TestCreate(t *testing.T) { UpdatedTime: ptypes.TimestampNow(), Type: "password", }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Must specify type", @@ -419,8 +422,8 @@ func TestCreate(t *testing.T) { Name: &wrapperspb.StringValue{Value: "must specify type"}, Description: &wrapperspb.StringValue{Value: "must specify type"}, }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Attributes must be valid for type", @@ -434,8 +437,8 @@ func TestCreate(t *testing.T) { "min_login_name_length": structpb.NewNumberValue(3), }}, }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { @@ -446,7 +449,10 @@ func TestCreate(t *testing.T) { require.NoError(err, "Error when getting new auth_method service.") got, gErr := s.CreateAuthMethod(auth.DisabledAuthTestContext(auth.WithScopeId(tc.req.GetItem().GetScopeId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "CreateAuthMethod(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "CreateAuthMethod(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } if tc.res == nil { require.Nil(got) } @@ -510,10 +516,10 @@ func TestUpdate(t *testing.T) { } cases := []struct { - name string - req *pbs.UpdateAuthMethodRequest - res *pbs.UpdateAuthMethodResponse - errCode codes.Code + name string + req *pbs.UpdateAuthMethodRequest + res *pbs.UpdateAuthMethodResponse + err error }{ { name: "Update an Existing AuthMethod", @@ -540,7 +546,6 @@ func TestUpdate(t *testing.T) { Scope: defaultScopeInfo, }, }, - errCode: codes.OK, }, { name: "Multiple Paths in single string", @@ -567,7 +572,6 @@ func TestUpdate(t *testing.T) { Scope: defaultScopeInfo, }, }, - errCode: codes.OK, }, { name: "No Update Mask", @@ -577,7 +581,7 @@ func TestUpdate(t *testing.T) { Description: &wrapperspb.StringValue{Value: "updated desc"}, }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "No Paths in Mask", @@ -588,7 +592,7 @@ func TestUpdate(t *testing.T) { Description: &wrapperspb.StringValue{Value: "updated desc"}, }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Only non-existant paths in Mask", @@ -599,7 +603,7 @@ func TestUpdate(t *testing.T) { Description: &wrapperspb.StringValue{Value: "updated desc"}, }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant change type", @@ -610,7 +614,7 @@ func TestUpdate(t *testing.T) { Type: "oidc", }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Unset Name", @@ -634,7 +638,6 @@ func TestUpdate(t *testing.T) { Scope: defaultScopeInfo, }, }, - errCode: codes.OK, }, { name: "Update Only Name", @@ -660,7 +663,6 @@ func TestUpdate(t *testing.T) { Scope: defaultScopeInfo, }, }, - errCode: codes.OK, }, { name: "Update Only Description", @@ -686,7 +688,6 @@ func TestUpdate(t *testing.T) { Scope: defaultScopeInfo, }, }, - errCode: codes.OK, }, { name: "Update a Non Existing AuthMethod", @@ -700,7 +701,7 @@ func TestUpdate(t *testing.T) { Description: &wrapperspb.StringValue{Value: "desc"}, }, }, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { name: "Cant change Id", @@ -713,8 +714,8 @@ func TestUpdate(t *testing.T) { Name: &wrapperspb.StringValue{Value: "new"}, Description: &wrapperspb.StringValue{Value: "new desc"}, }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant specify Created Time", @@ -726,8 +727,8 @@ func TestUpdate(t *testing.T) { CreatedTime: ptypes.TimestampNow(), }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant specify Updated Time", @@ -739,8 +740,8 @@ func TestUpdate(t *testing.T) { UpdatedTime: ptypes.TimestampNow(), }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant specify Type", @@ -752,8 +753,8 @@ func TestUpdate(t *testing.T) { Type: "oidc", }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Update login name length", @@ -783,7 +784,6 @@ func TestUpdate(t *testing.T) { Scope: defaultScopeInfo, }, }, - errCode: codes.OK, }, { name: "Update password length", @@ -813,7 +813,6 @@ func TestUpdate(t *testing.T) { Scope: defaultScopeInfo, }, }, - errCode: codes.OK, }, } for _, tc := range cases { @@ -834,7 +833,10 @@ func TestUpdate(t *testing.T) { } got, gErr := tested.UpdateAuthMethod(auth.DisabledAuthTestContext(auth.WithScopeId(o.GetPublicId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "UpdateAuthMethod(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "UpdateAuthMethod(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } if tc.res == nil { require.Nil(got) diff --git a/internal/servers/controller/handlers/authtokens/authtoken_service_test.go b/internal/servers/controller/handlers/authtokens/authtoken_service_test.go index 3ac09f1c3..c84e36330 100644 --- a/internal/servers/controller/handlers/authtokens/authtoken_service_test.go +++ b/internal/servers/controller/handlers/authtokens/authtoken_service_test.go @@ -1,6 +1,7 @@ package authtokens_test import ( + "errors" "testing" "github.com/google/go-cmp/cmp" @@ -12,10 +13,10 @@ import ( pbs "github.com/hashicorp/boundary/internal/gen/controller/api/services" "github.com/hashicorp/boundary/internal/iam" "github.com/hashicorp/boundary/internal/kms" + "github.com/hashicorp/boundary/internal/servers/controller/handlers" "github.com/hashicorp/boundary/internal/servers/controller/handlers/authtokens" "github.com/hashicorp/boundary/internal/types/scope" "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" "google.golang.org/protobuf/testing/protocmp" "github.com/stretchr/testify/assert" @@ -54,41 +55,43 @@ func TestGet(t *testing.T) { } cases := []struct { - name string - req *pbs.GetAuthTokenRequest - res *pbs.GetAuthTokenResponse - errCode codes.Code + name string + req *pbs.GetAuthTokenRequest + res *pbs.GetAuthTokenResponse + err error }{ { - name: "Get an existing auth token", - req: &pbs.GetAuthTokenRequest{Id: wireAuthToken.GetId()}, - res: &pbs.GetAuthTokenResponse{Item: &wireAuthToken}, - errCode: codes.OK, + name: "Get an existing auth token", + req: &pbs.GetAuthTokenRequest{Id: wireAuthToken.GetId()}, + res: &pbs.GetAuthTokenResponse{Item: &wireAuthToken}, }, { - name: "Get a non existing auth token", - req: &pbs.GetAuthTokenRequest{Id: authtoken.AuthTokenPrefix + "_DoesntExis"}, - res: nil, - errCode: codes.NotFound, + name: "Get a non existing auth token", + req: &pbs.GetAuthTokenRequest{Id: authtoken.AuthTokenPrefix + "_DoesntExis"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { - name: "Wrong id prefix", - req: &pbs.GetAuthTokenRequest{Id: "j_1234567890"}, - res: nil, - errCode: codes.InvalidArgument, + name: "Wrong id prefix", + req: &pbs.GetAuthTokenRequest{Id: "j_1234567890"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { - name: "space in id", - req: &pbs.GetAuthTokenRequest{Id: authtoken.AuthTokenPrefix + "_1 23456789"}, - res: nil, - errCode: codes.InvalidArgument, + name: "space in id", + req: &pbs.GetAuthTokenRequest{Id: authtoken.AuthTokenPrefix + "_1 23456789"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) got, gErr := s.GetAuthToken(auth.DisabledAuthTestContext(auth.WithScopeId(org.GetPublicId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "GetAuthToken(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "GetAuthToken(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } assert.Empty(cmp.Diff(got, tc.res, protocmp.Transform()), "GetAuthToken(%q) got response %q, wanted %q", tc.req, got, tc.res) }) } @@ -146,34 +149,31 @@ func TestList(t *testing.T) { } cases := []struct { - name string - scope string - res *pbs.ListAuthTokensResponse - errCode codes.Code + name string + scope string + res *pbs.ListAuthTokensResponse + err error }{ { - name: "List Some Tokens", - scope: orgWithSomeTokens.GetPublicId(), - res: &pbs.ListAuthTokensResponse{Items: wantSomeTokens}, - errCode: codes.OK, + name: "List Some Tokens", + scope: orgWithSomeTokens.GetPublicId(), + res: &pbs.ListAuthTokensResponse{Items: wantSomeTokens}, }, { - name: "List Other Tokens", - scope: orgWithOtherTokens.GetPublicId(), - res: &pbs.ListAuthTokensResponse{Items: wantOtherTokens}, - errCode: codes.OK, + name: "List Other Tokens", + scope: orgWithOtherTokens.GetPublicId(), + res: &pbs.ListAuthTokensResponse{Items: wantOtherTokens}, }, { - name: "List No Token", - scope: orgNoTokens.GetPublicId(), - res: &pbs.ListAuthTokensResponse{}, - errCode: codes.OK, + name: "List No Token", + scope: orgNoTokens.GetPublicId(), + res: &pbs.ListAuthTokensResponse{}, }, // TODO: When an org doesn't exist, we should return a 404 instead of an empty list. { - name: "Unfound Org", - scope: scope.Org.Prefix() + "_DoesntExis", - errCode: codes.NotFound, + name: "Unfound Org", + scope: scope.Org.Prefix() + "_DoesntExis", + err: handlers.ApiErrorWithCode(codes.NotFound), }, } for _, tc := range cases { @@ -182,7 +182,10 @@ func TestList(t *testing.T) { require.NoError(t, err, "Couldn't create new user service.") got, gErr := s.ListAuthTokens(auth.DisabledAuthTestContext(auth.WithScopeId(tc.scope)), &pbs.ListAuthTokensRequest{ScopeId: tc.scope}) - assert.Equal(t, tc.errCode, status.Code(gErr), "ListAuthTokens() with scope %q got error %v, wanted %v", tc.scope, gErr, tc.errCode) + if tc.err != nil { + require.Error(t, gErr) + assert.True(t, errors.Is(gErr, tc.err), "ListAuthTokens() with scope %q got error %v, wanted %v", tc.scope, gErr, tc.err) + } assert.Empty(t, cmp.Diff(got, tc.res, protocmp.Transform(), protocmp.SortRepeatedFields(got)), "ListAuthTokens() with scope %q got response %q, wanted %q", tc.scope, got, tc.res) }) } @@ -208,11 +211,11 @@ func TestDelete(t *testing.T) { require.NoError(t, err, "Error when getting new user service.") cases := []struct { - name string - scope string - req *pbs.DeleteAuthTokenRequest - res *pbs.DeleteAuthTokenResponse - errCode codes.Code + name string + scope string + req *pbs.DeleteAuthTokenRequest + res *pbs.DeleteAuthTokenResponse + err error }{ { name: "Delete an existing token", @@ -220,8 +223,7 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteAuthTokenRequest{ Id: at.GetPublicId(), }, - res: &pbs.DeleteAuthTokenResponse{}, - errCode: codes.OK, + res: &pbs.DeleteAuthTokenResponse{}, }, { name: "Delete bad token id", @@ -229,7 +231,7 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteAuthTokenRequest{ Id: authtoken.AuthTokenPrefix + "_doesntexis", }, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { name: "Bad token id formatting", @@ -237,21 +239,24 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteAuthTokenRequest{ Id: "bad_format", }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) got, gErr := s.DeleteAuthToken(auth.DisabledAuthTestContext(auth.WithScopeId(tc.scope)), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "DeleteAuthToken(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "DeleteAuthToken(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } assert.EqualValuesf(tc.res, got, "DeleteAuthToken(%q) got response %q, wanted %q", tc.req, got, tc.res) }) } } func TestDelete_twice(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) conn, _ := db.TestSetup(t, "postgres") rw := db.New(conn) wrap := db.TestWrapper(t) @@ -268,7 +273,7 @@ func TestDelete_twice(t *testing.T) { at := authtoken.TestAuthToken(t, conn, kms, org.GetPublicId()) s, err := authtokens.NewService(repoFn, iamRepoFn) - require.NoError(t, err, "Error when getting new user service") + require.NoError(err, "Error when getting new user service") req := &pbs.DeleteAuthTokenRequest{ Id: at.GetPublicId(), } @@ -276,5 +281,5 @@ func TestDelete_twice(t *testing.T) { assert.NoError(gErr, "First attempt") _, gErr = s.DeleteAuthToken(auth.DisabledAuthTestContext(auth.WithScopeId(at.GetScopeId())), req) assert.Error(gErr, "Second attempt") - assert.Equal(codes.NotFound, status.Code(gErr), "Expected permission denied for the second delete.") + assert.True(errors.Is(gErr, handlers.ApiErrorWithCode(codes.NotFound)), "Expected permission denied for the second delete.") } diff --git a/internal/servers/controller/handlers/errors.go b/internal/servers/controller/handlers/errors.go index e2fe8e494..d6712a872 100644 --- a/internal/servers/controller/handlers/errors.go +++ b/internal/servers/controller/handlers/errors.go @@ -2,97 +2,193 @@ package handlers import ( "context" + "errors" "fmt" "io" "net/http" "sort" + "strings" "github.com/grpc-ecosystem/grpc-gateway/v2/runtime" + "github.com/hashicorp/boundary/internal/db" pb "github.com/hashicorp/boundary/internal/gen/controller/api" "github.com/hashicorp/go-hclog" - "google.golang.org/genproto/googleapis/rpc/errdetails" + "github.com/hashicorp/vault/sdk/helper/base62" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) +const ( + genericUniquenessMsg = "Invalid request. Request attempted to make second resource with the same field value that must be unique." + genericNotFoundMsg = "Unable to find requested resource." +) + +type apiError struct { + inner *pb.Error +} + +func (e *apiError) Error() string { + res := fmt.Sprintf("Status: %d, Code: %q, Error: %q", e.inner.GetStatus(), e.inner.GetCode(), e.inner.GetMessage()) + var dets []string + for _, rf := range e.inner.GetDetails().GetRequestFields() { + dets = append(dets, fmt.Sprintf("{name: %q, desc: %q}", rf.GetName(), rf.GetDescription())) + } + if len(dets) > 0 { + det := strings.Join(dets, ", ") + res = fmt.Sprintf("%s, Details: {%s}", res, det) + } + return res +} + +func (e *apiError) Is(target error) bool { + var tApiErr *apiError + if !errors.As(target, &tApiErr) { + return false + } + return tApiErr.inner.Code == e.inner.Code && tApiErr.inner.Status == e.inner.Status +} + +// ApiErrorWithCode returns an api error with the provided code. +func ApiErrorWithCode(c codes.Code) error { + return &apiError{inner: &pb.Error{ + Status: int32(runtime.HTTPStatusFromCode(c)), + Code: c.String(), + }} +} + +// ApiErrorWithCodeAndMessage returns an api error with the provided code and message. +func ApiErrorWithCodeAndMessage(c codes.Code, msg string, args ...interface{}) error { + return &apiError{inner: &pb.Error{ + Status: int32(runtime.HTTPStatusFromCode(c)), + Code: c.String(), + Message: fmt.Sprintf(msg, args...), + }} +} + // NotFoundError returns an ApiError indicating a resource couldn't be found. func NotFoundError() error { - return status.Error(codes.NotFound, "Resource not found.") + return &apiError{&pb.Error{ + Status: http.StatusNotFound, + Code: codes.NotFound.String(), + Message: "Resource not found.", + }} } // NotFoundErrorf returns an ApiError indicating a resource couldn't be found. func NotFoundErrorf(msg string, a ...interface{}) error { - return status.Errorf(codes.NotFound, msg, a...) + return &apiError{&pb.Error{ + Status: http.StatusNotFound, + Code: codes.NotFound.String(), + Message: fmt.Sprintf(msg, a...), + }} } func ForbiddenError() error { - return status.Error(codes.PermissionDenied, "Forbidden.") + return &apiError{&pb.Error{ + Status: http.StatusForbidden, + Code: codes.PermissionDenied.String(), + Message: "Forbidden.", + }} } func UnauthenticatedError() error { - return status.Error(codes.Unauthenticated, "Unauthenticated, or invalid token.") + return &apiError{&pb.Error{ + Status: http.StatusUnauthorized, + Code: codes.Unauthenticated.String(), + Message: "Unauthenticated, or invalid token.", + }} } func InvalidArgumentErrorf(msg string, fields map[string]string) error { - st := status.New(codes.InvalidArgument, msg) - br := &errdetails.BadRequest{} + err := ApiErrorWithCodeAndMessage(codes.InvalidArgument, msg) + var apiErr *apiError + if !errors.As(err, &apiErr) { + hclog.L().Error("Unable to build invalid argument api error.", "original error", err) + } + + if len(fields) > 0 { + apiErr.inner.Details = &pb.ErrorDetails{} + } for k, v := range fields { - br.FieldViolations = append(br.FieldViolations, &errdetails.BadRequest_FieldViolation{Field: k, Description: v}) + apiErr.inner.Details.RequestFields = append(apiErr.inner.Details.RequestFields, &pb.FieldError{Name: k, Description: v}) } - sort.Slice(br.FieldViolations, func(i, j int) bool { - return br.FieldViolations[i].GetField() < br.FieldViolations[j].GetField() + sort.Slice(apiErr.inner.GetDetails().GetRequestFields(), func(i, j int) bool { + return apiErr.inner.Details.RequestFields[i].GetName() < apiErr.inner.Details.RequestFields[j].GetName() }) - st, err := st.WithDetails(br) - if err != nil { - hclog.Default().Error("failure building status with details", "details", br, "error", err) - return status.Error(codes.Internal, "Failed to build InvalidArgument error.") - } - return st.Err() -} - -func statusErrorToApiError(s *status.Status) *pb.Error { - apiErr := &pb.Error{} - apiErr.Status = int32(runtime.HTTPStatusFromCode(s.Code())) - if s.Code() == codes.Unimplemented { - // Instead of returning a 501 we always want to return a 405 when a method isn't implemented. - apiErr.Status = http.StatusMethodNotAllowed - } - apiErr.Message = s.Message() - // TODO(ICU-193): Decouple from the status codes and instead use codes defined specifically for our API. - apiErr.Code = s.Code().String() - - for _, ed := range s.Details() { - switch ed.(type) { - case *errdetails.BadRequest: - br := ed.(*errdetails.BadRequest) - for _, fv := range br.GetFieldViolations() { - if apiErr.Details == nil { - apiErr.Details = &pb.ErrorDetails{} - } - apiErr.Details.RequestFields = append(apiErr.Details.RequestFields, &pb.FieldError{Name: fv.GetField(), Description: fv.GetDescription()}) - } - } - } return apiErr } -// TODO(ICU-194): Remove all information from internal errors. +// Converts a known errors into an error that can presented to an end user over the API. +func backendErrorToApiError(inErr error) error { + stErr := status.Convert(inErr) + + switch { + case errors.Is(inErr, runtime.ErrNotMatch): + // grpc gateway uses this error when the path was not matched, but the error uses codes.Unimplemented which doesn't match the intention. + // Overwrite the error to match our expected behavior. + return &apiError{inner: &pb.Error{ + Status: http.StatusNotFound, + Code: codes.NotFound.String(), + Message: http.StatusText(http.StatusNotFound), + }} + case status.Code(inErr) == codes.Unimplemented: + // Instead of returning a 501 we always want to return a 405 when a method isn't implemented. + return &apiError{inner: &pb.Error{ + Status: http.StatusMethodNotAllowed, + Code: codes.Unimplemented.String(), + Message: stErr.Message(), + }} + case errors.Is(inErr, db.ErrRecordNotFound): + return NotFoundErrorf(genericNotFoundMsg) + case errors.Is(inErr, db.ErrInvalidFieldMask), errors.Is(inErr, db.ErrEmptyFieldMask): + return InvalidArgumentErrorf("Error in provided request", map[string]string{"update_mask": "Invalid update mask provided."}) + case db.IsUniqueError(inErr), errors.Is(inErr, db.ErrNotUnique): + return InvalidArgumentErrorf(genericUniquenessMsg, nil) + } + return nil +} + +func getInternalError(id string) *apiError { + return &apiError{&pb.Error{ + Status: http.StatusInternalServerError, + Code: codes.Internal.String(), + Details: &pb.ErrorDetails{ErrorId: id}, + }} +} + +func internalErrorId() (string, error) { + errId, err := base62.Random(10) + if err != nil { + return "", fmt.Errorf("unable to generate id: %w", err) + } + return errId, nil +} + func ErrorHandler(logger hclog.Logger) runtime.ErrorHandlerFunc { const errorFallback = `{"error": "failed to marshal error message"}` - return func(ctx context.Context, _ *runtime.ServeMux, marshaler runtime.Marshaler, w http.ResponseWriter, r *http.Request, inErr error) { - if inErr == runtime.ErrNotMatch { - // grpc gateway uses this error when the path was not matched, but the error uses codes.Unimplemented which doesn't match the intention. - // Overwrite the error to match our expected behavior. - inErr = status.Error(codes.NotFound, http.StatusText(http.StatusNotFound)) + return func(ctx context.Context, _ *runtime.ServeMux, mar runtime.Marshaler, w http.ResponseWriter, r *http.Request, inErr error) { + // API specified error, otherwise we need to translate repo/db errors. + var apiErr *apiError + isApiErr := errors.As(inErr, &apiErr) + if !isApiErr { + if err := backendErrorToApiError(inErr); err != nil && !errors.As(err, &apiErr) { + logger.Error("failed to cast error to api error", "error", err) + } } - s, ok := status.FromError(inErr) - if !ok { - s = status.New(codes.Unknown, inErr.Error()) + + if apiErr == nil || apiErr.inner.GetStatus() == http.StatusInternalServerError { + errId, err := internalErrorId() + if err != nil { + logger.Error("unable to generate internal error id", "error", err) + errId = "failed_to_generate_error_id" + } + logger.Error("internal error returned", "error id", errId, "error", inErr) + apiErr = getInternalError(errId) } - apiErr := statusErrorToApiError(s) - buf, merr := marshaler.Marshal(apiErr) + + buf, merr := mar.Marshal(apiErr.inner) if merr != nil { - logger.Error("failed to marshal error response", "response", fmt.Sprintf("%#v", apiErr), "error", merr) + logger.Error("failed to marshal error response", "response", fmt.Sprintf("%#v", apiErr.inner), "error", merr) w.WriteHeader(http.StatusInternalServerError) if _, err := io.WriteString(w, errorFallback); err != nil { logger.Error("failed to write response", "error", err) @@ -100,8 +196,8 @@ func ErrorHandler(logger hclog.Logger) runtime.ErrorHandlerFunc { return } - w.Header().Set("Content-Type", marshaler.ContentType(apiErr)) - w.WriteHeader(int(apiErr.GetStatus())) + w.Header().Set("Content-Type", mar.ContentType(apiErr.inner)) + w.WriteHeader(int(apiErr.inner.GetStatus())) if _, err := w.Write(buf); err != nil { logger.Error("failed to send response chunk", "error", err) return diff --git a/internal/servers/controller/handlers/errors_test.go b/internal/servers/controller/handlers/errors_test.go index 97fea9d57..a643728c8 100644 --- a/internal/servers/controller/handlers/errors_test.go +++ b/internal/servers/controller/handlers/errors_test.go @@ -3,6 +3,7 @@ package handlers import ( "context" "errors" + "fmt" "io/ioutil" "net/http" "net/http/httptest" @@ -10,6 +11,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/grpc-ecosystem/grpc-gateway/v2/runtime" + "github.com/hashicorp/boundary/internal/db" pb "github.com/hashicorp/boundary/internal/gen/controller/api" "github.com/hashicorp/go-hclog" "github.com/stretchr/testify/assert" @@ -20,10 +22,9 @@ import ( ) func TestApiErrorHandler(t *testing.T) { - assert, require := assert.New(t), require.New(t) ctx := context.Background() req, err := http.NewRequest("GET", "madeup/for/the/test", nil) - require.NoError(err) + require.NoError(t, err) mux := runtime.NewServeMux() inMarsh, outMarsh := runtime.MarshalerForRequest(mux, req) @@ -90,13 +91,79 @@ func TestApiErrorHandler(t *testing.T) { err: errors.New("Some random error"), expected: &pb.Error{ Status: http.StatusInternalServerError, - Code: "Unknown", - Message: "Some random error", + Code: "Internal", + Details: &pb.ErrorDetails{ErrorId: ""}, + }, + }, + { + name: "Db invalid public id error", + err: fmt.Errorf("test error: %w", db.ErrInvalidPublicId), + expected: &pb.Error{ + Status: http.StatusInternalServerError, + Code: "Internal", + Details: &pb.ErrorDetails{ErrorId: ""}, + }, + }, + { + name: "Db invalid parameter", + err: fmt.Errorf("test error: %w", db.ErrInvalidParameter), + expected: &pb.Error{ + Status: http.StatusInternalServerError, + Code: "Internal", + Details: &pb.ErrorDetails{ErrorId: ""}, + }, + }, + { + name: "Db invalid field mask", + err: fmt.Errorf("test error: %w", db.ErrInvalidFieldMask), + expected: &pb.Error{ + Status: http.StatusBadRequest, + Code: "InvalidArgument", + Message: "Error in provided request", + Details: &pb.ErrorDetails{RequestFields: []*pb.FieldError{{Name: "update_mask", Description: "Invalid update mask provided."}}}, + }, + }, + { + name: "Db empty field mask", + err: fmt.Errorf("test error: %w", db.ErrEmptyFieldMask), + expected: &pb.Error{ + Status: http.StatusBadRequest, + Code: "InvalidArgument", + Message: "Error in provided request", + Details: &pb.ErrorDetails{RequestFields: []*pb.FieldError{{Name: "update_mask", Description: "Invalid update mask provided."}}}, + }, + }, + { + name: "Db not unique", + err: fmt.Errorf("test error: %w", db.ErrNotUnique), + expected: &pb.Error{ + Status: http.StatusBadRequest, + Code: "InvalidArgument", + Message: genericUniquenessMsg, + }, + }, + { + name: "Db record not found", + err: fmt.Errorf("test error: %w", db.ErrRecordNotFound), + expected: &pb.Error{ + Status: http.StatusNotFound, + Code: "NotFound", + Message: genericNotFoundMsg, + }, + }, + { + name: "Db multiple records", + err: fmt.Errorf("test error: %w", db.ErrMultipleRecords), + expected: &pb.Error{ + Status: http.StatusInternalServerError, + Code: "Internal", + Details: &pb.ErrorDetails{ErrorId: ""}, }, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { + assert, require := assert.New(t), require.New(t) w := httptest.NewRecorder() tested(ctx, mux, outMarsh, w, req, tc.err) resp := w.Result() @@ -108,6 +175,12 @@ func TestApiErrorHandler(t *testing.T) { gotErr := &pb.Error{} err = inMarsh.Unmarshal(got, gotErr) require.NoError(err) + + if tc.expected.Status == http.StatusInternalServerError { + require.NotNil(tc.expected.GetDetails()) + tc.expected.GetDetails().ErrorId = gotErr.GetDetails().GetErrorId() + } + assert.Empty(cmp.Diff(tc.expected, gotErr, protocmp.Transform())) }) } diff --git a/internal/servers/controller/handlers/groups/group_service_test.go b/internal/servers/controller/handlers/groups/group_service_test.go index c4ba77fed..697e1cbbd 100644 --- a/internal/servers/controller/handlers/groups/group_service_test.go +++ b/internal/servers/controller/handlers/groups/group_service_test.go @@ -2,6 +2,7 @@ package groups_test import ( "context" + "errors" "fmt" "strings" "testing" @@ -14,6 +15,7 @@ import ( "github.com/hashicorp/boundary/internal/gen/controller/api/resources/scopes" pbs "github.com/hashicorp/boundary/internal/gen/controller/api/services" "github.com/hashicorp/boundary/internal/iam" + "github.com/hashicorp/boundary/internal/servers/controller/handlers" "github.com/hashicorp/boundary/internal/servers/controller/handlers/groups" "github.com/hashicorp/boundary/internal/types/scope" "google.golang.org/genproto/protobuf/field_mask" @@ -129,60 +131,58 @@ func TestGet(t *testing.T) { scopeId string req *pbs.GetGroupRequest res *pbs.GetGroupResponse - errCode codes.Code + err error }{ { name: "Get an Existing Group", scopeId: og.GetScopeId(), req: &pbs.GetGroupRequest{Id: og.GetPublicId()}, res: &pbs.GetGroupResponse{Item: wantOrgGroup}, - errCode: codes.OK, }, { - name: "Get a non existant Group", - req: &pbs.GetGroupRequest{Id: iam.GroupPrefix + "_DoesntExis"}, - res: nil, - errCode: codes.NotFound, + name: "Get a non existant Group", + req: &pbs.GetGroupRequest{Id: iam.GroupPrefix + "_DoesntExis"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { - name: "Wrong id prefix", - req: &pbs.GetGroupRequest{Id: "j_1234567890"}, - res: nil, - errCode: codes.InvalidArgument, + name: "Wrong id prefix", + req: &pbs.GetGroupRequest{Id: "j_1234567890"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { - name: "space in id", - req: &pbs.GetGroupRequest{Id: iam.GroupPrefix + "_1 23456789"}, - res: nil, - errCode: codes.InvalidArgument, + name: "space in id", + req: &pbs.GetGroupRequest{Id: iam.GroupPrefix + "_1 23456789"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Project Scoped Get an Existing Group", scopeId: pg.GetScopeId(), req: &pbs.GetGroupRequest{Id: pg.GetPublicId()}, res: &pbs.GetGroupResponse{Item: wantProjGroup}, - errCode: codes.OK, }, { name: "Project Scoped Get a non existant Group", scopeId: pg.GetScopeId(), req: &pbs.GetGroupRequest{Id: iam.GroupPrefix + "_DoesntExis"}, res: nil, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { name: "Project Scoped Wrong id prefix", scopeId: pg.GetScopeId(), req: &pbs.GetGroupRequest{Id: "j_1234567890"}, res: nil, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Project Scoped space in id", scopeId: pg.GetScopeId(), req: &pbs.GetGroupRequest{Id: iam.GroupPrefix + "_1 23456789"}, res: nil, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { @@ -195,7 +195,10 @@ func TestGet(t *testing.T) { require.NoError(err, "Couldn't create new group service.") got, gErr := s.GetGroup(auth.DisabledAuthTestContext(auth.WithScopeId(tc.scopeId)), req) - assert.Equal(tc.errCode, status.Code(gErr), "GetGroup(%+v) got error %v, wanted %v", req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "GetGroup(%+v) got error %v, wanted %v", req, gErr, tc.err) + } assert.Empty(cmp.Diff(got, tc.res, protocmp.Transform()), "GetGroup(%q) got response\n%q, wanted\n%q", req, got, tc.res) }) } @@ -237,31 +240,27 @@ func TestList(t *testing.T) { name string scopeId string res *pbs.ListGroupsResponse - errCode codes.Code + err error }{ { name: "List Many Group", scopeId: oWithGroups.GetPublicId(), res: &pbs.ListGroupsResponse{Items: wantOrgGroups}, - errCode: codes.OK, }, { name: "List No Groups", scopeId: oNoGroups.GetPublicId(), res: &pbs.ListGroupsResponse{}, - errCode: codes.OK, }, { name: "List Many Project Group", scopeId: pWithGroups.GetPublicId(), res: &pbs.ListGroupsResponse{Items: wantProjGroups}, - errCode: codes.OK, }, { name: "List No Project Groups", scopeId: pNoGroups.GetPublicId(), res: &pbs.ListGroupsResponse{}, - errCode: codes.OK, }, } for _, tc := range cases { @@ -271,25 +270,27 @@ func TestList(t *testing.T) { require.NoError(err, "Couldn't create new group service.") got, gErr := s.ListGroups(auth.DisabledAuthTestContext(auth.WithScopeId(tc.scopeId)), &pbs.ListGroupsRequest{ScopeId: tc.scopeId}) - assert.Equal(tc.errCode, status.Code(gErr), "ListGroups(%q) got error %v, wanted %v", tc.scopeId, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "ListGroups(%q) got error %v, wanted %v", tc.scopeId, gErr, tc.err) + } assert.Empty(cmp.Diff(got, tc.res, protocmp.Transform()), "ListGroups(%q) got response %q, wanted %q", tc.scopeId, got, tc.res) }) } } func TestDelete(t *testing.T) { - require := require.New(t) og, pg, repo := createDefaultGroupsAndRepo(t) s, err := groups.NewService(repo) - require.NoError(err, "Error when getting new group service.") + require.NoError(t, err, "Error when getting new group service.") cases := []struct { name string scopeId string req *pbs.DeleteGroupRequest res *pbs.DeleteGroupResponse - errCode codes.Code + err error }{ { name: "Delete an Existing Group", @@ -297,8 +298,7 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteGroupRequest{ Id: og.GetPublicId(), }, - res: &pbs.DeleteGroupResponse{}, - errCode: codes.OK, + res: &pbs.DeleteGroupResponse{}, }, { name: "Delete bad group id", @@ -306,7 +306,7 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteGroupRequest{ Id: iam.GroupPrefix + "_doesntexis", }, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { name: "Bad Group Id formatting", @@ -314,7 +314,7 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteGroupRequest{ Id: "bad_format", }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Project Scoped Delete an Existing Group", @@ -322,8 +322,7 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteGroupRequest{ Id: pg.GetPublicId(), }, - res: &pbs.DeleteGroupResponse{}, - errCode: codes.OK, + res: &pbs.DeleteGroupResponse{}, }, { name: "Project Scoped Delete bad group id", @@ -331,22 +330,24 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteGroupRequest{ Id: iam.GroupPrefix + "_doesntexis", }, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) got, gErr := s.DeleteGroup(auth.DisabledAuthTestContext(auth.WithScopeId(tc.scopeId)), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "DeleteGroup(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "DeleteGroup(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } assert.EqualValuesf(tc.res, got, "DeleteGroup(%+v) got response %q, wanted %q", tc.req, got, tc.res) }) } } func TestDelete_twice(t *testing.T) { - assert := assert.New(t) - require := require.New(t) + assert, require := assert.New(t), require.New(t) og, pg, repo := createDefaultGroupsAndRepo(t) s, err := groups.NewService(repo) @@ -360,7 +361,7 @@ func TestDelete_twice(t *testing.T) { assert.NoError(gErr, "First attempt") _, gErr = s.DeleteGroup(ctx, req) assert.Error(gErr, "Second attempt") - assert.Equal(codes.NotFound, status.Code(gErr), "Expected permission denied for the second delete.") + assert.True(errors.Is(gErr, handlers.ApiErrorWithCode(codes.NotFound)), "Expected permission denied for the second delete.") scopeId = pg.GetScopeId() projReq := &pbs.DeleteGroupRequest{ @@ -371,21 +372,20 @@ func TestDelete_twice(t *testing.T) { assert.NoError(gErr, "First attempt") _, gErr = s.DeleteGroup(ctx, projReq) assert.Error(gErr, "Second attempt") - assert.Equal(codes.NotFound, status.Code(gErr), "Expected permission denied for the second delete.") + assert.True(errors.Is(gErr, handlers.ApiErrorWithCode(codes.NotFound)), "Expected permission denied for the second delete.") } func TestCreate(t *testing.T) { - require := require.New(t) defaultOGroup, defaultPGroup, repo := createDefaultGroupsAndRepo(t) defaultCreated, err := ptypes.Timestamp(defaultOGroup.GetCreateTime().GetTimestamp()) - require.NoError(err, "Error converting proto to timestamp.") + require.NoError(t, err, "Error converting proto to timestamp.") toMerge := &pbs.CreateGroupRequest{} cases := []struct { - name string - req *pbs.CreateGroupRequest - res *pbs.CreateGroupResponse - errCode codes.Code + name string + req *pbs.CreateGroupRequest + res *pbs.CreateGroupResponse + err error }{ { name: "Create a valid Group", @@ -404,7 +404,6 @@ func TestCreate(t *testing.T) { Version: 1, }, }, - errCode: codes.OK, }, { name: "Create a global Group", @@ -423,7 +422,6 @@ func TestCreate(t *testing.T) { Version: 1, }, }, - errCode: codes.OK, }, { name: "Create a valid Project Scoped Group", @@ -444,36 +442,35 @@ func TestCreate(t *testing.T) { Version: 1, }, }, - errCode: codes.OK, }, { name: "Can't specify Id", req: &pbs.CreateGroupRequest{Item: &pb.Group{ Id: iam.GroupPrefix + "_notallowed", }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Can't specify Created Time", req: &pbs.CreateGroupRequest{Item: &pb.Group{ CreatedTime: ptypes.TimestampNow(), }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Can't specify Update Time", req: &pbs.CreateGroupRequest{Item: &pb.Group{ UpdatedTime: ptypes.TimestampNow(), }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) req := proto.Clone(toMerge).(*pbs.CreateGroupRequest) proto.Merge(req, tc.req) @@ -481,7 +478,10 @@ func TestCreate(t *testing.T) { require.NoError(err, "Error when getting new group service.") got, gErr := s.CreateGroup(auth.DisabledAuthTestContext(auth.WithScopeId(req.GetItem().GetScopeId())), req) - assert.Equal(tc.errCode, status.Code(gErr), "CreateGroup(%+v) got error %v, wanted %v", req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "CreateGroup(%+v) got error %v, wanted %v", req, gErr, tc.err) + } if got != nil { assert.Contains(got.GetUri(), tc.res.Uri) assert.True(strings.HasPrefix(got.GetItem().GetId(), iam.GroupPrefix+"_")) @@ -504,7 +504,6 @@ func TestCreate(t *testing.T) { } func TestUpdate(t *testing.T) { - require := require.New(t) conn, _ := db.TestSetup(t, "postgres") wrap := db.TestWrapper(t) iamRepo := iam.TestRepo(t, conn, wrap) @@ -526,22 +525,22 @@ func TestUpdate(t *testing.T) { resetGroups := func(proj bool) { repo, err := repoFn() - require.NoError(err, "Couldn't get a new repo") + require.NoError(t, err, "Couldn't get a new repo") if proj { pgVersion++ pg, _, _, err = repo.UpdateGroup(context.Background(), pg, pgVersion, []string{"Name", "Description"}) - require.NoError(err, "Failed to reset the group") + require.NoError(t, err, "Failed to reset the group") pgVersion++ } else { ogVersion++ og, _, _, err = repo.UpdateGroup(context.Background(), og, ogVersion, []string{"Name", "Description"}) - require.NoError(err, "Failed to reset the group") + require.NoError(t, err, "Failed to reset the group") ogVersion++ } } created, err := ptypes.Timestamp(og.GetCreateTime().GetTimestamp()) - require.NoError(err, "Error converting proto to timestamp") + require.NoError(t, err, "Error converting proto to timestamp") toMerge := &pbs.UpdateGroupRequest{ Id: og.GetPublicId(), } @@ -552,7 +551,7 @@ func TestUpdate(t *testing.T) { scopeId string req *pbs.UpdateGroupRequest res *pbs.UpdateGroupResponse - errCode codes.Code + err error }{ { name: "Update an Existing Group", @@ -583,7 +582,6 @@ func TestUpdate(t *testing.T) { }, }, }, - errCode: codes.OK, }, { name: "Multiple Paths in single string", @@ -614,7 +612,6 @@ func TestUpdate(t *testing.T) { }, }, }, - errCode: codes.OK, }, { name: "Update an Existing Project Scoped Group", @@ -646,7 +643,6 @@ func TestUpdate(t *testing.T) { }, }, }, - errCode: codes.OK, }, { name: "Multiple Paths in single string", @@ -678,7 +674,6 @@ func TestUpdate(t *testing.T) { }, }, }, - errCode: codes.OK, }, { name: "No Update Mask", @@ -688,7 +683,7 @@ func TestUpdate(t *testing.T) { Description: &wrapperspb.StringValue{Value: "updated desc"}, }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "No Paths in Mask", @@ -700,7 +695,7 @@ func TestUpdate(t *testing.T) { Description: &wrapperspb.StringValue{Value: "updated desc"}, }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Only non-existant paths in Mask", @@ -712,7 +707,7 @@ func TestUpdate(t *testing.T) { Description: &wrapperspb.StringValue{Value: "updated desc"}, }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Unset Name", @@ -741,7 +736,6 @@ func TestUpdate(t *testing.T) { }, }, }, - errCode: codes.OK, }, { name: "Update Only Name", @@ -772,7 +766,6 @@ func TestUpdate(t *testing.T) { }, }, }, - errCode: codes.OK, }, { name: "Update Only Description", @@ -803,7 +796,6 @@ func TestUpdate(t *testing.T) { }, }, }, - errCode: codes.OK, }, { name: "Update a Non Existing Group", @@ -817,7 +809,7 @@ func TestUpdate(t *testing.T) { Description: &wrapperspb.StringValue{Value: "desc"}, }, }, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { name: "Cant change Id", @@ -831,8 +823,8 @@ func TestUpdate(t *testing.T) { Name: &wrapperspb.StringValue{Value: "new"}, Description: &wrapperspb.StringValue{Value: "new desc"}, }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant specify Created Time", @@ -844,8 +836,8 @@ func TestUpdate(t *testing.T) { CreatedTime: ptypes.TimestampNow(), }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant specify Updated Time", @@ -857,8 +849,8 @@ func TestUpdate(t *testing.T) { UpdatedTime: ptypes.TimestampNow(), }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { @@ -869,7 +861,7 @@ func TestUpdate(t *testing.T) { } tc.req.Item.Version = ver - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) req := proto.Clone(toMerge).(*pbs.UpdateGroupRequest) proto.Merge(req, tc.req) @@ -883,9 +875,12 @@ func TestUpdate(t *testing.T) { req.Item.Version = ver got, gErr := tested.UpdateGroup(auth.DisabledAuthTestContext(auth.WithScopeId(tc.scopeId)), req) - assert.Equal(tc.errCode, status.Code(gErr), "UpdateGroup(%+v) got error %v, wanted %v", req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "UpdateGroup(%+v) got error %v, wanted %v", req, gErr, tc.err) + } - if tc.errCode == codes.OK { + if tc.err == nil { defer resetGroups(req.Id == pg.PublicId) } @@ -991,9 +986,9 @@ func TestAddMember(t *testing.T) { grp := iam.TestGroup(t, conn, p.GetPublicId()) failCases := []struct { - name string - req *pbs.AddGroupMembersRequest - errCode codes.Code + name string + req *pbs.AddGroupMembersRequest + err error }{ { name: "Bad Group Id", @@ -1001,14 +996,17 @@ func TestAddMember(t *testing.T) { Id: "bad id", Version: grp.GetVersion(), }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range failCases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) _, gErr := s.AddGroupMembers(auth.DisabledAuthTestContext(auth.WithScopeId(grp.GetScopeId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "AddGroupMembers(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "AddGroupMembers(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } }) } } @@ -1092,9 +1090,9 @@ func TestSetMember(t *testing.T) { grp := iam.TestGroup(t, conn, p.GetPublicId()) failCases := []struct { - name string - req *pbs.SetGroupMembersRequest - errCode codes.Code + name string + req *pbs.SetGroupMembersRequest + err error }{ { name: "Bad Group Id", @@ -1102,14 +1100,17 @@ func TestSetMember(t *testing.T) { Id: "bad id", Version: grp.GetVersion(), }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range failCases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) _, gErr := s.SetGroupMembers(auth.DisabledAuthTestContext(auth.WithScopeId(grp.GetScopeId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "SetGroupMembers(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "SetGroupMembers(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } }) } } @@ -1201,9 +1202,9 @@ func TestRemoveMember(t *testing.T) { grp := iam.TestGroup(t, conn, p.GetPublicId()) failCases := []struct { - name string - req *pbs.AddGroupMembersRequest - errCode codes.Code + name string + req *pbs.AddGroupMembersRequest + err error }{ { name: "Bad Group Id", @@ -1211,14 +1212,17 @@ func TestRemoveMember(t *testing.T) { Id: "bad id", Version: grp.GetVersion(), }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range failCases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) _, gErr := s.AddGroupMembers(auth.DisabledAuthTestContext(auth.WithScopeId(grp.GetScopeId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "AddGroupMembers(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "AddGroupMembers(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } }) } } diff --git a/internal/servers/controller/handlers/host_catalogs/host_catalog_service_test.go b/internal/servers/controller/handlers/host_catalogs/host_catalog_service_test.go index 81682fe06..e5a9d1fe5 100644 --- a/internal/servers/controller/handlers/host_catalogs/host_catalog_service_test.go +++ b/internal/servers/controller/handlers/host_catalogs/host_catalog_service_test.go @@ -2,6 +2,7 @@ package host_catalogs_test import ( "context" + "errors" "fmt" "strings" "testing" @@ -18,11 +19,11 @@ import ( "github.com/hashicorp/boundary/internal/iam" "github.com/hashicorp/boundary/internal/kms" "github.com/hashicorp/boundary/internal/servers/controller/common" + "github.com/hashicorp/boundary/internal/servers/controller/handlers" "github.com/hashicorp/boundary/internal/servers/controller/handlers/host_catalogs" "github.com/hashicorp/boundary/internal/types/scope" "google.golang.org/genproto/protobuf/field_mask" "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/testing/protocmp" @@ -76,47 +77,49 @@ func TestGet(t *testing.T) { } cases := []struct { - name string - req *pbs.GetHostCatalogRequest - res *pbs.GetHostCatalogResponse - errCode codes.Code + name string + req *pbs.GetHostCatalogRequest + res *pbs.GetHostCatalogResponse + err error }{ { - name: "Get an Existing HostCatalog", - req: &pbs.GetHostCatalogRequest{Id: hc.GetPublicId()}, - res: &pbs.GetHostCatalogResponse{Item: pHostCatalog}, - errCode: codes.OK, + name: "Get an Existing HostCatalog", + req: &pbs.GetHostCatalogRequest{Id: hc.GetPublicId()}, + res: &pbs.GetHostCatalogResponse{Item: pHostCatalog}, }, { - name: "Get a non existing Host Catalog", - req: &pbs.GetHostCatalogRequest{Id: static.HostCatalogPrefix + "_DoesntExis"}, - res: nil, - errCode: codes.NotFound, + name: "Get a non existing Host Catalog", + req: &pbs.GetHostCatalogRequest{Id: static.HostCatalogPrefix + "_DoesntExis"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { - name: "Wrong id prefix", - req: &pbs.GetHostCatalogRequest{Id: "j_1234567890"}, - res: nil, - errCode: codes.InvalidArgument, + name: "Wrong id prefix", + req: &pbs.GetHostCatalogRequest{Id: "j_1234567890"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { - name: "space in id", - req: &pbs.GetHostCatalogRequest{Id: static.HostCatalogPrefix + "_1 23456789"}, - res: nil, - errCode: codes.InvalidArgument, + name: "space in id", + req: &pbs.GetHostCatalogRequest{Id: static.HostCatalogPrefix + "_1 23456789"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) req := proto.Clone(toMerge).(*pbs.GetHostCatalogRequest) proto.Merge(req, tc.req) s, err := host_catalogs.NewService(repo, iamRepoFn) - require.NoError(t, err, "Couldn't create a new host catalog service.") + require.NoError(err, "Couldn't create a new host catalog service.") got, gErr := s.GetHostCatalog(auth.DisabledAuthTestContext(auth.WithScopeId(proj.GetPublicId())), req) - assert.Equal(tc.errCode, status.Code(gErr), "GetHostCatalog(%+v) got error %v, wanted %v", req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "GetHostCatalog(%+v) got error %v, wanted %v", req, gErr, tc.err) + } if tc.res != nil { tc.res.Item.Version = 1 @@ -173,35 +176,32 @@ func TestList(t *testing.T) { name string scopeId string res *pbs.ListHostCatalogsResponse - errCode codes.Code + err error }{ { name: "List Some Catalogs", scopeId: pWithCatalogs.GetPublicId(), res: &pbs.ListHostCatalogsResponse{Items: wantSomeCatalogs}, - errCode: codes.OK, }, { name: "List Other Catalogs", scopeId: pWithOtherCatalogs.GetPublicId(), res: &pbs.ListHostCatalogsResponse{Items: wantOtherCatalogs}, - errCode: codes.OK, }, { name: "List No Catalogs", scopeId: pNoCatalogs.GetPublicId(), res: &pbs.ListHostCatalogsResponse{}, - errCode: codes.OK, }, { name: "Unfound Catalogs", scopeId: scope.Project.Prefix() + "_DoesntExis", - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { name: "Bad scope level", scopeId: scope.Global.String(), - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { @@ -211,7 +211,10 @@ func TestList(t *testing.T) { require.NoError(err, "Couldn't create new auth_method service.") got, gErr := s.ListHostCatalogs(auth.DisabledAuthTestContext(auth.WithScopeId(tc.scopeId)), &pbs.ListHostCatalogsRequest{ScopeId: tc.scopeId}) - assert.Equal(tc.errCode, status.Code(gErr), "ListHostCatalogs() for scope %q got error %v, wanted %v", tc.scopeId, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "ListHostCatalogs() for scope %q got error %v, wanted %v", tc.scopeId, gErr, tc.err) + } assert.Empty(cmp.Diff(got, tc.res, protocmp.Transform()), "ListHostCatalogs() for scope %q got response %q, wanted %q", tc.scopeId, got, tc.res) }) } @@ -219,18 +222,17 @@ func TestList(t *testing.T) { func TestDelete(t *testing.T) { t.Parallel() - require := require.New(t) hc, proj, repo, iamRepoFn := createDefaultHostCatalogAndRepo(t) s, err := host_catalogs.NewService(repo, iamRepoFn) - require.NoError(err, "Couldn't create a new host catalog service.") + require.NoError(t, err, "Couldn't create a new host catalog service.") cases := []struct { name string scopeId string req *pbs.DeleteHostCatalogRequest res *pbs.DeleteHostCatalogResponse - errCode codes.Code + err error }{ { name: "Delete an Existing HostCatalog", @@ -238,8 +240,7 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteHostCatalogRequest{ Id: hc.GetPublicId(), }, - res: &pbs.DeleteHostCatalogResponse{}, - errCode: codes.OK, + res: &pbs.DeleteHostCatalogResponse{}, }, { name: "Delete bad id HostCatalog", @@ -247,7 +248,7 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteHostCatalogRequest{ Id: static.HostCatalogPrefix + "_doesntexis", }, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { name: "Bad HostCatalog Id formatting", @@ -255,14 +256,17 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteHostCatalogRequest{ Id: static.HostCatalogPrefix + "_bad_format", }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) got, gErr := s.DeleteHostCatalog(auth.DisabledAuthTestContext(auth.WithScopeId(tc.scopeId)), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "DeleteHostCatalog(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "DeleteHostCatalog(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } assert.EqualValuesf(tc.res, got, "DeleteHostCatalog(%q) got response %q, wanted %q", tc.req, got, tc.res) }) } @@ -270,11 +274,11 @@ func TestDelete(t *testing.T) { func TestDelete_twice(t *testing.T) { t.Parallel() - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) hc, proj, repo, iamRepoFn := createDefaultHostCatalogAndRepo(t) s, err := host_catalogs.NewService(repo, iamRepoFn) - require.NoError(t, err, "Couldn't create a new host catalog service.") + require.NoError(err, "Couldn't create a new host catalog service.") req := &pbs.DeleteHostCatalogRequest{ Id: hc.GetPublicId(), } @@ -283,7 +287,7 @@ func TestDelete_twice(t *testing.T) { assert.NoError(gErr, "First attempt") _, gErr = s.DeleteHostCatalog(ctx, req) assert.Error(gErr, "Second attempt") - assert.Equal(codes.NotFound, status.Code(gErr), "Expected permission denied for the second delete.") + assert.True(errors.Is(gErr, handlers.ApiErrorWithCode(codes.NotFound)), "Expected permission denied for the second delete.") } func TestCreate(t *testing.T) { @@ -294,10 +298,10 @@ func TestCreate(t *testing.T) { toMerge := &pbs.CreateHostCatalogRequest{} cases := []struct { - name string - req *pbs.CreateHostCatalogRequest - res *pbs.CreateHostCatalogResponse - errCode codes.Code + name string + req *pbs.CreateHostCatalogRequest + res *pbs.CreateHostCatalogResponse + err error }{ { name: "Create a valid HostCatalog", @@ -317,7 +321,6 @@ func TestCreate(t *testing.T) { Type: "static", }, }, - errCode: codes.OK, }, { name: "Cant create in org", @@ -327,7 +330,7 @@ func TestCreate(t *testing.T) { Description: &wrappers.StringValue{Value: "desc"}, Type: "static", }}, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant create in global", @@ -337,7 +340,7 @@ func TestCreate(t *testing.T) { Description: &wrappers.StringValue{Value: "desc"}, Type: "static", }}, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Create with unknown type", @@ -346,7 +349,7 @@ func TestCreate(t *testing.T) { Description: &wrappers.StringValue{Value: "desc"}, Type: "thisismadeup", }}, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Create with no type", @@ -354,31 +357,31 @@ func TestCreate(t *testing.T) { Name: &wrappers.StringValue{Value: "name"}, Description: &wrappers.StringValue{Value: "desc"}, }}, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Can't specify Id", req: &pbs.CreateHostCatalogRequest{Item: &pb.HostCatalog{ Id: "not allowed to be set", }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Can't specify Created Time", req: &pbs.CreateHostCatalogRequest{Item: &pb.HostCatalog{ CreatedTime: ptypes.TimestampNow(), }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Can't specify Update Time", req: &pbs.CreateHostCatalogRequest{Item: &pb.HostCatalog{ UpdatedTime: ptypes.TimestampNow(), }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { @@ -391,7 +394,10 @@ func TestCreate(t *testing.T) { require.NoError(err, "Failed to create a new host catalog service.") got, gErr := s.CreateHostCatalog(auth.DisabledAuthTestContext(auth.WithScopeId(proj.GetPublicId())), req) - assert.Equal(tc.errCode, status.Code(gErr), "CreateHostCatalog(%+v) got error %v, wanted %v", req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "CreateHostCatalog(%+v) got error %v, wanted %v", req, gErr, tc.err) + } if got != nil { assert.Contains(got.GetUri(), tc.res.GetUri()) assert.True(strings.HasPrefix(got.GetItem().GetId(), static.HostCatalogPrefix)) @@ -440,10 +446,10 @@ func TestUpdate(t *testing.T) { } cases := []struct { - name string - req *pbs.UpdateHostCatalogRequest - res *pbs.UpdateHostCatalogResponse - errCode codes.Code + name string + req *pbs.UpdateHostCatalogRequest + res *pbs.UpdateHostCatalogResponse + err error }{ { name: "Update an Existing HostCatalog", @@ -468,7 +474,6 @@ func TestUpdate(t *testing.T) { Type: "static", }, }, - errCode: codes.OK, }, { name: "Multiple Paths in single string", @@ -493,7 +498,6 @@ func TestUpdate(t *testing.T) { Type: "static", }, }, - errCode: codes.OK, }, { name: "No Update Mask", @@ -503,7 +507,7 @@ func TestUpdate(t *testing.T) { Description: &wrappers.StringValue{Value: "updated desc"}, }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Empty Path", @@ -514,7 +518,7 @@ func TestUpdate(t *testing.T) { Description: &wrappers.StringValue{Value: "updated desc"}, }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Only non-existant paths in Mask", @@ -525,7 +529,7 @@ func TestUpdate(t *testing.T) { Description: &wrappers.StringValue{Value: "updated desc"}, }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Unset Name", @@ -547,7 +551,6 @@ func TestUpdate(t *testing.T) { Type: "static", }, }, - errCode: codes.OK, }, { name: "Unset Description", @@ -569,7 +572,6 @@ func TestUpdate(t *testing.T) { Type: "static", }, }, - errCode: codes.OK, }, { name: "Update Only Name", @@ -593,7 +595,6 @@ func TestUpdate(t *testing.T) { Type: "static", }, }, - errCode: codes.OK, }, { name: "Update Only Description", @@ -617,7 +618,6 @@ func TestUpdate(t *testing.T) { Type: "static", }, }, - errCode: codes.OK, }, { name: "Cant change type", @@ -630,7 +630,7 @@ func TestUpdate(t *testing.T) { Type: "ec2", }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Update a Non Existing HostCatalog", @@ -645,7 +645,7 @@ func TestUpdate(t *testing.T) { Description: &wrappers.StringValue{Value: "desc"}, }, }, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { name: "Cant change Id", @@ -660,8 +660,8 @@ func TestUpdate(t *testing.T) { Name: &wrappers.StringValue{Value: "new"}, Description: &wrappers.StringValue{Value: "new desc"}, }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant specify Created Time", @@ -673,8 +673,8 @@ func TestUpdate(t *testing.T) { CreatedTime: ptypes.TimestampNow(), }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant specify Updated Time", @@ -686,8 +686,8 @@ func TestUpdate(t *testing.T) { UpdatedTime: ptypes.TimestampNow(), }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Valid mask, cant specify type", @@ -699,8 +699,8 @@ func TestUpdate(t *testing.T) { Type: "unknown", }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { @@ -721,9 +721,12 @@ func TestUpdate(t *testing.T) { req.Item.Version = version got, gErr := tested.UpdateHostCatalog(auth.DisabledAuthTestContext(auth.WithScopeId(proj.GetPublicId())), req) - assert.Equal(tc.errCode, status.Code(gErr), "UpdateHostCatalog(%+v) got error %v, wanted %v", req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "UpdateHostCatalog(%+v) got error %v, wanted %v", req, gErr, tc.err) + } - if tc.errCode == codes.OK { + if tc.err == nil { defer resetHostCatalog() } diff --git a/internal/servers/controller/handlers/host_sets/host_set_service_test.go b/internal/servers/controller/handlers/host_sets/host_set_service_test.go index 465b01fc4..7ffff3eb6 100644 --- a/internal/servers/controller/handlers/host_sets/host_set_service_test.go +++ b/internal/servers/controller/handlers/host_sets/host_set_service_test.go @@ -2,6 +2,7 @@ package host_sets_test import ( "context" + "errors" "fmt" "strings" "testing" @@ -18,6 +19,7 @@ import ( "github.com/hashicorp/boundary/internal/host/static" "github.com/hashicorp/boundary/internal/iam" "github.com/hashicorp/boundary/internal/kms" + "github.com/hashicorp/boundary/internal/servers/controller/handlers" "github.com/hashicorp/boundary/internal/servers/controller/handlers/host_sets" "github.com/hashicorp/boundary/internal/types/scope" "github.com/stretchr/testify/assert" @@ -61,47 +63,49 @@ func TestGet(t *testing.T) { } cases := []struct { - name string - req *pbs.GetHostSetRequest - res *pbs.GetHostSetResponse - errCode codes.Code + name string + req *pbs.GetHostSetRequest + res *pbs.GetHostSetResponse + err error }{ { - name: "Get an Existing Host", - req: &pbs.GetHostSetRequest{Id: hs.GetPublicId()}, - res: &pbs.GetHostSetResponse{Item: pHost}, - errCode: codes.OK, + name: "Get an Existing Host", + req: &pbs.GetHostSetRequest{Id: hs.GetPublicId()}, + res: &pbs.GetHostSetResponse{Item: pHost}, }, { - name: "Get a non existing Host Set", - req: &pbs.GetHostSetRequest{Id: static.HostSetPrefix + "_DoesntExis"}, - res: nil, - errCode: codes.NotFound, + name: "Get a non existing Host Set", + req: &pbs.GetHostSetRequest{Id: static.HostSetPrefix + "_DoesntExis"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { - name: "Wrong id prefix", - req: &pbs.GetHostSetRequest{Id: "j_1234567890"}, - res: nil, - errCode: codes.InvalidArgument, + name: "Wrong id prefix", + req: &pbs.GetHostSetRequest{Id: "j_1234567890"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { - name: "space in id", - req: &pbs.GetHostSetRequest{Id: static.HostPrefix + "_1 23456789"}, - res: nil, - errCode: codes.InvalidArgument, + name: "space in id", + req: &pbs.GetHostSetRequest{Id: static.HostPrefix + "_1 23456789"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) req := proto.Clone(toMerge).(*pbs.GetHostSetRequest) proto.Merge(req, tc.req) s, err := host_sets.NewService(repoFn) - require.NoError(t, err, "Couldn't create a new host set service.") + require.NoError(err, "Couldn't create a new host set service.") got, gErr := s.GetHostSet(auth.DisabledAuthTestContext(auth.WithScopeId(proj.GetPublicId())), req) - assert.Equal(tc.errCode, status.Code(gErr), "GetHostSet(%+v) got error %v, wanted %v", req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "GetHostSet(%+v) got error %v, wanted %v", req, gErr, tc.err) + } if tc.res != nil { tc.res.Item.Version = 1 @@ -142,19 +146,17 @@ func TestList(t *testing.T) { name string hostCatalogId string res *pbs.ListHostSetsResponse - errCode codes.Code + err error }{ { name: "List Many Host Sets", hostCatalogId: hc.GetPublicId(), res: &pbs.ListHostSetsResponse{Items: wantHs}, - errCode: codes.OK, }, { name: "List No Host Sets", hostCatalogId: hcNoHosts.GetPublicId(), res: &pbs.ListHostSetsResponse{}, - errCode: codes.OK, }, } for _, tc := range cases { @@ -164,7 +166,10 @@ func TestList(t *testing.T) { require.NoError(err, "Couldn't create new host set service.") got, gErr := s.ListHostSets(auth.DisabledAuthTestContext(auth.WithScopeId(proj.GetPublicId())), &pbs.ListHostSetsRequest{HostCatalogId: tc.hostCatalogId}) - assert.Equal(tc.errCode, status.Code(gErr), "ListHostSets(%q) got error %v, wanted %v", tc.hostCatalogId, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "ListHostSets(%q) got error %v, wanted %v", tc.hostCatalogId, gErr, tc.err) + } assert.Empty(cmp.Diff(got, tc.res, protocmp.Transform()), "ListHostSets(%q) got response %q, wanted %q", tc.hostCatalogId, got, tc.res) }) } @@ -193,7 +198,7 @@ func TestDelete(t *testing.T) { scopeId string req *pbs.DeleteHostSetRequest res *pbs.DeleteHostSetResponse - errCode codes.Code + err error }{ { name: "Delete an Existing Host Set", @@ -201,8 +206,7 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteHostSetRequest{ Id: h.GetPublicId(), }, - res: &pbs.DeleteHostSetResponse{}, - errCode: codes.OK, + res: &pbs.DeleteHostSetResponse{}, }, { name: "Delete bad id Host Set", @@ -210,7 +214,7 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteHostSetRequest{ Id: static.HostSetPrefix + "_doesntexis", }, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { name: "Delete bad host catalog id in Host Set", @@ -218,7 +222,7 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteHostSetRequest{ Id: h.GetPublicId(), }, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { name: "Bad Host Id formatting", @@ -226,14 +230,17 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteHostSetRequest{ Id: static.HostSetPrefix + "_bad_format", }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) got, gErr := s.DeleteHostSet(auth.DisabledAuthTestContext(auth.WithScopeId(tc.scopeId)), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "DeleteHostSet(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "DeleteHostSet(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } assert.Empty(cmp.Diff(tc.res, got, protocmp.Transform()), "DeleteHostSet(%q) got response %q, wanted %q", tc.req, got, tc.res) }) } @@ -241,7 +248,7 @@ func TestDelete(t *testing.T) { func TestDelete_twice(t *testing.T) { t.Parallel() - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) conn, _ := db.TestSetup(t, "postgres") wrapper := db.TestWrapper(t) kms := kms.TestKms(t, conn, wrapper) @@ -256,7 +263,7 @@ func TestDelete_twice(t *testing.T) { h := static.TestSets(t, conn, hc.GetPublicId(), 1)[0] s, err := host_sets.NewService(repoFn) - require.NoError(t, err, "Couldn't create a new host set service.") + require.NoError(err, "Couldn't create a new host set service.") req := &pbs.DeleteHostSetRequest{ Id: h.GetPublicId(), } @@ -265,7 +272,7 @@ func TestDelete_twice(t *testing.T) { assert.NoError(gErr, "First attempt") _, gErr = s.DeleteHostSet(ctx, req) assert.Error(gErr, "Second attempt") - assert.Equal(codes.NotFound, status.Code(gErr), "Expected permission denied for the second delete.") + assert.True(errors.Is(gErr, handlers.ApiErrorWithCode(codes.NotFound)), "Expected permission denied for the second delete.") } func TestCreate(t *testing.T) { @@ -286,10 +293,10 @@ func TestCreate(t *testing.T) { require.NoError(t, err) cases := []struct { - name string - req *pbs.CreateHostSetRequest - res *pbs.CreateHostSetResponse - errCode codes.Code + name string + req *pbs.CreateHostSetRequest + res *pbs.CreateHostSetResponse + err error }{ { name: "Create a valid Host", @@ -309,7 +316,6 @@ func TestCreate(t *testing.T) { Type: "static", }, }, - errCode: codes.OK, }, { name: "Create with unknown type", @@ -319,7 +325,7 @@ func TestCreate(t *testing.T) { Description: &wrappers.StringValue{Value: "desc"}, Type: "ThisIsMadeUp", }}, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Create with no type", @@ -338,7 +344,6 @@ func TestCreate(t *testing.T) { Type: "static", }, }, - errCode: codes.OK, }, { name: "Can't specify Id", @@ -346,8 +351,8 @@ func TestCreate(t *testing.T) { HostCatalogId: hc.GetPublicId(), Id: "not allowed to be set", }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Can't specify Created Time", @@ -355,8 +360,8 @@ func TestCreate(t *testing.T) { HostCatalogId: hc.GetPublicId(), CreatedTime: ptypes.TimestampNow(), }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Can't specify Update Time", @@ -364,8 +369,8 @@ func TestCreate(t *testing.T) { HostCatalogId: hc.GetPublicId(), UpdatedTime: ptypes.TimestampNow(), }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { @@ -376,7 +381,10 @@ func TestCreate(t *testing.T) { require.NoError(err, "Failed to create a new host set service.") got, gErr := s.CreateHostSet(auth.DisabledAuthTestContext(auth.WithScopeId(proj.GetPublicId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "CreateHostSet(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "CreateHostSet(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } if got != nil { assert.Contains(got.GetUri(), tc.res.GetUri()) assert.True(strings.HasPrefix(got.GetItem().GetId(), static.HostSetPrefix), got.GetItem().GetId()) @@ -447,10 +455,10 @@ func TestUpdate(t *testing.T) { require.NoError(t, err, "Failed to create a new host set service.") cases := []struct { - name string - req *pbs.UpdateHostSetRequest - res *pbs.UpdateHostSetResponse - errCode codes.Code + name string + req *pbs.UpdateHostSetRequest + res *pbs.UpdateHostSetResponse + err error }{ { name: "Update an Existing Host", @@ -476,7 +484,6 @@ func TestUpdate(t *testing.T) { HostIds: hIds, }, }, - errCode: codes.OK, }, { name: "Multiple Paths in single string", @@ -502,7 +509,6 @@ func TestUpdate(t *testing.T) { HostIds: hIds, }, }, - errCode: codes.OK, }, { name: "Cant modify type", @@ -516,7 +522,7 @@ func TestUpdate(t *testing.T) { Type: "ec2", }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "No Update Mask", @@ -526,7 +532,7 @@ func TestUpdate(t *testing.T) { Description: &wrappers.StringValue{Value: "updated desc"}, }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Empty Path", @@ -537,7 +543,7 @@ func TestUpdate(t *testing.T) { Description: &wrappers.StringValue{Value: "updated desc"}, }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Only non-existant paths in Mask", @@ -548,7 +554,7 @@ func TestUpdate(t *testing.T) { Description: &wrappers.StringValue{Value: "updated desc"}, }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Unset Name", @@ -571,7 +577,6 @@ func TestUpdate(t *testing.T) { HostIds: hIds, }, }, - errCode: codes.OK, }, { name: "Unset Description", @@ -594,7 +599,6 @@ func TestUpdate(t *testing.T) { HostIds: hIds, }, }, - errCode: codes.OK, }, { name: "Update Only Name", @@ -619,7 +623,6 @@ func TestUpdate(t *testing.T) { HostIds: hIds, }, }, - errCode: codes.OK, }, { name: "Update Only Description", @@ -644,7 +647,6 @@ func TestUpdate(t *testing.T) { HostIds: hIds, }, }, - errCode: codes.OK, }, { name: "Update a Non Existing Host Set", @@ -659,7 +661,7 @@ func TestUpdate(t *testing.T) { Description: &wrappers.StringValue{Value: "desc"}, }, }, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { name: "Cant change Id", @@ -674,8 +676,8 @@ func TestUpdate(t *testing.T) { Name: &wrappers.StringValue{Value: "new"}, Description: &wrappers.StringValue{Value: "new desc"}, }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant specify Created Time", @@ -687,8 +689,8 @@ func TestUpdate(t *testing.T) { CreatedTime: ptypes.TimestampNow(), }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant specify Updated Time", @@ -700,8 +702,8 @@ func TestUpdate(t *testing.T) { UpdatedTime: ptypes.TimestampNow(), }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Valid mask, cant specify type", @@ -713,8 +715,8 @@ func TestUpdate(t *testing.T) { Type: "Unknown", }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { @@ -735,9 +737,12 @@ func TestUpdate(t *testing.T) { req.Item.Version = version got, gErr := tested.UpdateHostSet(auth.DisabledAuthTestContext(auth.WithScopeId(proj.GetPublicId())), req) - assert.Equal(tc.errCode, status.Code(gErr), "UpdateHostSet(%+v) got error %v, wanted %v", req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "UpdateHostSet(%+v) got error %v, wanted %v", req, gErr, tc.err) + } - if tc.errCode == codes.OK { + if tc.err == nil { defer resetHostSet() } @@ -821,9 +826,9 @@ func TestAddHostSetHosts(t *testing.T) { ss := static.TestSets(t, conn, hc.GetPublicId(), 1)[0] failCases := []struct { - name string - req *pbs.AddHostSetHostsRequest - errCode codes.Code + name string + req *pbs.AddHostSetHostsRequest + err error }{ { name: "Bad Set Id", @@ -832,7 +837,7 @@ func TestAddHostSetHosts(t *testing.T) { Version: ss.GetVersion(), HostIds: []string{hs[0].GetPublicId()}, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Empty host list", @@ -840,14 +845,17 @@ func TestAddHostSetHosts(t *testing.T) { Id: ss.GetPublicId(), Version: ss.GetVersion(), }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range failCases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) _, gErr := s.AddHostSetHosts(auth.DisabledAuthTestContext(auth.WithScopeId(proj.GetPublicId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "AddHostSetHosts(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "AddHostSetHosts(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } }) } } @@ -917,9 +925,9 @@ func TestSetHostSetHosts(t *testing.T) { ss := static.TestSets(t, conn, hc.GetPublicId(), 1)[0] failCases := []struct { - name string - req *pbs.SetHostSetHostsRequest - errCode codes.Code + name string + req *pbs.SetHostSetHostsRequest + err error }{ { name: "Bad Set Id", @@ -928,14 +936,17 @@ func TestSetHostSetHosts(t *testing.T) { Version: ss.GetVersion(), HostIds: []string{hs[0].GetPublicId()}, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range failCases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) _, gErr := s.SetHostSetHosts(auth.DisabledAuthTestContext(auth.WithScopeId(proj.GetPublicId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "SetHostSetHosts(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "SetHostSetHosts(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } }) } } @@ -1014,9 +1025,9 @@ func TestRemoveHostSetHosts(t *testing.T) { ss := static.TestSets(t, conn, hc.GetPublicId(), 1)[0] failCases := []struct { - name string - req *pbs.RemoveHostSetHostsRequest - errCode codes.Code + name string + req *pbs.RemoveHostSetHostsRequest + err error }{ { name: "Bad set Id", @@ -1025,7 +1036,7 @@ func TestRemoveHostSetHosts(t *testing.T) { Version: ss.GetVersion(), HostIds: []string{hs[0].GetPublicId()}, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "empty hosts", @@ -1034,14 +1045,17 @@ func TestRemoveHostSetHosts(t *testing.T) { Version: ss.GetVersion(), HostIds: []string{}, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range failCases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) _, gErr := s.RemoveHostSetHosts(auth.DisabledAuthTestContext(auth.WithScopeId(proj.GetPublicId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "RemoveHostSetHosts(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "RemoveHostSetHosts(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } }) } } diff --git a/internal/servers/controller/handlers/hosts/host_service_test.go b/internal/servers/controller/handlers/hosts/host_service_test.go index 0fec73dc3..56f520695 100644 --- a/internal/servers/controller/handlers/hosts/host_service_test.go +++ b/internal/servers/controller/handlers/hosts/host_service_test.go @@ -2,6 +2,7 @@ package hosts_test import ( "context" + "errors" "fmt" "strings" "testing" @@ -18,13 +19,13 @@ import ( "github.com/hashicorp/boundary/internal/host/static" "github.com/hashicorp/boundary/internal/iam" "github.com/hashicorp/boundary/internal/kms" + "github.com/hashicorp/boundary/internal/servers/controller/handlers" "github.com/hashicorp/boundary/internal/servers/controller/handlers/hosts" "github.com/hashicorp/boundary/internal/types/scope" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "google.golang.org/genproto/protobuf/field_mask" "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/testing/protocmp" "google.golang.org/protobuf/types/known/structpb" @@ -58,44 +59,46 @@ func TestGet(t *testing.T) { } cases := []struct { - name string - req *pbs.GetHostRequest - res *pbs.GetHostResponse - errCode codes.Code + name string + req *pbs.GetHostRequest + res *pbs.GetHostResponse + err error }{ { - name: "Get an Existing Host", - req: &pbs.GetHostRequest{Id: h.GetPublicId()}, - res: &pbs.GetHostResponse{Item: pHost}, - errCode: codes.OK, + name: "Get an Existing Host", + req: &pbs.GetHostRequest{Id: h.GetPublicId()}, + res: &pbs.GetHostResponse{Item: pHost}, }, { - name: "Get a non existing Host Set", - req: &pbs.GetHostRequest{Id: static.HostPrefix + "_DoesntExis"}, - res: nil, - errCode: codes.NotFound, + name: "Get a non existing Host Set", + req: &pbs.GetHostRequest{Id: static.HostPrefix + "_DoesntExis"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { - name: "Wrong id prefix", - req: &pbs.GetHostRequest{Id: "j_1234567890"}, - res: nil, - errCode: codes.InvalidArgument, + name: "Wrong id prefix", + req: &pbs.GetHostRequest{Id: "j_1234567890"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { - name: "space in id", - req: &pbs.GetHostRequest{Id: static.HostPrefix + "_1 23456789"}, - res: nil, - errCode: codes.InvalidArgument, + name: "space in id", + req: &pbs.GetHostRequest{Id: static.HostPrefix + "_1 23456789"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) s, err := hosts.NewService(repoFn) - require.NoError(t, err, "Couldn't create a new host set service.") + require.NoError(err, "Couldn't create a new host set service.") got, gErr := s.GetHost(auth.DisabledAuthTestContext(auth.WithScopeId(proj.GetPublicId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "GetHost(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "GetHost(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } if tc.res != nil { tc.res.Item.Version = 1 @@ -138,19 +141,17 @@ func TestList(t *testing.T) { name string hostCatalogId string res *pbs.ListHostsResponse - errCode codes.Code + err error }{ { name: "List Many Hosts", hostCatalogId: hc.GetPublicId(), res: &pbs.ListHostsResponse{Items: wantHs}, - errCode: codes.OK, }, { name: "List No Hosts", hostCatalogId: hcNoHosts.GetPublicId(), res: &pbs.ListHostsResponse{}, - errCode: codes.OK, }, } for _, tc := range cases { @@ -160,7 +161,10 @@ func TestList(t *testing.T) { require.NoError(err, "Couldn't create new host set service.") got, gErr := s.ListHosts(auth.DisabledAuthTestContext(auth.WithScopeId(proj.GetPublicId())), &pbs.ListHostsRequest{HostCatalogId: tc.hostCatalogId}) - assert.Equal(tc.errCode, status.Code(gErr), "ListHosts(%q) got error %v, wanted %v", tc.hostCatalogId, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "ListHosts(%q) got error %v, wanted %v", tc.hostCatalogId, gErr, tc.err) + } assert.Empty(cmp.Diff(got, tc.res, protocmp.Transform()), "ListHosts(%q) got response %q, wanted %q", tc.hostCatalogId, got, tc.res) }) } @@ -189,7 +193,7 @@ func TestDelete(t *testing.T) { scopeId string req *pbs.DeleteHostRequest res *pbs.DeleteHostResponse - errCode codes.Code + err error }{ { name: "Delete an Existing Host", @@ -197,8 +201,7 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteHostRequest{ Id: h.GetPublicId(), }, - res: &pbs.DeleteHostResponse{}, - errCode: codes.OK, + res: &pbs.DeleteHostResponse{}, }, { name: "Delete bad id Host", @@ -206,7 +209,7 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteHostRequest{ Id: static.HostPrefix + "_doesntexis", }, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { name: "Bad Host Id formatting", @@ -214,14 +217,17 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteHostRequest{ Id: static.HostPrefix + "_bad_format", }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) got, gErr := s.DeleteHost(auth.DisabledAuthTestContext(auth.WithScopeId(tc.scopeId)), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "DeleteHost(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "DeleteHost(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } assert.Empty(cmp.Diff(tc.res, got, protocmp.Transform()), "DeleteHost(%q) got response %q, wanted %q", tc.req, got, tc.res) }) } @@ -229,7 +235,7 @@ func TestDelete(t *testing.T) { func TestDelete_twice(t *testing.T) { t.Parallel() - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) conn, _ := db.TestSetup(t, "postgres") wrapper := db.TestWrapper(t) kms := kms.TestKms(t, conn, wrapper) @@ -244,7 +250,7 @@ func TestDelete_twice(t *testing.T) { h := static.TestHosts(t, conn, hc.GetPublicId(), 1)[0] s, err := hosts.NewService(repoFn) - require.NoError(t, err, "Couldn't create a new host set service.") + require.NoError(err, "Couldn't create a new host set service.") req := &pbs.DeleteHostRequest{ Id: h.GetPublicId(), } @@ -253,7 +259,7 @@ func TestDelete_twice(t *testing.T) { assert.NoError(gErr, "First attempt") _, gErr = s.DeleteHost(ctx, req) assert.Error(gErr, "Second attempt") - assert.Equal(codes.NotFound, status.Code(gErr), "Expected permission denied for the second delete.") + assert.True(errors.Is(gErr, handlers.ApiErrorWithCode(codes.NotFound)), "Expected permission denied for the second delete.") } func TestCreate(t *testing.T) { @@ -274,10 +280,10 @@ func TestCreate(t *testing.T) { require.NoError(t, err) cases := []struct { - name string - req *pbs.CreateHostRequest - res *pbs.CreateHostResponse - errCode codes.Code + name string + req *pbs.CreateHostRequest + res *pbs.CreateHostResponse + err error }{ { name: "Create a valid Host", @@ -303,7 +309,6 @@ func TestCreate(t *testing.T) { }}, }, }, - errCode: codes.OK, }, { name: "Create with empty address", @@ -316,7 +321,7 @@ func TestCreate(t *testing.T) { "address": structpb.NewStringValue(""), }}, }}, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Create without address", @@ -327,7 +332,7 @@ func TestCreate(t *testing.T) { Type: "static", Attributes: &structpb.Struct{Fields: map[string]*structpb.Value{}}, }}, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Create with unknown type", @@ -337,7 +342,7 @@ func TestCreate(t *testing.T) { Description: &wrappers.StringValue{Value: "desc"}, Type: "ThisIsMadeUp", }}, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Create with no type", @@ -362,7 +367,6 @@ func TestCreate(t *testing.T) { }}, }, }, - errCode: codes.OK, }, { name: "Can't specify Id", @@ -370,8 +374,8 @@ func TestCreate(t *testing.T) { HostCatalogId: hc.GetPublicId(), Id: "not allowed to be set", }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Can't specify Created Time", @@ -379,16 +383,16 @@ func TestCreate(t *testing.T) { HostCatalogId: hc.GetPublicId(), CreatedTime: ptypes.TimestampNow(), }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Can't specify Update Time", req: &pbs.CreateHostRequest{Item: &pb.Host{ UpdatedTime: ptypes.TimestampNow(), }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { @@ -398,7 +402,10 @@ func TestCreate(t *testing.T) { require.NoError(err, "Failed to create a new host set service.") got, gErr := s.CreateHost(auth.DisabledAuthTestContext(auth.WithScopeId(proj.GetPublicId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "CreateHost(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "CreateHost(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } if got != nil { assert.Contains(got.GetUri(), tc.res.GetUri()) assert.True(strings.HasPrefix(got.GetItem().GetId(), static.HostPrefix)) @@ -464,10 +471,10 @@ func TestUpdate(t *testing.T) { require.NoError(t, err, "Failed to create a new host set service.") cases := []struct { - name string - req *pbs.UpdateHostRequest - res *pbs.UpdateHostResponse - errCode codes.Code + name string + req *pbs.UpdateHostRequest + res *pbs.UpdateHostResponse + err error }{ { name: "Update an Existing Host", @@ -495,7 +502,6 @@ func TestUpdate(t *testing.T) { }}, }, }, - errCode: codes.OK, }, { name: "Multiple Paths in single string", @@ -523,7 +529,6 @@ func TestUpdate(t *testing.T) { }}, }, }, - errCode: codes.OK, }, { name: "No Update Mask", @@ -533,7 +538,7 @@ func TestUpdate(t *testing.T) { Description: &wrappers.StringValue{Value: "updated desc"}, }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "No Update Mask", @@ -546,7 +551,7 @@ func TestUpdate(t *testing.T) { Type: "ec2", }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Empty Path", @@ -557,7 +562,7 @@ func TestUpdate(t *testing.T) { Description: &wrappers.StringValue{Value: "updated desc"}, }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Only non-existant paths in Mask", @@ -568,7 +573,7 @@ func TestUpdate(t *testing.T) { Description: &wrappers.StringValue{Value: "updated desc"}, }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Unset Name", @@ -593,7 +598,6 @@ func TestUpdate(t *testing.T) { }}, }, }, - errCode: codes.OK, }, { name: "Unset Description", @@ -618,7 +622,6 @@ func TestUpdate(t *testing.T) { }}, }, }, - errCode: codes.OK, }, { name: "Update Only Name", @@ -645,7 +648,6 @@ func TestUpdate(t *testing.T) { }}, }, }, - errCode: codes.OK, }, { name: "Update Only Description", @@ -672,7 +674,6 @@ func TestUpdate(t *testing.T) { }}, }, }, - errCode: codes.OK, }, { name: "Update a Non Existing Host", @@ -687,7 +688,7 @@ func TestUpdate(t *testing.T) { Description: &wrappers.StringValue{Value: "desc"}, }, }, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { name: "Cant change Id", @@ -702,8 +703,8 @@ func TestUpdate(t *testing.T) { Name: &wrappers.StringValue{Value: "new"}, Description: &wrappers.StringValue{Value: "new desc"}, }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant unset address", @@ -717,8 +718,8 @@ func TestUpdate(t *testing.T) { "address": structpb.NewNullValue(), }}, }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant set address to empty string", @@ -732,8 +733,8 @@ func TestUpdate(t *testing.T) { "address": structpb.NewStringValue(""), }}, }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant specify Created Time", @@ -745,8 +746,8 @@ func TestUpdate(t *testing.T) { CreatedTime: ptypes.TimestampNow(), }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant specify Updated Time", @@ -758,8 +759,8 @@ func TestUpdate(t *testing.T) { UpdatedTime: ptypes.TimestampNow(), }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Valid mask, cant specify type", @@ -771,8 +772,8 @@ func TestUpdate(t *testing.T) { Type: "Unknown", }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { @@ -793,9 +794,12 @@ func TestUpdate(t *testing.T) { req.Item.Version = version got, gErr := tested.UpdateHost(auth.DisabledAuthTestContext(auth.WithScopeId(proj.GetPublicId())), req) - assert.Equal(tc.errCode, status.Code(gErr), "UpdateHost(%+v) got error %v, wanted %v", req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "UpdateHost(%+v) got error %v, wanted %v", req, gErr, tc.err) + } - if tc.errCode == codes.OK { + if tc.err == nil { defer resetHost() } diff --git a/internal/servers/controller/handlers/roles/role_service_test.go b/internal/servers/controller/handlers/roles/role_service_test.go index 95c6f1b88..ab9a22286 100644 --- a/internal/servers/controller/handlers/roles/role_service_test.go +++ b/internal/servers/controller/handlers/roles/role_service_test.go @@ -2,6 +2,7 @@ package roles_test import ( "context" + "errors" "fmt" "strings" "testing" @@ -15,6 +16,7 @@ import ( pbs "github.com/hashicorp/boundary/internal/gen/controller/api/services" "github.com/hashicorp/boundary/internal/iam" "github.com/hashicorp/boundary/internal/perms" + "github.com/hashicorp/boundary/internal/servers/controller/handlers" "github.com/hashicorp/boundary/internal/servers/controller/handlers/roles" "github.com/hashicorp/boundary/internal/types/scope" "google.golang.org/genproto/protobuf/field_mask" @@ -68,8 +70,7 @@ func equalPrincipals(role *pb.Role, principals []string) bool { } func TestGet(t *testing.T) { - assert := assert.New(t) - require := require.New(t) + assert, require := assert.New(t), require.New(t) or, pr, repo := createDefaultRolesAndRepo(t) toMerge := &pbs.GetRoleRequest{ Id: or.GetPublicId(), @@ -104,60 +105,58 @@ func TestGet(t *testing.T) { scopeId string req *pbs.GetRoleRequest res *pbs.GetRoleResponse - errCode codes.Code + err error }{ { name: "Get an Existing Role", scopeId: or.GetScopeId(), req: &pbs.GetRoleRequest{Id: or.GetPublicId()}, res: &pbs.GetRoleResponse{Item: wantOrgRole}, - errCode: codes.OK, }, { - name: "Get a non existant Role", - req: &pbs.GetRoleRequest{Id: iam.RolePrefix + "_DoesntExis"}, - res: nil, - errCode: codes.NotFound, + name: "Get a non existant Role", + req: &pbs.GetRoleRequest{Id: iam.RolePrefix + "_DoesntExis"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { - name: "Wrong id prefix", - req: &pbs.GetRoleRequest{Id: "j_1234567890"}, - res: nil, - errCode: codes.InvalidArgument, + name: "Wrong id prefix", + req: &pbs.GetRoleRequest{Id: "j_1234567890"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { - name: "space in id", - req: &pbs.GetRoleRequest{Id: iam.RolePrefix + "_1 23456789"}, - res: nil, - errCode: codes.InvalidArgument, + name: "space in id", + req: &pbs.GetRoleRequest{Id: iam.RolePrefix + "_1 23456789"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Project Scoped Get an Existing Role", scopeId: pr.GetScopeId(), req: &pbs.GetRoleRequest{Id: pr.GetPublicId()}, res: &pbs.GetRoleResponse{Item: wantProjRole}, - errCode: codes.OK, }, { name: "Project Scoped Get a non existant Role", scopeId: pr.GetScopeId(), req: &pbs.GetRoleRequest{Id: iam.RolePrefix + "_DoesntExis"}, res: nil, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { name: "Project Scoped Wrong id prefix", scopeId: pr.GetScopeId(), req: &pbs.GetRoleRequest{Id: "j_1234567890"}, res: nil, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Project Scoped space in id", scopeId: pr.GetScopeId(), req: &pbs.GetRoleRequest{Id: iam.RolePrefix + "_1 23456789"}, res: nil, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { @@ -169,7 +168,10 @@ func TestGet(t *testing.T) { require.NoError(err, "Couldn't create new role service.") got, gErr := s.GetRole(auth.DisabledAuthTestContext(auth.WithScopeId(tc.scopeId)), req) - assert.Equal(tc.errCode, status.Code(gErr), "GetRole(%+v) got error %v, wanted %v", req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "GetRole(%+v) got error %v, wanted %v", req, gErr, tc.err) + } assert.Empty(cmp.Diff(got, tc.res, protocmp.Transform()), "GetRole(%q) got response\n%q, wanted\n%q", req, got, tc.res) }) } @@ -211,34 +213,29 @@ func TestList(t *testing.T) { } cases := []struct { - name string - req *pbs.ListRolesRequest - res *pbs.ListRolesResponse - errCode codes.Code + name string + req *pbs.ListRolesRequest + res *pbs.ListRolesResponse }{ { - name: "List Many Role", - req: &pbs.ListRolesRequest{ScopeId: oWithRoles.GetPublicId()}, - res: &pbs.ListRolesResponse{Items: wantOrgRoles}, - errCode: codes.OK, + name: "List Many Role", + req: &pbs.ListRolesRequest{ScopeId: oWithRoles.GetPublicId()}, + res: &pbs.ListRolesResponse{Items: wantOrgRoles}, }, { - name: "List No Roles", - req: &pbs.ListRolesRequest{ScopeId: oNoRoles.GetPublicId()}, - res: &pbs.ListRolesResponse{}, - errCode: codes.OK, + name: "List No Roles", + req: &pbs.ListRolesRequest{ScopeId: oNoRoles.GetPublicId()}, + res: &pbs.ListRolesResponse{}, }, { - name: "List Many Project Role", - req: &pbs.ListRolesRequest{ScopeId: pWithRoles.GetPublicId()}, - res: &pbs.ListRolesResponse{Items: wantProjRoles}, - errCode: codes.OK, + name: "List Many Project Role", + req: &pbs.ListRolesRequest{ScopeId: pWithRoles.GetPublicId()}, + res: &pbs.ListRolesResponse{Items: wantProjRoles}, }, { - name: "List No Project Roles", - req: &pbs.ListRolesRequest{ScopeId: pNoRoles.GetPublicId()}, - res: &pbs.ListRolesResponse{}, - errCode: codes.OK, + name: "List No Project Roles", + req: &pbs.ListRolesRequest{ScopeId: pNoRoles.GetPublicId()}, + res: &pbs.ListRolesResponse{}, }, } for _, tc := range cases { @@ -247,25 +244,24 @@ func TestList(t *testing.T) { require.NoError(err, "Couldn't create new role service.") got, gErr := s.ListRoles(auth.DisabledAuthTestContext(auth.WithScopeId(tc.req.GetScopeId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "ListRoles(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + assert.NoError(gErr) assert.Empty(cmp.Diff(got, tc.res, protocmp.Transform()), "ListRoles(%q) got response %q, wanted %q", tc.req, got, tc.res) }) } } func TestDelete(t *testing.T) { - require := require.New(t) or, pr, repo := createDefaultRolesAndRepo(t) s, err := roles.NewService(repo) - require.NoError(err, "Error when getting new role service.") + require.NoError(t, err, "Error when getting new role service.") cases := []struct { name string scopeId string req *pbs.DeleteRoleRequest res *pbs.DeleteRoleResponse - errCode codes.Code + err error }{ { name: "Delete an Existing Role", @@ -273,8 +269,7 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteRoleRequest{ Id: or.GetPublicId(), }, - res: &pbs.DeleteRoleResponse{}, - errCode: codes.OK, + res: &pbs.DeleteRoleResponse{}, }, { name: "Delete bad role id", @@ -282,7 +277,7 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteRoleRequest{ Id: iam.RolePrefix + "_doesntexis", }, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { name: "Delete default role", @@ -290,7 +285,7 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteRoleRequest{ Id: "r_default", }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Bad Role Id formatting", @@ -298,7 +293,7 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteRoleRequest{ Id: "bad_format", }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Project Scoped Delete an Existing Role", @@ -306,8 +301,7 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteRoleRequest{ Id: pr.GetPublicId(), }, - res: &pbs.DeleteRoleResponse{}, - errCode: codes.OK, + res: &pbs.DeleteRoleResponse{}, }, { name: "Project Scoped Delete bad Role id", @@ -315,22 +309,24 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteRoleRequest{ Id: iam.RolePrefix + "_doesntexis", }, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) got, gErr := s.DeleteRole(auth.DisabledAuthTestContext(auth.WithScopeId(tc.scopeId)), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "DeleteRole(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.NotNil(gErr) + assert.True(errors.Is(gErr, tc.err), "DeleteRole(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } assert.EqualValuesf(tc.res, got, "DeleteRole(%q) got response %q, wanted %q", tc.req, got, tc.res) }) } } func TestDelete_twice(t *testing.T) { - assert := assert.New(t) - require := require.New(t) + assert, require := assert.New(t), require.New(t) or, pr, repo := createDefaultRolesAndRepo(t) s, err := roles.NewService(repo) @@ -343,7 +339,7 @@ func TestDelete_twice(t *testing.T) { assert.NoError(gErr, "First attempt") _, gErr = s.DeleteRole(ctx, req) assert.Error(gErr, "Second attempt") - assert.Equal(codes.NotFound, status.Code(gErr), "Expected permission denied for the second delete.") + assert.True(errors.Is(gErr, handlers.ApiErrorWithCode(codes.NotFound)), "Expected permission denied for the second delete.") projReq := &pbs.DeleteRoleRequest{ Id: pr.GetPublicId(), @@ -353,22 +349,21 @@ func TestDelete_twice(t *testing.T) { assert.NoError(gErr, "First attempt") _, gErr = s.DeleteRole(ctx, projReq) assert.Error(gErr, "Second attempt") - assert.Equal(codes.NotFound, status.Code(gErr), "Expected permission denied for the second delete.") + assert.True(errors.Is(gErr, handlers.ApiErrorWithCode(codes.NotFound)), "Expected permission denied for the second delete.") } func TestCreate(t *testing.T) { - require := require.New(t) defaultOrgRole, defaultProjRole, repo := createDefaultRolesAndRepo(t) defaultCreated, err := ptypes.Timestamp(defaultOrgRole.GetCreateTime().GetTimestamp()) - require.NoError(err, "Error converting proto to timestamp.") + require.NoError(t, err, "Error converting proto to timestamp.") toMerge := &pbs.CreateRoleRequest{} cases := []struct { - name string - req *pbs.CreateRoleRequest - res *pbs.CreateRoleResponse - errCode codes.Code + name string + req *pbs.CreateRoleRequest + res *pbs.CreateRoleResponse + err error }{ { name: "Create a valid Role", @@ -389,7 +384,6 @@ func TestCreate(t *testing.T) { Version: 1, }, }, - errCode: codes.OK, }, { name: "Create a valid Global Role", @@ -410,7 +404,6 @@ func TestCreate(t *testing.T) { Version: 1, }, }, - errCode: codes.OK, }, { name: "Create a valid Project Scoped Role", @@ -432,7 +425,6 @@ func TestCreate(t *testing.T) { Version: 1, }, }, - errCode: codes.OK, }, { name: "Invalid grant scope ID", @@ -444,8 +436,8 @@ func TestCreate(t *testing.T) { GrantScopeId: &wrapperspb.StringValue{Value: defaultOrgRole.GetScopeId()}, }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Can't specify Id", @@ -453,8 +445,8 @@ func TestCreate(t *testing.T) { ScopeId: defaultProjRole.GetScopeId(), Id: iam.RolePrefix + "_notallowed", }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Can't specify Created Time", @@ -462,8 +454,8 @@ func TestCreate(t *testing.T) { ScopeId: defaultProjRole.GetScopeId(), CreatedTime: ptypes.TimestampNow(), }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Can't specify Update Time", @@ -471,13 +463,13 @@ func TestCreate(t *testing.T) { ScopeId: defaultProjRole.GetScopeId(), UpdatedTime: ptypes.TimestampNow(), }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) req := proto.Clone(toMerge).(*pbs.CreateRoleRequest) proto.Merge(req, tc.req) @@ -485,7 +477,10 @@ func TestCreate(t *testing.T) { require.NoError(err, "Error when getting new role service.") got, gErr := s.CreateRole(auth.DisabledAuthTestContext(auth.WithScopeId(tc.req.GetItem().GetScopeId())), req) - assert.Equal(tc.errCode, status.Code(gErr), "CreateRole(%+v) got error %v, wanted %v", req, gErr, tc.errCode) + if tc.err != nil { + require.NotNil(gErr) + assert.True(errors.Is(gErr, tc.err), "CreateRole(%+v) got error %v, wanted %v", req, gErr, tc.err) + } if got != nil { assert.Contains(got.GetUri(), tc.res.Uri) assert.True(strings.HasPrefix(got.GetItem().GetId(), iam.RolePrefix+"_"), "Expected %q to have the prefix %q", got.GetItem().GetId(), iam.RolePrefix+"_") @@ -578,7 +573,7 @@ func TestUpdate(t *testing.T) { scopeId string req *pbs.UpdateRoleRequest res *pbs.UpdateRoleResponse - errCode codes.Code + err error }{ { name: "Update an Existing Role", @@ -608,7 +603,6 @@ func TestUpdate(t *testing.T) { Principals: []*pb.Principal{principal}, }, }, - errCode: codes.OK, }, { name: "Multiple Paths in single string", @@ -637,7 +631,6 @@ func TestUpdate(t *testing.T) { Principals: []*pb.Principal{principal}, }, }, - errCode: codes.OK, }, { name: "Update an Existing Project Scoped Role", @@ -667,7 +660,6 @@ func TestUpdate(t *testing.T) { Principals: []*pb.Principal{principal}, }, }, - errCode: codes.OK, }, { name: "Multiple Paths in single string", @@ -697,7 +689,6 @@ func TestUpdate(t *testing.T) { Principals: []*pb.Principal{principal}, }, }, - errCode: codes.OK, }, { name: "No Update Mask", @@ -707,7 +698,7 @@ func TestUpdate(t *testing.T) { Description: &wrapperspb.StringValue{Value: "updated desc"}, }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "No Paths in Mask", @@ -719,7 +710,7 @@ func TestUpdate(t *testing.T) { Description: &wrapperspb.StringValue{Value: "updated desc"}, }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Only non-existant paths in Mask", @@ -731,7 +722,7 @@ func TestUpdate(t *testing.T) { Description: &wrapperspb.StringValue{Value: "updated desc"}, }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Unset Name", @@ -758,7 +749,6 @@ func TestUpdate(t *testing.T) { Principals: []*pb.Principal{principal}, }, }, - errCode: codes.OK, }, { name: "Update Only Name", @@ -787,7 +777,6 @@ func TestUpdate(t *testing.T) { Principals: []*pb.Principal{principal}, }, }, - errCode: codes.OK, }, { name: "Update Only Description", @@ -816,7 +805,6 @@ func TestUpdate(t *testing.T) { Principals: []*pb.Principal{principal}, }, }, - errCode: codes.OK, }, { name: "Update a Non Existing Role", @@ -830,7 +818,7 @@ func TestUpdate(t *testing.T) { Description: &wrapperspb.StringValue{Value: "desc"}, }, }, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { name: "Cant change Id", @@ -843,8 +831,8 @@ func TestUpdate(t *testing.T) { Name: &wrapperspb.StringValue{Value: "new"}, Description: &wrapperspb.StringValue{Value: "new desc"}, }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant specify Created Time", @@ -856,8 +844,8 @@ func TestUpdate(t *testing.T) { CreatedTime: ptypes.TimestampNow(), }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant specify Updated Time", @@ -869,8 +857,8 @@ func TestUpdate(t *testing.T) { UpdatedTime: ptypes.TimestampNow(), }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant specify grants", @@ -882,8 +870,8 @@ func TestUpdate(t *testing.T) { GrantStrings: []string{"anything"}, }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant specify principals", @@ -895,8 +883,8 @@ func TestUpdate(t *testing.T) { PrincipalIds: []string{"u_0987654321"}, }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { @@ -921,9 +909,12 @@ func TestUpdate(t *testing.T) { req.Item.Version = ver got, gErr := tested.UpdateRole(auth.DisabledAuthTestContext(auth.WithScopeId(tc.scopeId)), req) - assert.Equal(tc.errCode, status.Code(gErr), "UpdateRole(%+v) got error %v, wanted %v", req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "UpdateRole(%+v) got error %v, wanted %v", req, gErr, tc.err) + } - if tc.errCode == codes.OK { + if tc.err == nil { defer resetRoles(req.Id == pr.PublicId) } @@ -1048,9 +1039,9 @@ func TestAddPrincipal(t *testing.T) { role := iam.TestRole(t, conn, p.GetPublicId()) failCases := []struct { - name string - req *pbs.AddRolePrincipalsRequest - errCode codes.Code + name string + req *pbs.AddRolePrincipalsRequest + err error }{ { name: "Bad Role Id", @@ -1058,14 +1049,17 @@ func TestAddPrincipal(t *testing.T) { Id: "bad id", Version: role.GetVersion(), }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range failCases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) _, gErr := s.AddRolePrincipals(auth.DisabledAuthTestContext(auth.WithScopeId(p.GetPublicId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "AddRolePrincipals(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "AddRolePrincipals(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } }) } } @@ -1174,9 +1168,9 @@ func TestSetPrincipal(t *testing.T) { role := iam.TestRole(t, conn, p.GetPublicId()) failCases := []struct { - name string - req *pbs.SetRolePrincipalsRequest - errCode codes.Code + name string + req *pbs.SetRolePrincipalsRequest + err error }{ { name: "Bad Role Id", @@ -1184,14 +1178,17 @@ func TestSetPrincipal(t *testing.T) { Id: "bad id", Version: role.GetVersion(), }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range failCases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) _, gErr := s.SetRolePrincipals(auth.DisabledAuthTestContext(auth.WithScopeId(p.GetPublicId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "SetRolePrincipals(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "SetRolePrincipals(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } }) } } @@ -1312,9 +1309,9 @@ func TestRemovePrincipal(t *testing.T) { role := iam.TestRole(t, conn, p.GetPublicId()) failCases := []struct { - name string - req *pbs.AddRolePrincipalsRequest - errCode codes.Code + name string + req *pbs.AddRolePrincipalsRequest + err error }{ { name: "Bad Role Id", @@ -1322,14 +1319,17 @@ func TestRemovePrincipal(t *testing.T) { Id: "bad id", Version: role.GetVersion(), }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range failCases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) _, gErr := s.AddRolePrincipals(auth.DisabledAuthTestContext(auth.WithScopeId(p.GetPublicId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "AddRolePrincipals(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "AddRolePrincipals(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } }) } } @@ -1393,7 +1393,7 @@ func TestAddGrants(t *testing.T) { o, p := iam.TestScopes(t, iamRepo) for _, scope := range []*iam.Scope{o, p} { t.Run(tc.name+"_"+scope.Type, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) role := iam.TestRole(t, conn, scope.GetPublicId()) for _, e := range tc.existing { _ = iam.TestRoleGrant(t, conn, role.GetPublicId(), e) @@ -1412,7 +1412,7 @@ func TestAddGrants(t *testing.T) { assert.Error(err) return } - require.NoError(t, err) + require.NoError(err) checkEqualGrants(t, tc.result, got.GetItem()) }) } @@ -1421,9 +1421,9 @@ func TestAddGrants(t *testing.T) { _, p := iam.TestScopes(t, iamRepo) role := iam.TestRole(t, conn, p.GetPublicId()) failCases := []struct { - name string - req *pbs.AddRoleGrantsRequest - errCode codes.Code + name string + req *pbs.AddRoleGrantsRequest + err error }{ { name: "Bad Role Id", @@ -1432,14 +1432,17 @@ func TestAddGrants(t *testing.T) { GrantStrings: []string{"id=*;actions=create"}, Version: role.GetVersion(), }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range failCases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) _, gErr := s.AddRoleGrants(auth.DisabledAuthTestContext(auth.WithScopeId(p.GetPublicId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "AddRoleGrants(%+v) got error %#v, wanted %#v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "AddRoleGrants(%+v) got error %#v, wanted %#v", tc.req, gErr, tc.err) + } }) } } @@ -1491,7 +1494,7 @@ func TestSetGrants(t *testing.T) { o, p := iam.TestScopes(t, iamRepo) for _, scope := range []*iam.Scope{o, p} { t.Run(tc.name+"_"+scope.Type, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) role := iam.TestRole(t, conn, scope.GetPublicId()) for _, e := range tc.existing { _ = iam.TestRoleGrant(t, conn, role.GetPublicId(), e) @@ -1511,7 +1514,7 @@ func TestSetGrants(t *testing.T) { return } s, _ := status.FromError(err) - require.NoError(t, err, "Got error %v", s) + require.NoError(err, "Got error %v", s) checkEqualGrants(t, tc.result, got.GetItem()) }) } @@ -1521,9 +1524,9 @@ func TestSetGrants(t *testing.T) { role := iam.TestRole(t, conn, p.GetPublicId()) failCases := []struct { - name string - req *pbs.SetRoleGrantsRequest - errCode codes.Code + name string + req *pbs.SetRoleGrantsRequest + err error }{ { name: "Bad Role Id", @@ -1532,14 +1535,17 @@ func TestSetGrants(t *testing.T) { GrantStrings: []string{"id=*;actions=create"}, Version: role.GetVersion(), }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range failCases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) _, gErr := s.SetRoleGrants(auth.DisabledAuthTestContext(auth.WithScopeId(p.GetPublicId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "SetRoleGrants(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "SetRoleGrants(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } }) } } @@ -1590,7 +1596,7 @@ func TestRemoveGrants(t *testing.T) { o, p := iam.TestScopes(t, iamRepo) for _, scope := range []*iam.Scope{o, p} { t.Run(tc.name+"_"+scope.Type, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) role := iam.TestRole(t, conn, scope.GetPublicId()) for _, e := range tc.existing { _ = iam.TestRoleGrant(t, conn, role.GetPublicId(), e) @@ -1611,7 +1617,7 @@ func TestRemoveGrants(t *testing.T) { } s, ok := status.FromError(err) assert.True(ok) - require.NoError(t, err, "Got error %v", s) + require.NoError(err, "Got error %v", s) checkEqualGrants(t, tc.result, got.GetItem()) }) } @@ -1623,7 +1629,7 @@ func TestRemoveGrants(t *testing.T) { name string req *pbs.RemoveRoleGrantsRequest - errCode codes.Code + err error }{ { name: "Bad Role Id", @@ -1632,14 +1638,17 @@ func TestRemoveGrants(t *testing.T) { GrantStrings: []string{"id=*;actions=create"}, Version: role.GetVersion(), }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range failCases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) _, gErr := s.RemoveRoleGrants(auth.DisabledAuthTestContext(auth.WithScopeId(p.GetPublicId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "RemoveRoleGrants(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "RemoveRoleGrants(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } }) } } diff --git a/internal/servers/controller/handlers/scopes/scope_service_test.go b/internal/servers/controller/handlers/scopes/scope_service_test.go index b9a28f63a..37ce989f1 100644 --- a/internal/servers/controller/handlers/scopes/scope_service_test.go +++ b/internal/servers/controller/handlers/scopes/scope_service_test.go @@ -2,6 +2,7 @@ package scopes_test import ( "context" + "errors" "fmt" "strings" "testing" @@ -14,11 +15,11 @@ import ( pb "github.com/hashicorp/boundary/internal/gen/controller/api/resources/scopes" pbs "github.com/hashicorp/boundary/internal/gen/controller/api/services" "github.com/hashicorp/boundary/internal/iam" + "github.com/hashicorp/boundary/internal/servers/controller/handlers" "github.com/hashicorp/boundary/internal/servers/controller/handlers/scopes" "github.com/hashicorp/boundary/internal/types/scope" "google.golang.org/genproto/protobuf/field_mask" "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/testing/protocmp" "google.golang.org/protobuf/types/known/wrapperspb" @@ -89,55 +90,52 @@ func TestGet(t *testing.T) { scopeId string req *pbs.GetScopeRequest res *pbs.GetScopeResponse - errCode codes.Code + err error }{ { name: "Get an existing org", scopeId: "global", req: &pbs.GetScopeRequest{Id: org.GetPublicId()}, res: &pbs.GetScopeResponse{Item: oScope}, - errCode: codes.OK, }, { name: "Get a non existing org", scopeId: "global", req: &pbs.GetScopeRequest{Id: "o_DoesntExis"}, res: nil, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { name: "Get an existing project", scopeId: org.GetPublicId(), req: &pbs.GetScopeRequest{Id: proj.GetPublicId()}, res: &pbs.GetScopeResponse{Item: pScope}, - errCode: codes.OK, }, { name: "Get a non existing project", scopeId: org.GetPublicId(), req: &pbs.GetScopeRequest{Id: "p_DoesntExis"}, res: nil, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { name: "Wrong id prefix", scopeId: org.GetPublicId(), req: &pbs.GetScopeRequest{Id: "j_1234567890"}, res: nil, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "space in id", scopeId: org.GetPublicId(), req: &pbs.GetScopeRequest{Id: "p_1 23456789"}, res: nil, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) - require := require.New(t) + assert, require := assert.New(t), require.New(t) req := proto.Clone(toMerge).(*pbs.GetScopeRequest) proto.Merge(req, tc.req) @@ -146,7 +144,10 @@ func TestGet(t *testing.T) { require.NoError(err, "Couldn't create new project service.") got, gErr := s.GetScope(auth.DisabledAuthTestContext(auth.WithScopeId(tc.scopeId)), req) - assert.Equal(tc.errCode, status.Code(gErr), "GetScope(%+v) got error\n%v, wanted\n%v", req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "GetScope(%+v) got error\n%v, wanted\n%v", req, gErr, tc.err) + } assert.Empty(cmp.Diff(tc.res, got, protocmp.Transform()), "GetScope(%q) got response\n%q, wanted\n%q", req, got, tc.res) }) } @@ -183,21 +184,19 @@ func TestList(t *testing.T) { scopeId string req *pbs.ListScopesRequest res *pbs.ListScopesResponse - errCode codes.Code + err error }{ { name: "List initial orgs", scopeId: scope.Global.String(), req: &pbs.ListScopesRequest{ScopeId: "global"}, res: &pbs.ListScopesResponse{Items: initialOrgs}, - errCode: codes.OK, }, { name: "List No Projects", scopeId: oNoProjects.GetPublicId(), req: &pbs.ListScopesRequest{ScopeId: oNoProjects.GetPublicId()}, res: &pbs.ListScopesResponse{}, - errCode: codes.OK, }, } for _, tc := range cases { @@ -207,7 +206,10 @@ func TestList(t *testing.T) { require.NoError(err, "Couldn't create new role service.") got, gErr := s.ListScopes(auth.DisabledAuthTestContext(auth.WithScopeId(tc.scopeId)), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "ListScopes(%+v) got error\n%v, wanted\n%v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "ListScopes(%+v) got error\n%v, wanted\n%v", tc.req, gErr, tc.err) + } assert.Empty(cmp.Diff(got, tc.res, protocmp.Transform()), "ListScopes(%q) got response\n%q\nwanted\n%q", tc.req, got, tc.res) }) } @@ -254,21 +256,19 @@ func TestList(t *testing.T) { scopeId string req *pbs.ListScopesRequest res *pbs.ListScopesResponse - errCode codes.Code + err error }{ { name: "List Many Orgs", scopeId: scope.Global.String(), req: &pbs.ListScopesRequest{ScopeId: "global"}, res: &pbs.ListScopesResponse{Items: wantOrgs}, - errCode: codes.OK, }, { name: "List Many Projects", scopeId: oWithProjects.GetPublicId(), req: &pbs.ListScopesRequest{ScopeId: oWithProjects.GetPublicId()}, res: &pbs.ListScopesResponse{Items: wantProjects}, - errCode: codes.OK, }, } for _, tc := range cases { @@ -278,25 +278,27 @@ func TestList(t *testing.T) { require.NoError(err, "Couldn't create new role service.") got, gErr := s.ListScopes(auth.DisabledAuthTestContext(auth.WithScopeId(tc.scopeId)), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "ListScopes(%+v) got error\n%v, wanted\n%v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "ListScopes(%+v) got error\n%v, wanted\n%v", tc.req, gErr, tc.err) + } assert.Empty(cmp.Diff(got, tc.res, protocmp.Transform()), "ListScopes(%q) got response\n%q, wanted\n%q", tc.req, got, tc.res) }) } } func TestDelete(t *testing.T) { - require := require.New(t) org, proj, repo := createDefaultScopesAndRepo(t) s, err := scopes.NewService(repo) - require.NoError(err, "Error when getting new project service.") + require.NoError(t, err, "Error when getting new project service.") cases := []struct { name string scopeId string req *pbs.DeleteScopeRequest res *pbs.DeleteScopeResponse - errCode codes.Code + err error }{ { name: "Delete an Existing Project", @@ -304,8 +306,7 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteScopeRequest{ Id: proj.GetPublicId(), }, - res: &pbs.DeleteScopeResponse{}, - errCode: codes.OK, + res: &pbs.DeleteScopeResponse{}, }, { name: "Delete bad project id Project", @@ -313,7 +314,7 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteScopeRequest{ Id: "p_doesntexis", }, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { name: "Bad Project Id formatting", @@ -321,7 +322,7 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteScopeRequest{ Id: "bad_format", }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Delete an Existing Org", @@ -329,8 +330,7 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteScopeRequest{ Id: org.GetPublicId(), }, - res: &pbs.DeleteScopeResponse{}, - errCode: codes.OK, + res: &pbs.DeleteScopeResponse{}, }, { name: "Delete bad org id Org", @@ -338,7 +338,7 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteScopeRequest{ Id: "p_doesntexis", }, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { name: "Bad Org Id formatting", @@ -346,22 +346,24 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteScopeRequest{ Id: "bad_format", }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) got, gErr := s.DeleteScope(auth.DisabledAuthTestContext(auth.WithScopeId(tc.scopeId)), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "DeleteScope(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "DeleteScope(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } assert.EqualValuesf(tc.res, got, "DeleteScope(%q) got response %q, wanted %q", tc.req, got, tc.res) }) } } func TestDelete_twice(t *testing.T) { - assert := assert.New(t) - require := require.New(t) + assert, require := assert.New(t), require.New(t) org, proj, repo := createDefaultScopesAndRepo(t) s, err := scopes.NewService(repo) @@ -374,7 +376,7 @@ func TestDelete_twice(t *testing.T) { assert.NoError(gErr, "First attempt") _, gErr = s.DeleteScope(ctx, req) assert.Error(gErr, "Second attempt") - assert.Equal(codes.NotFound, status.Code(gErr), "Expected not found for the second delete.") + assert.True(errors.Is(gErr, handlers.ApiErrorWithCode(codes.NotFound)), "Expected not found for the second delete.") ctx = auth.DisabledAuthTestContext(auth.WithScopeId(scope.Global.String())) req = &pbs.DeleteScopeRequest{ @@ -384,7 +386,7 @@ func TestDelete_twice(t *testing.T) { assert.NoError(gErr, "First attempt") _, gErr = s.DeleteScope(ctx, req) assert.Error(gErr, "Second attempt") - assert.Equal(codes.NotFound, status.Code(gErr), "Expected not found for the second delete.") + assert.True(errors.Is(gErr, handlers.ApiErrorWithCode(codes.NotFound)), "Expected not found for the second delete.") } func TestCreate(t *testing.T) { @@ -410,7 +412,7 @@ func TestCreate(t *testing.T) { scopeId string req *pbs.CreateScopeRequest res *pbs.CreateScopeResponse - errCode codes.Code + err error }{ { name: "Create a valid Project", @@ -433,7 +435,6 @@ func TestCreate(t *testing.T) { Type: scope.Project.String(), }, }, - errCode: codes.OK, }, { name: "Create a valid Org", @@ -456,7 +457,6 @@ func TestCreate(t *testing.T) { Type: scope.Org.String(), }, }, - errCode: codes.OK, }, { name: "Create a valid Project with type specified", @@ -478,7 +478,6 @@ func TestCreate(t *testing.T) { Type: scope.Project.String(), }, }, - errCode: codes.OK, }, { name: "Create a valid Org with type specified", @@ -500,7 +499,6 @@ func TestCreate(t *testing.T) { Type: scope.Org.String(), }, }, - errCode: codes.OK, }, { name: "Project with bad type specified", @@ -512,7 +510,7 @@ func TestCreate(t *testing.T) { Type: scope.Org.String(), }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Org with bad type specified", @@ -524,7 +522,7 @@ func TestCreate(t *testing.T) { Type: scope.Project.String(), }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Can't specify Id", @@ -532,8 +530,8 @@ func TestCreate(t *testing.T) { req: &pbs.CreateScopeRequest{Item: &pb.Scope{ Id: "not allowed to be set", }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Can't specify Created Time", @@ -541,8 +539,8 @@ func TestCreate(t *testing.T) { req: &pbs.CreateScopeRequest{Item: &pb.Scope{ CreatedTime: ptypes.TimestampNow(), }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Can't specify Update Time", @@ -550,8 +548,8 @@ func TestCreate(t *testing.T) { req: &pbs.CreateScopeRequest{Item: &pb.Scope{ UpdatedTime: ptypes.TimestampNow(), }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { @@ -586,7 +584,10 @@ func TestCreate(t *testing.T) { assert.NotEmpty(userId) } got, gErr := s.CreateScope(auth.DisabledAuthTestContext(auth.WithScopeId(tc.scopeId), auth.WithUserId(userId)), req) - assert.Equal(tc.errCode, status.Code(gErr), "CreateScope(%+v) got error %v, wanted %v", req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "CreateScope(%+v) got error %v, wanted %v", req, gErr, tc.err) + } if got != nil { assert.Contains(got.GetUri(), tc.res.Uri) switch tc.scopeId { @@ -668,7 +669,7 @@ func TestUpdate(t *testing.T) { scopeId string req *pbs.UpdateScopeRequest res *pbs.UpdateScopeResponse - errCode codes.Code + err error }{ { name: "Update an Existing Project", @@ -694,7 +695,6 @@ func TestUpdate(t *testing.T) { Type: scope.Project.String(), }, }, - errCode: codes.OK, }, { name: "Update an Existing Org", @@ -720,7 +720,6 @@ func TestUpdate(t *testing.T) { Type: scope.Org.String(), }, }, - errCode: codes.OK, }, { name: "Multiple Paths in single string", @@ -745,7 +744,6 @@ func TestUpdate(t *testing.T) { Type: scope.Project.String(), }, }, - errCode: codes.OK, }, { name: "No Update Mask", @@ -756,7 +754,7 @@ func TestUpdate(t *testing.T) { Description: &wrapperspb.StringValue{Value: "updated desc"}, }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant modify type", @@ -770,7 +768,7 @@ func TestUpdate(t *testing.T) { Type: scope.Org.String(), }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "No Paths in Mask", @@ -782,7 +780,7 @@ func TestUpdate(t *testing.T) { Description: &wrapperspb.StringValue{Value: "updated desc"}, }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Only non-existant paths in Mask", @@ -794,7 +792,7 @@ func TestUpdate(t *testing.T) { Description: &wrapperspb.StringValue{Value: "updated desc"}, }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Unset Name", @@ -817,7 +815,6 @@ func TestUpdate(t *testing.T) { Type: scope.Project.String(), }, }, - errCode: codes.OK, }, { name: "Unset Description", @@ -840,7 +837,6 @@ func TestUpdate(t *testing.T) { Type: scope.Project.String(), }, }, - errCode: codes.OK, }, { name: "Update Only Name", @@ -865,7 +861,6 @@ func TestUpdate(t *testing.T) { Type: scope.Project.String(), }, }, - errCode: codes.OK, }, { name: "Update Only Description", @@ -890,7 +885,6 @@ func TestUpdate(t *testing.T) { Type: scope.Project.String(), }, }, - errCode: codes.OK, }, { name: "Update a Non Existing Project", @@ -905,7 +899,7 @@ func TestUpdate(t *testing.T) { Description: &wrapperspb.StringValue{Value: "desc"}, }, }, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { name: "Cant change Id", @@ -921,8 +915,8 @@ func TestUpdate(t *testing.T) { Name: &wrapperspb.StringValue{Value: "new"}, Description: &wrapperspb.StringValue{Value: "new desc"}, }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant specify Created Time", @@ -935,8 +929,8 @@ func TestUpdate(t *testing.T) { CreatedTime: ptypes.TimestampNow(), }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant specify Updated Time", @@ -949,8 +943,8 @@ func TestUpdate(t *testing.T) { UpdatedTime: ptypes.TimestampNow(), }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { @@ -966,21 +960,24 @@ func TestUpdate(t *testing.T) { switch tc.scopeId { case scope.Global.String(): req = proto.Clone(orgToMerge).(*pbs.UpdateScopeRequest) - if tc.errCode == codes.OK { + if tc.err == nil { defer resetOrg() } default: ver = projVersion tc.req.Item.Version = ver req = proto.Clone(projToMerge).(*pbs.UpdateScopeRequest) - if tc.errCode == codes.OK { + if tc.err == nil { defer resetProject() } } proto.Merge(req, tc.req) got, gErr := tested.UpdateScope(auth.DisabledAuthTestContext(auth.WithScopeId(tc.scopeId)), req) - assert.Equal(tc.errCode, status.Code(gErr), "UpdateScope(%+v) got error\n%v, wanted\n%v", req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "UpdateScope(%+v) got error\n%v, wanted\n%v", req, gErr, tc.err) + } if got != nil { assert.NotNilf(tc.res, "Expected UpdateScope response to be nil, but was %v", got) diff --git a/internal/servers/controller/handlers/sessions/session_service_test.go b/internal/servers/controller/handlers/sessions/session_service_test.go index 3f570bfc1..09debd1d0 100644 --- a/internal/servers/controller/handlers/sessions/session_service_test.go +++ b/internal/servers/controller/handlers/sessions/session_service_test.go @@ -1,6 +1,7 @@ package sessions_test import ( + "errors" "testing" "time" @@ -14,6 +15,7 @@ import ( "github.com/hashicorp/boundary/internal/host/static" "github.com/hashicorp/boundary/internal/iam" "github.com/hashicorp/boundary/internal/kms" + "github.com/hashicorp/boundary/internal/servers/controller/handlers" "github.com/hashicorp/boundary/internal/servers/controller/handlers/sessions" "github.com/hashicorp/boundary/internal/session" "github.com/hashicorp/boundary/internal/target" @@ -21,7 +23,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" "google.golang.org/protobuf/testing/protocmp" ) @@ -84,32 +85,31 @@ func TestGetSession(t *testing.T) { scopeId string req *pbs.GetSessionRequest res *pbs.GetSessionResponse - errCode codes.Code + err error }{ { name: "Get a session", scopeId: sess.ScopeId, req: &pbs.GetSessionRequest{Id: sess.GetPublicId()}, res: &pbs.GetSessionResponse{Item: wireSess}, - errCode: codes.OK, }, { - name: "Get a non existant Session", - req: &pbs.GetSessionRequest{Id: session.SessionPrefix + "_DoesntExis"}, - res: nil, - errCode: codes.NotFound, + name: "Get a non existant Session", + req: &pbs.GetSessionRequest{Id: session.SessionPrefix + "_DoesntExis"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { - name: "Wrong id prefix", - req: &pbs.GetSessionRequest{Id: "j_1234567890"}, - res: nil, - errCode: codes.InvalidArgument, + name: "Wrong id prefix", + req: &pbs.GetSessionRequest{Id: "j_1234567890"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { - name: "space in id", - req: &pbs.GetSessionRequest{Id: session.SessionPrefix + "_1 23456789"}, - res: nil, - errCode: codes.InvalidArgument, + name: "space in id", + req: &pbs.GetSessionRequest{Id: session.SessionPrefix + "_1 23456789"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { @@ -120,7 +120,10 @@ func TestGetSession(t *testing.T) { require.NoError(err, "Couldn't create new session service.") got, gErr := s.GetSession(auth.DisabledAuthTestContext(auth.WithScopeId(tc.scopeId)), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "GetSession(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "GetSession(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } if tc.res != nil { assert.True(got.GetItem().GetExpirationTime().AsTime().Sub(tc.res.GetItem().GetExpirationTime().AsTime()) < 10*time.Millisecond) tc.res.GetItem().ExpirationTime = got.GetItem().GetExpirationTime() @@ -192,22 +195,20 @@ func TestList(t *testing.T) { } cases := []struct { - name string - req *pbs.ListSessionsRequest - res *pbs.ListSessionsResponse - errCode codes.Code + name string + req *pbs.ListSessionsRequest + res *pbs.ListSessionsResponse + err error }{ { - name: "List Many Sessions", - req: &pbs.ListSessionsRequest{ScopeId: pWithSessions.GetPublicId()}, - res: &pbs.ListSessionsResponse{Items: wantSession}, - errCode: codes.OK, + name: "List Many Sessions", + req: &pbs.ListSessionsRequest{ScopeId: pWithSessions.GetPublicId()}, + res: &pbs.ListSessionsResponse{Items: wantSession}, }, { - name: "List No Sessions", - req: &pbs.ListSessionsRequest{ScopeId: pNoSessions.GetPublicId()}, - res: &pbs.ListSessionsResponse{}, - errCode: codes.OK, + name: "List No Sessions", + req: &pbs.ListSessionsRequest{ScopeId: pNoSessions.GetPublicId()}, + res: &pbs.ListSessionsResponse{}, }, } for _, tc := range cases { @@ -216,7 +217,10 @@ func TestList(t *testing.T) { require.NoError(t, err, "Couldn't create new session service.") got, gErr := s.ListSessions(auth.DisabledAuthTestContext(auth.WithScopeId(tc.req.GetScopeId())), tc.req) - assert.Equal(t, tc.errCode, status.Code(gErr), "ListSessions(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(t, gErr) + assert.True(t, errors.Is(gErr, tc.err), "ListSessions(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } if tc.res != nil { for i, wantSess := range tc.res.GetItems() { assert.True(t, got.GetItems()[i].GetExpirationTime().AsTime().Sub(wantSess.GetExpirationTime().AsTime()) < 10*time.Millisecond) @@ -304,32 +308,31 @@ func TestCancel(t *testing.T) { scopeId string req *pbs.CancelSessionRequest res *pbs.CancelSessionResponse - errCode codes.Code + err error }{ { name: "Cancel a session", scopeId: sess.ScopeId, req: &pbs.CancelSessionRequest{Id: sess.GetPublicId()}, res: &pbs.CancelSessionResponse{Item: wireSess}, - errCode: codes.OK, }, { - name: "Cancel a non existing Session", - req: &pbs.CancelSessionRequest{Id: session.SessionPrefix + "_DoesntExis"}, - res: nil, - errCode: codes.NotFound, + name: "Cancel a non existing Session", + req: &pbs.CancelSessionRequest{Id: session.SessionPrefix + "_DoesntExis"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { - name: "Wrong id prefix", - req: &pbs.CancelSessionRequest{Id: "j_1234567890"}, - res: nil, - errCode: codes.InvalidArgument, + name: "Wrong id prefix", + req: &pbs.CancelSessionRequest{Id: "j_1234567890"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { - name: "space in id", - req: &pbs.CancelSessionRequest{Id: session.SessionPrefix + "_1 23456789"}, - res: nil, - errCode: codes.InvalidArgument, + name: "space in id", + req: &pbs.CancelSessionRequest{Id: session.SessionPrefix + "_1 23456789"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { @@ -342,7 +345,10 @@ func TestCancel(t *testing.T) { tc.req.Version = version got, gErr := s.CancelSession(auth.DisabledAuthTestContext(auth.WithScopeId(tc.scopeId)), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "GetSession(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "GetSession(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } if tc.res == nil { require.Nil(got) diff --git a/internal/servers/controller/handlers/targets/target_service.go b/internal/servers/controller/handlers/targets/target_service.go index 7a7004048..65e0e260f 100644 --- a/internal/servers/controller/handlers/targets/target_service.go +++ b/internal/servers/controller/handlers/targets/target_service.go @@ -26,7 +26,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/proto" "google.golang.org/protobuf/types/known/timestamppb" "google.golang.org/protobuf/types/known/wrapperspb" @@ -444,14 +443,14 @@ func (s Service) createInRepo(ctx context.Context, item *pb.Target) (*pb.Target, } tcpAttrs := &pb.TcpTargetAttributes{} if err := handlers.StructToProto(item.GetAttributes(), tcpAttrs); 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 tcpAttrs.GetDefaultPort().GetValue() != 0 { opts = append(opts, target.WithDefaultPort(tcpAttrs.GetDefaultPort().GetValue())) } u, err := target.NewTcpTarget(item.GetScopeId(), opts...) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to build target for creation: %v.", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to build target for creation: %v.", err) } repo, err := s.repoFn() if err != nil { @@ -459,10 +458,10 @@ func (s Service) createInRepo(ctx context.Context, item *pb.Target) (*pb.Target, } out, m, err := repo.CreateTcpTarget(ctx, u) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to create target: %v.", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to create target: %v.", err) } if out == nil { - return nil, status.Error(codes.Internal, "Unable to create target but no error returned from repository.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to create target but no error returned from repository.") } return toProto(out, m) } @@ -483,7 +482,7 @@ func (s Service) updateInRepo(ctx context.Context, scopeId, id string, mask []st } tcpAttrs := &pb.TcpTargetAttributes{} if err := handlers.StructToProto(item.GetAttributes(), tcpAttrs); 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 tcpAttrs.GetDefaultPort().GetValue() != 0 { opts = append(opts, target.WithDefaultPort(tcpAttrs.GetDefaultPort().GetValue())) @@ -491,7 +490,7 @@ func (s Service) updateInRepo(ctx context.Context, scopeId, id string, mask []st version := item.GetVersion() u, err := target.NewTcpTarget(scopeId, opts...) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to build target for update: %v.", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to build target for update: %v.", err) } u.PublicId = id dbMask := maskManager.Translate(mask) @@ -504,7 +503,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, status.Errorf(codes.Internal, "Unable to update target: %v.", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to update target: %v.", err) } if rowsUpdated == 0 { return nil, handlers.NotFoundErrorf("Target %q not found or incorrect version provided.", id) @@ -522,7 +521,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 target: %v.", err) + return false, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to delete target: %v.", err) } return rows > 0, nil } @@ -540,7 +539,7 @@ func (s Service) listFromRepo(ctx context.Context, scopeId string) ([]*pb.Target for _, u := range ul { o, err := toProto(u, nil) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to convert value to proto: %v.", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to convert value to proto: %v.", err) } outUl = append(outUl, o) } @@ -554,10 +553,10 @@ func (s Service) addInRepo(ctx context.Context, targetId string, hostSetId []str } out, m, err := repo.AddTargetHostSets(ctx, targetId, version, hostSetId) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to add host sets to target: %v.", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to add host sets to target: %v.", err) } if out == nil { - return nil, status.Error(codes.Internal, "Unable to lookup target after adding host sets to it.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to lookup target after adding host sets to it.") } return toProto(out, m) } @@ -569,15 +568,15 @@ func (s Service) setInRepo(ctx context.Context, targetId string, hostSetIds []st } _, _, err = repo.SetTargetHostSets(ctx, targetId, version, hostSetIds) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to set host sets in target: %v.", err) + 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, status.Errorf(codes.Internal, "Unable to look up target: %v.", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to look up target: %v.", err) } if out == nil { - return nil, status.Error(codes.Internal, "Unable to lookup target after setting host sets for it.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to lookup target after setting host sets for it.") } return toProto(out, m) } @@ -589,14 +588,14 @@ func (s Service) removeInRepo(ctx context.Context, targetId string, hostSetIds [ } _, err = repo.DeleteTargeHostSets(ctx, targetId, version, hostSetIds) if err != nil { - return nil, status.Errorf(codes.Internal, "Unable to remove host sets from target: %v.", err) + 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, status.Errorf(codes.Internal, "Unable to look up target: %v.", err) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to look up target: %v.", err) } if out == nil { - return nil, status.Error(codes.Internal, "Unable to lookup target after removing host sets from it.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Unable to lookup target after removing host sets from it.") } return toProto(out, m) } @@ -668,7 +667,7 @@ func toProto(in target.Target, m []*target.TargetSet) (*pb.Target, error) { } st, err := handlers.ProtoToStruct(attrs) 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 for _, hs := range m { diff --git a/internal/servers/controller/handlers/targets/target_service_test.go b/internal/servers/controller/handlers/targets/target_service_test.go index 857313ab4..725fdae5d 100644 --- a/internal/servers/controller/handlers/targets/target_service_test.go +++ b/internal/servers/controller/handlers/targets/target_service_test.go @@ -2,6 +2,7 @@ package targets_test import ( "context" + "errors" "fmt" "strings" "testing" @@ -17,6 +18,7 @@ import ( "github.com/hashicorp/boundary/internal/iam" "github.com/hashicorp/boundary/internal/kms" "github.com/hashicorp/boundary/internal/servers" + "github.com/hashicorp/boundary/internal/servers/controller/handlers" "github.com/hashicorp/boundary/internal/servers/controller/handlers/targets" "github.com/hashicorp/boundary/internal/session" "github.com/hashicorp/boundary/internal/target" @@ -85,45 +87,47 @@ func TestGet(t *testing.T) { } cases := []struct { - name string - req *pbs.GetTargetRequest - res *pbs.GetTargetResponse - errCode codes.Code + name string + req *pbs.GetTargetRequest + res *pbs.GetTargetResponse + err error }{ { - name: "Get an Existing Target", - req: &pbs.GetTargetRequest{Id: tar.GetPublicId()}, - res: &pbs.GetTargetResponse{Item: pTar}, - errCode: codes.OK, + name: "Get an Existing Target", + req: &pbs.GetTargetRequest{Id: tar.GetPublicId()}, + res: &pbs.GetTargetResponse{Item: pTar}, }, { - name: "Get a non existing Target", - req: &pbs.GetTargetRequest{Id: target.TcpTargetPrefix + "_DoesntExis"}, - res: nil, - errCode: codes.NotFound, + name: "Get a non existing Target", + req: &pbs.GetTargetRequest{Id: target.TcpTargetPrefix + "_DoesntExis"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { - name: "Wrong id prefix", - req: &pbs.GetTargetRequest{Id: "j_1234567890"}, - res: nil, - errCode: codes.InvalidArgument, + name: "Wrong id prefix", + req: &pbs.GetTargetRequest{Id: "j_1234567890"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { - name: "space in id", - req: &pbs.GetTargetRequest{Id: target.TcpTargetPrefix + "_1 23456789"}, - res: nil, - errCode: codes.InvalidArgument, + name: "space in id", + req: &pbs.GetTargetRequest{Id: target.TcpTargetPrefix + "_1 23456789"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) s, err := testService(t, conn, kms, wrapper) - require.NoError(t, err, "Couldn't create a new host set service.") + require.NoError(err, "Couldn't create a new host set service.") got, gErr := s.GetTarget(auth.DisabledAuthTestContext(auth.WithScopeId(proj.GetPublicId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "GetTarget(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "GetTarget(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } if tc.res != nil { tc.res.Item.Version = 1 @@ -166,19 +170,17 @@ func TestList(t *testing.T) { name string scopeId string res *pbs.ListTargetsResponse - errCode codes.Code + err error }{ { name: "List Many Host Sets", scopeId: proj.GetPublicId(), res: &pbs.ListTargetsResponse{Items: wantTars}, - errCode: codes.OK, }, { name: "List No Host Sets", scopeId: projNoTar.GetPublicId(), res: &pbs.ListTargetsResponse{}, - errCode: codes.OK, }, } for _, tc := range cases { @@ -188,7 +190,10 @@ func TestList(t *testing.T) { require.NoError(err, "Couldn't create new host set service.") got, gErr := s.ListTargets(auth.DisabledAuthTestContext(auth.WithScopeId(tc.scopeId)), &pbs.ListTargetsRequest{ScopeId: tc.scopeId}) - assert.Equal(tc.errCode, status.Code(gErr), "ListTargets(%q) got error %v, wanted %v", tc.scopeId, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "ListTargets(%q) got error %v, wanted %v", tc.scopeId, gErr, tc.err) + } assert.Empty(cmp.Diff(got, tc.res, protocmp.Transform()), "ListTargets(%q) got response %q, wanted %q", tc.scopeId, got, tc.res) }) } @@ -211,7 +216,7 @@ func TestDelete(t *testing.T) { scopeId string req *pbs.DeleteTargetRequest res *pbs.DeleteTargetResponse - errCode codes.Code + err error }{ { name: "Delete an Existing Target", @@ -219,8 +224,7 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteTargetRequest{ Id: tar.GetPublicId(), }, - res: &pbs.DeleteTargetResponse{}, - errCode: codes.OK, + res: &pbs.DeleteTargetResponse{}, }, { name: "Delete Not Existing Target", @@ -228,7 +232,7 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteTargetRequest{ Id: target.TcpTargetPrefix + "_doesntexis", }, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { name: "Bad target id formatting", @@ -236,14 +240,17 @@ func TestDelete(t *testing.T) { req: &pbs.DeleteTargetRequest{ Id: target.TcpTargetPrefix + "_bad_format", }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) got, gErr := s.DeleteTarget(auth.DisabledAuthTestContext(auth.WithScopeId(tc.scopeId)), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "DeleteTarget(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "DeleteTarget(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } assert.Empty(cmp.Diff(tc.res, got, protocmp.Transform()), "DeleteTarget(%q) got response %q, wanted %q", tc.req, got, tc.res) }) } @@ -251,7 +258,7 @@ func TestDelete(t *testing.T) { func TestDelete_twice(t *testing.T) { t.Parallel() - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) conn, _ := db.TestSetup(t, "postgres") wrapper := db.TestWrapper(t) kms := kms.TestKms(t, conn, wrapper) @@ -261,7 +268,7 @@ func TestDelete_twice(t *testing.T) { tar := target.TestTcpTarget(t, conn, proj.GetPublicId(), "test") s, err := testService(t, conn, kms, wrapper) - require.NoError(t, err, "Couldn't create a new target service.") + require.NoError(err, "Couldn't create a new target service.") req := &pbs.DeleteTargetRequest{ Id: tar.GetPublicId(), } @@ -270,7 +277,7 @@ func TestDelete_twice(t *testing.T) { assert.NoError(gErr, "First attempt") _, gErr = s.DeleteTarget(ctx, req) assert.Error(gErr, "Second attempt") - assert.Equal(codes.NotFound, status.Code(gErr), "Expected permission denied for the second delete.") + assert.True(errors.Is(gErr, handlers.ApiErrorWithCode(codes.NotFound)), "Expected permission denied for the second delete.") } func TestCreate(t *testing.T) { @@ -282,10 +289,10 @@ func TestCreate(t *testing.T) { _, proj := iam.TestScopes(t, iam.TestRepo(t, conn, wrapper)) cases := []struct { - name string - req *pbs.CreateTargetRequest - res *pbs.CreateTargetResponse - errCode codes.Code + name string + req *pbs.CreateTargetRequest + res *pbs.CreateTargetResponse + err error }{ { name: "Create a valid target", @@ -313,7 +320,6 @@ func TestCreate(t *testing.T) { SessionConnectionLimit: wrapperspb.Int32(1), }, }, - errCode: codes.OK, }, { name: "Create with default port 0", @@ -325,7 +331,7 @@ func TestCreate(t *testing.T) { "default_port": structpb.NewNumberValue(0), }}, }}, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Create with unknown type", @@ -334,7 +340,7 @@ func TestCreate(t *testing.T) { Description: wrapperspb.String("desc"), Type: "ThisIsMadeUp", }}, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Create with no type", @@ -342,31 +348,31 @@ func TestCreate(t *testing.T) { Name: wrapperspb.String("name"), Description: wrapperspb.String("desc"), }}, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Can't specify Id", req: &pbs.CreateTargetRequest{Item: &pb.Target{ Id: "not allowed to be set", }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Can't specify Created Time", req: &pbs.CreateTargetRequest{Item: &pb.Target{ CreatedTime: ptypes.TimestampNow(), }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Can't specify Update Time", req: &pbs.CreateTargetRequest{Item: &pb.Target{ UpdatedTime: ptypes.TimestampNow(), }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { @@ -377,7 +383,10 @@ func TestCreate(t *testing.T) { require.NoError(err, "Failed to create a new host set service.") got, gErr := s.CreateTarget(auth.DisabledAuthTestContext(auth.WithScopeId(proj.GetPublicId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "CreateTarget(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "CreateTarget(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } if got != nil { assert.Contains(got.GetUri(), tc.res.GetUri()) assert.True(strings.HasPrefix(got.GetItem().GetId(), target.TcpTargetPrefix), got.GetItem().GetId()) @@ -444,10 +453,10 @@ func TestUpdate(t *testing.T) { require.NoError(t, err, "Failed to create a new host set service.") cases := []struct { - name string - req *pbs.UpdateTargetRequest - res *pbs.UpdateTargetResponse - errCode codes.Code + name string + req *pbs.UpdateTargetRequest + res *pbs.UpdateTargetResponse + err error }{ { name: "Update an Existing Target", @@ -480,7 +489,6 @@ func TestUpdate(t *testing.T) { SessionConnectionLimit: wrapperspb.Int32(5), }, }, - errCode: codes.OK, }, { name: "Multiple Paths in single string", @@ -511,7 +519,6 @@ func TestUpdate(t *testing.T) { SessionConnectionLimit: wrapperspb.Int32(5), }, }, - errCode: codes.OK, }, { name: "No Update Mask", @@ -521,7 +528,7 @@ func TestUpdate(t *testing.T) { Description: wrapperspb.String("updated desc"), }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Empty Path", @@ -532,7 +539,7 @@ func TestUpdate(t *testing.T) { Description: wrapperspb.String("updated desc"), }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Update port to 0", @@ -544,7 +551,7 @@ func TestUpdate(t *testing.T) { }}, }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Only non-existant paths in Mask", @@ -555,7 +562,7 @@ func TestUpdate(t *testing.T) { Description: wrapperspb.String("updated desc"), }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Unset Name", @@ -567,7 +574,7 @@ func TestUpdate(t *testing.T) { Description: wrapperspb.String("ignored"), }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Unset Description", @@ -596,7 +603,6 @@ func TestUpdate(t *testing.T) { SessionConnectionLimit: wrapperspb.Int32(5), }, }, - errCode: codes.OK, }, { name: "Update Only Name", @@ -627,7 +633,6 @@ func TestUpdate(t *testing.T) { SessionConnectionLimit: wrapperspb.Int32(5), }, }, - errCode: codes.OK, }, { name: "Update Only Description", @@ -658,7 +663,6 @@ func TestUpdate(t *testing.T) { SessionConnectionLimit: wrapperspb.Int32(5), }, }, - errCode: codes.OK, }, { name: "Update a Non Existing Target", @@ -673,7 +677,7 @@ func TestUpdate(t *testing.T) { Description: wrapperspb.String("desc"), }, }, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { name: "Cant change Id", @@ -688,8 +692,8 @@ func TestUpdate(t *testing.T) { Name: wrapperspb.String("new"), Description: wrapperspb.String("new desc"), }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant specify Created Time", @@ -701,8 +705,8 @@ func TestUpdate(t *testing.T) { CreatedTime: ptypes.TimestampNow(), }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant specify Updated Time", @@ -714,8 +718,8 @@ func TestUpdate(t *testing.T) { UpdatedTime: ptypes.TimestampNow(), }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Valid mask, cant specify type", @@ -727,8 +731,8 @@ func TestUpdate(t *testing.T) { Type: "Unknown", }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { @@ -740,9 +744,12 @@ func TestUpdate(t *testing.T) { proto.Merge(req, tc.req) got, gErr := tested.UpdateTarget(auth.DisabledAuthTestContext(auth.WithScopeId(proj.GetPublicId())), req) - assert.Equal(tc.errCode, status.Code(gErr), "UpdateTarget(%+v) got error %v, wanted %v", req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "UpdateTarget(%+v) got error %v, wanted %v", req, gErr, tc.err) + } - if tc.errCode == codes.OK { + if tc.err == nil { defer resetTarget() } @@ -818,9 +825,9 @@ func TestAddTargetHostSets(t *testing.T) { tar := target.TestTcpTarget(t, conn, proj.GetPublicId(), "test") failCases := []struct { - name string - req *pbs.AddTargetHostSetsRequest - errCode codes.Code + name string + req *pbs.AddTargetHostSetsRequest + err error }{ { name: "Bad Set Id", @@ -829,7 +836,7 @@ func TestAddTargetHostSets(t *testing.T) { Version: tar.GetVersion(), HostSetIds: []string{hs[0].GetPublicId()}, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Bad version", @@ -838,7 +845,7 @@ func TestAddTargetHostSets(t *testing.T) { Version: tar.GetVersion() + 2, HostSetIds: []string{hs[0].GetPublicId()}, }, - errCode: codes.Internal, + err: handlers.ApiErrorWithCode(codes.Internal), }, { name: "Empty host set list", @@ -846,14 +853,17 @@ func TestAddTargetHostSets(t *testing.T) { Id: tar.GetPublicId(), Version: tar.GetVersion(), }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range failCases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) _, gErr := s.AddTargetHostSets(auth.DisabledAuthTestContext(auth.WithScopeId(proj.GetPublicId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "AddTargetHostSets(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "AddTargetHostSets(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } }) } } @@ -913,9 +923,9 @@ func TestSetTargetHostSets(t *testing.T) { tar := target.TestTcpTarget(t, conn, proj.GetPublicId(), "test name") failCases := []struct { - name string - req *pbs.SetTargetHostSetsRequest - errCode codes.Code + name string + req *pbs.SetTargetHostSetsRequest + err error }{ { name: "Bad target Id", @@ -924,7 +934,7 @@ func TestSetTargetHostSets(t *testing.T) { Version: tar.GetVersion(), HostSetIds: []string{hs[0].GetPublicId()}, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Bad version", @@ -933,14 +943,17 @@ func TestSetTargetHostSets(t *testing.T) { Version: tar.GetVersion() + 3, HostSetIds: []string{hs[0].GetPublicId()}, }, - errCode: codes.Internal, + err: handlers.ApiErrorWithCode(codes.Internal), }, } for _, tc := range failCases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) _, gErr := s.SetTargetHostSets(auth.DisabledAuthTestContext(auth.WithScopeId(proj.GetPublicId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "SetTargetHostSets(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "SetTargetHostSets(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } }) } } @@ -1009,9 +1022,9 @@ func TestRemoveTargetHostSets(t *testing.T) { tar := target.TestTcpTarget(t, conn, proj.GetPublicId(), "testing") failCases := []struct { - name string - req *pbs.RemoveTargetHostSetsRequest - errCode codes.Code + name string + req *pbs.RemoveTargetHostSetsRequest + err error }{ { name: "Bad version", @@ -1020,7 +1033,7 @@ func TestRemoveTargetHostSets(t *testing.T) { Version: tar.GetVersion() + 3, HostSetIds: []string{hs[0].GetPublicId()}, }, - errCode: codes.Internal, + err: handlers.ApiErrorWithCode(codes.Internal), }, { name: "Bad target Id", @@ -1029,7 +1042,7 @@ func TestRemoveTargetHostSets(t *testing.T) { Version: tar.GetVersion(), HostSetIds: []string{hs[0].GetPublicId()}, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "empty sets", @@ -1038,14 +1051,17 @@ func TestRemoveTargetHostSets(t *testing.T) { Version: tar.GetVersion(), HostSetIds: []string{}, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range failCases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) _, gErr := s.RemoveTargetHostSets(auth.DisabledAuthTestContext(auth.WithScopeId(proj.GetPublicId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "RemoveTargetHostSets(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "RemoveTargetHostSets(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } }) } } diff --git a/internal/servers/controller/handlers/users/user_service_test.go b/internal/servers/controller/handlers/users/user_service_test.go index a2e08bf80..fd91ee004 100644 --- a/internal/servers/controller/handlers/users/user_service_test.go +++ b/internal/servers/controller/handlers/users/user_service_test.go @@ -2,6 +2,7 @@ package users_test import ( "context" + "errors" "fmt" "strings" "testing" @@ -14,11 +15,11 @@ import ( pb "github.com/hashicorp/boundary/internal/gen/controller/api/resources/users" pbs "github.com/hashicorp/boundary/internal/gen/controller/api/services" "github.com/hashicorp/boundary/internal/iam" + "github.com/hashicorp/boundary/internal/servers/controller/handlers" "github.com/hashicorp/boundary/internal/servers/controller/handlers/users" "github.com/hashicorp/boundary/internal/types/scope" "google.golang.org/genproto/protobuf/field_mask" "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/testing/protocmp" "google.golang.org/protobuf/types/known/wrapperspb" @@ -41,8 +42,7 @@ func createDefaultUserAndRepo(t *testing.T) (*iam.User, func() (*iam.Repository, } func TestGet(t *testing.T) { - assert := assert.New(t) - require := require.New(t) + assert, require := assert.New(t), require.New(t) u, repoFn := createDefaultUserAndRepo(t) toMerge := &pbs.GetUserRequest{ Id: u.GetPublicId(), @@ -60,34 +60,33 @@ func TestGet(t *testing.T) { } cases := []struct { - name string - req *pbs.GetUserRequest - res *pbs.GetUserResponse - errCode codes.Code + name string + req *pbs.GetUserRequest + res *pbs.GetUserResponse + err error }{ { - name: "Get an Existing User", - req: &pbs.GetUserRequest{Id: u.GetPublicId()}, - res: &pbs.GetUserResponse{Item: wantU}, - errCode: codes.OK, + name: "Get an Existing User", + req: &pbs.GetUserRequest{Id: u.GetPublicId()}, + res: &pbs.GetUserResponse{Item: wantU}, }, { - name: "Get a non existant User", - req: &pbs.GetUserRequest{Id: iam.UserPrefix + "_DoesntExis"}, - res: nil, - errCode: codes.NotFound, + name: "Get a non existant User", + req: &pbs.GetUserRequest{Id: iam.UserPrefix + "_DoesntExis"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { - name: "Wrong id prefix", - req: &pbs.GetUserRequest{Id: "j_1234567890"}, - res: nil, - errCode: codes.InvalidArgument, + name: "Wrong id prefix", + req: &pbs.GetUserRequest{Id: "j_1234567890"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { - name: "space in id", - req: &pbs.GetUserRequest{Id: iam.UserPrefix + "_1 23456789"}, - res: nil, - errCode: codes.InvalidArgument, + name: "space in id", + req: &pbs.GetUserRequest{Id: iam.UserPrefix + "_1 23456789"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { @@ -99,7 +98,10 @@ func TestGet(t *testing.T) { require.NoError(err, "Couldn't create new user service.") got, gErr := s.GetUser(auth.DisabledAuthTestContext(auth.WithScopeId(u.GetScopeId())), req) - assert.Equal(tc.errCode, status.Code(gErr), "GetUser(%+v) got error %v, wanted %v", req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "GetUser(%+v) got error %v, wanted %v", req, gErr, tc.err) + } assert.Empty(cmp.Diff(got, tc.res, protocmp.Transform()), "GetUser(%q) got response %q, wanted %q", req, got, tc.res) }) } @@ -136,22 +138,20 @@ func TestList(t *testing.T) { } cases := []struct { - name string - req *pbs.ListUsersRequest - res *pbs.ListUsersResponse - errCode codes.Code + name string + req *pbs.ListUsersRequest + res *pbs.ListUsersResponse + err error }{ { - name: "List Many Users", - req: &pbs.ListUsersRequest{ScopeId: oWithUsers.GetPublicId()}, - res: &pbs.ListUsersResponse{Items: wantUsers}, - errCode: codes.OK, + name: "List Many Users", + req: &pbs.ListUsersRequest{ScopeId: oWithUsers.GetPublicId()}, + res: &pbs.ListUsersResponse{Items: wantUsers}, }, { - name: "List No Users", - req: &pbs.ListUsersRequest{ScopeId: oNoUsers.GetPublicId()}, - res: &pbs.ListUsersResponse{}, - errCode: codes.OK, + name: "List No Users", + req: &pbs.ListUsersRequest{ScopeId: oNoUsers.GetPublicId()}, + res: &pbs.ListUsersResponse{}, }, } for _, tc := range cases { @@ -160,60 +160,63 @@ func TestList(t *testing.T) { require.NoError(err, "Couldn't create new user service.") got, gErr := s.ListUsers(auth.DisabledAuthTestContext(auth.WithScopeId(tc.req.GetScopeId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "ListUsers(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "ListUsers(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } assert.Empty(cmp.Diff(got, tc.res, protocmp.Transform()), "ListUsers(%q) got response %q, wanted %q", tc.req, got, tc.res) }) } } func TestDelete(t *testing.T) { - require := require.New(t) u, repo := createDefaultUserAndRepo(t) s, err := users.NewService(repo) - require.NoError(err, "Error when getting new user service.") + require.NoError(t, err, "Error when getting new user service.") cases := []struct { - name string - req *pbs.DeleteUserRequest - res *pbs.DeleteUserResponse - errCode codes.Code + name string + req *pbs.DeleteUserRequest + res *pbs.DeleteUserResponse + err error }{ { name: "Delete an Existing User", req: &pbs.DeleteUserRequest{ Id: u.GetPublicId(), }, - errCode: codes.OK, }, { name: "Delete bad user id", req: &pbs.DeleteUserRequest{ Id: iam.UserPrefix + "_doesntexis", }, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { name: "Bad User Id formatting", req: &pbs.DeleteUserRequest{ Id: "bad_format", }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) got, gErr := s.DeleteUser(auth.DisabledAuthTestContext(auth.WithScopeId(u.GetScopeId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "DeleteUser(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "DeleteUser(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } assert.EqualValuesf(tc.res, got, "DeleteUser(%q) got response %q, wanted %q", tc.req, got, tc.res) }) } } func TestDelete_twice(t *testing.T) { - assert := assert.New(t) - require := require.New(t) + assert, require := assert.New(t), require.New(t) u, repo := createDefaultUserAndRepo(t) s, err := users.NewService(repo) @@ -226,20 +229,19 @@ func TestDelete_twice(t *testing.T) { assert.NoError(gErr, "First attempt") _, gErr = s.DeleteUser(ctx, req) assert.Error(gErr, "Second attempt") - assert.Equal(codes.NotFound, status.Code(gErr), "Expected permission denied for the second delete.") + assert.True(errors.Is(gErr, handlers.ApiErrorWithCode(codes.NotFound)), "Expected permission denied for the second delete.") } func TestCreate(t *testing.T) { - require := require.New(t) defaultUser, repo := createDefaultUserAndRepo(t) defaultCreated, err := ptypes.Timestamp(defaultUser.GetCreateTime().GetTimestamp()) - require.NoError(err, "Error converting proto to timestamp.") + require.NoError(t, err, "Error converting proto to timestamp.") cases := []struct { - name string - req *pbs.CreateUserRequest - res *pbs.CreateUserResponse - errCode codes.Code + name string + req *pbs.CreateUserRequest + res *pbs.CreateUserResponse + err error }{ { name: "Create a valid User", @@ -258,7 +260,6 @@ func TestCreate(t *testing.T) { Version: 1, }, }, - errCode: codes.OK, }, { name: "Create a valid Global User", @@ -277,7 +278,6 @@ func TestCreate(t *testing.T) { Version: 1, }, }, - errCode: codes.OK, }, { name: "Can't specify Id", @@ -285,8 +285,8 @@ func TestCreate(t *testing.T) { ScopeId: defaultUser.GetScopeId(), Id: iam.UserPrefix + "_notallowed", }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Can't specify Created Time", @@ -294,8 +294,8 @@ func TestCreate(t *testing.T) { ScopeId: defaultUser.GetScopeId(), CreatedTime: ptypes.TimestampNow(), }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Can't specify Update Time", @@ -303,18 +303,21 @@ func TestCreate(t *testing.T) { ScopeId: defaultUser.GetScopeId(), UpdatedTime: ptypes.TimestampNow(), }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) s, err := users.NewService(repo) require.NoError(err, "Error when getting new user service.") got, gErr := s.CreateUser(auth.DisabledAuthTestContext(auth.WithScopeId(tc.req.GetItem().GetScopeId())), tc.req) - assert.Equal(tc.errCode, status.Code(gErr), "CreateUser(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode) + if tc.err != nil { + require.Error(gErr) + assert.True(errors.Is(gErr, tc.err), "CreateUser(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } if got != nil { assert.Contains(got.GetUri(), tc.res.Uri) assert.True(strings.HasPrefix(got.GetItem().GetId(), iam.UserPrefix+"_")) @@ -359,10 +362,10 @@ func TestUpdate(t *testing.T) { } cases := []struct { - name string - req *pbs.UpdateUserRequest - res *pbs.UpdateUserResponse - errCode codes.Code + name string + req *pbs.UpdateUserRequest + res *pbs.UpdateUserResponse + err error }{ { name: "Update an Existing User", @@ -385,7 +388,6 @@ func TestUpdate(t *testing.T) { CreatedTime: u.GetCreateTime().GetTimestamp(), }, }, - errCode: codes.OK, }, { name: "Multiple Paths in single string", @@ -408,7 +410,6 @@ func TestUpdate(t *testing.T) { CreatedTime: u.GetCreateTime().GetTimestamp(), }, }, - errCode: codes.OK, }, { name: "No Update Mask", @@ -418,7 +419,7 @@ func TestUpdate(t *testing.T) { Description: &wrapperspb.StringValue{Value: "updated desc"}, }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "No Paths in Mask", @@ -429,7 +430,7 @@ func TestUpdate(t *testing.T) { Description: &wrapperspb.StringValue{Value: "updated desc"}, }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Only non-existant paths in Mask", @@ -440,7 +441,7 @@ func TestUpdate(t *testing.T) { Description: &wrapperspb.StringValue{Value: "updated desc"}, }, }, - errCode: codes.InvalidArgument, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Unset Name", @@ -461,7 +462,6 @@ func TestUpdate(t *testing.T) { CreatedTime: u.GetCreateTime().GetTimestamp(), }, }, - errCode: codes.OK, }, { name: "Update Only Name", @@ -484,7 +484,6 @@ func TestUpdate(t *testing.T) { CreatedTime: u.GetCreateTime().GetTimestamp(), }, }, - errCode: codes.OK, }, { name: "Update Only Description", @@ -507,7 +506,6 @@ func TestUpdate(t *testing.T) { CreatedTime: u.GetCreateTime().GetTimestamp(), }, }, - errCode: codes.OK, }, { name: "Update a Non Existing User", @@ -521,7 +519,7 @@ func TestUpdate(t *testing.T) { Description: &wrapperspb.StringValue{Value: "desc"}, }, }, - errCode: codes.NotFound, + err: handlers.ApiErrorWithCode(codes.NotFound), }, { name: "Cant change Id", @@ -535,8 +533,8 @@ func TestUpdate(t *testing.T) { Name: &wrapperspb.StringValue{Value: "new"}, Description: &wrapperspb.StringValue{Value: "new desc"}, }}, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant specify Created Time", @@ -548,8 +546,8 @@ func TestUpdate(t *testing.T) { CreatedTime: ptypes.TimestampNow(), }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "Cant specify Updated Time", @@ -561,8 +559,8 @@ func TestUpdate(t *testing.T) { UpdatedTime: ptypes.TimestampNow(), }, }, - res: nil, - errCode: codes.InvalidArgument, + res: nil, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, } for _, tc := range cases { @@ -583,9 +581,10 @@ func TestUpdate(t *testing.T) { req.Item.Version = version got, gErr := tested.UpdateUser(auth.DisabledAuthTestContext(auth.WithScopeId(u.GetScopeId())), req) - require.Equal(tc.errCode, status.Code(gErr), "UpdateUser(%+v) got error %v, wanted %v", req, gErr, tc.errCode) - - if tc.errCode == codes.OK { + if tc.err != nil { + require.Error(gErr) + require.True(errors.Is(gErr, tc.err), "UpdateUser(%+v) got error %v, wanted %v", req, gErr, tc.err) + } else { defer resetUser() } diff --git a/internal/servers/controller/handlers/verifiers.go b/internal/servers/controller/handlers/verifiers.go index fddbce5ee..ea7feeca6 100644 --- a/internal/servers/controller/handlers/verifiers.go +++ b/internal/servers/controller/handlers/verifiers.go @@ -48,7 +48,7 @@ func ValidateCreateRequest(i ApiResource, fn CustomValidatorFunc) error { badFields[k] = v } if len(badFields) > 0 { - return InvalidArgumentErrorf("Argument errors found in the request.", badFields) + return InvalidArgumentErrorf("Error in provided request.", badFields) } return nil } @@ -106,7 +106,7 @@ func ValidateUpdateRequest(prefix string, r UpdateRequest, i ApiResource, fn Cus } if len(badFields) > 0 { - return InvalidArgumentErrorf("Errors in provided fields.", badFields) + return InvalidArgumentErrorf("Error in provided request.", badFields) } return nil } @@ -124,7 +124,7 @@ func ValidateGetRequest(prefix string, r GetRequest, fn CustomValidatorFunc) err badFields[k] = v } if len(badFields) > 0 { - return InvalidArgumentErrorf("Improperly formatted identifier.", badFields) + return InvalidArgumentErrorf("Error in provided request.", badFields) } return nil } @@ -142,7 +142,7 @@ func ValidateDeleteRequest(prefix string, r DeleteRequest, fn CustomValidatorFun badFields[k] = v } if len(badFields) > 0 { - return InvalidArgumentErrorf("Errors in provided fields.", badFields) + return InvalidArgumentErrorf("Error in provided request.", badFields) } return nil } diff --git a/internal/servers/controller/handlers/verifiers_test.go b/internal/servers/controller/handlers/verifiers_test.go index 24d9ae1ea..568cf71de 100644 --- a/internal/servers/controller/handlers/verifiers_test.go +++ b/internal/servers/controller/handlers/verifiers_test.go @@ -1,28 +1,24 @@ package handlers import ( + "errors" "testing" pb "github.com/hashicorp/boundary/internal/gen/controller/api/resources/users" pbs "github.com/hashicorp/boundary/internal/gen/controller/api/services" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "google.golang.org/genproto/googleapis/rpc/errdetails" - "google.golang.org/grpc/status" "google.golang.org/protobuf/types/known/fieldmaskpb" "google.golang.org/protobuf/types/known/timestamppb" ) func errorIncludesFields(t *testing.T, err error, wantFields []string) { t.Helper() - s, ok := status.FromError(err) - require.True(t, ok) - require.Len(t, s.Details(), 1) - rd, ok := s.Details()[0].(*errdetails.BadRequest) - require.True(t, ok) + var apiErr *apiError + require.True(t, errors.As(err, &apiErr)) var gotFields []string - for _, d := range rd.FieldViolations { - gotFields = append(gotFields, d.Field) + for _, d := range apiErr.inner.GetDetails().GetRequestFields() { + gotFields = append(gotFields, d.GetName()) } assert.ElementsMatch(t, gotFields, wantFields) } diff --git a/sdk/strutil/strutil_test.go b/sdk/strutil/strutil_test.go index 698d2e826..18cd52bec 100644 --- a/sdk/strutil/strutil_test.go +++ b/sdk/strutil/strutil_test.go @@ -152,7 +152,7 @@ func TestStrutil_ParseKeyValues(t *testing.T) { if !reflect.DeepEqual(expected, actual) { t.Fatalf("bad: expected: %#v\nactual: %#v", expected, actual) } - for k, _ := range actual { + for k := range actual { delete(actual, k) } @@ -164,7 +164,7 @@ func TestStrutil_ParseKeyValues(t *testing.T) { if !reflect.DeepEqual(expected, actual) { t.Fatalf("bad: expected: %#v\nactual: %#v", expected, actual) } - for k, _ := range actual { + for k := range actual { delete(actual, k) } @@ -173,7 +173,7 @@ func TestStrutil_ParseKeyValues(t *testing.T) { if err == nil { t.Fatalf("expected an error") } - for k, _ := range actual { + for k := range actual { delete(actual, k) } @@ -182,7 +182,7 @@ func TestStrutil_ParseKeyValues(t *testing.T) { if err == nil { t.Fatalf("expected an error") } - for k, _ := range actual { + for k := range actual { delete(actual, k) } @@ -211,7 +211,7 @@ func TestStrutil_ParseArbitraryKeyValues(t *testing.T) { if !reflect.DeepEqual(expected, actual) { t.Fatalf("bad: expected: %#v\nactual: %#v", expected, actual) } - for k, _ := range actual { + for k := range actual { delete(actual, k) } @@ -224,7 +224,7 @@ func TestStrutil_ParseArbitraryKeyValues(t *testing.T) { if !reflect.DeepEqual(expected, actual) { t.Fatalf("bad: expected: %#v\nactual: %#v", expected, actual) } - for k, _ := range actual { + for k := range actual { delete(actual, k) } @@ -237,7 +237,7 @@ func TestStrutil_ParseArbitraryKeyValues(t *testing.T) { if !reflect.DeepEqual(expected, actual) { t.Fatalf("bad: expected: %#v\nactual: %#v", expected, actual) } - for k, _ := range actual { + for k := range actual { delete(actual, k) } @@ -250,7 +250,7 @@ func TestStrutil_ParseArbitraryKeyValues(t *testing.T) { if !reflect.DeepEqual(expected, actual) { t.Fatalf("bad: expected: %#v\nactual: %#v", expected, actual) } - for k, _ := range actual { + for k := range actual { delete(actual, k) } } @@ -326,23 +326,23 @@ func TestGlobbedStringsMatch(t *testing.T) { } tCases := []tCase{ - tCase{"", "", true}, - tCase{"*", "*", true}, - tCase{"**", "**", true}, - tCase{"*t", "t", true}, - tCase{"*t", "test", true}, - tCase{"t*", "test", true}, - tCase{"*test", "test", true}, - tCase{"*test", "a test", true}, - tCase{"test", "a test", false}, - tCase{"*test", "tests", false}, - tCase{"test*", "test", true}, - tCase{"test*", "testsss", true}, - tCase{"test**", "testsss", false}, - tCase{"test**", "test*", true}, - tCase{"**test", "*test", true}, - tCase{"TEST", "test", false}, - tCase{"test", "test", true}, + {"", "", true}, + {"*", "*", true}, + {"**", "**", true}, + {"*t", "t", true}, + {"*t", "test", true}, + {"t*", "test", true}, + {"*test", "test", true}, + {"*test", "a test", true}, + {"test", "a test", false}, + {"*test", "tests", false}, + {"test*", "test", true}, + {"test*", "testsss", true}, + {"test**", "testsss", false}, + {"test**", "test*", true}, + {"**test", "*test", true}, + {"TEST", "test", false}, + {"test", "test", true}, } for _, tc := range tCases { @@ -424,11 +424,11 @@ func TestStrUtil_RemoveDuplicates(t *testing.T) { } tCases := []tCase{ - tCase{[]string{}, []string{}, false}, - tCase{[]string{}, []string{}, true}, - tCase{[]string{"a", "b", "a"}, []string{"a", "b"}, false}, - tCase{[]string{"A", "b", "a"}, []string{"A", "a", "b"}, false}, - tCase{[]string{"A", "b", "a"}, []string{"a", "b"}, true}, + {[]string{}, []string{}, false}, + {[]string{}, []string{}, true}, + {[]string{"a", "b", "a"}, []string{"a", "b"}, false}, + {[]string{"A", "b", "a"}, []string{"A", "a", "b"}, false}, + {[]string{"A", "b", "a"}, []string{"a", "b"}, true}, } for _, tc := range tCases { @@ -448,14 +448,14 @@ func TestStrUtil_RemoveDuplicatesStable(t *testing.T) { } tCases := []tCase{ - tCase{[]string{}, []string{}, false}, - tCase{[]string{}, []string{}, true}, - tCase{[]string{"a", "b", "a"}, []string{"a", "b"}, false}, - tCase{[]string{"A", "b", "a"}, []string{"A", "b", "a"}, false}, - tCase{[]string{"A", "b", "a"}, []string{"A", "b"}, true}, - tCase{[]string{" ", "d", "c", "d"}, []string{"d", "c"}, false}, - tCase{[]string{"Z ", " z", " z ", "y"}, []string{"Z ", "y"}, true}, - tCase{[]string{"Z ", " z", " z ", "y"}, []string{"Z ", " z", "y"}, false}, + {[]string{}, []string{}, false}, + {[]string{}, []string{}, true}, + {[]string{"a", "b", "a"}, []string{"a", "b"}, false}, + {[]string{"A", "b", "a"}, []string{"A", "b", "a"}, false}, + {[]string{"A", "b", "a"}, []string{"A", "b"}, true}, + {[]string{" ", "d", "c", "d"}, []string{"d", "c"}, false}, + {[]string{"Z ", " z", " z ", "y"}, []string{"Z ", "y"}, true}, + {[]string{"Z ", " z", " z ", "y"}, []string{"Z ", " z", "y"}, false}, } for _, tc := range tCases { @@ -475,13 +475,13 @@ func TestStrUtil_ParseStringSlice(t *testing.T) { } tCases := []tCase{ - tCase{"", "", []string{}}, - tCase{" ", ",", []string{}}, - tCase{", ", ",", []string{"", ""}}, - tCase{"a", ",", []string{"a"}}, - tCase{" a, b, c ", ",", []string{"a", "b", "c"}}, - tCase{" a; b; c ", ";", []string{"a", "b", "c"}}, - tCase{" a :: b :: c ", "::", []string{"a", "b", "c"}}, + {"", "", []string{}}, + {" ", ",", []string{}}, + {", ", ",", []string{"", ""}}, + {"a", ",", []string{"a"}}, + {" a, b, c ", ",", []string{"a", "b", "c"}}, + {" a; b; c ", ";", []string{"a", "b", "c"}}, + {" a :: b :: c ", "::", []string{"a", "b", "c"}}, } for _, tc := range tCases {