Review Request 120313: Do not keep trying to open KWallet if we failed before

Discussion:

Jan Grulich

2014-09-22 08:52:41 UTC

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120313/
-----------------------------------------------------------

Review request for Network Management, Luk?? Tinkl and Lamarque Souza.

Repository: plasma-nm

Description
-------

In the secret agent, if any of functions for processing secrets fails to open KWallet, then m_wallet will be always set to 0, which leads again to another attempt to open KWallet. We should check if opening of KWallet failed before and do not try to open it again, which should be done in my patch.

BUG: https://bugs.kde.org/show_bug.cgi?id=339223

Diffs
-----

kded/secretagent.h a438191
kded/secretagent.cpp 77438ca

Diff: https://git.reviewboard.kde.org/r/120313/diff/

Testing
-------

Thanks,

Jan Grulich

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20140922/c434302c/attachment.html>

Lukáš Tinkl

2014-09-22 11:36:14 UTC

Permalink

Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/120313/
-----------------------------------------------------------
(Updated Z??. 22, 2014, 10:52 dop.)
Review request for Network Management, Luk?? Tinkl and Lamarque Souza.
Repository: plasma-nm
Description
-------
In the secret agent, if any of functions for processing secrets fails to open KWallet, then m_wallet will be always set to 0, which leads again to another attempt to open KWallet. We should check if opening of KWallet failed before and do not try to open it again, which should be done in my patch.
BUG: https://bugs.kde.org/show_bug.cgi?id=339223
Diffs
-----
kded/secretagent.h a438191
kded/secretagent.cpp 77438ca
Diff: https://git.reviewboard.kde.org/r/120313/diff/
Testing
-------
Thanks,
Jan Grulich

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20140922/7899b072/attachment.html>

Jan Grulich

2014-09-22 11:38:36 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120313/
-----------------------------------------------------------

(Updated Sept. 22, 2014, 11:38 a.m.)

Status
------

This change has been marked as submitted.

Review request for Network Management, Luk?? Tinkl and Lamarque Souza.

Repository: plasma-nm

Description
-------

In the secret agent, if any of functions for processing secrets fails to open KWallet, then m_wallet will be always set to 0, which leads again to another attempt to open KWallet. We should check if opening of KWallet failed before and do not try to open it again, which should be done in my patch.

BUG: https://bugs.kde.org/show_bug.cgi?id=339223

Diffs
-----

kded/secretagent.h a438191
kded/secretagent.cpp 77438ca

Diff: https://git.reviewboard.kde.org/r/120313/diff/

Testing
-------

Thanks,

Jan Grulich

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20140922/332b896d/attachment.html>

Lamarque Souza

2014-09-22 11:40:09 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120313/#review67182
-----------------------------------------------------------

kded/secretagent.cpp
<https://git.reviewboard.kde.org/r/120313/#comment46882>

Why not just do the following in this line?

int i = 0;
while (i < m_calls.size()) {
SecretsRequest &request = m_calls[i];
sendError(SecretAgent::InternalError, QLatin1String("KWallet could not be opened"), request.message);
m_calls.removeAt(i);
++i;
}
walletClosed();

I do not see a reason to keep requests in m_calls if they cannot be processed and we should show user what is causing requests to fail. Besides, if kwallet is only temporarily failing your implementation will prevent the secret agent from trying to open wallet until kded is restarted.

- Lamarque Souza

Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/120313/
-----------------------------------------------------------
(Updated Sept. 22, 2014, 11:38 a.m.)
Review request for Network Management, Luk?? Tinkl and Lamarque Souza.
Repository: plasma-nm
Description
-------
In the secret agent, if any of functions for processing secrets fails to open KWallet, then m_wallet will be always set to 0, which leads again to another attempt to open KWallet. We should check if opening of KWallet failed before and do not try to open it again, which should be done in my patch.
BUG: https://bugs.kde.org/show_bug.cgi?id=339223
Diffs
-----
kded/secretagent.h a438191
kded/secretagent.cpp 77438ca
Diff: https://git.reviewboard.kde.org/r/120313/diff/
Testing
-------
Thanks,
Jan Grulich

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20140922/844630a8/attachment.html>

