[PATCH] crypt32/tests: Fix a registerOIDInfo() failure when missing elevated privileges.
And only accept the access denied errors when running without elevated privileges. Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- dlls/crypt32/tests/oid.c | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/dlls/crypt32/tests/oid.c b/dlls/crypt32/tests/oid.c index ae03bba90e8..d135092a3ad 100644 --- a/dlls/crypt32/tests/oid.c +++ b/dlls/crypt32/tests/oid.c @@ -30,6 +30,22 @@ #include "wine/test.h" +static BOOL is_process_elevated(void) +{ + HANDLE token; + if (OpenProcessToken( GetCurrentProcess(), TOKEN_QUERY, &token )) + { + TOKEN_ELEVATION_TYPE type; + DWORD size; + BOOL ret; + + ret = GetTokenInformation( token, TokenElevationType, &type, sizeof(type), &size ); + CloseHandle( token ); + return (ret && type == TokenElevationTypeFull); + } + return FALSE; +} + static BOOL (WINAPI *pCryptEnumOIDInfo)(DWORD,DWORD,void*,PFN_CRYPT_ENUM_OID_INFO); @@ -314,7 +330,7 @@ static void test_registerOIDFunction(void) SetLastError(0xdeadbeef); ret = CryptRegisterOIDFunction(X509_ASN_ENCODING, "CryptDllEncodeObject", "1.2.3.4.5.6.7.8.9.10", bogusDll, NULL); - if (!ret && GetLastError() == ERROR_ACCESS_DENIED) + if (!ret && GetLastError() == ERROR_ACCESS_DENIED && !is_process_elevated()) { skip("Need admin rights\n"); return; @@ -393,7 +409,7 @@ static void test_registerDefaultOIDFunction(void) SetLastError(0xdeadbeef); ret = CryptRegisterDefaultOIDFunction(0, "CertDllOpenStoreProv", 0, bogusDll); - if (!ret && GetLastError() == ERROR_ACCESS_DENIED) + if (!ret && GetLastError() == ERROR_ACCESS_DENIED && !is_process_elevated()) { skip("Need admin rights\n"); return; @@ -499,7 +515,7 @@ static void test_getDefaultOIDFunctionAddress(void) SetLastError(0xdeadbeef); ret = CryptRegisterDefaultOIDFunction(0, "CertDllOpenStoreProv", 0, bogusDll); - if (!ret && GetLastError() == ERROR_ACCESS_DENIED) + if (!ret && GetLastError() == ERROR_ACCESS_DENIED && !is_process_elevated()) skip("Need admin rights\n"); else ok(ret, "CryptRegisterDefaultOIDFunction failed: %08x\n", GetLastError()); @@ -645,7 +661,11 @@ static void test_registerOIDInfo(void) info1.pszOID = test_oid; SetLastError(0xdeadbeef); ret = CryptUnregisterOIDInfo(&info1); - ok(!ret, "should fail\n"); + if (!ret && GetLastError() == ERROR_ACCESS_DENIED && !is_process_elevated()) + { + skip("Need admin rights FIXME\n"); + return; + } ok(!ret, "should fail\n"); ok(GetLastError() == ERROR_FILE_NOT_FOUND, "got %u\n", GetLastError()); info2 = CryptFindOIDInfo(CRYPT_OID_INFO_OID_KEY, (void *)test_oid, 0); @@ -668,7 +688,7 @@ static void test_registerOIDInfo(void) info1.dwGroupId = CRYPT_HASH_ALG_OID_GROUP_ID; SetLastError(0xdeadbeef); ret = CryptRegisterOIDInfo(&info1, CRYPT_INSTALL_OID_INFO_BEFORE_FLAG); - if (!ret && GetLastError() == ERROR_ACCESS_DENIED) + if (!ret && GetLastError() == ERROR_ACCESS_DENIED && !is_process_elevated()) { skip("Need admin rights\n"); return; -- 2.20.1
Francois Gouget <fgouget(a)codeweavers.com> wrote:
@@ -314,7 +330,7 @@ static void test_registerOIDFunction(void) SetLastError(0xdeadbeef); ret = CryptRegisterOIDFunction(X509_ASN_ENCODING, "CryptDllEncodeObject", "1.2.3.4.5.6.7.8.9.10", bogusDll, NULL); - if (!ret && GetLastError() == ERROR_ACCESS_DENIED) + if (!ret && GetLastError() == ERROR_ACCESS_DENIED && !is_process_elevated()) { skip("Need admin rights\n"); return; @@ -393,7 +409,7 @@ static void test_registerDefaultOIDFunction(void) SetLastError(0xdeadbeef); ret = CryptRegisterDefaultOIDFunction(0, "CertDllOpenStoreProv", 0, bogusDll); - if (!ret && GetLastError() == ERROR_ACCESS_DENIED) + if (!ret && GetLastError() == ERROR_ACCESS_DENIED && !is_process_elevated()) { skip("Need admin rights\n"); return; @@ -499,7 +515,7 @@ static void test_getDefaultOIDFunctionAddress(void) SetLastError(0xdeadbeef); ret = CryptRegisterDefaultOIDFunction(0, "CertDllOpenStoreProv", 0, bogusDll); - if (!ret && GetLastError() == ERROR_ACCESS_DENIED) + if (!ret && GetLastError() == ERROR_ACCESS_DENIED && !is_process_elevated()) skip("Need admin rights\n"); else ok(ret, "CryptRegisterDefaultOIDFunction failed: %08x\n", GetLastError());
In which cases the changes above are needed? It seems that this only complicates things without a reason and could potentially slow down the tests execution.
@@ -645,7 +661,11 @@ static void test_registerOIDInfo(void) info1.pszOID = test_oid; SetLastError(0xdeadbeef); ret = CryptUnregisterOIDInfo(&info1); - ok(!ret, "should fail\n"); + if (!ret && GetLastError() == ERROR_ACCESS_DENIED && !is_process_elevated()) + { + skip("Need admin rights FIXME\n"); + return; + } ok(!ret, "should fail\n");
This change doesn't look right. Most likely it's not needed here. -- Dmitry.
On Mon, 14 Dec 2020, Dmitry Timoshkov wrote:
Francois Gouget <fgouget(a)codeweavers.com> wrote:
@@ -314,7 +330,7 @@ static void test_registerOIDFunction(void) SetLastError(0xdeadbeef); ret = CryptRegisterOIDFunction(X509_ASN_ENCODING, "CryptDllEncodeObject", "1.2.3.4.5.6.7.8.9.10", bogusDll, NULL); - if (!ret && GetLastError() == ERROR_ACCESS_DENIED) + if (!ret && GetLastError() == ERROR_ACCESS_DENIED && !is_process_elevated()) { skip("Need admin rights\n"); return; @@ -393,7 +409,7 @@ static void test_registerDefaultOIDFunction(void) SetLastError(0xdeadbeef); ret = CryptRegisterDefaultOIDFunction(0, "CertDllOpenStoreProv", 0, bogusDll); - if (!ret && GetLastError() == ERROR_ACCESS_DENIED) + if (!ret && GetLastError() == ERROR_ACCESS_DENIED && !is_process_elevated()) { skip("Need admin rights\n"); return; @@ -499,7 +515,7 @@ static void test_getDefaultOIDFunctionAddress(void) SetLastError(0xdeadbeef); ret = CryptRegisterDefaultOIDFunction(0, "CertDllOpenStoreProv", 0, bogusDll); - if (!ret && GetLastError() == ERROR_ACCESS_DENIED) + if (!ret && GetLastError() == ERROR_ACCESS_DENIED && !is_process_elevated()) skip("Need admin rights\n"); else ok(ret, "CryptRegisterDefaultOIDFunction failed: %08x\n", GetLastError());
In which cases the changes above are needed? It seems that this only complicates things without a reason and could potentially slow down the tests execution.
These are not strictly needed. Rather the goal is to verify that we only get ERROR_ACCESS_DENIED when missing elevated privileges rather than blindly assuming the access denied error is justified.
@@ -645,7 +661,11 @@ static void test_registerOIDInfo(void) info1.pszOID = test_oid; SetLastError(0xdeadbeef); ret = CryptUnregisterOIDInfo(&info1); - ok(!ret, "should fail\n"); + if (!ret && GetLastError() == ERROR_ACCESS_DENIED && !is_process_elevated()) + { + skip("Need admin rights FIXME\n"); + return; + } ok(!ret, "should fail\n");
This change doesn't look right. Most likely it's not needed here.
It is needed: https://test.winehq.org/data/3acb0b3326c4120ea0c4c6076bd03c9cfe82c744/win10_... -- Francois Gouget <fgouget(a)codeweavers.com>
Francois Gouget <fgouget(a)codeweavers.com> wrote:
@@ -499,7 +515,7 @@ static void test_getDefaultOIDFunctionAddress(void) SetLastError(0xdeadbeef); ret = CryptRegisterDefaultOIDFunction(0, "CertDllOpenStoreProv", 0, bogusDll); - if (!ret && GetLastError() == ERROR_ACCESS_DENIED) + if (!ret && GetLastError() == ERROR_ACCESS_DENIED && !is_process_elevated()) skip("Need admin rights\n"); else ok(ret, "CryptRegisterDefaultOIDFunction failed: %08x\n", GetLastError());
In which cases the changes above are needed? It seems that this only complicates things without a reason and could potentially slow down the tests execution.
These are not strictly needed. Rather the goal is to verify that we only get ERROR_ACCESS_DENIED when missing elevated privileges rather than blindly assuming the access denied error is justified.
If the intent to add is_process_elevated() checks to every place where ERROR_ACCESS_DENIED error returned then it's not justified.
@@ -645,7 +661,11 @@ static void test_registerOIDInfo(void) info1.pszOID = test_oid; SetLastError(0xdeadbeef); ret = CryptUnregisterOIDInfo(&info1); - ok(!ret, "should fail\n"); + if (!ret && GetLastError() == ERROR_ACCESS_DENIED && !is_process_elevated()) + { + skip("Need admin rights FIXME\n"); + return; + } ok(!ret, "should fail\n");
This change doesn't look right. Most likely it's not needed here.
It is needed: https://test.winehq.org/data/3acb0b3326c4120ea0c4c6076bd03c9cfe82c744/win10_...
In addition to broken indentation and strange skip message the test bot results seem not related, in both cases !ret is the only accepted value. -- Dmitry.
On Mon, 14 Dec 2020, Dmitry Timoshkov wrote: [...]
If the intent to add is_process_elevated() checks to every place where ERROR_ACCESS_DENIED error returned then it's not justified.
Ok.
@@ -645,7 +661,11 @@ static void test_registerOIDInfo(void) info1.pszOID = test_oid; SetLastError(0xdeadbeef); ret = CryptUnregisterOIDInfo(&info1); - ok(!ret, "should fail\n"); + if (!ret && GetLastError() == ERROR_ACCESS_DENIED && !is_process_elevated()) + { + skip("Need admin rights FIXME\n"); + return; + } ok(!ret, "should fail\n");
This change doesn't look right. Most likely it's not needed here.
It is needed: https://test.winehq.org/data/3acb0b3326c4120ea0c4c6076bd03c9cfe82c744/win10_...
In addition to broken indentation and strange skip message the test bot results seem not related, in both cases !ret is the only accepted value.
Oh, right. I missed the cleanup step and mixed it up with the fix for another test too. I will resubmit it. -- Francois Gouget <fgouget(a)codeweavers.com>
participants (2)
-
Dmitry Timoshkov -
Francois Gouget