cleanup

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

changelog/unreleased/publicstorageprovider-rewrite.md

@@ -1,6 +1,6 @@
-Bugfix: Make public link access use same fileids as authenticated access
+Bugfix: replace public mountpoint fileid with grant fileid in ocdav
 
-A resource now always hase the same fileid, regardless of how it is accessed.
+We now show the same resoucre id for resources when accessing them via a public links as when using a logged in user. This allows the web ui to start a WOPI session with the correct resource id.
 
 https://github.com/cs3org/reva/pull/2646
 https://github.com/cs3org/reva/issues/2635

internal/grpc/services/publicstorageprovider/publicstorageprovider.go

@@ -16,6 +16,8 @@
 // granted to it by virtue of its status as an Intergovernmental Organization
 // or submit itself to any jurisdiction.
 
+// Package publicstorageprovider provides a CS3 storageprovider implementation for public links.
+// It will list spaces with type `grant` and `mountpoint` when a public scope is present.
 package publicstorageprovider
 
 import (
@@ -45,9 +47,6 @@ import (
 	gstatus "google.golang.org/grpc/status"
 )
 
-// SpaceTypePublic is the public space type
-var SpaceTypePublic = "public"
-
 func init() {
 	rgrpc.Register("publicstorageprovider", New)
 }
@@ -82,7 +81,7 @@ func parseConfig(m map[string]interface{}) (*config, error) {
 	return c, nil
 }
 
-// New creates a new IsPublic Storage Provider service.
+// New creates a new publicstorageprovider service.
 func New(m map[string]interface{}, ss *grpc.Server) (rgrpc.Service, error) {
 	c, err := parseConfig(m)
 	if err != nil {
@@ -169,13 +168,9 @@ func (s *service) translatePublicRefToCS3Ref(ctx context.Context, ref *provider.
 	if !ok {
 		return nil, "", nil, nil, gstatus.Errorf(codes.NotFound, "share or token not found")
 	}
-	/*
-		tkn, opaqueid, relativePath, err := s.unwrap(ctx, ref)
-		if err != nil {
-			return nil, "", nil, nil, err
-		}
-	*/
 
+	// the share is minimally populated, we need more than the token
+	// look up complete share
 	ls, shareInfo, st, err := s.resolveToken(ctx, share.Token)
 	switch {
 	case err != nil:
@@ -213,13 +208,6 @@ func (s *service) translatePublicRefToCS3Ref(ctx context.Context, ref *provider.
 	return cs3Ref, share.Token, ls, nil, nil
 }
 
-// Both, t.dir and tokenPath paths need to be merged:
-// tokenPath   = /oc/einstein/public-links
-// t.dir       = /public/ausGxuUePCOi/foldera/folderb/
-// res         = /public-links/foldera/folderb/
-// this `res` will get then expanded taking into account the authenticated user and the storage:
-// end         = /einstein/files/public-links/foldera/folderb/
-
 func (s *service) initiateFileDownload(ctx context.Context, req *provider.InitiateFileDownloadRequest) (*provider.InitiateFileDownloadResponse, error) {
 	cs3Ref, _, ls, st, err := s.translatePublicRefToCS3Ref(ctx, req.Ref)
 	switch {
@@ -344,12 +332,17 @@ func (s *service) CreateStorageSpace(ctx context.Context, req *provider.CreateSt
 	return nil, gstatus.Errorf(codes.Unimplemented, "method not implemented")
 }
 
-// ListStorageSpaces returns a Storage spaces of type "public" when given a filter by id with  the public link token as spaceid.
-// The root node of every storag space is the real (spaceid, nodeid) of the publicly shared node
-// The ocdav service has to
-// 1. Authenticate / Log in at the gateway using the token and can then
-// 2. look up the storage space using ListStorageSpaces.
-// 3. make related requests to that (spaceid, nodeid)
+// ListStorageSpaces returns storage spaces when a public scope is present
+// in the context.
+//
+// On the one hand, it lists a `mountpoint` space that can be used by the
+// registry to construct a mount path. These spaces have their root
+// storageid set to 7993447f-687f-490d-875c-ac95e89a62a4 and the
+// opaqueid set to the link token.
+//
+// On the other hand, it lists a `grant` space for the shared resource id,
+// so id based requests can find the correct storage provider. These spaces
+// have their root set to the shared resource.
 func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStorageSpacesRequest) (*provider.ListStorageSpacesResponse, error) {
 	spaceTypes := map[string]struct{}{}
 	var exists = struct{}{}
@@ -359,7 +352,6 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
 		switch f.Type {
 		case provider.ListStorageSpacesRequest_Filter_TYPE_SPACE_TYPE:
 			spaceType := f.GetSpaceType()
-			// do we need to fetch the shares?
 			if spaceType == "+mountpoint" || spaceType == "+grant" {
 				appendTypes = append(appendTypes, strings.TrimPrefix(spaceType, "+"))
 				continue
@@ -383,10 +375,6 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
 	// if there is no public scope there are no publicstorage spaces
 	share, _, ok := extractLinkAndInfo(ctx)
 	if !ok {
-		// TODO when the ocdav publicfile handler sends the token as opaqueid the
-		// public scope needs to change
-		// it always needs to check if the accessed path is a child of the resource identified by
-		// the token
 		return &provider.ListStorageSpacesResponse{
 			Status: &rpc.Status{Code: rpc.Code_CODE_OK},
 		}, nil
@@ -408,20 +396,12 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
 			// when a list storage space with the resourceid of an external
 			// resource is made we may have a grant for it
 			root := share.ResourceId
-			// do we filter by id?
 			if spaceID != nil && !utils.ResourceIDEqual(spaceID, root) {
 				// none of our business
 				continue
 			}
-			/*
-				var opaque *typesv1beta1.Opaque
-				if etag, ok := shareEtags[share.Id.OpaqueId]; ok {
-					opaque = utils.AppendPlainToOpaque(opaque, "etag", etag)
-				}
-			*/
 			// we know a grant for this resource
 			space := &provider.StorageSpace{
-				//Opaque: opaque,
 				Id: &provider.StorageSpaceId{
 					OpaqueId: root.StorageId + "!" + root.OpaqueId,
 				},
@@ -437,7 +417,6 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
 				StorageId: utils.PublicStorageProviderID,
 				OpaqueId:  share.Token, // the link share has no id, only the token
 			}
-			// do we filter by id
 			if spaceID != nil {
 				switch {
 				case utils.ResourceIDEqual(spaceID, root):
@@ -450,28 +429,16 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
 					continue
 				}
 			}
-			/*
-				var opaque *typesv1beta1.Opaque
-				if etag, ok := shareEtags[receivedShare.Share.Id.OpaqueId]; ok {
-					opaque = utils.AppendPlainToOpaque(opaque, "etag", etag)
-				}
-			*/
 			space := &provider.StorageSpace{
-				//Opaque: opaque,
 				Id: &provider.StorageSpaceId{
 					OpaqueId: root.StorageId + "!" + root.OpaqueId,
 				},
 				SpaceType: "mountpoint",
-				Owner:     &userv1beta1.User{Id: share.Owner}, // FIXME actually, the mount point belongs to the recipient
+				Owner:     &userv1beta1.User{Id: share.Owner}, // FIXME actually, the mount point belongs to no one?
 				// the publicstorageprovider keeps track of mount points
 				Root: root,
 			}
 
-			// "Public share ~token"? nah, the spaceregistry uses the token in the name as the mountpoint
-			// TODO use token in root opaqueid when building path
-			space.Name = share.Token
-
-			// what if we don't have a name?
 			res.StorageSpaces = append(res.StorageSpaces, space)
 		}
 	}
@@ -818,8 +785,8 @@ func (s *service) ListContainer(ctx context.Context, req *provider.ListContainer
 
 	for i := range listContainerR.Infos {
 		// FIXME how do we reduce permissions to what is granted by the public link?
+		// only a problem for id based access -> middleware
 		filterPermissions(listContainerR.Infos[i].PermissionSet, share.GetPermissions().Permissions)
-		// 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")
 		}

pkg/auth/scope/publicshare.go

@@ -158,29 +158,6 @@ func checkStorageRef(ctx context.Context, s *link.PublicShare, r *provider.Refer
 	return false
 }
 
-// public link access must send a filter with id or type
-/*
-func checkPublicListStorageSpacesFilter(filters []*provider.ListStorageSpacesRequest_Filter) bool {
-	// return true
-	for _, f := range filters {
-		switch f.Type {
-		case provider.ListStorageSpacesRequest_Filter_TYPE_SPACE_TYPE:
-			switch f.GetSpaceType() {
-			case "mountpoint", "+mountpoint":
-				return true
-			case "grant", "+grant":
-				return true
-			}
-		case provider.ListStorageSpacesRequest_Filter_TYPE_ID:
-			if f.GetId().OpaqueId != "" {
-				return true
-			}
-		}
-	}
-	return false
-}
-*/
-
 func checkPublicShareRef(s *link.PublicShare, ref *link.PublicShareReference) bool {
 	// ref: <token:$token >
 	return ref.GetToken() == s.Token

tests/oc-integration-tests/drone/gateway.toml

@@ -79,7 +79,7 @@ home_template = "/users/{{.Id.OpaqueId}}"
 ## a public link is accessed.
 [grpc.services.storageregistry.drivers.spaces.providers."localhost:13000".spaces]
 "grant" = { "mount_point" = "." }
-"mountpoint" = { "mount_point" = "/public", "path_template" = "/public/{{.Space.Name}}" }
+"mountpoint" = { "mount_point" = "/public", "path_template" = "/public/{{.Space.Root.OpaqueId}}" }
 
 [http]
 address = "0.0.0.0:19001"

tests/oc-integration-tests/local/gateway.toml

@@ -85,7 +85,7 @@ home_template = "/users/{{.Id.OpaqueId}}"
 ## a public link is accessed.
 [grpc.services.storageregistry.drivers.spaces.providers."localhost:13000".spaces]
 "grant" = { "mount_point" = "." }
-"mountpoint" = { "mount_point" = "/public", "path_template" = "/public/{{.Space.Name}}" }
+"mountpoint" = { "mount_point" = "/public", "path_template" = "/public/{{.Space.Root.OpaqueId}}" }
 
 [http]
 address = "0.0.0.0:19001"