From 8da19dfd64182b8765c4912adf56a0913f825a3a Mon Sep 17 00:00:00 2001 From: Jesse Geens Date: Wed, 8 Jan 2025 11:09:01 +0100 Subject: [PATCH] Pseudo-transactionalize sharing --- changelog/unreleased/pseudo-tx-share.md | 8 +++ .../services/gateway/usershareprovider.go | 71 ++++++++++++------- .../usershareprovider/usershareprovider.go | 2 +- 3 files changed, 55 insertions(+), 26 deletions(-) create mode 100644 changelog/unreleased/pseudo-tx-share.md diff --git a/changelog/unreleased/pseudo-tx-share.md b/changelog/unreleased/pseudo-tx-share.md new file mode 100644 index 0000000000..837d487315 --- /dev/null +++ b/changelog/unreleased/pseudo-tx-share.md @@ -0,0 +1,8 @@ +Enhancement: pseudo-transactionalize sharing + +Currently, sharing is not transactionalized: setting ACLs and writing the share to the db is completely independent. In the current situation, shares are written to the db before setting the ACL, letting users falsely believe that they successfully shared a resource, even if setting the ACL afterwards fails. his enhancement improves the situation by doing the least reliable (setting ACLs on EOS) first: +a) first pinging the db +b) writing the ACLs +c) writing to the db + +https://github.com/cs3org/reva/pull/5029 \ No newline at end of file diff --git a/internal/grpc/services/gateway/usershareprovider.go b/internal/grpc/services/gateway/usershareprovider.go index 74e612b0c9..258f2bd6d7 100644 --- a/internal/grpc/services/gateway/usershareprovider.go +++ b/internal/grpc/services/gateway/usershareprovider.go @@ -22,6 +22,7 @@ import ( "context" "fmt" "path" + "strings" userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" @@ -37,34 +38,42 @@ import ( "github.com/pkg/errors" ) -// TODO(labkode): add multi-phase commit logic when commit share or commit ref is enabled. func (s *svc) CreateShare(ctx context.Context, req *collaboration.CreateShareRequest) (*collaboration.CreateShareResponse, error) { if s.isSharedFolder(ctx, req.ResourceInfo.GetPath()) { return nil, errtypes.AlreadyExists("gateway: can't share the share folder itself") } - c, err := pool.GetUserShareProviderClient(pool.Endpoint(s.c.UserShareProviderEndpoint)) + log := appctx.GetLogger(ctx) + + shareClient, err := pool.GetUserShareProviderClient(pool.Endpoint(s.c.UserShareProviderEndpoint)) if err != nil { return &collaboration.CreateShareResponse{ Status: status.NewInternal(ctx, err, "error getting user share provider client"), }, nil } - // TODO the user share manager needs to be able to decide if the current user is allowed to create that share (and not eg. incerase permissions) - // jfd: AFAICT this can only be determined by a storage driver - either the storage provider is queried first or the share manager needs to access the storage using a storage driver - res, err := c.CreateShare(ctx, req) - if err != nil { - return nil, errors.Wrap(err, "gateway: error calling CreateShare") - } - if res.Status.Code != rpc.Code_CODE_OK { - return res, nil - } + // First we ping the db + // -------------------- + // See ADR-REVA-003 + _, err = shareClient.GetShare(ctx, &collaboration.GetShareRequest{ + Ref: &collaboration.ShareReference{ + Spec: &collaboration.ShareReference_Id{ + Id: &collaboration.ShareId{ + OpaqueId: "0", + }, + }, + }, + }) - // if we don't need to commit we return earlier - if !s.c.CommitShareToStorageGrant && !s.c.CommitShareToStorageRef { - return res, nil + // We expect a "not found" error when querying ID 0 + // error checking is kind of ugly, because we lose the original error object over grpc + if !strings.HasSuffix(err.Error(), errtypes.NotFound("0").Error()) { + return nil, errtypes.InternalError("ShareManager is not online") } + // Then we set ACLs on the storage layer + // ------------------------------------- + // TODO(labkode): if both commits are enabled they could be done concurrently. if s.c.CommitShareToStorageGrant { // If the share is a denial we call denyGrant instead. @@ -76,20 +85,32 @@ func (s *svc) CreateShare(ctx context.Context, req *collaboration.CreateShareReq if denyGrantStatus.Code != rpc.Code_CODE_OK { return &collaboration.CreateShareResponse{ Status: denyGrantStatus, - }, err + }, nil + } + } else { + addGrantStatus, err := s.addGrant(ctx, req.ResourceInfo.Id, req.Grant.Grantee, req.Grant.Permissions.Permissions) + if err != nil { + log.Error().Err(err).Msg("Failed to Create Share: error during addGrant") + return nil, errors.Wrap(err, "gateway: error adding grant to storage") + } + if addGrantStatus.Code != rpc.Code_CODE_OK { + return &collaboration.CreateShareResponse{ + Status: addGrantStatus, + }, nil } - return res, nil } + } - addGrantStatus, err := s.addGrant(ctx, req.ResourceInfo.Id, req.Grant.Grantee, req.Grant.Permissions.Permissions) - if err != nil { - return nil, errors.Wrap(err, "gateway: error adding grant to storage") - } - if addGrantStatus.Code != rpc.Code_CODE_OK { - return &collaboration.CreateShareResponse{ - Status: addGrantStatus, - }, err - } + // Then we commit to the db + // ------------------------ + res, err := shareClient.CreateShare(ctx, req) + + if err != nil { + log.Error().Msg("Failed to Create Share but ACLs are already set") + return nil, errors.Wrap(err, "gateway: error calling CreateShare") + } + if res.Status.Code != rpc.Code_CODE_OK { + return nil, errors.New("ShareClient returned error: " + res.Status.Code.String() + ": " + res.Status.Message) } return res, nil diff --git a/internal/grpc/services/usershareprovider/usershareprovider.go b/internal/grpc/services/usershareprovider/usershareprovider.go index 6df38cb029..1bd566c289 100644 --- a/internal/grpc/services/usershareprovider/usershareprovider.go +++ b/internal/grpc/services/usershareprovider/usershareprovider.go @@ -172,7 +172,7 @@ func (s *service) GetShare(ctx context.Context, req *collaboration.GetShareReque if err != nil { return &collaboration.GetShareResponse{ Status: status.NewInternal(ctx, err, "error getting share"), - }, nil + }, err } return &collaboration.GetShareResponse{