-
Notifications
You must be signed in to change notification settings - Fork 23.8k
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
win_group_membership failing if user/group specified is already a member of the group #40649
Comments
In the event that anyone has a similar issue and needs a workaround until this is fixed - I use the following snippet instead - name: Add new Role Group to Local Admins via PowerShell
raw: "powershell.exe -ExecutionPolicy Unrestricted -command \"& { ([adsi]'WinNT://./administrators').Add('WinNT://{{ windows_domain_name }}/{{ ansible_hostname|upper }},group'); }\"" |
!component =lib/ansible/modules/windows/win_group_membership.ps1 |
Hey @kxseven, I suspect the problem you're seeing is actually what was previously identified in #39358, where the module fails to add/remove members of a group where the system hostname is part of the member name. This should only occur in domain user/group scenarios, as local groups cannot be added to another local group, and local users the same name as the hostname cannot be created. In general, the module should work without issue for any other user or group you try that doesn't contain the hostname within its name. The initial solution proposed by @phobos786 I don't believe covered this particular case, since your domain group is an exact match of the hostname, rather than the hostname being a subset of the group name. My testing shows changing line 136 of the module, which is used to identify local objects, to the following fixes this: if ($split_adspath -match "^$env:COMPUTERNAME$" -and $split_adspath[-1] -notmatch $env:COMPUTERNAME) {
...
} This covers both preventing against an accidental substring match, while also allowing for domain users and groups to have a name that can exactly match the hostname. i.e. Anything other than the final ADsPath item once split must match the hostname exactly to be considered local. I'll see if I can put in a quick PR later to address this, since a couple of folks have encountered it. |
After looking at this I think the best and proper option to go forward with is to not compare against strings but against the SID's of the objects. We can easily get the SIDs of the accounts in a group with
This returns an array of SecurityIdentifier objects that are members of that group. From there we can compare the members that we want to add/remove/set from the Ansible args and act accordingly. This was we don't need to worry about checking domain, account names but the unique identifier for each account. |
Hey @jborean93, that's a good idea, and would definitely simplify a lot of the code, as most of the work could probably be offloaded to the SID ModuleUtils you wrote. The only concern I can think of in doing this would be the change in return values. I'm guessing it would still make sense to return either ADsPath or the result of Either of these options would mean return values for local accounts would no longer match across hosts (without some parsing), as computer name would be included in the path. However, to maintain purely identical results to what we have now would require much of the logic that exists today. If this sounds like a good path to pursue, I may be able to work on something for this in the coming weeks. |
I think that’s a good plan, having the hostname isn’t the end of the world, you can always split by \ if you just want the account. Definitely a +1 to returning the account name and not the SID, the SIDs should just be for internal comparison. |
Was able to replicate this locally by creating an AD account with the server's hostname set as the username. When applying #40725 the issue is no longer there. |
SUMMARY
ISSUE TYPE
COMPONENT NAME
win_group_membership.ps1
(Ref: https://devdocs.io/ansible~2.4/win_group_membership_module)
ANSIBLE VERSION
CONFIGURATION
OS / ENVIRONMENT
ansible -m setup servername123
(some info redacted)STEPS TO REPRODUCE
EXPECTED RESULTS
ACTUAL RESULTS
The Ansible task fails with a fatal error
The text was updated successfully, but these errors were encountered: