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

vsx-registry: implement verified extension filtering #12995

Merged
merged 7 commits into from
Jan 15, 2024

Conversation

vladarama
Copy link
Contributor

@vladarama vladarama commented Oct 12, 2023

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:

  • Adding a blue checkmark near the publisher name for verified extensions.
  • Implementing a extensions.onlyShowVerifiedExtensions preference to control the default behavior of the vsx-registry.
  • Creating a toggle to only show verified extensions when searching in the vsx-registry.
  • Showing a ConfirmDialog to warn the user when he is trying to install an unverified extension.

Demo:
verified extensions demo

How to test

  1. Open Theia.
  2. Visit the extension view and perform a search.
  3. Verified extensions should have a blue checkmark near the publisher name.
  4. Try installing an unverified extension -> It should open a confirmation dialog.
  5. Toggle the Only Show Verified Extension button near the search-bar -> Only verified extensions should be shown.
  6. The extensions.onlyShowVerifiedExtensions preference should now be set to True.
  7. Untoggle the Only Show Verified Extension button -> All extensions should be shown -> The preference should be set to False or removed.

Review checklist

Reminder for reviewers

@tsmaeder tsmaeder self-requested a review October 30, 2023 09:49
Copy link
Contributor

@tsmaeder tsmaeder left a 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 });
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@vladarama vladarama requested a review from tsmaeder November 1, 2023 15:13
@Mehmethanife
Copy link

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:

  • Adding a blue checkmark near the publisher name for verified extensions.
  • Implementing a extensions.onlyShowVerifiedExtensions preference to control the default behavior of the vsx-registry.
  • Creating a toggle to only show verified extensions when searching in the vsx-registry.
  • Showing a ConfirmDialog to warn the user when he is trying to install an unverified extension.

Demo:
verified extensions demo

How to test

  1. Open Theia.
  2. Visit the extension view and perform a search.
  3. Verified extensions should have a blue checkmark near the publisher name.
  4. Try installing an unverified extension -> It should open a confirmation dialog.
  5. Toggle the Only Show Verified Extension button near the search-bar -> Only verified extensions should be shown.
  6. The extensions.onlyShowVerifiedExtensions preference should now be set to True.
  7. Untoggle the Only Show Verified Extension button -> All extensions should be shown -> The preference should be set to False or removed.

Review checklist

Reminder for reviewers

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++;
Copy link
Contributor

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 });
Copy link
Contributor

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.

@tsmaeder
Copy link
Contributor

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.
My proposal would be to make sure that we only do the extra queries when we really care about them. The idea would be to remove the "verified" icon from the individual elements in the search result view and only keep the filter preference. I know this is not ideal, but until we can filter for the "verified" flag in the search (or get it back in the search result), I don't think it's worth the performance hit.
@MatthewKhouzam any thoughts on the matter?

@MatthewKhouzam
Copy link
Contributor

MatthewKhouzam commented Nov 10, 2023

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.

@tsmaeder
Copy link
Contributor

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" (unknown). Sounds like a neat exercise in managing parallel promises, as well ;-)

@vladarama
Copy link
Contributor Author

Thanks for the feedback Thomas,
Let's discuss it on tuesday, I should be able to attend the dev-meeting.

@vladarama vladarama requested a review from tsmaeder November 23, 2023 19:39
@vladarama
Copy link
Contributor Author

vladarama commented Nov 23, 2023

Hey @tsmaeder ,
The behavior should be a lot cleaner now. During a default search, the verified (or unverified) icons will only pop in when the fetching is done, which should not cause any performance issue from my testing. If the users are willing to take the small performance hit, the toggle (or preference) can be activated to only show verified extensions.

Here is a quick demo:
updated verified extensions

Copy link
Contributor

@tsmaeder tsmaeder left a 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.

packages/vsx-registry/src/browser/vsx-extensions-model.ts Outdated Show resolved Hide resolved
packages/vsx-registry/src/browser/vsx-extensions-model.ts Outdated Show resolved Hide resolved
packages/vsx-registry/src/browser/vsx-extensions-model.ts Outdated Show resolved Hide resolved
packages/vsx-registry/src/browser/vsx-extensions-model.ts Outdated Show resolved Hide resolved
@JonasHelming
Copy link
Contributor

@tsmaeder Could you have a final look and merge if OK, please?

@tsmaeder
Copy link
Contributor

tsmaeder commented Jan 5, 2024

@vladarama I'll get to this one next week.

@tsmaeder
Copy link
Contributor

@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 <vlad.arama@ericsson.com>
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 <vlad.arama@ericsson.com>
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 <vlad.arama@ericsson.com>
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 <vlad.arama@ericsson.com>
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 <vlad.arama@ericsson.com>
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 <vlad.arama@ericsson.com>
@vladarama
Copy link
Contributor Author

@tsmaeder no worries,
Thank you for taking the time to review it. I rebased the PR on master.

@tsmaeder tsmaeder merged commit abc49cd into eclipse-theia:master Jan 15, 2024
14 checks passed
@jfaltermeier jfaltermeier added this to the 1.46.0 milestone Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants