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

Respect custom API endpoint when the api-override feature is enabled #5674

Merged

Conversation

MarkusPettersson98
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 commented Jan 10, 2024

This PR fixes an issue with the daemon not configuring the firewall correctly in case the api-override feature is enabled.

More specifically, this PR moves the responsibility for using overridden API endpoints for API calls from mullvad-api::rest to mullvad_daemon::api. This is in line with how the interaction between the two crates work for a normal release build , i.e. when the api-override feature is disabled.

This commit also removes the force_direct_connection, since it was always true when the api-override feature was enabled. This change also affects the Android client code. Since the flag does not exist in the mullvad-api or mullvad-jni rust crates anymore, it would be erroneous to try to serialize/deserialize the value in the Android client.

The boy scout rule was generously applied to mullvad_api::ApiEndpoint, since I found the logic of initializing it hard to read due to how intertwined it was with the #[cfg(feature = "api-override")] attribute.

Fixes DES-550


This change is Reviewable

@MarkusPettersson98 MarkusPettersson98 force-pushed the firewall-not-updated-when-api-override-is-enabled-550 branch 2 times, most recently from 503888d to f6773cf Compare January 10, 2024 09:30
Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 11 files at r1.
Reviewable status: 3 of 11 files reviewed, all discussions resolved

Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

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


mullvad-api/src/lib.rs line 164 at r1 (raw file):

        api.address = match address_var {
            Some(user_addr) => {
                let addr = user_addr.parse().unwrap_or_else(|_| {

Nit: Can't we just use .expect()?


mullvad-api/src/lib.rs line 238 at r1 (raw file):

    /// [`Self::API_HOST_DEFAULT`] as default value if it does not exist.
    pub fn host(&self) -> String {
        self.host

Nit: Is it possible to return a &str using self.host.as_ref().unwrap_or(...)?

Copy link

linear bot commented Jan 10, 2024

Copy link
Contributor

@Pururun Pururun left a comment

Choose a reason for hiding this comment

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

:lgtm:

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


mullvad-api/src/lib.rs line 107 at r1 (raw file):

pub struct ApiEndpoint {
    /// An overriden API hostname. Initialized with the value of the environment
    /// variable `MULLVAD_API_HOSt` if it has been set.

Nit: This should be MULLVAD_API_HOST with a capital T right?

@MarkusPettersson98 MarkusPettersson98 force-pushed the firewall-not-updated-when-api-override-is-enabled-550 branch from f6773cf to 7a30392 Compare January 10, 2024 13:07
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: 7 of 11 files reviewed, 3 unresolved discussions (waiting on @dlon and @Pururun)


mullvad-api/src/lib.rs line 107 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Nit: This should be MULLVAD_API_HOST with a capital T right?

Yes, of course 🦅 👁️
Thanks!


mullvad-api/src/lib.rs line 164 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Nit: Can't we just use .expect()?

Because I want to use format! in that case, clippy will issue a warning for the expect_fun_call lint 😊

https://rust-lang.github.io/rust-clippy/master/index.html#/expect_fun_call


mullvad-api/src/lib.rs line 238 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Nit: Is it possible to return a &str using self.host.as_ref().unwrap_or(...)?

Yes, it proved to be possible! Thanks for pointing it out:)

@dlon dlon requested a review from Pururun January 10, 2024 13:24
Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @MarkusPettersson98 and @Pururun)


mullvad-api/src/lib.rs line 164 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Because I want to use format! in that case, clippy will issue a warning for the expect_fun_call lint 😊

https://rust-lang.github.io/rust-clippy/master/index.html#/expect_fun_call

I see!


mullvad-daemon/src/api.rs line 350 at r2 (raw file):

            log::warn!(
                "The `api-override` feature is enabled, but the API address \

This should not trigger a warning, IMO. It will do so for all dev builds.

@MarkusPettersson98 MarkusPettersson98 force-pushed the firewall-not-updated-when-api-override-is-enabled-550 branch 2 times, most recently from 8e6ac98 to d2e9fea Compare January 10, 2024 14:18
Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Pururun)

@MarkusPettersson98 MarkusPettersson98 force-pushed the firewall-not-updated-when-api-override-is-enabled-550 branch from d2e9fea to 36afe0d Compare January 10, 2024 14:29
Copy link
Member

@dlon dlon 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 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Pururun)

@MarkusPettersson98 MarkusPettersson98 force-pushed the firewall-not-updated-when-api-override-is-enabled-550 branch from 36afe0d to c65dda8 Compare January 11, 2024 07:10
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: 10 of 11 files reviewed, 1 unresolved discussion (waiting on @dlon and @Pururun)


mullvad-daemon/src/api.rs line 350 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

This should not trigger a warning, IMO. It will do so for all dev builds.

Lowered the log-level to debug 😊

feature is enabled

Move the logic for using overridden API endpoints for API calls from
`mullvad-api::rest` to `mullvad_daemon::api`. This is in line with how
the interaction between the two crates work for a normal release build,
i.e. when the `api-override` feature is disabled.

This commit also removes references to `force_direct_connection` in the
Android code. The flag does not exist in the `mullvad-*` rust crates
anymore, so it would be erroneous to try to serialize/deserialize the
value from the Android client.
@MarkusPettersson98 MarkusPettersson98 force-pushed the firewall-not-updated-when-api-override-is-enabled-550 branch from c65dda8 to 8b0fd0d Compare January 11, 2024 08:18
Copy link
Member

@dlon dlon 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 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Pururun)

@MarkusPettersson98 MarkusPettersson98 merged commit b5decd1 into main Jan 11, 2024
41 of 42 checks passed
@MarkusPettersson98 MarkusPettersson98 deleted the firewall-not-updated-when-api-override-is-enabled-550 branch January 11, 2024 08:45
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.

4 participants