diff --git a/changelog/unreleased/ocis-cache-displayname.md b/changelog/unreleased/ocis-cache-displayname.md new file mode 100644 index 0000000000..bfd9b72f9f --- /dev/null +++ b/changelog/unreleased/ocis-cache-displayname.md @@ -0,0 +1,5 @@ +Bugfix: Cache display names in ocs service + +The ocs list shares endpoint may need to fetch the displayname for multiple different users. We are now caching the lookup fo 60 seconds to save redundant RPCs to the users service. + +https://github.com/cs3org/reva/pull/1161 diff --git a/internal/http/services/owncloud/ocs/conversions/main.go b/internal/http/services/owncloud/ocs/conversions/main.go index 1b1d3bf689..4f74d924cf 100644 --- a/internal/http/services/owncloud/ocs/conversions/main.go +++ b/internal/http/services/owncloud/ocs/conversions/main.go @@ -20,6 +20,7 @@ package conversions import ( + "context" "fmt" "net/http" "path" @@ -272,6 +273,29 @@ func AsCS3Permissions(p int, rp *provider.ResourcePermissions) *provider.Resourc return rp } +// UserShare2ShareData converts a cs3api user share into shareData data model +// TODO(jfd) merge userShare2ShareData with publicShare2ShareData +func UserShare2ShareData(ctx context.Context, share *collaboration.Share) (*ShareData, error) { + sd := &ShareData{ + Permissions: UserSharePermissions2OCSPermissions(share.GetPermissions()), + ShareType: ShareTypeUser, + UIDOwner: LocalUserIDToString(share.GetCreator()), + UIDFileOwner: LocalUserIDToString(share.GetOwner()), + ShareWith: LocalUserIDToString(share.GetGrantee().GetId()), + } + + if share.Id != nil && share.Id.OpaqueId != "" { + sd.ID = share.Id.OpaqueId + } + if share.Ctime != nil { + sd.STime = share.Ctime.Seconds // TODO CS3 api birth time = btime + } + // actually clients should be able to GET and cache the user info themselves ... + // TODO only return the userid, let the clientso look up the displayname + // TODO check grantee type for user vs group + return sd, nil +} + // PublicShare2ShareData converts a cs3api public share into shareData data model func PublicShare2ShareData(share *link.PublicShare, r *http.Request, publicURL string) *ShareData { var expiration string @@ -281,30 +305,29 @@ func PublicShare2ShareData(share *link.PublicShare, r *http.Request, publicURL s expiration = "" } - shareWith := "" - if share.PasswordProtected { - shareWith = "***redacted***" + sd := &ShareData{ + // share.permissions are mapped below + // Displaynames are added later + ID: share.Id.OpaqueId, + ShareType: ShareTypePublicLink, + STime: share.Ctime.Seconds, // TODO CS3 api birth time = btime + Token: share.Token, + Expiration: expiration, + MimeType: share.Mtime.String(), + Name: share.DisplayName, + MailSend: 0, + URL: publicURL + path.Join("/", "#/s/"+share.Token), + Permissions: publicSharePermissions2OCSPermissions(share.GetPermissions()), + UIDOwner: LocalUserIDToString(share.Creator), + UIDFileOwner: LocalUserIDToString(share.Owner), } - return &ShareData{ - // share.permissions ar mapped below - // DisplaynameOwner: creator.DisplayName, - // DisplaynameFileOwner: share.GetCreator().String(), - ID: share.Id.OpaqueId, - ShareType: ShareTypePublicLink, - ShareWith: shareWith, - ShareWithDisplayname: shareWith, - STime: share.Ctime.Seconds, // TODO CS3 api birth time = btime - Token: share.Token, - Expiration: expiration, - MimeType: share.Mtime.String(), - Name: share.DisplayName, - MailSend: 0, - URL: publicURL + path.Join("/", "#/s/"+share.Token), - Permissions: publicSharePermissions2OCSPermissions(share.GetPermissions()), - UIDOwner: LocalUserIDToString(share.Creator), - UIDFileOwner: LocalUserIDToString(share.Owner), + if share.PasswordProtected { + sd.ShareWith = "***redacted***" + sd.ShareWithDisplayname = "***redacted***" } + + return sd // actually clients should be able to GET and cache the user info themselves ... // TODO check grantee type for user vs group } @@ -318,6 +341,8 @@ func LocalUserIDToString(userID *userpb.UserId) string { } // UserIDToString transforms a cs3api user id into an ocs data model +// TODO This should be used instead of LocalUserIDToString bit it requires interpreting an @ on the client side +// TODO An alternative would be to send the idp / iss as an additional attribute. might be less intrusive func UserIDToString(userID *userpb.UserId) string { if userID == nil || userID.OpaqueId == "" { return "" 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 a530ab292b..fe170753a5 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 @@ -30,6 +30,7 @@ import ( "strings" "time" + gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" invitepb "github.com/cs3org/go-cs3apis/cs3/ocm/invite/v1beta1" ocmprovider "github.com/cs3org/go-cs3apis/cs3/ocm/provider/v1beta1" @@ -47,19 +48,22 @@ import ( "github.com/cs3org/reva/pkg/appctx" "github.com/cs3org/reva/pkg/rgrpc/todo/pool" "github.com/cs3org/reva/pkg/rhttp/router" + "github.com/cs3org/reva/pkg/ttlmap" "github.com/pkg/errors" ) // Handler implements the shares part of the ownCloud sharing API type Handler struct { - gatewayAddr string - publicURL string + gatewayAddr string + publicURL string + displayNameCache *ttlmap.TTLMap } // Init initializes this and any contained handlers func (h *Handler) Init(c *config.Config) error { h.gatewayAddr = c.GatewaySvc h.publicURL = c.Config.Host + h.displayNameCache = ttlmap.New(1000, 60) return nil } @@ -298,7 +302,7 @@ func (h *Handler) createUserShare(w http.ResponseWriter, r *http.Request) { response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "grpc create share request failed", err) return } - s, err := h.userShare2ShareData(ctx, createShareResponse.Share) + s, err := conversions.UserShare2ShareData(ctx, createShareResponse.Share) if err != nil { response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error mapping share data", err) return @@ -308,6 +312,8 @@ func (h *Handler) createUserShare(w http.ResponseWriter, r *http.Request) { response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error adding fileinfo to share", err) return } + h.addDisplaynames(ctx, c, s) + response.WriteOCSSuccess(w, r, s) } @@ -423,11 +429,11 @@ func (h *Handler) createPublicLinkShare(w http.ResponseWriter, r *http.Request) s := conversions.PublicShare2ShareData(createRes.Share, r, h.publicURL) err = h.addFileInfo(ctx, s, statRes.Info) - if err != nil { response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error enhancing response with share data", err) return } + h.addDisplaynames(ctx, c, s) response.WriteOCSSuccess(w, r, s) } @@ -739,7 +745,7 @@ func (h *Handler) getShare(w http.ResponseWriter, r *http.Request, shareID strin if err == nil && uRes.GetShare() != nil { resourceID = uRes.Share.ResourceId - share, err = h.userShare2ShareData(ctx, uRes.Share) + share, err = conversions.UserShare2ShareData(ctx, uRes.Share) if err != nil { response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error mapping share data", err) return @@ -780,6 +786,7 @@ func (h *Handler) getShare(w http.ResponseWriter, r *http.Request, shareID strin log.Error().Err(err).Str("status", statResponse.Status.Code.String()).Msg("error mapping share data") response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error mapping share data", err) } + h.addDisplaynames(ctx, client, share) response.WriteOCSSuccess(w, r, []*conversions.ShareData{share}) } @@ -866,7 +873,7 @@ func (h *Handler) updateShare(w http.ResponseWriter, r *http.Request, shareID st return } - share, err := h.userShare2ShareData(ctx, gRes.Share) + share, err := conversions.UserShare2ShareData(ctx, gRes.Share) if err != nil { response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error mapping share data", err) return @@ -902,6 +909,7 @@ func (h *Handler) updateShare(w http.ResponseWriter, r *http.Request, shareID st response.WriteOCSError(w, r, response.MetaServerError.StatusCode, err.Error(), err) return } + h.addDisplaynames(ctx, uClient, share) response.WriteOCSSuccess(w, r, share) } @@ -1048,7 +1056,7 @@ func (h *Handler) listSharesWithMe(w http.ResponseWriter, r *http.Request) { return } - data, err := h.userShare2ShareData(r.Context(), rs.Share) + data, err := conversions.UserShare2ShareData(r.Context(), rs.Share) if err != nil { response.WriteOCSError(w, r, response.MetaServerError.StatusCode, err.Error(), err) return @@ -1070,6 +1078,7 @@ func (h *Handler) listSharesWithMe(w http.ResponseWriter, r *http.Request) { response.WriteOCSError(w, r, response.MetaServerError.StatusCode, err.Error(), err) return } + h.addDisplaynames(r.Context(), gwc, data) shares = append(shares, data) } @@ -1173,6 +1182,7 @@ func (h *Handler) listPublicShares(r *http.Request, filters []*link.ListPublicSh if h.addFileInfo(ctx, sData, statResponse.Info) != nil { return nil, err } + h.addDisplaynames(ctx, c, sData) log.Debug().Interface("share", share).Interface("info", statResponse.Info).Interface("shareData", share).Msg("mapped") @@ -1272,7 +1282,7 @@ func (h *Handler) listUserShares(r *http.Request, filters []*collaboration.ListS // build OCS response payload for _, s := range lsUserSharesResponse.Shares { - share, err := h.userShare2ShareData(ctx, s) + share, err := conversions.UserShare2ShareData(ctx, s) if err != nil { return nil, err } @@ -1305,6 +1315,7 @@ func (h *Handler) listUserShares(r *http.Request, filters []*collaboration.ListS if err != nil { return nil, err } + h.addDisplaynames(ctx, c, share) log.Debug().Interface("share", s).Interface("info", rInfo).Interface("shareData", share).Msg("mapped") ocsDataPayload = append(ocsDataPayload, share) @@ -1347,174 +1358,79 @@ func (h *Handler) addFileInfo(ctx context.Context, s *conversions.ShareData, inf // item type s.ItemType = conversions.ResourceType(info.GetType()).String() - c, err := pool.GetGatewayServiceClient(h.gatewayAddr) - if err != nil { - return err - } - - // owner, err := c.GetUser(ctx, &userpb.GetUserRequest{ - // UserId: info.Owner, - // }) - // if err != nil { - // return err - // } - - // if owner.Status.Code == rpc.Code_CODE_OK { - // // TODO the user from GetUser might not have an ID set, so we are using the one we have - // s.DisplaynameFileOwner = owner.GetUser().DisplayName - // } else { - // err := errors.New("could not look up share owner") - // log.Err(err). - // Str("user_idp", info.Owner.GetIdp()). - // Str("user_opaque_id", info.Owner.GetOpaqueId()). - // Str("code", owner.Status.Code.String()). - // Msg(owner.Status.Message) - // return err - // } - // file owner might not yet be set. Use file info if s.UIDFileOwner == "" { - // TODO we don't know if info.Owner is always set. - s.UIDFileOwner = response.UserIDToString(info.Owner) - } - if s.DisplaynameFileOwner == "" && info.Owner != nil { - owner, err := c.GetUser(ctx, &userpb.GetUserRequest{ - UserId: info.Owner, - }) - if err != nil { - return err - } - - if owner.Status.Code == rpc.Code_CODE_OK { - // TODO the user from GetUser might not have an ID set, so we are using the one we have - s.DisplaynameFileOwner = owner.GetUser().DisplayName - } else { - err := errors.New("could not look up share owner") - log.Err(err). - Str("user_idp", info.Owner.GetIdp()). - Str("user_opaque_id", info.Owner.GetOpaqueId()). - Str("code", owner.Status.Code.String()). - Msg(owner.Status.Message) - return err - } + s.UIDFileOwner = info.GetOwner().GetOpaqueId() } // share owner might not yet be set. Use file info if s.UIDOwner == "" { - // TODO we don't know if info.Owner is always set. - s.UIDOwner = response.UserIDToString(info.Owner) - } - if s.DisplaynameOwner == "" && info.Owner != nil { - owner, err := c.GetUser(ctx, &userpb.GetUserRequest{ - UserId: info.Owner, - }) - - if err != nil { - return err - } - - if owner.Status.Code == rpc.Code_CODE_OK { - // TODO the user from GetUser might not have an ID set, so we are using the one we have - s.DisplaynameOwner = owner.User.DisplayName - } else { - err := errors.New("could not look up file owner") - log.Err(err). - Str("user_idp", info.Owner.GetIdp()). - Str("user_opaque_id", info.Owner.GetOpaqueId()). - Str("code", owner.Status.Code.String()). - Msg(owner.Status.Message) - return err - } + s.UIDOwner = info.GetOwner().GetOpaqueId() } } return nil } -// TODO(jfd) merge userShare2ShareData with publicShare2ShareData -func (h *Handler) userShare2ShareData(ctx context.Context, share *collaboration.Share) (*conversions.ShareData, error) { - sd := &conversions.ShareData{ - Permissions: conversions.UserSharePermissions2OCSPermissions(share.GetPermissions()), - ShareType: conversions.ShareTypeUser, - } - - c, err := pool.GetGatewayServiceClient(h.gatewayAddr) - if err != nil { - return nil, err - } - +func (h *Handler) getDisplayname(ctx context.Context, c gateway.GatewayAPIClient, userid string) string { log := appctx.GetLogger(ctx) - - if share.Creator != nil { - creator, err := c.GetUser(ctx, &userpb.GetUserRequest{ - UserId: share.Creator, - }) - if err != nil { - return nil, err - } - - if creator.Status.Code == rpc.Code_CODE_OK { - // TODO the user from GetUser might not have an ID set, so we are using the one we have - sd.UIDOwner = response.UserIDToString(share.Creator) - sd.DisplaynameOwner = creator.GetUser().DisplayName - } else { - log.Err(errors.Wrap(err, "could not look up creator")). - Str("user_idp", share.Creator.GetIdp()). - Str("user_opaque_id", share.Creator.GetOpaqueId()). - Str("code", creator.Status.Code.String()). - Msg(creator.Status.Message) - return nil, err - } + if userid == "" { + return "" } - if share.Owner != nil { - owner, err := c.GetUser(ctx, &userpb.GetUserRequest{ - UserId: share.Owner, - }) - if err != nil { - return nil, err - } - - if owner.Status.Code == rpc.Code_CODE_OK { - // TODO the user from GetUser might not have an ID set, so we are using the one we have - sd.UIDFileOwner = response.UserIDToString(share.Owner) - sd.DisplaynameFileOwner = owner.GetUser().DisplayName - } else { - log.Err(errors.Wrap(err, "could not look up owner")). - Str("user_idp", share.Owner.GetIdp()). - Str("user_opaque_id", share.Owner.GetOpaqueId()). - Str("code", owner.Status.Code.String()). - Msg(owner.Status.Message) - return nil, err - } + if dn := h.displayNameCache.Get(userid); dn != "" { + log.Debug().Str("userid", userid).Msg("cache hit") + return dn } - if share.Grantee.Id != nil { - grantee, err := c.GetUser(ctx, &userpb.GetUserRequest{ - UserId: share.Grantee.GetId(), - }) - if err != nil { - return nil, err - } + log.Debug().Str("userid", userid).Msg("cache miss") + res, err := c.GetUser(ctx, &userpb.GetUserRequest{ + UserId: &userpb.UserId{ + OpaqueId: userid, + }, + }) + if err != nil { + log.Err(err). + Str("userid", userid). + Msg("could not look up user") + return "" + } + if res.GetStatus().GetCode() != rpc.Code_CODE_OK { + log.Err(err). + Str("opaque_id", userid). + Int32("code", int32(res.GetStatus().GetCode())). + Str("message", res.GetStatus().GetMessage()). + Msg("get user call failed") + return "" + } + if res.User == nil { + log.Debug(). + Str("opaque_id", userid). + Int32("code", int32(res.GetStatus().GetCode())). + Str("message", res.GetStatus().GetMessage()). + Msg("user not found") + return "" + } + if res.User.DisplayName == "" { + log.Debug(). + Str("opaque_id", userid). + Int32("code", int32(res.GetStatus().GetCode())). + Str("message", res.GetStatus().GetMessage()). + Msg("Displayname empty") + return "" + } + + h.displayNameCache.Put(userid, res.User.DisplayName) + log.Debug().Str("userid", userid).Msg("cache update") + return res.User.DisplayName +} - if grantee.Status.Code == rpc.Code_CODE_OK { - // TODO the user from GetUser might not have an ID set, so we are using the one we have - sd.ShareWith = response.UserIDToString(share.Grantee.Id) - sd.ShareWithDisplayname = grantee.GetUser().DisplayName - } else { - log.Err(errors.Wrap(err, "could not look up grantee")). - Str("user_idp", share.Grantee.GetId().GetIdp()). - Str("user_opaque_id", share.Grantee.GetId().GetOpaqueId()). - Str("code", grantee.Status.Code.String()). - Msg(grantee.Status.Message) - return nil, err - } +func (h *Handler) addDisplaynames(ctx context.Context, c gateway.GatewayAPIClient, s *conversions.ShareData) { + if s.DisplaynameOwner == "" { + s.DisplaynameOwner = h.getDisplayname(ctx, c, s.UIDOwner) } - if share.Id != nil && share.Id.OpaqueId != "" { - sd.ID = share.Id.OpaqueId + if s.DisplaynameFileOwner == "" { + s.DisplaynameFileOwner = h.getDisplayname(ctx, c, s.UIDFileOwner) } - if share.Ctime != nil { - sd.STime = share.Ctime.Seconds // TODO CS3 api birth time = btime + if s.ShareWithDisplayname == "" { + s.ShareWithDisplayname = h.getDisplayname(ctx, c, s.ShareWith) } - // actually clients should be able to GET and cache the user info themselves ... - // TODO check grantee type for user vs group - return sd, nil } func (h *Handler) isPublicShare(r *http.Request, oid string) bool { @@ -1723,11 +1639,11 @@ func (h *Handler) updatePublicShare(w http.ResponseWriter, r *http.Request, shar s := conversions.PublicShare2ShareData(publicShare, r, h.publicURL) err = h.addFileInfo(r.Context(), s, statRes.Info) - if err != nil { response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error enhancing response with share data", err) return } + h.addDisplaynames(r.Context(), gwC, s) response.WriteOCSSuccess(w, r, s) } diff --git a/internal/http/services/owncloud/ocs/response/response.go b/internal/http/services/owncloud/ocs/response/response.go index 2ca93e099c..4dedb1a30d 100644 --- a/internal/http/services/owncloud/ocs/response/response.go +++ b/internal/http/services/owncloud/ocs/response/response.go @@ -26,7 +26,6 @@ import ( "net/http" "reflect" - user "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" "github.com/cs3org/reva/pkg/appctx" ) @@ -183,14 +182,6 @@ func WriteOCSResponse(w http.ResponseWriter, r *http.Request, res Response, err } } -// UserIDToString returns a userid string with an optional idp separated by @: "[@]" -func UserIDToString(userID *user.UserId) string { - if userID == nil || userID.OpaqueId == "" { - return "" - } - return userID.OpaqueId -} - func encodeXML(res Response) ([]byte, error) { marshalled, err := xml.Marshal(res.OCS) if err != nil { diff --git a/pkg/ttlmap/ttlmap.go b/pkg/ttlmap/ttlmap.go new file mode 100644 index 0000000000..a559a37ea4 --- /dev/null +++ b/pkg/ttlmap/ttlmap.go @@ -0,0 +1,82 @@ +// Copyright 2018-2020 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 ttlmap + +import ( + "sync" + "time" +) + +// TTLMap is a simple kv cache, based on https://stackoverflow.com/a/25487392 +// The ttl of an item will be reset whenever it is read or written. +type TTLMap struct { + m map[string]*item + l sync.Mutex +} + +type item struct { + value string + lastAccess int64 +} + +// New creates a new ttl cache, preallocating space for ln items and the given maxttl +func New(ln int, maxTTL int) (m *TTLMap) { + m = &TTLMap{m: make(map[string]*item, ln)} + go func() { + for now := range time.Tick(time.Second) { + m.l.Lock() + for k, v := range m.m { + if now.Unix()-v.lastAccess > int64(maxTTL) { + delete(m.m, k) + } + } + m.l.Unlock() + } + }() + return +} + +// Len returns the current number of items in the cache +func (m *TTLMap) Len() int { + return len(m.m) +} + +// Put sets or overwrites an item, resetting the ttl +func (m *TTLMap) Put(k, v string) { + m.l.Lock() + it, ok := m.m[k] + if !ok { + it = &item{value: v} + m.m[k] = it + } + it.lastAccess = time.Now().Unix() + m.l.Unlock() +} + +// Get retrieves an item from the cache, resetting the ttl +func (m *TTLMap) Get(k string) (v string) { + m.l.Lock() + if it, ok := m.m[k]; ok { + v = it.value + it.lastAccess = time.Now().Unix() + } + m.l.Unlock() + return + +}