fix: incorporate code review

services/graph/pkg/linktype/linktype.go

@@ -10,13 +10,17 @@ import (
 	"github.com/owncloud/ocis/v2/services/graph/pkg/unifiedrole"
 )
 
+// NoPermissionMatchError is the message returned by a failed conversion
 const NoPermissionMatchError = "no matching permission set found"
 
+// LinkType contains cs3 permissions and a libregraph
+// linktype reference
 type LinkType struct {
 	Permissions *provider.ResourcePermissions
 	linkType    libregraph.SharingLinkType
 }
 
+// GetPermissions returns the cs3 permissions type
 func (l *LinkType) GetPermissions() *provider.ResourcePermissions {
 	if l != nil {
 		return l.Permissions
@@ -36,6 +40,8 @@ func SharingLinkTypeFromCS3Permissions(permissions *linkv1beta1.PublicSharePermi
 	return nil, unifiedrole.CS3ResourcePermissionsToLibregraphActions(*permissions.GetPermissions())
 }
 
+// CS3ResourcePermissionsFromSharingLink creates a cs3 resource permissions type
+// it returns an error when the link type is not allowed or empty
 func CS3ResourcePermissionsFromSharingLink(createLink libregraph.DriveItemCreateLink, info provider.ResourceType) (*provider.ResourcePermissions, error) {
 	switch createLink.GetType() {
 	case "":
@@ -64,6 +70,7 @@ func CS3ResourcePermissionsFromSharingLink(createLink libregraph.DriveItemCreate
 	}
 }
 
+// NewInternalLinkPermissionSet creates cs3 permissions for the internal link type
 func NewInternalLinkPermissionSet() *LinkType {
 	return &LinkType{
 		Permissions: &provider.ResourcePermissions{},
@@ -71,6 +78,7 @@ func NewInternalLinkPermissionSet() *LinkType {
 	}
 }
 
+// NewViewLinkPermissionSet creates cs3 permissions for the view link type
 func NewViewLinkPermissionSet() *LinkType {
 	return &LinkType{
 		Permissions: &provider.ResourcePermissions{
@@ -86,6 +94,7 @@ func NewViewLinkPermissionSet() *LinkType {
 	}
 }
 
+// NewFileEditLinkPermissionSet creates cs3 permissions for the file edit link type
 func NewFileEditLinkPermissionSet() *LinkType {
 	return &LinkType{
 		Permissions: &provider.ResourcePermissions{
@@ -104,6 +113,7 @@ func NewFileEditLinkPermissionSet() *LinkType {
 	}
 }
 
+// NewFolderEditLinkPermissionSet creates cs3 permissions for the folder edit link type
 func NewFolderEditLinkPermissionSet() *LinkType {
 	return &LinkType{
 		Permissions: &provider.ResourcePermissions{
@@ -125,6 +135,7 @@ func NewFolderEditLinkPermissionSet() *LinkType {
 	}
 }
 
+// NewFolderDropLinkPermissionSet creates cs3 permissions for the folder createOnly link type
 func NewFolderDropLinkPermissionSet() *LinkType {
 	return &LinkType{
 		Permissions: &provider.ResourcePermissions{
@@ -137,6 +148,7 @@ func NewFolderDropLinkPermissionSet() *LinkType {
 	}
 }
 
+// NewFolderUploadLinkPermissionSet creates cs3 permissions for the folder upload link type
 func NewFolderUploadLinkPermissionSet() *LinkType {
 	return &LinkType{
 		Permissions: &provider.ResourcePermissions{
@@ -153,6 +165,7 @@ func NewFolderUploadLinkPermissionSet() *LinkType {
 	}
 }
 
+// GetAvailableLinkTypes returns a slice of all available link types
 func GetAvailableLinkTypes() []*LinkType {
 	return []*LinkType{
 		NewInternalLinkPermissionSet(),

services/graph/pkg/service/v0/links.go

@@ -17,23 +17,16 @@ import (
 	"github.com/owncloud/ocis/v2/services/graph/pkg/service/v0/errorcode"
 )
 
+// CreateLink creates a public link on the cs3 api
 func (g Graph) CreateLink(w http.ResponseWriter, r *http.Request) {
 	logger := g.logger.SubloggerWithRequestID(r.Context())
 	logger.Info().Msg("calling create link")
-	driveID, err := parseIDParam(r, "driveID")
+
-	if err != nil {
+	_, driveItemID, ok := g.GetDriveAndItemIDParam(w, r)
-		errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, err.Error())
+	if !ok {
-		return
-	}
-	driveItemID, err := parseIDParam(r, "itemID")
-	if err != nil {
-		errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, err.Error())
-		return
-	}
-	if driveID.StorageId != driveItemID.StorageId || driveID.SpaceId != driveItemID.SpaceId {
-		errorcode.ItemNotFound.Render(w, r, http.StatusNotFound, "Item does not exist")
 		return
 	}
+
 	var createLink libregraph.DriveItemCreateLink
 	if err := StrictJSONUnmarshal(r.Body, &createLink); err != nil {
 		logger.Error().Err(err).Interface("body", r.Body).Msg("could not create link: invalid body schema definition")

services/graph/pkg/service/v0/links_test.go

@@ -216,7 +216,7 @@ var _ = Describe("createLinkTests", func() {
 			Expect(err).ToNot(HaveOccurred())
 			getError := odataError.GetError()
 			Expect(getError.GetCode()).To(Equal("invalidRequest"))
-			Expect(getError.GetMessage()).To(Equal("invalidRequest: can't split empty storage space ID: invalid storage space id"))
+			Expect(getError.GetMessage()).To(Equal("invalid driveID"))
 		})
 
 		It("fails due to an empty itemID", func() {
@@ -240,7 +240,7 @@ var _ = Describe("createLinkTests", func() {
 			Expect(err).ToNot(HaveOccurred())
 			getError := odataError.GetError()
 			Expect(getError.GetCode()).To(Equal("invalidRequest"))
-			Expect(getError.GetMessage()).To(Equal("invalidRequest: can't split empty storage space ID: invalid storage space id"))
+			Expect(getError.GetMessage()).To(Equal("invalid itemID"))
 		})
 
 		It("fails due to an itemID on a different storage", func() {
@@ -265,7 +265,7 @@ var _ = Describe("createLinkTests", func() {
 			Expect(err).ToNot(HaveOccurred())
 			getError := odataError.GetError()
 			Expect(getError.GetCode()).To(Equal("itemNotFound"))
-			Expect(getError.GetMessage()).To(Equal("Item does not exist"))
+			Expect(getError.GetMessage()).To(Equal("driveID and itemID do not match"))
 		})
 
 		// Public Shares / "links" in graph terms

services/graph/pkg/service/v0/utils.go

@@ -3,6 +3,10 @@ package svc
 import (
 	"encoding/json"
 	"io"
+	"net/http"
+
+	storageprovider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
+	"github.com/owncloud/ocis/v2/services/graph/pkg/service/v0/errorcode"
 )
 
 // StrictJSONUnmarshal is a wrapper around json.Unmarshal that returns an error if the json contains unknown fields.
@@ -11,3 +15,37 @@ func StrictJSONUnmarshal(r io.Reader, v interface{}) error {
 	dec.DisallowUnknownFields()
 	return dec.Decode(v)
 }
+
+// GetDriveAndItemIDParam parses the driveID and itemID from the request,
+// validates the common fields and returns the parsed IDs if ok.
+func (g Graph) GetDriveAndItemIDParam(w http.ResponseWriter, r *http.Request) (storageprovider.ResourceId, storageprovider.ResourceId, bool) {
+	empty := storageprovider.ResourceId{}
+
+	driveID, err := parseIDParam(r, "driveID")
+	if err != nil {
+		g.logger.Debug().Err(err).Msg("could not parse driveID")
+		errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "invalid driveID")
+		return empty, empty, false
+	}
+
+	itemID, err := parseIDParam(r, "itemID")
+	if err != nil {
+		g.logger.Debug().Err(err).Msg("could not parse itemID")
+		errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "invalid itemID")
+		return empty, empty, false
+	}
+
+	if itemID.GetOpaqueId() == "" {
+		g.logger.Debug().Interface("driveID", driveID).Interface("itemID", itemID).Msg("empty item opaqueID")
+		errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "invalid itemID")
+		return empty, empty, false
+	}
+
+	if driveID.GetStorageId() != itemID.GetStorageId() || driveID.GetSpaceId() != itemID.GetSpaceId() {
+		g.logger.Debug().Interface("driveID", driveID).Interface("itemID", itemID).Msg("driveID and itemID do not match")
+		errorcode.ItemNotFound.Render(w, r, http.StatusNotFound, "driveID and itemID do not match")
+		return empty, empty, false
+	}
+
+	return driveID, itemID, true
+}