Skip to content

Commit

Permalink
Merge pull request #300 from canonical/IAM-867-fix-groups-handlers
Browse files Browse the repository at this point in the history 
IAM 867 fix groups handlers
  • Loading branch information
@shipperizer
shipperizer committed Apr 30, 2024
2 parents e7b52dd + 240be82 commit c8ad0b5
Show file tree
Hide file tree
Showing 3 changed files with 81 additions and 59 deletions.
15 changes: 13 additions & 2 deletions pkg/groups/handlers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand All @@ -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,
},
Expand Down
55 changes: 35 additions & 20 deletions pkg/groups/service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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{}
Expand All @@ -311,19 +320,22 @@ 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()

// 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
Expand Down Expand Up @@ -458,38 +470,37 @@ 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
if cToken = r.ContinuationToken; cToken != "" {
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())
}
}
Expand Down Expand Up @@ -518,16 +529,20 @@ 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)
}
}

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)
Expand Down
70 changes: 33 additions & 37 deletions pkg/groups/service_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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{}

Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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)
}
})
}
}
Expand Down

0 comments on commit c8ad0b5

Please sign in to comment.