Add a "Turn Off/On" toggle
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.
Activity
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
countdownandfindClients. I pulled this out into asetIntervalcall 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
-
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.
Trac:
Status: assigned to needs_review 
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.
Trac:
Status: needs_review to merge_ready-
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? -
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.
-
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.
-
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.
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.
-
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.
Trac:
Status: needs_review to merge_ready -
-
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
-
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.
-
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 detailsOn 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
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.
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 setactiveafter receiving the client offer, compared to now where it is setactiveimmediately and made inactive when there's an error. Forgive me, I might be misinterpreting the code.-
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
-
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 setactiveafter receiving the client offer, compared to now where it is setactiveimmediately 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. -
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 setactiveafter receiving the client offer, compared to now where it is setactiveimmediately 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. -
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.
Trac:
Status: needs_revision to needs_review -
Thanks, sorry for that. Pushed a fix.
Alright, lgtm.
Not sure if you want to wait on #31067 (moved) for the next release, but we should at least get a patch like the one in #30999 (moved) in.
Trac:
Status: needs_review to merge_ready -
I just merged to master. I think we can wait for #31067 (moved) 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 (moved).
-
I just merged to master.
Great, so have a look at #31067 (moved) and #30999 (moved). Think we can call this done.
Trac:
Resolution: N/A to implemented
Status: merge_ready to closed mentioned in issue #30962 (moved)
mentioned in issue #30999 (moved)
mentioned in issue #31067 (moved)
mentioned in issue #31067 (moved)
