Implement light-weight Workspace Trust API for plugins #10473

martin-fleck-at · 2021-11-26T15:52:41Z

What it does

Add trust-relevant methods and interfaces to the plugin workspace API
Add request for workspace trust to workspace service
Allow application setting to auto-respond to trust request

Note: I share this PR to showcase how the issue might be solved as I tried to double-check the issue I was facing. While it is working and hopefully not doing any harm, further discussions and improvements might be needed for a real support of the workspace trust API.

How to test

Create a Theia Electron application as described in Workspace Trust API to support newer VS Code Git plugins #10472
Trigger the remote pickup through: Source Control View > Remote > Add Remote...

(working Remote query)

(user trust query)

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

packages/plugin-ext/src/main/browser/workspace-main.ts

packages/plugin-ext/src/plugin/workspace.ts

packages/workspace/src/browser/workspace-service.ts

packages/workspace/src/browser/workspace-trust-preferences.ts

packages/workspace/src/browser/workspace-trust-service.ts

packages/plugin-ext/src/plugin/workspace.ts

packages/workspace/src/browser/workspace-trust-service.ts

packages/plugin-ext/src/plugin/workspace.ts

packages/workspace/src/browser/workspace-trust-preferences.ts

packages/workspace/src/browser/workspace-trust-service.ts

martin-fleck-at · 2022-01-05T09:28:02Z

@colin-grant-work I hope you had a great start into the new year and thank you very much for your extensive review! It took me some time to come back to this but I updated the PR as you suggested and everything should now be working as expected.

colin-grant-work

Something seems to be malfunctioning. I modified my frontend config to enable workspace trust (see comment below), confirmed that, on startup, workspace trust was not resolved, and then ran the Add remote... command.

When I selected the Add remote from GitHub option, I got this notice:

That looks like a problem with the authentication API, not the workspace trust API (probably).

When I just put in the URL of the Theia repo, I was prompted to name the remote, and the process concluded. I was never prompted to confirm workspace trust, but I think I should have been? Or should the request only have come when using GitHub as an authentication provider?

colin-grant-work · 2022-01-25T17:55:55Z

packages/workspace/src/browser/workspace-trust-preferences.ts

+        [WORKSPACE_TRUST_ENABLED]: {
+            description: nls.localize('theia/workspace/trustEnabled', 'Controls whether or not workspace trust is enabled. If disabled, all workspaces are trusted.'),
+            type: 'boolean',
+            defaultValue: false


Is it your intent to make this disabled by default (i.e. make all workspaces trusted?)

Yes, that was the intent. I believe VS Code does it similarly, see https://github.com/microsoft/vscode/blob/f7bfe5ca6682f27bc0ef19b412a4888078265ecc/src/vs/workbench/services/workspaces/common/workspaceTrust.ts#L291

In VSCode, the workspace trust feature is definitely enabled by default. You are correct, though, that if workspace trust is not enabled, all workspaces are trusted.

I don't have a strong opinion about whether to enable the feature by default. Perhaps @JonasHelming can weigh in there, from the perspective of known adopters?

martin-fleck-at · 2022-01-26T15:28:36Z

@colin-grant-work That behavior is expected now that we no longer explicitly call requestWorkspaceTrust whenever a plugin queries the isTrusted flag. I had that originally but I think it was removed as part of the review #10473 (comment). So we either need to find a good trigger point to request the workspace trust, e.g., when an API registers a listener for it with onDidGrantWorkspaceTrust in which case the asynchronous nature does not hurt us or we expect the plugins to call that behavior explicitly which may be a bad idea seeing that the vscode git-plugin already does not do that. I suggest to add it on the onDidGrantWorkspaceTrust if the trusted flag is too risky, what do you think?

(Background: The vscode git plugin actually queries the workspace API, if you also install the github and github-authentication plugin from VSCode the dropdown should show you a little bit more but you'll still end up with a missing authentication provider since the activation event for github-authentication is not yet supported by Theia.)

colin-grant-work · 2022-01-26T16:15:02Z

@martin-fleck-at, that all sounds reasonable, and it explains why, the first time I ran through the test steps with workspace trust automatically enabled, I got an additional prompt asking me if I wanted to do automatic fetches, which is the feature that's checking workspace trust.

So as you say, the question is when to prompt, I guess. I find VSCode's prompt-on-start pretty annoying, so maybe your original idea of requesting trust when anyone checks for it is the best idea. The GitHub plugin immediately creates a listener if it finds the workspace is untrusted, and that seems like the only good way for a plugin to handle finding isTrusted === false. That has the benefit of not requesting trust if no one depends on it and requesting trust in a timely manner if a feature activates that does depend on it. In that case, though, we probably want to have a system that caches a definite negative response so that we don't pester the user with multiple requests if they've already denied it.

colin-grant-work · 2022-01-26T16:58:09Z

I've implemented a tiny VSCode plugin here with commands to exercise the workspace API.

martin-fleck-at · 2022-01-27T10:29:51Z

@colin-grant-work Thank you very much for the plugin, it was very useful to test the different scenarios! As you suggested, we now request the workspace trust whenever the trust is queried. In addition to that, we report any workspace trust state back to the plugin (onWorkspaceTrustChanged) even when we determine that it is false. In general, the user should not be pestered with multiple requests anyway as once we resolve the workspace trust no further dialogs are shown. This is also the reason that we trigger a restart when the respective preference was changed, similar to VS Code.

Thank you for your review and effort!

colin-grant-work

I'm happy with the code and the current behavior. The only outstanding question is whether to enable or disable the workspace trust feature by default, and I'd like to give others a chance to weigh in.

colin-grant-work · 2022-01-31T16:52:54Z

packages/workspace/src/browser/workspace-trust-preferences.ts

+        [WORKSPACE_TRUST_ENABLED]: {
+            description: nls.localize('theia/workspace/trustEnabled', 'Controls whether or not workspace trust is enabled. If disabled, all workspaces are trusted.'),
+            type: 'boolean',
+            defaultValue: false


In VSCode, the workspace trust feature is definitely enabled by default. You are correct, though, that if workspace trust is not enabled, all workspaces are trusted.

I don't have a strong opinion about whether to enable the feature by default. Perhaps @JonasHelming can weigh in there, from the perspective of known adopters?

JonasHelming · 2022-02-02T10:02:26Z

I think we should enable the feature by default as it provides a more defensive strategy when it comes to security. Adopters can still choose to disable the feature in their application default preferences if they accept any risk that may come with it. However, if they do not do that it is probably better to inform the user that a plugin requests workspace trust.

martin-fleck-at · 2022-02-02T10:34:25Z

@JonasHelming @colin-grant-work Thank you for your feedback! Thinking about it a bit more, I fully agree with Jonas that if no plugin requests the trust, the user is not bothered anyway but if a plugin needs it, it may be better to inform the user unless some adopters explicitly choose not to do so. As such, I changed the default enablement preference to true and squashed and rebased the change. If there are any more changes necessary before this can be merged, please let me know.

JonasHelming · 2022-02-02T10:45:24Z

@colin-grant-work : I would merge this now, OK?

- Add trust-relevant methods and interfaces to the plugin workspace API - Implement dedicated workspace trust service to manage trust - Add trust-specific settings to 'Security' category in preferences -- Enable/Disable trust feature: enabled by default -- Show trust prompot on startup: always by default -- Auto-trust empty workspaces: true by default -- Inform user about required application restart on preference change - Calculate trust the first time it is requested from a plugin

msujew

@colin-grant-work I don't really have a strong opinion on that. I think both would be fine, but I'm slightly leaning towards not enabling it by default.

In the end, downstream users of Theia can always choose to have it enabled by default in their product.

JonasHelming · 2022-02-02T16:07:46Z

Just to mention, I also do not have a strong opinion about it. So @colin-grant-work Your vote will decide on this, no option is "wrong"

colin-grant-work · 2022-02-04T23:08:03Z

@msujew, @JonasHelming, @martin-fleck-at, thanks for your input. As annoying as I find VSCode's modal at start up, I think enabling security by default is better than disabling it by default - adopters and users are free to follow whatever policies their risk managers will let them get away with :-). So default on sounds good to me, and I'll merge.

colin-grant-work

The latest code looks good to me, and I think we've settled on enabling the feature by default.

vince-fugnitto added the vscode issues related to VSCode compatibility label Nov 26, 2021

vince-fugnitto reviewed Nov 26, 2021

View reviewed changes

packages/plugin-ext/src/main/browser/workspace-main.ts Outdated Show resolved Hide resolved

packages/plugin-ext/src/plugin/workspace.ts Outdated Show resolved Hide resolved

colin-grant-work reviewed Nov 29, 2021

View reviewed changes

packages/workspace/src/browser/workspace-service.ts Outdated Show resolved Hide resolved

colin-grant-work self-requested a review December 1, 2021 15:47

colin-grant-work reviewed Dec 1, 2021

View reviewed changes

colin-grant-work self-requested a review December 2, 2021 14:48

colin-grant-work reviewed Dec 2, 2021

View reviewed changes

colin-grant-work self-requested a review December 3, 2021 15:04

colin-grant-work reviewed Dec 9, 2021

View reviewed changes

martin-fleck-at mentioned this pull request Jan 17, 2022

Add support for PluginContext.logUri and PluginContext.extension #10458

Closed

1 task

colin-grant-work self-requested a review January 24, 2022 15:43

colin-grant-work reviewed Jan 25, 2022

View reviewed changes

colin-grant-work self-requested a review January 25, 2022 18:00

JonasHelming assigned colin-grant-work Jan 26, 2022

colin-grant-work reviewed Jan 31, 2022

View reviewed changes

colin-grant-work self-requested a review January 31, 2022 16:54

msujew reviewed Feb 2, 2022

View reviewed changes

colin-grant-work approved these changes Feb 4, 2022

View reviewed changes

colin-grant-work merged commit 1425de1 into eclipse-theia:master Feb 4, 2022

vince-fugnitto added this to the 1.23.0 milestone Feb 24, 2022

marcdumais-work mentioned this pull request Dec 8, 2022

Workspace Trust API to support newer VS Code Git plugins #10472

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement light-weight Workspace Trust API for plugins #10473

Implement light-weight Workspace Trust API for plugins #10473

martin-fleck-at commented Nov 26, 2021 •

edited

Loading

martin-fleck-at commented Jan 5, 2022

colin-grant-work left a comment

colin-grant-work Jan 25, 2022

martin-fleck-at Jan 26, 2022

colin-grant-work Jan 31, 2022

martin-fleck-at commented Jan 26, 2022 •

edited

Loading

colin-grant-work commented Jan 26, 2022

colin-grant-work commented Jan 26, 2022

martin-fleck-at commented Jan 27, 2022

colin-grant-work left a comment

colin-grant-work Jan 31, 2022

JonasHelming commented Feb 2, 2022

martin-fleck-at commented Feb 2, 2022

JonasHelming commented Feb 2, 2022

msujew left a comment

JonasHelming commented Feb 2, 2022

colin-grant-work commented Feb 4, 2022

colin-grant-work left a comment

Implement light-weight Workspace Trust API for plugins #10473

Implement light-weight Workspace Trust API for plugins #10473

Conversation

martin-fleck-at commented Nov 26, 2021 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

martin-fleck-at commented Jan 5, 2022

colin-grant-work left a comment

Choose a reason for hiding this comment

colin-grant-work Jan 25, 2022

Choose a reason for hiding this comment

martin-fleck-at Jan 26, 2022

Choose a reason for hiding this comment

colin-grant-work Jan 31, 2022

Choose a reason for hiding this comment

martin-fleck-at commented Jan 26, 2022 • edited Loading

colin-grant-work commented Jan 26, 2022

colin-grant-work commented Jan 26, 2022

martin-fleck-at commented Jan 27, 2022

colin-grant-work left a comment

Choose a reason for hiding this comment

colin-grant-work Jan 31, 2022

Choose a reason for hiding this comment

JonasHelming commented Feb 2, 2022

martin-fleck-at commented Feb 2, 2022

JonasHelming commented Feb 2, 2022

msujew left a comment

Choose a reason for hiding this comment

JonasHelming commented Feb 2, 2022

colin-grant-work commented Feb 4, 2022

colin-grant-work left a comment

Choose a reason for hiding this comment

martin-fleck-at commented Nov 26, 2021 •

edited

Loading

martin-fleck-at commented Jan 26, 2022 •

edited

Loading