Add API access methods UI/part of backend IOS-369

[pronebird]

This PR introduces the UI and a part of backend for managing API access methods.

Backend

• AccessMethodRepository - a manager around API access method storage.
• PersistentAccessMethod - a persistent model stored within the access method storage and managed by the repo.
• PersistentProxyConfiguration - a persistent proxy configuration for an access method. Part of PersistentAccessMethod.
• ProxyConfigurationTester - an object responsible for testing API access method configuration. Typically operates on PersistentAccessMethod.
• ShadowsocksCipher - a wrapper type for SS cipher.

Backend limitations

• Only provides an in-memory storage.
• It's not wired up into the transport system we have.

API access methods

All API access methods can be disabled/enabled, but not all are configurable, since they may not have additional settings. Some API access methods like direct or bridges are permanent, meaning that they cannot be removed.

• Direct - direct communication, not configurable, not removable.
• Bridges - Mullvad bridges, not configurable, not removable.
• Socks5 - SOCKS5 proxy, configurable (server IP, port, username/password authentication), removable, many can be added.
• Shadowsocks - custom shadowsocks proxy, configurable (server IP, port, password, cipher), removable, many can be added.

User interface

User interface is implemented using:

• Diffable data sources.
• Combine, mostly CurrentValueSubject as it provides a two-way channel, but also is a reference type and can easily be shared between objects.
• Many view controllers use child table view controllers. That's because table view controllers automatically adjust content insets for keyboard.
• Cells are configured using content configuration.

File structure

Current structure puts everything related to the API access methods under Coordinators/Settings/APIAccess. As we discussed earlier, I think the file structure should be reworked and perhaps most of it should be uprooted somewhere else and united with View Controllers/.

Everything under APIAccess is structured either grouped by class of objects having some purpose or by sub-route (i.e Add/Edit/List).

Relationships

• Coordinators are at the top, manage view controllers using delegates.
• View controllers are injected with a vide model and intercator object that implements business logic.
• Interactors mostly follow command pattern and most of methods they implement do not take any arguments. That's because all dependencies are injected by coordinator and the view model is shared between. That makes working with UI a little easier.
• Part of interactor may mutate the view model, this is the case when testing API methods. This eliminates some code duplication.
• View controllers use a sink to react to changes to view model and reconfigure the diffable data source and some other parts of UI.

Table view cell configuration

All new code configures cells using content and background configuration. Given that we could register UITableViewCell directly with our table views, however we customize discolosure image and background configuration, so for that reason a BasicCell is used instead. It implements two protocols:

• DynamicBackgroundConfiguration - this protocol implements dynamic background configuration for cells which is necessary to update the cell background on selection or highlight.
  Before iOS 15 this was done by subclassing cells and updating background in func updateConfiguration(using state: UICellConfigurationState).
  Since iOS 15 the var configurationUpdateHandler: UITableViewCell.ConfigurationUpdateHandler? { get set } can be used instead.
• CustomCellDisclosureHandling - provides a way to set custom disclosure, old good SettingsDisclosureType that we have in Settings.

You will find various content configurations and views under Coordinators/Settings/APIAccess/Cells. Also look at UIBackgroundConfiguration+Extensions.swift and UIListContentConfiguration+Extensions.swift which provide factories for creating and styling standard content configurations and our background configurations.

Custom sheet

Part of UI is implemented using a custom action sheet which is put as an overlay above the view controllers when testing existing API access method or adding a new method. See AccessMethodActionSheetPresentation.

---

This change is 

[linear]

IOS-369 Add API connection methods to settings

Extend settings to allow storing data about API access methods. It should be possible to store the following 3 types of API access methods:

• Direct
• Any number of Mullvad bridges
• Any number of arbitrary proxy configs of possibly arbitrary type, but currently only remote SOCKS proxies

There can be placeholder UI for managing the methods, as the UI designs are not quite yet finished, like listing each method, adding one and enabling it. The UI should be only enabled in development builds, and can be a placeholder for now.

API method configuration must be editable before a user logs in. Any changes to the settings schema should only take effect in debug builds.

The settings will have to store:

• whether direct API access should be used
• whether and which bridge to use
• arbitrary amount of SOCKS configurations
  • SOCKS socket address
  • optional user name & password combo

As far as UI goes, for now it can be a list view of at least 2 items:

• Direct connection method, togglable on/off
• Bridge connection method, toggleable on/off
• Optionally also contains SOCKS proxy configs, each must be configurable and toggleable

[rablador]

Reviewed 7 of 96 files at r1.
Reviewable status: 6 of 96 files reviewed, 5 unresolved discussions (waiting on @pronebird)

---

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

    func add(_ method: PersistentAccessMethod) {
        guard !memoryStore.contains(where: { $0.id == method.id }) else { return }

Perhaps PersistentAccessMethod can adhere to Equatable to facilitate checks like this and others below?

---

ios/MullvadVPN/AccessMethodRepository/AccessMethodRepositoryProtocol.swift line 20 at r1 (raw file):

    func add(_ method: PersistentAccessMethod)

    /// Persist modified access method locating existing entry by id.

Update rather than persist, right?

---

ios/MullvadVPN/AccessMethodRepository/AccessMethodRepositoryProtocol.swift line 33 at r1 (raw file):

    func fetch(by id: UUID) -> PersistentAccessMethod?

    /// Fetch all access method from the persistent store.

Nit: method(s), here and below.

---

ios/MullvadVPN/Containers/Navigation/CustomNavigationController.swift line 11 at r1 (raw file):

import UIKit

/// Custom navigation controller that applies the

Documentation ends mid-sentence.

---

ios/MullvadVPN/Containers/Navigation/CustomNavigationController.swift line 30 at r1 (raw file):

        super.viewDidLayoutSubviews()

        // Navigation bar updates the prompt color on layout so we have to force out own appearance on each layout pass.

Nit: out -> our

[rablador]

Reviewed 1 of 3 files at r2, all commit messages.
Reviewable status: 7 of 96 files reviewed, 3 unresolved discussions (waiting on @pronebird)

[rablador]

Reviewed 10 of 96 files at r1, 1 of 1 files at r4.
Reviewable status: 18 of 96 files reviewed, 8 unresolved discussions (waiting on @pronebird)

---

ios/MullvadVPN/Coordinators/Settings/SettingsCoordinator.swift line 20 at r4 (raw file):

    /// VPN settings.
    case preferences

Nit: Change name to vpnSettings?

---

ios/MullvadVPN/Coordinators/Settings/SettingsCoordinator.swift line 69 at r4 (raw file):

    }

    /// Start the coordinator fllow.

Nit: "fllow"

---

ios/MullvadVPN/Coordinators/Settings/SettingsCoordinator.swift line 88 at r4 (raw file):

    ///   - route: the route to present.
    ///   - animated: whether transition should be animated.
    ///   - completion: a completion handler, typically called immediately for horizontal navigation and

Nit: Documentation ends mid-sentence.

---

