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

Add support for various transports, and mechanism to collect connectivity report #137

Closed
wants to merge 52 commits into from

Conversation

amircybersec
Copy link
Contributor

@amircybersec amircybersec commented Dec 3, 2023

Connectivity Checker GUI Example App is extended to offer report collecting to a remote destination. One simple approach to set up a remote collector using Google Spreadsheet is discussed here.

CHANGELOG:

  1. Add a new report collector url field to the GUI - can be left blank/ optional (front-end)
  2. Add support for any transport and transport chain (e.g. split, socks5, tls, ss, etc)
  3. Sanitize ss:// credentials in report using hash digest(user info): e.g.: split:5|split:10|ss://8d52841cf9@104.238.100.16:65496
  4. Collect the connectivity test result (back-end)
  5. Include transport specific setup errors (e.g. split not supported over udp) in response error field
  6. Display error message on the GUI (front-end)
  7. Rename app to Transport Tester
  8. Added pop-up to show information on each field

TODO:

  • Improve text style in popup and hyperlink to docs
  • Add tags to errors, e.g.: transport config error, etc
Screenshot 2023-12-11 at 11 38 48 AM Screenshot 2023-12-05 at 3 12 55 PM Screenshot 2023-12-11 at 8 53 52 AM

@amircybersec amircybersec changed the title Add report collecting mechanism to GUI app with report package Revamp GUI connectivity tester app Dec 6, 2023
@amircybersec amircybersec marked this pull request as ready for review December 11, 2023 17:43
Copy link
Contributor

@daniellacosse daniellacosse left a comment

Choose a reason for hiding this comment

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

Do you mind editing the PR title to make it more specific? Something like "add transport support to connectivity test app"

// Observations
Time time.Time `json:"time"`
DurationMs int64 `json:"durationMs"`
Error *ConnectivityTestError `json:"error"`
}

func (r ConnectivityTestResult) IsSuccess() bool {
if r.Error == nil {
Copy link
Contributor

@daniellacosse daniellacosse Dec 11, 2023

Choose a reason for hiding this comment

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

Why not just return r.Error == nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is fixed

.popup {
display: none;
position: absolute;
z-index: 1000; /* A high value to ensure it's on top */
Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, this is why I recommended looking into the popover attribute. High z-indexes are a pretty major CSS code smell and are not scalable. Check it out: https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/popover

If you don't want to use popover, I can guide you to avoiding high z-indices, but it will take some iterations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@daniellacosse Thanks for the feedback. I got popover working on browser and iOS and desktop app but on Android it does not function as intended. Please see the screen shots below for Android and iOS. I suspect the WebView on Android (emulator pixel 7, API level 33) maybe using a Chrome version older than 114 which does not support popover. I am still investigating that theory. Please also let me know if you have any suggestions on how to best position the popover box on the screen.

iOS:
Screenshot 2023-12-12 at 12 27 19 PM

Android:
Screenshot 2023-12-11 at 9 50 01 PM

Copy link
Contributor

@daniellacosse daniellacosse Dec 13, 2023

Choose a reason for hiding this comment

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

You can use feature detection to hide the elements when popovers aren't supported.

With respect to positioning, I think you'll need the wrap the target and the popover in an element that's positioned relatively. Then position the popover element absolutely.

Alternatively we could just center the popover in the page and add a backdrop. That avoids having to write Javascript do deal with the intersection logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I managed to get popover working on all platforms.

The issue on Android was due to the fact that Capacitor uses WebView which is not automatically updated on Emulator. Once I logged into Google Play on Emulator and upgrade WebView manually it fixed the issue. I think this should work fine on most recent Android devices but If the device runs a very old version of Android API / WebView, the popover element won't work as expected.

Also, I observed that on iOS tapping outside of the popover window does not hide it as other platforms. This is not a big deal since I added a close button x to the popover window to offer an alternative to close it down.

Screenshot 2023-12-25 at 6 45 52 PM Screenshot 2023-12-25 at 8 21 34 PM

@@ -598,22 +600,24 @@ export class ConnectivityTestPage extends LitElement {
// TODO: move language definitions to a centralized place
return html`<main dir="${this.locale === "fa-IR" ? "rtl" : "ltr"}">
<header class=${this.platform?.operatingSystem === OperatingSystem.IOS ? "header--ios" : "header"}>
<h1 class="header-text">${msg("Outline Connectivity Test")}</h1>
<h1 class="header-text">${msg("Transport Tester")}</h1>
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're changing the title of the app, we'll need to change the folder name as well.

Why not Outline SDK Transport Tester?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea behind the name change was that with latest changes users can now test any of the supported transports in the SDK addition to ss/outline. Maybe we can drop SDK and call it Outline Transport Tester?

We can even keep the previous name. I think it will better to keep the title short.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's drop "Outline", since this is not an official Outline product. "Connectivity Tester" is probably more user-friendly than "Transport Tester".

Copy link
Contributor

@daniellacosse daniellacosse Dec 13, 2023

Choose a reason for hiding this comment

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

Are we sure we want to drop "Outline"? I know it's not an official product, but a) it's free advertising and b) people will want to know the tool they're using.

I'm okay with shortening it to Connectivity Tester if we have a link somewhere explaining what the SDK is. Keeps the name short but serves the same goal of driving people to the SDK.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. Perhaps we add a line somewhere, as a subtitle or perhaps a footer, saying Powered by the Outline SDK? We could link to it too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked through the branding guideline and used the Powered By Outline SVG logo at the bottom of the page. This way we can keep the title short and still attribute to Outline SDK. I am fine with either Transport Tester or Connectivity Tester for the title, but I think the former rhymes better :)

Screenshot 2023-12-25 at 6 45 52 PM

@amircybersec amircybersec changed the title Revamp GUI connectivity tester app Add support for various transports, and mechanism to collect connectivity report Dec 12, 2023
@amircybersec
Copy link
Contributor Author

converting to draft as it needs rebase & some refactoring

amircybersec and others added 27 commits January 21, 2024 16:56
Co-authored-by: Vinicius Fortuna <fortuna@users.noreply.github.com>
Co-authored-by: Vinicius Fortuna <fortuna@users.noreply.github.com>
Co-authored-by: Vinicius Fortuna <fortuna@users.noreply.github.com>
Explain what can be done with the SDK, expand integration documentation and create command line tools section.
Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.7.0 to 0.17.0.
- [Commits](golang/crypto@v0.7.0...v0.17.0)

---
updated-dependencies:
- dependency-name: golang.org/x/crypto
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.14.0 to 0.17.0.
- [Commits](golang/crypto@v0.14.0...v0.17.0)

---
updated-dependencies:
- dependency-name: golang.org/x/crypto
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.14.0 to 0.17.0.
- [Commits](golang/crypto@v0.14.0...v0.17.0)

---
updated-dependencies:
- dependency-name: golang.org/x/crypto
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [golang.org/x/net](https://github.com/golang/net) from 0.10.0 to 0.17.0.
- [Commits](golang/net@v0.10.0...v0.17.0)

---
updated-dependencies:
- dependency-name: golang.org/x/net
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>
Let me know if you'd like more detail.
@amircybersec
Copy link
Contributor Author

I had to rebase and refactor this PR and it ended up getting a bit messy. I decided to create a new branch off of main and related cherry pick commits into it. The result is a new PR #170. I am going to close this PR as a result.

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

Successfully merging this pull request may close these issues.

4 participants