Skip to content

Commit

Permalink
enhancement(sharing): Return newly created driveItem
Browse files Browse the repository at this point in the history 
When accepting a share via 'POST /v1beta1/drives/{driveId}/root/children'
return the newly created driveItem. This driveItem wraps the accepted
remoteItem representing the shared resource (similar to the
'sharedWithMe' response.

This also refactors some of the helpers for user lookup and CS3 share to
driveItem conversion so they can be more easily shared.
  • Loading branch information
@rhafer
rhafer committed Feb 21, 2024
1 parent b83af70 commit a46cb0d
Show file tree
Hide file tree
Showing 7 changed files with 533 additions and 379 deletions.
64 changes: 48 additions & 16 deletions services/graph/pkg/service/v0/api_drives_drive_item.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"errors"
"net/http"
"path/filepath"

gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1"
collaboration "github.com/cs3org/go-cs3apis/cs3/sharing/collaboration/v1beta1"
Expand All @@ -14,10 +15,10 @@ import (

"github.com/cs3org/reva/v2/pkg/rgrpc/todo/pool"
"github.com/cs3org/reva/v2/pkg/storagespace"
"github.com/cs3org/reva/v2/pkg/utils"

"github.com/owncloud/ocis/v2/ocis-pkg/log"
"github.com/owncloud/ocis/v2/services/graph/pkg/errorcode"
"github.com/owncloud/ocis/v2/services/graph/pkg/identity"
)

const (
Expand All @@ -33,15 +34,19 @@ type DrivesDriveItemProvider interface {

// DrivesDriveItemService contains the production business logic for everything that relates to drives
type DrivesDriveItemService struct {
logger log.Logger
gatewaySelector pool.Selectable[gateway.GatewayAPIClient]
logger log.Logger
gatewaySelector pool.Selectable[gateway.GatewayAPIClient]
identityCache identity.IdentityCache
resharingEnabled bool
}

// NewDrivesDriveItemService creates a new DrivesDriveItemService
func NewDrivesDriveItemService(logger log.Logger, gatewaySelector pool.Selectable[gateway.GatewayAPIClient]) (DrivesDriveItemService, error) {
func NewDrivesDriveItemService(logger log.Logger, gatewaySelector pool.Selectable[gateway.GatewayAPIClient], identityCache identity.IdentityCache, resharing bool) (DrivesDriveItemService, error) {
return DrivesDriveItemService{
logger: log.Logger{Logger: logger.With().Str("graph api", "DrivesDriveItemService").Logger()},
gatewaySelector: gatewaySelector,
logger: log.Logger{Logger: logger.With().Str("graph api", "DrivesDriveItemService").Logger()},
gatewaySelector: gatewaySelector,
identityCache: identityCache,
resharingEnabled: resharing,
}, nil
}

Expand Down Expand Up @@ -96,11 +101,17 @@ func (s DrivesDriveItemService) UnmountShare(ctx context.Context, resourceID sto

// MountShare mounts a share
func (s DrivesDriveItemService) MountShare(ctx context.Context, resourceID storageprovider.ResourceId, name string) (libregraph.DriveItem, error) {
if filepath.IsAbs(name) {
return libregraph.DriveItem{}, errorcode.New(errorcode.InvalidRequest, "name cannot be an absolute path")
}
name = filepath.Clean(name)

gatewayClient, err := s.gatewaySelector.Next()
if err != nil {
return libregraph.DriveItem{}, err
}

// Get all shares that the user has received for this resource. There might be multiple
receivedSharesResponse, err := gatewayClient.ListReceivedShares(ctx, &collaboration.ListReceivedSharesRequest{
Filters: []*collaboration.Filter{
{
Expand Down Expand Up @@ -129,6 +140,10 @@ func (s DrivesDriveItemService) MountShare(ctx context.Context, resourceID stora

var errs []error

var acceptedShares []*collaboration.ReceivedShare

// try to accept all of the received shares for this resource. So that the stat is in sync across all
// shares
for _, receivedShare := range receivedSharesResponse.GetShares() {
updateMask := &fieldmaskpb.FieldMask{Paths: []string{_fieldMaskPathState}}
receivedShare.State = collaboration.ShareState_SHARE_STATE_ACCEPTED
Expand All @@ -140,9 +155,8 @@ func (s DrivesDriveItemService) MountShare(ctx context.Context, resourceID stora
mountPoint = &storageprovider.Reference{}
}

newPath := utils.MakeRelativePath(name)
if mountPoint.GetPath() != newPath {
mountPoint.Path = newPath
if filepath.Clean(mountPoint.GetPath()) != name {
mountPoint.Path = name
receivedShare.MountPoint = mountPoint
updateMask.Paths = append(updateMask.Paths, _fieldMaskPathMountPoint)
}
Expand All @@ -154,17 +168,35 @@ func (s DrivesDriveItemService) MountShare(ctx context.Context, resourceID stora
}

updateReceivedShareResponse, err := gatewayClient.UpdateReceivedShare(ctx, updateReceivedShareRequest)
if err != nil {
errs = append(errs, err)
continue
switch errCode := errorcode.FromCS3Status(updateReceivedShareResponse.GetStatus(), err); {
case errCode == nil:
acceptedShares = append(acceptedShares, updateReceivedShareResponse.GetShare())
default:
// Just log at debug level here. If a single accept for any of the received shares failed this
// is not a critical problem. We mainly need to handle the case where all accepts fail. (Outside
// the loop)
s.logger.Debug().Err(errCode).
Str("shareid", receivedShare.GetShare().GetId().String()).
Str("resourceid", receivedShare.GetShare().GetResourceId().String()).
Msg("failed to accept share")
errs = append(errs, errCode)
}
}

// fixMe: send to nirvana, wait for toDriverItem func
_ = updateReceivedShareResponse
if len(receivedSharesResponse.GetShares()) == len(errs) {
// none of the received shares could be accepted. This is an error. Return it.
return libregraph.DriveItem{}, errors.Join(errs...)
}

// fixMe: return a concrete driveItem
return libregraph.DriveItem{}, errors.Join(errs...)
// As the accepted shares are all for the same resource they should collapse to a single driveitem
items, err := cs3ReceivedSharesToDriveItems(ctx, &s.logger, gatewayClient, s.identityCache, s.resharingEnabled, acceptedShares)
switch {
case err != nil:
return libregraph.DriveItem{}, nil
case len(items) != 1:
return libregraph.DriveItem{}, errorcode.New(errorcode.GeneralException, "failed to convert accepted shares into driveitem")
}
return items[0], nil
}

// DrivesDriveItemApi is the api that registers the http endpoints which expose needed operation to the graph api.
Expand Down
138 changes: 128 additions & 10 deletions services/graph/pkg/service/v0/api_drives_drive_item_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"fmt"
"net/http"
"net/http/httptest"
"strconv"

gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1"
collaborationv1beta1 "github.com/cs3org/go-cs3apis/cs3/sharing/collaboration/v1beta1"
Expand All @@ -20,10 +21,12 @@ import (
"github.com/tidwall/gjson"
"google.golang.org/grpc"

"github.com/cs3org/reva/v2/pkg/rgrpc/status"
"github.com/cs3org/reva/v2/pkg/storagespace"
cs3mocks "github.com/cs3org/reva/v2/tests/cs3mocks/mocks"
"github.com/owncloud/ocis/v2/ocis-pkg/log"
"github.com/owncloud/ocis/v2/services/graph/mocks"
"github.com/owncloud/ocis/v2/services/graph/pkg/identity"
svc "github.com/owncloud/ocis/v2/services/graph/pkg/service/v0"
)

Expand All @@ -41,7 +44,9 @@ var _ = Describe("DrivesDriveItemService", func() {
gatewaySelector = mocks.NewSelectable[gateway.GatewayAPIClient](GinkgoT())
gatewaySelector.On("Next").Return(gatewayClient, nil)

service, err := svc.NewDrivesDriveItemService(logger, gatewaySelector)
cache := identity.NewIdentityCache(identity.IdentityCacheWithGatewaySelector(gatewaySelector))

service, err := svc.NewDrivesDriveItemService(logger, gatewaySelector, cache, false)
Expect(err).ToNot(HaveOccurred())
drivesDriveItemService = service
})
Expand Down Expand Up @@ -111,7 +116,12 @@ var _ = Describe("DrivesDriveItemService", func() {
Describe("gateway client share update", func() {
It("updates the share state to be accepted", func() {
expectedShareID := collaborationv1beta1.ShareId{
OpaqueId: "1$2!3",
OpaqueId: "1:2:3",
}
expectedResourceID := storageprovider.ResourceId{
StorageId: "1",
SpaceId: "2",
OpaqueId: "3",
}

gatewayClient.
Expand All @@ -135,14 +145,42 @@ var _ = Describe("DrivesDriveItemService", func() {
Expect(in.GetUpdateMask().GetPaths()).To(Equal([]string{"state"}))
Expect(in.GetShare().GetState()).To(Equal(collaborationv1beta1.ShareState_SHARE_STATE_ACCEPTED))
Expect(in.GetShare().GetShare().GetId().GetOpaqueId()).To(Equal(expectedShareID.GetOpaqueId()))
return &collaborationv1beta1.UpdateReceivedShareResponse{}, nil
return &collaborationv1beta1.UpdateReceivedShareResponse{
Status: status.NewOK(ctx),
Share: &collaborationv1beta1.ReceivedShare{
State: collaborationv1beta1.ShareState_SHARE_STATE_ACCEPTED,
Share: &collaborationv1beta1.Share{
Id: &expectedShareID,
ResourceId: &expectedResourceID,
},
},
}, nil
})
gatewayClient.
On("Stat", mock.Anything, mock.Anything, mock.Anything).
Return(func(ctx context.Context, in *storageprovider.StatRequest, opts ...grpc.CallOption) (*storageprovider.StatResponse, error) {
return &storageprovider.StatResponse{
Status: status.NewOK(ctx),
Info: &storageprovider.ResourceInfo{
Id: &expectedResourceID,
Name: "name",
},
}, nil
})

_, err := drivesDriveItemService.MountShare(context.Background(), storageprovider.ResourceId{}, "")
Expect(err).ToNot(HaveOccurred())
})

It("updates the mountPoint", func() {
expectedShareID := collaborationv1beta1.ShareId{
OpaqueId: "1:2:3",
}
expectedResourceID := storageprovider.ResourceId{
StorageId: "1",
SpaceId: "2",
OpaqueId: "3",
}

gatewayClient.
On("ListReceivedShares", mock.Anything, mock.Anything, mock.Anything).
Return(func(ctx context.Context, in *collaborationv1beta1.ListReceivedSharesRequest, opts ...grpc.CallOption) (*collaborationv1beta1.ListReceivedSharesResponse, error) {
Expand All @@ -158,15 +196,45 @@ var _ = Describe("DrivesDriveItemService", func() {
Return(func(ctx context.Context, in *collaborationv1beta1.UpdateReceivedShareRequest, opts ...grpc.CallOption) (*collaborationv1beta1.UpdateReceivedShareResponse, error) {
Expect(in.GetUpdateMask().GetPaths()).To(HaveLen(2))
Expect(in.GetUpdateMask().GetPaths()).To(ContainElements("mount_point"))
Expect(in.GetShare().GetMountPoint().GetPath()).To(Equal("./new name"))
return &collaborationv1beta1.UpdateReceivedShareResponse{}, nil
Expect(in.GetShare().GetMountPoint().GetPath()).To(Equal("new name"))
return &collaborationv1beta1.UpdateReceivedShareResponse{
Status: status.NewOK(ctx),
Share: &collaborationv1beta1.ReceivedShare{
State: collaborationv1beta1.ShareState_SHARE_STATE_ACCEPTED,
Share: &collaborationv1beta1.Share{
Id: &expectedShareID,
ResourceId: &expectedResourceID,
},
MountPoint: &storageprovider.Reference{
Path: "new name",
},
},
}, nil
})
gatewayClient.
On("Stat", mock.Anything, mock.Anything, mock.Anything).
Return(func(ctx context.Context, in *storageprovider.StatRequest, opts ...grpc.CallOption) (*storageprovider.StatResponse, error) {
return &storageprovider.StatResponse{
Status: status.NewOK(ctx),
Info: &storageprovider.ResourceInfo{
Id: &expectedResourceID,
Name: "name",
},
}, nil
})

_, err := drivesDriveItemService.MountShare(context.Background(), storageprovider.ResourceId{}, "new name")
di, err := drivesDriveItemService.MountShare(context.Background(), storageprovider.ResourceId{}, "new name")
Expect(err).ToNot(HaveOccurred())
Expect(di.GetName()).To(Equal("new name"))
})

It("bubbles errors and continues", func() {
It("succeeds when any of the shares was accepted", func() {
expectedResourceID := storageprovider.ResourceId{
StorageId: "1",
SpaceId: "2",
OpaqueId: "3",
}

gatewayClient.
On("ListReceivedShares", mock.Anything, mock.Anything, mock.Anything).
Return(func(ctx context.Context, in *collaborationv1beta1.ListReceivedSharesRequest, opts ...grpc.CallOption) (*collaborationv1beta1.ListReceivedSharesResponse, error) {
Expand All @@ -190,11 +258,61 @@ var _ = Describe("DrivesDriveItemService", func() {
return nil, fmt.Errorf("error %d", calls)
}

return &collaborationv1beta1.UpdateReceivedShareResponse{}, nil
return &collaborationv1beta1.UpdateReceivedShareResponse{
Status: status.NewOK(ctx),
Share: &collaborationv1beta1.ReceivedShare{
State: collaborationv1beta1.ShareState_SHARE_STATE_ACCEPTED,
Share: &collaborationv1beta1.Share{
Id: &collaborationv1beta1.ShareId{
OpaqueId: strconv.Itoa(calls),
},
ResourceId: &expectedResourceID,
},
},
}, nil
})
gatewayClient.
On("Stat", mock.Anything, mock.Anything, mock.Anything).
Return(func(ctx context.Context, in *storageprovider.StatRequest, opts ...grpc.CallOption) (*storageprovider.StatResponse, error) {
return &storageprovider.StatResponse{
Status: status.NewOK(ctx),
Info: &storageprovider.ResourceInfo{
Id: &expectedResourceID,
Name: "name",
},
}, nil
})

di, err := drivesDriveItemService.MountShare(context.Background(), storageprovider.ResourceId{}, "new name")
Expect(err).To(BeNil())
Expect(di.GetId()).ToNot(BeEmpty())
})
It("errors when none of the shares can be accepted", func() {
gatewayClient.
On("ListReceivedShares", mock.Anything, mock.Anything, mock.Anything).
Return(func(ctx context.Context, in *collaborationv1beta1.ListReceivedSharesRequest, opts ...grpc.CallOption) (*collaborationv1beta1.ListReceivedSharesResponse, error) {
return &collaborationv1beta1.ListReceivedSharesResponse{
Shares: []*collaborationv1beta1.ReceivedShare{
{},
{},
{},
},
}, nil
})

var calls int
gatewayClient.
On("UpdateReceivedShare", mock.Anything, mock.Anything, mock.Anything).
Return(func(ctx context.Context, in *collaborationv1beta1.UpdateReceivedShareRequest, opts ...grpc.CallOption) (*collaborationv1beta1.UpdateReceivedShareResponse, error) {
calls++
Expect(calls).To(BeNumerically("<=", 3))
return nil, fmt.Errorf("error %d", calls)
})

_, err := drivesDriveItemService.MountShare(context.Background(), storageprovider.ResourceId{}, "new name")
Expect(fmt.Sprint(err)).To(Equal("error 1\nerror 2"))
Expect(fmt.Sprint(err)).To(ContainSubstring("error 1"))
Expect(fmt.Sprint(err)).To(ContainSubstring("error 2"))
Expect(fmt.Sprint(err)).To(ContainSubstring("error 3"))
})
})
})
Expand Down
8 changes: 4 additions & 4 deletions services/graph/pkg/service/v0/drives.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -932,17 +932,17 @@ func (g Graph) cs3PermissionsToLibreGraph(ctx context.Context, space *storagepro
tmp := id
var identitySet libregraph.IdentitySet
if _, ok := groupsMap[id]; ok {
group, err := g.identityCache.GetGroup(ctx, tmp)
identity, err := groupIdToIdentity(ctx, g.identityCache, tmp)
if err != nil {
g.logger.Warn().Str("groupid", tmp).Msg("Group not found by id")
}
identitySet = libregraph.IdentitySet{Group: &libregraph.Identity{Id: &tmp, DisplayName: group.GetDisplayName()}}
identitySet = libregraph.IdentitySet{Group: &identity}
} else {
user, err := g.identityCache.GetUser(ctx, tmp)
identity, err := userIdToIdentity(ctx, g.identityCache, tmp)
if err != nil {
g.logger.Warn().Str("userid", tmp).Msg("User not found by id")
}
identitySet = libregraph.IdentitySet{User: &libregraph.Identity{Id: &tmp, DisplayName: user.GetDisplayName()}}
identitySet = libregraph.IdentitySet{User: &identity}
}

p := libregraph.Permission{
Expand Down
2 changes: 1 addition & 1 deletion services/graph/pkg/service/v0/service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ func NewService(opts ...Option) (Graph, error) {
requireAdmin = options.RequireAdminMiddleware
}

drivesDriveItemService, err := NewDrivesDriveItemService(options.Logger, options.GatewaySelector)
drivesDriveItemService, err := NewDrivesDriveItemService(options.Logger, options.GatewaySelector, identityCache, options.Config.FilesSharing.EnableResharing)
if err != nil {
return svc, err
}
Expand Down
Loading

0 comments on commit a46cb0d

Please sign in to comment.