refactor(client/windows): move fetching logic from TypeScript to Go #2220
Conversation
| @@ -98,4 +98,7 @@ export interface VpnApi { | |||
|
|
|||
| /** Sets a listener, to be called when the tunnel status changes. */ | |||
| onStatusChange(listener: (id: string, status: TunnelStatus) => void): void; | |||
|
|
|||
| /** Fetches the config from a dynamic key URL. */ | |||
| fetchDynamicConfig(url: string): Promise<string>; | |||
There was a problem hiding this comment.
This is not part of the VPN API. Please remove. We need to put it somewhere else.
There was a problem hiding this comment.
How about putting it in nativeHelper?
There was a problem hiding this comment.
We already have the concept of OutlinePlatform: https://github.com/Jigsaw-Code/outline-apps/blob/master/client/src/www/app/platform.ts
I think we need to add something like a getUrlFetcher()?
There was a problem hiding this comment.
Done, added getResourceFetcher.
There was a problem hiding this comment.
This file should be reverted, since there's no change needed for the VPN tunnel.
client/electron/go_vpn_tunnel.ts
Outdated
| * @throws ProcessTerminatedExitCodeError if tun2socks failed to run successfully. | ||
| */ | ||
| checkConnectivity() { | ||
| async checkConnectivity(config: TransportConfigJson): Promise<boolean> { |
There was a problem hiding this comment.
Can you move this to a standalone function?
There's no real dependency on internal state, only this.process, which you can replace with new ChildProcessHelper(pathToEmbeddedTun2socksBinary()).
client/go/outline/dynamic_key.go
Outdated
| // | ||
| // The function makes an HTTP GET request to the specified URL and returns the response body as a | ||
| // string. If the request fails or the server returns a non-2xx status code, an error is returned. | ||
| func FetchDynamicKey(url string) *FetchDynamicKeyResult { |
There was a problem hiding this comment.
Call it FetchUrl?
Is there anything specific to dynamic keys?
There was a problem hiding this comment.
Nope. Updated to FetchResource
client/electron/index.ts
Outdated
| @@ -498,6 +501,12 @@ function main() { | |||
| mainWindow?.webContents.send('outline-ipc-push-clipboard'); | |||
| }); | |||
|
|
|||
| // Fetches dynamic key config from a remote URL. | |||
| ipcMain.handle( | |||
| 'outline-ipc-fetch-config', | |||
There was a problem hiding this comment.
Perhaps rename this to outline-ipc-fetch-resource to align with the function name and avoid confusion.
You'll need to change in the caller as well.
| // | ||
| // The function makes an HTTP GET request to the specified URL and returns the response body as a | ||
| // string. If the request fails or the server returns a non-2xx status code, an error is returned. | ||
| func FetchResource(url string) *FetchResourceResult { |
There was a problem hiding this comment.
No need for this PR, but in the future we will likely want a FetchResourceRequest, so we can add other parameters, like a certificate fingerprint.
| /** | ||
| * Fetches resources using Electron's IPC to communicate with the main process. | ||
| */ | ||
| export class ElectronResourceFetcher implements ResourceFetcher { |
There was a problem hiding this comment.
window.electron doesn't exist in Cordova. This shouldn't be in a file that Cordova loads.
You can define it in the electron main.ts or create a separate file in the electron code.
There was a problem hiding this comment.
This looks great. Thanks for the cleanup of udp connectivity test too.
We now have a place to improve the fetching, opening up the possibility to use proxyless solutions in case of blocking, or use certificate fingerprint to fetch from IP locations with self-signed certificates. That will in turn allow the server management API to serve dynamic keys, for example
|
Perhaps the title needs to be updated to Electron instead of Windows? |
We removed that in #2082 . |
New Fetch Error