Created attachment 195914 [details] [review]
network-agent: Fix exception for unhandled connections

The calling code expects _getContent() to always return a valid
object, however this is not true for unhandled requests.
Bail out early in that case.

It looks like we don't handle VPN connections currently - we should. In the meantime, this patch should fix the exception.

Created attachment 195915 [details] [review]
network-agent: Handle VPN passwords

Add 'vpn' to the list of connection types handled by shell's
network agent.

I probably miss a newer NetworkManager, as I still get the nm-applet dialog, but the patch is working for Matthias ...

The patch fixes things for me, thanks !

I think I spoke too early, maybe. The patch does fix the dialog not showing up.
But I don't actually get a vpn connection. Maybe related:

Sep  7 13:44:51 lemur NetworkManager[924]: NetworkManager[924]: get_secrets: assertion `secrets_idx < SECRETS_REQ_LAST' failed

And if I cancel the shell dialog, the nm-applet dialog shows up after a while

Comment on attachment 195914 [details] [review]
network-agent: Fix exception for unhandled connections

>+        if (!this._content)
>+            return;

won't this just result in the creation (and display) of an empty "password" dialog then?

I tried testing, but I get a different error when trying to activate the VPN:

    JS ERROR: !!!   Exception was: Error: Argument 'path' (type utf8) may not be null
    JS ERROR: !!!     lineNumber = '0'
    JS ERROR: !!!     fileName = '"gjs_throw"'
    JS ERROR: !!!     stack = '"("Argument 'path' (type utf8) may not be null")@gjs_throw:0
()@/home/danw/gshell/gnome-shell/js/ui/status/network.js:1837
([object _private_NMClient_Client],[object _private_GLib_ParamSpec])@/home/danw/gshell/gnome-shell/js/ui/status/network.js:2002
"'

Comment on attachment 195915 [details] [review]
network-agent: Handle VPN passwords

looks right. can't test

Comment on attachment 195915 [details] [review]
network-agent: Handle VPN passwords

(In reply to comment #7)
> (From update of attachment 195915 [details] [review])
> looks right. can't test

Can't test either, but apparently it doesn't work:

<dcbw> fmuellner: so we need to handle VPN secrets differently
<dcbw> each vpn is different and we don't know what secrets it may want
<dcbw> fmuellner: so what the applet does is call a vpn-specific "auth dialog"
<dcbw> fmuellner: basically we call the auth dialog binary (which we get from the .service files in /etc/NetworkManager/VPN) and pass it some arguments, and send it existing secrets if any on stdin, then wait for it to print some stuff to stdout, which we grab and send back to NM

Comment on attachment 195914 [details] [review]
network-agent: Fix exception for unhandled connections

(In reply to comment #6)
> (From update of attachment 195914 [details] [review])
> >+        if (!this._content)
> >+            return;
> 
> won't this just result in the creation (and display) of an empty "password"
> dialog then?

Yes.

*** Bug 658707 has been marked as a duplicate of this bug. ***

Any workaround, in lieu of a fix?

*** Bug 658994 has been marked as a duplicate of this bug. ***

Is there a way to revert to the "fallback mode" nm-applet without sacrificing the rest of gnome-shell? What I've doing so far to get around this is

$ metacity --replace & sleep 5 && gnome-panel &
<connect to vpn with the old nm-applet>
$ gnome-shell --replace

(alternatively, an easier way to switch between gnome-shell and fallback mode without a log-out/log-in cycle would suffice)

Created attachment 197100 [details] [review]
shell-network-agent: Do not handle VPN secrets

VPN secrets are currently unhandled by the UI code. To avoid
lengthy timeouts, bail out early with an error, so NetworkManager
falls back to the nm-applet agent directly.

(In reply to comment #14)
> Created an attachment (id=197100) [details] [review]
> shell-network-agent: Do not handle VPN secrets
> 
> VPN secrets are currently unhandled by the UI code. To avoid
> lengthy timeouts, bail out early with an error, so NetworkManager
> falls back to the nm-applet agent directly.

Yeah, this looks like the right behavior until we handle VPN natively in the shell.

Review of attachment 197100 [details] [review]:

Looks right to me.

Comment on attachment 197100 [details] [review]
shell-network-agent: Do not handle VPN secrets

Attachment 197100 [details] pushed as 526a53b - shell-network-agent: Do not handle VPN secrets

Created attachment 200665 [details] [review]
NetworkAgent: add support for VPN connections

VPN secrets are stored by the plugins, that provide separate
helpers for authentication. This commit adds the support for invoking
the binaries and pass them connection details.

(In reply to comment #18)
> VPN secrets are stored by the plugins, that provide separate
> helpers for authentication. This commit adds the support for invoking
> the binaries and pass them connection details.

This would have been nice for 3.2, but for 3.4 we shouldn't just copy nm-applet and strive for native shell dialogs instead.

(In reply to comment #19)
> (In reply to comment #18)
> > VPN secrets are stored by the plugins, that provide separate
> > helpers for authentication. This commit adds the support for invoking
> > the binaries and pass them connection details.
> 
> This would have been nice for 3.2, but for 3.4 we shouldn't just copy nm-applet
> and strive for native shell dialogs instead.

This won't be easy, as it will require integrating all VPN plugins inside gnome-shell core, or provide some higher level protocol for marking what should asked and how.

Are extensions integrated enough that each VPN plugin could ship its own?

(In reply to comment #20)
> This won't be easy, as it will require integrating all VPN plugins inside
> gnome-shell core, or provide some higher level protocol for marking what should
> asked and how.

The current idea is to go with the second option.

Created attachment 201626 [details] [review]
NetworkAgent: add support for VPN connections

VPN secrets are stored by the plugins, that provide separate
helpers for authentication. This commit adds the support for invoking
the binaries and pass them connection details.
For plugins that support it (as reported by their keyfile), we invoke
them in "shell-mode", and expect a set of metadata about their secrets,
which is used to build a shell styled dialog.

Created attachment 201627 [details] [review]
NetworkAgent: add support for VPN connections

VPN secrets are stored by the plugins, that provide separate
helpers for authentication. This commit adds the support for invoking
the binaries and pass them connection details.
For plugins that support it (as exposed by their keyfile), we invoke
them in "shell-mode" and expect a set of metadata about the secrets
which is used to build a shell styled dialog.

Updated for new protocol. I have patches for OpenVPN, and I tested them.
Soon I'll start working on PPTP and vpnc, and then submit everything
in a separate bug for NetworkManager.

I'm not sure that shell-native dialogs are the correct thing. Do we really want to put up a fancy dialog prompting the user for information they may not have, or, even worse, may be on a web page they cannot access because the dialog is now modal?

Yeah; I don’t understand why the existing wireless password dialogs are modal, either.  I don’t want my work interrupted just because I wandered into range of a password-protected AP.

Created attachment 201865 [details] [review]
StImText: fix for multiple disposal

If the actor is destroyed explicitly, it can be disposed multiple
times. We should not emit criticals in that case.

A small fix on which the following patch depends. Can be pushed
independently on the rest.

Created attachment 201866 [details] [review]
NetworkAgent: rewrite to use a message tray notification

Using a modal dialog can be annoying, as it can pop up at random
times as NM finds known AP, and it can prevent retrieving the password
stored on disk.
The new notification is not critical, otherwise it would be modal and
bring no improvement. This means that will popup only briefly and
then end up in the message tray.

Here you are: network dialogs as message tray notifications (similar
to those for bluetooth). Now, designers, it's up to you.

Comment on attachment 201866 [details] [review]
NetworkAgent: rewrite to use a message tray notification

I talked with the designers in IRC, and we agree that modal dialogs are fine for networks, as they should always appear as a result of a user action.

(In reply to comment #29)
> (From update of attachment 201866 [details] [review])
> I talked with the designers in IRC, and we agree that modal dialogs are fine
> for networks, as they should always appear as a result of a user action.
What will happen if you come in reach of a known encrypted wireless network whose passphrase has changed since last time you connect to it? That's a pretty rare use case, but it might still happen. Will you show a modal dialog anyway, or will you fail to connect, requiring manual action?

There is a technical problem there: we cannot, at the shell level, know if a secret request is a result of user action, of automatic connection at login, or anything else. The shell just knows "NetworkManager needs secrets for this connection", and does its best to provide it.

Comment on attachment 201865 [details] [review]
StImText: fix for multiple disposal

same as patch in bug 665000 which I recently reviewed

(In reply to comment #32)
> (From update of attachment 201865 [details] [review])
> same as patch in bug 665000 which I recently reviewed

Owen, I think you rejected the wrong patch/bug? Those two don't seem to be in any way related.

oh, my bad. I was thinking that comment was about the patch that is now marked "rejected", but it was actually a patch that you marked obsolete.

Comment on attachment 201627 [details] [review]
NetworkAgent: add support for VPN connections

>For plugins that support it (as exposed by their keyfile), we invoke
>them in "shell-mode"

this patch will need updates throughout to match whatever naming bug 664335 ends up with.

>+            log('arguments: ' + argv.join(' '));

lose that before the final version...

>+            let [success, pid, stdin, stdout, stderr] = GLib.spawn_async_with_pipes(null, /* pwd */
>+                                                                                    argv,

maybe newline after the "=" to avoid the line getting so long?

>+        GLib.spawn_close_pid(this._childPid);

you don't need to do that since we don't care about Windows portability

>+            if ((status & 0x7F) == 0) { // WIFEXITED, straight from glibc
>+                let exitStatus = (status & 0xFF00) >> 8;

ew. glib really ought to have introspectable support for this.

anyway, that's probably not going to work on solaris and bsd. shell-util to the rescue?

>+                log('key is ' + key);

more debugging to get rid of

>+                    this._title = title.substr('TITLE='.length);
>+                    this._description = description.substr('DESCRIPTION='.length);

This is weird: the new format has NAME=value pairs, but the shell code never really looks at the names and instead just assumes that the data is all written out in a specific order. (And it even gets it wrong, because the patches in 664335 all write out DESCRIPTION first and then TITLE.)

I'd say make it actually parse the names.

Actually... maybe make the version 2 format be a GKeyFile, with one group for the initial metadata and then another group for each key? And the shell could just write the "QUIT\n\n" line right away, and then rather than reading the key data line by line, it can just read the whole thing all at once when the helper exits, and then parse it all at once...

Anyway, either way, it might be cleaner to separate _vpnChildOutput into two functions, one for old-style and one for new?

>+                let contentItem = { key: key.substr('ENTRY_LEN='.length),

ENTRY_KEY not ENTRY_LEN

>+        /* In theory, we need to force / as prefix here and below, to ensure
>+           we pick the plugins matching the installed NetworkManager.
>+           For testing purposes, though, we need /opt/gnome as the no distro packaged
>+           plugin has support for gnome-shell mode */
>+        let dir = Gio.file_new_for_path('/opt/gnome/etc/NetworkManager/VPN');

need something better there...

>+                let path = GLib.build_filenamev(['/opt/gnome/libexec', binary]);

nm-applet also does g_path_get_basename() before prepending $(libexecdir)

(in addition to the previous problems with assuming /opt/gnome, this line has the additional problem of assuming a libexec directory, which IIRC is wrong on Debian)

>+  if (request->is_vpn)
>+    request->entries = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_free);
>+  else
>+    request->entries = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, gvalue_destroy_notify);

This is weird and scary since there are various other functions that assume entries contains GValues. I think it would be better to have a separate vpn_entries hash table.

>+  if (request->is_vpn ||
>+      (flags & NM_SECRET_AGENT_GET_SECRETS_FLAG_REQUEST_NEW) ||
>       (flags && NM_SECRET_AGENT_GET_SECRETS_FLAG_ALLOW_INTERACTION
>        && is_connection_always_ask (request->connection)))

nm-applet doesn't have a special case for is_vpn... although it also looks in gnome-keyring before doing this check, whereas ShellNetworkAgent does this first and then looks in gnome-keyring as fallback... Not sure what's right here.

(In reply to comment #35)
> (From update of attachment 201627 [details] [review])
> >For plugins that support it (as exposed by their keyfile), we invoke
> >them in "shell-mode"
> 
> this patch will need updates throughout to match whatever naming bug 664335
> ends up with.

I think I'll go with external-ui[-mode].

> 
> >+                    this._title = title.substr('TITLE='.length);
> >+                    this._description = description.substr('DESCRIPTION='.length);
> 
> This is weird: the new format has NAME=value pairs, but the shell code never
> really looks at the names and instead just assumes that the data is all written
> out in a specific order. (And it even gets it wrong, because the patches in
> 664335 all write out DESCRIPTION first and then TITLE.)

No, it's right, first it reads description and then title.

> I'd say make it actually parse the names.
> 
> Actually... maybe make the version 2 format be a GKeyFile, with one group for
> the initial metadata and then another group for each key? And the shell could
> just write the "QUIT\n\n" line right away, and then rather than reading the key
> data line by line, it can just read the whole thing all at once when the helper
> exits, and then parse it all at once...
> 
> Anyway, either way, it might be cleaner to separate _vpnChildOutput into two
> functions, one for old-style and one for new?

Problem is, we won't know if it's old or new style until we read something (given that I want to be backward compatible with old style plugins).
But given that keyfiles start with [, and that's not a valid GObject property name, it could work.

> >+  if (request->is_vpn)
> >+    request->entries = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_free);
> >+  else
> >+    request->entries = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, gvalue_destroy_notify);
> 
> This is weird and scary since there are various other functions that assume
> entries contains GValues. I think it would be better to have a separate
> vpn_entries hash table.
> 
> >+  if (request->is_vpn ||
> >+      (flags & NM_SECRET_AGENT_GET_SECRETS_FLAG_REQUEST_NEW) ||
> >       (flags && NM_SECRET_AGENT_GET_SECRETS_FLAG_ALLOW_INTERACTION
> >        && is_connection_always_ask (request->connection)))
> 
> nm-applet doesn't have a special case for is_vpn... although it also looks in
> gnome-keyring before doing this check, whereas ShellNetworkAgent does this
> first and then looks in gnome-keyring as fallback... Not sure what's right
> here.

VPN plugins are expected to check gnome-keyring first, and then show the UI if nothing is found. Checking also in gnome-shell should be unnecessary (and also often we don't have the necessary information, that is, we could only have partial secrets and not know if anything else is needed).

(In reply to comment #36)
> > out in a specific order. (And it even gets it wrong, because the patches in
> > 664335 all write out DESCRIPTION first and then TITLE.)
> 
> No, it's right, first it reads description and then title.

oh, right, it reads them in the right order, it's just the post-processing that happens in the other order.

> > Anyway, either way, it might be cleaner to separate _vpnChildOutput into two
> > functions, one for old-style and one for new?
> 
> Problem is, we won't know if it's old or new style until we read something

Can't you just assume that if the plugin supports external-ui-mode, that it will always use the new style when we asked for it?

(In reply to comment #37)
> > > Anyway, either way, it might be cleaner to separate _vpnChildOutput into two
> > > functions, one for old-style and one for new?
> > 
> > Problem is, we won't know if it's old or new style until we read something
> 
> Can't you just assume that if the plugin supports external-ui-mode, that it
> will always use the new style when we asked for it?

I wanted to keep old-style for when the plugin already has all needed secrets, and simply needs to retrieve them.
Turns out that:
1) First, we need one gnome-keyring pass in the Shell, and only ask the helper if new secrets are needed. Otherwise configuration dialogs can get confused trying to retrieve stored secrets.
2) Second, pipe semantics confuse the input streams (GUnixInputStream calls poll(), but then doesn't check for POLLHUP, which makes GDataInputStream return NULL, losing one line of input), which means we need two completely different code paths to get something that can be fed to GKeyFile.

In any case, uploading newer patch, tested with OpenVPN and VPNC (to the point of spawning the service, not of connecting, since I don't have VPN), plus some related minor patches.

Created attachment 207454 [details] [review]
GDataInputStream: don't segfault on async line reads

If an async line read fails, it returns NULL. In that case, we
must return NULL before validating the line, or we segfault.

Created attachment 207455 [details] [review]
GKeyFile: fix annotation of g_key_file_load_from_data

(array) without (element-type) means "array of the same type as
the C type", so gchar* with (array) is interpreted as an array of
strings. Since the purpose is to have an array of binary data,
add an (element-type guint8) annotation.

Created attachment 207456 [details] [review]
Config: use sed for substituting variables

Substitutions generated by configure don't resolve prefixes, so
cannot be used for paths. Config already had localedir, and next
commit will need libexecdir and sysconfdir, so just bite the bullet
and move to sed.

Created attachment 207457 [details] [review]
NetworkAgent: add support for VPN connections

VPN secrets are stored by the plugins, that provide separate
helpers for authentication. This commit adds the support for invoking
the binaries and pass them connection details.
For plugins that support it (as exposed by their keyfile), we invoke
them in "shell-mode" and expect a set of metadata about the secrets
which is used to build a shell styled dialog.

Created attachment 207458 [details] [review]
ShellNetworkAgent: don't access request fields if the operation is cancelled

When the operation is cancelled by NetworkManager, the request is
cancelled immediately. Later when gnome-keyring invokes the callback
notifying the error we must therefore not access its memory.
Previously the callback would mistakenly treat "cancelled" (which
indicates a programmatic cancel) as "denied" (which means the user
clicked "Cancel" on the keyring prompt)

(In reply to comment #38)
> 2) Second, pipe semantics confuse the input streams (GUnixInputStream calls
> poll(), but then doesn't check for POLLHUP, which makes GDataInputStream return
> NULL, losing one line of input), which means we need two completely different
> code paths to get something that can be fed to GKeyFile.

Can you clarify what you mean here? Is this a GUnixInputStream bug?

What should be happening is that when the pipe is closed, g_unix_input_stream_read() returns 0.

Comment on attachment 207455 [details] [review]
GKeyFile: fix annotation of g_key_file_load_from_data

>- * @data: (array length=length): key file loaded in memory
>+ * @data: (array length=length) (element-type guint8): key file loaded in memory
>  * @length: the length of @data in bytes

the docs at the top of the file state:

 *   <listitem>Key files are always encoded in UTF-8.</listitem>

so shouldn't @data just be unannotated and treated as a string?

(@length should also note that you can pass -1)

Comment on attachment 207456 [details] [review]
Config: use sed for substituting variables

>+EXTRA_DIST = config.js.in

need "misc/"

>+misc/config.js: misc/config.js.in

should probably depend on Makefile as well so it gets rebuilt when you reconfigure

Comment on attachment 207457 [details] [review]
NetworkAgent: add support for VPN connections

basically looks good. I haven't tested yet, but I will.

>For plugins that support it (as exposed by their keyfile), we invoke
>them in "shell-mode" and expect a set of metadata about the secrets

"external-ui-mode"

>+    _readStdoutNewStyle: function() {
>+        this._dataStdout.fill_async(-1, GLib.PRIORITY_DEFAULT, null, Lang.bind(this, function(stream, result) {

Doing it this way (as opposed to reading it in normally and concatenating to a JS variable) seems a little weird, but I guess it works...

>+            keyfile.load_from_data(this._dataStdout.peek_buffer(),
>+                                   GLib.KeyFileFlags.NONE);
>+
>+            log(keyfile.to_data());

kill debugging statement

(In reply to comment #44)
> (In reply to comment #38)
> > 2) Second, pipe semantics confuse the input streams (GUnixInputStream calls
> > poll(), but then doesn't check for POLLHUP, which makes GDataInputStream return
> > NULL, losing one line of input), which means we need two completely different
> > code paths to get something that can be fed to GKeyFile.
> 
> Can you clarify what you mean here? Is this a GUnixInputStream bug?
> 
> What should be happening is that when the pipe is closed,
> g_unix_input_stream_read() returns 0.

Yes, but then g_data_input_stream_read_line() returns null, not "", and that somehow makes it miss one line, which coincidentally is the line starting with [ that triggers new style. But none of this applies any more, as new-style plugins are now expected to be always new-style.

(In reply to comment #45)
> (From update of attachment 207455 [details] [review])
> >- * @data: (array length=length): key file loaded in memory
> >+ * @data: (array length=length) (element-type guint8): key file loaded in memory
> >  * @length: the length of @data in bytes
> 
> the docs at the top of the file state:
> 
>  *   <listitem>Key files are always encoded in UTF-8.</listitem>

desktop-entry-spec says otherwise (appendix D, Legacy-Mixed encoding), but if these are the constraint of the GLib implementation, then ok.

> so shouldn't @data just be unannotated and treated as a string?
> 
> (@length should also note that you can pass -1)

That's fine, although it's technically an API break for gjs and pygobject (unlikely to matter, given it was not possible to use this function directly).

(In reply to comment #38)
> In any case, uploading newer patch, tested with OpenVPN and VPNC (to the point
> of spawning the service, not of connecting, since I don't have VPN)

It works to connect to the VPN.

But if nm-applet is running, I get a password dialog from nm-applet first, and have to cancel that before I get the shell one. I don't know how the whole "secret agent" thing works in detail, but that seems weird to me... but if that's expected behavior then we'll need to do some other thing as well. Is this the last thing left that we use nm-applet for? In that case I guess the fix would be just removing nm-applet from the default session.

Created attachment 207492 [details] [review]
GKeyFile: fix annotation of g_key_file_load_from_data

(array) without (element-type) means "array of the same type as
the C type", so gchar* with (array) is interpreted as an array of
strings. Since GKeyFiles must be UTF-8 encoded anyway, just
annotate it as a string.

(In reply to comment #49)
> (In reply to comment #38)
> > In any case, uploading newer patch, tested with OpenVPN and VPNC (to the point
> > of spawning the service, not of connecting, since I don't have VPN)
> 
> It works to connect to the VPN.
> 
> But if nm-applet is running, I get a password dialog from nm-applet first, and
> have to cancel that before I get the shell one. I don't know how the whole
> "secret agent" thing works in detail, but that seems weird to me... but if
> that's expected behavior then we'll need to do some other thing as well. Is
> this the last thing left that we use nm-applet for? In that case I guess the
> fix would be just removing nm-applet from the default session.

Yes, the whole point of this exercise is to kick nm-applet out of the shell session, and let it slowly die in the fallback. There is no need to fix NetworkManager to deal with multiple secret agents in the same session at the same time.

Created attachment 207493 [details] [review]
Config: use sed for substituting variables

Substitutions generated by configure don't resolve prefixes, so
cannot be used for paths. Config already had localedir, and next
commit will need libexecdir and sysconfdir, so just bite the bullet
and move to sed.

Created attachment 207494 [details] [review]
NetworkAgent: add support for VPN connections

VPN secrets are stored by the plugins, that provide separate
helpers for authentication. This commit adds the support for invoking
the binaries and pass them connection details.
For plugins that support it (as exposed by their keyfile), we invoke
them in "external-ui-mode" and expect a set of metadata about the
secrets which is used to build a shell styled dialog.

Comment on attachment 207492 [details] [review]
GKeyFile: fix annotation of g_key_file_load_from_data

>+ * @length: the length of @data in bytes (or -1 if data is NULL-terminated)

"nul" (0 byte) not "NULL" (0 pointer)

Attachment 207454 [details] pushed as 5b8a690 - GDataInputStream: don't segfault on async line reads
Attachment 207492 [details] pushed as d240b88 - GKeyFile: fix annotation of g_key_file_load_from_data
Next I'll push the update to gobject-introspection...

Attachment 207458 [details] pushed as c5804c1 - ShellNetworkAgent: don't access request fields if the operation is cancelled
Attachment 207493 [details] pushed as 62c0088 - Config: use sed for substituting variables
Attachment 207494 [details] pushed as 92276c5 - NetworkAgent: add support for VPN connections

This parses the .name files itself but doesn't honour the 'aliases' field. So with https://git.gnome.org/browse/network-manager-openconnect/commit/?id=34051bb I get:
Gjs-Message: JS LOG: Invalid VPN service type (cannot find authentication binary)

If I actually knew any JavaScript, there might be a chance that this would fix it...

--- a/js/ui/components/networkAgent.js
+++ b/js/ui/components/networkAgent.js
@@ -796,9 +796,14 @@ const NetworkAgent = new Lang.Class({
                         path = GLib.build_filenamev([Config.LIBEXECDIR, path]);
                     }
 
-                    if (GLib.file_test(path, GLib.FileTest.IS_EXECUTABLE))
+                    if (GLib.file_test(path, GLib.FileTest.IS_EXECUTABLE)) {
                         this._vpnBinaries[service] = { fileName: path, externalUIMode: externalUIMode, supportsHints: hints };
-                    else
+                        try {
+                            var alias;
+                            for (alias in keyfile.get_string_list('VPN Connection', 'aliases'))
+                                this._vpnBinaries[alias] = { fileName: path, externalUIMode: externalUIMode, supportsHints: hints };
+                        } catch(e) { } // ignore errors if key does not exist
+                    } else
                         throw new Error('VPN plugin at %s is not executable'.format(path));
                 } catch(e) {
                     log('Error \'%s\' while processing VPN keyfile \'%s\''.

Created attachment 329058 [details] [review]
Honour 'aliases' in NM VPN configuration

(In reply to David Woodhouse from comment #58)
> If I actually knew any JavaScript, there might be a chance that this would
> fix it...

This appears to work, so attaching it properly as a patch.

Review of attachment 329058 [details] [review]:

Looks mostly good to me. Can you include a commit message with git-format?

::: js/ui/components/networkAgent.js
@@ +801,3 @@
+                        try {
+                            let aliases = keyfile.get_string_list('VPN Connection', 'aliases');
+                            var alias;

Should use let instead of var; also, could be writen as:

for (let alias of aliases)
    this._vpnBinaries[alias] = ...

Created attachment 329081 [details] [review]
Honour 'aliases' in NM VPN configuration (v2)

Thanks for the feedback. I was surprised I managed to make something that even worked :)

Review of attachment 329081 [details] [review]:

I didn't test the patch, but assuming the updated patch still works for you, the code looks good to me.

::: js/ui/components/networkAgent.js
@@ +806,3 @@
+                            }
+                        } catch(e) { } // ignore errors if key does not exist
+                    } else

Oh, one more nit:
Coding style is to either not have braces on any blocks or on all blocks, so the else block needs braces now as well.

(In reply to Florian Müllner from comment #62)
> Coding style is to either not have braces on any blocks or on all blocks, so
> the else block needs braces now as well.


Fixed and pushed as 2705434. Thanks.

However, I stupidly pushed it to gnome-3-20 branch first (since that's what I'd been building and testing). It does want fixing there, but I was intending to fix it in master first and then explicitly *ask* for a backport. Rather than just pushing it.

Want me to revert it and then ask for permission? Or is it sufficient just to ask for forgiveness? Sorry!

(In reply to David Woodhouse from comment #63)
> Want me to revert it and then ask for permission?

Nah, that sounds silly - just consider yourself absolved :-)