[PATCH 0/1] MR1987: Draft: webservices/tests: Move firewall code to a common header.

[Hans Leidekker (＠hans)]

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/1987

[PATCH 1/1] webservices/tests: Move firewall code to a common header.

[Hans Leidekker]

From: Hans Leidekker <hans(a)codeweavers.com>

---
 dlls/webservices/tests/channel.c | 139 ++-----------------------------
 include/Makefile.in              |   1 +
 include/wine/test_fw.h           | 131 +++++++++++++++++++++++++++++
 3 files changed, 137 insertions(+), 134 deletions(-)
 create mode 100644 include/wine/test_fw.h

diff --git a/dlls/webservices/tests/channel.c b/dlls/webservices/tests/channel.c
index 7e0a9becbc5..7e9587481a2 100644
--- a/dlls/webservices/tests/channel.c
+++ b/dlls/webservices/tests/channel.c
@@ -24,6 +24,7 @@
 #include "initguid.h"
 #include "netfw.h"
 #include "wine/test.h"
+#include "wine/test_fw.h"
 
 static void test_WsCreateChannel(void)
 {
@@ -1410,132 +1411,10 @@ static HANDLE start_listener( struct listener_info *info )
     return thread;
 }
 
-enum firewall_op
-{
-    APP_ADD,
-    APP_REMOVE
-};
-
-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 is_firewall_enabled(void)
-{
-    HRESULT hr, init;
-    INetFwMgr *mgr = NULL;
-    INetFwPolicy *policy = NULL;
-    INetFwProfile *profile = NULL;
-    VARIANT_BOOL enabled = VARIANT_FALSE;
-
-    init = CoInitializeEx( 0, COINIT_APARTMENTTHREADED );
-
-    hr = CoCreateInstance( &CLSID_NetFwMgr, NULL, CLSCTX_INPROC_SERVER, &IID_INetFwMgr,
-                           (void **)&mgr );
-    ok( hr == S_OK, "got %#lx\n", hr );
-    if (hr != S_OK) goto done;
-
-    hr = INetFwMgr_get_LocalPolicy( mgr, &policy );
-    ok( hr == S_OK, "got %#lx\n", hr );
-    if (hr != S_OK) goto done;
-
-    hr = INetFwPolicy_get_CurrentProfile( policy, &profile );
-    if (hr != S_OK) goto done;
-
-    hr = INetFwProfile_get_FirewallEnabled( profile, &enabled );
-    ok( hr == S_OK, "got %#lx\n", hr );
-
-done:
-    if (policy) INetFwPolicy_Release( policy );
-    if (profile) INetFwProfile_Release( profile );
-    if (mgr) INetFwMgr_Release( mgr );
-    if (SUCCEEDED( init )) CoUninitialize();
-    return (enabled == VARIANT_TRUE);
-}
-
-static HRESULT set_firewall( enum firewall_op op )
-{
-    HRESULT hr, init;
-    INetFwMgr *mgr = NULL;
-    INetFwPolicy *policy = NULL;
-    INetFwProfile *profile = NULL;
-    INetFwAuthorizedApplication *app = NULL;
-    INetFwAuthorizedApplications *apps = NULL;
-    BSTR name, image = SysAllocStringLen( NULL, MAX_PATH );
-
-    if (!GetModuleFileNameW( NULL, image, MAX_PATH ))
-    {
-        SysFreeString( image );
-        return E_FAIL;
-    }
-    init = CoInitializeEx( 0, COINIT_APARTMENTTHREADED );
-
-    hr = CoCreateInstance( &CLSID_NetFwMgr, NULL, CLSCTX_INPROC_SERVER, &IID_INetFwMgr,
-                           (void **)&mgr );
-    ok( hr == S_OK, "got %#lx\n", hr );
-    if (hr != S_OK) goto done;
-
-    hr = INetFwMgr_get_LocalPolicy( mgr, &policy );
-    ok( hr == S_OK, "got %#lx\n", hr );
-    if (hr != S_OK) goto done;
-
-    hr = INetFwPolicy_get_CurrentProfile( policy, &profile );
-    if (hr != S_OK) goto done;
-
-    hr = INetFwProfile_get_AuthorizedApplications( profile, &apps );
-    ok( hr == S_OK, "got %#lx\n", hr );
-    if (hr != S_OK) goto done;
-
-    hr = CoCreateInstance( &CLSID_NetFwAuthorizedApplication, NULL, CLSCTX_INPROC_SERVER,
-                           &IID_INetFwAuthorizedApplication, (void **)&app );
-    ok( hr == S_OK, "got %#lx\n", hr );
-    if (hr != S_OK) goto done;
-
-    hr = INetFwAuthorizedApplication_put_ProcessImageFileName( app, image );
-    if (hr != S_OK) goto done;
-
-    name = SysAllocString( L"webservices_test" );
-    hr = INetFwAuthorizedApplication_put_Name( app, name );
-    SysFreeString( name );
-    ok( hr == S_OK, "got %#lx\n", hr );
-    if (hr != S_OK) goto done;
-
-    if (op == APP_ADD)
-        hr = INetFwAuthorizedApplications_Add( apps, app );
-    else if (op == APP_REMOVE)
-        hr = INetFwAuthorizedApplications_Remove( apps, image );
-    else
-        hr = E_INVALIDARG;
-
-done:
-    if (app) INetFwAuthorizedApplication_Release( app );
-    if (apps) INetFwAuthorizedApplications_Release( apps );
-    if (policy) INetFwPolicy_Release( policy );
-    if (profile) INetFwProfile_Release( profile );
-    if (mgr) INetFwMgr_Release( mgr );
-    if (SUCCEEDED( init )) CoUninitialize();
-    SysFreeString( image );
-    return hr;
-}
-
 START_TEST(channel)
 {
-    BOOL firewall_enabled = is_firewall_enabled();
     struct listener_info info;
     HANDLE thread;
-    HRESULT hr;
     unsigned int i;
     static const struct test
     {
@@ -1558,18 +1437,10 @@ START_TEST(channel)
         { WS_TCP_CHANNEL_BINDING, WS_CHANNEL_TYPE_DUPLEX_SESSION, NULL, client_duplex_session_dict, listener_socket_proc, 4096 },
     };
 
-    if (firewall_enabled)
+    if (!winetest_set_firewall( L"webservices_test", WINETEST_FW_APP_ADD ))
     {
-        if (!is_process_elevated())
-        {
-            skip( "no privileges, skipping tests to avoid firewall dialog\n" );
-            return;
-        }
-        if ((hr = set_firewall( APP_ADD )) != S_OK)
-        {
-            skip( "can't authorize app in firewall %#lx\n", hr );
-            return;
-        }
+        skip( "can't authorize app in firewall\n" );
+        return;
     }
 
     test_WsCreateChannel();
@@ -1598,5 +1469,5 @@ START_TEST(channel)
         CloseHandle( thread );
     }
 
-    if (firewall_enabled) set_firewall( APP_REMOVE );
+    winetest_set_firewall( L"webservices_test", WINETEST_FW_APP_REMOVE );
 }
diff --git a/include/Makefile.in b/include/Makefile.in
index 54cbf4d955c..0aa8b5278e1 100644
--- a/include/Makefile.in
+++ b/include/Makefile.in
@@ -857,6 +857,7 @@ SOURCES = \
 	wine/strmbase.h \
 	wine/svcctl.idl \
 	wine/test.h \
+	wine/test_fw.h \
 	wine/unixlib.h \
 	wine/vulkan.h \
 	wine/vulkan_driver.h \
diff --git a/include/wine/test_fw.h b/include/wine/test_fw.h
new file mode 100644
index 00000000000..f9dbf60be3f
--- /dev/null
+++ b/include/wine/test_fw.h
@@ -0,0 +1,131 @@
+/*
+ * Copyright 2023 Hans Leidekker for CodeWeavers
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
+ */
+
+#include "windef.h"
+#include "winbase.h"
+#include "netfw.h"
+
+static BOOL winetest_is_process_elevated(void)
+{
+    HANDLE token;
+    BOOL ret;
+    TOKEN_ELEVATION_TYPE type;
+    DWORD size;
+
+    if (!OpenProcessToken( GetCurrentProcess(), TOKEN_QUERY, &token )) return FALSE;
+    ret = GetTokenInformation( token, TokenElevationType, &type, sizeof(type), &size );
+    CloseHandle( token );
+    return (ret && type == TokenElevationTypeFull);
+}
+
+static BOOL winetest_is_firewall_enabled(void)
+{
+    HRESULT hr, init;
+    INetFwMgr *mgr = NULL;
+    INetFwPolicy *policy = NULL;
+    INetFwProfile *profile = NULL;
+    VARIANT_BOOL enabled = VARIANT_FALSE;
+
+    init = CoInitializeEx( 0, COINIT_APARTMENTTHREADED );
+
+    hr = CoCreateInstance( &CLSID_NetFwMgr, NULL, CLSCTX_INPROC_SERVER, &IID_INetFwMgr, (void **)&mgr );
+    if (hr != S_OK) goto done;
+
+    hr = INetFwMgr_get_LocalPolicy( mgr, &policy );
+    if (hr != S_OK) goto done;
+
+    hr = INetFwPolicy_get_CurrentProfile( policy, &profile );
+    if (hr != S_OK) goto done;
+
+    hr = INetFwProfile_get_FirewallEnabled( profile, &enabled );
+
+done:
+    if (policy) INetFwPolicy_Release( policy );
+    if (profile) INetFwProfile_Release( profile );
+    if (mgr) INetFwMgr_Release( mgr );
+    if (SUCCEEDED( init )) CoUninitialize();
+    return (enabled == VARIANT_TRUE);
+}
+
+enum winetest_firewall_op
+{
+    WINETEST_FW_APP_ADD,
+    WINETEST_FW_APP_REMOVE
+};
+
+static BOOL winetest_set_firewall( const WCHAR *name, enum winetest_firewall_op op )
+{
+    HRESULT hr, init;
+    INetFwMgr *mgr = NULL;
+    INetFwPolicy *policy = NULL;
+    INetFwProfile *profile = NULL;
+    INetFwAuthorizedApplication *app = NULL;
+    INetFwAuthorizedApplications *apps = NULL;
+    BSTR str, image = SysAllocStringLen( NULL, MAX_PATH );
+
+    if (!winetest_is_process_elevated()) return FALSE;
+    if (!winetest_is_firewall_enabled()) return TRUE;
+
+    if (!GetModuleFileNameW( NULL, image, MAX_PATH ))
+    {
+        SysFreeString( image );
+        return E_FAIL;
+    }
+    init = CoInitializeEx( 0, COINIT_APARTMENTTHREADED );
+
+    hr = CoCreateInstance( &CLSID_NetFwMgr, NULL, CLSCTX_INPROC_SERVER, &IID_INetFwMgr, (void **)&mgr );
+    if (hr != S_OK) goto done;
+
+    hr = INetFwMgr_get_LocalPolicy( mgr, &policy );
+    if (hr != S_OK) goto done;
+
+    hr = INetFwPolicy_get_CurrentProfile( policy, &profile );
+    if (hr != S_OK) goto done;
+
+    hr = INetFwProfile_get_AuthorizedApplications( profile, &apps );
+    if (hr != S_OK) goto done;
+
+    hr = CoCreateInstance( &CLSID_NetFwAuthorizedApplication, NULL, CLSCTX_INPROC_SERVER,
+                           &IID_INetFwAuthorizedApplication, (void **)&app );
+    if (hr != S_OK) goto done;
+
+    hr = INetFwAuthorizedApplication_put_ProcessImageFileName( app, image );
+    if (hr != S_OK) goto done;
+
+    str = SysAllocString( name );
+    hr = INetFwAuthorizedApplication_put_Name( app, str );
+    SysFreeString( str );
+    if (hr != S_OK) goto done;
+
+    if (op == WINETEST_FW_APP_ADD)
+        hr = INetFwAuthorizedApplications_Add( apps, app );
+    else if (op == WINETEST_FW_APP_REMOVE)
+        hr = INetFwAuthorizedApplications_Remove( apps, image );
+    else
+        hr = E_INVALIDARG;
+
+done:
+    if (app) INetFwAuthorizedApplication_Release( app );
+    if (apps) INetFwAuthorizedApplications_Release( apps );
+    if (policy) INetFwPolicy_Release( policy );
+    if (profile) INetFwProfile_Release( profile );
+    if (mgr) INetFwMgr_Release( mgr );
+    if (SUCCEEDED( init )) CoUninitialize();
+    SysFreeString( image );
+    return (hr == S_OK);
+}
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/1987

[Jinoh Kang (＠iamahuman)]

Jinoh Kang (@iamahuman) commented about include/wine/test_fw.h:
> +    WINETEST_FW_APP_ADD,
> +    WINETEST_FW_APP_REMOVE
> +};
> +
> +static BOOL winetest_set_firewall( const WCHAR *name, enum winetest_firewall_op op )
> +{
> +    HRESULT hr, init;
> +    INetFwMgr *mgr = NULL;
> +    INetFwPolicy *policy = NULL;
> +    INetFwProfile *profile = NULL;
> +    INetFwAuthorizedApplication *app = NULL;
> +    INetFwAuthorizedApplications *apps = NULL;
> +    BSTR str, image = SysAllocStringLen( NULL, MAX_PATH );
> +
> +    if (!winetest_is_process_elevated()) return FALSE;
> +    if (!winetest_is_firewall_enabled()) return TRUE;
> The order of tests are swapped.

```suggestion:-1+0
    if (!winetest_is_firewall_enabled()) return TRUE;
    if (!winetest_is_process_elevated()) return FALSE;
```

That said, is the process elevation check still needed? If accessing NetFw APIs causes Windows to show a warning dialog or has other undesirable side effects, that should certainly be avoided. Otherwise, we could just use the "ask for forgiveness, not permission" approach, since we no longer have `ok()` tests for calls anymore.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/1987#note_21212

[Hans Leidekker (＠hans)]

On Thu Jan 19 15:36:40 2023 +0000, Jinoh Kang wrote:
> The order of tests are swapped.
> ```suggestion:-1+0
>     if (!winetest_is_firewall_enabled()) return TRUE;
>     if (!winetest_is_process_elevated()) return FALSE;
> ```
> That said, is the process elevation check still needed? If accessing
> NetFw APIs causes Windows to show a warning dialog or has other
> undesirable side effects, that should certainly be avoided. Otherwise,
> we could just use the "ask for forgiveness, not permission" approach,
> since we no longer have `ok()` tests for calls anymore.
> Thanks, I'll fix the order. Without the elevation check the firewall dialog pops up.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/1987#note_21216