New module: Add module or managing Windows Active Directory users (windows/win_domain_user) #24075

nwchandler · 2017-04-27T18:26:44Z

SUMMARY

This PR adds support for managing Windows Active Directory users with Ansible. The existing win_user module is designed to support management of local users. There are enough differences between local user management and domain user management that a separate module seemed to make sense. See discussion on Issue #20428.

ISSUE TYPE

New Module Pull Request

COMPONENT NAME

windows/win_domain_user.ps1

ANSIBLE VERSION

ansible 2.3.0.0
  config file =
  configured module search path = Default w/o overrides
  python version = 2.7.12 (default, Jun 29 2016, 14:05:02) [GCC 4.2.1 Compatible Apple LLVM 7.3.0 (clang-703.0.31)]

ADDITIONAL INFORMATION

ansibot · 2017-04-27T18:42:03Z

The test ansible-test sanity --test validate-modules failed with the following error:

lib/ansible/modules/windows/win_domain_user.py:0:0: E312 No RETURN documentation provided

click here for bot help

jhawkesworth · 2017-04-28T07:21:33Z

lib/ansible/modules/windows/win_domain_user.ps1

+# POWERSHELL_COMMON
+
+########
+Import-Module ActiveDirectory


I think this line will fail if run against a non-active directory server. Consider a try catch and fail-json if the ActiveDirectory module is not available.

Thanks, for the review! Good idea. I will make the update.

jborean93

Looks like a great module to have, just a few questions and queries.

jborean93 · 2017-05-13T01:51:38Z

lib/ansible/modules/windows/win_domain_user.ps1

+}
+
+# Parameter validation
+If ($account_locked -ne $null -and $account_locked) {


My brain isn't working right now are you able to explain this logic? To me it seems like it will fail if $account_locked is set to true if it has been set? Why can't we create a locked account?

I followed Microsoft's lead on this one. There is an Unlock-ADAccount cmdlet, but no corresponding Lock-ADAccount. The thinking is that an account would be locked because a user tried to login too many times with incorrect credentials, so an admin would unlock an account, but not lock it. If an account needed to be "locked" by an admin, he/she would just disable the account by setting "enabled" to false.

No worries that makes sense, I can see the same functionality in the win_user module this was just me being ignorant :)

jborean93 · 2017-05-13T01:53:34Z

lib/ansible/modules/windows/win_domain_user.ps1

