From d8b94836cfa28f1c323b6cc10f0f7ee90be778ce Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Wed, 10 Feb 2021 23:41:34 +0530 Subject: [PATCH] MM-32471: Use created user to update roles (#16895) During user creation via CLI, we would create the user, but pass the user ID instead when updating the roles. This falls into the category of read-after-write within a single request. We fix this by passing the already created user and directly update the roles. https://mattermost.atlassian.net/browse/MM-32471 ```release-note NONE ``` --- app/app_iface.go | 1 + app/opentracing/opentracing_layer.go | 22 ++++++++++++++++++++++ app/user.go | 7 ++++++- app/user_test.go | 20 ++++++++++++++++++++ cmd/mattermost/commands/user.go | 4 ++-- 5 files changed, 51 insertions(+), 3 deletions(-) diff --git a/app/app_iface.go b/app/app_iface.go index 4e96506893..407b89d106 100644 --- a/app/app_iface.go +++ b/app/app_iface.go @@ -1023,6 +1023,7 @@ type AppIface interface { UpdateUserAuth(userID string, userAuth *model.UserAuth) (*model.UserAuth, *model.AppError) UpdateUserNotifyProps(userID string, props map[string]string, sendNotifications bool) (*model.User, *model.AppError) UpdateUserRoles(userID string, newRoles string, sendWebSocketEvent bool) (*model.User, *model.AppError) + UpdateUserRolesWithUser(user *model.User, newRoles string, sendWebSocketEvent bool) (*model.User, *model.AppError) UploadData(us *model.UploadSession, rd io.Reader) (*model.FileInfo, *model.AppError) UploadEmojiImage(id string, imageData *multipart.FileHeader) *model.AppError UploadMultipartFiles(teamID string, channelId string, userID string, fileHeaders []*multipart.FileHeader, clientIds []string, now time.Time) (*model.FileUploadResponse, *model.AppError) diff --git a/app/opentracing/opentracing_layer.go b/app/opentracing/opentracing_layer.go index 27df8c0573..086fb828ae 100644 --- a/app/opentracing/opentracing_layer.go +++ b/app/opentracing/opentracing_layer.go @@ -15732,6 +15732,28 @@ func (a *OpenTracingAppLayer) UpdateUserRoles(userID string, newRoles string, se return resultVar0, resultVar1 } +func (a *OpenTracingAppLayer) UpdateUserRolesWithUser(user *model.User, newRoles string, sendWebSocketEvent bool) (*model.User, *model.AppError) { + origCtx := a.ctx + span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.UpdateUserRolesWithUser") + + a.ctx = newCtx + a.app.Srv().Store.SetContext(newCtx) + defer func() { + a.app.Srv().Store.SetContext(origCtx) + a.ctx = origCtx + }() + + defer span.Finish() + resultVar0, resultVar1 := a.app.UpdateUserRolesWithUser(user, newRoles, sendWebSocketEvent) + + if resultVar1 != nil { + span.LogFields(spanlog.Error(resultVar1)) + ext.Error.Set(span, true) + } + + return resultVar0, resultVar1 +} + func (a *OpenTracingAppLayer) UpdateViewedProductNotices(userID string, noticeIds []string) *model.AppError { origCtx := a.ctx span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.UpdateViewedProductNotices") diff --git a/app/user.go b/app/user.go index 8c553115d2..c318730f77 100644 --- a/app/user.go +++ b/app/user.go @@ -1547,6 +1547,11 @@ func (a *App) UpdateUserRoles(userID string, newRoles string, sendWebSocketEvent return nil, err } + return a.UpdateUserRolesWithUser(user, newRoles, sendWebSocketEvent) +} + +func (a *App) UpdateUserRolesWithUser(user *model.User, newRoles string, sendWebSocketEvent bool) (*model.User, *model.AppError) { + if err := a.CheckRolesExist(strings.Fields(newRoles)); err != nil { return nil, err } @@ -1586,7 +1591,7 @@ func (a *App) UpdateUserRoles(userID string, newRoles string, sendWebSocketEvent mlog.Warn("Failed during updating user roles", mlog.Err(result.NErr)) } - a.InvalidateCacheForUser(userID) + a.InvalidateCacheForUser(user.Id) a.ClearSessionCacheForUser(user.Id) if sendWebSocketEvent { diff --git a/app/user_test.go b/app/user_test.go index dfcd5c3ac8..c39d89e8f6 100644 --- a/app/user_test.go +++ b/app/user_test.go @@ -1374,3 +1374,23 @@ func TestDeactivateGuests(t *testing.T) { assert.Nil(t, err) assert.Equal(t, int64(0), user.DeleteAt) } + +func TestUpdateUserRolesWithUser(t *testing.T) { + // InitBasic is used to let the first CreateUser call not be + // a system_admin + th := Setup(t).InitBasic() + defer th.TearDown() + + // Create normal user. + user := th.CreateUser() + assert.Equal(t, user.Roles, model.SYSTEM_USER_ROLE_ID) + + // Upgrade to sysadmin. + user, err := th.App.UpdateUserRolesWithUser(user, model.SYSTEM_USER_ROLE_ID+" "+model.SYSTEM_ADMIN_ROLE_ID, false) + require.Nil(t, err) + assert.Equal(t, user.Roles, model.SYSTEM_USER_ROLE_ID+" "+model.SYSTEM_ADMIN_ROLE_ID) + + // Test bad role. + _, err = th.App.UpdateUserRolesWithUser(user, "does not exist", false) + require.NotNil(t, err) +} diff --git a/cmd/mattermost/commands/user.go b/cmd/mattermost/commands/user.go index 30fdb7ca5d..e45bbbe7a9 100644 --- a/cmd/mattermost/commands/user.go +++ b/cmd/mattermost/commands/user.go @@ -377,13 +377,13 @@ func userCreateCmdF(command *cobra.Command, args []string) error { } if systemAdmin { - if _, err := a.UpdateUserRoles(ruser.Id, "system_user system_admin", false); err != nil { + if _, err := a.UpdateUserRolesWithUser(ruser, "system_user system_admin", false); err != nil { return errors.New("Unable to make user system admin. Error: " + err.Error()) } } else { // This else case exists to prevent the first user created from being // created as a system admin unless explicitly specified. - if _, err := a.UpdateUserRoles(ruser.Id, "system_user", false); err != nil { + if _, err := a.UpdateUserRolesWithUser(ruser, "system_user", false); err != nil { return errors.New("If this is the first user: Unable to prevent user from being system admin. Error: " + err.Error()) } }