Repository: owncloud/ocis
Pull request #5365 (2023-01-10 09:51:28)

[test-only] apiTest. Delete group from the space
- Tests for removing a group from a space





update-docs[bot]: Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a [changelog](https://github.com/owncloud/ocis/blob/master/changelog/README.md) item based on your changes.


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5c5e48d1097..aa6e0bec168 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -14,6 +14,7 @@ The following sections list the changes for unreleased.
 * Enhancement - Bump libre-graph-api-go: [#5309](https://github.com/owncloud/ocis/pull/5309)
 * Enhancement - Bump reva version: [#5243](https://github.com/owncloud/ocis/pull/5243)
 * Enhancement - Drive group permissions: [#5312](https://github.com/owncloud/ocis/pull/5312)
+* Enhancement - Make the group members addition limit configurable: [#5357](https://github.com/owncloud/ocis/pull/5357)
 * Enhancement - Extended search: [#5221](https://github.com/owncloud/ocis/pull/5221)
 * Enhancement - Resource tags: [#5227](https://github.com/owncloud/ocis/pull/5227)
 
diff --git a/changelog/unreleased/enhancement-add-configurable-group-members-patch-limit.md b/changelog/unreleased/enhancement-add-configurable-group-members-patch-limit.md
new file mode 100644
index 00000000000..261fb3757fe
--- /dev/null
+++ b/changelog/unreleased/enhancement-add-configurable-group-members-patch-limit.md
@@ -0,0 +1,8 @@
+Enhancement: Make the group members addition limit configurable
+
+It's now possible to configure the limit of group members addition by PATCHing `/graph/v1.0/groups/{groupID}`.
+It still defaults to 20 as defined in the spec but it can be configured via `.graph.api.group_members_patch_limit`
+in `ocis.yaml` or via the `GRAPH_GROUP_MEMBERS_PATCH_LIMIT` environment variable.
+
+https://github.com/owncloud/ocis/pull/5357
+https://github.com/owncloud/ocis/issues/5262
diff --git a/docs/helpers/extendedEnv.go b/docs/helpers/extendedEnv.go
index 4bc0b577af0..b9271dd9ff3 100644
--- a/docs/helpers/extendedEnv.go
+++ b/docs/helpers/extendedEnv.go
@@ -70,7 +70,7 @@ func GetRogueEnvs() {
 		log.Fatal(err)
 	}
 	fmt.Println("Gathering variable definitions from source")
-	out, err := exec.Command("sh", "-c", "grep -RHn os.Getenv | grep -v rogueEnv.go |grep \\.go").Output()
+	out, err := exec.Command("sh", "-c", "grep -RHn os.Getenv | grep -v extendedEnv.go |grep \\.go").Output()
 	if err != nil {
 		log.Fatal(err)
 	}
diff --git a/docs/templates/ADOC_extended.tmpl b/docs/templates/ADOC_extended.tmpl
index 301369bdb3f..16c558f0d33 100644
--- a/docs/templates/ADOC_extended.tmpl
+++ b/docs/templates/ADOC_extended.tmpl
@@ -1,4 +1,4 @@
-// collected through docs/helpers/rougeEnv.go
+// collected through docs/helpers/extendedEnv.go
 
 [caption=]
 .Environment variables with extended scope not included in a service
diff --git a/services/graph/pkg/config/config.go b/services/graph/pkg/config/config.go
index 79d18268a9a..5fa9843a319 100644
--- a/services/graph/pkg/config/config.go
+++ b/services/graph/pkg/config/config.go
@@ -19,6 +19,8 @@ type Config struct {
 
 	HTTP HTTP `yaml:"http"`
 
+	API API `yaml:"api"`
+
 	Reva          *shared.Reva          `yaml:"reva"`
 	TokenManager  *TokenManager         `yaml:"token_manager"`
 	GRPCClientTLS *shared.GRPCClientTLS `yaml:"grpc_client_tls"`
@@ -85,6 +87,11 @@ type Identity struct {
 	LDAP    LDAP   `yaml:"ldap"`
 }
 
+// API represents API configuration parameters.
+type API struct {
+	GroupMembersPatchLimit int `yaml:"group_members_patch_limit" env:"GRAPH_GROUP_MEMBERS_PATCH_LIMIT" desc:"The amount of group members allowed to be added with a single patch request."`
+}
+
 // Events combines the configuration options for the event bus.
 type Events struct {
 	Endpoint             string `yaml:"endpoint" env:"GRAPH_EVENTS_ENDPOINT" desc:"The address of the event system. The event system is the message queuing service. It is used as message broker for the microservice architecture. Set to a empty string to disable emitting events."`
diff --git a/services/graph/pkg/config/defaults/defaultconfig.go b/services/graph/pkg/config/defaults/defaultconfig.go
index 84c0fc94936..5f4d15cdb67 100644
--- a/services/graph/pkg/config/defaults/defaultconfig.go
+++ b/services/graph/pkg/config/defaults/defaultconfig.go
@@ -30,6 +30,9 @@ func DefaultConfig() *config.Config {
 		Service: config.Service{
 			Name: "graph",
 		},
+		API: config.API{
+			GroupMembersPatchLimit: 20,
+		},
 		Reva: shared.DefaultRevaConfig(),
 		Spaces: config.Spaces{
 			WebDavBase:   "https://localhost:9200",
diff --git a/services/graph/pkg/service/v0/groups.go b/services/graph/pkg/service/v0/groups.go
index 2df0cc39666..283676b9503 100644
--- a/services/graph/pkg/service/v0/groups.go
+++ b/services/graph/pkg/service/v0/groups.go
@@ -19,7 +19,6 @@ import (
 	"github.com/go-chi/render"
 )
 
-const memberRefsLimit = 20
 const memberTypeUsers = "users"
 
 // GetGroups implements the Service interface.
@@ -124,13 +123,13 @@ func (g Graph) PatchGroup(w http.ResponseWriter, r *http.Request) {
 
 	if memberRefs, ok := changes.GetMembersodataBindOk(); ok {
 		// The spec defines a limit of 20 members maxium per Request
-		if len(memberRefs) > memberRefsLimit {
+		if len(memberRefs) > g.config.API.GroupMembersPatchLimit {
 			logger.Debug().
 				Int("number", len(memberRefs)).
-				Int("limit", memberRefsLimit).
+				Int("limit", g.config.API.GroupMembersPatchLimit).
 				Msg("could not create group, exceeded members limit")
 			errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest,
-				fmt.Sprintf("Request is limited to %d members", memberRefsLimit))
+				fmt.Sprintf("Request is limited to %d members", g.config.API.GroupMembersPatchLimit))
 			return
 		}
 		memberIDs := make([]string, 0, len(memberRefs))


Test patch:
diff --git a/services/graph/pkg/service/v0/groups_test.go b/services/graph/pkg/service/v0/groups_test.go
index 02cbe9d71fc..d5ada43d196 100644
--- a/services/graph/pkg/service/v0/groups_test.go
+++ b/services/graph/pkg/service/v0/groups_test.go
@@ -6,6 +6,7 @@ import (
 	"encoding/json"
 	"errors"
 	"io"
+	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
 
@@ -284,8 +285,10 @@ var _ = Describe("Groups", func() {
 			It("fails when the number of users is exceeded - spec says 20 max", func() {
 				updatedGroup := libregraph.NewGroup()
 				updatedGroup.SetDisplayName("group1 updated")
-				updatedGroup.SetMembersodataBind([]string{"1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "12", "13", "14", "15", "16", "17", "18",
-					"19", "20", "21"})
+				updatedGroup.SetMembersodataBind([]string{
+					"1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "12", "13", "14", "15", "16", "17", "18",
+					"19", "20", "21",
+				})
 				updatedGroupJson, err := json.Marshal(updatedGroup)
 				Expect(err).ToNot(HaveOccurred())
 
@@ -295,6 +298,41 @@ var _ = Describe("Groups", func() {
 				r = r.WithContext(context.WithValue(revactx.ContextSetUser(ctx, currentUser), chi.RouteCtxKey, rctx))
 				svc.PatchGroup(rr, r)
 
+				resp, err := ioutil.ReadAll(rr.Body)
+				Expect(err).ToNot(HaveOccurred())
+
+				Expect(string(resp)).To(ContainSubstring("Request is limited to 20"))
+				Expect(rr.Code).To(Equal(http.StatusBadRequest))
+			})
+
+			It("succeeds when the number of users is over 20 but the limit is raised to 21", func() {
+				updatedGroup := libregraph.NewGroup()
+				updatedGroup.SetDisplayName("group1 updated")
+				updatedGroup.SetMembersodataBind([]string{
+					"1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "12", "13", "14", "15", "16", "17", "18",
+					"19", "20", "21",
+				})
+				updatedGroupJson, err := json.Marshal(updatedGroup)
+				Expect(err).ToNot(HaveOccurred())
+
+				cfg.API.GroupMembersPatchLimit = 21
+				svc = service.NewService(
+					service.Config(cfg),
+					service.WithGatewayClient(gatewayClient),
+					service.EventsPublisher(&eventsPublisher),
+					service.WithIdentityBackend(identityBackend),
+				)
+
+				r := httptest.NewRequest(http.MethodPatch, "/graph/v1.0/me/groups", bytes.NewBuffer(updatedGroupJson))
+				rctx := chi.NewRouteContext()
+				rctx.URLParams.Add("groupID", *newGroup.Id)
+				r = r.WithContext(context.WithValue(revactx.ContextSetUser(ctx, currentUser), chi.RouteCtxKey, rctx))
+				svc.PatchGroup(rr, r)
+
+				resp, err := ioutil.ReadAll(rr.Body)
+				Expect(err).ToNot(HaveOccurred())
+
+				Expect(string(resp)).To(ContainSubstring("Error parsing member@odata.bind values"))
 				Expect(rr.Code).To(Equal(http.StatusBadRequest))
 			})
 
diff --git a/tests/acceptance/features/apiSpacesShares/shareSpaces.feature b/tests/acceptance/features/apiSpacesShares/shareSpaces.feature
index 035871cf486..75b03955c7f 100644
--- a/tests/acceptance/features/apiSpacesShares/shareSpaces.feature
+++ b/tests/acceptance/features/apiSpacesShares/shareSpaces.feature
@@ -60,7 +60,7 @@ Feature: Share spaces
       | name      | share space |
     When user "Alice" unshares a space "share space" to user "Brian"
     Then the HTTP status code should be "200"
-    Then the user "Brian" should not have a space called "share space"
+    And the user "Brian" should not have a space called "share space"
 
 
   Scenario Outline: Owner of a space cannot see the space after removing his access to the space
@@ -188,3 +188,44 @@ Feature: Share spaces
       | manager |
       | editor  |
       | viewer  |
+
+
+  Scenario Outline: The user has no access to the space if access for the group has been removed
+    Given group "group2" has been created
+    And the administrator has added a user "Brian" to the group "group2" using GraphApi
+    And user "Alice" has shared a space "share space" to group "group2" with role "<role>"
+    When user "Alice" unshares a space "share space" to group "group2"
+    Then the HTTP status code should be "200"
+    And the user "Brian" should not have a space called "share space"
+    Examples:
+      | role    |
+      | manager |
+      | editor  |
+      | viewer  |
+
+
+  Scenario: The user has no access to the space if he has been removed from the group
+    Given group "group2" has been created
+    And the administrator has added a user "Brian" to the group "group2" using GraphApi
+    And the administrator has added a user "Bob" to the group "group2" using GraphApi
+    And user "Alice" has shared a space "share space" to group "group2" with role "editor"
+    When the administrator removes the following users from the following groups using the Graph API
+      | username | groupname |
+      | Brian    | group2    |
+    Then the HTTP status code of responses on all endpoints should be "204"
+    And the user "Brian" should not have a space called "share space"
+    But the user "Bob" should have a space called "share space" with these key and value pairs:
+      | key       | value       |
+      | driveType | project     |
+      | name      | share space |
+
+
+  Scenario: Users don't have access to the space if the group has been deleted
+    Given group "group2" has been created
+    And the administrator has added a user "Brian" to the group "group2" using GraphApi
+    And the administrator has added a user "Bob" to the group "group2" using GraphApi
+    And user "Alice" has shared a space "share space" to group "group2" with role "editor"
+    When the administrator deletes group "group2" using the Graph API
+    Then the HTTP status code should be "204"
+    And the user "Brian" should not have a space called "share space"
+    And the user "Bob" should not have a space called "share space"
\ No newline at end of file
diff --git a/tests/acceptance/features/bootstrap/GraphContext.php b/tests/acceptance/features/bootstrap/GraphContext.php
index 92b857452fd..4d48836667d 100644
--- a/tests/acceptance/features/bootstrap/GraphContext.php
+++ b/tests/acceptance/features/bootstrap/GraphContext.php
@@ -1021,7 +1021,7 @@ class GraphContext implements Context {
 	 *
 	 * @return void
 	 */
-	public function userDeletesGroupUsingTheGraphApi(string $group, ?string $user): void {
+	public function userDeletesGroupUsingTheGraphApi(string $group, ?string $user = null): void {
 		$groupId = $this->featureContext->getAttributeOfCreatedGroup($group, "id");
 		$response = $this->userDeletesGroupWithGroupId($groupId, $user);
 		$this->featureContext->setResponse($response);
diff --git a/tests/acceptance/features/bootstrap/SpacesContext.php b/tests/acceptance/features/bootstrap/SpacesContext.php
index c4d77784ee7..7d1e0d7e7d5 100644
--- a/tests/acceptance/features/bootstrap/SpacesContext.php
+++ b/tests/acceptance/features/bootstrap/SpacesContext.php
@@ -2040,11 +2040,11 @@ class SpacesContext implements Context {
 	}
 
 	/**
-	 * @Given /^user "([^"]*)" has shared a space "([^"]*)" to user "([^"]*)" with role "([^"]*)"$/
+	 * @Given /^user "([^"]*)" has shared a space "([^"]*)" to (?:user|group) "([^"]*)" with role "([^"]*)"$/
 	 *
 	 * @param  string $user
 	 * @param  string $spaceName
-	 * @param  string $userRecipient
+	 * @param  string $recipient
 	 * @param  string $role
 	 *
 	 * @return void
@@ -2053,10 +2053,10 @@ class SpacesContext implements Context {
 	public function userHasSharedSpace(
 		string $user,
 		string $spaceName,
-		string $userRecipient,
+		string $recipient,
 		string $role
 	): void {
-		$this->sendShareSpaceRequest($user, $spaceName, $userRecipient, $role);
+		$this->sendShareSpaceRequest($user, $spaceName, $recipient, $role);
 		$expectedHTTPStatus = "200";
 		$this->featureContext->theHTTPStatusCodeShouldBe(
 			$expectedHTTPStatus,
@@ -2067,11 +2067,11 @@ class SpacesContext implements Context {
 	}
 
 	/**
-	 * @When /^user "([^"]*)" unshares a space "([^"]*)" to user "([^"]*)"$/
+	 * @When /^user "([^"]*)" unshares a space "([^"]*)" to (?:user|group) "([^"]*)"$/
 	 *
 	 * @param  string $user
 	 * @param  string $spaceName
-	 * @param  string $userRecipient
+	 * @param  string $recipient
 	 *
 	 * @return void
 	 * @throws GuzzleException
@@ -2079,10 +2079,10 @@ class SpacesContext implements Context {
 	public function sendUnshareSpaceRequest(
 		string $user,
 		string $spaceName,
-		string $userRecipient
+		string $recipient
 	): void {
 		$space = $this->getSpaceByName($user, $spaceName);
-		$fullUrl = $this->baseUrl . $this->ocsApiUrl . "/" . $space['id'] . "?shareWith=" . $userRecipient;
+		$fullUrl = $this->baseUrl . $this->ocsApiUrl . "/" . $space['id'] . "?shareWith=" . $recipient;
 
 		$this->featureContext->setResponse(
 			HttpRequestHelper::delete(