-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(workspace): implement workspace trust dialog and management #16809
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
base: master
Are you sure you want to change the base?
Conversation
Adds workspace trust functionality: - Trust dialog on folder open (respecting startupPrompt preference) - Trusted folders list preference for global trust decisions - Per-workspace trust storage for 'once' mode - Restricted Mode banner when workspace is untrusted - "Manage Workspace Trust" command to toggle trust state - onDidChangeWorkspaceTrust event for plugins Plugin API: - Add onDidChangeWorkspaceTrust: Event<boolean> to workspace namespace - Update workspace.ts and workspace-main.ts for RPC event flow Preferences added: - security.workspace.trust.trustedFolders (string array) Closes #16653
ndoschek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @eneufeld, thanks for this improvement!
Unfortunately it does not work as expected for me. If I trust new folders, the decision is not saved, i.e., I cannot see it in the settings and hence I get prompted on each reload.
If I add the path manually via the settings UI it works, however only after a manual reload, it seems the manual settings change does not trigger the update listener.
Could you have a look if it behaves the same on your machine?
Besides that, I added a few minor comments inline, would be great if you could have a look. TIA!
ndoschek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update Eugen!
It works much better now, but I still found a few cases which are not working correctly, yet.
- After hitting Yes, I trust the authors in the dialog, I still get the restricted mode popup which is incocrrect, as the trusted path has been saved to the settings alrleady.
Also the additional notification from before keeps showing after I trust the workspace. Not sure if we should keep this at all? wdyt? In either case we should clear this popup if we trusted the workspace.
-
If i cancel the trust dialog, the restriced mode popup shows as expected. But then the "Manage Workspace Trust" does not work if I try to select trust, this does not update the settings accordingly.
-
The dialog does not show for an empty window if the setting is disabled:
Overall it looks good to me, it also works fine with the remaining settings 👍
Would be great if you could have another look, TIA!
|
@ndoschek about If we have an empty window we don't have a workspace that we can trust or not trust. |
|
Hi @eneufeld, thank you for the updates! About the emptyWindow setting: Ah that totally makes sense, thanks! And unfortunately, point 1. from my previous comment is still not resolved for me:
And as mentioned above, the restricted popup does not listen to any settings change, either for the emptyWindow above. Also in this case: dismiss the dialog, restricted popup appears, executing the command and trust the workspace => the trust setting was updated for this folder, but the restricted popup is still showing and has to be dismissed manually. I think we should clear the restricted popup when the settings change. Would be great if you could have another look. |
ndoschek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @eneufeld this is a great improvement, and I think we are very close now.
I added a few comments inline, but nothing major.
Besides that one of my previous comments is still unresolved.
The notification we are now still getting is the one shown below, I would still be in favour of removing it, wdyt?
Also, the actions do not work properly, e.g. if I dismiss the main dialog and then click on yes in this notification the workspace is not trusted.
Besides that, as discussed offline, I will work on the theme update soon and then we can also make use of the different styles for status bar items (there is a prominent styling for status bar items, which we should the pick for this item).
But I can take care of that in a follow up.
| const items: QuickPickItem[] = [ | ||
| { | ||
| label: trust, | ||
| description: currentTrust ? nls.localize('theia/workspace/currentTrustState', '(current)') : undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| description: currentTrust ? nls.localize('theia/workspace/currentTrustState', '(current)') : undefined | |
| const currentSuffix = `(${nls.localizeByDefault('current')})`, | |
| ... | |
| description: currentTrust ? currentSuffix : undefined |
And also for the item below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test file creates emitters but doesn't dispose of them after tests complete. While this may not cause issues in isolated tests, it's good practice to clean up.
| priority: 5000, | ||
| tooltip: nls.localize('theia/workspace/restrictedModeTooltip', | ||
| 'Running in Restricted Mode. Some features are disabled because this folder is not trusted. Click to manage trust settings.'), | ||
| command: 'workspace:manageTrust' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| command: 'workspace:manageTrust' | |
| command: MANAGE_WORKSPACE_TRUST.id |
| return trustedFolders.some(folder => { | ||
| // Strip trailing slash from folder string before creating URI | ||
| const normalizedFolder = folder.endsWith('/') ? folder.slice(0, -1) : folder; | ||
| const folderUri = new URI(normalizedFolder); | ||
| return workspaceUri.isEqual(folderUri, caseSensitive); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use URI's built-in normalization?
| return trustedFolders.some(folder => { | |
| // Strip trailing slash from folder string before creating URI | |
| const normalizedFolder = folder.endsWith('/') ? folder.slice(0, -1) : folder; | |
| const folderUri = new URI(normalizedFolder); | |
| return workspaceUri.isEqual(folderUri, caseSensitive); | |
| }); | |
| return trustedFolders.some(folder => { | |
| try { | |
| const folderUri = new URI(folder).normalizePath(); | |
| return workspaceUri.normalizePath().isEqual(folderUri, caseSensitive); | |
| } catch { | |
| return false; // Invalid URI in preferences | |
| } | |
| }); |
| // React to trust changes | ||
| this.onDidChangeWorkspaceTrust(trust => { | ||
| this.updateRestrictedModeIndicator(trust); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This subscription should be ideally stored in a disposable collection, as we never dispose it., probably also for the other subscriptions above.
What it does
Adds workspace trust functionality:
Plugin API:
Preferences added:
Closes #16653
How to test
Open a new workspace after enabling workspace trust checks.
See a dialog pop up. Test that trusted workspaces don't show dialog anymore. Untrusted workspaces show a message.
Follow-ups
Breaking changes
Attribution
Review checklist
nlsservice (for details, please see the Internationalization/Localization section in the Coding Guidelines)Reminder for reviewers