Kindness Mode UX Update

[tladesignz]

Fixes issue #1509.

[tladesignz]

@bitmold, it's about time you have a look at this.

Since you know the Tor-controlling part way better than me: What would you consider the best way to implement the test?

Additionally, I would be happy for any testing against non-standard phones and tablets.

Thank you!

[syphyr]

please rebase

[bitmold]

Awesome, I'll take a look at it tonight + tomorrow morning.

[syphyr]

Something is strange with the latest merge. This commit tladesignz@b070bf1 changes String to String?, but then the following change to override fun natTypeUpdated in SnowflakeProxyWrapper.kt is 6bfcb7e which shows String not String?, but this does not match HEAD.

Also, kindness_mode_disclaimer_bridge should be removed from app/src/main/res/values-tr/strings.xml

diff --git a/app/src/main/java/org/torproject/android/ui/kindness/SnowflakeProxyWrapper.kt b/app/src/main/java/org/torproject/android/ui/kindness/SnowflakeProxyWrapper.kt
index eefd62d14..6ae9cc305 100644
--- a/app/src/main/java/org/torproject/android/ui/kindness/SnowflakeProxyWrapper.kt
+++ b/app/src/main/java/org/torproject/android/ui/kindness/SnowflakeProxyWrapper.kt
@@ -89,8 +89,8 @@ class SnowflakeProxyWrapper(private val service: SnowflakeProxyService) {
                         // Ignored.
                     }
 
-                    override fun natTypeUpdated(natType: String?) {
-                        // TODO feature added in IPtProxy 5.4.1
+                    override fun natTypeUpdated(natType: String) {
+                        service.updateNatType(natType)
                     }
                 }
 
diff --git a/app/src/main/res/values-tr/strings.xml b/app/src/main/res/values-tr/strings.xml
index 695dc397c..bd94cd87a 100644
--- a/app/src/main/res/values-tr/strings.xml
+++ b/app/src/main/res/values-tr/strings.xml
@@ -277,7 +277,6 @@
     <string name="kindness_never_had_a_direct_connection">Paylaşım Modunu kullanabilmek için, en az bir kez Tor ağına doğrudan (herhangi bir köprü kullanmadan) bağlanmanız gerekir. Bağlantı kurduktan sonra, lütfen buraya geri dönerek Paylaşım Modunu etkinleştirin. Bu doğrudan bağlantı testini tamamladıktan sonra, Tor ağına bağlı olsanız da olmasanız da Paylaşım Modunu dilediğiniz gibi kullanabilirsiniz.</string>
     <string name="kindness_mode_cant_run_with_bridge">Tor ağına bağlanmak için bir köprü kullanırken Paylaşım Modu çalıştırılamaz.</string>
     <string name="kindness_mode_cant_run_in_your_country">Sansür nedeniyle, ülkenizde diğer kişilerin Tor ağına bağlanmasına yardımcı olmak için kullanılan Paylaşım Modu şu anda devre dışıdır.</string>
-    <string name="kindness_mode_disclaimer_bridge">Sansür uygulanan bir bölgedeyseniz veya Tor ağına bağlanmak için bir köprü kullanmanız gerekiyorsa, Paylaşım Modunu kullanmayın.</string>
     <string name="battery_optimization_title">Pil İyileştirmelerini Devre Dışı Bırak</string>
     <string name="battery_optimization_pref_msg">Aygıtlarının Orbot uygulamasını arka planda çalışırken erken bir şekilde kapattığından şüphelenen kullanıcılar için ayar.</string>
     <string name="battery_optimization_dialog_msg">Bazı Android aygıtlar, Orbot arka planda çalışırken uygulamayı kapatabilir. Orbot\'un bağlantısının kesilmemesini ve çalışmaya devam etmesini isteyen çoğu kullanıcı için, sistem ayarlarından Orbot\'u \"Her Zaman Açık\" VPN olarak ayarlamak yeterlidir. Bununla birlikte, gerekli görürseniz, aygıtınızın Orbot üzerinde herhangi bir pil iyileştirmesi yapmasını engellemek için gerekli adımları atabilirsiniz.</string>

[bitmold]

Noting this regression here, though I understand that this dialog that checks if a direct tor connection has been established is being replaced in some way by this testing dialog.

