Discussion:
Review Request 122012: Add support for storing Openconnect secrets to KWallet
Jan Grulich
2015-01-12 11:31:43 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122012/
-----------------------------------------------------------

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


Repository: plasma-nm


Description
-------

This patch adds support for storing secrets to KWallet from Openconnect auth dialog, which is the only part where secrets could be stored. It should behave similar to nm-openconnect, which means that all secrets which are OC_FORM_OPT_PASSWORD are stored to KWallet or they are not stored at all. I know I should avoid using synchronous calls when opening KWallet, but in this case it would be probably complicated to use it asynchronously.


Diffs
-----


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


Testing
-------


Thanks,

Jan Grulich
Jan Grulich
2015-01-12 11:35:06 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122012/
-----------------------------------------------------------

(Updated Led. 12, 2015, 11:35 dop.)


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


Changes
-------

Add diff, it previously failed to load it.


Repository: plasma-nm


Description
-------

This patch adds support for storing secrets to KWallet from Openconnect auth dialog, which is the only part where secrets could be stored. It should behave similar to nm-openconnect, which means that all secrets which are OC_FORM_OPT_PASSWORD are stored to KWallet or they are not stored at all. I know I should avoid using synchronous calls when opening KWallet, but in this case it would be probably complicated to use it asynchronously.


Diffs (updated)
-----

kded/passworddialog.cpp 96cc789
libs/editor/connectiondetaileditor.cpp ee9bced
libs/editor/widgets/settingwidget.h 8d07b73
libs/editor/widgets/settingwidget.cpp c876e79
vpn/openconnect/openconnectauth.cpp ebce2be
vpn/openconnect/openconnectwidget.cpp f9bc3cf

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


Testing
-------


Thanks,

Jan Grulich
Lukáš Tinkl
2015-01-12 12:30:25 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122012/#review73841
-----------------------------------------------------------

Ship it!


Ship It!

- Lukáš Tinkl
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/122012/
-----------------------------------------------------------
(Updated Led. 12, 2015, 12:35 odp.)
Review request for Network Management, Lukáš Tinkl and Lamarque Souza.
Repository: plasma-nm
Description
-------
This patch adds support for storing secrets to KWallet from Openconnect auth dialog, which is the only part where secrets could be stored. It should behave similar to nm-openconnect, which means that all secrets which are OC_FORM_OPT_PASSWORD are stored to KWallet or they are not stored at all. I know I should avoid using synchronous calls when opening KWallet, but in this case it would be probably complicated to use it asynchronously.
Diffs
-----
kded/passworddialog.cpp 96cc789
libs/editor/connectiondetaileditor.cpp ee9bced
libs/editor/widgets/settingwidget.h 8d07b73
libs/editor/widgets/settingwidget.cpp c876e79
vpn/openconnect/openconnectauth.cpp ebce2be
vpn/openconnect/openconnectwidget.cpp f9bc3cf
Diff: https://git.reviewboard.kde.org/r/122012/diff/
Testing
-------
Thanks,
Jan Grulich
Lamarque Souza
2015-01-12 12:50:05 UTC
Permalink
Post by Jan Grulich
Post by Lukáš Tinkl
Ship It!
The big problem with using synchronous API is blocking the entire Plasma Desktop sometimes. That is very annoying from user point of view.


- Lamarque


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122012/#review73841
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/122012/
-----------------------------------------------------------
(Updated Jan. 12, 2015, 11:35 a.m.)
Review request for Network Management, Lukáš Tinkl and Lamarque Souza.
Repository: plasma-nm
Description
-------
This patch adds support for storing secrets to KWallet from Openconnect auth dialog, which is the only part where secrets could be stored. It should behave similar to nm-openconnect, which means that all secrets which are OC_FORM_OPT_PASSWORD are stored to KWallet or they are not stored at all. I know I should avoid using synchronous calls when opening KWallet, but in this case it would be probably complicated to use it asynchronously.
Diffs
-----
kded/passworddialog.cpp 96cc789
libs/editor/connectiondetaileditor.cpp ee9bced
libs/editor/widgets/settingwidget.h 8d07b73
libs/editor/widgets/settingwidget.cpp c876e79
vpn/openconnect/openconnectauth.cpp ebce2be
vpn/openconnect/openconnectwidget.cpp f9bc3cf
Diff: https://git.reviewboard.kde.org/r/122012/diff/
Testing
-------
Thanks,
Jan Grulich
Jan Grulich
2015-01-12 12:55:58 UTC
Permalink
Post by Lamarque Souza
Post by Lukáš Tinkl
Ship It!
The big problem with using synchronous API is blocking the entire Plasma Desktop sometimes. That is very annoying from user point of view.
It shouldn't block plasma-desktop or plasmashell in case of Plasma 5. This is called either from our kded module or from the connection editor.


- Jan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122012/#review73841
-----------------------------------------------------------
Post by Lamarque Souza
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/122012/
-----------------------------------------------------------
(Updated Led. 12, 2015, 11:35 dop.)
Review request for Network Management, Lukáš Tinkl and Lamarque Souza.
Repository: plasma-nm
Description
-------
This patch adds support for storing secrets to KWallet from Openconnect auth dialog, which is the only part where secrets could be stored. It should behave similar to nm-openconnect, which means that all secrets which are OC_FORM_OPT_PASSWORD are stored to KWallet or they are not stored at all. I know I should avoid using synchronous calls when opening KWallet, but in this case it would be probably complicated to use it asynchronously.
Diffs
-----
kded/passworddialog.cpp 96cc789
libs/editor/connectiondetaileditor.cpp ee9bced
libs/editor/widgets/settingwidget.h 8d07b73
libs/editor/widgets/settingwidget.cpp c876e79
vpn/openconnect/openconnectauth.cpp ebce2be
vpn/openconnect/openconnectwidget.cpp f9bc3cf
Diff: https://git.reviewboard.kde.org/r/122012/diff/
Testing
-------
Thanks,
Jan Grulich
Lamarque Souza
2015-01-12 13:04:21 UTC
Permalink
Post by Lamarque Souza
Post by Lukáš Tinkl
Ship It!
The big problem with using synchronous API is blocking the entire Plasma Desktop sometimes. That is very annoying from user point of view.
It shouldn't block plasma-desktop or plasmashell in case of Plasma 5. This is called either from our kded module or from the connection editor.
blocking a kded module is what indirectly blocks plasma shell. If any plasma applet does a sync call to kded then the entire plasma shell will be blocked if kded is blocked waiting for the wallet to be opened. I just do not know if there is any Plasma 5 applet doing sync calls to kded5. If there is none then this patch is good to go.


- Lamarque


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122012/#review73841
-----------------------------------------------------------
Post by Lamarque Souza
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/122012/
-----------------------------------------------------------
(Updated Jan. 12, 2015, 11:35 a.m.)
Review request for Network Management, Lukáš Tinkl and Lamarque Souza.
Repository: plasma-nm
Description
-------
This patch adds support for storing secrets to KWallet from Openconnect auth dialog, which is the only part where secrets could be stored. It should behave similar to nm-openconnect, which means that all secrets which are OC_FORM_OPT_PASSWORD are stored to KWallet or they are not stored at all. I know I should avoid using synchronous calls when opening KWallet, but in this case it would be probably complicated to use it asynchronously.
Diffs
-----
kded/passworddialog.cpp 96cc789
libs/editor/connectiondetaileditor.cpp ee9bced
libs/editor/widgets/settingwidget.h 8d07b73
libs/editor/widgets/settingwidget.cpp c876e79
vpn/openconnect/openconnectauth.cpp ebce2be
vpn/openconnect/openconnectwidget.cpp f9bc3cf
Diff: https://git.reviewboard.kde.org/r/122012/diff/
Testing
-------
Thanks,
Jan Grulich
Jan Grulich
2015-01-12 13:25:00 UTC
Permalink
Post by Lamarque Souza
Post by Lukáš Tinkl
Ship It!
The big problem with using synchronous API is blocking the entire Plasma Desktop sometimes. That is very annoying from user point of view.
It shouldn't block plasma-desktop or plasmashell in case of Plasma 5. This is called either from our kded module or from the connection editor.
blocking a kded module is what indirectly blocks plasma shell. If any plasma applet does a sync call to kded then the entire plasma shell will be blocked if kded is blocked waiting for the wallet to be opened. I just do not know if there is any Plasma 5 applet doing sync calls to kded5. If there is none then this patch is good to go.
I'll try to make the call in kded asynchronous, it should be doable given that this action is independent and doesn't cooperate with NM.


- Jan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122012/#review73841
-----------------------------------------------------------
Post by Lamarque Souza
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/122012/
-----------------------------------------------------------
(Updated Led. 12, 2015, 11:35 dop.)
Review request for Network Management, Lukáš Tinkl and Lamarque Souza.
Repository: plasma-nm
Description
-------
This patch adds support for storing secrets to KWallet from Openconnect auth dialog, which is the only part where secrets could be stored. It should behave similar to nm-openconnect, which means that all secrets which are OC_FORM_OPT_PASSWORD are stored to KWallet or they are not stored at all. I know I should avoid using synchronous calls when opening KWallet, but in this case it would be probably complicated to use it asynchronously.
Diffs
-----
kded/passworddialog.cpp 96cc789
libs/editor/connectiondetaileditor.cpp ee9bced
libs/editor/widgets/settingwidget.h 8d07b73
libs/editor/widgets/settingwidget.cpp c876e79
vpn/openconnect/openconnectauth.cpp ebce2be
vpn/openconnect/openconnectwidget.cpp f9bc3cf
Diff: https://git.reviewboard.kde.org/r/122012/diff/
Testing
-------
Thanks,
Jan Grulich
Lamarque Souza
2015-01-12 12:48:22 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122012/#review73845
-----------------------------------------------------------



vpn/openconnect/openconnectauth.cpp
<https://git.reviewboard.kde.org/r/122012/#comment51348>

I must delete this pointer after use, otherwise there will be a memory leak.



vpn/openconnect/openconnectwidget.cpp
<https://git.reviewboard.kde.org/r/122012/#comment51349>

Same here.


- Lamarque Souza
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/122012/
-----------------------------------------------------------
(Updated Jan. 12, 2015, 11:35 a.m.)
Review request for Network Management, Lukáš Tinkl and Lamarque Souza.
Repository: plasma-nm
Description
-------
This patch adds support for storing secrets to KWallet from Openconnect auth dialog, which is the only part where secrets could be stored. It should behave similar to nm-openconnect, which means that all secrets which are OC_FORM_OPT_PASSWORD are stored to KWallet or they are not stored at all. I know I should avoid using synchronous calls when opening KWallet, but in this case it would be probably complicated to use it asynchronously.
Diffs
-----
kded/passworddialog.cpp 96cc789
libs/editor/connectiondetaileditor.cpp ee9bced
libs/editor/widgets/settingwidget.h 8d07b73
libs/editor/widgets/settingwidget.cpp c876e79
vpn/openconnect/openconnectauth.cpp ebce2be
vpn/openconnect/openconnectwidget.cpp f9bc3cf
Diff: https://git.reviewboard.kde.org/r/122012/diff/
Testing
-------
Thanks,
Jan Grulich
Jan Grulich
2015-01-12 14:21:59 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122012/
-----------------------------------------------------------

(Updated Led. 12, 2015, 2:21 odp.)


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


Bugs: 309931 and 334474
http://bugs.kde.org/show_bug.cgi?id=309931
http://bugs.kde.org/show_bug.cgi?id=334474


Repository: plasma-nm


Description
-------

This patch adds support for storing secrets to KWallet from Openconnect auth dialog, which is the only part where secrets could be stored. It should behave similar to nm-openconnect, which means that all secrets which are OC_FORM_OPT_PASSWORD are stored to KWallet or they are not stored at all. I know I should avoid using synchronous calls when opening KWallet, but in this case it would be probably complicated to use it asynchronously.


Diffs
-----

kded/passworddialog.cpp 96cc789
libs/editor/connectiondetaileditor.cpp ee9bced
libs/editor/widgets/settingwidget.h 8d07b73
libs/editor/widgets/settingwidget.cpp c876e79
vpn/openconnect/openconnectauth.cpp ebce2be
vpn/openconnect/openconnectwidget.cpp f9bc3cf

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


Testing
-------


Thanks,

Jan Grulich
Jan Grulich
2015-01-13 14:43:39 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122012/
-----------------------------------------------------------

(Updated Led. 13, 2015, 2:43 odp.)


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


Changes
-------

Update diff. I've found another approach which should be more simple and actually uses NM to handle storing secrets into KWallet.


Bugs: 309931 and 334474
http://bugs.kde.org/show_bug.cgi?id=309931
http://bugs.kde.org/show_bug.cgi?id=334474


Repository: plasma-nm


Description
-------

This patch adds support for storing secrets to KWallet from Openconnect auth dialog, which is the only part where secrets could be stored. It should behave similar to nm-openconnect, which means that all secrets which are OC_FORM_OPT_PASSWORD are stored to KWallet or they are not stored at all. I know I should avoid using synchronous calls when opening KWallet, but in this case it would be probably complicated to use it asynchronously.


Diffs (updated)
-----

kded/secretagent.cpp 67e929f
vpn/openconnect/openconnectauth.cpp ebce2be
vpn/openconnect/openconnectwidget.cpp f9bc3cf

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


Testing
-------


Thanks,

Jan Grulich
Lukáš Tinkl
2015-01-13 14:54:25 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122012/#review73940
-----------------------------------------------------------

Ship it!


Since the blocking KWallet code is gone, +1 from me

- Lukáš Tinkl
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/122012/
-----------------------------------------------------------
(Updated Led. 13, 2015, 3:43 odp.)
Review request for Network Management, Lukáš Tinkl and Lamarque Souza.
Bugs: 309931 and 334474
http://bugs.kde.org/show_bug.cgi?id=309931
http://bugs.kde.org/show_bug.cgi?id=334474
Repository: plasma-nm
Description
-------
This patch adds support for storing secrets to KWallet from Openconnect auth dialog, which is the only part where secrets could be stored. It should behave similar to nm-openconnect, which means that all secrets which are OC_FORM_OPT_PASSWORD are stored to KWallet or they are not stored at all. I know I should avoid using synchronous calls when opening KWallet, but in this case it would be probably complicated to use it asynchronously.
Diffs
-----
kded/secretagent.cpp 67e929f
vpn/openconnect/openconnectauth.cpp ebce2be
vpn/openconnect/openconnectwidget.cpp f9bc3cf
Diff: https://git.reviewboard.kde.org/r/122012/diff/
Testing
-------
Thanks,
Jan Grulich
Lamarque Souza
2015-01-13 15:24:59 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122012/#review73945
-----------------------------------------------------------

Ship it!


Ship It!

- Lamarque Souza
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/122012/
-----------------------------------------------------------
(Updated Jan. 13, 2015, 2:43 p.m.)
Review request for Network Management, Lukáš Tinkl and Lamarque Souza.
Bugs: 309931 and 334474
http://bugs.kde.org/show_bug.cgi?id=309931
http://bugs.kde.org/show_bug.cgi?id=334474
Repository: plasma-nm
Description
-------
This patch adds support for storing secrets to KWallet from Openconnect auth dialog, which is the only part where secrets could be stored. It should behave similar to nm-openconnect, which means that all secrets which are OC_FORM_OPT_PASSWORD are stored to KWallet or they are not stored at all. I know I should avoid using synchronous calls when opening KWallet, but in this case it would be probably complicated to use it asynchronously.
Diffs
-----
kded/secretagent.cpp 67e929f
vpn/openconnect/openconnectauth.cpp ebce2be
vpn/openconnect/openconnectwidget.cpp f9bc3cf
Diff: https://git.reviewboard.kde.org/r/122012/diff/
Testing
-------
Thanks,
Jan Grulich
Jan Grulich
2015-01-13 15:29:29 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122012/
-----------------------------------------------------------

(Updated Jan. 13, 2015, 3:29 p.m.)


Status
------

This change has been marked as submitted.


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


Bugs: 309931 and 334474
http://bugs.kde.org/show_bug.cgi?id=309931
http://bugs.kde.org/show_bug.cgi?id=334474


Repository: plasma-nm


Description
-------

This patch adds support for storing secrets to KWallet from Openconnect auth dialog, which is the only part where secrets could be stored. It should behave similar to nm-openconnect, which means that all secrets which are OC_FORM_OPT_PASSWORD are stored to KWallet or they are not stored at all. I know I should avoid using synchronous calls when opening KWallet, but in this case it would be probably complicated to use it asynchronously.


Diffs
-----

kded/secretagent.cpp 67e929f
vpn/openconnect/openconnectauth.cpp ebce2be
vpn/openconnect/openconnectwidget.cpp f9bc3cf

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


Testing
-------


Thanks,

Jan Grulich

Loading...