From 8476da2f7ba815f0e3708296a3e055307d2a4839 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Tue, 20 Aug 2024 15:58:31 +0200 Subject: [PATCH 1/2] fix(graph): treat memberOf and userType as read-only attributes Fixes: #9858 --- .../fix-graph-readonly-attributes.md | 7 ++ services/graph/pkg/service/v0/users.go | 48 ++++++---- services/graph/pkg/service/v0/users_test.go | 93 +++++++++---------- 3 files changed, 83 insertions(+), 65 deletions(-) create mode 100644 changelog/unreleased/fix-graph-readonly-attributes.md diff --git a/changelog/unreleased/fix-graph-readonly-attributes.md b/changelog/unreleased/fix-graph-readonly-attributes.md new file mode 100644 index 00000000000..aa170e39b2d --- /dev/null +++ b/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 diff --git a/services/graph/pkg/service/v0/users.go b/services/graph/pkg/service/v0/users.go index 0b942a819d1..3bdc88ccc77 100644 --- a/services/graph/pkg/service/v0/users.go +++ b/services/graph/pkg/service/v0/users.go @@ -368,21 +368,26 @@ 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 } + // 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() { - 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") + 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 { @@ -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) diff --git a/services/graph/pkg/service/v0/users_test.go b/services/graph/pkg/service/v0/users_test.go index ac7ae6958b9..9d3dbb84c89 100644 --- a/services/graph/pkg/service/v0/users_test.go +++ b/services/graph/pkg/service/v0/users_test.go @@ -829,42 +829,25 @@ 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("creates a 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) - 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("Member")) + It("handles set userType - it's read-only", func() { + user.SetUserType("Member") + assertHandleBadAttributes(user) }) - It("creates a guest user", func() { + It("creates a 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()) @@ -879,7 +862,7 @@ var _ = Describe("Users", func() { createdUser := libregraph.User{} err = json.Unmarshal(data, &createdUser) Expect(err).ToNot(HaveOccurred()) - Expect(createdUser.GetUserType()).To(Equal("Guest")) + Expect(createdUser.GetUserType()).To(Equal("Member")) }) It("creates a member user", func() { @@ -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,22 +1056,26 @@ var _ = Describe("Users", func() { Expect(rr.Code).To(Equal(http.StatusBadRequest)) }) - It("handles invalid email", func() { - userUpdate.SetMail("invalid") - data, err := json.Marshal(userUpdate) - Expect(err).ToNot(HaveOccurred()) + It("handles set Ids - they are read-only", func() { + userUpdate.SetId("/users/user") + assertHandleBadAttributes(user.GetId(), userUpdate) + }) - 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) + 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) + }) - Expect(rr.Code).To(Equal(http.StatusBadRequest)) + It("handles set userType - it's read-only", func() { + userUpdate.SetUserType("Member") + assertHandleBadAttributes(user.GetId(), userUpdate) }) - It("handles invalid userType", func() { - userUpdate.SetUserType("Clown") + It("handles invalid email", func() { + userUpdate.SetMail("invalid") data, err := json.Marshal(userUpdate) Expect(err).ToNot(HaveOccurred()) @@ -1092,12 +1089,12 @@ var _ = Describe("Users", func() { }) 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")) }) }) From f1a02732aea4da0fc8ae0f9f0d39372a8d4c9414 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Tue, 20 Aug 2024 16:16:02 +0200 Subject: [PATCH 2/2] Bump libregraph to latest --- go.mod | 2 +- go.sum | 4 ++-- .../github.com/owncloud/libre-graph-api-go/model_identity.go | 2 +- vendor/modules.txt | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/go.mod b/go.mod index ba3fdfab21f..88a56f0ea4c 100644 --- a/go.mod +++ b/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.19.1 diff --git a/go.sum b/go.sum index 247756aaf28..5a15d75d909 100644 --- a/go.sum +++ b/go.sum @@ -936,8 +936,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= diff --git a/vendor/github.com/owncloud/libre-graph-api-go/model_identity.go b/vendor/github.com/owncloud/libre-graph-api-go/model_identity.go index 7108355b2f2..330a65e5912 100644 --- a/vendor/github.com/owncloud/libre-graph-api-go/model_identity.go +++ b/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"` } diff --git a/vendor/modules.txt b/vendor/modules.txt index 1adeec353b3..03d618bdb90 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -1625,7 +1625,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