[PATCH 0/1] MR4052: secur32/tests: Skip the tests instead of crashing if Kerberos is not supported.

[Francois Gouget (＠fgouget)]

Wine-Bug: https://bugs.winehq.org//show_bug.cgi?id=55555

[PATCH 1/1] secur32/tests: Skip the tests instead of crashing if Kerberos is not supported.

[Francois Gouget]

From: Francois Gouget

Wine-Bug: https://bugs.winehq.org//show_bug.cgi?id=55555
---
 dlls/secur32/tests/secur32.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/dlls/secur32/tests/secur32.c b/dlls/secur32/tests/secur32.c
index b1a1db76c3e..db81e23bd7e 100644
--- a/dlls/secur32/tests/secur32.c
+++ b/dlls/secur32/tests/secur32.c
@@ -56,6 +56,8 @@ static BOOLEAN (WINAPI * pGetUserNameExW)(EXTENDED_NAME_FORMAT NameFormat, LPWST
 static PSecurityFunctionTableA (SEC_ENTRY * pInitSecurityInterfaceA)(void);
 static PSecurityFunctionTableW (SEC_ENTRY * pInitSecurityInterfaceW)(void);

+static BOOLEAN has_kerberos;
+
 static EXTENDED_NAME_FORMAT formats[] = {
     NameUnknown, NameFullyQualifiedDN, NameSamCompatible, NameDisplay,
     NameUniqueId, NameCanonical, NameUserPrincipal, NameCanonicalEx,
@@ -446,9 +448,13 @@ static void test_kerberos(void)

status = QuerySecurityPackageInfoA(provider, &info);
-    ok(status == SEC_E_OK, "Kerberos package not installed, skipping test\n");
+    if(status == SEC_E_SECPKG_NOT_FOUND)
+        skip("Skipping the Kerberos tests (package is not installed).\n");
+    else
+        ok(status == SEC_E_OK, "Failed to query the Kerberos package: %lx\n", status);
     if(status != SEC_E_OK)
         return;
+    has_kerberos = TRUE;

ok( (info->fCapabilities & ~optional_mask) == expected_flags, "got %08lx, expected %08lx\n", info->fCapabilities, expected_flags );
     ok( info->wVersion == 1, "got %u\n", info->wVersion );
@@ -479,7 +485,15 @@ static void test_ticket_cache(void)

RtlInitAnsiString( &name, MICROSOFT_KERBEROS_NAME_A );
     status = LsaLookupAuthenticationPackage( lsa, &name, &package );
-    ok( !status, "got %08lx\n", status );
+    if (has_kerberos)
+        ok( !status, "got %08lx\n", status );
+    else
+    {
+        ok( status, "expected Kerberos lookup to fail\n" );
+        skip("Skipping the ticket cache tests (no Kerberos: %lx).\n", status);
+        LsaDeregisterLogonProcess( lsa );
+        return;
+    }

status = LsaCallAuthenticationPackage( lsa, package, &req, sizeof(req), (void **)&resp, &len, &status );
     ok( !status, "got %08lx\n", status );

[PATCH 1/1] secur32/tests: Skip the tests instead of crashing if Kerberos is not supported.

[Dmitry Timoshkov]

Francois Gouget  wrote:
> +static BOOLEAN has_kerberos;

Please use BOOL instead.
>  status = QuerySecurityPackageInfoA(provider, &info);
> - ok(status == SEC_E_OK, "Kerberos package not installed, skipping test\n");
> -
>
> if(status == SEC_E_SECPKG_NOT_FOUND)
> -
>    skip("Skipping the Kerberos tests (package is not installed).\n");
> -
>
> else
> -
>    ok(status == SEC_E_OK, "Failed to query the Kerberos package: %lx\n", status);
>
> if(status != SEC_E_OK)
>        return;
> -
>
> has_kerberos = TRUE;
>
> ok( (info->fCapabilities & ~optional_mask) == expected_flags, "got %08lx, expected %08lx\n", info->fCapabilities, expected_flags );
> ok( info->wVersion == 1, "got %u\n", info->wVersion );
>
> @@ -479,7 +485,15 @@ static void test_ticket_cache(void)
>  RtlInitAnsiString( &name, MICROSOFT_KERBEROS_NAME_A );
>  status = LsaLookupAuthenticationPackage( lsa, &name, &package );
> - ok( !status, "got %08lx\n", status );
> -
>
> if (has_kerberos)
> -
>    ok( !status, "got %08lx\n", status );
> -
>
> else
> -
>
> {
> -
>    ok( status, "expected Kerberos lookup to fail\n" );
> -
>    skip("Skipping the ticket cache tests (no Kerberos: %lx).\n", status);
> -
>    LsaDeregisterLogonProcess( lsa );
> -
>    return;
> -
>
> }
>
> status = LsaCallAuthenticationPackage( lsa, package, &req, sizeof(req), (void **)&resp, &len, &status );
> ok( !status, "got %08lx\n", status );

Missing Kerberos makes the installation broken and not supported. Crashing
in this situation is correct behaviour. It's better to spend time on fixing
broken configuration instead of hiding the problem.

[Hans Leidekker (＠hans)]

The tests don't crash anymore according to the patterns page.

[Francois Gouget (＠fgouget)]

None of the test failures are related to this MR:
* amstream:amstream -> bug 55725
* msxml3:domdoc -> bug 55720
* psapi:psapi_main -> bug 55744
* quartz:filtergraph -> bug 55758
* tasklist.exe:tasklist -> bug 55745
* wscript.exe:run -> bug 55746

[Hans Leidekker (＠hans)]

And I think the test should fail if the package is not available, like we do in the NTLM test.

[Francois Gouget (＠fgouget)]

On Wed Oct 11 08:28:43 2023 +0000, Hans Leidekker wrote:
> The tests don't crash anymore according to the patterns page.

Right. I wanted to check out the remaining failures on macOS but I don't have a Mac. So I compiled Wine using --without-krb5 instead and I got a crash on line 486 because LsaCallAuthenticationPackage() failed and set resp to NULL.

So the failures on Remi's macOS machines is different but I think this crash is worth fixing anyway.

So you would prefer something more like test_kerberos(): an ok() that says skip and a return?

[Hans Leidekker (＠hans)]

On Wed Oct 11 08:28:43 2023 +0000, Francois Gouget wrote:
> Right. I wanted to check out the remaining failures on macOS but I don't
> have a Mac. So I compiled Wine using --without-krb5 instead and I got a
> crash on line 486 because LsaCallAuthenticationPackage() failed and set
> resp to NULL.
> So the failures on Remi's macOS machines is different but I think this
> crash is worth fixing anyway.
> So you would prefer something more like test_kerberos(): an ok() that
> says skip and a return?

Yes.