[PATCH] libnm-glib/libnm/vpn: fix handling of ConnectInteractive() failure (rh #1298732)

[Dan Williams <dcbw redhat com>]

If the plugin supports interactive mode, but the VPN binary (like vpnc
or openvpn) doesn't support it, then the plugin should return
NM_VPN_PLUGIN_ERROR_INTERACTIVE_NOT_SUPPORTED from its connect_interactive()
hook.  This lets NetworkManager know to fall back to plain Connect().

Since this notification is done through an error return, the VPN service
plugin code sees the failure and moves the plugin state back to
STOPPED.  NetworkManager sees that state change, and terminates the
connection attempt while waiting for a reply to the Connect() method.

(VPN service plugins that don't support interactive mode at all don't
have this problem because that error is returned before the plugin's
state is moved to STARTING.)

To fix this, do two things:

1) if the connect_interactive() hook fails and returns the error
NM_VPN_PLUGIN_ERROR_INTERACTIVE_NOT_SUPPORTED, postpone the STOPPED
state change for a few seconds to allow NM time to fall back to
plain Connect().  We still want to move the plugin state back to
STOPPED eventually, because otherwise it could stay in STARTING
forever.

2) change state to STARTING only if the connect/connect_interactive
plugin hooks were successful.  Otherwise the plugin would still be
in STARTING state, and it's not valid to call Connect()/ConnectInteractive()
during the STARTING state.

https://bugzilla.redhat.com/show_bug.cgi?id=1298732
---
 libnm-glib/nm-vpn-plugin.c    | 24 +++++++++++++++++-------
 libnm/nm-vpn-plugin-old.c     | 20 +++++++++++++++-----
 libnm/nm-vpn-service-plugin.c | 20 +++++++++++++++-----
 3 files changed, 47 insertions(+), 17 deletions(-)

diff --git a/libnm-glib/nm-vpn-plugin.c b/libnm-glib/nm-vpn-plugin.c
index 67ddd83..9f941fc 100644
--- a/libnm-glib/nm-vpn-plugin.c
+++ b/libnm-glib/nm-vpn-plugin.c
@@ -301,12 +301,15 @@ fail_stop (gpointer data)
 }
 
 static void
-schedule_fail_stop (NMVPNPlugin *plugin)
+schedule_fail_stop (NMVPNPlugin *plugin, guint timeout_secs)
 {
        NMVPNPluginPrivate *priv = NM_VPN_PLUGIN_GET_PRIVATE (plugin);
 
        nm_clear_g_source (&priv->fail_stop_id);
-       priv->fail_stop_id = g_idle_add (fail_stop, plugin);
+       if (timeout_secs)
+               priv->fail_stop_id = g_timeout_add_seconds (timeout_secs, fail_stop, plugin);
+       else
+               priv->fail_stop_id = g_idle_add (fail_stop, plugin);
 }
 
 static void
@@ -439,6 +442,7 @@ _connect_generic (NMVPNPlugin *plugin,
        NMConnection *connection;
        gboolean success = FALSE;
        GError *local = NULL;
+       guint fail_stop_timeout = 0;
 
        if (priv->state != NM_VPN_SERVICE_STATE_STOPPED &&
            priv->state != NM_VPN_SERVICE_STATE_INIT) {
@@ -457,7 +461,6 @@ _connect_generic (NMVPNPlugin *plugin,
                return FALSE;
        }
 
-
        priv->interactive = FALSE;
        if (details && !vpn_class->connect_interactive) {
                g_set_error_literal (error, NM_VPN_PLUGIN_ERROR, 
NM_VPN_PLUGIN_ERROR_INTERACTIVE_NOT_SUPPORTED,
@@ -465,22 +468,29 @@ _connect_generic (NMVPNPlugin *plugin,
                return FALSE;
        }
 
-       nm_vpn_plugin_set_state (plugin, NM_VPN_SERVICE_STATE_STARTING);
+       nm_clear_g_source (&priv->fail_stop_id);
 
        if (details) {
                priv->interactive = TRUE;
-               success = vpn_class->connect_interactive (plugin, connection, details, error);
+               success = vpn_class->connect_interactive (plugin, connection, details, &local);
+               if (g_error_matches (local, NM_VPN_PLUGIN_ERROR, 
NM_VPN_PLUGIN_ERROR_INTERACTIVE_NOT_SUPPORTED)) {
+                       /* Give NetworkManager a bit of time to fall back to Connect() */
+                       fail_stop_timeout = 5;
+               }
+               g_propagate_error (error, local);
        } else
                success = vpn_class->connect (plugin, connection, error);
 
        if (success) {
+               nm_vpn_plugin_set_state (plugin, NM_VPN_SERVICE_STATE_STARTING);
+
                /* Add a timer to make sure we do not wait indefinitely for the successful connect. */
                connect_timer_start (plugin);
        } else {
                /* Stop the plugin from an idle handler so that the Connect
                 * method return gets sent before the STOP StateChanged signal.
                 */
-               schedule_fail_stop (plugin);
+               schedule_fail_stop (plugin, fail_stop_timeout);
        }
 
        g_object_unref (connection);
@@ -606,7 +616,7 @@ impl_vpn_plugin_new_secrets (NMVPNPlugin *plugin,
                /* Stop the plugin from and idle handler so that the NewSecrets
                 * method return gets sent before the STOP StateChanged signal.
                 */
-               schedule_fail_stop (plugin);
+               schedule_fail_stop (plugin, 0);
        }
 
        g_object_unref (connection);
diff --git a/libnm/nm-vpn-plugin-old.c b/libnm/nm-vpn-plugin-old.c
index 9bbac41..19f1417 100644
--- a/libnm/nm-vpn-plugin-old.c
+++ b/libnm/nm-vpn-plugin-old.c
@@ -293,12 +293,15 @@ fail_stop (gpointer data)
 }
 
 static void
-schedule_fail_stop (NMVpnPluginOld *plugin)
+schedule_fail_stop (NMVpnPluginOld *plugin, guint timeout_secs)
 {
        NMVpnPluginOldPrivate *priv = NM_VPN_PLUGIN_OLD_GET_PRIVATE (plugin);
 
        nm_clear_g_source (&priv->fail_stop_id);
-       priv->fail_stop_id = g_idle_add (fail_stop, plugin);
+       if (timeout_secs)
+               priv->fail_stop_id = g_timeout_add_seconds (timeout_secs, fail_stop, plugin);
+       else
+               priv->fail_stop_id = g_idle_add (fail_stop, plugin);
 }
 
 /**
@@ -450,6 +453,7 @@ _connect_generic (NMVpnPluginOld *plugin,
        NMConnection *connection;
        gboolean success = FALSE;
        GError *error = NULL;
+       guint fail_stop_timeout = 0;
 
        if (priv->state != NM_VPN_SERVICE_STATE_STOPPED &&
            priv->state != NM_VPN_SERVICE_STATE_INIT) {
@@ -480,15 +484,21 @@ _connect_generic (NMVpnPluginOld *plugin,
                return;
        }
 
-       nm_vpn_plugin_old_set_state (plugin, NM_VPN_SERVICE_STATE_STARTING);
+       nm_clear_g_source (&priv->fail_stop_id);
 
        if (details) {
                priv->interactive = TRUE;
                success = vpn_class->connect_interactive (plugin, connection, details, &error);
+               if (g_error_matches (error, NM_VPN_PLUGIN_ERROR, 
NM_VPN_PLUGIN_ERROR_INTERACTIVE_NOT_SUPPORTED)) {
+                       /* Give NetworkManager a bit of time to fall back to Connect() */
+                       fail_stop_timeout = 5;
+               }
        } else
                success = vpn_class->connect (plugin, connection, &error);
 
        if (success) {
+               nm_vpn_plugin_old_set_state (plugin, NM_VPN_SERVICE_STATE_STARTING);
+
                g_dbus_method_invocation_return_value (context, NULL);
 
                /* Add a timer to make sure we do not wait indefinitely for the successful connect. */
@@ -499,7 +509,7 @@ _connect_generic (NMVpnPluginOld *plugin,
                /* Stop the plugin from an idle handler so that the Connect
                 * method return gets sent before the STOP StateChanged signal.
                 */
-               schedule_fail_stop (plugin);
+               schedule_fail_stop (plugin, fail_stop_timeout);
        }
 
        g_object_unref (connection);
@@ -628,7 +638,7 @@ impl_vpn_plugin_old_new_secrets (NMVpnPluginOld *plugin,
                /* Stop the plugin from and idle handler so that the NewSecrets
                 * method return gets sent before the STOP StateChanged signal.
                 */
-               schedule_fail_stop (plugin);
+               schedule_fail_stop (plugin, 0);
        }
 
        g_object_unref (connection);
diff --git a/libnm/nm-vpn-service-plugin.c b/libnm/nm-vpn-service-plugin.c
index 4e301e9..5fbd359 100644
--- a/libnm/nm-vpn-service-plugin.c
+++ b/libnm/nm-vpn-service-plugin.c
@@ -291,12 +291,15 @@ fail_stop (gpointer data)
 }
 
 static void
-schedule_fail_stop (NMVpnServicePlugin *plugin)
+schedule_fail_stop (NMVpnServicePlugin *plugin, guint timeout_secs)
 {
        NMVpnServicePluginPrivate *priv = NM_VPN_SERVICE_PLUGIN_GET_PRIVATE (plugin);
 
        nm_clear_g_source (&priv->fail_stop_id);
-       priv->fail_stop_id = g_idle_add (fail_stop, plugin);
+       if (timeout_secs)
+               priv->fail_stop_id = g_timeout_add_seconds (timeout_secs, fail_stop, plugin);
+       else
+               priv->fail_stop_id = g_idle_add (fail_stop, plugin);
 }
 
 void
@@ -466,6 +469,7 @@ _connect_generic (NMVpnServicePlugin *plugin,
        NMConnection *connection;
        gboolean success = FALSE;
        GError *error = NULL;
+       guint fail_stop_timeout = 0;
 
        if (priv->state != NM_VPN_SERVICE_STATE_STOPPED &&
            priv->state != NM_VPN_SERVICE_STATE_INIT) {
@@ -497,7 +501,7 @@ _connect_generic (NMVpnServicePlugin *plugin,
                return;
        }
 
-       nm_vpn_service_plugin_set_state (plugin, NM_VPN_SERVICE_STATE_STARTING);
+       nm_clear_g_source (&priv->fail_stop_id);
 
        if (priv->dbus_watch_peer)
                priv->peer_watch_id = watch_peer (plugin, context);
@@ -505,10 +509,16 @@ _connect_generic (NMVpnServicePlugin *plugin,
        if (details) {
                priv->interactive = TRUE;
                success = vpn_class->connect_interactive (plugin, connection, details, &error);
+               if (g_error_matches (error, NM_VPN_PLUGIN_ERROR, 
NM_VPN_PLUGIN_ERROR_INTERACTIVE_NOT_SUPPORTED)) {
+                       /* Give NetworkManager a bit of time to fall back to Connect() */
+                       fail_stop_timeout = 5;
+               }
        } else
                success = vpn_class->connect (plugin, connection, &error);
 
        if (success) {
+               nm_vpn_service_plugin_set_state (plugin, NM_VPN_SERVICE_STATE_STARTING);
+
                g_dbus_method_invocation_return_value (context, NULL);
 
                /* Add a timer to make sure we do not wait indefinitely for the successful connect. */
@@ -519,7 +529,7 @@ _connect_generic (NMVpnServicePlugin *plugin,
                /* Stop the plugin from an idle handler so that the Connect
                 * method return gets sent before the STOP StateChanged signal.
                 */
-               schedule_fail_stop (plugin);
+               schedule_fail_stop (plugin, fail_stop_timeout);
        }
 
        g_object_unref (connection);
@@ -648,7 +658,7 @@ impl_vpn_service_plugin_new_secrets (NMVpnServicePlugin *plugin,
                /* Stop the plugin from and idle handler so that the NewSecrets
                 * method return gets sent before the STOP StateChanged signal.
                 */
-               schedule_fail_stop (plugin);
+               schedule_fail_stop (plugin, 0);
        }
 
        g_object_unref (connection);
-- 
2.5.0