Skip to content
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.

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

Support custom ports #269

Closed
jurajhilje opened this issue Jul 8, 2022 · 11 comments · Fixed by #270
Closed

Support custom ports #269

jurajhilje opened this issue Jul 8, 2022 · 11 comments · Fixed by #270
Assignees
Milestone

Comments

@jurajhilje
Copy link
Member

In addition to existing port selection, we need to add a custom input field that accepts a single port number within the supported ranges. Supported port ranges are fetched from the servers.json API, separately for OpenVPN and WireGuard.

@jurajhilje jurajhilje added this to the 2.7.0 milestone Jul 8, 2022
@jurajhilje jurajhilje self-assigned this Jul 8, 2022
@jurajhilje jurajhilje mentioned this issue Jul 14, 2022
6 tasks
@jurajhilje jurajhilje assigned gorkapernas and unassigned jurajhilje Jul 21, 2022
@jurajhilje
Copy link
Member Author

@gorkapernas Available in 2.6.4 (13) (staging API).

@gorkapernas
Copy link
Member

@jurajhilje some early observations:

  • When entering invalid data or random data, the validation is missing, nothing happens. Please add a validation for such cases.
  • Tapping on "UDP", below "Add custom port", selects the first port on the list (UDP 53). Nothing should happen when tapping on this (unless we are planning to open a selection dropdown if TCP protocol would be available as a custom port)

@jurajhilje jurajhilje modified the milestones: 2.7.0, 2.8.0 Sep 9, 2022
@jurajhilje jurajhilje modified the milestones: 2.8.0, 2.7.0 Nov 9, 2022
@jurajhilje jurajhilje assigned gorkapernas and unassigned jurajhilje Nov 9, 2022
@jurajhilje
Copy link
Member Author

@gorkapernas Available in 2.7.0 (6), production API.

@gorkapernas
Copy link
Member

Verified on version 2.7.0 (6), @jurajhilje please see my comments below.

  • It is possible to add multiple times the same port, please note that on the desktop apps this is restricted, so it is suggested to add a validation in order to prevent the user from adding the same port more than once. Tested with OpenVPN and WireGuard, UDP and TCP. See screenshot below.

multiple instances of the same port

  • On the desktop apps when adding a new port, this will appear on both protocols, i.e. if I add a new port to OpenVPN (UDP), this same port will be available for WireGuard and vice versa. Do we want to have the same implementation on iOS?

  • It is possible to add non numeric characters to the port input field, it is suggested to restrict the input to numbers only.


Question:
Currently the new ports are displayed at the end of the list according the last added without any type of sorting rules, consequently the list could end up mixing UDP and TCP ports and higher and lower values. Ideally we should have some type of sorting rule in order to avoid confusion, however, if we do so:

  1. Should we sort all ports or only custom ports?
  2. Sorting rule: by type (UDP or TCP) and then by a port number (lower to higher) inside the type?

This also should be implemented in the desktop apps //Cc @stenya

@jurajhilje
Copy link
Member Author

@gorkapernas I would keep the logic from the current Desktop app regarding sorting, it's more clear when you have your custom ports at the end of the list. I'll also update the iOS app to add the same UDP port to both protocols.

@jurajhilje
Copy link
Member Author

@gorkapernas New TF build 2.7.0 (8) is available.

@jurajhilje jurajhilje assigned gorkapernas and unassigned jurajhilje Nov 11, 2022
@gorkapernas
Copy link
Member

Verified on 2.7.0(10), the issues reported have been fixed.
@jurajhilje I have one note, it seems like the iOS app does not remember the port after switching protocols, it always resets to default when changing protocols, this is not the case in the desktop apps.
Do you want to keep the actual implementation or should we align it with the desktop apps' implementation to keep consistency?

@jurajhilje
Copy link
Member Author

@gorkapernas I would like to update this to have the same behaviour as our Desktop app. Will submit that in the next build.

@jurajhilje
Copy link
Member Author

jurajhilje commented Nov 15, 2022

@gorkapernas This last change (persist port number and UDP/TCP when switching VPN protocols) will require more significant update in the code than I initially planned. Can you please open a new ticket? So we can plan that update in one of the next releases.
Thanks!

@gorkapernas
Copy link
Member

@jurajhilje Done -> #294

FYI, I'll run a few more tests before moving this ticket to deployable.

@jurajhilje jurajhilje assigned gorkapernas and unassigned jurajhilje Nov 17, 2022
@gorkapernas
Copy link
Member

@jurajhilje this looks good to go.
Tested on latest TF version 2.7.0 (12), iPhone XR iOS 16.2, iPad 6 iOS 15.7.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants