Skip to content

Commit

Permalink
Modify group managers to skip fetching members when specified
Browse files Browse the repository at this point in the history
  • Loading branch information
@ishank011
ishank011 committed Oct 26, 2021
1 parent 3d4e609 commit e86bced
Show file tree
Hide file tree
Showing 11 changed files with 112 additions and 46 deletions.
3 changes: 3 additions & 0 deletions changelog/unreleased/fetch-groups-members-config.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Enhancement: Modify group and user managers to skip fetching specified metadata

https://github.com/cs3org/reva/pull/2205
6 changes: 3 additions & 3 deletions internal/grpc/services/groupprovider/groupprovider.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ func (s *service) Register(ss *grpc.Server) {
}

func (s *service) GetGroup(ctx context.Context, req *grouppb.GetGroupRequest) (*grouppb.GetGroupResponse, error) {
group, err := s.groupmgr.GetGroup(ctx, req.GroupId)
group, err := s.groupmgr.GetGroup(ctx, req.GroupId, req.SkipFetchingMembers)
if err != nil {
err = errors.Wrap(err, "groupprovidersvc: error getting group")
return &grouppb.GetGroupResponse{
Expand All @@ -116,7 +116,7 @@ func (s *service) GetGroup(ctx context.Context, req *grouppb.GetGroupRequest) (*
}

func (s *service) GetGroupByClaim(ctx context.Context, req *grouppb.GetGroupByClaimRequest) (*grouppb.GetGroupByClaimResponse, error) {
group, err := s.groupmgr.GetGroupByClaim(ctx, req.Claim, req.Value)
group, err := s.groupmgr.GetGroupByClaim(ctx, req.Claim, req.Value, req.SkipFetchingMembers)
if err != nil {
err = errors.Wrap(err, "groupprovidersvc: error getting group by claim")
return &grouppb.GetGroupByClaimResponse{
Expand All @@ -131,7 +131,7 @@ func (s *service) GetGroupByClaim(ctx context.Context, req *grouppb.GetGroupByCl
}

func (s *service) FindGroups(ctx context.Context, req *grouppb.FindGroupsRequest) (*grouppb.FindGroupsResponse, error) {
groups, err := s.groupmgr.FindGroups(ctx, req.Filter)
groups, err := s.groupmgr.FindGroups(ctx, req.Filter, req.SkipFetchingMembers)
if err != nil {
err = errors.Wrap(err, "groupprovidersvc: error finding groups")
return &grouppb.FindGroupsResponse{
Expand Down
4 changes: 2 additions & 2 deletions internal/http/services/owncloud/ocs/handlers/apps/sharing/sharees/sharees.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ func (h *Handler) FindSharees(w http.ResponseWriter, r *http.Request) {
response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error getting gateway grpc client", err)
return
}
usersRes, err := gwc.FindUsers(r.Context(), &userpb.FindUsersRequest{Filter: term})
usersRes, err := gwc.FindUsers(r.Context(), &userpb.FindUsersRequest{Filter: term, SkipFetchingUserGroups: true})
if err != nil {
response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error searching users", err)
return
Expand All @@ -73,7 +73,7 @@ func (h *Handler) FindSharees(w http.ResponseWriter, r *http.Request) {
userMatches = append(userMatches, match)
}

groupsRes, err := gwc.FindGroups(r.Context(), &grouppb.FindGroupsRequest{Filter: term})
groupsRes, err := gwc.FindGroups(r.Context(), &grouppb.FindGroupsRequest{Filter: term, SkipFetchingMembers: true})
if err != nil {
response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error searching groups", err)
return
Expand Down
5 changes: 3 additions & 2 deletions internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/group.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,9 @@ func (h *Handler) createGroupShare(w http.ResponseWriter, r *http.Request, statI
}

groupRes, err := c.GetGroupByClaim(ctx, &grouppb.GetGroupByClaimRequest{
Claim: "group_name",
Value: shareWith,
Claim: "group_name",
Value: shareWith,
SkipFetchingMembers: true,
})
if err != nil {
response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error searching recipient", err)
Expand Down
2 changes: 2 additions & 0 deletions internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -897,6 +897,7 @@ func (h *Handler) mustGetIdentifiers(ctx context.Context, client gateway.Gateway
GroupId: &grouppb.GroupId{
OpaqueId: id,
},
SkipFetchingMembers: true,
})
if err != nil {
sublog.Err(err).Msg("could not look up group")
Expand Down Expand Up @@ -926,6 +927,7 @@ func (h *Handler) mustGetIdentifiers(ctx context.Context, client gateway.Gateway
UserId: &userpb.UserId{
OpaqueId: id,
},
SkipFetchingUserGroups: true,
})
if err != nil {
sublog.Err(err).Msg("could not look up user")
Expand Down
5 changes: 3 additions & 2 deletions internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/user.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,9 @@ func (h *Handler) createUserShare(w http.ResponseWriter, r *http.Request, statIn
}

userRes, err := c.GetUserByClaim(ctx, &userpb.GetUserByClaimRequest{
Claim: "username",
Value: shareWith,
Claim: "username",
Value: shareWith,
SkipFetchingUserGroups: true,
})
if err != nil {
response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error searching recipient", err)
Expand Down
42 changes: 28 additions & 14 deletions pkg/cbox/group/rest/rest.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ func (m *manager) parseAndCacheGroup(ctx context.Context, groupData map[string]i

}

func (m *manager) GetGroup(ctx context.Context, gid *grouppb.GroupId) (*grouppb.Group, error) {
func (m *manager) GetGroup(ctx context.Context, gid *grouppb.GroupId, skipFetchingMembers bool) (*grouppb.Group, error) {
g, err := m.fetchCachedGroupDetails(gid)
if err != nil {
groupData, err := m.getGroupByParam(ctx, "groupIdentifier", gid.OpaqueId)
Expand All @@ -202,20 +202,22 @@ func (m *manager) GetGroup(ctx context.Context, gid *grouppb.GroupId) (*grouppb.
g = m.parseAndCacheGroup(ctx, groupData)
}

groupMembers, err := m.GetMembers(ctx, gid)
if err != nil {
return nil, err
if !skipFetchingMembers {
groupMembers, err := m.GetMembers(ctx, gid)
if err != nil {
return nil, err
}
g.Members = groupMembers
}
g.Members = groupMembers

return g, nil
}

func (m *manager) GetGroupByClaim(ctx context.Context, claim, value string) (*grouppb.Group, error) {
func (m *manager) GetGroupByClaim(ctx context.Context, claim, value string, skipFetchingMembers bool) (*grouppb.Group, error) {
value = url.QueryEscape(value)
opaqueID, err := m.fetchCachedParam(claim, value)
if err == nil {
return m.GetGroup(ctx, &grouppb.GroupId{OpaqueId: opaqueID})
return m.GetGroup(ctx, &grouppb.GroupId{OpaqueId: opaqueID}, skipFetchingMembers)
}

switch claim {
Expand All @@ -236,17 +238,19 @@ func (m *manager) GetGroupByClaim(ctx context.Context, claim, value string) (*gr
}
g := m.parseAndCacheGroup(ctx, groupData)

groupMembers, err := m.GetMembers(ctx, g.Id)
if err != nil {
return nil, err
if !skipFetchingMembers {
groupMembers, err := m.GetMembers(ctx, g.Id)
if err != nil {
return nil, err
}
g.Members = groupMembers
}
g.Members = groupMembers

return g, nil

}

func (m *manager) findGroupsByFilter(ctx context.Context, url string, groups map[string]*grouppb.Group) error {
func (m *manager) findGroupsByFilter(ctx context.Context, url string, groups map[string]*grouppb.Group, skipFetchingMembers bool) error {

groupData, err := m.apiTokenManager.SendAPIGetRequest(ctx, url, false)
if err != nil {
Expand All @@ -265,23 +269,33 @@ func (m *manager) findGroupsByFilter(ctx context.Context, url string, groups map
OpaqueId: id,
Idp: m.conf.IDProvider,
}

var groupMembers []*userpb.UserId
if !skipFetchingMembers {
groupMembers, err = m.GetMembers(ctx, groupID)
if err != nil {
return err
}
}
gid, ok := grpInfo["gid"].(int64)
if !ok {
gid = 0
}

groups[groupID.OpaqueId] = &grouppb.Group{
Id: groupID,
GroupName: id,
Mail: id + "@cern.ch",
DisplayName: name,
GidNumber: gid,
Members: groupMembers,
}
}

return nil
}

func (m *manager) FindGroups(ctx context.Context, query string) ([]*grouppb.Group, error) {
func (m *manager) FindGroups(ctx context.Context, query string, skipFetchingMembers bool) ([]*grouppb.Group, error) {

// Look at namespaces filters. If the query starts with:
// "a" or none => get egroups
Expand All @@ -308,7 +322,7 @@ func (m *manager) FindGroups(ctx context.Context, query string) ([]*grouppb.Grou
for _, f := range filters {
url := fmt.Sprintf("%s/Group/?filter=%s:contains:%s&field=groupIdentifier&field=displayName&field=gid",
m.conf.APIBaseURL, f, url.QueryEscape(query))
err := m.findGroupsByFilter(ctx, url, groups)
err := m.findGroupsByFilter(ctx, url, groups, skipFetchingMembers)
if err != nil {
return nil, err
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/group/group.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ import (

// Manager is the interface to implement to manipulate groups.
type Manager interface {
GetGroup(ctx context.Context, gid *grouppb.GroupId) (*grouppb.Group, error)
GetGroupByClaim(ctx context.Context, claim, value string) (*grouppb.Group, error)
FindGroups(ctx context.Context, query string) ([]*grouppb.Group, error)
GetGroup(ctx context.Context, gid *grouppb.GroupId, skipFetchingMembers bool) (*grouppb.Group, error)
GetGroupByClaim(ctx context.Context, claim, value string, skipFetchingMembers bool) (*grouppb.Group, error)
FindGroups(ctx context.Context, query string, skipFetchingMembers bool) ([]*grouppb.Group, error)
GetMembers(ctx context.Context, gid *grouppb.GroupId) ([]*userpb.UserId, error)
HasMember(ctx context.Context, gid *grouppb.GroupId, uid *userpb.UserId) (bool, error)
}
24 changes: 18 additions & 6 deletions pkg/group/manager/json/json.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,19 +87,27 @@ func New(m map[string]interface{}) (group.Manager, error) {
}, nil
}

func (m *manager) GetGroup(ctx context.Context, gid *grouppb.GroupId) (*grouppb.Group, error) {
func (m *manager) GetGroup(ctx context.Context, gid *grouppb.GroupId, skipFetchingMembers bool) (*grouppb.Group, error) {
for _, g := range m.groups {
if g.Id.GetOpaqueId() == gid.OpaqueId || g.GroupName == gid.OpaqueId {
return g, nil
group := *g
if skipFetchingMembers {
group.Members = nil
}
return &group, nil
}
}
return nil, errtypes.NotFound(gid.OpaqueId)
}

func (m *manager) GetGroupByClaim(ctx context.Context, claim, value string) (*grouppb.Group, error) {
func (m *manager) GetGroupByClaim(ctx context.Context, claim, value string, skipFetchingMembers bool) (*grouppb.Group, error) {
for _, g := range m.groups {
if groupClaim, err := extractClaim(g, claim); err == nil && value == groupClaim {
return g, nil
group := *g
if skipFetchingMembers {
group.Members = nil
}
return &group, nil
}
}
return nil, errtypes.NotFound(value)
Expand All @@ -119,11 +127,15 @@ func extractClaim(g *grouppb.Group, claim string) (string, error) {
return "", errors.New("json: invalid field")
}

func (m *manager) FindGroups(ctx context.Context, query string) ([]*grouppb.Group, error) {
func (m *manager) FindGroups(ctx context.Context, query string, skipFetchingMembers bool) ([]*grouppb.Group, error) {
groups := []*grouppb.Group{}
for _, g := range m.groups {
if groupContains(g, query) {
groups = append(groups, g)
group := *g
if skipFetchingMembers {
group.Members = nil
}
groups = append(groups, &group)
}
}
return groups, nil
Expand Down
23 changes: 18 additions & 5 deletions pkg/group/manager/json/json_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,30 +102,43 @@ func TestUserManager(t *testing.T) {
DisplayName: "Sailing Lovers",
Members: members,
}
groupWithoutMembers := &grouppb.Group{
Id: gid,
GroupName: "sailing-lovers",
Mail: "[email protected]",
GidNumber: 1234,
DisplayName: "Sailing Lovers",
}
groupFake := &grouppb.GroupId{OpaqueId: "fake-group"}

// positive test GetGroup
resGroup, _ := manager.GetGroup(ctx, gid)
resGroup, _ := manager.GetGroup(ctx, gid, false)
if !reflect.DeepEqual(resGroup, group) {
t.Fatalf("group differs: expected=%v got=%v", group, resGroup)
}

// positive test GetGroup without members
resGroupWithoutMembers, _ := manager.GetGroup(ctx, gid, true)
if !reflect.DeepEqual(resGroupWithoutMembers, groupWithoutMembers) {
t.Fatalf("group differs: expected=%v got=%v", groupWithoutMembers, resGroupWithoutMembers)
}

// negative test GetGroup
expectedErr := errtypes.NotFound(groupFake.OpaqueId)
_, err = manager.GetGroup(ctx, groupFake)
_, err = manager.GetGroup(ctx, groupFake, false)
if !reflect.DeepEqual(err, expectedErr) {
t.Fatalf("group not found error differ: expected='%v' got='%v'", expectedErr, err)
}

// positive test GetGroupByClaim by mail
resGroupByEmail, _ := manager.GetGroupByClaim(ctx, "mail", "[email protected]")
resGroupByEmail, _ := manager.GetGroupByClaim(ctx, "mail", "[email protected]", false)
if !reflect.DeepEqual(resGroupByEmail, group) {
t.Fatalf("group differs: expected=%v got=%v", group, resGroupByEmail)
}

// negative test GetGroupByClaim by mail
expectedErr = errtypes.NotFound("[email protected]")
_, err = manager.GetGroupByClaim(ctx, "mail", "[email protected]")
_, err = manager.GetGroupByClaim(ctx, "mail", "[email protected]", false)
if !reflect.DeepEqual(err, expectedErr) {
t.Fatalf("group not found error differs: expected='%v' got='%v'", expectedErr, err)
}
Expand All @@ -149,7 +162,7 @@ func TestUserManager(t *testing.T) {
}

// test FindGroups
resFind, _ := manager.FindGroups(ctx, "sail")
resFind, _ := manager.FindGroups(ctx, "sail", false)
if len(resFind) != 1 {
t.Fatalf("too many groups found: expected=%d got=%d", 1, len(resFind))
}
Expand Down
Loading

0 comments on commit e86bced

Please sign in to comment.