1c8b1da · parseable-macos

git.iko.soy/parseable-macos

on master help sign in

Fix connection lifecycle and credential checks

1c8b1daClaude1 week ago

ParseableViewer/App/AppState.swift+81−5

compare with master

unifiedsplit

@@ -15,6 +15,10 @@ final class AppState {

15 private let networkMonitor = NWPathMonitor()

16 private let networkMonitorQueue = DispatchQueue(label: "com.parseableviewer.network-monitor")

17 private var connectTask: Task<Bool, Never>?

18+ /// The connection `connectTask` is connecting to. `activeConnection` only

19+ /// switches once the new server answers, so this is what identifies an

20+ /// in-flight connect whose target the user deletes (see `removeConnection`).

21+ private var connectingConnectionID: UUID?

19 // MARK: - Stream State

23 // MARK: - Stream State

20 var streams: [LogStream] = []

24 var streams: [LogStream] = []

@@ -96,6 +100,7 @@ final class AppState {

100

97 if let activeID = ConnectionStore.loadActiveConnectionID(),

101 if let activeID = ConnectionStore.loadActiveConnectionID(),

98 let connection = connections.first(where: { $0.id == activeID }) {

102 let connection = connections.first(where: { $0.id == activeID }) {

103+ connectingConnectionID = connection.id

99 connectTask = Task { @MainActor [weak self] in

104 connectTask = Task { @MainActor [weak self] in

100 await self?.performConnect(to: connection) ?? false

105 await self?.performConnect(to: connection) ?? false

101 }

106 }

@@ -115,6 +120,7 @@ final class AppState {

115 existing.cancel()

120 existing.cancel()

116 _ = await existing.value

121 _ = await existing.value

117 }

122 }

123+ connectingConnectionID = connection.id

118 let task = Task { @MainActor [weak self] in

124 let task = Task { @MainActor [weak self] in

119 await self?.performConnect(to: connection) ?? false

125 await self?.performConnect(to: connection) ?? false

120 }

126 }

@@ -124,9 +130,16 @@ final class AppState {

124

130

125 @MainActor

131 @MainActor

126 private func performConnect(to connection: ServerConnection) async -> Bool {

132 private func performConnect(to connection: ServerConnection) async -> Bool {

133+ defer {

134+ if connectingConnectionID == connection.id { connectingConnectionID = nil }

135+ }

127 guard !isConnecting else { return false }

136 guard !isConnecting else { return false }

128

137

138+ // NWPathMonitor ignores loopback, so with no active network interface it

139+ // reports "unsatisfied" even though a local server (the common docker /

140+ // dev setup) is reachable. Don't block those; a genuinely unreachable

141+ // host still fails below with the same offline message.

129− if !isNetworkAvailable {

142+ if !isNetworkAvailable && !Self.isLoopback(connection.baseURL) {

130 self.errorMessage = "No internet connection. Check your network and try again."

143 self.errorMessage = "No internet connection. Check your network and try again."

131 self.showError = true

144 self.showError = true

132 return false

145 return false

@@ -134,7 +147,12 @@ final class AppState {

134

147

135 isConnecting = true

148 isConnecting = true

136 defer { isConnecting = false }

149 defer { isConnecting = false }

150+ // Reset the flag with the message: a previous failed connect left

151+ // `showError` set, and if its alert couldn't present (e.g. behind the

152+ // connection sheet), it would pop up later — after a successful retry —

153+ // reading "An unknown error occurred".

137 errorMessage = nil

154 errorMessage = nil

155+ showError = false

138

156

139 // When switching to a different server, clear stream-specific state

157 // When switching to a different server, clear stream-specific state

140 // so stale selections don't reference streams that don't exist.

158 // so stale selections don't reference streams that don't exist.

@@ -164,6 +182,7 @@ final class AppState {

164

182

165 // Test connection — throws on non-200 or network failure

183 // Test connection — throws on non-200 or network failure

166 try await newClient.checkHealth()

184 try await newClient.checkHealth()

185+ try await Self.verifyCredentials(with: newClient)

167

186

168 // If the task was cancelled while awaiting (e.g. user clicked

187 // If the task was cancelled while awaiting (e.g. user clicked

169 // Disconnect during auto-reconnect), bail before installing the

188 // Disconnect during auto-reconnect), bail before installing the

@@ -200,6 +219,12 @@ final class AppState {

200 case .success(let list):

219 case .success(let list):

201 self.streams = list

220 self.streams = list

202 self.streamLoadError = nil

221 self.streamLoadError = nil

222+ // Like refreshStreams: a selection that doesn't exist on this server

223+ // (e.g. one picked from a stale list while the connect was pending)

224+ // would keep every stream tab querying a missing stream.

225+ if let selected = selectedStream, !list.contains(where: { $0.name == selected }) {

226+ selectedStream = nil

227+ }

203 case .failure(let error):

228 case .failure(let error):

204 self.streams = []

229 self.streams = []

205 self.streamLoadError = ParseableError.userFriendlyMessage(for: error)

230 self.streamLoadError = ParseableError.userFriendlyMessage(for: error)

@@ -265,8 +290,13 @@ final class AppState {

265 let loaded = try await client.listStreams()

290 let loaded = try await client.listStreams()

266 // Drop results if the active connection changed while awaiting. Still

291 // Drop results if the active connection changed while awaiting. Still

267 // clear isLoadingStreams: this task set the spinner, so leaving it true

292 // clear isLoadingStreams: this task set the spinner, so leaving it true

268− // wedges the indicator and disables Refresh.

293+ // wedges the indicator and disables Refresh. Compare the client too:

294+ // editing the active connection's URL/credentials keeps its id but

295+ // installs a new client for a possibly different server.

269− guard connID == activeConnection?.id else { isLoadingStreams = false; return }

296+ guard connID == activeConnection?.id, client === self.client else {

297+ isLoadingStreams = false

298+ return

299+ }

270 streams = loaded

300 streams = loaded

271 // Drop a selection that no longer exists server-side (e.g. the stream

301 // Drop a selection that no longer exists server-side (e.g. the stream

272 // was deleted from another client); otherwise the detail tabs keep

302 // was deleted from another client); otherwise the detail tabs keep

@@ -275,7 +305,10 @@ final class AppState {

275 selectedStream = nil

305 selectedStream = nil

276 }

306 }

277 } catch {

307 } catch {

278− guard connID == activeConnection?.id else { isLoadingStreams = false; return }

308+ guard connID == activeConnection?.id, client === self.client else {

309+ isLoadingStreams = false

310+ return

311+ }

279 let message = ParseableError.userFriendlyMessage(for: error)

312 let message = ParseableError.userFriendlyMessage(for: error)

280 streamLoadError = message

313 streamLoadError = message

281 self.errorMessage = "Failed to load streams: \(message)"

314 self.errorMessage = "Failed to load streams: \(message)"

@@ -342,9 +375,49 @@ final class AppState {

342 ConnectionStore.saveConnections(connections)

375 ConnectionStore.saveConnections(connections)

343 if activeConnection?.id == connection.id {

376 if activeConnection?.id == connection.id {

344 disconnect()

377 disconnect()

378+ } else if connectingConnectionID == connection.id {

379+ // The removed connection is still being connected to (e.g. Cancel in the

380+ // connection sheet, or deleting it in Settings during the launch

381+ // reconnect). `activeConnection` doesn't point at it yet, so without

382+ // this the connect would carry on and install a client for a

383+ // connection that no longer exists (or tear down the current session

384+ // when it fails) after the user abandoned it.

385+ connectTask?.cancel()

386+ connectTask = nil

387+ connectingConnectionID = nil

388+ // Starting that connect already cleared the current server's stream

389+ // list and filters; reload them if we're still connected to it.

390+ if client != nil {

391+ Task { @MainActor [weak self] in

392+ await self?.refreshStreams()

393+ await self?.refreshFilters()

394+ }

395+ }

396+ }

397+ }

398+

399+ /// `/api/v1/liveness` is unauthenticated on Parseable (it answers 200 to

400+ /// anyone, for load-balancer / k8s probes), so a health check alone accepts

401+ /// any username and password. Probe an authenticated endpoint and fail on

402+ /// 401. Other failures — e.g. a 403 for a role without the about permission —

403+ /// don't show the credentials are wrong, so they're ignored here. The client

404+ /// caches the response, so the follow-up `getAbout()` load reuses it.

405+ static func verifyCredentials(with client: ParseableClient) async throws {

406+ do {

407+ _ = try await client.getAbout()

408+ } catch ParseableError.unauthorized {

409+ throw ParseableError.unauthorized

410+ } catch {

411+ // Not an authentication failure; leave it to the best-effort loads.

345 }

412 }

346 }

413 }

347

414

415+ private static func isLoopback(_ url: URL?) -> Bool {

416+ guard let host = url?.host?.lowercased() else { return false }

417+ return host == "localhost" || host.hasSuffix(".localhost")

418+ || host == "::1" || host.hasPrefix("127.")

419+ }

420+

348 // MARK: - Stream Management

421 // MARK: - Stream Management

349

422

350 @MainActor

423 @MainActor

@@ -377,7 +450,10 @@ final class AppState {

377 let connID = activeConnection?.id

450 let connID = activeConnection?.id

378 isLoadingFilters = true

451 isLoadingFilters = true

379 let loaded = (try? await client.listFilters()) ?? []

452 let loaded = (try? await client.listFilters()) ?? []

380− guard connID == activeConnection?.id else { isLoadingFilters = false; return }

453+ guard connID == activeConnection?.id, client === self.client else {

454+ isLoadingFilters = false

455+ return

456+ }

381 filters = loaded

457 filters = loaded

382 isLoadingFilters = false

458 isLoadingFilters = false

383 }

459 }

ParseableViewer/App/ParseableViewerApp.swift+13−1

@@ -4,6 +4,13 @@ import SwiftUI

4struct ParseableViewerApp: App {

5 @State private var appState = AppState()

7+ init() {

8+ // All window content shares one AppState (see the File > New group below),

9+ // so don't offer window tabs either: the tab bar's "+" would open a second

10+ // window just like File > New Window.

11+ NSWindow.allowsAutomaticWindowTabbing = false

12+ }

13+

7 var body: some Scene {

14 var body: some Scene {

8 WindowGroup {

15 WindowGroup {

9 ContentView()

16 ContentView()

@@ -17,7 +24,12 @@ struct ParseableViewerApp: App {

17 AboutCommand()

24 AboutCommand()

18 }

25 }

27+ // Replace (rather than extend) the File > New group to drop "New Window":

28+ // connection, sheet, alert, selection and pending-filter state all live

29+ // in the single shared AppState, so a second window would present every

30+ // sheet and alert twice and race the first window's QueryView for

31+ // saved-filter SQL.

20− CommandGroup(after: .newItem) {

32+ CommandGroup(replacing: .newItem) {

21 Button("New Connection...") {

33 Button("New Connection...") {

22 appState.editingConnection = nil

34 appState.editingConnection = nil

23 appState.showConnectionSheet = true

35 appState.showConnectionSheet = true

ParseableViewer/Info.plist+5

@@ -24,6 +24,11 @@

24 <string>public.app-category.developer-tools</string>

25 <key>LSMinimumSystemVersion</key>

26 <string>$(MACOSX_DEPLOYMENT_TARGET)</string>

27+ <key>NSAppTransportSecurity</key>

28+ <dict>

29+ <key>NSAllowsArbitraryLoads</key>

30+ <true/>

31+ </dict>

27 <key>NSMainNibFile</key>

32 <key>NSMainNibFile</key>

28 <string></string>

33 <string></string>

29 <key>NSPrincipalClass</key>

34 <key>NSPrincipalClass</key>

ParseableViewer/Models/ServerConnection.swift+19−2

@@ -56,15 +56,32 @@ struct ServerConnection: Identifiable, Codable, Hashable, Sendable {

56 var baseURL: URL? {

57 var urlString = url.trimmingCharacters(in: .whitespacesAndNewlines)

58 if urlString.isEmpty { return nil }

59− if !urlString.hasPrefix("http://") && !urlString.hasPrefix("https://") {

59+ if let schemeRange = Self.urlSchemeRange(in: urlString) {

60+ // Normalize the (case-insensitive) scheme so `HTTP://host` behaves

61+ // exactly like `http://host` everywhere downstream.

62+ urlString.replaceSubrange(schemeRange, with: urlString[schemeRange].lowercased())

63+ } else {

60 urlString = "https://\(urlString)"

64 urlString = "https://\(urlString)"

61 }

65 }

62 while urlString.hasSuffix("/") {

66 while urlString.hasSuffix("/") {

63 urlString.removeLast()

67 urlString.removeLast()

64 }

68 }

65− guard let parsed = URL(string: urlString), parsed.host != nil else {

69+ guard let parsed = URL(string: urlString), parsed.host != nil,

70+ parsed.scheme == "http" || parsed.scheme == "https" else {

66 return nil

71 return nil

67 }

72 }

68 return parsed

73 return parsed

69 }

74 }

75+

76+ /// Whether `string` already starts with a URL scheme (`http://`, `HTTPS://`,

77+ /// `ftp://`, …). Schemes are case-insensitive, so a case-sensitive

78+ /// `hasPrefix("http://")` check turned `HTTP://host` into

79+ /// `https://HTTP://host` — a URL whose host is "HTTP".

80+ static func hasURLScheme(_ string: String) -> Bool {

81+ urlSchemeRange(in: string) != nil

82+ }

83+

84+ private static func urlSchemeRange(in string: String) -> Range<String.Index>? {

85+ string.range(of: "^[A-Za-z][A-Za-z0-9+.-]*://", options: .regularExpression)

86+ }

70}

87}

ParseableViewer/Views/ConnectionSheet.swift+27−7

@@ -18,6 +18,10 @@ struct ConnectionSheet: View {

18 /// retry (after a failed connect) updates it in place rather than appending a

19 /// duplicate.

20 @State private var createdConnection: ServerConnection?

21+ /// Bumped on every field edit and every test run, so an in-flight Test

22+ /// Connection whose fields have since changed doesn't post its (now stale)

23+ /// success/failure under values that were never tested.

24+ @State private var testGeneration = 0

22 enum TestResult {

26 enum TestResult {

23 case success(String)

27 case success(String)

@@ -43,7 +47,7 @@ struct ConnectionSheet: View {

43 TextField("Name:", text: $name)

47 TextField("Name:", text: $name)

44 .textFieldStyle(.roundedBorder)

48 .textFieldStyle(.roundedBorder)

45 .accessibilityLabel("Connection name")

49 .accessibilityLabel("Connection name")

46− .onChange(of: name) { _, _ in testResult = nil }

50+ .onChange(of: name) { _, _ in invalidateTestResult() }

48 TextField("Server URL:", text: $url)

52 TextField("Server URL:", text: $url)

49 .textFieldStyle(.roundedBorder)

53 .textFieldStyle(.roundedBorder)

@@ -51,7 +55,7 @@ struct ConnectionSheet: View {

51 .onChange(of: url) { _, newValue in

55 .onChange(of: url) { _, newValue in

52 urlValidationError = Self.validateURL(newValue)

56 urlValidationError = Self.validateURL(newValue)

53 // A prior test result no longer reflects the edited fields.

57 // A prior test result no longer reflects the edited fields.

54− testResult = nil

58+ invalidateTestResult()

55 }

59 }

57 if let urlError = urlValidationError, !url.isEmpty {

61 if let urlError = urlValidationError, !url.isEmpty {

@@ -63,12 +67,12 @@ struct ConnectionSheet: View {

63 TextField("Username:", text: $username)

67 TextField("Username:", text: $username)

64 .textFieldStyle(.roundedBorder)

68 .textFieldStyle(.roundedBorder)

65 .accessibilityLabel("Username")

69 .accessibilityLabel("Username")

66− .onChange(of: username) { _, _ in testResult = nil }

70+ .onChange(of: username) { _, _ in invalidateTestResult() }

68 SecureField("Password:", text: $password)

72 SecureField("Password:", text: $password)

69 .textFieldStyle(.roundedBorder)

73 .textFieldStyle(.roundedBorder)

70 .accessibilityLabel("Password")

74 .accessibilityLabel("Password")

71− .onChange(of: password) { _, _ in testResult = nil }

75+ .onChange(of: password) { _, _ in invalidateTestResult() }

72 }

76 }

73 .formStyle(.grouped)

77 .formStyle(.grouped)

74 .padding()

78 .padding()

@@ -165,9 +169,16 @@ struct ConnectionSheet: View {

165 }

169 }

166 }

170 }

167

171

172+ private func invalidateTestResult() {

173+ testGeneration += 1

174+ testResult = nil

175+ }

176+

168 private func testConnection() {

177 private func testConnection() {

169 isTesting = true

178 isTesting = true

170 testResult = nil

179 testResult = nil

180+ testGeneration += 1

181+ let generation = testGeneration

171

182

172 Task {

183 Task {

173 do {

184 do {

@@ -179,14 +190,19 @@ struct ConnectionSheet: View {

179 )

190 )

180 let client = try ParseableClient(connection: conn)

191 let client = try ParseableClient(connection: conn)

181 try await client.checkHealth()

192 try await client.checkHealth()

193+ // The health endpoint doesn't check credentials; see

194+ // AppState.verifyCredentials.

195+ try await AppState.verifyCredentials(with: client)

182 await MainActor.run {

196 await MainActor.run {

183− testResult = .success("Connection successful")

184 isTesting = false

197 isTesting = false

198+ guard generation == testGeneration else { return }

199+ testResult = .success("Connection successful")

185 }

200 }

186 } catch {

201 } catch {

187 await MainActor.run {

202 await MainActor.run {

188− testResult = .failure(ParseableError.userFriendlyMessage(for: error))

189 isTesting = false

203 isTesting = false

204+ guard generation == testGeneration else { return }

205+ testResult = .failure(ParseableError.userFriendlyMessage(for: error))

190 }

206 }

191 }

207 }

192 }

208 }

@@ -221,6 +237,10 @@ struct ConnectionSheet: View {

221 dismiss()

237 dismiss()

222 } else {

238 } else {

223 testResult = .failure(appState.errorMessage ?? "Connection failed")

239 testResult = .failure(appState.errorMessage ?? "Connection failed")

240+ // The sheet reports the failure inline; drop the app-wide alert

241+ // performConnect also raised, which would otherwise surface

242+ // behind/after the sheet (or twice, from the Settings window).

243+ appState.showError = false

224 }

244 }

225 }

245 }

226 }

246 }

@@ -231,7 +251,7 @@ struct ConnectionSheet: View {

231 guard !trimmed.isEmpty else { return nil }

251 guard !trimmed.isEmpty else { return nil }

232

252

233 var normalized = trimmed

253 var normalized = trimmed

234− if !normalized.hasPrefix("http://") && !normalized.hasPrefix("https://") {

254+ if !ServerConnection.hasURLScheme(normalized) {

235 normalized = "https://\(normalized)"

255 normalized = "https://\(normalized)"

236 }

256 }

237

257

ParseableViewer/Views/SidebarView.swift+19−1

@@ -6,6 +6,14 @@ struct SidebarView: View {

6 @State private var newStreamName = ""

7 @State private var showDeleteConfirm = false

8 @State private var streamToDelete: String?

9+ /// The connection `streamToDelete` was picked on. Stream names aren't unique

10+ /// across servers, and the delete runs against whatever client is current

11+ /// when the user confirms — so a server switch while the stats lookup was in

12+ /// flight could otherwise delete a same-named stream on the *new* server.

13+ @State private var streamToDeleteConnectionID: UUID?

14+ /// The client the stream was picked with. Editing the active connection's URL

15+ /// keeps its id but installs a new client for a possibly different server.

16+ @State private var streamToDeleteClient: ParseableClient?

9 @State private var deleteStreamDetail: String?

17 @State private var deleteStreamDetail: String?

11 /// Validates a stream name, returning an error message or nil if valid.

19 /// Validates a stream name, returning an error message or nil if valid.

@@ -65,7 +73,10 @@ struct SidebarView: View {

65 // would show the wrong stats on a destructive

73 // would show the wrong stats on a destructive

66 // confirmation.

74 // confirmation.

67 let target = stream.name

75 let target = stream.name

76+ let targetConnectionID = appState.activeConnection?.id

68 streamToDelete = target

77 streamToDelete = target

78+ streamToDeleteConnectionID = targetConnectionID

79+ streamToDeleteClient = appState.client

69 deleteStreamDetail = nil

80 deleteStreamDetail = nil

70 Task {

81 Task {

71 if let client = appState.client,

82 if let client = appState.client,

@@ -81,7 +92,9 @@ struct SidebarView: View {

81 deleteStreamDetail = parts.joined(separator: ", ")

92 deleteStreamDetail = parts.joined(separator: ", ")

82 }

93 }

83 }

94 }

84− if streamToDelete == target {

95+ if streamToDelete == target,

96+ appState.activeConnection?.id == targetConnectionID,

97+ appState.client === streamToDeleteClient {

85 showDeleteConfirm = true

98 showDeleteConfirm = true

86 }

99 }

87 }

100 }

@@ -187,6 +200,11 @@ struct SidebarView: View {

187 .alert("Delete Stream", isPresented: $showDeleteConfirm) {

200 .alert("Delete Stream", isPresented: $showDeleteConfirm) {

188 Button("Delete", role: .destructive) {

201 Button("Delete", role: .destructive) {

189 if let name = streamToDelete {

202 if let name = streamToDelete {

203+ guard appState.activeConnection?.id == streamToDeleteConnectionID,

204+ appState.client === streamToDeleteClient else {

205+ appState.showErrorMessage("The server changed since \"\(name)\" was selected, so it was not deleted.")

206+ return

207+ }

190 Task {

208 Task {

191 do {

209 do {

192 try await appState.deleteStream(name: name)

210 try await appState.deleteStream(name: name)

ParseableViewerTests/ServerConnectionTests.swift+10

@@ -77,4 +77,14 @@ final class ServerConnectionTests: XCTestCase {

77 let conn = ServerConnection(name: "test", url: "https://", username: "admin", password: "pass")

78 XCTAssertNil(conn.baseURL)

79 }

80+

81+ func testBaseURLAcceptsUppercaseScheme() {

82+ let conn = ServerConnection(name: "t", url: "HTTP://10.0.0.5:8000", username: "u", password: "p")

83+ XCTAssertEqual(conn.baseURL?.absoluteString, "http://10.0.0.5:8000")

84+ }

85+

86+ func testBaseURLRejectsNonHTTPScheme() {

87+ let conn = ServerConnection(name: "t", url: "ftp://example.com", username: "u", password: "p")

88+ XCTAssertNil(conn.baseURL)

89+ }

80}

90}