Opened 3 months ago

Closed 3 months ago

#30934 closed defect (implemented)

Add a "Turn Off/On" toggle

Reported by: arlolra Owned by: cohosh
Priority: Medium Milestone:
Component: Circumvention/Snowflake Version:
Severity: Normal Keywords: snowflake-webextension
Cc: arlolra, cohosh, phw, dcf Actual Points:
Parent ID: Points:
Reviewer: arlolra Sponsor:

Description

As in the mockup in comment:5:ticket:23888

For now, the extension can be disabled from the browser's management pane, if so desired.

Child Tickets

Change History (32)

comment:1 Changed 3 months ago by cohosh

Owner: set to cohosh
Status: newassigned

comment:2 Changed 3 months ago by cohosh

I got a start on implementing this here: https://github.com/cohosh/snowflake/tree/webext-toggle

The first commit is a UI update from antonela that adds the toggle button and reformats the Learn more link

The second commit is a refactoring of the broker poll. To disable snowflake, we need to somehow break the infinite poll loop that cycles between countdown and findClients. I pulled this out into a setInterval call and slightly changed how to determine whether there are available proxyPairs.

Finally in the third commit, to add the Off/On toggle functionality, we have to change how the webpage handles this. It doesn't make sense to use cookies and completely reload the web extension as before, so I added an enabled variable to the UI class an messaging back and forth to update this.

There's still a few more things to do:

  • change the snowflake image to reflect whether or not it's enabled
  • update unit tests
Last edited 3 months ago by cohosh (previous) (diff)

comment:3 Changed 3 months ago by cohosh

Status: assignedneeds_review

Okay I added a few lint and test fixes and then updated the popup display here. These commits could do with a squashing but I'll put this into review for now: https://github.com/cohosh/snowflake/pull/4

I haven't gotten the rotation to work yet with the svg image.

comment:4 Changed 3 months ago by gaba

Reviewer: arlolra

comment:5 Changed 3 months ago by arlolra

Status: needs_reviewmerge_ready

Looks good to me, just some nits about being consistent with whitespace (tabs vs spaces, and all that).

These commits could do with a squashing

Also, please rebase your commits on master before merging so that we don't have any merge commits.

I made a couple of follow up commits that I'd appreciate you having a look at in,
https://github.com/keroserene/snowflake/commits/webext-toggle

I haven't gotten the rotation to work yet with the svg image.

Yeah, some sort of indication that it is active would be nice, like rotation or switching to green. Is rotation going to be too distracting in the browserAction icon?

A final note is that this doesn't save the toggle state so a user needs to turn it on at every browser restart.

comment:6 in reply to:  5 ; Changed 3 months ago by cohosh

Replying to arlolra:

Looks good to me, just some nits about being consistent with whitespace (tabs vs spaces, and all that).

These commits could do with a squashing

Also, please rebase your commits on master before merging so that we don't have any merge commits.

Done, and fixed the whitespace errors by moving everything in the webext/ subfolder to the 2 spaces for indentation, no tabs convention.

I made a couple of follow up commits that I'd appreciate you having a look at in,
https://github.com/keroserene/snowflake/commits/webext-toggle

This look good to me. I like the cleanup of cease/disable/reset. Merged these as well.

I haven't gotten the rotation to work yet with the svg image.

Yeah, some sort of indication that it is active would be nice, like rotation or switching to green. Is rotation going to be too distracting in the browserAction icon?

I'll work on this next.

A final note is that this doesn't save the toggle state so a user needs to turn it on at every browser restart.

Yeah, I noticed that as well. I think this is okay.. the only thing we'd want to save is the displayed stats for:
Your snowflake has helped 0 users circumvent censorship in the last 24 hours.
My guess is that it makes sense to reset this on a browser restart since 24 hours isn't that long anyway? If we want to allow users to see long-term usage we could save this, but is that even a good idea?

comment:7 in reply to:  6 Changed 3 months ago by cohosh

Replying to cohosh:

Replying to arlolra:

A final note is that this doesn't save the toggle state so a user needs to turn it on at every browser restart.

Yeah, I noticed that as well. I think this is okay.. the only thing we'd want to save is the displayed stats for:
Your snowflake has helped 0 users circumvent censorship in the last 24 hours.
My guess is that it makes sense to reset this on a browser restart since 24 hours isn't that long anyway? If we want to allow users to see long-term usage we could save this, but is that even a good idea?

Oh wait my bad, it's the opt-in thing they have to reset. Hm. Okay I'll look into saving this state.

comment:8 Changed 3 months ago by cohosh

Okay here are two more commits for

  • changing the snowflake green when a user is currently using the proxy
  • saving the toggle state in local storage so users don't need to opt-in every time the browser restarts.

