Merge pull request #4630 from butonic/fix-ocm-share-id

use share id to identify ocm shares

changelog/unreleased/fix-ocm-share-id.md

@@ -0,0 +1,5 @@
+Bugfix: fix ocm-share-id
+
+We now use the share id to correctly identify ocm shares.
+
+https://github.com/cs3org/reva/pull/4630

internal/grpc/interceptors/auth/scope.go

@@ -89,7 +89,7 @@ func expandAndVerifyScope(ctx context.Context, req interface{}, tokenScope map[s
 					return nil
 				}
 			}
-			log.Err(err).Msgf("error resolving reference %s under scope %+v", ref.String(), k)
+			log.Err(err).Interface("ref", ref).Interface("scope", k).Msg("error resolving reference under scope")
 		}
 
 	} else if ref, ok := extractShareRef(req); ok {
@@ -179,6 +179,11 @@ func resolveOCMShare(ctx context.Context, ref *provider.Reference, scope *authpb
 		return err
 	}
 
+	// for ListOCMSharesRequest, the ref resource id is empty and we set path to . to indicate the root of the share
+	if ref.GetResourceId() == nil && ref.Path == "." {
+		ref.ResourceId = share.GetResourceId()
+	}
+
 	if err := checkCacheForNestedResource(ctx, ref, share.ResourceId, client, mgr); err == nil {
 		return nil
 	}
@@ -213,7 +218,7 @@ func checkRelativeReference(ctx context.Context, requested *provider.Reference,
 	if sharedResource.ParentId == nil {
 		// Is the requested resource part of the shared space?
 		if requested.ResourceId.StorageId != sharedResource.Id.StorageId || requested.ResourceId.SpaceId != sharedResource.Id.SpaceId {
-			return errtypes.PermissionDenied("access forbidden via public link")
+			return errtypes.PermissionDenied("space access forbidden via public link")
 		}
 	} else {
 		parentID := sharedResource.ParentId
@@ -388,6 +393,11 @@ func extractRefForReaderRole(req interface{}) (*provider.Reference, bool) {
 		return v.GetRef(), true
 	case *provider.UnlockRequest:
 		return v.GetRef(), true
+
+	// OCM shares
+	case *ocmv1beta1.ListReceivedOCMSharesRequest:
+		return &provider.Reference{Path: "."}, true // we will try to stat the shared node
+
 	}
 
 	return nil, false

internal/grpc/services/gateway/ocmshareprovider.go

@@ -39,6 +39,11 @@ import (
 
 // TODO(labkode): add multi-phase commit logic when commit share or commit ref is enabled.
 func (s *svc) CreateOCMShare(ctx context.Context, req *ocm.CreateOCMShareRequest) (*ocm.CreateOCMShareResponse, error) {
+	if len(req.AccessMethods) == 0 {
+		return &ocm.CreateOCMShareResponse{
+			Status: status.NewInvalidArg(ctx, "access methods cannot be empty"),
+		}, nil
+	}
 	c, err := pool.GetOCMShareProviderClient(s.c.OCMShareProviderEndpoint)
 	if err != nil {
 		return &ocm.CreateOCMShareResponse{
@@ -48,7 +53,7 @@ func (s *svc) CreateOCMShare(ctx context.Context, req *ocm.CreateOCMShareRequest
 
 	res, err := c.CreateOCMShare(ctx, req)
 	if err != nil {
-		return nil, errors.Wrap(err, "gateway: error calling CreateShare")
+		return nil, errors.Wrap(err, "gateway: error calling CreateOCMShare")
 	}
 
 	status, err := s.addGrant(ctx, req.ResourceId, req.Grantee, req.AccessMethods[0].GetWebdavOptions().Permissions, req.Expiration, nil)
@@ -78,7 +83,7 @@ func (s *svc) RemoveOCMShare(ctx context.Context, req *ocm.RemoveOCMShareRequest
 
 	res, err := c.RemoveOCMShare(ctx, req)
 	if err != nil {
-		return nil, errors.Wrap(err, "gateway: error calling RemoveShare")
+		return nil, errors.Wrap(err, "gateway: error calling RemoveOCMShare")
 	}
 
 	return res, nil
@@ -102,7 +107,7 @@ func (s *svc) getOCMShare(ctx context.Context, req *ocm.GetOCMShareRequest) (*oc
 
 	res, err := c.GetOCMShare(ctx, req)
 	if err != nil {
-		return nil, errors.Wrap(err, "gateway: error calling GetShare")
+		return nil, errors.Wrap(err, "gateway: error calling GetOCMShare")
 	}
 
 	return res, nil
@@ -134,7 +139,7 @@ func (s *svc) ListOCMShares(ctx context.Context, req *ocm.ListOCMSharesRequest)
 
 	res, err := c.ListOCMShares(ctx, req)
 	if err != nil {
-		return nil, errors.Wrap(err, "gateway: error calling ListShares")
+		return nil, errors.Wrap(err, "gateway: error calling ListOCMShares")
 	}
 
 	return res, nil
@@ -151,7 +156,7 @@ func (s *svc) UpdateOCMShare(ctx context.Context, req *ocm.UpdateOCMShareRequest
 
 	res, err := c.UpdateOCMShare(ctx, req)
 	if err != nil {
-		return nil, errors.Wrap(err, "gateway: error calling UpdateShare")
+		return nil, errors.Wrap(err, "gateway: error calling UpdateOCMShare")
 	}
 
 	return res, nil
@@ -168,7 +173,7 @@ func (s *svc) ListReceivedOCMShares(ctx context.Context, req *ocm.ListReceivedOC
 
 	res, err := c.ListReceivedOCMShares(ctx, req)
 	if err != nil {
-		return nil, errors.Wrap(err, "gateway: error calling ListReceivedShares")
+		return nil, errors.Wrap(err, "gateway: error calling ListReceivedOCMShares")
 	}
 
 	return res, nil
@@ -223,7 +228,7 @@ func (s *svc) UpdateReceivedOCMShare(ctx context.Context, req *ocm.UpdateReceive
 
 	res, err := c.UpdateReceivedOCMShare(ctx, req)
 	if err != nil {
-		log.Err(err).Msg("gateway: error calling UpdateReceivedShare")
+		log.Err(err).Msg("gateway: error calling UpdateReceivedOCMShare")
 		return &ocm.UpdateReceivedOCMShareResponse{
 			Status: &rpc.Status{
 				Code: rpc.Code_CODE_INTERNAL,
@@ -256,7 +261,7 @@ func (s *svc) UpdateReceivedOCMShare(ctx context.Context, req *ocm.UpdateReceive
 	// handle transfer in case it has not already been accepted
 	if s.isTransferShare(share) && req.GetShare().State == ocm.ShareState_SHARE_STATE_ACCEPTED {
 		if share.State == ocm.ShareState_SHARE_STATE_ACCEPTED {
-			log.Err(err).Msg("gateway: error calling UpdateReceivedShare, share already accepted.")
+			log.Err(err).Msg("gateway: error calling UpdateReceivedOCMShare, share already accepted.")
 			return &ocm.UpdateReceivedOCMShareResponse{
 				Status: &rpc.Status{
 					Code:    rpc.Code_CODE_FAILED_PRECONDITION,
@@ -268,7 +273,7 @@ func (s *svc) UpdateReceivedOCMShare(ctx context.Context, req *ocm.UpdateReceive
 		transferDestinationPath, err := s.getTransferDestinationPath(ctx, req)
 		if err != nil {
 			if err != nil {
-				log.Err(err).Msg("gateway: error calling UpdateReceivedShare")
+				log.Err(err).Msg("gateway: error calling UpdateReceivedOCMShare")
 				return &ocm.UpdateReceivedOCMShareResponse{
 					Status: &rpc.Status{
 						Code: rpc.Code_CODE_INTERNAL,
@@ -279,7 +284,7 @@ func (s *svc) UpdateReceivedOCMShare(ctx context.Context, req *ocm.UpdateReceive
 
 		error := s.handleTransfer(ctx, share, transferDestinationPath)
 		if error != nil {
-			log.Err(error).Msg("gateway: error handling transfer in UpdateReceivedShare")
+			log.Err(error).Msg("gateway: error handling transfer in UpdateReceivedOCMShare")
 			return &ocm.UpdateReceivedOCMShareResponse{
 				Status: &rpc.Status{
 					Code: rpc.Code_CODE_INTERNAL,
@@ -309,17 +314,17 @@ func (s *svc) handleTransfer(ctx context.Context, share *ocm.ReceivedShare, tran
 	}
 	destWebdavEndpoint, err := s.getWebdavEndpoint(ctx, granteeIdp)
 	if err != nil {
-		log.Err(err).Msg("gateway: error calling UpdateReceivedShare")
+		log.Err(err).Msg("gateway: error calling UpdateReceivedOCMShare")
 		return err
 	}
 	destWebdavEndpointURL, err := url.Parse(destWebdavEndpoint)
 	if err != nil {
-		log.Err(err).Msg("gateway: error calling UpdateReceivedShare: unable to parse webdav endpoint \"" + destWebdavEndpoint + "\" into URL structure")
+		log.Err(err).Msg("gateway: error calling UpdateReceivedOCMShare: unable to parse webdav endpoint \"" + destWebdavEndpoint + "\" into URL structure")
 		return err
 	}
 	destWebdavHost, err := s.getWebdavHost(ctx, granteeIdp)
 	if err != nil {
-		log.Err(err).Msg("gateway: error calling UpdateReceivedShare")
+		log.Err(err).Msg("gateway: error calling UpdateReceivedOCMShare")
 		return err
 	}
 	var dstWebdavURLString string
@@ -330,7 +335,7 @@ func (s *svc) handleTransfer(ctx context.Context, share *ocm.ReceivedShare, tran
 	}
 	dstWebdavHostURL, err := url.Parse(dstWebdavURLString)
 	if err != nil {
-		log.Err(err).Msg("gateway: error calling UpdateReceivedShare: unable to parse webdav service host \"" + dstWebdavURLString + "\" into URL structure")
+		log.Err(err).Msg("gateway: error calling UpdateReceivedOCMShare: unable to parse webdav service host \"" + dstWebdavURLString + "\" into URL structure")
 		return err
 	}
 	destServiceHost := dstWebdavHostURL.Host + dstWebdavHostURL.Path
@@ -393,7 +398,7 @@ func (s *svc) GetReceivedOCMShare(ctx context.Context, req *ocm.GetReceivedOCMSh
 
 	res, err := c.GetReceivedOCMShare(ctx, req)
 	if err != nil {
-		return nil, errors.Wrap(err, "gateway: error calling GetReceivedShare")
+		return nil, errors.Wrap(err, "gateway: error calling GetReceivedOCMShare")
 	}
 
 	return res, nil

internal/grpc/services/ocmshareprovider/ocmshareprovider.go

@@ -171,9 +171,9 @@ func getResourceType(info *providerpb.ResourceInfo) string {
 	return "unknown"
 }
 
-func (s *service) webdavURL(ctx context.Context, share *ocm.Share) string {
+func (s *service) webdavURL(_ context.Context, share *ocm.Share) string {
 	// the url is in the form of https://cernbox.cern.ch/remote.php/dav/ocm/token
-	p, _ := url.JoinPath(s.conf.WebDAVEndpoint, "/dav/ocm", share.Token)
+	p, _ := url.JoinPath(s.conf.WebDAVEndpoint, "/dav/ocm", share.GetId().GetOpaqueId())
 	return p
 }
 

internal/grpc/services/publicstorageprovider/publicstorageprovider.go

@@ -405,7 +405,7 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
 					Status: &rpc.Status{Code: rpc.Code_CODE_INVALID_ARGUMENT, Message: err.Error()},
 				}, nil
 			}
-			if resID.SpaceId != utils.PublicStorageSpaceID {
+			if resID.SpaceId != utils.PublicStorageSpaceID && resID.SpaceId != utils.OCMStorageSpaceID {
 				return &provider.ListStorageSpacesResponse{
 					// a specific id was requested, return not found instead of empty list
 					Status: &rpc.Status{Code: rpc.Code_CODE_NOT_FOUND},
@@ -415,7 +415,7 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
 		}
 	}
 
-	info, _, grantee, token, err := s.extractLinkFromScope(ctx)
+	info, share, grantee, token, err := s.extractLinkFromScope(ctx)
 	if err != nil {
 		switch err.(type) {
 		case errtypes.NotFound:
@@ -468,6 +468,9 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
 				SpaceId:   utils.PublicStorageSpaceID,
 				OpaqueId:  token, // the link share has no id, only the token
 			}
+			if ocmShare, ok := share.(*ocm.Share); ok {
+				root.OpaqueId = ocmShare.GetId().GetOpaqueId()
+			}
 			if spaceID != nil {
 				switch {
 				case utils.ResourceIDEqual(spaceID, root):
@@ -506,7 +509,7 @@ func (s *service) extractLinkFromScope(ctx context.Context) (*provider.ResourceI
 			share := &ocm.Share{}
 			err := utils.UnmarshalJSONToProtoV1(v.Resource.Value, share)
 			if err != nil {
-				return nil, nil, nil, "", errtypes.InternalError("failed to unmarshal public share")
+				return nil, nil, nil, "", errtypes.InternalError("failed to unmarshal ocm share")
 			}
 
 			// the share is minimally populated, we need more than the token

internal/http/services/owncloud/ocdav/dav.go

@@ -185,38 +185,38 @@ func (h *DavHandler) Handler(s *svc) http.Handler {
 			}
 
 			// OC10 and Nextcloud (OCM 1.0) are using basic auth for carrying the
-			// shared token.
-			var token string
+			// ocm share id.
+			var ocmshare, sharedSecret string
 			username, _, ok := r.BasicAuth()
 			if ok {
 				// OCM 1.0
-				token = username
-				r.URL.Path = filepath.Join("/", token, r.URL.Path)
-				ctx = context.WithValue(ctx, net.CtxOCM10, true)
+				ocmshare = username
+				sharedSecret = username
+				r.URL.Path = filepath.Join("/", ocmshare, r.URL.Path)
 			} else {
-				token, _ = router.ShiftPath(r.URL.Path)
-				ctx = context.WithValue(ctx, net.CtxOCM10, false)
+				ocmshare, _ = router.ShiftPath(r.URL.Path)
+				sharedSecret = strings.TrimPrefix(r.Header.Get("Authorization"), "Bearer ")
 			}
 
-			authRes, err := handleOCMAuth(ctx, c, token)
+			authRes, err := handleOCMAuth(ctx, c, ocmshare, sharedSecret)
 			switch {
 			case err != nil:
 				log.Error().Err(err).Msg("error during ocm authentication")
 				w.WriteHeader(http.StatusInternalServerError)
 				return
 			case authRes.Status.Code == rpc.Code_CODE_PERMISSION_DENIED:
-				log.Debug().Str("token", token).Msg("permission denied")
+				log.Debug().Str("ocmshare", ocmshare).Msg("permission denied")
 				fallthrough
 			case authRes.Status.Code == rpc.Code_CODE_UNAUTHENTICATED:
-				log.Debug().Str("token", token).Msg("unauthorized")
+				log.Debug().Str("ocmshare", ocmshare).Msg("unauthorized")
 				w.WriteHeader(http.StatusUnauthorized)
 				return
 			case authRes.Status.Code == rpc.Code_CODE_NOT_FOUND:
-				log.Debug().Str("token", token).Msg("not found")
+				log.Debug().Str("ocmshare", ocmshare).Msg("not found")
 				w.WriteHeader(http.StatusNotFound)
 				return
 			case authRes.Status.Code != rpc.Code_CODE_OK:
-				log.Error().Str("token", token).Interface("status", authRes.Status).Msg("grpc auth request failed")
+				log.Error().Str("ocmshare", ocmshare).Interface("status", authRes.Status).Msg("grpc auth request failed")
 				w.WriteHeader(http.StatusInternalServerError)
 				return
 			}
@@ -225,7 +225,7 @@ func (h *DavHandler) Handler(s *svc) http.Handler {
 			ctx = ctxpkg.ContextSetUser(ctx, authRes.User)
 			ctx = metadata.AppendToOutgoingContext(ctx, ctxpkg.TokenHeader, authRes.Token)
 
-			log.Debug().Str("token", token).Interface("user", authRes.User).Msg("OCM user authenticated")
+			log.Debug().Str("ocmshare", ocmshare).Interface("user", authRes.User).Msg("OCM user authenticated")
 
 			r = r.WithContext(ctx)
 			h.OCMSharesHandler.Handler(s).ServeHTTP(w, r)
@@ -375,9 +375,10 @@ func handleSignatureAuth(ctx context.Context, selector pool.Selectable[gatewayv1
 	return c.Authenticate(ctx, &authenticateRequest)
 }
 
-func handleOCMAuth(ctx context.Context, c gatewayv1beta1.GatewayAPIClient, token string) (*gatewayv1beta1.AuthenticateResponse, error) {
+func handleOCMAuth(ctx context.Context, c gatewayv1beta1.GatewayAPIClient, ocmshare, sharedSecret string) (*gatewayv1beta1.AuthenticateResponse, error) {
 	return c.Authenticate(ctx, &gatewayv1beta1.AuthenticateRequest{
-		Type:     "ocmshares",
-		ClientId: token,
+		Type:         "ocmshares",
+		ClientId:     ocmshare,
+		ClientSecret: sharedSecret,
 	})
 }

internal/http/services/owncloud/ocdav/net/net.go

@@ -35,7 +35,6 @@ type ctxKey int
 const (
 	// CtxKeyBaseURI is the key of the base URI context field
 	CtxKeyBaseURI ctxKey = iota
-	CtxOCM10
 
 	// NsDav is the Dav ns
 	NsDav = "DAV:"

pkg/auth/manager/ocmshares/ocmshares.go

@@ -82,10 +82,11 @@ func (m *manager) Configure(ml map[string]interface{}) error {
 	return nil
 }
 
-func (m *manager) Authenticate(ctx context.Context, token, _ string) (*userpb.User, map[string]*authpb.Scope, error) {
-	log := appctx.GetLogger(ctx).With().Str("token", token).Logger()
+func (m *manager) Authenticate(ctx context.Context, ocmshare, sharedSecret string) (*userpb.User, map[string]*authpb.Scope, error) {
+	log := appctx.GetLogger(ctx).With().Str("ocmshare", ocmshare).Logger()
+	// We need to use GetOCMShareByToken, as GetOCMShare would require a user in the context
 	shareRes, err := m.gw.GetOCMShareByToken(ctx, &ocm.GetOCMShareByTokenRequest{
-		Token: token,
+		Token: sharedSecret,
 	})
 
 	switch {
@@ -103,6 +104,12 @@ func (m *manager) Authenticate(ctx context.Context, token, _ string) (*userpb.Us
 		return nil, nil, errtypes.InternalError(shareRes.Status.Message)
 	}
 
+	// compare ocm share id
+	if shareRes.GetShare().GetId().GetOpaqueId() != ocmshare {
+		log.Error().Str("persisted", ocmshare).Str("requested", shareRes.GetShare().GetId().GetOpaqueId()).Msg("mismatching ocm share id for existing secret")
+		return nil, nil, errtypes.InvalidCredentials("invalid shared secret")
+	}
+
 	// the user authenticated using the ocmshares authentication method
 	// is the recipient of the share
 	u := shareRes.Share.Grantee.GetUserId()