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

Anyway to not overwrite the current values? #28

Closed
brwilkinson opened this issue Mar 10, 2017 · 14 comments
Closed

Anyway to not overwrite the current values? #28

brwilkinson opened this issue Mar 10, 2017 · 14 comments
Labels
enhancement The issue is an enhancement request.

Comments

@brwilkinson
Copy link

Can we add the option to not remove the current settings when adding new identities for UserRightsAssignment

@jcwalker
Copy link
Contributor

The resource was first written to behave in that way but I got feedback asking to overwrite so the admin could be certain there were not any unwanted identities assigned to the policy.

@zjalexander
Copy link

@brwilkinson can you explain a little more how you are using or plan to use this resource? Where are any existing identities with User Rights Assignments coming from? Are they default identities on the machine or are they coming from e.g. Group Policy?

@brwilkinson
Copy link
Author

They are the default identities on the machine.

@brwilkinson
Copy link
Author

for the following account "NT SERVICE\ALL SERVICES" is the default for local right "Log_on_as_a_service"

I want to add another service account in there E.g. "Contoso\appservice".

I don't want to have to specify:
identity = "Contoso\appservice","NT SERVICE\ALL SERVICES"
I just want to specify the additional account
identity = "Contoso\appservice"

So I think there should be a attribute with a Boolean value called: OverWrite.

You specify the following if you want to blow away the original identities, otherwise the default is to add.
OverWrite = $True

@zjalexander
Copy link

got it, thanks. just needed to make sure I understood the exact situation.

@randomnote1
Copy link

I agree with @brwilkinson on this one. Maybe add an Ensure parameter that accepts Present, Absent, and Exactly. Present would ensure that the specified accounts are added to the right, Absent would ensure the specified accounts are removed from the right, and Exactly would ensure the specified accounts are the only accounts that have the right. Exactly would mimic how group policy applies.

@zjalexander
Copy link

The parameter we discussed previously was "Force". Because Ensure is a common parameter across many DSC resources, I'm not sure how to feel about adding into that enum. @kwirkykat or @mbreakey3 might have more nuanced opinions on the matter.

but, anyway, this is a good change to make

@kwirkykat
Copy link
Contributor

kwirkykat commented Mar 22, 2017

@brwilkinson @jcwalker @zjalexander @randomnote1 I definitely would not suggests adding another enum value to an Ensure property. Instead I would suggest adding an IdentitiesToInclude parameter and an IdentitiesToExclude parameter similar to how this is taken care of in the Group resource schema with Members, MembersToInclude, and MembersToExclude.

@kwirkykat kwirkykat added the enhancement The issue is an enhancement request. label Mar 22, 2017
@michaeltlombardi
Copy link
Contributor

I would argue that it is probably an anti-pattern to have a configuration item apply additively instead of exactly - it doesn't seem like a whole lot of additional work to add the explicit identities but it does help someone reading through the configuration later.

I'd rather check a config and see three identities matching those on my server than just one with possibly stale identities/settings.

@brwilkinson
Copy link
Author

brwilkinson commented Mar 22, 2017

okay I was thinking that a single Force parameter would be good enough as @zjalexander suggested.

However I actually like the way that the xGroup resource works as @kwirkykat linked to.

Either of these would work (provide the desired minimum functionality), however the second method would provide additional functionality, that I could see would be useful. i.e. to be able to explicitly remove as well as add.

So my vote would be with the 3 parameters: Members, MembersToInclude, and MembersToExclude.

@michaeltlombardi you still have the functionality to do what you want to do with this update by just using the Members parameter, so you don't lose out on anything here.

@randomnote1
Copy link

@kwirkykat, What's the reasoning behind not adding another enum value to Ensure? Just curious.

I'm on board for using the 3 parameters.

@kwirkykat
Copy link
Contributor

@randomnote1 It would be a breaking change for the resource because any scripts that currently declare Ensure as Present would go from overwriting all identities to merely adding on any missing identities.

If we were still back writing this resource from scratch, it would be more acceptable, but I would still suggest the 3 parameter model instead just for consistency with all the other DSC resources. I don't think we have any resources in the DSC Resource Kit with anything other than Present and Absent for the Ensure property. Plus I feel like adding 'Exactly' to the enum makes it vague about what 'Present' means so that enum name would probably have to change in that case too.

@jcwalker
Copy link
Contributor

jcwalker commented Jul 7, 2017

UserRightsAssignment now has a optional Force parameter. When set to true will apply only the identities in the configuration script. If Force isn't specified the identities will be appended. If nobody objects I'm going to close this issue.

@BernieWhite
Copy link
Contributor

Not sure if this is already released but should be a major version increment, breaking change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement The issue is an enhancement request.
Projects
None yet
Development

No branches or pull requests

7 participants