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 socks5 authentication check from the TransportStrategy layer #5708

Merged

Conversation

mojganii
Copy link
Collaborator

@mojganii mojganii commented Jan 19, 2024

We don't necessarily need a switch-case for SOCKS authentication that involves retrieving credentials in the TransportStrategy when we can achieve the same outcome using an optional value for the authentication part. This pull request transforms the SocksAuthentication enum into a struct and makes it an optional variable in SocksConfiguration.


This change is Reviewable

@mojganii mojganii self-assigned this Jan 19, 2024
@mojganii mojganii added iOS Issues related to iOS enhancement labels Jan 19, 2024
@mojganii mojganii changed the title remove-socks5-authentication-check-from-the-transportstrategy-layer Remove socks5 authentication check from the TransportStrategy layer Jan 19, 2024
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 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mojganii)


ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/PersistentProxyConfiguration+ViewModel.swift line 24 at r1 (raw file):

        )

        socks.username = config.authentication?.username ?? ""

Nit: I know that username and password are "" by default, but perhaps we could do something like the snippet below in order to make it clearer that we're only assigning username and password when we actually have authentication.

Code snippet:

config.authentication.flatMap { authentication in
    socks.username = authentication.username
    socks.password = authentication.password
}

@mojganii mojganii force-pushed the refacorting-socks5-authentication-data-structure-455 branch from f51c485 to e21a122 Compare January 22, 2024 09:11
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 2 of 6 files at r2, all commit messages.
Reviewable status: 2 of 6 files reviewed, 1 unresolved discussion (waiting on @mojganii)

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 6 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@mojganii mojganii requested a review from rablador January 22, 2024 10:00
Copy link
Contributor

@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.

:lgtm:

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

@buggmagnet buggmagnet force-pushed the refacorting-socks5-authentication-data-structure-455 branch from e21a122 to 986e7d5 Compare January 23, 2024 14:05
@buggmagnet buggmagnet merged commit 3a74932 into main Jan 23, 2024
4 of 5 checks passed
@buggmagnet buggmagnet deleted the refacorting-socks5-authentication-data-structure-455 branch January 23, 2024 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants