diff options
author | Joram Wilander <jwawilander@gmail.com> | 2018-04-20 08:44:18 -0400 |
---|---|---|
committer | GitHub <noreply@github.com> | 2018-04-20 08:44:18 -0400 |
commit | 283f34b9c6d207f0a103e7b4c7f6da2c7481c3ef (patch) | |
tree | 9a9d0dfb9f536d37e9817e3407c32e7ec0c11cdf | |
parent | 7987c95fcd7f7a9e6d4d174be403bf170f7b9115 (diff) | |
download | chat-283f34b9c6d207f0a103e7b4c7f6da2c7481c3ef.tar.gz chat-283f34b9c6d207f0a103e7b4c7f6da2c7481c3ef.tar.bz2 chat-283f34b9c6d207f0a103e7b4c7f6da2c7481c3ef.zip |
MM-10007 Send an admin and regular WS events when a user is updated (#8588)
* Add user.DeepCopy() function
* Add omit admins/non-admins to WS broadcast and use for updating users
* Updates per feedback and adding unit test for ShouldSendEvent
-rw-r--r-- | app/apptestlib.go | 10 | ||||
-rw-r--r-- | app/user.go | 9 | ||||
-rw-r--r-- | app/web_conn.go | 22 | ||||
-rw-r--r-- | app/web_conn_test.go | 82 | ||||
-rw-r--r-- | model/user.go | 17 | ||||
-rw-r--r-- | model/user_test.go | 32 | ||||
-rw-r--r-- | model/utils.go | 8 | ||||
-rw-r--r-- | model/utils_test.go | 12 | ||||
-rw-r--r-- | model/websocket_message.go | 10 |
9 files changed, 197 insertions, 5 deletions
diff --git a/app/apptestlib.go b/app/apptestlib.go index 1b22831c9..a5c2db91c 100644 --- a/app/apptestlib.go +++ b/app/apptestlib.go @@ -31,6 +31,8 @@ type TestHelper struct { BasicChannel *model.Channel BasicPost *model.Post + SystemAdminUser *model.User + tempConfigPath string tempWorkspace string pluginHooks map[string]plugin.Hooks @@ -143,6 +145,14 @@ func (me *TestHelper) InitBasic() *TestHelper { return me } +func (me *TestHelper) InitSystemAdmin() *TestHelper { + me.SystemAdminUser = me.CreateUser() + me.App.UpdateUserRoles(me.SystemAdminUser.Id, model.SYSTEM_USER_ROLE_ID+" "+model.SYSTEM_ADMIN_ROLE_ID, false) + me.SystemAdminUser, _ = me.App.GetUser(me.SystemAdminUser.Id) + + return me +} + func (me *TestHelper) MakeEmail() string { return "success_" + model.NewId() + "@simulator.amazonses.com" } diff --git a/app/user.go b/app/user.go index 80c8b6ef2..34076555e 100644 --- a/app/user.go +++ b/app/user.go @@ -1021,10 +1021,17 @@ func (a *App) UpdateUserAuth(userId string, userAuth *model.UserAuth) (*model.Us } func (a *App) sendUpdatedUserEvent(user model.User) { - a.SanitizeProfile(&user, false) + adminCopyOfUser := user.DeepCopy() + a.SanitizeProfile(adminCopyOfUser, true) + adminMessage := model.NewWebSocketEvent(model.WEBSOCKET_EVENT_USER_UPDATED, "", "", "", nil) + adminMessage.Add("user", *adminCopyOfUser) + adminMessage.Broadcast.ContainsSensitiveData = true + a.Publish(adminMessage) + a.SanitizeProfile(&user, false) message := model.NewWebSocketEvent(model.WEBSOCKET_EVENT_USER_UPDATED, "", "", "", nil) message.Add("user", user) + message.Broadcast.ContainsSanitizedData = true a.Publish(message) } diff --git a/app/web_conn.go b/app/web_conn.go index 33c285af3..9ae5505b2 100644 --- a/app/web_conn.go +++ b/app/web_conn.go @@ -287,6 +287,28 @@ func (webCon *WebConn) ShouldSendEvent(msg *model.WebSocketEvent) bool { return false } + // If the event contains sanitized data, only send to users that don't have permission to + // see sensitive data. Prevents admin clients from receiving events with bad data + var hasReadPrivateDataPermission *bool + if msg.Broadcast.ContainsSanitizedData { + hasReadPrivateDataPermission = model.NewBool(webCon.App.RolesGrantPermission(webCon.GetSession().GetUserRoles(), model.PERMISSION_MANAGE_SYSTEM.Id)) + + if *hasReadPrivateDataPermission { + return false + } + } + + // If the event contains sensitive data, only send to users with permission to see it + if msg.Broadcast.ContainsSensitiveData { + if hasReadPrivateDataPermission == nil { + hasReadPrivateDataPermission = model.NewBool(webCon.App.RolesGrantPermission(webCon.GetSession().GetUserRoles(), model.PERMISSION_MANAGE_SYSTEM.Id)) + } + + if !*hasReadPrivateDataPermission { + return false + } + } + // If the event is destined to a specific user if len(msg.Broadcast.UserId) > 0 { if webCon.UserId == msg.Broadcast.UserId { diff --git a/app/web_conn_test.go b/app/web_conn_test.go new file mode 100644 index 000000000..c378c8370 --- /dev/null +++ b/app/web_conn_test.go @@ -0,0 +1,82 @@ +// Copyright (c) 2016-present Mattermost, Inc. All Rights Reserved. +// See License.txt for license information. + +package app + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/mattermost/mattermost-server/model" + "github.com/mattermost/mattermost-server/utils" +) + +func TestWebConnShouldSendEvent(t *testing.T) { + th := Setup().InitBasic().InitSystemAdmin() + defer th.TearDown() + + session, err := th.App.CreateSession(&model.Session{UserId: th.BasicUser.Id, Roles: th.BasicUser.GetRawRoles()}) + require.Nil(t, err) + + basicUserWc := &WebConn{ + App: th.App, + UserId: th.BasicUser.Id, + T: utils.T, + } + + basicUserWc.SetSession(session) + basicUserWc.SetSessionToken(session.Token) + basicUserWc.SetSessionExpiresAt(session.ExpiresAt) + + session2, err := th.App.CreateSession(&model.Session{UserId: th.BasicUser2.Id, Roles: th.BasicUser2.GetRawRoles()}) + require.Nil(t, err) + + basicUser2Wc := &WebConn{ + App: th.App, + UserId: th.BasicUser2.Id, + T: utils.T, + } + + basicUser2Wc.SetSession(session2) + basicUser2Wc.SetSessionToken(session2.Token) + basicUser2Wc.SetSessionExpiresAt(session2.ExpiresAt) + + session3, err := th.App.CreateSession(&model.Session{UserId: th.SystemAdminUser.Id, Roles: th.SystemAdminUser.GetRawRoles()}) + require.Nil(t, err) + + adminUserWc := &WebConn{ + App: th.App, + UserId: th.SystemAdminUser.Id, + T: utils.T, + } + + adminUserWc.SetSession(session3) + adminUserWc.SetSessionToken(session3.Token) + adminUserWc.SetSessionExpiresAt(session3.ExpiresAt) + + cases := []struct { + Description string + Broadcast *model.WebsocketBroadcast + User1Expected bool + User2Expected bool + AdminExpected bool + }{ + {"should send to all", &model.WebsocketBroadcast{}, true, true, true}, + {"should only send to basic user", &model.WebsocketBroadcast{UserId: th.BasicUser.Id}, true, false, false}, + {"should omit basic user 2", &model.WebsocketBroadcast{OmitUsers: map[string]bool{th.BasicUser2.Id: true}}, true, false, true}, + {"should only send to admin", &model.WebsocketBroadcast{ContainsSensitiveData: true}, false, false, true}, + {"should only send to non-admins", &model.WebsocketBroadcast{ContainsSanitizedData: true}, true, true, false}, + {"should send to nobody", &model.WebsocketBroadcast{ContainsSensitiveData: true, ContainsSanitizedData: true}, false, false, false}, + // needs more cases to get full coverage + } + + event := &model.WebSocketEvent{Event: "some_event"} + for _, c := range cases { + event.Broadcast = c.Broadcast + assert.Equal(t, c.User1Expected, basicUserWc.ShouldSendEvent(event), c.Description) + assert.Equal(t, c.User2Expected, basicUser2Wc.ShouldSendEvent(event), c.Description) + assert.Equal(t, c.AdminExpected, adminUserWc.ShouldSendEvent(event), c.Description) + } +} diff --git a/model/user.go b/model/user.go index efe8f8db9..6fbdb09e8 100644 --- a/model/user.go +++ b/model/user.go @@ -96,6 +96,23 @@ type UserAuth struct { AuthService string `json:"auth_service,omitempty"` } +func (u *User) DeepCopy() *User { + copyUser := *u + if u.AuthData != nil { + copyUser.AuthData = NewString(*u.AuthData) + } + if u.Props != nil { + copyUser.Props = CopyStringMap(u.Props) + } + if u.NotifyProps != nil { + copyUser.NotifyProps = CopyStringMap(u.NotifyProps) + } + if u.Timezone != nil { + copyUser.Timezone = CopyStringMap(u.Timezone) + } + return ©User +} + // IsValid validates the user and returns an error if it isn't configured // correctly. func (u *User) IsValid() *AppError { diff --git a/model/user_test.go b/model/user_test.go index c1d1dafbd..645eaadff 100644 --- a/model/user_test.go +++ b/model/user_test.go @@ -8,6 +8,8 @@ import ( "net/http" "strings" "testing" + + "github.com/stretchr/testify/assert" ) func TestPasswordHash(t *testing.T) { @@ -22,6 +24,36 @@ func TestPasswordHash(t *testing.T) { } } +func TestUserDeepCopy(t *testing.T) { + id := NewId() + authData := "authdata" + mapKey := "key" + mapValue := "key" + + user := &User{Id: id, AuthData: NewString(authData), Props: map[string]string{}, NotifyProps: map[string]string{}, Timezone: map[string]string{}} + user.Props[mapKey] = mapValue + user.NotifyProps[mapKey] = mapValue + user.Timezone[mapKey] = mapValue + + copyUser := user.DeepCopy() + copyUser.Id = "someid" + *copyUser.AuthData = "changed" + copyUser.Props[mapKey] = "changed" + copyUser.NotifyProps[mapKey] = "changed" + copyUser.Timezone[mapKey] = "changed" + + assert.Equal(t, id, user.Id) + assert.Equal(t, authData, *user.AuthData) + assert.Equal(t, mapValue, user.Props[mapKey]) + assert.Equal(t, mapValue, user.NotifyProps[mapKey]) + assert.Equal(t, mapValue, user.Timezone[mapKey]) + + user = &User{Id: id} + copyUser = user.DeepCopy() + + assert.Equal(t, id, user.Id) +} + func TestUserJson(t *testing.T) { user := User{Id: NewId(), Username: NewId()} json := user.ToJson() diff --git a/model/utils.go b/model/utils.go index 2d61b49f6..9f8ef90e1 100644 --- a/model/utils.go +++ b/model/utils.go @@ -148,6 +148,14 @@ func GetMillis() int64 { return time.Now().UnixNano() / int64(time.Millisecond) } +func CopyStringMap(originalMap map[string]string) map[string]string { + copyMap := make(map[string]string) + for k, v := range originalMap { + copyMap[k] = v + } + return copyMap +} + // MapToJson converts a map to a json string func MapToJson(objmap map[string]string) string { b, _ := json.Marshal(objmap) diff --git a/model/utils_test.go b/model/utils_test.go index 92354c0a1..b7f5dc628 100644 --- a/model/utils_test.go +++ b/model/utils_test.go @@ -8,6 +8,7 @@ import ( "strings" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -47,6 +48,17 @@ func TestAppErrorJunk(t *testing.T) { } } +func TestCopyStringMap(t *testing.T) { + itemKey := "item1" + originalMap := make(map[string]string) + originalMap[itemKey] = "val1" + + copyMap := CopyStringMap(originalMap) + copyMap[itemKey] = "changed" + + assert.Equal(t, "val1", originalMap[itemKey]) +} + func TestMapJson(t *testing.T) { m := make(map[string]string) diff --git a/model/websocket_message.go b/model/websocket_message.go index 4a547bb6a..08c238480 100644 --- a/model/websocket_message.go +++ b/model/websocket_message.go @@ -57,10 +57,12 @@ type WebSocketMessage interface { } type WebsocketBroadcast struct { - OmitUsers map[string]bool `json:"omit_users"` // broadcast is omitted for users listed here - UserId string `json:"user_id"` // broadcast only occurs for this user - ChannelId string `json:"channel_id"` // broadcast only occurs for users in this channel - TeamId string `json:"team_id"` // broadcast only occurs for users in this team + OmitUsers map[string]bool `json:"omit_users"` // broadcast is omitted for users listed here + UserId string `json:"user_id"` // broadcast only occurs for this user + ChannelId string `json:"channel_id"` // broadcast only occurs for users in this channel + TeamId string `json:"team_id"` // broadcast only occurs for users in this team + ContainsSanitizedData bool `json:"-"` + ContainsSensitiveData bool `json:"-"` } type precomputedWebSocketEventJSON struct { |