Jan Grulich

2014-09-22 11:45:34 UTC

Permalink

Post by Jan Grulich

kded/secretagent.cpp, line 259
<https://git.reviewboard.kde.org/r/120313/diff/1/?file=314383#file314383line259>
Why not just do the following in this line?
int i = 0;
while (i < m_calls.size()) {
SecretsRequest &request = m_calls[i];
sendError(SecretAgent::InternalError, QLatin1String("KWallet could not be opened"), request.message);
m_calls.removeAt(i);
++i;
}
walletClosed();
I do not see a reason to keep requests in m_calls if they cannot be processed and we should show user what is causing requests to fail. Besides, if kwallet is only temporarily failing your implementation will prevent the secret agent from trying to open wallet until kded is restarted.

Check the pushed fix, the patch looks different here on reviewboard and it's quite confusing. All m_calls will be processed, because once I know that KWallet failed to be open before, then I'm not trying to open it again and continue. In case of processGetSecrets it will show a password dialog. Also the openKWalletFailed flag is set again to false after I use it so it doesn't prevent from trying to open KWallet again.

- Jan

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120313/#review67182
-----------------------------------------------------------

Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/120313/
-----------------------------------------------------------
(Updated Z??. 22, 2014, 11:38 dop.)
Review request for Network Management, Luk?? Tinkl and Lamarque Souza.
Repository: plasma-nm
Description
-------
In the secret agent, if any of functions for processing secrets fails to open KWallet, then m_wallet will be always set to 0, which leads again to another attempt to open KWallet. We should check if opening of KWallet failed before and do not try to open it again, which should be done in my patch.
BUG: https://bugs.kde.org/show_bug.cgi?id=339223
Diffs
-----
kded/secretagent.h a438191
kded/secretagent.cpp 77438ca
Diff: https://git.reviewboard.kde.org/r/120313/diff/
Testing
-------
Thanks,
Jan Grulich

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20140922/f94396af/attachment-0001.html>

Lamarque Souza

2014-09-22 12:18:35 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120313/#review67189
-----------------------------------------------------------

kded/secretagent.cpp
<https://git.reviewboard.kde.org/r/120313/#comment46886>

Ok, your patch works. However, in case of processGetSecrets() it will show a password dialog *for every request* that needs secrets, which I do not think users are going to like.

Anyway, we should warn the user that kwallet is not working or he/she will just cancel the password dialog and try again. Think that most kwallet users use it so they do not need to remember their passwords. Just showing a password dialog when kwallet fails is not a good usability action in my opinion.

- Lamarque Souza

Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/120313/
-----------------------------------------------------------
(Updated Sept. 22, 2014, 11:38 a.m.)
Review request for Network Management, Luk?? Tinkl and Lamarque Souza.
Repository: plasma-nm
Description
-------
In the secret agent, if any of functions for processing secrets fails to open KWallet, then m_wallet will be always set to 0, which leads again to another attempt to open KWallet. We should check if opening of KWallet failed before and do not try to open it again, which should be done in my patch.
BUG: https://bugs.kde.org/show_bug.cgi?id=339223
Diffs
-----
kded/secretagent.h a438191
kded/secretagent.cpp 77438ca
Diff: https://git.reviewboard.kde.org/r/120313/diff/
Testing
-------
Thanks,
Jan Grulich

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20140922/1382b606/attachment.html>

Jan Grulich

2014-09-22 12:38:18 UTC

Permalink

Post by Jan Grulich

