[MM-50352] Improve URL validation and add/edit server experience (#2720)
* [MM-50352] Improve URL validation and add/edit server experience * Fix build * Fix translations * First pass of fixes * Some changes to avoid 2 clicks, tests * PR feedback * Update translations * PR feedback * Fix translations * PR feedback * E2E test fixes
This commit is contained in:
Devin Binnie
@@ -41,6 +41,7 @@ import {
|
||||
WINDOW_MINIMIZE,
|
||||
WINDOW_RESTORE,
|
||||
DOUBLE_CLICK_ON_WINDOW,
|
||||
VALIDATE_SERVER_URL,
|
||||
} from 'common/communication';
|
||||
import Config from 'common/config';
|
||||
import {isTrustedURL, parseURL} from 'common/utils/url';
|
||||
@@ -97,6 +98,7 @@ import {
|
||||
handleEditServerModal,
|
||||
handleNewServerModal,
|
||||
handleRemoveServerModal,
|
||||
handleServerURLValidation,
|
||||
switchServer,
|
||||
} from './servers';
|
||||
import {
|
||||
@@ -287,6 +289,7 @@ function initializeInterCommunicationEventListeners() {
|
||||
ipcMain.on(SHOW_NEW_SERVER_MODAL, handleNewServerModal);
|
||||
ipcMain.on(SHOW_EDIT_SERVER_MODAL, handleEditServerModal);
|
||||
ipcMain.on(SHOW_REMOVE_SERVER_MODAL, handleRemoveServerModal);
|
||||
ipcMain.handle(VALIDATE_SERVER_URL, handleServerURLValidation);
|
||||
ipcMain.handle(GET_AVAILABLE_SPELL_CHECKER_LANGUAGES, () => session.defaultSession.availableSpellCheckerLanguages);
|
||||
ipcMain.on(START_UPDATE_DOWNLOAD, handleStartDownload);
|
||||
ipcMain.on(START_UPGRADE, handleStartUpgrade);
|
||||
|
||||
@@ -47,7 +47,6 @@ describe('main/app/intercom', () => {
|
||||
getLocalPreload.mockReturnValue('/some/preload.js');
|
||||
MainWindow.get.mockReturnValue({});
|
||||
|
||||
ServerManager.getAllServers.mockReturnValue([]);
|
||||
ServerManager.hasServers.mockReturnValue(false);
|
||||
});
|
||||
|
||||
@@ -56,7 +55,7 @@ describe('main/app/intercom', () => {
|
||||
ModalManager.addModal.mockReturnValue(promise);
|
||||
|
||||
handleWelcomeScreenModal();
|
||||
expect(ModalManager.addModal).toHaveBeenCalledWith('welcomeScreen', '/some/index.html', '/some/preload.js', [], {}, true);
|
||||
expect(ModalManager.addModal).toHaveBeenCalledWith('welcomeScreen', '/some/index.html', '/some/preload.js', null, {}, true);
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -97,7 +97,7 @@ export function handleWelcomeScreenModal() {
|
||||
if (!mainWindow) {
|
||||
return;
|
||||
}
|
||||
const modalPromise = ModalManager.addModal<UniqueServer[], UniqueServer>('welcomeScreen', html, preload, ServerManager.getAllServers().map((server) => server.toUniqueServer()), mainWindow, !ServerManager.hasServers());
|
||||
const modalPromise = ModalManager.addModal<null, UniqueServer>('welcomeScreen', html, preload, null, mainWindow, !ServerManager.hasServers());
|
||||
if (modalPromise) {
|
||||
modalPromise.then((data) => {
|
||||
const newServer = ServerManager.addServer(data);
|
||||
|
||||
@@ -1,9 +1,12 @@
|
||||
// Copyright (c) 2016-present Mattermost, Inc. All Rights Reserved.
|
||||
// See LICENSE.txt for license information.
|
||||
|
||||
import {MattermostServer} from 'common/servers/MattermostServer';
|
||||
import ServerManager from 'common/servers/serverManager';
|
||||
import {URLValidationStatus} from 'common/utils/constants';
|
||||
import {getDefaultViewsForConfigServer} from 'common/views/View';
|
||||
|
||||
import {ServerInfo} from 'main/server/serverInfo';
|
||||
import ModalManager from 'main/views/modalManager';
|
||||
import {getLocalURLString, getLocalPreload} from 'main/utils';
|
||||
import MainWindow from 'main/windows/mainWindow';
|
||||
@@ -28,10 +31,17 @@ jest.mock('common/servers/serverManager', () => ({
|
||||
getView: jest.fn(),
|
||||
getLastActiveTabForServer: jest.fn(),
|
||||
getServerLog: jest.fn(),
|
||||
lookupViewByURL: jest.fn(),
|
||||
}));
|
||||
jest.mock('common/servers/MattermostServer', () => ({
|
||||
MattermostServer: jest.fn(),
|
||||
}));
|
||||
jest.mock('common/views/View', () => ({
|
||||
getDefaultViewsForConfigServer: jest.fn(),
|
||||
}));
|
||||
jest.mock('main/server/serverInfo', () => ({
|
||||
ServerInfo: jest.fn(),
|
||||
}));
|
||||
jest.mock('main/views/modalManager', () => ({
|
||||
addModal: jest.fn(),
|
||||
}));
|
||||
@@ -315,4 +325,156 @@ describe('main/app/servers', () => {
|
||||
}));
|
||||
});
|
||||
});
|
||||
|
||||
describe('handleServerURLValidation', () => {
|
||||
beforeEach(() => {
|
||||
MattermostServer.mockImplementation(({url}) => ({url}));
|
||||
ServerInfo.mockImplementation(({url}) => ({
|
||||
fetchRemoteInfo: jest.fn().mockImplementation(() => ({
|
||||
serverVersion: '7.8.0',
|
||||
siteName: 'Mattermost',
|
||||
siteURL: url,
|
||||
})),
|
||||
}));
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
jest.resetAllMocks();
|
||||
});
|
||||
|
||||
it('should return Missing when you get no URL', async () => {
|
||||
const result = await Servers.handleServerURLValidation({});
|
||||
expect(result.status).toBe(URLValidationStatus.Missing);
|
||||
});
|
||||
|
||||
it('should return Invalid when you pass in invalid characters', async () => {
|
||||
const result = await Servers.handleServerURLValidation({}, '!@#$%^&*()!@#$%^&*()');
|
||||
expect(result.status).toBe(URLValidationStatus.Invalid);
|
||||
});
|
||||
|
||||
it('should include HTTPS when missing', async () => {
|
||||
const result = await Servers.handleServerURLValidation({}, 'server.com');
|
||||
expect(result.status).toBe(URLValidationStatus.OK);
|
||||
expect(result.validatedURL).toBe('https://server.com/');
|
||||
});
|
||||
|
||||
it('should correct typos in the protocol', async () => {
|
||||
const result = await Servers.handleServerURLValidation({}, 'htpst://server.com');
|
||||
expect(result.status).toBe(URLValidationStatus.OK);
|
||||
expect(result.validatedURL).toBe('https://server.com/');
|
||||
});
|
||||
|
||||
it('should replace HTTP with HTTPS when applicable', async () => {
|
||||
const result = await Servers.handleServerURLValidation({}, 'http://server.com');
|
||||
expect(result.status).toBe(URLValidationStatus.OK);
|
||||
expect(result.validatedURL).toBe('https://server.com/');
|
||||
});
|
||||
|
||||
it('should generate a warning when the server already exists', async () => {
|
||||
ServerManager.lookupViewByURL.mockReturnValue({server: {id: 'server-1', url: new URL('https://server.com')}});
|
||||
const result = await Servers.handleServerURLValidation({}, 'https://server.com');
|
||||
expect(result.status).toBe(URLValidationStatus.URLExists);
|
||||
expect(result.validatedURL).toBe('https://server.com/');
|
||||
});
|
||||
|
||||
it('should generate a warning if the server exists when editing', async () => {
|
||||
ServerManager.lookupViewByURL.mockReturnValue({server: {name: 'Server 1', id: 'server-1', url: new URL('https://server.com')}});
|
||||
const result = await Servers.handleServerURLValidation({}, 'https://server.com', 'server-2');
|
||||
expect(result.status).toBe(URLValidationStatus.URLExists);
|
||||
expect(result.validatedURL).toBe('https://server.com/');
|
||||
expect(result.existingServerName).toBe('Server 1');
|
||||
});
|
||||
|
||||
it('should not generate a warning if editing the same server', async () => {
|
||||
ServerManager.lookupViewByURL.mockReturnValue({server: {name: 'Server 1', id: 'server-1', url: new URL('https://server.com')}});
|
||||
const result = await Servers.handleServerURLValidation({}, 'https://server.com', 'server-1');
|
||||
expect(result.status).toBe(URLValidationStatus.OK);
|
||||
expect(result.validatedURL).toBe('https://server.com/');
|
||||
});
|
||||
|
||||
it('should attempt HTTP when HTTPS fails, and generate a warning', async () => {
|
||||
ServerInfo.mockImplementation(({url}) => ({
|
||||
fetchRemoteInfo: jest.fn().mockImplementation(() => {
|
||||
if (url.startsWith('https:')) {
|
||||
return undefined;
|
||||
}
|
||||
|
||||
return {
|
||||
serverVersion: '7.8.0',
|
||||
siteName: 'Mattermost',
|
||||
siteURL: url,
|
||||
};
|
||||
}),
|
||||
}));
|
||||
|
||||
const result = await Servers.handleServerURLValidation({}, 'http://server.com');
|
||||
expect(result.status).toBe(URLValidationStatus.Insecure);
|
||||
expect(result.validatedURL).toBe('http://server.com/');
|
||||
});
|
||||
|
||||
it('should show a warning when the ping request times out', async () => {
|
||||
ServerInfo.mockImplementation(() => ({
|
||||
fetchRemoteInfo: jest.fn().mockImplementation(() => {
|
||||
throw new Error();
|
||||
}),
|
||||
}));
|
||||
|
||||
const result = await Servers.handleServerURLValidation({}, 'https://not-server.com');
|
||||
expect(result.status).toBe(URLValidationStatus.NotMattermost);
|
||||
expect(result.validatedURL).toBe('https://not-server.com/');
|
||||
});
|
||||
|
||||
it('should update the users URL when the Site URL is different', async () => {
|
||||
ServerInfo.mockImplementation(() => ({
|
||||
fetchRemoteInfo: jest.fn().mockImplementation(() => {
|
||||
return {
|
||||
serverVersion: '7.8.0',
|
||||
siteName: 'Mattermost',
|
||||
siteURL: 'https://mainserver.com/',
|
||||
};
|
||||
}),
|
||||
}));
|
||||
|
||||
const result = await Servers.handleServerURLValidation({}, 'https://server.com');
|
||||
expect(result.status).toBe(URLValidationStatus.URLUpdated);
|
||||
expect(result.validatedURL).toBe('https://mainserver.com/');
|
||||
});
|
||||
|
||||
it('should warn the user when the Site URL is different but unreachable', async () => {
|
||||
ServerInfo.mockImplementation(({url}) => ({
|
||||
fetchRemoteInfo: jest.fn().mockImplementation(() => {
|
||||
if (url === 'https://mainserver.com/') {
|
||||
return undefined;
|
||||
}
|
||||
return {
|
||||
serverVersion: '7.8.0',
|
||||
siteName: 'Mattermost',
|
||||
siteURL: 'https://mainserver.com/',
|
||||
};
|
||||
}),
|
||||
}));
|
||||
|
||||
const result = await Servers.handleServerURLValidation({}, 'https://server.com');
|
||||
expect(result.status).toBe(URLValidationStatus.URLNotMatched);
|
||||
expect(result.validatedURL).toBe('https://server.com/');
|
||||
});
|
||||
|
||||
it('should warn the user when the Site URL already exists as another server', async () => {
|
||||
ServerManager.lookupViewByURL.mockReturnValue({server: {name: 'Server 1', id: 'server-1', url: new URL('https://mainserver.com')}});
|
||||
ServerInfo.mockImplementation(() => ({
|
||||
fetchRemoteInfo: jest.fn().mockImplementation(() => {
|
||||
return {
|
||||
serverVersion: '7.8.0',
|
||||
siteName: 'Mattermost',
|
||||
siteURL: 'https://mainserver.com',
|
||||
};
|
||||
}),
|
||||
}));
|
||||
|
||||
const result = await Servers.handleServerURLValidation({}, 'https://server.com');
|
||||
expect(result.status).toBe(URLValidationStatus.URLExists);
|
||||
expect(result.validatedURL).toBe('https://mainserver.com/');
|
||||
expect(result.existingServerName).toBe('Server 1');
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,18 +1,23 @@
|
||||
// Copyright (c) 2016-present Mattermost, Inc. All Rights Reserved.
|
||||
// See LICENSE.txt for license information.
|
||||
|
||||
import {IpcMainEvent, ipcMain} from 'electron';
|
||||
import {IpcMainEvent, IpcMainInvokeEvent, ipcMain} from 'electron';
|
||||
|
||||
import {UniqueServer, Server} from 'types/config';
|
||||
import {URLValidationResult} from 'types/server';
|
||||
|
||||
import {UPDATE_SHORTCUT_MENU} from 'common/communication';
|
||||
import {Logger} from 'common/log';
|
||||
import ServerManager from 'common/servers/serverManager';
|
||||
import {MattermostServer} from 'common/servers/MattermostServer';
|
||||
import {isValidURI, isValidURL, parseURL} from 'common/utils/url';
|
||||
import {URLValidationStatus} from 'common/utils/constants';
|
||||
|
||||
import ViewManager from 'main/views/viewManager';
|
||||
import ModalManager from 'main/views/modalManager';
|
||||
import MainWindow from 'main/windows/mainWindow';
|
||||
import {getLocalPreload, getLocalURLString} from 'main/utils';
|
||||
import {ServerInfo} from 'main/server/serverInfo';
|
||||
|
||||
const log = new Logger('App.Servers');
|
||||
|
||||
@@ -49,7 +54,7 @@ export const handleNewServerModal = () => {
|
||||
if (!mainWindow) {
|
||||
return;
|
||||
}
|
||||
const modalPromise = ModalManager.addModal<UniqueServer[], Server>('newServer', html, preload, ServerManager.getAllServers().map((server) => server.toUniqueServer()), mainWindow, !ServerManager.hasServers());
|
||||
const modalPromise = ModalManager.addModal<null, Server>('newServer', html, preload, null, mainWindow, !ServerManager.hasServers());
|
||||
if (modalPromise) {
|
||||
modalPromise.then((data) => {
|
||||
const newServer = ServerManager.addServer(data);
|
||||
@@ -80,14 +85,11 @@ export const handleEditServerModal = (e: IpcMainEvent, id: string) => {
|
||||
if (!server) {
|
||||
return;
|
||||
}
|
||||
const modalPromise = ModalManager.addModal<{currentServers: UniqueServer[]; server: UniqueServer}, Server>(
|
||||
const modalPromise = ModalManager.addModal<UniqueServer, Server>(
|
||||
'editServer',
|
||||
html,
|
||||
preload,
|
||||
{
|
||||
currentServers: ServerManager.getAllServers().map((server) => server.toUniqueServer()),
|
||||
server: server.toUniqueServer(),
|
||||
},
|
||||
server.toUniqueServer(),
|
||||
mainWindow);
|
||||
if (modalPromise) {
|
||||
modalPromise.then((data) => ServerManager.editServer(id, data)).catch((e) => {
|
||||
@@ -132,3 +134,96 @@ export const handleRemoveServerModal = (e: IpcMainEvent, id: string) => {
|
||||
log.warn('There is already an edit server modal');
|
||||
}
|
||||
};
|
||||
|
||||
export const handleServerURLValidation = async (e: IpcMainInvokeEvent, url?: string, currentId?: string): Promise<URLValidationResult> => {
|
||||
log.debug('handleServerURLValidation', url, currentId);
|
||||
|
||||
// If the URL is missing or null, reject
|
||||
if (!url) {
|
||||
return {status: URLValidationStatus.Missing};
|
||||
}
|
||||
|
||||
let httpUrl = url;
|
||||
if (!isValidURL(url)) {
|
||||
// If it already includes the protocol, tell them it's invalid
|
||||
if (isValidURI(url)) {
|
||||
httpUrl = url.replace(/^(.+):/, 'https:');
|
||||
} else {
|
||||
// Otherwise add HTTPS for them
|
||||
httpUrl = `https://${url}`;
|
||||
}
|
||||
}
|
||||
|
||||
// Make sure the final URL is valid
|
||||
const parsedURL = parseURL(httpUrl);
|
||||
if (!parsedURL) {
|
||||
return {status: URLValidationStatus.Invalid};
|
||||
}
|
||||
|
||||
// Try and add HTTPS to see if we can get a more secure URL
|
||||
let secureURL = parsedURL;
|
||||
if (parsedURL.protocol === 'http:') {
|
||||
secureURL = parseURL(parsedURL.toString().replace(/^http:/, 'https:')) ?? parsedURL;
|
||||
}
|
||||
|
||||
// Tell the user if they already have a server for this URL
|
||||
const existingServer = ServerManager.lookupViewByURL(secureURL, true);
|
||||
if (existingServer && existingServer.server.id !== currentId) {
|
||||
return {status: URLValidationStatus.URLExists, existingServerName: existingServer.server.name, validatedURL: existingServer.server.url.toString()};
|
||||
}
|
||||
|
||||
// Try and get remote info from the most secure URL, otherwise use the insecure one
|
||||
let remoteURL = secureURL;
|
||||
let remoteInfo = await testRemoteServer(secureURL);
|
||||
if (!remoteInfo) {
|
||||
if (secureURL.toString() !== parsedURL.toString()) {
|
||||
remoteURL = parsedURL;
|
||||
remoteInfo = await testRemoteServer(parsedURL);
|
||||
}
|
||||
}
|
||||
|
||||
// If we can't get the remote info, warn the user that this might not be the right URL
|
||||
// If the original URL was invalid, don't replace that as they probably have a typo somewhere
|
||||
if (!remoteInfo) {
|
||||
return {status: URLValidationStatus.NotMattermost, validatedURL: parsedURL.toString()};
|
||||
}
|
||||
|
||||
// If we were only able to connect via HTTP, warn the user that the connection is not secure
|
||||
if (remoteURL.protocol === 'http:') {
|
||||
return {status: URLValidationStatus.Insecure, serverVersion: remoteInfo.serverVersion, validatedURL: remoteURL.toString()};
|
||||
}
|
||||
|
||||
// If the URL doesn't match the Site URL, set the URL to the correct one
|
||||
if (remoteInfo.siteURL && remoteURL.toString() !== new URL(remoteInfo.siteURL).toString()) {
|
||||
const parsedSiteURL = parseURL(remoteInfo.siteURL);
|
||||
if (parsedSiteURL) {
|
||||
// Check the Site URL as well to see if it's already pre-configured
|
||||
const existingServer = ServerManager.lookupViewByURL(parsedSiteURL, true);
|
||||
if (existingServer && existingServer.server.id !== currentId) {
|
||||
return {status: URLValidationStatus.URLExists, existingServerName: existingServer.server.name, validatedURL: existingServer.server.url.toString()};
|
||||
}
|
||||
|
||||
// If we can't reach the remote Site URL, there's probably a configuration issue
|
||||
const remoteSiteURLInfo = await testRemoteServer(parsedSiteURL);
|
||||
if (!remoteSiteURLInfo) {
|
||||
return {status: URLValidationStatus.URLNotMatched, serverVersion: remoteInfo.serverVersion, serverName: remoteInfo.siteName, validatedURL: remoteURL.toString()};
|
||||
}
|
||||
}
|
||||
|
||||
// Otherwise fix it for them and return
|
||||
return {status: URLValidationStatus.URLUpdated, serverVersion: remoteInfo.serverVersion, serverName: remoteInfo.siteName, validatedURL: remoteInfo.siteURL};
|
||||
}
|
||||
|
||||
return {status: URLValidationStatus.OK, serverVersion: remoteInfo.serverVersion, serverName: remoteInfo.siteName, validatedURL: remoteInfo.siteURL};
|
||||
};
|
||||
|
||||
const testRemoteServer = async (parsedURL: URL) => {
|
||||
const server = new MattermostServer({name: 'temp', url: parsedURL.toString()}, false);
|
||||
const serverInfo = new ServerInfo(server);
|
||||
try {
|
||||
const remoteInfo = await serverInfo.fetchRemoteInfo();
|
||||
return remoteInfo;
|
||||
} catch (error) {
|
||||
return undefined;
|
||||
}
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user