Skip to content
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

Jump to bottom

improve trust handling in tasks #160362

Merged
merged 15 commits into from Sep 8, 2022
Merged

improve trust handling in tasks #160362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
e6da8ff
fix #160013
meganrogge Sep 7, 2022
3a812e9
rm var
meganrogge Sep 7, 2022
2f8ace4
fix #160368
meganrogge Sep 8, 2022
80bc53a
actually call isWorkspaceTrusted
meganrogge Sep 8, 2022
1a90bb1
Revert "fix #160368"
meganrogge Sep 8, 2022
3f67611
Revert "rm var"
meganrogge Sep 8, 2022
0be7d1f
Revert "fix #160013"
meganrogge Sep 8, 2022
b3dad68
simplify fix
meganrogge Sep 8, 2022
13ef309
simplify
meganrogge Sep 8, 2022
f0d01c0
further simplify
meganrogge Sep 8, 2022
872c907
static -> private
meganrogge Sep 8, 2022
4f30ea0
Update src/vs/workbench/contrib/tasks/browser/runAutomaticTasks.ts
meganrogge Sep 8, 2022
61e4441
set boolean after conditional
meganrogge Sep 8, 2022
e40e8b9
remove additional check
meganrogge Sep 8, 2022
8781dfc
rearrange
meganrogge Sep 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 0 additions & 5 deletions src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts
View file
Open in desktop
Expand Up @@ -58,7 +58,6 @@ import { TerminalTaskSystem } from './terminalTaskSystem';
import { IQuickInputService, IQuickPick, IQuickPickItem, QuickPickInput } from 'vs/platform/quickinput/common/quickInput';

import { IContextKey, IContextKeyService } from 'vs/platform/contextkey/common/contextkey';
import { RunAutomaticTasks } from 'vs/workbench/contrib/tasks/browser/runAutomaticTasks';
import { TaskDefinitionRegistry } from 'vs/workbench/contrib/tasks/common/taskDefinitionRegistry';

