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

proposal(www): allow providers to make access keys shareable. #1836

Closed
wants to merge 4 commits into from

Conversation

daniellacosse
Copy link
Contributor

Similar to #1830, this PR extends the extra field now with the ability for providers to enable end users to share their access keys with others:

{
  host: '10.0.0.24',
  port: 123,
  method: 'chacha20-ietf-poly1305',
  tag: 'Fake Unreachable Server',
  extra: {
     share: true
  }
}

#1830 and this PR in tandem enable the following scenario:

Screenshot 2024-02-09 at 1 47 34 PM

Copy link

codecov bot commented Feb 9, 2024

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (69bdfd7) 32% compared to head (f2ac0af) 32%.

Files Patch % Lines
src/www/app/app.ts 0% 15 Missing ⚠️
...servers_view/server_list_item/server_card/index.ts 0% 3 Missing ⚠️
Additional details and impacted files
@@                            Coverage Diff                             @@
##           daniellacosse/provider_message_and_contact   #1836   +/-   ##
==========================================================================
- Coverage                                          32%     32%   -1%     
==========================================================================
  Files                                              45      45           
  Lines                                            2620    2637   +17     
  Branches                                          342     347    +5     
==========================================================================
+ Hits                                              859     861    +2     
- Misses                                           1761    1776   +15     
Flag Coverage Δ
apple 15% <ø> (ø)
ios 15% <ø> (ø)
maccatalyst 15% <ø> (ø)
macos 15% <ø> (ø)
unittests 32% <5%> (-1%) ⬇️
www 40% <5%> (-1%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

return;
}

await navigator.share({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to test this on multiple platforms!

Copy link
Collaborator

Choose a reason for hiding this comment

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

What Chrome version are we using with Electron? Compare with https://caniuse.com/?search=navigator.share

Copy link
Contributor Author

@daniellacosse daniellacosse Feb 14, 2024

Choose a reason for hiding this comment

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

I believe it's over 100, we should be okay - but I will test to confirm once we're aligned on this idea

@sbruens
Copy link
Contributor

sbruens commented Feb 13, 2024

For this and #1830, if we're going to use the extra value for different configuration options, should we avoid non-object types at the top-level of extra to avoid it becoming a long grab-bag list of properties? I could imagine the sharing options to expand in future, such that a separate sharing key:value configuration object would be useful:

{
  "host": "10.0.0.24",
  "port": 123,
  "method": "chacha20-ietf-poly1305",
  "tag": "Fake Unreachable Server",
  "extra": {
    "sharing": {
      "enabled": true
    },
    "contact": {
      "messageContent": "We are experiencing increased demand.",
      "messageType": "warning",
      "email": "[email protected]"
    }
  }
}

return;
}

await navigator.share({
Copy link
Collaborator

Choose a reason for hiding this comment

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

What Chrome version are we using with Electron? Compare with https://caniuse.com/?search=navigator.share

@@ -614,6 +633,10 @@ export class App {
email: extraParams.email,
};
}

if (extraParams.share) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it should be called shareable, to indicate that the key is shareable. With ephemeral keys, access keys may not be shared.

Also, we could add this to the static keys as well.

@@ -614,6 +633,10 @@ export class App {
email: extraParams.email,
};
}

if (extraParams.share) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do worry about extending our bad config format that doesn't differentiate service and transport configuration, and has a very rigid transport configuration. I would like us to not get stuck with it.
At a minimum we should have our own internal format that is more sensible and can be mapped from the serialized formats.

Copy link
Contributor Author

@daniellacosse daniellacosse Feb 14, 2024

Choose a reason for hiding this comment

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

I hear you, but I wonder the degree to which we're tied to SIPOO2 already? I mean people use the client to connect to other shadowsocks servers all the time.

How about this - if we don't block adding these features to the access keys on inventing a new format I promise to draft a proposal for an outline:// config that we can all iterate on. Then I'll replace ShadowsocksConfig with a less overengineered solution that also handles outline2sskey

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
Copy link
Contributor Author

For this and #1830, if we're going to use the extra value for different configuration options, should we avoid non-object types at the top-level of extra to avoid it becoming a long grab-bag list of properties? I could imagine the sharing options to expand in future, such that a separate sharing key:value configuration object would be useful:

{
  "host": "10.0.0.24",
  "port": 123,
  "method": "chacha20-ietf-poly1305",
  "tag": "Fake Unreachable Server",
  "extra": {
    "sharing": {
      "enabled": true
    },
    "contact": {
      "messageContent": "We are experiencing increased demand.",
      "messageType": "warning",
      "email": "[email protected]"
    }
  }
}

For this and #1830, if we're going to use the extra value for different configuration options, should we avoid non-object types at the top-level of extra to avoid it becoming a long grab-bag list of properties? I could imagine the sharing options to expand in future, such that a separate sharing key:value configuration object would be useful:

{
  "host": "10.0.0.24",
  "port": 123,
  "method": "chacha20-ietf-poly1305",
  "tag": "Fake Unreachable Server",
  "extra": {
    "sharing": {
      "enabled": true
    },
    "contact": {
      "messageContent": "We are experiencing increased demand.",
      "messageType": "warning",
      "email": "[email protected]"
    }
  }
}

I -believe- extra values are meant to be strings, but there's nothing stopping us from passing through base64'd json i suppose!

Base automatically changed from daniellacosse/provider_message_and_contact to master March 13, 2024 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants