decomposedfs now ignores the idp

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

changelog/unreleased/decomposedfs-drop-idp.md

@@ -0,0 +1,6 @@
+Change: decomposedfs no longer stores the idp
+
+We no longer persist the IDP of a user id in decomposedfs grants. As a consequence listing or reading Grants no longer returns the IDP for the Creator.
+It never did for the Grantee. Whatever credentials are used to authenticate a user we internally have to create a UUID anyway. Either by lookung it up in an external service (eg. LDAP or SIEM) or we autoprovision it.
+
+https://github.com/cs3org/reva/pull/3267

internal/grpc/services/gateway/usershareprovider.go

@@ -178,8 +178,8 @@ func (s *svc) GetReceivedShare(ctx context.Context, req *collaboration.GetReceiv
 
 // When updating a received share:
 // if the update contains update for displayName:
-//   1) if received share is mounted: we also do a rename in the storage
-//   2) if received share is not mounted: we only rename in user share provider.
+//  1. if received share is mounted: we also do a rename in the storage
+//  2. if received share is not mounted: we only rename in user share provider.
 func (s *svc) UpdateReceivedShare(ctx context.Context, req *collaboration.UpdateReceivedShareRequest) (*collaboration.UpdateReceivedShareResponse, error) {
 	ctx, span := appctx.GetTracerProvider(ctx).Tracer("gateway").Start(ctx, "Gateway.UpdateReceivedShare")
 	defer span.End()
@@ -367,6 +367,7 @@ func (s *svc) denyGrant(ctx context.Context, id *provider.ResourceId, g *provide
 		Ref:     ref,
 		Grantee: g,
 		Opaque:  opaque,
+		// TODO add creator
 	}
 
 	c, _, err := s.find(ctx, ref)
@@ -393,11 +394,13 @@ func (s *svc) addGrant(ctx context.Context, id *provider.ResourceId, g *provider
 		ResourceId: id,
 	}
 
+	creator := ctxpkg.ContextMustGetUser(ctx)
 	grantReq := &provider.AddGrantRequest{
 		Ref: ref,
 		Grant: &provider.Grant{
 			Grantee:     g,
 			Permissions: p,
+			Creator:     creator.GetId(),
 		},
 		Opaque: opaque,
 	}
@@ -425,11 +428,14 @@ func (s *svc) updateGrant(ctx context.Context, id *provider.ResourceId, g *provi
 	ref := &provider.Reference{
 		ResourceId: id,
 	}
+
+	creator := ctxpkg.ContextMustGetUser(ctx)
 	grantReq := &provider.UpdateGrantRequest{
 		Ref: ref,
 		Grant: &provider.Grant{
 			Grantee:     g,
 			Permissions: p,
+			Creator:     creator.GetId(),
 		},
 	}
 

pkg/storage/utils/ace/ace.go

@@ -30,86 +30,146 @@ import (
 	provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
 )
 
-// ACE represents an Access Control Entry, mimicing NFSv4 ACLs
-// The difference is tht grant ACEs are not propagated down the tree when being set on a dir.
-// The tradeoff is that every read has to check the permissions of all path segments up to the root,
-// to determine the permissions. But reads can be scaled better than writes, so here we are.
-// See https://github.com/cs3org/reva/pull/1170#issuecomment-700526118 for more details.
-//
-// The following is taken from the nfs4_acl man page,
-// see https://linux.die.net/man/5/nfs4_acl:
-// the extended attributes will look like this
-// "user.oc.grant.<type>:<flags>:<principal>:<permissions>"
-// - *type* will be limited to A for now
-//     A: Allow - allow *principal* to perform actions requiring *permissions*
-//   In the future we can use:
-//     U: aUdit - log any attempted access by principal which requires
-//                permissions.
-//     L: aLarm - generate a system alarm at any attempted access by
-//                principal which requires permissions
-//     D: for Deny is not recommended
-// - *flags* for now empty or g for group, no inheritance yet
-//   - d directory-inherit - newly-created subdirectories will inherit the
-//                           ACE.
-//   - f file-inherit - newly-created files will inherit the ACE, minus its
-//                      inheritance flags. Newly-created subdirectories
-//                      will inherit the ACE; if directory-inherit is not
-//                      also specified in the parent ACE, inherit-only will
-//                      be added to the inherited ACE.
-//   - n no-propagate-inherit - newly-created subdirectories will inherit
-//                              the ACE, minus its inheritance flags.
-//   - i inherit-only - the ACE is not considered in permissions checks,
-//                      but it is heritable; however, the inherit-only
-//                      flag is stripped from inherited ACEs.
-// - *principal* a named user, group or special principal
-//   - the oidc sub@iss maps nicely to this
-//   - 'OWNER@', 'GROUP@', and 'EVERYONE@', which are, respectively, analogous to the POSIX user/group/other
-// - *permissions*
-//   - r read-data (files) / list-directory (directories)
-//   - w write-data (files) / create-file (directories)
-//   - a append-data (files) / create-subdirectory (directories)
-//   - x execute (files) / change-directory (directories)
-//   - d delete - delete the file/directory. Some servers will allow a delete to occur if either this permission is set in the file/directory or if the delete-child permission is set in its parent directory.
-//   - D delete-child - remove a file or subdirectory from within the given directory (directories only)
-//   - t read-attributes - read the attributes of the file/directory.
-//   - T write-attributes - write the attributes of the file/directory.
-//   - n read-named-attributes - read the named attributes of the file/directory.
-//   - N write-named-attributes - write the named attributes of the file/directory.
-//   - c read-ACL - read the file/directory NFSv4 ACL.
-//   - C write-ACL - write the file/directory NFSv4 ACL.
-//   - o write-owner - change ownership of the file/directory.
-//   - y synchronize - allow clients to use synchronous I/O with the server.
-// TODO implement OWNER@ as "user.oc.grant.A::OWNER@:rwaDxtTnNcCy"
-// attribute names are limited to 255 chars by the linux kernel vfs, values to 64 kb
-// ext3 extended attributes must fit inside a single filesystem block ... 4096 bytes
-// that leaves us with "user.oc.grant.A::someonewithaslightlylongersubject@whateverissuer:rwaDxtTnNcCy" ~80 chars
-// 4096/80 = 51 shares ... with luck we might move the actual permissions to the value, saving ~15 chars
-// 4096/64 = 64 shares ... still meh ... we can do better by using ints instead of strings for principals
-//   "user.oc.grant.u:100000" is pretty neat, but we can still do better: base64 encode the int
-//   "user.oc.grant.u:6Jqg" but base64 always has at least 4 chars, maybe hex is better for smaller numbers
-//   well use 4 chars in addition to the ace: "user.oc.grant.u:////" = 65535 -> 18 chars
-// 4096/18 = 227 shares
-// still ... ext attrs for this are not infinite scale ...
-// so .. attach shares via fileid.
-// <userhome>/metadata/<fileid>/shares, similar to <userhome>/files
-// <userhome>/metadata/<fileid>/shares/u/<issuer>/<subject>/A:fdi:rwaDxtTnNcCy permissions as filename to keep them in the stat cache?
-//
-// whatever ... 50 shares is good enough. If more is needed we can delegate to the metadata
-// if "user.oc.grant.M" is present look inside the metadata app.
-// - if we cannot set an ace we might get an io error.
-//   in that case convert all shares to metadata and try to set "user.oc.grant.m"
-//
-// what about metadata like share creator, share time, expiry?
-// - creator is same as owner, but can be set
-// - share date, or abbreviated st is a unix timestamp
-// - expiry is a unix timestamp
-// - can be put inside the value
-// - we need to reorder the fields:
-// "user.oc.grant.<u|g|o>:<principal>" -> "kv:t=<type>:f=<flags>:p=<permissions>:st=<share time>:c=<creator>:e=<expiry>:pw=<password>:n=<name>"
-// "user.oc.grant.<u|g|o>:<principal>" -> "v1:<type>:<flags>:<permissions>:<share time>:<creator>:<expiry>:<password>:<name>"
-// or the first byte determines the format
-// 0x00 = key value
-// 0x01 = v1 ...
+/*
+ACE represents an Access Control Entry, mimicing NFSv4 ACLs
+The difference is tht grant ACEs are not propagated down the tree when being set on a dir.
+The tradeoff is that every read has to check the permissions of all path segments up to the root,
+to determine the permissions. But reads can be scaled better than writes, so here we are.
+See https://github.com/cs3org/reva/pull/1170#issuecomment-700526118 for more details.
+
+The following is taken from the nfs4_acl man page,
+see https://linux.die.net/man/5/nfs4_acl:
+the extended attributes will look like this
+"user.oc.grant.<type>:<flags>:<principal>:<permissions>"
+
+*type*: will be limited to A for now
+
+  - A: Allow
+
+    allow *principal* to perform actions requiring *permissions*
+    In the future we can use:
+
+  - U: aUdit
+
+    log any attempted access by principal which requires
+    permissions.
+
+  - L: aLarm
+
+    generate a system alarm at any attempted access by
+    principal which requires permissions
+
+  - D: for Deny is not recommended
+
+*flags*: for now empty or g for group, no inheritance yet
+
+  - d directory-inherit
+
+    newly-created subdirectories will inherit the
+    ACE.
+
+  - f file-inherit
+
+    newly-created files will inherit the ACE, minus its
+    inheritance flags. Newly-created subdirectories
+    will inherit the ACE; if directory-inherit is not
+    also specified in the parent ACE, inherit-only will
+    be added to the inherited ACE.
+
+  - n no-propagate-inherit
+
+    newly-created subdirectories will inherit
+    the ACE, minus its inheritance flags.
+
+  - i inherit-only
+
+    the ACE is not considered in permissions checks,
+    but it is heritable; however, the inherit-only
+    flag is stripped from inherited ACEs.
+
+*principal* a named user, group or special principal
+
+  - the oidc sub@iss maps nicely to this
+
+  - 'OWNER@', 'GROUP@', and 'EVERYONE@', which are, respectively, analogous to the POSIX user/group/other
+
+*permissions*
+
+  - r read-data (files) / list-directory (directories)
+
+  - w write-data (files) / create-file (directories)
+
+  - a append-data (files) / create-subdirectory (directories)
+
+  - x execute (files) / change-directory (directories)
+
+  - d delete - delete the file/directory. Some servers will allow a delete to occur if either this permission is set in the file/directory or if the delete-child permission is set in its parent directory.
+
+  - D delete-child - remove a file or subdirectory from within the given directory (directories only)
+
+  - t read-attributes - read the attributes of the file/directory.
+
+  - T write-attributes - write the attributes of the file/directory.
+
+  - n read-named-attributes - read the named attributes of the file/directory.
+
+  - N write-named-attributes - write the named attributes of the file/directory.
+
+  - c read-ACL - read the file/directory NFSv4 ACL.
+
+  - C write-ACL - write the file/directory NFSv4 ACL.
+
+  - o write-owner - change ownership of the file/directory.
+
+  - y synchronize - allow clients to use synchronous I/O with the server.
+
+*TODO*
+
+  - implement OWNER@ as "user.oc.grant.A::OWNER@:rwaDxtTnNcCy"
+
+*Limitations*
+
+	attribute names are limited to 255 chars by the linux kernel vfs, values to 64 kb
+	ext3 extended attributes must fit inside a single filesystem block ... 4096 bytes
+	that leaves us with "user.oc.grant.A::someonewithaslightlylongersubject@whateverissuer:rwaDxtTnNcCy" ~80 chars
+	4096/80 = 51 shares ... with luck we might move the actual permissions to the value, saving ~15 chars
+	4096/64 = 64 shares ... still meh ... we can do better by using ints instead of strings for principals
+
+	"user.oc.grant.u:100000" is pretty neat, but we can still do better: base64 encode the int
+	"user.oc.grant.u:6Jqg" but base64 always has at least 4 chars, maybe hex is better for smaller numbers
+	well use 4 chars in addition to the ace: "user.oc.grant.u:////" = 65535 -> 18 chars
+
+	4096/18 = 227 shares
+	still ... ext attrs for this are not infinite scale ...
+	so .. attach shares via fileid.
+	<userhome>/metadata/<fileid>/shares, similar to <userhome>/files
+	<userhome>/metadata/<fileid>/shares/u/<issuer>/<subject>/A:fdi:rwaDxtTnNcCy permissions as filename to keep them in the stat cache?
+
+	whatever ... 50 shares is good enough. If more is needed we can delegate to the metadata
+	if "user.oc.grant.M" is present look inside the metadata app.
+
+*Notes*
+
+  - if we cannot set an ace we might get an io error.
+    in that case convert all shares to metadata and try to set "user.oc.grant.m"
+
+    what about metadata like share creator, share time, expiry?
+
+  - creator is same as owner, but can be set
+
+  - share date, or abbreviated st is a unix timestamp
+
+  - expiry is a unix timestamp
+
+  - can be put inside the value
+
+  - we need to reorder the fields:
+    "user.oc.grant.<u|g|o>:<principal>" -> "kv:t=<type>:f=<flags>:p=<permissions>:st=<share time>:c=<creator>:e=<expiry>:pw=<password>:n=<name>"
+    "user.oc.grant.<u|g|o>:<principal>" -> "v1:<type>:<flags>:<permissions>:<share time>:<creator>:<expiry>:<password>:<name>"
+    or the first byte determines the format
+    0x00 = key value
+    0x01 = v1 ...
+*/
 type ACE struct {
 	// NFSv4 acls
 	_type       string // t
@@ -149,8 +209,8 @@ func (e *ACE) Principal() string {
 // Marshal renders a principal and byte[] that can be used to persist the ACE as an extended attribute
 func (e *ACE) Marshal() (string, []byte) {
 	// NOTE: first byte will be replaced after converting to byte array
-	b := bytes.NewBuffer([]byte{})
-	w := csv.NewWriter(b)
+	var b bytes.Buffer
+	w := csv.NewWriter(&b)
 	w.Comma = ':'
 	if err := w.Write([]string{
 		fmt.Sprintf("_t=%s", e._type),
@@ -395,16 +455,12 @@ func getACEPerm(set *provider.ResourcePermissions) string {
 }
 
 func userIDToString(u *userpb.UserId) string {
-	return u.GetOpaqueId() + "!" + u.GetIdp()
+	return u.GetOpaqueId()
 }
 
 func userIDFromString(uid string) *userpb.UserId {
 	s := strings.SplitN(uid, "!", 2)
-	if len(s) != 2 {
-		return nil
-	}
 	return &userpb.UserId{
 		OpaqueId: s[0],
-		Idp:      s[1],
 	}
 }

pkg/storage/utils/ace/ace_test.go

@@ -45,7 +45,6 @@ var _ = Describe("ACE", func() {
 			Permissions: &provider.ResourcePermissions{},
 			Creator: &userpb.UserId{
 				OpaqueId: "baz",
-				Idp:      "idp",
 			},
 		}
 

pkg/storage/utils/decomposedfs/grants.go

@@ -54,10 +54,6 @@ func (fs *Decomposedfs) AddGrant(ctx context.Context, ref *provider.Reference, g
 		return fs.UpdateGrant(ctx, ref, g)
 	}
 
-	// add acting user
-	u := ctxpkg.ContextMustGetUser(ctx)
-	g.Creator = u.GetId()
-
 	owner := node.Owner()
 	grants, err := node.ListGrants(ctx)
 	if err != nil {

pkg/storage/utils/decomposedfs/grants_test.go

@@ -65,6 +65,9 @@ var _ = Describe("Grants", func() {
 				Move:   true,
 				Delete: false,
 			},
+			Creator: &userpb.UserId{
+				OpaqueId: helpers.OwnerID,
+			},
 		}
 	})
 
@@ -119,7 +122,7 @@ var _ = Describe("Grants", func() {
 				localPath := n.InternalPath()
 				attr, err := xattr.Get(localPath, xattrs.GrantUserAcePrefix+grant.Grantee.GetUserId().OpaqueId)
 				Expect(err).ToNot(HaveOccurred())
-				Expect(string(attr)).To(Equal(fmt.Sprintf("\x00t=A:f=:p=rw:c=%s", o.GetOpaqueId()+"!"+o.GetIdp()+"\n"))) // NOTE: this tests ace package
+				Expect(string(attr)).To(Equal(fmt.Sprintf("\x00t=A:f=:p=rw:c=%s", o.GetOpaqueId()+"\n"))) // NOTE: this tests ace package
 			})
 
 			It("creates a storage space per created grant", func() {