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.

Sign up for GitHub

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

Jump to bottom

win_user_right: add module with tests #26276

Merged

jborean93 merged 7 commits into ansible:devel from jborean93:win_user_right-module

Aug 10, 2017

+955 −0

Contributor

jborean93 commented Jun 30, 2017

SUMMARY

Added a new module win_user_right that allows you to add/remove/set user rights for either local or domain accounts.

ISSUE TYPE

New Module Pull Request

COMPONENT NAME

win_user_right

ANSIBLE VERSION

ansible 2.4.0 (win_user_right-module d5bfe4cf93) last updated 2017/06/30 18:20:08 (GMT +1000)
  config file = None
  configured module search path = [u'/home/jborean/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/jborean/dev/ansible/lib/ansible
  executable location = /home/jborean/dev/ansible/bin/ansible
  python version = 2.7.13 (default, Jun 28 2017, 21:13:49) [GCC 6.3.1 20161221 (Red Hat 6.3.1-1)]

ADDITIONAL INFORMATION

This module is to extend #22775 where it allows you to idempotently set users/groups on user rights without knowing the SID for said account.

ansibot added affects_2.4 community_review module needs_triage new_module new_plugin support:community test_pull_requests windows labels

Contributor

ansibot commented Jun 30, 2017

The test ansible-test sanity --test pep8 failed with the following error:

lib/ansible/modules/windows/win_user_right.py:99:31: W291 trailing whitespace

click here for bot help

ansibot added needs_revision and removed community_review labels

Contributor

ansibot commented Jun 30, 2017

@SamLiu79 @timothyvandenbrande @ar7z1 @blakfeld @brianlloyd @chrishoffman @if-meaton @joshludwig @petemounce @schwartzmx @smadam813

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

ansibot added community_review and removed needs_revision labels

s-hertel removed the needs_triage label

dagwieers mentioned this pull request

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

Closed

ansibot added stale_ci support:core and removed community_review module new_module labels

nitzmahone requested changes

View reviewed changes

Member

nitzmahone left a comment

Also looks great- a couple things that could be tightened up and a couple of potential perf issues for large policies.

lib/ansible/modules/windows/win_user_right.ps1 Outdated

    
                      foreach ($existing_user in $existing_users) {

                          $user_match = $true

                          foreach ($user in $users) {

                              $user_sid = Get-SID -account_name $user

Member

nitzmahone Jul 12, 2017 •

edited

This is potentially expensive if either list is large, since you have to re-lookup the specified user SIDs on each value . Might be better to do a single-pass over each upfront to build a HashSet and use set theory methods to find users to remove. Could also probably call the LINQ set-theory extension methods (they'd take care of building the underlying sets for you).

lib/ansible/modules/windows/win_user_right.ps1 Outdated

    
                  # sort the user objects for later comparison and remove duplicates

                  $new_users = $new_users | Sort-Object -Unique

                  $existing_users = $existing_users | Sort-Object -Unique

Member

nitzmahone Jul 12, 2017

Shouldn't at least the uniqueness be handled inside the Build-XList function (if not also the sort)?

lib/ansible/modules/windows/win_user_right.ps1 Outdated

    
                      removed = @()

                  }

                  foreach ($entry in $existing_list) {

Member

nitzmahone Jul 12, 2017

Another potentially good place for HashSet and set-theory methods (especially if they got built once outside as HashSet, since order probably doesn't matter)...

lib/ansible/modules/windows/win_user_right.ps1

    
              $name = Get-AnsibleParam -obj $params -name "name" -type "str" -failifempty $true

              $users = Get-AnsibleParam -obj $params -name "users" -type "list" -failifempty $true

              $action = Get-AnsibleParam -obj $params -name "action" -type "str" -default "set" -validateset "add","remove","set"

Member

nitzmahone Jul 12, 2017

Do we want to add diff support (esp since you've already done the work of calculating the diff)?

Contributor Author

jborean93 Jul 17, 2017

Have added diff support, I didn't remove the added/removed return items as they were used in the tests and thought they were still useful to have.

lib/ansible/modules/windows/win_user_right.ps1 Outdated

    
                      # check the return code and if the file has been populated, otherwise error out

                      if (($export_result.rc -ne 0) -or ((Get-Item -Path $secedit_ini_path).Length -eq 0)) {

                          Remove-Item -Path $secedit_ini_path # file is empty and we don't need it

                          Fail-Json $result "Failed to export secedit.ini file to $($secedit_ini_path).`nRC: $($export_result.rc)`nSTDOUT: $($export_result.stdout)`nSTDERR: $($export_result.stderr)`nLOG: $($export_result.log)"

Member

nitzmahone Jul 12, 2017

Do we want to include all that in the message, or just return discrete keys in the fail dict? Could see reasons for either...

lib/ansible/modules/windows/win_user_right.ps1 Outdated

    
                          Remove-Item -Path $secedit_ini_path # file is empty and we don't need it

                          Fail-Json $result "Failed to export secedit.ini file to $($secedit_ini_path).`nRC: $($export_result.rc)`nSTDOUT: $($export_result.stdout)`nSTDERR: $($export_result.stderr)`nLOG: $($export_result.log)"

                      }

                      $secedit_ini = ConvertFrom-Ini -file_path $secedit_ini_path

Member

nitzmahone Jul 12, 2017

Couldn't we just extract the initial change calculation into a function and re-run it here, failing if it reported changed?

lib/ansible/modules/windows/win_user_right.py Outdated

    
              options:

                name:

                  description:

                  - The name of the User Right as shown by the Constant name here

Member

nitzmahone Jul 12, 2017

s/Constant name here/C(Constant Name) value from/

test/integration/targets/win_user_right/tasks/tests.yml Outdated

    
                  that:

                  - add_right_on_existing_check|changed

                  - add_right_on_existing_check.removed == []

                  - add_right_on_existing_check.added == ["BUILTIN\\Users", "BUILTIN\\Guests"]

Member

nitzmahone Jul 12, 2017

Why the mix of double-quoted/escaped and single-quoted stuff below?

Contributor Author

jborean93 Jul 17, 2017

\U and \Gthrows an error when using single quotes, I'll change it so they are all consistent and use double quotes.

Contributor

dagwieers Jul 17, 2017 •

edited

You mean single backslashes ?

We are recommending in our new documentation to use single quotes as much as possible to prevent this behavior and the need to escape backslashes.

Contributor Author

jborean93 Jul 17, 2017

so 'BUILTIN\User' fails but "BUILTIN\\Users" is fine, note this is only for the assertions and not for the module parameters, there are test cases where there is an entry called '{{ansible_hostanme}}\Users' and that works fine.

Contributor

dagwieers Jul 17, 2017 •

edited

Oh right, Jinja2 and Python don't care about single or double quotes, which is making our generic advice a bit more difficult than we planned :-( We still have to document the behavior of Jinja2...

cc: @jhawkesworth

ansibot added the needs_revision label

dagwieers mentioned this pull request

Windows Working Group agenda - Jul-Dec 2017 ansible/community#195

Closed

Contributor

dagwieers commented Jul 13, 2017

+label new_module

ansibot added new_module and removed stale_ci labels

Contributor Author

jborean93 commented Jul 17, 2017

@nitzmahone, I've made the changes from your comments, let me know if there is anything I've miseed.

ansibot added the module label

ansibot added stale_ci stale_review labels

jborean93 added 5 commits

August 8, 2017 16:11


          win_user_right: add module with tests

96a2912


          fixed up name of module in docs

9af18c6


          forgot the test module

a8d1d2f


          fixed up whitespace


          changes made to win_user_right based on feedback

e3b0269

jborean93 force-pushed the win_user_right-module branch from 8b19df7 to e3b0269 Compare

August 8, 2017 06:12

ansibot removed stale_ci stale_review labels

jborean93 added 2 commits

August 8, 2017 16:45


          moved away from using secedit to Win32 with P/Invoke

25a50a7


          tidied up copyright for documentation

c3bf6cd

Contributor Author

jborean93 commented Aug 8, 2017

@nitzmahone, just updated this PR to no longer use SecEdit.exe, makes things simpler as we don't need to parse an ini file.

Contributor

dagwieers commented Aug 8, 2017 •

edited

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.

jborean93 merged commit e46adec into ansible:devel

jborean93 deleted the win_user_right-module branch

August 10, 2017 21:52

ansible locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

affects_2.4 module needs_revision new_module new_plugin support:community support:core windows