diff --git a/changelog/unreleased/ignore-stray-shares.md b/changelog/unreleased/ignore-stray-shares.md new file mode 100644 index 0000000000..c7172283d0 --- /dev/null +++ b/changelog/unreleased/ignore-stray-shares.md @@ -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 \ No newline at end of file diff --git a/internal/grpc/services/gateway/storageprovider.go b/internal/grpc/services/gateway/storageprovider.go index 656019cfaf..d83b75ca05 100644 --- a/internal/grpc/services/gateway/storageprovider.go +++ b/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,10 +256,13 @@ 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 } @@ -265,7 +270,6 @@ func (s *svc) InitiateFileUpload(ctx context.Context, req *provider.InitiateFile 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,10 +426,13 @@ 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 } @@ -433,7 +440,6 @@ func (s *svc) CreateContainer(ctx context.Context, req *provider.CreateContainer 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,10 +533,13 @@ 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 } @@ -538,7 +547,6 @@ func (s *svc) Delete(ctx context.Context, req *provider.DeleteRequest) (*provide 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: /@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") diff --git a/internal/http/services/owncloud/ocdav/move.go b/internal/http/services/owncloud/ocdav/move.go index bc28c9a2a6..fd919e050b 100644 --- a/internal/http/services/owncloud/ocdav/move.go +++ b/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 } 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 229665c009..236e84f29b 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 @@ -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) diff --git a/pkg/storage/fs/owncloud/owncloud.go b/pkg/storage/fs/owncloud/owncloud.go index a44f46263c..b229a3ff72 100644 --- a/pkg/storage/fs/owncloud/owncloud.go +++ b/pkg/storage/fs/owncloud/owncloud.go @@ -688,14 +688,32 @@ func readOrCreateID(ctx context.Context, np string, conn redis.Conn) string { } func (fs *ocfs) getPath(ctx context.Context, id *provider.ResourceId) (string, error) { + log := appctx.GetLogger(ctx) c := fs.pool.Get() defer c.Close() fs.scanFiles(ctx, c) np, err := redis.String(c.Do("GET", id.OpaqueId)) if err != nil { - appctx.GetLogger(ctx).Error().Err(err).Interface("id", id).Msg("error looking up fileid") - return "", err + return "", errtypes.NotFound(id.OpaqueId) + } + + idFromXattr, err := xattr.Get(np, idAttribute) + if err != nil { + return "", errtypes.NotFound(id.OpaqueId) + } + + uid, err := uuid.FromBytes(idFromXattr) + if err != nil { + log.Error().Err(err).Msg("error parsing uuid") + } + + if uid.String() != id.OpaqueId { + if _, err := c.Do("DEL", id.OpaqueId); err != nil { + return "", err + } + return "", errtypes.NotFound(id.OpaqueId) } + return np, nil } @@ -1490,6 +1508,9 @@ func (fs *ocfs) Move(ctx context.Context, oldRef, newRef *provider.Reference) (e func (fs *ocfs) GetMD(ctx context.Context, ref *provider.Reference, mdKeys []string) (*provider.ResourceInfo, error) { np, err := fs.resolve(ctx, ref) if err != nil { + if _, ok := err.(errtypes.IsNotFound); ok { + return nil, err + } return nil, errors.Wrap(err, "ocfs: error resolving reference") } p := fs.unwrap(ctx, np) diff --git a/tests/acceptance/expected-failures.txt b/tests/acceptance/expected-failures.txt index 5b9dedd4bb..02d7f75f55 100644 --- a/tests/acceptance/expected-failures.txt +++ b/tests/acceptance/expected-failures.txt @@ -900,7 +900,7 @@ apiWebdavUpload2/uploadFileUsingNewChunking.feature:137 apiWebdavUpload2/uploadFileUsingNewChunking.feature:157 apiWebdavUpload2/uploadFileUsingNewChunking.feature:158 # -# https://github.com/owncloud/ocis-reva/issues/17 uploading with old-chunking does not work +# https://github.com/owncloud/ocis-reva/issues/17 uploading with old-chunking does not work apiWebdavUpload2/uploadFileUsingOldChunking.feature:13 apiWebdavUpload2/uploadFileUsingOldChunking.feature:26 apiWebdavUpload2/uploadFileUsingOldChunking.feature:35 @@ -911,4 +911,4 @@ apiWebdavUpload2/uploadFileUsingOldChunking.feature:97 apiWebdavUpload2/uploadFileUsingOldChunking.feature:98 apiWebdavUpload2/uploadFileUsingOldChunking.feature:99 apiWebdavUpload2/uploadFileUsingOldChunking.feature:100 -apiWebdavUpload2/uploadFileUsingOldChunking.feature:101 +apiWebdavUpload2/uploadFileUsingOldChunking.feature:101 \ No newline at end of file diff --git a/tests/acceptance/features/apiOcisSpecific/apiShareManagementBasic-createShare.feature b/tests/acceptance/features/apiOcisSpecific/apiShareManagementBasic-createShare.feature index a4b6a73e74..998e370762 100644 --- a/tests/acceptance/features/apiOcisSpecific/apiShareManagementBasic-createShare.feature +++ b/tests/acceptance/features/apiOcisSpecific/apiShareManagementBasic-createShare.feature @@ -101,17 +101,17 @@ Feature: sharing And user "Alice" has shared folder "/folder1/folder2" with user "Emily" When user "Alice" sends HTTP method "GET" to OCS API endpoint "/apps/files_sharing/api/v1/shares" Then the OCS status code should be "" - And the HTTP status code should be "" - # On OCIS and reva the response is currently not there -# And the response should contain 4 entries -# And folder "/folder1" should be included as path in the response -# And folder "/folder1/folder2" should be included as path in the response + And the HTTP status code should be "200" + And the response should contain 4 entries + And folder "/folder1" should be included as path in the response + # And folder "/folder1/folder2" should be included as path in the response + And folder "/folder2" should be included as path in the response And user "Alice" sends HTTP method "GET" to OCS API endpoint "/apps/files_sharing/api/v1/shares?path=/folder1/folder2" And the response should contain 2 entries And folder "/folder1" should not be included as path in the response + # And folder "/folder1/folder2" should be included as path in the response And folder "/folder2" should be included as path in the response -# And folder "/folder1/folder2" should be included as path in the response Examples: - | ocs_api_version | http_status_code | ocs_status_code | - | 1 | 200 | 996 | - | 2 | 500 | 996 | + | ocs_api_version | ocs_status_code | + | 1 | 100 | + | 2 | 200 | \ No newline at end of file