+        If ($password -and (($new_user -and $update_password -eq "on_create") -or $update_password -eq "always")) {
+            $secure_password = ConvertTo-SecureString $password -AsPlainText -Force
+            Set-ADAccountPassword -Identity $username -Reset:$true -Confirm:$false -NewPassword $secure_password
+            $user_obj = Get-ADUser -Identity $username -Properties *


Should we just run this once after creating the user to save getting these details everytime we want to make a change?

That's what I did initially, but I found problems in testing. As accounts were updated, I ran into problems where later logic would fail. I ended up deciding it was better to refresh the state of the user object after every update.
(Sorry, I know that's vague; I don't recall the specific issues I ran into off-hand.)

Fair enough, sounds like a typical Microsoft thing to me.

jborean93 · 2017-05-13T01:55:33Z

lib/ansible/modules/windows/win_domain_user.ps1

+$password_never_expires = Get-AnsibleParam -obj $params -name "password_never_expires" -type "bool"
+$user_cannot_change_password = Get-AnsibleParam -obj $params -name "user_cannot_change_password" -type "bool"
+$account_locked = Get-AnsibleParam -obj $params -name "account_locked" -type "bool"
+$groups = Get-AnsibleParam -obj $params -name "groups"


There is now a list type that will automatically convert a the item to a list if it isn't already.

Hi, @jborean93 - My apologies for the late reply, but thank you for the comments. Good to know about the list type; I'll update this.

jborean93 · 2017-05-13T01:56:13Z

lib/ansible/modules/windows/win_domain_user.ps1

+
+        # Configure group assignment
+        If ($groups -ne $null) {
+            If ($groups -is [System.String]) {


If using the list type it will automatically convert it to a list and split it by , if it is a string.

jborean93 · 2017-05-13T01:58:41Z

lib/ansible/modules/windows/win_domain_user.py

+
+
+DOCUMENTATION = r'''
+---


Can you add a notes section around where this module can run and the requirements?

Yes, I'll add that.

nathanwebsterdotme · 2017-06-26T11:14:25Z

Looking forward to this one!

dagwieers · 2017-06-26T12:53:23Z

@nathanwebsterdotme If you do, I'd suggest you test it out in your environment and provide feedback. And help with code-review. Those things will speed up it being merged.

dagwieers · 2017-05-09T09:34:58Z

lib/ansible/modules/windows/win_domain_user.ps1

+########
+
+$result = @{
+    changed = $false;


Trailing semi-colon is not needed.

dagwieers · 2017-05-09T09:36:33Z

lib/ansible/modules/windows/win_domain_user.ps1

+    Fail-Json $result "Failed to import ActiveDirectory PowerShell module. This module should be run on a domain controller, and the ActiveDirectory module must be available."
+}
+
+$params = Parse-Args $args


Please look into adding check-mode support. Often you can get away with some strategically places -WhatIf:$check_mode statements !

dagwieers · 2017-05-09T09:38:21Z

lib/ansible/modules/windows/win_domain_user.py

+      - When C(present), creates or updates the user account.  When C(absent),
+        removes the user account if it exists.  When C(query),
+        retrieves the user account details without making any changes.
+    required: false


If parameters are not required, you don't have to add required: false. Only add required: true if they are required.

dagwieers · 2017-05-09T09:38:53Z

lib/ansible/modules/windows/win_domain_user.py

+      - C(no) will unlock the user account if locked.
+    required: false
+    choices: [ 'no' ]
+    default: null


This is not needed, the default value is null, so it's implicit.

dagwieers · 2017-05-09T09:39:23Z

lib/ansible/modules/windows/win_domain_user.py

+    description:
+      - C(no) will unlock the user account if locked.
+    required: false
+    choices: [ 'no' ]


Wouldn't the choice be [ 'yes', 'no' ] as you'd expect from a boolean ?
As per previous remark, this would become:

type: bool

dagwieers · 2017-06-26T12:56:32Z

lib/ansible/modules/windows/win_domain_user.py

+    description:
+      - C(yes) will enable the user account. C(no) will disable the account.
+    required: false
+    choices:


Nowadays we do:

type: bool default: 'yes'

dagwieers · 2017-06-26T12:59:26Z

lib/ansible/modules/windows/win_domain_user.py

+version_added: "2.4"
+short_description: Manages Windows Active Directory user accounts
+description:
+     - Manages Windows Active Directory user accounts


Trailing dot required. I know, quite pedantic rules and all.

dagwieers · 2017-06-26T13:00:36Z

lib/ansible/modules/windows/win_domain_user.py

+    default: null
+
+author:
+    - "Nick Chandler (@nwchandler)"


We tend to not add quotes where they are no needed in YAML.
Here they are not needed.

nwchandler · 2017-07-27T21:09:47Z

Hi, all. I believe I've addressed the comments in the new version of code (pending one last Shippable test). @dagwieers - I did some work on enabling check_mode, but it gave me some problems. I think it may take a little more re-work than I originally envisioned. So, I pushed the other requested changes. I can take another swing at it at some point, but it may be a while before I can, and I wanted to get the other updates in. I'll defer to you whether that would be a blocker or would be something we could add in the future.

dagwieers · 2017-07-27T21:53:05Z

lib/ansible/modules/windows/win_domain_user.py

+DOCUMENTATION = r'''
+---
+module: win_domain_user
+version_added: 2.4


This should be a string:

version_added: '2.4'

dagwieers · 2017-07-27T21:54:25Z

lib/ansible/modules/windows/win_domain_user.py

+      - C(yes) will require the user to change their password at next login.
+        C(no) will clear the expired password flag. This is mutually exclusive
+        with I(password_never_expires).
+    choices: [ 'yes', 'no' ]


This should be:

type: bool

dagwieers · 2017-07-27T21:54:44Z

lib/ansible/modules/windows/win_domain_user.py

+    description:
+      - C(yes) will set the password to never expire.  C(no) will allow the
+        password to expire. This is mutually exclusive with I(password_expired)
+    choices: [ 'yes', 'no' ]


This should be:

type: bool

dagwieers · 2017-07-27T21:54:55Z

lib/ansible/modules/windows/win_domain_user.py

+    description:
+      - C(yes) will prevent the user from changing their password.  C(no) will
+        allow the user to change their password.
+    choices: [ 'yes', 'no' ]


This should be:

type: bool

dagwieers · 2017-07-27T21:56:59Z

lib/ansible/modules/windows/win_domain_user.ps1

+        # If the account does not exist, create it
+        If (-not $user_obj) {
+            If ($path -ne $null){
+                New-ADUser -Name $username -Path $path


Add here -WhatIf:$check_mode

dagwieers · 2017-07-27T21:57:08Z

lib/ansible/modules/windows/win_domain_user.ps1

+                New-ADUser -Name $username -Path $path
+            }
+            Else {
+                New-ADUser -Name $username


Add here -WhatIf:$check_mode

dagwieers · 2017-07-27T21:57:53Z

lib/ansible/modules/windows/win_domain_user.ps1

+            Else {
+                New-ADUser -Name $username
+            }
+            $user_obj = Get-ADUser -Identity $username -Properties *


I guess, skip everything that follows on $check_mode (e.g. by running Exit-Json)

dagwieers · 2017-07-27T21:58:57Z

lib/ansible/modules/windows/win_domain_user.ps1

+
+        # Configure password policies
+        If (($password_never_expires -ne $null) -and ($password_never_expires -ne $user_obj.PasswordNeverExpires)) {
+            Set-ADUser -Identity $username -PasswordNeverExpires $password_never_expires


Add here -WhatIf:$check_mode

dagwieers · 2017-07-27T21:59:08Z

lib/ansible/modules/windows/win_domain_user.ps1

+            $result.changed = $true
+        }
+        If (($password_expired -ne $null) -and ($password_expired -ne $user_obj.PasswordExpired)) {
+            Set-ADUser -Identity $username -ChangePasswordAtLogon $password_expired


Add here -WhatIf:$check_mode

dagwieers · 2017-07-27T21:59:17Z

lib/ansible/modules/windows/win_domain_user.ps1

+            $result.changed = $true
+        }
+        If (($user_cannot_change_password -ne $null) -and ($user_cannot_change_password -ne $user_obj.CannotChangePassword)) {
+            Set-ADUser -Identity $username -CannotChangePassword $user_cannot_change_password


Add here -WhatIf:$check_mode

dagwieers · 2017-07-27T22:05:03Z

@nwchandler check-mode is a must-have for every (new) module. It is best to first make it full-featured, then look at supporting check-mode. In this case, I think you can get away with adding -WhatIf:$check_mode to most statements that modify things (like Add-ADUser, Set-ADUser, ...) and leave out some of the post-change validation/lookups. Maybe terminate the module prematurely if needed (even if that means not everything can be checked properly). Check-mode is often best-effort (and heavily depends on low-level implementation details we have no control over).

Let me know if you need help.

nwchandler · 2017-07-28T00:40:06Z

Thanks for the feedback and ideas on check_mode support, @dagwieers. I have made some updates accordingly. Short-circuiting and exiting early is a good idea.

jborean93

Looks good, just did some local tests and things seem to work correctly here. Keen to get this in and people testing it in real life situations.

jborean93 · 2017-07-31T22:55:06Z

shitpit

nwchandler · 2017-08-03T19:34:28Z

Thanks for everyone's reviews and feedback!

dagwieers · 2017-08-08T14:20:55Z

So the module freeze deadline for v2.4 is getting closer (2017-08-29). Now is the time to finish up, get it reviewed and merged with no delay.

nwchandler · 2017-08-08T21:34:23Z

It looks like @jborean93 approved - anything else I need to do to make this happen, @dagwieers ?

dagwieers · 2017-08-08T22:13:42Z

@nwchandler If you're asking me specifically, the only thing I noted was the use of type: bool in the documentation. Ah, and now I see you did add it, but it should replace the choices: ['no', 'yes'] part.

jborean93 · 2017-08-11T00:23:03Z

Thanks for the work here @nwchandler, I'm going to merge this in so people can start using it in devel. Are you able to update https://github.com/ansible/ansible/blob/devel/.github/BOTMETA.yml and https://github.com/ansible/ansible/blob/devel/CHANGELOG.md with the relevant info for the new module.

dagwieers · 2017-08-11T02:41:18Z

BOTMETA should not require updating if the author(s) are listed in the module.

jborean93 · 2017-08-11T02:43:33Z

oh did not know that, the more you know

nwchandler mentioned this pull request Apr 27, 2017

win_user: Does not allow modification of domain users #20428

Closed

ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Apr 27, 2017

Qalthos removed needs_triage Needs a first human triage before being processed. new_plugin This PR includes a new plugin. labels Apr 27, 2017

ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Apr 27, 2017

jhawkesworth reviewed Apr 28, 2017

View reviewed changes

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label May 9, 2017

dagwieers mentioned this pull request May 12, 2017

Windows Working Group agenda - Mar-Jun 2017 ansible/community#153

Closed

jborean93 requested changes May 13, 2017

View reviewed changes

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels May 13, 2017

alikins changed the title ~~Initial win_domain_user module support~~ New module: Add module or managing Windows Active Directory users (windows/win_domain_user) May 22, 2017

dagwieers suggested changes Jun 26, 2017

View reviewed changes

Nick Chandler added 2 commits July 27, 2017 16:52

Improve win_domain_user module

cdf5c7e

Fix bad merge

9cacd83

nwchandler force-pushed the devel branch from 4ef91bb to 9cacd83 Compare July 27, 2017 20:53

ansibot removed merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Jul 27, 2017

Fix pep8 failure

45d4d27

Actually fix pep8 failure

39daf85

dagwieers suggested changes Jul 27, 2017

View reviewed changes

Nick Chandler added 2 commits July 27, 2017 20:03

Update win_domain_user.py to meet standards

fee14ac

Add check_mode support for win_domain_user

1361bbc

jborean93 approved these changes Jul 31, 2017

View reviewed changes

ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Aug 5, 2017

Updated documentation before merge

f0aa314

ansibot removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Aug 11, 2017

jborean93 merged commit 22533c0 into ansible:devel Aug 11, 2017

ansible locked and limited conversation to collaborators Apr 26, 2019



		DOCUMENTATION = r'''
		---

New module: Add module or managing Windows Active Directory users (windows/win_domain_user) #24075

New module: Add module or managing Windows Active Directory users (windows/win_domain_user) #24075

Conversation

nwchandler commented Apr 27, 2017

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

ansibot commented Apr 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jborean93 left a 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

nathanwebsterdotme commented Jun 26, 2017

dagwieers commented Jun 26, 2017

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

dagwieers May 9, 2017 • edited Loading

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

nwchandler commented Jul 27, 2017

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

dagwieers commented Jul 27, 2017

nwchandler commented Jul 28, 2017 • edited Loading

jborean93 left a comment

Choose a reason for hiding this comment

jborean93 commented Jul 31, 2017

nwchandler commented Aug 3, 2017

dagwieers commented Aug 8, 2017 • edited Loading

nwchandler commented Aug 8, 2017

dagwieers commented Aug 8, 2017 • edited Loading

jborean93 commented Aug 11, 2017

dagwieers commented Aug 11, 2017

jborean93 commented Aug 11, 2017

dagwieers May 9, 2017 •

edited

Loading

nwchandler commented Jul 28, 2017 •

edited

Loading

dagwieers commented Aug 8, 2017 •

edited

Loading

dagwieers commented Aug 8, 2017 •

edited

Loading