From 8476da2f7ba815f0e3708296a3e055307d2a4839 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Tue, 20 Aug 2024 15:58:31 +0200 Subject: [PATCH] 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 | 50 ++++++---- services/graph/pkg/service/v0/users_test.go | 97 +++++++++---------- 3 files changed, 86 insertions(+), 68 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 000000000..aa170e39b --- /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 0b942a819..3bdc88ccc 100644 --- a/services/graph/pkg/service/v0/users.go +++ b/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) diff --git a/services/graph/pkg/service/v0/users_test.go b/services/graph/pkg/service/v0/users_test.go index ac7ae6958..9d3dbb84c 100644 --- a/services/graph/pkg/service/v0/users_test.go +++ b/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")) }) })