Skip to content

Commit

Permalink
Ensure stray shares get ignored (#1064)
Browse files Browse the repository at this point in the history
  • Loading branch information
@refs
refs authored Aug 12, 2020
1 parent 4f25d27 commit f325194
Show file tree
Hide file tree
Showing 7 changed files with 110 additions and 75 deletions.
5 changes: 5 additions & 0 deletions changelog/unreleased/ignore-stray-shares.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Bugfix: Ensure ignoring stray shares

When using the json shares manager, it can be the case we found a share with a resource_id that no longer exists. This PR aims to address his case by changing the contract of getPath and return the result of the STAT call instead of a generic error, so we can check the cause.

https://github.com/cs3org/reva/pull/1064
121 changes: 60 additions & 61 deletions internal/grpc/services/gateway/storageprovider.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ type transferClaims struct {
Target string `json:"target"`
}

func (s *svc) sign(ctx context.Context, target string) (string, error) {
func (s *svc) sign(_ context.Context, target string) (string, error) {
ttl := time.Duration(s.c.TransferExpires) * time.Second
claims := transferClaims{
StandardClaims: jwt.StandardClaims{
Expand Down Expand Up @@ -93,21 +93,24 @@ func (s *svc) CreateHome(ctx context.Context, req *provider.CreateHomeRequest) (
return res, nil

}
func (s *svc) GetHome(ctx context.Context, req *provider.GetHomeRequest) (*provider.GetHomeResponse, error) {
func (s *svc) GetHome(ctx context.Context, _ *provider.GetHomeRequest) (*provider.GetHomeResponse, error) {
home := s.getHome(ctx)
homeRes := &provider.GetHomeResponse{Path: home, Status: status.NewOK(ctx)}
return homeRes, nil
}

func (s *svc) getHome(ctx context.Context) string {
func (s *svc) getHome(_ context.Context) string {
// TODO(labkode): issue #601, /home will be hardcoded.
return "/home"
}
func (s *svc) InitiateFileDownload(ctx context.Context, req *provider.InitiateFileDownloadRequest) (*gateway.InitiateFileDownloadResponse, error) {
p, err := s.getPath(ctx, req.Ref)
if err != nil {
log := appctx.GetLogger(ctx)
p, st := s.getPath(ctx, req.Ref)
if st.Code != rpc.Code_CODE_OK {
log.Error().Str("rpc_code", st.Code.String()).
Msgf("error initiating file download id: %v", req.Ref.GetId())
return &gateway.InitiateFileDownloadResponse{
Status: status.NewInternal(ctx, err, "gateway: error gettng path for ref"),
Status: st,
}, nil
}

Expand All @@ -133,7 +136,6 @@ func (s *svc) InitiateFileDownload(ctx context.Context, req *provider.InitiateFi
return s.initiateFileDownload(ctx, req)
}

log := appctx.GetLogger(ctx)
if s.isSharedFolder(ctx, p) || s.isShareName(ctx, p) {
log.Debug().Msgf("path:%s points to shared folder or share name", p)
err := errtypes.PermissionDenied("gateway: cannot download share folder or share name: path=" + p)
Expand Down Expand Up @@ -254,18 +256,20 @@ func (s *svc) initiateFileDownload(ctx context.Context, req *provider.InitiateFi
}

func (s *svc) InitiateFileUpload(ctx context.Context, req *provider.InitiateFileUploadRequest) (*gateway.InitiateFileUploadResponse, error) {
p, err := s.getPath(ctx, req.Ref)
if err != nil {
log := appctx.GetLogger(ctx)
p, st := s.getPath(ctx, req.Ref)
if st.Code != rpc.Code_CODE_OK {
log.Error().Str("rpc_code", st.Code.String()).
Msgf("error initiating file upload id: %v", req.Ref.GetId())
return &gateway.InitiateFileUploadResponse{
Status: status.NewInternal(ctx, err, "gateway: error gettng path for ref"),
Status: st,
}, nil
}

if !s.inSharedFolder(ctx, p) {
return s.initiateFileUpload(ctx, req)
}

log := appctx.GetLogger(ctx)
if s.isSharedFolder(ctx, p) || s.isShareName(ctx, p) {
log.Debug().Msgf("path:%s points to shared folder or share name", p)
err := errtypes.PermissionDenied("gateway: cannot upload to share folder or share name: path=" + p)
Expand Down Expand Up @@ -422,18 +426,20 @@ func (s *svc) GetPath(ctx context.Context, req *provider.GetPathRequest) (*provi
}

func (s *svc) CreateContainer(ctx context.Context, req *provider.CreateContainerRequest) (*provider.CreateContainerResponse, error) {
p, err := s.getPath(ctx, req.Ref)
if err != nil {
log := appctx.GetLogger(ctx)
p, st := s.getPath(ctx, req.Ref)
if st.Code != rpc.Code_CODE_OK {
log.Error().Str("rpc_code", st.Code.String()).
Msgf("error creating container on reference id: %v", req.Ref.GetId())
return &provider.CreateContainerResponse{
Status: status.NewInternal(ctx, err, "gateway: error gettng path for ref"),
Status: st,
}, nil
}

if !s.inSharedFolder(ctx, p) {
return s.createContainer(ctx, req)
}

log := appctx.GetLogger(ctx)
if s.isSharedFolder(ctx, p) || s.isShareName(ctx, p) {
log.Debug().Msgf("path:%s points to shared folder or share name", p)
err := errtypes.PermissionDenied("gateway: cannot create container on share folder or share name: path=" + p)
Expand Down Expand Up @@ -527,18 +533,20 @@ func (s *svc) inSharedFolder(ctx context.Context, p string) bool {
}

func (s *svc) Delete(ctx context.Context, req *provider.DeleteRequest) (*provider.DeleteResponse, error) {
p, err := s.getPath(ctx, req.Ref)
if err != nil {
log := appctx.GetLogger(ctx)
p, st := s.getPath(ctx, req.Ref)
if st.Code != rpc.Code_CODE_OK {
log.Error().Str("rpc_code", st.Code.String()).
Msgf("error deleting reference id: %v", req.Ref.GetId())
return &provider.DeleteResponse{
Status: status.NewInternal(ctx, err, "gateway: error gettng path for ref"),
Status: st,
}, nil
}

if !s.inSharedFolder(ctx, p) {
return s.delete(ctx, req)
}

log := appctx.GetLogger(ctx)
if s.isSharedFolder(ctx, p) {
// TODO(labkode): deleting share names should be allowed, means unmounting.
log.Debug().Msgf("path:%s points to shared folder or share name", p)
Expand Down Expand Up @@ -643,20 +651,19 @@ func (s *svc) delete(ctx context.Context, req *provider.DeleteRequest) (*provide

func (s *svc) Move(ctx context.Context, req *provider.MoveRequest) (*provider.MoveResponse, error) {
log := appctx.GetLogger(ctx)

p, err := s.getPath(ctx, req.Source)
if err != nil {
log.Err(err).Msg("gateway: error moving")
p, st := s.getPath(ctx, req.Source)
if st.Code != rpc.Code_CODE_OK {
log.Error().Str("rpc_code", st.Code.String()).
Msgf("error moving reference id: %v to `%v`", req.Source.GetId(), req.Destination.String())
return &provider.MoveResponse{
Status: status.NewInternal(ctx, err, "gateway: error gettng path for ref"),
Status: st,
}, nil
}

dp, err := s.getPath(ctx, req.Destination)
if err != nil {
log.Err(err).Msg("gateway: error moving")
dp, st2 := s.getPath(ctx, req.Destination)
if st2.Code != rpc.Code_CODE_OK {
return &provider.MoveResponse{
Status: status.NewInternal(ctx, err, "gateway: error gettng path for ref"),
Status: st,
}, nil
}

Expand Down Expand Up @@ -844,10 +851,13 @@ func (s *svc) stat(ctx context.Context, req *provider.StatRequest) (*provider.St
}

func (s *svc) Stat(ctx context.Context, req *provider.StatRequest) (*provider.StatResponse, error) {
p, err := s.getPath(ctx, req.Ref, req.ArbitraryMetadataKeys...)
if err != nil {
log := appctx.GetLogger(ctx)
p, st := s.getPath(ctx, req.Ref, req.ArbitraryMetadataKeys...)
if st.Code != rpc.Code_CODE_OK {
log.Error().Str("rpc_code", st.Code.String()).
Msgf("error during STAT id: %v", req.Ref.GetId())
return &provider.StatResponse{
Status: status.NewInternal(ctx, err, "gateway: error getting path for ref"),
Status: st,
}, nil
}

Expand All @@ -860,8 +870,6 @@ func (s *svc) Stat(ctx context.Context, req *provider.StatRequest) (*provider.St
return s.stat(ctx, req)
}

log := appctx.GetLogger(ctx)

// we need to provide the info of the target, not the reference.
if s.isShareName(ctx, p) {
res, err := s.stat(ctx, req)
Expand Down Expand Up @@ -1048,13 +1056,13 @@ func (s *svc) handleCS3Ref(ctx context.Context, opaque string) (*provider.Resour
return res.Info, nil
}

func (s *svc) handleWebdavRef(ctx context.Context, ri *provider.ResourceInfo) (*provider.ResourceInfo, error) {
func (s *svc) handleWebdavRef(_ context.Context, ri *provider.ResourceInfo) (*provider.ResourceInfo, error) {
// A webdav ref has the following layout: <storage_id>/<opaque_id>@webdav_endpoint
// TODO: Once file transfer functionalities have been added.
return ri, nil
}

func (s *svc) ListContainerStream(req *provider.ListContainerStreamRequest, ss gateway.GatewayAPI_ListContainerStreamServer) error {
func (s *svc) ListContainerStream(_ *provider.ListContainerStreamRequest, _ gateway.GatewayAPI_ListContainerStreamServer) error {
return errors.New("Unimplemented")
}

Expand All @@ -1080,10 +1088,13 @@ func (s *svc) listContainer(ctx context.Context, req *provider.ListContainerRequ
}

func (s *svc) ListContainer(ctx context.Context, req *provider.ListContainerRequest) (*provider.ListContainerResponse, error) {
p, err := s.getPath(ctx, req.Ref, req.ArbitraryMetadataKeys...)
if err != nil {
log := appctx.GetLogger(ctx)
p, st := s.getPath(ctx, req.Ref, req.ArbitraryMetadataKeys...)
if st.Code != rpc.Code_CODE_OK {
log.Error().Str("rpc_code", st.Code.String()).
Msgf("error listing directory id: %v", req.Ref.GetId())
return &provider.ListContainerResponse{
Status: status.NewInternal(ctx, err, "gateway: error getting path for ref"),
Status: st,
}, nil
}

Expand All @@ -1102,25 +1113,19 @@ func (s *svc) ListContainer(ctx context.Context, req *provider.ListContainerRequ
}

for i, ref := range lcr.Infos {

info, err := s.checkRef(ctx, ref)
if err != nil {
return &provider.ListContainerResponse{
Status: status.NewInternal(ctx, err, "gateway: error resolving reference:"+info.Path),
Status: status.NewInternal(ctx, err, "gateway: error resolving reference:"+ref.Path),
}, nil
}

base := path.Base(ref.Path)
info.Path = path.Join(p, base)

lcr.Infos[i] = info

}
return lcr, nil
}

log := appctx.GetLogger(ctx)

// we need to provide the info of the target, not the reference.
if s.isShareName(ctx, p) {
ref := &provider.Reference{
Expand Down Expand Up @@ -1268,28 +1273,22 @@ func (s *svc) ListContainer(ctx context.Context, req *provider.ListContainerRequ
panic("gateway: stating an unknown path:" + p)
}

func (s *svc) getPath(ctx context.Context, ref *provider.Reference, keys ...string) (string, error) {
func (s *svc) getPath(ctx context.Context, ref *provider.Reference, keys ...string) (string, *rpc.Status) {
if ref.GetPath() != "" {
return ref.GetPath(), nil
return ref.GetPath(), &rpc.Status{Code: rpc.Code_CODE_OK}
}

if ref.GetId() != nil && ref.GetId().GetOpaqueId() != "" {
req := &provider.StatRequest{Ref: ref, ArbitraryMetadataKeys: keys}
res, err := s.stat(ctx, req)
if err != nil {
err = errors.Wrap(err, "gateway: error stating ref:"+ref.String())
return "", err
}

if res.Status.Code != rpc.Code_CODE_OK {
err := status.NewErrorFromCode(res.Status.Code, "gateway")
return "", err
if (res != nil && res.Status.Code != rpc.Code_CODE_OK) || err != nil {
return "", res.Status
}

return res.Info.Path, nil
return res.Info.Path, res.Status
}

return "", errors.New("gateway: ref is invalid:" + ref.String())
return "", &rpc.Status{Code: rpc.Code_CODE_INTERNAL}
}

// /home/MyShares/
Expand Down Expand Up @@ -1350,7 +1349,7 @@ func (s *svc) splitShare(ctx context.Context, p string) (string, string) {
return shareName, shareChild
}

func (s *svc) splitPath(ctx context.Context, p string) []string {
func (s *svc) splitPath(_ context.Context, p string) []string {
p = strings.Trim(p, "/")
return strings.SplitN(p, "/", 4) // ["home", "MyShares", "photos", "Ibiza/beach.png"]
}
Expand Down Expand Up @@ -1403,7 +1402,7 @@ func (s *svc) RestoreFileVersion(ctx context.Context, req *provider.RestoreFileV
return res, nil
}

func (s *svc) ListRecycleStream(req *gateway.ListRecycleStreamRequest, ss gateway.GatewayAPI_ListRecycleStreamServer) error {
func (s *svc) ListRecycleStream(_ *gateway.ListRecycleStreamRequest, _ gateway.GatewayAPI_ListRecycleStreamServer) error {
return errors.New("Unimplemented")
}

Expand Down Expand Up @@ -1478,7 +1477,7 @@ func (s *svc) PurgeRecycle(ctx context.Context, req *gateway.PurgeRecycleRequest
return res, nil
}

func (s *svc) GetQuota(ctx context.Context, req *gateway.GetQuotaRequest) (*provider.GetQuotaResponse, error) {
func (s *svc) GetQuota(ctx context.Context, _ *gateway.GetQuotaRequest) (*provider.GetQuotaResponse, error) {
res := &provider.GetQuotaResponse{
Status: status.NewUnimplemented(ctx, nil, "GetQuota not yet implemented"),
}
Expand Down Expand Up @@ -1511,7 +1510,7 @@ func (s *svc) find(ctx context.Context, ref *provider.Reference) (provider.Provi
return s.getStorageProviderClient(ctx, p)
}

func (s *svc) getStorageProviderClient(ctx context.Context, p *registry.ProviderInfo) (provider.ProviderAPIClient, error) {
func (s *svc) getStorageProviderClient(_ context.Context, p *registry.ProviderInfo) (provider.ProviderAPIClient, error) {
c, err := pool.GetStorageProviderServiceClient(p.Address)
if err != nil {
err = errors.Wrap(err, "gateway: error getting a storage provider client")
Expand Down
4 changes: 4 additions & 0 deletions internal/http/services/owncloud/ocdav/move.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ func (s *svc) handleMove(w http.ResponseWriter, r *http.Request, ns string) {

// TODO return a forbidden status if read only?
if delRes.Status.Code != rpc.Code_CODE_OK {
log.Error().Err(err).Str("move_request", delRes.Status.Code.String()).Msg("error handling delete request")
w.WriteHeader(http.StatusInternalServerError)
return
}
Expand Down Expand Up @@ -179,9 +180,11 @@ func (s *svc) handleMove(w http.ResponseWriter, r *http.Request, ns string) {
}

if mRes.Status.Code != rpc.Code_CODE_OK {
log.Error().Err(err).Str("move_request", mRes.Status.Code.String()).Msg("error handling move request")
w.WriteHeader(http.StatusInternalServerError)
return
}
log.Info().Str("move", mRes.Status.Code.String()).Msg("move request status")

dstStatRes, err = client.Stat(ctx, dstStatReq)
if err != nil {
Expand All @@ -191,6 +194,7 @@ func (s *svc) handleMove(w http.ResponseWriter, r *http.Request, ns string) {
}

if dstStatRes.Status.Code != rpc.Code_CODE_OK {
log.Error().Err(err).Str("status", dstStatRes.Status.Code.String()).Msg("error doing stat")
w.WriteHeader(http.StatusInternalServerError)
return
}
Expand Down
8 changes: 7 additions & 1 deletion internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1295,7 +1295,13 @@ func (h *Handler) listUserShares(r *http.Request, filters []*collaboration.ListS
}

if statResponse.Status.Code != rpc.Code_CODE_OK {
return nil, errors.New("could not stat share target")
if statResponse.Status.Code == rpc.Code_CODE_NOT_FOUND {
// TODO share target was not found, we should not error here.
// return nil, errors.New(fmt.Sprintf("could not stat share target: %v, code: %v", s.ResourceId, statResponse.Status))
continue
}

return nil, errors.New(fmt.Sprintf("could not stat share target: %v, code: %v", s.ResourceId, statResponse.Status))
}

err = h.addFileInfo(ctx, share, statResponse.Info)
Expand Down
Loading

0 comments on commit f325194

Please sign in to comment.