Skip to content

Commit

Permalink
Prevent user from selecting same entry and exit relay for multihop
Browse files Browse the repository at this point in the history
  • Loading branch information
Jon Petersson committed Jul 9, 2024
1 parent c86bcc3 commit 68356f0
Show file tree
Hide file tree
Showing 7 changed files with 88 additions and 53 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -161,10 +161,6 @@ extension AddLocationsDataSource {
func nodeShouldBeSelected(_ node: LocationNode) -> Bool {
customListLocationNode.children.contains(node)
}

func excludedRelayTitle(_ node: LocationNode) -> String? {
nil // N/A
}
}

// MARK: - Toggle selection in table view
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ import Foundation
import MullvadREST
import MullvadTypes

class CountryLocationNode: LocationNode {}
class CityLocationNode: LocationNode {}
class HostLocationNode: LocationNode {}

class AllLocationDataSource: LocationDataSourceProtocol {
private(set) var nodes = [LocationNode]()

Expand Down Expand Up @@ -60,7 +64,7 @@ class AllLocationDataSource: LocationDataSourceProtocol {
) {
switch location {
case let .country(countryCode):
let countryNode = LocationNode(
let countryNode = CountryLocationNode(
name: serverLocation.country,
code: LocationNode.combineNodeCodes([countryCode]),
locations: [location],
Expand All @@ -73,7 +77,7 @@ class AllLocationDataSource: LocationDataSourceProtocol {
}

case let .city(countryCode, cityCode):
let cityNode = LocationNode(
let cityNode = CityLocationNode(
name: serverLocation.city,
code: LocationNode.combineNodeCodes([countryCode, cityCode]),
locations: [location],
Expand All @@ -88,7 +92,7 @@ class AllLocationDataSource: LocationDataSourceProtocol {
}

case let .hostname(countryCode, cityCode, hostCode):
let hostNode = LocationNode(
let hostNode = HostLocationNode(
name: relay.hostname,
code: LocationNode.combineNodeCodes([hostCode]),
locations: [location],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,6 @@ class LocationCell: UITableViewCell {

private func updateDisabled(_ isDisabled: Bool) {
locationLabel.alpha = isDisabled ? 0.2 : 1
collapseButton.alpha = isDisabled ? 0.2 : 1

if isDisabled {
accessibilityTraits.insert(.notEnabled)
Expand Down Expand Up @@ -339,14 +338,14 @@ extension LocationCell {
}
}

setExcludedRelayTitle(item.excludedRelayTitle)
setBehavior(behavior)
}

private func setExcludedRelayTitle(_ title: String?) {
if let title {
locationLabel.text! += " (\(title))"
updateDisabled(true)
func setExcluded(relayTitle: String? = nil) {
updateDisabled(true)

if let relayTitle {
locationLabel.text! += " (\(relayTitle))"
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,7 @@ struct LocationCellViewModel: Hashable {
}

extension [LocationCellViewModel] {
mutating func addSubNodes(
from item: LocationCellViewModel,
at indexPath: IndexPath,
excludedRelayTitleCallback: ((LocationNode) -> String?)?
) {
mutating func addSubNodes(from item: LocationCellViewModel, at indexPath: IndexPath) {
let section = LocationSection.allCases[indexPath.section]
let row = indexPath.row + 1

Expand All @@ -44,8 +40,7 @@ extension [LocationCellViewModel] {
section: section,
node: $0,
indentationLevel: item.indentationLevel + 1,
isSelected: false,
excludedRelayTitle: excludedRelayTitleCallback?($0)
isSelected: false
)
}

Expand All @@ -65,3 +60,53 @@ extension [LocationCellViewModel] {
}
}
}

extension LocationCellViewModel {
/* Exclusion of other locations in the same node tree as the currently excluded location
happens when there are no more hosts in that tree that can be selected.
We check this by doing the following, in order:

1. Count host names in the tree. More than one means that there are other locations than
the excluded one for the relay selector to choose from. No exlusion.

2. Count host names in the excluded node. More than one means that there are multiple
locations for the relay selector to choose from. No exlusion.

3. Check existance of a location in the tree that match the currently excluded location.
No match means no exclusion.
*/
func shouldExcludeLocation(_ excludedLocation: LocationCellViewModel?) -> Bool {
guard let excludedLocation else {
return false
}

var proxyNode = RootLocationNode(children: [node])

Check warning on line 83 in ios/MullvadVPN/View controllers/SelectLocation/LocationCellViewModel.swift

View workflow job for this annotation

GitHub Actions / Unit tests

variable 'proxyNode' was never mutated; consider changing to 'let' constant
let allLocations = Set(proxyNode.flattened.flatMap { $0.locations })
let hostCount = allLocations.filter { location in
if case .hostname = location { true } else { false }
}.count

// If the there are more than one selectable relay in the current node we don't need
// show this in the location tree and can return early.
guard hostCount == 1 else { return false }

let proxyExcludedNode = RootLocationNode(children: [excludedLocation.node])
let allExcludedLocations = Set(proxyExcludedNode.flattened.flatMap { $0.locations })
let excludedHostCount = allExcludedLocations.filter { location in
if case .hostname = location { true } else { false }
}.count

// If the there are more than one selectable relay in the excluded node we don't need
// show this in the location tree and can return early.
guard excludedHostCount == 1 else { return false }

var containsExcludedLocation = false
if allLocations.contains(where: { allExcludedLocations.contains($0) }) {
containsExcludedLocation = true
}

// If the tree doesn't contain the excluded node we do nothing, otherwise the
// required conditions have now all been met.
return containsExcludedLocation
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -142,10 +142,8 @@ final class LocationDataSource:
func setSelectedRelays(_ selectedRelays: RelaySelection) {
selectedLocation = mapSelection(from: selectedRelays.selected)

if selectedRelays.hasExcludedRelay {
excludedLocation = mapSelection(from: selectedRelays.excluded)
excludedLocation?.excludedRelayTitle = selectedRelays.excludedTitle
}
excludedLocation = mapSelection(from: selectedRelays.excluded)
excludedLocation?.excludedRelayTitle = selectedRelays.excludedTitle

tableView.reloadData()
}
Expand Down Expand Up @@ -246,9 +244,24 @@ final class LocationDataSource:
}

override func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {
// swiftlint:disable:next force_cast
let cell = super.tableView(tableView, cellForRowAt: indexPath) as! LocationCell
let cell = super.tableView(tableView, cellForRowAt: indexPath)
guard let cell = cell as? LocationCell, let item = itemIdentifier(for: indexPath) else {
return cell
}

cell.delegate = self

if item.shouldExcludeLocation(excludedLocation) {
// Only host locations should have an excluded title. Since custom list nodes contain
// all locations of all child nodes, its first location could possibly be a host.
// Therefore we need to check for that as well.
if case .hostname = item.node.locations.first, !(item.node is CustomListLocationNode) {
cell.setExcluded(relayTitle: excludedLocation?.excludedRelayTitle)
} else {
cell.setExcluded()
}
}

return cell
}
}
Expand All @@ -263,14 +276,6 @@ extension LocationDataSource {
func nodeShouldBeSelected(_ node: LocationNode) -> Bool {
false // N/A
}

func excludedRelayTitle(_ node: LocationNode) -> String? {
if nodeMatchesExcludedLocation(node) {
excludedLocation?.excludedRelayTitle
} else {
nil
}
}
}

extension LocationDataSource: UITableViewDelegate {
Expand Down Expand Up @@ -307,7 +312,7 @@ extension LocationDataSource: UITableViewDelegate {

func tableView(_ tableView: UITableView, shouldHighlightRowAt indexPath: IndexPath) -> Bool {
guard let item = itemIdentifier(for: indexPath) else { return false }
return !nodeMatchesExcludedLocation(item.node) && item.node.isActive
return !item.shouldExcludeLocation(excludedLocation) && item.node.isActive
}

func tableView(_ tableView: UITableView, indentationLevelForRowAt indexPath: IndexPath) -> Int {
Expand Down Expand Up @@ -360,9 +365,7 @@ extension LocationDataSource: LocationCellDelegate {
guard let indexPath = tableView.indexPath(for: cell),
let item = itemIdentifier(for: indexPath) else { return }

let items = toggledItems(for: cell, excludedRelayTitleCallback: { node in
self.excludedRelayTitle(node)
})
let items = toggledItems(for: cell)

updateDataSnapshot(with: items, reloadExisting: true, completion: {
self.scroll(to: item, animated: true)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ protocol LocationDiffableDataSourceProtocol: UITableViewDiffableDataSource<Locat
var sections: [LocationSection] { get }
func nodeShowsChildren(_ node: LocationNode) -> Bool
func nodeShouldBeSelected(_ node: LocationNode) -> Bool
func excludedRelayTitle(_ node: LocationNode) -> String?
}

extension LocationDiffableDataSourceProtocol {
Expand All @@ -40,10 +39,7 @@ extension LocationDiffableDataSourceProtocol {
}
}

func toggledItems(
for cell: LocationCell,
excludedRelayTitleCallback: ((LocationNode) -> String?)? = nil
) -> [[LocationCellViewModel]] {
func toggledItems(for cell: LocationCell) -> [[LocationCellViewModel]] {
guard let indexPath = tableView.indexPath(for: cell),
let item = itemIdentifier(for: indexPath) else { return [[]] }

Expand All @@ -54,7 +50,7 @@ extension LocationDiffableDataSourceProtocol {
item.node.showsChildren = !isExpanded

if !isExpanded {
locationList.addSubNodes(from: item, at: indexPath, excludedRelayTitleCallback: excludedRelayTitleCallback)
locationList.addSubNodes(from: item, at: indexPath)
} else {
locationList.removeSubNodes(from: item.node)
}
Expand Down Expand Up @@ -103,8 +99,7 @@ extension LocationDiffableDataSourceProtocol {
section: section,
node: childNode,
indentationLevel: indentationLevel,
isSelected: nodeShouldBeSelected(childNode),
excludedRelayTitle: excludedRelayTitle(childNode)
isSelected: nodeShouldBeSelected(childNode)
)
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,4 @@ struct RelaySelection {
var selected: UserSelectedRelays?
var excluded: UserSelectedRelays?
var excludedTitle: String?

var hasExcludedRelay: Bool {
if excluded?.locations.count == 1, case .hostname = excluded?.locations.first {
return true
}
return false
}
}

0 comments on commit 68356f0

Please sign in to comment.