kded/secretagent.cpp, line 259
<https://git.reviewboard.kde.org/r/120313/diff/1/?file=314383#file314383line259>
Ok, your patch works. However, in case of processGetSecrets() it will show a password dialog *for every request* that needs secrets, which I do not think users are going to like.
Anyway, we should warn the user that kwallet is not working or he/she will just cancel the password dialog and try again. Think that most kwallet users use it so they do not need to remember their passwords. Just showing a password dialog when kwallet fails is not a good usability action in my opinion.

Why for every request? It should show a password dialog only when we fail to open KWallet. In case that KWallet is opened successfuly it should get secrets correctly and not continue with showing a dialog.

- Jan

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120313/#review67189
-----------------------------------------------------------

Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/120313/
-----------------------------------------------------------
(Updated Z??. 22, 2014, 11:38 dop.)
Review request for Network Management, Luk?? Tinkl and Lamarque Souza.
Repository: plasma-nm
Description
-------
In the secret agent, if any of functions for processing secrets fails to open KWallet, then m_wallet will be always set to 0, which leads again to another attempt to open KWallet. We should check if opening of KWallet failed before and do not try to open it again, which should be done in my patch.
BUG: https://bugs.kde.org/show_bug.cgi?id=339223
Diffs
-----
kded/secretagent.h a438191
kded/secretagent.cpp 77438ca
Diff: https://git.reviewboard.kde.org/r/120313/diff/
Testing
-------
Thanks,
Jan Grulich

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20140922/29b3561a/attachment.html>

Lamarque Souza

2014-09-22 13:03:12 UTC

Permalink

Post by Jan Grulich

Ok, I see that processGetSecrets() never opens a second dialog, so the dialog is not a issue. I am still not comfortable in just showing a password when kwallet fails to open.

- Lamarque

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120313/#review67189
-----------------------------------------------------------

Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/120313/
-----------------------------------------------------------
(Updated Sept. 22, 2014, 11:38 a.m.)
Review request for Network Management, Luk?? Tinkl and Lamarque Souza.
Repository: plasma-nm
Description
-------
In the secret agent, if any of functions for processing secrets fails to open KWallet, then m_wallet will be always set to 0, which leads again to another attempt to open KWallet. We should check if opening of KWallet failed before and do not try to open it again, which should be done in my patch.
BUG: https://bugs.kde.org/show_bug.cgi?id=339223
Diffs
-----
kded/secretagent.h a438191
kded/secretagent.cpp 77438ca
Diff: https://git.reviewboard.kde.org/r/120313/diff/
Testing
-------
Thanks,
Jan Grulich

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20140922/f21d2a3c/attachment.html>

Jan Grulich

2014-09-22 13:06:06 UTC

Permalink

Post by Jan Grulich

Why for every request? It should show a password dialog only when we fail to open KWallet. In case that KWallet is opened successfuly it should get secrets correctly and not continue with showing a dialog.
Ok, I see that processGetSecrets() never opens a second dialog, so the dialog is not a issue. I am still not comfortable in just showing a password when kwallet fails to open.

I agree, but how do you want to inform users about that? We could use either notification or print some error into logs.

- Jan

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120313/#review67189
-----------------------------------------------------------

Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/120313/
-----------------------------------------------------------
(Updated Z??. 22, 2014, 11:38 dop.)
Review request for Network Management, Luk?? Tinkl and Lamarque Souza.
Repository: plasma-nm
Description
-------
In the secret agent, if any of functions for processing secrets fails to open KWallet, then m_wallet will be always set to 0, which leads again to another attempt to open KWallet. We should check if opening of KWallet failed before and do not try to open it again, which should be done in my patch.
BUG: https://bugs.kde.org/show_bug.cgi?id=339223
Diffs
-----
kded/secretagent.h a438191
kded/secretagent.cpp 77438ca
Diff: https://git.reviewboard.kde.org/r/120313/diff/
Testing
-------
Thanks,
Jan Grulich

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20140922/fa432335/attachment-0001.html>