diff --git a/services/graph/pkg/service/v0/drives.go b/services/graph/pkg/service/v0/drives.go index 954cd0791d..c1091f11d3 100644 --- a/services/graph/pkg/service/v0/drives.go +++ b/services/graph/pkg/service/v0/drives.go @@ -20,6 +20,7 @@ import ( storageprovider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" types "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" "github.com/go-chi/render" + "github.com/google/uuid" libregraph "github.com/opencloud-eu/libre-graph-api-go" revactx "github.com/opencloud-eu/reva/v2/pkg/ctx" "github.com/opencloud-eu/reva/v2/pkg/storagespace" @@ -429,6 +430,28 @@ func (g Graph) CreateDrive(w http.ResponseWriter, r *http.Request) { Quota: getQuota(drive.Quota, g.config.Spaces.DefaultQuota), } + if id := drive.GetId(); id != "" { + // the id ends up in storage paths and in "storageid$spaceid!opaqueid" references, + // so only accept the canonical form of the uuids the storage drivers generate themselves + if u, err := uuid.Parse(id); err != nil || u.String() != id { + log.Debug().Str("id", id).Msg("could not create drive: id is not a canonical uuid") + errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "invalid drive id: must be a canonical UUID") + return + } + // personal spaces may use their owner's user id as space id, so an id that belongs to a user + // must not be taken by another space, or that user's personal space could not be created anymore + if _, err := g.identityBackend.GetUser(r.Context(), id, &godata.GoDataRequest{}); err == nil { + log.Debug().Str("id", id).Msg("could not create drive: id belongs to a user") + errorcode.NameAlreadyExists.Render(w, r, http.StatusConflict, "drive already exists") + return + } else if !errorcode.IsErrorCode(err, errorcode.ItemNotFound) { + log.Error().Err(err).Str("id", id).Msg("could not create drive: could not check whether the id belongs to a user") + errorcode.RenderError(w, r, err) + return + } + csr.Opaque = utils.AppendPlainToOpaque(csr.Opaque, "spaceid", id) + } + if drive.Description != nil { csr.Opaque = utils.AppendPlainToOpaque(csr.Opaque, "description", *drive.Description) } @@ -463,6 +486,11 @@ func (g Graph) CreateDrive(w http.ResponseWriter, r *http.Request) { errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, resp.GetStatus().GetMessage()) return } + if resp.GetStatus().GetCode() == cs3rpc.Code_CODE_ALREADY_EXISTS { + log.Debug().Str("grpcmessage", resp.GetStatus().GetMessage()).Msg("could not create drive: already exists") + errorcode.NameAlreadyExists.Render(w, r, http.StatusConflict, "drive already exists") + return + } log.Debug().Interface("grpcmessage", csr).Str("grpc", resp.GetStatus().GetMessage()).Msg("could not create drive: grpc error") errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, resp.GetStatus().GetMessage()) return diff --git a/services/graph/pkg/service/v0/graph_test.go b/services/graph/pkg/service/v0/graph_test.go index 0b2d2a2d3f..11a89671c0 100644 --- a/services/graph/pkg/service/v0/graph_test.go +++ b/services/graph/pkg/service/v0/graph_test.go @@ -8,6 +8,7 @@ import ( "io" "net/http" "net/http/httptest" + "strings" "time" gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" @@ -38,6 +39,8 @@ import ( "github.com/opencloud-eu/opencloud/services/graph/pkg/config" "github.com/opencloud-eu/opencloud/services/graph/pkg/config/defaults" "github.com/opencloud-eu/opencloud/services/graph/pkg/errorcode" + "github.com/opencloud-eu/opencloud/services/graph/pkg/identity" + identitymocks "github.com/opencloud-eu/opencloud/services/graph/pkg/identity/mocks" "github.com/opencloud-eu/opencloud/services/graph/pkg/metrics" service "github.com/opencloud-eu/opencloud/services/graph/pkg/service/v0" "github.com/opencloud-eu/opencloud/services/graph/pkg/unifiedrole" @@ -50,6 +53,7 @@ var _ = Describe("Graph", func() { gatewaySelector pool.Selectable[gateway.GatewayAPIClient] eventsPublisher mocks.Publisher permissionService mocks.Permissions + identityBackend *identitymocks.Backend ctx context.Context cfg *config.Config rr *httptest.ResponseRecorder @@ -86,6 +90,7 @@ var _ = Describe("Graph", func() { eventsPublisher = mocks.Publisher{} permissionService = mocks.Permissions{} + identityBackend = &identitymocks.Backend{} var err error svc, err = service.NewService( @@ -94,6 +99,7 @@ var _ = Describe("Graph", func() { service.WithGatewaySelector(gatewaySelector), service.EventsPublisher(&eventsPublisher), service.PermissionService(&permissionService), + service.WithIdentityBackend(identityBackend), ) Expect(err).ToNot(HaveOccurred()) }) @@ -843,6 +849,156 @@ var _ = Describe("Graph", func() { Expect(*response.Description).To(Equal("This space is for testing")) Expect(*response.LibreGraphContentType).To(Equal("application/vnd.opencloud.test")) }) + Context("drive id handling", func() { + const spaceID = "0b9a7b84-3e71-4a4e-9c34-0e6a4c3f5f21" + + // csr captures the request the handler sends to the gateway + var csr *provider.CreateStorageSpaceRequest + mockSuccessfulCreate := func(createdSpaceID string) { + gatewayClient.On("CreateStorageSpace", mock.Anything, mock.Anything).Run(func(args mock.Arguments) { + csr = args.Get(1).(*provider.CreateStorageSpaceRequest) + }).Return(&provider.CreateStorageSpaceResponse{ + Status: status.NewOK(ctx), + StorageSpace: &provider.StorageSpace{ + Id: &provider.StorageSpaceId{OpaqueId: "pro-1$" + createdSpaceID}, + Name: "Test Space", + SpaceType: "project", + Root: &provider.ResourceId{StorageId: "pro-1", SpaceId: createdSpaceID, OpaqueId: createdSpaceID}, + }, + }, nil) + gatewayClient.On("GetQuota", mock.Anything, mock.Anything).Return(&provider.GetQuotaResponse{ + Status: status.NewOK(ctx), + TotalBytes: 500, + }, nil) + } + + BeforeEach(func() { + csr = nil + permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settingssvc.GetPermissionByIDResponse{ + Permission: &v0.Permission{ + Operation: v0.Permission_OPERATION_READWRITE, + Constraint: v0.Permission_CONSTRAINT_ALL, + }, + }, nil) + }) + + DescribeTable("lets the storage provider generate the id when none is supplied, without looking up users", + func(jsonBody string) { + mockSuccessfulCreate("generated-id") + + r := httptest.NewRequest(http.MethodPost, "/graph/v1.0/drives", bytes.NewBufferString(jsonBody)).WithContext(ctx) + rr := httptest.NewRecorder() + svc.CreateDrive(rr, r) + Expect(rr.Code).To(Equal(http.StatusCreated)) + Expect(csr).ToNot(BeNil()) + Expect(csr.GetOpaque().GetMap()).ToNot(HaveKey("spaceid")) + identityBackend.AssertNotCalled(GinkgoT(), "GetUser", mock.Anything, mock.Anything, mock.Anything) + }, + Entry("absent id", `{"name": "Test Space"}`), + Entry("empty id", `{"name": "Test Space", "id": ""}`), + ) + + It("forwards the id to the storage provider when it does not belong to a user", func() { + identityBackend.On("GetUser", mock.Anything, spaceID, mock.Anything).Return(nil, identity.ErrNotFound) + mockSuccessfulCreate(spaceID) + + jsonBody := []byte(`{"name": "Test Space", "id": "` + spaceID + `"}`) + r := httptest.NewRequest(http.MethodPost, "/graph/v1.0/drives", bytes.NewBuffer(jsonBody)).WithContext(ctx) + rr := httptest.NewRecorder() + svc.CreateDrive(rr, r) + Expect(rr.Code).To(Equal(http.StatusCreated)) + Expect(utils.ReadPlainFromOpaque(csr.GetOpaque(), "spaceid")).To(Equal(spaceID)) + + var response libregraph.Drive + Expect(json.Unmarshal(rr.Body.Bytes(), &response)).To(Succeed()) + Expect(response.GetId()).To(Equal("pro-1$" + spaceID)) + identityBackend.AssertCalled(GinkgoT(), "GetUser", mock.Anything, spaceID, mock.Anything) + }) + + // In the next two tests a successful CreateStorageSpace is mocked on purpose: a handler that + // skips the user lookup then fails on the status and AssertNotCalled checks, not on an + // unexpected mock call. + It("returns 409 Conflict without creating the space when the id belongs to a user", func() { + identityBackend.On("GetUser", mock.Anything, spaceID, mock.Anything).Return(&libregraph.User{Id: libregraph.PtrString(spaceID)}, nil) + mockSuccessfulCreate(spaceID) + + jsonBody := []byte(`{"name": "Test Space", "id": "` + spaceID + `"}`) + r := httptest.NewRequest(http.MethodPost, "/graph/v1.0/drives", bytes.NewBuffer(jsonBody)).WithContext(ctx) + rr := httptest.NewRecorder() + svc.CreateDrive(rr, r) + Expect(rr.Code).To(Equal(http.StatusConflict)) + + var libreError libregraph.OdataError + Expect(json.Unmarshal(rr.Body.Bytes(), &libreError)).To(Succeed()) + Expect(libreError.Error.Code).To(Equal(errorcode.NameAlreadyExists.String())) + Expect(libreError.Error.Message).To(Equal("drive already exists")) + gatewayClient.AssertNotCalled(GinkgoT(), "CreateStorageSpace", mock.Anything, mock.Anything) + }) + + It("does not create the space when the identity backend fails to look up the id", func() { + identityBackend.On("GetUser", mock.Anything, spaceID, mock.Anything).Return(nil, errorcode.New(errorcode.ServiceNotAvailable, "identity backend unavailable")) + mockSuccessfulCreate(spaceID) + + jsonBody := []byte(`{"name": "Test Space", "id": "` + spaceID + `"}`) + r := httptest.NewRequest(http.MethodPost, "/graph/v1.0/drives", bytes.NewBuffer(jsonBody)).WithContext(ctx) + rr := httptest.NewRecorder() + svc.CreateDrive(rr, r) + Expect(rr.Code).To(Equal(http.StatusInternalServerError)) + + var libreError libregraph.OdataError + Expect(json.Unmarshal(rr.Body.Bytes(), &libreError)).To(Succeed()) + Expect(libreError.Error.Code).To(Equal(errorcode.ServiceNotAvailable.String())) + gatewayClient.AssertNotCalled(GinkgoT(), "CreateStorageSpace", mock.Anything, mock.Anything) + }) + + DescribeTable("rejects ids that are not canonical UUIDs before looking up users or calling the storage provider", + func(id string) { + gatewayClient.On("CreateStorageSpace", mock.Anything, mock.Anything).Return(&provider.CreateStorageSpaceResponse{ + Status: status.NewInternal(ctx, "storage provider must not be called"), + }, nil).Maybe() + + body, err := json.Marshal(map[string]string{"name": "Test Space", "id": id}) + Expect(err).ToNot(HaveOccurred()) + r := httptest.NewRequest(http.MethodPost, "/graph/v1.0/drives", bytes.NewBuffer(body)).WithContext(ctx) + rr := httptest.NewRecorder() + svc.CreateDrive(rr, r) + Expect(rr.Code).To(Equal(http.StatusBadRequest)) + + var libreError libregraph.OdataError + Expect(json.Unmarshal(rr.Body.Bytes(), &libreError)).To(Succeed()) + Expect(libreError.Error.Code).To(Equal(errorcode.InvalidRequest.String())) + Expect(libreError.Error.Message).To(Equal("invalid drive id: must be a canonical UUID")) + identityBackend.AssertNotCalled(GinkgoT(), "GetUser", mock.Anything, mock.Anything, mock.Anything) + gatewayClient.AssertNotCalled(GinkgoT(), "CreateStorageSpace", mock.Anything, mock.Anything) + }, + Entry("path traversal", "../../x"), + Entry("storage id delimiter", "pro-1$"+spaceID), + Entry("opaque id delimiter", spaceID+"!"+spaceID), + Entry("arbitrary string", "not-a-uuid"), + Entry("uppercase", strings.ToUpper(spaceID)), + Entry("braces", "{"+spaceID+"}"), + Entry("urn", "urn:uuid:"+spaceID), + Entry("no hyphens", strings.ReplaceAll(spaceID, "-", "")), + ) + + It("returns 409 Conflict when the storage provider reports that the space already exists", func() { + identityBackend.On("GetUser", mock.Anything, spaceID, mock.Anything).Return(nil, identity.ErrNotFound) + gatewayClient.On("CreateStorageSpace", mock.Anything, mock.Anything).Return(&provider.CreateStorageSpaceResponse{ + Status: status.NewAlreadyExists(ctx, nil, "decomposedfs: spaces: space already exists"), + }, nil) + + jsonBody := []byte(`{"name": "Test Space", "id": "` + spaceID + `"}`) + r := httptest.NewRequest(http.MethodPost, "/graph/v1.0/drives", bytes.NewBuffer(jsonBody)).WithContext(ctx) + rr := httptest.NewRecorder() + svc.CreateDrive(rr, r) + Expect(rr.Code).To(Equal(http.StatusConflict)) + + var libreError libregraph.OdataError + Expect(json.Unmarshal(rr.Body.Bytes(), &libreError)).To(Succeed()) + Expect(libreError.Error.Code).To(Equal(errorcode.NameAlreadyExists.String())) + Expect(libreError.Error.Message).To(Equal("drive already exists")) + }) + }) It("Incomplete space", func() { gatewayClient.On("CreateStorageSpace", mock.Anything, mock.Anything).Return(&provider.CreateStorageSpaceResponse{ Status: status.NewOK(ctx), diff --git a/tests/acceptance/bootstrap/SpacesContext.php b/tests/acceptance/bootstrap/SpacesContext.php index 1d422b2f0c..cc30e6ffea 100644 --- a/tests/acceptance/bootstrap/SpacesContext.php +++ b/tests/acceptance/bootstrap/SpacesContext.php @@ -805,6 +805,30 @@ public function theUserCreatesASpaceWithQuotaUsingTheGraphApi( ); } + /** + * @param string $user + * @param string $spaceName + * @param string $spaceType + * @param string $spaceId + * + * @return void + * + * @throws GuzzleException + * @throws Exception + */ + #[When('/^user "([^"]*)" (?:creates|tries to create) a space "([^"]*)" of type "([^"]*)" with id "([^"]*)" using the Graph API$/')] + public function theUserCreatesASpaceWithIdUsingTheGraphApi( + string $user, + string $spaceName, + string $spaceType, + string $spaceId + ): void { + $space = ["name" => $spaceName, "driveType" => $spaceType, "id" => $spaceId]; + $this->featureContext->setResponse( + $this->createSpace($user, $space) + ); + } + /** * Remember the available Spaces * diff --git a/tests/acceptance/features/apiSpaces/createSpace.feature b/tests/acceptance/features/apiSpaces/createSpace.feature index c463560a31..348cae4a11 100644 --- a/tests/acceptance/features/apiSpaces/createSpace.feature +++ b/tests/acceptance/features/apiSpaces/createSpace.feature @@ -153,3 +153,44 @@ Feature: create space | user-role | | Admin | | Space Admin | + + + Scenario: space admin creates a project space with a client-supplied id via the Graph API + Given the administrator has assigned the role "Space Admin" to user "Alice" using the Graph API + When user "Alice" creates a space "Project Mars" of type "project" with id "4e3b1a52-8f0d-4c6e-9a71-2d5f6b8c9e10" using the Graph API + Then the HTTP status code should be "201" + And the JSON response should contain space called "Project Mars" and match + """ + { + "type": "object", + "required": ["id"], + "properties": { + "id": { + "type": "string", + "pattern": "\\$4e3b1a52-8f0d-4c6e-9a71-2d5f6b8c9e10$" + } + } + } + """ + When user "Alice" looks up the single space "Project Mars" via the Graph API by using its id + Then the HTTP status code should be "200" + And the JSON response should contain space called "Project Mars" and match + """ + { + "type": "object", + "required": ["id", "name"], + "properties": { + "id": { + "type": "string", + "pattern": "\\$4e3b1a52-8f0d-4c6e-9a71-2d5f6b8c9e10$" + }, + "name": { + "type": "string", + "enum": ["Project Mars"] + } + } + } + """ + When user "Alice" tries to create a space "Project Venus" of type "project" with id "4e3b1a52-8f0d-4c6e-9a71-2d5f6b8c9e10" using the Graph API + Then the HTTP status code should be "409" + And the user "Alice" should not have a space called "Project Venus"