Skip to content

Add leak test for when VPN settings change#7439

Merged
Pururun merged 1 commit intomainfrom
leak-test-when-vpn-settings-change-droid-1558
Jan 13, 2025
Merged

Add leak test for when VPN settings change#7439
Pururun merged 1 commit intomainfrom
leak-test-when-vpn-settings-change-droid-1558

Conversation

@Pururun
Copy link
Copy Markdown
Contributor

@Pururun Pururun commented Jan 9, 2025

Test for leaks to specific host when changing DAITA and Shadowsocks VPN settings. To test the changes out run testLeakWhenVpnSettingsChangelocally or run the Android - build and test workflow in GitHub.


This change is Reviewable

@Pururun Pururun added the Android Issues related to Android label Jan 9, 2025
@Pururun Pururun requested review from Rawa and albin-mullvad January 9, 2025 13:54
@linear
Copy link
Copy Markdown

linear Bot commented Jan 9, 2025

Copy link
Copy Markdown
Collaborator

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

Reviewed 9 of 11 files at r1.
Reviewable status: 9 of 11 files reviewed, 5 unresolved discussions (waiting on @Pururun)


android/test/common/src/main/kotlin/net/mullvad/mullvadvpn/test/common/page/DaitaSettingsPage.kt line 13 at r1 (raw file):

    }

    fun clickEnableSwitch() {

nit: EnableSwitch might disable since it is a Switch. Maybe we should call it clickDaitaSwitch or something similar?


android/test/common/src/main/kotlin/net/mullvad/mullvadvpn/test/common/interactor/AppInteractor.kt line 61 at r1 (raw file):

    fun enableLocalNetworkSharing() {
        on<TopBar> { clickSettings() }

TopBar isn't really a screen, don't we expect this to be the ConnectPage? Maybe this should rather be part of the ConnectScreen? than the app, or some new concept. E.g if not logged in TopBar would not be able to go to VpnSettingsPage since it is hidden.


android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/LeakTest.kt line 169 at r1 (raw file):

        app.launch()
        disableObfuscation()
        disablePostQuantum()

Don't we have a clean slate with these off? Or is it not clean?


android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/LeakTest.kt line 182 at r1 (raw file):

        // Capture generated traffic to a specific host
        val targetIpAddress = BuildConfig.TRAFFIC_GENERATION_IP_ADDRESS

Before starting capture and generating traffic we should ensure we are connected. Otherwise we might send traffic outside the tunnel, and thus failing the test incorrectly.


android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/LeakTest.kt line 192 at r1 (raw file):

                    enableDAITA()
                    enableShadowsocks()

Maybe should assert that we are connected after changing the settings?

Copy link
Copy Markdown
Collaborator

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 11 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Pururun)

Copy link
Copy Markdown
Collaborator

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Pururun)


android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/LeakTest.kt line 169 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Don't we have a clean slate with these off? Or is it not clean?

Edit: Clarify with comment that this is to disable automatic mode.

@niklasberglund niklasberglund force-pushed the leak-test-when-vpn-settings-change-droid-1558 branch from 77aecf3 to 7b6a39b Compare January 9, 2025 16:46
Copy link
Copy Markdown
Contributor

@niklasberglund niklasberglund left a comment

Choose a reason for hiding this comment

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

Reviewable status: 6 of 16 files reviewed, 4 unresolved discussions (waiting on @Rawa)


android/test/common/src/main/kotlin/net/mullvad/mullvadvpn/test/common/interactor/AppInteractor.kt line 61 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

TopBar isn't really a screen, don't we expect this to be the ConnectPage? Maybe this should rather be part of the ConnectScreen? than the app, or some new concept. E.g if not logged in TopBar would not be able to go to VpnSettingsPage since it is hidden.

Moved the functions from TopBar to LoginPage and ConnectPage and removed TopBar


android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/LeakTest.kt line 169 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Edit: Clarify with comment that this is to disable automatic mode.

See added comment


android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/LeakTest.kt line 182 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Before starting capture and generating traffic we should ensure we are connected. Otherwise we might send traffic outside the tunnel, and thus failing the test incorrectly.

Good catch, this was accidentally removed. Have added it.


android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/LeakTest.kt line 192 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Maybe should assert that we are connected after changing the settings?

Yes that's a very good idea 👍 Now waiting for connected label.

Rawa
Rawa previously approved these changes Jan 13, 2025
Copy link
Copy Markdown
Collaborator

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

Reviewed 10 of 10 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@Pururun Pururun force-pushed the leak-test-when-vpn-settings-change-droid-1558 branch from 7b6a39b to 988db3a Compare January 13, 2025 09:36
@Pururun Pururun force-pushed the leak-test-when-vpn-settings-change-droid-1558 branch from 988db3a to dd3ee95 Compare January 13, 2025 10:04
Copy link
Copy Markdown
Collaborator

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

Reviewed 12 of 12 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@Pururun Pururun merged commit b59ff27 into main Jan 13, 2025
@Pururun Pururun deleted the leak-test-when-vpn-settings-change-droid-1558 branch January 13, 2025 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Android Issues related to Android

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants