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

Add a policy for profile sources #18009

Merged
merged 5 commits into from
Oct 15, 2024
Merged

Add a policy for profile sources #18009

merged 5 commits into from
Oct 15, 2024

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Oct 8, 2024

This adds a basic policy check for DisabledProfileSources, so that
organizations can easily disable certain profiles like the Azure one.

Closes #17964

Validation Steps Performed

  • Add a policy to disable Azure under HKCU. Disabled ✅
  • Add a policy to disable nothing under HKLM. Enabled ✅
    (...because it overrides the HKCU setting.)

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Area-Settings Issues related to settings and customizability, for console or terminal Product-Terminal The new Windows Terminal. labels Oct 8, 2024
@htcfreek
Copy link

htcfreek commented Oct 8, 2024

Love this. ♥️♥️♥️

Will you provide special admx templates or will they be part of the os built-in templates?

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

simple and glorious

@lhecker
Copy link
Member Author

lhecker commented Oct 8, 2024

Will you provide special admx templates or will they be part of the os built-in templates?

So far, I wasn't planning on doing that, but if it helps you a lot, I could look into it. I've never written one, so I'd have to figure that out first.

@htcfreek
Copy link

htcfreek commented Oct 8, 2024

Will you provide special admx templates or will they be part of the os built-in templates?

So far, I wasn't planning on doing that, but if it helps you a lot, I could look into it. I've never written one, so I'd have to figure that out first.

@lhecker
Otherwise it makes no sense to add it as policy in the registry.

You can look at the files in the PowerToys project. And if you have questions you can ask me for help.

https://github.com/microsoft/PowerToys/tree/main/src%2Fgpo%2Fassets

https://github.com/microsoft/PowerToys/blob/main/src%2Fgpo%2Fassets%2FPowerToys.admx#L609 => Multiline Textbox sample: MwbPolicyDefinedIpMappingRules policy.

@DHowett
Copy link
Member

DHowett commented Oct 8, 2024

it makes no sense to add it as policy in the registry.

The Policy subtree has different permissions, even under HKCU, than the rest of the registry. There is absolutely value in putting it there, especially if we continue to add enterprise manageability features.

@htcfreek
Copy link

htcfreek commented Oct 8, 2024

it makes no sense to add it as policy in the registry.

The Policy subtree has different permissions, even under HKCU, than the rest of the registry. There is absolutely value in putting it there, especially if we continue to add enterprise manageability features.

Sure. But for managing and using them you either need ADMX templates or you have to create registry group policy preferences. So creating ADMX templates is a must have and makes it much easier for admins.

Imagin you like to set the policy for some users (HKCU) in an enterprise environment where the users have no administrative permissions. This doesn't work without ADMX templates/Group Policy Preferences.

@@ -0,0 +1,23 @@
<?xml version="1.0" encoding="utf-8"?>

Check failure

Code scanning / check-spelling

Check File Path Error

admx is not a recognized word. (check-file-path)

Choose a reason for hiding this comment

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

Check File Path

admx is not a recognized word. (check-file-path)

Show more details

c82ab28

policies/en-US/WindowsTerminal.adml Fixed Show fixed Hide fixed

This comment has been minimized.

@htcfreek
Copy link

htcfreek commented Oct 9, 2024

@lhecker
Small tip for you: You can test the templates by compy-paste to %windir%\policydefinitions and then opening gpedit.msc.

@DHowett
Copy link
Member

DHowett commented Oct 11, 2024

@htcfreek if you're comfortable signing off or rejecting based on the ADMX changes, I would appreciate it! You know more about this than I do, that's for sure. 🙂

Copy link

@htcfreek htcfreek left a comment

Choose a reason for hiding this comment

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

A small improvement suggestion.

Will do a technical test of the ADMX template tomorrow. After that I will approve the PR.

<policies>
<policy name="DisabledProfileSources" class="Both" displayName="$(string.DisabledProfileSources)" explainText="$(string.DisabledProfileSourcesText)" presentation="$(presentation.DisabledProfileSources)" key="Software\Policies\Microsoft\Windows\Terminal">
<parentCategory ref="WindowsTerminal" />
<supportedOn ref="terminal:SUPPORTED_WindowsTerminal_1_21" />

Choose a reason for hiding this comment

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

Suggested change
<supportedOn ref="terminal:SUPPORTED_WindowsTerminal_1_21" />
<supportedOn ref="SUPPORTED_WindowsTerminal_1_21" />

Is unique enough.

Choose a reason for hiding this comment

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

1.21 is the current stable and I am sure it does not support the policy. Shouldn't it be 1.22 or 1.23?

Copy link
Member Author

Choose a reason for hiding this comment

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

We intend to backport this change to 1.21 along with other bug fixes to help a customer. 🙂

Choose a reason for hiding this comment

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

But then please provide an exact version to avoid confusion: 1.21.

Copy link
Member Author

Choose a reason for hiding this comment

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

The .adml contains:

