diff --git a/changelog/unreleased/revamp-rest-used-group-drivers.md b/changelog/unreleased/revamp-rest-used-group-drivers.md new file mode 100644 index 0000000000..f841f6f8d9 --- /dev/null +++ b/changelog/unreleased/revamp-rest-used-group-drivers.md @@ -0,0 +1,11 @@ +Enhancement: Revamp user/group drivers and fix user type +for lightweight accounts + +* Fix the user type for lightweight accounts, using the +source field to differentiate between a primary and lw account +* Remove all the code with manual parsing of the json returned +by the CERN provider +* Introduce pagination for `GetMembers` method in the group driver +* Reduced network transfer size by requesting only needed fields for `GetMembers` method + +https://github.com/cs3org/reva/pull/3821 diff --git a/pkg/cbox/group/rest/cache.go b/pkg/cbox/group/rest/cache.go index 84723bd610..2f2baf429b 100644 --- a/pkg/cbox/group/rest/cache.go +++ b/pkg/cbox/group/rest/cache.go @@ -106,14 +106,6 @@ func (m *manager) getVal(key string) (string, error) { return "", errors.New("rest: unable to get connection from redis pool") } -func (m *manager) fetchCachedInternalID(gid *grouppb.GroupId) (string, error) { - return m.getVal(groupPrefix + groupInternalIDPrefix + gid.OpaqueId) -} - -func (m *manager) cacheInternalID(gid *grouppb.GroupId, internalID string) error { - return m.setVal(groupPrefix+groupInternalIDPrefix+gid.OpaqueId, internalID, -1) -} - func (m *manager) findCachedGroups(query string) ([]*grouppb.Group, error) { conn := m.redisPool.Get() defer conn.Close() diff --git a/pkg/cbox/group/rest/rest.go b/pkg/cbox/group/rest/rest.go index f2501677f1..e7a8cc8bc2 100644 --- a/pkg/cbox/group/rest/rest.go +++ b/pkg/cbox/group/rest/rest.go @@ -20,8 +20,8 @@ package rest import ( "context" - "errors" "fmt" + "net/url" "os" "os/signal" "strings" @@ -31,9 +31,11 @@ import ( grouppb "github.com/cs3org/go-cs3apis/cs3/identity/group/v1beta1" userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" "github.com/cs3org/reva/pkg/appctx" + user "github.com/cs3org/reva/pkg/cbox/user/rest" utils "github.com/cs3org/reva/pkg/cbox/utils" "github.com/cs3org/reva/pkg/group" "github.com/cs3org/reva/pkg/group/manager/registry" + "github.com/cs3org/reva/pkg/utils/list" "github.com/gomodule/redigo/redis" "github.com/mitchellh/mapstructure" "github.com/rs/zerolog/log" @@ -126,12 +128,12 @@ func New(m map[string]interface{}) (group.Manager, error) { redisPool: redisPool, apiTokenManager: apiTokenManager, } - go mgr.fetchAllGroups() + go mgr.fetchAllGroups(context.Background()) return mgr, nil } -func (m *manager) fetchAllGroups() { - _ = m.fetchAllGroupAccounts() +func (m *manager) fetchAllGroups(ctx context.Context) { + _ = m.fetchAllGroupAccounts(ctx) ticker := time.NewTicker(time.Duration(m.conf.GroupFetchInterval) * time.Second) work := make(chan os.Signal, 1) signal.Notify(work, syscall.SIGHUP, syscall.SIGINT, syscall.SIGQUIT) @@ -141,89 +143,76 @@ func (m *manager) fetchAllGroups() { case <-work: return case <-ticker.C: - _ = m.fetchAllGroupAccounts() + _ = m.fetchAllGroupAccounts(ctx) } } } -func (m *manager) fetchAllGroupAccounts() error { - ctx := context.Background() +// Group contains the information about a group. +type Group struct { + GroupIdentifier string `json:"groupIdentifier"` + DisplayName string `json:"displayName"` + GID int `json:"gid,omitempty"` + IsComputingGroup bool `json:"isComputingGroup"` +} + +// GroupsResponse contains the expected response from grappa +// when getting the list of groups. +type GroupsResponse struct { + Pagination struct { + Links struct { + Next *string `json:"next"` + } `json:"links"` + } `json:"pagination"` + Data []*Group `json:"data"` +} + +func (m *manager) fetchAllGroupAccounts(ctx context.Context) error { url := fmt.Sprintf("%s/api/v1.0/Group?field=groupIdentifier&field=displayName&field=gid&field=isComputingGroup", m.conf.APIBaseURL) - for url != "" { - result, err := m.apiTokenManager.SendAPIGetRequest(ctx, url, false) - if err != nil { + var r GroupsResponse + for { + if err := m.apiTokenManager.SendAPIGetRequest(ctx, url, false, &r); err != nil { return err } - responseData, ok := result["data"].([]interface{}) - if !ok { - return errors.New("rest: error in type assertion") - } - for _, usr := range responseData { - groupData, ok := usr.(map[string]interface{}) - if !ok { - continue - } - - // filter computing groups - if is, ok := groupData["isComputingGroup"].(bool); ok && is { + for _, g := range r.Data { + if g.IsComputingGroup { continue } - - _, err = m.parseAndCacheGroup(ctx, groupData) - if err != nil { + if _, err := m.parseAndCacheGroup(ctx, g); err != nil { continue } } - url = "" - if pagination, ok := result["pagination"].(map[string]interface{}); ok { - if links, ok := pagination["links"].(map[string]interface{}); ok { - if next, ok := links["next"].(string); ok { - url = fmt.Sprintf("%s%s", m.conf.APIBaseURL, next) - } - } + if r.Pagination.Links.Next == nil { + break } + url = fmt.Sprintf("%s%s", m.conf.APIBaseURL, *r.Pagination.Links.Next) } return nil } -func (m *manager) parseAndCacheGroup(ctx context.Context, groupData map[string]interface{}) (*grouppb.Group, error) { - id, ok := groupData["groupIdentifier"].(string) - if !ok { - return nil, errors.New("rest: missing upn in user data") - } - - name, _ := groupData["displayName"].(string) +func (m *manager) parseAndCacheGroup(ctx context.Context, g *Group) (*grouppb.Group, error) { groupID := &grouppb.GroupId{ - OpaqueId: id, Idp: m.conf.IDProvider, + OpaqueId: g.GroupIdentifier, } - gid, ok := groupData["gid"].(int64) - if !ok { - gid = 0 - } - g := &grouppb.Group{ + + group := &grouppb.Group{ Id: groupID, - GroupName: id, - Mail: id + "@cern.ch", - DisplayName: name, - GidNumber: gid, + GroupName: g.GroupIdentifier, + Mail: g.GroupIdentifier + "@cern.ch", + DisplayName: g.DisplayName, + GidNumber: int64(g.GID), } - if err := m.cacheGroupDetails(g); err != nil { + if err := m.cacheGroupDetails(group); err != nil { log.Error().Err(err).Msg("rest: error caching group details") } - if internalID, ok := groupData["id"].(string); ok { - if err := m.cacheInternalID(groupID, internalID); err != nil { - log.Error().Err(err).Msg("rest: error caching group details") - } - } - - return g, nil + return group, nil } func (m *manager) GetGroup(ctx context.Context, gid *grouppb.GroupId, skipFetchingMembers bool) (*grouppb.Group, error) { @@ -288,38 +277,39 @@ func (m *manager) GetMembers(ctx context.Context, gid *grouppb.GroupId) ([]*user return users, nil } - internalID, err := m.fetchCachedInternalID(gid) - if err != nil { - return nil, err - } - url := fmt.Sprintf("%s/api/v1.0/Group/%s/memberidentities/precomputed", m.conf.APIBaseURL, internalID) - result, err := m.apiTokenManager.SendAPIGetRequest(ctx, url, false) + url, err := url.JoinPath(m.conf.APIBaseURL, "/api/v1.0/Group", gid.OpaqueId, "/memberidentities/precomputed?limit=10&field=upn&field=primaryAccountEmail&field=displayName&field=uid&field=gid&field=type&field=source") if err != nil { return nil, err } - userData := result["data"].([]interface{}) - users = []*userpb.UserId{} - - for _, u := range userData { - userInfo, ok := u.(map[string]interface{}) - if !ok { - return nil, errors.New("rest: error in type assertion") + var r user.IdentitiesResponse + members := []*userpb.UserId{} + for { + if err := m.apiTokenManager.SendAPIGetRequest(ctx, url, false, &r); err != nil { + return nil, err } - if id, ok := userInfo["upn"].(string); ok { - users = append(users, &userpb.UserId{OpaqueId: id, Idp: m.conf.IDProvider}) + + users := list.Map(r.Data, func(i *user.Identity) *userpb.UserId { + return &userpb.UserId{OpaqueId: i.Upn, Idp: m.conf.IDProvider, Type: i.UserType()} + }) + members = append(members, users...) + + if r.Pagination.Links.Next == nil { + break } + url = fmt.Sprintf("%s%s", m.conf.APIBaseURL, *r.Pagination.Links.Next) } - if err = m.cacheGroupMembers(gid, users); err != nil { - log := appctx.GetLogger(ctx) - log.Error().Err(err).Msg("rest: error caching group members") + if err = m.cacheGroupMembers(gid, members); err != nil { + appctx.GetLogger(ctx).Error().Err(err).Msg("rest: error caching group members") } return users, nil } func (m *manager) HasMember(ctx context.Context, gid *grouppb.GroupId, uid *userpb.UserId) (bool, error) { + // TODO (gdelmont): this can be improved storing the users a group is composed of as a list in redis + // and, instead of returning all the members, use the redis apis to check if the user is in the list. groupMemers, err := m.GetMembers(ctx, gid) if err != nil { return false, err diff --git a/pkg/cbox/user/rest/rest.go b/pkg/cbox/user/rest/rest.go index 443d86b0da..a3e1de1a23 100644 --- a/pkg/cbox/user/rest/rest.go +++ b/pkg/cbox/user/rest/rest.go @@ -32,9 +32,9 @@ import ( utils "github.com/cs3org/reva/pkg/cbox/utils" "github.com/cs3org/reva/pkg/user" "github.com/cs3org/reva/pkg/user/manager/registry" + "github.com/cs3org/reva/pkg/utils/list" "github.com/gomodule/redigo/redis" "github.com/mitchellh/mapstructure" - "github.com/pkg/errors" "github.com/rs/zerolog/log" ) @@ -134,12 +134,12 @@ func (m *manager) Configure(ml map[string]interface{}) error { // Since we're starting a subroutine which would take some time to execute, // we can't wait to see if it works before returning the user.Manager object // TODO: return err if the fetch fails - go m.fetchAllUsers() + go m.fetchAllUsers(context.Background()) return nil } -func (m *manager) fetchAllUsers() { - _ = m.fetchAllUserAccounts() +func (m *manager) fetchAllUsers(ctx context.Context) { + _ = m.fetchAllUserAccounts(ctx) ticker := time.NewTicker(time.Duration(m.conf.UserFetchInterval) * time.Second) work := make(chan os.Signal, 1) signal.Notify(work, syscall.SIGHUP, syscall.SIGINT, syscall.SIGQUIT) @@ -149,79 +149,94 @@ func (m *manager) fetchAllUsers() { case <-work: return case <-ticker.C: - _ = m.fetchAllUserAccounts() + _ = m.fetchAllUserAccounts(ctx) } } } -func (m *manager) fetchAllUserAccounts() error { - ctx := context.Background() - url := fmt.Sprintf("%s/api/v1.0/Identity?field=upn&field=primaryAccountEmail&field=displayName&field=uid&field=gid&field=type", m.conf.APIBaseURL) +// Identity contains the information of a single user. +type Identity struct { + PrimaryAccountEmail string `json:"primaryAccountEmail,omitempty"` + Type string `json:"type,omitempty"` + Upn string `json:"upn"` + DisplayName string `json:"displayName"` + Source string `json:"source,omitempty"` + UID int `json:"uid,omitempty"` + GID int `json:"gid,omitempty"` +} - for url != "" { - result, err := m.apiTokenManager.SendAPIGetRequest(ctx, url, false) - if err != nil { - return err +// IdentitiesResponse contains the expected response from grappa +// when getting the list of users. +type IdentitiesResponse struct { + Pagination struct { + Links struct { + Next *string `json:"next"` + } `json:"links"` + } `json:"pagination"` + Data []*Identity `json:"data"` +} + +// UserType convert the user type in grappa to CS3APIs. +func (i *Identity) UserType() userpb.UserType { + switch i.Type { + case "Application": + return userpb.UserType_USER_TYPE_APPLICATION + case "Service": + return userpb.UserType_USER_TYPE_SERVICE + case "Secondary": + return userpb.UserType_USER_TYPE_SECONDARY + case "Person": + if i.Source == "cern" { + return userpb.UserType_USER_TYPE_PRIMARY } + return userpb.UserType_USER_TYPE_LIGHTWEIGHT + default: + return userpb.UserType_USER_TYPE_INVALID + } +} + +func (m *manager) fetchAllUserAccounts(ctx context.Context) error { + url := fmt.Sprintf("%s/api/v1.0/Identity?field=upn&field=primaryAccountEmail&field=displayName&field=uid&field=gid&field=type&field=source", m.conf.APIBaseURL) - responseData, ok := result["data"].([]interface{}) - if !ok { - return errors.New("rest: error in type assertion") + var r IdentitiesResponse + for { + if err := m.apiTokenManager.SendAPIGetRequest(ctx, url, false, &r); err != nil { + return err } - for _, usr := range responseData { - userData, ok := usr.(map[string]interface{}) - if !ok { - continue - } - _, err = m.parseAndCacheUser(ctx, userData) - if err != nil { + for _, usr := range r.Data { + if _, err := m.parseAndCacheUser(ctx, usr); err != nil { continue } } - url = "" - if pagination, ok := result["pagination"].(map[string]interface{}); ok { - if links, ok := pagination["links"].(map[string]interface{}); ok { - if next, ok := links["next"].(string); ok { - url = fmt.Sprintf("%s%s", m.conf.APIBaseURL, next) - } - } + if r.Pagination.Links.Next == nil { + break } + url = fmt.Sprintf("%s%s", m.conf.APIBaseURL, *r.Pagination.Links.Next) } return nil } -func (m *manager) parseAndCacheUser(ctx context.Context, userData map[string]interface{}) (*userpb.User, error) { - upn, ok := userData["upn"].(string) - if !ok { - return nil, errors.New("rest: missing upn in user data") - } - mail, _ := userData["primaryAccountEmail"].(string) - name, _ := userData["displayName"].(string) - uidNumber, _ := userData["uid"].(float64) - gidNumber, _ := userData["gid"].(float64) - t, _ := userData["type"].(string) - userType := getUserType(t, upn) - - userID := &userpb.UserId{ - OpaqueId: upn, - Idp: m.conf.IDProvider, - Type: userType, - } +func (m *manager) parseAndCacheUser(ctx context.Context, i *Identity) (*userpb.User, error) { u := &userpb.User{ - Id: userID, - Username: upn, - Mail: mail, - DisplayName: name, - UidNumber: int64(uidNumber), - GidNumber: int64(gidNumber), + Id: &userpb.UserId{ + OpaqueId: i.Upn, + Idp: m.conf.IDProvider, + Type: i.UserType(), + }, + Username: i.Upn, + Mail: i.PrimaryAccountEmail, + DisplayName: i.DisplayName, + UidNumber: int64(i.UID), + GidNumber: int64(i.GID), } if err := m.cacheUserDetails(u); err != nil { log.Error().Err(err).Msg("rest: error caching user details") } + return u, nil } @@ -309,31 +324,37 @@ func isUserAnyType(user *userpb.User, types []userpb.UserType) bool { return false } +// Group contains the information about a group. +type Group struct { + DisplayName string `json:"displayName"` +} + +// GroupsResponse contains the expected response from grappa +// when getting the list of groups. +type GroupsResponse struct { + Pagination struct { + Links struct { + Next *string `json:"next"` + } `json:"links"` + } `json:"pagination"` + Data []Group `json:"data"` +} + func (m *manager) GetUserGroups(ctx context.Context, uid *userpb.UserId) ([]string, error) { groups, err := m.fetchCachedUserGroups(uid) if err == nil { return groups, nil } - url := fmt.Sprintf("%s/api/v1.0/Identity/%s/groups?recursive=true", m.conf.APIBaseURL, uid.OpaqueId) - result, err := m.apiTokenManager.SendAPIGetRequest(ctx, url, false) - if err != nil { + // TODO (gdelmont): support pagination! we may have problems with users having more than 1000 groups + url := fmt.Sprintf("%s/api/v1.0/Identity/%s/groups?field=displayName&recursive=true", m.conf.APIBaseURL, uid.OpaqueId) + + var r GroupsResponse + if err := m.apiTokenManager.SendAPIGetRequest(ctx, url, false, &r); err != nil { return nil, err } - groupData := result["data"].([]interface{}) - groups = []string{} - - for _, g := range groupData { - groupInfo, ok := g.(map[string]interface{}) - if !ok { - return nil, errors.New("rest: error in type assertion") - } - name, ok := groupInfo["displayName"].(string) - if ok { - groups = append(groups, name) - } - } + groups = list.Map(r.Data, func(g Group) string { return g.DisplayName }) if err = m.cacheUserGroups(uid, groups); err != nil { log := appctx.GetLogger(ctx) @@ -344,6 +365,8 @@ func (m *manager) GetUserGroups(ctx context.Context, uid *userpb.UserId) ([]stri } func (m *manager) IsInGroup(ctx context.Context, uid *userpb.UserId, group string) (bool, error) { + // TODO (gdelmont): this can be improved storing the groups a user belong to as a list in redis + // and, instead of returning all the groups, use the redis apis to check if the group is in the list. userGroups, err := m.GetUserGroups(ctx, uid) if err != nil { return false, err @@ -356,27 +379,3 @@ func (m *manager) IsInGroup(ctx context.Context, uid *userpb.UserId, group strin } return false, nil } - -func getUserType(userType, upn string) userpb.UserType { - var t userpb.UserType - switch userType { - case "Application": - t = userpb.UserType_USER_TYPE_APPLICATION - case "Service": - t = userpb.UserType_USER_TYPE_SERVICE - case "Secondary": - t = userpb.UserType_USER_TYPE_SECONDARY - case "Person": - switch { - case strings.HasPrefix(upn, "guest"): - t = userpb.UserType_USER_TYPE_LIGHTWEIGHT - case strings.Contains(upn, "@"): - t = userpb.UserType_USER_TYPE_FEDERATED - default: - t = userpb.UserType_USER_TYPE_PRIMARY - } - default: - t = userpb.UserType_USER_TYPE_INVALID - } - return t -} diff --git a/pkg/cbox/utils/tokenmanagement.go b/pkg/cbox/utils/tokenmanagement.go index 19d78924dc..6eea766d92 100644 --- a/pkg/cbox/utils/tokenmanagement.go +++ b/pkg/cbox/utils/tokenmanagement.go @@ -128,15 +128,15 @@ func (a *APITokenManager) getAPIToken(ctx context.Context) (string, time.Time, e } // SendAPIGetRequest makes an API GET Request to the passed URL. -func (a *APITokenManager) SendAPIGetRequest(ctx context.Context, url string, forceRenewal bool) (map[string]interface{}, error) { +func (a *APITokenManager) SendAPIGetRequest(ctx context.Context, url string, forceRenewal bool, v any) error { err := a.renewAPIToken(ctx, forceRenewal) if err != nil { - return nil, err + return err } httpReq, err := http.NewRequest(http.MethodGet, url, nil) if err != nil { - return nil, err + return err } // We don't need to take the lock when reading apiToken, because if we reach here, @@ -146,28 +146,17 @@ func (a *APITokenManager) SendAPIGetRequest(ctx context.Context, url string, for httpRes, err := a.client.Do(httpReq) if err != nil { - return nil, err + return err } defer httpRes.Body.Close() if httpRes.StatusCode == http.StatusUnauthorized { // The token is no longer valid, try renewing it - return a.SendAPIGetRequest(ctx, url, true) + return a.SendAPIGetRequest(ctx, url, true, v) } if httpRes.StatusCode < 200 || httpRes.StatusCode > 299 { - return nil, errors.New("rest: API request returned " + httpRes.Status) - } - - body, err := io.ReadAll(httpRes.Body) - if err != nil { - return nil, err - } - - var result map[string]interface{} - err = json.Unmarshal(body, &result) - if err != nil { - return nil, err + return errors.New("rest: API request returned " + httpRes.Status) } - return result, nil + return json.NewDecoder(httpRes.Body).Decode(v) } diff --git a/pkg/utils/list/list.go b/pkg/utils/list/list.go new file mode 100644 index 0000000000..ba83b2de86 --- /dev/null +++ b/pkg/utils/list/list.go @@ -0,0 +1,29 @@ +// Copyright 2018-2023 CERN +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// In applying this license, CERN does not waive the privileges and immunities +// granted to it by virtue of its status as an Intergovernmental Organization +// or submit itself to any jurisdiction. + +package list + +// Map returns a list constructed by appling a function f +// to all items in the list l. +func Map[T, V any](l []T, f func(T) V) []V { + m := make([]V, 0, len(l)) + for _, e := range l { + m = append(m, f(e)) + } + return m +}