Added functionality to set credientials on Set-Service command #4844

joandrsn · 2017-09-15T07:33:07Z

Added functionality to set credientials on Set-Service command.
Using this you can now specifiy the -Credential parameter which makes the service you have specfied change the log-on account.

Issue #4843

msftclas · 2017-09-15T07:33:12Z

@joandrsn,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla.microsoft.com.

It will cover your contributions to all Microsoft-managed open source projects.
Thanks,
Microsoft Pull Request Bot

msftclas · 2017-09-15T07:36:58Z

@joandrsn, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, Microsoft Pull Request Bot

iSazonov

@joandrsn Many thanks for your contribution!

Please add a description in the PR.
Also please add tests (see Set-Service.Tests.ps1).

iSazonov · 2017-09-15T08:37:05Z

src/Microsoft.PowerShell.Commands.Management/commands/management/Service.cs

+            get { return credential; }
+            set { credential = value; }
+        }
+        internal PSCredential credential = null;


For the new code, it is preferable to use auto property. Especially because credential is not used.

iSazonov · 2017-09-15T08:40:31Z

src/Microsoft.PowerShell.Commands.Management/commands/management/Service.cs

@@ -1679,9 +1693,9 @@ protected override void ProcessRecord()
                            continue;
                        }

-                        // modify startup type or display name
+                        // modify startup type or display name or credential


Please start comments with capitalized char. We shouldn't add bad patterns.

iSazonov · 2017-09-15T08:41:33Z

src/Microsoft.PowerShell.Commands.Management/commands/management/Service.cs

@@ -1701,6 +1715,14 @@ protected override void ProcessRecord()
                                        "bad StartupType");
                                    break;
                            }
+
+                            // set up the Credential parameter


Please remove obvious comment.

joandrsn · 2017-09-15T10:04:55Z

@iSazonov thank you for the feedback. I have updated my pull request. But I still need to create tests for the code I have written.
Unfortunatly I don't know the credentials of the computer that the tests will be running on, so I'm not sure how to write the testcase.
Do you have any documentation on what I can and can't write in the tests?

iSazonov · 2017-09-15T10:21:47Z

@joandrsn Please see tests for New-Service
I think you should (1) create new test service (2) check that you can change its properties. (3) remove the test service.

Also you should add "[Feature]" in header first position of last commit that CI start the feature tests.

joandrsn · 2017-09-15T11:58:21Z

@iSazonov I have now added the test, but when executing the test with what it is written within the test, you will receive an error The account name is invalid or does not exist, or the password is invalid for the account name specified since the users username and username2 does not exist.

I have tested with my own 2 users and it seems to work, but is this okay?

iSazonov · 2017-09-15T12:47:03Z

test/powershell/Modules/Microsoft.PowerShell.Management/Set-Service.Tests.ps1

@@ -110,6 +110,39 @@ Describe "Set/New-Service cmdlet tests" -Tags "Feature", "RequireAdminOnWindows"
        $newServiceCommand.$parameter | Should Be $value
    }

+    It "Set-Service can create a new service called '<name>' and change the credentials" -TestCases @(


"Set-Service can create a new service" - it is incorrect.

iSazonov · 2017-09-15T12:51:40Z

test/powershell/Modules/Microsoft.PowerShell.Management/Set-Service.Tests.ps1

+        finally {
+            $service = Get-CimInstance Win32_Service -Filter "name='$name'"
+            if ($service -ne $null) {
+                $service | Remove-CimInstance


Please add "-Force -ErrorAction SilentlyContinue"

Neither Get-CimInstance nor Remote-CimInstance has a force parameter. But I can add -ErrorAction SilentlyContinue

iSazonov · 2017-09-15T12:56:51Z

@joandrsn I think we should try to create a real user account for the test with "Log on as a service" privilege.

joandrsn · 2017-09-15T13:01:18Z

@iSazonov
I figured out that I could use some built in users from Windows.
Currently I'm using .\Guest when creating the service and .\Administrator when changing the credentials. Would you rather have it create a user and delete the user again?

iSazonov · 2017-09-15T13:46:38Z

Guest and Administrator accounts haven't "Log on as a service" privilege and we don't know password. So we should create test users.

joandrsn · 2017-09-15T14:22:05Z

They don't have the access, but the test passes. I guess that might not be a good way to test (using invalid credentials to validate a test).

Do you have suggestion on how you would add the user? Add-LocalUser has been disabled since #4272. I can't find any other test modules in the repository which creates and deletes a user.

SteveL-MSFT · 2017-09-15T14:25:23Z

@joandrsn thanks for the contribution! Look at what I've done here to create my own user and removing at end of test

iSazonov · 2017-09-15T15:19:55Z

For PowerShell team: We haven't tests to really create new service and change its properties. So we should refactor all tests after the PR will be merged. Maybe create tracking Issue?

SteveL-MSFT · 2017-09-15T16:55:37Z

@iSazonov it seems the test case added by @joandrsn does a new and then set, were you thinking of something different?

SteveL-MSFT · 2017-09-15T16:59:44Z

test/powershell/Modules/Microsoft.PowerShell.Management/Set-Service.Tests.ps1

+            $service = New-Service @parameters
+            $service | Should Not BeNullOrEmpty
+            $service = Get-CimInstance Win32_Service -Filter "name='$name'"
+            $service | Should Not BeNullOrEmpty


this check isn't necessary as the next line will fail

SteveL-MSFT · 2017-09-15T16:59:59Z

test/powershell/Modules/Microsoft.PowerShell.Management/Set-Service.Tests.ps1

+            $service | Should Not BeNullOrEmpty
+            $service = Get-CimInstance Win32_Service -Filter "name='$name'"
+            $service | Should Not BeNullOrEmpty
+            $service.StartName | Should Be $creds.UserName


SteveL-MSFT · 2017-09-15T17:00:27Z

test/powershell/Modules/Microsoft.PowerShell.Management/Set-Service.Tests.ps1

+            $creds = [pscredential]::new(".\$endUsername", $password)
+            Set-Service -Name $name -Credential $creds
+            $service = Get-CimInstance Win32_Service -Filter "name='$name'"
+            $service | Should Not BeNullOrEmpty


This check isn't necessary

SteveL-MSFT · 2017-09-15T17:02:41Z

test/powershell/Modules/Microsoft.PowerShell.Management/Set-Service.Tests.ps1

+            $service.StartName | Should Be $creds.UserName
+        }
+        finally {
+            $service = Get-CimInstance Win32_Service -Filter "name='$name'" -ErrorAction SilentlyContinue


You can simply this to just:

Get-CimInstance ... | Remove-CimInstance -ErrorAction SilentlyContinue

iSazonov · 2017-09-15T17:02:37Z

test/powershell/Modules/Microsoft.PowerShell.Management/Set-Service.Tests.ps1

        try {
+            $testPass = "Secret123!"
+            $null = net user $startUsername $testPass /add
+            $null = net user $endUsername $testPass /add


Minor comment - we usually use the pattern net user $startUsername $testPass /add > $null

iSazonov · 2017-09-15T17:03:17Z

test/powershell/Modules/Microsoft.PowerShell.Management/Set-Service.Tests.ps1

-                (ConvertTo-SecureString "PlainTextPassword" -AsPlainText -Force)); 
-            endCredential = [System.Management.Automation.PSCredential]::new(".\Administrator", 
-                (ConvertTo-SecureString "PlainTextPassword" -AsPlainText -Force))}
+        @{name = "testsetcredential"; startUsername = "user1"; endUsername = "user2"}


It is single test and we should remove TestCases.

iSazonov · 2017-09-15T17:04:45Z

test/powershell/Modules/Microsoft.PowerShell.Management/Set-Service.Tests.ps1

-            $service.StartName | Should Be $startCredential.UserName
-            Set-Service -Name $name -Credential $endCredential
+            $service.StartName | Should Be $creds.UserName
+            $creds = [pscredential]::new(".\$endUsername", $password)


Please add new line to split into logical blocks.

iSazonov · 2017-09-15T17:13:38Z

test/powershell/Modules/Microsoft.PowerShell.Management/Set-Service.Tests.ps1

        }
        finally {
            $service = Get-CimInstance Win32_Service -Filter "name='$name'" -ErrorAction SilentlyContinue
            if ($service -ne $null) {
                $service | Remove-CimInstance -ErrorAction SilentlyContinue
            }
+            $null = net user $startUsername /delete
+            $null = net user $endUsername /delete


The same about $null

iSazonov · 2017-09-15T17:27:12Z

test/powershell/Modules/Microsoft.PowerShell.Management/Set-Service.Tests.ps1

@@ -110,6 +110,40 @@ Describe "Set/New-Service cmdlet tests" -Tags "Feature", "RequireAdminOnWindows"
        $newServiceCommand.$parameter | Should Be $value
    }

+    It "Set-Service can change credentials of a service" {
+        param()


Please remove.

iSazonov

@joandrsn Many thanks!

I'll merge on next week.

iSazonov · 2017-09-16T19:20:34Z

it seems the test case added by @joandrsn does a new and then set , were you thinking of something different?
@SteveL-MSFT I've analyzed these tests more closely.
I don't understand why we need test "SetServiceCommand can be used as API for '' with ''". It seems we should do the checks on real service by invoke.
It seems we don't check that New- and Set-Service ignore System and Boot in StartupType.

SteveL-MSFT · 2017-09-16T19:41:23Z

@iSazonov The "use as API" tests are specifically for covering the getter/setters for the parameters which is only used when called as an API and not when used as cmdlet parameter. If we're missing some variations, we should add those, but perhaps as a different PR.

iSazonov · 2017-09-18T10:23:54Z

@joandrsn Many thanks for your contribution!

#4843 Credentials on Set-Service

b686ec4

joandrsn requested review from adityapatwardhan and daxian-dbw as code owners September 15, 2017 07:33

msftclas added the cla-required label Sep 15, 2017

joandrsn changed the title ~~PowerShell/PowerShell#4843 Credentials on Set-Service~~ Credentials on Set-Service Sep 15, 2017

msftclas added cla-signed and removed cla-required labels Sep 15, 2017

iSazonov reviewed Sep 15, 2017

View reviewed changes

Clean up

58383f4

[Feature]Test added for Set-Service with credentials

6fe54d4

iSazonov reviewed Sep 15, 2017

View reviewed changes

[Feature]Test with windows builtin users

f09e87d

iSazonov reviewed Sep 15, 2017

View reviewed changes

[Feature]Descriptive test in Set-Service with credentials

d1a02ce

daxian-dbw requested a review from SteveL-MSFT September 15, 2017 15:44

daxian-dbw assigned iSazonov Sep 15, 2017

[Feature]Set-Service with credentials creates users for the test

3bd3376

SteveL-MSFT requested changes Sep 15, 2017

View reviewed changes

[Feature]Clean Set-Service credential test

c08466f

iSazonov reviewed Sep 15, 2017

View reviewed changes

[Feature]Remove TestCases from Set-Service with credential test

e0f2cd8

SteveL-MSFT approved these changes Sep 15, 2017

View reviewed changes

iSazonov reviewed Sep 15, 2017

View reviewed changes

[Feature]Remove unnecessary param in Set-Service with credentials test

ad1b5dd

iSazonov approved these changes Sep 15, 2017

View reviewed changes

iSazonov changed the title ~~Credentials on Set-Service~~ Added functionality to set credientials on Set-Service command Sep 18, 2017

iSazonov merged commit b7ec5da into PowerShell:master Sep 18, 2017

joandrsn mentioned this pull request Sep 18, 2017

Credentials for Set-Service command #4843

Closed

joandrsn mentioned this pull request Oct 25, 2017

Removed ComputerName and added Credential from the Set-Service docume… MicrosoftDocs/PowerShell-Docs#1805

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added functionality to set credientials on Set-Service command #4844

Added functionality to set credientials on Set-Service command #4844

Added functionality to set credientials on Set-Service command #4844

Added functionality to set credientials on Set-Service command #4844

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment