Persist servers by access key #988

alalamav · 2021-03-08T19:50:50Z

The motivation behind this is to lay the groundwork for a single persistence format in the context of dynamic access keys (online config):

Changes the server persistence format to {accessKey, name}, from ShadowsocksConfig.
Implements unit tests for the data storage migration.
Updates the Tunnel interface not to own a ShadowsocksConfig.
Removes ServerConfig, PersitentServer and PersitentServerFactory.
Moves the ShadowsocksConfig definition out of model.

…omparison; fix alreadyadded error message

src/www/app/outline_server.ts

src/www/infrastructure/memory_storage.ts

src/www/app/config.ts

fortuna · 2021-03-12T20:47:59Z

src/www/app/outline_server.ts

+    return this.serverById.get(serverId);
+  }
+
+  add(accessKey: string, serverName: string) {


Let's make add not take the serverName. I'd like this to be aligned with the product.
It's ok to have unnamed servers. The application/ui code should have the logic: if server has a name, use it, otherwise use the default name. This is the same approach we use for access keys in the manager.

In order to support Outline/Shadowsocks Server, you'll need to expose a getter to differentiate them.

Note that the access key may have a name, so we can leverage that for the fake servers.

Done in 76bde1c. I also changed the persistence format to encode the name. In order to support default (outlne/non-outline) server name, I added an extra field to ShadowsocksConfig.

fortuna · 2021-03-12T20:48:47Z

src/www/app/electron_main.ts

-  getPersistentServerFactory: () => {
-    return (serverId: string, config: ServerConfig, eventQueue: EventQueue) => {
+  getServerFactory: () => {
+    return (serverId: string, serverName: string, config: ShadowsocksConfig,


Revert serverName. See my other comment for explanation.

Done in 76bde1c.

fortuna

Looks good. I think we can tweak the storage format, but all the remaining changes are localized there.

fortuna · 2021-03-17T21:31:55Z

src/www/app/app.ts

@@ -543,8 +523,18 @@ export class App {
    this.rootEl.changePage(this.rootEl.DEFAULT_PAGE);
  }

+  // Returns the server's name, if present, or a default server name.
+  private getServerName(server: Server): string {


I'd name this getServerDisplayName(server), so it's clearer it's the name for display.

Done in 7a6d05e.

fortuna · 2021-03-17T21:43:45Z

src/www/app/outline_server.ts

  }

  set name(newName: string) {
    this.config.name = newName;
  }

-  get host() {
-    return this.config.host;
+  get accessKey() {


Where is this used? Delete if not used.

OutlineServer needs to expose the access key so the repo can find servers by access key to prevent adding duplicate servers.

fortuna · 2021-03-17T21:48:15Z

src/www/app/outline_server.ts

+      public readonly createServer: OutlineServerFactory, private eventQueue: events.EventQueue,
+      private storage: Storage) {
+    try {
+      migrateServerStorageToV1(this.storage);


There's no need to explicitly migrate if there were no changes.
Instead, just call loadServers(). loadServers() will have to try v1 and fallback to v0 forever anyway.

Then, on storeServers(), you write the new format when change happens.

Done in bc68e17.

src/www/app/outline_server.ts

fortuna · 2021-03-17T21:54:30Z

src/www/app/outline_server.ts

    return cipher !== undefined && OutlineServer.SUPPORTED_CIPHERS.includes(cipher);
  }
 }
+
+// DEPRECATED: V0 server persistence format.
+export interface ConfigById {


Perhaps name them ServersStorageV0 and ServersStorageV1?

Done in bc68e17.

fortuna · 2021-03-17T22:08:20Z

src/www/app/outline_server.ts

+}
+
+// V1 server persistence format.
+export interface AccessKeyById {


A dictionary is not a very convenient format, because the type is weird. It's not a fixed schema, since you have variable fields. It's better to use a list.

Also, it seems the access key is not all we need.

I think we need something like:

type AccessKey = string; interface ServerJson { id: string name?: string accessKey: AccessKey // device key for connection. Whatever you get from the permanent redirects. sharedAccessKey?: AccessKey // multi-user key for sharing }

accessKey could be name connectionAccessKey.
sharedAccessKey could have other names, such as originalAccessKey.

The is_outline bit could be derived from the sharedAccessKey, or accessKey if absent.

The localStorage format under servers_v1 would be ServerJson[].

Nice suggestion. Done in bc68e17 (without multi-user keys). I also updated the storage migration unit tests.

fortuna · 2021-03-17T22:08:46Z

src/www/app/config.ts

@@ -19,4 +19,5 @@ export interface ShadowsocksConfig {
  password?: string;
  method?: string;
  name?: string;
+  extra?: {[key: string]: string};


Let's remove this. See my comments on the storage format.

Done in bc68e17. We now preserve the original access key and inject it into OutlineServer.

fortuna · 2021-03-18T13:22:14Z

src/www/app/outline_server.ts

-    for (const serverId in configByAccessKey) {
-      if (configByAccessKey.hasOwnProperty(serverId)) {
-        const accessKey = configByAccessKey[serverId];
+    for (const serverId in configById) {


No need to change, but I think for (const serverId of Object.keys(configById)) is the right way to do this. It removes the need for the hasOwnProperty below, I believe.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/keys

Done in 3e18673.

fortuna · 2021-03-18T13:27:55Z

src/www/app/outline_server.ts

+    }
+  }
+
+  private loadServer(serverId: string, accessKey: string, config: ShadowsocksConfig) {


Change loadServer to take one parameter: OutlineServerJson.
It will simplify the loadServer methods.
Also, if you can't load the server from the OutlineServerJson, something is wrong. That will make it clearer what is needed to load a server.

Done in 3e18673.

src/www/app/outline_server.ts

fortuna · 2021-03-18T13:54:43Z

src/www/app/outline_server.ts

@@ -31,8 +31,9 @@ export class OutlineServer implements Server {
  errorMessageId?: string;

  constructor(
-      public readonly id: string, private readonly config: ShadowsocksConfig,
-      private tunnel: Tunnel, private eventQueue: events.EventQueue) {
+      public readonly id: string, public readonly accessKey: string,


It's confusing to have both the accessKey and the config.
Can't we have the same paramters as OutlineServerJson?
Pass id, name, and accessKey (besides tunnel and eventQueue).
We need to change the server factory accordingly too.

I think we can ditch ShadowsocksConfig for the most part, and have something specific for the proxying.

We have a design problem with the factory. We should eventually get rid of that, this code should control that. Instead we could be injecting a tunnel factory instead, but that a bit of a bigger change we can do later.

Done in 3e18673. I updated the server factory and OutlineServer constructor per your suggestion. I think it's clearer now, at the expense of parsing the access key twice, since the server repository needs to flag unsupported ciphers for backwards compatibility.

I kept the serve name getter/setter in order to update config.name, which is used by native code.

We have a design problem with the factory.

I have upcoming changes that will refactor the server factory into an interface that will allow native platforms to create only the tunnel.

alalamav added 4 commits March 8, 2021 14:11

persist access key instead of config; update tunnel interface

786a44a

rm persistent_server.ts

4be559f

remove ShadowsocksConfig from model

c536cf5

unit test storage migration

3a5cecc

alalamav requested a review from fortuna March 8, 2021 19:50

alalamav self-assigned this Mar 8, 2021

alalamav added 6 commits March 8, 2021 15:11

lint; small fixes

b36598f

check cipher after parsing config; move errorMessageId from model

fa5f9e5

server: s/host/address

a9b54fd

inject config into OutlineServer; normalize access key encoding and c…

de262a9

…omparison; fix alreadyadded error message

reset add server view state

5c5ccb5

move access key encoding/decoding to outline_server; validateAccessKey

76828fa

fortuna requested changes Mar 12, 2021

View reviewed changes

alalamav added 2 commits March 15, 2021 15:25

fix copyright headers

a77bce1

revert server name

76bde1c

alalamav requested a review from fortuna March 15, 2021 22:19

alalamav added 2 commits March 15, 2021 18:53

fix types

f26b026

rm dead code

26d6f9c

fortuna requested changes Mar 17, 2021

View reviewed changes

alalamav added 3 commits March 17, 2021 19:29

server display name

7a6d05e

update server storage format, migrate on change

e579a50

preserve access keys, remove ss config extra; improve unit tests

bc68e17

alalamav requested a review from fortuna March 18, 2021 03:12

fortuna requested changes Mar 18, 2021

View reviewed changes

create server with access key, no config

3e18673

alalamav requested a review from fortuna March 22, 2021 19:49

fortuna approved these changes Mar 23, 2021

View reviewed changes

alalamav merged commit 74dcb85 into master Mar 23, 2021

alalamav deleted the alalama-server-persistence branch March 23, 2021 22:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Persist servers by access key #988

Persist servers by access key #988

alalamav commented Mar 8, 2021

fortuna Mar 12, 2021

alalamav Mar 15, 2021

fortuna Mar 12, 2021

alalamav Mar 15, 2021

fortuna left a comment

fortuna Mar 17, 2021

alalamav Mar 18, 2021

fortuna Mar 17, 2021

alalamav Mar 18, 2021

fortuna Mar 17, 2021

alalamav Mar 18, 2021

fortuna Mar 17, 2021

alalamav Mar 18, 2021

fortuna Mar 17, 2021

alalamav Mar 18, 2021 •

edited

fortuna Mar 17, 2021

alalamav Mar 18, 2021

fortuna Mar 18, 2021

alalamav Mar 22, 2021

fortuna Mar 18, 2021

alalamav Mar 22, 2021

fortuna Mar 18, 2021

alalamav Mar 22, 2021

Persist servers by access key #988

Persist servers by access key #988

Conversation

alalamav commented Mar 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fortuna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alalamav Mar 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alalamav Mar 18, 2021 •

edited