From d9fa7455b6e23d66510af52ee3a4a6b61728eb81 Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Fri, 11 Nov 2022 10:45:11 +0100 Subject: [PATCH] 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 | 169 ++++++++++++++++-- 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, 188 insertions(+), 20 deletions(-) diff --git a/protogen/gen/ocis/messages/settings/v0/settings.pb.go b/protogen/gen/ocis/messages/settings/v0/settings.pb.go index 70600cb9f1..ede907dba3 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 25633f26e5..0329e98191 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 e75b09a86b..6d2940e010 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 45f6ab68a7..f833551832 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 6eff1f2da1..ff9ac7d479 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,16 +309,6 @@ var _ = Describe("Graph", func() { Expect(libreError.Error.Code).To(Equal(errorcode.InvalidRequest.String())) }) It("can list a spaces with invalid query parameter", func() { - gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Return(&provider.ListStorageSpacesResponse{ - Status: status.NewOK(ctx), - StorageSpaces: []*provider.StorageSpace{}}, 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"), - }, nil) - r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/me/drives?§orderby=owner%20asc", nil) rr := httptest.NewRecorder() svc.GetDrives(rr, r) @@ -318,5 +321,141 @@ var _ = Describe("Graph", func() { 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.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.NewInternal(ctx, "internal quota error"), + }, 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 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 80503fe9be..0b20788c26 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 51213475d7..e9bd3aa12e 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 94be2c07b1..85fee6d918 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 d3a0167b46..0721dd63eb 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) +}