After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 767321 - nm-applet timeout on lengthy password input -> clicking 'connect' does nothing
nm-applet timeout on lengthy password input -> clicking 'connect' does nothing
Status: RESOLVED OBSOLETE
Product: NetworkManager
Classification: Platform
Component: nm-applet
unspecified
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
: 767667 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2016-06-07 10:03 UTC by Etienne Papegnies
Modified: 2020-11-12 14:34 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[PATCH 1/2] secret-agent: increase timeout for GetSecrets D-Bus call (1.62 KB, patch)
2016-06-10 14:33 UTC, Beniamino Galvani
none Details | Review
[PATCH 2/2] secrets: call CancelGetSecrets after a timeout in secret request (5.34 KB, patch)
2016-06-10 14:34 UTC, Beniamino Galvani
none Details | Review
[PATCH v2 2/2] secrets: call CancelGetSecrets after a timeout in secret request (6.50 KB, patch)
2016-06-15 08:38 UTC, Beniamino Galvani
none Details | Review

Description Etienne Papegnies 2016-06-07 10:03:44 UTC
Overview:

Upon entering a long WPA/WPA2 Personal password after clicking the network icon in the top right panel, the connection is not added to network-manager if password input takes more than about 25 seconds


Steps to Reproduce:

- Install Ubuntu MATE 16.04 LTS (possibly other Ubuntu flavours using nm-applet too)
- Run normal post-install full system update and reboot
- Click the network icon in the top right corner of the default panel layout
- Select from the Wi-Fi network list a network using WPA/WPA2 Personal
- Enter the password, wait 30 seconds to click 'connect'


Actual Results:

Nothing happens. No crashes as far as I can see, Wi-Fi icon stays on disconnected status


Expected Results:

New connection should be added to network-manager and icon should show connected status


Platforms: 

Ubuntu MATE 16.04 LTS, up to date as of 2016/06/07


Additional Information:

While this issue might seem trivial (using 'System -> Preferences -> Internet and Network -> Network Connections' allows one to correctly save the password) (and quickly copy/pasting the password from a text file works too) it is a serious usability problem for many people in France.

