MM-53467 Unrevert changes that update current user object (#23928)

* Revert "Revert "[MM-52547] Include current user profile in every redux action (#23219)""

This reverts commit 8f96888b1a.

* Revert "Revert "[MM-52546] webapp/channels : Update current user and status on WebSocket reconnect (#23071)""

This reverts commit 69ee162a6e.

* MM-53647 Fix overwriting the current user with sanitized data
This commit is contained in:
Harrison Healey 2023-07-11 12:51:02 -04:00 committed by GitHub
parent 116728424c
commit 2672af30ea
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 496 additions and 68 deletions

View File

@ -0,0 +1,94 @@
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
// See LICENSE.txt for license information.
// ***************************************************************
// - [#] indicates a test step (e.g. #. Go to a page)
// - [*] indicates an assertion (e.g. * Check the title)
// - Use element ID when selecting an element. Create one if none.
// ***************************************************************
// Group: @channels
describe('MM-53377 Regression tests', () => {
let testTeam;
let testUser;
let testUser2;
before(() => {
cy.apiUpdateConfig({
PrivacySettings: {
ShowEmailAddress: false,
ShowFullName: false,
},
});
cy.apiInitSetup().then(({team, user, offTopicUrl}) => {
testTeam = team;
testUser = user;
cy.apiCreateUser().then((payload) => {
testUser2 = payload.user;
cy.apiAddUserToTeam(testTeam.id, payload.user.id);
});
cy.visit(offTopicUrl);
});
});
beforeEach(() => {
// # Login as testUser
cy.apiLogin(testUser);
});
it('should still have your email loaded after using the at-mention autocomplete', () => {
// * Ensure that this user is not an admin
cy.wrap(testUser).its('roles').should('equal', 'system_user');
// # Send a couple at mentions, quickly enough that the at mention autocomplete won't appear
cy.uiPostMessageQuickly(`@${testUser.username} @${testUser2.username}`);
// # Open the profile popover for the current user
cy.contains('.mention-link', `@${testUser.username}`).click();
// * Ensure that all fields are visible for the current user
cy.get('#user-profile-popover').within(() => {
cy.findByText(`@${testUser.username}`).should('exist');
cy.findByText(`${testUser.first_name} ${testUser.last_name}`).should('exist');
cy.findByText(testUser.email).should('exist');
});
// # Click anywhere to close profile popover
cy.get('#channelHeaderInfo').click();
// # Open the profile popover for another user
cy.contains('.mention-link', `@${testUser2.username}`).click();
// * Ensure that only the username is visible for another user
cy.get('#user-profile-popover').within(() => {
cy.findByText(`@${testUser2.username}`).should('exist');
cy.findByText(`${testUser2.first_name} ${testUser2.last_name}`).should('not.exist');
cy.findByText(testUser2.email).should('not.exist');
});
// # Start to type another at mention so that the autocomplete loads
cy.get('#post_textbox').type(`@${testUser.username}`);
// # Wait for the autocomplete to appear with the current user in it
cy.get('.suggestion-list').within(() => {
cy.findByText(`@${testUser.username}`);
});
// # Clear the post textbox to hide the autocomplete
cy.get('#post_textbox').clear();
// # Open the profile popover for the current user again
cy.contains('.mention-link', `@${testUser.username}`).click();
// * Ensure that all fields are still visible for the current user
cy.get('#user-profile-popover').within(() => {
cy.findByText(`@${testUser.username}`).should('exist');
cy.findByText(`${testUser.first_name} ${testUser.last_name}`).should('exist');
cy.findByText(testUser.email).should('exist');
});
});
});

View File

@ -234,7 +234,7 @@ export function reconnect() {
// we can request for getPosts again when socket is connected
dispatch(getPosts(currentChannelId));
}
StatusActions.loadStatusesForChannelAndSidebar();
dispatch(StatusActions.loadStatusesForChannelAndSidebar());
const crtEnabled = isCollapsedThreadsEnabled(state);
dispatch(TeamActions.getMyTeamUnreads(crtEnabled, true));

View File

@ -35,7 +35,6 @@ import {getServerVersion} from 'mattermost-redux/selectors/entities/general';
import {getCurrentUserId, getUsers} from 'mattermost-redux/selectors/entities/users';
import {isCollapsedThreadsEnabled} from 'mattermost-redux/selectors/entities/preferences';
import {removeUserFromList} from 'mattermost-redux/utils/user_utils';
import {isMinimumServerVersion} from 'mattermost-redux/utils/helpers';
import {General} from 'mattermost-redux/constants';
@ -215,12 +214,10 @@ export function getFilteredUsersStats(options: GetFilteredUsersStatsOpts = {}, u
export function getProfiles(page = 0, perPage: number = General.PROFILE_CHUNK_SIZE, options: any = {}): ActionFunc {
return async (dispatch: DispatchFunc, getState: GetStateFunc) => {
const {currentUserId} = getState().entities.users;
let profiles: UserProfile[];
try {
profiles = await Client4.getProfiles(page, perPage, options);
removeUserFromList(currentUserId, profiles);
} catch (error) {
forceLogoutIfNecessary(error, dispatch, getState);
dispatch(logError(error));
@ -280,12 +277,10 @@ export function getMissingProfilesByUsernames(usernames: string[]): ActionFunc {
export function getProfilesByIds(userIds: string[], options?: any): ActionFunc {
return async (dispatch: DispatchFunc, getState: GetStateFunc) => {
const {currentUserId} = getState().entities.users;
let profiles: UserProfile[];
try {
profiles = await Client4.getProfilesByIds(userIds, options);
removeUserFromList(currentUserId, profiles);
} catch (error) {
forceLogoutIfNecessary(error, dispatch, getState);
dispatch(logError(error));
@ -303,12 +298,10 @@ export function getProfilesByIds(userIds: string[], options?: any): ActionFunc {
export function getProfilesByUsernames(usernames: string[]): ActionFunc {
return async (dispatch: DispatchFunc, getState: GetStateFunc) => {
const {currentUserId} = getState().entities.users;
let profiles;
try {
profiles = await Client4.getProfilesByUsernames(usernames);
removeUserFromList(currentUserId, profiles);
} catch (error) {
forceLogoutIfNecessary(error, dispatch, getState);
dispatch(logError(error));
@ -326,7 +319,6 @@ export function getProfilesByUsernames(usernames: string[]): ActionFunc {
export function getProfilesInTeam(teamId: string, page: number, perPage: number = General.PROFILE_CHUNK_SIZE, sort = '', options: any = {}): ActionFunc {
return async (dispatch: DispatchFunc, getState: GetStateFunc) => {
const {currentUserId} = getState().entities.users;
let profiles;
try {
@ -345,7 +337,7 @@ export function getProfilesInTeam(teamId: string, page: number, perPage: number
},
{
type: UserTypes.RECEIVED_PROFILES_LIST,
data: removeUserFromList(currentUserId, [...profiles]),
data: profiles,
},
]));
@ -415,7 +407,6 @@ export enum ProfilesInChannelSortBy {
export function getProfilesInChannel(channelId: string, page: number, perPage: number = General.PROFILE_CHUNK_SIZE, sort = '', options: {active?: boolean} = {}): ActionFunc {
return async (dispatch: DispatchFunc, getState: GetStateFunc) => {
const {currentUserId} = getState().entities.users;
let profiles;
try {
@ -434,7 +425,7 @@ export function getProfilesInChannel(channelId: string, page: number, perPage: n
},
{
type: UserTypes.RECEIVED_PROFILES_LIST,
data: removeUserFromList(currentUserId, [...profiles]),
data: profiles,
},
]));
@ -444,7 +435,6 @@ export function getProfilesInChannel(channelId: string, page: number, perPage: n
export function getProfilesInGroupChannels(channelsIds: string[]): ActionFunc {
return async (dispatch: DispatchFunc, getState: GetStateFunc) => {
const {currentUserId} = getState().entities.users;
let channelProfiles;
try {
@ -468,7 +458,7 @@ export function getProfilesInGroupChannels(channelsIds: string[]): ActionFunc {
},
{
type: UserTypes.RECEIVED_PROFILES_LIST,
data: removeUserFromList(currentUserId, [...profiles]),
data: profiles,
},
);
}
@ -482,7 +472,6 @@ export function getProfilesInGroupChannels(channelsIds: string[]): ActionFunc {
export function getProfilesNotInChannel(teamId: string, channelId: string, groupConstrained: boolean, page: number, perPage: number = General.PROFILE_CHUNK_SIZE): ActionFunc {
return async (dispatch: DispatchFunc, getState: GetStateFunc) => {
const {currentUserId} = getState().entities.users;
let profiles;
try {
@ -503,7 +492,7 @@ export function getProfilesNotInChannel(teamId: string, channelId: string, group
},
{
type: UserTypes.RECEIVED_PROFILES_LIST,
data: removeUserFromList(currentUserId, [...profiles]),
data: profiles,
},
]));
@ -564,7 +553,6 @@ export function updateMyTermsOfServiceStatus(termsOfServiceId: string, accepted:
export function getProfilesInGroup(groupId: string, page = 0, perPage: number = General.PROFILE_CHUNK_SIZE, sort = ''): ActionFunc {
return async (dispatch: DispatchFunc, getState: GetStateFunc) => {
const {currentUserId} = getState().entities.users;
let profiles;
try {
@ -583,7 +571,7 @@ export function getProfilesInGroup(groupId: string, page = 0, perPage: number =
},
{
type: UserTypes.RECEIVED_PROFILES_LIST,
data: removeUserFromList(currentUserId, [...profiles]),
data: profiles,
},
]));
@ -593,7 +581,6 @@ export function getProfilesInGroup(groupId: string, page = 0, perPage: number =
export function getProfilesNotInGroup(groupId: string, page = 0, perPage: number = General.PROFILE_CHUNK_SIZE): ActionFunc {
return async (dispatch: DispatchFunc, getState: GetStateFunc) => {
const {currentUserId} = getState().entities.users;
let profiles;
try {
@ -612,7 +599,7 @@ export function getProfilesNotInGroup(groupId: string, page = 0, perPage: number
},
{
type: UserTypes.RECEIVED_PROFILES_LIST,
data: removeUserFromList(currentUserId, [...profiles]),
data: profiles,
},
]));
@ -844,9 +831,6 @@ export function autocompleteUsers(term: string, teamId = '', channelId = '', opt
}): ActionFunc {
return async (dispatch: DispatchFunc, getState: GetStateFunc) => {
dispatch({type: UserTypes.AUTOCOMPLETE_USERS_REQUEST, data: null});
const {currentUserId} = getState().entities.users;
let data;
try {
data = await Client4.autocompleteUsers(term, teamId, channelId, options);
@ -861,7 +845,6 @@ export function autocompleteUsers(term: string, teamId = '', channelId = '', opt
if (data.out_of_channel) {
users = [...users, ...data.out_of_channel];
}
removeUserFromList(currentUserId, users);
const actions: AnyAction[] = [{
type: UserTypes.RECEIVED_PROFILES_LIST,
data: users,
@ -904,8 +887,6 @@ export function autocompleteUsers(term: string, teamId = '', channelId = '', opt
export function searchProfiles(term: string, options: any = {}): ActionFunc {
return async (dispatch: DispatchFunc, getState: GetStateFunc) => {
const {currentUserId} = getState().entities.users;
let profiles;
try {
profiles = await Client4.searchUsers(term, options);
@ -915,7 +896,7 @@ export function searchProfiles(term: string, options: any = {}): ActionFunc {
return {error};
}
const actions: AnyAction[] = [{type: UserTypes.RECEIVED_PROFILES_LIST, data: removeUserFromList(currentUserId, [...profiles])}];
const actions: AnyAction[] = [{type: UserTypes.RECEIVED_PROFILES_LIST, data: profiles}];
if (options.in_channel_id) {
actions.push({

View File

@ -1,9 +1,16 @@
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
// See LICENSE.txt for license information.
import {UserProfile} from '@mattermost/types/users';
import {IDMappedObjects} from '@mattermost/types/utilities';
import {UserTypes, ChannelTypes} from 'mattermost-redux/action_types';
import {GenericAction} from 'mattermost-redux/types/actions';
import reducer from 'mattermost-redux/reducers/entities/users';
import deepFreezeAndThrowOnMutation from 'mattermost-redux/utils/deep_freeze';
import {TestHelper} from 'utils/test_helper';
type ReducerState = ReturnType<typeof reducer>;
describe('Reducers.users', () => {
@ -673,4 +680,331 @@ describe('Reducers.users', () => {
expect(newState.profilesNotInGroup).toEqual(expectedState.profilesNotInGroup);
});
});
describe('profiles', () => {
function sanitizeUser(user: UserProfile) {
const sanitized = {
...user,
email: '',
first_name: '',
last_name: '',
auth_service: '',
};
Reflect.deleteProperty(sanitized, 'email_verify');
Reflect.deleteProperty(sanitized, 'last_password_update');
Reflect.deleteProperty(sanitized, 'notify_props');
Reflect.deleteProperty(sanitized, 'terms_of_service_id');
Reflect.deleteProperty(sanitized, 'terms_of_service_create_at');
return sanitized;
}
for (const actionType of [UserTypes.RECEIVED_ME, UserTypes.RECEIVED_PROFILE]) {
test(`should store a new user (${actionType})`, () => {
const user1 = TestHelper.getUserMock({id: 'user_id1'});
const user2 = TestHelper.getUserMock({id: 'user_id2'});
const state = deepFreezeAndThrowOnMutation({
profiles: {
[user1.id]: user1,
},
});
const nextState = reducer(state, {
type: actionType,
data: user2,
});
expect(nextState.profiles).toEqual({
[user1.id]: user1,
[user2.id]: user2,
});
});
test(`should update an existing user (${actionType})`, () => {
const user1 = TestHelper.getUserMock({id: 'user_id1'});
const state = deepFreezeAndThrowOnMutation({
profiles: {
[user1.id]: user1,
},
});
const nextState = reducer(state, {
type: actionType,
data: {
...user1,
username: 'a different username',
},
});
expect(nextState.profiles).toEqual({
[user1.id]: {
...user1,
username: 'a different username',
},
});
});
test(`should not overwrite unsanitized data with sanitized data (${actionType})`, () => {
const user1 = TestHelper.getUserMock({
id: 'user_id1',
email: 'user1@example.com',
first_name: 'User',
last_name: 'One',
auth_service: 'saml',
});
const state = deepFreezeAndThrowOnMutation({
profiles: {
[user1.id]: user1,
},
});
const nextState = reducer(state, {
type: actionType,
data: {
...sanitizeUser(user1),
username: 'a different username',
},
});
expect(nextState.profiles).toEqual({
[user1.id]: {
...user1,
username: 'a different username',
},
});
expect(nextState.profiles[user1.id].email).toBe(user1.email);
expect(nextState.profiles[user1.id].auth_service).toBe(user1.auth_service);
});
test(`should return the same state when given an identical user object (${actionType})`, () => {
const user1 = TestHelper.getUserMock({id: 'user_id1'});
const state = deepFreezeAndThrowOnMutation({
profiles: {
[user1.id]: user1,
},
});
const nextState = reducer(state, {
type: actionType,
data: user1,
});
expect(nextState.profiles).toBe(state.profiles);
});
test(`should return the same state when given an sanitized but otherwise identical user object (${actionType})`, () => {
const user1 = TestHelper.getUserMock({
id: 'user_id1',
email: 'user1@example.com',
first_name: 'User',
last_name: 'One',
auth_service: 'saml',
});
const state = deepFreezeAndThrowOnMutation({
profiles: {
[user1.id]: user1,
},
});
const nextState = reducer(state, {
type: actionType,
data: sanitizeUser(user1),
});
expect(nextState.profiles).toBe(state.profiles);
});
}
for (const actionType of [UserTypes.RECEIVED_PROFILES, UserTypes.RECEIVED_PROFILES_LIST]) {
function usersToData(users: UserProfile[]) {
if (actionType === UserTypes.RECEIVED_PROFILES) {
const userMap: IDMappedObjects<UserProfile> = {};
for (const user of users) {
userMap[user.id] = user;
}
return userMap;
}
return users;
}
test(`should store new users (${actionType})`, () => {
const user1 = TestHelper.getUserMock({id: 'user_id1'});
const user2 = TestHelper.getUserMock({id: 'user_id2'});
const user3 = TestHelper.getUserMock({id: 'user_id3'});
const state = deepFreezeAndThrowOnMutation({
profiles: {
[user1.id]: user1,
},
});
const nextState = reducer(state, {
type: actionType,
data: usersToData([user2, user3]),
});
expect(nextState.profiles).toEqual({
[user1.id]: user1,
[user2.id]: user2,
[user3.id]: user3,
});
});
test(`should update existing users (${actionType})`, () => {
const user1 = TestHelper.getUserMock({id: 'user_id1'});
const user2 = TestHelper.getUserMock({id: 'user_id2'});
const user3 = TestHelper.getUserMock({id: 'user_id3'});
const state = deepFreezeAndThrowOnMutation({
profiles: {
[user1.id]: user1,
[user2.id]: user2,
[user3.id]: user3,
},
});
const newUser1 = {
...user1,
username: 'a different username',
};
const newUser2 = {
...user2,
nickname: 'a different nickname',
};
const nextState = reducer(state, {
type: actionType,
data: usersToData([newUser1, newUser2]),
});
expect(nextState.profiles).toEqual({
[user1.id]: newUser1,
[user2.id]: newUser2,
[user3.id]: user3,
});
});
test(`should not overwrite unsanitized data with sanitized data (${actionType})`, () => {
const user1 = TestHelper.getUserMock({
id: 'user_id1',
email: 'user1@example.com',
first_name: 'User',
last_name: 'One',
auth_service: 'saml',
});
const user2 = TestHelper.getUserMock({id: 'user_id2'});
const state = deepFreezeAndThrowOnMutation({
profiles: {
[user1.id]: user1,
},
});
const newUser1 = {
...sanitizeUser(user1),
username: 'a different username',
};
const newUser2 = {
...sanitizeUser(user2),
nickname: 'a different nickname',
};
const nextState = reducer(state, {
type: actionType,
data: usersToData([newUser1, newUser2]),
});
expect(nextState.profiles).toEqual({
[user1.id]: {
...user1,
username: 'a different username',
},
[user2.id]: newUser2,
});
expect(nextState.profiles[user1.id].email).toBe(user1.email);
expect(nextState.profiles[user1.id].auth_service).toBe(user1.auth_service);
});
test(`should return the same state when given identical user objects (${actionType})`, () => {
const user1 = TestHelper.getUserMock({id: 'user_id1'});
const user2 = TestHelper.getUserMock({id: 'user_id2'});
const state = deepFreezeAndThrowOnMutation({
profiles: {
[user1.id]: user1,
[user2.id]: user2,
},
});
const nextState = reducer(state, {
type: actionType,
data: usersToData([user1, user2]),
});
expect(nextState.profiles).toBe(state.profiles);
});
test(`should return the same state when given an sanitized but otherwise identical user object (${actionType})`, () => {
const user1 = TestHelper.getUserMock({
id: 'user_id1',
email: 'user1@example.com',
first_name: 'User',
last_name: 'One',
auth_service: 'saml',
});
const user2 = TestHelper.getUserMock({id: 'user_id2'});
const state = deepFreezeAndThrowOnMutation({
profiles: {
[user1.id]: user1,
[user2.id]: user2,
},
});
const nextState = reducer(state, {
type: actionType,
data: usersToData([sanitizeUser(user1), sanitizeUser(user2)]),
});
expect(nextState.profiles).toBe(state.profiles);
});
}
test('UserTypes.RECEIVED_PROFILES_LIST, should merge existing users with new ones', () => {
const firstUser = TestHelper.getUserMock({id: 'first_user_id'});
const secondUser = TestHelper.getUserMock({id: 'seocnd_user_id'});
const thirdUser = TestHelper.getUserMock({id: 'third_user_id'});
const partialUpdatedFirstUser = {
...firstUser,
update_at: 123456789,
};
Reflect.deleteProperty(partialUpdatedFirstUser, 'email');
Reflect.deleteProperty(partialUpdatedFirstUser, 'notify_props');
const state = {
profiles: {
first_user_id: firstUser,
second_user_id: secondUser,
},
};
const action = {
type: UserTypes.RECEIVED_PROFILES_LIST,
data: [
partialUpdatedFirstUser,
thirdUser,
],
};
const {profiles: newProfiles} = reducer(state as unknown as ReducerState, action);
expect(newProfiles.first_user_id).toEqual({...firstUser, ...partialUpdatedFirstUser});
expect(newProfiles.second_user_id).toEqual(secondUser);
expect(newProfiles.third_user_id).toEqual(thirdUser);
});
});
});

View File

@ -105,7 +105,7 @@ function removeProfileFromSet(state: RelationOneToMany<Team, UserProfile>, actio
function currentUserId(state = '', action: GenericAction) {
switch (action.type) {
case UserTypes.RECEIVED_ME: {
const data = action.data || action.payload;
const data = action.data;
return data.id;
}
@ -173,62 +173,81 @@ function myAudits(state = [], action: GenericAction) {
}
}
function receiveUserProfile(state: IDMappedObjects<UserProfile>, received: UserProfile) {
const existing = state[received.id];
if (!existing) {
// No existing data to merge with
return {
...state,
[received.id]: received,
};
}
const merged = {
...existing,
...received,
};
// MM-53377:
// For non-admin users, certain API responses don't return details for the current user that would be sanitized
// out for others. This currently includes:
// - email (if PrivacySettings.ShowEmailAddress is false)
// - first_name/last_name (if PrivacySettings.ShowFullName is false)
// - last_password_update
// - auth_service
// - notify_props
//
// Because email, first_name, last_name, and auth_service can all be empty strings regularly, we can't just
// merge the received user and the existing one together like we normally would. Instead, we can use the
// existence of existing.notify_props or existing.last_password_update to determine which object has that extra
// data so that it can take precedence. Those fields are:
// 1. Never empty or zero by Go standards
// 2. Only ever sent to the current user, not even to admins, so we know that the object contains privileged data
//
// Note that admins may have the email/name/auth_service of other users loaded as well. This does not prevent that
// data from being replaced when merging sanitized user objects. There doesn't seem to be a way for us to detect
// whether the object is sanitized for admins.
if (existing.notify_props && (!received.notify_props || Object.keys(received.notify_props).length === 0)) {
merged.email = existing.email;
merged.first_name = existing.first_name;
merged.last_name = existing.last_name;
merged.last_password_update = existing.last_password_update;
merged.auth_service = existing.auth_service;
merged.notify_props = existing.notify_props;
}
if (isEqual(existing, merged)) {
return state;
}
return {
...state,
[merged.id]: merged,
};
}
function profiles(state: IDMappedObjects<UserProfile> = {}, action: GenericAction) {
switch (action.type) {
case UserTypes.RECEIVED_ME:
case UserTypes.RECEIVED_PROFILE: {
const data = action.data || action.payload;
const user = {...data};
const oldUser = state[data.id];
if (oldUser) {
user.terms_of_service_id = oldUser.terms_of_service_id;
user.terms_of_service_create_at = oldUser.terms_of_service_create_at;
const user = action.data;
if (isEqual(user, oldUser)) {
return state;
}
}
return {
...state,
[data.id]: user,
};
return receiveUserProfile(state, user);
}
case UserTypes.RECEIVED_PROFILES_LIST: {
const users: UserProfile[] = action.data;
return users.reduce((nextState, user) => {
const oldUser = nextState[user.id];
if (oldUser && isEqual(user, oldUser)) {
return nextState;
}
return {
...nextState,
[user.id]: user,
};
}, state);
return users.reduce(receiveUserProfile, state);
}
case UserTypes.RECEIVED_PROFILES: {
const users: UserProfile[] = Object.values(action.data);
return users.reduce((nextState, user) => {
const oldUser = nextState[user.id];
if (oldUser && isEqual(user, oldUser)) {
return nextState;
}
return {
...nextState,
[user.id]: user,
};
}, state);
return users.reduce(receiveUserProfile, state);
}
case UserTypes.RECEIVED_TERMS_OF_SERVICE_STATUS: {
const data = action.data || action.payload;
const data = action.data;
return {
...state,
[data.user_id]: {