Skip to content

Commit

Permalink
ocm: fix federated userid
Browse files Browse the repository at this point in the history
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
  • Loading branch information
butonic committed Sep 4, 2024
1 parent c991ee0 commit 3e6ce69
Show file tree
Hide file tree
Showing 9 changed files with 155 additions and 38 deletions.
6 changes: 6 additions & 0 deletions changelog/unreleased/ocm-fix-federated-userid.md
Original file line number Diff line number Diff line change
@@ -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
4 changes: 4 additions & 0 deletions internal/grpc/services/gateway/ocmshareprovider.go
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
10 changes: 9 additions & 1 deletion internal/grpc/services/ocminvitemanager/ocminvitemanager.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
"github.com/cs3org/reva/v2/pkg/ocm/client"
"github.com/cs3org/reva/v2/pkg/ocm/invite"
"github.com/cs3org/reva/v2/pkg/ocm/invite/repository/registry"
ocmuser "github.com/cs3org/reva/v2/pkg/ocm/user"
"github.com/cs3org/reva/v2/pkg/rgrpc"
"github.com/cs3org/reva/v2/pkg/rgrpc/status"
"github.com/cs3org/reva/v2/pkg/rgrpc/todo/pool"
Expand Down Expand Up @@ -210,6 +211,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 = ocmuser.FederatedID(remoteUserID)

