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

Reflect obfuscation protocol in UI ios 398 #5548

Merged
merged 1 commit into from
Dec 6, 2023

Conversation

buggmagnet
Copy link
Contributor

@buggmagnet buggmagnet commented Dec 4, 2023

Final part of the UDP-over-TCP obfuscation feature.

This PR :

  • Refactors TunnelControlViewModel
  • Unifies how TunnelControlView gets its data source (more could be done, but the PR was already big enough)
  • Resurfaces the port and protocol layer used by the Packet Tunnel to connect to an endpoint
  • Introduces an ObfuscateConnection method that will take a MullvadEndpoint and seamlessly create an obfsucator to use

This change is Reviewable

@buggmagnet buggmagnet added the iOS Issues related to iOS label Dec 4, 2023
@buggmagnet buggmagnet self-assigned this Dec 4, 2023
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 15 of 15 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @buggmagnet)


ios/MullvadVPN/View controllers/Tunnel/TunnelControlView.swift line 156 at r1 (raw file):

        countryLabel.attributedText = attributedStringForLocation(string: model.country)
        connectionPanel.connectedRelayName = model.connectedRelayName
        connectionPanel.dataSource = model.connectionPanel

Duplicate line.


ios/MullvadVPN/View controllers/Tunnel/TunnelControlViewModel.swift line 38 at r1 (raw file):

    }

    init(from other: TunnelControlViewModel) {

This seems to never be used.


ios/PacketTunnelCore/Actor/PacketTunnelActor.swift line 344 at r1 (raw file):

        reason: ReconnectReason
    ) throws -> ConnectionState? {
        guard let connectionState = try makeConnectionState(nextRelay: nextRelay, settings: settings, reason: reason)

We now have three function in a chain handling the creation of a connection state, which seems unnecessarily deep. Also, we always overwrite the last three params in ConnectionState before returning, which makes things feel even more disjointed. Could we perhaps refactor these three function into one or two somehow?


ios/PacketTunnelCore/Actor/ProtocolObfuscator.swift line 35 at r1 (raw file):

    public var transportLayer: TransportLayer? {
        guard let tunnelObfuscator else { return nil }
        return tunnelObfuscator.transportLayer

Nit: Simplify to return tunnelObfuscator?.transportLayer?

@buggmagnet buggmagnet force-pushed the reflect-obfuscation-protocol-in-ui-ios-398 branch from 56ac2e6 to 2909b92 Compare December 6, 2023 10:15
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: 6 of 15 files reviewed, 4 unresolved discussions (waiting on @rablador)


ios/MullvadVPN/View controllers/Tunnel/TunnelControlView.swift line 156 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Duplicate line.

Done.


ios/MullvadVPN/View controllers/Tunnel/TunnelControlViewModel.swift line 38 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

This seems to never be used.

You're right, it looks like I forgot to remove that from an old revision. Good catch !


ios/PacketTunnelCore/Actor/PacketTunnelActor.swift line 344 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

We now have three function in a chain handling the creation of a connection state, which seems unnecessarily deep. Also, we always overwrite the last three params in ConnectionState before returning, which makes things feel even more disjointed. Could we perhaps refactor these three function into one or two somehow?

Done.


ios/PacketTunnelCore/Actor/ProtocolObfuscator.swift line 35 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: Simplify to return tunnelObfuscator?.transportLayer?

Yup !

@buggmagnet buggmagnet force-pushed the reflect-obfuscation-protocol-in-ui-ios-398 branch from 2909b92 to 8ab686c Compare December 6, 2023 10:34
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 8 of 9 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@buggmagnet buggmagnet force-pushed the reflect-obfuscation-protocol-in-ui-ios-398 branch from 8ab686c to ebc80ae Compare December 6, 2023 10:50
@buggmagnet buggmagnet force-pushed the reflect-obfuscation-protocol-in-ui-ios-398 branch from ebc80ae to 0ee4187 Compare December 6, 2023 10:58
@buggmagnet buggmagnet merged commit ac3e222 into main Dec 6, 2023
4 of 5 checks passed
@buggmagnet buggmagnet deleted the reflect-obfuscation-protocol-in-ui-ios-398 branch December 6, 2023 10:58
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.

2 participants