Add ACL constructor that tolerates files with unknown ACE types

[jrobhoward]

I'd like to use JNA's ACL constructor on an NTFS file that happens to contain both supported ACE types (ACCESS_ALLOWED_ACE_TYPE, ACCESS_DENIED_ACE_TYPE) as well as an unsupported type (ACCESS_ALLOWED_CALLBACK_ACE_TYPE). The constructor throws an IllegalArgumentException.

This change preserves the same default behavior, but also allows folks to construct ACL objects against files containing unsupported/unknown ACE types.

The goal here isn't to access the unsupported type, just to get a fully-constructed ACL object so it's possible to access the supported ACCESS_ALLOWED_ACE_TYPE & ACCESS_DENIED_ACE_TYPE ACE types.

[matthiasblaesing]

I understand your idea and usecase, though I'm not happy with overloading the constructor this way. Here is what I would do (I assume here, that the ACEs are either read once of the array is stored by the caller):

• remove the private variable for ACEs
• move the extraction code into getACEStructures
• overload getACEStructures with the suggested tolerateUnknownAceTypes parameter

For the unknown type - I would not use ACCESS_DENIED_ACE structure to decode it, but use WinNT.ACE_HEADER. This way you don't pretend to know more about it, than what is the case.

Please add documentation for the parameter - and also consider if you want to return an array with holes (the unsupported entries) or just a shorted array. You could gather the ACE entries in a list and convert that to an array on return.

Could you add a test for this? I think building an ACL in memory and trying to read that should be enough.

[jrobhoward]

A change in contract:

• IllegalArgumentException (on construction) now becomes an IllegalStateException (on consumption)

[jrobhoward]

Possible source of confusion:

• If tolerateUnknownAceTypes is true, and an unknown ACE type is encountered, the returned array size may be less than AceCount
• If they iterate through AceCount (calling GetAce()), it will yield different results

[jrobhoward]

I left the default behavior to not tolerate unknown ACE types

• It's closer to the original implementation ..but an exception is now thrown from getACEStructures() instead of the constructor.

[jrobhoward]

Thanks for the feedback.

I know JavaDoc (for the new parameter) and unit tests (verifying we get an exception when expected, verifying we get a reduced array when expected) are still missing. I can take care of those with another commit after I figure out how to mock/construct a Pointer with the desired characteristics.

Do the updated changes I have so far look reasonable?

[matthiasblaesing]

I had another look at it and I think we should take one more step and cleanup ACEStructure vs. ACE_HEADER and unity the API to always return ACE_HEADER as the base class. Users can then check for subclasses and act accordingly. Please have a look at this:

https://github.com/matthiasblaesing/jna/tree/pr947

This is a special situation, as the step up to 5.0 will allow for API breaches. I don't want to force them, but in this case I think its a valid cleanup. What do you think?

[matthiasblaesing]

@jrobhoward did you find some time to have another look at this?

[jrobhoward]

Sorry, I've been a bit busy.. I just looked over the source diff (but did not compile/execute it). This should accomplish what I need.

Comments (minor):

• Inside AdvApi32Util.java, original line number 2234 was removed (a memory.clear()). Just to confirm: that was intentional (i.e. the clear wasn't necessary because memory variable isn't used past that point)?
• Should getACEStructures() method be renamed to getACEHeaders() along with the type change?
• The new implementation returns an array with a size that matches AceCount (good / +1).
• Is there a whitespace convention? Advapi32Util.java seems to contain a mix of tabs & spaces.

[matthiasblaesing]

@jrobhoward thank you for taking a look - for your comments:

• The removed memory.clear() clears memory, that is used as backing memory for the SECURITY_DESCRIPTOR_RELATIVE structure. The clearing is not necessary and might be even dangerous, if the structure is read again. So yes it is intentional. The memory is freeed by the Memory#finalizer when it goes out of scope.
• I renamed the method getACEStructures to getACEs I think this correctly summarizes the method
• yes - that was the intention :-)
• There is no whitespace convention - I try to stay with the style of the existing code, but it might differ

I pushed the rename to the branch mentioned above. I'd appretiate another look. If you aggree, I'll squash your changes together into one commit and will do the same for my commits and merge both commits to master.

[jrobhoward]

I pushed the rename to the branch mentioned above. I'd appretiate another look. If you aggree, I'll squash your changes together into one commit and will do the same for my commits and merge both commits to master.

The code change looks good. I agree / see no problems squashing the commits & performing a merge.

[matthiasblaesing]

I merged the updated PR. Thank you for your contribution.