Ensure stray shares get ignored

.drone.yml

@@ -342,9 +342,8 @@ steps:
     image: owncloudci/php:7.2
     commands:
       - git clone -b master --depth=1 https://github.com/owncloud/testing.git /drone/src/tmp/testing
-      - git clone -b master --single-branch --no-tags https://github.com/owncloud/core.git /drone/src/tmp/testrunner
+      - git clone -b testFixOCISPR409 --single-branch --no-tags https://github.com/owncloud/core.git /drone/src/tmp/testrunner
       - cd /drone/src/tmp/testrunner
-      - git checkout 9ebf886779b98fc3f78e11cf138246f80d5cab39
       - make test-acceptance-api
     environment:
       TEST_SERVER_URL: 'http://revad-services:20080'

changelog/unreleased/ensure-stray-shares.md

@@ -0,0 +1,5 @@
+Bugfix: Ensure stray shares get ignored
+
+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

internal/grpc/services/gateway/storageprovider.go

@@ -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{
@@ -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
 	}
 
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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
 	}
 
@@ -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
 	}
 
@@ -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)
@@ -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")
 }
 
@@ -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
 	}
 
@@ -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{
@@ -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/
@@ -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"]
 }
@@ -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")
 }
 
@@ -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"),
 	}
@@ -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")

internal/http/services/owncloud/ocdav/move.go

@@ -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
 		}
@@ -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 {
@@ -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
 	}

internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go

@@ -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)