From 196b9c5683f7c561b797d2fed6599be9f8711374 Mon Sep 17 00:00:00 2001 From: Ishank Arora Date: Tue, 26 Oct 2021 10:21:28 +0200 Subject: [PATCH 1/4] Modify user managers to skip fetching groups when specified --- examples/plugin/json/json.go | 26 ++++++--- go.mod | 1 + go.sum | 2 + .../services/userprovider/userprovider.go | 6 +-- pkg/cbox/user/rest/rest.go | 41 +++++++++----- pkg/user/manager/demo/demo.go | 26 ++++++--- pkg/user/manager/demo/demo_test.go | 53 ++++++++++++------- pkg/user/manager/json/json.go | 26 ++++++--- pkg/user/manager/json/json_test.go | 30 +++++++---- pkg/user/manager/ldap/ldap.go | 39 +++++++++----- pkg/user/manager/nextcloud/nextcloud.go | 6 +-- pkg/user/manager/nextcloud/nextcloud_test.go | 7 +-- pkg/user/manager/owncloudsql/owncloudsql.go | 23 ++++---- pkg/user/rpc_user.go | 35 ++++++------ pkg/user/user.go | 6 +-- 15 files changed, 214 insertions(+), 113 deletions(-) diff --git a/examples/plugin/json/json.go b/examples/plugin/json/json.go index ce9cd59c54..fc3f8beb4c 100644 --- a/examples/plugin/json/json.go +++ b/examples/plugin/json/json.go @@ -81,20 +81,28 @@ func (m *Manager) Configure(ml map[string]interface{}) error { } // GetUser returns the user based on the uid. -func (m *Manager) GetUser(ctx context.Context, uid *userpb.UserId) (*userpb.User, error) { +func (m *Manager) GetUser(ctx context.Context, uid *userpb.UserId, skipFetchingGroups bool) (*userpb.User, error) { for _, u := range m.users { if (u.Id.GetOpaqueId() == uid.OpaqueId || u.Username == uid.OpaqueId) && (uid.Idp == "" || uid.Idp == u.Id.GetIdp()) { - return u, nil + user := *u + if skipFetchingGroups { + user.Groups = nil + } + return &user, nil } } return nil, nil } // GetUserByClaim returns user based on the claim -func (m *Manager) GetUserByClaim(ctx context.Context, claim, value string) (*userpb.User, error) { +func (m *Manager) GetUserByClaim(ctx context.Context, claim, value string, skipFetchingGroups bool) (*userpb.User, error) { for _, u := range m.users { if userClaim, err := extractClaim(u, claim); err == nil && value == userClaim { - return u, nil + user := *u + if skipFetchingGroups { + user.Groups = nil + } + return &user, nil } } return nil, errtypes.NotFound(value) @@ -126,11 +134,15 @@ func userContains(u *userpb.User, query string) bool { } // FindUsers returns the user based on the query -func (m *Manager) FindUsers(ctx context.Context, query string) ([]*userpb.User, error) { +func (m *Manager) FindUsers(ctx context.Context, query string, skipFetchingGroups bool) ([]*userpb.User, error) { users := []*userpb.User{} for _, u := range m.users { if userContains(u, query) { - users = append(users, u) + user := *u + if skipFetchingGroups { + user.Groups = nil + } + users = append(users, &user) } } return users, nil @@ -138,7 +150,7 @@ func (m *Manager) FindUsers(ctx context.Context, query string) ([]*userpb.User, // GetUserGroups returns the user groups func (m *Manager) GetUserGroups(ctx context.Context, uid *userpb.UserId) ([]string, error) { - user, err := m.GetUser(ctx, uid) + user, err := m.GetUser(ctx, uid, false) if err != nil { return nil, err } diff --git a/go.mod b/go.mod index 2fdb36d86d..0d74ad4863 100644 --- a/go.mod +++ b/go.mod @@ -87,6 +87,7 @@ require ( go 1.16 replace ( + github.com/cs3org/go-cs3apis => github.com/ishank011/go-cs3apis v0.0.0-20211026072245-95303cdc06dc github.com/eventials/go-tus => github.com/andrewmostello/go-tus v0.0.0-20200314041820-904a9904af9a github.com/oleiade/reflections => github.com/oleiade/reflections v1.0.1 google.golang.org/grpc => google.golang.org/grpc v1.26.0 // temporary downgrade diff --git a/go.sum b/go.sum index ae31f5ef35..911fbb67e0 100644 --- a/go.sum +++ b/go.sum @@ -438,6 +438,8 @@ github.com/ianlancetaylor/demangle v0.0.0-20200824232613-28f6c0f3b639/go.mod h1: github.com/imdario/mergo v0.3.12 h1:b6R2BslTbIEToALKP7LxUvijTsNI9TAe80pLWN2g/HU= github.com/imdario/mergo v0.3.12/go.mod h1:jmQim1M+e3UYxmgPu/WyfjB3N3VflVyUjjjwH0dnCYA= github.com/inconshreveable/mousetrap v1.0.0/go.mod h1:PxqpIevigyE2G7u3NXJIT2ANytuPF1OarO4DADm73n8= +github.com/ishank011/go-cs3apis v0.0.0-20211026072245-95303cdc06dc h1:S2KTqUo/trscihgP250SJoXA+JRVNIk/FWXIgFy32D0= +github.com/ishank011/go-cs3apis v0.0.0-20211026072245-95303cdc06dc/go.mod h1:UXha4TguuB52H14EMoSsCqDj7k8a/t7g4gVP+bgY5LY= github.com/jedib0t/go-pretty v4.3.0+incompatible h1:CGs8AVhEKg/n9YbUenWmNStRW2PHJzaeDodcfvRAbIo= github.com/jedib0t/go-pretty v4.3.0+incompatible/go.mod h1:XemHduiw8R651AF9Pt4FwCTKeG3oo7hrHJAoznj9nag= github.com/jessevdk/go-flags v1.5.0/go.mod h1:Fw0T6WPc1dYxT4mKEZRfG5kJhaTDP9pj1c2EWnYs/m4= diff --git a/internal/grpc/services/userprovider/userprovider.go b/internal/grpc/services/userprovider/userprovider.go index cd82bf3304..f9e8bc2bd5 100644 --- a/internal/grpc/services/userprovider/userprovider.go +++ b/internal/grpc/services/userprovider/userprovider.go @@ -125,7 +125,7 @@ func (s *service) Register(ss *grpc.Server) { } func (s *service) GetUser(ctx context.Context, req *userpb.GetUserRequest) (*userpb.GetUserResponse, error) { - user, err := s.usermgr.GetUser(ctx, req.UserId) + user, err := s.usermgr.GetUser(ctx, req.UserId, req.SkipFetchingUserGroups) if err != nil { res := &userpb.GetUserResponse{} if _, ok := err.(errtypes.NotFound); ok { @@ -145,7 +145,7 @@ func (s *service) GetUser(ctx context.Context, req *userpb.GetUserRequest) (*use } func (s *service) GetUserByClaim(ctx context.Context, req *userpb.GetUserByClaimRequest) (*userpb.GetUserByClaimResponse, error) { - user, err := s.usermgr.GetUserByClaim(ctx, req.Claim, req.Value) + user, err := s.usermgr.GetUserByClaim(ctx, req.Claim, req.Value, req.SkipFetchingUserGroups) if err != nil { res := &userpb.GetUserByClaimResponse{} if _, ok := err.(errtypes.NotFound); ok { @@ -165,7 +165,7 @@ func (s *service) GetUserByClaim(ctx context.Context, req *userpb.GetUserByClaim } func (s *service) FindUsers(ctx context.Context, req *userpb.FindUsersRequest) (*userpb.FindUsersResponse, error) { - users, err := s.usermgr.FindUsers(ctx, req.Filter) + users, err := s.usermgr.FindUsers(ctx, req.Filter, req.SkipFetchingUserGroups) if err != nil { err = errors.Wrap(err, "userprovidersvc: error finding users") res := &userpb.FindUsersResponse{ diff --git a/pkg/cbox/user/rest/rest.go b/pkg/cbox/user/rest/rest.go index fbd1b125b2..c291f7f1cf 100644 --- a/pkg/cbox/user/rest/rest.go +++ b/pkg/cbox/user/rest/rest.go @@ -233,7 +233,7 @@ func (m *manager) parseAndCacheUser(ctx context.Context, userData map[string]int } -func (m *manager) GetUser(ctx context.Context, uid *userpb.UserId) (*userpb.User, error) { +func (m *manager) GetUser(ctx context.Context, uid *userpb.UserId, skipFetchingGroups bool) (*userpb.User, error) { u, err := m.fetchCachedUserDetails(uid) if err != nil { var ( @@ -252,19 +252,21 @@ func (m *manager) GetUser(ctx context.Context, uid *userpb.UserId) (*userpb.User u = m.parseAndCacheUser(ctx, userData) } - userGroups, err := m.GetUserGroups(ctx, uid) - if err != nil { - return nil, err + if !skipFetchingGroups { + userGroups, err := m.GetUserGroups(ctx, uid) + if err != nil { + return nil, err + } + u.Groups = userGroups } - u.Groups = userGroups return u, nil } -func (m *manager) GetUserByClaim(ctx context.Context, claim, value string) (*userpb.User, error) { +func (m *manager) GetUserByClaim(ctx context.Context, claim, value string, skipFetchingGroups bool) (*userpb.User, error) { opaqueID, err := m.fetchCachedParam(claim, value) if err == nil { - return m.GetUser(ctx, &userpb.UserId{OpaqueId: opaqueID}) + return m.GetUser(ctx, &userpb.UserId{OpaqueId: opaqueID}, skipFetchingGroups) } switch claim { @@ -289,17 +291,19 @@ func (m *manager) GetUserByClaim(ctx context.Context, claim, value string) (*use } u := m.parseAndCacheUser(ctx, userData) - userGroups, err := m.GetUserGroups(ctx, u.Id) - if err != nil { - return nil, err + if !skipFetchingGroups { + userGroups, err := m.GetUserGroups(ctx, u.Id) + if err != nil { + return nil, err + } + u.Groups = userGroups } - u.Groups = userGroups return u, nil } -func (m *manager) findUsersByFilter(ctx context.Context, url string, users map[string]*userpb.User) error { +func (m *manager) findUsersByFilter(ctx context.Context, url string, users map[string]*userpb.User, skipFetchingGroups bool) error { userData, err := m.apiTokenManager.SendAPIGetRequest(ctx, url, false) if err != nil { @@ -329,6 +333,14 @@ func (m *manager) findUsersByFilter(ctx context.Context, url string, users map[s Idp: m.conf.IDProvider, Type: userType, } + var userGroups []string + if !skipFetchingGroups { + userGroups, err = m.GetUserGroups(ctx, uid) + if err != nil { + return err + } + } + users[uid.OpaqueId] = &userpb.User{ Id: uid, Username: upn, @@ -336,13 +348,14 @@ func (m *manager) findUsersByFilter(ctx context.Context, url string, users map[s DisplayName: name, UidNumber: int64(uidNumber), GidNumber: int64(gidNumber), + Groups: userGroups, } } return nil } -func (m *manager) FindUsers(ctx context.Context, query string) ([]*userpb.User, error) { +func (m *manager) FindUsers(ctx context.Context, query string, skipFetchingGroups bool) ([]*userpb.User, error) { // Look at namespaces filters. If the query starts with: // "a" => look into primary/secondary/service accounts @@ -372,7 +385,7 @@ func (m *manager) FindUsers(ctx context.Context, query string) ([]*userpb.User, for _, f := range filters { url := fmt.Sprintf("%s/Identity/?filter=%s:contains:%s&field=id&field=upn&field=primaryAccountEmail&field=displayName&field=uid&field=gid&field=type", m.conf.APIBaseURL, f, url.QueryEscape(query)) - err := m.findUsersByFilter(ctx, url, users) + err := m.findUsersByFilter(ctx, url, users, skipFetchingGroups) if err != nil { return nil, err } diff --git a/pkg/user/manager/demo/demo.go b/pkg/user/manager/demo/demo.go index 68672917b7..f8ccc83e02 100644 --- a/pkg/user/manager/demo/demo.go +++ b/pkg/user/manager/demo/demo.go @@ -54,19 +54,27 @@ func (m *manager) Configure(ml map[string]interface{}) error { return nil } -func (m *manager) GetUser(ctx context.Context, uid *userpb.UserId) (*userpb.User, error) { +func (m *manager) GetUser(ctx context.Context, uid *userpb.UserId, skipFetchingGroups bool) (*userpb.User, error) { if user, ok := m.catalog[uid.OpaqueId]; ok { if uid.Idp == "" || user.Id.Idp == uid.Idp { - return user, nil + u := *user + if skipFetchingGroups { + u.Groups = nil + } + return &u, nil } } return nil, errtypes.NotFound(uid.OpaqueId) } -func (m *manager) GetUserByClaim(ctx context.Context, claim, value string) (*userpb.User, error) { +func (m *manager) GetUserByClaim(ctx context.Context, claim, value string, skipFetchingGroups bool) (*userpb.User, error) { for _, u := range m.catalog { if userClaim, err := extractClaim(u, claim); err == nil && value == userClaim { - return u, nil + user := *u + if skipFetchingGroups { + user.Groups = nil + } + return &user, nil } } return nil, errtypes.NotFound(value) @@ -91,18 +99,22 @@ func userContains(u *userpb.User, query string) bool { return strings.Contains(u.Username, query) || strings.Contains(u.DisplayName, query) || strings.Contains(u.Mail, query) || strings.Contains(u.Id.OpaqueId, query) } -func (m *manager) FindUsers(ctx context.Context, query string) ([]*userpb.User, error) { +func (m *manager) FindUsers(ctx context.Context, query string, skipFetchingGroups bool) ([]*userpb.User, error) { users := []*userpb.User{} for _, u := range m.catalog { if userContains(u, query) { - users = append(users, u) + user := *u + if skipFetchingGroups { + user.Groups = nil + } + users = append(users, &user) } } return users, nil } func (m *manager) GetUserGroups(ctx context.Context, uid *userpb.UserId) ([]string, error) { - user, err := m.GetUser(ctx, uid) + user, err := m.GetUser(ctx, uid, false) if err != nil { return nil, err } diff --git a/pkg/user/manager/demo/demo_test.go b/pkg/user/manager/demo/demo_test.go index 9eab45cd26..07c3ce9369 100644 --- a/pkg/user/manager/demo/demo_test.go +++ b/pkg/user/manager/demo/demo_test.go @@ -44,49 +44,64 @@ func TestUserManager(t *testing.T) { UidNumber: 123, GidNumber: 987, } - uidFake := &userpb.UserId{Idp: "nonesense", OpaqueId: "fakeUser"} - groupsEinstein := []string{"sailing-lovers", "violin-haters", "physics-lovers"} - - // positive test GetUserGroups - resGroups, _ := manager.GetUserGroups(ctx, uidEinstein) - if !reflect.DeepEqual(resGroups, groupsEinstein) { - t.Fatalf("groups differ: expected=%v got=%v", groupsEinstein, resGroups) + userEinsteinWithoutGroups := &userpb.User{ + Id: uidEinstein, + Username: "einstein", + Mail: "einstein@example.org", + DisplayName: "Albert Einstein", + UidNumber: 123, + GidNumber: 987, } - // negative test GetUserGroups - expectedErr := errtypes.NotFound(uidFake.OpaqueId) - _, err := manager.GetUserGroups(ctx, uidFake) - if !reflect.DeepEqual(err, expectedErr) { - t.Fatalf("user not found error differs: expected='%v' got='%v'", expectedErr, err) - } + uidFake := &userpb.UserId{Idp: "nonesense", OpaqueId: "fakeUser"} + groupsEinstein := []string{"sailing-lovers", "violin-haters", "physics-lovers"} // positive test GetUserByClaim by uid - resUserByUID, _ := manager.GetUserByClaim(ctx, "uid", "123") + resUserByUID, _ := manager.GetUserByClaim(ctx, "uid", "123", false) if !reflect.DeepEqual(resUserByUID, userEinstein) { t.Fatalf("user differs: expected=%v got=%v", userEinstein, resUserByUID) } // negative test GetUserByClaim by uid - expectedErr = errtypes.NotFound("789") - _, err = manager.GetUserByClaim(ctx, "uid", "789") + expectedErr := errtypes.NotFound("789") + _, err := manager.GetUserByClaim(ctx, "uid", "789", false) if !reflect.DeepEqual(err, expectedErr) { t.Fatalf("user not found error differs: expected='%v' got='%v'", expectedErr, err) } // positive test GetUserByClaim by mail - resUserByEmail, _ := manager.GetUserByClaim(ctx, "mail", "einstein@example.org") + resUserByEmail, _ := manager.GetUserByClaim(ctx, "mail", "einstein@example.org", false) if !reflect.DeepEqual(resUserByEmail, userEinstein) { t.Fatalf("user differs: expected=%v got=%v", userEinstein, resUserByEmail) } + // positive test GetUserByClaim by uid without groups + resUserByUIDWithoutGroups, _ := manager.GetUserByClaim(ctx, "uid", "123", true) + if !reflect.DeepEqual(resUserByUIDWithoutGroups, userEinsteinWithoutGroups) { + t.Fatalf("user differs: expected=%v got=%v", userEinsteinWithoutGroups, resUserByUIDWithoutGroups) + } + + // positive test GetUserGroups + resGroups, _ := manager.GetUserGroups(ctx, uidEinstein) + if !reflect.DeepEqual(resGroups, groupsEinstein) { + t.Fatalf("groups differ: expected=%v got=%v", groupsEinstein, resGroups) + } + + // negative test GetUserGroups + expectedErr = errtypes.NotFound(uidFake.OpaqueId) + _, err = manager.GetUserGroups(ctx, uidFake) + if !reflect.DeepEqual(err, expectedErr) { + t.Fatalf("user not found error differs: expected='%v' got='%v'", expectedErr, err) + } + // test FindUsers - resUser, _ := manager.FindUsers(ctx, "einstein") + resUser, _ := manager.FindUsers(ctx, "einstein", false) if !reflect.DeepEqual(resUser, []*userpb.User{userEinstein}) { t.Fatalf("user differs: expected=%v got=%v", []*userpb.User{userEinstein}, resUser) } // negative test FindUsers - resUsers, _ := manager.FindUsers(ctx, "notARealUser") + resUsers, _ := manager.FindUsers(ctx, "notARealUser", false) if len(resUsers) > 0 { t.Fatalf("user not in group: expected=%v got=%v", []*userpb.User{}, resUsers) } diff --git a/pkg/user/manager/json/json.go b/pkg/user/manager/json/json.go index 0966ae2349..5a47d16c64 100644 --- a/pkg/user/manager/json/json.go +++ b/pkg/user/manager/json/json.go @@ -94,19 +94,27 @@ func (m *manager) Configure(ml map[string]interface{}) error { return nil } -func (m *manager) GetUser(ctx context.Context, uid *userpb.UserId) (*userpb.User, error) { +func (m *manager) GetUser(ctx context.Context, uid *userpb.UserId, skipFetchingGroups bool) (*userpb.User, error) { for _, u := range m.users { if (u.Id.GetOpaqueId() == uid.OpaqueId || u.Username == uid.OpaqueId) && (uid.Idp == "" || uid.Idp == u.Id.GetIdp()) { - return u, nil + user := *u + if skipFetchingGroups { + user.Groups = nil + } + return &user, nil } } return nil, errtypes.NotFound(uid.OpaqueId) } -func (m *manager) GetUserByClaim(ctx context.Context, claim, value string) (*userpb.User, error) { +func (m *manager) GetUserByClaim(ctx context.Context, claim, value string, skipFetchingGroups bool) (*userpb.User, error) { for _, u := range m.users { if userClaim, err := extractClaim(u, claim); err == nil && value == userClaim { - return u, nil + user := *u + if skipFetchingGroups { + user.Groups = nil + } + return &user, nil } } return nil, errtypes.NotFound(value) @@ -133,18 +141,22 @@ func userContains(u *userpb.User, query string) bool { strings.Contains(strings.ToLower(u.Mail), query) || strings.Contains(strings.ToLower(u.Id.OpaqueId), query) } -func (m *manager) FindUsers(ctx context.Context, query string) ([]*userpb.User, error) { +func (m *manager) FindUsers(ctx context.Context, query string, skipFetchingGroups bool) ([]*userpb.User, error) { users := []*userpb.User{} for _, u := range m.users { if userContains(u, query) { - users = append(users, u) + user := *u + if skipFetchingGroups { + user.Groups = nil + } + users = append(users, &user) } } return users, nil } func (m *manager) GetUserGroups(ctx context.Context, uid *userpb.UserId) ([]string, error) { - user, err := m.GetUser(ctx, uid) + user, err := m.GetUser(ctx, uid, false) if err != nil { return nil, err } diff --git a/pkg/user/manager/json/json_test.go b/pkg/user/manager/json/json_test.go index 66838bd983..f591d94a69 100644 --- a/pkg/user/manager/json/json_test.go +++ b/pkg/user/manager/json/json_test.go @@ -97,15 +97,15 @@ func TestUserManager(t *testing.T) { Mail: "einstein@example.org", DisplayName: "Albert Einstein", } + userEinsteinWithoutGroups := &userpb.User{ + Id: uidEinstein, + Username: "einstein", + Mail: "einstein@example.org", + DisplayName: "Albert Einstein", + } userFake := &userpb.UserId{Idp: "localhost", OpaqueId: "fakeUser", Type: userpb.UserType_USER_TYPE_PRIMARY} groupsEinstein := []string{"sailing-lovers", "violin-haters", "physics-lovers"} - // positive test GetUserGroups - resGroups, _ := manager.GetUserGroups(ctx, uidEinstein) - if !reflect.DeepEqual(resGroups, groupsEinstein) { - t.Fatalf("groups differ: expected=%v got=%v", resGroups, groupsEinstein) - } - // negative test GetUserGroups expectedErr := errtypes.NotFound(userFake.OpaqueId) _, err = manager.GetUserGroups(ctx, userFake) @@ -114,20 +114,32 @@ func TestUserManager(t *testing.T) { } // positive test GetUserByClaim by mail - resUserByEmail, _ := manager.GetUserByClaim(ctx, "mail", "einstein@example.org") + resUserByEmail, _ := manager.GetUserByClaim(ctx, "mail", "einstein@example.org", false) if !reflect.DeepEqual(resUserByEmail, userEinstein) { t.Fatalf("user differs: expected=%v got=%v", userEinstein, resUserByEmail) } // negative test GetUserByClaim by mail expectedErr = errtypes.NotFound("abc@example.com") - _, err = manager.GetUserByClaim(ctx, "mail", "abc@example.com") + _, err = manager.GetUserByClaim(ctx, "mail", "abc@example.com", false) if !reflect.DeepEqual(err, expectedErr) { t.Fatalf("user not found error differs: expected='%v' got='%v'", expectedErr, err) } + // positive test GetUserByClaim by mail without groups + resUserByEmailWithoutGroups, _ := manager.GetUserByClaim(ctx, "mail", "einstein@example.org", true) + if !reflect.DeepEqual(resUserByEmailWithoutGroups, userEinsteinWithoutGroups) { + t.Fatalf("user differs: expected=%v got=%v", userEinsteinWithoutGroups, resUserByEmailWithoutGroups) + } + + // positive test GetUserGroups + resGroups, _ := manager.GetUserGroups(ctx, uidEinstein) + if !reflect.DeepEqual(resGroups, groupsEinstein) { + t.Fatalf("groups differ: expected=%v got=%v", resGroups, groupsEinstein) + } + // test FindUsers - resUser, _ := manager.FindUsers(ctx, "stein") + resUser, _ := manager.FindUsers(ctx, "stein", false) if len(resUser) != 1 { t.Fatalf("too many users found: expected=%d got=%d", 1, len(resUser)) } diff --git a/pkg/user/manager/ldap/ldap.go b/pkg/user/manager/ldap/ldap.go index dbaf00b09e..402240996d 100644 --- a/pkg/user/manager/ldap/ldap.go +++ b/pkg/user/manager/ldap/ldap.go @@ -141,7 +141,7 @@ func (m *manager) Configure(ml map[string]interface{}) error { return nil } -func (m *manager) GetUser(ctx context.Context, uid *userpb.UserId) (*userpb.User, error) { +func (m *manager) GetUser(ctx context.Context, uid *userpb.UserId, skipFetchingGroups bool) (*userpb.User, error) { log := appctx.GetLogger(ctx) l, err := utils.GetLDAPConnection(&m.c.LDAPConn) if err != nil { @@ -174,10 +174,15 @@ func (m *manager) GetUser(ctx context.Context, uid *userpb.UserId) (*userpb.User OpaqueId: sr.Entries[0].GetEqualFoldAttributeValue(m.c.Schema.UID), Type: userpb.UserType_USER_TYPE_PRIMARY, } - groups, err := m.GetUserGroups(ctx, id) - if err != nil { - return nil, err + + groups := []string{} + if !skipFetchingGroups { + groups, err = m.GetUserGroups(ctx, id) + if err != nil { + return nil, err + } } + gidNumber := m.c.Nobody gidValue := sr.Entries[0].GetEqualFoldAttributeValue(m.c.Schema.GIDNumber) if gidValue != "" { @@ -207,7 +212,7 @@ func (m *manager) GetUser(ctx context.Context, uid *userpb.UserId) (*userpb.User return u, nil } -func (m *manager) GetUserByClaim(ctx context.Context, claim, value string) (*userpb.User, error) { +func (m *manager) GetUserByClaim(ctx context.Context, claim, value string, skipFetchingGroups bool) (*userpb.User, error) { // TODO align supported claims with rest driver and the others, maybe refactor into common mapping switch claim { case "mail": @@ -256,10 +261,15 @@ func (m *manager) GetUserByClaim(ctx context.Context, claim, value string) (*use OpaqueId: sr.Entries[0].GetEqualFoldAttributeValue(m.c.Schema.UID), Type: userpb.UserType_USER_TYPE_PRIMARY, } - groups, err := m.GetUserGroups(ctx, id) - if err != nil { - return nil, err + + groups := []string{} + if !skipFetchingGroups { + groups, err = m.GetUserGroups(ctx, id) + if err != nil { + return nil, err + } } + gidNumber := m.c.Nobody gidValue := sr.Entries[0].GetEqualFoldAttributeValue(m.c.Schema.GIDNumber) if gidValue != "" { @@ -290,7 +300,7 @@ func (m *manager) GetUserByClaim(ctx context.Context, claim, value string) (*use } -func (m *manager) FindUsers(ctx context.Context, query string) ([]*userpb.User, error) { +func (m *manager) FindUsers(ctx context.Context, query string, skipFetchingGroups bool) ([]*userpb.User, error) { l, err := utils.GetLDAPConnection(&m.c.LDAPConn) if err != nil { return nil, err @@ -319,10 +329,15 @@ func (m *manager) FindUsers(ctx context.Context, query string) ([]*userpb.User, OpaqueId: entry.GetEqualFoldAttributeValue(m.c.Schema.UID), Type: userpb.UserType_USER_TYPE_PRIMARY, } - groups, err := m.GetUserGroups(ctx, id) - if err != nil { - return nil, err + + groups := []string{} + if !skipFetchingGroups { + groups, err = m.GetUserGroups(ctx, id) + if err != nil { + return nil, err + } } + gidNumber := m.c.Nobody gidValue := sr.Entries[0].GetEqualFoldAttributeValue(m.c.Schema.GIDNumber) if gidValue != "" { diff --git a/pkg/user/manager/nextcloud/nextcloud.go b/pkg/user/manager/nextcloud/nextcloud.go index 87a15c0165..ece1b039d1 100644 --- a/pkg/user/manager/nextcloud/nextcloud.go +++ b/pkg/user/manager/nextcloud/nextcloud.go @@ -145,7 +145,7 @@ func (um *Manager) Configure(ml map[string]interface{}) error { } // GetUser method as defined in https://github.com/cs3org/reva/blob/v1.13.0/pkg/user/user.go#L29-L35 -func (um *Manager) GetUser(ctx context.Context, uid *userpb.UserId) (*userpb.User, error) { +func (um *Manager) GetUser(ctx context.Context, uid *userpb.UserId, skipFetchingGroups bool) (*userpb.User, error) { bodyStr, err := json.Marshal(uid) if err != nil { return nil, err @@ -164,7 +164,7 @@ func (um *Manager) GetUser(ctx context.Context, uid *userpb.UserId) (*userpb.Use } // GetUserByClaim method as defined in https://github.com/cs3org/reva/blob/v1.13.0/pkg/user/user.go#L29-L35 -func (um *Manager) GetUserByClaim(ctx context.Context, claim, value string) (*userpb.User, error) { +func (um *Manager) GetUserByClaim(ctx context.Context, claim, value string, skipFetchingGroups bool) (*userpb.User, error) { type paramsObj struct { Claim string `json:"claim"` Value string `json:"value"` @@ -205,7 +205,7 @@ func (um *Manager) GetUserGroups(ctx context.Context, uid *userpb.UserId) ([]str } // FindUsers method as defined in https://github.com/cs3org/reva/blob/v1.13.0/pkg/user/user.go#L29-L35 -func (um *Manager) FindUsers(ctx context.Context, query string) ([]*userpb.User, error) { +func (um *Manager) FindUsers(ctx context.Context, query string, skipFetchingGroups bool) ([]*userpb.User, error) { _, respBody, err := um.do(ctx, Action{"FindUsers", query}) if err != nil { return nil, err diff --git a/pkg/user/manager/nextcloud/nextcloud_test.go b/pkg/user/manager/nextcloud/nextcloud_test.go index d043038925..e4f1062286 100644 --- a/pkg/user/manager/nextcloud/nextcloud_test.go +++ b/pkg/user/manager/nextcloud/nextcloud_test.go @@ -133,7 +133,8 @@ var _ = Describe("Nextcloud", func() { Idp: "some-idp", OpaqueId: "some-opaque-user-id", Type: 1, - }) + }, + false) Expect(err).ToNot(HaveOccurred()) Expect(user).To(Equal(&userpb.User{ Id: &userpb.UserId{ @@ -160,7 +161,7 @@ var _ = Describe("Nextcloud", func() { um, called, teardown := setUpNextcloudServer() defer teardown() - user, err := um.GetUserByClaim(ctx, "claim-string", "value-string") + user, err := um.GetUserByClaim(ctx, "claim-string", "value-string", false) Expect(err).ToNot(HaveOccurred()) Expect(user).To(Equal(&userpb.User{ Id: &userpb.UserId{ @@ -204,7 +205,7 @@ var _ = Describe("Nextcloud", func() { um, called, teardown := setUpNextcloudServer() defer teardown() - users, err := um.FindUsers(ctx, "some-query") + users, err := um.FindUsers(ctx, "some-query", false) Expect(err).ToNot(HaveOccurred()) Expect(len(users)).To(Equal(1)) Expect(*users[0]).To(Equal(userpb.User{ diff --git a/pkg/user/manager/owncloudsql/owncloudsql.go b/pkg/user/manager/owncloudsql/owncloudsql.go index b86c9ce917..e56bc7cf0f 100644 --- a/pkg/user/manager/owncloudsql/owncloudsql.go +++ b/pkg/user/manager/owncloudsql/owncloudsql.go @@ -102,26 +102,26 @@ func parseConfig(m map[string]interface{}) (*config, error) { return c, nil } -func (m *manager) GetUser(ctx context.Context, uid *userpb.UserId) (*userpb.User, error) { +func (m *manager) GetUser(ctx context.Context, uid *userpb.UserId, skipFetchingGroups bool) (*userpb.User, error) { // search via the user_id a, err := m.db.GetAccountByClaim(ctx, "userid", uid.OpaqueId) if err == sql.ErrNoRows { return nil, errtypes.NotFound(uid.OpaqueId) } - return m.convertToCS3User(ctx, a) + return m.convertToCS3User(ctx, a, skipFetchingGroups) } -func (m *manager) GetUserByClaim(ctx context.Context, claim, value string) (*userpb.User, error) { +func (m *manager) GetUserByClaim(ctx context.Context, claim, value string, skipFetchingGroups bool) (*userpb.User, error) { a, err := m.db.GetAccountByClaim(ctx, claim, value) if err == sql.ErrNoRows { return nil, errtypes.NotFound(claim + "=" + value) } else if err != nil { return nil, err } - return m.convertToCS3User(ctx, a) + return m.convertToCS3User(ctx, a, skipFetchingGroups) } -func (m *manager) FindUsers(ctx context.Context, query string) ([]*userpb.User, error) { +func (m *manager) FindUsers(ctx context.Context, query string, skipFetchingGroups bool) ([]*userpb.User, error) { accounts, err := m.db.FindAccounts(ctx, query) if err == sql.ErrNoRows { @@ -132,7 +132,7 @@ func (m *manager) FindUsers(ctx context.Context, query string) ([]*userpb.User, users := make([]*userpb.User, 0, len(accounts)) for i := range accounts { - u, err := m.convertToCS3User(ctx, &accounts[i]) + u, err := m.convertToCS3User(ctx, &accounts[i], skipFetchingGroups) if err != nil { appctx.GetLogger(ctx).Error().Err(err).Interface("account", accounts[i]).Msg("could not convert account, skipping") continue @@ -153,7 +153,7 @@ func (m *manager) GetUserGroups(ctx context.Context, uid *userpb.UserId) ([]stri return groups, nil } -func (m *manager) convertToCS3User(ctx context.Context, a *accounts.Account) (*userpb.User, error) { +func (m *manager) convertToCS3User(ctx context.Context, a *accounts.Account, skipFetchingGroups bool) (*userpb.User, error) { u := &userpb.User{ Id: &userpb.UserId{ Idp: m.c.Idp, @@ -172,9 +172,12 @@ func (m *manager) convertToCS3User(ctx context.Context, a *accounts.Account) (*u if u.DisplayName == "" { u.DisplayName = u.Id.OpaqueId } - var err error - if u.Groups, err = m.GetUserGroups(ctx, u.Id); err != nil { - return nil, err + + if !skipFetchingGroups { + var err error + if u.Groups, err = m.GetUserGroups(ctx, u.Id); err != nil { + return nil, err + } } return u, nil } diff --git a/pkg/user/rpc_user.go b/pkg/user/rpc_user.go index ec7946cad0..9c64c8572d 100644 --- a/pkg/user/rpc_user.go +++ b/pkg/user/rpc_user.go @@ -75,8 +75,9 @@ func (m *RPCClient) Configure(ml map[string]interface{}) error { // GetUserArg for RPC type GetUserArg struct { - Ctx map[interface{}]interface{} - UID *userpb.UserId + Ctx map[interface{}]interface{} + UID *userpb.UserId + SkipFetchingGroups bool } // GetUserReply for RPC @@ -86,9 +87,9 @@ type GetUserReply struct { } // GetUser RPCClient GetUser method -func (m *RPCClient) GetUser(ctx context.Context, uid *userpb.UserId) (*userpb.User, error) { +func (m *RPCClient) GetUser(ctx context.Context, uid *userpb.UserId, skipFetchingGroups bool) (*userpb.User, error) { ctxVal := appctx.GetKeyValuesFromCtx(ctx) - args := GetUserArg{Ctx: ctxVal, UID: uid} + args := GetUserArg{Ctx: ctxVal, UID: uid, SkipFetchingGroups: skipFetchingGroups} resp := GetUserReply{} err := m.Client.Call("Plugin.GetUser", args, &resp) if err != nil { @@ -99,9 +100,10 @@ func (m *RPCClient) GetUser(ctx context.Context, uid *userpb.UserId) (*userpb.Us // GetUserByClaimArg for RPC type GetUserByClaimArg struct { - Ctx map[interface{}]interface{} - Claim string - Value string + Ctx map[interface{}]interface{} + Claim string + Value string + SkipFetchingGroups bool } // GetUserByClaimReply for RPC @@ -111,9 +113,9 @@ type GetUserByClaimReply struct { } // GetUserByClaim RPCClient GetUserByClaim method -func (m *RPCClient) GetUserByClaim(ctx context.Context, claim, value string) (*userpb.User, error) { +func (m *RPCClient) GetUserByClaim(ctx context.Context, claim, value string, skipFetchingGroups bool) (*userpb.User, error) { ctxVal := appctx.GetKeyValuesFromCtx(ctx) - args := GetUserByClaimArg{Ctx: ctxVal, Claim: claim, Value: value} + args := GetUserByClaimArg{Ctx: ctxVal, Claim: claim, Value: value, SkipFetchingGroups: skipFetchingGroups} resp := GetUserByClaimReply{} err := m.Client.Call("Plugin.GetUserByClaim", args, &resp) if err != nil { @@ -148,8 +150,9 @@ func (m *RPCClient) GetUserGroups(ctx context.Context, user *userpb.UserId) ([]s // FindUsersArg for RPC type FindUsersArg struct { - Ctx map[interface{}]interface{} - Query string + Ctx map[interface{}]interface{} + Query string + SkipFetchingGroups bool } // FindUsersReply for RPC @@ -159,9 +162,9 @@ type FindUsersReply struct { } // FindUsers RPCClient FindUsers method -func (m *RPCClient) FindUsers(ctx context.Context, query string) ([]*userpb.User, error) { +func (m *RPCClient) FindUsers(ctx context.Context, query string, skipFetchingGroups bool) ([]*userpb.User, error) { ctxVal := appctx.GetKeyValuesFromCtx(ctx) - args := FindUsersArg{Ctx: ctxVal, Query: query} + args := FindUsersArg{Ctx: ctxVal, Query: query, SkipFetchingGroups: skipFetchingGroups} resp := FindUsersReply{} err := m.Client.Call("Plugin.FindUsers", args, &resp) if err != nil { @@ -185,14 +188,14 @@ func (m *RPCServer) Configure(args ConfigureArg, resp *ConfigureReply) error { // GetUser RPCServer GetUser method func (m *RPCServer) GetUser(args GetUserArg, resp *GetUserReply) error { ctx := appctx.PutKeyValuesToCtx(args.Ctx) - resp.User, resp.Err = m.Impl.GetUser(ctx, args.UID) + resp.User, resp.Err = m.Impl.GetUser(ctx, args.UID, args.SkipFetchingGroups) return nil } // GetUserByClaim RPCServer GetUserByClaim method func (m *RPCServer) GetUserByClaim(args GetUserByClaimArg, resp *GetUserByClaimReply) error { ctx := appctx.PutKeyValuesToCtx(args.Ctx) - resp.User, resp.Err = m.Impl.GetUserByClaim(ctx, args.Claim, args.Value) + resp.User, resp.Err = m.Impl.GetUserByClaim(ctx, args.Claim, args.Value, args.SkipFetchingGroups) return nil } @@ -206,6 +209,6 @@ func (m *RPCServer) GetUserGroups(args GetUserGroupsArg, resp *GetUserGroupsRepl // FindUsers RPCServer FindUsers method func (m *RPCServer) FindUsers(args FindUsersArg, resp *FindUsersReply) error { ctx := appctx.PutKeyValuesToCtx(args.Ctx) - resp.User, resp.Err = m.Impl.FindUsers(ctx, args.Query) + resp.User, resp.Err = m.Impl.FindUsers(ctx, args.Query, args.SkipFetchingGroups) return nil } diff --git a/pkg/user/user.go b/pkg/user/user.go index c81438e451..16e7bf57a6 100644 --- a/pkg/user/user.go +++ b/pkg/user/user.go @@ -28,8 +28,8 @@ import ( // Manager is the interface to implement to manipulate users. type Manager interface { plugin.Plugin - GetUser(ctx context.Context, uid *userpb.UserId) (*userpb.User, error) - GetUserByClaim(ctx context.Context, claim, value string) (*userpb.User, error) + GetUser(ctx context.Context, uid *userpb.UserId, skipFetchingGroups bool) (*userpb.User, error) + GetUserByClaim(ctx context.Context, claim, value string, skipFetchingGroups bool) (*userpb.User, error) GetUserGroups(ctx context.Context, uid *userpb.UserId) ([]string, error) - FindUsers(ctx context.Context, query string) ([]*userpb.User, error) + FindUsers(ctx context.Context, query string, skipFetchingGroups bool) ([]*userpb.User, error) } From 6be46e4477051877e72e4f44580762e96d4d2812 Mon Sep 17 00:00:00 2001 From: Ishank Arora Date: Tue, 26 Oct 2021 10:36:19 +0200 Subject: [PATCH 2/4] Modify group managers to skip fetching members when specified --- .../unreleased/fetch-groups-members-config.md | 3 ++ .../services/groupprovider/groupprovider.go | 6 +-- .../handlers/apps/sharing/sharees/sharees.go | 4 +- .../ocs/handlers/apps/sharing/shares/group.go | 5 ++- .../handlers/apps/sharing/shares/shares.go | 2 + .../ocs/handlers/apps/sharing/shares/user.go | 5 ++- pkg/cbox/group/rest/rest.go | 42 ++++++++++++------- pkg/group/group.go | 6 +-- pkg/group/manager/json/json.go | 24 ++++++++--- pkg/group/manager/json/json_test.go | 23 +++++++--- pkg/group/manager/ldap/ldap.go | 38 +++++++++++++---- 11 files changed, 112 insertions(+), 46 deletions(-) create mode 100644 changelog/unreleased/fetch-groups-members-config.md diff --git a/changelog/unreleased/fetch-groups-members-config.md b/changelog/unreleased/fetch-groups-members-config.md new file mode 100644 index 0000000000..6f375857c9 --- /dev/null +++ b/changelog/unreleased/fetch-groups-members-config.md @@ -0,0 +1,3 @@ +Enhancement: Modify group and user managers to skip fetching specified metadata + +https://github.com/cs3org/reva/pull/2205 \ No newline at end of file diff --git a/internal/grpc/services/groupprovider/groupprovider.go b/internal/grpc/services/groupprovider/groupprovider.go index 6fefa4cbe6..75cf828962 100644 --- a/internal/grpc/services/groupprovider/groupprovider.go +++ b/internal/grpc/services/groupprovider/groupprovider.go @@ -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 { res := &grouppb.GetGroupResponse{} if _, ok := err.(errtypes.NotFound); ok { @@ -120,7 +120,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 { res := &grouppb.GetGroupByClaimResponse{} if _, ok := err.(errtypes.NotFound); ok { @@ -139,7 +139,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{ diff --git a/internal/http/services/owncloud/ocs/handlers/apps/sharing/sharees/sharees.go b/internal/http/services/owncloud/ocs/handlers/apps/sharing/sharees/sharees.go index f15192d725..7231a2ff23 100644 --- a/internal/http/services/owncloud/ocs/handlers/apps/sharing/sharees/sharees.go +++ b/internal/http/services/owncloud/ocs/handlers/apps/sharing/sharees/sharees.go @@ -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 @@ -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 diff --git a/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/group.go b/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/group.go index 56663aec29..10d6eaf236 100644 --- a/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/group.go +++ b/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/group.go @@ -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) diff --git a/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go b/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go index 53c859c7c0..d302f9824e 100644 --- a/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go +++ b/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go @@ -918,6 +918,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") @@ -947,6 +948,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") diff --git a/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/user.go b/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/user.go index 76204897d7..cf45fa2d27 100644 --- a/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/user.go +++ b/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/user.go @@ -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) diff --git a/pkg/cbox/group/rest/rest.go b/pkg/cbox/group/rest/rest.go index 439ec9c6c9..3852b68027 100644 --- a/pkg/cbox/group/rest/rest.go +++ b/pkg/cbox/group/rest/rest.go @@ -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) @@ -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 { @@ -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 { @@ -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 @@ -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 } diff --git a/pkg/group/group.go b/pkg/group/group.go index f27766cbb3..e97d25f3f9 100644 --- a/pkg/group/group.go +++ b/pkg/group/group.go @@ -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) } diff --git a/pkg/group/manager/json/json.go b/pkg/group/manager/json/json.go index ad4a425dd6..3e54b7765c 100644 --- a/pkg/group/manager/json/json.go +++ b/pkg/group/manager/json/json.go @@ -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) @@ -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 diff --git a/pkg/group/manager/json/json_test.go b/pkg/group/manager/json/json_test.go index b79f1c2762..3960276d4a 100644 --- a/pkg/group/manager/json/json_test.go +++ b/pkg/group/manager/json/json_test.go @@ -102,30 +102,43 @@ func TestUserManager(t *testing.T) { DisplayName: "Sailing Lovers", Members: members, } + groupWithoutMembers := &grouppb.Group{ + Id: gid, + GroupName: "sailing-lovers", + Mail: "sailing-lovers@example.org", + 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", "sailing-lovers@example.org") + resGroupByEmail, _ := manager.GetGroupByClaim(ctx, "mail", "sailing-lovers@example.org", false) if !reflect.DeepEqual(resGroupByEmail, group) { t.Fatalf("group differs: expected=%v got=%v", group, resGroupByEmail) } // negative test GetGroupByClaim by mail expectedErr = errtypes.NotFound("abc@example.com") - _, err = manager.GetGroupByClaim(ctx, "mail", "abc@example.com") + _, err = manager.GetGroupByClaim(ctx, "mail", "abc@example.com", false) if !reflect.DeepEqual(err, expectedErr) { t.Fatalf("group not found error differs: expected='%v' got='%v'", expectedErr, err) } @@ -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)) } diff --git a/pkg/group/manager/ldap/ldap.go b/pkg/group/manager/ldap/ldap.go index d388e835e9..9b66d570f7 100644 --- a/pkg/group/manager/ldap/ldap.go +++ b/pkg/group/manager/ldap/ldap.go @@ -129,7 +129,7 @@ func New(m map[string]interface{}) (group.Manager, error) { return mgr, 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) { log := appctx.GetLogger(ctx) l, err := utils.GetLDAPConnection(&m.c.LDAPConn) if err != nil { @@ -161,10 +161,15 @@ func (m *manager) GetGroup(ctx context.Context, gid *grouppb.GroupId) (*grouppb. Idp: m.c.Idp, OpaqueId: sr.Entries[0].GetEqualFoldAttributeValue(m.c.Schema.GID), } - members, err := m.GetMembers(ctx, id) - if err != nil { - return nil, err + + var members []*userpb.UserId + if !skipFetchingMembers { + members, err = m.GetMembers(ctx, id) + if err != nil { + return nil, err + } } + gidNumber := m.c.Nobody gidValue := sr.Entries[0].GetEqualFoldAttributeValue(m.c.Schema.GIDNumber) if gidValue != "" { @@ -186,7 +191,7 @@ func (m *manager) GetGroup(ctx context.Context, gid *grouppb.GroupId) (*grouppb. 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) { // TODO align supported claims with rest driver and the others, maybe refactor into common mapping switch claim { case "mail": @@ -232,10 +237,15 @@ func (m *manager) GetGroupByClaim(ctx context.Context, claim, value string) (*gr Idp: m.c.Idp, OpaqueId: sr.Entries[0].GetEqualFoldAttributeValue(m.c.Schema.GID), } - members, err := m.GetMembers(ctx, id) - if err != nil { - return nil, err + + var members []*userpb.UserId + if !skipFetchingMembers { + members, err = m.GetMembers(ctx, id) + if err != nil { + return nil, err + } } + gidNumber, err := strconv.ParseInt(sr.Entries[0].GetEqualFoldAttributeValue(m.c.Schema.GIDNumber), 10, 64) if err != nil { return nil, err @@ -253,7 +263,7 @@ func (m *manager) GetGroupByClaim(ctx context.Context, claim, value string) (*gr return g, 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) { l, err := utils.GetLDAPConnection(&m.c.LDAPConn) if err != nil { return nil, err @@ -281,6 +291,15 @@ func (m *manager) FindGroups(ctx context.Context, query string) ([]*grouppb.Grou Idp: m.c.Idp, OpaqueId: entry.GetEqualFoldAttributeValue(m.c.Schema.GID), } + + var members []*userpb.UserId + if !skipFetchingMembers { + members, err = m.GetMembers(ctx, id) + if err != nil { + return nil, err + } + } + gidNumber, err := strconv.ParseInt(entry.GetEqualFoldAttributeValue(m.c.Schema.GIDNumber), 10, 64) if err != nil { return nil, err @@ -289,6 +308,7 @@ func (m *manager) FindGroups(ctx context.Context, query string) ([]*grouppb.Grou g := &grouppb.Group{ Id: id, GroupName: entry.GetEqualFoldAttributeValue(m.c.Schema.CN), + Members: members, Mail: entry.GetEqualFoldAttributeValue(m.c.Schema.Mail), DisplayName: entry.GetEqualFoldAttributeValue(m.c.Schema.DisplayName), GidNumber: gidNumber, From 215db00c1c249bd68227620b3cba45ca209b4593 Mon Sep 17 00:00:00 2001 From: Ishank Arora Date: Tue, 26 Oct 2021 13:10:17 +0200 Subject: [PATCH 3/4] Skip fetching metadata where not required --- internal/grpc/interceptors/auth/scope.go | 2 +- internal/http/services/ocmd/shares.go | 2 +- pkg/share/manager/sql/conversions.go | 8 +++++--- pkg/storage/utils/eosfs/eosfs.go | 8 +++++--- 4 files changed, 12 insertions(+), 8 deletions(-) diff --git a/internal/grpc/interceptors/auth/scope.go b/internal/grpc/interceptors/auth/scope.go index d2161b6eb5..a18ac245cf 100644 --- a/internal/grpc/interceptors/auth/scope.go +++ b/internal/grpc/interceptors/auth/scope.go @@ -186,7 +186,7 @@ func checkIfNestedResource(ctx context.Context, ref *provider.Reference, parent // We mint a token as the owner of the public share and try to stat the reference // TODO(ishank011): We need to find a better alternative to this - userResp, err := client.GetUser(ctx, &userpb.GetUserRequest{UserId: statResponse.Info.Owner}) + userResp, err := client.GetUser(ctx, &userpb.GetUserRequest{UserId: statResponse.Info.Owner, SkipFetchingUserGroups: true}) if err != nil || userResp.Status.Code != rpc.Code_CODE_OK { return false, err } diff --git a/internal/http/services/ocmd/shares.go b/internal/http/services/ocmd/shares.go index 60ebfa9f6d..140ab36f01 100644 --- a/internal/http/services/ocmd/shares.go +++ b/internal/http/services/ocmd/shares.go @@ -105,7 +105,7 @@ func (h *sharesHandler) createShare(w http.ResponseWriter, r *http.Request) { } userRes, err := gatewayClient.GetUser(ctx, &userpb.GetUserRequest{ - UserId: &userpb.UserId{OpaqueId: shareWith}, + UserId: &userpb.UserId{OpaqueId: shareWith}, SkipFetchingUserGroups: true, }) if err != nil { WriteError(w, r, APIErrorServerError, "error searching recipient", err) diff --git a/pkg/share/manager/sql/conversions.go b/pkg/share/manager/sql/conversions.go index 7abf2813d7..f3658b022c 100644 --- a/pkg/share/manager/sql/conversions.go +++ b/pkg/share/manager/sql/conversions.go @@ -79,7 +79,8 @@ func (c *GatewayUserConverter) UserIDToUserName(ctx context.Context, userid *use return "", err } getUserResponse, err := gwConn.GetUser(ctx, &userprovider.GetUserRequest{ - UserId: userid, + UserId: userid, + SkipFetchingUserGroups: true, }) if err != nil { return "", err @@ -97,8 +98,9 @@ func (c *GatewayUserConverter) UserNameToUserID(ctx context.Context, username st return nil, err } getUserResponse, err := gwConn.GetUserByClaim(ctx, &userpb.GetUserByClaimRequest{ - Claim: "username", - Value: username, + Claim: "username", + Value: username, + SkipFetchingUserGroups: true, }) if err != nil { return nil, err diff --git a/pkg/storage/utils/eosfs/eosfs.go b/pkg/storage/utils/eosfs/eosfs.go index 76034da770..f4c9b66e91 100644 --- a/pkg/storage/utils/eosfs/eosfs.go +++ b/pkg/storage/utils/eosfs/eosfs.go @@ -1917,7 +1917,8 @@ func (fs *eosfs) getUIDGateway(ctx context.Context, u *userpb.UserId) (eosclient return eosclient.Authorization{}, errors.Wrap(err, "eosfs: error getting gateway grpc client") } getUserResp, err := client.GetUser(ctx, &userpb.GetUserRequest{ - UserId: u, + UserId: u, + SkipFetchingUserGroups: true, }) if err != nil { _ = fs.userIDCache.SetWithTTL(u.OpaqueId, &userpb.User{}, 12*time.Hour) @@ -1950,8 +1951,9 @@ func (fs *eosfs) getUserIDGateway(ctx context.Context, uid string) (*userpb.User return nil, errors.Wrap(err, "eosfs: error getting gateway grpc client") } getUserResp, err := client.GetUserByClaim(ctx, &userpb.GetUserByClaimRequest{ - Claim: "uid", - Value: uid, + Claim: "uid", + Value: uid, + SkipFetchingUserGroups: true, }) if err != nil { // Insert an empty object in the cache so that we don't make another call From c6e6293cbc9b8cd86c0496b966ec62631dbcc3f6 Mon Sep 17 00:00:00 2001 From: Ishank Arora Date: Wed, 17 Nov 2021 14:43:07 +0100 Subject: [PATCH 4/4] Add comments for the user manager interface methods --- go.mod | 1 - go.sum | 2 -- pkg/user/user.go | 6 ++++++ 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 0d74ad4863..2fdb36d86d 100644 --- a/go.mod +++ b/go.mod @@ -87,7 +87,6 @@ require ( go 1.16 replace ( - github.com/cs3org/go-cs3apis => github.com/ishank011/go-cs3apis v0.0.0-20211026072245-95303cdc06dc github.com/eventials/go-tus => github.com/andrewmostello/go-tus v0.0.0-20200314041820-904a9904af9a github.com/oleiade/reflections => github.com/oleiade/reflections v1.0.1 google.golang.org/grpc => google.golang.org/grpc v1.26.0 // temporary downgrade diff --git a/go.sum b/go.sum index 911fbb67e0..ae31f5ef35 100644 --- a/go.sum +++ b/go.sum @@ -438,8 +438,6 @@ github.com/ianlancetaylor/demangle v0.0.0-20200824232613-28f6c0f3b639/go.mod h1: github.com/imdario/mergo v0.3.12 h1:b6R2BslTbIEToALKP7LxUvijTsNI9TAe80pLWN2g/HU= github.com/imdario/mergo v0.3.12/go.mod h1:jmQim1M+e3UYxmgPu/WyfjB3N3VflVyUjjjwH0dnCYA= github.com/inconshreveable/mousetrap v1.0.0/go.mod h1:PxqpIevigyE2G7u3NXJIT2ANytuPF1OarO4DADm73n8= -github.com/ishank011/go-cs3apis v0.0.0-20211026072245-95303cdc06dc h1:S2KTqUo/trscihgP250SJoXA+JRVNIk/FWXIgFy32D0= -github.com/ishank011/go-cs3apis v0.0.0-20211026072245-95303cdc06dc/go.mod h1:UXha4TguuB52H14EMoSsCqDj7k8a/t7g4gVP+bgY5LY= github.com/jedib0t/go-pretty v4.3.0+incompatible h1:CGs8AVhEKg/n9YbUenWmNStRW2PHJzaeDodcfvRAbIo= github.com/jedib0t/go-pretty v4.3.0+incompatible/go.mod h1:XemHduiw8R651AF9Pt4FwCTKeG3oo7hrHJAoznj9nag= github.com/jessevdk/go-flags v1.5.0/go.mod h1:Fw0T6WPc1dYxT4mKEZRfG5kJhaTDP9pj1c2EWnYs/m4= diff --git a/pkg/user/user.go b/pkg/user/user.go index 16e7bf57a6..15e952959b 100644 --- a/pkg/user/user.go +++ b/pkg/user/user.go @@ -28,8 +28,14 @@ import ( // Manager is the interface to implement to manipulate users. type Manager interface { plugin.Plugin + // GetUser returns the user metadata identified by a uid. + // The groups of the user are omitted if specified, as these might not be required for certain operations + // and might involve computational overhead. GetUser(ctx context.Context, uid *userpb.UserId, skipFetchingGroups bool) (*userpb.User, error) + // GetUserByClaim returns the user identified by a specific value for a given claim. GetUserByClaim(ctx context.Context, claim, value string, skipFetchingGroups bool) (*userpb.User, error) + // GetUserGroups returns the groups a user identified by a uid belongs to. GetUserGroups(ctx context.Context, uid *userpb.UserId) ([]string, error) + // FindUsers returns all the user objects which match a query parameter. FindUsers(ctx context.Context, query string, skipFetchingGroups bool) ([]*userpb.User, error) }