From 15a0f7394747bb3879a35d25b9684fb5a5126c1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Mon, 2 Sep 2024 22:11:11 +0200 Subject: [PATCH] ocm: fix federated userid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- .../unreleased/ocm-fix-federated-userid.md | 6 ++ .../grpc/services/gateway/ocmshareprovider.go | 4 + .../ocminvitemanager/ocminvitemanager.go | 27 +++++- .../ocmshareprovider/ocmshareprovider.go | 83 ++++++++++++++++--- .../fixtures/ocm-share/ocm-users.demo.json | 4 +- .../grpc/fixtures/ocm-users.demo.json | 4 +- tests/integration/grpc/ocm_share_test.go | 23 ++++- 7 files changed, 131 insertions(+), 20 deletions(-) create mode 100644 changelog/unreleased/ocm-fix-federated-userid.md diff --git a/changelog/unreleased/ocm-fix-federated-userid.md b/changelog/unreleased/ocm-fix-federated-userid.md new file mode 100644 index 0000000000..eb7843cf17 --- /dev/null +++ b/changelog/unreleased/ocm-fix-federated-userid.md @@ -0,0 +1,6 @@ +Bugfix: fix OCM userid encoding + +We now base64 encode the remote userid and provider as the local federated user id. This allows us to always differentiate them from local users and unpack the encoded user id and provider when making requests to the remote ocm provider. + +https://github.com/cs3org/reva/pull/4833 +https://github.com/owncloud/ocis/issues/9927 diff --git a/internal/grpc/services/gateway/ocmshareprovider.go b/internal/grpc/services/gateway/ocmshareprovider.go index 63e2a39eee..d5e0f49916 100644 --- a/internal/grpc/services/gateway/ocmshareprovider.go +++ b/internal/grpc/services/gateway/ocmshareprovider.go @@ -51,11 +51,15 @@ func (s *svc) CreateOCMShare(ctx context.Context, req *ocm.CreateOCMShareRequest }, nil } + // persist the OCM share in the ocm share provider res, err := c.CreateOCMShare(ctx, req) if err != nil { return nil, errors.Wrap(err, "gateway: error calling CreateOCMShare") } + // add a grant to the storage provider so the share can efficiently be listed + // the grant does not grant any permissions. access is granted by the OCM link token + // that is used by the public storage provider to impersonate the resource owner status, err := s.addGrant(ctx, req.ResourceId, req.Grantee, req.AccessMethods[0].GetWebdavOptions().Permissions, req.Expiration, nil) switch { case err != nil: diff --git a/internal/grpc/services/ocminvitemanager/ocminvitemanager.go b/internal/grpc/services/ocminvitemanager/ocminvitemanager.go index 671f864098..fe8cb8f186 100644 --- a/internal/grpc/services/ocminvitemanager/ocminvitemanager.go +++ b/internal/grpc/services/ocminvitemanager/ocminvitemanager.go @@ -20,6 +20,8 @@ package ocminvitemanager import ( "context" + "encoding/base64" + "net/url" "time" gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" @@ -210,6 +212,9 @@ func (s *service) ForwardInvite(ctx context.Context, req *invitepb.ForwardInvite OpaqueId: remoteUser.UserID, } + // we need to use a unique identifier for federated users + remoteUserID = federatedID(remoteUserID) + if err := s.repo.AddRemoteUser(ctx, user.Id, &userpb.User{ Id: remoteUserID, Mail: remoteUser.Email, @@ -240,6 +245,22 @@ func getOCMEndpoint(originProvider *ocmprovider.ProviderInfo) (string, error) { return "", errors.New("ocm endpoint not specified for mesh provider") } +// federatedID creates a federated user id by +// 1. stripping the protocol from the domain and +// 2. base64 encoding the opaque id with the domain to get a unique identifier that cannot collide with other users +func federatedID(id *userpb.UserId) *userpb.UserId { + // strip protocol from the domain + domain := id.Idp + if u, err := url.Parse(domain); err == nil && u.Host != "" { + domain = u.Host + } + return &userpb.UserId{ + Type: userpb.UserType_USER_TYPE_FEDERATED, + Idp: domain, + OpaqueId: base64.URLEncoding.EncodeToString([]byte(id.OpaqueId + "@" + domain)), + } +} + func (s *service) AcceptInvite(ctx context.Context, req *invitepb.AcceptInviteRequest) (*invitepb.AcceptInviteResponse, error) { token, err := s.repo.GetToken(ctx, req.InviteToken.Token) if err != nil { @@ -266,7 +287,11 @@ func (s *service) AcceptInvite(ctx context.Context, req *invitepb.AcceptInviteRe }, nil } - if err := s.repo.AddRemoteUser(ctx, token.GetUserId(), req.GetRemoteUser()); err != nil { + remoteUser := req.GetRemoteUser() + // we need to use a unique identifier for federated users + remoteUser.Id = federatedID(remoteUser.Id) + + if err := s.repo.AddRemoteUser(ctx, token.GetUserId(), remoteUser); err != nil { if errors.Is(err, invite.ErrUserAlreadyAccepted) { return &invitepb.AcceptInviteResponse{ Status: status.NewAlreadyExists(ctx, err, err.Error()), diff --git a/internal/grpc/services/ocmshareprovider/ocmshareprovider.go b/internal/grpc/services/ocmshareprovider/ocmshareprovider.go index b42ce7fa4f..60b58a7411 100644 --- a/internal/grpc/services/ocmshareprovider/ocmshareprovider.go +++ b/internal/grpc/services/ocmshareprovider/ocmshareprovider.go @@ -20,6 +20,7 @@ package ocmshareprovider import ( "context" + "encoding/base64" "fmt" "net/url" "path/filepath" @@ -35,6 +36,7 @@ import ( providerpb "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" typespb "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" "github.com/cs3org/reva/v2/internal/http/services/ocmd" + "github.com/cs3org/reva/v2/pkg/appctx" ctxpkg "github.com/cs3org/reva/v2/pkg/ctx" "github.com/cs3org/reva/v2/pkg/errtypes" "github.com/cs3org/reva/v2/pkg/ocm/client" @@ -157,6 +159,54 @@ func getOCMEndpoint(originProvider *ocmprovider.ProviderInfo) (string, error) { return "", errors.New("ocm endpoint not specified for mesh provider") } +// federatedID creates a federated user id by +// 1. stripping the protocol from the domain and +// 2. base64 encoding the opaque id with the domain to get a unique identifier that cannot collide with other users +func federatedID(id *userpb.UserId) *userpb.UserId { + // strip protocol from the domain + domain := id.Idp + if u, err := url.Parse(domain); err == nil && u.Host != "" { + domain = u.Host + } + return &userpb.UserId{ + Type: userpb.UserType_USER_TYPE_FEDERATED, + Idp: domain, + OpaqueId: base64.URLEncoding.EncodeToString([]byte(id.OpaqueId + "@" + domain)), + } +} + +// remoteID creates a remote user id by +// 1. decoding the base64 encoded opaque id +// 2. splitting the opaque id at the last @ to get the opaque id and the domain +func remoteID(id *userpb.UserId) *userpb.UserId { + bytes, err := base64.URLEncoding.DecodeString(id.GetOpaqueId()) + if err != nil { + return &userpb.UserId{ + Type: userpb.UserType_USER_TYPE_PRIMARY, + Idp: id.Idp, + OpaqueId: id.OpaqueId, + } + } + remote := string(bytes) + last := strings.LastIndex(remote, "@") + if last == -1 { + return &userpb.UserId{ + Type: userpb.UserType_USER_TYPE_PRIMARY, + Idp: id.Idp, + OpaqueId: id.OpaqueId, + } + } + opaqueid := remote[:last] + idp := remote[last+1:] + + return &userpb.UserId{ + Type: userpb.UserType_USER_TYPE_PRIMARY, + Idp: idp, + OpaqueId: opaqueid, + } +} + +// formatOCMUser formats a user id in the form of @ used by the OCM API func formatOCMUser(u *userpb.UserId) string { return fmt.Sprintf("%s@%s", u.OpaqueId, u.Idp) } @@ -288,6 +338,7 @@ func (s *service) CreateOCMShare(ctx context.Context, req *ocm.CreateOCMShareReq Nanos: uint32(now % 1000000000), } + // 1. persist the share in the repository ocmshare := &ocm.Share{ Token: tkn, Name: filepath.Base(info.Path), @@ -314,6 +365,8 @@ func (s *service) CreateOCMShare(ctx context.Context, req *ocm.CreateOCMShareReq }, nil } + // 2. create the share on the remote provider + // 2.a get the ocm endpoint of the remote provider ocmEndpoint, err := getOCMEndpoint(req.RecipientMeshProvider) if err != nil { return &ocm.CreateOCMShareResponse{ @@ -321,18 +374,20 @@ func (s *service) CreateOCMShare(ctx context.Context, req *ocm.CreateOCMShareReq }, nil } + // 2.b replace outgoing user ids with ocm user ids + // unpack the federated user id + shareWith := formatOCMUser(remoteID(req.GetGrantee().GetUserId())) + + // wrap the local user id in a federated user id + owner := formatOCMUser(federatedID(info.Owner)) + sender := formatOCMUser(federatedID(user.Id)) + newShareReq := &client.NewShareRequest{ - ShareWith: formatOCMUser(req.Grantee.GetUserId()), - Name: ocmshare.Name, - ProviderID: ocmshare.Id.OpaqueId, - Owner: formatOCMUser(&userpb.UserId{ - OpaqueId: info.Owner.OpaqueId, - Idp: s.conf.ProviderDomain, - }), - Sender: formatOCMUser(&userpb.UserId{ - OpaqueId: user.Id.OpaqueId, - Idp: s.conf.ProviderDomain, - }), + ShareWith: shareWith, + Name: ocmshare.Name, + ProviderID: ocmshare.Id.OpaqueId, + Owner: owner, + Sender: sender, SenderDisplayName: user.DisplayName, ShareType: "user", ResourceType: getResourceType(info), @@ -343,8 +398,14 @@ func (s *service) CreateOCMShare(ctx context.Context, req *ocm.CreateOCMShareReq newShareReq.Expiration = req.Expiration.Seconds } + // 2.c make POST /shares request newShareRes, err := s.client.NewShare(ctx, ocmEndpoint, newShareReq) if err != nil { + err2 := s.repo.DeleteShare(ctx, user, &ocm.ShareReference{Spec: &ocm.ShareReference_Id{Id: ocmshare.Id}}) + if err2 != nil { + appctx.GetLogger(ctx).Error().Err(err2).Str("shareid", ocmshare.GetId().GetOpaqueId()).Msg("could not delete local ocm share") + } + // TODO remove the share from the local storage switch { case errors.Is(err, client.ErrInvalidParameters): return &ocm.CreateOCMShareResponse{ diff --git a/tests/integration/grpc/fixtures/ocm-share/ocm-users.demo.json b/tests/integration/grpc/fixtures/ocm-share/ocm-users.demo.json index 2b836ff1aa..522e6fb08b 100644 --- a/tests/integration/grpc/fixtures/ocm-share/ocm-users.demo.json +++ b/tests/integration/grpc/fixtures/ocm-share/ocm-users.demo.json @@ -2,7 +2,7 @@ { "id": { "opaque_id": "4c510ada-c86b-4815-8820-42cdf82c3d51", - "idp": "cernbox.cern.ch", + "idp": "https://cernbox.cern.ch", "type": 1 }, "username": "einstein", @@ -32,7 +32,7 @@ { "id": { "opaque_id": "f7fbf8c8-139b-4376-b307-cf0a8c2d0d9c", - "idp": "cesnet.cz", + "idp": "https://cesnet.cz", "type": 1 }, "username": "marie", diff --git a/tests/integration/grpc/fixtures/ocm-users.demo.json b/tests/integration/grpc/fixtures/ocm-users.demo.json index 2b836ff1aa..522e6fb08b 100644 --- a/tests/integration/grpc/fixtures/ocm-users.demo.json +++ b/tests/integration/grpc/fixtures/ocm-users.demo.json @@ -2,7 +2,7 @@ { "id": { "opaque_id": "4c510ada-c86b-4815-8820-42cdf82c3d51", - "idp": "cernbox.cern.ch", + "idp": "https://cernbox.cern.ch", "type": 1 }, "username": "einstein", @@ -32,7 +32,7 @@ { "id": { "opaque_id": "f7fbf8c8-139b-4376-b307-cf0a8c2d0d9c", - "idp": "cesnet.cz", + "idp": "https://cesnet.cz", "type": 1 }, "username": "marie", diff --git a/tests/integration/grpc/ocm_share_test.go b/tests/integration/grpc/ocm_share_test.go index 926de16945..81a9fe0295 100644 --- a/tests/integration/grpc/ocm_share_test.go +++ b/tests/integration/grpc/ocm_share_test.go @@ -21,6 +21,7 @@ package grpc_test import ( "bytes" "context" + "encoding/base64" "io" "net/http" "path/filepath" @@ -116,17 +117,22 @@ var _ = Describe("ocm share", func() { einstein = &userpb.User{ Id: &userpb.UserId{ OpaqueId: "4c510ada-c86b-4815-8820-42cdf82c3d51", - Idp: "cernbox.cern.ch", + Idp: "https://cernbox.cern.ch", Type: userpb.UserType_USER_TYPE_PRIMARY, }, Username: "einstein", Mail: "einstein@cern.ch", DisplayName: "Albert Einstein", } + federatedEinsteinID = &userpb.UserId{ + Type: userpb.UserType_USER_TYPE_FEDERATED, + Idp: "cernbox.cern.ch", + OpaqueId: base64.URLEncoding.EncodeToString([]byte("4c510ada-c86b-4815-8820-42cdf82c3d51@cernbox.cern.ch")), + } marie = &userpb.User{ Id: &userpb.UserId{ OpaqueId: "f7fbf8c8-139b-4376-b307-cf0a8c2d0d9c", - Idp: "cesnet.cz", + Idp: "https://cesnet.cz", Type: userpb.UserType_USER_TYPE_PRIMARY, }, Username: "marie", @@ -134,9 +140,9 @@ var _ = Describe("ocm share", func() { DisplayName: "Marie Curie", } federatedMarieID = &userpb.UserId{ - OpaqueId: marie.Id.OpaqueId, - Idp: marie.Id.Idp, Type: userpb.UserType_USER_TYPE_FEDERATED, + Idp: "cesnet.cz", + OpaqueId: base64.URLEncoding.EncodeToString([]byte("f7fbf8c8-139b-4376-b307-cf0a8c2d0d9c@cesnet.cz")), } ) @@ -192,16 +198,25 @@ var _ = Describe("ocm share", func() { Describe("marie has already accepted the invitation workflow", func() { JustBeforeEach(func() { + // einstein generates an invite token tknRes, err := cernboxgw.GenerateInviteToken(ctxEinstein, &invitev1beta1.GenerateInviteTokenRequest{}) Expect(err).ToNot(HaveOccurred()) Expect(tknRes.Status.Code).To(Equal(rpcv1beta1.Code_CODE_OK)) + // marie accepts it and her provider forwards the invite back to the instance of einstein invRes, err := cesnetgw.ForwardInvite(ctxMarie, &invitev1beta1.ForwardInviteRequest{ InviteToken: tknRes.InviteToken, OriginSystemProvider: cernbox, }) Expect(err).ToNot(HaveOccurred()) Expect(invRes.Status.Code).To(Equal(rpcv1beta1.Code_CODE_OK)) + // Make sure the user is a federated user + // The user type must be a federated user + Expect(invRes.UserId.Type).To(Equal(userpb.UserType_USER_TYPE_FEDERATED)) + // Federated users use the OCM provider id which MUST NOT contain the protocol + Expect(invRes.UserId.Idp).To(Equal("cernbox.cern.ch")) + // The OpaqueId is the base64 encoded user id and the provider id to provent collisions with other users on the graph API + Expect(invRes.UserId.OpaqueId).To(Equal(federatedEinsteinID.OpaqueId)) }) Context("einstein shares a file with view permissions", func() {