set public storage ids for publicly shared resources

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

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")
 	}
 

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,21 +146,21 @@ 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
 	case st != nil:
 		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,48 +462,95 @@ 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
 	case st != nil:
 		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 == "") || shareInfo.Id.OpaqueId == 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 = path.Base(shareInfo.Path)
+		} else {
+			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 +578,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,15 +587,15 @@ 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
 	}
 
 	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 +604,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 +727,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 +746,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 +758,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
 }

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

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"

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"
 

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"

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"