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

Win_dacl module: fix FULLCONTROL / FILE_ALL_ACCESS definition #31906

Merged
merged 1 commit into from Mar 16, 2016
Merged

Win_dacl module: fix FULLCONTROL / FILE_ALL_ACCESS definition #31906

merged 1 commit into from Mar 16, 2016

Conversation

sbreidba
Copy link
Contributor

What does this PR do?

Fix FULLCONTROL / FILE_ALL_ACCESS definition (bugfix and code simplification).
Use consistent mechanism for obtaining user SID.
Allow wildcarding (via optional parameters) for a variety of methods (get, rm_ace, check_ace).

What issues does this PR fix or reference?

None.

Previous Behavior

Adding a FULLCONTROL ACE with FOLDER&SUBFOLDERS&FILES propagation to a file system object resulted in Windows Explorer showing "Special" for that file system object. This is because multiple ACEs were created as a result - one that was "folder only" and one that was "subfolders and files".

This had the side effect of causing check_ace (and therefore the win_dacl.present state) to always report that ACE as missing, so each repeated state run would add another pair of ACEs to the DACL.

Secondarily, most functions required exact matching to execute. For example, rm_ace required that the user, access type, permission, and propagation all match exactly. This made it impossible to remove all ACEs for a given user (for example).

New Behavior

When using FULLCONTROL and FOLDER&SUBFOLDERS&FILES propagation, a single ACE is added and is correctly found after it has been added.

Secondarily, if optional parameters are omitted (or set to None) then wildcard matching is allowed for certain functions, making it possible to remove all ACEs for a given user (or all DENY ACEs for that user, etc.)

Tests written?

  • Yes
  • No

…ication).

Use consistent mechanism fro obtaining user SID.
Allow wildcarding (via optional parameters) for a variety of methods (get, rm_ace, check_ace).
@cachedout
Copy link
Contributor

@UtahDave or @twangboy Can one of you please look this over?

@sbreidba
Copy link
Contributor Author

If it's helpful, the following was done on a Windows 7 machine to test:

test_dacl:
  win_dacl.present:
    - name: c:/foo
    - permission: FullControl
    - acetype: allow
    - propagation: 'FOLDER&SUBFOLDERS&FILES'
    - objectType: directory
    - user: Everyone
  1. Create a temporary directory (c:\foo is used in the test state above). By default (at least on Win7) "Everyone" does not have an ACE listed in Windows Explorer (or visible via salt <minion> win_dacl.get path=c:/foo objectType=DIRECTORY) for a new directory created off of the root of the volume.
  2. Run this state pre-fix, view in Windows Explorer, and note that c:/foo shows "Special" permissions for Everyone. Also run salt <minion> win_dacl.get path=c:/foo objectType=DIRECTORY to see how the Everyone ACE became two ACEs.
  3. Apply the fix.
  4. Remove the existing Everyone ACEs, either via Windows Explorer or via salt <minion> win_dacl.rm_ace path=c:/foo objectType=DIRECTORY user=Everyone. (This would have failed with missing parameters before the fix.)
  5. Re-run the test state. After this, win_dacl.get should list a single entry for Everyone and Windows Explorer should show a clean line of checkboxes as in the screenshot below.
    image

@twangboy
Copy link
Contributor

@cachedout Looks good to me.

cachedout pushed a commit that referenced this pull request Mar 16, 2016
Win_dacl module: fix FULLCONTROL / FILE_ALL_ACCESS definition
@cachedout cachedout merged commit a4b3462 into saltstack:2015.8 Mar 16, 2016
@cachedout
Copy link
Contributor

Thanks very much for this one, @sbreidba. Looks great.

@sbreidba sbreidba deleted the win_dacl_fixes branch March 16, 2016 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants