Skip to content

Conversation

kvprasoon
Copy link
Contributor

@kvprasoon kvprasoon commented Sep 4, 2019

PR Summary

This PR is to add -SecurityDescriptorSDDL parameter to New-Service cmdlet so that its in par with Set-Service in setting sddl for a service.

PR Checklist

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Sep 5, 2019
@iSazonov iSazonov added this to the 7.0.0-preview.4 milestone Sep 5, 2019
{
var rawSecurityDescriptor = new RawSecurityDescriptor(securityDescriptorSddl);
RawAcl rawDiscretionaryAcl = rawSecurityDescriptor.DiscretionaryAcl ;
var discretionaryAcl = new DiscretionaryAcl (false, false, rawDiscretionaryAcl);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Extra spaces in two lines above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, why didn't CodeFactor catch this ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sometimes it is not predictable :-)

bool status = NativeMethods.SetServiceObjectSecurity(
hService,
SecurityInfos.DiscretionaryAcl,
securityDescriptorByte);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indentation must be 4 spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, why didn't CodeFactor catch this ?

@adityapatwardhan
Copy link
Member

@PoshChan please retry static

@@ -2273,8 +2294,14 @@ protected override void BeginProcessing()
}
}

if (!string.IsNullOrEmpty(SecurityDescriptorSddl))
Copy link
Member

Choose a reason for hiding this comment

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

would this work?

service = new ServiceController(Name);

if (!string.IsNullOrEmpty(SecurityDescriptorSddl))
{
    SetServiceSecurityDescriptor(service, SecurityDescriptorSddl, hService);
}

Copy link
Collaborator

@vexx32 vexx32 Sep 5, 2019

Choose a reason for hiding this comment

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

That's a parameter, right? Shouldn't we be using if (MyInvocation.BoundParameters.ContainsKey(nameof(SecurityDescriptorSddl))) { ... } for parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adityapatwardhan @vexx32 yes, I've used the same way how its implemented for Set-Service. I referred

L1883 if (!string.IsNullOrEmpty(Status)) which is used for -Status parameter in Set-Service.

@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Sep 5, 2019
@adityapatwardhan
Copy link
Member

@kvprasoon Please follow the template and file a doc issue for the new parameter. If it is filed, please paste the link here.

@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Sep 6, 2019
@kvprasoon
Copy link
Contributor Author

doc changes MicrosoftDocs/PowerShell-Docs#4735

@adityapatwardhan adityapatwardhan merged commit 768e79a into PowerShell:master Oct 4, 2019
@iSazonov
Copy link
Collaborator

iSazonov commented Oct 4, 2019

@kvprasoon Thanks for your contribution! Sorry for delay.

@kvprasoon kvprasoon deleted the new-srv-sdset branch October 4, 2019 04:13
@ghost
Copy link

ghost commented Oct 23, 2019

🎉v7.0.0-preview.5 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants