diff --git a/internal/auth/password/account.go b/internal/auth/password/account.go index 6e296deb5..a934df142 100644 --- a/internal/auth/password/account.go +++ b/internal/auth/password/account.go @@ -25,24 +25,20 @@ func allocAccount() Account { } } -// NewAccount creates a new in memory Account with loginName assigned to -// authMethodId. Name and description are the only valid options. All other -// options are ignored. -func NewAccount(authMethodId string, loginName string, opt ...Option) (*Account, error) { +// NewAccount creates a new in memory Account. LoginName, name, and +// description are the only valid options. All other options are ignored. +func NewAccount(authMethodId string, opt ...Option) (*Account, error) { // NOTE(mgaffney): The scopeId in the embedded *store.Account is // populated by a trigger in the database. if authMethodId == "" { return nil, fmt.Errorf("new: password account: no auth method id: %w", db.ErrInvalidParameter) } - if loginName == "" { - return nil, fmt.Errorf("new: password account: no user name: %w", db.ErrInvalidParameter) - } opts := getOpts(opt...) a := &Account{ Account: &store.Account{ AuthMethodId: authMethodId, - LoginName: loginName, + LoginName: opts.withLoginName, Name: opts.withName, Description: opts.withDescription, }, diff --git a/internal/auth/password/account_test.go b/internal/auth/password/account_test.go index 1b2743bb3..97543231c 100644 --- a/internal/auth/password/account_test.go +++ b/internal/auth/password/account_test.go @@ -1,7 +1,6 @@ package password import ( - "context" "testing" "github.com/hashicorp/boundary/internal/auth/password/store" @@ -14,7 +13,6 @@ import ( func TestAccount_New(t *testing.T) { conn, _ := db.TestSetup(t, "postgres") - w := db.New(conn) o, _ := iam.TestScopes(t, conn) auts := TestAuthMethods(t, conn, o.GetPublicId(), 1) aut := auts[0] @@ -43,12 +41,25 @@ func TestAccount_New(t *testing.T) { name: "valid-no-options", args: args{ authMethodId: aut.GetPublicId(), - loginName: "kazmierczak", }, want: &Account{ Account: &store.Account{ AuthMethodId: aut.GetPublicId(), - LoginName: "kazmierczak", + }, + }, + }, + { + name: "valid-with-user-name", + args: args{ + authMethodId: aut.GetPublicId(), + opts: []Option{ + WithLoginName("kazmierczak1"), + }, + }, + want: &Account{ + Account: &store.Account{ + AuthMethodId: aut.GetPublicId(), + LoginName: "kazmierczak1", }, }, }, @@ -56,7 +67,6 @@ func TestAccount_New(t *testing.T) { name: "valid-with-name", args: args{ authMethodId: aut.GetPublicId(), - loginName: "kazmierczak1", opts: []Option{ WithName("test-name"), }, @@ -64,7 +74,6 @@ func TestAccount_New(t *testing.T) { want: &Account{ Account: &store.Account{ AuthMethodId: aut.GetPublicId(), - LoginName: "kazmierczak1", Name: "test-name", }, }, @@ -73,7 +82,6 @@ func TestAccount_New(t *testing.T) { name: "valid-with-description", args: args{ authMethodId: aut.GetPublicId(), - loginName: "kazmierczak2", opts: []Option{ WithDescription("test-description"), }, @@ -81,7 +89,6 @@ func TestAccount_New(t *testing.T) { want: &Account{ Account: &store.Account{ AuthMethodId: aut.GetPublicId(), - LoginName: "kazmierczak2", Description: "test-description", }, }, @@ -92,7 +99,7 @@ func TestAccount_New(t *testing.T) { tt := tt t.Run(tt.name, func(t *testing.T) { assert, require := assert.New(t), require.New(t) - got, err := NewAccount(tt.args.authMethodId, tt.args.loginName, tt.args.opts...) + got, err := NewAccount(tt.args.authMethodId, tt.args.opts...) if tt.wantErr { assert.Error(err) require.Nil(got) @@ -103,15 +110,6 @@ func TestAccount_New(t *testing.T) { assert.Emptyf(got.PublicId, "PublicId set") assert.Equal(tt.want, got) - - id, err := newAccountId() - assert.NoError(err) - - tt.want.PublicId = id - got.PublicId = id - - err2 := w.Create(context.Background(), got) - assert.NoError(err2) }) } } diff --git a/internal/auth/password/options.go b/internal/auth/password/options.go index 375936ede..eefd90d1a 100644 --- a/internal/auth/password/options.go +++ b/internal/auth/password/options.go @@ -16,6 +16,7 @@ type Option func(*options) type options struct { withName string withDescription string + withLoginName string withLimit int withConfig Configuration withPublicId string @@ -50,6 +51,13 @@ func WithName(name string) Option { } } +// WithLoginName provides an optional login name. +func WithLoginName(loginName string) Option { + return func(o *options) { + o.withLoginName = loginName + } +} + // WithLimit provides an option to provide a limit. Intentionally allowing // negative integers. If WithLimit < 0, then unlimited results are returned. // If WithLimit == 0, then default limits are used for results. diff --git a/internal/auth/password/options_test.go b/internal/auth/password/options_test.go index 01aca8896..649eaf3c7 100644 --- a/internal/auth/password/options_test.go +++ b/internal/auth/password/options_test.go @@ -15,6 +15,12 @@ func Test_GetOpts(t *testing.T) { testOpts.withPublicId = "test id" assert.Equal(t, opts, testOpts) }) + t.Run("WithLoginName", func(t *testing.T) { + opts := getOpts(WithLoginName("test")) + testOpts := getDefaultOptions() + testOpts.withLoginName = "test" + assert.Equal(t, opts, testOpts) + }) t.Run("WithName", func(t *testing.T) { opts := getOpts(WithName("test")) testOpts := getDefaultOptions() diff --git a/internal/auth/password/repository_account.go b/internal/auth/password/repository_account.go index 9b91bb17d..7c29ca5b9 100644 --- a/internal/auth/password/repository_account.go +++ b/internal/auth/password/repository_account.go @@ -93,8 +93,8 @@ func (r *Repository) CreateAccount(ctx context.Context, a *Account, opt ...Optio if err != nil { if db.IsUniqueError(err) { - return nil, fmt.Errorf("create: password account: in auth method: %s: name %s already exists: %w", - a.AuthMethodId, a.Name, db.ErrNotUnique) + return nil, fmt.Errorf("create: password account: in auth method: %s: name %q or loginName %q already exists: %w", + a.AuthMethodId, a.Name, a.LoginName, db.ErrNotUnique) } return nil, fmt.Errorf("create: password account: in auth method: %s: %w", a.AuthMethodId, err) } diff --git a/internal/auth/password/testing.go b/internal/auth/password/testing.go index 900487a54..7f533acdd 100644 --- a/internal/auth/password/testing.go +++ b/internal/auth/password/testing.go @@ -58,7 +58,7 @@ func TestAccounts(t *testing.T, conn *gorm.DB, authMethodId string, count int) [ w := db.New(conn) var auts []*Account for i := 0; i < count; i++ { - cat, err := NewAccount(authMethodId, fmt.Sprintf("name%d", i)) + cat, err := NewAccount(authMethodId, WithLoginName(fmt.Sprintf("name%d", i))) assert.NoError(err) require.NotNil(cat) id, err := newAccountId() diff --git a/internal/cmd/base/servers.go b/internal/cmd/base/servers.go index 48687d720..a0ba9c857 100644 --- a/internal/cmd/base/servers.go +++ b/internal/cmd/base/servers.go @@ -535,7 +535,7 @@ func (b *Server) CreateDevDatabase(dialect string) error { } } - acct, err := password.NewAccount(amId, acctLoginName) + acct, err := password.NewAccount(amId, password.WithLoginName(acctLoginName)) if err != nil { return fmt.Errorf("error creating new in memory auth account: %w", err) } diff --git a/internal/servers/controller/handlers/accounts/account_service.go b/internal/servers/controller/handlers/accounts/account_service.go index 1623493fa..9248e6904 100644 --- a/internal/servers/controller/handlers/accounts/account_service.go +++ b/internal/servers/controller/handlers/accounts/account_service.go @@ -103,7 +103,19 @@ func (s Service) CreateAccount(ctx context.Context, req *pbs.CreateAccountReques // UpdateAccount implements the interface pbs.AccountServiceServer. func (s Service) UpdateAccount(ctx context.Context, req *pbs.UpdateAccountRequest) (*pbs.UpdateAccountResponse, error) { - panic("UpdateAccount is not implemented.") + authResults := auth.Verify(ctx) + if !authResults.Valid { + return nil, handlers.ForbiddenError() + } + if err := validateUpdateRequest(req); err != nil { + return nil, err + } + u, err := s.updateInRepo(ctx, req.GetAuthMethodId(), req.GetId(), req.GetVersion(), req.GetUpdateMask().GetPaths(), req.GetItem()) + if err != nil { + return nil, err + } + u.Scope = authResults.Scope + return &pbs.UpdateAccountResponse{Item: u}, nil } // DeleteAccount implements the interface pbs.AccountServiceServer. @@ -145,14 +157,17 @@ func (s Service) createInRepo(ctx context.Context, authMethodId string, item *pb if err := handlers.StructToProto(item.GetAttributes(), pwAttrs); err != nil { return nil, status.Errorf(codes.InvalidArgument, "Provided attributes don't match expected format.") } - var opts []password.Option + opts := []password.Option{password.WithLoginName(pwAttrs.GetLoginName())} + if pwAttrs.GetPassword() != nil { + opts = append(opts, password.WithPassword(pwAttrs.GetPassword().GetValue())) + } if item.GetName() != nil { opts = append(opts, password.WithName(item.GetName().GetValue())) } if item.GetDescription() != nil { opts = append(opts, password.WithDescription(item.GetDescription().GetValue())) } - a, err := password.NewAccount(authMethodId, pwAttrs.GetLoginName(), opts...) + a, err := password.NewAccount(authMethodId, opts...) if err != nil { return nil, status.Errorf(codes.Internal, "Unable to build user for creation: %v.", err) } @@ -170,6 +185,37 @@ func (s Service) createInRepo(ctx context.Context, authMethodId string, item *pb return toProto(out) } +func (s Service) updateInRepo(ctx context.Context, authMethodId, id string, version uint32, mask []string, item *pb.Account) (*pb.Account, error) { + var opts []password.Option + if desc := item.GetDescription(); desc != nil { + opts = append(opts, password.WithDescription(desc.GetValue())) + } + if name := item.GetName(); name != nil { + opts = append(opts, password.WithName(name.GetValue())) + } + u, err := password.NewAccount(authMethodId, opts...) + if err != nil { + return nil, status.Errorf(codes.Internal, "Unable to build auth method for update: %v.", err) + } + u.PublicId = id + dbMask := maskManager.Translate(mask) + if len(dbMask) == 0 { + return nil, handlers.InvalidArgumentErrorf("No valid fields included in the update mask.", map[string]string{"update_mask": "No valid paths provided in the update mask."}) + } + repo, err := s.repoFn() + if err != nil { + return nil, err + } + out, rowsUpdated, err := repo.UpdateAccount(ctx, u, version, dbMask) + if err != nil { + return nil, status.Errorf(codes.Internal, "Unable to update auth method: %v.", err) + } + if rowsUpdated == 0 { + return nil, handlers.NotFoundErrorf("AuthMethod %q doesn't exist.", id) + } + return toProto(out) +} + func (s Service) deleteFromRepo(ctx context.Context, id string) (bool, error) { repo, err := s.repoFn() if err != nil { @@ -308,6 +354,49 @@ func validateListRequest(req *pbs.ListAccountsRequest) error { return nil } +func validateUpdateRequest(req *pbs.UpdateAccountRequest) error { + badFields := map[string]string{} + if !validId(req.GetId(), password.AccountPrefix+"_") { + badFields["id"] = "Improperly formatted identifier." + } + if !validId(req.GetAuthMethodId(), password.AuthMethodPrefix+"_") { + badFields["auth_method_id"] = "Improperly formatted identifier." + } + if req.GetUpdateMask() == nil { + badFields["update_mask"] = "UpdateMask not provided but is required to update this resource." + } + if req.GetVersion() == 0 { + badFields["version"] = "Existing resource version is required for an update." + } + + item := req.GetItem() + if item == nil { + // It is legitimate for no item to be specified in an update request as it indicates all fields provided in + // the mask will be marked as unset. + return nil + } + if item.GetId() != "" { + badFields["id"] = "This is a read only field and cannot be specified in an update request." + } + if item.GetAuthMethodId() != "" { + badFields["auth_method_id"] = "This is a read only field and cannot be specified in an update request." + } + if item.GetCreatedTime() != nil { + badFields["created_time"] = "This is a read only field and cannot be specified in an update request." + } + if item.GetUpdatedTime() != nil { + badFields["updated_time"] = "This is a read only field and cannot be specified in an update request." + } + if item.GetType() != "" { + badFields["type"] = "This is a read only field and cannot be specified in an update request." + } + if len(badFields) > 0 { + return handlers.InvalidArgumentErrorf("Errors in provided fields.", badFields) + } + + return nil +} + func validId(id, prefix string) bool { if !strings.HasPrefix(id, prefix) { return false diff --git a/internal/servers/controller/handlers/accounts/account_service_test.go b/internal/servers/controller/handlers/accounts/account_service_test.go index f182872a2..deaf01550 100644 --- a/internal/servers/controller/handlers/accounts/account_service_test.go +++ b/internal/servers/controller/handlers/accounts/account_service_test.go @@ -12,7 +12,7 @@ import ( "github.com/hashicorp/boundary/internal/authtoken" "github.com/hashicorp/boundary/internal/db" pb "github.com/hashicorp/boundary/internal/gen/controller/api/resources/authmethods" - "github.com/hashicorp/boundary/internal/gen/controller/api/resources/scopes" + scopepb "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" @@ -20,8 +20,10 @@ import ( "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" "google.golang.org/protobuf/types/known/wrapperspb" @@ -47,7 +49,7 @@ func TestGet(t *testing.T) { AuthMethodId: aa.GetAuthMethodId(), CreatedTime: aa.GetCreateTime().GetTimestamp(), UpdatedTime: aa.GetUpdateTime().GetTimestamp(), - Scope: &scopes.ScopeInfo{Id: org.GetPublicId(), Type: scope.Org.String()}, + Scope: &scopepb.ScopeInfo{Id: org.GetPublicId(), Type: scope.Org.String()}, Version: 1, Type: "password", Attributes: &structpb.Struct{Fields: map[string]*structpb.Value{"login_name": structpb.NewStringValue(aa.GetLoginName())}}, @@ -113,7 +115,7 @@ func TestList(t *testing.T) { AuthMethodId: aa.GetAuthMethodId(), CreatedTime: aa.GetCreateTime().GetTimestamp(), UpdatedTime: aa.GetUpdateTime().GetTimestamp(), - Scope: &scopes.ScopeInfo{Id: o.GetPublicId(), Type: scope.Org.String()}, + Scope: &scopepb.ScopeInfo{Id: o.GetPublicId(), Type: scope.Org.String()}, Version: 1, Type: "password", Attributes: &structpb.Struct{Fields: map[string]*structpb.Value{"login_name": structpb.NewStringValue(aa.GetLoginName())}}, @@ -127,7 +129,7 @@ func TestList(t *testing.T) { AuthMethodId: aa.GetAuthMethodId(), CreatedTime: aa.GetCreateTime().GetTimestamp(), UpdatedTime: aa.GetUpdateTime().GetTimestamp(), - Scope: &scopes.ScopeInfo{Id: o.GetPublicId(), Type: scope.Org.String()}, + Scope: &scopepb.ScopeInfo{Id: o.GetPublicId(), Type: scope.Org.String()}, Version: 1, Type: "password", Attributes: &structpb.Struct{Fields: map[string]*structpb.Value{"login_name": structpb.NewStringValue(aa.GetLoginName())}}, @@ -302,7 +304,9 @@ func TestCreate(t *testing.T) { defaultCreated, err := ptypes.Timestamp(defaultAccount.GetCreateTime().GetTimestamp()) require.NoError(t, err, "Error converting proto to timestamp.") - defaultSt, err := handlers.ProtoToStruct(&pb.PasswordAccountAttributes{LoginName: "thetestloginname"}) + structNoPw, err := handlers.ProtoToStruct(&pb.PasswordAccountAttributes{LoginName: "thetestloginname"}) + require.NoError(t, err, "Error converting proto to struct.") + structWithPw, err := handlers.ProtoToStruct(&pb.PasswordAccountAttributes{LoginName: "adifferentusername", Password: wrapperspb.String("somerandompassword")}) require.NoError(t, err, "Error converting proto to struct.") cases := []struct { @@ -319,7 +323,7 @@ func TestCreate(t *testing.T) { Name: &wrapperspb.StringValue{Value: "name"}, Description: &wrapperspb.StringValue{Value: "desc"}, Type: "password", - Attributes: defaultSt, + Attributes: structNoPw, }, }, res: &pbs.CreateAccountResponse{ @@ -328,10 +332,39 @@ func TestCreate(t *testing.T) { AuthMethodId: defaultAccount.GetAuthMethodId(), Name: &wrapperspb.StringValue{Value: "name"}, Description: &wrapperspb.StringValue{Value: "desc"}, - Scope: &scopes.ScopeInfo{Id: o.GetPublicId(), Type: scope.Org.String()}, + Scope: &scopepb.ScopeInfo{Id: o.GetPublicId(), Type: scope.Org.String()}, Version: 1, Type: "password", - Attributes: defaultSt, + Attributes: structNoPw, + }, + }, + errCode: codes.OK, + }, + { + name: "Create a valid Account with password defined", + req: &pbs.CreateAccountRequest{ + AuthMethodId: defaultAccount.GetAuthMethodId(), + Item: &pb.Account{ + Name: &wrapperspb.StringValue{Value: "name_with_password"}, + Description: &wrapperspb.StringValue{Value: "desc"}, + Type: "password", + Attributes: structWithPw, + }, + }, + res: &pbs.CreateAccountResponse{ + Uri: fmt.Sprintf("scopes/%s/auth-methods/%s/accounts/%s_", o.GetPublicId(), defaultAccount.GetAuthMethodId(), password.AccountPrefix), + Item: &pb.Account{ + AuthMethodId: defaultAccount.GetAuthMethodId(), + Name: &wrapperspb.StringValue{Value: "name_with_password"}, + Description: &wrapperspb.StringValue{Value: "desc"}, + Scope: &scopepb.ScopeInfo{Id: o.GetPublicId(), Type: scope.Org.String()}, + Version: 1, + Type: "password", + Attributes: func() *structpb.Struct { + newSt := proto.Clone(structWithPw).(*structpb.Struct) + delete(newSt.Fields, "password") + return newSt + }(), }, }, errCode: codes.OK, @@ -343,7 +376,7 @@ func TestCreate(t *testing.T) { Item: &pb.Account{ Id: password.AccountPrefix + "_notallowed", Type: "password", - Attributes: defaultSt, + Attributes: structNoPw, }, }, res: nil, @@ -356,7 +389,7 @@ func TestCreate(t *testing.T) { Item: &pb.Account{ AuthMethodId: defaultAccount.GetAuthMethodId(), Type: "password", - Attributes: defaultSt, + Attributes: structNoPw, }, }, res: nil, @@ -369,7 +402,7 @@ func TestCreate(t *testing.T) { Item: &pb.Account{ CreatedTime: ptypes.TimestampNow(), Type: "password", - Attributes: defaultSt, + Attributes: structNoPw, }, }, res: nil, @@ -382,7 +415,7 @@ func TestCreate(t *testing.T) { Item: &pb.Account{ UpdatedTime: ptypes.TimestampNow(), Type: "password", - Attributes: defaultSt, + Attributes: structNoPw, }, }, res: nil, @@ -393,7 +426,7 @@ func TestCreate(t *testing.T) { req: &pbs.CreateAccountRequest{ AuthMethodId: defaultAccount.GetAuthMethodId(), Item: &pb.Account{ - Attributes: defaultSt, + Attributes: structNoPw, }, }, res: nil, @@ -436,3 +469,324 @@ func TestCreate(t *testing.T) { }) } } + +func TestUpdate(t *testing.T) { + conn, _ := db.TestSetup(t, "postgres") + rw := db.New(conn) + wrap := db.TestWrapper(t) + repoFn := func() (*password.Repository, error) { + return password.NewRepository(rw, rw, wrap) + } + + o, _ := iam.TestScopes(t, conn) + am := password.TestAuthMethods(t, conn, o.GetPublicId(), 1)[0] + tested, err := accounts.NewService(repoFn) + require.NoError(t, err, "Error when getting new auth_method service.") + + defaultScopeInfo := &scopepb.ScopeInfo{Id: o.GetPublicId(), Type: o.GetType()} + defaultAttributes := &structpb.Struct{Fields: map[string]*structpb.Value{ + "login_name": structpb.NewStringValue("default"), + }} + + freshAccount := func(t *testing.T) (*pb.Account, func()) { + t.Helper() + acc, err := tested.CreateAccount(auth.DisabledAuthTestContext(auth.WithScopeId(o.GetPublicId())), + &pbs.CreateAccountRequest{ + AuthMethodId: am.GetPublicId(), + Item: &pb.Account{ + Name: wrapperspb.String("default"), + Description: wrapperspb.String("default"), + Type: "password", + Attributes: defaultAttributes, + }}, + ) + require.NoError(t, err) + + clean := func() { + _, err := tested.DeleteAccount(auth.DisabledAuthTestContext(auth.WithScopeId(o.GetPublicId())), + &pbs.DeleteAccountRequest{AuthMethodId: am.GetPublicId(), Id: acc.GetItem().GetId()}) + require.NoError(t, err) + } + + return acc.GetItem(), clean + } + + cases := []struct { + name string + req *pbs.UpdateAccountRequest + res *pbs.UpdateAccountResponse + errCode codes.Code + }{ + { + name: "Update an Existing AuthMethod", + req: &pbs.UpdateAccountRequest{ + AuthMethodId: am.GetPublicId(), + UpdateMask: &field_mask.FieldMask{ + Paths: []string{"name", "description"}, + }, + Item: &pb.Account{ + Name: &wrapperspb.StringValue{Value: "new"}, + Description: &wrapperspb.StringValue{Value: "desc"}, + }, + }, + res: &pbs.UpdateAccountResponse{ + Item: &pb.Account{ + AuthMethodId: am.GetPublicId(), + Name: &wrapperspb.StringValue{Value: "new"}, + Description: &wrapperspb.StringValue{Value: "desc"}, + Type: "password", + Attributes: defaultAttributes, + Scope: defaultScopeInfo, + }, + }, + errCode: codes.OK, + }, + { + name: "Multiple Paths in single string", + req: &pbs.UpdateAccountRequest{ + AuthMethodId: am.GetPublicId(), + UpdateMask: &field_mask.FieldMask{ + Paths: []string{"name,description"}, + }, + Item: &pb.Account{ + Name: &wrapperspb.StringValue{Value: "new"}, + Description: &wrapperspb.StringValue{Value: "desc"}, + }, + }, + res: &pbs.UpdateAccountResponse{ + Item: &pb.Account{ + AuthMethodId: am.GetPublicId(), + Name: &wrapperspb.StringValue{Value: "new"}, + Description: &wrapperspb.StringValue{Value: "desc"}, + Type: "password", + Attributes: defaultAttributes, + Scope: defaultScopeInfo, + }, + }, + errCode: codes.OK, + }, + { + name: "No Update Mask", + req: &pbs.UpdateAccountRequest{ + AuthMethodId: am.GetPublicId(), + Item: &pb.Account{ + Name: &wrapperspb.StringValue{Value: "updated name"}, + Description: &wrapperspb.StringValue{Value: "updated desc"}, + }, + }, + errCode: codes.InvalidArgument, + }, + { + name: "No Paths in Mask", + req: &pbs.UpdateAccountRequest{ + AuthMethodId: am.GetPublicId(), + UpdateMask: &field_mask.FieldMask{Paths: []string{}}, + Item: &pb.Account{ + Name: &wrapperspb.StringValue{Value: "updated name"}, + Description: &wrapperspb.StringValue{Value: "updated desc"}, + }, + }, + errCode: codes.InvalidArgument, + }, + { + name: "Only non-existant paths in Mask", + req: &pbs.UpdateAccountRequest{ + AuthMethodId: am.GetPublicId(), + UpdateMask: &field_mask.FieldMask{Paths: []string{"nonexistant_field"}}, + Item: &pb.Account{ + Name: &wrapperspb.StringValue{Value: "updated name"}, + Description: &wrapperspb.StringValue{Value: "updated desc"}, + }, + }, + errCode: codes.InvalidArgument, + }, + { + name: "Unset Name", + req: &pbs.UpdateAccountRequest{ + AuthMethodId: am.GetPublicId(), + UpdateMask: &field_mask.FieldMask{ + Paths: []string{"name"}, + }, + Item: &pb.Account{ + Description: &wrapperspb.StringValue{Value: "ignored"}, + }, + }, + res: &pbs.UpdateAccountResponse{ + Item: &pb.Account{ + AuthMethodId: am.GetPublicId(), + Description: &wrapperspb.StringValue{Value: "default"}, + Type: "password", + Attributes: defaultAttributes, + Scope: defaultScopeInfo, + }, + }, + errCode: codes.OK, + }, + { + name: "Update Only Name", + req: &pbs.UpdateAccountRequest{ + AuthMethodId: am.GetPublicId(), + UpdateMask: &field_mask.FieldMask{ + Paths: []string{"name"}, + }, + Item: &pb.Account{ + Name: &wrapperspb.StringValue{Value: "updated"}, + Description: &wrapperspb.StringValue{Value: "ignored"}, + }, + }, + res: &pbs.UpdateAccountResponse{ + Item: &pb.Account{ + AuthMethodId: am.GetPublicId(), + Name: &wrapperspb.StringValue{Value: "updated"}, + Description: &wrapperspb.StringValue{Value: "default"}, + Type: "password", + Attributes: defaultAttributes, + Scope: defaultScopeInfo, + }, + }, + errCode: codes.OK, + }, + { + name: "Update Only Description", + req: &pbs.UpdateAccountRequest{ + AuthMethodId: am.GetPublicId(), + UpdateMask: &field_mask.FieldMask{ + Paths: []string{"description"}, + }, + Item: &pb.Account{ + Name: &wrapperspb.StringValue{Value: "ignored"}, + Description: &wrapperspb.StringValue{Value: "notignored"}, + }, + }, + res: &pbs.UpdateAccountResponse{ + Item: &pb.Account{ + AuthMethodId: am.GetPublicId(), + Name: &wrapperspb.StringValue{Value: "default"}, + Description: &wrapperspb.StringValue{Value: "notignored"}, + Type: "password", + Attributes: defaultAttributes, + Scope: defaultScopeInfo, + }, + }, + errCode: codes.OK, + }, + // TODO: Updating a non existant auth_method should result in a NotFound exception but currently results in + // the repoFn returning an internal error. + { + name: "Update a Non Existing Account", + req: &pbs.UpdateAccountRequest{ + AuthMethodId: am.GetPublicId(), + Id: password.AccountPrefix + "_DoesntExis", + UpdateMask: &field_mask.FieldMask{ + Paths: []string{"description"}, + }, + Item: &pb.Account{ + Name: &wrapperspb.StringValue{Value: "new"}, + Description: &wrapperspb.StringValue{Value: "desc"}, + }, + }, + errCode: codes.Internal, + }, + { + name: "Cant change Id", + req: &pbs.UpdateAccountRequest{ + AuthMethodId: am.GetPublicId(), + UpdateMask: &field_mask.FieldMask{ + Paths: []string{"id"}, + }, + Item: &pb.Account{ + Id: password.AccountPrefix + "_somethinge", + Name: &wrapperspb.StringValue{Value: "new"}, + Description: &wrapperspb.StringValue{Value: "new desc"}, + }}, + res: nil, + errCode: codes.InvalidArgument, + }, + { + name: "Cant specify Created Time", + req: &pbs.UpdateAccountRequest{ + AuthMethodId: am.GetPublicId(), + UpdateMask: &field_mask.FieldMask{ + Paths: []string{"created_time"}, + }, + Item: &pb.Account{ + CreatedTime: ptypes.TimestampNow(), + }, + }, + res: nil, + errCode: codes.InvalidArgument, + }, + { + name: "Cant specify Updated Time", + req: &pbs.UpdateAccountRequest{ + AuthMethodId: am.GetPublicId(), + UpdateMask: &field_mask.FieldMask{ + Paths: []string{"updated_time"}, + }, + Item: &pb.Account{ + UpdatedTime: ptypes.TimestampNow(), + }, + }, + res: nil, + errCode: codes.InvalidArgument, + }, + { + name: "Cant specify Type", + req: &pbs.UpdateAccountRequest{ + AuthMethodId: am.GetPublicId(), + UpdateMask: &field_mask.FieldMask{ + Paths: []string{"type"}, + }, + Item: &pb.Account{ + Type: "oidc", + }, + }, + res: nil, + errCode: codes.InvalidArgument, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert, require := assert.New(t), require.New(t) + acc, cleanup := freshAccount(t) + defer cleanup() + + tc.req.Version = 1 + + if tc.req.GetId() == "" { + tc.req.Id = acc.GetId() + } + + if tc.res != nil && tc.res.Item != nil { + tc.res.Item.Id = acc.GetId() + tc.res.Item.CreatedTime = acc.GetCreatedTime() + } + + 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.res == nil { + require.Nil(got) + } + + if got != nil { + assert.NotNilf(tc.res, "Expected UpdateAccount response to be nil, but was %v", got) + gotUpdateTime, err := ptypes.Timestamp(got.GetItem().GetUpdatedTime()) + require.NoError(err, "Error converting proto to timestamp") + + created, err := ptypes.Timestamp(acc.GetCreatedTime()) + require.NoError(err, "Error converting proto to timestamp") + + // Verify it is a auth_method updated after it was created + assert.True(gotUpdateTime.After(created), "Updated account should have been updated after it's creation. Was updated %v, which is after %v", gotUpdateTime, created) + + // Clear all values which are hard to compare against. + got.Item.UpdatedTime, tc.res.Item.UpdatedTime = nil, nil + + assert.EqualValues(2, got.Item.Version) + tc.res.Item.Version = 2 + } + assert.Empty(cmp.Diff(got, tc.res, protocmp.Transform()), "UpdateAccount(%q) got response %q, wanted %q", tc.req, got, tc.res) + }) + } +} diff --git a/internal/servers/controller/handlers/authenticate/authenticate_service_test.go b/internal/servers/controller/handlers/authenticate/authenticate_service_test.go index 3e254f3ff..290da7a79 100644 --- a/internal/servers/controller/handlers/authenticate/authenticate_service_test.go +++ b/internal/servers/controller/handlers/authenticate/authenticate_service_test.go @@ -35,7 +35,7 @@ func TestAuthenticate(t *testing.T) { passwordRepoFn := func() (*password.Repository, error) { return password.NewRepository(rw, rw, wrapper) } am := password.TestAuthMethods(t, conn, o.GetPublicId(), 1)[0] - acct, err := password.NewAccount(am.GetPublicId(), testLoginName) + acct, err := password.NewAccount(am.GetPublicId(), password.WithLoginName(testLoginName)) require.NoError(t, err) pwRepo, err := passwordRepoFn() @@ -178,7 +178,7 @@ func TestAuthenticate_AuthAccountConnectedToIamUser(t *testing.T) { iamRepoFn := func() (*iam.Repository, error) { return iam.NewRepository(rw, rw, wrapper) } am := password.TestAuthMethods(t, conn, o.GetPublicId(), 1)[0] - acct, err := password.NewAccount(am.GetPublicId(), testLoginName) + acct, err := password.NewAccount(am.GetPublicId(), password.WithLoginName(testLoginName)) require.NoError(err) pwRepo, err := passwordRepoFn() diff --git a/internal/servers/controller/handlers/authmethods/authmethod_service.go b/internal/servers/controller/handlers/authmethods/authmethod_service.go index d0f2e220a..6d1fc6d34 100644 --- a/internal/servers/controller/handlers/authmethods/authmethod_service.go +++ b/internal/servers/controller/handlers/authmethods/authmethod_service.go @@ -112,6 +112,7 @@ func (s Service) UpdateAuthMethod(ctx context.Context, req *pbs.UpdateAuthMethod if err != nil { return nil, err } + u.Scope = authResults.Scope return &pbs.UpdateAuthMethodResponse{Item: u}, nil } diff --git a/internal/servers/controller/handlers/authmethods/authmethod_service_test.go b/internal/servers/controller/handlers/authmethods/authmethod_service_test.go index 8d7f41c92..ee43d7ba5 100644 --- a/internal/servers/controller/handlers/authmethods/authmethod_service_test.go +++ b/internal/servers/controller/handlers/authmethods/authmethod_service_test.go @@ -11,7 +11,7 @@ import ( "github.com/hashicorp/boundary/internal/auth/password" "github.com/hashicorp/boundary/internal/db" pb "github.com/hashicorp/boundary/internal/gen/controller/api/resources/authmethods" - "github.com/hashicorp/boundary/internal/gen/controller/api/resources/scopes" + scopepb "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/authmethods" @@ -48,7 +48,7 @@ func TestGet(t *testing.T) { "min_login_name_length": structpb.NewNumberValue(3), }}, Version: 1, - Scope: &scopes.ScopeInfo{ + Scope: &scopepb.ScopeInfo{ Id: o.GetPublicId(), Type: o.GetType(), }, @@ -122,7 +122,7 @@ func TestList(t *testing.T) { Id: am.GetPublicId(), CreatedTime: am.GetCreateTime().GetTimestamp(), UpdatedTime: am.GetUpdateTime().GetTimestamp(), - Scope: &scopes.ScopeInfo{Id: oWithAuthMethods.GetPublicId(), Type: scope.Org.String()}, + Scope: &scopepb.ScopeInfo{Id: oWithAuthMethods.GetPublicId(), Type: scope.Org.String()}, Version: 1, Type: "password", Attributes: &structpb.Struct{Fields: map[string]*structpb.Value{ @@ -138,7 +138,7 @@ func TestList(t *testing.T) { Id: aa.GetPublicId(), CreatedTime: aa.GetCreateTime().GetTimestamp(), UpdatedTime: aa.GetUpdateTime().GetTimestamp(), - Scope: &scopes.ScopeInfo{Id: oWithOtherAuthMethods.GetPublicId(), Type: scope.Org.String()}, + Scope: &scopepb.ScopeInfo{Id: oWithOtherAuthMethods.GetPublicId(), Type: scope.Org.String()}, Version: 1, Type: "password", Attributes: &structpb.Struct{Fields: map[string]*structpb.Value{ @@ -312,7 +312,7 @@ func TestCreate(t *testing.T) { UpdatedTime: defaultAm.GetUpdateTime().GetTimestamp(), Name: &wrapperspb.StringValue{Value: "name"}, Description: &wrapperspb.StringValue{Value: "desc"}, - Scope: &scopes.ScopeInfo{Id: o.GetPublicId(), Type: scope.Org.String()}, + Scope: &scopepb.ScopeInfo{Id: o.GetPublicId(), Type: o.GetType()}, Version: 1, Type: "password", Attributes: &structpb.Struct{Fields: map[string]*structpb.Value{ @@ -427,6 +427,8 @@ func TestUpdate(t *testing.T) { tested, err := authmethods.NewService(repoFn) require.NoError(t, err, "Error when getting new auth_method service.") + defaultScopeInfo := &scopepb.ScopeInfo{Id: o.GetPublicId(), Type: o.GetType()} + freshAuthMethod := func() (*pb.AuthMethod, func()) { am, err := tested.CreateAuthMethod(auth.DisabledAuthTestContext(auth.WithScopeId(o.GetPublicId())), &pbs.CreateAuthMethodRequest{Item: &pb.AuthMethod{ @@ -471,6 +473,7 @@ func TestUpdate(t *testing.T) { "min_password_length": structpb.NewNumberValue(8), "min_login_name_length": structpb.NewNumberValue(3), }}, + Scope: defaultScopeInfo, }, }, errCode: codes.OK, @@ -495,6 +498,7 @@ func TestUpdate(t *testing.T) { "min_password_length": structpb.NewNumberValue(8), "min_login_name_length": structpb.NewNumberValue(3), }}, + Scope: defaultScopeInfo, }, }, errCode: codes.OK, @@ -549,6 +553,7 @@ func TestUpdate(t *testing.T) { "min_password_length": structpb.NewNumberValue(8), "min_login_name_length": structpb.NewNumberValue(3), }}, + Scope: defaultScopeInfo, }, }, errCode: codes.OK, @@ -573,6 +578,7 @@ func TestUpdate(t *testing.T) { "min_password_length": structpb.NewNumberValue(8), "min_login_name_length": structpb.NewNumberValue(3), }}, + Scope: defaultScopeInfo, }, }, errCode: codes.OK, @@ -597,6 +603,7 @@ func TestUpdate(t *testing.T) { "min_password_length": structpb.NewNumberValue(8), "min_login_name_length": structpb.NewNumberValue(3), }}, + Scope: defaultScopeInfo, }, }, errCode: codes.OK,