Skip to content

Commit

Permalink
fix: remove assignees tuples on DeleteRole
Browse files Browse the repository at this point in the history
closes #285
  • Loading branch information
shipperizer committed Apr 23, 2024
1 parent 2fbf898 commit 5772334
Show file tree
Hide file tree
Showing 3 changed files with 123 additions and 16 deletions.
1 change: 1 addition & 0 deletions pkg/roles/handlers.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ func (a *API) RegisterEndpoints(mux *chi.Mux) {
mux.Delete("/api/v0/roles/{id:.+}/entitlements/{e_id:.+}", a.handleRemovePermission)
mux.Get("/api/v0/roles/{id:.+}/groups", a.handleListRoleGroup)
}

func (a *API) RegisterValidation(v validation.ValidationRegistryInterface) {
err := v.RegisterPayloadValidator("roles", a.payloadValidator)
if err != nil {
Expand Down
59 changes: 57 additions & 2 deletions pkg/roles/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -253,9 +253,12 @@ func (s *Service) DeleteRole(ctx context.Context, ID string) error {
// https://go.dev/ref/spec#Send_statements
// A send on an unbuffered channel can proceed if a receiver is ready.
// A send on a buffered channel can proceed if there is room in the buffer
results := make(chan *pool.Result[any], len(s.permissionTypes()))
jobs := len(s.permissionTypes()) + 1

results := make(chan *pool.Result[any], jobs)
wg := sync.WaitGroup{}
wg.Add(len(s.permissionTypes()))
// number of types + 1 for assignees job
wg.Add(jobs)

// TODO @shipperizer use a background operator
for _, t := range s.permissionTypes() {
Expand All @@ -266,6 +269,12 @@ func (s *Service) DeleteRole(ctx context.Context, ID string) error {
)
}

s.wpool.Submit(
s.removeAssigneesFunc(ctx, ID),
results,
&wg,
)

// wait for tasks to finish
wg.Wait()

Expand Down Expand Up @@ -326,11 +335,51 @@ func (s *Service) removePermissionsByType(ctx context.Context, ID, pType string)
break
}

if len(permissions) == 0 {
return
}

if err := s.ofga.DeleteTuples(ctx, permissions...); err != nil {
s.logger.Error(err.Error())
}
}

func (s *Service) removeAssignees(ctx context.Context, ID string) {
ctx, span := s.tracer.Start(ctx, "roles.Service.removeAssignees")
defer span.End()

cToken := ""
assignees := make([]ofga.Tuple, 0)
for {
r, err := s.ofga.ReadTuples(ctx, "", ASSIGNEE_RELATION, fmt.Sprintf("role:%s", ID), cToken)

if err != nil {
s.logger.Errorf("error when retrieving tuples for %s role:%s", ASSIGNEE_RELATION, ID)
return
}

for _, t := range r.Tuples {
assignees = append(assignees, *ofga.NewTuple(t.Key.User, t.Key.Relation, t.Key.Object))
}

// if there are more pages, keep going with the loop
if cToken = r.ContinuationToken; cToken != "" {
continue
}

// TODO @shipperizer understand if better breaking at every cycle or reverting if clause
break
}

if len(assignees) == 0 {
return
}

if err := s.ofga.DeleteTuples(ctx, assignees...); err != nil {
s.logger.Error(err.Error())
}
}

func (s *Service) listPermissionsFunc(ctx context.Context, roleID, ofgaType, cToken string) func() any {
return func() any {
p, token, err := s.listPermissionsByType(
Expand All @@ -355,6 +404,12 @@ func (s *Service) removePermissionsFunc(ctx context.Context, roleID, ofgaType st
}
}

func (s *Service) removeAssigneesFunc(ctx context.Context, roleID string) func() {
return func() {
s.removeAssignees(ctx, roleID)
}
}

func (s *Service) permissionTypes() []string {
return []string{"role", "group", "identity", "scheme", "provider", "client"}
}
Expand Down
79 changes: 65 additions & 14 deletions pkg/roles/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -461,14 +461,15 @@ func TestServiceDeleteRole(t *testing.T) {
mockOpenFGA := NewMockOpenFGAClientInterface(ctrl)

workerPool := pool.NewMockWorkerPoolInterface(ctrl)
for i := 0; i < 6; i++ {
for i := 0; i < 7; i++ {
setupMockSubmit(workerPool, nil)
}

svc := NewService(mockOpenFGA, workerPool, mockTracer, mockMonitor, mockLogger)

mockTracer.EXPECT().Start(gomock.Any(), "roles.Service.DeleteRole").Times(1).Return(context.TODO(), trace.SpanFromContext(context.TODO()))
mockTracer.EXPECT().Start(gomock.Any(), "roles.Service.removePermissionsByType").Times(6).Return(context.TODO(), trace.SpanFromContext(context.TODO()))
mockTracer.EXPECT().Start(gomock.Any(), "roles.Service.removeAssignees").Times(1).Return(context.TODO(), trace.SpanFromContext(context.TODO()))

pTypes := []string{"role", "group", "identity", "scheme", "provider", "client"}

Expand Down Expand Up @@ -504,28 +505,78 @@ func TestServiceDeleteRole(t *testing.T) {

}

calls = append(
calls,
mockOpenFGA.EXPECT().ReadTuples(gomock.Any(), "", ASSIGNEE_RELATION, fmt.Sprintf("role:%s", test.input), "").Times(1).DoAndReturn(
func(ctx context.Context, user, relation, object, continuationToken string) (*client.ClientReadResponse, error) {
if test.expected != nil {
return nil, test.expected
}

tuples := []openfga.Tuple{
*openfga.NewTuple(
*openfga.NewTupleKey(
"user:test", ASSIGNEE_RELATION, object,
),
time.Now(),
),
*openfga.NewTuple(
*openfga.NewTupleKey(
"group:test#member", ASSIGNEE_RELATION, object,
),
time.Now(),
),
}

r := new(client.ClientReadResponse)
r.SetContinuationToken("")
r.SetTuples(tuples)

return r, nil
},
),
)

if test.expected == nil {
mockOpenFGA.EXPECT().DeleteTuples(
gomock.Any(),
gomock.Any(),
).Times(7).DoAndReturn(
).Times(8).DoAndReturn(
func(ctx context.Context, tuples ...ofga.Tuple) error {
if len(tuples) != 1 {
t.Errorf("too many tuples")
}

tuple := tuples[0]
switch len(tuples) {
case 1:
tuple := tuples[0]

if tuple.User != fmt.Sprintf("role:%s#%s", test.input, ASSIGNEE_RELATION) && tuple.User != authorization.ADMIN_PRIVILEGE {
t.Errorf("expected user to be one of %v got %v", []string{fmt.Sprintf("role:%s#%s", test.input, ASSIGNEE_RELATION), authorization.ADMIN_PRIVILEGE}, tuple.User)
}
if tuple.User != fmt.Sprintf("role:%s#%s", test.input, ASSIGNEE_RELATION) && tuple.User != authorization.ADMIN_PRIVILEGE {
t.Errorf("expected user to be one of %v got %v", []string{fmt.Sprintf("role:%s#%s", test.input, ASSIGNEE_RELATION), authorization.ADMIN_PRIVILEGE}, tuple.User)
}

if tuple.Relation != "privileged" && tuple.Relation != "can_edit" {
t.Errorf("expected relation to be one of %v got %v", []string{"privileged", "can_edit"}, tuple.Relation)
}
if tuple.Relation != "privileged" && tuple.Relation != "can_edit" {
t.Errorf("expected relation to be one of %v got %v", []string{"privileged", "can_edit"}, tuple.Relation)
}

if tuple.Object != fmt.Sprintf("role:%s", test.input) && !strings.HasSuffix(tuple.Object, ":test") {
t.Errorf("expected object to be one of %v got %v", []string{fmt.Sprintf("role:%s", test.input), "<*>:test"}, tuple.Object)
}
case 2:

for _, tuple := range tuples {
if tuple.User != "user:test" && tuple.User != "group:test#member" {
t.Errorf("expected user to be one of %v got %v", []string{"user:test", "group:test#member"}, tuple.User)
}

if tuple.Relation != ASSIGNEE_RELATION {
t.Errorf("expected relation to be of %v got %v", ASSIGNEE_RELATION, tuple.Relation)
}

if tuple.Object != fmt.Sprintf("role:%s", test.input) && !strings.HasSuffix(tuple.Object, ":test") {
t.Errorf("expected object to be one of %v got %v", []string{fmt.Sprintf("role:%s", test.input), "<*>:test"}, tuple.Object)
if tuple.Object != fmt.Sprintf("role:%s", test.input) {
t.Errorf("expected object to be one of %v got %v", fmt.Sprintf("role:%s", test.input), tuple.Object)
}
}

default:
t.Errorf("too many tuples")
}

return nil
Expand Down

0 comments on commit 5772334

Please sign in to comment.