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

#379 expose 'samDesired' param to all methods #1001

Merged
merged 7 commits into from
Aug 26, 2018

Conversation

CamW
Copy link
Contributor

@CamW CamW commented Aug 12, 2018

Exposes samDesired as discussed in the issue. Includes tests which use HKEY_LOCAL_MACHINE instead of HKEY_CURRENT_USER. This is because HKEY_LOCAL_MACHINE is redirected whereas HKEY_CURRENT_USER is shared. https://docs.microsoft.com/en-us/windows/desktop/winprog64/shared-registry-keys#redirected-shared-and-reflected-keys-under-wow64 Using HKEY_LOCAL_MACHINE allows for tests which ensure that the extra samDesired flags are applied by performing operations on the same registry keys across the 32 and 64 bit registry without conflict. Tests will only run on 64bit windows since WOW64 would be ignored on 32bit and break the tests.

Copy link
Member

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general the idea looks sound - I left one comment inline about the implementation.

The unittests for the extended rights require administrator rights - can you explain why that is and maybe circumvent it?

And please add an entry to the enhancements section in CHANGES.md for the next release. Refer to the existing entries for the desired structure.

@@ -593,9 +609,28 @@ public static boolean registryKeyExists(HKEY root, String key) {
* @return True if the value exists.
*/
public static boolean registryValueExists(HKEY root, String key,
String value) {
String value) {
return registryValueExists(root, key, value, WinNT.REG_NONE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't use WinNT.REG_NONE for this use-case. WinNT.REG_NONE is a type, not an access level. There is no "NONE" access level for the registry. I would just use 0 here. I would not invent a constant.

@CamW
Copy link
Contributor Author

CamW commented Aug 14, 2018

I've switched out the bad WinNT.REG_NONEs with 0 as requested. The tests require admin rights because they're accessing HKLM. HKCU\Software isn't redirected as mentioned originally but HKCU\Software\Classes\CLSID is so I can use that instead. I've demonstrated the change in the testRegistryCreateDeleteKeySamExtra test in the latest commit. If you find that preferable, I can modify the others to match.

@matthiasblaesing
Copy link
Member

Thank you for the change - I think using the HKCU\Software\Classes\CLSID is better. Requiring administrator rights to run unittests should be avoided if possible (yes there are tests, that require it, but it should be an exception).

The unittest testRegistryKeyExistsSamExtra fails on appveyor. See: https://ci.appveyor.com/api/buildjobs/s6a0brgcwimsro9b/log (line 1916). Any idea why that fails and why the other checks succeed?

@CamW
Copy link
Contributor Author

CamW commented Aug 26, 2018

In unit test testRegistryKeyExistsSamExtra I tried to use pre-existing registry entries. It's possible that those entries don't always exist. I've changed that test to a create-test-delete style test.
I've also changed the rest of the samExtra tests to use the new HKCU\Software\Classes\CLSID path which means that Advapi32UtilTest no longer requires administrator rights.

@matthiasblaesing
Copy link
Member

Thank you - with the change the unittests run cleanly and also the appveyor build is happy.

@matthiasblaesing matthiasblaesing merged commit 4e8dd7f into java-native-access:master Aug 26, 2018
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.

2 participants