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

Customization: Add general system and virtualization feature settings #3268

Merged
merged 3 commits into from
Jun 25, 2024

Conversation

nieubank
Copy link
Contributor

@nieubank nieubank commented Jun 20, 2024

Summary of the pull request

Adds virtualization feature management and general system settings initial implementation.

NOTE: Content is still in review, so string changes and some design elements may be needed in a subsequent craftsmanship pass.
 

References and relevant issues

#2920 #3249 #2871

Detailed description of the pull request / Additional comments

  • Updates ManagementInfrastructureHelper.cs for enhanced Windows feature management, including a refactored method for checking feature availability with modifications to return more detailed feature information.
  • Introduced a RestartHelper.cs class with a method to restart the computer immediately, to avoid disruption I did not point existing callers from Environments to this method since they have additional telemetry for this space. I can file an issue to track consolidation here.
  • Updated WindowsIdentityHelper.cs with a new method to check for whether a user has the capability to acquire administrative privileges, not just whether the current process is running elevated.
  • Added WindowsOptionalFeatures.cs and WindowsOptionalFeatureState.cs to represent Windows optional features, including a new static class for feature names and descriptions and a class to represent the state of an optional feature, specifically for UI.
  • Introduced ModifyLongPathsSetting.cs and ModifyWindowsOptionalFeatures.cs for enabling/disabling Windows settings and optional features through PowerShell scripts which require elevation. This allows the full script content to be visible to the user when accepting or rejecting the UAC prompt providing extra transparency, this is done in the same way as environments feature management. There are limitations here in that if scripts are blocked on the machine we're unable to make this changes, but until we finish security review on the elevated server, this is the best we've got.
  • Expanded Windows Customization with new views and dialogs for managing general system settings and virtualization features, including GeneralSystemView, ModifyFeaturesDialog, and VirtualizationFeatureManagementPage, alongside updates to existing views to accommodate these new features.

Main page navigation entries

image

Virtualization feature management page

Here the apply button is disabled until a change is made
image

Applying shows a modal dialog with an option to cancel and a horizontal progress bar
image

Successfully applying the changes shows a modal restart prompt that can be ignored, an Info Bar is updated with notice to restart as well, persists across navigation
image

Failure to apply will prompt an error on the Info Bar
image

When the user is not an admin, options are disabled with an info bar warning
image

General system

image

Changes to max path require a restart for existing/long-running processes to change, but any new processes don't so a modal dialog was not chosen for this path, just the restart Info Bar warning.
image

Validation steps performed

  • Test toggling a single and multiple features and confirm that the correct action is taken.
  • Revert changes and confirm that the InfoBar warning instructing to restart is cleared
  • Navigate away and come back to see that the InfoBar warning is still present.
  • Confirm that restart buttons correctly restart the machine.
  • Test cancelation with multiple features and ensure that the UI accurately reflects the state of the machine.
  • Confirm that registry values are set appropriately for long path settings.
  • Tested on Home SKU and non-admin user

PR checklist

@nieubank nieubank marked this pull request as ready for review June 20, 2024 18:34
Copy link
Contributor

@bbonaby bbonaby left a comment

Choose a reason for hiding this comment

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

Just need to finish looking at the viewmodel and view changes then I'll be done reviewing, Will continue later today. Looking good so far

common/Helpers/WindowsIdentityHelper.cs Outdated Show resolved Hide resolved
common/Models/WindowsOptionalFeatureState.cs Outdated Show resolved Hide resolved
/// This list should be kept consistent with the list of features in the WindowsOptionalFeatureNames class.
///
/// </summary>
private const string Script =
Copy link
Contributor

Choose a reason for hiding this comment

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

This is separate from the code review:

But I didn't think about this before but in the future I'm thinking, maybe we should use the built in WindowsOptionalFeature DSC resource in the PSDscResources module for enabling the features in the future so Windows customization, can enable any feature. @AmelBawa-msft is actually moving out the DSC execution code from the setup flow and into a general service that anyone in Dev Home can use in this PR: #3134

