From 83c5528eb7e358d8b4c533a88a8be50315fb2d20 Mon Sep 17 00:00:00 2001 From: Ishank Arora Date: Mon, 3 Aug 2020 15:25:52 +0200 Subject: [PATCH] Do not stat shared resources when downloading (#1038) --- changelog/unreleased/download-shares-fix.md | 9 +++++ examples/storage-references/storage-reva.toml | 7 ++++ .../grpc/services/gateway/storageprovider.go | 40 +++++++++---------- .../storageprovider/storageprovider.go | 5 +++ pkg/user/manager/rest/rest.go | 27 +++++++------ 5 files changed, 56 insertions(+), 32 deletions(-) create mode 100644 changelog/unreleased/download-shares-fix.md diff --git a/changelog/unreleased/download-shares-fix.md b/changelog/unreleased/download-shares-fix.md new file mode 100644 index 0000000000..4e2f5aaba4 --- /dev/null +++ b/changelog/unreleased/download-shares-fix.md @@ -0,0 +1,9 @@ +Bugfix: Do not stat shared resources when downloading + +Previously, we statted the resources in all download requests resulting in +failures when downloading references. This PR fixes that by statting only in +case the resource is not present in the shares folder. It also fixes a bug where +we allowed uploading to the mount path, resulting in overwriting the user home +directory. + +https://github.com/cs3org/reva/pull/1038 diff --git a/examples/storage-references/storage-reva.toml b/examples/storage-references/storage-reva.toml index b6b6209db1..5386e54cd0 100644 --- a/examples/storage-references/storage-reva.toml +++ b/examples/storage-references/storage-reva.toml @@ -5,3 +5,10 @@ address = "0.0.0.0:18000" driver = "local" mount_path = "/reva" mount_id = "123e4567-e89b-12d3-a456-426655440000" +data_server_url = "http://localhost:18001/data" + +[http] +address = "0.0.0.0:18001" + +[http.services.dataprovider] +driver = "local" diff --git a/internal/grpc/services/gateway/storageprovider.go b/internal/grpc/services/gateway/storageprovider.go index fc6f565081..932aa79f35 100644 --- a/internal/grpc/services/gateway/storageprovider.go +++ b/internal/grpc/services/gateway/storageprovider.go @@ -104,25 +104,6 @@ func (s *svc) getHome(ctx context.Context) string { return "/home" } func (s *svc) InitiateFileDownload(ctx context.Context, req *provider.InitiateFileDownloadRequest) (*gateway.InitiateFileDownloadResponse, error) { - statReq := &provider.StatRequest{Ref: req.Ref} - statRes, err := s.stat(ctx, statReq) - if err != nil { - return &gateway.InitiateFileDownloadResponse{ - 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, err := s.getPath(ctx, req.Ref) if err != nil { return &gateway.InitiateFileDownloadResponse{ @@ -131,13 +112,31 @@ func (s *svc) InitiateFileDownload(ctx context.Context, req *provider.InitiateFi } if !s.inSharedFolder(ctx, p) { + statReq := &provider.StatRequest{Ref: req.Ref} + statRes, err := s.stat(ctx, statReq) + if err != nil { + return &gateway.InitiateFileDownloadResponse{ + 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 + } 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 upload to share folder or share name: path=" + p) + 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"), @@ -194,6 +193,7 @@ func (s *svc) InitiateFileDownload(ctx context.Context, req *provider.InitiateFi }, } req.Ref = ref + log.Debug().Msg("download path: " + target) return s.initiateFileDownload(ctx, req) } diff --git a/internal/grpc/services/storageprovider/storageprovider.go b/internal/grpc/services/storageprovider/storageprovider.go index 04b79ecb17..845f7a0e71 100644 --- a/internal/grpc/services/storageprovider/storageprovider.go +++ b/internal/grpc/services/storageprovider/storageprovider.go @@ -273,6 +273,11 @@ func (s *service) InitiateFileUpload(ctx context.Context, req *provider.Initiate Status: status.NewInternal(ctx, err, "error unwrapping path"), }, nil } + if newRef.GetPath() == "/" { + return &provider.InitiateFileUploadResponse{ + Status: status.NewInternal(ctx, errors.New("can't upload to mount path"), ""), + }, nil + } url := *s.dataServerURL if s.conf.DisableTus { url.Path = path.Join("/", url.Path, newRef.GetPath()) diff --git a/pkg/user/manager/rest/rest.go b/pkg/user/manager/rest/rest.go index 7b9f92e5b3..a482fa648b 100644 --- a/pkg/user/manager/rest/rest.go +++ b/pkg/user/manager/rest/rest.go @@ -353,26 +353,24 @@ func (m *manager) GetUserByClaim(ctx context.Context, claim, value string) (*use } -func (m *manager) findUsersByFilter(ctx context.Context, url string) ([]*userpb.User, error) { +func (m *manager) findUsersByFilter(ctx context.Context, url string, users map[string]*userpb.User) error { userData, err := m.sendAPIRequest(ctx, url) if err != nil { - return nil, err + return err } - users := []*userpb.User{} - for _, usr := range userData { usrInfo, ok := usr.(map[string]interface{}) if !ok { - return nil, errors.New("rest: error in type assertion") + return errors.New("rest: error in type assertion") } uid := &userpb.UserId{ OpaqueId: usrInfo["upn"].(string), Idp: m.conf.IDProvider, } - users = append(users, &userpb.User{ + users[uid.OpaqueId] = &userpb.User{ Id: uid, Username: usrInfo["upn"].(string), Mail: usrInfo["primaryAccountEmail"].(string), @@ -389,10 +387,10 @@ func (m *manager) findUsersByFilter(ctx context.Context, url string) ([]*userpb. }, }, }, - }) + } } - return users, nil + return nil } func (m *manager) FindUsers(ctx context.Context, query string) ([]*userpb.User, error) { @@ -407,18 +405,23 @@ func (m *manager) FindUsers(ctx context.Context, query string) ([]*userpb.User, return nil, errors.New("rest: illegal characters present in query") } - users := []*userpb.User{} + users := make(map[string]*userpb.User) for _, f := range filters { url := fmt.Sprintf("%s/Identity/?filter=%s:contains:%s&field=id&field=upn&field=primaryAccountEmail&field=displayName&field=uid&field=gid", m.conf.APIBaseURL, f, query) - filteredUsers, err := m.findUsersByFilter(ctx, url) + err := m.findUsersByFilter(ctx, url, users) if err != nil { return nil, err } - users = append(users, filteredUsers...) } - return users, nil + + userSlice := make([]*userpb.User, len(users)) + for _, v := range users { + userSlice = append(userSlice, v) + } + + return userSlice, nil } func (m *manager) GetUserGroups(ctx context.Context, uid *userpb.UserId) ([]string, error) {