Skip to content

feat: package manager permissions when using API #218

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

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

karthiknadig
Copy link
Member

No description provided.

Copy link
Member

@cwebster-99 cwebster-99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure where this is changed but I also saw a new option for "Skip package installation" after setting permissions to "Ask". I think we could remove this - was there a specific reason this was added?
image

export const allow = l10n.t('Allow');
export const deny = l10n.t('Deny');
export const ask = l10n.t('Ask');
export const setPermissions = l10n.t('Set Permissions');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export const setPermissions = l10n.t('Set Permissions');
export const setPermissions = l10n.t('Update Permissions');

export namespace PermissionsCommon {
export const allow = l10n.t('Allow');
export const deny = l10n.t('Deny');
export const ask = l10n.t('Ask');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure "Ask" is easily understandable in this context. I wonder if we should do something like "Always Ask" or "Confirm each time" at the tradeoff of having a slightly longer button title

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed- I also wanted this button to be explicit it was a long-term choice

Comment on lines +169 to +202
'Set permissions for the extension {0} to install, upgrade, or uninstall packages from your Python environments',
extensionId,
),
{
modal: true,
detail: currentPermission ? l10n.t('Current permission: {0}', currentPermission) : undefined,
},
PermissionsCommon.ask,
PermissionsCommon.allow,
PermissionsCommon.deny,
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This scenario currently shows a "Cancel" option as well. Can we remove this in favor of the X on the dialog?
image

setImmediate(async () => {
const response = await showWarningMessage(
l10n.t(
'The extension `{0}` is not allowed to {1} packages into your Python environment.',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest we update language to match "permissions"

Suggested change
'The extension `{0}` is not allowed to {1} packages into your Python environment.',
'The extension `{0}` is not permitted to {1} packages into your Python environment.',

@eleanorjboyd
Copy link
Member

  • Do we think it adds something to specify to the user experience to call our the actions the extension can make instead of just a generally statement like "can make changes to packages in your environment" or something? Also should we add "downgrade" in addition to upgrade since technically that is an option an extension could take right?
Screenshot 2025-03-18 at 6 50 06 PM
  • Love this page here where it shows the current permissions!!
Screenshot 2025-03-18 at 6 52 28 PM
  • Also related to the conversation @cwebster-99 started on wording for the set permissions button text but I feel like both the setting long term permissions and in this menu the short term permissions should not have the same button since I find it a bit confusing what action Im actually taking. Since this is the prompt that shows up when someone has this pkg manager at "always ask" so that dialog should be different than the setting long term permissions.
Screenshot 2025-03-18 at 6 52 49 PM
  • when selecting "reset package permissions" I feel like a confirmation dialog would be good in case someone hits it by accident or so the user knows the weight of what they are doing. Would be nice to just have a dialog box that is like- remove all pkg management permissions set so far, this action is irreversible. We could even go a step further if we like a list all the existing pkg permissions set to show what would be deleted if we wanted.

  • Got a bit overwhelmed at this list- would be cool almost to surface the ones extensions that have values set at the top of the list in a different section, then all other extensions below but this is a stretch idea.
Screenshot 2025-03-18 at 6 48 47 PM
  • one final random item, but any reason why you can't search in the extension menu for "publisher.extension" and only "extension". Might be worthwhile allowing users to search with that whole term or maybe even be able to search by a publisher (ie search for extensions published by microsft). Just an idea, curious @cwebster-99 and your thoughts here.

@cwebster-99
Copy link
Member

Do we think it adds something to specify to the user experience to call our the actions the extension can make instead of just a generally statement like "can make changes to packages in your environment" or something?

I had the same thought initially. I think for these general views we could say "requests permissions to make changes to packages in your environment."

Since this is the prompt that shows up when someone has this pkg manager at "always ask" so that dialog should be different than the setting long term permissions.

Maybe we can say "Would you like to set permissions for {extension} to make changes to packages in your environment" and then the follow up prompts if a user selects "ask" are "the extension wants.....". So using set permissions vs the actual action that is being confirmed each time.

would be cool almost to surface the ones extensions that have values set at the top of the list in a different section, then all other extensions below but this is a stretch idea.

Ooh if would be cool if we could organize them similar to how we present environments i.e. a section for Allow, Deny, Ask and then no set permissions. A nice to have not necessarily a need to have IMO

image

Might be worthwhile allowing users to search with that whole term or maybe even be able to search by a publisher (ie search for extensions published by microsft).

I think this would be a good exploration but also a nice to have :)

@karthiknadig karthiknadig self-assigned this Mar 20, 2025
karthiknadig and others added 7 commits March 21, 2025 15:11
* ensure skip install is only shown when relevant 
* ensure delete env message is modal

fixes microsoft#220
Bumps [tar-fs](https://github.com/mafintosh/tar-fs) from 2.1.1 to 2.1.2.
<details>
<summary>Commits</summary>
<ul>
<li><a
href="https://github.com/mafintosh/tar-fs/commit/d97731b0e1b8a244ab859784b514cfcf5585ad3d"><code>d97731b</code></a>
2.1.2</li>
<li><a
href="https://github.com/mafintosh/tar-fs/commit/fd1634e869e7c5f85948e95eabdaa8451a085de5"><code>fd1634e</code></a>
symlink tweak from main</li>
<li>See full diff in <a
href="https://github.com/mafintosh/tar-fs/compare/v2.1.1...v2.1.2">compare
view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility
score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=tar-fs&package-manager=npm_and_yarn&previous-version=2.1.1&new-version=2.1.2)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after
your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating
it. You can achieve the same result by closing it manually
- `@dependabot show <dependency name> ignore conditions` will show all
of the ignore conditions of the specified dependency
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)
You can disable automated security fix PRs for this repo from the
[Security Alerts
page](https://github.com/microsoft/vscode-python-environments/network/alerts).

</details>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants