D17834: Allow the use of One-Time Password

[Jan Grulich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R116:b8f580a749f8: Allow the use of  One-Time Password 
(authored by enriquem, committed by jgrulich).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D17834?vs=48852&id=48856#toc

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17834?vs=48852&id=48856

REVISION DETAIL
  https://phabricator.kde.org/D17834

AFFECTED FILES
  vpn/fortisslvpn/fortisslvpnadvanced.ui
  vpn/fortisslvpn/fortisslvpnauth.cpp
  vpn/fortisslvpn/fortisslvpnauth.ui
  vpn/fortisslvpn/fortisslvpnwidget.cpp
  vpn/fortisslvpn/nm-fortisslvpn-service.h

To: enriquem, jgrulich
Cc: plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17834: Allow the use of One-Time Password

[Enrique Melendez]

enriquem updated this revision to Diff 48852.
enriquem added a comment.


  Now it builds.
  
  I tested it to write and read from the config file, interchangeably with 
nm-connection-editor,

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17834?vs=48760&id=48852

REVISION DETAIL
  https://phabricator.kde.org/D17834

AFFECTED FILES
  vpn/fortisslvpn/fortisslvpnadvanced.ui
  vpn/fortisslvpn/fortisslvpnauth.cpp
  vpn/fortisslvpn/fortisslvpnauth.ui
  vpn/fortisslvpn/fortisslvpnwidget.cpp
  vpn/fortisslvpn/nm-fortisslvpn-service.h

To: enriquem, jgrulich
Cc: plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17834: Allow the use of One-Time Password

[Jan Grulich]

jgrulich added a comment.


  It doesn't build now, can you please fix that?

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D17834

To: enriquem, jgrulich
Cc: plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17834: Allow the use of One-Time Password

[Enrique Melendez]

enriquem added inline comments.

INLINE COMMENTS

> jgrulich wrote in fortisslvpnwidget.cpp:201
> I don't understand why you hare check for prevData.value(), but later on you 
> get optFlag from data (not prevData). Still I don't think this is needed at 
> all. I would go back and use your previous approach, because the result will 
> be the same.

You are probably right. Networkmanager-fortisslvpn is probably drawing code 
form other VPN plugins where there can be more flags set. Here, as you say, the 
only relevant flag is NotSaved.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D17834

To: enriquem, jgrulich
Cc: plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17834: Allow the use of One-Time Password

[Enrique Melendez]

enriquem updated this revision to Diff 48760.
enriquem added a comment.


  I implemented all of your comments, reverting to my previous approach

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17834?vs=48750&id=48760

REVISION DETAIL
  https://phabricator.kde.org/D17834

AFFECTED FILES
  vpn/fortisslvpn/fortisslvpnadvanced.ui
  vpn/fortisslvpn/fortisslvpnauth.cpp
  vpn/fortisslvpn/fortisslvpnauth.ui
  vpn/fortisslvpn/fortisslvpnwidget.cpp
  vpn/fortisslvpn/nm-fortisslvpn-service.h

To: enriquem, jgrulich
Cc: plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17834: Allow the use of One-Time Password

[Jan Grulich]

jgrulich added inline comments.

INLINE COMMENTS

> fortisslvpnauth.cpp:44
> +const NetworkManager::Setting::SecretFlags otpFlag = 
> static_cast(data.value(NM_FORTISSLVPN_KEY_OTP"-flags").toInt());
> +if (otpFlag == NetworkManager::Setting::NotSaved){
> +d->ui.otpFrame->setVisible(true);

Missing space before "{".

> fortisslvpnauth.cpp:47
> +} else {
> +d->ui.otpFrame->setVisible(false);
> +}

You can make this shorter:
d->ui.otpFrame->setVisible(otpFlag == NetworkManager::Setting::NotSaved)

> fortisslvpnauth.cpp:69
> +
> +if (!data.value(NM_FORTISSLVPN_KEY_OTP"-flags").isEmpty()){
> +const NetworkManager::Setting::SecretFlags otpFlag = 
> static_cast(data.value(NM_FORTISSLVPN_KEY_OTP"-flags").toInt());

Missing space before "{".

> fortisslvpnauth.cpp:71
> +const NetworkManager::Setting::SecretFlags otpFlag = 
> static_cast(data.value(NM_FORTISSLVPN_KEY_OTP"-flags").toInt());
> +if (otpFlag == NetworkManager::Setting::NotSaved && 
> !d->ui.otp->text().isEmpty()){
> +
> secrets.insert(QLatin1String(NM_FORTISSLVPN_KEY_OTP),d->ui.otp->text());

Missing space before "{".

> fortisslvpnauth.cpp:72
> +if (otpFlag == NetworkManager::Setting::NotSaved && 
> !d->ui.otp->text().isEmpty()){
> +
> secrets.insert(QLatin1String(NM_FORTISSLVPN_KEY_OTP),d->ui.otp->text());
> +}

Missing space after second parameter.

> fortisslvpnwidget.cpp:131
>  
> +if (!data.value(NM_FORTISSLVPN_KEY_OTP"-flags").isEmpty()){
> +const NetworkManager::Setting::SecretFlags otpFlag = 
> static_cast(data.value(NM_FORTISSLVPN_KEY_OTP"-flags").toInt());

Missing space before "{".

> fortisslvpnwidget.cpp:133
> +const NetworkManager::Setting::SecretFlags otpFlag = 
> static_cast(data.value(NM_FORTISSLVPN_KEY_OTP"-flags").toInt());
> +if (otpFlag & NetworkManager::Setting::NotSaved){
> +d->advUi.otp->setChecked(true);

Missing space before "{".

> fortisslvpnwidget.cpp:201
> +const NMStringMap prevData = d->setting->data();
> +if (!prevData.value(NM_FORTISSLVPN_KEY_OTP"-flags").isEmpty()){
> +const NetworkManager::Setting::SecretFlags otpFlag = 
> static_cast(data.value(NM_FORTISSLVPN_KEY_OTP"-flags").toInt());

Missing space before "{".

> fortisslvpnwidget.cpp:201
> +const NMStringMap prevData = d->setting->data();
> +if (!prevData.value(NM_FORTISSLVPN_KEY_OTP"-flags").isEmpty()){
> +const NetworkManager::Setting::SecretFlags otpFlag = 
> static_cast(data.value(NM_FORTISSLVPN_KEY_OTP"-flags").toInt());

I don't understand why you hare check for prevData.value(), but later on you 
get optFlag from data (not prevData). Still I don't think this is needed at 
all. I would go back and use your previous approach, because the result will be 
the same.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D17834

To: enriquem, jgrulich
Cc: plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17834: Allow the use of One-Time Password

[Enrique Melendez]

enriquem updated this revision to Diff 48750.
enriquem added a comment.


  Treat otp-flags the same way NetworkManager-fortisslvpn does

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17834?vs=48744&id=48750

REVISION DETAIL
  https://phabricator.kde.org/D17834

AFFECTED FILES
  vpn/fortisslvpn/fortisslvpnadvanced.ui
  vpn/fortisslvpn/fortisslvpnauth.cpp
  vpn/fortisslvpn/fortisslvpnauth.ui
  vpn/fortisslvpn/fortisslvpnwidget.cpp
  vpn/fortisslvpn/nm-fortisslvpn-service.h

To: enriquem, jgrulich
Cc: plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17834: Allow the use of One-Time Password

[Enrique Melendez]

enriquem added a comment.


  >> I see. Maybe do not set it at all if one-time password is not used and let 
NM to set the default value? That way we won't be using a wrong value in case 
this changes in future.
  > 
  > Fair enough. I will update the diff with this. However, judging from the 
current behavior, if we do not set the value, nothing will be added to the 
config file. No harm will be made, though.
  
  Actually, this is not what the gnome applet does. Per 
https://github.com/GNOME/NetworkManager-fortisslvpn/blob/master/properties/nm-fortisslvpn-editor.c
 what they do is set the flags via bit-wise operations. I will update the diff 
with this behaviour
  
  Please, be aware that I cannot compile the code now, so there may be errors. 
I expect your understanding.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D17834

To: enriquem, jgrulich
Cc: plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17834: Allow the use of One-Time Password

[Enrique Melendez]

enriquem updated this revision to Diff 48744.
enriquem added a comment.


  Removed setting the otp-flag in case it is not needed for the connection

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17834?vs=48701&id=48744

REVISION DETAIL
  https://phabricator.kde.org/D17834

AFFECTED FILES
  vpn/fortisslvpn/fortisslvpnadvanced.ui
  vpn/fortisslvpn/fortisslvpnauth.cpp
  vpn/fortisslvpn/fortisslvpnauth.ui
  vpn/fortisslvpn/fortisslvpnwidget.cpp
  vpn/fortisslvpn/nm-fortisslvpn-service.h

To: enriquem, jgrulich
Cc: plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17834: Allow the use of One-Time Password

[Enrique Melendez]

enriquem added a comment.


  > I see. Maybe do not set it at all if one-time password is not used and let 
NM to set the default value? That way we won't be using a wrong value in case 
this changes in future.
  
  Fair enough. I will update the diff with this. However, judging from the 
current behavior, if we do not set the value, nothing will be added to the 
config file. No harm will be made, though.

INLINE COMMENTS

> jgrulich wrote in fortisslvpnwidget.cpp:204
> Shouldn't it be NetworkManager::Setting::NotRequired? Using 
> NetworkManager::Setting::None says the one-time password should be saved to 
> NetworkManager.

I don't think so; I created a new VPN setting wit nm-connection-editor, and it 
sets it to 0, that is, to NetworkManager::Setting::None

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D17834

To: enriquem, jgrulich
Cc: plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17834: Allow the use of One-Time Password

[Jan Grulich]

jgrulich added a comment.


  In D17834#386573 , @enriquem wrote:
  
  > I implemented all of your comments except one. I created a new VPN setting 
wit nm-connection-editor, and it sets it to 0, that is, to 
NetworkManager::Setting::None. Thus, I believe that part is correct.
  
  
  I see. Maybe do not set it at all if one-time password is not used and let NM 
to set the default value? That way we won't be using a wrong value in case this 
changes in future.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D17834

To: enriquem, jgrulich
Cc: plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17834: Allow the use of One-Time Password

[Enrique Melendez]

enriquem updated this revision to Diff 48701.
enriquem added a comment.


  I implemented all of your comments except one. I created a new VPN setting 
wit nm-connection-editor, and it sets it to 0, that is, to 
NetworkManager::Setting::None. Thus, I believe that part is correct.

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17834?vs=48614&id=48701

REVISION DETAIL
  https://phabricator.kde.org/D17834

AFFECTED FILES
  vpn/fortisslvpn/fortisslvpnadvanced.ui
  vpn/fortisslvpn/fortisslvpnauth.cpp
  vpn/fortisslvpn/fortisslvpnauth.ui
  vpn/fortisslvpn/fortisslvpnwidget.cpp
  vpn/fortisslvpn/nm-fortisslvpn-service.h

To: enriquem, jgrulich
Cc: plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17834: Allow the use of One-Time Password

[Jan Grulich]

jgrulich added inline comments.

INLINE COMMENTS

> fortisslvpnauth.cpp:44
> +const NetworkManager::Setting::SecretFlags otpFlag = 
> static_cast(data.value(NM_FORTISSLVPN_KEY_OTP"-flags").toInt());
> +if (otpFlag == NetworkManager::Setting::None){
> +d->ui.otpFrame->setVisible(false);

I think you should check for NetworkManager::Setting::NotSaved only, if it's 
this case, make it visible, if not, make it hidden. It's a one-time password so 
it won't be saved.

> fortisslvpnauth.cpp:46
> +d->ui.otpFrame->setVisible(false);
> +}
> +else {

Coding style.

> fortisslvpnwidget.cpp:202
> +data.insert(QLatin1String(NM_FORTISSLVPN_KEY_OTP"-flags"), 
> QString::number(NetworkManager::Setting::NotSaved);
> +}
> +else {

Coding style.

> fortisslvpnwidget.cpp:204
> +else {
> +data.insert(QLatin1String(NM_FORTISSLVPN_KEY_OTP"-flags"), 
> QString::number(NetworkManager::Setting::None);
> +}

Shouldn't it be NetworkManager::Setting::NotRequired? Using 
NetworkManager::Setting::None says the one-time password should be saved to 
NetworkManager.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D17834

To: enriquem, jgrulich
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17834: Allow the use of One-Time Password

[Enrique Melendez]

enriquem updated this revision to Diff 48614.
enriquem added a comment.


  Treated otp-flags the same way as password-flags. But we have to be careful 
to consider the case where the option is not yet in the config file (as will 
certainly be the case the firs time it is used if there was a vpn already 
configured), so I added a isEmpty() test not to break the toInt() call.

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17834?vs=48397&id=48614

REVISION DETAIL
  https://phabricator.kde.org/D17834

AFFECTED FILES
  vpn/fortisslvpn/fortisslvpnadvanced.ui
  vpn/fortisslvpn/fortisslvpnauth.cpp
  vpn/fortisslvpn/fortisslvpnauth.ui
  vpn/fortisslvpn/fortisslvpnwidget.cpp
  vpn/fortisslvpn/nm-fortisslvpn-service.h

To: enriquem, jgrulich
Cc: plasma-devel, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D17834: Allow the use of One-Time Password

[Jan Grulich]

jgrulich added a comment.


  Can you please work with "otp-flags" same way we work with "password-flags"? 
You work with it as with string, it would be more readable if you use 
NetworkManager::Setting::SecretFlags.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D17834

To: enriquem, jgrulich
Cc: plasma-devel, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D17834: Allow the use of One-Time Password

[Enrique Melendez]

enriquem updated this revision to Diff 48397.
enriquem edited the summary of this revision.

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17834?vs=48328&id=48397

REVISION DETAIL
  https://phabricator.kde.org/D17834

AFFECTED FILES
  vpn/fortisslvpn/fortisslvpnadvanced.ui
  vpn/fortisslvpn/fortisslvpnauth.cpp
  vpn/fortisslvpn/fortisslvpnauth.ui
  vpn/fortisslvpn/fortisslvpnwidget.cpp
  vpn/fortisslvpn/nm-fortisslvpn-service.h

To: enriquem, jgrulich
Cc: plasma-devel, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D17834: Allow the use of One-Time Password

[Enrique Melendez]

enriquem added a comment.


  You are, of course, correct. How can I have missed that?

REVISION DETAIL
  https://phabricator.kde.org/D17834

To: enriquem, jgrulich
Cc: plasma-devel, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D17834: Allow the use of One-Time Password

[Enrique Melendez]

enriquem updated this revision to Diff 48328.
enriquem edited the summary of this revision.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17834?vs=48289&id=48328

REVISION DETAIL
  https://phabricator.kde.org/D17834

AFFECTED FILES
  vpn/fortisslvpn/fortisslvpnadvanced.ui
  vpn/fortisslvpn/fortisslvpnauth.cpp
  vpn/fortisslvpn/fortisslvpnauth.ui
  vpn/fortisslvpn/fortisslvpnwidget.cpp
  vpn/fortisslvpn/nm-fortisslvpn-service.h

To: enriquem, jgrulich
Cc: plasma-devel, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D17834: Allow the use of One-Time Password

[Jan Grulich]

jgrulich added a comment.


  I don't think this is a complete support for this. You need to add support 
into the auth-dialog as well, without it the connection would expect an otp 
password, but there will be no way how to provide it.
  
  See 
https://github.com/GNOME/network-manager-fortisslvpn/blob/master/auth-dialog/main.c
 for how it's done in Gnome.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D17834

To: enriquem, jgrulich
Cc: plasma-devel, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D17834: Allow the use of One-Time Password

[Enrique Melendez]

enriquem created this revision.
enriquem added a reviewer: jgrulich.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
enriquem requested review of this revision.

REVISION SUMMARY
  NetworkManager-fortisslvpn allows the user to use a One-Time Password 
challenge, but this option has not been included in the plasma applet. This 
change makes allowance for that setting, changing the "Advanced" config dialog 
to include a checkBox to that effect.
  
  The layout has been taken from the NetworkManager "Advanced" config dialog.
  
  The change has been tested for writing to and reading from the config file, 
but should still be fully tested

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D17834

AFFECTED FILES
  vpn/fortisslvpn/fortisslvpnadvanced.ui
  vpn/fortisslvpn/fortisslvpnwidget.cpp
  vpn/fortisslvpn/nm-fortisslvpn-service.h

To: enriquem, jgrulich
Cc: plasma-devel, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart