diff --git a/pkg/groups/handlers.go b/pkg/groups/handlers.go index c83178134..632e17558 100644 --- a/pkg/groups/handlers.go +++ b/pkg/groups/handlers.go @@ -146,7 +146,7 @@ func (a *API) handleDetail(w http.ResponseWriter, r *http.Request) { ID := chi.URLParam(r, "id") user := a.userFromContext(r.Context()) - role, err := a.service.GetGroup(r.Context(), user.ID, ID) + group, err := a.service.GetGroup(r.Context(), user.ID, ID) if err != nil { @@ -161,10 +161,21 @@ func (a *API) handleDetail(w http.ResponseWriter, r *http.Request) { return } + if group == "" { + w.WriteHeader(http.StatusNotFound) + json.NewEncoder(w).Encode( + types.Response{ + Message: "Group not found", + Status: http.StatusNotFound, + }, + ) + return + } + w.WriteHeader(http.StatusOK) json.NewEncoder(w).Encode( types.Response{ - Data: []string{role}, + Data: []string{group}, Message: "Group detail", Status: http.StatusOK, }, diff --git a/pkg/groups/service.go b/pkg/groups/service.go index 9aefe6bdb..efb9bff10 100644 --- a/pkg/groups/service.go +++ b/pkg/groups/service.go @@ -21,6 +21,8 @@ import ( const ( MEMBER_RELATION = "member" ASSIGNEE_RELATION = "assignee" + // TODO: @barco centralize common relation name definitions + CAN_VIEW_RELATION = "can_view" ) type listPermissionsResult struct { @@ -174,10 +176,14 @@ func (s *Service) CreateGroup(ctx context.Context, userID, ID string) error { // might sort the problem // TODO @shipperizer offload to privileged creator object + group := fmt.Sprintf("group:%s", ID) + user := fmt.Sprintf("user:%s", userID) + err := s.ofga.WriteTuples( ctx, - *ofga.NewTuple(fmt.Sprintf("user:%s", userID), MEMBER_RELATION, fmt.Sprintf("group:%s", ID)), - *ofga.NewTuple(authorization.ADMIN_PRIVILEGE, "privileged", fmt.Sprintf("group:%s", ID)), + *ofga.NewTuple(user, MEMBER_RELATION, group), + *ofga.NewTuple(authorization.ADMIN_PRIVILEGE, "privileged", group), + *ofga.NewTuple(user, CAN_VIEW_RELATION, group), ) if err != nil { @@ -296,7 +302,10 @@ func (s *Service) DeleteGroup(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 - jobs := len(s.permissionTypes()) + 1 + permissionTypes := s.permissionTypes() + directRelations := s.directRelations() + + jobs := len(permissionTypes) + len(directRelations) results := make(chan *pool.Result[any], jobs) wg := sync.WaitGroup{} @@ -311,11 +320,13 @@ func (s *Service) DeleteGroup(ctx context.Context, ID string) error { ) } - s.wpool.Submit( - s.removeMembersFunc(ctx, ID), - results, - &wg, - ) + for _, t := range directRelations { + s.wpool.Submit( + s.removeDirectAssociationsFunc(ctx, ID, t), + results, + &wg, + ) + } // wait for tasks to finish wg.Wait() @@ -323,7 +334,8 @@ func (s *Service) DeleteGroup(ctx context.Context, ID string) error { // close result channel close(results) - return s.ofga.DeleteTuples(ctx, *ofga.NewTuple(authorization.ADMIN_PRIVILEGE, "privileged", fmt.Sprintf("group:%s", ID))) + // TODO: @barco collect errors from results chan and return composite error or single summing up + return nil } // ListIdentities returns all the identities (users for now) assigned to a group @@ -458,22 +470,22 @@ func (s *Service) removePermissionsByType(ctx context.Context, ID, pType string) } } -func (s *Service) removeMembers(ctx context.Context, ID string) { - ctx, span := s.tracer.Start(ctx, "roles.Service.removeMembers") +func (s *Service) removeDirectAssociations(ctx context.Context, ID, relation string) { + ctx, span := s.tracer.Start(ctx, "groups.Service.removeDirectAssociations") defer span.End() cToken := "" - members := make([]ofga.Tuple, 0) + directs := make([]ofga.Tuple, 0) for { - r, err := s.ofga.ReadTuples(ctx, "", MEMBER_RELATION, fmt.Sprintf("group:%s", ID), cToken) + r, err := s.ofga.ReadTuples(ctx, "", relation, fmt.Sprintf("group:%s", ID), cToken) if err != nil { - s.logger.Errorf("error when retrieving tuples for %s group:%s", MEMBER_RELATION, ID) + s.logger.Errorf("error when retrieving tuples for %s group, %s relation", relation, ID) return } for _, t := range r.Tuples { - members = append(members, *ofga.NewTuple(t.Key.User, t.Key.Relation, t.Key.Object)) + directs = append(directs, *ofga.NewTuple(t.Key.User, t.Key.Relation, t.Key.Object)) } // if there are more pages, keep going with the loop @@ -481,15 +493,14 @@ func (s *Service) removeMembers(ctx context.Context, ID string) { continue } - // TODO @shipperizer understand if better breaking at every cycle or reverting if clause break } - if len(members) == 0 { + if len(directs) == 0 { return } - if err := s.ofga.DeleteTuples(ctx, members...); err != nil { + if err := s.ofga.DeleteTuples(ctx, directs...); err != nil { s.logger.Error(err.Error()) } } @@ -518,9 +529,9 @@ func (s *Service) removePermissionsFunc(ctx context.Context, groupID, ofgaType s } } -func (s *Service) removeMembersFunc(ctx context.Context, roleID string) func() { +func (s *Service) removeDirectAssociationsFunc(ctx context.Context, groupID, relation string) func() { return func() { - s.removeMembers(ctx, roleID) + s.removeDirectAssociations(ctx, groupID, relation) } } @@ -528,6 +539,10 @@ func (s *Service) permissionTypes() []string { return []string{"group", "role", "identity", "scheme", "provider", "client"} } +func (s *Service) directRelations() []string { + return []string{"privileged", "member", "can_create", "can_delete", "can_edit", "can_view"} +} + // NewService returns the implementtation of the business logic for the groups API func NewService(ofga OpenFGAClientInterface, wpool pool.WorkerPoolInterface, tracer trace.Tracer, monitor monitoring.MonitorInterface, logger logging.LoggerInterface) *Service { s := new(Service) diff --git a/pkg/groups/service_test.go b/pkg/groups/service_test.go index 41bb89da1..4c4441e0d 100644 --- a/pkg/groups/service_test.go +++ b/pkg/groups/service_test.go @@ -779,6 +779,7 @@ func TestServiceCreateGroup(t *testing.T) { ps, *ofga.NewTuple(fmt.Sprintf("user:%s", test.input.user), MEMBER_RELATION, fmt.Sprintf("group:%s", test.input.group)), *ofga.NewTuple(authorization.ADMIN_PRIVILEGE, "privileged", fmt.Sprintf("group:%s", test.input.group)), + *ofga.NewTuple(fmt.Sprintf("user:%s", test.input.user), CAN_VIEW_RELATION, fmt.Sprintf("group:%s", test.input.group)), ) if !reflect.DeepEqual(ps, tuples) { @@ -840,9 +841,10 @@ func TestServiceDeleteGroup(t *testing.T) { mockTracer.EXPECT().Start(gomock.Any(), "groups.Service.buildGroupMember").AnyTimes().Return(context.TODO(), trace.SpanFromContext(context.TODO())) mockTracer.EXPECT().Start(gomock.Any(), "groups.Service.DeleteGroup").Times(1).Return(context.TODO(), trace.SpanFromContext(context.TODO())) mockTracer.EXPECT().Start(gomock.Any(), "groups.Service.removePermissionsByType").Times(6).Return(context.TODO(), trace.SpanFromContext(context.TODO())) - mockTracer.EXPECT().Start(gomock.Any(), "roles.Service.removeMembers").Times(1).Return(context.TODO(), trace.SpanFromContext(context.TODO())) + mockTracer.EXPECT().Start(gomock.Any(), "groups.Service.removeDirectAssociations").Times(6).Return(context.TODO(), trace.SpanFromContext(context.TODO())) pTypes := []string{"role", "group", "identity", "scheme", "provider", "client"} + directRelations := []string{"privileged", "member", "can_create", "can_delete", "can_edit", "can_view"} calls := []*gomock.Call{} @@ -876,43 +878,45 @@ func TestServiceDeleteGroup(t *testing.T) { } - calls = append( - calls, - mockOpenFGA.EXPECT().ReadTuples(gomock.Any(), "", MEMBER_RELATION, fmt.Sprintf("group:%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 - } + for _, relation := range directRelations { + calls = append( + calls, + mockOpenFGA.EXPECT().ReadTuples(gomock.Any(), "", relation, fmt.Sprintf("group:%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", MEMBER_RELATION, object, + tuples := []openfga.Tuple{ + *openfga.NewTuple( + *openfga.NewTupleKey( + "user:test", MEMBER_RELATION, object, + ), + time.Now(), ), - time.Now(), - ), - *openfga.NewTuple( - *openfga.NewTupleKey( - "group:test#member", MEMBER_RELATION, object, + *openfga.NewTuple( + *openfga.NewTupleKey( + "group:test#member", MEMBER_RELATION, object, + ), + time.Now(), ), - time.Now(), - ), - } + } - r := new(client.ClientReadResponse) - r.SetContinuationToken("") - r.SetTuples(tuples) + r := new(client.ClientReadResponse) + r.SetContinuationToken("") + r.SetTuples(tuples) - return r, nil - }, - ), - ) + return r, nil + }, + ), + ) + } if test.expected == nil { mockOpenFGA.EXPECT().DeleteTuples( gomock.Any(), gomock.Any(), - ).Times(8).DoAndReturn( + ).Times(12).DoAndReturn( func(ctx context.Context, tuples ...ofga.Tuple) error { switch len(tuples) { case 1: @@ -952,18 +956,10 @@ func TestServiceDeleteGroup(t *testing.T) { } else { // TODO @shipperizer fix this so that we can pin it down to the error case only mockLogger.EXPECT().Errorf(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes() - mockOpenFGA.EXPECT().DeleteTuples( - gomock.Any(), - *ofga.NewTuple(authorization.ADMIN_PRIVILEGE, "privileged", fmt.Sprintf("group:%s", test.input)), - ).Return(test.expected) } - gomock.InAnyOrder(calls) - err := svc.DeleteGroup(context.Background(), test.input) + _ = svc.DeleteGroup(context.Background(), test.input) - if err != test.expected { - t.Errorf("expected error to be %v got %v", test.expected, err) - } }) } }