Skip to content

Commit

Permalink
Avoid copying proto messages
Browse files Browse the repository at this point in the history
protobuf messages are not supposed to be copied. This changes
the code to use pointers instead. Or proto.Clone() if a copy is
really needed.

Addresss ("copylocks: xxxx passes lock by value ...") issues raised by go-vet
  • Loading branch information
rhafer committed Jul 10, 2024
1 parent 98c4f37 commit a402931
Show file tree
Hide file tree
Showing 74 changed files with 344 additions and 315 deletions.
8 changes: 4 additions & 4 deletions internal/grpc/interceptors/auth/scope.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ func resolveLightweightScope(ctx context.Context, ref *provider.Reference, scope
}

for _, share := range shares.Shares {
shareKey := "lw:" + user.Id.OpaqueId + scopeDelimiter + storagespace.FormatResourceID(*share.Share.ResourceId)
shareKey := "lw:" + user.Id.OpaqueId + scopeDelimiter + storagespace.FormatResourceID(share.Share.ResourceId)
_ = scopeExpansionCache.SetWithExpire(shareKey, nil, scopeCacheExpiration*time.Second)

if ref.ResourceId != nil && utils.ResourceIDEqual(share.Share.ResourceId, ref.ResourceId) {
Expand Down Expand Up @@ -229,7 +229,7 @@ func checkRelativeReference(ctx context.Context, requested *provider.Reference,
}
}

key := storagespace.FormatResourceID(*sharedResourceID) + scopeDelimiter + getRefKey(requested)
key := storagespace.FormatResourceID(sharedResourceID) + scopeDelimiter + getRefKey(requested)
_ = scopeExpansionCache.SetWithExpire(key, nil, scopeCacheExpiration*time.Second)
return nil
}
Expand All @@ -246,7 +246,7 @@ func resolveUserShare(ctx context.Context, ref *provider.Reference, scope *authp

func checkCacheForNestedResource(ctx context.Context, ref *provider.Reference, resource *provider.ResourceId, client gateway.GatewayAPIClient, mgr token.Manager) error {
// Check if this ref is cached
key := storagespace.FormatResourceID(*resource) + scopeDelimiter + getRefKey(ref)
key := storagespace.FormatResourceID(resource) + scopeDelimiter + getRefKey(ref)
if _, err := scopeExpansionCache.Get(key); err == nil {
return nil
}
Expand Down Expand Up @@ -520,7 +520,7 @@ func getRefKey(ref *provider.Reference) string {
}

if ref.GetResourceId() != nil {
return storagespace.FormatResourceID(*ref.ResourceId)
return storagespace.FormatResourceID(ref.ResourceId)
}

// on malicious request both path and rid could be empty
Expand Down
5 changes: 3 additions & 2 deletions internal/grpc/services/gateway/authprovider.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import (
"github.com/cs3org/reva/v2/pkg/sharedconf"
"github.com/pkg/errors"
"google.golang.org/grpc/metadata"
"google.golang.org/protobuf/proto"
)

func (s *svc) Authenticate(ctx context.Context, req *gateway.AuthenticateRequest) (*gateway.AuthenticateResponse, error) {
Expand Down Expand Up @@ -92,12 +93,12 @@ func (s *svc) Authenticate(ctx context.Context, req *gateway.AuthenticateRequest
}, nil
}

u := *res.User
u := proto.Clone(res.User).(*userpb.User)
if sharedconf.SkipUserGroupsInToken() {
u.Groups = []string{}
}

token, err := s.tokenmgr.MintToken(ctx, &u, res.TokenScope)
token, err := s.tokenmgr.MintToken(ctx, u, res.TokenScope)
if err != nil {
err = errors.Wrap(err, "authsvc: error in MintToken")
res := &gateway.AuthenticateResponse{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ func (s *service) RemovePublicShare(ctx context.Context, req *link.RemovePublicS
Status: status.NewInternal(ctx, "error loading public share"),
}, err
}
if !publicshare.IsCreatedByUser(*ps, user) {
if !publicshare.IsCreatedByUser(ps, user) {
sRes, err := gatewayClient.Stat(ctx, &provider.StatRequest{Ref: &provider.Reference{ResourceId: ps.ResourceId}})
if err != nil {
log.Err(err).Interface("resource_id", ps.ResourceId).Msg("failed to stat shared resource")
Expand Down Expand Up @@ -480,7 +480,7 @@ func (s *service) UpdatePublicShare(ctx context.Context, req *link.UpdatePublicS
// users should always be able to downgrade links to internal links
// when they are the creator of the link
// all other users should have the WritePublicLink permission
if !isInternalLink && !publicshare.IsCreatedByUser(*ps, user) {
if !isInternalLink && !publicshare.IsCreatedByUser(ps, user) {
canWriteLink, err := utils.CheckPermission(ctx, permission.WritePublicLink, gatewayClient)
if err != nil {
return &link.UpdatePublicShareResponse{
Expand All @@ -502,7 +502,7 @@ func (s *service) UpdatePublicShare(ctx context.Context, req *link.UpdatePublicS
}, err
}

if !publicshare.IsCreatedByUser(*ps, user) {
if !publicshare.IsCreatedByUser(ps, user) {
if !sRes.GetInfo().GetPermissionSet().UpdateGrant {
return &link.UpdatePublicShareResponse{
Status: status.NewPermissionDenied(ctx, nil, "no permission to update public share"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,7 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
// we know a grant for this resource
space := &provider.StorageSpace{
Id: &provider.StorageSpaceId{
OpaqueId: storagespace.FormatResourceID(*root),
OpaqueId: storagespace.FormatResourceID(root),
},
SpaceType: "grant",
Owner: &userv1beta1.User{Id: grantee},
Expand Down Expand Up @@ -485,7 +485,7 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
}
space := &provider.StorageSpace{
Id: &provider.StorageSpaceId{
OpaqueId: storagespace.FormatResourceID(*root),
OpaqueId: storagespace.FormatResourceID(root),
},
SpaceType: "mountpoint",
Owner: &userv1beta1.User{Id: grantee}, // FIXME actually, the mount point belongs to no one?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,7 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
space := &provider.StorageSpace{
Opaque: opaque,
Id: &provider.StorageSpaceId{
OpaqueId: storagespace.FormatResourceID(*virtualRootID),
OpaqueId: storagespace.FormatResourceID(virtualRootID),
},
SpaceType: "virtual",
//Owner: &userv1beta1.User{Id: receivedShare.Share.Owner}, // FIXME actually, the mount point belongs to the recipient
Expand All @@ -454,7 +454,7 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
// we know a grant for this resource
space := &provider.StorageSpace{
Id: &provider.StorageSpaceId{
OpaqueId: storagespace.FormatResourceID(*root),
OpaqueId: storagespace.FormatResourceID(root),
},
SpaceType: "grant",
Owner: &userv1beta1.User{Id: receivedShare.Share.Owner},
Expand Down Expand Up @@ -508,7 +508,7 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
space := &provider.StorageSpace{
Opaque: opaque,
Id: &provider.StorageSpaceId{
OpaqueId: storagespace.FormatResourceID(*root),
OpaqueId: storagespace.FormatResourceID(root),
},
SpaceType: "mountpoint",
Owner: &userv1beta1.User{Id: receivedShare.Share.Owner}, // FIXME actually, the mount point belongs to the recipient
Expand Down Expand Up @@ -818,7 +818,7 @@ func (s *service) ListContainer(ctx context.Context, req *provider.ListContainer
if receivedShare.GetState() != collaboration.ShareState_SHARE_STATE_ACCEPTED {
continue
}
rIDStr := storagespace.FormatResourceID(*receivedShare.GetShare().GetResourceId())
rIDStr := storagespace.FormatResourceID(receivedShare.GetShare().GetResourceId())
if oldest, ok := oldestReceivedSharesByResourceID[rIDStr]; ok {
// replace if older than current oldest
if utils.TSToTime(receivedShare.GetShare().GetCtime()).Before(utils.TSToTime(oldest.GetShare().GetCtime())) {
Expand Down
4 changes: 2 additions & 2 deletions internal/grpc/services/storageprovider/storageprovider.go
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ func (s *Service) InitiateFileDownload(ctx context.Context, req *provider.Initia
if utils.IsRelativeReference(req.Ref) {
s.addMissingStorageProviderID(req.GetRef().GetResourceId(), nil)
protocol.Protocol = "spaces"
u.Path = path.Join(u.Path, "spaces", storagespace.FormatResourceID(*req.Ref.ResourceId), req.Ref.Path)
u.Path = path.Join(u.Path, "spaces", storagespace.FormatResourceID(req.Ref.ResourceId), req.Ref.Path)
} else {
// Currently, we only support the simple protocol for GET requests
// Once we have multiple protocols, this would be moved to the fs layer
Expand Down Expand Up @@ -612,7 +612,7 @@ func (s *Service) DeleteStorageSpace(ctx context.Context, req *provider.DeleteSt
// FIXME: why is this string parsing necessary?
idraw, _ := storagespace.ParseID(req.Id.GetOpaqueId())
idraw.OpaqueId = idraw.GetSpaceId()
id := &provider.StorageSpaceId{OpaqueId: storagespace.FormatResourceID(idraw)}
id := &provider.StorageSpaceId{OpaqueId: storagespace.FormatResourceID(&idraw)}

spaces, err := s.Storage.ListStorageSpaces(ctx, []*provider.ListStorageSpacesRequest_Filter{{Type: provider.ListStorageSpacesRequest_Filter_TYPE_ID, Term: &provider.ListStorageSpacesRequest_Filter_Id{Id: id}}}, true)
if err != nil {
Expand Down
9 changes: 6 additions & 3 deletions internal/http/services/appprovider/appprovider.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ import (
"github.com/mitchellh/mapstructure"
"github.com/pkg/errors"
"github.com/rs/zerolog"
"google.golang.org/protobuf/proto"
)

func init() {
Expand Down Expand Up @@ -304,7 +305,7 @@ func (s *svc) handleNew(w http.ResponseWriter, r *http.Request) {
writeError(w, r, appErrorInvalidParameter, "the given file id does not point to a file", nil)
return
}
fileid = storagespace.FormatResourceID(*statRes.Info.Id)
fileid = storagespace.FormatResourceID(statRes.Info.Id)
}

js, err := json.Marshal(
Expand Down Expand Up @@ -574,7 +575,8 @@ func buildApps(mimeTypes []*appregistry.MimeTypeInfo, userAgent, secureViewAppAd
for _, m := range mimeTypes {
apps := []*ProviderInfo{}
for _, p := range m.AppProviders {
ep := &ProviderInfo{ProviderInfo: *p}
ep := &ProviderInfo{}
proto.Merge(&ep.ProviderInfo, p)
if p.Address == secureViewAppAddr {
ep.SecureView = true
}
Expand All @@ -586,7 +588,8 @@ func buildApps(mimeTypes []*appregistry.MimeTypeInfo, userAgent, secureViewAppAd
}
}
if len(apps) > 0 {
mt := &MimeTypeInfo{MimeTypeInfo: *m}
mt := &MimeTypeInfo{}
proto.Merge(&mt.MimeTypeInfo, m)
mt.AppProviders = apps
res = append(res, mt)
}
Expand Down
4 changes: 2 additions & 2 deletions internal/http/services/owncloud/ocdav/copy.go
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ func (s *svc) executePathCopy(ctx context.Context, selector pool.Selectable[gate
return fmt.Errorf("status code %d", r.GetStatus().GetCode())
}

fileid = storagespace.FormatResourceID(*r.GetInfo().GetId())
fileid = storagespace.FormatResourceID(r.GetInfo().GetId())
} else {
// copy file

Expand Down Expand Up @@ -438,7 +438,7 @@ func (s *svc) executeSpacesCopy(ctx context.Context, w http.ResponseWriter, sele
return fmt.Errorf("stat: status code %d", r.GetStatus().GetCode())
}

fileid = storagespace.FormatResourceID(*r.GetInfo().GetId())
fileid = storagespace.FormatResourceID(r.GetInfo().GetId())
} else {
// copy file
// 1. get download url
Expand Down
2 changes: 1 addition & 1 deletion internal/http/services/owncloud/ocdav/dav.go
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ func (h *DavHandler) Handler(s *svc) http.Handler {
// If the link is internal then 307 redirect
if psRes.Status.Code == rpc.Code_CODE_OK && grants.PermissionsEqual(psRes.Share.Permissions.GetPermissions(), &provider.ResourcePermissions{}) {
if psRes.GetShare().GetResourceId() != nil {
rUrl := path.Join("/dav/spaces", storagespace.FormatResourceID(*psRes.GetShare().GetResourceId()))
rUrl := path.Join("/dav/spaces", storagespace.FormatResourceID(psRes.GetShare().GetResourceId()))
http.Redirect(w, r, rUrl, http.StatusTemporaryRedirect)
return
}
Expand Down
2 changes: 1 addition & 1 deletion internal/http/services/owncloud/ocdav/head.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ func (s *svc) handleHead(ctx context.Context, w http.ResponseWriter, r *http.Req
info := res.Info
w.Header().Set(net.HeaderContentType, info.MimeType)
w.Header().Set(net.HeaderETag, info.Etag)
w.Header().Set(net.HeaderOCFileID, storagespace.FormatResourceID(*info.Id))
w.Header().Set(net.HeaderOCFileID, storagespace.FormatResourceID(info.Id))
w.Header().Set(net.HeaderOCETag, info.Etag)
if info.Checksum != nil {
w.Header().Set(net.HeaderOCChecksum, fmt.Sprintf("%s:%s", strings.ToUpper(string(storageprovider.GRPC2PKGXS(info.Checksum.Type))), info.Checksum.Sum))
Expand Down
2 changes: 1 addition & 1 deletion internal/http/services/owncloud/ocdav/meta.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ func (h *MetaHandler) handlePathForUser(w http.ResponseWriter, r *http.Request,
ctx, span := appctx.GetTracerProvider(r.Context()).Tracer(tracerName).Start(r.Context(), "meta_propfind")
defer span.End()

id := storagespace.FormatResourceID(*rid)
id := storagespace.FormatResourceID(rid)
sublog := appctx.GetLogger(ctx).With().Str("path", r.URL.Path).Str("resourceid", id).Logger()
sublog.Info().Msg("calling get path for user")

Expand Down
2 changes: 1 addition & 1 deletion internal/http/services/owncloud/ocdav/move.go
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ func (s *svc) handleMove(ctx context.Context, w http.ResponseWriter, r *http.Req
info := dstStatRes.Info
w.Header().Set(net.HeaderContentType, info.MimeType)
w.Header().Set(net.HeaderETag, info.Etag)
w.Header().Set(net.HeaderOCFileID, storagespace.FormatResourceID(*info.Id))
w.Header().Set(net.HeaderOCFileID, storagespace.FormatResourceID(info.Id))
w.Header().Set(net.HeaderOCETag, info.Etag)
w.WriteHeader(successCode)
}
6 changes: 3 additions & 3 deletions internal/http/services/owncloud/ocdav/ocdav_blackbox_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ var _ = Describe("ocdav", func() {
},
},
},
Id: &cs3storageprovider.StorageSpaceId{OpaqueId: storagespace.FormatResourceID(cs3storageprovider.ResourceId{StorageId: "provider-1", SpaceId: "foospace", OpaqueId: "root"})},
Id: &cs3storageprovider.StorageSpaceId{OpaqueId: storagespace.FormatResourceID(&cs3storageprovider.ResourceId{StorageId: "provider-1", SpaceId: "foospace", OpaqueId: "root"})},
Root: &cs3storageprovider.ResourceId{StorageId: "provider-1", SpaceId: "userspace", OpaqueId: "root"},
Name: "username",
RootInfo: &cs3storageprovider.ResourceInfo{
Expand Down Expand Up @@ -864,7 +864,7 @@ var _ = Describe("ocdav", func() {
// setup the request
// set the webdav endpoint to test
basePath = "/webdav"
userspace.Id = &cs3storageprovider.StorageSpaceId{OpaqueId: storagespace.FormatResourceID(cs3storageprovider.ResourceId{StorageId: "provider-1", SpaceId: "userspace", OpaqueId: "userspace"})}
userspace.Id = &cs3storageprovider.StorageSpaceId{OpaqueId: storagespace.FormatResourceID(&cs3storageprovider.ResourceId{StorageId: "provider-1", SpaceId: "userspace", OpaqueId: "userspace"})}
userspace.Root = &cs3storageprovider.ResourceId{StorageId: "provider-1", SpaceId: "userspace", OpaqueId: "userspace"}

// path based requests at the /webdav endpoint first look up the storage space
Expand Down Expand Up @@ -938,7 +938,7 @@ var _ = Describe("ocdav", func() {
BeforeEach(func() {
basePath = "/dav/spaces"

userspace.Id = &cs3storageprovider.StorageSpaceId{OpaqueId: storagespace.FormatResourceID(cs3storageprovider.ResourceId{StorageId: "provider-1", SpaceId: "userspace", OpaqueId: "userspace"})}
userspace.Id = &cs3storageprovider.StorageSpaceId{OpaqueId: storagespace.FormatResourceID(&cs3storageprovider.ResourceId{StorageId: "provider-1", SpaceId: "userspace", OpaqueId: "userspace"})}
userspace.Root = &cs3storageprovider.ResourceId{StorageId: "provider-1", SpaceId: "userspace", OpaqueId: "userspace"}
// path based requests at the /webdav endpoint first look up the storage space
client.On("ListStorageSpaces", mock.Anything, mock.MatchedBy(func(req *cs3storageprovider.ListStorageSpacesRequest) bool {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import (

func TestWrapResourceID(t *testing.T) {
expected := "storageid" + "$" + "spaceid" + "!" + "opaqueid"
wrapped := storagespace.FormatResourceID(sprovider.ResourceId{StorageId: "storageid", SpaceId: "spaceid", OpaqueId: "opaqueid"})
wrapped := storagespace.FormatResourceID(&sprovider.ResourceId{StorageId: "storageid", SpaceId: "spaceid", OpaqueId: "opaqueid"})

if wrapped != expected {
t.Errorf("wrapped id doesn't have the expected format: got %s expected %s", wrapped, expected)
Expand Down
12 changes: 6 additions & 6 deletions internal/http/services/owncloud/ocdav/propfind/propfind.go
Original file line number Diff line number Diff line change
Expand Up @@ -1177,7 +1177,7 @@ func mdToPropResponse(ctx context.Context, pf *XML, md *provider.ResourceInfo, p
// return all known properties

if id != nil {
sid := storagespace.FormatResourceID(*id)
sid := storagespace.FormatResourceID(id)
appendToOK(
prop.Escaped("oc:id", sid),
prop.Escaped("oc:fileid", sid),
Expand All @@ -1186,7 +1186,7 @@ func mdToPropResponse(ctx context.Context, pf *XML, md *provider.ResourceInfo, p
}

if md.ParentId != nil {
appendToOK(prop.Escaped("oc:file-parent", storagespace.FormatResourceID(*md.ParentId)))
appendToOK(prop.Escaped("oc:file-parent", storagespace.FormatResourceID(md.ParentId)))
} else {
appendToNotFound(prop.NotFound("oc:file-parent"))
}
Expand Down Expand Up @@ -1305,19 +1305,19 @@ func mdToPropResponse(ctx context.Context, pf *XML, md *provider.ResourceInfo, p
// I tested the desktop client and phoenix to annotate which properties are requestted, see below cases
case "fileid": // phoenix only
if id != nil {
appendToOK(prop.Escaped("oc:fileid", storagespace.FormatResourceID(*id)))
appendToOK(prop.Escaped("oc:fileid", storagespace.FormatResourceID(id)))
} else {
appendToNotFound(prop.NotFound("oc:fileid"))
}
case "id": // desktop client only
if id != nil {
appendToOK(prop.Escaped("oc:id", storagespace.FormatResourceID(*id)))
appendToOK(prop.Escaped("oc:id", storagespace.FormatResourceID(id)))
} else {
appendToNotFound(prop.NotFound("oc:id"))
}
case "file-parent":
if md.ParentId != nil {
appendToOK(prop.Escaped("oc:file-parent", storagespace.FormatResourceID(*md.ParentId)))
appendToOK(prop.Escaped("oc:file-parent", storagespace.FormatResourceID(md.ParentId)))
} else {
appendToNotFound(prop.NotFound("oc:file-parent"))
}
Expand Down Expand Up @@ -1521,7 +1521,7 @@ func mdToPropResponse(ctx context.Context, pf *XML, md *provider.ResourceInfo, p
case "privatelink":
privateURL, err := url.Parse(publicURL)
if err == nil && id != nil {
privateURL.Path = path.Join(privateURL.Path, "f", storagespace.FormatResourceID(*id))
privateURL.Path = path.Join(privateURL.Path, "f", storagespace.FormatResourceID(id))
propstatOK.Prop = append(propstatOK.Prop, prop.Escaped("oc:privatelink", privateURL.String()))
} else {
propstatNotFound.Prop = append(propstatNotFound.Prop, prop.NotFound("oc:privatelink"))
Expand Down
Loading

0 comments on commit a402931

Please sign in to comment.