The default home router Wi-Fi setup in France is WPA/WPA2 Personal with a random 26-character hexadecimal password. It takes a while to type, and once it's finally done nothing happens because of the ~25 second timeout
Comment 1 Beniamino Galvani 2016-06-08 15:58:12 UTC
(In reply to Etienne Papegnies from comment #0)
> Overview:
>
> Upon entering a long WPA/WPA2 Personal password after clicking the network
> icon in the top right panel, the connection is not added to network-manager
> if password input takes more than about 25 seconds

25 seconds is the default value for g_dbus_proxy_call()	if no timeout
is passed and gdbus-codegen doesn't specify a timeout in the generated
nmdbus_secret_agent_call_get_secrets().

Probably we can increase the timeout for the whole proxy to 60
changing the "g-default-timeout" property after creation. It doesn't
seem possible to tell gdbus-codegen to use a different value for a
single method.

Regarding user feedback, I don't think it's possible to detect the
timeout in the client agent in order to hide the dialog, unless we
modify NM to call CancelGetSecrets() after GetSecrets() timeouts.
Comment 2 Beniamino Galvani 2016-06-10 14:33:52 UTC
Created attachment 329575 [details] [review]
[PATCH 1/2] secret-agent: increase timeout for GetSecrets D-Bus call
Comment 3 Beniamino Galvani 2016-06-10 14:34:27 UTC
Created attachment 329576 [details] [review]
[PATCH 2/2] secrets: call CancelGetSecrets after a timeout in secret request
Comment 4 Thomas Haller 2016-06-14 21:32:31 UTC
*** Bug 767667 has been marked as a duplicate of this bug. ***
Comment 5 Beniamino Galvani 2016-06-15 08:38:26 UTC
Created attachment 329847 [details] [review]
[PATCH v2 2/2] secrets: call CancelGetSecrets after a timeout in secret request

Reworked patch #2 to handle the cancellation in the agent. It turned out to be a bit more complex than expected because we have to track pending cancellations and cancel them if the agent is destroyed.
Comment 6 Thomas Haller 2016-06-15 08:50:38 UTC
(In reply to Beniamino Galvani from comment #5)
> Created attachment 329847 [details] [review] [review]
> [PATCH v2 2/2] secrets: call CancelGetSecrets after a timeout in secret
> request
> 
> Reworked patch #2 to handle the cancellation in the agent. It turned out to
> be a bit more complex than expected because we have to track pending
> cancellations and cancel them if the agent is destroyed.

looks correct, but I don't understand why you need TimeoutCancelInfo at all.

If you look below to do_cancel_secrets(), it calls nmdbus_secret_agent_call_cancel_get_secrets() without passing on any info data or any cancellable instance. Especially, it does not pass on a @self pointer, thereby avoiding the complexity of tracking TimeoutCancelInfo instances.

I think you could just call

»···»···nmdbus_secret_agent_call_cancel_get_secrets (priv->proxy,
»···»···                                             r->path, r->setting_name,
»···»···                                             NULL,
»···»···                                             cancel_done,
»···»···                                             g_strdup (nm_secret_agent_get_description (self)));
Comment 7 David Woodhouse 2016-06-15 09:21:06 UTC
Hm, how does this cancellation actually work for a VPN authentication dialog?

NM-openconnect's auth-dialog basically does its thing and then just ends with wait_for_quit(). I assume that on cancellation, it would get 'QUIT' (or something else?) on stdin. Which it wouldn't notice until later.

We'd need it to be monitoring stdin for the 'QUIT' or 'CANCEL' at runtime, not just wait for it when it's done. 

And if it does happen, we'd need it to behave appropriately. How is the user *told* that the timeout happened and the login failed?

They might have just typed their password (slowly), and hit 'login'. It takes a second or so for the communication with the authentication server to happen, and to be granted their login cookie (the 'secret' for the connection).

The user is *expecting* the auth-dialog to disappear when it's finished, and the VPN connection to be established. If they fat-finger their password, or the server can't be reached, or anything else goes wrong, then the auth-dialog sticks around until it's explicitly banished by the user.

To have it disappear silently on timeout is not a viable user experience.

Why time out when the auth-dialog is still active anyway? Can't you just ping it, if you really must?

Some users have to wait for an incoming phone call with a PIN code to type into the auth dialog at this point. It's not quick...
Comment 8 Beniamino Galvani 2016-06-15 09:40:42 UTC
(In reply to Thomas Haller from comment #6)

> looks correct, but I don't understand why you need TimeoutCancelInfo at all.
> 
> If you look below to do_cancel_secrets(), it calls
> nmdbus_secret_agent_call_cancel_get_secrets() without passing on any info
> data or any cancellable instance. Especially, it does not pass on a @self
> pointer, thereby avoiding the complexity of tracking TimeoutCancelInfo
> instances.
> 
> I think you could just call
> 
> »···»···nmdbus_secret_agent_call_cancel_get_secrets (priv->proxy,
> »···»···                                             r->path,
> r->setting_name,
> »···»···                                             NULL,
> »···»···                                             cancel_done,
> »···»···                                             g_strdup
> (nm_secret_agent_get_description (self)));

Will the callback (cancel_done) still be invoked if the agent (and thus also the proxy) is destroyed in the meanwhile? If not, you're right. Otherwise the TimeoutCancelInfo hash (or something similar) is needed to cancel pending actions upon destroy.
Comment 9 Thomas Haller 2016-06-15 09:50:15 UTC
(In reply to Beniamino Galvani from comment #8)
> (In reply to Thomas Haller from comment #6)
> 
> > looks correct, but I don't understand why you need TimeoutCancelInfo at all.
> > 
> > If you look below to do_cancel_secrets(), it calls
> > nmdbus_secret_agent_call_cancel_get_secrets() without passing on any info
> > data or any cancellable instance. Especially, it does not pass on a @self
> > pointer, thereby avoiding the complexity of tracking TimeoutCancelInfo
> > instances.
> > 
> > I think you could just call
> > 
> > »···»···nmdbus_secret_agent_call_cancel_get_secrets (priv->proxy,
> > »···»···                                             r->path,
> > r->setting_name,
> > »···»···                                             NULL,
> > »···»···                                             cancel_done,
> > »···»···                                             g_strdup
> > (nm_secret_agent_get_description (self)));
> 
> Will the callback (cancel_done) still be invoked if the agent (and thus also
> the proxy) is destroyed in the meanwhile? If not, you're right. Otherwise
> the TimeoutCancelInfo hash (or something similar) is needed to cancel
> pending actions upon destroy.

To my understanding, the callback will always be invoked.
As cancel_done() does not at all access @self, so on that front there is no problem.
Also, even though @self might already be gone (and thus it's proxy is unrefed by @self), I would strongly suspect that the pending request still holds another reference to the proxy. Otherwise the @proxy argument in cancel_done() would be a dangling pointer, which would be wrong from gdbus.
Comment 10 Thomas Haller 2016-06-15 10:25:11 UTC
(In reply to David Woodhouse from comment #7)
> Hm, how does this cancellation actually work for a VPN authentication dialog?
> 
> NM-openconnect's auth-dialog basically does its thing and then just ends
> with wait_for_quit(). I assume that on cancellation, it would get 'QUIT' (or
> something else?) on stdin. Which it wouldn't notice until later.
> 
> We'd need it to be monitoring stdin for the 'QUIT' or 'CANCEL' at runtime,
> not just wait for it when it's done. 
> 
> And if it does happen, we'd need it to behave appropriately. How is the user
> *told* that the timeout happened and the login failed?
> 
> They might have just typed their password (slowly), and hit 'login'. It
> takes a second or so for the communication with the authentication server to
> happen, and to be granted their login cookie (the 'secret' for the
> connection).
> 
> The user is *expecting* the auth-dialog to disappear when it's finished, and
> the VPN connection to be established. If they fat-finger their password, or
> the server can't be reached, or anything else goes wrong, then the
> auth-dialog sticks around until it's explicitly banished by the user.
> 
> To have it disappear silently on timeout is not a viable user experience.
> 
> Why time out when the auth-dialog is still active anyway? Can't you just
> ping it, if you really must?
> 
> Some users have to wait for an incoming phone call with a PIN code to type
> into the auth dialog at this point. It's not quick...

maybe we should increase the timeout even more (3 minutes)? But eventually there must be a timeout.

After timeout (thus cancelling the request), NM will no longer accept a response.

It's entirely up to the client/secret-agent/auth-dialog to communicate a timeout to the user. If silently letting the password dialog is a bad idea, then it's up to the client to show a notification.

I think that is out-of-scope for this bug.
Comment 11 David Woodhouse 2016-06-15 10:34:43 UTC
(In reply to Thomas Haller from comment #10)
> maybe we should increase the timeout even more (3 minutes)? But eventually
> there must be a timeout.

Why? If the auth-dialog is still present, actively displaying a request to the user and you can ping it to confirm that it really is *active* and hasn't gone off into the weeds... why would you need a timeout?

Let the auth-dialog time out under its own control, perhaps. With a *visible* countdown to the user once it starts getting bored... "Do something within 20 seconds or the attempt is cancelled"... 19... 18... etc.

But why do *you* need a timeout?

> After timeout (thus cancelling the request), NM will no longer accept a
> response.
> 
> It's entirely up to the client/secret-agent/auth-dialog to communicate a
> timeout to the user. If silently letting the password dialog is a bad idea,
> then it's up to the client to show a notification.
> 
> I think that is out-of-scope for this bug.

No. The question of "how does a failure get presented to the user" is *never* out of scope, whenever we're working on any given failure mode. That way lies madness, and the clusterf*ck of unexplicable failures we get with 802.1x authentication when the user doesn't even know that *something* went wrong, let alone *what*. Even when it's a clear-cut failure like the certificate being expired. Next person to tell me that reporting errors coherently is "out of scope" is getting nailed to a tree... :)

But OK, let's pretend you said something that didn't trigger my pet peeve. Let's pretend you said:

> Aha yes, we need to make it entirely clear to all VPN plugin authors that 
> they must handle this failure mode correctly, and give guidelines on how
> to do so.

The technical part of my question remains: in the auth-dialog, how do I *know* about the timeout and when it expire[sd]? Do I actually get QUIT on stdin when the timeout happens? Or TIMEOUT? Or something else?
Comment 12 Thomas Haller 2016-06-15 11:12:59 UTC
(In reply to David Woodhouse from comment #11)
> (In reply to Thomas Haller from comment #10)
> > maybe we should increase the timeout even more (3 minutes)? But eventually
> > there must be a timeout.
> 
> Why? If the auth-dialog is still present, actively displaying a request to
> the user and you can ping it to confirm that it really is *active* and
> hasn't gone off into the weeds... why would you need a timeout?
> 
> Let the auth-dialog time out under its own control, perhaps. With a
> *visible* countdown to the user once it starts getting bored... "Do
> something within 20 seconds or the attempt is cancelled"... 19... 18... etc.
> 
> But why do *you* need a timeout?

The client can have it's own timeout. And optimally, the timeout enforced by the server must be long enough to cover every valid client use-case. Ok, then 5 minutes? Or 10? Fine with me.


I think a timeout is needed. Say, the user has two connections wifi1 and wifi2. The former needs secrets (always-ask), the latter has secrets stored.
Computer starts, NM autoconnects to wifi1 and pops up a secret request in the UI. The user is away from screen. After timeout, the activation shall fail, and NM can autoactivate wifi2.
That seems useful, instead of indefinitely waiting for the user to enter a password.


A (server) timeout is no different from any other reason that makes the requested secret unneeded, and it shouldn't be handled differently.

E.g. say you are in the process of activating "wifi1" which is currently requesting a secret. When then another user issues `nmcli connection up wifi2`, then the original activation is aborted for another one. In this case, the secret-request is cancelled too. From UI point of view, there is almost no difference to a timeout. Should the UI silently hide the password dialog? Maybe not, but that discussion isn't specific about timeouts and it's a UI question.

If timeout appears any different in the client then a regular CancelGetSecrets, then that is a bug in libnm's NMSecretAgentOld and core.


> > After timeout (thus cancelling the request), NM will no longer accept a
> > response.
> > 
> > It's entirely up to the client/secret-agent/auth-dialog to communicate a
> > timeout to the user. If silently letting the password dialog is a bad idea,
> > then it's up to the client to show a notification.
> > 
> > I think that is out-of-scope for this bug.
> 
> No. The question of "how does a failure get presented to the user" is
> *never* out of scope, whenever we're working on any given failure mode. That
> way lies madness, and the clusterf*ck of unexplicable failures we get with
> 802.1x authentication when the user doesn't even know that *something* went
> wrong, let alone *what*. Even when it's a clear-cut failure like the
> certificate being expired. Next person to tell me that reporting errors
> coherently is "out of scope" is getting nailed to a tree... :)
> 
> But OK, let's pretend you said something that didn't trigger my pet peeve.
> Let's pretend you said:
> 
> > Aha yes, we need to make it entirely clear to all VPN plugin authors that 
> > they must handle this failure mode correctly, and give guidelines on how
> > to do so.



> The technical part of my question remains: in the auth-dialog, how do I
> *know* about the timeout and when it expire[sd]? Do I actually get QUIT on
> stdin when the timeout happens? Or TIMEOUT? Or something else?

You don't need to know about the timeout. The reason why the secret request got cancelled shouldn't matter.


As nm-applet spawns the auth-dialog, it is also the responsibility of nm-applet to notify the auth-helper that the request got cancelled. I don't know how that works, we have to look at 
https://git.gnome.org/browse/network-manager-applet/tree/src/applet-agent.c?id=37cd5d6c919eeaa5d1d4146d01038e9033795e30#n491
and similar for gnome-shell, etc.
Preferably, libnm's NMSecretAgentOld would help the clients dealing with the auth-helper.
Comment 13 David Woodhouse 2016-06-15 12:10:17 UTC
(In reply to Thomas Haller from comment #12)
> You don't need to know about the timeout. The reason why the secret request
> got cancelled shouldn't matter.

I disagree. You're asking the user for input, which implies that they actually had some *interest* in entering the information and allowing the connection to happen.

You don't just get to say "never mind" and take the dialog away. You need to let the user know *why* the dialog disappeared and she just typed the password into an IRC window instead, because she didn't notice the newly-changed focus.

> I don't know how that works, 

I'm going to guess at "it doesn't". See previous comments about needing to nail people to things.
Comment 14 Thomas Haller 2016-06-15 12:24:25 UTC
(In reply to David Woodhouse from comment #13)
> (In reply to Thomas Haller from comment #12)
> > You don't need to know about the timeout. The reason why the secret request
> > got cancelled shouldn't matter.
> 
> I disagree. You're asking the user for input, which implies that they
> actually had some *interest* in entering the information and allowing the
> connection to happen.
> 
> You don't just get to say "never mind" and take the dialog away. You need to
> let the user know *why* the dialog disappeared and she just typed the
> password into an IRC window instead, because she didn't notice the
> newly-changed focus.

Let's say there is ~almost no~ difference. Yes, CancelGetSecrets should carry a reason-argument, so that the UI could show a better error message (which it currently doesn't).


> > I don't know how that works, 
> 
> I'm going to guess at "it doesn't". See previous comments about needing to
> nail people to things.

I had that feeling too from a quick look at the code. "Out-of-scope for this bug" merely states that this bug is about something smaller. But yeah sure, let's improve this now, I agree it should be improved...
Comment 15 David Woodhouse 2016-06-15 20:00:33 UTC
(In reply to Thomas Haller from comment #14)
> Let's say there is ~almost no~ difference. Yes, CancelGetSecrets should
> carry a reason-argument, so that the UI could show a better error message
> (which it currently doesn't).

Yes. Let's talk about a reason-argument, so the UI can show a better error message.

I'll put the nail-gun down :)
Comment 16 Beniamino Galvani 2016-06-16 19:05:43 UTC
In my opinion a timeout for requests is needed so that NM might try
other agents if the first doesn't respond; but what really needs to be
improved is how a timeout is reported to users. Right now the
auth-dialog doesn't show any clue to users that the request has
expired, and after the patch I've attached it would simply disappear.

Instead we should notify the auth-dialog with a message ("QUIT"?) and
let the process live. The auth-dialog should then disable all
input fields and buttons (except Cancel) and show a message stating
that the request has expired (and if possible a reason). However with
the current D-Bus API it's not possible to pass a reason for the
cancelation, so we can only show a generic message.

What do you think? A sample implementation in branches:

https://git.gnome.org/browse/network-manager-openvpn/log/?h=bg/secret-timeout-bgo767321
https://git.gnome.org/browse/network-manager-applet/log/?h=bg/secret-timeout-bgo767321
https://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=bg/secret-agent-timeout-bgo767321
Comment 17 Dan Williams 2016-06-16 22:28:49 UTC
The branches look OK to me, but we could certainly do a copule things:

1) add a new CancelGetSecretsWithReason() D-Bus method that NM calls, and if it gets an error back (because the agent doesn't support it) then NM falls back to CancelGetSecrets()

2) add a new "EXPIRE" or something like that for auth dialogs instead of overloading QUIT?

Just some thoughts.
Comment 18 Thomas Haller 2016-06-17 06:31:49 UTC
(regardless of bg/secret-agent-timeout-bgo767321, let's finish the two patches, they fix a real issue).
Comment 19 Thomas Haller 2016-06-17 06:34:02 UTC
(In reply to Thomas Haller from comment #18)
> (regardless of bg/secret-agent-timeout-bgo767321, let's finish the two
> patches, they fix a real issue).

ah, they are on bg/secret-agent-timeout-bgo767321 :). Sorry for the noise.

NM's bg/secret-agent-timeout-bgo767321 lgtm
Comment 20 Thomas Haller 2016-06-17 07:11:53 UTC
Maybe we could extent the "QUIT" message to include the reason:

Like
  "QUIT <QUIT_REASON_ID> <LOCALIZED_MESSAGE>\n"



For now, you could only send:

  "QUIT 0 Secret no longer needed\n"

with
  #define NM_QUIT_REASON_UNSPECIFIED 0



(one question is, how to get a localized message. E.g. when the original error message comes from the server, it's unclear how the client can localize it. Anyway, that would be nm-applet's job. For example, if the message comes from a VPN plugin, it could as libnm-vpn-plugin-openvpn.sh to localize it...
but as far as the QUIT message is concerned, this shall be of no concern for the auth-helper).
Comment 21 Beniamino Galvani 2016-06-17 07:43:56 UTC
(In reply to Thomas Haller from comment #19)
> (In reply to Thomas Haller from comment #18)
> > (regardless of bg/secret-agent-timeout-bgo767321, let's finish the two
> > patches, they fix a real issue).
> 
> ah, they are on bg/secret-agent-timeout-bgo767321 :). Sorry for the noise.
> 
> NM's bg/secret-agent-timeout-bgo767321 lgtm

The first patch bumps the timeout to 60 seconds, but probably a greater value is better, how about 2 minutes?
Comment 22 Beniamino Galvani 2016-06-21 08:44:01 UTC
(In reply to Dan Williams from comment #17)
> 1) add a new CancelGetSecretsWithReason() D-Bus method that NM calls, and if
> it gets an error back (because the agent doesn't support it) then NM falls
> back to CancelGetSecrets()
> 
> 2) add a new "EXPIRE" or something like that for auth dialogs instead of
> overloading QUIT?

Make sense, done.

(In reply to Thomas Haller from comment #20)
> Maybe we could extent the "QUIT" message to include the reason:
> 
> Like
>   "QUIT <QUIT_REASON_ID> <LOCALIZED_MESSAGE>\n"

> (one question is, how to get a localized message. E.g. when the original
> error message comes from the server, it's unclear how the client can
> localize it. Anyway, that would be nm-applet's job. For example, if the
> message comes from a VPN plugin, it could as libnm-vpn-plugin-openvpn.sh to
> localize it...
> but as far as the QUIT message is concerned, this shall be of no concern for
> the auth-helper).

I think the server should send only a reason code and let the clients translate and localize it. In this case the applet should do the localization.

Pushed branch bg/secret-timeout-bgo767321 (NM, nm-applet, nm-openvpn)
Comment 23 Thomas Haller 2016-06-24 16:37:28 UTC
looks good.


Except, if you merge the patch to nm-openvpn you require new 1.4 API from libnma. In that case, we have to split master and nm-1-2 branches again.

If that's what it takes ok. But maybe we can avoid that? I would even prefer dlopening libnma.
Comment 24 Dan Williams 2016-06-24 23:11:44 UTC
LGTM, but...

Maybe I'm not seeing it, but why would nm-openvpn need to dlopen libnma?  I don't see anything about that in the nm-openvpn auth dialog patch, all I see is  a change to watch for CANCEL.

But it's true that would change behavior, since plugins would no longer be told to quit, we'd have to update all of them for the CANCEL behavior.  Is there a way we can send plugins QUIT anyway, but specify that if they receive a CANCEL they can ignore all other commands and quit when they have displayed the cancel message to the user?
Comment 25 Thomas Haller 2016-06-25 08:18:51 UTC
(In reply to Dan Williams from comment #24)
> LGTM, but...
> 
> Maybe I'm not seeing it, but why would nm-openvpn need to dlopen libnma?  I
> don't see anything about that in the nm-openvpn auth dialog patch, all I see
> is  a change to watch for CANCEL.

Currently, nm-openvpn's "master" and "nm-1-2" branch are identical (pointing to the same commit). Also, they work fine with libnm's and libnma's 1.2.x API (or newer). I think there is value in letting the VPN plugin only depend on the least required libnm/libnma API. It allows a user to run the same plugin against any 1.2.0+ API. It also decreases our maintenance effort. Instead of having nm-1-2 and nm-1-4 and newer branches, there is only one. And a user gets all the new features (as the API permits).   

Beniamino's patch uses new API from libnma 1.4.0, so that will require the nm-1-2 branch to branch-off from master.
The price for that is to not strictly depending on 1.4.0 API. Either at compile-time, or much preferably at runtime (dlopen seems an acceptable price, you can put a wrapper nma_vpn_password_dialog_set_expired_compat() into shared/nm-utils/nm-vpn-plugin-utils.h).



>> vpn-request: don't kill the auth-dialog upon request destroy
    
On another note, as nm-applet now no longer kills the plugin but sends the CANCEL method, how does this work for old auth-dialogs that don't handle this new message? Will they now hang indefinitely? I suspect, the plugin needs to communicate that it supports the new CANCEL method and accordingly kill or wait. The plugin could indicate whether it supports CANCEL via nm_vpn_editor_plugin_get_capabilities()).
Comment 26 Beniamino Galvani 2016-07-01 21:27:46 UTC
(In reply to Thomas Haller from comment #25)
> Beniamino's patch uses new API from libnma 1.4.0, so that will require the
> nm-1-2 branch to branch-off from master.
> The price for that is to not strictly depending on 1.4.0 API. Either at
> compile-time, or much preferably at runtime (dlopen seems an acceptable
> price, you can put a wrapper nma_vpn_password_dialog_set_expired_compat()
> into shared/nm-utils/nm-vpn-plugin-utils.h).

If the plan is to keep plugins and nm-applet at 1.2 when NM 1.4 gets
released, then dlopening is the only solution I can imagine.

> >> vpn-request: don't kill the auth-dialog upon request destroy
>     
> On another note, as nm-applet now no longer kills the plugin but sends the
> CANCEL method, how does this work for old auth-dialogs that don't handle
> this new message? Will they now hang indefinitely? I suspect, the plugin
> needs to communicate that it supports the new CANCEL method and accordingly
> kill or wait. The plugin could indicate whether it supports CANCEL via
> nm_vpn_editor_plugin_get_capabilities()).

This is the scenario we were trying to improve (1):
 - NM asks secret
 - (timeout)
 - auth-dialog stays active until user exits (no feedback to user)

So, we've modified NM to send CanceGetSecrets after a timeout. Now (2):
 - NM asks secret
 - (timeout)
 - NM sends CancelGetSecrets
 - request is destroyed, auth-dialog is killed

Then nm-applet was modified to send a CANCEL to auth-dialog (3):
 - NM asks secret
 - (timeout)
 - NM sends CancelGetSecrets
 - nm-applet sends CANCEL to auth-dialog
 - auth-dialog still doesn't understand it, no feedback to user, stays
   active

After the changes to nm-openvpn (4):
 - NM asks secret
 - (timeout)
 - NM sends CancelGetSecrets
 - nm-applet sends CANCEL to auth-dialog
 - auth-dialog understands CANCEL, shows a message and disable controls

Of course the optimal behavior is when NM, nm-applet and nm-openvpn
are all updated (4).

In case the auth-dialog doesn't understand CANCEL, we can either kill
it or let it stay active. Probably killing dialogs is not very good
from the UX point of view (as evidenced in comment 13), so I tend to
favor the latter option. This would be (3), not optimal but still
acceptable.

The problem is when NM is updated while the nm-applet is old (2). In
this case the dialog will disappear after the timeout. Honestly I have
no idea how to fix this; but maybe it's not a big issue now that the
timeout is much longer.
Comment 27 Beniamino Galvani 2016-07-07 12:40:20 UTC
(In reply to Beniamino Galvani from comment #26)
> (In reply to Thomas Haller from comment #25)
> > Beniamino's patch uses new API from libnma 1.4.0, so that will require the
> > nm-1-2 branch to branch-off from master.
> > The price for that is to not strictly depending on 1.4.0 API. Either at
> > compile-time, or much preferably at runtime (dlopen seems an acceptable
> > price, you can put a wrapper nma_vpn_password_dialog_set_expired_compat()
> > into shared/nm-utils/nm-vpn-plugin-utils.h).
> 
> If the plan is to keep plugins and nm-applet at 1.2 when NM 1.4 gets
> released, then dlopening is the only solution I can imagine.

(pushed a fixup to nm-openvpn bg/secret-timeout-bgo767321 branch to dynamically load the symbol if available).
Comment 28 Thomas Haller 2016-07-07 13:05:03 UTC
(In reply to Beniamino Galvani from comment #27)
> (In reply to Beniamino Galvani from comment #26)
> > (In reply to Thomas Haller from comment #25)
> > > Beniamino's patch uses new API from libnma 1.4.0, so that will require the
> > > nm-1-2 branch to branch-off from master.
> > > The price for that is to not strictly depending on 1.4.0 API. Either at
> > > compile-time, or much preferably at runtime (dlopen seems an acceptable
> > > price, you can put a wrapper nma_vpn_password_dialog_set_expired_compat()
> > > into shared/nm-utils/nm-vpn-plugin-utils.h).
> > 
> > If the plan is to keep plugins and nm-applet at 1.2 when NM 1.4 gets
> > released, then dlopening is the only solution I can imagine.
> 
> (pushed a fixup to nm-openvpn bg/secret-timeout-bgo767321 branch to
> dynamically load the symbol if available).

yeah something like that.

But why not add nma_vpn_password_dialog_set_expired_compat() to nm-vpn-plugin-utils.[hc] ?
Maybe, we could do:

gboolean
nma_vpn_password_dialog_set_expired_compat(...)
{
#ifdef NMA_HAVE_VPN_PASSWORD_DIALOG_SET_EXPIRED
    nma_vpn_password_dialog_set_expired (...);
#else
    handle = dlopen ();
    if (!handle)
        return FALSE;
    sym = dlsym (handle, nma_vpn_password_dialog_set_expired);
    if (!sym) {
        dlclose (handle);
        return FALSE;
    }
    sym (...);
#endif
    return TRUE;
}

yes, once you compile the plugin against nma-1.4.0, it will require the new version. But I think that is acceptable. If you compile against 1.2.0 and upgrade libnm, it will continue to work (indeed, it starts working). I think that's nice.



Anyway, let's commit changes to shared/nm-utils/nm-vpn-plugin-utils.[hc] to NetworkManager's repo first, and then copy over the entire file.
Comment 29 Beniamino Galvani 2016-07-07 13:27:53 UTC
(In reply to Thomas Haller from comment #28)
> 
> yeah something like that.
> 
> But why not add nma_vpn_password_dialog_set_expired_compat() to
> nm-vpn-plugin-utils.[hc] ?

Because then the file needs to include nma-vpn-password-dialog.h and I think there can be users of the file which don't have it in the include path. Maybe we need a new dedicated nma-compat.h file?

> yes, once you compile the plugin against nma-1.4.0, it will require the new
> version. But I think that is acceptable. If you compile against 1.2.0 and
> upgrade libnm, it will continue to work (indeed, it starts working). I think
> that's nice.

Sounds good.
Comment 30 Thomas Haller 2016-07-07 13:59:22 UTC
(In reply to Beniamino Galvani from comment #29)
> (In reply to Thomas Haller from comment #28)
> > 
> > yeah something like that.
> > 
> > But why not add nma_vpn_password_dialog_set_expired_compat() to
> > nm-vpn-plugin-utils.[hc] ?
> 
> Because then the file needs to include nma-vpn-password-dialog.h and I think
> there can be users of the file which don't have it in the include path.
> Maybe we need a new dedicated nma-compat.h file?

nma-vpn-password-dialog.h should either be already included via nm-default.h or it is not available.

Thus, the check #ifdef NMA_HAVE_VPN_PASSWORD_DIALOG_SET_EXPIRED nails it.
-- provided that nma-vpn-password-dialog.h does define a NMA_HAVE_VPN_PASSWORD_DIALOG_SET_EXPIRED

(unfortunately, libnma doesn't have version macros NMA_VERSION() like libnm has).
Comment 31 Tony Espy 2016-07-07 20:19:55 UTC
Just a quick note to point out that this has also been reported against Ubuntu Touch:

https://bugs.launchpad.net/ubuntu/+source/ubuntu-system-settings/+bug/1593686

...and may also be part of the problem with a more generic bug reported by BQ customers after we released out last update (OTA11):

https://bugs.launchpad.net/canonical-devices-system-image/+bug/1589489

At minimum, it looks like restoring the 2m timeout gets us back to par with the 0.9.10x behavior.

Note, I'll also make sure this gets reported generically against 16.04 Desktop, so we can get a SRU in the works.
Comment 32 Beniamino Galvani 2016-07-08 11:53:01 UTC
For the moment I've applied to master the first patch that increases the timeout to 120 seconds. This should solve most of the issues; the rest is a nice to have, but still work in progress.

https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=10c53528552d1f0df8c28326a8f0bd2a31b9fa46
Comment 33 André Klapper 2020-11-12 14:34:14 UTC
bugzilla.gnome.org is being shut down in favor of a GitLab instance. 
We are closing all old bug reports and feature requests in GNOME Bugzilla which have not seen updates for a long time.

If you still use NetworkManager and if you still see this bug / want this feature in a recent and supported version of NetworkManager, then please feel free to report it at https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/

Thank you for creating this report and we are sorry it could not be implemented (workforce and time is unfortunately limited).