MM-56625: Use patchConfig to update configs from system console (#26332)

While updating config from admin console, the webapp would set
any config param to null if the user doesn't have permission to edit
that setting.

This is common in cloud environments where a lot of config settings
are set to `cloud_restrictable`.

The problem due to that is since the client uses the updateConfig
endpoint, this acts as a full config replace and therefore, the null
fields get replaced by their default values.

To fix this, we use the patch endpoint which only updates the fields
that are actually set.

https://mattermost.atlassian.net/browse/MM-56625

```release-note
Fix a bug where config cannot be updated from admin console
in cloud environments.
```


Co-authored-by: Mattermost Build <build@mattermost.com>
This commit is contained in:
Agniva De Sarker 2024-03-08 21:21:23 +05:30 committed by GitHub
parent 1eec4ad30f
commit f34445a6f4
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
22 changed files with 67 additions and 52 deletions

View File

@ -77,6 +77,7 @@ exports[`components/admin_console/SchemaAdminSettings should match snapshot with
isCurrentUserSystemAdmin={false}
isDisabled={false}
license={Object {}}
patchConfig={[MockFunction]}
roles={Object {}}
schema={
Object {
@ -84,7 +85,6 @@ exports[`components/admin_console/SchemaAdminSettings should match snapshot with
}
}
setNavigationBlocked={[MockFunction]}
updateConfig={[MockFunction]}
/>
`;

View File

@ -8,6 +8,7 @@ import type {RouteComponentProps} from 'react-router-dom';
import type {CloudState} from '@mattermost/types/cloud';
import type {AdminConfig, ClientLicense, EnvironmentConfig} from '@mattermost/types/config';
import type {Role} from '@mattermost/types/roles';
import type {DeepPartial} from '@mattermost/types/utilities';
import type {ActionResult} from 'mattermost-redux/types/actions';
@ -43,7 +44,7 @@ type ExtraProps = {
setNavigationBlocked: (blocked: boolean) => void;
roles: Record<string, Role>;
editRole: (role: Role) => void;
updateConfig: (config: AdminConfig) => Promise<ActionResult>;
patchConfig: (config: DeepPartial<AdminConfig>) => Promise<ActionResult>;
cloud: CloudState;
isCurrentUserSystemAdmin: boolean;
}
@ -173,7 +174,7 @@ class AdminConsole extends React.PureComponent<Props, State> {
showNavigationPrompt,
roles,
} = this.props;
const {setNavigationBlocked, cancelNavigation, confirmNavigation, editRole, updateConfig} = this.props.actions;
const {setNavigationBlocked, cancelNavigation, confirmNavigation, editRole, patchConfig} = this.props.actions;
if (!this.props.currentUserHasAnAdminRole) {
return (
@ -203,7 +204,7 @@ class AdminConsole extends React.PureComponent<Props, State> {
setNavigationBlocked,
roles,
editRole,
updateConfig,
patchConfig,
cloud: this.props.cloud,
isCurrentUserSystemAdmin: this.props.isCurrentUserSystemAdmin,
};

View File

@ -19,7 +19,7 @@ export type BaseProps = {
environmentConfig?: EnvironmentConfig;
setNavigationBlocked?: (blocked: boolean) => void;
isDisabled?: boolean;
updateConfig?: (config: AdminConfig) => {data: AdminConfig; error: ClientErrorPlaceholder};
patchConfig?: (config: DeepPartial<AdminConfig>) => {data: AdminConfig; error: ClientErrorPlaceholder};
}
export type BaseState = {
@ -31,7 +31,7 @@ export type BaseState = {
}
// Placeholder type until ClientError is exported from redux.
// TODO: remove ClientErrorPlaceholder and change the return type of updateConfig
// TODO: remove ClientErrorPlaceholder and change the return type of patchConfig
type ClientErrorPlaceholder = {
message: string;
server_error_id: string;
@ -107,8 +107,8 @@ export default abstract class AdminSettings <Props extends BaseProps, State exte
let config = JSON.parse(JSON.stringify(this.props.config));
config = this.getConfigFromState(config);
if (this.props.updateConfig) {
const {data, error} = await this.props.updateConfig(config);
if (this.props.patchConfig) {
const {data, error} = await this.props.patchConfig(config);
if (data) {
this.setState(this.getStateFromConfig(data) as State);

View File

@ -137,7 +137,7 @@ describe('components/admin_console/CustomPluginSettings', () => {
{...baseProps}
config={config}
schema={{...plugin.settings_schema, id: plugin.id, name: plugin.name, settings}}
updateConfig={jest.fn()}
patchConfig={jest.fn()}
/>,
);
expect(wrapper).toMatchSnapshot();
@ -152,7 +152,7 @@ describe('components/admin_console/CustomPluginSettings', () => {
id: 'testplugin',
name: 'testplugin',
}}
updateConfig={jest.fn()}
patchConfig={jest.fn()}
/>,
);
expect(wrapper).toMatchSnapshot();
@ -171,7 +171,7 @@ describe('components/admin_console/CustomPluginSettings', () => {
} as PluginSettings,
}}
schema={{...plugin.settings_schema, id: plugin.id, name: plugin.name, settings}}
updateConfig={jest.fn()}
patchConfig={jest.fn()}
/>,
);
expect(wrapper).toMatchSnapshot();

View File

@ -25,7 +25,7 @@ describe('components/admin_console/CustomTermsOfServiceSettings', () => {
CustomTermsOfService: 'true',
},
setNavigationBlocked: jest.fn(),
updateConfig: jest.fn(),
patchConfig: jest.fn(),
};
test('should match snapshot', () => {

View File

@ -31,7 +31,7 @@ type Props = BaseProps & {
/*
* Action to save config file
*/
updateConfig: () => void;
patchConfig: () => void;
};
type State = BaseState & {
@ -120,7 +120,7 @@ export default class CustomTermsOfServiceSettings extends AdminSettings<Props, S
let config = JSON.parse(JSON.stringify(this.props.config));
config = this.getConfigFromState(config);
const {data, error} = await this.props.updateConfig(config);
const {data, error} = await this.props.patchConfig(config);
if (data) {
this.setState(this.getStateFromConfig(data));

View File

@ -29,7 +29,7 @@ describe('components/admin_console/data_retention_settings/data_retention_settin
createJob: jest.fn(),
getJobsByType: jest.fn().mockResolvedValue([]),
deleteDataRetentionCustomPolicy: jest.fn(),
updateConfig: jest.fn(),
patchConfig: jest.fn(),
},
};

View File

@ -44,7 +44,7 @@ type Props = {
createJob: (job: JobTypeBase) => Promise<ActionResult>;
getJobsByType: (job: JobType) => Promise<ActionResult>;
deleteDataRetentionCustomPolicy: (id: string) => Promise<ActionResult>;
updateConfig: (config: AdminConfig) => Promise<ActionResult>;
patchConfig: (config: DeepPartial<AdminConfig>) => Promise<ActionResult>;
};
} & WrappedComponentProps;
@ -437,7 +437,7 @@ class DataRetentionSettings extends React.PureComponent<Props, State> {
const newConfig = JSON.parse(JSON.stringify(this.props.config));
newConfig.DataRetentionSettings.DeletionJobStartTime = value;
await this.props.actions.updateConfig(newConfig);
await this.props.actions.patchConfig(newConfig);
this.inputRef.current?.blur();
};

View File

@ -21,7 +21,7 @@ describe('components/PluginManagement', () => {
fileRetentionHours: '2400',
environmentConfig: {},
actions: {
updateConfig: jest.fn(),
patchConfig: jest.fn(),
setNavigationBlocked: jest.fn(),
},
};

View File

@ -32,7 +32,7 @@ type Props = {
fileRetentionHours: string | undefined;
environmentConfig: Partial<EnvironmentConfig>;
actions: {
updateConfig: (config: AdminConfig) => Promise<ActionResult>;
patchConfig: (config: DeepPartial<AdminConfig>) => Promise<ActionResult>;
setNavigationBlocked: (blocked: boolean) => void;
};
};
@ -121,7 +121,7 @@ export default class GlobalPolicyForm extends React.PureComponent<Props, State>
newConfig.DataRetentionSettings.FileRetentionHours = this.setRetentionHours(fileRetentionDropdownValue.value, fileRetentionInputValue);
}
const {error} = await this.props.actions.updateConfig(newConfig);
const {error} = await this.props.actions.patchConfig(newConfig);
if (error) {
this.setState({serverError: error.message, saving: false});

View File

@ -6,7 +6,7 @@ import {bindActionCreators} from 'redux';
import type {Dispatch} from 'redux';
import {
updateConfig,
patchConfig,
} from 'mattermost-redux/actions/admin';
import {getEnvironmentConfig} from 'mattermost-redux/selectors/entities/admin';
import {getConfig} from 'mattermost-redux/selectors/entities/general';
@ -31,7 +31,7 @@ function mapStateToProps(state: GlobalState) {
function mapDispatchToProps(dispatch: Dispatch) {
return {
actions: bindActionCreators({
updateConfig,
patchConfig,
setNavigationBlocked,
}, dispatch),
};

View File

@ -5,7 +5,7 @@ import {connect} from 'react-redux';
import {bindActionCreators} from 'redux';
import type {Dispatch} from 'redux';
import {getDataRetentionCustomPolicies as fetchDataRetentionCustomPolicies, deleteDataRetentionCustomPolicy, updateConfig} from 'mattermost-redux/actions/admin';
import {getDataRetentionCustomPolicies as fetchDataRetentionCustomPolicies, deleteDataRetentionCustomPolicy, patchConfig} from 'mattermost-redux/actions/admin';
import {createJob, getJobsByType} from 'mattermost-redux/actions/jobs';
import {getDataRetentionCustomPolicies, getDataRetentionCustomPoliciesCount} from 'mattermost-redux/selectors/entities/admin';
import {getConfig} from 'mattermost-redux/selectors/entities/general';
@ -35,7 +35,7 @@ function mapDispatchToProps(dispatch: Dispatch) {
createJob,
getJobsByType,
deleteDataRetentionCustomPolicy,
updateConfig,
patchConfig,
}, dispatch),
};
}

View File

@ -6,7 +6,7 @@ import type {ConnectedProps} from 'react-redux';
import {bindActionCreators} from 'redux';
import type {Dispatch} from 'redux';
import {getConfig, getEnvironmentConfig, updateConfig} from 'mattermost-redux/actions/admin';
import {getConfig, getEnvironmentConfig, patchConfig} from 'mattermost-redux/actions/admin';
import {loadRolesIfNeeded, editRole} from 'mattermost-redux/actions/roles';
import {selectTeam} from 'mattermost-redux/actions/teams';
import {General} from 'mattermost-redux/constants';
@ -60,7 +60,7 @@ function mapDispatchToProps(dispatch: Dispatch) {
actions: bindActionCreators({
getConfig,
getEnvironmentConfig,
updateConfig,
patchConfig,
setNavigationBlocked,
deferNavigation,
cancelNavigation,

View File

@ -5,14 +5,14 @@ import {connect} from 'react-redux';
import {bindActionCreators} from 'redux';
import type {Dispatch} from 'redux';
import {updateConfig} from 'mattermost-redux/actions/admin';
import {patchConfig} from 'mattermost-redux/actions/admin';
import OpenIdConvert from './openid_convert';
function mapDispatchToProps(dispatch: Dispatch) {
return {
actions: bindActionCreators({
updateConfig,
patchConfig,
}, dispatch),
};
}

View File

@ -9,7 +9,7 @@ import OpenIdConvert from 'components/admin_console/openid_convert/openid_conver
describe('components/OpenIdConvert', () => {
const baseProps = {
actions: {
updateConfig: jest.fn(),
patchConfig: jest.fn(),
},
};

View File

@ -5,6 +5,7 @@ import React from 'react';
import {FormattedMessage} from 'react-intl';
import type {AdminConfig} from '@mattermost/types/config';
import type {DeepPartial} from '@mattermost/types/utilities';
import type {ActionResult} from 'mattermost-redux/types/actions';
@ -21,7 +22,7 @@ import './openid_convert.scss';
type Props = BaseProps & {
disabled?: boolean;
actions: {
updateConfig: (config: AdminConfig) => Promise<ActionResult>;
patchConfig: (config: DeepPartial<AdminConfig>) => Promise<ActionResult>;
};
};
type State = {
@ -59,7 +60,7 @@ export default class OpenIdConvert extends React.PureComponent<Props, State> {
newConfig[setting].TokenEndpoint = '';
});
const {error: err} = await this.props.actions.updateConfig(newConfig);
const {error: err} = await this.props.actions.patchConfig(newConfig);
if (err) {
this.setState({serverError: err.message});
} else {

View File

@ -6,6 +6,7 @@ import {Modal} from 'react-bootstrap';
import {FormattedMessage} from 'react-intl';
import type {AdminConfig} from '@mattermost/types/config';
import type {DeepPartial} from '@mattermost/types/utilities';
import type {ActionResult} from 'mattermost-redux/types/actions';
@ -22,7 +23,7 @@ type Props ={
show: boolean;
onClose: () => void;
actions: {
updateConfig: (config: AdminConfig) => Promise<ActionResult>;
patchConfig: (config: DeepPartial<AdminConfig>) => Promise<ActionResult>;
};
}
@ -48,7 +49,7 @@ export default function EditPostTimeLimitModal(props: Props) {
const newConfig = JSON.parse(JSON.stringify(props.config));
newConfig.ServiceSettings.PostEditTimeLimit = alwaysAllowPostEditing ? Constants.UNSET_POST_EDIT_TIME_LIMIT : postEditTimeLimit;
const {error} = await props.actions.updateConfig(newConfig);
const {error} = await props.actions.patchConfig(newConfig);
if (error) {
setErrorMessage(error.message);
setSaving(false);

View File

@ -5,7 +5,7 @@ import {connect} from 'react-redux';
import {bindActionCreators} from 'redux';
import type {Dispatch} from 'redux';
import {updateConfig} from 'mattermost-redux/actions/admin';
import {patchConfig} from 'mattermost-redux/actions/admin';
import {getConfig} from 'mattermost-redux/selectors/entities/admin';
import type {GlobalState} from 'types/store';
@ -20,7 +20,7 @@ function mapStateToProps(state: GlobalState) {
function mapDispatchToProps(dispatch: Dispatch) {
return {
actions: bindActionCreators({updateConfig}, dispatch),
actions: bindActionCreators({patchConfig}, dispatch),
};
}

View File

@ -256,7 +256,7 @@ describe('components/admin_console/SchemaAdminSettings', () => {
config={config}
environmentConfig={environmentConfig}
schema={{...schema} as AdminDefinitionSubSectionSchema}
updateConfig={jest.fn()}
patchConfig={jest.fn()}
/>,
);
expect(wrapper).toMatchSnapshot();
@ -269,7 +269,7 @@ describe('components/admin_console/SchemaAdminSettings', () => {
config={config}
environmentConfig={environmentConfig}
schema={{component: () => <p>{'Test'}</p>} as AdminDefinitionSubSectionSchema}
updateConfig={jest.fn()}
patchConfig={jest.fn()}
/>,
);
expect(wrapper).toMatchSnapshot();
@ -285,7 +285,7 @@ describe('components/admin_console/SchemaAdminSettings', () => {
...schema,
header: headerText,
} as AdminDefinitionSubSectionSchema,
updateConfig: jest.fn(),
patchConfig: jest.fn(),
};
const wrapper = shallowWithIntl(<SchemaAdminSettings {...props}/>);
@ -308,7 +308,7 @@ describe('components/admin_console/SchemaAdminSettings', () => {
...schema,
footer: footerText,
} as AdminDefinitionSubSectionSchema,
updateConfig: jest.fn(),
patchConfig: jest.fn(),
};
const wrapper = shallowWithIntl(<SchemaAdminSettings {...props}/>);
@ -327,7 +327,7 @@ describe('components/admin_console/SchemaAdminSettings', () => {
config,
environmentConfig,
schema: null,
updateConfig: jest.fn(),
patchConfig: jest.fn(),
};
const wrapper = shallowWithIntl(<SchemaAdminSettings {...props}/>);
@ -360,7 +360,7 @@ describe('components/admin_console/SchemaAdminSettings', () => {
id: '',
environmentConfig,
schema: localSchema,
updateConfig: jest.fn(),
patchConfig: jest.fn(),
};
const wrapper = shallowWithIntl(<SchemaAdminSettings {...props}/>);
@ -389,7 +389,7 @@ describe('components/admin_console/SchemaAdminSettings', () => {
config,
environmentConfig,
schema: localSchema,
updateConfig: jest.fn(),
patchConfig: jest.fn(),
};
const wrapper = shallowWithIntl(<SchemaAdminSettings {...props}/>);

View File

@ -10,6 +10,7 @@ import {Link} from 'react-router-dom';
import type {CloudState} from '@mattermost/types/cloud';
import type {AdminConfig, ClientLicense, EnvironmentConfig} from '@mattermost/types/config';
import type {Role} from '@mattermost/types/roles';
import type {DeepPartial} from '@mattermost/types/utilities';
import type {ActionResult} from 'mattermost-redux/types/actions';
@ -53,7 +54,7 @@ type Props = {
roles: Record<string, Role>;
license: ClientLicense;
editRole: (role: Role) => void;
updateConfig: (config: AdminConfig) => Promise<ActionResult>;
patchConfig: (config: DeepPartial<AdminConfig>) => Promise<ActionResult>;
isDisabled: boolean;
consoleAccess: ConsoleAccess;
cloud: CloudState;
@ -1145,7 +1146,7 @@ export class SchemaAdminSettings extends React.PureComponent<Props, State> {
let config = JSON.parse(JSON.stringify(this.props.config));
config = this.getConfigFromState(config);
const {error} = await this.props.updateConfig(config);
const {error} = await this.props.patchConfig(config);
if (error) {
this.setState({
serverError: error.message,

View File

@ -112,26 +112,35 @@ describe('Actions.Admin', () => {
expect(config.TeamSettings.SiteName === 'Mattermost').toBeTruthy();
});
it('updateConfig', async () => {
it('patchConfig', async () => {
nock(Client4.getBaseRoute()).
get('/config').
reply(200, {
TeamSettings: {
SiteName: 'Mattermost',
TeammateNameDisplay: 'username',
},
});
const {data} = await store.dispatch(Actions.getConfig());
const updated = JSON.parse(JSON.stringify(data));
// Creating a copy.
const reply = JSON.parse(JSON.stringify(data));
const oldSiteName = updated.TeamSettings.SiteName;
const oldNameDisplay = updated.TeamSettings.TeammateNameDisplay;
const testSiteName = 'MattermostReduxTest';
updated.TeamSettings.SiteName = testSiteName;
reply.TeamSettings.SiteName = testSiteName;
// Testing partial config patch.
updated.TeamSettings.TeammateNameDisplay = null;
nock(Client4.getBaseRoute()).
put('/config').
reply(200, updated);
put('/config/patch').
reply(200, reply);
await store.dispatch(Actions.updateConfig(updated));
await store.dispatch(Actions.patchConfig(updated));
let state = store.getState();
@ -139,14 +148,15 @@ describe('Actions.Admin', () => {
expect(config).toBeTruthy();
expect(config.TeamSettings).toBeTruthy();
expect(config.TeamSettings.SiteName === testSiteName).toBeTruthy();
expect(config.TeamSettings.TeammateNameDisplay === oldNameDisplay).toBeTruthy();
updated.TeamSettings.SiteName = oldSiteName;
nock(Client4.getBaseRoute()).
put('/config').
put('/config/patch').
reply(200, updated);
await store.dispatch(Actions.updateConfig(updated));
await store.dispatch(Actions.patchConfig(updated));
state = store.getState();

View File

@ -23,6 +23,7 @@ import type {
Team,
TeamSearchOpts,
} from '@mattermost/types/teams';
import type {DeepPartial} from '@mattermost/types/utilities';
import {AdminTypes} from 'mattermost-redux/action_types';
import {getUsersLimits} from 'mattermost-redux/actions/limits';
@ -79,9 +80,9 @@ export function getConfig() {
});
}
export function updateConfig(config: AdminConfig) {
export function patchConfig(config: DeepPartial<AdminConfig>) {
return bindClientFunc({
clientFunc: Client4.updateConfig,
clientFunc: Client4.patchConfig,
onSuccess: [AdminTypes.RECEIVED_CONFIG],
params: [
config,