Skip to content

VPN Panel Redesign #208

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
danirabbit merged 38 commits into from
Oct 30, 2019
Merged

VPN Panel Redesign #208

danirabbit merged 38 commits into from
Oct 30, 2019

Conversation

@arshubham
Copy link
Collaborator

@arshubham arshubham commented Oct 27, 2019 •
edited
Loading

Fixes #139 , Fixes #126 , Fixes #122 Fixes #121 , Fixes #67, Fixes #66 , Fixes #59 , Fixes #54 , Fixes #52 , Fixes #46 , Fixes #44 , Fixes #40 , Fixes #33 , Fixes #7

image

image

@arshubham arshubham mentioned this pull request Oct 27, 2019
Closed
@arshubham arshubham marked this pull request as ready for review October 28, 2019 11:05
@tintou
Copy link
Member

tintou commented Oct 28, 2019

I was going to do a proper review but it looks like all what you did in get_key_group_username (and similar ones) was replaced in GNOME Control Center here GNOME/gnome-control-center@aabc162#diff-6cde69db28e078281497952725f10e3a it might be worth investigate a proper way to do this instead of relying on string comparisons :)

@arshubham
Copy link
Collaborator Author

arshubham commented Oct 28, 2019 •
edited
Loading

@tintou I looked into it. Gnome now doesn't show any VPN details at all. I can't find of any other why to so this, without some string comparisons since each VPN type produces different key value pairs. For now in this PR, I just preserved the existing functionality.

PPTP:

gateway => uk.pptpvpn.net
lcp-echo-failure => 5
refuse-chap => yes
user => listvpn.net-qwerty123
require-mppe => yes
password-flags => 0
lcp-echo-interval => 30

Open VPN

ta => /home/shubhamarora/.cert/nm-openvpn/ca.protonvpn.com.udp-tls-auth.pem
dev => tun
ca => /home/shubhamarora/.cert/nm-openvpn/ca.protonvpn.com.udp-ca.pem
remote-cert-tls => server
username => arshubhampm
mssfix => 1450
tunnel-mtu => 1500
reneg-seconds => 0
comp-lzo => no-by-default
cipher => AES-256-CBC
remote => ca.protonvpn.com:80, ca.protonvpn.com:443, ca.protonvpn.com:4569, ca.protonvpn.com:1194, ca.protonvpn.com:5060
password-flags => 1
auth => SHA512
connection-type => password
ta-dir => 1
remote-random => yes

L2TP

password-flags => 0
user => listvpn.net-qwerty1234
gateway => us3.listvpn.net

I can see two alternatives, here

  1. Support only the most famous/used protocols. (OpenVPN, PPTP, L2TP and Wiregurard) and only show status and VPN type for others.
  2. Print all info from the connection.get_setting_vpn () as seen above in a the existing format.

Thoughts?

danirabbit
danirabbit requested changes Oct 28, 2019
View reviewed changes
Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking really good so far! Nice work. I have a few comments here :)

donadigo
donadigo requested changes Oct 28, 2019
View reviewed changes
Copy link
Collaborator

@donadigo donadigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's my couple of comments.

tintou
tintou reviewed Oct 28, 2019
View reviewed changes
tintou
tintou reviewed Oct 28, 2019
View reviewed changes
danirabbit
danirabbit reviewed Oct 28, 2019
View reviewed changes
@danirabbit
Copy link
Member

danirabbit commented Oct 28, 2019 •
edited
Loading

I have a branch that uses Granite.MessageDialog here: #209

Otherwise, seems good from a UX perspective 👍

danirabbit
danirabbit reviewed Oct 28, 2019
View reviewed changes
danirabbit
danirabbit requested changes Oct 28, 2019
View reviewed changes
danirabbit
danirabbit previously approved these changes Oct 29, 2019
View reviewed changes
Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some outstanding little niggles to me like handling the item removal with undo, but I feel like this is a good point to start and we can always iterate. There's a lot of improvements here

@danirabbit danirabbit requested a review from tintou October 29, 2019 16:34
donadigo
donadigo approved these changes Oct 29, 2019
View reviewed changes
Copy link
Collaborator

@donadigo donadigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good.

@danirabbit danirabbit merged commit 72854b5 into master Oct 30, 2019
@danirabbit danirabbit deleted the vpn-redesign branch October 30, 2019 16:56
@peteruithoven peteruithoven mentioned this pull request Oct 30, 2019
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@donadigo donadigo donadigo approved these changes

@danirabbit danirabbit danirabbit left review comments

@cassidyjames cassidyjames Awaiting requested review from cassidyjames

@tintou tintou Awaiting requested review from tintou

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

6 participants

@arshubham @tintou @danirabbit @cassidyjames @donadigo