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

Make win_domain_user idempotent for password changes #58383

Open
wants to merge 8 commits into
base: devel
from

Conversation

Projects
None yet
4 participants
@agowa338
Copy link
Contributor

commented Jun 26, 2019

SUMMARY

win_domain_user always returns changed if a password is specified.
This pr implements an additional option to not only update the password "on_create" or "always", but also "when_changed"

Fixes #58246

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

win_domain_user

OT side note I just learned, that GitHub does not allow to reopen PRs after you force pushed to the branche.

@ansibot

This comment has been minimized.

@ShachafGoldstein

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

If you could change the test function to get a psctedential object it would be better, that change will have to happen on all modules eventually so best have it done on new changes

@ansibot ansibot removed the needs_triage label Jun 26, 2019

@agowa338

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

I don't think I understand what you mean. Do you mean, changing it to the following?

The caller of the test function should convert the plaintext credentials into a pscredential object and pass that object to the function.
The test-credential function should take a pscredential object as param extract plaintext credentials again using GetNetworkCredential().Password and throw them against the native windows api?

I don't know how retrieving the password as a string could be circumvented entirely and from the above, has that any benefit? I currently can only see additional complexity, therefore this might not be what you actually meant...

@agowa338

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

@ShachafGoldstein

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2019

@jborean93

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2019

@ShachafGoldstein we typically ignore securestrings and PSCredentials unless the internal function we call requires that object. Because all the data is stored as a string in memory there is very little benefit to them cast it to a secure string only to then cast it back to a string later on. I don’t see this changing anytime soon and would avoid recommending it for an Ansible module.

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2019

The test ansible-test sanity --test pslint [explain] failed with 1 error:

lib/ansible/modules/windows/win_domain_user.ps1:232:102: MissingEqualsInHashLiteral Missing '=' operator after key in hash literal.

click here for bot help

@agowa338 agowa338 force-pushed the agowa338:fix_#58246 branch from fe08f98 to 2f15f1a Jul 3, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

The test ansible-test sanity --test pslint [explain] failed with 1 error:

lib/ansible/modules/windows/win_domain_user.ps1:22:9: PSUseDeclaredVarsMoreThanAssignments The variable 'Domain' is assigned but never used.

click here for bot help

@ansibot ansibot added ci_verified and removed needs_rebase labels Jul 3, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

The test ansible-test sanity --test pslint [explain] failed with 1 error:

lib/ansible/modules/windows/win_domain_user.ps1:22:9: PSUseDeclaredVarsMoreThanAssignments The variable 'Domain' is assigned but never used.

click here for bot help

@ansibot ansibot removed the ci_verified label Jul 3, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

The test ansible-test sanity --test pslint [explain] failed with 1 error:

lib/ansible/modules/windows/win_domain_user.ps1:19:11: MissingStatementBlockAfterElse Missing statement block after 'else' keyword.

click here for bot help

@ansibot ansibot added the ci_verified label Jul 3, 2019

@agowa338

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

As discussed in the last meeting @jborean93, this pr should now be ready for review.

Show resolved Hide resolved changelogs/fragments/58246-win_domain_user-idempotency.yaml Outdated
Show resolved Hide resolved lib/ansible/modules/windows/win_domain_user.ps1
} else {
# No domain provided, so maybe local user?
}
$null = $Domain # Make CI-Check happy...

This comment has been minimized.

Copy link
@jborean93

jborean93 Jul 4, 2019

Contributor

What's the CI check, we use $Domain down below so I assume it's not for an unused variable.

This comment has been minimized.

Copy link
@agowa338

This comment has been minimized.

Copy link
@jborean93

jborean93 Jul 5, 2019

Contributor

That doesn't make sense, you are using $Domain later on, will have to look into PSScriptAnalyzer to try and figure out what's happening there.

Show resolved Hide resolved lib/ansible/modules/windows/win_domain_user.ps1 Outdated
Show resolved Hide resolved lib/ansible/modules/windows/win_domain_user.ps1 Outdated
Show resolved Hide resolved lib/ansible/modules/windows/win_domain_user.ps1 Outdated
Show resolved Hide resolved lib/ansible/modules/windows/win_domain_user.ps1 Outdated
)
) {
If (-not $new_user) { # Don't uncecessary check if the credentials works, if we know it doesn't.
$test_new_credentials = Test-Credential -Username $username -Password $password

This comment has been minimized.

Copy link
@jborean93

jborean93 Jul 4, 2019

Contributor

We also don't want to test the credentials if update_password is on_create and we didn't create a new user. Right now this will always check and update the password for both on_create and when_changed effectively making them the same thing. We could always make this more efficient by not testing the credentials when update_password is always as we will always update it regardless of this result.

This comment has been minimized.

Copy link
@agowa338

agowa338 Jul 5, 2019

Author Contributor

No it won't $new_user is only true if the user was newly created https://github.com/ansible/ansible/pull/58383/files#diff-a814da53b65fce105cb5fb909b79b267R201 therefore it won't be touched.

The outer if-statement checks:

($password) ∧ (($new_user ∧ $update_password == "on_create") ∨ ($update_password ∈ {'always', 'when_chantged'}))

The inner if-statement:

¬($new_user)

So Test-Credential is executed when:

($password) ∧ (($new_user ∧ $update_password == "on_create") ∨ ($update_password ∈ {'always', 'when_chantged'})) ∧ ¬($new_user)

And for the case where $password = 'something' ∧ update_password == 'on_create' ∧ $new_user == $true (we just created the user) it will results in:

  1. $password => $true
  2. (($new_user ∧ $update_password == "on_create") => $true
  3. ¬($new_user) => $false

=> So it will not be executed.

And for the case where $password = 'something' ∧ update_password == 'on_create' ∧ $new_user == $false (we the user already existed) it will results in:

  1. $password => $true
  2. ($new_user ∧ $update_password == "on_create") => $false
  3. ($update_password ∈ {'always', 'when_chantged'}) => $false

=> So it will not be executed either.

Maybe this if statement is too complicated, I'll consider rewriting it.

This comment has been minimized.

Copy link
@jborean93

jborean93 Jul 5, 2019

Contributor

Ahh yes I see it now sorry, we should still avoid testing the credentials when the credentials is always as we will be changing it anyway.

It did seem a bit confusing when I was reading through it originally so if you can simplify it then by all means go ahead.

Show resolved Hide resolved lib/ansible/modules/windows/win_domain_user.py Outdated
Show resolved Hide resolved lib/ansible/modules/windows/win_domain_user.py Outdated

@ansibot ansibot added needs_revision and removed core_review labels Jul 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.