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

BREAKING CHANGE: xADUser: Suggest renaming parameter PasswordNeverResets to EnforcePassword #627

Open
johlju opened this issue Aug 25, 2020 · 9 comments
Labels
breaking change When used on an issue, the issue has been determined to be a breaking change. enhancement The issue is an enhancement request. help wanted The issue is up for grabs for anyone in the community.

Comments

@johlju
Copy link
Member

johlju commented Aug 25, 2020

Details of the scenario you tried and the problem that is occurring

Currently the parameter PasswordNeverResets is not very intuitive of what it meant to do, to enforce the password for an account or not. Suggest renaming the parameter to EnforcePassword to clearly show what it is meant to do.

Verbose logs showing the problem

n/a

Suggested solution to the issue

n/a

The DSC configuration that is used to reproduce the issue (as detailed as possible)

n/a

The operating system the target node is running

n/a

Version and build of PowerShell the target node is running

n/a

Version of the DSC module that was used

v6.1.0-preview0005

@johlju johlju added breaking change When used on an issue, the issue has been determined to be a breaking change. enhancement The issue is an enhancement request. good first issue The issue should be easier to fix and can be taken up by a beginner to learn to contribute on GitHub help wanted The issue is up for grabs for anyone in the community. and removed good first issue The issue should be easier to fix and can be taken up by a beginner to learn to contribute on GitHub labels Aug 25, 2020
@johlju
Copy link
Member Author

johlju commented Aug 25, 2020

Adding this to track this as discussed in the Slack #DSC channel.

@kungfoome
Copy link

kungfoome commented Aug 25, 2020

I would recommend changing to PasswordResets, PasswordChange or UpdatePassword instead and have 3 states.

Always
OnCreate
WhenDifferent

Always would always set the password
OnCreate only resets when the account is created
WhenDifferent sets the password back to the one from DSC (if it gets reset outside of DSC, reset it back)

Deprecate the PasswordNeverResets parameter. If this is set to true, then throw warning and set PasswordResets to OnCreate and if it's false, set to WhenDifferent.

Default option could probably be whatever. I would suggest OnCreate as I am just thinking this is probably about 90% of the use cases. I think right now the default is to reset if password is different. So if neither parameter is specified, it would default to this. Could also throw a deprecated warning here as well if the default behavior changes and to explicitly set what you want.

New parameter takes priority, so if both options are set, ignore PasswordNeverResets and use PasswordResets and throw deprecated warning.

This could allow for other options later on as well. Like WhenExpired, AfterDate, WhenMoonIsFull. You get the idea.

@X-Guardian
Copy link
Contributor

I don't understand what would be the difference between Always and WhenDifferent.

@kungfoome
Copy link

I don't understand what would be the difference between Always and WhenDifferent.

Always will always set the password no matter what (it will basically be out of state anytime you run the config). WhenDifferent only sets the password if it's different than the password that is being set from DSC. If the passwords match, it won't change it. Always will.

Always is useful if you need to force a password change though.

@X-Guardian
Copy link
Contributor

Dsc resources should only apply changes when a resource is detected to be not in the desired state, so Always does not fit this pattern. I only see two options; only set the password when the user is initially created or always set the password when it is detected to not be in the desired state.

Making a breaking change to rename this parameter seems overkill to me. Much better to just add a better description for it in the help/Wiki.

@kungfoome
Copy link

kungfoome commented Aug 25, 2020

Dsc resources should only apply changes when a resource is detected to be not in the desired state, so Always does not fit this pattern. I only see two options; only set the password when the user is initially created or always set the password when it is detected to not be in the desired state.

Making a breaking change to rename this parameter seems overkill to me. Much better to just add a better description for it in the help/Wiki.

Makes sense. This is what ansible does or is supposed to do. Currently, 'Always' is broken and its the same two states you mentioned.

ansible/ansible#58246

I'm indifferent to whatever is decided. The only major thing from me is just adding a state to only set the password when the account is created.

@X-Guardian
Copy link
Contributor

That setting is already there @kungfu71186, use PasswordNeverResets=$true

@kungfoome
Copy link

That setting is already there @kungfu71186, use PasswordNeverResets=$true

I see, so when this is true it only sets the password on create and when false it updates when the passwords differ. In that case, then yeah, it does make sense.

@johlju
Copy link
Member Author

johlju commented Aug 25, 2020

Yes the two states already exist and works. It was just the naming of the parameter that I thought could be more intuitive.

But since that is a breaking change (eventually, first we could deprecate the old parameter name) we could do it when there are more breaking changes to be merged. But if we decide to just improve the documentation and provide a better example then that might be sufficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change When used on an issue, the issue has been determined to be a breaking change. enhancement The issue is an enhancement request. help wanted The issue is up for grabs for anyone in the community.
Projects
None yet
Development

No branches or pull requests

3 participants