https://github.com/cohosh/snowflake/pull/5

Last edited 3 months ago by cohosh (previous) (diff)

comment:9 Changed 3 months ago by cohosh

Status: merge_readyneeds_review

Putting this in needs_review again before pushing to master

comment:10 Changed 3 months ago by cypherpunks

I haven't checked the code but is it the case that by default it's set to "On" (without any user input)?

comment:11 in reply to:  10 ; Changed 3 months ago by cohosh

Replying to cypherpunks:

I haven't checked the code but is it the case that by default it's set to "On" (without any user input)?

No, the default is for the Snowflake proxy to be disabled and a user has to opt-in by clicking the popup and turning it on.

This is the same case as the current webpage snowflake.torproject.org. We could perhaps add some more obvious instructions to enable it if it's a usability problem.

comment:12 in reply to:  11 Changed 3 months ago by cypherpunks

Replying to cohosh:

No, the default is for the Snowflake proxy to be disabled and a user has to opt-in by clicking the popup and turning it on.

This is the same case as the current webpage snowflake.torproject.org. We could perhaps add some more obvious instructions to enable it if it's a usability problem.

Doesn't the mere fact that someone installed that extension mean that they want to (immediately) start acting as a proxy? I don't think your choice for the default makes sense then.

comment:13 Changed 3 months ago by arlolra

Status: needs_reviewmerge_ready

Okay here are two more commits for ...

On the whole, these changes seem fine. I left two comments on the pull.

This is the same case as the current webpage snowflake.torproject.org. We could perhaps add some more obvious instructions to enable it if it's a usability problem.

I agree with the cypherpunk that it should default to "on" since this isn't the same as browsing to a webpage that potentially has a snowflake embed and could start proxying traffic against the user's will; here we have explicit interaction of installing the extension.

comment:15 in reply to:  13 Changed 3 months ago by cohosh

Replying to arlolra:

I agree with the cypherpunk that it should default to "on" since this isn't the same as browsing to a webpage that potentially has a snowflake embed and could start proxying traffic against the user's will; here we have explicit interaction of installing the extension.

Sounds good. I made the suggested fixes and also enabled it by default with the latest commit: https://github.com/cohosh/snowflake/pull/5#pullrequestreview-257382200

comment:16 in reply to:  14 Changed 3 months ago by cohosh

Replying to arlolra:

I pushed a small fixup in,
https://gitweb.torproject.org/pluggable-transports/snowflake.git/commit/?id=526e42a443d81b121ac14c125a2a85e0170b8cff

Good catch, thanks. I'll merge this in with the rest of the changes.

comment:17 Changed 3 months ago by cohosh

Hrm I'm seeing what looks like an infinite loop that the extension eventually goes into.

On Firefox:

Snowflake: At client capacity. snowflake.js:1186:13
XML Parsing Error: no element found
Location: https://snowflake-broker.bamsoftware.com/answer
Line Number 1, Column 1: answer:1:1
XML Parsing Error: no element found
Location: https://snowflake-broker.bamsoftware.com/proxy
Line Number 1, Column 1: proxy:1:1
Snowflake: At client capacity.
snowflake.js:1186:13
ICE failed, add a STUN server and see about:webrtc for more details

On Chrome:

POST https://snowflake-broker.bamsoftware.com/proxy 504
(2) snowflake.js:1186 Snowflake: At client capacity.
proxy:1 POST https://snowflake-broker.bamsoftware.com/proxy 504
(35) snowflake.js:1186 Snowflake: At client capacity.

It doesn't seem to happen with the currently published version 0.0.1, so I'll look into whether the refactoring changes might have caused it

Last edited 3 months ago by cohosh (previous) (diff)

comment:18 in reply to:  13 ; Changed 3 months ago by dcf

Replying to arlolra:

This is the same case as the current webpage snowflake.torproject.org. We could perhaps add some more obvious instructions to enable it if it's a usability problem.

I agree with the cypherpunk that it should default to "on" since this isn't the same as browsing to a webpage that potentially has a snowflake embed and could start proxying traffic against the user's will; here we have explicit interaction of installing the extension.

I think so too. The way I see it, installing the extension is opting in (equivalent to setting a cookie on the web page), and the on/off toggle is for the convenience of not having to dig into the browser's extension settings if you want to disable it temporarily.

comment:19 in reply to:  17 ; Changed 3 months ago by dcf

Replying to cohosh:

Hrm I'm seeing what looks like an infinite loop that the extension eventually goes into.

Could it be if broker.getClientOffer (first HTTP request to broker) succeeds, but then ProxyPair.receiveOffer fails (JSON parsing and second HTTP request to broker). In that case, the ProxyPair is neither closed nor set inactive? Formerly, the ProxyPair was only set active after receiving the client offer, compared to now where it is set active immediately and made inactive when there's an error. Forgive me, I might be misinterpreting the code.

comment:20 in reply to:  18 Changed 3 months ago by cohosh

Replying to dcf:

Replying to arlolra:

This is the same case as the current webpage snowflake.torproject.org. We could perhaps add some more obvious instructions to enable it if it's a usability problem.

I agree with the cypherpunk that it should default to "on" since this isn't the same as browsing to a webpage that potentially has a snowflake embed and could start proxying traffic against the user's will; here we have explicit interaction of installing the extension.

I think so too. The way I see it, installing the extension is opting in (equivalent to setting a cookie on the web page), and the on/off toggle is for the convenience of not having to dig into the browser's extension settings if you want to disable it temporarily.

Cool, added here

comment:21 in reply to:  19 ; Changed 3 months ago by cohosh

Replying to dcf:

Replying to cohosh:

Hrm I'm seeing what looks like an infinite loop that the extension eventually goes into.

Could it be if broker.getClientOffer (first HTTP request to broker) succeeds, but then ProxyPair.receiveOffer fails (JSON parsing and second HTTP request to broker). In that case, the ProxyPair is neither closed nor set inactive? Formerly, the ProxyPair was only set active after receiving the client offer, compared to now where it is set active immediately and made inactive when there's an error. Forgive me, I might be misinterpreting the code.

Yeah that's what I'm looking into, I'll add a fix to cover that case but it actually seems that the WebRTC might not be closing correctly. My snowflake has been green (active) for well over an hour now. Usually the connection times out during a browsing session before then.

comment:22 in reply to:  21 Changed 3 months ago by cohosh

Replying to cohosh:

Replying to dcf:

Replying to cohosh:

Hrm I'm seeing what looks like an infinite loop that the extension eventually goes into.

Could it be if broker.getClientOffer (first HTTP request to broker) succeeds, but then ProxyPair.receiveOffer fails (JSON parsing and second HTTP request to broker). In that case, the ProxyPair is neither closed nor set inactive? Formerly, the ProxyPair was only set active after receiving the client offer, compared to now where it is set active immediately and made inactive when there's an error. Forgive me, I might be misinterpreting the code.

Yeah that's what I'm looking into, I'll add a fix to cover that case but it actually seems that the WebRTC might not be closing correctly. My snowflake has been green (active) for well over an hour now. Usually the connection times out during a browsing session before then.

This commit should put us back at the state we were at before the refactor in terms of error checking and returning the pair to inactive.

comment:23 Changed 3 months ago by cohosh

Status: merge_readyneeds_review

It looks like the ICE server error with the Firefox addon is a different issue and applies to both the currently published version and these changes.

I'm going to put this back into needs_review because of the last 2 commits.

comment:24 Changed 3 months ago by arlolra

Status: needs_reviewneeds_revision

I'm going to put this back into needs_review because of the last 2 commits.

I left some review on the pull.

comment:25 in reply to:  24 Changed 3 months ago by cohosh

Status: needs_revisionneeds_review

Replying to arlolra:

I'm going to put this back into needs_review because of the last 2 commits.

I left some review on the pull.

Thanks, modified as per feedback.

comment:26 Changed 3 months ago by arlolra

Status: needs_reviewneeds_revision

Thanks, modified as per feedback.

There was one gotcha about that, see the pull.

comment:27 Changed 3 months ago by cohosh

Status: needs_revisionneeds_review

Ah >.< thanks, in review again

Last edited 3 months ago by cohosh (previous) (diff)

comment:28 Changed 3 months ago by arlolra

Status: needs_reviewneeds_revision

Ah >.< thanks, in review again

Sorry about all the back and forth. Another couple comments on the pull for you :)

comment:29 Changed 3 months ago by cohosh

Status: needs_revisionneeds_review

Thanks, sorry for that. Pushed a fix.

comment:30 Changed 3 months ago by arlolra

Status: needs_reviewmerge_ready

Thanks, sorry for that. Pushed a fix.

Alright, lgtm.

Not sure if you want to wait on #31067 for the next release, but we should at least get a patch like the one in #30999 in.

comment:31 Changed 3 months ago by cohosh

I just merged to master. I think we can wait for #31067 for the next release. I think we'll want to update the screenshots anyway with the current UI from this ticket and from the new feature added in #31067.

comment:32 Changed 3 months ago by arlolra

Resolution: implemented
Status: merge_readyclosed

I just merged to master.

Great, so have a look at #31067 and #30999. Think we can call this done.

Note: See TracTickets for help on using tickets.