-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
vsx-registry: implement verified extension filtering #12995
Conversation
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.
See the comments. There is also the problem that when I change the "only show verified extensions" preference in the preference editor, the VSX search results are not refreshed. When I click the little start icon in the search field, it is refreshed.
version: allVersions.version | ||
})); | ||
searchResult.add(id); | ||
const res = await client.query({ extensionId: id, extensionVersion: allVersions.version, includeAllVersions: true }); |
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.
We're doing an extra query for each extension. Can't we get this info from the initial query? If not, let's open an issue a https://github.com/eclipse/openvsx to either be able to specify "verified" as a criterion or to get the flag in the search result.
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.
During the search, we are using the Search API (https://open-vsx.org/swagger-ui/index.html#/registry-api/search) to get the list of extensions but they do not include the verified
flag, which is why I had to do this extra query.
I have opened an issue: eclipse/openvsx#824
Thanks for the feedback.
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 opening the issue, it would be by far the best solution to just do this in the original search.
|
msg: nls.localize('theia/vsx-registry/confirmDialogMessage', 'The extension "{0}" is unverified and might pose a security risk.', this.displayName) | ||
}).open(); | ||
if (choice) { | ||
this._busy++; |
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 code is duplicated just below. You could easily clean this up writing
if (this.verified || this.confirmUnverifiedInstallation(this.displayName)) {
...
}
or by extracting the installation into a doInstall
method.
version: allVersions.version | ||
})); | ||
searchResult.add(id); | ||
const res = await client.query({ extensionId: id, extensionVersion: allVersions.version, includeAllVersions: true }); |
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 opening the issue, it would be by far the best solution to just do this in the original search.
I've done some more testing, and the added queries for the "verified" status make searching for something like "github" got from < two seconds to > 9 seconds. IMO, this is not what we want. |
I would propose we disucss at the theia-dev meeting on tuesday. @vladarama can you make it? Having the verified items pop in asynchronously would be cool too. But I would rather involve the author. |
Doing async queries would work for me: since they are i/o-bound, the local performance hit would be negligible. We'd have to introduce a third value for "verified" ( |
Thanks for the feedback Thomas, |
Hey @tsmaeder , |
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.
Much better, I still have a couple of questions/suggestions.
@tsmaeder Could you have a final look and merge if OK, please? |
@vladarama I'll get to this one next week. |
@vladarama thanks for this PR, it works very nicely now. Sorry it took so long, but Dec./Jan. was very busy for me. Before I merge this, could you please rebase the PR on master in order to make sure the license check is O.K.? |
This commit handles the contribution of the 'verified' field when searching for extensions in the vsx-registry. It implements a checkmark near the publisher name when the extension's publisher is verified. Signed-off-by: Vlad Arama <[email protected]>
This commit adds a toggle button near the search bar in the vsx registry which allows for the filtering of extensions. When toggled, only verified extensions will be shown during the search which allows the user to quickly filter out extensions that might be deemed unsafe. Signed-off-by: Vlad Arama <[email protected]>
This commit does the following: - Adds `extensions.onlyShowVerifiedExtensions` preference to control default behavior of the vsx-registry. - Changes the `verified` icon to `codicon-verified-filled` and registers a blue color to match vscode styling. - Adds a `ConfirmDialog` when trying to install an unverified extension Signed-off-by: Vlad Arama <[email protected]>
This commit fixes the following issues: - Stops preventing the installation of verified extensions. - Refresh search results when the preference is changed from the preference editor.
This commit removes duplicated code by adding a `doInstall` protected method. Signed-off-by: Vlad Arama <[email protected]>
This commit allows the fetching of the verified status to be performed asynchronously during the search to avoid causing a performance hit. The verified (or unverified) icons will pop in asynchronously when the fetching is done. The following has been added: - `verified-filled` icon for verified extensions - `verified` icon for unverified extensions (unfilled verified) - `question` icon for extensions with an unknown verification status (appears before the fetching is completed) A small cleanup has also been performed with the creation of the following functions: - `fetchVerifiedStatus` - `updateExtensions` Signed-off-by: Vlad Arama <[email protected]>
This commit addresses review feedback and cleansup the code in the following way: - rename `updateExtensions` to `addExtensions` which is more appropriate - remove unnecessary `async` - fire parallel requests instead of serializing them Signed-off-by: Vlad Arama <[email protected]>
6d1f700
to
68c39a0
Compare
@tsmaeder no worries, |
What it does
This PR implements various features allowing for the filtering of verified vsx-extensions. This helps the user distinguish verified extensions from non-verified ones which might pose a security risk. The changes include:
extensions.onlyShowVerifiedExtensions
preference to control the default behavior of the vsx-registry.ConfirmDialog
to warn the user when he is trying to install an unverified extension.Demo:
How to test
Only Show Verified Extension
button near the search-bar -> Only verified extensions should be shown.extensions.onlyShowVerifiedExtensions
preference should now be set toTrue
.Only Show Verified Extension
button -> All extensions should be shown -> The preference should be set toFalse
or removed.Review checklist
Reminder for reviewers