import { CancellationToken, CancellationTokenSource } from 'vs/base/common/cancellation';
Expand Down Expand Up @@ -1220,10 +1219,6 @@ export abstract class AbstractTaskService extends Disposable implements ITaskSer
} else {
executeTaskResult = await this._executeTask(task, resolver, runSource);
}
if (runSource === TaskRunSource.User) {
const workspaceTasks = await this.getWorkspaceTasks();
RunAutomaticTasks.runWithPermission(this, this._storageService, this._notificationService, this._workspaceTrustManagementService, this._openerService, this._configurationService, workspaceTasks);
meganrogge marked this conversation as resolved.
Show resolved Hide resolved
}
return executeTaskResult;
} catch (error) {
this._handleError(error);
Expand Down
42 changes: 24 additions & 18 deletions src/vs/workbench/contrib/tasks/browser/runAutomaticTasks.ts
View file
Open in desktop
Expand Up @@ -25,6 +25,7 @@ const HAS_PROMPTED_FOR_AUTOMATIC_TASKS = 'task.hasPromptedForAutomaticTasks';
const ALLOW_AUTOMATIC_TASKS = 'task.allowAutomaticTasks';

export class RunAutomaticTasks extends Disposable implements IWorkbenchContribution {
private _hasRunTasks: boolean = false;
constructor(
@ITaskService private readonly _taskService: ITaskService,
@IConfigurationService private readonly _configurationService: IConfigurationService,
Expand All @@ -34,23 +35,33 @@ export class RunAutomaticTasks extends Disposable implements IWorkbenchContribut
@IOpenerService private readonly _openerService: IOpenerService,
@INotificationService private readonly _notificationService: INotificationService) {
super();
this._tryRunTasks();
if (this._workspaceTrustManagementService.isWorkspaceTrusted()) {
this._tryRunTasks();
}
this._register(this._workspaceTrustManagementService.onDidChangeTrust(async trusted => {
if (trusted) {
await this._tryRunTasks();
}
}));
}

private async _tryRunTasks() {
if (this._hasRunTasks || this._configurationService.getValue(ALLOW_AUTOMATIC_TASKS) === 'off') {
return;
}
this._hasRunTasks = true;
this._logService.trace('RunAutomaticTasks: Trying to run tasks.');
// Wait until we have task system info (the extension host and workspace folders are available).
if (!this._taskService.hasTaskSystemInfo) {
this._logService.trace('RunAutomaticTasks: Awaiting task system info.');
await Event.toPromise(Event.once(this._taskService.onDidChangeTaskSystemInfo));
}

const workspaceTasks = await this._taskService.getWorkspaceTasks(TaskRunSource.FolderOpen);
this._logService.trace(`RunAutomaticTasks: Found ${workspaceTasks.size} automatic tasks`);
await RunAutomaticTasks.runWithPermission(this._taskService, this._storageService, this._notificationService, this._workspaceTrustManagementService, this._openerService, this._configurationService, workspaceTasks);
await this._runWithPermission(this._taskService, this._storageService, this._notificationService, this._workspaceTrustManagementService, this._openerService, this._configurationService, workspaceTasks);
}

private static _runTasks(taskService: ITaskService, tasks: Array<Task | Promise<Task | undefined>>) {
private _runTasks(taskService: ITaskService, tasks: Array<Task | Promise<Task | undefined>>) {
tasks.forEach(task => {
if (task instanceof Promise) {
task.then(promiseResult => {
Expand All @@ -64,7 +75,7 @@ export class RunAutomaticTasks extends Disposable implements IWorkbenchContribut
});
}

private static _getTaskSource(source: TaskSource): URI | undefined {
private _getTaskSource(source: TaskSource): URI | undefined {
const taskKind = TaskSourceKind.toConfigurationTarget(source.kind);
switch (taskKind) {
case ConfigurationTarget.WORKSPACE_FOLDER: {
Expand All @@ -77,7 +88,7 @@ export class RunAutomaticTasks extends Disposable implements IWorkbenchContribut
return undefined;
}

private static _findAutoTasks(taskService: ITaskService, workspaceTaskResult: Map<string, IWorkspaceFolderTaskResult>): { tasks: Array<Task | Promise<Task | undefined>>; taskNames: Array<string>; locations: Map<string, URI> } {
private _findAutoTasks(taskService: ITaskService, workspaceTaskResult: Map<string, IWorkspaceFolderTaskResult>): { tasks: Array<Task | Promise<Task | undefined>>; taskNames: Array<string>; locations: Map<string, URI> } {
const tasks = new Array<Task | Promise<Task | undefined>>();
const taskNames = new Array<string>();
const locations = new Map<string, URI>();
Expand All @@ -89,7 +100,7 @@ export class RunAutomaticTasks extends Disposable implements IWorkbenchContribut
if (task.runOptions.runOn === RunOnOptions.folderOpen) {
tasks.push(task);
taskNames.push(task._label);
const location = RunAutomaticTasks._getTaskSource(task._source);
const location = this._getTaskSource(task._source);
if (location) {
locations.set(location.fsPath, location);
}
Expand All @@ -107,7 +118,7 @@ export class RunAutomaticTasks extends Disposable implements IWorkbenchContribut
} else {
taskNames.push(configuredTask.configures.task);
}
const location = RunAutomaticTasks._getTaskSource(configuredTask._source);
const location = this._getTaskSource(configuredTask._source);
if (location) {
locations.set(location.fsPath, location);
}
Expand All @@ -119,35 +130,30 @@ export class RunAutomaticTasks extends Disposable implements IWorkbenchContribut
return { tasks, taskNames, locations };
}

public static async runWithPermission(taskService: ITaskService, storageService: IStorageService, notificationService: INotificationService, workspaceTrustManagementService: IWorkspaceTrustManagementService,
private async _runWithPermission(taskService: ITaskService, storageService: IStorageService, notificationService: INotificationService, workspaceTrustManagementService: IWorkspaceTrustManagementService,
openerService: IOpenerService, configurationService: IConfigurationService, workspaceTaskResult: Map<string, IWorkspaceFolderTaskResult>) {
const isWorkspaceTrusted = workspaceTrustManagementService.isWorkspaceTrusted;
if (!isWorkspaceTrusted || configurationService.getValue(ALLOW_AUTOMATIC_TASKS) === 'off') {
return;
}

const hasShownPromptForAutomaticTasks = storageService.getBoolean(HAS_PROMPTED_FOR_AUTOMATIC_TASKS, StorageScope.WORKSPACE, false);
const { tasks, taskNames, locations } = RunAutomaticTasks._findAutoTasks(taskService, workspaceTaskResult);
const { tasks, taskNames, locations } = this._findAutoTasks(taskService, workspaceTaskResult);

if (taskNames.length === 0) {
return;
}

if (configurationService.getValue(ALLOW_AUTOMATIC_TASKS) === 'on') {
RunAutomaticTasks._runTasks(taskService, tasks);
this._runTasks(taskService, tasks);
} else if (configurationService.getValue(ALLOW_AUTOMATIC_TASKS) === 'auto' && !hasShownPromptForAutomaticTasks) {
// by default, only prompt once per folder
// otherwise, this can be configured via the setting
this._showPrompt(notificationService, storageService, openerService, configurationService, taskNames, locations).then(allow => {
Copy link
Member

Choose a reason for hiding this comment

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

If I'm reading this correctly it looks like we now only show the prompt on folder open in folders where there are automatic tasks. This removes the "context-aware" approach that we used to have: show the prompt when the user has just run a task. The context-aware approach was done to avoid contributing to the notification fatigue that we have upon opening a folder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My concern is that if I configure automatic tasks to happen on folderOpen, when I open the folder, i'd expect them to run or for me to be asked if they can run and would feel they're broken if it doesn't happen.

if (allow) {
storageService.store(HAS_PROMPTED_FOR_AUTOMATIC_TASKS, true, StorageScope.WORKSPACE, StorageTarget.USER);
RunAutomaticTasks._runTasks(taskService, tasks);
this._runTasks(taskService, tasks);
}
});
}
}

private static _showPrompt(notificationService: INotificationService, storageService: IStorageService,
private _showPrompt(notificationService: INotificationService, storageService: IStorageService,
openerService: IOpenerService, configurationService: IConfigurationService, taskNames: Array<string>, locations: Map<string, URI>): Promise<boolean> {
return new Promise<boolean>(resolve => {
notificationService.prompt(Severity.Info, nls.localize('tasks.run.allowAutomatic',
Expand Down