Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure stray shares get ignored #1064

Merged
merged 21 commits into from
Aug 12, 2020
Merged

Ensure stray shares get ignored #1064

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
50420e1
ignore stray shares
refs Aug 6, 2020
95927bf
Merge branch 'master' into fix-error-propagation
refs Aug 6, 2020
7650a6d
stray block
refs Aug 7, 2020
18ff637
log reason of STAT failure
refs Aug 7, 2020
c433b7d
fix warnings and offenses
refs Aug 7, 2020
6bd190f
add changelog
refs Aug 7, 2020
0ef80b3
fix move1
refs Aug 10, 2020
79c6a59
test using testFixOCISPR409
refs Aug 10, 2020
e10e878
Merge branch 'master' into fix-error-propagation
refs Aug 10, 2020
d6f059f
use reva-tests branch
refs Aug 10, 2020
f0aeb92
Merge branch 'master' into fix-error-propagation
refs Aug 11, 2020
26576d1
make getPath deal with out of sync cached values
refs Aug 11, 2020
391ad65
add changelog
refs Aug 11, 2020
8dc7c90
update drone commit
refs Aug 12, 2020
7dc3bd5
Revert "update drone commit"
refs Aug 12, 2020
0146550
Merge branch 'master' into fix-error-propagation
refs Aug 12, 2020
1b1b910
copy feature over from core
refs Aug 12, 2020
2489aad
adapt assert
refs Aug 12, 2020
5a104df
add expected failures on owncloud/core to the list of expected failures
refs Aug 12, 2020
ee3bf31
re-add missing comment
refs Aug 12, 2020
b00e51f
get rid of expected failures (╯°□°）╯︵ ┻━┻
refs Aug 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions changelog/unreleased/ignore-stray-shares.md
View file
Open in desktop
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
Open in desktop
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 {
refs marked this conversation as resolved.
Show resolved Hide resolved
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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to check res != nil? There might arise cases where we might have initialized it and then run into some error but returned it anyway.

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
Open in desktop
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
Open in desktop
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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add this in listPublicShares?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

totally. I'd get hands on it tomorrow morning first thing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually giving this a second thought, as this PR has been messy enough with the testing I would like to create a new one for these changes and keep changes as "atomic" as possible. This PR is blocking us from finishing http://github.com/owncloud/ocis/pull/409 and the earlier we merge it the better 🙄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh okay. No worries then. I'll merge it

// 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