ios/MullvadVPN/Coordinators/Settings/SettingsCoordinator.swift line 186 at r4 (raw file):

    ///   - result: the result of creating a child representing a route.
    ///   - animated: whether to animate the transition.
    private func push(from result: MakeChildResult, animated: Bool) {

Push "from" doesn't sound quite right, although I understand why it was chosen. When reading a call site, push(makeChild(for: .root)) sounds more intuitive I think.

---

ios/MullvadVPN/Coordinators/Settings/SettingsCoordinator.swift line 217 at r4 (raw file):

        /// Child coordinator that should be added to the children hierarchy.
        /// The child is responsile for presenting itself.

Nit: Responsible

[rablador]

Reviewed 2 of 96 files at r1.
Reviewable status: 20 of 96 files reviewed, 11 unresolved discussions (waiting on @pronebird)

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Add/AddAccessMethodViewController.swift line 252 at r4 (raw file):

        snapshot.appendItems([.protocol], toSection: .protocol)
        // Reconfigure the protocol item on the access method change.
        if let previousValue, previousValue.method != newValue.method {

Nit: `previousValue doesn't need unwrapping.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Common/ShadowsocksSectionHandler.swift line 48 at r4 (raw file):

        contentConfiguration.editingEvents.onChange = subject.bindTextAction(to: \.shadowsocks.port)
        contentConfiguration.textFieldProperties = .withSmartFeaturesDisabled()
        if case .phone = cell.traitCollection.userInterfaceIdiom {

Why distinguish here?

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Common/SocksSectionHandler.swift line 50 at r4 (raw file):

        contentConfiguration.editingEvents.onChange = subject.bindTextAction(to: \.socks.port)
        contentConfiguration.textFieldProperties = .withSmartFeaturesDisabled()
        if case .phone = cell.traitCollection.userInterfaceIdiom {

Why distinguish here?

[rablador]

Reviewed 5 of 96 files at r1.
Reviewable status: 25 of 96 files reviewed, 13 unresolved discussions (waiting on @pronebird)

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Add/AddAccessMethodViewController.swift line 260 at r4 (raw file):

        }

        switch newValue.method {

This switch should go inside the if newValue.method.hasProxyConfiguration condition above. If the section isn't present and there's an attempt to add items to it, the app will crash. Maybe in practice one doesn't exist without the other, but I think it's better to be on the safe side.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Cells/ButtonCellContentView.swift line 49 at r4 (raw file):

    }

    func configureSubviews(previousConfiguration: ButtonCellContentConfiguration? = nil) {

This is slightly confusing, since at a glance you would expect that by applying a config you update the view with new data (rather than old). previousConfiguration is only passed along here to remove an existing action in configureActions(), so maybe it would be better to scrap this function and call configureButton() and configureActions(previousConfiguration: previousConfiguration) directly from the var configuration: UIContentConfiguration setter instead.

[rablador]

Reviewed 6 of 96 files at r1, 2 of 2 files at r3.
Reviewable status: 33 of 96 files reviewed, 16 unresolved discussions (waiting on @pronebird)

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Cells/SwitchCellContentConfiguration.swift line 13 at r4 (raw file):

/// Content configuration presenting a label and switch control.
struct SwitchCellContentConfiguration: UIContentConfiguration, Equatable {
    struct TextProperties: Equatable {

Duplicate, use the one at the bottom instead.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Cells/SwitchCellContentView.swift line 97 at r4 (raw file):

    // MARK: - Accessibility

If we use the accessibility traits of the switch control, do we really need to set anything in the setters below?

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Cells/TextCellContentConfiguration.swift line 75 at r4 (raw file):

    /// Text field configuration.
    struct TextFieldProperties: Equatable {

Although a comprehensive is good in terms of completeness, perhaps we can trim the list where the default is usually desirable anyway. Then we can re-add attributes as needed. Also, we already introduce helpers for common use cases in extension TextCellContentConfiguration.TextFieldProperties below, so maybe those could do some of the heavy lifting as well.

[rablador]

Reviewed 3 of 96 files at r1.
Reviewable status: 36 of 96 files reviewed, 17 unresolved discussions (waiting on @pronebird)

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Cells/TextCellContentView.swift line 109 at r4 (raw file):

    }

    @objc private func handleTap(_ gestureRecognizer: UIGestureRecognizer) {

Nit: Comment about why we do this.

[rablador]

Reviewed 18 of 96 files at r1.
Reviewable status: 54 of 96 files reviewed, 26 unresolved discussions (waiting on @pronebird)

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Common/SocksSectionHandler.swift line 14 at r4 (raw file):

/// Type responsible for handling cells in socks table view section.
struct SocksSectionHandler {
    let tableStyle: UITableView.Style

This variable is never used.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Edit/EditAccessMethodViewController.swift line 287 at r4 (raw file):

    private func onDelete() {
        interactor.deleteAccessMethod()
        delegate?.controllerDidDeleteAccessMethod(self)

Nit: I think the controller here is implied and necessary to spell out.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Edit/EditAccessMethodViewController.swift line 292 at r4 (raw file):

    private func onSave() {
        interactor.saveAccessMethod()
        delegate?.controllerDidSaveAccessMethod(self)

Nit: I think the controller here is implied and necessary to spell out.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Edit/ProxyConfigurationInteractorProtocol.swift line 30 at r4 (raw file):

extension ProxyConfigurationInteractorProtocol {
    /// Start testing proxy configuration with data from view model.
    func startProxyConfigurationTest() {

Can't we remove this variation by simply adding a default nil value to the original function above?

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/List/ListAccessMethodCoordinator.swift line 46 at r4 (raw file):

    private func edit(item: ListAccessMethodItem) {
        // Remove previous edit coordinator to prevent accumulation.
        childCoordinators.filter { $0 is EditAccessMethodCoordinator }.forEach { $0.removeFromParent() }

Why do we do this here? Shouldn't popToList() handle this?

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/List/ListAccessMethodCoordinator.swift line 77 at r4 (raw file):

        // swiftlint:enable line_length

        let aboutController = AboutViewController(markdown: aboutMarkdown)

Nit: Perhaps the constructor can accept a title as well, to let the view controller handle the assignment itself?

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/List/ListAccessMethodCoordinator.swift line 86 at r4 (raw file):

        )

        aboutController.navigationItem.rightBarButtonItem = UIBarButtonItem(

Nit: I think this can be assumed to be standard and therefore handled inside the view controller.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Sheet/Content view/AccessMethodActionSheetConfiguration.swift line 22 at r4 (raw file):

    ///
    /// In this context, the sheet only offers user to cancel testing the access method.
    case proxyConfiguration

I think we should call this "testing" or something instead, to better reflect what I actually does.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Sheet/Content view/AccessMethodActionSheetContentConfiguration.swift line 33 at r4 (raw file):

extension AccessMethodActionSheetContentConfiguration.Status {
    /// The text label descirbing the status of testing and suitable for user presentation.

-> "describing"

[rablador]

Reviewed 14 of 96 files at r1.
Reviewable status: 68 of 96 files reviewed, 30 unresolved discussions (waiting on @pronebird)

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/List/ListAccessMethodViewController.swift line 49 at r4 (raw file):

    }

    override func viewDidLoad() {

Nit: Setup has been tidier in other view controllers. Perhaps we can keep it similarly structured?

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/List/ListAccessMethodViewController.swift line 127 at r4 (raw file):

        snapshot.appendItems(itemIdentifiers, toSection: .primary)

        for newFetchedItem in newFetchedItems {

Do we really need to do this, seeing that we replace the whole data source with new items on each update anyway?

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/List/ListAccessMethodViewControllerDelegate.swift line 14 at r4 (raw file):

    /// The view controller requests the delegate to present the about view.
    ///
    /// - Parameter controller: the calling view controller.

Nit: I think controller can be implied here, not necessary to include in name.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/List/ListAccessMethodViewControllerDelegate.swift line 19 at r4 (raw file):

    /// The view controller requests the delegate to present the add new method controller.
    ///
    /// - Parameter controller: the calling view controller.

Nit: I think controller can be implied here, not necessary to include in name.

[rablador]

Reviewed 12 of 96 files at r1.
Reviewable status: 80 of 96 files reviewed, 36 unresolved discussions (waiting on @pronebird)

---

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

    var socksViewModel: AccessMethodViewModel.Socks {
        guard case let .socks5(config) = self else {
            return AccessMethodViewModel.Socks()

This can only happen to an error somewhere, right? If so, I think we should log it. Same for shadowsocksViewModel below.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Pickers/AccessMethodProtocolPicker.swift line 47 at r4 (raw file):

        let method: AccessMethodKind

        var id: AccessMethodKind { method }

Nit: Do we need an explicit when it's the same as medthod?

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Pickers/ListItemPickerViewController.swift line 74 at r4 (raw file):

        guard !scrolledToSelection else { return }

        scrolledToSelection = true

Do we want to set this regardless of if a scroll was made or not?

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Sheet/Content view/AccessMethodActionSheetContentView.swift line 126 at r4 (raw file):

        // Text label is always the last one, so only add it into the stack if it's not there yet.
        if textLabel.superview == nil {

Nit: This is fine, but if horizontalStackView.arrangedSubviews.contains(textLabel) feels a little more in line with UI API:s.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Sheet/Presentation/AccessMethodActionSheetPresentation.swift line 84 at r4 (raw file):

            self.presentationView.alpha = 0
        } completion: { position in
            guard position == .end else { return }

We should not depend on .end here since it only tells us where the animation ended rather than that it ended at the end. If the animation stops at eg. the beginning the view will never be removed.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Sheet/Presentation/AccessMethodActionSheetPresentationView.swift line 12 at r4 (raw file):

/// The sheet presentation view implementing a layout similar to the one used by system action sheet.
class AccessMethodActionSheetPresentationView: UIView {

Nit: This view mimicks some of the behavior of a UIContentView + config. Maybe it should be converted to a real one instead?

[rablador]

Reviewed 14 of 96 files at r1, 2 of 3 files at r2.
Reviewable status: all files reviewed, 36 unresolved discussions (waiting on @pronebird)

[buggmagnet]

Reviewed 13 of 96 files at r1, 1 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 39 unresolved discussions (waiting on @pronebird)

---

ios/MullvadVPN/AccessMethodRepository/AccessMethodRepository.swift line 55 at r4 (raw file):

        guard let index = memoryStore.firstIndex(where: { $0.id == id }) else { return }

        // Prevent removing methods that have static UUIDs and always present.

nit:
and are always present

---

ios/MullvadVPN/AccessMethodRepository/ProxyConfigurationTester.swift line 13 at r4 (raw file):

/// A concrete implementation of an access method proxy configuration.
class ProxyConfigurationTester: ProxyConfigurationTesterProtocol {

nit
Let's schedule to remove this once we have a real implementation

---

ios/MullvadVPN/AccessMethodRepository/ShadowsocksCipher.swift line 12 at r4 (raw file):

/// Type representing a shadowsocks cipher.
struct ShadowsocksCipher: RawRepresentable, CustomStringConvertible, Equatable, Hashable, Codable {

this could (should ?) probably be an enum

[buggmagnet]

Reviewed 7 of 96 files at r1, 1 of 1 files at r6.
Reviewable status: 94 of 96 files reviewed, 40 unresolved discussions (waiting on @pronebird and @rablador)

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Cells/DynamicBackgroundConfiguration.swift line 12 at r6 (raw file):

/// Types providing dynamic background configuration based on cell configuration state.
protocol DynamicBackgroundConfiguration: UITableViewCell {

Why is this inheriting from UITableViewCell ?

[rablador]

Reviewed 2 of 2 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 39 unresolved discussions (waiting on @pronebird)

[rablador]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 39 unresolved discussions (waiting on @pronebird)

[buggmagnet]

Reviewed 56 of 96 files at r1, 1 of 2 files at r3, 1 of 1 files at r4, 1 of 2 files at r5.
Reviewable status: all files reviewed, 41 unresolved discussions (waiting on @pronebird)

---

ios/MullvadVPN/Containers/Navigation/UINavigationBar+Appearance.swift line 15 at r6 (raw file):

    /// - Note: Navigation bar does not provide the appearance configuration for the prompt.
    func overridePromptColor() {
        let promptView = subviews.first { $0.description.contains("Prompt") }

This looks like a hack that doesn't seem reliable to me.
If we really need to identify subviews, we can always use the tag property on UIView instead.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Common/SocksItemIdentifier.swift line 12 at r6 (raw file):

/// Item identifier used by diffable data sources implementing socks configuration.
enum SocksItemIdentifier: Hashable, CaseIterable {

This could probably be merged with the ShadowSocksIdentifier cheaply

[buggmagnet]

Reviewed 13 of 96 files at r1, 1 of 3 files at r2, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 41 unresolved discussions (waiting on @pronebird)

[buggmagnet]

Reviewed 11 of 11 files at r10, all commit messages.
Reviewable status: all files reviewed, 41 unresolved discussions (waiting on @pronebird)