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.

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

Workspace shell permissions are not remembered in multi-root workspaces #80674

Open
LAK132 opened this issue Sep 10, 2019 · 13 comments

Comments

@LAK132
Copy link

@LAK132 LAK132 commented Sep 10, 2019

My workflow requires the use of cygwin in particular workspaces, so I have my workspace settings set up to default to cygwin for the integrated terminal. The "Do you allow this workspace to modify your terminal shell?" comes up every single time I open this workspace, which is absolutely painful (requires using the mouse) when I always allow it.

image

This wouldn't be such an issue if it just showed up the first time I opened vscode, but it also shows up every time I duplicate the workspace as well (which I do regularly).

image

Is it possible to get an option to "Remember for this workspace", or at least to not ask every time I duplicate a workspace if I had already allowed/disallowed?

@vscodebot

This comment has been minimized.

Copy link

@vscodebot vscodebot bot commented Sep 10, 2019

(Experimental duplicate detection)
Thanks for submitting this issue. Please also check if it is already covered by an existing one, like:

@sandy081 sandy081 assigned Tyriar and unassigned sandy081 Sep 12, 2019
@Tyriar

This comment has been minimized.

Copy link
Member

@Tyriar Tyriar commented Sep 12, 2019

I guess the workspace shell permissions don't work currently for multi-root but they're meant to.

@Tyriar Tyriar added this to the Backlog milestone Sep 12, 2019
@Tyriar Tyriar changed the title Option to remember if I allow a workspace to modify terminal shell setting Workspace shell permissions are not remembered in multi-root workspaces Oct 9, 2019
@w9jds

This comment has been minimized.

Copy link
Contributor

@w9jds w9jds commented Oct 10, 2019

I'd be interested in grabbing this issue. Any specific place I should start looking?

@Tyriar

This comment has been minimized.

Copy link
Member

@Tyriar Tyriar commented Oct 10, 2019

Currently it's stored against the workspace:

public setWorkspaceShellAllowed(isAllowed: boolean): void {
this._onWorkspacePermissionsChanged.fire(isAllowed);
this._storageService.store(IS_WORKSPACE_SHELL_ALLOWED_STORAGE_KEY, isAllowed, StorageScope.WORKSPACE);
}
public isWorkspaceShellAllowed(defaultValue: boolean | undefined = undefined): boolean | undefined {
return this._storageService.getBoolean(IS_WORKSPACE_SHELL_ALLOWED_STORAGE_KEY, StorageScope.WORKSPACE, defaultValue);
}

Should probably be storing the folder against the user scope.

@w9jds

This comment has been minimized.

Copy link
Contributor

@w9jds w9jds commented Oct 10, 2019

So the storage scope only allows GLOBAL and WORKSPACE. Wouldn't we want to modify the duplicate action to properly duplicate the storage too, as it only copies settings currently?

async run(): Promise<any> {
const folders = this.workspaceContextService.getWorkspace().folders;
const remoteAuthority = this.environmentService.configuration.remoteAuthority;
const newWorkspace = await this.workspacesService.createUntitledWorkspace(folders, remoteAuthority);
await this.workspaceEditingService.copyWorkspaceSettings(newWorkspace);
return this.hostService.openWindow([{ workspaceUri: newWorkspace.configPath }], { forceNewWindow: true });

@Tyriar

This comment has been minimized.

Copy link
Member

@Tyriar Tyriar commented Oct 10, 2019

Yeah global is what we actually want. Not sure what you mean with the duplicate action, my thinking was changing the key to something that would be unique for a folder like ${IS_WORKSPACE_SHELL_ALLOWED_STORAGE_KEY}_${folderUri.toString()}

@w9jds

This comment has been minimized.

Copy link
Contributor

@w9jds w9jds commented Oct 11, 2019

If we make it global and locked to a specific folder wouldn't that just make it permanent for that folder regardless of what workspace the user is in?

So the original issue was that this happens when duplicating workspaces. Since the notification comes up when duplicating a workspace, and the action doesn't actually duplicate that workspace's storage too, shouldn't we just copy the storage for this value when the workspace is duplicated?

@Tyriar

This comment has been minimized.

Copy link
Member

@Tyriar Tyriar commented Oct 11, 2019

@w9jds but the problem is a little more general than that, duplicating the workspace is one way to get into this state, creating a new window and adding the folder as a workspace folder would also trigger this. The shell "workspace" permissions was added to ensure users trusted that particular folder to modify those settings, I would argue that the fact that the particular folder is in a different workspace doesn't matter, the user has already trusted the folder.

@Tyriar

This comment has been minimized.

Copy link
Member

@Tyriar Tyriar commented Oct 11, 2019

The naming kind of sucks with this thinking, "Manage Folder Shell Permissions" would be more correct if we go with this model.

@w9jds

This comment has been minimized.

Copy link
Contributor

@w9jds w9jds commented Oct 11, 2019

I can see that, I'll go ahead and see if I can get this working properly then based on folder instead of workspace.

@emilyoon

This comment has been minimized.

Copy link

@emilyoon emilyoon commented Nov 16, 2019

@w9jds are you still working on this? If not, @jhuree and I would like to take on this issue. Let us know if you have made any progress on this!

@w9jds

This comment has been minimized.

Copy link
Contributor

@w9jds w9jds commented Nov 17, 2019

@emilyoon sorry I got distracted, I didn't find a good way to reliably get the folder URI, so if you would like to grab this and finish it. Go for it!

@jhuree

This comment has been minimized.

Copy link

@jhuree jhuree commented Nov 20, 2019

@w9jds Sounds good, thanks for the quick reply. We'll work on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.