Extend Windows ACL matchers

[TheLonelyGhost]

---

Suggested Tags: enhancement

---

Given the limitations outlined in #1743, it would be convenient to have a better interface with the Windows permissions.

I've chosen the following approach:

describe file('C:/Temp/file.txt') do
  it { should allow('read').by_user('SomeOtherUser') }
  it { should allow('delete').by_user('MyUser') }
  it { should_not allow('change-permissions').by_user('SomeOtherUser') }
end

Note that allow('read') is synonymous with be_readable, but is included for consistency's sake.

[adamleff]

Hi, @TheLonelyGhost! Thanks for your contribution, and thanks for reaching out for guidance.

I'm going to defer to @chris-rock and @arlimus for more feedback. My initial reaction is that I feel this is a bit confusing in that there would be two ways to determine if a file is readable or not (the be_readable syntax and the allow('read') syntax. I'd rather we keep the same syntax and just enhance it to be smarter.

As for the other two permissions you've proposed, I think it's be good to introduce the right matcher for those... such as allow_full_control and be_modifiable or something like that.

Again, deferring to @chris-rock and @arlimus for more feedback.

[TheLonelyGhost]

I considered that method too, but then considered that noting compatibility for each platform would be easier to list as values for this one matcher allow() versus a list of methods.

For instance:

Possible values for the allow matcher depend on the platform. Unix-based platforms support the following:

• read
• write
• execute

Windows platforms support the following:

• full-control (alias for all permissions below)
• modify (alias for read, write, and execute)
• read
• write
• execute
• change-permissions
• delete
• delete-subdirectories-and-files
• take-ownership

Windows platforms also support more granular selectors already covered with read and write, such as:

• read-data
• read-permissions
• read-attributes
• read-extended-attributes
• write-data
• write-attributes
• write-extended-attributes
• append-data

(More info on the Windows ACL)

Example:

it { should allow 'change-permissions' }

As a developer, I would have difficulty guessing the matcher form of each permission, but with the allow() syntax I can see the heuristic of changing the permission to a lowercase, hyphenated version from the official windows permissions. It's also modeled similar to the way Chef handles the Windows ACL.

I'd be open to switching from string values for allow() to symbols, closer to how Chef handles it. Thoughts?

[adamleff]

@TheLonelyGhost I totally see your point. I think the allow syntax is likely acceptable, but I'd like @chris-rock and @arlimus to weigh in on this one.

[arlimus]

@TheLonelyGhost Thank you for your contribution! Your explanation makes a lot of sense. Traditionally we have been very hesitant in introducing new matchers (at least for regular resources). However, this is a different case and I see the use-case.
We are a bit swamped this week but we'll discuss this PR before Friday. Sorry for the delay.

[adamleff]

@TheLonelyGhost I wanted you to know we haven't forgotten about you and this PR. This is the week leading up to ChefConf and everyone is pretty heads-down on prep. I promise we'll get back to you as soon as possible, likely after ChefConf. Thank you for your patience. :)

[TheLonelyGhost]

Not a problem. I have a temporary workaround until this is merged, so it has bumped down a few notches in priority. I expect that other profiles I write will need to check permissions in a fine-grained way like this so I'm still very invested in getting this feature extension pushed through.

Here's hoping there's an announcement at Chef Conf about vendoring custom InSpec resources... 🤞

[TheLonelyGhost]

Now that Chef Conf is over (sorry I didn't get to meet you in-person @arlimus! great presentation though), where did we land with this PR?

[TheLonelyGhost]

Rebased from latest changes on master

[TheLonelyGhost]

Rebasing again...

[adamleff]

I'm adding this PR to the next maintainers meeting which should take place next week. Thanks for your patience.

[adamleff]

Hi @TheLonelyGhost, and thanks for your patience. We discussed this PR today. We think this is a fine addition, but we would prefer to not introduce any additional global matchers, such as allow. However, I'd like to propose a slightly different syntax that will get you this feature but also move the matcher logic into the resource itself.

If we create an allowed? method, we get a be_allowed matcher for this resource and we can then pass the additional data as hash options. For example:

describe file('c:\temp\foo.txt') do
  it { should be_allowed('read', by_user: 'Administrator') }
end

While it's not as natural as your proposal, it's pretty close and I think a nice compromise to get the feature supported without making global changes.

What do you think?

[TheLonelyGhost]

You know what? I really like that. I forgot about the automatic matcher part of RSpec and it bothered me to have the allows() matcher so global.

The hash of options thing I'll have to dig into more to make that change in a way that makes sense, but I also like that one. 👍

Will make the changes this week.

[adamleff]

@TheLonelyGhost awesome! Thank you for being receptive to our feedback, and thanks again for your patience. Please reach out if I can help in any way.

[TheLonelyGhost]

A little belated, but rebases off of master (again) and pushed an additional commit per the change request. 👍

[adamleff]

@TheLonelyGhost This is looking really good! However, it looks like you need to update your tests to use be_allowed instead of allow - you have a whole bunch of integration test failures.

Let me know when you've fixed those up and the tests are green, and I'll be happy to re-review!

[TheLonelyGhost]

Ah shoot, it has been long enough that I assumed the integration tests were included in rake test. Fixed and waiting for the tests to finish running locally before I push. Also rebasing off of master again.

[TheLonelyGhost]

There we go. All integration tests passing again. My fault!

/cc @adamleff

[adamleff]

This looks wonderful, @TheLonelyGhost! Thank you for working with us on this.

[TheLonelyGhost]

Fantastic. Glad to help!

[chris-rock]

@TheLonelyGhost This is a great improvement.