Skip to content

Commit

Permalink
feat: Dashboard should not allow to create a workspace if repo url is… (
Browse files Browse the repository at this point in the history
#1192)

* feat: Dashboard should not allow to create a workspace if repo url is not in allowed list

Signed-off-by: Anatolii Bazko <abazko@redhat.com>
Co-authored-by: Oleksii Orel <oorel@redhat.com>
  • Loading branch information
tolusha and olexii4 authored Sep 18, 2024
1 parent e1fe428 commit 8c8b3f8
Show file tree
Hide file tree
Showing 20 changed files with 251 additions and 4 deletions.
1 change: 1 addition & 0 deletions packages/common/src/dto/api/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ export interface IServerConfig {
pluginRegistryURL: string;
pluginRegistryInternalURL: string;
dashboardLogo?: { base64data: string; mediatype: string };
allowedSourceUrls: string[];
}

export interface IAdvancedAuthorization {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,11 @@ describe('Server Config API Service', () => {
const res = serverConfigService.getAdvancedAuthorization(buildCustomResource());
expect(res).toEqual({ allowUsers: ['user1', 'user2'] });
});

test('getting allowed source urls', () => {
const res = serverConfigService.getAllowedSourceUrls(buildCustomResource());
expect(res).toEqual(['https://github.com']);
});
});

function buildCustomResourceList(): { body: CustomResourceDefinitionList } {
Expand Down Expand Up @@ -219,6 +224,9 @@ function buildCustomResource(options?: { openVSXURL?: string }): CheClusterCusto
secondsOfRunBeforeIdling: -1,
maxNumberOfRunningWorkspacesPerCluster: 100,
storage: { pvcStrategy: 'per-user' },
allowedSources: {
urls: ['https://github.com'],
},
},
networking: {
auth: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -228,4 +228,8 @@ export class ServerConfigApiService implements IServerConfigApi {
getAutoProvision(cheCustomResource: CheClusterCustomResource): boolean {
return cheCustomResource.spec.devEnvironments?.defaultNamespace?.autoProvision || false;
}

getAllowedSourceUrls(cheCustomResource: CheClusterCustomResource): string[] {
return cheCustomResource.spec.devEnvironments?.allowedSources?.urls || [];
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,9 @@ export type CheClusterCustomResourceSpecDevEnvironments = {
maxNumberOfRunningWorkspacesPerUser?: number;
maxNumberOfRunningWorkspacesPerCluster?: number;
maxNumberOfWorkspacesPerUser?: number;
allowedSources?: {
urls?: string[];
};
};

export function isCheClusterCustomResourceSpecDevEnvironments(
Expand Down Expand Up @@ -362,6 +365,11 @@ export interface IServerConfigApi {
* Returns the autoProvision value
*/
getAutoProvision(cheCustomResource: CheClusterCustomResource): boolean;

/**
* Returns the allowed source URLs
*/
getAllowedSourceUrls(cheCustomResource: CheClusterCustomResource): string[];
}

export interface IKubeConfigApi {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ describe('Server Config Route', () => {
base64data: 'base64-encoded-data',
mediatype: 'image/svg+xml',
},
allowedSourceUrls: [],
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,8 @@ export const stubAutoProvision = true;

export const stubAdvancedAuthorization = {};

export const stubAllowedSourceUrls: string[] = [];

export const stubWorkspacePreferences: api.IWorkspacePreferences = {
'skip-authorisation': ['github'],
'trusted-sources': '*',
Expand Down Expand Up @@ -186,6 +188,7 @@ export const getDevWorkspaceClient = jest.fn(
getDashboardLogo: _cheCustomResource => dashboardLogo,
getAutoProvision: _cheCustomResource => stubAutoProvision,
getAdvancedAuthorization: _cheCustomResource => stubAdvancedAuthorization,
getAllowedSourceUrls: _cheCustomResource => stubAllowedSourceUrls,
} as IServerConfigApi,
devworkspaceApi: {
create: (_devworkspace, _namespace) =>
Expand Down
2 changes: 2 additions & 0 deletions packages/dashboard-backend/src/routes/api/serverConfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ export function registerServerConfigRoute(instance: FastifyInstance) {
const dashboardLogo = serverConfigApi.getDashboardLogo(cheCustomResource);
const advancedAuthorization = serverConfigApi.getAdvancedAuthorization(cheCustomResource);
const autoProvision = serverConfigApi.getAutoProvision(cheCustomResource);
const allowedSourceUrls = serverConfigApi.getAllowedSourceUrls(cheCustomResource);

const serverConfig: api.IServerConfig = {
containerBuild,
Expand All @@ -72,6 +73,7 @@ export function registerServerConfigRoute(instance: FastifyInstance) {
cheNamespace,
pluginRegistryURL,
pluginRegistryInternalURL,
allowedSourceUrls,
};

if (dashboardLogo !== undefined) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,17 @@ describe('Untrusted Repo Warning Modal', () => {
expect(modal).toBeNull();
});

test('modal is hidden :: allowed sources configured', () => {
const store = storeBuilder
.withDwServerConfig({
allowedSourceUrls: ['*'],
})
.build();
renderComponent(store, 'source-location', false);
const modal = screen.queryByRole('dialog');
expect(modal).toBeNull();
});

test('modal is visible', () => {
const store = storeBuilder
.withWorkspacePreferences({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import { connect, ConnectedProps } from 'react-redux';

import { AppAlerts } from '@/services/alerts/appAlerts';
import { AppState } from '@/store';
import { selectIsAllowedSourcesConfigured } from '@/store/ServerConfig/selectors';
import { workspacePreferencesActionCreators } from '@/store/Workspaces/Preferences';
import {
selectPreferencesIsTrustedSource,
Expand All @@ -43,6 +44,7 @@ export type State = {
canContinue: boolean;
continueButtonDisabled: boolean;
isTrusted: boolean;
isAllowedSourcesConfigured: boolean;
trustAllCheckbox: boolean;
};

Expand All @@ -57,6 +59,7 @@ class UntrustedSourceModal extends React.Component<Props, State> {
canContinue: true,
continueButtonDisabled: false,
isTrusted: this.props.isTrustedSource(props.location),
isAllowedSourcesConfigured: this.props.isAllowedSourcesConfigured,
trustAllCheckbox: false,
};
}
Expand Down Expand Up @@ -84,26 +87,32 @@ class UntrustedSourceModal extends React.Component<Props, State> {
return true;
}

if (this.state.isAllowedSourcesConfigured !== nextState.isAllowedSourcesConfigured) {
return true;
}

return false;
}

public componentDidMount(): void {
if (this.props.isOpen && this.state.isTrusted) {
if (this.props.isOpen && (this.state.isTrusted || this.state.isAllowedSourcesConfigured)) {
this.handleContinue();
}
}

public componentDidUpdate(prevProps: Readonly<Props>): void {
const isTrusted = this.props.isTrustedSource(this.props.location);
const isAllowedSourcesConfigured = this.props.isAllowedSourcesConfigured;

this.setState({
isTrusted,
isAllowedSourcesConfigured,
});

if (
prevProps.isOpen === false &&
this.props.isOpen === true &&
isTrusted === true &&
(isTrusted === true || isAllowedSourcesConfigured) &&
this.state.canContinue === true
) {
this.handleContinue();
Expand Down Expand Up @@ -229,6 +238,7 @@ class UntrustedSourceModal extends React.Component<Props, State> {
const mapStateToProps = (state: AppState) => ({
trustedSources: selectPreferencesTrustedSources(state),
isTrustedSource: selectPreferencesIsTrustedSource(state),
isAllowedSourcesConfigured: selectIsAllowedSourcesConfigured(state),
});

const connector = connect(mapStateToProps, workspacePreferencesActionCreators, null, {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,32 @@ describe('Creating steps, initializing', () => {
expect(mockOnNextStep).not.toHaveBeenCalled();
});

test('source URL is not allowed', async () => {
const store = new FakeStoreBuilder()
.withDwServerConfig({ allowedSourceUrls: ['allowed-source'] })
.withInfrastructureNamespace([{ name: 'user-che', attributes: { phase: 'Active' } }])
.withSshKeys({
keys: [{ name: 'key1', keyPub: 'ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQD' }],
})
.build();
const searchParams = new URLSearchParams({
[FACTORY_URL_ATTR]: factoryUrl,
});

renderComponent(store, searchParams);

const stepTitle = screen.getByTestId('step-title');
expect(stepTitle.textContent).toContain('Initializing');

await jest.advanceTimersByTimeAsync(MIN_STEP_DURATION_MS);

const stepTitleNext = screen.getByTestId('step-title');
expect(stepTitleNext.textContent).toContain('Initializing');

expect(mockOnNextStep).not.toHaveBeenCalled();
expect(mockOnError).toHaveBeenCalled();
});

test('samples are trusted', async () => {
const store = new FakeStoreBuilder()
.withInfrastructureNamespace([{ name: 'user-che', attributes: { phase: 'Active' } }])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ import { AppState } from '@/store';
import { selectAllWorkspacesLimit } from '@/store/ClusterConfig/selectors';
import { selectIsRegistryDevfile } from '@/store/DevfileRegistries/selectors';
import { selectInfrastructureNamespaces } from '@/store/InfrastructureNamespaces/selectors';
import { isSourceAllowed } from '@/store/ServerConfig/helpers';
import { selectAllowedSources } from '@/store/ServerConfig/selectors';
import { selectSshKeys } from '@/store/SshKeys/selectors';
import { selectPreferencesIsTrustedSource } from '@/store/Workspaces/Preferences';
import { selectAllWorkspaces } from '@/store/Workspaces/selectors';
Expand All @@ -47,6 +49,8 @@ export type Props = MappedProps &
export type State = ProgressStepState & {
factoryParams: FactoryParams;
isSourceTrusted: boolean;
allowedSources: string[];
isSourceAllowed: boolean;
isWarning: boolean;
};

Expand All @@ -61,6 +65,8 @@ class CreatingStepInitialize extends ProgressStep<Props, State> {
isSourceTrusted: false,
name: this.name,
isWarning: false,
allowedSources: [],
isSourceAllowed: false,
};
}

Expand Down Expand Up @@ -101,6 +107,13 @@ class CreatingStepInitialize extends ProgressStep<Props, State> {
return true;
}

if (
this.state.isSourceAllowed !== nextState.isSourceAllowed ||
this.state.allowedSources !== nextState.allowedSources
) {
return true;
}

// name or warning changed
if (this.state.name !== nextState.name || this.state.isWarning !== nextState.isWarning) {
return true;
Expand All @@ -126,8 +139,17 @@ class CreatingStepInitialize extends ProgressStep<Props, State> {

// skip source validation for devworkspace resources (samples)
if (useDevWorkspaceResources === false) {
const isSourceAllowed = this.isSourceAllowed(sourceUrl);
if (isSourceAllowed === false) {
throw new Error(
`The specified source URL "${sourceUrl}" is not permitted for creating a workspace. Please contact your system administrator.`,
);
}

// check if the source is trusted
const isSourceTrusted = this.isSourceTrusted(sourceUrl);
const isSourceTrusted =
this.isSourceTrusted(sourceUrl) || this.props.allowedSources.length > 0;

if (isSourceTrusted === true) {
this.setState({
isSourceTrusted,
Expand Down Expand Up @@ -248,6 +270,12 @@ class CreatingStepInitialize extends ProgressStep<Props, State> {
return false;
}

private isSourceAllowed(sourceUrl: string): boolean {
const isRegistryDevfile = this.props.isRegistryDevfile(sourceUrl);
const isAllowed = isSourceAllowed(this.props.allowedSources, sourceUrl);

return isRegistryDevfile || isAllowed;
}
render(): React.ReactElement {
const { distance, hasChildren } = this.props;
const { name, lastError } = this.state;
Expand Down Expand Up @@ -292,6 +320,7 @@ const mapStateToProps = (state: AppState) => ({
infrastructureNamespaces: selectInfrastructureNamespaces(state),
isRegistryDevfile: selectIsRegistryDevfile(state),
isTrustedSource: selectPreferencesIsTrustedSource(state),
allowedSources: selectAllowedSources(state),
sshKeys: selectSshKeys(state),
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ const serverConfig: api.IServerConfig = {
},
pluginRegistryURL: '',
pluginRegistryInternalURL: '',
allowedSourceUrls: [],
};

describe('Starting steps, starting a workspace', () => {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* Copyright (c) 2018-2024 Red Hat, Inc.
* This program and the accompanying materials are made
* available under the terms of the Eclipse Public License 2.0
* which is available at https://www.eclipse.org/legal/epl-2.0/
*
* SPDX-License-Identifier: EPL-2.0
*
* Contributors:
* Red Hat, Inc. - initial API and implementation
*/

import { isSourceAllowed } from '@/store/ServerConfig/helpers';

describe('helpers', () => {
describe('isAllowedSourceUrl', () => {
test('allowed urls', () => {
expect(isSourceAllowed(['https://a/*'], 'https://a/b/c')).toBe(true);
expect(isSourceAllowed(['https://a/*/c'], 'https://a/b/c')).toBe(true);
expect(isSourceAllowed(['https://a/b/c'], 'https://a/b/c')).toBe(true);
expect(isSourceAllowed(['*'], 'https://a/b/c/')).toBe(true);
expect(isSourceAllowed(undefined, 'https://a/b/c')).toBe(true);
expect(isSourceAllowed([], 'https://a/b/c')).toBe(true);
});

test('disallowed urls', () => {
expect(isSourceAllowed(['https://a'], 'https://a/b/c/')).toBe(false);
});
});
});
Loading

0 comments on commit 8c8b3f8

Please sign in to comment.