-
Notifications
You must be signed in to change notification settings - Fork 356
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
Add counter and 'all' switch to DNS content blockers #6800
Add counter and 'all' switch to DNS content blockers #6800
Conversation
c44e08b
to
d8da5b6
Compare
d8da5b6
to
f555080
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The requirement stated that the custom DNS view should be rewritten in SwiftUI, but I opted to not do this. The PR is easily implemented in UIKit and mostly consists of data juggling.
Reviewable status: 0 of 6 files reviewed, all discussions resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rablador)
ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsViewModel.swift
line 107 at r1 (raw file):
static let defaultWireGuardPorts: [UInt16] = [51820, 53]
we can map the content blockers which we have in DataSource
into the model through a dictionary: and having access to the values through setter
and getter
. in this way if we add another options into content blockers the only thing we need to add into model is a variable which is reading and writing the dictionary. what do you think?
Code snippet:
struct VPNSettingsViewModel: Equatable {
private(set) var blockAll : Bool {
get {
contentBlockers[.blockAll] ?? false
} set {
contentBlockers[.blockAll] = newValue
}
}
private(set) var blockAdvertising: Bool {
get {
contentBlockers[.blockAdvertising] ?? false
} set {
contentBlockers[.blockAdvertising] = newValue
}
}
private(set) var blockTracking: Bool {
get {
contentBlockers[.blockTracking] ?? false
} set {
contentBlockers[.blockTracking] = newValue
}
}
private(set) var blockMalware: Bool {
get {
contentBlockers[.blockMalware] ?? false
} set {
contentBlockers[.blockMalware] = newValue
}
}
private(set) var blockAdultContent: Bool {
get {
contentBlockers[.blockAdultContent] ?? false
} set {
contentBlockers[.blockAdultContent] = newValue
}
}
private(set) var blockGambling: Bool {
get {
contentBlockers[.blockGambling] ?? false
} set {
contentBlockers[.blockGambling] = newValue
}
}
private(set) var blockSocialMedia: Bool {
get {
contentBlockers[.blockSocialMedia] ?? false
} set {
contentBlockers[.blockSocialMedia] = newValue
}
}
private(set) var enableCustomDNS: Bool
private(set) var wireGuardPort: UInt16?
var customDNSDomains: [DNSServerEntry]
var availableWireGuardPortRanges: [[UInt16]] = []
private(set) var obfuscationState: WireGuardObfuscationState
private(set) var obfuscationPort: WireGuardObfuscationPort
private(set) var quantumResistance: TunnelQuantumResistance
private(set) var multihopState: MultihopState
private(set) var daitaSettings: DAITASettings
static let defaultWireGuardPorts: [UInt16] = [51820, 53]
private var contentBlockers: [CustomDNSDataSource.Item: Bool]
var enabledBlockersCount: Int {
contentBlockers.count(where: { $0.key != .blockAll && $0.value })
}
var allBlockersEnabled: Bool {
contentBlockers.allSatisfy({$0.key != .blockAll && $0.value})
}
mutating func setBlockAll(_ newValue: Bool) {
blockAll = newValue
self.contentBlockers = CustomDNSDataSource.Item.contentBlockers
.reduce(into: [CustomDNSDataSource.Item: Bool]()) {
$0[$1] = newValue
}
enableCustomDNS = false
}
mutating func setBlockAdvertising(_ newValue: Bool) {
blockAdvertising = newValue
blockAll = allBlockersEnabled
enableCustomDNS = false
}
mutating func setBlockTracking(_ newValue: Bool) {
blockTracking = newValue
blockAll = allBlockersEnabled
enableCustomDNS = false
}
mutating func setBlockMalware(_ newValue: Bool) {
blockMalware = newValue
blockAll = allBlockersEnabled
enableCustomDNS = false
}
mutating func setBlockAdultContent(_ newValue: Bool) {
blockAdultContent = newValue
blockAll = allBlockersEnabled
enableCustomDNS = false
}
mutating func setBlockGambling(_ newValue: Bool) {
blockGambling = newValue
blockAll = allBlockersEnabled
enableCustomDNS = false
}
mutating func setBlockSocialMedia(_ newValue: Bool) {
blockSocialMedia = newValue
blockAll = allBlockersEnabled
enableCustomDNS = false
}
mutating func setEnableCustomDNS(_ newValue: Bool) {
enableCustomDNS = newValue
}
mutating func setWireGuardPort(_ newValue: UInt16?) {
wireGuardPort = newValue
}
mutating func setWireGuardObfuscationState(_ newState: WireGuardObfuscationState) {
obfuscationState = newState
}
mutating func setWireGuardObfuscationPort(_ newPort: WireGuardObfuscationPort) {
obfuscationPort = newPort
}
mutating func setQuantumResistance(_ newState: TunnelQuantumResistance) {
quantumResistance = newState
}
mutating func setMultihop(_ newState: MultihopState) {
multihopState = newState
}
mutating func setDAITASettings(_ newSettings: DAITASettings) {
daitaSettings = newSettings
}
/// Precondition for enabling Custom DNS.
var customDNSPrecondition: CustomDNSPrecondition {
if blockAdvertising || blockTracking || blockMalware ||
blockAdultContent || blockGambling || blockSocialMedia {
return .conflictsWithOtherSettings
} else {
let hasValidDNSDomains = customDNSDomains.contains { entry in
AnyIPAddress(entry.address) != nil
}
if hasValidDNSDomains {
return .satisfied
} else {
return .emptyDNSDomains
}
}
}
/// Effective state of the custom DNS setting.
var effectiveEnableCustomDNS: Bool {
customDNSPrecondition == .satisfied && enableCustomDNS
}
var customWireGuardPort: UInt16? {
wireGuardPort.flatMap { port in
Self.defaultWireGuardPorts.contains(port) ? nil : port
}
}
init(from tunnelSettings: LatestTunnelSettings = LatestTunnelSettings()) {
let dnsSettings = tunnelSettings.dnsSettings
self.contentBlockers = CustomDNSDataSource.Item.contentBlockers
.reduce(into: [CustomDNSDataSource.Item: Bool]()) {
$0[$1] = false
}
enableCustomDNS = dnsSettings.enableCustomDNS
customDNSDomains = dnsSettings.customDNSDomains.map { ipAddress in
DNSServerEntry(identifier: UUID(), address: "\(ipAddress)")
}
wireGuardPort = tunnelSettings.relayConstraints.port.value
obfuscationState = tunnelSettings.wireGuardObfuscation.state
obfuscationPort = tunnelSettings.wireGuardObfuscation.port
quantumResistance = tunnelSettings.tunnelQuantumResistance
multihopState = tunnelSettings.tunnelMultihopState
daitaSettings = tunnelSettings.daita
blockAdvertising = dnsSettings.blockingOptions.contains(.blockAdvertising)
blockTracking = dnsSettings.blockingOptions.contains(.blockTracking)
blockMalware = dnsSettings.blockingOptions.contains(.blockMalware)
blockAdultContent = dnsSettings.blockingOptions.contains(.blockAdultContent)
blockGambling = dnsSettings.blockingOptions.contains(.blockGambling)
blockSocialMedia = dnsSettings.blockingOptions.contains(.blockSocialMedia)
blockAll = contentBlockers.allSatisfy { $0.value }
}
/// Produce merged view model keeping entry `identifier` for matching DNS entries.
func merged(_ other: VPNSettingsViewModel) -> VPNSettingsViewModel {
var mergedViewModel = other
mergedViewModel.customDNSDomains = merge(customDNSDomains, with: other.customDNSDomains)
return mergedViewModel
}
/// Sanitize custom DNS entries.
mutating func sanitizeCustomDNSEntries() {
// Sanitize DNS domains, drop invalid entries.
customDNSDomains = customDNSDomains.compactMap { entry in
if let canonicalAddress = AnyIPAddress(entry.address) {
var newEntry = entry
newEntry.address = "\(canonicalAddress)"
return newEntry
} else {
return nil
}
}
// Toggle off custom DNS when no domains specified.
if customDNSDomains.isEmpty {
enableCustomDNS = false
}
}
func dnsEntry(entryIdentifier: UUID) -> DNSServerEntry? {
customDNSDomains.first { entry in
entry.identifier == entryIdentifier
}
}
/// Returns an index of entry in `customDNSDomains`, otherwise `nil`.
func indexOfDNSEntry(entryIdentifier: UUID) -> Int? {
customDNSDomains.firstIndex { entry in
entry.identifier == entryIdentifier
}
}
/// Update the address for the DNS entry with the given UUID.
mutating func updateDNSEntry(entryIdentifier: UUID, newAddress: String) {
guard let index = indexOfDNSEntry(entryIdentifier: entryIdentifier) else { return }
var entry = customDNSDomains[index]
entry.address = newAddress
customDNSDomains[index] = entry
}
/// Converts view model into `DNSSettings`.
func asDNSSettings() -> DNSSettings {
var blockingOptions = DNSBlockingOptions()
if blockAdvertising {
blockingOptions.insert(.blockAdvertising)
}
if blockTracking {
blockingOptions.insert(.blockTracking)
}
if blockMalware {
blockingOptions.insert(.blockMalware)
}
if blockAdultContent {
blockingOptions.insert(.blockAdultContent)
}
if blockGambling {
blockingOptions.insert(.blockGambling)
}
if blockSocialMedia {
blockingOptions.insert(.blockSocialMedia)
}
var dnsSettings = DNSSettings()
dnsSettings.blockingOptions = blockingOptions
dnsSettings.enableCustomDNS = enableCustomDNS
dnsSettings.customDNSDomains = customDNSDomains.compactMap { entry in
AnyIPAddress(entry.address)
}
return dnsSettings
}
/// Returns true if the given string is empty or a valid IP address.
func isDNSDomainUserInputValid(_ string: String) -> Bool {
string.isEmpty || AnyIPAddress(string) != nil
}
/// Returns true if the given port is in within the supported ranges.
func isPortWithinValidWireGuardRanges(_ port: UInt16) -> Bool {
availableWireGuardPortRanges.contains { range in
if let minPort = range.first, let maxPort = range.last {
return (minPort ... maxPort).contains(port)
}
return false
}
}
/// Replaces all old domains with new, keeping only those that share the same id and updating their content.
private func merge(_ oldDomains: [DNSServerEntry], with newDomains: [DNSServerEntry]) -> [DNSServerEntry] {
var oldDomains = oldDomains
return newDomains.map { otherEntry in
let sameEntryIndex = oldDomains.firstIndex { entry in
entry.address == otherEntry.address
}
if let sameEntryIndex {
let sourceEntry = oldDomains[sameEntryIndex]
oldDomains.remove(at: sameEntryIndex)
return sourceEntry
} else {
return otherEntry
}
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mojganii)
ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsViewModel.swift
line 107 at r1 (raw file):
Previously, mojganii wrote…
we can map the content blockers which we have in
DataSource
into the model through a dictionary: and having access to the values throughsetter
andgetter
. in this way if we add another options into content blockers the only thing we need to add into model is a variable which is reading and writing the dictionary. what do you think?
We don't need a dictionary approach for this. The DNS proxy that would be used is a bit set in the first place (see DNSBlockingOptions
) so this would be overkill IMHO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)
ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsViewModel.swift
line 107 at r1 (raw file):
Previously, buggmagnet wrote…
We don't need a dictionary approach for this. The DNS proxy that would be used is a bit set in the first place (see
DNSBlockingOptions
) so this would be overkill IMHO
I think following the code for how and what conditions are meeting to take proper action is a bit hard.we are hiding the complexity behind the another object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mojganii)
ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsViewModel.swift
line 107 at r1 (raw file):
Previously, mojganii wrote…
I think following the code for how and what conditions are meeting to take proper action is a bit hard.we are hiding the complexity behind the another object.
I like the idea of now having to make updates to multiple places in case of future changes, but as @buggmagnet points out it's not really necessary. Can we come to concensus on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet and @rablador)
ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsViewModel.swift
line 107 at r1 (raw file):
Previously, rablador (Jon Petersson) wrote…
I like the idea of now having to make updates to multiple places in case of future changes, but as @buggmagnet points out it's not really necessary. Can we come to concensus on this?
If you both agreed, it's over-engineering then let's go with the current one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet and @rablador)
f555080
to
e77fba7
Compare
🚨 End to end tests failed. Please check the failed workflow run. |
Currently there is no way to know that you have content blockers enabled unless you expand the tree.
This first iteration would have an "All" switch that activates all toggles - and the number of activated toggles would be shown at the top level.
Toggling "All" would activate all the other toggles. But if the user then deactivates a toggle, "All" would have to be deactivated as well since all of them are not activated. Only by actively deactivating "All" does the user deactivate all the other toggles.
This change is