When you click activate on a cold install, the dialog to ensure that you have at least historically have had a direct tor connection appears. At this moment snowflake proxy is running and there's no way to turn it off.

If I go ahead and connect to tor, then go to activate kindness mode, the bug still exists. The testing dialog appears, but kindness mode is already running.

[bitmold]

On small screens, the headings get cramped

[bitmold]

The connected UI looks great, but unfortunately it breaks in landscape

And this is a bit nitpicky, but the labels aren't lining up with these grey panels

Also @cstiens it just dawned on me that on the kindness screen rectangular grey panels are used, whereas on the "more" screen to the right rounded panels are used. Personally, I think the slightly rounded ones look much nicer.

[tladesignz]

Noting this regression here, though I understand that this dialog that checks if a direct tor connection has been established is being replaced in some way by this testing dialog.

When you click activate on a cold install, the dialog to ensure that you have at least historically have had a direct tor connection appears. At this moment snowflake proxy is running and there's no way to turn it off.

If I go ahead and connect to tor, then go to activate kindness mode, the bug still exists. The testing dialog appears, but kindness mode is already running.

Good catch! I removed the legacy test.

[tladesignz]

• Fixed the header indent.
• Added rounded cards as suggested.
• Added first test code and a mock test to be able to continue development.
• Added final behaviour to show initial scene only, when no valid test result. (older than 24 hours.)

Found another problem:

We need to bind to the SnowflakeProxyService to receive the evaluation of the proxy quality.

However, doing so unintentionally starts a SnowflakeProxy. I'm afraid, SnowflakeProxyService is in for a redesign. @bitmold, what do you think?

[bitmold]

We need to bind to the SnowflakeProxyService to receive the evaluation of the proxy quality.
However, doing so unintentionally starts a SnowflakeProxy. I'm afraid, SnowflakeProxyService is in for a redesign. @bitmold, what do you think?

This is true, it needs to be modified, however I don't think a thorough redesign is needed.

SnowflakeProxyService is capable of running as a foreground service without actually running the snowflake proxy in IPtProxy. For instance, if the Service is configured to run on WiFi only and/or if the device is connected to a power source, the Service is capable of calling stopSnowflakeProxy(String?) to turn off the proxying.

When the Service is created in onCreate() it calls initNetworkCallbacks() which immediately starts proxying if the network conditions are met. In this function you could add preconditions to ensure that the correct proxy evaluations are met before invoking startSnowflakeProxy(String?)

[bitmold]

One thing, though, @bitmold : Can you please tune down your logging? Log.wtf() (highest level) is certainly inappropriate for most of the newly added log lines. How about Log.d()?

Re the logging, I had mentioned on the call that for the time being it's excessively logged and that we'd want to tune things down. Perhaps I should've made things clearer on the PR though.

When viewing both sets of logs (the main app's and the logs from the tor process side-by-side with ./scripts/view_logs_tmux.shthe intensity of the error logs makes it easy to filter out the ongoing work on the noise. Personal preference for me. Pipingadb logcatintogrep` can also be helpful, but this quickly gets complicated if the logs you're interested in following have different tag arguments. Either way, feedback received, it'll be tuned way down before merging.

[bitmold]

One quick thing: in this commit e9bb460 I added small horizontal margins (2dp @ start/end) to the kindness mode header, the one on the screen where you have the option to activate kindness mode.

If you are using a small screen, it feels very cramped spanning the maximum possible space

[bitmold]

@cstiens would appreciate your feedback on how to go about visually defining some of these additional states Benjamin and I were discussing above

Stronger wording about not using this in censored environments and also not behind VPNs on the initial scene.

Also clear wording that a test might happen which might shortly disconnect them a direct Tor connection is attempted. (Either on the initial scene or probably better in the testing scene, where users can return before it happens.)

An additional status line on the active kindness mode scene, where the actual status of the Snowflake Proxy is shown:

• Active
• Waiting for power
• Waiting for Wi-Fi
• Stopped

@tladesignz Happy to start implementing this though

[bitmold]

@tladesignz I have some ideas and will work on this. Will report back here in the coming days with something for you to review...

To communicate the quality of the Snowflake Proxy, KindnessFragment needs to bind to SnowflakeProxyService.
However, the way SnowflakeProxyService is currently built, binding will automatically trigger a start, which is not good.
@bitmold, can you rework SnowflakeProxyService such, that it behaves properly with that changed requirement. Or should I take care of that?

[bitmold]

@tladesignz I have some questions about NAT type quality, which as far as I'm aware only applies to Wi-Fi

        // TODO: We need this, to receive the proxy quality, but on the other hand, this
        //  will unintentionally start the SnowflakeProxyService, which we don't want, because
        //  it will automatically start SnowflakeProxy if there are no Wi-Fi/battery limits.
        //  -> hence a redesign of the SnowflakeProxyService seems to await us.

I understand that In order to get the NAT type, we need to start snowflake proxy, and that's why you are saying it's problematic that we can start SnowflakeProxyService since it will just start being a proxy when all we want is to know the NAT type (since in Orbot kindness mode is disabled).

My understanding from reading snowflake.go is that in Snowlfake v2.313.1
this code runs in proxy/lib/snowflake.go when you start snowflake from within IPtProxy:

// some earlier initial config 
	err = sf.checkNATType(config, sf.NATProbeURL)
	if err != nil {
		// non-fatal error. Log it and continue
		log.Printf("Error checking NAT type: %s", err.Error())
		setCurrentNATType(NATUnknown)
	}
	sf.EventDispatcher.OnNewSnowflakeEvent(event.EventOnCurrentNATTypeDetermined{CurNATType: getCurrentNATType()})

	NatRetestTask := task.Periodic{
		Interval: sf.NATTypeMeasurementInterval,
		Execute: func() error {
			return sf.checkNATType(config, sf.NATProbeURL)
		},
		// Not setting OnError would shut down the periodic task on error by default.
		OnError: func(err error) {
			log.Printf("Periodic probetest failed: %s, retaining current NAT type: %s", err.Error(), getCurrentNATType())
		},
	}
	if sf.NATTypeMeasurementInterval != 0 {
		NatRetestTask.WaitThenStart()
		defer NatRetestTask.Close()
	}

// more config and eventually we get to the main loop...

So essentially, you get the NAT type once Snowflake Proxy starts, and then every NATTypeMeasurementInterval you get that information updated again (so long as snowflake proxy is running)

Pls, correct me if I'm wrong in my understnading of how any of the above works...

But with this understanding a few questions and concerns come to mind:

• NATTypeMeasurementInterval isn't set in Orbot. Is this intentional? Since this is 0, you never find out what the NAT type is again if snowflake is running for a long time (even throughout the not-too-long timespan of a phone traveling in a users pocket throughout a day...). So, is the design here with the deactivated screen to get the NAT type by starting snowflake proxy in the fragment's onStart() & then stopping it once we learn the NAT type, and thus preenting it from doing any actual proxying? And then this initial value that you just set from when you opened the deactivated screen for kindness mode is then updated exactly one more time once the user actually enables kindness mode (thus starting starts snowflake proxy a second time, but of course keeping it running this time)

• Let's say somehow we are able to obtain the NAT type before the user toggles on kindness mode. It's restricted and i'm at my house and configure my router to be unrestricted. I then turn on kindness mode for the next three weeks, as far as I can tell I wouldn't get any more NAT type updates from snowflake until I turn off/on kindness mode again, correct? This seems like a bug/something tht's not accounted for in the design. If I'm unrestricted, and I go over to my friends house and they're restricted, I'd incorrectly think that I'm still on an unrestricted network even though I'm not. This can get even more confusing if my friend turned on kindness mode on their device, and their Orbot said "Restricted " while mine still said "Unrestricted" even though we're still on the same network.

• If Orbot should be setting NATTypeMeasurementInterval when starting snowflake proxy, what's a reasonable amount of time? If a user sets kindness mode to run when they're disconnected from power and whether or not they're on Wi-Fi, they really won't get at a new NAT type until they manually restart snowflake. Oudated NAT type info will follow them around everywhere...

• in SnowflakeProxyWrapper.kt's stopProxy() we release the mapped ports and then tell SnowflakeProxyService it's new NAT type is IPtProxy.IPtProxy.NATUnknown. This can happen when a user manually turns off kindness mode in the UI. But also, while kindness mode is running (ie KM's configured to be Wi-Fi only and the user loses Wi-Fi...) In this case The Android Service stays running, but it turns off Snowflake Proxy until the restrictions are complied with. From a user's perspective, kindness mode stays on the entire time, but when the new restrictions are met we of course get the new NAT type again.

• Exactly how are we even supposed to get the NAT type when kindness mode is disabled? Would it go like this:

1. the user opens the fragment
2. snowflake proxy is started until NATTypeUpdated is fires off in some async way
3. we learn the NAT type, update the disabled UI to show it the NAT type
4. turn off snowflake proxy before it starts proxying stuff, all while making sure to shutdown this ephemeral invocation of snowflake proxy if the user moves away from the fragment? Or if somehow the user turns on kindness mode (which actually starts snowflake proxy, all while the NAT type is being determined).

That ⬆️ seems like a lot, and for what? All of these questions/uncertainties lead me to think that we shouldn't display the "Proxy Quality: unknown/restricted/unrestricted" label when Kindness Mode is disabled. It suggests that you actualy have a proxy running when you don't, and may very well change once you start the proxy, and as far as I can tell you do actually have to start the proxy to learn this. Why not hide it when it's disabled, and then re-enable its visibility when kindness mode is turned on?

Unless I'm missing something, it seems like SnowflakeProxyService shouldn't be redesigned. it runs if and only if kindness mode is turned on, and while it runs it displays a notification that tells the user if snowflake proxy is running or not (depending on Wi-Fi/Power settings). Starting SnowflakeProxyService, while kindness mode is disabled means you give the user a notification bar icon and a persistent notification - for a short period of time - just to determine a NAT type. Even if you kill the service immediately right after learning the NAT type, the user sees a quickly appearing notification/top icon appear and quickly disappear, just from opening the kindness mode screen... Running foreground services is a big deal on Android, they should be launched only by intentional direct user action (such as clicking Start) instead of launching automatically launching without consent after a user opens a screen.

[bitmold]

If, in onStart() we jiust want to show the Fragment's NAT type value when kindness mode is turned On, that's easy. OrbotActivity can have a BroadcastReceiver that we use when binding to SnowflakeProxyService. Whenever we get a new NAT type from IPtProxy, it sends an update to the Activity which stores it in its view model. The Fragment, can query this value whenever it's opened up without needing to directly communicate to the Service.

Another, i had another idea that came to mind just now... Every time we get a new network event, (gain Wi-Fi, revert to cellular, connect to a new Wi-Fi, etc) SnowflakeProxyWrapper can be modified to stop and then restart snowflake proxy. This causes the initial NAT Type evaluation logic that happens when snowflake proxy first starts to fire off on every Android network event. You'd get accurate + timely NAT type updates for whatever connection you're currently on as soon as you connect to it (provided that kindness mode is turned on, of course). To me it seems better than finding an optimal repeat interval for NATTypeMeasurementInterval. As stated above, we already do something like this if the user has a Wi-Fi/power restriction. The difference is that rather than disconnecting when you're on Wi-Fi only and drop to a cellular network, you instead always disconnect on a new network, and then either immediately restart snowflake proxy if conditions are met/absent, or otherwise do what it already does today and wait to start snowflake proxy back up @ some future point after the device rejoins a Wi-Fi network.

Alternatively, is there anyway snowflake.go could be patched/modified for to accept a signal that on-demand fires off its NAT evaluation instead of us starting/stopping it in Orbot. This seems like the most elegant approach instead of my above idea ofkilling/relaunching SnowflakeProxyService on new network events, or using a polling interval.

[tladesignz]

Well, well, well. TBH, I'm not really fond of that approach to deny Kindness Mode when the user selected a certain country in an unrelated feature.

You're reusing information which was meant for a completely different feature. In the context of that feature, the country list makes sense, as for the countries there are specific DNS servers to use.

But said country list is not a definitive list of countries where censorship happens. These two things correlate, but are not necessarily the same.

Think of special connections: white SIM cards; Starlink; university research Wi-Fi

That's why I always pushed for the ultimate test: Try to run a straight Tor connection. That's the actual always true fact upon which we can decide if running a Snowflake Proxy makes sense or not.
How about this:
There is certainly the case, that for some people, it could theoretically be detrimental to get their Tor connection closed and a straight Tor connection established.
In the current implementation, this is somewhat surprising.
We certainly need some interstitial alert before actually starting the test.
That seems to me the right place to use the country information.
"We need to run a straight Tor connection as a test to see, if your current location is a good place for running a Snowflake Proxy. We will now disconnect your Tor VPN and run that test.

DO NOT PROCEED, if running an unobfuscated Tor connection right now could get you in trouble."
And, additionally or alternatively, when we see a country on our list:
"You selected XXX as your country. Running a Snowflake Proxy in that country most probably does not make sense. You cannot help others here. STOP RIGHT NOW."
I leave it to @cstiens to improve my wording.

[tladesignz]

@bitmold re NAT quality evaluation:

Wow, you quite overthought this a lot.

No, I don't want to start Snowflake Proxy just to get the NAT quality info.

I just want to get that info at all, when it is available. My first approach was to bind to the service and get the value communicated that way, but that turned out to be a bad idea, because that would actually start the Snowflake Proxy as the service is currently set up.

Your idea of rather using a local broadcast of course also works. AFAIR, that is discouraged though since recently and my Android Studio complains about it.

But, if you think to stay on local broadcasts is better than to redesign the service so it doesn't automatically start the Snowflake Proxy, I'm fine.

I like the idea of restarting the Snowflake Proxy on network changes.

And thanks for the hin with the NATTypeMeasurementInterval. I totally forgot about it and was actually waiting that something like that gets implemented. 🤣 I'll use that on iOS, as there it's more complicated to listen for network changes.

I still think that showing NATUnknown when the Snowflake Proxy connection stops is fine. I wouldn't want to show/hide whole elements all the time. That seems to bring a sort of restlessness into the UI.

[bitmold]

No, I don't want to start Snowflake Proxy just to get the NAT quality info.

I just want to get that info at all, when it is available. My first approach was to bind to the service and get the value communicated that way, but that turned out to be a bad idea, because that would actually start the Snowflake Proxy as the service is currently set up.

@tladesignz my confusion is there's currently a view displaying the NAT type, introduced by this PR, when kindness mode is turned off. Why even show it? Since the only way for the NAT type to be available is after starting snowflake proxy, correct?

Or to put it another way, the only way to get the NAT type in snowflake.go is to start proxying, it tells you the NAT type when the proxy first starts, and from there, it optionally can do the test at fixed intervals.

[bitmold]

Curious on your thoughts for a reasonable NATTypeMeasurementInterval value. Is 5 minutes too much, too little?

[tladesignz]

@tladesignz my confusion is there's currently a view displaying the NAT type, introduced by this PR, when kindness mode is turned off. Why even show it? Since the only way for the NAT type to be available is after starting snowflake proxy, correct?

Or to put it another way, the only way to get the NAT type in snowflake.go is to start proxying, it tells you the NAT type when the proxy first starts, and from there, it optionally can do the test at fixed intervals.

As I already stated above:

I still think that showing NATUnknown when the Snowflake Proxy connection stops is fine. I wouldn't want to show/hide whole elements all the time. That seems to bring a sort of restlessness into the UI.

[tladesignz]

Curious on your thoughts for a reasonable NATTypeMeasurementInterval value. Is 5 minutes too much, too little?

For iOS/macOS I decided upon 1 hour. Since it's a foreground thing on iOS, I suppose user's are not going to move much while running the Snowflake Proxy.

Of course, that can be very different on Android. But 5 minutes still seems a little much.

[bitmold]

Since there's no way to simply ask snowflake to do the test for us on demand, I'm going to take 20 min and implement restarting it on network change, and then test it by biking to around to several nearby friends' front porches (I'm on their Wi-Fi networks). if this works, we'll go with it. If not, I might try something like NATTypeMeasurementInterval = 30 minutes 🤷‍♀️

For mac an hour seems perfect though

[bitmold]

That's why I always pushed for the ultimate test: Try to run a straight Tor connection. That's the actual always true fact upon which we can decide if running a Snowflake Proxy makes sense or not.
How about this:
There is certainly the case, that for some people, it could theoretically be detrimental to get their Tor connection closed and a straight Tor connection established.
In the current implementation, this is somewhat surprising.
We certainly need some interstitial alert before actually starting the test.
That seems to me the right place to use the country information.
"We need to run a straight Tor connection as a test to see, if your current location is a good place for running a Snowflake Proxy. We will now disconnect your Tor VPN and run that test.

I didn't mean to suggest that those countries were an exhaustive list of regions where you can't run snowflake. I made the change after it seemed like a user of the iOS app based in Turkey complained after failing the test. Of course, in all the countries not in that list, if you try to start kindness mode you have the opportunity to pass or fail the definitive test.

The idea was to save users who want to just get online to talk to their families or read the news time by blocking off part of this app because even though there's been all this amazing UX work put into Orbot, the app still is still inevitably filled with a lot of confusing terminology and technical concepts that most users do not have a solid understanding of.

I'd rather just block it in some places where it's almost always not going to work anytime soon, rather than say something to the effect of

"You selected XXX as your country. Running a Snowflake Proxy in that country most probably does not make sense. You cannot help others here. STOP RIGHT NOW."

I think many users would find this warning confusing and scary. To me, keeping things streamlined and uncomplicated for users who have chosen to specify that they're in a sensitive area vastly outweighs being able to on ramp the rare starlink user in say Afghanistan, for example. And even if If they're clandestinely using starlink against the violent morality laws of the Taliban, then it's safe to assume that they probably have bigger concerns than helping users connect to the tor network via Orbot's kindness mode. If I had to guess, the lion's share of people in that region who are able to pass the test and sustainably run kindness mode would probably be some less than tech-savvy people in proximity to the Supreme Leader that have Orbot installed and then randomly turned on kindness mode and are somewhat unknowingly dedicating bandwidth to help people over the world connect to Tor.

But say you do have Starlink in a violently censored place and do use kindness mode - well for those, what 2 dozen users, having kindness mode on is only useful while they're on that starlink network, the minute they go 6-15ft (1.8-4.5m) away from the router and possibly join another network then they're running kindness mode uselessly, while perhaps also jeopardizing their personal safety.

As for someone on a privileged university Wi-Fi the same drawback is the case. Maybe, this very negligible amount of users could be snowflakes, but they're still going to become immediately unhelpful proxies the minute they leave their campus network. I would imagine this hypothetical researcher would probably be more drawn to the standalone proxy anyway.

As for white SIM card holders in Iran, my understanding was those SIMs were issued to those aligned with IRGC. Since it seems like you have to be aligned with the state's project of censorship to get one of those SIM cards, I don't think those Orbot users would be interested in helping to run a technology that's historically been used for circumvention in Iran.

How about this:

• we remove some countries from the list I borrowed from DNSTT to only include ones where it's been and probably will continue to be a statistical improbability that someone could effectively use kindness mode in a sustained and helpful way. Off the top of my head, Afghanistan and Iran come to mind as ones to keep, but I'm sure there are a few more.
• if a user selected that they are in a country on this list, instead of saying my proposed "Thank you so much for your interest in helping others connect to Tor. Unfortunately, Kindness Mode isn't available in your country at this time. We appreciate your generosity." we take your suggestion "You selected XXX as your country. Running a Snowflake Proxy in that country most probably does not make sense. You cannot help others here. STOP RIGHT NOW." and modify it to ➡️ "You selected XXX as your country. Running a Snowflake Proxy in that country most probably does not make sense. If you still want to try to see if your device is capable of being a Snowflake Proxy, please clear your selected country." It's less scary that way, and it gives users who really want to do this the option to try the test just like everyone else, even though they'd likely fail it.

[bitmold]

I 100% agree with you though that the testing dialog should have some kind of interstitial warning like you mentioned in that comment I just quoted

[tladesignz]

Since there's no way to simply ask snowflake to do the test for us on demand, I'm going to take 20 min and implement restarting it on network change, and then test it by biking to around to several nearby friends' front porches (I'm on their Wi-Fi networks). if this works, we'll go with it. If not, I might try something like NATTypeMeasurementInterval = 30 minutes 🤷‍♀️

For mac an hour seems perfect though

Love your physical dedication! 😁