Include Principals and Grants on Roles When Updating the Role. (#203)

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)

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=*"})

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

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)

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) {

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)
 		})
 	}
 }