From 1bebe7a26164fda97954a5bbe0dce0eab3fa517a Mon Sep 17 00:00:00 2001 From: Jim Date: Wed, 15 Jul 2020 13:12:15 -0400 Subject: [PATCH] Convert option db.WithVersion to *uint32 (#195) --- internal/db/option.go | 8 +- internal/db/option_test.go | 7 +- internal/db/read_writer.go | 8 +- internal/db/read_writer_test.go | 30 +++- internal/iam/repository_group.go | 26 ++-- internal/iam/repository_group_test.go | 67 ++++++++- internal/iam/repository_principal_role.go | 24 +++- .../iam/repository_principal_role_test.go | 119 ++++++++++++--- internal/iam/repository_role_grant.go | 23 ++- internal/iam/repository_role_grant_test.go | 135 +++++++++++++++++- 10 files changed, 390 insertions(+), 57 deletions(-) diff --git a/internal/db/option.go b/internal/db/option.go index aed0cd4c7..896b0c996 100644 --- a/internal/db/option.go +++ b/internal/db/option.go @@ -32,8 +32,8 @@ type Options struct { newOplogMsg *oplog.Message newOplogMsgs *[]*oplog.Message - // WithVersion must be accessible from other packages - WithVersion uint32 + // WithVersion must be accessible from other packages. + WithVersion *uint32 withSkipVetForWrite bool } @@ -54,7 +54,7 @@ func getDefaultOptions() Options { WithFieldMaskPaths: []string{}, WithNullPaths: []string{}, WithLimit: 0, - WithVersion: 0, + WithVersion: nil, } } @@ -121,7 +121,7 @@ func WithLimit(limit int) Option { } // WithVersion provides an option version number for update operations. -func WithVersion(version uint32) Option { +func WithVersion(version *uint32) Option { return func(o *Options) { o.WithVersion = version } diff --git a/internal/db/option_test.go b/internal/db/option_test.go index 181a343dd..b7c9f3e08 100644 --- a/internal/db/option_test.go +++ b/internal/db/option_test.go @@ -125,11 +125,12 @@ func Test_getOpts(t *testing.T) { // test default of 0 opts := GetOpts() testOpts := getDefaultOptions() - testOpts.WithVersion = 0 + testOpts.WithVersion = nil assert.Equal(opts, testOpts) - opts = GetOpts(WithVersion(2)) + versionTwo := uint32(2) + opts = GetOpts(WithVersion(&versionTwo)) testOpts = getDefaultOptions() - testOpts.WithVersion = 2 + testOpts.WithVersion = &versionTwo assert.Equal(opts, testOpts) }) t.Run("WithSkipVetForWrite", func(t *testing.T) { diff --git a/internal/db/read_writer.go b/internal/db/read_writer.go index b0ffc6fdc..6e7f13699 100644 --- a/internal/db/read_writer.go +++ b/internal/db/read_writer.go @@ -357,7 +357,8 @@ func (rw *Db) CreateItems(ctx context.Context, createItems []interface{}, opt .. // used together. If WithVersion is used, then the update will include the // version number in the update where clause, which basically makes the update // use optimistic locking and the update will only succeed if the existing rows -// version matches the WithVersion option. +// version matches the WithVersion option. Zero is not a valid value for the +// WithVersion option and will return an error. func (rw *Db) Update(ctx context.Context, i interface{}, fieldMaskPaths []string, setToNullPaths []string, opt ...Option) (int, error) { if rw.underlying == nil { return NoRowsAffected, fmt.Errorf("update: missing underlying db %w", ErrNilParameter) @@ -426,7 +427,10 @@ func (rw *Db) Update(ctx context.Context, i interface{}, fieldMaskPaths []string } var underlying *gorm.DB switch { - case opts.WithVersion > 0: + case opts.WithVersion != nil: + if *opts.WithVersion == 0 { + return NoRowsAffected, fmt.Errorf("update: with version option is zero: %w", ErrInvalidParameter) + } if _, ok := scope.FieldByName("version"); !ok { return NoRowsAffected, fmt.Errorf("update: %s does not have a version field", scope.TableName()) } diff --git a/internal/db/read_writer_test.go b/internal/db/read_writer_test.go index 6c508747d..289ac0891 100644 --- a/internal/db/read_writer_test.go +++ b/internal/db/read_writer_test.go @@ -29,6 +29,11 @@ func TestDb_Update(t *testing.T) { publicId, err := NewPublicId("testuser") require.NoError(t, err) id := testId(t) + + badVersion := uint32(22) + versionOne := uint32(1) + versionZero := uint32(0) + type args struct { i *db_test.TestUser fieldMaskPaths []string @@ -78,12 +83,30 @@ func TestDb_Update(t *testing.T) { }, fieldMaskPaths: []string{"Name", "PhoneNumber"}, setToNullPaths: []string{"Email"}, - opt: []Option{WithVersion(22)}, + opt: []Option{WithVersion(&badVersion)}, }, want: 0, wantErr: false, wantErrMsg: "", }, + { + name: "simple-with-zero-version", + args: args{ + i: &db_test.TestUser{ + StoreTestUser: &db_test.StoreTestUser{ + Name: "simple-with-bad-version" + id, + Email: "updated" + id, + PhoneNumber: "updated" + id, + }, + }, + fieldMaskPaths: []string{"Name", "PhoneNumber"}, + setToNullPaths: []string{"Email"}, + opt: []Option{WithVersion(&versionZero)}, + }, + want: 0, + wantErr: true, + wantErrMsg: "update: with version option is zero: invalid parameter", + }, { name: "simple-with-version", args: args{ @@ -96,7 +119,7 @@ func TestDb_Update(t *testing.T) { }, fieldMaskPaths: []string{"Name", "PhoneNumber"}, setToNullPaths: []string{"Email"}, - opt: []Option{WithVersion(1)}, + opt: []Option{WithVersion(&versionOne)}, }, want: 1, wantErr: false, @@ -270,7 +293,8 @@ func TestDb_Update(t *testing.T) { car := testCar(t, db, "foo-"+id, id, int32(100)) car.Name = "friendly-" + id - rowsUpdated, err := w.Update(context.Background(), car, []string{"Name"}, nil, WithVersion(1)) + versionOne := uint32(1) + rowsUpdated, err := w.Update(context.Background(), car, []string{"Name"}, nil, WithVersion(&versionOne)) assert.Error(err) assert.Equal(0, rowsUpdated) }) diff --git a/internal/iam/repository_group.go b/internal/iam/repository_group.go index aa43cb0fd..f77422cd0 100644 --- a/internal/iam/repository_group.go +++ b/internal/iam/repository_group.go @@ -143,16 +143,18 @@ func (r *Repository) ListGroupMembers(ctx context.Context, withGroupId string, o // AddGroupMembers provides the ability to add members (userIds) to a group // (groupId). The group's current db version must match the groupVersion or an -// error will be returned. +// error will be returned. Zero is not a valid value for the WithVersion option +// and will return an error. func (r *Repository) AddGroupMembers(ctx context.Context, groupId string, groupVersion uint32, userIds []string, opt ...Option) ([]*GroupMember, error) { - if groupId == "" { return nil, fmt.Errorf("add group members: missing group id %w", db.ErrInvalidParameter) } if len(userIds) == 0 { return nil, fmt.Errorf("add group members: missing user ids to add %w", db.ErrInvalidParameter) } - + if groupVersion == 0 { + return nil, fmt.Errorf("add group members: version cannot be zero: %w", db.ErrInvalidParameter) + } group := allocGroup() group.PublicId = groupId scope, err := group.GetScope(ctx, r.reader) @@ -184,7 +186,7 @@ func (r *Repository) AddGroupMembers(ctx context.Context, groupId string, groupV updatedGroup.PublicId = groupId updatedGroup.Version = uint32(groupVersion) + 1 var groupOplogMsg oplog.Message - rowsUpdated, err := w.Update(ctx, &updatedGroup, []string{"Version"}, nil, db.NewOplogMsg(&groupOplogMsg), db.WithVersion(groupVersion)) + rowsUpdated, err := w.Update(ctx, &updatedGroup, []string{"Version"}, nil, db.NewOplogMsg(&groupOplogMsg), db.WithVersion(&groupVersion)) if err != nil { return fmt.Errorf("add group members: unable to update group version: %w", err) } @@ -228,7 +230,8 @@ func (r *Repository) AddGroupMembers(ctx context.Context, groupId string, groupV } // DeleteGroupMembers (userIds) from a group (groupId). The group's current db version -// must match the groupVersion or an error will be returned. +// must match the groupVersion or an error will be returned. Zero is not a valid +// value for the WithVersion option and will return an error. func (r *Repository) DeleteGroupMembers(ctx context.Context, groupId string, groupVersion uint32, userIds []string, opt ...Option) (int, error) { if groupId == "" { return db.NoRowsAffected, fmt.Errorf("delete group members: missing group id: %w", db.ErrInvalidParameter) @@ -236,6 +239,9 @@ func (r *Repository) DeleteGroupMembers(ctx context.Context, groupId string, gro if len(userIds) == 0 { return db.NoRowsAffected, fmt.Errorf("delete group members: missing either user or groups to delete %w", db.ErrInvalidParameter) } + if groupVersion == 0 { + return db.NoRowsAffected, fmt.Errorf("delete group members: version cannot be zero: %w", db.ErrInvalidParameter) + } group := allocGroup() group.PublicId = groupId scope, err := group.GetScope(ctx, r.reader) @@ -267,7 +273,7 @@ func (r *Repository) DeleteGroupMembers(ctx context.Context, groupId string, gro updatedGroup.PublicId = groupId updatedGroup.Version = uint32(groupVersion) + 1 var groupOplogMsg oplog.Message - rowsUpdated, err := w.Update(ctx, &updatedGroup, []string{"Version"}, nil, db.NewOplogMsg(&groupOplogMsg), db.WithVersion(groupVersion)) + rowsUpdated, err := w.Update(ctx, &updatedGroup, []string{"Version"}, nil, db.NewOplogMsg(&groupOplogMsg), db.WithVersion(&groupVersion)) if err != nil { return fmt.Errorf("delete group members: unable to update group version: %w", err) } @@ -304,11 +310,15 @@ func (r *Repository) DeleteGroupMembers(ctx context.Context, groupId string, gro } // SetGroupMembers will set the group's members. If userIds is empty, the -// members will be cleared. +// members will be cleared. Zero is not a valid value for the WithVersion option +// and will return an error. func (r *Repository) SetGroupMembers(ctx context.Context, groupId string, groupVersion uint32, userIds []string, opt ...Option) ([]*GroupMember, int, error) { if groupId == "" { return nil, db.NoRowsAffected, fmt.Errorf("set group members: missing role id: %w", db.ErrInvalidParameter) } + if groupVersion == 0 { + return nil, db.NoRowsAffected, fmt.Errorf("set group members: version cannot be zero: %w", db.ErrInvalidParameter) + } group := allocGroup() group.PublicId = groupId scope, err := group.GetScope(ctx, r.reader) @@ -384,7 +394,7 @@ func (r *Repository) SetGroupMembers(ctx context.Context, groupId string, groupV updatedGroup.PublicId = groupId updatedGroup.Version = uint32(groupVersion) + 1 var groupOplogMsg oplog.Message - rowsUpdated, err := w.Update(ctx, &updatedGroup, []string{"Version"}, nil, db.NewOplogMsg(&groupOplogMsg), db.WithVersion(groupVersion)) + rowsUpdated, err := w.Update(ctx, &updatedGroup, []string{"Version"}, nil, db.NewOplogMsg(&groupOplogMsg), db.WithVersion(&groupVersion)) if err != nil { return fmt.Errorf("set group members: unable to update group version: %w", err) } diff --git a/internal/iam/repository_group_test.go b/internal/iam/repository_group_test.go index 8b7144f0c..2a51bfc40 100644 --- a/internal/iam/repository_group_test.go +++ b/internal/iam/repository_group_test.go @@ -759,7 +759,7 @@ func TestRepository_AddGroupMembers(t *testing.T) { groupVersion := uint32(0) type args struct { groupId string - groupVersion uint32 + groupVersion *uint32 userIds []string opt []Option } @@ -789,7 +789,16 @@ func TestRepository_AddGroupMembers(t *testing.T) { name: "bad-version", args: args{ groupId: group.PublicId, - groupVersion: 1000, + groupVersion: func() *uint32 { v := uint32(1000); return &v }(), + userIds: createUsersFn(), + }, + wantErr: true, + }, + { + name: "zero-version", + args: args{ + groupId: group.PublicId, + groupVersion: func() *uint32 { v := uint32(0); return &v }(), userIds: createUsersFn(), }, wantErr: true, @@ -808,11 +817,12 @@ func TestRepository_AddGroupMembers(t *testing.T) { assert, require := assert.New(t), require.New(t) var version uint32 - if tt.args.groupVersion == 0 { + switch { + case tt.args.groupVersion != nil: + version = *tt.args.groupVersion + default: groupVersion += 1 version = groupVersion - } else { - version = tt.args.groupVersion } require.NoError(conn.Where("1=1").Delete(allocGroupMember()).Error) got, err := repo.AddGroupMembers(context.Background(), tt.args.groupId, version, tt.args.userIds, tt.args.opt...) @@ -919,6 +929,29 @@ func TestRepository_DeleteGroupMembers(t *testing.T) { wantErr: true, wantIsErr: db.ErrInvalidParameter, }, + { + name: "bad-version", + args: args{ + group: TestGroup(t, conn, org.PublicId), + createUserCnt: 5, + deleteUserCnt: 5, + groupVersion: 10000, + }, + wantRowsDeleted: 0, + wantErr: true, + }, + { + name: "zero-version", + args: args{ + group: TestGroup(t, conn, org.PublicId), + createUserCnt: 5, + deleteUserCnt: 5, + groupVersion: 0, + }, + wantRowsDeleted: 0, + wantErr: true, + wantIsErr: db.ErrInvalidParameter, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -1051,6 +1084,30 @@ func TestRepository_SetGroupMembers(t *testing.T) { wantErr: false, wantAffectedRows: 6, }, + { + name: "bad version", + setup: setupFn, + args: args{ + group: TestGroup(t, conn, proj.PublicId), + groupVersion: 1000, + userIds: []string{testUser.PublicId}, + addToOrigUsers: true, + }, + wantErr: true, + wantAffectedRows: 0, + }, + { + name: "zero version", + setup: setupFn, + args: args{ + group: TestGroup(t, conn, proj.PublicId), + groupVersion: 0, + userIds: []string{testUser.PublicId}, + addToOrigUsers: true, + }, + wantErr: true, + wantAffectedRows: 0, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/internal/iam/repository_principal_role.go b/internal/iam/repository_principal_role.go index 3e84ba0ad..e1221bb99 100644 --- a/internal/iam/repository_principal_role.go +++ b/internal/iam/repository_principal_role.go @@ -12,11 +12,15 @@ import ( // AddPrincipalRoles provides the ability to add principals (userIds and // groupIds) to a role (roleId). The role's current db version must match the // roleVersion or an error will be returned. The list of current PrincipalRoles -// after the adds will be returned on success. +// after the adds will be returned on success. Zero is not a valid value for +// the WithVersion option and will return an error. func (r *Repository) AddPrincipalRoles(ctx context.Context, roleId string, roleVersion uint32, principalIds []string, opt ...Option) ([]PrincipalRole, error) { if roleId == "" { return nil, fmt.Errorf("add principal roles: missing role id: %w", db.ErrInvalidParameter) } + if roleVersion == 0 { + return nil, fmt.Errorf("add principal roles: version cannot be zero: %w", db.ErrInvalidParameter) + } userIds, groupIds, err := splitPrincipals(principalIds) if err != nil { return nil, fmt.Errorf("add principal roles: error parsing principals: %w", err) @@ -63,7 +67,7 @@ func (r *Repository) AddPrincipalRoles(ctx context.Context, roleId string, roleV updatedRole.PublicId = roleId updatedRole.Version = roleVersion + 1 var roleOplogMsg oplog.Message - rowsUpdated, err := w.Update(ctx, &updatedRole, []string{"Version"}, nil, db.NewOplogMsg(&roleOplogMsg), db.WithVersion(roleVersion)) + rowsUpdated, err := w.Update(ctx, &updatedRole, []string{"Version"}, nil, db.NewOplogMsg(&roleOplogMsg), db.WithVersion(&roleVersion)) if err != nil { return fmt.Errorf("add principal roles: unable to update role version: %w", err) } @@ -118,11 +122,15 @@ func (r *Repository) AddPrincipalRoles(ctx context.Context, roleId string, roleV // SetPrincipalRoles will set the role's principals. Set add and/or delete // principals as need to reconcile the existing principals with the principals // requested. If both userIds and groupIds are empty, the principal roles will -// be cleared. +// be cleared. Zero is not a valid value for the WithVersion option and will +// return an error. func (r *Repository) SetPrincipalRoles(ctx context.Context, roleId string, roleVersion uint32, principalIds []string, opt ...Option) ([]PrincipalRole, int, error) { if roleId == "" { return nil, db.NoRowsAffected, fmt.Errorf("set principal roles: missing role id: %w", db.ErrInvalidParameter) } + if roleVersion == 0 { + return nil, db.NoRowsAffected, fmt.Errorf("set principal roles: version cannot be zero: %w", db.ErrInvalidParameter) + } role := allocRole() role.PublicId = roleId scope, err := role.GetScope(ctx, r.reader) @@ -164,7 +172,7 @@ func (r *Repository) SetPrincipalRoles(ctx context.Context, roleId string, roleV updatedRole.PublicId = roleId updatedRole.Version = roleVersion + 1 var roleOplogMsg oplog.Message - rowsUpdated, err := w.Update(ctx, &updatedRole, []string{"Version"}, nil, db.NewOplogMsg(&roleOplogMsg), db.WithVersion(roleVersion)) + rowsUpdated, err := w.Update(ctx, &updatedRole, []string{"Version"}, nil, db.NewOplogMsg(&roleOplogMsg), db.WithVersion(&roleVersion)) if err != nil { return fmt.Errorf("set principal roles: unable to update role version: %w", err) } @@ -251,7 +259,8 @@ func (r *Repository) SetPrincipalRoles(ctx context.Context, roleId string, roleV // DeletePrincipalRoles principals (userIds and/or groupIds) from a role // (roleId). The role's current db version must match the roleVersion or an -// error will be returned. +// error will be returned. Zero is not a valid value for the WithVersion option +// and will return an error. func (r *Repository) DeletePrincipalRoles(ctx context.Context, roleId string, roleVersion uint32, principalIds []string, opt ...Option) (int, error) { if roleId == "" { return db.NoRowsAffected, fmt.Errorf("delete principal roles: missing role id: %w", db.ErrInvalidParameter) @@ -263,6 +272,9 @@ func (r *Repository) DeletePrincipalRoles(ctx context.Context, roleId string, ro if len(userIds) == 0 && len(groupIds) == 0 { return db.NoRowsAffected, fmt.Errorf("delete principal roles: missing either user or groups to delete: %w", db.ErrInvalidParameter) } + if roleVersion == 0 { + return db.NoRowsAffected, fmt.Errorf("delete principal roles: version cannot be zero: %w", db.ErrInvalidParameter) + } role := allocRole() role.PublicId = roleId scope, err := role.GetScope(ctx, r.reader) @@ -301,7 +313,7 @@ func (r *Repository) DeletePrincipalRoles(ctx context.Context, roleId string, ro updatedRole.PublicId = roleId updatedRole.Version = roleVersion + 1 var roleOplogMsg oplog.Message - rowsUpdated, err := w.Update(ctx, &updatedRole, []string{"Version"}, nil, db.NewOplogMsg(&roleOplogMsg), db.WithVersion(roleVersion)) + rowsUpdated, err := w.Update(ctx, &updatedRole, []string{"Version"}, nil, db.NewOplogMsg(&roleOplogMsg), db.WithVersion(&roleVersion)) if err != nil { return fmt.Errorf("delete principal roles: unable to update role version: %w", err) } diff --git a/internal/iam/repository_principal_role_test.go b/internal/iam/repository_principal_role_test.go index 715ccc6b9..4fc993e9b 100644 --- a/internal/iam/repository_principal_role_test.go +++ b/internal/iam/repository_principal_role_test.go @@ -97,6 +97,15 @@ func TestRepository_AddPrincipalRoles(t *testing.T) { }, wantErr: true, }, + { + name: "zero-version", + args: args{ + roleVersion: 0, + wantUserIds: true, + wantGroupIds: true, + }, + wantErr: true, + }, { name: "no-principals", args: args{ @@ -114,6 +123,9 @@ func TestRepository_AddPrincipalRoles(t *testing.T) { var userIds, groupIds []string for _, roleId := range []string{orgRole.PublicId, projRole.PublicId} { + origRole, _, _, err := repo.LookupRole(context.Background(), roleId) + require.NoError(err) + if tt.args.wantUserIds { userIds = createUsersFn(orgs) u := TestUser(t, conn, staticOrg.PublicId) @@ -159,6 +171,12 @@ func TestRepository_AddPrincipalRoles(t *testing.T) { assert.Equal(gotPrincipal[principalId].GetPrincipalScopeId(), r.GetPrincipalScopeId()) assert.Equal(gotPrincipal[principalId].GetType(), r.GetType()) } + + r, _, _, err := repo.LookupRole(context.Background(), roleId) + require.NoError(err) + assert.Equal(tt.args.roleVersion+1, r.Version) + assert.Equal(origRole.Version, r.Version-1) + t.Logf("roleScope: %v and origVersion/newVersion: %d/%d", r.ScopeId, origRole.Version, r.Version) } }) } @@ -280,13 +298,14 @@ func TestRepository_DeletePrincipalRoles(t *testing.T) { org, _ := TestScopes(t, conn) type args struct { - role *Role - roleIdOverride *string - createUserCnt int - createGroupCnt int - deleteUserCnt int - deleteGroupCnt int - opt []Option + role *Role + roleIdOverride *string + roleVersionOverride *uint32 + createUserCnt int + createGroupCnt int + deleteUserCnt int + deleteGroupCnt int + opt []Option } tests := []struct { name string @@ -377,6 +396,33 @@ func TestRepository_DeletePrincipalRoles(t *testing.T) { wantErr: true, wantIsErr: db.ErrInvalidParameter, }, + { + name: "zero-version", + args: args{ + role: TestRole(t, conn, org.PublicId), + roleVersionOverride: func() *uint32 { v := uint32(0); return &v }(), + createUserCnt: 5, + createGroupCnt: 5, + deleteUserCnt: 5, + deleteGroupCnt: 5, + }, + wantRowsDeleted: 0, + wantErr: true, + wantIsErr: db.ErrInvalidParameter, + }, + { + name: "bad-version", + args: args{ + role: TestRole(t, conn, org.PublicId), + roleVersionOverride: func() *uint32 { v := uint32(1000); return &v }(), + createUserCnt: 5, + createGroupCnt: 5, + deleteUserCnt: 5, + deleteGroupCnt: 5, + }, + wantRowsDeleted: 0, + wantErr: true, + }, } createScopesFn := func() (orgs []string, projects []string) { for i := 0; i < 5; i++ { @@ -428,8 +474,15 @@ func TestRepository_DeletePrincipalRoles(t *testing.T) { default: roleId = tt.args.role.PublicId } + var roleVersion uint32 + switch { + case tt.args.roleVersionOverride != nil: + roleVersion = *tt.args.roleVersionOverride + default: + roleVersion = 2 + } principalIds = append(deleteUserIds, deleteGroupIds...) - deletedRows, err := repo.DeletePrincipalRoles(context.Background(), roleId, 2, principalIds, tt.args.opt...) + deletedRows, err := repo.DeletePrincipalRoles(context.Background(), roleId, roleVersion, principalIds, tt.args.opt...) if tt.wantErr { assert.Error(err) assert.Equal(0, deletedRows) @@ -504,18 +557,18 @@ func TestRepository_SetPrincipalRoles(t *testing.T) { wantAffectedRows int wantErr bool }{ - // { - // name: "clear", - // setup: setupFn, - // args: args{ - // role: TestRole(t, conn, proj.PublicId), - // roleVersion: 2, // yep, since setupFn will increment it to 2 - // userIds: []string{}, - // groupIds: []string{}, - // }, - // wantErr: false, - // wantAffectedRows: 12, - // }, + { + name: "clear", + setup: setupFn, + args: args{ + role: TestRole(t, conn, proj.PublicId), + roleVersion: 2, // yep, since setupFn will increment it to 2 + userIds: []string{}, + groupIds: []string{}, + }, + wantErr: false, + wantAffectedRows: 12, + }, { name: "no change", setup: setupFn, @@ -544,6 +597,19 @@ func TestRepository_SetPrincipalRoles(t *testing.T) { wantErr: false, wantAffectedRows: 2, }, + { + name: "add users and grps with zero version", + setup: setupFn, + args: args{ + role: TestRole(t, conn, proj.PublicId), + roleVersion: 0, // yep, since setupFn will increment it to 2 + userIds: []string{testUser.PublicId}, + groupIds: []string{testGrp.PublicId}, + addToOrigUsers: true, + addToOrigGrps: true, + }, + wantErr: true, + }, { name: "remove existing and add users and grps", setup: setupFn, @@ -572,12 +638,17 @@ func TestRepository_SetPrincipalRoles(t *testing.T) { if tt.args.addToOrigGrps { tt.args.groupIds = append(tt.args.groupIds, origGrps...) } + origRole, _, _, err := repo.LookupRole(context.Background(), tt.args.role.PublicId) + require.NoError(err) + principalIds := append(tt.args.userIds, tt.args.groupIds...) got, affectedRows, err := repo.SetPrincipalRoles(context.Background(), tt.args.role.PublicId, tt.args.roleVersion, principalIds, tt.args.opt...) if tt.wantErr { require.Error(err) + t.Log(err) return } + t.Log(err) require.NoError(err) assert.Equal(tt.wantAffectedRows, affectedRows) assert.Equal(len(tt.args.userIds)+len(tt.args.groupIds), len(got)) @@ -591,6 +662,14 @@ func TestRepository_SetPrincipalRoles(t *testing.T) { sort.Strings(wantIds) sort.Strings(gotIds) assert.Equal(wantIds, gotIds) + + r, _, _, err := repo.LookupRole(context.Background(), tt.args.role.PublicId) + require.NoError(err) + if tt.name != "no change" { + assert.Equalf(tt.args.roleVersion+1, r.Version, "%s unexpected version: %d/%d", tt.name, tt.args.roleVersion+1, r.Version) + assert.Equalf(origRole.Version, r.Version-1, "%s unexpected version: %d/%d", tt.name, origRole.Version, r.Version-1) + } + t.Logf("roleScope: %v and origVersion/newVersion: %d/%d", r.ScopeId, origRole.Version, r.Version) }) } } diff --git a/internal/iam/repository_role_grant.go b/internal/iam/repository_role_grant.go index 3e1399cc0..33bbf1dc1 100644 --- a/internal/iam/repository_role_grant.go +++ b/internal/iam/repository_role_grant.go @@ -10,7 +10,8 @@ import ( ) // AddRoleGrant will add role grants associated with the role ID in the -// repository. No options are currently supported. +// repository. No options are currently supported. Zero is not a valid value for +// the WithVersion option and will return an error. func (r *Repository) AddRoleGrants(ctx context.Context, roleId string, roleVersion uint32, grants []string, opt ...Option) ([]*RoleGrant, error) { if roleId == "" { return nil, fmt.Errorf("add role grants: missing role id %w", db.ErrInvalidParameter) @@ -18,6 +19,9 @@ func (r *Repository) AddRoleGrants(ctx context.Context, roleId string, roleVersi if len(grants) == 0 { return nil, fmt.Errorf("add role grants: missing grants: %w", db.ErrInvalidParameter) } + if roleVersion == 0 { + return nil, fmt.Errorf("add role grants: version cannot be zero: %w", db.ErrInvalidParameter) + } role := allocRole() role.PublicId = roleId @@ -46,7 +50,7 @@ func (r *Repository) AddRoleGrants(ctx context.Context, roleId string, roleVersi updatedRole.PublicId = roleId updatedRole.Version = uint32(roleVersion) + 1 var roleOplogMsg oplog.Message - rowsUpdated, err := w.Update(ctx, &updatedRole, []string{"Version"}, nil, db.NewOplogMsg(&roleOplogMsg), db.WithVersion(roleVersion)) + rowsUpdated, err := w.Update(ctx, &updatedRole, []string{"Version"}, nil, db.NewOplogMsg(&roleOplogMsg), db.WithVersion(&roleVersion)) if err != nil { return fmt.Errorf("unable to update role version: %w", err) } @@ -90,6 +94,8 @@ func (r *Repository) AddRoleGrants(ctx context.Context, roleId string, roleVersi // DeleteRoleGrants deletes grants (as strings) from a role (roleId). The role's // current db version must match the roleVersion or an error will be returned. +// Zero is not a valid value for the WithVersion option and will return an +// error. func (r *Repository) DeleteRoleGrants(ctx context.Context, roleId string, roleVersion uint32, grants []string, opt ...Option) (int, error) { if roleId == "" { return 0, fmt.Errorf("delete role grants: missing role id %w", db.ErrInvalidParameter) @@ -97,6 +103,9 @@ func (r *Repository) DeleteRoleGrants(ctx context.Context, roleId string, roleVe if len(grants) == 0 { return 0, fmt.Errorf("delete role grants: missing grants: %w", db.ErrInvalidParameter) } + if roleVersion == 0 { + return 0, fmt.Errorf("delete role grants: version cannot be zero: %w", db.ErrInvalidParameter) + } role := allocRole() role.PublicId = roleId @@ -115,7 +124,7 @@ func (r *Repository) DeleteRoleGrants(ctx context.Context, roleId string, roleVe updatedRole.PublicId = roleId updatedRole.Version = uint32(roleVersion) + 1 var roleOplogMsg oplog.Message - rowsUpdated, err := w.Update(ctx, &updatedRole, []string{"Version"}, nil, db.NewOplogMsg(&roleOplogMsg), db.WithVersion(roleVersion)) + rowsUpdated, err := w.Update(ctx, &updatedRole, []string{"Version"}, nil, db.NewOplogMsg(&roleOplogMsg), db.WithVersion(&roleVersion)) if err != nil { return fmt.Errorf("delete role grants: unable to update role version: %w", err) } @@ -195,11 +204,15 @@ func (r *Repository) DeleteRoleGrants(ctx context.Context, roleId string, roleVe } // SetRoleGrants sets grants on a role (roleId). The role's current db version -// must match the roleVersion or an error will be returned. +// must match the roleVersion or an error will be returned. Zero is not a valid +// value for the WithVersion option and will return an error. func (r *Repository) SetRoleGrants(ctx context.Context, roleId string, roleVersion uint32, grants []string, opt ...Option) ([]*RoleGrant, int, error) { if roleId == "" { return nil, 0, fmt.Errorf("set role grants: missing role id %w", db.ErrInvalidParameter) } + if roleVersion == 0 { + return nil, 0, fmt.Errorf("set role grants: version cannot be zero: %w", db.ErrInvalidParameter) + } // Explicitly set to zero clears, but treat nil as a mistake if grants == nil { @@ -278,7 +291,7 @@ func (r *Repository) SetRoleGrants(ctx context.Context, roleId string, roleVersi updatedRole.PublicId = roleId updatedRole.Version = roleVersion + 1 var roleOplogMsg oplog.Message - rowsUpdated, err := w.Update(ctx, &updatedRole, []string{"Version"}, nil, db.NewOplogMsg(&roleOplogMsg), db.WithVersion(roleVersion)) + rowsUpdated, err := w.Update(ctx, &updatedRole, []string{"Version"}, nil, db.NewOplogMsg(&roleOplogMsg), db.WithVersion(&roleVersion)) if err != nil { return fmt.Errorf("set role grants: unable to update role version: %w", err) } diff --git a/internal/iam/repository_role_grant_test.go b/internal/iam/repository_role_grant_test.go index f60c83e4d..ef5659e77 100644 --- a/internal/iam/repository_role_grant_test.go +++ b/internal/iam/repository_role_grant_test.go @@ -70,6 +70,15 @@ func TestRepository_AddRoleGrants(t *testing.T) { }, wantErr: true, }, + { + name: "zero-version", + args: args{ + roleId: role.PublicId, + roleVersion: 0, + grants: createGrantsFn(), + }, + wantErr: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -224,6 +233,7 @@ func TestRepository_DeleteRoleGrants(t *testing.T) { type args struct { role *Role roleIdOverride *string + roleVersionOverride *uint32 grantStringsOverride []string createCnt int deleteCnt int @@ -301,6 +311,28 @@ func TestRepository_DeleteRoleGrants(t *testing.T) { }, wantRowsDeleted: 2, }, + { + name: "zero-version", + args: args{ + role: TestRole(t, conn, org.PublicId), + createCnt: 5, + deleteCnt: 5, + roleVersionOverride: func() *uint32 { v := uint32(0); return &v }(), + }, + wantRowsDeleted: 0, + wantErr: true, + }, + { + name: "bad-version", + args: args{ + role: TestRole(t, conn, org.PublicId), + createCnt: 5, + deleteCnt: 5, + roleVersionOverride: func() *uint32 { v := uint32(1000); return &v }(), + }, + wantRowsDeleted: 0, + wantErr: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -336,7 +368,14 @@ func TestRepository_DeleteRoleGrants(t *testing.T) { default: roleId = tt.args.role.PublicId } - deletedRows, err := repo.DeleteRoleGrants(context.Background(), roleId, 2, deleteGrants, tt.args.opt...) + var roleVersion uint32 + switch { + case tt.args.roleVersionOverride != nil: + roleVersion = *tt.args.roleVersionOverride + default: + roleVersion = 2 + } + deletedRows, err := repo.DeleteRoleGrants(context.Background(), roleId, roleVersion, deleteGrants, tt.args.opt...) if tt.wantErr { assert.Error(err) assert.Equal(0, deletedRows) @@ -439,3 +478,97 @@ func TestRepository_SetRoleGrants_Randomize(t *testing.T) { require.NoError(err) require.Empty(roleGrants) } + +func TestRepository_SetRoleGrants_Parameters(t *testing.T) { + t.Parallel() + conn, _ := db.TestSetup(t, "postgres") + rw := db.New(conn) + wrapper := db.TestWrapper(t) + repo, err := NewRepository(rw, rw, wrapper) + require.NoError(t, err) + org, _ := TestScopes(t, conn) + role := TestRole(t, conn, org.PublicId) + type args struct { + roleId string + roleVersion uint32 + grants []string + opt []Option + } + tests := []struct { + name string + args args + want []*RoleGrant + wantAffectedRows int + wantErr bool + }{ + { + name: "missing-roleid", + args: args{ + roleId: "", + roleVersion: 1, + grants: []string{"id=s_1;actions=*"}, + }, + want: nil, + wantAffectedRows: 0, + wantErr: true, + }, + { + name: "bad-roleid", + args: args{ + roleId: "bad-roleid", + roleVersion: 1, + grants: []string{"id=s_1;actions=*"}, + }, + want: nil, + wantAffectedRows: 0, + wantErr: true, + }, + { + name: "nil-grants", + args: args{ + roleId: role.PublicId, + roleVersion: 1, + grants: nil, + }, + want: nil, + wantAffectedRows: 0, + wantErr: true, + }, + { + name: "zero-version", + args: args{ + roleId: role.PublicId, + roleVersion: 0, + grants: []string{"id=s_1;actions=*"}, + }, + want: nil, + wantAffectedRows: 0, + wantErr: true, + }, + { + name: "bad-version", + args: args{ + roleId: role.PublicId, + roleVersion: 1000, + grants: []string{"id=s_1;actions=*"}, + }, + want: nil, + wantAffectedRows: 0, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert, require := assert.New(t), require.New(t) + require.NoError(conn.Where("1=1").Delete(allocRoleGrant()).Error) + got, gotAffectedRows, err := repo.SetRoleGrants(context.Background(), tt.args.roleId, tt.args.roleVersion, tt.args.grants, tt.args.opt...) + if tt.wantErr { + require.Error(err) + } else { + require.NoError(err) + } + assert.Equal(tt.wantAffectedRows, gotAffectedRows) + assert.Equal(tt.want, got) + }) + } +}