SMQ-2704 - Update listing of group hierarchy (#2710)

Signed-off-by: Felix Gateru <felix.gateru@gmail.com>

groups/groups.go

@@ -109,7 +109,7 @@ type Repository interface {
 	// RetrieveByIDs retrieves group by ids and query.
 	RetrieveByIDs(ctx context.Context, pm PageMeta, ids ...string) (Page, error)
 
-	RetrieveHierarchy(ctx context.Context, id string, hm HierarchyPageMeta) (HierarchyPage, error)
+	RetrieveHierarchy(ctx context.Context, domainID, userID, groupID string, hm HierarchyPageMeta) (HierarchyPage, error)
 
 	// ChangeStatus changes groups status to active or inactive
 	ChangeStatus(ctx context.Context, group Group) (Group, error)

groups/mocks/repository.go

@@ -1324,8 +1324,8 @@ func (_c *Repository_RetrieveEntityRole_Call) RunAndReturn(run func(ctx context.
 }
 
 // RetrieveHierarchy provides a mock function for the type Repository
-func (_mock *Repository) RetrieveHierarchy(ctx context.Context, id string, hm groups.HierarchyPageMeta) (groups.HierarchyPage, error) {
-	ret := _mock.Called(ctx, id, hm)
+func (_mock *Repository) RetrieveHierarchy(ctx context.Context, domainID string, userID string, groupID string, hm groups.HierarchyPageMeta) (groups.HierarchyPage, error) {
+	ret := _mock.Called(ctx, domainID, userID, groupID, hm)
 
 	if len(ret) == 0 {
 		panic("no return value specified for RetrieveHierarchy")
@@ -1333,16 +1333,16 @@ func (_mock *Repository) RetrieveHierarchy(ctx context.Context, id string, hm gr
 
 	var r0 groups.HierarchyPage
 	var r1 error
-	if returnFunc, ok := ret.Get(0).(func(context.Context, string, groups.HierarchyPageMeta) (groups.HierarchyPage, error)); ok {
-		return returnFunc(ctx, id, hm)
+	if returnFunc, ok := ret.Get(0).(func(context.Context, string, string, string, groups.HierarchyPageMeta) (groups.HierarchyPage, error)); ok {
+		return returnFunc(ctx, domainID, userID, groupID, hm)
 	}
-	if returnFunc, ok := ret.Get(0).(func(context.Context, string, groups.HierarchyPageMeta) groups.HierarchyPage); ok {
-		r0 = returnFunc(ctx, id, hm)
+	if returnFunc, ok := ret.Get(0).(func(context.Context, string, string, string, groups.HierarchyPageMeta) groups.HierarchyPage); ok {
+		r0 = returnFunc(ctx, domainID, userID, groupID, hm)
 	} else {
 		r0 = ret.Get(0).(groups.HierarchyPage)
 	}
-	if returnFunc, ok := ret.Get(1).(func(context.Context, string, groups.HierarchyPageMeta) error); ok {
-		r1 = returnFunc(ctx, id, hm)
+	if returnFunc, ok := ret.Get(1).(func(context.Context, string, string, string, groups.HierarchyPageMeta) error); ok {
+		r1 = returnFunc(ctx, domainID, userID, groupID, hm)
 	} else {
 		r1 = ret.Error(1)
 	}
@@ -1356,13 +1356,15 @@ type Repository_RetrieveHierarchy_Call struct {
 
 // RetrieveHierarchy is a helper method to define mock.On call
 //   - ctx context.Context
-//   - id string
+//   - domainID string
+//   - userID string
+//   - groupID string
 //   - hm groups.HierarchyPageMeta
-func (_e *Repository_Expecter) RetrieveHierarchy(ctx interface{}, id interface{}, hm interface{}) *Repository_RetrieveHierarchy_Call {
-	return &Repository_RetrieveHierarchy_Call{Call: _e.mock.On("RetrieveHierarchy", ctx, id, hm)}
+func (_e *Repository_Expecter) RetrieveHierarchy(ctx interface{}, domainID interface{}, userID interface{}, groupID interface{}, hm interface{}) *Repository_RetrieveHierarchy_Call {
+	return &Repository_RetrieveHierarchy_Call{Call: _e.mock.On("RetrieveHierarchy", ctx, domainID, userID, groupID, hm)}
 }
 
-func (_c *Repository_RetrieveHierarchy_Call) Run(run func(ctx context.Context, id string, hm groups.HierarchyPageMeta)) *Repository_RetrieveHierarchy_Call {
+func (_c *Repository_RetrieveHierarchy_Call) Run(run func(ctx context.Context, domainID string, userID string, groupID string, hm groups.HierarchyPageMeta)) *Repository_RetrieveHierarchy_Call {
 	_c.Call.Run(func(args mock.Arguments) {
 		var arg0 context.Context
 		if args[0] != nil {
@@ -1372,14 +1374,24 @@ func (_c *Repository_RetrieveHierarchy_Call) Run(run func(ctx context.Context, i
 		if args[1] != nil {
 			arg1 = args[1].(string)
 		}
-		var arg2 groups.HierarchyPageMeta
+		var arg2 string
 		if args[2] != nil {
-			arg2 = args[2].(groups.HierarchyPageMeta)
+			arg2 = args[2].(string)
+		}
+		var arg3 string
+		if args[3] != nil {
+			arg3 = args[3].(string)
+		}
+		var arg4 groups.HierarchyPageMeta
+		if args[4] != nil {
+			arg4 = args[4].(groups.HierarchyPageMeta)
 		}
 		run(
 			arg0,
 			arg1,
 			arg2,
+			arg3,
+			arg4,
 		)
 	})
 	return _c
@@ -1390,7 +1402,7 @@ func (_c *Repository_RetrieveHierarchy_Call) Return(hierarchyPage groups.Hierarc
 	return _c
 }
 
-func (_c *Repository_RetrieveHierarchy_Call) RunAndReturn(run func(ctx context.Context, id string, hm groups.HierarchyPageMeta) (groups.HierarchyPage, error)) *Repository_RetrieveHierarchy_Call {
+func (_c *Repository_RetrieveHierarchy_Call) RunAndReturn(run func(ctx context.Context, domainID string, userID string, groupID string, hm groups.HierarchyPageMeta) (groups.HierarchyPage, error)) *Repository_RetrieveHierarchy_Call {
 	_c.Call.Return(run)
 	return _c
 }

groups/postgres/groups.go

@@ -494,60 +494,57 @@ func (repo groupRepository) RetrieveByIDs(ctx context.Context, pm groups.PageMet
 	return page, nil
 }
 
-func (repo groupRepository) RetrieveHierarchy(ctx context.Context, id string, hm groups.HierarchyPageMeta) (groups.HierarchyPage, error) {
-	query := ""
+func (repo groupRepository) RetrieveHierarchy(ctx context.Context, domainID, userID, groupID string, hm groups.HierarchyPageMeta) (groups.HierarchyPage, error) {
+	var dirQuery string
 	switch {
-	// ancestors
 	case hm.Direction >= 0:
-		query = `
-		SELECT
-			g.id,
-			COALESCE(g.parent_id, '') AS parent_id,
-			g.domain_id,
-			g.name,
-			g.description,
-			g.tags,
-			g.metadata,
-			g.created_at,
-			g.updated_at,
-			g.updated_by,
-			g.status,
-			g.path,
-			nlevel(g.path) AS level
-		FROM
-			groups g
-		WHERE
-			g.path @> (SELECT path FROM groups WHERE id = :id LIMIT 1);
-		`
-	// descendants
-	case hm.Direction < 0:
-		fallthrough
+		dirQuery = "g.path @> (SELECT path FROM groups WHERE id = :id)"
 	default:
-		query = `
-		SELECT
-			g.id,
-			COALESCE(g.parent_id, '') AS parent_id,
-			g.domain_id,
-			g.name,
-			g.tags,
-			g.description,
-			g.metadata,
-			g.created_at,
-			g.updated_at,
-			g.updated_by,
-			g.status,
-			g.path,
-			nlevel(g.path) AS level
-		FROM
-			groups g
-		WHERE
-			g.path <@ (SELECT path FROM groups WHERE id = :id LIMIT 1);
-		`
+		dirQuery = "g.path <@ (SELECT path FROM groups WHERE id = :id)"
 	}
+
+	baseQuery := repo.userGroupsBaseQuery(domainID, userID)
+	query := fmt.Sprintf(`%s,
+		target_hierarchy AS (
+			SELECT
+				g.id,
+				g.parent_id,
+				g.domain_id,
+				g.name,
+				g.tags,
+				g.description,
+				g.metadata,
+				g.created_at,
+				g.updated_at,
+				g.updated_by,
+				g.status,
+				g.path,
+				nlevel(g.path) AS level
+			FROM
+				groups g
+			WHERE
+				%s
+		),
+		filtered_hierarchy AS (
+			SELECT
+				th.*
+			FROM
+				target_hierarchy th
+			JOIN
+				final_groups fg ON th.id = fg.id
+		)
+		SELECT
+			*
+		FROM
+			filtered_hierarchy
+		ORDER BY path;
+		`, baseQuery, dirQuery)
+
 	parameters := map[string]interface{}{
-		"id":    id,
+		"id":    groupID,
 		"level": hm.Level,
 	}
+
 	rows, err := repo.db.NamedQueryContext(ctx, query, parameters)
 	if err != nil {
 		return groups.HierarchyPage{}, errors.Wrap(repoerr.ErrFailedToRetrieveAllGroups, err)

groups/postgres/groups_test.go

@@ -1509,6 +1509,8 @@ func TestRetrieveHierarchy(t *testing.T) {
 
 	repo := postgres.New(database)
 
+	userID := testsutil.GenerateUUID(t)
+	domainID := testsutil.GenerateUUID(t)
 	num := 10
 
 	var items []groups.Group
@@ -1517,7 +1519,7 @@ func TestRetrieveHierarchy(t *testing.T) {
 		name := namegen.Generate()
 		group := groups.Group{
 			ID:          testsutil.GenerateUUID(t),
-			Domain:      testsutil.GenerateUUID(t),
+			Domain:      domainID,
 			Parent:      parentID,
 			Name:        name,
 			Description: desc,
@@ -1527,6 +1529,21 @@ func TestRetrieveHierarchy(t *testing.T) {
 		}
 		_, err := repo.Save(context.Background(), group)
 		require.Nil(t, err, fmt.Sprintf("create group unexpected error: %s", err))
+		newRolesProvision := []roles.RoleProvision{
+			{
+				Role: roles.Role{
+					ID:        testsutil.GenerateUUID(t) + "_" + group.ID,
+					Name:      "admin",
+					EntityID:  group.ID,
+					CreatedAt: validTimestamp,
+					CreatedBy: userID,
+				},
+				OptionalActions: availableActions,
+				OptionalMembers: []string{userID},
+			},
+		}
+		_, err = repo.AddRoles(context.Background(), newRolesProvision)
+		require.Nil(t, err, fmt.Sprintf("add roles unexpected error: %s", err))
 		items = append(items, group)
 		if i == 0 {
 			parentID = group.ID
@@ -1534,15 +1551,19 @@ func TestRetrieveHierarchy(t *testing.T) {
 	}
 
 	cases := []struct {
-		desc string
-		id   string
-		hm   groups.HierarchyPageMeta
-		resp groups.HierarchyPage
-		err  error
+		desc     string
+		groupID  string
+		userID   string
+		domainID string
+		hm       groups.HierarchyPageMeta
+		resp     groups.HierarchyPage
+		err      error
 	}{
 		{
-			desc: "retrieve ancestors successfully",
-			id:   items[1].ID,
+			desc:     "retrieve ancestors successfully",
+			groupID:  items[1].ID,
+			userID:   userID,
+			domainID: domainID,
 			hm: groups.HierarchyPageMeta{
 				Level:     1,
 				Direction: +1,
@@ -1559,8 +1580,10 @@ func TestRetrieveHierarchy(t *testing.T) {
 			err: nil,
 		},
 		{
-			desc: "retrieve descendants successfully",
-			id:   items[0].ID,
+			desc:     "retrieve descendants successfully",
+			groupID:  items[0].ID,
+			userID:   userID,
+			domainID: domainID,
 			hm: groups.HierarchyPageMeta{
 				Level:     1,
 				Direction: -1,
@@ -1577,20 +1600,64 @@ func TestRetrieveHierarchy(t *testing.T) {
 			err: nil,
 		},
 		{
-			desc: "retrieve hierarchy with invalid ID",
-			id:   testsutil.GenerateUUID(t),
-			err:  nil,
+			desc:     "retrieve hierarchy with invalid ID",
+			groupID:  testsutil.GenerateUUID(t),
+			userID:   userID,
+			domainID: domainID,
+			err:      nil,
 		},
 		{
-			desc: "retrieve hierarchy with empty ID",
-			id:   "",
-			err:  nil,
+			desc:     "retrieve hierarchy with empty ID",
+			groupID:  "",
+			userID:   userID,
+			domainID: domainID,
+			err:      nil,
+		},
+		{
+			desc:     "retrieve hierarchy with invalid domain ID",
+			groupID:  items[0].ID,
+			userID:   userID,
+			domainID: testsutil.GenerateUUID(t),
+			hm: groups.HierarchyPageMeta{
+				Level:     1,
+				Direction: -1,
+				Tree:      false,
+			},
+			resp: groups.HierarchyPage{
+				Groups: []groups.Group(nil),
+				HierarchyPageMeta: groups.HierarchyPageMeta{
+					Level:     1,
+					Direction: -1,
+					Tree:      false,
+				},
+			},
+			err: nil,
+		},
+		{
+			desc:     "retrieve hierarchy with invalid user ID",
+			groupID:  items[0].ID,
+			userID:   testsutil.GenerateUUID(t),
+			domainID: domainID,
+			hm: groups.HierarchyPageMeta{
+				Level:     1,
+				Direction: -1,
+				Tree:      false,
+			},
+			resp: groups.HierarchyPage{
+				Groups: []groups.Group(nil),
+				HierarchyPageMeta: groups.HierarchyPageMeta{
+					Level:     1,
+					Direction: -1,
+					Tree:      false,
+				},
+			},
+			err: nil,
 		},
 	}
 
 	for _, tc := range cases {
 		t.Run(tc.desc, func(t *testing.T) {
-			gpPage, err := repo.RetrieveHierarchy(context.Background(), tc.id, tc.hm)
+			gpPage, err := repo.RetrieveHierarchy(context.Background(), tc.domainID, tc.userID, tc.groupID, tc.hm)
 			assert.True(t, errors.Contains(err, tc.err), fmt.Sprintf("%s: expected %s got %s\n", tc.desc, tc.err, err))
 			if err == nil {
 				got := stripGroupDetails(gpPage.Groups)

groups/service.go

@@ -199,53 +199,13 @@ func (svc service) DisableGroup(ctx context.Context, session smqauthn.Session, i
 }
 
 func (svc service) RetrieveGroupHierarchy(ctx context.Context, session smqauthn.Session, id string, hm HierarchyPageMeta) (HierarchyPage, error) {
-	hp, err := svc.repo.RetrieveHierarchy(ctx, id, hm)
+	hp, err := svc.repo.RetrieveHierarchy(ctx, session.DomainID, session.UserID, id, hm)
 	if err != nil {
 		return HierarchyPage{}, errors.Wrap(svcerr.ErrViewEntity, err)
 	}
-	hids := svc.getGroupIDs(hp.Groups)
-	ids, err := svc.filterAllowedGroupIDsOfUserID(ctx, session.DomainUserID, "read_permission", hids)
-	if err != nil {
-		return HierarchyPage{}, errors.Wrap(svcerr.ErrViewEntity, err)
-	}
-	hp.Groups = svc.allowedGroups(hp.Groups, ids)
 	return hp, nil
 }
 
-func (svc service) allowedGroups(gps []Group, ids []string) []Group {
-	aIDs := make(map[string]struct{}, len(ids))
-
-	for _, id := range ids {
-		aIDs[id] = struct{}{}
-	}
-
-	aGroups := []Group{}
-	for _, g := range gps {
-		ag := g
-		if _, ok := aIDs[g.ID]; !ok {
-			ag = Group{ID: "xxxx-xxxx-xxxx-xxxx", Level: g.Level}
-		}
-		aGroups = append(aGroups, ag)
-	}
-	return aGroups
-}
-
-func (svc service) getGroupIDs(gps []Group) []string {
-	hids := []string{}
-	for _, g := range gps {
-		hids = append(hids, g.ID)
-		if len(g.Children) > 0 {
-			children := make([]Group, len(g.Children))
-			for i, child := range g.Children {
-				children[i] = *child
-			}
-			cids := svc.getGroupIDs(children)
-			hids = append(hids, cids...)
-		}
-	}
-	return hids
-}
-
 func (svc service) AddParentGroup(ctx context.Context, session smqauthn.Session, id, parentID string) (retErr error) {
 	group, err := svc.repo.RetrieveByID(ctx, id)
 	if err != nil {
@@ -486,36 +446,6 @@ func (svc service) DeleteGroup(ctx context.Context, session smqauthn.Session, id
 	return nil
 }
 
-func (svc service) filterAllowedGroupIDsOfUserID(ctx context.Context, userID, permission string, groupIDs []string) ([]string, error) {
-	var ids []string
-	allowedIDs, err := svc.listAllGroupsOfUserID(ctx, userID, permission)
-	if err != nil {
-		return []string{}, err
-	}
-
-	for _, gid := range groupIDs {
-		for _, id := range allowedIDs {
-			if id == gid {
-				ids = append(ids, id)
-			}
-		}
-	}
-	return ids, nil
-}
-
-func (svc service) listAllGroupsOfUserID(ctx context.Context, userID, permission string) ([]string, error) {
-	allowedIDs, err := svc.policy.ListAllObjects(ctx, policies.Policy{
-		SubjectType: policies.UserType,
-		Subject:     userID,
-		Permission:  permission,
-		ObjectType:  policies.GroupType,
-	})
-	if err != nil {
-		return []string{}, err
-	}
-	return allowedIDs.Policies, nil
-}
-
 func (svc service) changeGroupStatus(ctx context.Context, session smqauthn.Session, group Group) (Group, error) {
 	dbGroup, err := svc.repo.RetrieveByID(ctx, group.ID)
 	if err != nil {

groups/service_test.go

@@ -664,8 +664,6 @@ func TestRetrieveGroupHierarchy(t *testing.T) {
 		pageMeta             groups.HierarchyPageMeta
 		retrieveHierarchyRes groups.HierarchyPage
 		retrieveHierarchyErr error
-		listAllObjectsRes    policysvc.PolicyPage
-		listAllObjectsErr    error
 		err                  error
 	}{
 		{
@@ -684,9 +682,6 @@ func TestRetrieveGroupHierarchy(t *testing.T) {
 				},
 				Groups: []groups.Group{parentGroup},
 			},
-			listAllObjectsRes: policysvc.PolicyPage{
-				Policies: []string{parentGroupID, childGroupID},
-			},
 			err: nil,
 		},
 		{
@@ -701,64 +696,28 @@ func TestRetrieveGroupHierarchy(t *testing.T) {
 			err:                  repoerr.ErrNotFound,
 		},
 		{
-			desc: "retrieve group hierarchy with failed to list all objects",
-			id:   parentGroup.ID,
+			desc: "retrieve group hierarchy with invalid group ID",
+			id:   testsutil.GenerateUUID(t),
 			pageMeta: groups.HierarchyPageMeta{
 				Level:     1,
 				Direction: -1,
 				Tree:      false,
 			},
-			retrieveHierarchyRes: groups.HierarchyPage{
-				HierarchyPageMeta: groups.HierarchyPageMeta{
-					Level:     1,
-					Direction: -1,
-					Tree:      false,
-				},
-				Groups: []groups.Group{parentGroup},
-			},
-			listAllObjectsErr: svcerr.ErrAuthorization,
-			err:               svcerr.ErrAuthorization,
-		},
-		{
-			desc: "retrieve group hierarchy for group not allowed for user",
-			id:   parentGroup.ID,
-			pageMeta: groups.HierarchyPageMeta{
-				Level:     1,
-				Direction: -1,
-				Tree:      false,
-			},
-			retrieveHierarchyRes: groups.HierarchyPage{
-				HierarchyPageMeta: groups.HierarchyPageMeta{
-					Level:     1,
-					Direction: -1,
-					Tree:      false,
-				},
-				Groups: []groups.Group{parentGroup},
-			},
-			listAllObjectsRes: policysvc.PolicyPage{
-				Policies: []string{testsutil.GenerateUUID(t)},
-			},
-			err: nil,
+			retrieveHierarchyErr: nil,
+			err:                  nil,
 		},
 	}
 
 	for _, tc := range cases {
 		t.Run(tc.desc, func(t *testing.T) {
-			repoCall := repo.On("RetrieveHierarchy", context.Background(), tc.id, tc.pageMeta).Return(tc.retrieveHierarchyRes, tc.retrieveHierarchyErr)
-			policyCall := policies.On("ListAllObjects", context.Background(), policysvc.Policy{
-				SubjectType: policysvc.UserType,
-				Subject:     validID,
-				Permission:  "read_permission",
-				ObjectType:  policysvc.GroupType,
-			}).Return(tc.listAllObjectsRes, tc.listAllObjectsErr)
+			repoCall := repo.On("RetrieveHierarchy", context.Background(), validSession.DomainID, validSession.UserID, tc.id, tc.pageMeta).Return(tc.retrieveHierarchyRes, tc.retrieveHierarchyErr)
 			_, err := svc.RetrieveGroupHierarchy(context.Background(), validSession, tc.id, tc.pageMeta)
 			assert.True(t, errors.Contains(err, tc.err), fmt.Sprintf("expected error %v to contain %v", err, tc.err))
 			if tc.err == nil {
-				ok := repo.AssertCalled(t, "RetrieveHierarchy", context.Background(), tc.id, tc.pageMeta)
+				ok := repo.AssertCalled(t, "RetrieveHierarchy", context.Background(), validSession.DomainID, validSession.UserID, tc.id, tc.pageMeta)
 				assert.True(t, ok, fmt.Sprintf("RetrieveHierarchy was not called on %s", tc.desc))
 			}
 			repoCall.Unset()
-			policyCall.Unset()
 		})
 	}
 }