From 624a2a956c778ef3af337f0b9684d0452c9d1787 Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Tue, 11 Jan 2022 15:04:06 +0100 Subject: [PATCH 1/5] add filter by id and driveType --- changelog/unreleased/spaces-filters.md | 6 ++++ graph/pkg/service/v0/drives.go | 38 +++++++++++++++++++++++++- 2 files changed, 43 insertions(+), 1 deletion(-) create mode 100644 changelog/unreleased/spaces-filters.md diff --git a/changelog/unreleased/spaces-filters.md b/changelog/unreleased/spaces-filters.md new file mode 100644 index 0000000000..7b3f675e3f --- /dev/null +++ b/changelog/unreleased/spaces-filters.md @@ -0,0 +1,6 @@ +Enhancement: Add filter by driveType and id to /me/drives + +We added two possible filter terms (driveType, id) to the /me/drives endpoint on the graph api. These can be used with the odata query parameter "$filter". +We only support the "eq" operator for now. + +https://github.com/owncloud/ocis/pull/2946 diff --git a/graph/pkg/service/v0/drives.go b/graph/pkg/service/v0/drives.go index 12059dd0b7..abc6ae4742 100644 --- a/graph/pkg/service/v0/drives.go +++ b/graph/pkg/service/v0/drives.go @@ -12,6 +12,7 @@ import ( "strings" "time" + "github.com/CiscoM31/godata" gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" userv1beta1 "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" cs3rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" @@ -31,6 +32,14 @@ import ( // GetDrives implements the Service interface. func (g Graph) GetDrives(w http.ResponseWriter, r *http.Request) { + sanitized := strings.TrimPrefix(r.URL.Path, "/graph/v1.0/") + // Parse the request with odata parser + odataReq, err := godata.ParseRequest(r.Context(), sanitized, r.URL.Query()) + if err != nil { + g.logger.Err(err).Msg("unable to parse odata request") + errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, err.Error()) + return + } g.logger.Info().Msg("Calling GetDrives") ctx := r.Context() @@ -64,7 +73,7 @@ func (g Graph) GetDrives(w http.ResponseWriter, r *http.Request) { Value: value, }, }}, - // TODO add filters? + Filters: generateCs3Filters(odataReq), }) switch { case err != nil: @@ -553,3 +562,30 @@ func canSetSpaceQuota(ctx context.Context, user *userv1beta1.User) (bool, error) } return true, nil } + +func generateCs3Filters(request *godata.GoDataRequest) []*storageprovider.ListStorageSpacesRequest_Filter { + var filters []*storageprovider.ListStorageSpacesRequest_Filter + if request.Query.Filter != nil && request.Query.Filter.Tree.Token.Value == "eq" { + switch request.Query.Filter.Tree.Children[0].Token.Value { + case "driveType": + filter1 := &storageprovider.ListStorageSpacesRequest_Filter{ + Type: storageprovider.ListStorageSpacesRequest_Filter_TYPE_SPACE_TYPE, + Term: &storageprovider.ListStorageSpacesRequest_Filter_SpaceType{ + SpaceType: strings.Trim(request.Query.Filter.Tree.Children[1].Token.Value, "'"), + }, + } + filters = append(filters, filter1) + case "id": + filter2 := &storageprovider.ListStorageSpacesRequest_Filter{ + Type: storageprovider.ListStorageSpacesRequest_Filter_TYPE_ID, + Term: &storageprovider.ListStorageSpacesRequest_Filter_Id{ + Id: &storageprovider.StorageSpaceId{ + OpaqueId: strings.Trim(request.Query.Filter.Tree.Children[1].Token.Value, "'"), + }, + }, + } + filters = append(filters, filter2) + } + } + return filters +} From bca954cae70cfef4afa13fafc039b1720303f52b Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Tue, 11 Jan 2022 23:42:05 +0100 Subject: [PATCH 2/5] more detailed logging --- graph/pkg/service/v0/drives.go | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/graph/pkg/service/v0/drives.go b/graph/pkg/service/v0/drives.go index abc6ae4742..b0acbd22ee 100644 --- a/graph/pkg/service/v0/drives.go +++ b/graph/pkg/service/v0/drives.go @@ -32,11 +32,11 @@ import ( // GetDrives implements the Service interface. func (g Graph) GetDrives(w http.ResponseWriter, r *http.Request) { - sanitized := strings.TrimPrefix(r.URL.Path, "/graph/v1.0/") + sanitizedPath := strings.TrimPrefix(r.URL.Path, "/graph/v1.0/") // Parse the request with odata parser - odataReq, err := godata.ParseRequest(r.Context(), sanitized, r.URL.Query()) + odataReq, err := godata.ParseRequest(r.Context(), sanitizedPath, r.URL.Query()) if err != nil { - g.logger.Err(err).Msg("unable to parse odata request") + g.logger.Err(err).Interface("query", r.URL.Query()).Msg("query error") errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, err.Error()) return } @@ -66,6 +66,12 @@ func (g Graph) GetDrives(w http.ResponseWriter, r *http.Request) { errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error()) return } + filters, err := generateCs3Filters(odataReq) + if err != nil { + g.logger.Err(err).Interface("query", r.URL.Query()).Msg("query error") + errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, err.Error()) + return + } res, err := client.ListStorageSpaces(ctx, &storageprovider.ListStorageSpacesRequest{ Opaque: &types.Opaque{Map: map[string]*types.OpaqueEntry{ "permissions": { @@ -73,7 +79,7 @@ func (g Graph) GetDrives(w http.ResponseWriter, r *http.Request) { Value: value, }, }}, - Filters: generateCs3Filters(odataReq), + Filters: filters, }) switch { case err != nil: @@ -563,7 +569,7 @@ func canSetSpaceQuota(ctx context.Context, user *userv1beta1.User) (bool, error) return true, nil } -func generateCs3Filters(request *godata.GoDataRequest) []*storageprovider.ListStorageSpacesRequest_Filter { +func generateCs3Filters(request *godata.GoDataRequest) ([]*storageprovider.ListStorageSpacesRequest_Filter, error) { var filters []*storageprovider.ListStorageSpacesRequest_Filter if request.Query.Filter != nil && request.Query.Filter.Tree.Token.Value == "eq" { switch request.Query.Filter.Tree.Children[0].Token.Value { @@ -586,6 +592,9 @@ func generateCs3Filters(request *godata.GoDataRequest) []*storageprovider.ListSt } filters = append(filters, filter2) } + } else { + err := fmt.Errorf("unsupported filter operand: %s", request.Query.Filter.Tree.Token.Value) + return nil, err } - return filters + return filters, nil } From f7bf42bce001618fe95a5105a49de0353a352d63 Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Wed, 12 Jan 2022 20:27:40 +0100 Subject: [PATCH 3/5] fix case for unsupported operators --- graph/pkg/service/v0/drives.go | 44 ++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/graph/pkg/service/v0/drives.go b/graph/pkg/service/v0/drives.go index b0acbd22ee..c83fc5dd31 100644 --- a/graph/pkg/service/v0/drives.go +++ b/graph/pkg/service/v0/drives.go @@ -571,30 +571,32 @@ func canSetSpaceQuota(ctx context.Context, user *userv1beta1.User) (bool, error) func generateCs3Filters(request *godata.GoDataRequest) ([]*storageprovider.ListStorageSpacesRequest_Filter, error) { var filters []*storageprovider.ListStorageSpacesRequest_Filter - if request.Query.Filter != nil && request.Query.Filter.Tree.Token.Value == "eq" { - switch request.Query.Filter.Tree.Children[0].Token.Value { - case "driveType": - filter1 := &storageprovider.ListStorageSpacesRequest_Filter{ - Type: storageprovider.ListStorageSpacesRequest_Filter_TYPE_SPACE_TYPE, - Term: &storageprovider.ListStorageSpacesRequest_Filter_SpaceType{ - SpaceType: strings.Trim(request.Query.Filter.Tree.Children[1].Token.Value, "'"), - }, - } - filters = append(filters, filter1) - case "id": - filter2 := &storageprovider.ListStorageSpacesRequest_Filter{ - Type: storageprovider.ListStorageSpacesRequest_Filter_TYPE_ID, - Term: &storageprovider.ListStorageSpacesRequest_Filter_Id{ - Id: &storageprovider.StorageSpaceId{ - OpaqueId: strings.Trim(request.Query.Filter.Tree.Children[1].Token.Value, "'"), + if request.Query.Filter != nil { + if request.Query.Filter.Tree.Token.Value == "eq" { + switch request.Query.Filter.Tree.Children[0].Token.Value { + case "driveType": + filter1 := &storageprovider.ListStorageSpacesRequest_Filter{ + Type: storageprovider.ListStorageSpacesRequest_Filter_TYPE_SPACE_TYPE, + Term: &storageprovider.ListStorageSpacesRequest_Filter_SpaceType{ + SpaceType: strings.Trim(request.Query.Filter.Tree.Children[1].Token.Value, "'"), }, - }, + } + filters = append(filters, filter1) + case "id": + filter2 := &storageprovider.ListStorageSpacesRequest_Filter{ + Type: storageprovider.ListStorageSpacesRequest_Filter_TYPE_ID, + Term: &storageprovider.ListStorageSpacesRequest_Filter_Id{ + Id: &storageprovider.StorageSpaceId{ + OpaqueId: strings.Trim(request.Query.Filter.Tree.Children[1].Token.Value, "'"), + }, + }, + } + filters = append(filters, filter2) } - filters = append(filters, filter2) + } else { + err := fmt.Errorf("unsupported filter operand: %s", request.Query.Filter.Tree.Token.Value) + return nil, err } - } else { - err := fmt.Errorf("unsupported filter operand: %s", request.Query.Filter.Tree.Token.Value) - return nil, err } return filters, nil } From 1db13d8a391b5fc754e5cdd7e2e987b7e97e3572 Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Thu, 13 Jan 2022 15:28:33 +0100 Subject: [PATCH 4/5] add tests --- graph/pkg/service/v0/drives.go | 2 +- .../features/apiSpaces/listSpaces.feature | 29 +++++++++ .../features/bootstrap/SpacesContext.php | 61 ++++++++++++++++++- 3 files changed, 90 insertions(+), 2 deletions(-) diff --git a/graph/pkg/service/v0/drives.go b/graph/pkg/service/v0/drives.go index c83fc5dd31..bfed8d4dcc 100644 --- a/graph/pkg/service/v0/drives.go +++ b/graph/pkg/service/v0/drives.go @@ -69,7 +69,7 @@ func (g Graph) GetDrives(w http.ResponseWriter, r *http.Request) { filters, err := generateCs3Filters(odataReq) if err != nil { g.logger.Err(err).Interface("query", r.URL.Query()).Msg("query error") - errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, err.Error()) + errorcode.NotSupported.Render(w, r, http.StatusNotImplemented, err.Error()) return } res, err := client.ListStorageSpaces(ctx, &storageprovider.ListStorageSpacesRequest{ diff --git a/tests/acceptance/features/apiSpaces/listSpaces.feature b/tests/acceptance/features/apiSpaces/listSpaces.feature index 8705184e30..aca06e03b4 100644 --- a/tests/acceptance/features/apiSpaces/listSpaces.feature +++ b/tests/acceptance/features/apiSpaces/listSpaces.feature @@ -19,6 +19,35 @@ Feature: List and create spaces | name | Alice Hansen | | quota@@@state | normal | | root@@@webDavUrl | %base_url%/dav/spaces/%space_id% | + And the json responded should contain a space "Shares Jail" with these key and value pairs: + | key | value | + | driveType | virtual | + | id | %space_id% | + | name | Shares Jail | + | root@@@webDavUrl | %base_url%/dav/spaces/%space_id% | + + Scenario: An ordinary user can request information about their Space via the Graph API using a filter + When user "Alice" lists all available spaces via the GraphApi with query "$filter=driveType eq 'personal'" + Then the HTTP status code should be "200" + And the json responded should contain a space "Alice Hansen" with these key and value pairs: + | key | value | + | driveType | personal | + | id | %space_id% | + | name | Alice Hansen | + | quota@@@state | normal | + | root@@@webDavUrl | %base_url%/dav/spaces/%space_id% | + And the json responded should not contain a space with name "Shares Jail" + And the json responded should contain spaces of type "personal" + And the json responded should only contain spaces of type "personal" + And the json responded should not contain spaces of type "project" + And the json responded should not contain spaces of type "virtual" + And the json responded should not contain spaces of type "public" + + Scenario: An ordinary user will not see any space when using a filter for project + When user "Alice" lists all available spaces via the GraphApi with query "$filter=driveType eq 'project'" + Then the HTTP status code should be "200" + And the json responded should not contain a space with name "Alice Hansen" + And the json responded should not contain spaces of type "personal" Scenario: An ordinary user can access their Space via the webDav API When user "Alice" lists all available spaces via the GraphApi diff --git a/tests/acceptance/features/bootstrap/SpacesContext.php b/tests/acceptance/features/bootstrap/SpacesContext.php index 7de32c7e3b..c5d5a220ae 100644 --- a/tests/acceptance/features/bootstrap/SpacesContext.php +++ b/tests/acceptance/features/bootstrap/SpacesContext.php @@ -350,6 +350,26 @@ class SpacesContext implements Context { $this->rememberTheAvailableSpaces(); } + /** + * @When /^user "([^"]*)" lists all available spaces via the GraphApi with query "([^"]*)"$/ + * + * @param string $user + * @param string $query + * + * @return void + * + * @throws GuzzleException + */ + public function theUserListsAllHisAvailableSpacesUsingTheGraphApiWithFilter(string $user, string $query): void { + $this->featureContext->setResponse( + $this->listSpacesRequest( + $user, + $this->featureContext->getPasswordForUser($user), + "?". $query + ) + ); + } + /** * @When /^user "([^"]*)" creates a space "([^"]*)" of type "([^"]*)" with the default quota using the GraphApi$/ * @@ -659,7 +679,7 @@ class SpacesContext implements Context { } /** - * @Then /^the json responded should not contain a space "([^"]*)"$/ + * @Then /^the json responded should not contain a space with name "([^"]*)"$/ * * @param string $spaceName * @@ -672,6 +692,45 @@ class SpacesContext implements Context { Assert::assertEmpty($this->getSpaceByNameFromResponse($spaceName), "space $spaceName should not be available for a user"); } + /** + * @Then /^the json responded should (not|only|)\s?contain spaces of type "([^"]*)"$/ + * + * @param string $shoulOrNot (not|only|) + * @param string $type + * + * @return void + * @throws Exception + */ + public function jsonRespondedShouldNotContainSpaceType( + string $onlyOrNot, + string $type + ): void { + Assert::assertNotEmpty( + $spaces = json_decode( + (string)$this->featureContext + ->getResponse()->getBody(), + true, + 512, + JSON_THROW_ON_ERROR + ) + ); + $matches = []; + foreach($spaces["value"] as $space) { + if($onlyOrNot === "not") { + Assert::assertNotEquals($space["driveType"], $type); + } + if($onlyOrNot === "only") { + Assert::assertEquals($space["driveType"], $type); + } + if($onlyOrNot === "" && $space["driveType"] === $type) { + $matches[] = $space; + } + } + if($onlyOrNot === "") { + Assert::assertNotEmpty($matches); + } + } + /** * @param string $shouldOrNot (not|) * @param TableNode $expectedFiles From 16c7706d6221160a6a3f1e81261aad860e3bdd5c Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Fri, 14 Jan 2022 00:13:17 +0100 Subject: [PATCH 5/5] remove redundancy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Jörn Friedrich Dreyer --- tests/acceptance/features/apiSpaces/listSpaces.feature | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/acceptance/features/apiSpaces/listSpaces.feature b/tests/acceptance/features/apiSpaces/listSpaces.feature index aca06e03b4..175c562cc7 100644 --- a/tests/acceptance/features/apiSpaces/listSpaces.feature +++ b/tests/acceptance/features/apiSpaces/listSpaces.feature @@ -37,11 +37,7 @@ Feature: List and create spaces | quota@@@state | normal | | root@@@webDavUrl | %base_url%/dav/spaces/%space_id% | And the json responded should not contain a space with name "Shares Jail" - And the json responded should contain spaces of type "personal" And the json responded should only contain spaces of type "personal" - And the json responded should not contain spaces of type "project" - And the json responded should not contain spaces of type "virtual" - And the json responded should not contain spaces of type "public" Scenario: An ordinary user will not see any space when using a filter for project When user "Alice" lists all available spaces via the GraphApi with query "$filter=driveType eq 'project'"