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

DISK: Add check for elevated privileges when DevDrive flag is set to true #292

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bbonaby
Copy link
Contributor

@bbonaby bbonaby commented Sep 16, 2024

Pull Request (PR) description

Currently DSC resources can be ran outside of the LCM. For these cases users will need to run their configuration files with Administrator privileges. When users run their Dev Drive specific configurations without running as administrator, the configuration fails due to one of the PowerShell cmdlets failing due to not being ran elevated. This then returns an error to the user that is true for what we were attempting to do but doesn't tell the user that anything failed due to the user not running the configuration as admin.

To fix this I have added a check for elevation in the Test-TargetResource and the Set-TargetResource methods when the $DevDrive parameter is true. I have also updated the tests for this as well.

I also updated the Disk dsc's readme file with to tell users that running as admin is required.

This Pull Request (PR) fixes the following issues

Task list

  • Added an entry to the change log under the Unreleased section of the
    file CHANGELOG.md. Entry should say what was changed and how that
    affects users (if applicable), and reference the issue being resolved
    (if applicable).
  • Resource documentation added/updated in README.md.
  • Resource parameter descriptions added/updated in README.md, schema.mof
    and comment-based help.
  • Comment-based help added/updated.
  • Localization strings added/updated in all localization files as appropriate.
  • Examples appropriately added/updated.
  • Unit tests added/updated. See DSC Community Testing Guidelines.
  • Integration tests added/updated (where possible). See DSC Community Testing Guidelines.
  • New/changed code adheres to DSC Community Style Guidelines.

This change is Reviewable

Copy link

codecov bot commented Sep 16, 2024

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 94%. Comparing base (215b284) to head (7a2c64e).

Files with missing lines Patch % Lines
...e/Modules/StorageDsc.Common/StorageDsc.Common.psm1 0% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@         Coverage Diff         @@
##           main   #292   +/-   ##
===================================
- Coverage    95%    94%   -1%     
===================================
  Files         9      9           
  Lines      1236   1239    +3     
===================================
+ Hits       1176   1177    +1     
- Misses       60     62    +2     
Files with missing lines Coverage Δ
source/DSCResources/DSC_Disk/DSC_Disk.psm1 98% <100%> (+<1%) ⬆️
...urces/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1 100% <100%> (ø)
...e/Modules/StorageDsc.Common/StorageDsc.Common.psm1 89% <0%> (-3%) ⬇️

Copy link

@dan-hughes dan-hughes left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 8 files at r1, all commit messages.
Reviewable status: 3 of 8 files reviewed, 6 unresolved discussions (waiting on @bbonaby)


CHANGELOG.md line 10 at r1 (raw file):

## [6.0.1] - 2024-06-11

### Changed

Also include the change to VirtualHardDisk


source/DSCResources/DSC_Disk/README.md line 39 at r1 (raw file):

This is a low-level concept that most users will never need to interact with but
for further reading, see the documentation [here](https://learn.microsoft.com/windows/dev-drive/#how-do-i-configure-additional-filters-on-dev-drive)
for further reading. In order to create a Dev Drive your configuration must run with

One of the for further reading's can be removed

Code quote:

for further reading.

source/DSCResources/DSC_Disk/en-US/DSC_Disk.strings.psd1 line 55 at r1 (raw file):

    TheVolumeIsNotConfiguredAsADevDriveVolume = The volume with path '{0}' and Drive letter '{1}' is not configured as a Dev Drive volume.
    TheVolumeIsCurrentlyConfiguredAsADevDriveVolume = The volume with path '{0}' and Drive letter '{1}' is currently configured as a Dev Drive volume.
    DevDriveAdminError = Creating a Dev Drive volume requires running local Administrator permissions. Please ensure this resource is being applied with an account with local Administrator permissions.

Can you add an id to the new entry Localization (dsccommunity.org).


source/Modules/StorageDsc.Common/StorageDsc.Common.psm1 line 638 at r1 (raw file):

        A user friendly error message specific to the caller.
#>
function Assert-ElevatedUserWithCustomErrorMessage

I wonder if it's worth extending Assert-ElevatedUser to accept a custom error message. @johlju thoughts?


source/Modules/StorageDsc.Common/StorageDsc.Common.psm1 line 656 at r1 (raw file):

    {
        # Use a user friendly error message specific to the caller
        throw $CustomErrorMessage

Need to not use throw and use ThrowTerminatingError e.g. in Assert-ElevatedUser.

That will depend on the overall strategy for this function though.


tests/Unit/DSC_Disk.Tests.ps1 line 713 at r1 (raw file):

        }

        function Assert-ElevatedUserWithCustomErrorMessageWithCustomErrorMessage

Adding a stub function here should not be required as it lives in this module.

@dan-hughes
Copy link

@bbonaby,

Assert-ElevatedUser in DscResource.Common now has an ErrorMessage parameter which removes the need for the Assert-ElevatedUserWithCustomErrorMessageWithCustomErrorMessage function added in this PR.

Are you able to re-download the dependencies locally as this is included in a new release and use this instead of using the try/catch here. It will remove the majority of the comments for the PR, improve test coverage and the overall size of the PR.

Ping me when you've completed and I'll review and get this merged.

@bbonaby
Copy link
Contributor Author

bbonaby commented Oct 23, 2024

Hey @dan-hughes sorry, I've been busy with work but will get to this, this weekend. Thanks!

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.

Dev Drive DSC error message when not run as admin
2 participants