<string id="SUPPORTED_WindowsTerminal_1_21">At least Windows Terminal 1.21</string>

Is that not enough? I know that the supportedOn element can contain a version, but I only saw it being used on the OS version, not on package versions...

Copy link

@htcfreek htcfreek left a comment

Choose a reason for hiding this comment

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

Small text improvements

policies/en-US/WindowsTerminal.adml Outdated Show resolved Hide resolved
- Windows.Terminal.PowershellCore
- Windows.Terminal.Wsl

For instance, setting this policy to Windows.Terminal.Wsl will disable the builtin WSL integration of Windows Terminal.</string>

Choose a reason for hiding this comment

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

Suggested change
For instance, setting this policy to Windows.Terminal.Wsl will disable the builtin WSL integration of Windows Terminal.</string>
For instance, setting this policy to Windows.Terminal.Wsl will disable the builtin WSL integration of Windows Terminal.
Note: Allready existing profiles will disappear from Windows Terminal after adding their profile source to the policy.</string>

Copy link
Member Author

Choose a reason for hiding this comment

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

I've adopted both of your suggestions with minor changes to them (on VS of, etc.). Thanks!

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 13, 2024
policies/WindowsTerminal.admx Outdated Show resolved Hide resolved
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 14, 2024
Copy link

@htcfreek htcfreek left a comment

Choose a reason for hiding this comment

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

ADMX/ADML and registry path lgtm.

Regarding the "Supported on" value of the ADMX: This should be as exact as possible. It would be a worst case for an admin reading 1.21, using 1.21.1 and the policy works not before 1.21.2 release.

Additional I suggest to add a page or something else to docs.msft.com describing how to install the admx files and what policies exist. As example you can look at the PowerToys documentation.

@lhecker
Copy link
Member Author

lhecker commented Oct 14, 2024

Regarding the "Supported on" value of the ADMX: This should be as exact as possible. It would be a worst case for an admin reading 1.21, using 1.21.1 and the policy works not before 1.21.2 release.

In that case, we'll just have to update the file after the update has shipped so that we know the correct number. Since the admx/adml files won't be part of the .msix package, we should have enough time to update it.

Additional I suggest to add a page or something else to docs.msft.com describing how to install the admx files and what policies exist. As example you can look at the PowerToys documentation.

Thanks! I'll keep it in mind and write a page soon. 🙂

@DHowett
Copy link
Member

DHowett commented Oct 15, 2024

@htcfreek thank you for the review, and all the help!

@DHowett DHowett merged commit 3a06826 into main Oct 15, 2024
19 of 21 checks passed
@DHowett DHowett deleted the dev/lhecker/17964-azure-gpo branch October 15, 2024 21:48
Copy link

@zadjii zadjii left a comment

Choose a reason for hiding this comment

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

I'm fine with this. IDK how we're shipping the translated .adml's, but looks fine to me.

@htcfreek
Copy link

I'm fine with this. IDK how we're shipping the translated .adml's, but looks fine to me.

That not a must have. But i imagine somehow that Visual Studio or VS Code has translated adml and an automated process.

DHowett pushed a commit that referenced this pull request Oct 17, 2024
This adds a basic policy check for DisabledProfileSources, so that
organizations can easily disable certain profiles like the Azure one.

Closes #17964

* Add a policy to disable Azure under HKCU. Disabled ✅
* Add a policy to disable nothing under HKLM. Enabled ✅
  (...because it overrides the HKCU setting.)

(cherry picked from commit 3a06826)
Service-Card-Id: PVTI_lADOAF3p4s4AmhmszgT6i0Y
Service-Version: 1.21
DHowett pushed a commit that referenced this pull request Oct 17, 2024
This adds a basic policy check for DisabledProfileSources, so that
organizations can easily disable certain profiles like the Azure one.

Closes #17964

## Validation Steps Performed
* Add a policy to disable Azure under HKCU. Disabled ✅
* Add a policy to disable nothing under HKLM. Enabled ✅
  (...because it overrides the HKCU setting.)

(cherry picked from commit 3a06826)
Service-Card-Id: PVTI_lADOAF3p4s4AmhmQzgT6i0c
Service-Version: 1.22
nguyen-dows added a commit to MicrosoftDocs/terminal that referenced this pull request Oct 28, 2024
Addresses microsoft/terminal#18095 and
microsoft/terminal#18009

---------

Co-authored-by: Heiko <[email protected]>
Co-authored-by: Matt Wojciakowski <[email protected]>
Copy link

@Jihad85-D Jihad85-D left a comment

Choose a reason for hiding this comment

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

Will upgrade soon

@microsoft microsoft deleted a comment from Dan860000 Nov 5, 2024
Copy link

@Jihad85-D Jihad85-D left a comment

Choose a reason for hiding this comment

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

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Settings Issues related to settings and customizability, for console or terminal Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal.
Projects
Development

Successfully merging this pull request may close these issues.

Provide an easy way to disable / remove Azure Cloud Shell from Terminal
8 participants