Enable testing API access with a given configuration

[buggmagnet]

This PR implements the "Test" button for testing our API via a custom configuration.

---

This change is 

[linear]

IOS-370 Allow testing an API access method

[mojganii]

Reviewable status: 0 of 15 files reviewed, 3 unresolved discussions (waiting on @buggmagnet)

---

ios/MullvadREST/ApiHandlers/HeadRequest.swift line 13 at r1 (raw file):

extension REST {
    public struct HeadRequest {

I'm not sure If I understood correctly or not. my understanding is this request is like a ping address. is HeadRequest well-defined name for that? the names seems to me every request with head method can go through with HeadRequest

---

ios/MullvadREST/Transport/ConfiguredTransportProvider.swift line 14 at r1 (raw file):

/// Allows the creation of `RESTTransport` objects that bypass the network routing logic provided by `TransportProvider`.
public class ConfiguredTransportProvider {

shall it be a part of MullvadREST framework? shouldn't it be better ConfigurableTransportProvider or ProxyConfigurationTransportProvider?

---

ios/MullvadRESTTests/HeadRequestTests.swift line 31 at r1 (raw file):

        let request = REST.HeadRequest(transport: transport)

        let successfulRequestExpectation = expectation(description: "HEAD request completed")

we expect the task to get done by error.so successfulRequestExpectation should be changed I believe

[buggmagnet]

Reviewable status: 0 of 16 files reviewed, 3 unresolved discussions (waiting on @mojganii)

---

ios/MullvadREST/ApiHandlers/HeadRequest.swift line 13 at r1 (raw file):

this request is like a ping address

That is not correct.
What is usually called ping request is in most cases an ICMP Ping, which happens at the Internet layer of the OSI model.

This is a HEAD request, which implies it runs at the application layer (because it's HTTP).

is HeadRequest well-defined name for that? the names seems to me every request with head method can go through with HeadRequest

I don't understand what you're suggesting.

---

ios/MullvadREST/Transport/ConfiguredTransportProvider.swift line 14 at r1 (raw file):

shall it be a part of MullvadREST framework?

It's already part of it. The whole REST namespace only makes sense if we're having types that have conflicting names with Apple's own types, otherwise, it's just visual noise.

shouldn't it be better ConfigurableTransportProvider or ProxyConfigurationTransportProvider?

I think ProxyConfiguredTransportProvider is a good middle ground. It implies we are configuring a transport provider with a proxy configuration, so I'll go with that.

---

ios/MullvadRESTTests/HeadRequestTests.swift line 31 at r1 (raw file):

Previously, mojganii wrote…

we expect the task to get done by error.so successfulRequestExpectation should be changed I believe

oh good point, it should be a failed request here, thanks for catching that !

[rablador]

Reviewed 4 of 15 files at r1, 1 of 6 files at r2.
Reviewable status: 4 of 16 files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @mojganii)

---

ios/MullvadREST/Transport/ConfiguredTransportProvider.swift line 14 at r1 (raw file):

Previously, mojganii wrote…

shall it be a part of MullvadREST framework? shouldn't it be better ConfigurableTransportProvider or ProxyConfigurationTransportProvider?

I like ProxyConfigurationTransportProvider

---

ios/MullvadREST/Transport/ConfiguredTransportProvider.swift line 55 at r1 (raw file):

            return URLSessionSocks5Transport(
                urlSession: urlSession,
                configuration: Socks5Configuration(

Nit: This sort of circumvents the decision to keep .noAuthentication around instead of making user and pass optional. I think this looks and functions well, but in TransportStrategy we do check for both cases and create our config accordingly.

---

ios/MullvadREST/Transport/ProxyConfiguredTransportProvider.swift line 1 at r2 (raw file):

//

This file is a duplicate of ConfiguredTransportProvider.swift.

[rablador]

Reviewed 5 of 15 files at r1, 5 of 6 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @buggmagnet and @mojganii)

---

ios/MullvadVPN/AccessMethodRepository/ProxyConfigurationTester.swift line 30 at r3 (raw file):

            let request = REST.HeadRequest(transport: transport)
            headRequest = request
            cancellable = request.makeRequest { error in

I know that we discussed the longevity of the request object before. Does the cancellable not keep the request alive then?

[buggmagnet]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @mojganii and @rablador)

---

ios/MullvadREST/Transport/ConfiguredTransportProvider.swift line 14 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

I like ProxyConfigurationTransportProvider

Meh I'm not a big fan of it, but I'm not gonna die on that hill either, consider it done !

---

ios/MullvadREST/Transport/ConfiguredTransportProvider.swift line 55 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: This sort of circumvents the decision to keep .noAuthentication around instead of making user and pass optional. I think this looks and functions well, but in TransportStrategy we do check for both cases and create our config accordingly.

The code was written before we had that, and I forgot to update it. Will do now, good catch !

---

ios/MullvadREST/Transport/ProxyConfiguredTransportProvider.swift line 1 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

This file is a duplicate of ConfiguredTransportProvider.swift.

Argh Xcode !!!!

---

ios/MullvadVPN/AccessMethodRepository/ProxyConfigurationTester.swift line 30 at r3 (raw file):

Previously, rablador (Jon Petersson) wrote…

I know that we discussed the longevity of the request object before. Does the cancellable not keep the request alive then?

If no one holds the cancellable, it gets deallocated when it gets out of scope here. But also we need to keep it around to cancel it anyway. Unless I misunderstood what you were suggesting ?

[mojganii]

Reviewable status: 10 of 16 files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadREST/ApiHandlers/HeadRequest.swift line 13 at r1 (raw file):

Previously, buggmagnet wrote…

this request is like a ping address

That is not correct.
What is usually called ping request is in most cases an ICMP Ping, which happens at the Internet layer of the OSI model.

This is a HEAD request, which implies it runs at the application layer (because it's HTTP).

is HeadRequest well-defined name for that? the names seems to me every request with head method can go through with HeadRequest

I don't understand what you're suggesting.

Certainly, considering your additional context, how about the name "ServerVisibilityChecker" or "ConnectionVerifier"? These names emphasise the functionality of the class in checking if the application can see our servers without explicitly mentioning the HTTP method in the name.

HeadRequest suggests that I can pass any 'HEAD' request through it but it's not correct.

[rablador]

Reviewed 6 of 6 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @mojganii)

---

ios/MullvadVPN/AccessMethodRepository/ProxyConfigurationTester.swift line 30 at r3 (raw file):

Previously, buggmagnet wrote…

If no one holds the cancellable, it gets deallocated when it gets out of scope here. But also we need to keep it around to cancel it anyway. Unless I misunderstood what you were suggesting ?

I was thinking that the headRequest would not go out of scope so long as it's bound to the cancellable.

[buggmagnet]

Reviewable status: 12 of 16 files reviewed, 4 unresolved discussions (waiting on @mojganii and @rablador)

---

ios/MullvadVPN/AccessMethodRepository/ProxyConfigurationTester.swift line 30 at r3 (raw file):

Previously, rablador (Jon Petersson) wrote…

I was thinking that the headRequest would not go out of scope so long as it's bound to the cancellable.

The request itself is the cancellable that's returned. We only need to do this because of the Shadowsocks proxy case using RAII design, if we don't hold it around, the local server dies before the request is finished, and the request fails.

---

ios/MullvadREST/ApiHandlers/HeadRequest.swift line 13 at r1 (raw file):

HeadRequest suggests that I can pass any 'HEAD' request through it but it's not correct.

Technically, any HEAD request done to our server will suffice, we just decided to use the api-addrs endpoint for that.

That being said, you do have a point that we don't need to specify that it's a HEAD request, or rather, it doesn't tell us how it's used.

I will rename this APIAvailabilityTestRequest instead.

[mojganii]

Reviewable status: 12 of 16 files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadVPN/AccessMethodRepository/ProxyConfigurationTester.swift line 19 at r5 (raw file):

    private var cancellable: MullvadTypes.Cancellable?
    private let transportProvider: ProxyConfigurationTransportProvider
    private var headRequest: REST.APIAvailabilityTestRequest?

shouldn't it be apiAvailabilityTestRequest?

---

ios/MullvadVPN/AccessMethodRepository/ProxyConfigurationTester.swift line 31 at r5 (raw file):

            headRequest = request
            cancellable = request.makeRequest { error in
                DispatchQueue.main.async {

it's better to do context switching in interactor.

---

ios/MullvadREST/ApiHandlers/APIAvailabilityTestRequest.swift line 2 at r5 (raw file):

//
//  HeadRequest.swift

it should be renamed

[rablador]

Reviewed 4 of 4 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet)

[buggmagnet]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mojganii)

---

ios/MullvadVPN/AccessMethodRepository/ProxyConfigurationTester.swift line 19 at r5 (raw file):

Previously, mojganii wrote…

shouldn't it be apiAvailabilityTestRequest?

From the swift guidelines
Name variables, parameters, and associated types according to their roles, rather than their type constraints.

---

ios/MullvadVPN/AccessMethodRepository/ProxyConfigurationTester.swift line 31 at r5 (raw file):

Previously, mojganii wrote…

it's better to do context switching in interactor.

Then I'd have to do that in both the AddAccessMethodInteractor and EditAccessMethodInteractor which is suboptimal IMHO

[mojganii]

Reviewable status: 15 of 16 files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadVPN/AccessMethodRepository/ProxyConfigurationTester.swift line 31 at r5 (raw file):

Previously, buggmagnet wrote…

Then I'd have to do that in both the AddAccessMethodInteractor and EditAccessMethodInteractor which is suboptimal IMHO

It's not a big deal.but If were I would do that at the layer is closer to the UI

[buggmagnet]

Reviewable status: 15 of 16 files reviewed, all discussions resolved (waiting on @rablador)

---

ios/MullvadREST/ApiHandlers/APIAvailabilityTestRequest.swift line 2 at r5 (raw file):

Previously, mojganii wrote…

it should be renamed

Done

[rablador]

Reviewed 1 of 1 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved