[sailfish-secrets] Ensure user input dialog timeout before request. Contributes to JB#44199

[dcaliste]

Introduce a timeout attribute to interaction parameters. Use this timeout value in password plugin to ensure that dialog is dismissed before the request itself timeout. This avoid the request to return an empty user input with a successful result while the dialog itself is still being displayed.

Solve issue #155

@chriadam there is still a possible issue with inconsistent hard coded timeout values: in secretdaemonconnection.cpp:createInterface() (/resp./ in crypto) the timeout is fixed to 3 minutes, which is fine for the default timeout of 2 minutes, but can create issues in my opinion if user sets up a longer timeout. The problem is that this timeout is set for the manager itself, so for all requests. I don't think it can be a good idea to change the timeout after creation when we want to manage a user input with a longer timeout. A possible solution would be to fix timeouts as a constant somewhere in secret API and use this constant in password plugin. What do you think ?

[inzanity]

Not commenting on whether this is a feasible change or not, but do note that added properties are not automatically passed over D-Bus, they need to be changed in serialization.cpp; and such changes should be reflected in D-Bus interface introspection data, and any changes there should also be adapted onto the C API side.

If the timeout should only be passed between daemon and plugins, it is not necessary to touch the D-Bus interfaces.

[chriadam]

I think specifying the timeout which the manager uses as some MaxTimeout value and exposing that in the API, makes sense. Then adding the timeout()/setTimeout() for the user makes sense, so long as we document that values over MaxTimeout will be ignored (e.g. have setTimeout(int seconds) { m_timeout = qMin(MaxTimeout, seconds); }

Also as inzanity mentioned, will need to update the C API etc.

[dcaliste]

Thank you @inzanity for your reminder. I've updated the first commit accordingly. Tell me if that's enough, particularly from the C API. It seems to me that InteractionParameters class is not wraped directly, but is just present in the xml file describing the API for DBus. I've added a i there.

@chriadam I'm proposing a second commit on top of the initial one, modifying the timeout setting per request in the Secret manager. I'm not convinced that it's a good choice. For requests without user interaction, I've used the default DBus value, for request with explicit interaction parameters, I've used the timeout chosen by the user. For the other requests that may use a built-in interaction parameter (read not set up by the user, but hard coded in the daemon), I'm using the default timeout value.

This is not translated into Crypto part. Indeed, trying to transfer it, I encounter the fact that InteractionParameters is duplicated (I know why), and didn't want necessary to transpose all Secret changes without discussion. And particularly to the fact that, even without actual user interaction, action may take time, more than the DBus default of 25 seconds, e.g. generation of random data. And some actions that don't take InteractionParameters as arguments may indeed create one from implementation. I'm thinking to the sign operation. In normal operation, it should not require a user interaction to unlock the key, because this is done in the fetch of the key. But for GnuPG plugin, the fetch is not unlocking the key, so the sign operation will take input from user to unlock the key… So when reading the implementation in cryptomanager.cpp, I didn't see any way to know which operation will actually require to wait for user interaction. So letting timeout at a hardcoded value like defaultTimeout() may work…

…or not: this is not taking into account possible operation time. Worst case scenario:

• the user choose a timeout of 3min ;
• manager set the timeout to 3min and launch the DBus call ;
• the operation starts by generating random data and show the user interface at T0+2min ;
• the user is distracted and takes more than one minute to enter is passphrase. The issue #155 is then happening. The request will timeout but the password dialog will stay there for an additional two minutes before timeouting itself…

I don't know if you're still following me, but I'm afraid, correcting #155 is more complicated than just synchronizing timeouts as done here. I currently don't see how the client can signal the daemon, that OK, I give up, trash the request and clean pending operations.

[inzanity]

The xml change is correct, but you'll need to add similar change in the second occurrence too, and the embedded xml in daemon/SecretsImpl/secrets_p.h and lib/Secrets/interactionservice_p.h. Also the C implementation needs to be changed, adding the i to the signatures where appropriate, and a value for it. Personally I'd suggest adding a sf_secrets_manager_set_user_interaction_timeout() and use the value wherever needed.

[dcaliste]

As discussed with @chriadam on IRC today, we've decided to postpone the addition of client-side timeout. @inzanity thank you very much for detailed explanations, I'll use it nonetheless on a later MR.

This MR is now, just a quick and dirty fix for #155 ensuring that the password dialog timeout is lower than the request timeout.

I'm investigating with @chriadam advices how the daemon can notify the password plugin that the client has disconnected already and withdraw the dialog accordingly. Some news to come later today or this week in a new MR.

Then, I can reintroduce this timeout attribute from client-side in a third MR.

[chriadam]

LGTM. Merging.