Skip to content

Commit

Permalink
[Dashboard] Fix Alias Redirect Race Condition (elastic#161043)
Browse files Browse the repository at this point in the history
Fixes a race condition that could happen in the case of an alias match redirect.

(cherry picked from commit 1d78311)
  • Loading branch information
ThomThomson committed Jul 6, 2023
1 parent 71227e0 commit eeed44d
Show file tree
Hide file tree
Showing 6 changed files with 48 additions and 29 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { pluginServices } from '../../services/plugin_services';
import { createDashboardEditUrl } from '../../dashboard_constants';
import { useDashboardMountContext } from './dashboard_mount_context';
import { LoadDashboardReturn } from '../../services/dashboard_content_management/types';
import { DashboardCreationOptions } from '../..';

export const useDashboardOutcomeValidation = () => {
const [aliasId, setAliasId] = useState<string>();
Expand All @@ -26,10 +27,10 @@ export const useDashboardOutcomeValidation = () => {
*/
const { screenshotMode, spaces } = pluginServices.getServices();

const validateOutcome = useCallback(
const validateOutcome: DashboardCreationOptions['validateLoadedSavedObject'] = useCallback(
({ dashboardFound, resolveMeta, dashboardId }: LoadDashboardReturn) => {
if (!dashboardFound) {
return false; // redirected. Stop loading dashboard.
return 'invalid';
}

if (resolveMeta && dashboardId) {
Expand All @@ -40,17 +41,17 @@ export const useDashboardOutcomeValidation = () => {
if (loadOutcome === 'aliasMatch' && dashboardId && alias) {
const path = scopedHistory.location.hash.replace(dashboardId, alias);
if (screenshotMode.isScreenshotMode()) {
scopedHistory.replace(path);
scopedHistory.replace(path); // redirect without the toast when in screenshot mode.
} else {
spaces.redirectLegacyUrl?.({ path, aliasPurpose });
return false; // redirected. Stop loading dashboard.
}
return 'redirected'; // redirected. Stop loading dashboard.
}
setAliasId(alias);
setOutcome(loadOutcome);
setSavedObjectId(dashboardId);
}
return true;
return 'valid';
},
[scopedHistory, screenshotMode, spaces]
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,21 @@ test('throws error when no data views are available', async () => {

test('throws error when provided validation function returns invalid', async () => {
const creationOptions: DashboardCreationOptions = {
validateLoadedSavedObject: jest.fn().mockImplementation(() => false),
validateLoadedSavedObject: jest.fn().mockImplementation(() => 'invalid'),
};
await expect(async () => {
await createDashboard(creationOptions, 0, 'test-id');
}).rejects.toThrow('Dashboard failed saved object result validation');
});

test('returns undefined when provided validation function returns redireted', async () => {
const creationOptions: DashboardCreationOptions = {
validateLoadedSavedObject: jest.fn().mockImplementation(() => 'redirected'),
};
const dashboard = await createDashboard(creationOptions, 0, 'test-id');
expect(dashboard).toBeUndefined();
});

test('pulls state from dashboard saved object when given a saved object id', async () => {
pluginServices.getServices().dashboardContentManagement.loadDashboardState = jest
.fn()
Expand All @@ -64,7 +72,8 @@ test('pulls state from dashboard saved object when given a saved object id', asy
expect(
pluginServices.getServices().dashboardContentManagement.loadDashboardState
).toHaveBeenCalledWith({ id: 'wow-such-id' });
expect(dashboard.getState().explicitInput.description).toBe(`wow would you look at that? Wow.`);
expect(dashboard).toBeDefined();
expect(dashboard!.getState().explicitInput.description).toBe(`wow would you look at that? Wow.`);
});

test('pulls state from session storage which overrides state from saved object', async () => {
Expand All @@ -80,7 +89,8 @@ test('pulls state from session storage which overrides state from saved object',
.fn()
.mockReturnValue({ description: 'wow this description marginally better' });
const dashboard = await createDashboard({ useSessionStorageIntegration: true }, 0, 'wow-such-id');
expect(dashboard.getState().explicitInput.description).toBe(
expect(dashboard).toBeDefined();
expect(dashboard!.getState().explicitInput.description).toBe(
'wow this description marginally better'
);
});
Expand All @@ -105,7 +115,8 @@ test('pulls state from creation options initial input which overrides all other
0,
'wow-such-id'
);
expect(dashboard.getState().explicitInput.description).toBe(
expect(dashboard).toBeDefined();
expect(dashboard!.getState().explicitInput.description).toBe(
'wow this description is a masterpiece'
);
});
Expand Down Expand Up @@ -165,7 +176,8 @@ test('applies time range from query service to initial input if time restore is
timeRange: savedTimeRange,
}),
});
expect(dashboard.getState().explicitInput.timeRange).toEqual(urlTimeRange);
expect(dashboard).toBeDefined();
expect(dashboard!.getState().explicitInput.timeRange).toEqual(urlTimeRange);
});

test('applies time range from query service to initial input if time restore is off', async () => {
Expand All @@ -179,7 +191,8 @@ test('applies time range from query service to initial input if time restore is
kbnUrlStateStorage: createKbnUrlStateStorage(),
},
});
expect(dashboard.getState().explicitInput.timeRange).toEqual(timeRange);
expect(dashboard).toBeDefined();
expect(dashboard!.getState().explicitInput.timeRange).toEqual(timeRange);
});

test('replaces panel with incoming embeddable if id matches existing panel', async () => {
Expand All @@ -205,7 +218,8 @@ test('replaces panel with incoming embeddable if id matches existing panel', asy
},
}),
});
expect(dashboard.getState().explicitInput.panels.i_match.explicitInput).toStrictEqual(
expect(dashboard).toBeDefined();
expect(dashboard!.getState().explicitInput.panels.i_match.explicitInput).toStrictEqual(
expect.objectContaining({
id: 'i_match',
firstName: 'wow look at this replacement wow',
Expand Down Expand Up @@ -323,7 +337,8 @@ test('searchSessionId is updated prior to child embeddable parent subscription e
createSessionRestorationDataProvider: () => {},
} as unknown as DashboardCreationOptions['searchSessionSettings'],
});
const embeddable = await dashboard.addNewEmbeddable<
expect(dashboard).toBeDefined();
const embeddable = await dashboard!.addNewEmbeddable<
ContactCardEmbeddableInput,
ContactCardEmbeddableOutput,
ContactCardEmbeddable
Expand All @@ -333,7 +348,7 @@ test('searchSessionId is updated prior to child embeddable parent subscription e

expect(embeddable.getInput().searchSessionId).toBe('searchSessionId1');

dashboard.updateInput({
dashboard!.updateInput({
timeRange: {
to: 'now',
from: 'now-7d',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export const createDashboard = async (
creationOptions?: DashboardCreationOptions,
dashboardCreationStartTime?: number,
savedObjectId?: string
): Promise<DashboardContainer> => {
): Promise<DashboardContainer | undefined> => {
const {
data: { dataViews },
dashboardContentManagement: { loadDashboardState },
Expand Down Expand Up @@ -75,11 +75,13 @@ export const createDashboard = async (
// --------------------------------------------------------------------------------------
// Initialize Dashboard integrations
// --------------------------------------------------------------------------------------
const { input, searchSessionId } = await initializeDashboard({
const initializeResult = await initializeDashboard({
loadDashboardReturn: savedObjectResult,
untilDashboardReady,
creationOptions,
});
if (!initializeResult) return;
const { input, searchSessionId } = initializeResult;

// --------------------------------------------------------------------------------------
// Build and return the dashboard container.
Expand Down Expand Up @@ -140,13 +142,12 @@ export const initializeDashboard = async ({
// --------------------------------------------------------------------------------------
// Run validation.
// --------------------------------------------------------------------------------------
if (
loadDashboardReturn &&
validateLoadedSavedObject &&
!validateLoadedSavedObject(loadDashboardReturn)
) {
const validationResult = loadDashboardReturn && validateLoadedSavedObject?.(loadDashboardReturn);
if (validationResult === 'invalid') {
// throw error to stop the rest of Dashboard loading and make the factory return an ErrorEmbeddable.
throw new Error('Dashboard failed saved object result validation');
} else if (validationResult === 'redirected') {
return;
}

// --------------------------------------------------------------------------------------
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -380,12 +380,14 @@ export class DashboardContainer extends Container<InheritedChildInput, Dashboard
});
});

const { input: newInput, searchSessionId } = await initializeDashboard({
const initializeResult = await initializeDashboard({
creationOptions: this.creationOptions,
controlGroup: this.controlGroup,
untilDashboardReady,
loadDashboardReturn,
});
if (!initializeResult) return;
const { input: newInput, searchSessionId } = initializeResult;

this.searchSessionId = searchSessionId;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export interface DashboardCreationOptions {
useUnifiedSearchIntegration?: boolean;
unifiedSearchSettings?: { kbnUrlStateStorage: IKbnUrlStateStorage };

validateLoadedSavedObject?: (result: LoadDashboardReturn) => boolean;
validateLoadedSavedObject?: (result: LoadDashboardReturn) => 'valid' | 'invalid' | 'redirected';

isEmbeddedExternally?: boolean;
}
Expand Down Expand Up @@ -95,7 +95,7 @@ export class DashboardContainerFactoryDefinition
parent?: Container,
creationOptions?: DashboardCreationOptions,
savedObjectId?: string
): Promise<DashboardContainer | ErrorEmbeddable> => {
): Promise<DashboardContainer | ErrorEmbeddable | undefined> => {
const dashboardCreationStartTime = performance.now();
const { createDashboard } = await import('./create/create_dashboard');
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,8 @@ export const DashboardRenderer = forwardRef<AwaitingDashboardAPI, DashboardRende
});
return;
}
setLoading(true);

setLoading(true);
let canceled = false;
(async () => {
const creationOptions = await getCreationOptions?.();
Expand All @@ -115,14 +115,14 @@ export const DashboardRenderer = forwardRef<AwaitingDashboardAPI, DashboardRende
creationOptions,
savedObjectId
);
setLoading(false);

if (canceled) {
container.destroy();
if (canceled || !container) {
setDashboardContainer(undefined);
container?.destroy();
return;
}

setLoading(false);

if (isErrorEmbeddable(container)) {
setFatalError(container);
if (container.error instanceof SavedObjectNotFound) {
Expand Down

0 comments on commit eeed44d

Please sign in to comment.