Merge pull request #9867 from rhafer/issue/9858

fix(graph): treat memberOf and userType as read-only attributes

changelog/unreleased/fix-graph-readonly-attributes.md

@@ -0,0 +1,7 @@
+Bugfix: The user attributes `userType` and `memberOf` are readonly
+
+The graph API now treats the user attributes `userType` and `memberOf` as
+read-only. They are not meant be updated directly by the client.
+
+https://github.com/owncloud/ocis/pull/9867
+https://github.com/owncloud/ocis/issues/9858

go.mod

@@ -71,7 +71,7 @@ require (
 	github.com/onsi/gomega v1.34.1
 	github.com/open-policy-agent/opa v0.67.1
 	github.com/orcaman/concurrent-map v1.0.0
-	github.com/owncloud/libre-graph-api-go v1.0.5-0.20240816082515-afcf4367b966
+	github.com/owncloud/libre-graph-api-go v1.0.5-0.20240820135012-5fac8096ce9c
 	github.com/pkg/errors v0.9.1
 	github.com/pkg/xattr v0.4.9
 	github.com/prometheus/client_golang v1.20.1

go.sum

@@ -940,8 +940,8 @@ github.com/oracle/oci-go-sdk v24.3.0+incompatible/go.mod h1:VQb79nF8Z2cwLkLS35uk
 github.com/orcaman/concurrent-map v1.0.0 h1:I/2A2XPCb4IuQWcQhBhSwGfiuybl/J0ev9HDbW65HOY=
 github.com/orcaman/concurrent-map v1.0.0/go.mod h1:Lu3tH6HLW3feq74c2GC+jIMS/K2CFcDWnWD9XkenwhI=
 github.com/ovh/go-ovh v1.1.0/go.mod h1:AxitLZ5HBRPyUd+Zl60Ajaag+rNTdVXWIkzfrVuTXWA=
-github.com/owncloud/libre-graph-api-go v1.0.5-0.20240816082515-afcf4367b966 h1:HlbrRDfVqRo/URoIw3AUuPF0/1bIysP5qN1A6Im8djM=
-github.com/owncloud/libre-graph-api-go v1.0.5-0.20240816082515-afcf4367b966/go.mod h1:yXI+rmE8yYx+ZsGVrnCpprw/gZMcxjwntnX2y2+VKxY=
+github.com/owncloud/libre-graph-api-go v1.0.5-0.20240820135012-5fac8096ce9c h1:vWUozQREEAnxNMaN+ZRlp7BFXohJr2H8tc0M2oYoAEw=
+github.com/owncloud/libre-graph-api-go v1.0.5-0.20240820135012-5fac8096ce9c/go.mod h1:yXI+rmE8yYx+ZsGVrnCpprw/gZMcxjwntnX2y2+VKxY=
 github.com/oxtoacart/bpool v0.0.0-20190530202638-03653db5a59c h1:rp5dCmg/yLR3mgFuSOe4oEnDDmGLROTvMragMUXpTQw=
 github.com/oxtoacart/bpool v0.0.0-20190530202638-03653db5a59c/go.mod h1:X07ZCGwUbLaax7L0S3Tw4hpejzu63ZrrQiUe6W0hcy0=
 github.com/pablodz/inotifywaitgo v0.0.7 h1:1ii49dGBnRn0t1Sz7RGZS6/NberPEDQprwKHN49Bv6U=

services/graph/pkg/service/v0/users.go

@@ -368,22 +368,27 @@ func (g Graph) PostUser(w http.ResponseWriter, r *http.Request) {
 	// Disallow user-supplied IDs. It's supposed to be readonly. We're either
 	// generating them in the backend ourselves or rely on the Backend's
 	// storage (e.g. LDAP) to provide a unique ID.
-	if _, ok := u.GetIdOk(); ok {
+	if u.HasId() {
 		logger.Info().Interface("user", u).Msg("could not create user: user id is a read-only attribute")
 		errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "user id is a read-only attribute")
 		return
 	}
 
-	if u.HasUserType() {
-		if !isValidUserType(*u.UserType) {
-			logger.Info().Interface("user", u).Msg("invalid userType attribute")
-			errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "invalid userType attribute, valid options are 'Member' or 'Guest'")
-			return
-		}
-	} else {
-		u.SetUserType("Member")
+	// treat memberOf as a read-only attribute.
+	if u.HasMemberOf() {
+		logger.Info().Interface("user", u).Msg("could not create user: memberOf id is a read-only attribute")
+		errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "memberOf id is a read-only attribute")
+		return
 	}
 
+	// treat userType as a read-only attribute.
+	if u.HasUserType() {
+		logger.Info().Interface("user", u).Msg("could not create user: userType is a read-only attribute")
+		errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "userType is a read-only attribute")
+		return
+	}
+	u.SetUserType("Member")
+
 	logger.Debug().Interface("user", u).Msg("calling create user on backend")
 	if u, err = g.identityBackend.CreateUser(r.Context(), *u); err != nil {
 		logger.Error().Err(err).Msg("could not create user: backend error")
@@ -790,6 +795,24 @@ func (g Graph) patchUser(w http.ResponseWriter, r *http.Request, nameOrID string
 		return
 	}
 
+	if changes.HasId() {
+		logger.Debug().Msg("could not update user: user id is a read-only attribute")
+		errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "user id is a read-only attribute")
+		return
+	}
+
+	if changes.HasMemberOf() {
+		logger.Debug().Msg("could not update user: memberOf id is a read-only attribute")
+		errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "memberOf id is a read-only attribute")
+		return
+	}
+
+	if changes.HasUserType() {
+		logger.Debug().Msg("could not update user: userType is a read-only attribute")
+		errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "userType is a read-only attribute")
+		return
+	}
+
 	if accountName, ok := changes.GetOnPremisesSamAccountNameOk(); ok {
 		if !g.isValidUsername(*accountName) {
 			logger.Info().Str("username", *accountName).Msg("could not update user: invalid username")
@@ -871,15 +894,6 @@ func (g Graph) patchUser(w http.ResponseWriter, r *http.Request, nameOrID string
 		addfeature("displayname", *name, &oldUserValues.DisplayName)
 	}
 
-	if userType, ok := changes.GetUserTypeOk(); ok {
-		if !isValidUserType(*changes.UserType) {
-			logger.Debug().Interface("user", changes).Msg("invalid userType attribute")
-			errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "invalid userType attribute, valid options are 'Member' or 'Guest'")
-			return
-		}
-		addfeature("userType", *userType, oldUserValues.UserType)
-	}
-
 	if accEnabled, ok := changes.GetAccountEnabledOk(); ok {
 		oldAccVal := strconv.FormatBool(oldUserValues.GetAccountEnabled())
 		addfeature("accountEnabled", strconv.FormatBool(*accEnabled), &oldAccVal)

services/graph/pkg/service/v0/users_test.go

@@ -829,8 +829,16 @@ var _ = Describe("Users", func() {
 				assertHandleBadAttributes(user)
 			})
 
-			It("handles invalid userType", func() {
-				user.SetUserType("Clown")
+			It("handles set memberOf - it's read-only", func() {
+				group := libregraph.Group{}
+				group.SetId("/groups/group")
+				group.SetDisplayName("Group")
+				user.SetMemberOf([]libregraph.Group{group})
+				assertHandleBadAttributes(user)
+			})
+
+			It("handles set userType - it's read-only", func() {
+				user.SetUserType("Member")
 				assertHandleBadAttributes(user)
 			})
 
@@ -857,31 +865,6 @@ var _ = Describe("Users", func() {
 				Expect(createdUser.GetUserType()).To(Equal("Member"))
 			})
 
-			It("creates a guest user", func() {
-				roleService.On("AssignRoleToUser", mock.Anything, mock.Anything).Return(&settings.AssignRoleToUserResponse{}, nil)
-				identityBackend.On("CreateUser", mock.Anything, mock.Anything).Return(func(ctx context.Context, user libregraph.User) *libregraph.User {
-					user.SetId("/users/user")
-					return &user
-				}, nil)
-
-				user.SetUserType("Guest")
-				userJson, err := json.Marshal(user)
-				Expect(err).ToNot(HaveOccurred())
-
-				r := httptest.NewRequest(http.MethodPost, "/graph/v1.0/users", bytes.NewBuffer(userJson))
-				r = r.WithContext(revactx.ContextSetUser(ctx, currentUser))
-				svc.PostUser(rr, r)
-
-				Expect(rr.Code).To(Equal(http.StatusCreated))
-				data, err := io.ReadAll(rr.Body)
-				Expect(err).ToNot(HaveOccurred())
-
-				createdUser := libregraph.User{}
-				err = json.Unmarshal(data, &createdUser)
-				Expect(err).ToNot(HaveOccurred())
-				Expect(createdUser.GetUserType()).To(Equal("Guest"))
-			})
-
 			It("creates a member user", func() {
 				roleService.On("AssignRoleToUser", mock.Anything, mock.Anything).Return(&settings.AssignRoleToUserResponse{}, nil)
 				identityBackend.On("CreateUser", mock.Anything, mock.Anything).Return(func(ctx context.Context, user libregraph.User) *libregraph.User {
@@ -889,7 +872,6 @@ var _ = Describe("Users", func() {
 					return &user
 				}, nil)
 
-				user.SetUserType("Member")
 				userJson, err := json.Marshal(user)
 				Expect(err).ToNot(HaveOccurred())
 
@@ -1026,9 +1008,21 @@ var _ = Describe("Users", func() {
 
 		Describe("PatchUser", func() {
 			var (
-				user         *libregraph.User
-				userUpdate   *libregraph.UserUpdate
-				expectedUser *libregraph.User
+				user                      *libregraph.User
+				userUpdate                *libregraph.UserUpdate
+				expectedUser              *libregraph.User
+				assertHandleBadAttributes = func(userid string, update *libregraph.UserUpdate) {
+					userJson, err := json.Marshal(update)
+					Expect(err).ToNot(HaveOccurred())
+
+					r := httptest.NewRequest(http.MethodPatch, "/graph/v1.0/users/{userid}", bytes.NewBuffer(userJson))
+					rctx := chi.NewRouteContext()
+					rctx.URLParams.Add("userID", userid)
+					r = r.WithContext(context.WithValue(revactx.ContextSetUser(ctx, currentUser), chi.RouteCtxKey, rctx))
+					svc.PatchUser(rr, r)
+
+					Expect(rr.Code).To(Equal(http.StatusBadRequest))
+				}
 			)
 
 			BeforeEach(func() {
@@ -1037,7 +1031,6 @@ var _ = Describe("Users", func() {
 				user.SetId("/users/user")
 
 				userUpdate = libregraph.NewUserUpdate()
-				userUpdate.SetId(user.GetId())
 
 				expectedUser = libregraph.NewUser("Display Name", "user")
 				expectedUser.SetMail(user.GetMail())
@@ -1063,6 +1056,24 @@ var _ = Describe("Users", func() {
 				Expect(rr.Code).To(Equal(http.StatusBadRequest))
 			})
 
+			It("handles set Ids - they are read-only", func() {
+				userUpdate.SetId("/users/user")
+				assertHandleBadAttributes(user.GetId(), userUpdate)
+			})
+
+			It("handles set memberOf - it's read-only", func() {
+				group := libregraph.Group{}
+				group.SetId("/groups/group")
+				group.SetDisplayName("Group")
+				userUpdate.SetMemberOf([]libregraph.Group{group})
+				assertHandleBadAttributes(user.GetId(), userUpdate)
+			})
+
+			It("handles set userType - it's read-only", func() {
+				userUpdate.SetUserType("Member")
+				assertHandleBadAttributes(user.GetId(), userUpdate)
+			})
+
 			It("handles invalid email", func() {
 				userUpdate.SetMail("invalid")
 				data, err := json.Marshal(userUpdate)
@@ -1077,27 +1088,13 @@ var _ = Describe("Users", func() {
 				Expect(rr.Code).To(Equal(http.StatusBadRequest))
 			})
 
-			It("handles invalid userType", func() {
-				userUpdate.SetUserType("Clown")
-				data, err := json.Marshal(userUpdate)
-				Expect(err).ToNot(HaveOccurred())
-
-				r := httptest.NewRequest(http.MethodPost, "/graph/v1.0/users?$invalid=true", bytes.NewBuffer(data))
-				rctx := chi.NewRouteContext()
-				rctx.URLParams.Add("userID", userUpdate.GetId())
-				r = r.WithContext(context.WithValue(revactx.ContextSetUser(ctx, currentUser), chi.RouteCtxKey, rctx))
-				svc.PatchUser(rr, r)
-
-				Expect(rr.Code).To(Equal(http.StatusBadRequest))
-			})
-
 			It("updates attributes", func() {
-				userUpdate.SetUserType("Member")
+				userUpdate.SetMail("mail@mail.test")
 				userUpdate.SetDisplayName("New Display Name")
 
-				expectedUser.SetUserType("Member")
+				expectedUser.SetMail("mail@mail.test")
 				expectedUser.SetDisplayName("New Display Name")
-				identityBackend.On("UpdateUser", mock.Anything, userUpdate.GetId(), mock.Anything).Return(expectedUser, nil)
+				identityBackend.On("UpdateUser", mock.Anything, user.GetId(), mock.Anything).Return(expectedUser, nil)
 
 				data, err := json.Marshal(userUpdate)
 				Expect(err).ToNot(HaveOccurred())
@@ -1115,7 +1112,7 @@ var _ = Describe("Users", func() {
 				unmarshaledUser := libregraph.User{}
 				err = json.Unmarshal(data, &unmarshaledUser)
 				Expect(err).ToNot(HaveOccurred())
-				Expect(unmarshaledUser.GetUserType()).To(Equal("Member"))
+				Expect(unmarshaledUser.GetMail()).To(Equal("mail@mail.test"))
 				Expect(unmarshaledUser.GetDisplayName()).To(Equal("New Display Name"))
 			})
 		})

vendor/github.com/owncloud/libre-graph-api-go/model_identity.go

@@ -25,7 +25,7 @@ type Identity struct {
 	DisplayName string `json:"displayName"`
 	// Unique identifier for the identity.
 	Id *string `json:"id,omitempty"`
-	// The type of the identity. This can be either \"Member\" for regular user, \"Guest\" for guest users or \"Federated\" for users imported from a federated instance. Can be used by clients to indicate the type of user. For more details, clients should look up and cache the user at the /users enpoint.
+	// The type of the identity. This can be either \"Member\" for regular user, \"Guest\" for guest users or \"Federated\" for users imported from a federated instance. Can be used by clients to indicate the type of user. For more details, clients should look up and cache the user at the /users endpoint.
 	LibreGraphUserType *string `json:"@libre.graph.userType,omitempty"`
 }
 

vendor/modules.txt

@@ -1628,7 +1628,7 @@ github.com/opentracing/opentracing-go/log
 # github.com/orcaman/concurrent-map v1.0.0
 ## explicit
 github.com/orcaman/concurrent-map
-# github.com/owncloud/libre-graph-api-go v1.0.5-0.20240816082515-afcf4367b966
+# github.com/owncloud/libre-graph-api-go v1.0.5-0.20240820135012-5fac8096ce9c
 ## explicit; go 1.18
 github.com/owncloud/libre-graph-api-go
 # github.com/oxtoacart/bpool v0.0.0-20190530202638-03653db5a59c