Allow the use of One-Time Password
ClosedPublic

Authored by enriquem on Dec 28 2018, 4:54 PM.

Details

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

EDIT: I added support in the auth dialog
EDIT2: Added a minor tweak in the auth dialog

Diff Detail

Repository
R116 Plasma Network Management Applet
Lint
Lint Skipped
Unit
Unit Tests Skipped
enriquem created this revision.Dec 28 2018, 4:54 PM
Restricted Application added a project: Plasma. · View Herald TranscriptDec 28 2018, 4:54 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
enriquem requested review of this revision.Dec 28 2018, 4:54 PM

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.

enriquem updated this revision to Diff 48328.Dec 28 2018, 11:19 PM
enriquem edited the summary of this revision. (Show Details)

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

enriquem updated this revision to Diff 48397.Dec 30 2018, 1:53 PM
enriquem edited the summary of this revision. (Show Details)

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.

enriquem updated this revision to Diff 48614.Jan 3 2019, 5:56 PM

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.

jgrulich added inline comments.Jan 4 2019, 7:57 AM
vpn/fortisslvpn/fortisslvpnauth.cpp
44

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.

46

Coding style.

vpn/fortisslvpn/fortisslvpnwidget.cpp
203

Coding style.

205

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

enriquem updated this revision to Diff 48701.Jan 4 2019, 9:23 PM

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 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.

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.

vpn/fortisslvpn/fortisslvpnwidget.cpp
205

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

enriquem updated this revision to Diff 48744.Jan 5 2019, 4:18 PM

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

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.

enriquem updated this revision to Diff 48750.Jan 5 2019, 5:09 PM

Treat otp-flags the same way NetworkManager-fortisslvpn does

jgrulich added inline comments.Jan 5 2019, 6:28 PM
vpn/fortisslvpn/fortisslvpnauth.cpp
44

Missing space before "{".

47

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

70

Missing space before "{".

72

Missing space before "{".

73

Missing space after second parameter.

vpn/fortisslvpn/fortisslvpnwidget.cpp
131

Missing space before "{".

133

Missing space before "{".

201

Missing space before "{".

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.

enriquem added inline comments.Jan 5 2019, 7:03 PM
vpn/fortisslvpn/fortisslvpnwidget.cpp
201

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.

enriquem updated this revision to Diff 48760.Jan 5 2019, 7:04 PM

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

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

enriquem updated this revision to Diff 48852.Jan 7 2019, 10:47 AM

Now it builds.

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

jgrulich accepted this revision.Jan 7 2019, 11:45 AM
This revision is now accepted and ready to land.Jan 7 2019, 11:45 AM
This revision was automatically updated to reflect the committed changes.