Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove block_when_disconnected setting on Android #7007

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MarkusPettersson98
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 commented Oct 17, 2024

This change is Reviewable

Copy link

linear bot commented Oct 17, 2024

@MarkusPettersson98 MarkusPettersson98 force-pushed the remove-lockdown-mode-in-settings-on-android-des-1316 branch 2 times, most recently from ae04f5b to 64ecd97 Compare October 17, 2024 17:32
@MarkusPettersson98 MarkusPettersson98 marked this pull request as ready for review October 17, 2024 17:33
Copy link
Contributor

@hulthe hulthe left a comment

Choose a reason for hiding this comment

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

Reviewed 13 of 15 files at r1, all commit messages.
Reviewable status: 13 of 15 files reviewed, 3 unresolved discussions (waiting on @MarkusPettersson98)


mullvad-daemon/src/settings/mod.rs line 184 at r1 (raw file):

                #[cfg(target_os = "android")]
                // On android, it is up to the OS to block connection when the VPN is disconnected.
                let settings = Self::default_settings();

Nit: this would look nicer imo

let settings = Settings {
    // lorem ipsum
    #[cfg(not(target_os = "android"))]
    block_when_disconnected: true,

    ..Self::default_settings()
};

mullvad-types/src/features.rs line 92 at r1 (raw file):

            FeatureIndicator::BridgeMode => "Bridge Mode",
            FeatureIndicator::SplitTunneling => "Split Tunneling",
            // TODO: target-gate this already??

What would be the point? We can't feature gate the proto-file anyway :'(


mullvad-types/src/features.rs line 133 at r1 (raw file):

    #[cfg(not(target_os = "android"))]
    let lockdown_mode = settings.block_when_disconnected;
    // TODO: Remove completely

yes

@MarkusPettersson98 MarkusPettersson98 force-pushed the remove-lockdown-mode-in-settings-on-android-des-1316 branch from 64ecd97 to 6fd5e68 Compare October 18, 2024 13:11
Copy link
Contributor Author

@MarkusPettersson98 MarkusPettersson98 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: 13 of 15 files reviewed, 2 unresolved discussions (waiting on @hulthe)


mullvad-daemon/src/settings/mod.rs line 184 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

Nit: this would look nicer imo

let settings = Settings {
    // lorem ipsum
    #[cfg(not(target_os = "android"))]
    block_when_disconnected: true,

    ..Self::default_settings()
};

That is indeed nicer!


mullvad-types/src/features.rs line 92 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

What would be the point? We can't feature gate the proto-file anyway :'(

Sad reality


mullvad-types/src/features.rs line 133 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

yes

Done

Copy link
Contributor

@hulthe hulthe 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: 10 of 15 files reviewed, 4 unresolved discussions (waiting on @MarkusPettersson98)


talpid-core/src/tunnel_state_machine/disconnected_state.rs line 107 at r1 (raw file):

    /// TODO: Document why this fn looks like this.
    #[cfg(target_os = "android")]
    fn set_firewall_policy(_: &mut SharedTunnelStateValues, _: bool) {}

ouf


talpid-core/src/tunnel_state_machine/mod.rs line 307 at r1 (raw file):

            // firewall stub completely.
            #[cfg(target_os = "android")]
            initial_state: InitialFirewallState::Blocked(args.settings.allowed_endpoint.clone()),

ouf

@MarkusPettersson98 MarkusPettersson98 force-pushed the remove-lockdown-mode-in-settings-on-android-des-1316 branch from 6fd5e68 to 80aa1e3 Compare October 18, 2024 14:20
Copy link
Contributor Author

@MarkusPettersson98 MarkusPettersson98 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: 8 of 15 files reviewed, 2 unresolved discussions (waiting on @hulthe)


talpid-core/src/tunnel_state_machine/disconnected_state.rs line 107 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

ouf

Done.


talpid-core/src/tunnel_state_machine/mod.rs line 307 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

ouf

Done.

Copy link
Contributor

@hulthe hulthe left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 15 files at r1, 3 of 3 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants