Skip to content

Remove control tower code for trusted notebooks #13394

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
joyceerhl merged 2 commits into from
Aug 12, 2020
Merged

Remove control tower code for trusted notebooks #13394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
85a30a2
Remove trusted notebooks experiment
joyceerhl Aug 12, 2020
2f3bbae
Fixes
joyceerhl Aug 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2069,7 +2069,6 @@
"CollectNodeLSRequestTiming - experiment",
"DeprecatePythonPath - experiment",
"RunByLine - experiment",
"EnableTrustedNotebooks",
"tryPylance",
"All"
]
Expand All @@ -2094,7 +2093,6 @@
"CollectNodeLSRequestTiming - experiment",
"DeprecatePythonPath - experiment",
"RunByLine - experiment",
"EnableTrustedNotebooks",
"tryPylance",
"All"
]
Expand Down Expand Up @@ -3759,4 +3757,4 @@
"publisherDisplayName": "Microsoft",
"publisherId": "998b010b-e2af-44a5-a6cd-0b5fd3b9b6f8"
}
}
}
5 changes: 0 additions & 5 deletions src/client/common/experiments/groups.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,6 @@ export enum RemoveKernelToolbarInInteractiveWindow {
experiment = 'RemoveKernelToolbarInInteractiveWindow'
}

// Experiment to turn on trusted notebooks checks
export enum EnableTrustedNotebooks {
experiment = 'EnableTrustedNotebooks'
}

// Experiment to offer switch to Pylance language server
export enum TryPylance {
experiment = 'tryPylance'
Expand Down
1 change: 0 additions & 1 deletion src/client/datascience/interactive-common/interactiveWindowTypes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,6 @@ export interface IRefreshVariablesRequest {
export interface ILoadAllCells {
cells: ICell[];
isNotebookTrusted?: boolean;
shouldShowTrustMessage?: boolean;
}

export interface IScrollToCell {
Expand Down
8 changes: 2 additions & 6 deletions src/client/datascience/interactive-ipynb/nativeEditor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@ import cloneDeep = require('lodash/cloneDeep');
import { concatMultilineString, splitMultilineString } from '../../../datascience-ui/common';
import { ServerStatus } from '../../../datascience-ui/interactive-common/mainState';
import { isTestExecution, PYTHON_LANGUAGE } from '../../common/constants';
import { EnableTrustedNotebooks } from '../../common/experiments/groups';
import { translateKernelLanguageToMonaco } from '../common';
import { IDataViewerFactory } from '../data-viewing/types';
import { getCellHashProvider } from '../editor-integration/cellhashprovider';
Expand Down Expand Up @@ -181,7 +180,7 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
notebookProvider: INotebookProvider,
useCustomEditorApi: boolean,
private trustService: ITrustService,
private expService: IExperimentService,
expService: IExperimentService,
private _model: INotebookModel,
webviewPanel: WebviewPanel | undefined,
selector: KernelSelector
Expand Down Expand Up @@ -225,7 +224,6 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
selector
);
asyncRegistry.push(this);

asyncRegistry.push(this.trustService.onDidSetNotebookTrust(this.monitorChangesToTrust, this));
this.synchronizer.subscribeToUserActions(this, this.postMessage.bind(this));

Expand Down Expand Up @@ -699,11 +697,9 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
private async sendInitialCellsToWebView(cells: ICell[], isNotebookTrusted: boolean): Promise<void> {
sendTelemetryEvent(Telemetry.CellCount, undefined, { count: cells.length });

const shouldShowTrustMessage = await this.expService.inExperiment(EnableTrustedNotebooks.experiment);
return this.postMessage(InteractiveWindowMessages.LoadAllCells, {
cells,
isNotebookTrusted,
shouldShowTrustMessage
isNotebookTrusted
});
}

Expand Down
9 changes: 2 additions & 7 deletions src/client/datascience/interactive-ipynb/trustCommandHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,8 @@ import { commands, Uri } from 'vscode';
import { IExtensionSingleActivationService } from '../../activation/types';
import { IApplicationShell, ICommandManager } from '../../common/application/types';
import { ContextKey } from '../../common/contextKey';
import { EnableTrustedNotebooks } from '../../common/experiments/groups';
import '../../common/extensions';
import { IDisposableRegistry, IExperimentService } from '../../common/types';
import { IDisposableRegistry } from '../../common/types';
import { swallowExceptions } from '../../common/utils/decorators';
import { DataScience } from '../../common/utils/localize';
import { sendTelemetryEvent } from '../../telemetry';
Expand All @@ -26,16 +25,12 @@ export class TrustCommandHandler implements IExtensionSingleActivationService {
@inject(INotebookStorageProvider) private readonly storageProvider: INotebookStorageProvider,
@inject(ICommandManager) private readonly commandManager: ICommandManager,
@inject(IApplicationShell) private readonly applicationShell: IApplicationShell,
@inject(IDisposableRegistry) private readonly disposables: IDisposableRegistry,
@inject(IExperimentService) private readonly experiments: IExperimentService
@inject(IDisposableRegistry) private readonly disposables: IDisposableRegistry
) {}
public async activate(): Promise<void> {
this.activateInBackground().ignoreErrors();
}
public async activateInBackground(): Promise<void> {
if (!(await this.experiments.inExperiment(EnableTrustedNotebooks.experiment))) {
return;
}
const context = new ContextKey('python.datascience.trustfeatureenabled', this.commandManager);
context.set(true).ignoreErrors();
this.disposables.push(this.commandManager.registerCommand(Commands.TrustNotebook, this.onTrustNotebook, this));
Expand Down
19 changes: 5 additions & 14 deletions src/client/datascience/interactive-ipynb/trustService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import { createHmac } from 'crypto';
import { inject, injectable } from 'inversify';
import { EventEmitter, Uri } from 'vscode';
import { EnableTrustedNotebooks } from '../../common/experiments/groups';
import { IConfigurationService, IExperimentService } from '../../common/types';
import { IConfigurationService } from '../../common/types';
import { IDigestStorage, ITrustService } from '../types';

@injectable()
Expand All @@ -14,14 +13,10 @@ export class TrustService implements ITrustService {
return this.configService.getSettings().datascience.alwaysTrustNotebooks;
}
protected readonly _onDidSetNotebookTrust = new EventEmitter<void>();
private enabled: Promise<boolean>;
constructor(
@inject(IExperimentService) private readonly experimentService: IExperimentService,
@inject(IDigestStorage) private readonly digestStorage: IDigestStorage,
@inject(IConfigurationService) private configService: IConfigurationService
) {
this.enabled = this.isInExperiment();
}
) {}

/**
* When a notebook is opened, we check the database to see if a trusted checkpoint
Expand All @@ -31,8 +26,8 @@ export class TrustService implements ITrustService {
* markdown will be rendered until notebook as a whole is marked trusted
*/
public async isNotebookTrusted(uri: Uri, notebookContents: string) {
if (this.alwaysTrustNotebooks || !(await this.enabled)) {
return true; // Skip check if user manually overrode our trust checking, or if user is not in experiment
if (this.alwaysTrustNotebooks) {
return true; // Skip check if user manually overrode our trust checking
Copy link
Member

@IanMatthewHuff IanMatthewHuff Aug 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated comments 👍

}
// Compute digest and see if notebook is trusted
const digest = await this.computeDigest(notebookContents);
Expand All @@ -45,7 +40,7 @@ export class TrustService implements ITrustService {
* I.e. if the notebook has already been trusted by the user
*/
public async trustNotebook(uri: Uri, notebookContents: string) {
if (!this.alwaysTrustNotebooks && (await this.enabled)) {
if (!this.alwaysTrustNotebooks) {
// Only update digest store if the user wants us to check trust
const digest = await this.computeDigest(notebookContents);
await this.digestStorage.saveDigest(uri, digest);
Expand All @@ -58,8 +53,4 @@ export class TrustService implements ITrustService {
hmac.update(notebookContents);
return hmac.digest('hex');
}

private async isInExperiment() {
return this.experimentService.inExperiment(EnableTrustedNotebooks.experiment);
}
}
16 changes: 7 additions & 9 deletions src/datascience-ui/interactive-common/jupyterInfo.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,15 +94,13 @@ export class JupyterInfo extends React.Component<IJupyterInfoProps> {
}

private renderTrustMessage() {
if (this.props.shouldShowTrustMessage) {
return (
<TrustMessage
shouldShowTrustMessage={this.props.shouldShowTrustMessage}
isNotebookTrusted={this.props.isNotebookTrusted}
launchNotebookTrustPrompt={this.props.launchNotebookTrustPrompt}
/>
);
}
return (
<TrustMessage
shouldShowTrustMessage={this.props.shouldShowTrustMessage}
isNotebookTrusted={this.props.isNotebookTrusted}
launchNotebookTrustPrompt={this.props.launchNotebookTrustPrompt}
/>
);
}

private selectKernel() {
Expand Down
4 changes: 1 addition & 3 deletions src/datascience-ui/interactive-common/mainState.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ export type IMainState = {
loaded: boolean;
kernel: IServerState;
isNotebookTrusted: boolean;
shouldShowTrustMessage: boolean;
};

export type SelectionAndFocusedInfo = {
Expand Down Expand Up @@ -198,8 +197,7 @@ export function generateTestState(filePath: string = '', editable: boolean = fal
jupyterServerStatus: ServerStatus.NotStarted,
language: PYTHON_LANGUAGE
},
isNotebookTrusted: true,
shouldShowTrustMessage: true
isNotebookTrusted: true
};
}

Expand Down
3 changes: 1 addition & 2 deletions src/datascience-ui/interactive-common/redux/store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,7 @@ function generateDefaultState(
},
settings: testMode ? getDefaultSettings() : undefined, // When testing, we don't send (or wait) for the real settings.
editorOptions: testMode ? computeEditorOptions(getDefaultSettings()) : undefined,
isNotebookTrusted: true,
shouldShowTrustMessage: false
isNotebookTrusted: true
};
}
}
Expand Down
7 changes: 1 addition & 6 deletions src/datascience-ui/native-editor/nativeEditor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,12 +126,7 @@ ${buildSettingsCss(this.props.settings)}`}</style>
setTimeout(() => this.props.insertAboveFirst(), 1);
}
private renderToolbarPanel() {
return (
<ToolbarComponent
shouldShowTrustMessage={this.props.shouldShowTrustMessage}
isNotebookTrusted={this.props.isNotebookTrusted}
></ToolbarComponent>
);
return <ToolbarComponent isNotebookTrusted={this.props.isNotebookTrusted}></ToolbarComponent>;
}

private renderVariablePanel(baseTheme: string) {
Expand Down
3 changes: 1 addition & 2 deletions src/datascience-ui/native-editor/redux/reducers/creation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,8 +338,7 @@ export namespace Creation {
undoStack: [],
cellVMs: vms,
loaded: true,
isNotebookTrusted: arg.payload.data.isNotebookTrusted!,
shouldShowTrustMessage: arg.payload.data.shouldShowTrustMessage!
isNotebookTrusted: arg.payload.data.isNotebookTrusted!
};
}

Expand Down
3 changes: 1 addition & 2 deletions src/datascience-ui/native-editor/toolbar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ export type INativeEditorToolbarProps = INativeEditorDataProps & {
selectServer: typeof actionCreators.selectServer;
launchNotebookTrustPrompt: typeof actionCreators.launchNotebookTrustPrompt;
isNotebookTrusted: boolean;
shouldShowTrustMessage: boolean;
};

function mapStateToProps(state: IStore): INativeEditorDataProps {
Expand Down Expand Up @@ -264,7 +263,7 @@ export class Toolbar extends React.PureComponent<INativeEditorToolbarProps> {
kernel={this.props.kernel}
selectServer={selectServer}
selectKernel={selectKernel}
shouldShowTrustMessage={this.props.shouldShowTrustMessage}
shouldShowTrustMessage={true}
isNotebookTrusted={this.props.isNotebookTrusted}
launchNotebookTrustPrompt={launchNotebookTrustPrompt}
/>
Expand Down
3 changes: 1 addition & 2 deletions src/test/datascience/.vscode/settings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@
"python.pythonPath": "python",
"python.experiments.optInto": [
"LocalZMQKernel - experiment",
"NativeNotebook - experiment",
"EnableTrustedNotebooks"
"NativeNotebook - experiment"
],
// Do not set this to "Microsoft", else it will result in LS being downloaded on CI
// and that slows down tests significantly. We have other tests on CI for testing
Expand Down
3 changes: 1 addition & 2 deletions src/test/datascience/dataScienceIocContainer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ import {
import { CryptoUtils } from '../../client/common/crypto';
import { DotNetCompatibilityService } from '../../client/common/dotnet/compatibilityService';
import { IDotNetCompatibilityService } from '../../client/common/dotnet/types';
import { EnableTrustedNotebooks, LocalZMQKernel } from '../../client/common/experiments/groups';
import { LocalZMQKernel } from '../../client/common/experiments/groups';
import { ExperimentsManager } from '../../client/common/experiments/manager';
import { ExperimentService } from '../../client/common/experiments/service';
import { InstallationChannelManager } from '../../client/common/installer/channelManager';
Expand Down Expand Up @@ -621,7 +621,6 @@ export class DataScienceIocContainer extends UnitTestIocContainer {
this.serviceManager.add<IStartPage>(IStartPage, StartPage);

const experimentService = mock(ExperimentService);
when(experimentService.inExperiment(EnableTrustedNotebooks.experiment)).thenResolve(true);
this.serviceManager.addSingletonInstance<IExperimentService>(IExperimentService, instance(experimentService));

this.serviceManager.addSingleton<IApplicationEnvironment>(IApplicationEnvironment, ApplicationEnvironment);
Expand Down
30 changes: 2 additions & 28 deletions src/test/datascience/interactive-common/trustCommandHandler.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,7 @@ import { IExtensionSingleActivationService } from '../../../client/activation/ty
import { IApplicationShell, ICommandManager } from '../../../client/common/application/types';
import { ContextKey } from '../../../client/common/contextKey';
import { CryptoUtils } from '../../../client/common/crypto';
import { EnableTrustedNotebooks } from '../../../client/common/experiments/groups';
import { IDisposable, IExperimentService } from '../../../client/common/types';
import { IDisposable } from '../../../client/common/types';
import { DataScience } from '../../../client/common/utils/localize';
import { Commands } from '../../../client/datascience/constants';
import { TrustCommandHandler } from '../../../client/datascience/interactive-ipynb/trustCommandHandler';
Expand All @@ -36,7 +35,6 @@ suite('DataScience - Trust Command Handler', () => {
let disposables: IDisposable[] = [];
let clock: fakeTimers.InstalledClock;
let contextKeySet: sinon.SinonStub<[boolean], Promise<void>>;
let experiments: IExperimentService;
let model: INotebookModel;
let trustNotebookCommandCallback: (uri: Uri) => Promise<void>;
let testIndex = 0;
Expand All @@ -54,12 +52,7 @@ suite('DataScience - Trust Command Handler', () => {
when(storageProvider.getOrCreateModel(anything())).thenResolve(model);
disposables = [];

experiments = mock<IExperimentService>();

when(trustService.trustNotebook(anything(), anything())).thenResolve();
when(experiments.inExperiment(anything())).thenCall((exp) =>
Promise.resolve(exp === EnableTrustedNotebooks.experiment)
);
when(commandManager.registerCommand(anything(), anything(), anything())).thenCall(() => ({ dispose: noop }));
when(commandManager.registerCommand(Commands.TrustNotebook, anything(), anything())).thenCall((_, cb) => {
trustNotebookCommandCallback = cb.bind(trustCommandHandler);
Expand All @@ -72,8 +65,7 @@ suite('DataScience - Trust Command Handler', () => {
instance(storageProvider),
instance(commandManager),
instance(applicationShell),
disposables,
instance(experiments)
disposables
);

clock = fakeTimers.install();
Expand All @@ -87,24 +79,6 @@ suite('DataScience - Trust Command Handler', () => {
clock.uninstall();
});

test('Context not set if not in experiment', async () => {
when(experiments.inExperiment(anything())).thenResolve(false);

await trustCommandHandler.activate();
await clock.runAllAsync();

assert.equal(contextKeySet.callCount, 0);
});
test('Context set if in experiment', async () => {
when(experiments.inExperiment(anything())).thenCall((exp) =>
Promise.resolve(exp === EnableTrustedNotebooks.experiment)
);

await trustCommandHandler.activate();
await clock.runAllAsync();

assert.equal(contextKeySet.callCount, 1);
});
test('Executing command will not update trust after dismissing the prompt', async () => {
when(applicationShell.showErrorMessage(anything(), anything(), anything(), anything())).thenResolve(
undefined as any
Expand Down
6 changes: 1 addition & 5 deletions src/test/datascience/interactive-common/trustService.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import { anything, instance, mock, when } from 'ts-mockito';
import * as typemoq from 'typemoq';
import { Uri } from 'vscode';
import { ConfigurationService } from '../../../client/common/configuration/service';
import { ExperimentService } from '../../../client/common/experiments/service';
import { IExtensionContext } from '../../../client/common/types';
import { DataScienceFileSystem } from '../../../client/datascience/dataScienceFileSystem';
import { DigestStorage } from '../../../client/datascience/interactive-ipynb/digestStorage';
Expand All @@ -21,7 +20,6 @@ suite('DataScience - TrustService', () => {
setup(() => {
alwaysTrustNotebooks = false;
const configService = mock(ConfigurationService);
const experimentService = mock(ExperimentService);
const fileSystem = mock(DataScienceFileSystem);
const context = typemoq.Mock.ofType<IExtensionContext>();
context.setup((c) => c.globalStoragePath).returns(() => os.tmpdir());
Expand All @@ -33,10 +31,8 @@ suite('DataScience - TrustService', () => {
when(fileSystem.readLocalFile(anything())).thenCall((f) => fs.readFile(f));
when(fileSystem.createLocalDirectory(anything())).thenCall((d) => fs.mkdir(d));
when(fileSystem.localDirectoryExists(anything())).thenCall((d) => fs.pathExists(d));
when(experimentService.inExperiment(anything())).thenResolve(true); // Pretend we're in an experiment so that trust checks proceed

const digestStorage = new DigestStorage(instance(fileSystem), context.object);
trustService = new TrustService(instance(experimentService), digestStorage, instance(configService));
trustService = new TrustService(digestStorage, instance(configService));
});

test('Trusting a notebook', async () => {
Expand Down
3 changes: 1 addition & 2 deletions src/test/datascience/interactivePanel.functional.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,7 @@ suite('DataScience Interactive Panel', () => {
setVariableExplorerHeight: noopAny,
editorOptions: {},
settings: { showCellInputCode: true, allowInput: true, extraSettings: { editor: {} } } as any,
isNotebookTrusted: true,
shouldShowTrustMessage: false
isNotebookTrusted: true
};
});
test('Input Cell is displayed', () => {
Expand Down
Loading