From 5964f8f5636c3319b8db62f61f612d6fed020a3b Mon Sep 17 00:00:00 2001 From: Jesse Geens Date: Wed, 23 Oct 2024 10:17:05 +0200 Subject: [PATCH 1/2] Make AddACL only recursive when it is passed a directory --- changelog/unreleased/addacl-recursive.md | 5 +++++ pkg/eosclient/eosgrpc/eosgrpc.go | 26 ++++++++++++++++++++---- 2 files changed, 27 insertions(+), 4 deletions(-) create mode 100644 changelog/unreleased/addacl-recursive.md diff --git a/changelog/unreleased/addacl-recursive.md b/changelog/unreleased/addacl-recursive.md new file mode 100644 index 0000000000..99586e4e45 --- /dev/null +++ b/changelog/unreleased/addacl-recursive.md @@ -0,0 +1,5 @@ +Bugfix: make AddACL only recursive on directories + +The current implementation of AddACL in the EOS gRPC client always sets msg.Recursive = true. This causes issues on the EOS side, because it will try running a recursive find on a file, which fails. This PR fixes this bug in Reva. + +https://github.com/cs3org/reva/pull/4898 \ No newline at end of file diff --git a/pkg/eosclient/eosgrpc/eosgrpc.go b/pkg/eosclient/eosgrpc/eosgrpc.go index f171511a17..6bc189944f 100644 --- a/pkg/eosclient/eosgrpc/eosgrpc.go +++ b/pkg/eosclient/eosgrpc/eosgrpc.go @@ -259,6 +259,13 @@ func (c *Client) AddACL(ctx context.Context, auth, rootAuth eosclient.Authorizat log := appctx.GetLogger(ctx) log.Info().Str("func", "AddACL").Str("uid,gid", auth.Role.UID+","+auth.Role.GID).Str("path", path).Msg("") + // First, we need to figure out if the path is a directory + // to know whether our request should be recursive + fileInfo, err := c.GetFileInfoByPath(ctx, auth, path) + if err != nil { + return err + } + // Init a new NSRequest rq, err := c.initNSRequest(ctx, rootAuth, "") if err != nil { @@ -272,7 +279,7 @@ func (c *Client) AddACL(ctx context.Context, auth, rootAuth eosclient.Authorizat msg := new(erpc.NSRequest_AclRequest) msg.Cmd = erpc.NSRequest_AclRequest_ACL_COMMAND(erpc.NSRequest_AclRequest_ACL_COMMAND_value["MODIFY"]) msg.Type = erpc.NSRequest_AclRequest_ACL_TYPE(erpc.NSRequest_AclRequest_ACL_TYPE_value["SYS_ACL"]) - msg.Recursive = true + msg.Recursive = fileInfo.IsDir msg.Rule = a.CitrineSerialize() msg.Id = new(erpc.MDId) @@ -302,11 +309,17 @@ func (c *Client) RemoveACL(ctx context.Context, auth, rootAuth eosclient.Authori log := appctx.GetLogger(ctx) log.Info().Str("func", "RemoveACL").Str("uid,gid", auth.Role.UID+","+auth.Role.GID).Str("path", path).Msg("") - acls, err := c.getACLForPath(ctx, auth, path) + // First, we need to figure out if the path is a directory + // to know whether our request should be recursive + fileInfo, err := c.GetFileInfoByPath(ctx, auth, path) if err != nil { return err } + acls, err := c.getACLForPath(ctx, auth, path) + if err != nil { + return err + } acls.DeleteEntry(a.Type, a.Qualifier) sysACL := acls.Serialize() @@ -319,7 +332,7 @@ func (c *Client) RemoveACL(ctx context.Context, auth, rootAuth eosclient.Authori msg := new(erpc.NSRequest_AclRequest) msg.Cmd = erpc.NSRequest_AclRequest_ACL_COMMAND(erpc.NSRequest_AclRequest_ACL_COMMAND_value["MODIFY"]) msg.Type = erpc.NSRequest_AclRequest_ACL_TYPE(erpc.NSRequest_AclRequest_ACL_TYPE_value["SYS_ACL"]) - msg.Recursive = true + msg.Recursive = fileInfo.IsDir msg.Rule = sysACL msg.Id = new(erpc.MDId) @@ -387,6 +400,11 @@ func (c *Client) getACLForPath(ctx context.Context, auth eosclient.Authorization log := appctx.GetLogger(ctx) log.Info().Str("func", "GetACLForPath").Str("uid,gid", auth.Role.UID+","+auth.Role.GID).Str("path", path).Msg("") + fileInfo, err := c.GetFileInfoByPath(ctx, auth, path) + if err != nil { + return nil, err + } + // Initialize the common fields of the NSReq rq, err := c.initNSRequest(ctx, auth, "") if err != nil { @@ -396,7 +414,7 @@ func (c *Client) getACLForPath(ctx context.Context, auth eosclient.Authorization msg := new(erpc.NSRequest_AclRequest) msg.Cmd = erpc.NSRequest_AclRequest_ACL_COMMAND(erpc.NSRequest_AclRequest_ACL_COMMAND_value["LIST"]) msg.Type = erpc.NSRequest_AclRequest_ACL_TYPE(erpc.NSRequest_AclRequest_ACL_TYPE_value["SYS_ACL"]) - msg.Recursive = true + msg.Recursive = fileInfo.IsDir msg.Id = new(erpc.MDId) msg.Id.Path = []byte(path) From bb0ac068f2dc88ff0b64efd4653c8684369c5b0b Mon Sep 17 00:00:00 2001 From: Jesse Geens Date: Wed, 23 Oct 2024 17:15:27 +0200 Subject: [PATCH 2/2] Make RemoveACL use AddACL with empty permissions instead of modifying the complete list directly --- changelog/unreleased/acl-grpc.md | 7 ++++ changelog/unreleased/addacl-recursive.md | 5 --- pkg/eosclient/eosgrpc/eosgrpc.go | 51 ++---------------------- 3 files changed, 11 insertions(+), 52 deletions(-) create mode 100644 changelog/unreleased/acl-grpc.md delete mode 100644 changelog/unreleased/addacl-recursive.md diff --git a/changelog/unreleased/acl-grpc.md b/changelog/unreleased/acl-grpc.md new file mode 100644 index 0000000000..a2d723c581 --- /dev/null +++ b/changelog/unreleased/acl-grpc.md @@ -0,0 +1,7 @@ +Bugfix: make ACL operations work over gRPC + +This change solves two issues: +* AddACL would fail, because the current implementation of AddACL in the EOS gRPC client always sets msg.Recursive = true. This causes issues on the EOS side, because it will try running a recursive find on a file, which fails. +* RemoveACL would fail, because it tried matching ACL rules with a uid to ACL rules with a username. This PR changes this approach to use an approach similar to what is used in the binary client: just set the rule that you want to have deleted with no permissions. + +https://github.com/cs3org/reva/pull/4898 \ No newline at end of file diff --git a/changelog/unreleased/addacl-recursive.md b/changelog/unreleased/addacl-recursive.md deleted file mode 100644 index 99586e4e45..0000000000 --- a/changelog/unreleased/addacl-recursive.md +++ /dev/null @@ -1,5 +0,0 @@ -Bugfix: make AddACL only recursive on directories - -The current implementation of AddACL in the EOS gRPC client always sets msg.Recursive = true. This causes issues on the EOS side, because it will try running a recursive find on a file, which fails. This PR fixes this bug in Reva. - -https://github.com/cs3org/reva/pull/4898 \ No newline at end of file diff --git a/pkg/eosclient/eosgrpc/eosgrpc.go b/pkg/eosclient/eosgrpc/eosgrpc.go index 6bc189944f..0e36b56194 100644 --- a/pkg/eosclient/eosgrpc/eosgrpc.go +++ b/pkg/eosclient/eosgrpc/eosgrpc.go @@ -307,54 +307,11 @@ func (c *Client) AddACL(ctx context.Context, auth, rootAuth eosclient.Authorizat // RemoveACL removes the acl from EOS. func (c *Client) RemoveACL(ctx context.Context, auth, rootAuth eosclient.Authorization, path string, a *acl.Entry) error { log := appctx.GetLogger(ctx) - log.Info().Str("func", "RemoveACL").Str("uid,gid", auth.Role.UID+","+auth.Role.GID).Str("path", path).Msg("") + log.Info().Str("func", "RemoveACL").Str("uid,gid", auth.Role.UID+","+auth.Role.GID).Str("path", path).Str("ACL", a.CitrineSerialize()).Msg("") - // First, we need to figure out if the path is a directory - // to know whether our request should be recursive - fileInfo, err := c.GetFileInfoByPath(ctx, auth, path) - if err != nil { - return err - } - - acls, err := c.getACLForPath(ctx, auth, path) - if err != nil { - return err - } - acls.DeleteEntry(a.Type, a.Qualifier) - sysACL := acls.Serialize() - - // Init a new NSRequest - rq, err := c.initNSRequest(ctx, auth, "") - if err != nil { - return err - } - - msg := new(erpc.NSRequest_AclRequest) - msg.Cmd = erpc.NSRequest_AclRequest_ACL_COMMAND(erpc.NSRequest_AclRequest_ACL_COMMAND_value["MODIFY"]) - msg.Type = erpc.NSRequest_AclRequest_ACL_TYPE(erpc.NSRequest_AclRequest_ACL_TYPE_value["SYS_ACL"]) - msg.Recursive = fileInfo.IsDir - msg.Rule = sysACL - - msg.Id = new(erpc.MDId) - msg.Id.Path = []byte(path) - - rq.Command = &erpc.NSRequest_Acl{Acl: msg} - - // Now send the req and see what happens - resp, err := c.cl.Exec(appctx.ContextGetClean(ctx), rq) - e := c.getRespError(resp, err) - if e != nil { - log.Error().Str("func", "RemoveACL").Str("path", path).Str("err", e.Error()).Msg("") - return e - } - - if resp == nil { - return errtypes.NotFound(fmt.Sprintf("Path: %s", path)) - } - - log.Debug().Str("func", "RemoveACL").Str("path", path).Str("resp:", fmt.Sprintf("%#v", resp)).Msg("grpc response") - - return err + // We set permissions to "", so the ACL will serialize to `u:123456=`, which will make EOS delete the entry + a.Permissions = "" + return c.AddACL(ctx, auth, rootAuth, path, eosclient.StartPosition, a) } // UpdateACL updates the EOS acl.