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 2 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
89 changes: 51 additions & 38 deletions internal/grpc/services/gateway/storageprovider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,10 +104,29 @@ func (s *svc) getHome(ctx context.Context) string {
return "/home"
}
func (s *svc) InitiateFileDownload(ctx context.Context, req *provider.InitiateFileDownloadRequest) (*gateway.InitiateFileDownloadResponse, error) {
p, err := s.getPath(ctx, req.Ref)
statReq := &provider.StatRequest{Ref: req.Ref}
ishank011 marked this conversation as resolved.
Show resolved Hide resolved
statRes, err := s.stat(ctx, statReq)
if err != nil {
return &gateway.InitiateFileDownloadResponse{
Status: status.NewInternal(ctx, err, "gateway: error gettng path for ref"),
Status: status.NewInternal(ctx, err, "gateway: error stating ref:"+req.Ref.String()),
}, nil
}
if statRes.Status.Code != rpc.Code_CODE_OK {
if statRes.Status.Code == rpc.Code_CODE_NOT_FOUND {
return &gateway.InitiateFileDownloadResponse{
Status: status.NewNotFound(ctx, "gateway: file not found"),
}, nil
}
err := status.NewErrorFromCode(statRes.Status.Code, "gateway")
return &gateway.InitiateFileDownloadResponse{
Status: status.NewInternal(ctx, err, "gateway: error stating ref"),
}, nil
}

p, st := s.getPath(ctx, req.Ref)
if st.Code != rpc.Code_CODE_OK {
return &gateway.InitiateFileDownloadResponse{
Status: st,
}, nil
}

Expand Down Expand Up @@ -139,7 +158,8 @@ func (s *svc) InitiateFileDownload(ctx context.Context, req *provider.InitiateFi
err := errtypes.PermissionDenied("gateway: cannot download share folder or share name: path=" + p)
log.Err(err).Msg("gateway: error downloading")
return &gateway.InitiateFileDownloadResponse{
Status: status.NewInvalidArg(ctx, "path points to share folder or share name"),
// Status: status.NewInvalidArg(ctx, "path points to share folder or share name"),
Copy link
Contributor

Choose a reason for hiding this comment

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

uncomment again ?

Status: st,
}, nil

}
Expand Down Expand Up @@ -254,10 +274,10 @@ 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 {
p, st := s.getPath(ctx, req.Ref)
if st.Code != rpc.Code_CODE_OK {
return &gateway.InitiateFileUploadResponse{
Status: status.NewInternal(ctx, err, "gateway: error gettng path for ref"),
Status: st,
}, nil
}

Expand Down Expand Up @@ -422,10 +442,10 @@ 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 {
p, st := s.getPath(ctx, req.Ref)
if st.Code != rpc.Code_CODE_OK {
return &provider.CreateContainerResponse{
Status: status.NewInternal(ctx, err, "gateway: error gettng path for ref"),
Status: st,
}, nil
}

Expand Down Expand Up @@ -527,11 +547,8 @@ 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 {
return &provider.DeleteResponse{
Status: status.NewInternal(ctx, err, "gateway: error gettng path for ref"),
}, nil
p, st := s.getPath(ctx, req.Ref)
if st.Code != rpc.Code_CODE_OK {
refs marked this conversation as resolved.
Show resolved Hide resolved
}

if !s.inSharedFolder(ctx, p) {
Expand Down Expand Up @@ -644,19 +661,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.Err(err).Msg("gateway: error moving")
Copy link
Contributor

Choose a reason for hiding this comment

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

stray comment

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")
// log.Err(err).Msg("gateway: error moving")
Copy link
Contributor

Choose a reason for hiding this comment

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

obsolete, or need to be re-enabled?

Copy link
Member Author

Choose a reason for hiding this comment

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

obsolete

return &provider.MoveResponse{
Status: status.NewInternal(ctx, err, "gateway: error gettng path for ref"),
Status: st,
}, nil
}

Expand Down Expand Up @@ -844,10 +861,11 @@ 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 {
p, st := s.getPath(ctx, req.Ref, req.ArbitraryMetadataKeys...)
if st.Code != rpc.Code_CODE_OK {
return &provider.StatResponse{
Status: status.NewInternal(ctx, err, "gateway: error getting path for ref"),
// Status: status.NewInternal(ctx, "", "gateway: error getting path for ref"),
Status: st,
}, nil
}

Expand Down Expand Up @@ -1080,10 +1098,11 @@ 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 {
p, st := s.getPath(ctx, req.Ref, req.ArbitraryMetadataKeys...)
if st.Code != rpc.Code_CODE_OK {
return &provider.ListContainerResponse{
Status: status.NewInternal(ctx, err, "gateway: error getting path for ref"),
// Status: status.NewInternal(ctx, err, "gateway: error getting path for ref"),
Status: st,
}, nil
}

Expand Down Expand Up @@ -1268,28 +1287,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.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
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