From 87ec698b966a6e6100435d0799b1f6d552232f82 Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Mon, 10 Feb 2025 16:50:36 +1100 Subject: [PATCH 1/6] fix: unquarantine dylib after download --- .../Coder Desktop/Coder_Desktop.entitlements | 6 --- .../Coder Desktop/NetworkExtension.swift | 9 ++-- Coder Desktop/Coder Desktop/VPNService.swift | 9 ++-- .../Coder Desktop/Views/VPNMenu.swift | 28 ++++++++---- .../Coder Desktop/Views/VPNState.swift | 21 ++++++--- .../Coder Desktop/XPCInterface.swift | 35 +++++++++++++++ .../Coder DesktopTests/VPNStateTests.swift | 9 ++-- Coder Desktop/VPN/Manager.swift | 42 +++++++++++++++++- Coder Desktop/VPN/PacketTunnelProvider.swift | 44 ++++++++++++++----- Coder Desktop/VPNLib/Util.swift | 6 +-- Coder Desktop/VPNLib/XPC.swift | 1 + Coder Desktop/project.yml | 3 -- 12 files changed, 161 insertions(+), 52 deletions(-) diff --git a/Coder Desktop/Coder Desktop/Coder_Desktop.entitlements b/Coder Desktop/Coder Desktop/Coder_Desktop.entitlements index 7d90a161..0d80c22d 100644 --- a/Coder Desktop/Coder Desktop/Coder_Desktop.entitlements +++ b/Coder Desktop/Coder Desktop/Coder_Desktop.entitlements @@ -8,15 +8,9 @@ com.apple.developer.system-extension.install - com.apple.security.app-sandbox - com.apple.security.application-groups $(TeamIdentifierPrefix)com.coder.Coder-Desktop - com.apple.security.files.user-selected.read-only - - com.apple.security.network.client - diff --git a/Coder Desktop/Coder Desktop/NetworkExtension.swift b/Coder Desktop/Coder Desktop/NetworkExtension.swift index 16d18bb4..effd1946 100644 --- a/Coder Desktop/Coder Desktop/NetworkExtension.swift +++ b/Coder Desktop/Coder Desktop/NetworkExtension.swift @@ -24,12 +24,13 @@ enum NetworkExtensionState: Equatable { /// An actor that handles configuring, enabling, and disabling the VPN tunnel via the /// NetworkExtension APIs. extension CoderVPNService { - func hasNetworkExtensionConfig() async -> Bool { + func loadNetworkExtensionConfig() async { do { - _ = try await getTunnelManager() - return true + let tm = try await getTunnelManager() + neState = .disabled + serverAddress = tm.protocolConfiguration?.serverAddress } catch { - return false + neState = .unconfigured } } diff --git a/Coder Desktop/Coder Desktop/VPNService.swift b/Coder Desktop/Coder Desktop/VPNService.swift index 657d9949..9d8abb84 100644 --- a/Coder Desktop/Coder Desktop/VPNService.swift +++ b/Coder Desktop/Coder Desktop/VPNService.swift @@ -63,15 +63,13 @@ final class CoderVPNService: NSObject, VPNService { // only stores a weak reference to the delegate. var systemExtnDelegate: SystemExtensionDelegate? + var serverAddress: String? + override init() { super.init() installSystemExtension() Task { - neState = if await hasNetworkExtensionConfig() { - .disabled - } else { - .unconfigured - } + await loadNetworkExtensionConfig() } xpc.connect() xpc.getPeerState() @@ -115,6 +113,7 @@ final class CoderVPNService: NSObject, VPNService { func configureTunnelProviderProtocol(proto: NETunnelProviderProtocol?) { Task { if let proto { + serverAddress = proto.serverAddress await configureNetworkExtension(proto: proto) // this just configures the VPN, it doesn't enable it tunnelState = .disabled diff --git a/Coder Desktop/Coder Desktop/Views/VPNMenu.swift b/Coder Desktop/Coder Desktop/Views/VPNMenu.swift index 26266c8d..759bce80 100644 --- a/Coder Desktop/Coder Desktop/Views/VPNMenu.swift +++ b/Coder Desktop/Coder Desktop/Views/VPNMenu.swift @@ -31,13 +31,7 @@ struct VPNMenu: View { Text("Workspace Agents") .font(.headline) .foregroundColor(.gray) - if session.hasSession { - VPNState() - } else { - Text("Sign in to use CoderVPN") - .font(.body) - .foregroundColor(.gray) - } + VPNState() }.padding([.horizontal, .top], Theme.Size.trayInset) Agents() // Trailing stack @@ -52,7 +46,15 @@ struct VPNMenu: View { }.buttonStyle(.plain) TrayDivider() } - AuthButton() + if vpn.state == .failed(.systemExtensionError(.needsUserApproval)) { + Button { + openSystemExtensionSettings() + } label: { + ButtonRowView { Text("Open System Preferences") } + }.buttonStyle(.plain) + } else { + AuthButton() + } Button { openSettings() appActivate() @@ -84,10 +86,18 @@ struct VPNMenu: View { private var vpnDisabled: Bool { !session.hasSession || vpn.state == .connecting || - vpn.state == .disconnecting + vpn.state == .disconnecting || + vpn.state == .failed(.systemExtensionError(.needsUserApproval)) } } +func openSystemExtensionSettings() { + // TODO: Check this still works in a new macOS version + // https://gist.github.com/rmcdongit/f66ff91e0dad78d4d6346a75ded4b751?permalink_comment_id=5261757 + // swiftlint:disable:next line_length + NSWorkspace.shared.open(URL(string: "x-apple.systempreferences:com.apple.ExtensionsPreferences?extensionPointIdentifier=com.apple.system_extension.network_extension.extension-point")!) +} + #Preview { VPNMenu().frame(width: 256) .environmentObject(PreviewVPN()) diff --git a/Coder Desktop/Coder Desktop/Views/VPNState.swift b/Coder Desktop/Coder Desktop/Views/VPNState.swift index 17102039..706b8cfb 100644 --- a/Coder Desktop/Coder Desktop/Views/VPNState.swift +++ b/Coder Desktop/Coder Desktop/Views/VPNState.swift @@ -1,18 +1,27 @@ import SwiftUI -struct VPNState: View { +struct VPNState: View { @EnvironmentObject var vpn: VPN + @EnvironmentObject var session: S let inspection = Inspection() var body: some View { Group { - switch vpn.state { - case .disabled: - Text("Enable CoderVPN to see agents") + switch (vpn.state, session.hasSession) { + case (.failed(.systemExtensionError(.needsUserApproval)), _): + Text("Awaiting System Extension Approval") + .font(.body) + .foregroundStyle(.gray) + case (_, false): + Text("Sign in to use CoderVPN") .font(.body) .foregroundColor(.gray) - case .connecting, .disconnecting: + case (.disabled, _): + Text("Enable CoderVPN to see agents") + .font(.body) + .foregroundStyle(.gray) + case (.connecting, _), (.disconnecting, _): HStack { Spacer() ProgressView( @@ -20,7 +29,7 @@ struct VPNState: View { ).padding() Spacer() } - case let .failed(vpnErr): + case let (.failed(vpnErr), _): Text("\(vpnErr.description)") .font(.headline) .foregroundColor(.red) diff --git a/Coder Desktop/Coder Desktop/XPCInterface.swift b/Coder Desktop/Coder Desktop/XPCInterface.swift index 74baab5a..73586cae 100644 --- a/Coder Desktop/Coder Desktop/XPCInterface.swift +++ b/Coder Desktop/Coder Desktop/XPCInterface.swift @@ -64,4 +64,39 @@ import VPNLib svc.onExtensionPeerUpdate(data) } } + + // The NE has verified the dylib and knows better than Gatekeeper + func removeQuarantine(path: String, reply: @escaping (Bool) -> Void) { + let reply = CallbackWrapper(reply) + Task { @MainActor in + let prompt = """ + Coder Desktop wants to execute code downloaded from \ + \(svc.serverAddress ?? "the Coder deployment"). The code has been \ + verified to be signed by Coder. + """ + let source = """ + do shell script "xattr -d com.apple.quarantine \(path)" \ + with prompt "\(prompt)" \ + with administrator privileges + """ + let success = await withCheckedContinuation { continuation in + guard let script = NSAppleScript(source: source) else { + continuation.resume(returning: false) + return + } + // Run on a background thread + Task.detached { + var error: NSDictionary? + script.executeAndReturnError(&error) + if let error { + self.logger.error("AppleScript error: \(error)") + continuation.resume(returning: false) + } else { + continuation.resume(returning: true) + } + } + } + reply(success) + } + } } diff --git a/Coder Desktop/Coder DesktopTests/VPNStateTests.swift b/Coder Desktop/Coder DesktopTests/VPNStateTests.swift index 4d630cd0..298bacd5 100644 --- a/Coder Desktop/Coder DesktopTests/VPNStateTests.swift +++ b/Coder Desktop/Coder DesktopTests/VPNStateTests.swift @@ -7,13 +7,16 @@ import ViewInspector @Suite(.timeLimit(.minutes(1))) struct VPNStateTests { let vpn: MockVPNService - let sut: VPNState + let session: MockSession + let sut: VPNState let view: any View init() { vpn = MockVPNService() - sut = VPNState() - view = sut.environmentObject(vpn) + sut = VPNState() + session = MockSession() + session.hasSession = true + view = sut.environmentObject(vpn).environmentObject(session) } @Test diff --git a/Coder Desktop/VPN/Manager.swift b/Coder Desktop/VPN/Manager.swift index c9388183..13afbd01 100644 --- a/Coder Desktop/VPN/Manager.swift +++ b/Coder Desktop/VPN/Manager.swift @@ -46,6 +46,11 @@ actor Manager { } catch { throw .validation(error) } + + // HACK: The downloaded dylib may be quarantined, but we've validated it's signature + // so it's safe to execute. However, this SE must be sandboxed, so we defer to the app. + try await removeQuarantine(dest) + do { try tunnelHandle = TunnelHandle(dylibPath: dest) } catch { @@ -85,7 +90,13 @@ actor Manager { } catch { logger.error("tunnel read loop failed: \(error.localizedDescription, privacy: .public)") try await tunnelHandle.close() - ptp.cancelTunnelWithError(error) + ptp.cancelTunnelWithError( + NSError( + domain: "\(Bundle.main.bundleIdentifier!).Manager", + code: -1, + userInfo: [NSLocalizedDescriptionKey: "Tunnel read loop failed: \(error.localizedDescription)"] + ) + ) return } logger.info("tunnel read loop exited") @@ -227,6 +238,9 @@ enum ManagerError: Error { case serverInfo(String) case errorResponse(msg: String) case noTunnelFileDescriptor + case noApp + case permissionDenied + case tunnelFail(any Error) var description: String { switch self { @@ -248,6 +262,12 @@ enum ManagerError: Error { msg case .noTunnelFileDescriptor: "Could not find a tunnel file descriptor" + case .noApp: + "The VPN must be started with the app open during first-time setup." + case .permissionDenied: + "Permission was not granted to execute the CoderVPN dylib" + case let .tunnelFail(err): + "Failed to communicate with dylib over tunnel: \(err)" } } } @@ -272,3 +292,23 @@ func writeVpnLog(_ log: Vpn_Log) { let fields = log.fields.map { "\($0.name): \($0.value)" }.joined(separator: ", ") logger.log(level: level, "\(log.message, privacy: .public): \(fields, privacy: .public)") } + +private func removeQuarantine(_ dest: URL) async throws(ManagerError) { + var flag: AnyObject? + let file = NSURL(fileURLWithPath: dest.path) + try? file.getResourceValue(&flag, forKey: kCFURLQuarantinePropertiesKey as URLResourceKey) + if flag != nil { + guard let conn = globalXPCListenerDelegate.conn else { + throw .noApp + } + // Wait for unsandboxed app to accept our file + let success = await withCheckedContinuation { [dest] continuation in + conn.removeQuarantine(path: dest.path) { success in + continuation.resume(returning: success) + } + } + if !success { + throw .permissionDenied + } + } +} diff --git a/Coder Desktop/VPN/PacketTunnelProvider.swift b/Coder Desktop/VPN/PacketTunnelProvider.swift index 3cad498b..71304dd8 100644 --- a/Coder Desktop/VPN/PacketTunnelProvider.swift +++ b/Coder Desktop/VPN/PacketTunnelProvider.swift @@ -43,26 +43,45 @@ class PacketTunnelProvider: NEPacketTunnelProvider, @unchecked Sendable { return nil } + // swiftlint:disable:next function_body_length override func startTunnel( options _: [String: NSObject]?, completionHandler: @escaping (Error?) -> Void ) { logger.info("startTunnel called") guard manager == nil else { logger.error("startTunnel called with non-nil Manager") - completionHandler(PTPError.alreadyRunning) + completionHandler( + NSError( + domain: "\(Bundle.main.bundleIdentifier!).PTP", + code: -1, + userInfo: [NSLocalizedDescriptionKey: "Already running"] + ) + ) return } guard let proto = protocolConfiguration as? NETunnelProviderProtocol, let baseAccessURL = proto.serverAddress else { logger.error("startTunnel called with nil protocolConfiguration") - completionHandler(PTPError.missingConfiguration) + completionHandler( + NSError( + domain: "\(Bundle.main.bundleIdentifier!).PTP", + code: -1, + userInfo: [NSLocalizedDescriptionKey: "Missing Configuration"] + ) + ) return } // HACK: We can't write to the system keychain, and the NE can't read the user keychain. guard let token = proto.providerConfiguration?["token"] as? String else { logger.error("startTunnel called with nil token") - completionHandler(PTPError.missingToken) + completionHandler( + NSError( + domain: "\(Bundle.main.bundleIdentifier!).PTP", + code: -1, + userInfo: [NSLocalizedDescriptionKey: "Missing Token"] + ) + ) return } logger.debug("retrieved token & access URL") @@ -70,7 +89,7 @@ class PacketTunnelProvider: NEPacketTunnelProvider, @unchecked Sendable { Task { do throws(ManagerError) { logger.debug("creating manager") - manager = try await Manager( + let manager = try await Manager( with: self, cfg: .init( apiToken: token, serverUrl: .init(string: baseAccessURL)! @@ -78,12 +97,19 @@ class PacketTunnelProvider: NEPacketTunnelProvider, @unchecked Sendable { ) globalXPCListenerDelegate.vpnXPCInterface.manager = manager logger.debug("starting vpn") - try await manager!.startVPN() + try await manager.startVPN() logger.info("vpn started") + self.manager = manager completionHandler(nil) } catch { logger.error("error starting manager: \(error.description, privacy: .public)") - completionHandler(error as NSError) + completionHandler( + NSError( + domain: "\(Bundle.main.bundleIdentifier!).Manager", + code: -1, + userInfo: [NSLocalizedDescriptionKey: error.description] + ) + ) } } } @@ -152,9 +178,3 @@ class PacketTunnelProvider: NEPacketTunnelProvider, @unchecked Sendable { try await setTunnelNetworkSettings(currentSettings) } } - -enum PTPError: Error { - case alreadyRunning - case missingConfiguration - case missingToken -} diff --git a/Coder Desktop/VPNLib/Util.swift b/Coder Desktop/VPNLib/Util.swift index ff31e4fd..e4716331 100644 --- a/Coder Desktop/VPNLib/Util.swift +++ b/Coder Desktop/VPNLib/Util.swift @@ -1,11 +1,11 @@ public struct CallbackWrapper: @unchecked Sendable { - private let block: (T?) -> U + private let block: (T) -> U - public init(_ block: @escaping (T?) -> U) { + public init(_ block: @escaping (T) -> U) { self.block = block } - public func callAsFunction(_ error: T?) -> U { + public func callAsFunction(_ error: T) -> U { block(error) } } diff --git a/Coder Desktop/VPNLib/XPC.swift b/Coder Desktop/VPNLib/XPC.swift index eda8ab01..dc79651e 100644 --- a/Coder Desktop/VPNLib/XPC.swift +++ b/Coder Desktop/VPNLib/XPC.swift @@ -10,4 +10,5 @@ import Foundation @objc public protocol VPNXPCClientCallbackProtocol { // data is a serialized `Vpn_PeerUpdate` func onPeerUpdate(_ data: Data) + func removeQuarantine(path: String, reply: @escaping (Bool) -> Void) } diff --git a/Coder Desktop/project.yml b/Coder Desktop/project.yml index 255bc538..54ce06af 100644 --- a/Coder Desktop/project.yml +++ b/Coder Desktop/project.yml @@ -116,9 +116,6 @@ targets: com.apple.developer.networking.networkextension: - packet-tunnel-provider com.apple.developer.system-extension.install: true - com.apple.security.app-sandbox: true - com.apple.security.files.user-selected.read-only: true - com.apple.security.network.client: true com.apple.security.application-groups: - $(TeamIdentifierPrefix)com.coder.Coder-Desktop settings: From 024d7e3d7989dba236c0cf871b3ce7133d6e7a44 Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Mon, 10 Feb 2025 19:18:40 +1100 Subject: [PATCH 2/6] capitalization --- Coder Desktop/Coder Desktop/Views/AuthButton.swift | 2 +- Coder Desktop/Coder Desktop/Views/VPNMenu.swift | 2 +- Coder Desktop/Coder Desktop/Views/VPNState.swift | 2 +- Coder Desktop/Coder DesktopTests/VPNMenuTests.swift | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Coder Desktop/Coder Desktop/Views/AuthButton.swift b/Coder Desktop/Coder Desktop/Views/AuthButton.swift index cfab0880..de102083 100644 --- a/Coder Desktop/Coder Desktop/Views/AuthButton.swift +++ b/Coder Desktop/Coder Desktop/Views/AuthButton.swift @@ -17,7 +17,7 @@ struct AuthButton: View { } } label: { ButtonRowView { - Text(session.hasSession ? "Sign Out" : "Sign In") + Text(session.hasSession ? "Sign out" : "Sign in") } }.buttonStyle(.plain) } diff --git a/Coder Desktop/Coder Desktop/Views/VPNMenu.swift b/Coder Desktop/Coder Desktop/Views/VPNMenu.swift index 759bce80..9137ac54 100644 --- a/Coder Desktop/Coder Desktop/Views/VPNMenu.swift +++ b/Coder Desktop/Coder Desktop/Views/VPNMenu.swift @@ -50,7 +50,7 @@ struct VPNMenu: View { Button { openSystemExtensionSettings() } label: { - ButtonRowView { Text("Open System Preferences") } + ButtonRowView { Text("Approve in System Settings") } }.buttonStyle(.plain) } else { AuthButton() diff --git a/Coder Desktop/Coder Desktop/Views/VPNState.swift b/Coder Desktop/Coder Desktop/Views/VPNState.swift index 706b8cfb..4afc6c26 100644 --- a/Coder Desktop/Coder Desktop/Views/VPNState.swift +++ b/Coder Desktop/Coder Desktop/Views/VPNState.swift @@ -10,7 +10,7 @@ struct VPNState: View { Group { switch (vpn.state, session.hasSession) { case (.failed(.systemExtensionError(.needsUserApproval)), _): - Text("Awaiting System Extension Approval") + Text("Awaiting System Extension approval") .font(.body) .foregroundStyle(.gray) case (_, false): diff --git a/Coder Desktop/Coder DesktopTests/VPNMenuTests.swift b/Coder Desktop/Coder DesktopTests/VPNMenuTests.swift index 4b446ac0..b0484a9f 100644 --- a/Coder Desktop/Coder DesktopTests/VPNMenuTests.swift +++ b/Coder Desktop/Coder DesktopTests/VPNMenuTests.swift @@ -27,7 +27,7 @@ struct VPNMenuTests { let toggle = try view.find(ViewType.Toggle.self) #expect(toggle.isDisabled()) #expect(throws: Never.self) { try view.find(text: "Sign in to use CoderVPN") } - #expect(throws: Never.self) { try view.find(button: "Sign In") } + #expect(throws: Never.self) { try view.find(button: "Sign in") } } } } From e64ea22eeb66dacc3323fc757839f391ddbc8dbf Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Tue, 11 Feb 2025 22:10:58 +1100 Subject: [PATCH 3/6] review --- .../Coder Desktop/Views/VPNMenu.swift | 3 +- Coder Desktop/VPN/Manager.swift | 6 +--- Coder Desktop/VPN/PacketTunnelProvider.swift | 31 +++---------------- Coder Desktop/VPNLib/Util.swift | 8 +++++ 4 files changed, 15 insertions(+), 33 deletions(-) diff --git a/Coder Desktop/Coder Desktop/Views/VPNMenu.swift b/Coder Desktop/Coder Desktop/Views/VPNMenu.swift index 9137ac54..3f253e19 100644 --- a/Coder Desktop/Coder Desktop/Views/VPNMenu.swift +++ b/Coder Desktop/Coder Desktop/Views/VPNMenu.swift @@ -92,8 +92,9 @@ struct VPNMenu: View { } func openSystemExtensionSettings() { - // TODO: Check this still works in a new macOS version + // Sourced from: // https://gist.github.com/rmcdongit/f66ff91e0dad78d4d6346a75ded4b751?permalink_comment_id=5261757 + // We'll need to ensure this continues to work in future macOS versions // swiftlint:disable:next line_length NSWorkspace.shared.open(URL(string: "x-apple.systempreferences:com.apple.ExtensionsPreferences?extensionPointIdentifier=com.apple.system_extension.network_extension.extension-point")!) } diff --git a/Coder Desktop/VPN/Manager.swift b/Coder Desktop/VPN/Manager.swift index 13afbd01..58a65b53 100644 --- a/Coder Desktop/VPN/Manager.swift +++ b/Coder Desktop/VPN/Manager.swift @@ -91,11 +91,7 @@ actor Manager { logger.error("tunnel read loop failed: \(error.localizedDescription, privacy: .public)") try await tunnelHandle.close() ptp.cancelTunnelWithError( - NSError( - domain: "\(Bundle.main.bundleIdentifier!).Manager", - code: -1, - userInfo: [NSLocalizedDescriptionKey: "Tunnel read loop failed: \(error.localizedDescription)"] - ) + makeNSError(suffix: "Manager", desc: "Tunnel read loop failed: \(error.localizedDescription)") ) return } diff --git a/Coder Desktop/VPN/PacketTunnelProvider.swift b/Coder Desktop/VPN/PacketTunnelProvider.swift index 71304dd8..01022950 100644 --- a/Coder Desktop/VPN/PacketTunnelProvider.swift +++ b/Coder Desktop/VPN/PacketTunnelProvider.swift @@ -43,45 +43,26 @@ class PacketTunnelProvider: NEPacketTunnelProvider, @unchecked Sendable { return nil } - // swiftlint:disable:next function_body_length override func startTunnel( options _: [String: NSObject]?, completionHandler: @escaping (Error?) -> Void ) { logger.info("startTunnel called") guard manager == nil else { logger.error("startTunnel called with non-nil Manager") - completionHandler( - NSError( - domain: "\(Bundle.main.bundleIdentifier!).PTP", - code: -1, - userInfo: [NSLocalizedDescriptionKey: "Already running"] - ) - ) + completionHandler(makeNSError(suffix: "PTP", desc: "Already running")) return } guard let proto = protocolConfiguration as? NETunnelProviderProtocol, let baseAccessURL = proto.serverAddress else { logger.error("startTunnel called with nil protocolConfiguration") - completionHandler( - NSError( - domain: "\(Bundle.main.bundleIdentifier!).PTP", - code: -1, - userInfo: [NSLocalizedDescriptionKey: "Missing Configuration"] - ) - ) + completionHandler(makeNSError(suffix: "PTP", desc: "Missing Configuration")) return } // HACK: We can't write to the system keychain, and the NE can't read the user keychain. guard let token = proto.providerConfiguration?["token"] as? String else { logger.error("startTunnel called with nil token") - completionHandler( - NSError( - domain: "\(Bundle.main.bundleIdentifier!).PTP", - code: -1, - userInfo: [NSLocalizedDescriptionKey: "Missing Token"] - ) - ) + completionHandler(makeNSError(suffix: "PTP", desc: "Missing Token")) return } logger.debug("retrieved token & access URL") @@ -104,11 +85,7 @@ class PacketTunnelProvider: NEPacketTunnelProvider, @unchecked Sendable { } catch { logger.error("error starting manager: \(error.description, privacy: .public)") completionHandler( - NSError( - domain: "\(Bundle.main.bundleIdentifier!).Manager", - code: -1, - userInfo: [NSLocalizedDescriptionKey: error.description] - ) + makeNSError(suffix: "Manager", desc: error.description) ) } } diff --git a/Coder Desktop/VPNLib/Util.swift b/Coder Desktop/VPNLib/Util.swift index e4716331..fd9bbc3f 100644 --- a/Coder Desktop/VPNLib/Util.swift +++ b/Coder Desktop/VPNLib/Util.swift @@ -21,3 +21,11 @@ public struct CompletionWrapper: @unchecked Sendable { block() } } + +public func makeNSError(suffix: String, code: Int = -1, desc: String) -> NSError { + NSError( + domain: "\(Bundle.main.bundleIdentifier!).\(suffix)", + code: code, + userInfo: [NSLocalizedDescriptionKey: desc] + ) +} From ce1883e1c30dc5493a984d4f58d60806b76ea7a7 Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Mon, 10 Feb 2025 19:06:11 +1100 Subject: [PATCH 4/6] fix: display offline workspaces --- .../Preview Content/PreviewVPN.swift | 4 +- .../Coder Desktop/VPNMenuState.swift | 116 +++++++++++++ Coder Desktop/Coder Desktop/VPNService.swift | 64 +------ Coder Desktop/Coder Desktop/Views/Agent.swift | 99 ----------- .../Coder Desktop/Views/Agents.swift | 12 +- .../Coder Desktop/Views/VPNMenu.swift | 2 +- .../Coder Desktop/Views/VPNMenuItem.swift | 105 ++++++++++++ .../Coder DesktopTests/AgentsTests.swift | 12 +- Coder Desktop/Coder DesktopTests/Util.swift | 2 +- .../VPNMenuStateTests.swift | 159 ++++++++++++++++++ .../Coder DesktopTests/VPNServiceTests.swift | 116 ------------- 11 files changed, 404 insertions(+), 287 deletions(-) create mode 100644 Coder Desktop/Coder Desktop/VPNMenuState.swift delete mode 100644 Coder Desktop/Coder Desktop/Views/Agent.swift create mode 100644 Coder Desktop/Coder Desktop/Views/VPNMenuItem.swift create mode 100644 Coder Desktop/Coder DesktopTests/VPNMenuStateTests.swift delete mode 100644 Coder Desktop/Coder DesktopTests/VPNServiceTests.swift diff --git a/Coder Desktop/Coder Desktop/Preview Content/PreviewVPN.swift b/Coder Desktop/Coder Desktop/Preview Content/PreviewVPN.swift index 5e66eb72..7e85a86c 100644 --- a/Coder Desktop/Coder Desktop/Preview Content/PreviewVPN.swift +++ b/Coder Desktop/Coder Desktop/Preview Content/PreviewVPN.swift @@ -4,7 +4,7 @@ import SwiftUI @MainActor final class PreviewVPN: Coder_Desktop.VPNService { @Published var state: Coder_Desktop.VPNServiceState = .disabled - @Published var agents: [UUID: Coder_Desktop.Agent] = [ + @Published var menuState: VPNMenuState = .init(agents: [ UUID(): Agent(id: UUID(), name: "dev", status: .error, copyableDNS: "asdf.coder", wsName: "dogfood2", wsID: UUID()), UUID(): Agent(id: UUID(), name: "dev", status: .okay, copyableDNS: "asdf.coder", @@ -25,7 +25,7 @@ final class PreviewVPN: Coder_Desktop.VPNService { wsID: UUID()), UUID(): Agent(id: UUID(), name: "dev", status: .off, copyableDNS: "asdf.coder", wsName: "example", wsID: UUID()), - ] + ], workspaces: [:]) let shouldFail: Bool let longError = "This is a long error to test the UI with long error messages" diff --git a/Coder Desktop/Coder Desktop/VPNMenuState.swift b/Coder Desktop/Coder Desktop/VPNMenuState.swift new file mode 100644 index 00000000..866afc23 --- /dev/null +++ b/Coder Desktop/Coder Desktop/VPNMenuState.swift @@ -0,0 +1,116 @@ +import Foundation +import SwiftUI +import VPNLib + +struct Agent: Identifiable, Equatable, Comparable { + let id: UUID + let name: String + let status: AgentStatus + let copyableDNS: String + let wsName: String + let wsID: UUID + + // Agents are sorted by status, and then by name + static func < (lhs: Agent, rhs: Agent) -> Bool { + if lhs.status != rhs.status { + return lhs.status < rhs.status + } + return lhs.wsName.localizedCompare(rhs.wsName) == .orderedAscending + } +} + +enum AgentStatus: Int, Equatable, Comparable { + case okay = 0 + case warn = 1 + case error = 2 + case off = 3 + + public var color: Color { + switch self { + case .okay: .green + case .warn: .yellow + case .error: .red + case .off: .gray + } + } + + static func < (lhs: AgentStatus, rhs: AgentStatus) -> Bool { + lhs.rawValue < rhs.rawValue + } +} + +struct Workspace: Identifiable, Equatable, Comparable { + let id: UUID + let name: String + var agents: [UUID] + + static func < (lhs: Workspace, rhs: Workspace) -> Bool { + lhs.name.localizedCompare(rhs.name) == .orderedAscending + } +} + +struct VPNMenuState { + var agents: [UUID: Agent] = [:] + var workspaces: [UUID: Workspace] = [:] + + mutating func upsertAgent(_ agent: Vpn_Agent) { + guard let id = UUID(uuidData: agent.id) else { return } + guard let wsID = UUID(uuidData: agent.workspaceID) else { return } + // An existing agent with the same name, belonging to the same workspace + // is from a previous workspace build, and should be removed. + agents.filter { $0.value.name == agent.name && $0.value.wsID == wsID } + .forEach { agents[$0.key] = nil } + workspaces[wsID]?.agents.append(id) + let wsName = workspaces[wsID]?.name ?? "Unknown Workspace" + agents[id] = Agent( + id: id, + name: agent.name, + // If last handshake was not within last five minutes, the agent is unhealthy + status: agent.lastHandshake.date > Date.now.addingTimeInterval(-300) ? .okay : .warn, + // Choose the shortest hostname, and remove trailing dot if present + copyableDNS: agent.fqdn.min(by: { $0.count < $1.count }) + .map { $0.hasSuffix(".") ? String($0.dropLast()) : $0 } ?? "UNKNOWN", + wsName: wsName, + wsID: wsID + ) + } + + mutating func deleteAgent(withId id: Data) { + guard let id = UUID(uuidData: id) else { return } + // Update Workspaces + if let agent = agents[id], var ws = workspaces[agent.wsID] { + ws.agents.removeAll { $0 == id } + workspaces[agent.wsID] = ws + } + agents[id] = nil + } + + mutating func upsertWorkspace(_ workspace: Vpn_Workspace) { + guard let id = UUID(uuidData: workspace.id) else { return } + workspaces[id] = Workspace(id: id, name: workspace.name, agents: []) + } + + mutating func deleteWorkspace(withId id: Data) { + guard let wsID = UUID(uuidData: id) else { return } + agents.filter { _, value in + value.wsID == wsID + }.forEach { key, _ in + agents[key] = nil + } + workspaces[wsID] = nil + } + + func sorted() -> [VPNMenuItem] { + var items = agents.values.map { VPNMenuItem.agent($0) } + // Workspaces with no agents are shown as offline + items += workspaces.filter { _, value in + value.agents.isEmpty + }.map { VPNMenuItem.offlineWorkspace(Workspace(id: $0.key, name: $0.value.name, agents: $0.value.agents)) } + return items.sorted() + } + + mutating func clear() { + agents.removeAll() + workspaces.removeAll() + } +} diff --git a/Coder Desktop/Coder Desktop/VPNService.swift b/Coder Desktop/Coder Desktop/VPNService.swift index 9d8abb84..95aff958 100644 --- a/Coder Desktop/Coder Desktop/VPNService.swift +++ b/Coder Desktop/Coder Desktop/VPNService.swift @@ -6,7 +6,7 @@ import VPNLib @MainActor protocol VPNService: ObservableObject { var state: VPNServiceState { get } - var agents: [UUID: Agent] { get } + var menuState: VPNMenuState { get } func start() async func stop() async func configureTunnelProviderProtocol(proto: NETunnelProviderProtocol?) @@ -41,7 +41,6 @@ enum VPNServiceError: Error, Equatable { final class CoderVPNService: NSObject, VPNService { var logger = Logger(subsystem: Bundle.main.bundleIdentifier!, category: "vpn") lazy var xpc: VPNXPCInterface = .init(vpn: self) - var workspaces: [UUID: String] = [:] @Published var tunnelState: VPNServiceState = .disabled @Published var sysExtnState: SystemExtensionState = .uninstalled @@ -56,7 +55,7 @@ final class CoderVPNService: NSObject, VPNService { return tunnelState } - @Published var agents: [UUID: Agent] = [:] + @Published var menuState: VPNMenuState = .init() // systemExtnDelegate holds a reference to the SystemExtensionDelegate so that it doesn't get // garbage collected while the OSSystemExtensionRequest is in flight, since the OS framework @@ -85,11 +84,6 @@ final class CoderVPNService: NSObject, VPNService { NotificationCenter.default.removeObserver(self) } - func clearPeers() { - agents = [:] - workspaces = [:] - } - func start() async { switch tunnelState { case .disabled, .failed: @@ -150,7 +144,7 @@ final class CoderVPNService: NSObject, VPNService { do { let msg = try Vpn_PeerUpdate(serializedBytes: data) debugPrint(msg) - clearPeers() + menuState.clear() applyPeerUpdate(with: msg) } catch { logger.error("failed to decode peer update \(error)") @@ -159,53 +153,11 @@ final class CoderVPNService: NSObject, VPNService { func applyPeerUpdate(with update: Vpn_PeerUpdate) { // Delete agents - update.deletedAgents - .compactMap { UUID(uuidData: $0.id) } - .forEach { agentID in - agents[agentID] = nil - } - update.deletedWorkspaces - .compactMap { UUID(uuidData: $0.id) } - .forEach { workspaceID in - workspaces[workspaceID] = nil - for (id, agent) in agents where agent.wsID == workspaceID { - agents[id] = nil - } - } - - // Update workspaces - for workspaceProto in update.upsertedWorkspaces { - if let workspaceID = UUID(uuidData: workspaceProto.id) { - workspaces[workspaceID] = workspaceProto.name - } - } - - for agentProto in update.upsertedAgents { - guard let agentID = UUID(uuidData: agentProto.id) else { - continue - } - guard let workspaceID = UUID(uuidData: agentProto.workspaceID) else { - continue - } - let workspaceName = workspaces[workspaceID] ?? "Unknown Workspace" - let newAgent = Agent( - id: agentID, - name: agentProto.name, - // If last handshake was not within last five minutes, the agent is unhealthy - status: agentProto.lastHandshake.date > Date.now.addingTimeInterval(-300) ? .okay : .off, - copyableDNS: agentProto.fqdn.first ?? "UNKNOWN", - wsName: workspaceName, - wsID: workspaceID - ) - - // An existing agent with the same name, belonging to the same workspace - // is from a previous workspace build, and should be removed. - agents - .filter { $0.value.name == agentProto.name && $0.value.wsID == workspaceID } - .forEach { agents[$0.key] = nil } - - agents[agentID] = newAgent - } + update.deletedAgents.forEach { menuState.deleteAgent(withId: $0.id) } + update.deletedWorkspaces.forEach { menuState.deleteWorkspace(withId: $0.id) } + // Upsert workspaces before agents to populate agent workspace names + update.upsertedWorkspaces.forEach { menuState.upsertWorkspace($0) } + update.upsertedAgents.forEach { menuState.upsertAgent($0) } } } diff --git a/Coder Desktop/Coder Desktop/Views/Agent.swift b/Coder Desktop/Coder Desktop/Views/Agent.swift deleted file mode 100644 index a24a5f79..00000000 --- a/Coder Desktop/Coder Desktop/Views/Agent.swift +++ /dev/null @@ -1,99 +0,0 @@ -import SwiftUI - -struct Agent: Identifiable, Equatable, Comparable { - let id: UUID - let name: String - let status: AgentStatus - let copyableDNS: String - let wsName: String - let wsID: UUID - - // Agents are sorted by status, and then by name - static func < (lhs: Agent, rhs: Agent) -> Bool { - if lhs.status != rhs.status { - return lhs.status < rhs.status - } - return lhs.wsName.localizedCompare(rhs.wsName) == .orderedAscending - } -} - -enum AgentStatus: Int, Equatable, Comparable { - case okay = 0 - case warn = 1 - case error = 2 - case off = 3 - - public var color: Color { - switch self { - case .okay: .green - case .warn: .yellow - case .error: .red - case .off: .gray - } - } - - static func < (lhs: AgentStatus, rhs: AgentStatus) -> Bool { - lhs.rawValue < rhs.rawValue - } -} - -struct AgentRowView: View { - let agent: Agent - let baseAccessURL: URL - @State private var nameIsSelected: Bool = false - @State private var copyIsSelected: Bool = false - - private var fmtWsName: AttributedString { - var formattedName = AttributedString(agent.wsName) - formattedName.foregroundColor = .primary - var coderPart = AttributedString(".coder") - coderPart.foregroundColor = .gray - formattedName.append(coderPart) - return formattedName - } - - private var wsURL: URL { - // TODO: CoderVPN currently only supports owned workspaces - baseAccessURL.appending(path: "@me").appending(path: agent.wsName) - } - - var body: some View { - HStack(spacing: 0) { - Link(destination: wsURL) { - HStack(spacing: Theme.Size.trayPadding) { - ZStack { - Circle() - .fill(agent.status.color.opacity(0.4)) - .frame(width: 12, height: 12) - Circle() - .fill(agent.status.color.opacity(1.0)) - .frame(width: 7, height: 7) - } - Text(fmtWsName).lineLimit(1).truncationMode(.tail) - Spacer() - }.padding(.horizontal, Theme.Size.trayPadding) - .frame(minHeight: 22) - .frame(maxWidth: .infinity, alignment: .leading) - .foregroundStyle(nameIsSelected ? Color.white : .primary) - .background(nameIsSelected ? Color.accentColor.opacity(0.8) : .clear) - .clipShape(.rect(cornerRadius: Theme.Size.rectCornerRadius)) - .onHover { hovering in nameIsSelected = hovering } - Spacer() - }.buttonStyle(.plain) - Button { - // TODO: Proper clipboard abstraction - NSPasteboard.general.setString(agent.copyableDNS, forType: .string) - } label: { - Image(systemName: "doc.on.doc") - .symbolVariant(.fill) - .padding(3) - }.foregroundStyle(copyIsSelected ? Color.white : .primary) - .imageScale(.small) - .background(copyIsSelected ? Color.accentColor.opacity(0.8) : .clear) - .clipShape(.rect(cornerRadius: Theme.Size.rectCornerRadius)) - .onHover { hovering in copyIsSelected = hovering } - .buttonStyle(.plain) - .padding(.trailing, Theme.Size.trayMargin) - } - } -} diff --git a/Coder Desktop/Coder Desktop/Views/Agents.swift b/Coder Desktop/Coder Desktop/Views/Agents.swift index 949ab109..44a8f138 100644 --- a/Coder Desktop/Coder Desktop/Views/Agents.swift +++ b/Coder Desktop/Coder Desktop/Views/Agents.swift @@ -12,15 +12,15 @@ struct Agents: View { Group { // Agents List if vpn.state == .connected { - let sortedAgents = vpn.agents.values.sorted() - let visibleData = viewAll ? sortedAgents[...] : sortedAgents.prefix(defaultVisibleRows) - ForEach(visibleData, id: \.id) { agent in - AgentRowView(agent: agent, baseAccessURL: session.baseAccessURL!) + let items = vpn.menuState.sorted() + let visibleItems = viewAll ? items[...] : items.prefix(defaultVisibleRows) + ForEach(visibleItems, id: \.id) { agent in + MenuItemView(item: agent, baseAccessURL: session.baseAccessURL!) .padding(.horizontal, Theme.Size.trayMargin) } - if vpn.agents.count > defaultVisibleRows { + if items.count > defaultVisibleRows { Toggle(isOn: $viewAll) { - Text(viewAll ? "Show Less" : "Show All") + Text(viewAll ? "Show less" : "Show all") .font(.headline) .foregroundColor(.gray) .padding(.horizontal, Theme.Size.trayInset) diff --git a/Coder Desktop/Coder Desktop/Views/VPNMenu.swift b/Coder Desktop/Coder Desktop/Views/VPNMenu.swift index 3f253e19..ec63d6e6 100644 --- a/Coder Desktop/Coder Desktop/Views/VPNMenu.swift +++ b/Coder Desktop/Coder Desktop/Views/VPNMenu.swift @@ -28,7 +28,7 @@ struct VPNMenu: View { .disabled(vpnDisabled) } Divider() - Text("Workspace Agents") + Text("Workspaces") .font(.headline) .foregroundColor(.gray) VPNState() diff --git a/Coder Desktop/Coder Desktop/Views/VPNMenuItem.swift b/Coder Desktop/Coder Desktop/Views/VPNMenuItem.swift new file mode 100644 index 00000000..6ccdcc05 --- /dev/null +++ b/Coder Desktop/Coder Desktop/Views/VPNMenuItem.swift @@ -0,0 +1,105 @@ +import SwiftUI + +// Each row in the workspaces list is an agent or an offline workspace +enum VPNMenuItem: Equatable, Comparable, Identifiable { + case agent(Agent) + case offlineWorkspace(Workspace) + + var wsName: String { + switch self { + case let .agent(agent): agent.wsName + case let .offlineWorkspace(workspace): workspace.name + } + } + + var status: AgentStatus { + switch self { + case let .agent(agent): agent.status + case .offlineWorkspace: .off + } + } + + var id: UUID { + switch self { + case let .agent(agent): agent.id + case let .offlineWorkspace(workspace): workspace.id + } + } + + static func < (lhs: VPNMenuItem, rhs: VPNMenuItem) -> Bool { + switch (lhs, rhs) { + case let (.agent(lhsAgent), .agent(rhsAgent)): + lhsAgent < rhsAgent + case let (.offlineWorkspace(lhsWorkspace), .offlineWorkspace(rhsWorkspace)): + lhsWorkspace < rhsWorkspace + // Agents always appear before offline workspaces + case (.offlineWorkspace, .agent): + false + case (.agent, .offlineWorkspace): + true + } + } +} + +struct MenuItemView: View { + let item: VPNMenuItem + let baseAccessURL: URL + @State private var nameIsSelected: Bool = false + @State private var copyIsSelected: Bool = false + + private var fmtWsName: AttributedString { + var formattedName = AttributedString(item.wsName) + formattedName.foregroundColor = .primary + var coderPart = AttributedString(".coder") + coderPart.foregroundColor = .gray + formattedName.append(coderPart) + return formattedName + } + + private var wsURL: URL { + // TODO: CoderVPN currently only supports owned workspaces + baseAccessURL.appending(path: "@me").appending(path: item.wsName) + } + + var body: some View { + HStack(spacing: 0) { + Link(destination: wsURL) { + HStack(spacing: Theme.Size.trayPadding) { + ZStack { + Circle() + .fill(item.status.color.opacity(0.4)) + .frame(width: 12, height: 12) + Circle() + .fill(item.status.color.opacity(1.0)) + .frame(width: 7, height: 7) + } + Text(fmtWsName).lineLimit(1).truncationMode(.tail) + Spacer() + }.padding(.horizontal, Theme.Size.trayPadding) + .frame(minHeight: 22) + .frame(maxWidth: .infinity, alignment: .leading) + .foregroundStyle(nameIsSelected ? Color.white : .primary) + .background(nameIsSelected ? Color.accentColor.opacity(0.8) : .clear) + .clipShape(.rect(cornerRadius: Theme.Size.rectCornerRadius)) + .onHover { hovering in nameIsSelected = hovering } + Spacer() + }.buttonStyle(.plain) + if case let .agent(agent) = item { + Button { + NSPasteboard.general.clearContents() + NSPasteboard.general.setString(agent.copyableDNS, forType: .string) + } label: { + Image(systemName: "doc.on.doc") + .symbolVariant(.fill) + .padding(3) + }.foregroundStyle(copyIsSelected ? Color.white : .primary) + .imageScale(.small) + .background(copyIsSelected ? Color.accentColor.opacity(0.8) : .clear) + .clipShape(.rect(cornerRadius: Theme.Size.rectCornerRadius)) + .onHover { hovering in copyIsSelected = hovering } + .buttonStyle(.plain) + .padding(.trailing, Theme.Size.trayMargin) + } + } + } +} diff --git a/Coder Desktop/Coder DesktopTests/AgentsTests.swift b/Coder Desktop/Coder DesktopTests/AgentsTests.swift index 8e06c8df..b93ca2bd 100644 --- a/Coder Desktop/Coder DesktopTests/AgentsTests.swift +++ b/Coder Desktop/Coder DesktopTests/AgentsTests.swift @@ -44,7 +44,7 @@ struct AgentsTests { @Test func agentsWhenVPNOn() throws { vpn.state = .connected - vpn.agents = createMockAgents(count: Theme.defaultVisibleAgents + 2) + vpn.menuState = .init(agents: createMockAgents(count: Theme.defaultVisibleAgents + 2)) let forEach = try view.inspect().find(ViewType.ForEach.self) #expect(forEach.count == Theme.defaultVisibleAgents) @@ -55,24 +55,24 @@ struct AgentsTests { @Test func showAllToggle() async throws { vpn.state = .connected - vpn.agents = createMockAgents(count: 7) + vpn.menuState = .init(agents: createMockAgents(count: 7)) try await ViewHosting.host(view) { try await sut.inspection.inspect { view in var toggle = try view.find(ViewType.Toggle.self) - #expect(try toggle.labelView().text().string() == "Show All") + #expect(try toggle.labelView().text().string() == "Show all") #expect(try !toggle.isOn()) try toggle.tap() toggle = try view.find(ViewType.Toggle.self) var forEach = try view.find(ViewType.ForEach.self) #expect(forEach.count == Theme.defaultVisibleAgents + 2) - #expect(try toggle.labelView().text().string() == "Show Less") + #expect(try toggle.labelView().text().string() == "Show less") try toggle.tap() toggle = try view.find(ViewType.Toggle.self) forEach = try view.find(ViewType.ForEach.self) - #expect(try toggle.labelView().text().string() == "Show All") + #expect(try toggle.labelView().text().string() == "Show all") #expect(forEach.count == Theme.defaultVisibleAgents) } } @@ -81,7 +81,7 @@ struct AgentsTests { @Test func noToggleFewAgents() throws { vpn.state = .connected - vpn.agents = createMockAgents(count: 3) + vpn.menuState = .init(agents: createMockAgents(count: 3)) #expect(throws: (any Error).self) { _ = try view.inspect().find(ViewType.Toggle.self) diff --git a/Coder Desktop/Coder DesktopTests/Util.swift b/Coder Desktop/Coder DesktopTests/Util.swift index d224615e..84f88212 100644 --- a/Coder Desktop/Coder DesktopTests/Util.swift +++ b/Coder Desktop/Coder DesktopTests/Util.swift @@ -8,7 +8,7 @@ import ViewInspector class MockVPNService: VPNService, ObservableObject { @Published var state: Coder_Desktop.VPNServiceState = .disabled @Published var baseAccessURL: URL = .init(string: "https://dev.coder.com")! - @Published var agents: [UUID: Coder_Desktop.Agent] = [:] + @Published var menuState: VPNMenuState = .init() var onStart: (() async -> Void)? var onStop: (() async -> Void)? diff --git a/Coder Desktop/Coder DesktopTests/VPNMenuStateTests.swift b/Coder Desktop/Coder DesktopTests/VPNMenuStateTests.swift new file mode 100644 index 00000000..439ebdb9 --- /dev/null +++ b/Coder Desktop/Coder DesktopTests/VPNMenuStateTests.swift @@ -0,0 +1,159 @@ +@testable import Coder_Desktop +import Testing +@testable import VPNLib + +@MainActor +@Suite +struct VPNMenuStateTests { + var state = VPNMenuState() + + @Test + mutating func testUpsertAgent_addsAgent() async throws { + let agentID = UUID() + let workspaceID = UUID() + state.upsertWorkspace(Vpn_Workspace.with { $0.id = workspaceID.uuidData; $0.name = "foo" }) + + let agent = Vpn_Agent.with { + $0.id = agentID.uuidData + $0.workspaceID = workspaceID.uuidData + $0.name = "dev" + $0.lastHandshake = .init(date: Date.now) + $0.fqdn = ["foo.coder"] + } + + state.upsertAgent(agent) + + let storedAgent = try #require(state.agents[agentID]) + #expect(storedAgent.name == "dev") + #expect(storedAgent.wsID == workspaceID) + #expect(storedAgent.wsName == "foo") + #expect(storedAgent.copyableDNS == "foo.coder") + #expect(storedAgent.status == .okay) + } + + @Test + mutating func testDeleteAgent_removesAgent() async throws { + let agentID = UUID() + let workspaceID = UUID() + state.upsertWorkspace(Vpn_Workspace.with { $0.id = workspaceID.uuidData; $0.name = "foo" }) + + let agent = Vpn_Agent.with { + $0.id = agentID.uuidData + $0.workspaceID = workspaceID.uuidData + $0.name = "agent1" + $0.lastHandshake = .init(date: Date.now) + $0.fqdn = ["foo.coder"] + } + + state.upsertAgent(agent) + state.deleteAgent(withId: agent.id) + + #expect(state.agents[agentID] == nil) + } + + @Test + mutating func testDeleteWorkspace_removesWorkspaceAndAgents() async throws { + let agentID = UUID() + let workspaceID = UUID() + state.upsertWorkspace(Vpn_Workspace.with { $0.id = workspaceID.uuidData; $0.name = "foo" }) + + let agent = Vpn_Agent.with { + $0.id = agentID.uuidData + $0.workspaceID = workspaceID.uuidData + $0.name = "agent1" + $0.lastHandshake = .init(date: Date.now) + $0.fqdn = ["foo.coder"] + } + + state.upsertAgent(agent) + state.deleteWorkspace(withId: workspaceID.uuidData) + + #expect(state.agents[agentID] == nil) + #expect(state.workspaces[workspaceID] == nil) + } + + @Test + mutating func testUpsertAgent_unhealthyAgent() async throws { + let agentID = UUID() + let workspaceID = UUID() + state.upsertWorkspace(Vpn_Workspace.with { $0.id = workspaceID.uuidData; $0.name = "foo" }) + + let agent = Vpn_Agent.with { + $0.id = agentID.uuidData + $0.workspaceID = workspaceID.uuidData + $0.name = "agent1" + $0.lastHandshake = .init(date: Date.now.addingTimeInterval(-600)) + $0.fqdn = ["foo.coder"] + } + + state.upsertAgent(agent) + + let storedAgent = try #require(state.agents[agentID]) + #expect(storedAgent.status == .warn) + } + + @Test + mutating func testUpsertAgent_replacesOldAgent() async throws { + let workspaceID = UUID() + let oldAgentID = UUID() + let newAgentID = UUID() + state.upsertWorkspace(Vpn_Workspace.with { $0.id = workspaceID.uuidData; $0.name = "foo" }) + + let oldAgent = Vpn_Agent.with { + $0.id = oldAgentID.uuidData + $0.workspaceID = workspaceID.uuidData + $0.name = "agent1" + $0.lastHandshake = .init(date: Date.now.addingTimeInterval(-600)) + $0.fqdn = ["foo.coder"] + } + + state.upsertAgent(oldAgent) + + let newAgent = Vpn_Agent.with { + $0.id = newAgentID.uuidData + $0.workspaceID = workspaceID.uuidData + $0.name = "agent1" // Same name as old agent + $0.lastHandshake = .init(date: Date.now) + $0.fqdn = ["foo.coder"] + } + + state.upsertAgent(newAgent) + + #expect(state.agents[oldAgentID] == nil) + let storedAgent = try #require(state.agents[newAgentID]) + #expect(storedAgent.name == "agent1") + #expect(storedAgent.wsID == workspaceID) + #expect(storedAgent.copyableDNS == "foo.coder") + #expect(storedAgent.status == .okay) + } + + @Test + mutating func testUpsertWorkspace_addsOfflineWorkspace() async throws { + let workspaceID = UUID() + state.upsertWorkspace(Vpn_Workspace.with { $0.id = workspaceID.uuidData; $0.name = "foo" }) + + let storedWorkspace = try #require(state.workspaces[workspaceID]) + #expect(storedWorkspace.name == "foo") + + var output = state.sorted() + #expect(output.count == 1) + #expect(output[0].id == workspaceID) + #expect(output[0].wsName == "foo") + + let agentID = UUID() + let agent = Vpn_Agent.with { + $0.id = agentID.uuidData + $0.workspaceID = workspaceID.uuidData + $0.name = "agent1" + $0.lastHandshake = .init(date: Date.now.addingTimeInterval(-200)) + $0.fqdn = ["foo.coder"] + } + state.upsertAgent(agent) + + output = state.sorted() + #expect(output.count == 1) + #expect(output[0].id == agentID) + #expect(output[0].wsName == "foo") + #expect(output[0].status == .okay) + } +} diff --git a/Coder Desktop/Coder DesktopTests/VPNServiceTests.swift b/Coder Desktop/Coder DesktopTests/VPNServiceTests.swift deleted file mode 100644 index 9d1370a3..00000000 --- a/Coder Desktop/Coder DesktopTests/VPNServiceTests.swift +++ /dev/null @@ -1,116 +0,0 @@ -@testable import Coder_Desktop -import Testing -@testable import VPNLib - -@MainActor -@Suite -struct CoderVPNServiceTests { - let service = CoderVPNService() - - init() { - service.workspaces = [:] - service.agents = [:] - } - - @Test - func testApplyPeerUpdate_upsertsAgents() async throws { - let agentID = UUID() - let workspaceID = UUID() - service.workspaces[workspaceID] = "foo" - - let update = Vpn_PeerUpdate.with { - $0.upsertedAgents = [Vpn_Agent.with { - $0.id = agentID.uuidData - $0.workspaceID = workspaceID.uuidData - $0.name = "dev" - $0.lastHandshake = .init(date: Date.now) - $0.fqdn = ["foo.coder"] - }] - } - - service.applyPeerUpdate(with: update) - - let agent = try #require(service.agents[agentID]) - #expect(agent.name == "dev") - #expect(agent.wsID == workspaceID) - #expect(agent.wsName == "foo") - #expect(agent.copyableDNS == "foo.coder") - #expect(agent.status == .okay) - } - - @Test - func testApplyPeerUpdate_deletesAgentsAndWorkspaces() async throws { - let agentID = UUID() - let workspaceID = UUID() - - service.agents[agentID] = Agent( - id: agentID, name: "agent1", status: .okay, - copyableDNS: "foo.coder", wsName: "foo", wsID: workspaceID - ) - service.workspaces[workspaceID] = "foo" - - let update = Vpn_PeerUpdate.with { - $0.deletedAgents = [Vpn_Agent.with { $0.id = agentID.uuidData }] - $0.deletedWorkspaces = [Vpn_Workspace.with { $0.id = workspaceID.uuidData }] - } - - service.applyPeerUpdate(with: update) - - #expect(service.agents[agentID] == nil) - #expect(service.workspaces[workspaceID] == nil) - } - - @Test - func testApplyPeerUpdate_unhealthyAgent() async throws { - let agentID = UUID() - let workspaceID = UUID() - service.workspaces[workspaceID] = "foo" - - let update = Vpn_PeerUpdate.with { - $0.upsertedAgents = [Vpn_Agent.with { - $0.id = agentID.uuidData - $0.workspaceID = workspaceID.uuidData - $0.name = "agent1" - $0.lastHandshake = .init(date: Date.now.addingTimeInterval(-600)) - $0.fqdn = ["foo.coder"] - }] - } - - service.applyPeerUpdate(with: update) - - let agent = try #require(service.agents[agentID]) - #expect(agent.status == .off) - } - - @Test - func testApplyPeerUpdate_replaceOldAgent() async throws { - let workspaceID = UUID() - let oldAgentID = UUID() - let newAgentID = UUID() - service.workspaces[workspaceID] = "foo" - - service.agents[oldAgentID] = Agent( - id: oldAgentID, name: "agent1", status: .off, - copyableDNS: "foo.coder", wsName: "foo", wsID: workspaceID - ) - - let update = Vpn_PeerUpdate.with { - $0.upsertedAgents = [Vpn_Agent.with { - $0.id = newAgentID.uuidData - $0.workspaceID = workspaceID.uuidData - $0.name = "agent1" // Same name as old agent - $0.lastHandshake = .init(date: Date.now) - $0.fqdn = ["foo.coder"] - }] - } - - service.applyPeerUpdate(with: update) - - #expect(service.agents[oldAgentID] == nil) - let newAgent = try #require(service.agents[newAgentID]) - #expect(newAgent.name == "agent1") - #expect(newAgent.wsID == workspaceID) - #expect(newAgent.copyableDNS == "foo.coder") - #expect(newAgent.status == .okay) - } -} From 972f269719387557571830c89d75e9226915e3ea Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Wed, 12 Feb 2025 18:08:07 +1100 Subject: [PATCH 5/6] review --- Coder Desktop/Coder Desktop/About.swift | 3 +- .../Preview Content/PreviewVPN.swift | 22 ++--- .../Coder Desktop/VPNMenuState.swift | 58 ++++++++---- .../Coder Desktop/Views/Agents.swift | 17 +++- .../Coder Desktop/Views/ButtonRow.swift | 11 ++- .../Coder Desktop/Views/InvalidAgents.swift | 56 ++++++++++++ Coder Desktop/Coder Desktop/Views/Util.swift | 8 ++ .../Coder Desktop/Views/VPNMenu.swift | 4 +- .../Coder Desktop/Views/VPNMenuItem.swift | 25 +++--- .../Coder Desktop/Views/VPNState.swift | 2 +- .../Coder DesktopTests/AgentsTests.swift | 89 +++++++++++++++++-- .../VPNMenuStateTests.swift | 60 ++++++++++++- .../Coder DesktopTests/VPNStateTests.swift | 2 +- 13 files changed, 301 insertions(+), 56 deletions(-) create mode 100644 Coder Desktop/Coder Desktop/Views/InvalidAgents.swift diff --git a/Coder Desktop/Coder Desktop/About.swift b/Coder Desktop/Coder Desktop/About.swift index 37711758..8849c9bd 100644 --- a/Coder Desktop/Coder Desktop/About.swift +++ b/Coder Desktop/Coder Desktop/About.swift @@ -1,6 +1,7 @@ import SwiftUI enum About { + public static let repo: String = "https://github.com/coder/coder-desktop-macos" private static var credits: NSAttributedString { let coder = NSMutableAttributedString( string: "Coder.com", @@ -21,7 +22,7 @@ enum About { string: "GitHub", attributes: [ .foregroundColor: NSColor.labelColor, - .link: NSURL(string: "https://github.com/coder/coder-desktop-macos")!, + .link: NSURL(string: About.repo)!, .font: NSFont.systemFont(ofSize: NSFont.systemFontSize), ] ) diff --git a/Coder Desktop/Coder Desktop/Preview Content/PreviewVPN.swift b/Coder Desktop/Coder Desktop/Preview Content/PreviewVPN.swift index 7e85a86c..4faa10fb 100644 --- a/Coder Desktop/Coder Desktop/Preview Content/PreviewVPN.swift +++ b/Coder Desktop/Coder Desktop/Preview Content/PreviewVPN.swift @@ -3,27 +3,27 @@ import SwiftUI @MainActor final class PreviewVPN: Coder_Desktop.VPNService { - @Published var state: Coder_Desktop.VPNServiceState = .disabled + @Published var state: Coder_Desktop.VPNServiceState = .connected @Published var menuState: VPNMenuState = .init(agents: [ - UUID(): Agent(id: UUID(), name: "dev", status: .error, copyableDNS: "asdf.coder", wsName: "dogfood2", + UUID(): Agent(id: UUID(), name: "dev", status: .error, hosts: ["asdf.coder"], wsName: "dogfood2", wsID: UUID()), - UUID(): Agent(id: UUID(), name: "dev", status: .okay, copyableDNS: "asdf.coder", + UUID(): Agent(id: UUID(), name: "dev", status: .okay, hosts: ["asdf.coder"], wsName: "testing-a-very-long-name", wsID: UUID()), - UUID(): Agent(id: UUID(), name: "dev", status: .warn, copyableDNS: "asdf.coder", wsName: "opensrc", + UUID(): Agent(id: UUID(), name: "dev", status: .warn, hosts: ["asdf.coder"], wsName: "opensrc", wsID: UUID()), - UUID(): Agent(id: UUID(), name: "dev", status: .off, copyableDNS: "asdf.coder", wsName: "gvisor", + UUID(): Agent(id: UUID(), name: "dev", status: .off, hosts: ["asdf.coder"], wsName: "gvisor", wsID: UUID()), - UUID(): Agent(id: UUID(), name: "dev", status: .off, copyableDNS: "asdf.coder", wsName: "example", + UUID(): Agent(id: UUID(), name: "dev", status: .off, hosts: ["asdf.coder"], wsName: "example", wsID: UUID()), - UUID(): Agent(id: UUID(), name: "dev", status: .error, copyableDNS: "asdf.coder", wsName: "dogfood2", + UUID(): Agent(id: UUID(), name: "dev", status: .error, hosts: ["asdf.coder"], wsName: "dogfood2", wsID: UUID()), - UUID(): Agent(id: UUID(), name: "dev", status: .okay, copyableDNS: "asdf.coder", + UUID(): Agent(id: UUID(), name: "dev", status: .okay, hosts: ["asdf.coder"], wsName: "testing-a-very-long-name", wsID: UUID()), - UUID(): Agent(id: UUID(), name: "dev", status: .warn, copyableDNS: "asdf.coder", wsName: "opensrc", + UUID(): Agent(id: UUID(), name: "dev", status: .warn, hosts: ["asdf.coder"], wsName: "opensrc", wsID: UUID()), - UUID(): Agent(id: UUID(), name: "dev", status: .off, copyableDNS: "asdf.coder", wsName: "gvisor", + UUID(): Agent(id: UUID(), name: "dev", status: .off, hosts: ["asdf.coder"], wsName: "gvisor", wsID: UUID()), - UUID(): Agent(id: UUID(), name: "dev", status: .off, copyableDNS: "asdf.coder", wsName: "example", + UUID(): Agent(id: UUID(), name: "dev", status: .off, hosts: ["asdf.coder"], wsName: "example", wsID: UUID()), ], workspaces: [:]) let shouldFail: Bool diff --git a/Coder Desktop/Coder Desktop/VPNMenuState.swift b/Coder Desktop/Coder Desktop/VPNMenuState.swift index 866afc23..e1a91a07 100644 --- a/Coder Desktop/Coder Desktop/VPNMenuState.swift +++ b/Coder Desktop/Coder Desktop/VPNMenuState.swift @@ -6,7 +6,7 @@ struct Agent: Identifiable, Equatable, Comparable { let id: UUID let name: String let status: AgentStatus - let copyableDNS: String + let hosts: [String] let wsName: String let wsID: UUID @@ -17,6 +17,9 @@ struct Agent: Identifiable, Equatable, Comparable { } return lhs.wsName.localizedCompare(rhs.wsName) == .orderedAscending } + + // Hosts arrive sorted by length, the shortest looks best in the UI. + var primaryHost: String? { hosts.first } } enum AgentStatus: Int, Equatable, Comparable { @@ -42,7 +45,7 @@ enum AgentStatus: Int, Equatable, Comparable { struct Workspace: Identifiable, Equatable, Comparable { let id: UUID let name: String - var agents: [UUID] + var agents: Set static func < (lhs: Workspace, rhs: Workspace) -> Bool { lhs.name.localizedCompare(rhs.name) == .orderedAscending @@ -52,42 +55,63 @@ struct Workspace: Identifiable, Equatable, Comparable { struct VPNMenuState { var agents: [UUID: Agent] = [:] var workspaces: [UUID: Workspace] = [:] + // Upserted agents that don't belong to any known workspace, have no FQDNs, + // or have any invalid UUIDs. + var invalidAgents: [Vpn_Agent] = [] mutating func upsertAgent(_ agent: Vpn_Agent) { - guard let id = UUID(uuidData: agent.id) else { return } - guard let wsID = UUID(uuidData: agent.workspaceID) else { return } + guard + let id = UUID(uuidData: agent.id), + let wsID = UUID(uuidData: agent.workspaceID), + var workspace = workspaces[wsID], + !agent.fqdn.isEmpty + else { + invalidAgents.append(agent) + return + } // An existing agent with the same name, belonging to the same workspace // is from a previous workspace build, and should be removed. agents.filter { $0.value.name == agent.name && $0.value.wsID == wsID } .forEach { agents[$0.key] = nil } - workspaces[wsID]?.agents.append(id) - let wsName = workspaces[wsID]?.name ?? "Unknown Workspace" + workspace.agents.insert(id) + workspaces[wsID] = workspace + agents[id] = Agent( id: id, name: agent.name, // If last handshake was not within last five minutes, the agent is unhealthy status: agent.lastHandshake.date > Date.now.addingTimeInterval(-300) ? .okay : .warn, - // Choose the shortest hostname, and remove trailing dot if present - copyableDNS: agent.fqdn.min(by: { $0.count < $1.count }) - .map { $0.hasSuffix(".") ? String($0.dropLast()) : $0 } ?? "UNKNOWN", - wsName: wsName, + // Remove trailing dot if present + hosts: agent.fqdn.map { $0.hasSuffix(".") ? String($0.dropLast()) : $0 }, + wsName: workspace.name, wsID: wsID ) } mutating func deleteAgent(withId id: Data) { - guard let id = UUID(uuidData: id) else { return } + guard let agentUUID = UUID(uuidData: id) else { return } // Update Workspaces - if let agent = agents[id], var ws = workspaces[agent.wsID] { - ws.agents.removeAll { $0 == id } + if let agent = agents[agentUUID], var ws = workspaces[agent.wsID] { + ws.agents.remove(agentUUID) workspaces[agent.wsID] = ws } - agents[id] = nil + agents[agentUUID] = nil + // Remove from invalid agents if present + invalidAgents.removeAll { invalidAgent in + invalidAgent.id == id + } } mutating func upsertWorkspace(_ workspace: Vpn_Workspace) { - guard let id = UUID(uuidData: workspace.id) else { return } - workspaces[id] = Workspace(id: id, name: workspace.name, agents: []) + guard let wsID = UUID(uuidData: workspace.id) else { return } + workspaces[wsID] = Workspace(id: wsID, name: workspace.name, agents: []) + // Check if we can associate any invalid agents with this workspace + invalidAgents.filter { agent in + agent.workspaceID == workspace.id + }.forEach { agent in + invalidAgents.removeAll { $0 == agent } + upsertAgent(agent) + } } mutating func deleteWorkspace(withId id: Data) { @@ -100,7 +124,7 @@ struct VPNMenuState { workspaces[wsID] = nil } - func sorted() -> [VPNMenuItem] { + var sorted: [VPNMenuItem] { var items = agents.values.map { VPNMenuItem.agent($0) } // Workspaces with no agents are shown as offline items += workspaces.filter { _, value in diff --git a/Coder Desktop/Coder Desktop/Views/Agents.swift b/Coder Desktop/Coder Desktop/Views/Agents.swift index 44a8f138..933a999a 100644 --- a/Coder Desktop/Coder Desktop/Views/Agents.swift +++ b/Coder Desktop/Coder Desktop/Views/Agents.swift @@ -12,13 +12,24 @@ struct Agents: View { Group { // Agents List if vpn.state == .connected { - let items = vpn.menuState.sorted() - let visibleItems = viewAll ? items[...] : items.prefix(defaultVisibleRows) + let items = vpn.menuState.sorted + let visibleOnlineItems = items.prefix(defaultVisibleRows) { + $0.status != .off + } + let visibleItems = viewAll ? items[...] : visibleOnlineItems ForEach(visibleItems, id: \.id) { agent in MenuItemView(item: agent, baseAccessURL: session.baseAccessURL!) .padding(.horizontal, Theme.Size.trayMargin) } - if items.count > defaultVisibleRows { + if visibleItems.count == 0 { + Text("No \(items.count > 0 ? "running " : "")workspaces!") + .font(.body) + .foregroundColor(.gray) + .padding(.horizontal, Theme.Size.trayInset) + .padding(.top, 2) + } + // Only show the toggle if there are more items to show + if visibleOnlineItems.count < items.count { Toggle(isOn: $viewAll) { Text(viewAll ? "Show less" : "Show all") .font(.headline) diff --git a/Coder Desktop/Coder Desktop/Views/ButtonRow.swift b/Coder Desktop/Coder Desktop/Views/ButtonRow.swift index 088eb136..9dbb916c 100644 --- a/Coder Desktop/Coder Desktop/Views/ButtonRow.swift +++ b/Coder Desktop/Coder Desktop/Views/ButtonRow.swift @@ -1,6 +1,13 @@ import SwiftUI struct ButtonRowView: View { + init(highlightColor: Color = .accentColor, isSelected: Bool = false, label: @escaping () -> Label) { + self.highlightColor = highlightColor + self.isSelected = isSelected + self.label = label + } + + let highlightColor: Color @State private var isSelected: Bool = false @ViewBuilder var label: () -> Label @@ -12,8 +19,8 @@ struct ButtonRowView: View { .padding(.horizontal, Theme.Size.trayPadding) .frame(minHeight: 22) .frame(maxWidth: .infinity, alignment: .leading) - .foregroundStyle(isSelected ? Color.white : .primary) - .background(isSelected ? Color.accentColor.opacity(0.8) : .clear) + .foregroundStyle(isSelected ? .white : .primary) + .background(isSelected ? highlightColor.opacity(0.8) : .clear) .clipShape(.rect(cornerRadius: Theme.Size.rectCornerRadius)) .onHover { hovering in isSelected = hovering } } diff --git a/Coder Desktop/Coder Desktop/Views/InvalidAgents.swift b/Coder Desktop/Coder Desktop/Views/InvalidAgents.swift new file mode 100644 index 00000000..9e27fa52 --- /dev/null +++ b/Coder Desktop/Coder Desktop/Views/InvalidAgents.swift @@ -0,0 +1,56 @@ +import SwiftUI +import VPNLib + +struct InvalidAgentsButton: View { + @Environment(\.dismiss) var dismiss + @EnvironmentObject var vpn: VPN + var msg: String { + "\(vpn.menuState.invalidAgents.count) invalid \(vpn.menuState.invalidAgents.count > 1 ? "agents" : "agent").." + } + + var body: some View { + Button { + showAlert() + } label: { + ButtonRowView(highlightColor: .red) { Text(msg) } + }.buttonStyle(.plain) + } + + // `.alert` from SwiftUI doesn't play nice when the calling view is in the + // menu bar. + private func showAlert() { + let formattedAgents = vpn.menuState.invalidAgents.map { agent in + let agent_id = if let agent_id = UUID(uuidData: agent.id) { + agent_id.uuidString + } else { + "Invalid ID: \(agent.id.base64EncodedString())" + } + let wsID = if let wsID = UUID(uuidData: agent.workspaceID) { + wsID.uuidString + } else { + "Invalid ID: \(agent.workspaceID.base64EncodedString())" + } + let lastHandshake = agent.hasLastHandshake ? "\(agent.lastHandshake)" : "Never" + return """ + Agent Name: \(agent.name) + ID: \(agent_id) + Workspace ID: \(wsID) + Last Handshake: \(lastHandshake) + FQDNs: \(agent.fqdn) + Addresses: \(agent.ipAddrs) + """ + }.joined(separator: "\n\n") + + let alert = NSAlert() + alert.messageText = "Invalid Agents" + alert.informativeText = """ + Coder Desktop received invalid agents from the VPN. This should + never happen. Please open an issue on \(About.repo). + + \(formattedAgents) + """ + alert.alertStyle = .warning + dismiss() + alert.runModal() + } +} diff --git a/Coder Desktop/Coder Desktop/Views/Util.swift b/Coder Desktop/Coder Desktop/Views/Util.swift index 693dc935..8f9ef31a 100644 --- a/Coder Desktop/Coder Desktop/Views/Util.swift +++ b/Coder Desktop/Coder Desktop/Views/Util.swift @@ -31,3 +31,11 @@ extension UUID { self.init(uuid: uuid) } } + +extension Array { + func prefix(_ maxCount: Int, while predicate: (Element) -> Bool) -> ArraySlice { + let failureIndex = enumerated().first(where: { !predicate($0.element) })?.offset ?? count + let endIndex = Swift.min(failureIndex, maxCount) + return self[..: View { // Trailing stack VStack(alignment: .leading, spacing: 3) { TrayDivider() + if vpn.state == .connected, !vpn.menuState.invalidAgents.isEmpty { + InvalidAgentsButton() + } if session.hasSession { Link(destination: session.baseAccessURL!.appending(path: "templates")) { ButtonRowView { Text("Create workspace") - EmptyView() } }.buttonStyle(.plain) TrayDivider() diff --git a/Coder Desktop/Coder Desktop/Views/VPNMenuItem.swift b/Coder Desktop/Coder Desktop/Views/VPNMenuItem.swift index 6ccdcc05..43aac471 100644 --- a/Coder Desktop/Coder Desktop/Views/VPNMenuItem.swift +++ b/Coder Desktop/Coder Desktop/Views/VPNMenuItem.swift @@ -47,12 +47,17 @@ struct MenuItemView: View { @State private var nameIsSelected: Bool = false @State private var copyIsSelected: Bool = false - private var fmtWsName: AttributedString { - var formattedName = AttributedString(item.wsName) + private var itemName: AttributedString { + let name = switch item { + case let .agent(agent): agent.primaryHost ?? "\(item.wsName).coder" + case .offlineWorkspace: "\(item.wsName).coder" + } + + var formattedName = AttributedString(name) formattedName.foregroundColor = .primary - var coderPart = AttributedString(".coder") - coderPart.foregroundColor = .gray - formattedName.append(coderPart) + if let range = formattedName.range(of: ".coder") { + formattedName[range].foregroundColor = .gray + } return formattedName } @@ -73,26 +78,26 @@ struct MenuItemView: View { .fill(item.status.color.opacity(1.0)) .frame(width: 7, height: 7) } - Text(fmtWsName).lineLimit(1).truncationMode(.tail) + Text(itemName).lineLimit(1).truncationMode(.tail) Spacer() }.padding(.horizontal, Theme.Size.trayPadding) .frame(minHeight: 22) .frame(maxWidth: .infinity, alignment: .leading) - .foregroundStyle(nameIsSelected ? Color.white : .primary) + .foregroundStyle(nameIsSelected ? .white : .primary) .background(nameIsSelected ? Color.accentColor.opacity(0.8) : .clear) .clipShape(.rect(cornerRadius: Theme.Size.rectCornerRadius)) .onHover { hovering in nameIsSelected = hovering } Spacer() }.buttonStyle(.plain) - if case let .agent(agent) = item { + if case let .agent(agent) = item, let copyableDNS = agent.primaryHost { Button { NSPasteboard.general.clearContents() - NSPasteboard.general.setString(agent.copyableDNS, forType: .string) + NSPasteboard.general.setString(copyableDNS, forType: .string) } label: { Image(systemName: "doc.on.doc") .symbolVariant(.fill) .padding(3) - }.foregroundStyle(copyIsSelected ? Color.white : .primary) + }.foregroundStyle(copyIsSelected ? .white : .primary) .imageScale(.small) .background(copyIsSelected ? Color.accentColor.opacity(0.8) : .clear) .clipShape(.rect(cornerRadius: Theme.Size.rectCornerRadius)) diff --git a/Coder Desktop/Coder Desktop/Views/VPNState.swift b/Coder Desktop/Coder Desktop/Views/VPNState.swift index 4afc6c26..b7a090b9 100644 --- a/Coder Desktop/Coder Desktop/Views/VPNState.swift +++ b/Coder Desktop/Coder Desktop/Views/VPNState.swift @@ -18,7 +18,7 @@ struct VPNState: View { .font(.body) .foregroundColor(.gray) case (.disabled, _): - Text("Enable CoderVPN to see agents") + Text("Enable CoderVPN to see workspaces") .font(.body) .foregroundStyle(.gray) case (.connecting, _), (.disconnecting, _): diff --git a/Coder Desktop/Coder DesktopTests/AgentsTests.swift b/Coder Desktop/Coder DesktopTests/AgentsTests.swift index b93ca2bd..1fd2022d 100644 --- a/Coder Desktop/Coder DesktopTests/AgentsTests.swift +++ b/Coder Desktop/Coder DesktopTests/AgentsTests.swift @@ -18,14 +18,14 @@ struct AgentsTests { view = sut.environmentObject(vpn).environmentObject(session) } - private func createMockAgents(count: Int) -> [UUID: Agent] { + private func createMockAgents(count: Int, status: AgentStatus = .okay) -> [UUID: Agent] { Dictionary(uniqueKeysWithValues: (1 ... count).map { let agent = Agent( id: UUID(), name: "dev", - status: .okay, - copyableDNS: "a\($0).example.com", - wsName: "a\($0)", + status: status, + hosts: ["a\($0).coder"], + wsName: "ws\($0)", wsID: UUID() ) return (agent.id, agent) @@ -41,6 +41,17 @@ struct AgentsTests { } } + @Test func noAgents() async throws { + vpn.state = .connected + vpn.menuState = .init(agents: [:]) + + try await ViewHosting.host(view) { + try await sut.inspection.inspect { view in + #expect(throws: Never.self) { try view.find(text: "No workspaces!") } + } + } + } + @Test func agentsWhenVPNOn() throws { vpn.state = .connected @@ -60,12 +71,14 @@ struct AgentsTests { try await ViewHosting.host(view) { try await sut.inspection.inspect { view in var toggle = try view.find(ViewType.Toggle.self) + var forEach = try view.find(ViewType.ForEach.self) + #expect(forEach.count == Theme.defaultVisibleAgents) #expect(try toggle.labelView().text().string() == "Show all") #expect(try !toggle.isOn()) try toggle.tap() toggle = try view.find(ViewType.Toggle.self) - var forEach = try view.find(ViewType.ForEach.self) + forEach = try view.find(ViewType.ForEach.self) #expect(forEach.count == Theme.defaultVisibleAgents + 2) #expect(try toggle.labelView().text().string() == "Show less") @@ -87,4 +100,70 @@ struct AgentsTests { _ = try view.inspect().find(ViewType.Toggle.self) } } + + @Test func showAllToggle_noOnlineWorkspaces() async throws { + vpn.state = .connected + let tmpAgents = createMockAgents(count: Theme.defaultVisibleAgents + 1, status: .off) + vpn.menuState = .init(agents: tmpAgents) + + try await ViewHosting.host(view) { + try await sut.inspection.inspect { view in + var toggle = try view.find(ViewType.Toggle.self) + var forEach = try view.find(ViewType.ForEach.self) + #expect(throws: Never.self) { try view.find(text: "No running workspaces!") } + #expect(forEach.count == 0) + #expect(try toggle.labelView().text().string() == "Show all") + #expect(try !toggle.isOn()) + + try toggle.tap() + toggle = try view.find(ViewType.Toggle.self) + forEach = try view.find(ViewType.ForEach.self) + #expect(forEach.count == Theme.defaultVisibleAgents + 1) + #expect(try toggle.labelView().text().string() == "Show less") + + try toggle.tap() + toggle = try view.find(ViewType.Toggle.self) + forEach = try view.find(ViewType.ForEach.self) + #expect(try toggle.labelView().text().string() == "Show all") + #expect(forEach.count == 0) + } + } + } + + @Test + func showAllToggle_oneOfflineWorkspace() async throws { + vpn.state = .connected + vpn.menuState = .init(agents: createMockAgents(count: Theme.defaultVisibleAgents - 2)) + let offlineAgent = Agent( + id: UUID(), + name: "dev", + status: .off, + hosts: ["offline.coder"], + wsName: "offlinews", + wsID: UUID() + ) + vpn.menuState.agents[offlineAgent.id] = offlineAgent + + try await ViewHosting.host(view) { + try await sut.inspection.inspect { view in + var toggle = try view.find(ViewType.Toggle.self) + var forEach = try view.find(ViewType.ForEach.self) + #expect(forEach.count == Theme.defaultVisibleAgents - 2) + #expect(try toggle.labelView().text().string() == "Show all") + #expect(try !toggle.isOn()) + + try toggle.tap() + toggle = try view.find(ViewType.Toggle.self) + forEach = try view.find(ViewType.ForEach.self) + #expect(forEach.count == Theme.defaultVisibleAgents - 1) + #expect(try toggle.labelView().text().string() == "Show less") + + try toggle.tap() + toggle = try view.find(ViewType.Toggle.self) + forEach = try view.find(ViewType.ForEach.self) + #expect(try toggle.labelView().text().string() == "Show all") + #expect(forEach.count == Theme.defaultVisibleAgents - 2) + } + } + } } diff --git a/Coder Desktop/Coder DesktopTests/VPNMenuStateTests.swift b/Coder Desktop/Coder DesktopTests/VPNMenuStateTests.swift index 439ebdb9..d82aff8e 100644 --- a/Coder Desktop/Coder DesktopTests/VPNMenuStateTests.swift +++ b/Coder Desktop/Coder DesktopTests/VPNMenuStateTests.swift @@ -27,7 +27,7 @@ struct VPNMenuStateTests { #expect(storedAgent.name == "dev") #expect(storedAgent.wsID == workspaceID) #expect(storedAgent.wsName == "foo") - #expect(storedAgent.copyableDNS == "foo.coder") + #expect(storedAgent.primaryHost == "foo.coder") #expect(storedAgent.status == .okay) } @@ -123,7 +123,7 @@ struct VPNMenuStateTests { let storedAgent = try #require(state.agents[newAgentID]) #expect(storedAgent.name == "agent1") #expect(storedAgent.wsID == workspaceID) - #expect(storedAgent.copyableDNS == "foo.coder") + #expect(storedAgent.primaryHost == "foo.coder") #expect(storedAgent.status == .okay) } @@ -135,7 +135,7 @@ struct VPNMenuStateTests { let storedWorkspace = try #require(state.workspaces[workspaceID]) #expect(storedWorkspace.name == "foo") - var output = state.sorted() + var output = state.sorted #expect(output.count == 1) #expect(output[0].id == workspaceID) #expect(output[0].wsName == "foo") @@ -150,10 +150,62 @@ struct VPNMenuStateTests { } state.upsertAgent(agent) - output = state.sorted() + output = state.sorted #expect(output.count == 1) #expect(output[0].id == agentID) #expect(output[0].wsName == "foo") #expect(output[0].status == .okay) } + + @Test + mutating func testUpsertAgent_invalidAgent_noUUID() async throws { + let agent = Vpn_Agent.with { + $0.name = "invalidAgent" + $0.fqdn = ["invalid.coder"] + } + + state.upsertAgent(agent) + + #expect(state.agents.isEmpty) + #expect(state.invalidAgents.count == 1) + } + + @Test + mutating func testUpsertAgent_outOfOrder() async throws { + let agentID = UUID() + let workspaceID = UUID() + + let agent = Vpn_Agent.with { + $0.id = agentID.uuidData + $0.workspaceID = workspaceID.uuidData + $0.name = "orphanAgent" + $0.lastHandshake = .init(date: Date.now) + $0.fqdn = ["orphan.coder"] + } + + state.upsertAgent(agent) + #expect(state.agents.isEmpty) + state.upsertWorkspace(Vpn_Workspace.with { $0.id = workspaceID.uuidData; $0.name = "validWorkspace" }) + #expect(state.agents.count == 1) + } + + @Test + mutating func testDeleteInvalidAgent_removesInvalid() async throws { + let agentID = UUID() + let workspaceID = UUID() + + let agent = Vpn_Agent.with { + $0.id = agentID.uuidData + $0.workspaceID = workspaceID.uuidData + $0.name = "invalidAgent" + $0.lastHandshake = .init(date: Date.now) + $0.fqdn = ["invalid.coder"] + } + + state.upsertAgent(agent) + #expect(state.agents.isEmpty) + state.deleteAgent(withId: agentID.uuidData) + #expect(state.agents.isEmpty) + #expect(state.invalidAgents.isEmpty) + } } diff --git a/Coder Desktop/Coder DesktopTests/VPNStateTests.swift b/Coder Desktop/Coder DesktopTests/VPNStateTests.swift index 298bacd5..1330f068 100644 --- a/Coder Desktop/Coder DesktopTests/VPNStateTests.swift +++ b/Coder Desktop/Coder DesktopTests/VPNStateTests.swift @@ -26,7 +26,7 @@ struct VPNStateTests { try await ViewHosting.host(view) { try await sut.inspection.inspect { view in #expect(throws: Never.self) { - try view.find(text: "Enable CoderVPN to see agents") + try view.find(text: "Enable CoderVPN to see workspaces") } } } From 63442fe5f5a5bcae80b3206e2fc450485a95d511 Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Wed, 12 Feb 2025 18:46:00 +1100 Subject: [PATCH 6/6] undo dumb idea --- .../Coder Desktop/Views/Agents.swift | 11 ++-- Coder Desktop/Coder Desktop/Views/Util.swift | 8 --- .../Coder DesktopTests/AgentsTests.swift | 63 +++---------------- 3 files changed, 11 insertions(+), 71 deletions(-) diff --git a/Coder Desktop/Coder Desktop/Views/Agents.swift b/Coder Desktop/Coder Desktop/Views/Agents.swift index 933a999a..53c04418 100644 --- a/Coder Desktop/Coder Desktop/Views/Agents.swift +++ b/Coder Desktop/Coder Desktop/Views/Agents.swift @@ -13,23 +13,20 @@ struct Agents: View { // Agents List if vpn.state == .connected { let items = vpn.menuState.sorted - let visibleOnlineItems = items.prefix(defaultVisibleRows) { - $0.status != .off - } - let visibleItems = viewAll ? items[...] : visibleOnlineItems + let visibleItems = viewAll ? items[...] : items.prefix(defaultVisibleRows) ForEach(visibleItems, id: \.id) { agent in MenuItemView(item: agent, baseAccessURL: session.baseAccessURL!) .padding(.horizontal, Theme.Size.trayMargin) } - if visibleItems.count == 0 { - Text("No \(items.count > 0 ? "running " : "")workspaces!") + if items.count == 0 { + Text("No workspaces!") .font(.body) .foregroundColor(.gray) .padding(.horizontal, Theme.Size.trayInset) .padding(.top, 2) } // Only show the toggle if there are more items to show - if visibleOnlineItems.count < items.count { + if items.count > defaultVisibleRows { Toggle(isOn: $viewAll) { Text(viewAll ? "Show less" : "Show all") .font(.headline) diff --git a/Coder Desktop/Coder Desktop/Views/Util.swift b/Coder Desktop/Coder Desktop/Views/Util.swift index 8f9ef31a..693dc935 100644 --- a/Coder Desktop/Coder Desktop/Views/Util.swift +++ b/Coder Desktop/Coder Desktop/Views/Util.swift @@ -31,11 +31,3 @@ extension UUID { self.init(uuid: uuid) } } - -extension Array { - func prefix(_ maxCount: Int, while predicate: (Element) -> Bool) -> ArraySlice { - let failureIndex = enumerated().first(where: { !predicate($0.element) })?.offset ?? count - let endIndex = Swift.min(failureIndex, maxCount) - return self[..