From d9fa7455b6e23d66510af52ee3a4a6b61728eb81 Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Fri, 11 Nov 2022 10:45:11 +0100 Subject: [PATCH 1/5] add more unit tests for the drives operations --- .../ocis/messages/settings/v0/settings.pb.go | 3 - .../services/thumbnails/v0/thumbnails.pb.go | 1 - services/graph/pkg/service/v0/drives.go | 2 +- services/graph/pkg/service/v0/graph.go | 1 + services/graph/pkg/service/v0/graph_test.go | 161 ++++++++++++++++-- services/graph/pkg/service/v0/instrument.go | 10 ++ services/graph/pkg/service/v0/logging.go | 10 ++ services/graph/pkg/service/v0/service.go | 2 + services/graph/pkg/service/v0/tracing.go | 10 ++ 9 files changed, 184 insertions(+), 16 deletions(-) diff --git a/protogen/gen/ocis/messages/settings/v0/settings.pb.go b/protogen/gen/ocis/messages/settings/v0/settings.pb.go index 70600cb9f19..ede907dba32 100644 --- a/protogen/gen/ocis/messages/settings/v0/settings.pb.go +++ b/protogen/gen/ocis/messages/settings/v0/settings.pb.go @@ -589,7 +589,6 @@ type Setting struct { DisplayName string `protobuf:"bytes,3,opt,name=display_name,json=displayName,proto3" json:"display_name,omitempty"` Description string `protobuf:"bytes,4,opt,name=description,proto3" json:"description,omitempty"` // Types that are assignable to Value: - // // *Setting_IntValue // *Setting_StringValue // *Setting_BoolValue @@ -1194,7 +1193,6 @@ type Value struct { AccountUuid string `protobuf:"bytes,4,opt,name=account_uuid,json=accountUuid,proto3" json:"account_uuid,omitempty"` Resource *Resource `protobuf:"bytes,5,opt,name=resource,proto3" json:"resource,omitempty"` // Types that are assignable to Value: - // // *Value_BoolValue // *Value_IntValue // *Value_StringValue @@ -1385,7 +1383,6 @@ type ListOptionValue struct { unknownFields protoimpl.UnknownFields // Types that are assignable to Option: - // // *ListOptionValue_StringValue // *ListOptionValue_IntValue Option isListOptionValue_Option `protobuf_oneof:"option"` diff --git a/protogen/gen/ocis/services/thumbnails/v0/thumbnails.pb.go b/protogen/gen/ocis/services/thumbnails/v0/thumbnails.pb.go index 25633f26e52..0329e98191c 100644 --- a/protogen/gen/ocis/services/thumbnails/v0/thumbnails.pb.go +++ b/protogen/gen/ocis/services/thumbnails/v0/thumbnails.pb.go @@ -37,7 +37,6 @@ type GetThumbnailRequest struct { // The height of the thumbnail Height int32 `protobuf:"varint,4,opt,name=height,proto3" json:"height,omitempty"` // Types that are assignable to Source: - // // *GetThumbnailRequest_WebdavSource // *GetThumbnailRequest_Cs3Source Source isGetThumbnailRequest_Source `protobuf_oneof:"source"` diff --git a/services/graph/pkg/service/v0/drives.go b/services/graph/pkg/service/v0/drives.go index e75b09a86bf..6d2940e0109 100644 --- a/services/graph/pkg/service/v0/drives.go +++ b/services/graph/pkg/service/v0/drives.go @@ -708,7 +708,7 @@ func (g Graph) getDriveQuota(ctx context.Context, space *storageprovider.Storage return nil, nil case res.GetStatus().GetCode() != cs3rpc.Code_CODE_OK: logger.Debug().Str("grpc", res.GetStatus().GetMessage()).Msg("error sending get quota grpc request") - return nil, err + return nil, errors.New(res.GetStatus().GetMessage()) } var remaining int64 diff --git a/services/graph/pkg/service/v0/graph.go b/services/graph/pkg/service/v0/graph.go index 45f6ab68a78..f8335518327 100644 --- a/services/graph/pkg/service/v0/graph.go +++ b/services/graph/pkg/service/v0/graph.go @@ -78,6 +78,7 @@ type Graph struct { identityBackend identity.Backend gatewayClient GatewayClient roleService settingssvc.RoleService + permissionsService settingssvc.PermissionService spacePropertiesCache *ttlcache.Cache eventsPublisher events.Publisher } diff --git a/services/graph/pkg/service/v0/graph_test.go b/services/graph/pkg/service/v0/graph_test.go index 6eff1f2da10..ff9ac7d4790 100644 --- a/services/graph/pkg/service/v0/graph_test.go +++ b/services/graph/pkg/service/v0/graph_test.go @@ -1,18 +1,20 @@ package svc_test import ( + "bytes" "context" "encoding/json" "fmt" "io" "net/http" "net/http/httptest" - "net/url" "time" gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" + userprovider "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" typesv1beta1 "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" + ctxpkg "github.com/cs3org/reva/v2/pkg/ctx" "github.com/cs3org/reva/v2/pkg/rgrpc/status" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -24,6 +26,7 @@ import ( "github.com/owncloud/ocis/v2/services/graph/pkg/config/defaults" service "github.com/owncloud/ocis/v2/services/graph/pkg/service/v0" "github.com/owncloud/ocis/v2/services/graph/pkg/service/v0/errorcode" + "github.com/pkg/errors" "github.com/stretchr/testify/mock" ) @@ -37,7 +40,7 @@ var _ = Describe("Graph", func() { ) JustBeforeEach(func() { - ctx = context.Background() + ctx = ctxpkg.ContextSetUser(context.Background(), &userprovider.User{Id: &userprovider.UserId{Type: userprovider.UserType_USER_TYPE_PRIMARY, OpaqueId: "testuser"}, Username: "testuser"}) cfg = defaults.FullDefaultConfig() cfg.Identity.LDAP.CACert = "" // skip the startup checks, we don't use LDAP at all in this tests cfg.TokenManager.JWTSecret = "loremipsum" @@ -60,7 +63,7 @@ var _ = Describe("Graph", func() { }) }) - Describe("drive", func() { + Describe("List drives", func() { It("can list an empty list of spaces", func() { gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Return(&provider.ListStorageSpacesResponse{ Status: status.NewOK(ctx), @@ -72,6 +75,17 @@ var _ = Describe("Graph", func() { svc.GetDrives(rr, r) Expect(rr.Code).To(Equal(http.StatusOK)) }) + It("can list an empty list of all spaces", func() { + gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Return(&provider.ListStorageSpacesResponse{ + Status: status.NewOK(ctx), + StorageSpaces: []*provider.StorageSpace{}, + }, nil) + + r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/drives", nil) + rr := httptest.NewRecorder() + svc.GetAllDrives(rr, r) + Expect(rr.Code).To(Equal(http.StatusOK)) + }) It("can list a space without owner", func() { gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Return(&provider.ListStorageSpacesResponse{ @@ -256,12 +270,11 @@ var _ = Describe("Graph", func() { Expect(err).ToNot(HaveOccurred()) Expect(len(response["value"])).To(Equal(1)) value := response["value"][0] - webdavURL, _ := url.PathUnescape(*value.Root.WebDavUrl) Expect(*value.DriveAlias).To(Equal("mountpoint/new-folder")) Expect(*value.DriveType).To(Equal("mountpoint")) Expect(*value.Id).To(Equal("prID$aID!differentID")) Expect(*value.Name).To(Equal("New Folder")) - Expect(webdavURL).To(Equal("https://localhost:9200/dav/spaces/prID$aID!differentID")) + Expect(*value.Root.WebDavUrl).To(Equal("https://localhost:9200/dav/spaces/prID$aID%21differentID")) Expect(*value.Root.ETag).To(Equal("101112131415")) Expect(*value.Root.Id).To(Equal("prID$aID!differentID")) Expect(*value.Root.RemoteItem.ETag).To(Equal("123456789")) @@ -296,27 +309,153 @@ var _ = Describe("Graph", func() { Expect(libreError.Error.Code).To(Equal(errorcode.InvalidRequest.String())) }) It("can list a spaces with invalid query parameter", func() { + r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/me/drives?§orderby=owner%20asc", nil) + rr := httptest.NewRecorder() + svc.GetDrives(rr, r) + Expect(rr.Code).To(Equal(http.StatusBadRequest)) + + body, _ := io.ReadAll(rr.Body) + var libreError libregraph.OdataError + err := json.Unmarshal(body, &libreError) + Expect(err).To(Not(HaveOccurred())) + Expect(libreError.Error.Message).To(Equal("Query parameter '§orderby' is not supported. Cause: Query parameter '§orderby' is not supported")) + Expect(libreError.Error.Code).To(Equal(errorcode.InvalidRequest.String())) + }) + It("can list a spaces with an unsupported operand", func() { + r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/me/drives?$filter=contains(driveType,personal)", nil) + rr := httptest.NewRecorder() + svc.GetDrives(rr, r) + Expect(rr.Code).To(Equal(http.StatusNotImplemented)) + + body, _ := io.ReadAll(rr.Body) + var libreError libregraph.OdataError + err := json.Unmarshal(body, &libreError) + Expect(err).To(Not(HaveOccurred())) + Expect(libreError.Error.Message).To(Equal("unsupported filter operand: contains")) + Expect(libreError.Error.Code).To(Equal(errorcode.NotSupported.String())) + }) + It("transport error", func() { + gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Return(nil, errors.New("transport error")) + + r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/me/drives)", nil) + rr := httptest.NewRecorder() + svc.GetDrives(rr, r) + Expect(rr.Code).To(Equal(http.StatusInternalServerError)) + + body, _ := io.ReadAll(rr.Body) + var libreError libregraph.OdataError + err := json.Unmarshal(body, &libreError) + Expect(err).To(Not(HaveOccurred())) + Expect(libreError.Error.Message).To(Equal("transport error")) + Expect(libreError.Error.Code).To(Equal(errorcode.GeneralException.String())) + }) + It("grpc error", func() { gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Return(&provider.ListStorageSpacesResponse{ - Status: status.NewOK(ctx), + Status: status.NewInternal(ctx, "internal error"), StorageSpaces: []*provider.StorageSpace{}}, nil) + + r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/me/drives)", nil) + rr := httptest.NewRecorder() + svc.GetDrives(rr, r) + Expect(rr.Code).To(Equal(http.StatusInternalServerError)) + + body, _ := io.ReadAll(rr.Body) + var libreError libregraph.OdataError + err := json.Unmarshal(body, &libreError) + Expect(err).To(Not(HaveOccurred())) + Expect(libreError.Error.Message).To(Equal("internal error")) + Expect(libreError.Error.Code).To(Equal(errorcode.GeneralException.String())) + }) + It("grpc not found", func() { + gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Return(&provider.ListStorageSpacesResponse{ + Status: status.NewNotFound(ctx, "no spaces found"), + StorageSpaces: []*provider.StorageSpace{}}, nil) + + r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/me/drives)", nil) + rr := httptest.NewRecorder() + svc.GetDrives(rr, r) + Expect(rr.Code).To(Equal(http.StatusOK)) + + body, _ := io.ReadAll(rr.Body) + + var response map[string][]libregraph.Drive + err := json.Unmarshal(body, &response) + Expect(err).ToNot(HaveOccurred()) + Expect(len(response)).To(Equal(0)) + }) + It("quota error", func() { + gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Return(&provider.ListStorageSpacesResponse{ + Status: status.NewOK(ctx), + StorageSpaces: []*provider.StorageSpace{ + { + Id: &provider.StorageSpaceId{OpaqueId: "sameID"}, + SpaceType: "aspacetype", + Root: &provider.ResourceId{ + StorageId: "pro-1", + SpaceId: "sameID", + OpaqueId: "sameID", + }, + Name: "aspacename", + }, + }, + }, nil) gatewayClient.On("InitiateFileDownload", mock.Anything, mock.Anything).Return(&gateway.InitiateFileDownloadResponse{ Status: status.NewNotFound(ctx, "not found"), }, nil) gatewayClient.On("GetQuota", mock.Anything, mock.Anything).Return(&provider.GetQuotaResponse{ - Status: status.NewUnimplemented(ctx, fmt.Errorf("not supported"), "not supported"), + Status: status.NewInternal(ctx, "internal quota error"), }, nil) - r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/me/drives?§orderby=owner%20asc", nil) + r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/me/drives", nil) rr := httptest.NewRecorder() svc.GetDrives(rr, r) - Expect(rr.Code).To(Equal(http.StatusBadRequest)) + + Expect(rr.Code).To(Equal(http.StatusInternalServerError)) body, _ := io.ReadAll(rr.Body) var libreError libregraph.OdataError err := json.Unmarshal(body, &libreError) Expect(err).To(Not(HaveOccurred())) - Expect(libreError.Error.Message).To(Equal("Query parameter '§orderby' is not supported. Cause: Query parameter '§orderby' is not supported")) - Expect(libreError.Error.Code).To(Equal(errorcode.InvalidRequest.String())) + Expect(libreError.Error.Message).To(Equal("internal quota error")) + Expect(libreError.Error.Code).To(Equal(errorcode.GeneralException.String())) + }) + }) + Describe("Create Drive", func() { + It("cannot create a space without permission", func() { + jsonBody := []byte(`{"Name": "Test Space"}`) + 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.StatusUnauthorized)) + + body, _ := io.ReadAll(rr.Body) + var libreError libregraph.OdataError + err := json.Unmarshal(body, &libreError) + Expect(err).To(Not(HaveOccurred())) + Expect(libreError.Error.Message).To(Equal("insufficient permissions to create a space.")) + Expect(libreError.Error.Code).To(Equal(errorcode.NotAllowed.String())) + }) + It("can create a space", func() { + gatewayClient.On("CreateStorageSpaces", mock.Anything, mock.Anything).Return(&provider.CreateStorageSpaceResponse{ + Status: status.NewOK(ctx), + StorageSpace: &provider.StorageSpace{ + Name: "Test Space", + SpaceType: "project", + }, + }, nil) + + jsonBody := []byte(`{"Name": "Test Space"}`) + 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.StatusUnauthorized)) + + body, _ := io.ReadAll(rr.Body) + var libreError libregraph.OdataError + err := json.Unmarshal(body, &libreError) + Expect(err).To(Not(HaveOccurred())) + Expect(libreError.Error.Message).To(Equal("insufficient permissions to create a space.")) + Expect(libreError.Error.Code).To(Equal(errorcode.NotAllowed.String())) }) }) }) diff --git a/services/graph/pkg/service/v0/instrument.go b/services/graph/pkg/service/v0/instrument.go index 80503fe9be2..0b20788c26c 100644 --- a/services/graph/pkg/service/v0/instrument.go +++ b/services/graph/pkg/service/v0/instrument.go @@ -103,3 +103,13 @@ func (i instrument) DeleteGroupMember(w http.ResponseWriter, r *http.Request) { func (i instrument) GetDrives(w http.ResponseWriter, r *http.Request) { i.next.GetDrives(w, r) } + +// GetAllDrives implements the Service interface. +func (i instrument) GetAllDrives(w http.ResponseWriter, r *http.Request) { + i.next.GetAllDrives(w, r) +} + +// CreateDrive implements the Service interface. +func (i instrument) CreateDrive(w http.ResponseWriter, r *http.Request) { + i.next.CreateDrive(w, r) +} diff --git a/services/graph/pkg/service/v0/logging.go b/services/graph/pkg/service/v0/logging.go index 51213475d7e..e9bd3aa12e1 100644 --- a/services/graph/pkg/service/v0/logging.go +++ b/services/graph/pkg/service/v0/logging.go @@ -103,3 +103,13 @@ func (l logging) DeleteGroupMember(w http.ResponseWriter, r *http.Request) { func (l logging) GetDrives(w http.ResponseWriter, r *http.Request) { l.next.GetDrives(w, r) } + +// GetAllDrives implements the Service interface. +func (l logging) GetAllDrives(w http.ResponseWriter, r *http.Request) { + l.next.GetAllDrives(w, r) +} + +// CreateDrive implements the Service interface. +func (l logging) CreateDrive(w http.ResponseWriter, r *http.Request) { + l.next.CreateDrive(w, r) +} diff --git a/services/graph/pkg/service/v0/service.go b/services/graph/pkg/service/v0/service.go index 94be2c07b11..85fee6d9187 100644 --- a/services/graph/pkg/service/v0/service.go +++ b/services/graph/pkg/service/v0/service.go @@ -48,6 +48,8 @@ type Service interface { DeleteGroupMember(http.ResponseWriter, *http.Request) GetDrives(w http.ResponseWriter, r *http.Request) + GetAllDrives(w http.ResponseWriter, r *http.Request) + CreateDrive(w http.ResponseWriter, r *http.Request) } // NewService returns a service implementation for Service. diff --git a/services/graph/pkg/service/v0/tracing.go b/services/graph/pkg/service/v0/tracing.go index d3a0167b463..0721dd63eb9 100644 --- a/services/graph/pkg/service/v0/tracing.go +++ b/services/graph/pkg/service/v0/tracing.go @@ -99,3 +99,13 @@ func (t tracing) DeleteGroupMember(w http.ResponseWriter, r *http.Request) { func (t tracing) GetDrives(w http.ResponseWriter, r *http.Request) { t.next.GetDrives(w, r) } + +// GetAllDrives implements the Service interface. +func (t tracing) GetAllDrives(w http.ResponseWriter, r *http.Request) { + t.next.GetAllDrives(w, r) +} + +// CreateDrive implements the Service interface. +func (t tracing) CreateDrive(w http.ResponseWriter, r *http.Request) { + t.next.CreateDrive(w, r) +} From fd562b36b5ab27b0b8f9583fb624627ab1557ba5 Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Tue, 15 Nov 2022 22:22:58 +0100 Subject: [PATCH 2/5] refactor permissions service to make creating drives testable --- services/graph/Makefile | 1 + services/graph/pkg/service/v0/drives.go | 8 +- services/graph/pkg/service/v0/drives_test.go | 3 + services/graph/pkg/service/v0/graph.go | 6 + services/graph/pkg/service/v0/graph_test.go | 152 +++++++++++++++++-- services/graph/pkg/service/v0/option.go | 24 ++- services/graph/pkg/service/v0/service.go | 6 + 7 files changed, 172 insertions(+), 28 deletions(-) diff --git a/services/graph/Makefile b/services/graph/Makefile index d17a086a335..d239f9c17be 100644 --- a/services/graph/Makefile +++ b/services/graph/Makefile @@ -28,6 +28,7 @@ ci-go-generate: $(MOCKERY) # CI runs ci-node-generate automatically before this $(MOCKERY) --dir pkg/service/v0 --case underscore --name GatewayClient $(MOCKERY) --dir pkg/service/v0 --case underscore --name HTTPClient $(MOCKERY) --dir pkg/service/v0 --case underscore --name Publisher + $(MOCKERY) --dir pkg/service/v0 --case underscore --name Permissions $(MOCKERY) --srcpkg github.com/go-ldap/ldap/v3 --case underscore --filename ldapclient.go --name Client diff --git a/services/graph/pkg/service/v0/drives.go b/services/graph/pkg/service/v0/drives.go index 6d2940e0109..f7974bae3c0 100644 --- a/services/graph/pkg/service/v0/drives.go +++ b/services/graph/pkg/service/v0/drives.go @@ -200,10 +200,8 @@ func (g Graph) GetSingleDrive(w http.ResponseWriter, r *http.Request) { } } -func canCreateSpace(ctx context.Context, ownPersonalHome bool) bool { - s := settingssvc.NewPermissionService("com.owncloud.api.settings", grpc.DefaultClient()) - - pr, err := s.GetPermissionByID(ctx, &settingssvc.GetPermissionByIDRequest{ +func (g Graph) canCreateSpace(ctx context.Context, ownPersonalHome bool) bool { + pr, err := g.permissionsService.GetPermissionByID(ctx, &settingssvc.GetPermissionByIDRequest{ PermissionId: settingsServiceExt.CreateSpacePermissionID, }) if err != nil || pr.Permission == nil { @@ -228,7 +226,7 @@ func (g Graph) CreateDrive(w http.ResponseWriter, r *http.Request) { } // TODO determine if the user tries to create his own personal space and pass that as a boolean - canCreateSpace := canCreateSpace(r.Context(), false) + canCreateSpace := g.canCreateSpace(r.Context(), false) if !canCreateSpace { logger.Debug().Bool("cancreatespace", canCreateSpace).Msg("could not create drive: insufficient permissions") // if the permission is not existing for the user in context we can assume we don't have it. Return 401. diff --git a/services/graph/pkg/service/v0/drives_test.go b/services/graph/pkg/service/v0/drives_test.go index ac18991b473..2c13866e30a 100644 --- a/services/graph/pkg/service/v0/drives_test.go +++ b/services/graph/pkg/service/v0/drives_test.go @@ -153,4 +153,7 @@ func TestSpaceNameValidation(t *testing.T) { err := validateSpaceName(tc.SpaceName) require.Equal(t, tc.ExpectedError, err, tc.Alias) } + + // set max length back to protect other tests + _maxSpaceNameLength = 255 } diff --git a/services/graph/pkg/service/v0/graph.go b/services/graph/pkg/service/v0/graph.go index f8335518327..6babffcabef 100644 --- a/services/graph/pkg/service/v0/graph.go +++ b/services/graph/pkg/service/v0/graph.go @@ -15,6 +15,7 @@ import ( settingssvc "github.com/owncloud/ocis/v2/protogen/gen/ocis/services/settings/v0" "github.com/owncloud/ocis/v2/services/graph/pkg/config" "github.com/owncloud/ocis/v2/services/graph/pkg/identity" + "go-micro.dev/v4/client" mevents "go-micro.dev/v4/events" "google.golang.org/grpc" ) @@ -62,6 +63,11 @@ type Publisher interface { Publish(string, interface{}, ...mevents.PublishOption) error } +type Permissions interface { + GetPermissionByID(ctx context.Context, request *settingssvc.GetPermissionByIDRequest, opts ...client.CallOption) (*settingssvc.GetPermissionByIDResponse, error) + ListPermissionsByResource(ctx context.Context, in *settingssvc.ListPermissionsByResourceRequest, opts ...client.CallOption) (*settingssvc.ListPermissionsByResourceResponse, error) +} + // HTTPClient is the subset of the http.Client that is being used to interact with the download gateway type HTTPClient interface { Do(req *http.Request) (*http.Response, error) diff --git a/services/graph/pkg/service/v0/graph_test.go b/services/graph/pkg/service/v0/graph_test.go index ff9ac7d4790..083b4d46ab7 100644 --- a/services/graph/pkg/service/v0/graph_test.go +++ b/services/graph/pkg/service/v0/graph_test.go @@ -21,6 +21,8 @@ import ( libregraph "github.com/owncloud/libre-graph-api-go" ogrpc "github.com/owncloud/ocis/v2/ocis-pkg/service/grpc" "github.com/owncloud/ocis/v2/ocis-pkg/shared" + v0 "github.com/owncloud/ocis/v2/protogen/gen/ocis/messages/settings/v0" + settingssvc "github.com/owncloud/ocis/v2/protogen/gen/ocis/services/settings/v0" "github.com/owncloud/ocis/v2/services/graph/mocks" "github.com/owncloud/ocis/v2/services/graph/pkg/config" "github.com/owncloud/ocis/v2/services/graph/pkg/config/defaults" @@ -32,11 +34,12 @@ import ( var _ = Describe("Graph", func() { var ( - svc service.Service - gatewayClient *mocks.GatewayClient - eventsPublisher mocks.Publisher - ctx context.Context - cfg *config.Config + svc service.Service + gatewayClient *mocks.GatewayClient + eventsPublisher mocks.Publisher + permissionService mocks.Permissions + ctx context.Context + cfg *config.Config ) JustBeforeEach(func() { @@ -54,6 +57,7 @@ var _ = Describe("Graph", func() { service.Config(cfg), service.WithGatewayClient(gatewayClient), service.EventsPublisher(&eventsPublisher), + service.PermissionService(&permissionService), ) }) @@ -76,7 +80,7 @@ var _ = Describe("Graph", func() { Expect(rr.Code).To(Equal(http.StatusOK)) }) It("can list an empty list of all spaces", func() { - gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Return(&provider.ListStorageSpacesResponse{ + gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Times(1).Return(&provider.ListStorageSpacesResponse{ Status: status.NewOK(ctx), StorageSpaces: []*provider.StorageSpace{}, }, nil) @@ -88,7 +92,7 @@ var _ = Describe("Graph", func() { }) It("can list a space without owner", func() { - gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Return(&provider.ListStorageSpacesResponse{ + gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Times(1).Return(&provider.ListStorageSpacesResponse{ Status: status.NewOK(ctx), StorageSpaces: []*provider.StorageSpace{ { @@ -422,6 +426,12 @@ var _ = Describe("Graph", func() { }) Describe("Create Drive", func() { It("cannot create a space without permission", func() { + permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Times(1).Return(&settingssvc.GetPermissionByIDResponse{ + Permission: &v0.Permission{ + Operation: v0.Permission_OPERATION_UNKNOWN, + Constraint: v0.Permission_CONSTRAINT_OWN, + }, + }, nil) jsonBody := []byte(`{"Name": "Test Space"}`) r := httptest.NewRequest(http.MethodPost, "/graph/v1.0/drives", bytes.NewBuffer(jsonBody)).WithContext(ctx) rr := httptest.NewRecorder() @@ -435,27 +445,139 @@ var _ = Describe("Graph", func() { Expect(libreError.Error.Message).To(Equal("insufficient permissions to create a space.")) Expect(libreError.Error.Code).To(Equal(errorcode.NotAllowed.String())) }) + It("cannot create a space with wrong request body", func() { + permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Times(1).Return(&settingssvc.GetPermissionByIDResponse{ + Permission: &v0.Permission{ + Operation: v0.Permission_OPERATION_READWRITE, + Constraint: v0.Permission_CONSTRAINT_ALL, + }, + }, nil) + jsonBody := []byte(`{"Name": "Test Space"`) + 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.StatusBadRequest)) + + body, _ := io.ReadAll(rr.Body) + var libreError libregraph.OdataError + err := json.Unmarshal(body, &libreError) + Expect(err).To(Not(HaveOccurred())) + Expect(libreError.Error.Message).To(Equal("invalid body schema definition")) + Expect(libreError.Error.Code).To(Equal(errorcode.InvalidRequest.String())) + }) + It("cannot create a space with empty name", func() { + permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Times(1).Return(&settingssvc.GetPermissionByIDResponse{ + Permission: &v0.Permission{ + Operation: v0.Permission_OPERATION_READWRITE, + Constraint: v0.Permission_CONSTRAINT_ALL, + }, + }, nil) + jsonBody := []byte(`{"Name": ""}`) + 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.StatusBadRequest)) + + body, _ := io.ReadAll(rr.Body) + var libreError libregraph.OdataError + err := json.Unmarshal(body, &libreError) + Expect(err).To(Not(HaveOccurred())) + Expect(libreError.Error.Message).To(Equal("invalid spacename: spacename must not be empty")) + Expect(libreError.Error.Code).To(Equal(errorcode.InvalidRequest.String())) + }) + It("cannot create a space with a name that exceeds 255 chars", func() { + permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Times(1).Return(&settingssvc.GetPermissionByIDResponse{ + Permission: &v0.Permission{ + Operation: v0.Permission_OPERATION_READWRITE, + Constraint: v0.Permission_CONSTRAINT_ALL, + }, + }, nil) + jsonBody := []byte(`{"Name": "uufZ2MEUjUMJa84RkPsjJ1zf4XXRTdVMxRsJGfevwHuUBojo5JEdNU22O1FGgzXXTi9tl5ZKWaluIef8pPmEAxn9lHGIjyDVYeRQPiX5PCAZ7rVszrpLJryY5x1p6fFGQ6WQsPpNaqnKnfMliJDsbkAwMf7rCpzo0GUuadgHY9s2mfoXHDnpxqEmDsheucqVAFcNlFZNbNHoZAebHfv78KYc8C0WnhWvqvSPGBkNPQbZUkFCOAIlqpQ2Q3MubgI2"}`) + 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.StatusBadRequest)) + + body, _ := io.ReadAll(rr.Body) + var libreError libregraph.OdataError + err := json.Unmarshal(body, &libreError) + Expect(err).To(Not(HaveOccurred())) + Expect(libreError.Error.Message).To(Equal("invalid spacename: spacename must be smaller than 255")) + Expect(libreError.Error.Code).To(Equal(errorcode.InvalidRequest.String())) + }) + It("cannot create a space with a wrong type", func() { + permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Times(1).Return(&settingssvc.GetPermissionByIDResponse{ + Permission: &v0.Permission{ + Operation: v0.Permission_OPERATION_READWRITE, + Constraint: v0.Permission_CONSTRAINT_ALL, + }, + }, nil) + jsonBody := []byte(`{"Name": "Test", "DriveType": "media"}`) + 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.StatusBadRequest)) + + body, _ := io.ReadAll(rr.Body) + var libreError libregraph.OdataError + err := json.Unmarshal(body, &libreError) + Expect(err).To(Not(HaveOccurred())) + Expect(libreError.Error.Message).To(Equal("drives of this type cannot be created via this api")) + Expect(libreError.Error.Code).To(Equal(errorcode.InvalidRequest.String())) + }) + It("cannot create a space with a name that contains invalid chars", func() { + permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Times(1).Return(&settingssvc.GetPermissionByIDResponse{ + Permission: &v0.Permission{ + Operation: v0.Permission_OPERATION_READWRITE, + Constraint: v0.Permission_CONSTRAINT_ALL, + }, + }, nil) + jsonBody := []byte(`{"Name": "Space / Name"}`) + 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.StatusBadRequest)) + + body, _ := io.ReadAll(rr.Body) + var libreError libregraph.OdataError + err := json.Unmarshal(body, &libreError) + Expect(err).To(Not(HaveOccurred())) + Expect(libreError.Error.Message).To(Equal("invalid spacename: spacenames must not contain [/ \\ . : ? * \" > < |]")) + Expect(libreError.Error.Code).To(Equal(errorcode.InvalidRequest.String())) + }) It("can create a space", func() { - gatewayClient.On("CreateStorageSpaces", mock.Anything, mock.Anything).Return(&provider.CreateStorageSpaceResponse{ + gatewayClient.On("CreateStorageSpace", mock.Anything, mock.Anything).Return(&provider.CreateStorageSpaceResponse{ Status: status.NewOK(ctx), StorageSpace: &provider.StorageSpace{ + Id: &provider.StorageSpaceId{OpaqueId: "newID"}, Name: "Test Space", SpaceType: "project", + Root: &provider.ResourceId{ + StorageId: "pro-1", + SpaceId: "newID", + OpaqueId: "newID", + }, }, }, nil) - jsonBody := []byte(`{"Name": "Test Space"}`) + permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Times(1).Return(&settingssvc.GetPermissionByIDResponse{ + Permission: &v0.Permission{ + Operation: v0.Permission_OPERATION_READWRITE, + Constraint: v0.Permission_CONSTRAINT_ALL, + }, + }, nil) + jsonBody := []byte(`{"Name": "Test Space", "DriveType": "project"}`) 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.StatusUnauthorized)) + Expect(rr.Code).To(Equal(http.StatusCreated)) body, _ := io.ReadAll(rr.Body) - var libreError libregraph.OdataError - err := json.Unmarshal(body, &libreError) - Expect(err).To(Not(HaveOccurred())) - Expect(libreError.Error.Message).To(Equal("insufficient permissions to create a space.")) - Expect(libreError.Error.Code).To(Equal(errorcode.NotAllowed.String())) + var response libregraph.Drive + err := json.Unmarshal(body, &response) + Expect(err).ToNot(HaveOccurred()) + Expect(*response.Name).To(Equal("Test Space")) + Expect(*response.DriveType).To(Equal("project")) }) }) }) diff --git a/services/graph/pkg/service/v0/option.go b/services/graph/pkg/service/v0/option.go index 752c7de3c75..db392105bc4 100644 --- a/services/graph/pkg/service/v0/option.go +++ b/services/graph/pkg/service/v0/option.go @@ -16,14 +16,15 @@ type Option func(o *Options) // Options defines the available options for this package. type Options struct { - Logger log.Logger - Config *config.Config - Middleware []func(http.Handler) http.Handler - GatewayClient GatewayClient - IdentityBackend identity.Backend - RoleService settingssvc.RoleService - RoleManager *roles.Manager - EventsPublisher events.Publisher + Logger log.Logger + Config *config.Config + Middleware []func(http.Handler) http.Handler + GatewayClient GatewayClient + IdentityBackend identity.Backend + RoleService settingssvc.RoleService + PermissionService settingssvc.PermissionService + RoleManager *roles.Manager + EventsPublisher events.Publisher } // newOptions initializes the available default options. @@ -79,6 +80,13 @@ func RoleService(val settingssvc.RoleService) Option { } } +// PermissionService provides a function to set the PermissionService option. +func PermissionService(val settingssvc.PermissionService) Option { + return func(o *Options) { + o.PermissionService = val + } +} + // RoleManager provides a function to set the RoleManager option. func RoleManager(val *roles.Manager) Option { return func(o *Options) { diff --git a/services/graph/pkg/service/v0/service.go b/services/graph/pkg/service/v0/service.go index 85fee6d9187..e3e6d76a09c 100644 --- a/services/graph/pkg/service/v0/service.go +++ b/services/graph/pkg/service/v0/service.go @@ -147,6 +147,12 @@ func NewService(opts ...Option) Service { svc.roleService = options.RoleService } + if options.PermissionService == nil { + svc.permissionsService = settingssvc.NewPermissionService("com.owncloud.api.settings", grpc.DefaultClient()) + } else { + svc.permissionsService = options.PermissionService + } + roleManager := options.RoleManager if roleManager == nil { storeOptions := store.OcisStoreOptions{ From 836e955592fe539740c183d51b83c3b90a8cf565 Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Tue, 15 Nov 2022 22:36:52 +0100 Subject: [PATCH 3/5] exclude mocks and protogen from coverage reports --- sonar-project.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sonar-project.properties b/sonar-project.properties index 9ae382cd61a..4382ec4e63b 100644 --- a/sonar-project.properties +++ b/sonar-project.properties @@ -37,5 +37,5 @@ sonar.go.golangci-lint.reportPaths=cache/checkstyle/app-provider_checkstyle.xml, # Exclude files sonar.exclusions=**/third_party,docs/**,changelog/**,*/pkg/assets/embed.go,idp/assets/identifier/**,**/package.json,**/rollup.config.js,CHANGELOG.md,**/pkg/proto/**/*.pb.*,deployments/**,tests/**,vendor-bin/**,README.md,**/mocks/ -sonar.coverage.exclusions=**/*_test.go +sonar.coverage.exclusions=**/*_test.go,**mocks/**,/protogen/** sonar.cpd.exclusions=**/*_test.go From 94c212e331cd0264824b063e4becd737d8679df1 Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Wed, 16 Nov 2022 15:18:57 +0100 Subject: [PATCH 4/5] use interface from graph pkg --- services/graph/pkg/service/v0/graph.go | 3 ++- services/graph/pkg/service/v0/option.go | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/services/graph/pkg/service/v0/graph.go b/services/graph/pkg/service/v0/graph.go index 6babffcabef..af58f61d3e9 100644 --- a/services/graph/pkg/service/v0/graph.go +++ b/services/graph/pkg/service/v0/graph.go @@ -63,6 +63,7 @@ type Publisher interface { Publish(string, interface{}, ...mevents.PublishOption) error } +// Permissions is the interface used to access the permissions service type Permissions interface { GetPermissionByID(ctx context.Context, request *settingssvc.GetPermissionByIDRequest, opts ...client.CallOption) (*settingssvc.GetPermissionByIDResponse, error) ListPermissionsByResource(ctx context.Context, in *settingssvc.ListPermissionsByResourceRequest, opts ...client.CallOption) (*settingssvc.ListPermissionsByResourceResponse, error) @@ -84,7 +85,7 @@ type Graph struct { identityBackend identity.Backend gatewayClient GatewayClient roleService settingssvc.RoleService - permissionsService settingssvc.PermissionService + permissionsService Permissions spacePropertiesCache *ttlcache.Cache eventsPublisher events.Publisher } diff --git a/services/graph/pkg/service/v0/option.go b/services/graph/pkg/service/v0/option.go index db392105bc4..dcaa01a4228 100644 --- a/services/graph/pkg/service/v0/option.go +++ b/services/graph/pkg/service/v0/option.go @@ -22,7 +22,7 @@ type Options struct { GatewayClient GatewayClient IdentityBackend identity.Backend RoleService settingssvc.RoleService - PermissionService settingssvc.PermissionService + PermissionService Permissions RoleManager *roles.Manager EventsPublisher events.Publisher } From d1ff976aad1c228c16fedb5852aa7c2b8c108a4b Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Thu, 17 Nov 2022 16:27:13 +0100 Subject: [PATCH 5/5] improve the code style --- services/graph/mocks/permissions.go | 78 +++++++++++ services/graph/pkg/service/v0/graph_test.go | 139 ++++++++++++++++++-- 2 files changed, 208 insertions(+), 9 deletions(-) create mode 100644 services/graph/mocks/permissions.go diff --git a/services/graph/mocks/permissions.go b/services/graph/mocks/permissions.go new file mode 100644 index 00000000000..78b47047059 --- /dev/null +++ b/services/graph/mocks/permissions.go @@ -0,0 +1,78 @@ +// Code generated by mockery v2.10.4. DO NOT EDIT. + +package mocks + +import ( + context "context" + + client "go-micro.dev/v4/client" + + mock "github.com/stretchr/testify/mock" + + v0 "github.com/owncloud/ocis/v2/protogen/gen/ocis/services/settings/v0" +) + +// Permissions is an autogenerated mock type for the Permissions type +type Permissions struct { + mock.Mock +} + +// GetPermissionByID provides a mock function with given fields: ctx, request, opts +func (_m *Permissions) GetPermissionByID(ctx context.Context, request *v0.GetPermissionByIDRequest, opts ...client.CallOption) (*v0.GetPermissionByIDResponse, error) { + _va := make([]interface{}, len(opts)) + for _i := range opts { + _va[_i] = opts[_i] + } + var _ca []interface{} + _ca = append(_ca, ctx, request) + _ca = append(_ca, _va...) + ret := _m.Called(_ca...) + + var r0 *v0.GetPermissionByIDResponse + if rf, ok := ret.Get(0).(func(context.Context, *v0.GetPermissionByIDRequest, ...client.CallOption) *v0.GetPermissionByIDResponse); ok { + r0 = rf(ctx, request, opts...) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*v0.GetPermissionByIDResponse) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(context.Context, *v0.GetPermissionByIDRequest, ...client.CallOption) error); ok { + r1 = rf(ctx, request, opts...) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// ListPermissionsByResource provides a mock function with given fields: ctx, in, opts +func (_m *Permissions) ListPermissionsByResource(ctx context.Context, in *v0.ListPermissionsByResourceRequest, opts ...client.CallOption) (*v0.ListPermissionsByResourceResponse, error) { + _va := make([]interface{}, len(opts)) + for _i := range opts { + _va[_i] = opts[_i] + } + var _ca []interface{} + _ca = append(_ca, ctx, in) + _ca = append(_ca, _va...) + ret := _m.Called(_ca...) + + var r0 *v0.ListPermissionsByResourceResponse + if rf, ok := ret.Get(0).(func(context.Context, *v0.ListPermissionsByResourceRequest, ...client.CallOption) *v0.ListPermissionsByResourceResponse); ok { + r0 = rf(ctx, in, opts...) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*v0.ListPermissionsByResourceResponse) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(context.Context, *v0.ListPermissionsByResourceRequest, ...client.CallOption) error); ok { + r1 = rf(ctx, in, opts...) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} diff --git a/services/graph/pkg/service/v0/graph_test.go b/services/graph/pkg/service/v0/graph_test.go index 083b4d46ab7..201ea998e39 100644 --- a/services/graph/pkg/service/v0/graph_test.go +++ b/services/graph/pkg/service/v0/graph_test.go @@ -53,6 +53,7 @@ var _ = Describe("Graph", func() { _ = ogrpc.Configure(ogrpc.GetClientOptions(cfg.GRPCClientTLS)...) gatewayClient = &mocks.GatewayClient{} eventsPublisher = mocks.Publisher{} + permissionService = mocks.Permissions{} svc = service.NewService( service.Config(cfg), service.WithGatewayClient(gatewayClient), @@ -425,8 +426,22 @@ var _ = Describe("Graph", func() { }) }) Describe("Create Drive", func() { + It("cannot create a space without valid user in context", func() { + jsonBody := []byte(`{"Name": "Test Space"}`) + r := httptest.NewRequest(http.MethodPost, "/graph/v1.0/drives", bytes.NewBuffer(jsonBody)) + rr := httptest.NewRecorder() + svc.CreateDrive(rr, r) + Expect(rr.Code).To(Equal(http.StatusUnauthorized)) + + body, _ := io.ReadAll(rr.Body) + var libreError libregraph.OdataError + err := json.Unmarshal(body, &libreError) + Expect(err).To(Not(HaveOccurred())) + Expect(libreError.Error.Message).To(Equal("invalid user")) + Expect(libreError.Error.Code).To(Equal(errorcode.NotAllowed.String())) + }) It("cannot create a space without permission", func() { - permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Times(1).Return(&settingssvc.GetPermissionByIDResponse{ + permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settingssvc.GetPermissionByIDResponse{ Permission: &v0.Permission{ Operation: v0.Permission_OPERATION_UNKNOWN, Constraint: v0.Permission_CONSTRAINT_OWN, @@ -446,7 +461,7 @@ var _ = Describe("Graph", func() { Expect(libreError.Error.Code).To(Equal(errorcode.NotAllowed.String())) }) It("cannot create a space with wrong request body", func() { - permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Times(1).Return(&settingssvc.GetPermissionByIDResponse{ + permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settingssvc.GetPermissionByIDResponse{ Permission: &v0.Permission{ Operation: v0.Permission_OPERATION_READWRITE, Constraint: v0.Permission_CONSTRAINT_ALL, @@ -465,8 +480,77 @@ var _ = Describe("Graph", func() { Expect(libreError.Error.Message).To(Equal("invalid body schema definition")) Expect(libreError.Error.Code).To(Equal(errorcode.InvalidRequest.String())) }) + It("transport error", func() { + permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settingssvc.GetPermissionByIDResponse{ + Permission: &v0.Permission{ + Operation: v0.Permission_OPERATION_READWRITE, + Constraint: v0.Permission_CONSTRAINT_ALL, + }, + }, nil) + gatewayClient.On("CreateStorageSpace", mock.Anything, mock.Anything).Return(&provider.CreateStorageSpaceResponse{}, errors.New("transport error")) + jsonBody := []byte(`{"Name": "Test Space"}`) + 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)) + + body, _ := io.ReadAll(rr.Body) + var libreError libregraph.OdataError + err := json.Unmarshal(body, &libreError) + Expect(err).To(Not(HaveOccurred())) + Expect(libreError.Error.Message).To(Equal("transport error")) + Expect(libreError.Error.Code).To(Equal(errorcode.GeneralException.String())) + }) + It("grpc permission denied error", func() { + permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settingssvc.GetPermissionByIDResponse{ + Permission: &v0.Permission{ + Operation: v0.Permission_OPERATION_READWRITE, + Constraint: v0.Permission_CONSTRAINT_ALL, + }, + }, nil) + gatewayClient.On("CreateStorageSpace", mock.Anything, mock.Anything).Return(&provider.CreateStorageSpaceResponse{ + Status: status.NewPermissionDenied(ctx, nil, "grpc permission denied"), + }, nil) + + jsonBody := []byte(`{"Name": "Test Space"}`) + 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.StatusForbidden)) + + body, _ := io.ReadAll(rr.Body) + var libreError libregraph.OdataError + err := json.Unmarshal(body, &libreError) + Expect(err).To(Not(HaveOccurred())) + Expect(libreError.Error.Message).To(Equal("permission denied")) + Expect(libreError.Error.Code).To(Equal(errorcode.NotAllowed.String())) + }) + It("grpc general error", func() { + permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settingssvc.GetPermissionByIDResponse{ + Permission: &v0.Permission{ + Operation: v0.Permission_OPERATION_READWRITE, + Constraint: v0.Permission_CONSTRAINT_ALL, + }, + }, nil) + gatewayClient.On("CreateStorageSpace", mock.Anything, mock.Anything).Return(&provider.CreateStorageSpaceResponse{ + Status: status.NewInternal(ctx, "grpc error"), + }, nil) + + jsonBody := []byte(`{"Name": "Test Space"}`) + 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)) + + body, _ := io.ReadAll(rr.Body) + var libreError libregraph.OdataError + err := json.Unmarshal(body, &libreError) + Expect(err).To(Not(HaveOccurred())) + Expect(libreError.Error.Message).To(Equal("grpc error")) + Expect(libreError.Error.Code).To(Equal(errorcode.GeneralException.String())) + }) It("cannot create a space with empty name", func() { - permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Times(1).Return(&settingssvc.GetPermissionByIDResponse{ + permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settingssvc.GetPermissionByIDResponse{ Permission: &v0.Permission{ Operation: v0.Permission_OPERATION_READWRITE, Constraint: v0.Permission_CONSTRAINT_ALL, @@ -486,7 +570,7 @@ var _ = Describe("Graph", func() { Expect(libreError.Error.Code).To(Equal(errorcode.InvalidRequest.String())) }) It("cannot create a space with a name that exceeds 255 chars", func() { - permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Times(1).Return(&settingssvc.GetPermissionByIDResponse{ + permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settingssvc.GetPermissionByIDResponse{ Permission: &v0.Permission{ Operation: v0.Permission_OPERATION_READWRITE, Constraint: v0.Permission_CONSTRAINT_ALL, @@ -506,7 +590,7 @@ var _ = Describe("Graph", func() { Expect(libreError.Error.Code).To(Equal(errorcode.InvalidRequest.String())) }) It("cannot create a space with a wrong type", func() { - permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Times(1).Return(&settingssvc.GetPermissionByIDResponse{ + permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settingssvc.GetPermissionByIDResponse{ Permission: &v0.Permission{ Operation: v0.Permission_OPERATION_READWRITE, Constraint: v0.Permission_CONSTRAINT_ALL, @@ -526,7 +610,7 @@ var _ = Describe("Graph", func() { Expect(libreError.Error.Code).To(Equal(errorcode.InvalidRequest.String())) }) It("cannot create a space with a name that contains invalid chars", func() { - permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Times(1).Return(&settingssvc.GetPermissionByIDResponse{ + permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settingssvc.GetPermissionByIDResponse{ Permission: &v0.Permission{ Operation: v0.Permission_OPERATION_READWRITE, Constraint: v0.Permission_CONSTRAINT_ALL, @@ -545,7 +629,7 @@ var _ = Describe("Graph", func() { Expect(libreError.Error.Message).To(Equal("invalid spacename: spacenames must not contain [/ \\ . : ? * \" > < |]")) Expect(libreError.Error.Code).To(Equal(errorcode.InvalidRequest.String())) }) - It("can create a space", func() { + It("can create a project space", func() { gatewayClient.On("CreateStorageSpace", mock.Anything, mock.Anything).Return(&provider.CreateStorageSpaceResponse{ Status: status.NewOK(ctx), StorageSpace: &provider.StorageSpace{ @@ -557,16 +641,22 @@ var _ = Describe("Graph", func() { SpaceId: "newID", OpaqueId: "newID", }, + Opaque: &typesv1beta1.Opaque{ + Map: map[string]*typesv1beta1.OpaqueEntry{ + "description": {Decoder: "plain", Value: []byte("This space is for testing")}, + "spaceAlias": {Decoder: "plain", Value: []byte("project/testspace")}, + }, + }, }, }, nil) - permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Times(1).Return(&settingssvc.GetPermissionByIDResponse{ + permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settingssvc.GetPermissionByIDResponse{ Permission: &v0.Permission{ Operation: v0.Permission_OPERATION_READWRITE, Constraint: v0.Permission_CONSTRAINT_ALL, }, }, nil) - jsonBody := []byte(`{"Name": "Test Space", "DriveType": "project"}`) + jsonBody := []byte(`{"Name": "Test Space", "DriveType": "project", "Description": "This space is for testing", "DriveAlias": "project/testspace"}`) r := httptest.NewRequest(http.MethodPost, "/graph/v1.0/drives", bytes.NewBuffer(jsonBody)).WithContext(ctx) rr := httptest.NewRecorder() svc.CreateDrive(rr, r) @@ -578,6 +668,37 @@ var _ = Describe("Graph", func() { Expect(err).ToNot(HaveOccurred()) Expect(*response.Name).To(Equal("Test Space")) Expect(*response.DriveType).To(Equal("project")) + Expect(*response.DriveAlias).To(Equal("project/testspace")) + Expect(*response.Description).To(Equal("This space is for testing")) + }) + It("Incomplete space", func() { + gatewayClient.On("CreateStorageSpace", mock.Anything, mock.Anything).Return(&provider.CreateStorageSpaceResponse{ + Status: status.NewOK(ctx), + StorageSpace: &provider.StorageSpace{ + Id: &provider.StorageSpaceId{OpaqueId: "newID"}, + Name: "Test Space", + SpaceType: "project", + }, + }, 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) + jsonBody := []byte(`{"Name": "Test Space"}`) + 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)) + + body, _ := io.ReadAll(rr.Body) + var libreError libregraph.OdataError + err := json.Unmarshal(body, &libreError) + Expect(err).To(Not(HaveOccurred())) + Expect(libreError.Error.Message).To(Equal("space has no root")) + Expect(libreError.Error.Code).To(Equal(errorcode.GeneralException.String())) }) }) })