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

Add a local network probe to trigger local network privacy dialog #5729

Merged

Conversation

buggmagnet
Copy link
Contributor

@buggmagnet buggmagnet commented Jan 25, 2024

It's all in the title


This change is Reviewable

@buggmagnet buggmagnet added the iOS Issues related to iOS label Jan 25, 2024
@buggmagnet buggmagnet self-assigned this Jan 25, 2024
@buggmagnet buggmagnet force-pushed the trigger-the-local-network-alert-before-trying-local-ios-465 branch 2 times, most recently from 510c155 to f655cf8 Compare January 29, 2024 12:33
Copy link
Contributor

@rablador rablador 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 r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Collaborator

@mojganii mojganii 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, 2 unresolved discussions (waiting on @buggmagnet)


ios/MullvadVPN/AccessMethodRepository/LocalNetworkProbe.swift line 26 at r1 (raw file):

            using: .udp
        )
        localIpv6Connection.start(queue: dispatchQueue)

What will happen if the connection establishment fails? This part establishes a UDP connection to trigger the dialog. Is there any impact when the user doesn't allow the app access to the local network? Does the app (access methods) work fine if access is restricted?

Moreover, is there any specific reason the type of connection is just UDP? Is it for bypassing ISPs?


ios/MullvadVPN/Supporting Files/Info.plist line 6 at r1 (raw file):

<dict>
	<key>NSLocalNetworkUsageDescription</key>
	<string>The app needs this to connect and test a new method.</string>

@emils is the description decribtive enough for users?

Copy link
Collaborator

@mojganii mojganii 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: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor Author

@buggmagnet buggmagnet 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: :shipit: complete! all files reviewed, all discussions resolved


ios/MullvadVPN/AccessMethodRepository/LocalNetworkProbe.swift line 26 at r1 (raw file):

What will happen if the connection establishment fails?
Not much, we don't do anything with the connection.

Is there any impact when the user doesn't allow the app access to the local network?
Yes, the user defined custom methods likely won't work, this is why we trigger this here

Moreover, is there any specific reason the type of connection is just UDP?
We don't send any traffic here so it doesn't matter what type of connection it is. But since UDP is "fire and forget", we don't need to have anyone listening on the other end to trigger the warning.


ios/MullvadVPN/Supporting Files/Info.plist line 6 at r1 (raw file):

Previously, mojganii wrote…

@emils is the description decribtive enough for users?

This was a suggestion from Matilda which Emils approved in the office.

@buggmagnet buggmagnet force-pushed the trigger-the-local-network-alert-before-trying-local-ios-465 branch from f655cf8 to 03f17f3 Compare January 30, 2024 13:27
@buggmagnet buggmagnet merged commit 620b3f5 into main Jan 30, 2024
5 checks passed
@buggmagnet buggmagnet deleted the trigger-the-local-network-alert-before-trying-local-ios-465 branch January 30, 2024 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants