diff --git a/api/roles/custom.go b/api/roles/custom.go index 0f2895e202..acb754f92c 100644 --- a/api/roles/custom.go +++ b/api/roles/custom.go @@ -122,7 +122,7 @@ func (s Role) AddGrants(ctx context.Context, grants []string) (*Role, *api.Error "version": s.Version, } if len(grants) > 0 { - body["grants"] = grants + body["grant_strings"] = grants } req, err := s.Client.NewRequest(ctx, "POST", fmt.Sprintf("roles/%s:add-grants", s.Id), body) @@ -156,9 +156,9 @@ func (s Role) SetGrants(ctx context.Context, grants []string) (*Role, *api.Error "version": s.Version, } if len(grants) > 0 { - body["grants"] = grants + body["grant_strings"] = grants } else if grants != nil { - body["grants"] = nil + body["grant_strings"] = nil } req, err := s.Client.NewRequest(ctx, "POST", fmt.Sprintf("roles/%s:set-grants", s.Id), body) @@ -192,7 +192,7 @@ func (s Role) RemoveGrants(ctx context.Context, grants []string) (*Role, *api.Er "version": s.Version, } if len(grants) > 0 { - body["grants"] = grants + body["grant_strings"] = grants } req, err := s.Client.NewRequest(ctx, "POST", fmt.Sprintf("roles/%s:remove-grants", s.Id), body) diff --git a/api/roles/role_test.go b/api/roles/role_test.go index 77455bda59..edeb67f37f 100644 --- a/api/roles/role_test.go +++ b/api/roles/role_test.go @@ -112,14 +112,14 @@ func TestCustom(t *testing.T) { require.NoError(err) require.Nil(apiErr, "Got error ", apiErr) assert.Equal(t, updatedRole.Version, r.Version+1) - assert.Contains(t, updatedRole.Grants, "id=*;actions=read") + assert.Contains(t, updatedRole.GrantStrings, "id=*;actions=read") r = updatedRole updatedRole, apiErr, err = r.SetGrants(ctx, []string{"id=*;actions=*"}) require.NoError(err) require.Nil(apiErr, "Got error ", apiErr) assert.Equal(t, updatedRole.Version, r.Version+1) - assert.Contains(t, updatedRole.Grants, "id=*;actions=*") + assert.Contains(t, updatedRole.GrantStrings, "id=*;actions=*") r = updatedRole updatedRole, apiErr, err = r.RemoveGrants(ctx, []string{"id=*;actions=*"}) diff --git a/internal/iam/repository_role.go b/internal/iam/repository_role.go index ac0a123916..741c8fa661 100644 --- a/internal/iam/repository_role.go +++ b/internal/iam/repository_role.go @@ -45,15 +45,15 @@ func (r *Repository) CreateRole(ctx context.Context, role *Role, opt ...Option) // included in fieldMask. Name, Description, and GrantScopeId are the only // updatable fields, If no updatable fields are included in the fieldMaskPaths, // then an error is returned. -func (r *Repository) UpdateRole(ctx context.Context, role *Role, fieldMaskPaths []string, opt ...Option) (*Role, int, error) { +func (r *Repository) UpdateRole(ctx context.Context, role *Role, fieldMaskPaths []string, opt ...Option) (*Role, []PrincipalRole, []*RoleGrant, int, error) { if role == nil { - return nil, db.NoRowsAffected, fmt.Errorf("update role: missing role %w", db.ErrNilParameter) + return nil, nil, nil, db.NoRowsAffected, fmt.Errorf("update role: missing role %w", db.ErrNilParameter) } if role.Role == nil { - return nil, db.NoRowsAffected, fmt.Errorf("update role: missing role store %w", db.ErrNilParameter) + return nil, nil, nil, db.NoRowsAffected, fmt.Errorf("update role: missing role store %w", db.ErrNilParameter) } if role.PublicId == "" { - return nil, db.NoRowsAffected, fmt.Errorf("update role: missing role public id %w", db.ErrInvalidParameter) + return nil, nil, nil, db.NoRowsAffected, fmt.Errorf("update role: missing role public id %w", db.ErrInvalidParameter) } for _, f := range fieldMaskPaths { switch { @@ -61,7 +61,7 @@ func (r *Repository) UpdateRole(ctx context.Context, role *Role, fieldMaskPaths case strings.EqualFold("description", f): case strings.EqualFold("grantscopeid", f): default: - return nil, db.NoRowsAffected, fmt.Errorf("update role: field: %s: %w", f, db.ErrInvalidFieldMask) + return nil, nil, nil, db.NoRowsAffected, fmt.Errorf("update role: field: %s: %w", f, db.ErrInvalidFieldMask) } } var dbMask, nullFields []string @@ -74,17 +74,45 @@ func (r *Repository) UpdateRole(ctx context.Context, role *Role, fieldMaskPaths fieldMaskPaths, ) if len(dbMask) == 0 && len(nullFields) == 0 { - return nil, db.NoRowsAffected, fmt.Errorf("update role: %w", db.ErrEmptyFieldMask) + return nil, nil, nil, db.NoRowsAffected, fmt.Errorf("update role: %w", db.ErrEmptyFieldMask) } - c := role.Clone().(*Role) - resource, rowsUpdated, err := r.update(ctx, c, dbMask, nullFields) + var resource Resource + var rowsUpdated int + var pr []PrincipalRole + var rg []*RoleGrant + _, err := r.writer.DoTx( + ctx, + db.StdRetryCnt, + db.ExpBackoff{}, + func(read db.Reader, w db.Writer) error { + var err error + c := role.Clone().(*Role) + resource, rowsUpdated, err = r.update(ctx, c, dbMask, nullFields) + if err != nil { + return err + } + repo, err := NewRepository(read, w, r.wrapper) + if err != nil { + return fmt.Errorf("update role: failed creating inner repo: %w for %s", err, role.PublicId) + } + pr, err = repo.ListPrincipalRoles(ctx, role.PublicId) + if err != nil { + return fmt.Errorf("update role: listing principal roles: %w for %s", err, role.PublicId) + } + rg, err = repo.ListRoleGrants(ctx, role.PublicId) + if err != nil { + return fmt.Errorf("update role: listing principal roles: %w for %s", err, role.PublicId) + } + return nil + }, + ) if err != nil { if db.IsUniqueError(err) { - return nil, db.NoRowsAffected, fmt.Errorf("update role: role %s already exists in org %s: %w", role.Name, role.ScopeId, db.ErrNotUnique) + return nil, nil, nil, db.NoRowsAffected, fmt.Errorf("update role: role %s already exists in org %s: %w", role.Name, role.ScopeId, db.ErrNotUnique) } - return nil, db.NoRowsAffected, fmt.Errorf("update role: %w for %s", err, c.PublicId) + return nil, nil, nil, db.NoRowsAffected, fmt.Errorf("update role: %w for %s", err, role.PublicId) } - return resource.(*Role), rowsUpdated, err + return resource.(*Role), pr, rg, rowsUpdated, err } // LookupRole will look up a role in the repository. If the role is not diff --git a/internal/iam/repository_role_test.go b/internal/iam/repository_role_test.go index d5034f5e6a..cc6e595b8b 100644 --- a/internal/iam/repository_role_test.go +++ b/internal/iam/repository_role_test.go @@ -3,12 +3,14 @@ package iam import ( "context" "errors" + "fmt" "testing" "time" "github.com/hashicorp/go-uuid" "github.com/hashicorp/watchtower/internal/db" dbassert "github.com/hashicorp/watchtower/internal/db/assert" + "github.com/hashicorp/watchtower/internal/iam/store" "github.com/hashicorp/watchtower/internal/oplog" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -181,6 +183,8 @@ func TestRepository_UpdateRole(t *testing.T) { require.NoError(t, err) org, proj := TestScopes(t, conn) + u := TestUser(t, conn, org.GetPublicId()) + pubId := func(s string) *string { return &s } type args struct { @@ -194,7 +198,7 @@ func TestRepository_UpdateRole(t *testing.T) { tests := []struct { name string newScopeId string - newGrpOpts []Option + newRoleOpts []Option args args wantRowsUpdate int wantErr bool @@ -221,7 +225,7 @@ func TestRepository_UpdateRole(t *testing.T) { ScopeId: org.PublicId, }, newScopeId: org.PublicId, - newGrpOpts: []Option{WithName("valid-no-op" + id)}, + newRoleOpts: []Option{WithName("valid-no-op" + id)}, wantErr: false, wantRowsUpdate: 1, }, @@ -247,7 +251,7 @@ func TestRepository_UpdateRole(t *testing.T) { ScopeId: org.PublicId, }, newScopeId: org.PublicId, - newGrpOpts: []Option{WithName("null-name" + id)}, + newRoleOpts: []Option{WithName("null-name" + id)}, wantErr: false, wantRowsUpdate: 1, }, @@ -259,7 +263,7 @@ func TestRepository_UpdateRole(t *testing.T) { ScopeId: org.PublicId, }, newScopeId: org.PublicId, - newGrpOpts: []Option{WithDescription("null-description" + id)}, + newRoleOpts: []Option{WithDescription("null-description" + id)}, wantErr: false, wantRowsUpdate: 1, }, @@ -359,7 +363,7 @@ func TestRepository_UpdateRole(t *testing.T) { ScopeId: proj.PublicId, }, newScopeId: proj.PublicId, - newGrpOpts: []Option{WithName("dup-name-in-diff-scope-pre-update" + id)}, + newRoleOpts: []Option{WithName("dup-name-in-diff-scope-pre-update" + id)}, wantErr: false, wantRowsUpdate: 1, wantDup: true, @@ -383,12 +387,31 @@ func TestRepository_UpdateRole(t *testing.T) { require, assert := require.New(t), assert.New(t) if tt.wantDup { r := TestRole(t, conn, org.PublicId) + _ = TestUserRole(t, conn, r.GetPublicId(), u.GetPublicId()) + _ = TestRoleGrant(t, conn, r.GetPublicId(), "id=*;actions=*") r.Name = tt.args.name - _, _, err := repo.UpdateRole(context.Background(), r, tt.args.fieldMaskPaths, tt.args.opt...) + _, _, _, _, err := repo.UpdateRole(context.Background(), r, tt.args.fieldMaskPaths, tt.args.opt...) assert.NoError(err) } - r := TestRole(t, conn, tt.newScopeId, tt.newGrpOpts...) + r := TestRole(t, conn, tt.newScopeId, tt.newRoleOpts...) + ur := TestUserRole(t, conn, r.GetPublicId(), u.GetPublicId()) + princRole := PrincipalRole{PrincipalRoleView: &store.PrincipalRoleView{ + Type: UserRoleType.String(), + CreateTime: ur.CreateTime, + RoleId: ur.RoleId, + PrincipalId: ur.PrincipalId, + PrincipalScopeId: org.GetPublicId(), + ScopedPrincipalId: ur.PrincipalId, + RoleScopeId: org.GetPublicId(), + }} + if tt.newScopeId != org.GetPublicId() { + // If the project is in a different scope from the created user we need to update the + // scope specific fields. + princRole.RoleScopeId = tt.newScopeId + princRole.ScopedPrincipalId = fmt.Sprintf("%s:%s", org.PublicId, ur.PrincipalId) + } + rGrant := TestRoleGrant(t, conn, r.GetPublicId(), "id=*;actions=*") updateRole := allocRole() updateRole.PublicId = r.PublicId @@ -399,7 +422,7 @@ func TestRepository_UpdateRole(t *testing.T) { updateRole.Name = tt.args.name updateRole.Description = tt.args.description - roleAfterUpdate, updatedRows, err := repo.UpdateRole(context.Background(), &updateRole, tt.args.fieldMaskPaths, tt.args.opt...) + roleAfterUpdate, principals, grants, updatedRows, err := repo.UpdateRole(context.Background(), &updateRole, tt.args.fieldMaskPaths, tt.args.opt...) if tt.wantErr { assert.Error(err) if tt.wantIsError != nil { @@ -416,6 +439,8 @@ func TestRepository_UpdateRole(t *testing.T) { require.NoError(err) require.NotNil(roleAfterUpdate) assert.Equal(tt.wantRowsUpdate, updatedRows) + assert.Equal([]PrincipalRole{princRole}, principals) + assert.Equal([]*RoleGrant{rGrant}, grants) switch tt.name { case "valid-no-op": assert.Equal(r.UpdateTime, roleAfterUpdate.UpdateTime) diff --git a/internal/servers/controller/handlers/roles/role_service.go b/internal/servers/controller/handlers/roles/role_service.go index 86d2a90ec8..0807af59ca 100644 --- a/internal/servers/controller/handlers/roles/role_service.go +++ b/internal/servers/controller/handlers/roles/role_service.go @@ -282,7 +282,7 @@ func (s Service) updateInRepo(ctx context.Context, scopeId, id string, mask []st if err != nil { return nil, err } - out, rowsUpdated, err := repo.UpdateRole(ctx, u, dbMask) + out, pr, gr, rowsUpdated, err := repo.UpdateRole(ctx, u, dbMask) if err != nil { return nil, status.Errorf(codes.Internal, "Unable to update role: %v.", err) } @@ -290,7 +290,7 @@ func (s Service) updateInRepo(ctx context.Context, scopeId, id string, mask []st return nil, handlers.NotFoundErrorf("Role %q doesn't exist.", id) } // TODO: Attach principals and grants to UpdateRole response - return toProto(out, nil, nil), nil + return toProto(out, pr, gr), nil } func (s Service) deleteFromRepo(ctx context.Context, id string) (bool, error) { diff --git a/internal/servers/controller/handlers/roles/role_service_test.go b/internal/servers/controller/handlers/roles/role_service_test.go index eb370fba42..f9c48905c7 100644 --- a/internal/servers/controller/handlers/roles/role_service_test.go +++ b/internal/servers/controller/handlers/roles/role_service_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/golang/protobuf/ptypes" + "github.com/google/go-cmp/cmp" "github.com/hashicorp/watchtower/internal/db" pb "github.com/hashicorp/watchtower/internal/gen/controller/api/resources/roles" pbs "github.com/hashicorp/watchtower/internal/gen/controller/api/services" @@ -18,6 +19,7 @@ import ( "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" "github.com/stretchr/testify/assert" @@ -539,22 +541,57 @@ func TestCreate(t *testing.T) { } func TestUpdate(t *testing.T) { - require := require.New(t) - or, pr, repoFn := createDefaultRolesAndRepo(t) + grantString := "id=*;actions=*" + g, err := perms.Parse("global", "", grantString) + require.NoError(t, err) + _, actions := g.Actions() + grant := &pb.Grant{ + Raw: grantString, + Canonical: g.CanonicalString(), + Json: &pb.GrantJson{ + Id: g.Id(), + Type: g.Type().String(), + Actions: actions, + }, + } + conn, _ := db.TestSetup(t, "postgres") + rw := db.New(conn) + wrap := db.TestWrapper(t) + repoFn := func() (*iam.Repository, error) { + return iam.NewRepository(rw, rw, wrap) + } + + o, p := iam.TestScopes(t, conn) + u := iam.TestUser(t, conn, o.GetPublicId()) + + or := iam.TestRole(t, conn, o.GetPublicId(), iam.WithDescription("default"), iam.WithName("default"), iam.WithGrantScopeId(p.GetPublicId())) + _ = iam.TestRoleGrant(t, conn, or.GetPublicId(), grantString) + _ = iam.TestUserRole(t, conn, or.GetPublicId(), u.GetPublicId()) + + pr := iam.TestRole(t, conn, p.GetPublicId(), iam.WithDescription("default"), iam.WithName("default")) + _ = iam.TestRoleGrant(t, conn, pr.GetPublicId(), grantString) + _ = iam.TestUserRole(t, conn, pr.GetPublicId(), u.GetPublicId()) + + principal := &pb.Principal{ + Id: u.GetPublicId(), + Type: iam.UserRoleType.String(), + ScopeId: u.GetScopeId(), + } + tested, err := roles.NewService(repoFn) - require.NoError(err, "Error when getting new role service.") + require.NoError(t, err, "Error when getting new role service.") resetRoles := func() { repo, err := repoFn() - require.NoError(err, "Couldn't get a new repo") - or, _, err = repo.UpdateRole(context.Background(), or, []string{"Name", "Description"}) - require.NoError(err, "Failed to reset the role") - pr, _, err = repo.UpdateRole(context.Background(), pr, []string{"Name", "Description"}) - require.NoError(err, "Failed to reset the role") + require.NoError(t, err, "Couldn't get a new repo") + or, _, _, _, err = repo.UpdateRole(context.Background(), or, []string{"Name", "Description"}) + require.NoError(t, err, "Failed to reset the role") + pr, _, _, _, err = repo.UpdateRole(context.Background(), pr, []string{"Name", "Description"}) + require.NoError(t, err, "Failed to reset the role") } created, err := ptypes.Timestamp(or.GetCreateTime().GetTimestamp()) - require.NoError(err, "Error converting proto to timestamp") + require.NoError(t, err, "Error converting proto to timestamp") toMerge := &pbs.UpdateRoleRequest{ OrgId: or.GetScopeId(), Id: or.GetPublicId(), @@ -585,6 +622,10 @@ func TestUpdate(t *testing.T) { Description: &wrapperspb.StringValue{Value: "desc"}, CreatedTime: or.GetCreateTime().GetTimestamp(), GrantScopeId: &wrapperspb.StringValue{Value: or.GetScopeId()}, + GrantStrings: []string{grant.GetRaw()}, + Grants: []*pb.Grant{grant}, + PrincipalIds: []string{u.GetPublicId()}, + Principals: []*pb.Principal{principal}, }, }, errCode: codes.OK, @@ -607,6 +648,10 @@ func TestUpdate(t *testing.T) { Description: &wrapperspb.StringValue{Value: "desc"}, CreatedTime: or.GetCreateTime().GetTimestamp(), GrantScopeId: &wrapperspb.StringValue{Value: or.GetScopeId()}, + GrantStrings: []string{grant.GetRaw()}, + Grants: []*pb.Grant{grant}, + PrincipalIds: []string{u.GetPublicId()}, + Principals: []*pb.Principal{principal}, }, }, errCode: codes.OK, @@ -631,6 +676,10 @@ func TestUpdate(t *testing.T) { Description: &wrapperspb.StringValue{Value: "desc"}, CreatedTime: pr.GetCreateTime().GetTimestamp(), GrantScopeId: &wrapperspb.StringValue{Value: pr.GetScopeId()}, + GrantStrings: []string{grant.GetRaw()}, + Grants: []*pb.Grant{grant}, + PrincipalIds: []string{u.GetPublicId()}, + Principals: []*pb.Principal{principal}, }, }, errCode: codes.OK, @@ -655,6 +704,10 @@ func TestUpdate(t *testing.T) { Description: &wrapperspb.StringValue{Value: "desc"}, CreatedTime: pr.GetCreateTime().GetTimestamp(), GrantScopeId: &wrapperspb.StringValue{Value: pr.GetScopeId()}, + GrantStrings: []string{grant.GetRaw()}, + Grants: []*pb.Grant{grant}, + PrincipalIds: []string{u.GetPublicId()}, + Principals: []*pb.Principal{principal}, }, }, errCode: codes.OK, @@ -707,6 +760,10 @@ func TestUpdate(t *testing.T) { Description: &wrapperspb.StringValue{Value: "default"}, CreatedTime: or.GetCreateTime().GetTimestamp(), GrantScopeId: &wrapperspb.StringValue{Value: or.GetScopeId()}, + GrantStrings: []string{grant.GetRaw()}, + Grants: []*pb.Grant{grant}, + PrincipalIds: []string{u.GetPublicId()}, + Principals: []*pb.Principal{principal}, }, }, errCode: codes.OK, @@ -729,6 +786,10 @@ func TestUpdate(t *testing.T) { Description: &wrapperspb.StringValue{Value: "default"}, CreatedTime: or.GetCreateTime().GetTimestamp(), GrantScopeId: &wrapperspb.StringValue{Value: or.GetScopeId()}, + GrantStrings: []string{grant.GetRaw()}, + Grants: []*pb.Grant{grant}, + PrincipalIds: []string{u.GetPublicId()}, + Principals: []*pb.Principal{principal}, }, }, errCode: codes.OK, @@ -751,6 +812,10 @@ func TestUpdate(t *testing.T) { Description: &wrapperspb.StringValue{Value: "notignored"}, CreatedTime: or.GetCreateTime().GetTimestamp(), GrantScopeId: &wrapperspb.StringValue{Value: or.GetScopeId()}, + GrantStrings: []string{grant.GetRaw()}, + Grants: []*pb.Grant{grant}, + PrincipalIds: []string{u.GetPublicId()}, + Principals: []*pb.Principal{principal}, }, }, errCode: codes.OK, @@ -812,11 +877,37 @@ func TestUpdate(t *testing.T) { res: nil, errCode: codes.InvalidArgument, }, + { + name: "Cant specify grants", + req: &pbs.UpdateRoleRequest{ + UpdateMask: &field_mask.FieldMask{ + Paths: []string{"grants"}, + }, + Item: &pb.Role{ + GrantStrings: []string{"anything"}, + }, + }, + res: nil, + errCode: codes.InvalidArgument, + }, + { + name: "Cant specify principals", + req: &pbs.UpdateRoleRequest{ + UpdateMask: &field_mask.FieldMask{ + Paths: []string{"principal_ids"}, + }, + Item: &pb.Role{ + PrincipalIds: []string{"u_0987654321"}, + }, + }, + res: nil, + errCode: codes.InvalidArgument, + }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { defer resetRoles() - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) req := proto.Clone(toMerge).(*pbs.UpdateRoleRequest) proto.Merge(req, tc.req) @@ -836,7 +927,7 @@ func TestUpdate(t *testing.T) { // TODO: Figure out the best way to test versions when updating roles got.GetItem().Version = 0 } - assert.True(proto.Equal(got, tc.res), "UpdateRole(%q) got response\n%q,\nwanted\n%q", req, got, tc.res) + assert.Empty(cmp.Diff(got, tc.res, protocmp.Transform()), "UpdateRole(%q) got response\n%q,\nwanted\n%q", req, got, tc.res) }) } }