if err := s.repo.AddRemoteUser(ctx, user.Id, &userpb.User{
Id: remoteUserID,
Mail: remoteUser.Email,
Expand Down Expand Up @@ -266,7 +270,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 = ocmuser.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()),
Expand Down
40 changes: 24 additions & 16 deletions internal/grpc/services/ocmshareprovider/ocmshareprovider.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ package ocmshareprovider

import (
"context"
"fmt"
"net/url"
"path/filepath"
"strings"
Expand All @@ -35,11 +34,13 @@ 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"
"github.com/cs3org/reva/v2/pkg/ocm/share"
"github.com/cs3org/reva/v2/pkg/ocm/share/repository/registry"
ocmuser "github.com/cs3org/reva/v2/pkg/ocm/user"
"github.com/cs3org/reva/v2/pkg/rgrpc"
"github.com/cs3org/reva/v2/pkg/rgrpc/status"
"github.com/cs3org/reva/v2/pkg/rgrpc/todo/pool"
Expand Down Expand Up @@ -157,10 +158,6 @@ func getOCMEndpoint(originProvider *ocmprovider.ProviderInfo) (string, error) {
return "", errors.New("ocm endpoint not specified for mesh provider")
}

func formatOCMUser(u *userpb.UserId) string {
return fmt.Sprintf("%s@%s", u.OpaqueId, u.Idp)
}

func getResourceType(info *providerpb.ResourceInfo) string {
switch info.Type {
case providerpb.ResourceType_RESOURCE_TYPE_FILE:
Expand Down Expand Up @@ -288,6 +285,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),
Expand All @@ -314,25 +312,29 @@ 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{
Status: status.NewInvalidArg(ctx, "the selected provider does not have an OCM endpoint"),
}, nil
}

// 2.b replace outgoing user ids with ocm user ids
// unpack the federated user id
shareWith := ocmuser.FormatOCMUser(ocmuser.RemoteID(req.GetGrantee().GetUserId()))

// wrap the local user id in a federated user id
owner := ocmuser.FormatOCMUser(ocmuser.FederatedID(info.Owner))
sender := ocmuser.FormatOCMUser(ocmuser.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),
Expand All @@ -343,8 +345,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{
Expand Down
55 changes: 55 additions & 0 deletions pkg/ocm/user/user.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
package user

import (
"encoding/base64"
"fmt"
"net/url"
"strings"

userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1"
)

// 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 {
remoteId := &userpb.UserId{
Type: userpb.UserType_USER_TYPE_PRIMARY,
Idp: id.Idp,
OpaqueId: id.OpaqueId,
}
bytes, err := base64.URLEncoding.DecodeString(id.GetOpaqueId())
if err != nil {
return remoteId
}
remote := string(bytes)
last := strings.LastIndex(remote, "@")
if last == -1 {
return remoteId
}
remoteId.OpaqueId = remote[:last]
remoteId.Idp = remote[last+1:]

return remoteId
}

// FormatOCMUser formats a user id in the form of <opaque-id>@<idp> used by the OCM API in shareWith, owner and creator fields
func FormatOCMUser(u *userpb.UserId) string {
return fmt.Sprintf("%s@%s", u.OpaqueId, u.Idp)
}
4 changes: 2 additions & 2 deletions tests/integration/grpc/fixtures/ocm-share/ocm-users.demo.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -32,7 +32,7 @@
{
"id": {
"opaque_id": "f7fbf8c8-139b-4376-b307-cf0a8c2d0d9c",
"idp": "cesnet.cz",
"idp": "https://cesnet.cz",
"type": 1
},
"username": "marie",
Expand Down
4 changes: 2 additions & 2 deletions tests/integration/grpc/fixtures/ocm-users.demo.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -32,7 +32,7 @@
{
"id": {
"opaque_id": "f7fbf8c8-139b-4376-b307-cf0a8c2d0d9c",
"idp": "cesnet.cz",
"idp": "https://cesnet.cz",
"type": 1
},
"username": "marie",
Expand Down
47 changes: 34 additions & 13 deletions tests/integration/grpc/ocm_invitation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ package grpc_test
import (
"bytes"
"context"
"encoding/base64"
"encoding/json"
"fmt"
"net/http"
Expand Down Expand Up @@ -106,23 +107,43 @@ var _ = Describe("ocm invitation workflow", 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: "[email protected]",
DisplayName: "Albert Einstein",
}
federatedEinstein = &userpb.User{
Id: &userpb.UserId{
Type: userpb.UserType_USER_TYPE_FEDERATED,
Idp: "cernbox.cern.ch",
OpaqueId: base64.URLEncoding.EncodeToString([]byte("[email protected]")),
},
Username: "einstein",
Mail: "[email protected]",
DisplayName: "Albert Einstein",
}
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",
Mail: "[email protected]",
DisplayName: "Marie Curie",
}
federatedMarie = &userpb.User{
Id: &userpb.UserId{
Type: userpb.UserType_USER_TYPE_FEDERATED,
Idp: "cesnet.cz",
OpaqueId: base64.URLEncoding.EncodeToString([]byte("[email protected]")),
},
Username: "marie",
Mail: "[email protected]",
DisplayName: "Marie Curie",
}
)

for _, driver := range []string{"json"} {
Expand Down Expand Up @@ -198,21 +219,21 @@ var _ = Describe("ocm invitation workflow", func() {

Expect(forwardRes.DisplayName).To(Equal(einstein.DisplayName))
Expect(forwardRes.Email).To(Equal(einstein.Mail))
Expect(utils.UserEqual(forwardRes.UserId, einstein.Id)).To(BeTrue())
Expect(utils.UserEqual(forwardRes.UserId, federatedEinstein.Id)).To(BeTrue())

usersRes1, err := cernboxgw.FindAcceptedUsers(ctxEinstein, &invitepb.FindAcceptedUsersRequest{})
Expect(err).ToNot(HaveOccurred())
Expect(usersRes1.Status.Code).To(Equal(rpc.Code_CODE_OK))
Expect(usersRes1.AcceptedUsers).To(HaveLen(1))
info1 := usersRes1.AcceptedUsers[0]
Expect(ocmUserEqual(info1, marie)).To(BeTrue())
Expect(ocmUserEqual(info1, federatedMarie)).To(BeTrue())

usersRes2, err := cesnetgw.FindAcceptedUsers(ctxMarie, &invitepb.FindAcceptedUsersRequest{})
Expect(err).ToNot(HaveOccurred())
Expect(usersRes2.Status.Code).To(Equal(rpc.Code_CODE_OK))
Expect(usersRes2.AcceptedUsers).To(HaveLen(1))
info2 := usersRes2.AcceptedUsers[0]
Expect(ocmUserEqual(info2, einstein)).To(BeTrue())
Expect(ocmUserEqual(info2, federatedEinstein)).To(BeTrue())
})

})
Expand All @@ -222,8 +243,8 @@ var _ = Describe("ocm invitation workflow", func() {
var cleanup func()
BeforeEach(func() {
variables, cleanup, err = initData(driver, nil, map[string][]*userpb.User{
einstein.Id.OpaqueId: {marie},
marie.Id.OpaqueId: {einstein},
einstein.Id.OpaqueId: {federatedMarie},
marie.Id.OpaqueId: {federatedEinstein},
})
Expect(err).ToNot(HaveOccurred())
})
Expand Down Expand Up @@ -417,16 +438,16 @@ var _ = Describe("ocm invitation workflow", func() {

users, code = findAccepted(tknEinstein, cernboxURL)
Expect(code).To(Equal(http.StatusOK))
Expect(ocmUsersEqual(list.Map(users, remoteToCs3User), []*userpb.User{marie})).To(BeTrue())
Expect(ocmUsersEqual(list.Map(users, remoteToCs3User), []*userpb.User{federatedMarie})).To(BeTrue())
})
})

Context("marie already accepted an invitation before", func() {
var cleanup func()
BeforeEach(func() {
variables, cleanup, err = initData(driver, nil, map[string][]*userpb.User{
einstein.Id.OpaqueId: {marie},
marie.Id.OpaqueId: {einstein},
einstein.Id.OpaqueId: {federatedMarie},
marie.Id.OpaqueId: {federatedEinstein},
})
Expect(err).ToNot(HaveOccurred())
})
Expand All @@ -438,14 +459,14 @@ var _ = Describe("ocm invitation workflow", func() {
It("fails the invitation workflow", func() {
users, code := findAccepted(tknEinstein, cernboxURL)
Expect(code).To(Equal(http.StatusOK))
Expect(ocmUsersEqual(list.Map(users, remoteToCs3User), []*userpb.User{marie})).To(BeTrue())
Expect(ocmUsersEqual(list.Map(users, remoteToCs3User), []*userpb.User{federatedMarie})).To(BeTrue())

code = acceptInvite(tknMarie, cesnetURL, "cernbox.cern.ch", token)
Expect(code).To(Equal(http.StatusConflict))

users, code = findAccepted(tknEinstein, cernboxURL)
Expect(code).To(Equal(http.StatusOK))
Expect(ocmUsersEqual(list.Map(users, remoteToCs3User), []*userpb.User{marie})).To(BeTrue())
Expect(ocmUsersEqual(list.Map(users, remoteToCs3User), []*userpb.User{federatedMarie})).To(BeTrue())
})
})

Expand Down Expand Up @@ -507,7 +528,7 @@ var _ = Describe("ocm invitation workflow", func() {

users, code = findAccepted(tknEinstein, cernboxURL)
Expect(code).To(Equal(http.StatusOK))
Expect(ocmUsersEqual(list.Map(users, remoteToCs3User), []*userpb.User{marie})).To(BeTrue())
Expect(ocmUsersEqual(list.Map(users, remoteToCs3User), []*userpb.User{federatedMarie})).To(BeTrue())
})
})

Expand Down
Loading

0 comments on commit 3e6ce69

Please sign in to comment.