I'll need to see if there is a DSC for adding users to a group as well like for the hyper-v admin group stuff or create one, but that would be something else we'd want to add to Windows customization. Thinking even further it'd be great if there was a Generate DSC file button for Windows Customization like there is for the setup flow. CC: @joadoumie / @shakersMSFT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, there are a bunch of valid long term options here. Another is just calling the various native APIs directly in an elevated zone, or other mechanism that Jeff is looking into. I did look into DSC execution here briefly and it was quite a bit slower than just calling into powershell, so something to look at in the future.

common/Scripts/ModifyWindowsOptionalFeatures.cs Outdated Show resolved Hide resolved
common/Strings/en-us/Resources.resw Show resolved Hide resolved
common/Strings/en-us/Resources.resw Show resolved Hide resolved
common/Strings/en-us/Resources.resw Show resolved Hide resolved
common/Strings/en-us/Resources.resw Outdated Show resolved Hide resolved
common/Strings/en-us/Resources.resw Outdated Show resolved Hide resolved
…settings

    - Updates `ManagementInfrastructureHelper.cs` for enhanced Windows feature management, including a refactored method for checking feature availability with modifications to return more detailed feature information.
    - Introduced a `RestartHelper.cs` class with a method to restart the computer immediately, to avoid disruption I did not point existing callers from Environments to this method since they have additional telemetry for this space. I can file an issue to track consolidation here.
    - Updated `WindowsIdentityHelper.cs` with a new method to check for whether a user has the capability to acquire administrative privileges, not just whether the current process is running elevated.
    - Added `WindowsOptionalFeatures.cs` and `WindowsOptionalFeatureState.cs` to represent Windows optional features, including a new static class for feature names and descriptions and a class to represent the state of an optional feature, specifically for UI.
    - Introduced `ModifyLongPathsSetting.cs` and `ModifyWindowsOptionalFeatures.cs` for enabling/disabling Windows settings and optional features through PowerShell scripts which require elevation. This allows the full script content to be visible to the user when accepting or rejecting the UAC prompt providing extra transparency, this is done in the same way as environments feature management. There are limitations here in that if scripts are blocked on the machine we're unable to make this changes, but until we finish security review on the elevated server, this is the best we've got.
    - Expanded Windows Customization with new views and dialogs for managing general system settings and virtualization features, including `GeneralSystemView`, `ModifyFeaturesDialog`, and `VirtualizationFeatureManagementPage`, alongside updates to existing views to accommodate these new features.
@nieubank nieubank force-pushed the user/nieubank/custom_genvirt branch from 0ae7f25 to daf0515 Compare June 21, 2024 18:58
nieubank added 2 commits June 24, 2024 14:44
- Updated ManagementInfrastructureHelper.cs to default `description` to an empty string if null.
- Changed `IsUserAdministrator` in WindowsIdentityHelper.cs to virtual for better extensibility.
- Enhanced WindowsOptionalFeatureState.cs with `[ObservableProperty]` for `_isEnabled` and added change notification.
- Modified PowerShell scripts in ModifyLongPathsSetting.cs and ModifyWindowsOptionalFeatures.cs for robust error handling with `$ErrorActionPreference='stop'`.
- Adjusted return value and simplified `ExitCode` in ModifyWindowsOptionalFeatures.cs for clarity and maintainability.
- Updated resource comments in Resources.resw for better clarity and localization support.
- Refactored exit code handling in ModifyWindowsOptionalFeaturesEvent.cs for consistency and removed no change for simplicity
- Adapted GeneralSystemViewModel.cs & VirtualizationFeatureManagementViewModel.cs to the non-static `IsUserAdministrator`.
- Improved XAML file readability and structure in various UI components.
Copy link
Contributor

@joadoumie joadoumie left a comment

Choose a reason for hiding this comment

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

Tested out the UX, seems to be functioning well! I have found one issue that is broader than just Windows Cust regarding restart logic in Dev Home when using a Dev Box.

I'm going to file an issue separately as this is not feature specific.

@nieubank nieubank merged commit 7f01cdc into main Jun 25, 2024
4 checks passed
@nieubank nieubank deleted the user/nieubank/custom_genvirt branch June 25, 2024 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants