Repository: owncloud/ocis
Pull request #5357 (2023-01-09 12:00:41)

Make amount of group members in patch configurable.
This PR changes the following:

* Create an API config section for API configurables.
* Add a setting `UserPatchLimit` that controls how many users can be changed in a PATCH request.
* Use this setting in the API to limit the amount of users that can be changed.

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.

sonarcloud[bot]: Kudos, SonarCloud Quality Gate passed!&nbsp; &nbsp; [![Quality Gate passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png 'Quality Gate passed')](https://sonarcloud.io/dashboard?id=owncloud_ocis&pullRequest=5357)

[![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=owncloud_ocis&pullRequest=5357&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=owncloud_ocis&pullRequest=5357&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=owncloud_ocis&pullRequest=5357&resolved=false&types=BUG)  
[![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=owncloud_ocis&pullRequest=5357&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=owncloud_ocis&pullRequest=5357&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=owncloud_ocis&pullRequest=5357&resolved=false&types=VULNERABILITY)  
[![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=owncloud_ocis&pullRequest=5357&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=owncloud_ocis&pullRequest=5357&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=owncloud_ocis&pullRequest=5357&resolved=false&types=SECURITY_HOTSPOT)  
[![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=owncloud_ocis&pullRequest=5357&resolved=false&types=CODE_SMELL) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=owncloud_ocis&pullRequest=5357&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=owncloud_ocis&pullRequest=5357&resolved=false&types=CODE_SMELL)

[![100.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/100-16px.png '100.0%')](https://sonarcloud.io/component_measures?id=owncloud_ocis&pullRequest=5357&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=owncloud_ocis&pullRequest=5357&metric=new_coverage&view=list)  
[![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '0.0%')](https://sonarcloud.io/component_measures?id=owncloud_ocis&pullRequest=5357&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=owncloud_ocis&pullRequest=5357&metric=new_duplicated_lines_density&view=list)


ownclouders: :boom: Acceptance test [<strong>Core-API-Tests-ocis-storage-2</strong>](https://drone.owncloud.com/owncloud/ocis/18060/50/1) failed. Further test are cancelled...
<!-- id: pr-5357 -->


Patch:
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/rogueEnv.go b/docs/helpers/extendedEnv.go
similarity index 97%
rename from docs/helpers/rogueEnv.go
rename to docs/helpers/extendedEnv.go
index 1b6bcd98857..b9271dd9ff3 100644
--- a/docs/helpers/rogueEnv.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)
 	}
@@ -146,7 +146,7 @@ func RenderGlobalVarsTemplate() {
 	}
 	fullYamlPath := filepath.Join(curdir, yamlSource)
 
-	content, err := ioutil.ReadFile("../../docs/templates/ADOC_global.tmpl")
+	content, err := ioutil.ReadFile("../../docs/templates/ADOC_extended.tmpl")
 	if err != nil {
 		log.Fatal(err)
 	}
diff --git a/docs/templates/ADOC_global.tmpl b/docs/templates/ADOC_extended.tmpl
similarity index 89%
rename from docs/templates/ADOC_global.tmpl
rename to docs/templates/ADOC_extended.tmpl
index 301369bdb3f..16c558f0d33 100644
--- a/docs/templates/ADOC_global.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))
 			})