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

Prevent user from selecting same entry and exit relay for multihop #6455

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -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 @@
}

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 @@
section: section,
node: $0,
indentationLevel: item.indentationLevel + 1,
isSelected: false,
excludedRelayTitle: excludedRelayTitleCallback?($0)
isSelected: false
)
}

Expand All @@ -65,3 +60,53 @@
}
}
}

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

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

View workflow job for this annotation

GitHub Actions / Screenshot 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
}
}
Loading