Allow users to import settings by pasting json blobs

[rablador]

Allow users to paste a JSON override into the app, which should then be saved. If the JSON blob contains extraneous keys, it should be rejected and the user should be notified with an error. When any override is active, the UI should indicate that overrides are in use, not necessarily show which overrides those might be. The user should be able to clear all overrides via the appropriate button.

Includes allow-users-to-import-json-files-ios-453as well:
Allow users to import JSON files. It should work the same way as pasting JSON files, but use a file picker instead.

---

This change is 

[linear]

IOS-451 Allow users to import settings by pasting JSON blobs

[mojganii]

Reviewable status: 0 of 14 files reviewed, 22 unresolved discussions (waiting on @rablador)

---

ios/MullvadSettings/IPOverride.swift line 30 at r1 (raw file):

    }

    init(hostname: String, ipv4Address: IPv4Address?, ipv6Address: IPv6Address?) throws {

do we really needs to consider nil values for IPs when they are imported through Json text or file in constructor? It makes sense to consider variables optional that's why user inputs invalid json format which we have but in constructor we knew these values should be valid and required. It's used in Unit test but I am thinking maybe this covers our assumption for unit case :

Code snippet:

    public init(hostname: String, ipv4Address: IPv4Address, ipv6Address: IPv6Address) {
        self.hostname = hostname
        self.ipv4Address = ipv4Address
        self.ipv6Address = ipv6Address
    }

---

ios/MullvadSettings/IPOverrideRepository.swift line 14 at r1 (raw file):

    func add(_ overrides: [IPOverride])
    func fetchAll() -> [IPOverride]
    func fetchByHostname(_ hostname: String) -> IPOverride?

for the code consistency let's keep the function name like the thing we have for fetch(by id: UUID) in AccessMethodRepositoryProtocol

func fetch(by hostname: String)

---

ios/MullvadSettings/IPOverrideRepository.swift line 16 at r1 (raw file):

    func fetchByHostname(_ hostname: String) -> IPOverride?
    func deleteAll()
    func parseData(_ data: Data) throws -> [IPOverride]

the same goes for here:
func parse(data: Data) throws-> [IPOverride]

---

ios/MullvadSettings/IPOverrideRepository.swift line 26 at r1 (raw file):

        overrides.forEach { override in
            if let existingOverrideIndex = storedOverrides.firstIndex(where: { $0.hostname == override.hostname }) {

do user really add/update the json? or any time user import them through file or text, shouldn't it be replaced with the old one?

---

ios/MullvadSettings/IPOverrideRepository.swift line 46 at r1 (raw file):

            try writeIpOverrides(storedOverrides)
        } catch {
            print("Could not add override(s): \(overrides) \nError: \(error)")

let's keep it in log

---

ios/MullvadSettings/IPOverrideRepository.swift line 62 at r1 (raw file):

            try SettingsManager.store.delete(key: .ipOverrides)
        } catch {
            print("Could not delete all overrides. \nError: \(error)")

let's keep it in log

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideCoordinator.swift line 14 at r1 (raw file):

import UIKit

class IPOverrideCoordinator: Coordinator, Presenting, SettingsChildCoordinator {

it seems we mixed the Intercator with Coordinator. in my opinion Coordinator is responsible for routing between deiffrent views not controlling the status and business logic

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideCoordinator.swift line 18 at r1 (raw file):

    let repository: IPOverrideRepositoryProtocol

    lazy var ipOverrideViewController: IPOverrideViewController = {

keep it private

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideCoordinator.swift line 35 at r1 (raw file):

    func start(animated: Bool) {
        navigationController.pushViewController(ipOverrideViewController, animated: animated)
        resetToDefaultStatus()

can not it be done in IPOverrideTextViewController?

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideCoordinator.swift line 38 at r1 (raw file):

    }

    private func showImportTextView() {

nit: this part could be put in and there is no need to make fiction for the thing calls once

 func controllerShouldShowTextImportView(_ controller: IPOverrideViewController)

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideCoordinator.swift line 79 at r1 (raw file):

        } catch {
            ipOverrideViewController.setStatus(.importFailed(context))
            print("Error importing ip overrides: \(error)")

having print is valid for logging?

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideStatus.swift line 11 at r1 (raw file):

import UIKit

enum IPOverrideStatus {

nit we could comform CustomStringConvertible and take advantage from description for statusTitle

Code snippet:

enum IPOverrideStatus : CustomStringConvertible {
    case active, noImports, importSuccessful(Context), importFailed(Context)

    enum Context {
        case file, text

        var description: String {
            switch self {
            case .file: "of file"
            case .text: "via text"
            }
        }
    }
    var description: String {
        switch self {
        case .active:
            NSLocalizedString(
                "IP_OVERRIDE_STATUS_TITLE_ACTIVE",
                tableName: "IPOverride",
                value: "Overrides active",
                comment: ""
            )
        case .noImports, .importFailed:
            NSLocalizedString(
                "IP_OVERRIDE_STATUS_TITLE_NO_IMPORTS",
                tableName: "IPOverride",
                value: "No overrides imported",
                comment: ""
            )
        case .importSuccessful:
            NSLocalizedString(
                "IP_OVERRIDE_STATUS_TITLE_IMPORT_SUCCESSFUL",
                tableName: "IPOverride",
                value: "Import successful",
                comment: ""
            )
        }
    }
    }

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideStatus.swift line 19 at r1 (raw file):

        var description: String {
            switch self {
            case .file: "of file"

shouldn't it be localized?

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideStatus.swift line 51 at r1 (raw file):

    }

    var statusIcon: UIImage? {

the name of enum implies Status. Including this keyword in the property names is redundant.

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideStatusView.swift line 14 at r1 (raw file):

    private lazy var titleLabel: UILabel = {
        let label = UILabel()
        label.font = .systemFont(ofSize: 22, weight: .bold)

based on design font size should be 15

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideViewController.swift line 59 at r1 (raw file):

        view.addConstrainedSubviews([containerView, clearButton]) {
            containerView.pinEdgesToSuperviewMargins(.all().excluding(.bottom))
            clearButton.pinEdgesToSuperviewMargins(.init([.leading(0), .trailing(0), .bottom(16)]))

based on google swift style guide :
clearButton.pinEdgesToSuperviewMargins(PinnableEdges([.leading(0), .trailing(0), .bottom(16)]))

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideViewControllerDelegate.swift line 11 at r1 (raw file):

import Foundation

protocol IPOverrideViewControllerDelegate: AnyObject {

nit :

Code snippet:

protocol IPOverrideViewControllerDelegate: AnyObject {
    func presentImportTextController(_ controller: IPOverrideViewController)
    func presentImportFileController(_ controller: IPOverrideViewController)
    func clearAllOverrides(_ controller: IPOverrideViewController)
}

---

ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionContentView.swift line 73 at r1 (raw file):

            This logs out all devices using this account and all \
            VPN access will be denied even if there is time left on the account. \
            Enter the last 4 digits of the account number and hit "Delete account" \

is it a minor bug fix on account deletion?

---

ios/MullvadVPNTests/IPOverrideRepositoryTests.swift line 33 at r1 (raw file):

    }

    func testCannotParseOverridesWithUnsupportedKeys() throws {

testFailedToParseOverridesWithUnsupportedKeys

---

ios/MullvadVPNTests/IPOverrideRepositoryTests.swift line 37 at r1 (raw file):

    }

    func testCannotParseOverridesWithMalformedValues() throws {

failed is be better key in comparison to Cannot.

---

ios/MullvadVPNTests/IPOverrideRepositoryTests.swift line 41 at r1 (raw file):

    }

    func testCanCreateOverrideWithOneAddress() throws {

the test is included functions for repository. if there is need to do test for IPOverride it should be better in another test case.e.g IPOverrideTests

---

ios/MullvadVPNTests/IPOverrideRepositoryTests.swift line 50 at r1 (raw file):

    }

    func testCanAddOverride() throws {

testAddIPOverride

[rablador]

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

---

ios/MullvadSettings/IPOverride.swift line 30 at r1 (raw file):

Previously, mojganii wrote…

do we really needs to consider nil values for IPs when they are imported through Json text or file in constructor? It makes sense to consider variables optional that's why user inputs invalid json format which we have but in constructor we knew these values should be valid and required. It's used in Unit test but I am thinking maybe this covers our assumption for unit case :

According to spec, only one of the two is required, which means that users can input json without values for either ipv4 or ipv6.

---

ios/MullvadSettings/IPOverrideRepository.swift line 14 at r1 (raw file):

Previously, mojganii wrote…

for the code consistency let's keep the function name like the thing we have for fetch(by id: UUID) in AccessMethodRepositoryProtocol

func fetch(by hostname: String)

Yes and no. I agree with the naming convention in general, but UUID has a specific type, which makes fetch(by: <UUID>) make sense at call site. For hostname requiring a string, fetch(by: <String>) doesn't tell you anything about what's required without looking at documentation or full function definition.

---

ios/MullvadSettings/IPOverrideRepository.swift line 16 at r1 (raw file):

Previously, mojganii wrote…

the same goes for here:
func parse(data: Data) throws-> [IPOverride]

This one I agree with. :)

---

ios/MullvadSettings/IPOverrideRepository.swift line 26 at r1 (raw file):

Previously, mojganii wrote…

do user really add/update the json? or any time user import them through file or text, shouldn't it be replaced with the old one?

The spec says that we should add to the list rather than replace it. Also, only affected values should be changes, eg. only the ipv4 address if ipv6 isn't in the data.

---

ios/MullvadSettings/IPOverrideRepository.swift line 46 at r1 (raw file):

Previously, mojganii wrote…

let's keep it in log

Done.

---

ios/MullvadSettings/IPOverrideRepository.swift line 62 at r1 (raw file):

Previously, mojganii wrote…

let's keep it in log

Done.

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideCoordinator.swift line 14 at r1 (raw file):

Previously, mojganii wrote…

it seems we mixed the Intercator with Coordinator. in my opinion Coordinator is responsible for routing between deiffrent views not controlling the status and business logic

I'll see if I can move things around a bit.

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideCoordinator.swift line 18 at r1 (raw file):

Previously, mojganii wrote…

keep it private

Done.

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideCoordinator.swift line 35 at r1 (raw file):

Previously, mojganii wrote…

can not it be done in IPOverrideTextViewController?

I'll move things around a bit.

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideCoordinator.swift line 38 at r1 (raw file):

Previously, mojganii wrote…

nit: this part could be put in and there is no need to make fiction for the thing calls once

 func controllerShouldShowTextImportView(_ controller: IPOverrideViewController)

I'll move things around a bit.

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideCoordinator.swift line 79 at r1 (raw file):

Previously, mojganii wrote…

having print is valid for logging?

Changed to log.

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideStatus.swift line 11 at r1 (raw file):

Previously, mojganii wrote…

nit we could comform CustomStringConvertible and take advantage from description for statusTitle

Fixed.

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideStatus.swift line 19 at r1 (raw file):

Previously, mojganii wrote…

shouldn't it be localized?

Used in "statusDescription" further down in the file to form a complete sentence and therefore not localized here.

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideStatus.swift line 51 at r1 (raw file):

Previously, mojganii wrote…

the name of enum implies Status. Including this keyword in the property names is redundant.

Done.

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideStatusView.swift line 14 at r1 (raw file):

Previously, mojganii wrote…

based on design font size should be 15

Accidentally took the wrong value for designs. Fixed.

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideViewController.swift line 59 at r1 (raw file):

Previously, mojganii wrote…

based on google swift style guide :
clearButton.pinEdgesToSuperviewMargins(PinnableEdges([.leading(0), .trailing(0), .bottom(16)]))

Done.

---

ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionContentView.swift line 73 at r1 (raw file):

Previously, mojganii wrote…

is it a minor bug fix on account deletion?

Changed "typo" from account :to account:.

---

ios/MullvadVPNTests/IPOverrideRepositoryTests.swift line 33 at r1 (raw file):

Previously, mojganii wrote…

testFailedToParseOverridesWithUnsupportedKeys

Done.

---

ios/MullvadVPNTests/IPOverrideRepositoryTests.swift line 37 at r1 (raw file):

Previously, mojganii wrote…

failed is be better key in comparison to Cannot.

Done.

---

ios/MullvadVPNTests/IPOverrideRepositoryTests.swift line 41 at r1 (raw file):

Previously, mojganii wrote…

the test is included functions for repository. if there is need to do test for IPOverride it should be better in another test case.e.g IPOverrideTests

Done.

---

ios/MullvadVPNTests/IPOverrideRepositoryTests.swift line 50 at r1 (raw file):

Previously, mojganii wrote…

testAddIPOverride

Done.

[rablador]

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

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideCoordinator.swift line 14 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

I'll see if I can move things around a bit.

I mostly wanted to keep things simple since, but I agree that it does break pattern.

[rablador]

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

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideCoordinator.swift line 14 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

I mostly wanted to keep things simple since, but I agree that it does break pattern.

Done.

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideCoordinator.swift line 35 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

I'll move things around a bit.

Done.

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideCoordinator.swift line 38 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

I'll move things around a bit.

Done.

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideViewControllerDelegate.swift line 11 at r1 (raw file):

Previously, mojganii wrote…

nit :

Changed.

[buggmagnet]

Reviewed 3 of 14 files at r1, 7 of 10 files at r2, 4 of 6 files at r3, 1 of 2 files at r4, all commit messages.
Reviewable status: 15 of 16 files reviewed, 28 unresolved discussions (waiting on @mojganii and @rablador)

---

ios/MullvadSettings/IPOverride.swift line 36 at r4 (raw file):

        if self.ipv4Address.isNil && self.ipv6Address.isNil {
            throw NSError(domain: "IPOverrideInitDomain", code: NSFormattingError)

We could / should probably add a constant for the domain / and why not use a custom error here too ?

---

ios/MullvadSettings/IPOverrideRepository.swift line 25 at r4 (raw file):

    public init() {}

    public func add(_ overrides: [IPOverride]) {

Wouldn't it be simpler to use sets here (unless I misunderstood this) ?

public func add(_ overrides: [IPOverride]) {
    let storedOverrides = Set(fetchAll())
    var overridesSet = Set(overrides)
    overridesSet.formUnion(storedOverrides)
    do {
        try writeIpOverrides(overridesSet.map { $0 })
    } catch {
        logger.error("Could not add override(s): \(overrides) \nError: \(error)")
    }
}

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideInteractor.swift line 37 at r4 (raw file):

    }

    func `import`(url: URL) {

I like the name of the API here 👍
swifty style, looks good !

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideInteractor.swift line 63 at r4 (raw file):

        }

        setDefaultStatus(delay: .seconds(10))

It would be nice to have a comment explaining the 10 seconds delay

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideViewController.swift line 244 at r4 (raw file):

    func documentPicker(_ controller: UIDocumentPickerViewController, didPickDocumentsAt urls: [URL]) {
        if let url = urls.first {
            interactor.import(url: url)

This won't work unless you do the following changes

guard url.startAccessingSecurityScopedResource() else {
    return //Indicate that we couldn't access the ressource here, maybe throw ?
}
interactor.import(url: url)
url.stopAccessingSecurityScopedResource()

---

ios/MullvadVPNTests/IPOverrideRepositoryTests.swift line 49 at r4 (raw file):

        let override1 = try IPOverride(hostname: "Host 1", ipv4Address: .any, ipv6Address: nil)
        let override2 = try IPOverride(hostname: "Host 1", ipv4Address: .allHostsGroup, ipv6Address: .broadcast)
        repository.add([override1, override2])

Technically, a user cannot add 2 different overrides in one action
It would be better IMHO to mimic the users action to make sure we test the right things

        let override1 = try IPOverride(hostname: "Host 1", ipv4Address: .any, ipv6Address: nil)
        repository.add([override1])
        let override2 = try IPOverride(hostname: "Host 1", ipv4Address: .allHostsGroup, ipv6Address: .broadcast)
        repository.add([override2])

---

ios/MullvadVPNTests/IPOverrideTests.swift line 16 at r4 (raw file):

    func testCanParseOverrides() throws {
        XCTAssertNoThrow(try parseData(from: overrides))

We should also make sure that the values are correclty imported given that we're relying on Apple's parser for IPv4 and IPv6 addresses, at least make sure that they're compatible with our own types AnyIPaddress

[rablador]

Reviewable status: 15 of 16 files reviewed, 28 unresolved discussions (waiting on @buggmagnet, @mojganii, and @pinkisemils)

---

ios/MullvadSettings/IPOverride.swift line 36 at r4 (raw file):

Previously, buggmagnet wrote…

We could / should probably add a constant for the domain / and why not use a custom error here too ?

Done, created a custom error.

---

ios/MullvadSettings/IPOverrideRepository.swift line 25 at r4 (raw file):

Previously, buggmagnet wrote…

Wouldn't it be simpler to use sets here (unless I misunderstood this) ?

public func add(_ overrides: [IPOverride]) {
    let storedOverrides = Set(fetchAll())
    var overridesSet = Set(overrides)
    overridesSet.formUnion(storedOverrides)
    do {
        try writeIpOverrides(overridesSet.map { $0 })
    } catch {
        logger.error("Could not add override(s): \(overrides) \nError: \(error)")
    }
}

Indeed. I'm just not sure about the spec here. I interpreted it as replacing only values that are clashing, down to individual values inside the overrides themselves. Looking at it again it might be enough to simply replace the entirety of a host with a new one. Waiting for answer from @pinkisemils.

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideInteractor.swift line 37 at r4 (raw file):

Previously, buggmagnet wrote…

I like the name of the API here 👍
swifty style, looks good !

:)

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideInteractor.swift line 63 at r4 (raw file):

Previously, buggmagnet wrote…

It would be nice to have a comment explaining the 10 seconds delay

Done.

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideViewController.swift line 244 at r4 (raw file):

Previously, buggmagnet wrote…

This won't work unless you do the following changes

guard url.startAccessingSecurityScopedResource() else {
    return //Indicate that we couldn't access the ressource here, maybe throw ?
}
interactor.import(url: url)
url.stopAccessingSecurityScopedResource()

It seems to be dependent on where you store your files. Eg. Downloads doesn't require scope, but I think we need this change anyway since people can store file basically anywhere.

---

ios/MullvadVPNTests/IPOverrideRepositoryTests.swift line 49 at r4 (raw file):

Previously, buggmagnet wrote…

Technically, a user cannot add 2 different overrides in one action
It would be better IMHO to mimic the users action to make sure we test the right things

        let override1 = try IPOverride(hostname: "Host 1", ipv4Address: .any, ipv6Address: nil)
        repository.add([override1])
        let override2 = try IPOverride(hostname: "Host 1", ipv4Address: .allHostsGroup, ipv6Address: .broadcast)
        repository.add([override2])

Done.

---

ios/MullvadVPNTests/IPOverrideTests.swift line 16 at r4 (raw file):

Previously, buggmagnet wrote…

We should also make sure that the values are correclty imported given that we're relying on Apple's parser for IPv4 and IPv6 addresses, at least make sure that they're compatible with our own types AnyIPaddress

Done.

[buggmagnet]

Reviewed 8 of 8 files at r5, all commit messages.
Reviewable status: all files reviewed, 24 unresolved discussions (waiting on @mojganii, @pinkisemils, and @rablador)

---

ios/MullvadVPN/Extensions/URL+Scoping.swift line 13 at r5 (raw file):

extension URL {
    func securelyScoped(_ completionHandler: (Self) -> Void) {
        if startAccessingSecurityScopedResource() {

I know I wrote that snippet, but we should probably indicate failure if startAccessingSecurityScopedResource() returns false, otherwise the UI will just deadlock won't it ?
Because we will never end up calling resetToDefaultStatus

It's okay to fix this in the followup PR

[mojganii]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @buggmagnet, @pinkisemils, and @rablador)

[buggmagnet]

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

[rablador]

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

---

ios/MullvadVPN/Extensions/URL+Scoping.swift line 13 at r5 (raw file):

Previously, buggmagnet wrote…

I know I wrote that snippet, but we should probably indicate failure if startAccessingSecurityScopedResource() returns false, otherwise the UI will just deadlock won't it ?
Because we will never end up calling resetToDefaultStatus

It's okay to fix this in the followup PR

Will follow up in next PR.