Skip to content

Conversation

@jamilbk
Copy link
Member

@jamilbk jamilbk commented Aug 29, 2024

When a user disconnects our VPN from system settings, Android does not call any of the typical TunnelService callbacks. It does, however, send a standard NetworkMonitor event we can intercept in the onLost callback. We use that to shut down the TunnelService if we detect our VPN network has been lost.

Unfortunately, network monitoring in Android is broad - we are notified when any network is lost, not just ours. So to filter others out, we listen for linkProperties changing, and if the link addresses match our tunnel IPv4 and IPv6, we save that network ID to match on in the onLost callback later.

Resolves #5413

@jamilbk jamilbk added area/android_client Issues related to the Android client kind/cust-feedback Customer feedback. Don't close these until resolution is reached. labels Aug 29, 2024
@jamilbk jamilbk requested a review from conectado August 29, 2024 23:29
@vercel
Copy link

vercel bot commented Aug 29, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
firezone ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 30, 2024 1:37am

</queries>

<application
android:enableOnBackInvokedCallback="true"
Copy link
Member Author

Choose a reason for hiding this comment

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

This prevents a minor warning on Android 13

Copy link
Member Author

Choose a reason for hiding this comment

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

It could have theoretically been possible for serviceBound = true even when tunnelService fails to initialize, so that's fixed here.

Copy link
Member

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

I am kind of worried about all these things being not tested in an automated way but I guess such is the life of app development.

Comment on lines +19 to +20
val ipv4Found = linkProperties.linkAddresses.find { it.address.hostAddress == tunnelService.tunnelIpv4Address }
val ipv6Found = linkProperties.linkAddresses.find { it.address.hostAddress == tunnelService.tunnelIpv6Address }
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to guard against tunnelIpv4Address being null here to avoid a potential null == null comparison? These are null before we start-up right? Might be more of a defense-in-depth thing because there is nothing to shut down if we aren't started up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, I don't think that's a possibility, but I'll refactor this just in case.

@jamilbk jamilbk enabled auto-merge August 30, 2024 01:50
@jamilbk jamilbk added this pull request to the merge queue Aug 30, 2024
Merged via the queue into main with commit 007323a Aug 30, 2024
@jamilbk jamilbk deleted the fix/handle-edge-case-tunnelService-null branch August 30, 2024 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/android_client Issues related to the Android client kind/cust-feedback Customer feedback. Don't close these until resolution is reached.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Android: Respond to user-initiated TunnelService disconnect

3 participants