diff --git a/changelog/unreleased/fix-view-only-wopi.md b/changelog/unreleased/fix-view-only-wopi.md new file mode 100644 index 00000000000..5a65a8aaa9f --- /dev/null +++ b/changelog/unreleased/fix-view-only-wopi.md @@ -0,0 +1,8 @@ +Bugfix: Enforce permissions in public share apps + +A receiver of a read-only public share could still edit files via apps like Collabora. +These changes enforce the share permissions in apps used on publicly shared resources. + +https://github.com/owncloud/web/issues/5776 +https://github.com/owncloud/ocis/issues/2479 +https://github.com/cs3org/reva/pull/22142214 diff --git a/internal/grpc/services/gateway/storageprovider.go b/internal/grpc/services/gateway/storageprovider.go index 1441389954e..27959e8acfb 100644 --- a/internal/grpc/services/gateway/storageprovider.go +++ b/internal/grpc/services/gateway/storageprovider.go @@ -28,7 +28,6 @@ import ( "time" "google.golang.org/protobuf/types/known/fieldmaskpb" - "grpc.go4.org/codes" gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" @@ -48,6 +47,7 @@ import ( "github.com/google/uuid" "github.com/pkg/errors" + "google.golang.org/grpc/codes" gstatus "google.golang.org/grpc/status" ) @@ -685,6 +685,9 @@ func (s *svc) initiateFileUpload(ctx context.Context, req *provider.InitiateFile storageRes, err := c.InitiateFileUpload(ctx, req) if err != nil { + if gstatus.Code(err) == codes.PermissionDenied { + return &gateway.InitiateFileUploadResponse{Status: &rpc.Status{Code: rpc.Code_CODE_PERMISSION_DENIED}}, nil + } return nil, errors.Wrap(err, "gateway: error calling InitiateFileUpload") } @@ -995,6 +998,9 @@ func (s *svc) delete(ctx context.Context, req *provider.DeleteRequest) (*provide res, err := c.Delete(ctx, req) if err != nil { + if gstatus.Code(err) == codes.PermissionDenied { + return &provider.DeleteResponse{Status: &rpc.Status{Code: rpc.Code_CODE_PERMISSION_DENIED}}, nil + } return nil, errors.Wrap(err, "gateway: error calling Delete") } diff --git a/internal/grpc/services/publicstorageprovider/publicstorageprovider.go b/internal/grpc/services/publicstorageprovider/publicstorageprovider.go index 9132811bb10..72fca7a6781 100644 --- a/internal/grpc/services/publicstorageprovider/publicstorageprovider.go +++ b/internal/grpc/services/publicstorageprovider/publicstorageprovider.go @@ -50,12 +50,14 @@ func init() { type config struct { MountPath string `mapstructure:"mount_path"` + MountID string `mapstructure:"mount_id"` GatewayAddr string `mapstructure:"gateway_addr"` } type service struct { conf *config mountPath string + mountID string gateway gateway.GatewayAPIClient } @@ -88,6 +90,7 @@ func New(m map[string]interface{}, ss *grpc.Server) (rgrpc.Service, error) { } mountPath := c.MountPath + mountID := c.MountID gateway, err := pool.GetGatewayServiceClient(c.GatewayAddr) if err != nil { @@ -97,6 +100,7 @@ func New(m map[string]interface{}, ss *grpc.Server) (rgrpc.Service, error) { service := &service{ conf: c, mountPath: mountPath, + mountID: mountID, gateway: gateway, } @@ -142,7 +146,7 @@ func (s *service) translatePublicRefToCS3Ref(ctx context.Context, ref *provider. return nil, "", nil, nil, err } - originalPath, ls, _, st, err := s.resolveToken(ctx, tkn) + ls, shareInfo, st, err := s.resolveToken(ctx, tkn) switch { case err != nil: return nil, "", nil, nil, err @@ -150,13 +154,13 @@ func (s *service) translatePublicRefToCS3Ref(ctx context.Context, ref *provider. return nil, "", nil, st, nil } - cs3Ref := &provider.Reference{Path: path.Join("/", originalPath, relativePath)} + cs3Ref := &provider.Reference{Path: path.Join("/", shareInfo.Path, relativePath)} log.Debug(). Interface("sourceRef", ref). Interface("cs3Ref", cs3Ref). Interface("share", ls). Str("tkn", tkn). - Str("originalPath", originalPath). + Str("originalPath", shareInfo.Path). Str("relativePath", relativePath). Msg("translatePublicRefToCS3Ref") return cs3Ref, tkn, ls, nil, nil @@ -458,12 +462,27 @@ func (s *service) Stat(ctx context.Context, req *provider.StatRequest) (*provide Value: attribute.StringValue(req.Ref.String()), }) - tkn, relativePath, err := s.unwrap(ctx, req.Ref) - if err != nil { - return nil, err + var ( + tkn string + relativePath string + nodeID string + ) + + if req.Ref.ResourceId != nil { + // Id based request. + // The OpaqueId in the public storage has the format `{shareToken}/{uuid}` + parts := strings.Split(req.Ref.ResourceId.OpaqueId, "/") + tkn = parts[0] + nodeID = parts[1] + } else if req.Ref.Path != "" { + var err error + tkn, relativePath, err = s.unwrap(ctx, req.Ref) + if err != nil { + return nil, err + } } - originalPath, ls, ri, st, err := s.resolveToken(ctx, tkn) + share, shareInfo, st, err := s.resolveToken(ctx, tkn) switch { case err != nil: return nil, err @@ -471,35 +490,65 @@ func (s *service) Stat(ctx context.Context, req *provider.StatRequest) (*provide return &provider.StatResponse{ Status: st, }, nil - case ls.GetPermissions() == nil || !ls.GetPermissions().Permissions.Stat: + case share.GetPermissions() == nil || !share.GetPermissions().Permissions.Stat: return &provider.StatResponse{ Status: status.NewPermissionDenied(ctx, nil, "share does not grant Stat permission"), }, nil } - p := originalPath - if ri.Type == provider.ResourceType_RESOURCE_TYPE_CONTAINER { - p = path.Join("/", p, relativePath) + if shareInfo.Type == provider.ResourceType_RESOURCE_TYPE_FILE || (relativePath == "" && nodeID == "") { + res := &provider.StatResponse{ + Status: status.NewOK(ctx), + Info: shareInfo, + } + s.augmentStatResponse(ctx, res, shareInfo, share, tkn) + return res, nil } - var statResponse *provider.StatResponse - // the call has to be made to the gateway instead of the storage. - statResponse, err = s.gateway.Stat(ctx, &provider.StatRequest{Ref: &provider.Reference{Path: p}}) + + var ref *provider.Reference + if req.Ref.ResourceId != nil { + ref = &provider.Reference{ResourceId: &provider.ResourceId{ + StorageId: share.ResourceId.StorageId, + OpaqueId: nodeID, + }} + } else if req.Ref.Path != "" { + p := path.Join("/", shareInfo.Path, relativePath) + ref = &provider.Reference{Path: p} + } + + statResponse, err := s.gateway.Stat(ctx, &provider.StatRequest{Ref: ref}) if err != nil { return &provider.StatResponse{ Status: status.NewInternal(ctx, err, "gateway: error calling Stat for ref:"+req.Ref.String()), }, nil } + s.augmentStatResponse(ctx, statResponse, shareInfo, share, tkn) + + return statResponse, nil +} + +func (s *service) augmentStatResponse(ctx context.Context, res *provider.StatResponse, shareInfo *provider.ResourceInfo, share *link.PublicShare, tkn string) { // prevent leaking internal paths - if statResponse.Info != nil { - if err := addShare(statResponse.Info, ls); err != nil { - appctx.GetLogger(ctx).Error().Err(err).Interface("share", ls).Interface("info", statResponse.Info).Msg("error when adding share") + if res.Info != nil { + if err := addShare(res.Info, share); err != nil { + appctx.GetLogger(ctx).Error().Err(err).Interface("share", share).Interface("info", res.Info).Msg("error when adding share") } - statResponse.Info.Path = path.Join(s.mountPath, "/", tkn, relativePath) - filterPermissions(statResponse.Info.PermissionSet, ls.GetPermissions().Permissions) + + var sharePath string + if shareInfo.Type != provider.ResourceType_RESOURCE_TYPE_FILE { + sharePath = strings.TrimPrefix(res.Info.Path, shareInfo.Path) + } + + res.Info.Path = path.Join(s.mountPath, "/", tkn, sharePath) + s.setPublicStorageID(res.Info, tkn) + filterPermissions(res.Info.PermissionSet, share.GetPermissions().Permissions) } +} - return statResponse, nil +func (s *service) setPublicStorageID(info *provider.ResourceInfo, shareToken string) { + info.Id.StorageId = s.mountID + info.Id.OpaqueId = shareToken + "/" + info.Id.OpaqueId } func addShare(i *provider.ResourceInfo, ls *link.PublicShare) error { @@ -527,7 +576,7 @@ func (s *service) ListContainer(ctx context.Context, req *provider.ListContainer return nil, err } - pathFromToken, ls, _, st, err := s.resolveToken(ctx, tkn) + share, shareInfo, st, err := s.resolveToken(ctx, tkn) switch { case err != nil: return nil, err @@ -536,7 +585,7 @@ func (s *service) ListContainer(ctx context.Context, req *provider.ListContainer Status: st, }, nil } - if ls.GetPermissions() == nil || !ls.GetPermissions().Permissions.ListContainer { + if share.GetPermissions() == nil || !share.GetPermissions().Permissions.ListContainer { return &provider.ListContainerResponse{ Status: status.NewPermissionDenied(ctx, nil, "share does not grant ListContainer permission"), }, nil @@ -544,7 +593,7 @@ func (s *service) ListContainer(ctx context.Context, req *provider.ListContainer listContainerR, err := s.gateway.ListContainer( ctx, - &provider.ListContainerRequest{Ref: &provider.Reference{Path: path.Join("/", pathFromToken, relativePath)}}, + &provider.ListContainerRequest{Ref: &provider.Reference{Path: path.Join("/", shareInfo.Path, relativePath)}}, ) if err != nil { return &provider.ListContainerResponse{ @@ -553,10 +602,11 @@ func (s *service) ListContainer(ctx context.Context, req *provider.ListContainer } for i := range listContainerR.Infos { - filterPermissions(listContainerR.Infos[i].PermissionSet, ls.GetPermissions().Permissions) + filterPermissions(listContainerR.Infos[i].PermissionSet, share.GetPermissions().Permissions) listContainerR.Infos[i].Path = path.Join(s.mountPath, "/", tkn, relativePath, path.Base(listContainerR.Infos[i].Path)) - if err := addShare(listContainerR.Infos[i], ls); err != nil { - appctx.GetLogger(ctx).Error().Err(err).Interface("share", ls).Interface("info", listContainerR.Infos[i]).Msg("error when adding share") + s.setPublicStorageID(listContainerR.Infos[i], tkn) + if err := addShare(listContainerR.Infos[i], share); err != nil { + appctx.GetLogger(ctx).Error().Err(err).Interface("share", share).Interface("info", listContainerR.Infos[i]).Msg("error when adding share") } } @@ -675,10 +725,10 @@ func (s *service) trimMountPrefix(fn string) (string, error) { } // resolveToken returns the path and share for the publicly shared resource. -func (s *service) resolveToken(ctx context.Context, token string) (string, *link.PublicShare, *provider.ResourceInfo, *rpc.Status, error) { +func (s *service) resolveToken(ctx context.Context, token string) (*link.PublicShare, *provider.ResourceInfo, *rpc.Status, error) { driver, err := pool.GetGatewayServiceClient(s.conf.GatewayAddr) if err != nil { - return "", nil, nil, nil, err + return nil, nil, nil, err } publicShareResponse, err := driver.GetPublicShare( @@ -694,19 +744,9 @@ func (s *service) resolveToken(ctx context.Context, token string) (string, *link ) switch { case err != nil: - return "", nil, nil, nil, err + return nil, nil, nil, err case publicShareResponse.Status.Code != rpc.Code_CODE_OK: - return "", nil, nil, publicShareResponse.Status, nil - } - - pathRes, err := s.gateway.GetPath(ctx, &provider.GetPathRequest{ - ResourceId: publicShareResponse.GetShare().GetResourceId(), - }) - switch { - case err != nil: - return "", nil, nil, nil, err - case pathRes.Status.Code != rpc.Code_CODE_OK: - return "", nil, nil, pathRes.Status, nil + return nil, nil, publicShareResponse.Status, nil } sRes, err := s.gateway.Stat(ctx, &provider.StatRequest{ @@ -716,9 +756,9 @@ func (s *service) resolveToken(ctx context.Context, token string) (string, *link }) switch { case err != nil: - return "", nil, nil, nil, err + return nil, nil, nil, err case sRes.Status.Code != rpc.Code_CODE_OK: - return "", nil, nil, sRes.Status, nil + return nil, nil, sRes.Status, nil } - return pathRes.Path, publicShareResponse.GetShare(), sRes.Info, nil, nil + return publicShareResponse.GetShare(), sRes.Info, nil, nil } diff --git a/pkg/auth/scope/publicshare.go b/pkg/auth/scope/publicshare.go index 8a3fe92ec80..65d8d86b42e 100644 --- a/pkg/auth/scope/publicshare.go +++ b/pkg/auth/scope/publicshare.go @@ -54,7 +54,6 @@ func publicshareScope(ctx context.Context, scope *authpb.Scope, resource interfa return checkStorageRef(ctx, &share, v.GetRef()), nil // Editor role - // TODO(ishank011): Add role checks, // need to return appropriate status codes in the ocs/ocdav layers. case *provider.CreateContainerRequest: return hasRoleEditor(*scope) && checkStorageRef(ctx, &share, v.GetRef()), nil diff --git a/tests/oc-integration-tests/drone/gateway.toml b/tests/oc-integration-tests/drone/gateway.toml index 5b7d4199afc..e00e43158da 100644 --- a/tests/oc-integration-tests/drone/gateway.toml +++ b/tests/oc-integration-tests/drone/gateway.toml @@ -72,6 +72,7 @@ home_provider = "/home" # another mount point might be "/projects/" "/public" = {"address" = "localhost:13000"} +"e1a73ede-549b-4226-abdf-40e69ca8230d" = {"address" = "localhost:13000"} [http] address = "0.0.0.0:19001" diff --git a/tests/oc-integration-tests/drone/storage-publiclink.toml b/tests/oc-integration-tests/drone/storage-publiclink.toml index 78b9afc6b32..f36aeb18fdd 100644 --- a/tests/oc-integration-tests/drone/storage-publiclink.toml +++ b/tests/oc-integration-tests/drone/storage-publiclink.toml @@ -12,5 +12,6 @@ address = "0.0.0.0:13000" # we have a locally running dataprovider [grpc.services.publicstorageprovider] mount_path = "/public/" +mount_id = "e1a73ede-549b-4226-abdf-40e69ca8230d" gateway_addr = "0.0.0.0:19000" diff --git a/tests/oc-integration-tests/local/gateway.toml b/tests/oc-integration-tests/local/gateway.toml index bdd0e33566b..6a4f46ee231 100644 --- a/tests/oc-integration-tests/local/gateway.toml +++ b/tests/oc-integration-tests/local/gateway.toml @@ -72,6 +72,7 @@ home_provider = "/home" # another mount point might be "/projects/" "/public" = {"address" = "localhost:13000"} +"e1a73ede-549b-4226-abdf-40e69ca8230d" = {"address" = "localhost:13000"} [http] address = "0.0.0.0:19001" diff --git a/tests/oc-integration-tests/local/storage-publiclink.toml b/tests/oc-integration-tests/local/storage-publiclink.toml index 07696c826b3..09b686d0209 100644 --- a/tests/oc-integration-tests/local/storage-publiclink.toml +++ b/tests/oc-integration-tests/local/storage-publiclink.toml @@ -12,4 +12,5 @@ address = "0.0.0.0:13000" # we have a locally running dataprovider [grpc.services.publicstorageprovider] mount_path = "/public/" +mount_id = "e1a73ede-549b-4226-abdf-40e69ca8230d" gateway_addr = "0.0.0.0:19000"