From 8648f40c07f43930fffeeaf2378f5fc6e900e309 Mon Sep 17 00:00:00 2001 From: Stefan Hausotte Date: Tue, 2 Jun 2026 18:20:12 +0200 Subject: [PATCH] fix: guard that prevents edit of accounts to add the same account --- Forji/Forji/Views/InstanceFormView.swift | 65 ++++++++++++++++++-- Forji/ForjiTests/InstanceFormViewTests.swift | 62 +++++++++++++++++++ 2 files changed, 121 insertions(+), 6 deletions(-) create mode 100644 Forji/ForjiTests/InstanceFormViewTests.swift diff --git a/Forji/Forji/Views/InstanceFormView.swift b/Forji/Forji/Views/InstanceFormView.swift index 1fdcebc..8a72c9e 100644 --- a/Forji/Forji/Views/InstanceFormView.swift +++ b/Forji/Forji/Views/InstanceFormView.swift @@ -263,15 +263,24 @@ struct InstanceFormView: View { case .add: try await performLogin() + // Resolve the account identity after login: token auth learns the + // real username from the server, so it isn't known before this point. + let normalizedURL = ForgejoClient.normalizeServerURL(serverURL) + let resolvedUsername = + authMode == .token ? (authService.currentUser?.login ?? username) : username + + // Two accounts sharing a server+username collide on the same sourceKey, + // which used to crash merged overviews. Reject the duplicate up front. + if rejectDuplicateAccount(serverURL: normalizedURL, username: resolvedUsername) { + return + } + if isDefault { for inst in instances where inst.isDefault { inst.isDefault = false } } - let normalizedURL = ForgejoClient.normalizeServerURL(serverURL) - let resolvedUsername = - authMode == .token ? (authService.currentUser?.login ?? username) : username let instance = ForgejoInstance( serverURL: normalizedURL, username: resolvedUsername, @@ -289,16 +298,30 @@ struct InstanceFormView: View { authService.currentInstance = instance case let .edit(instance): + // Resolve the would-be identity before mutating the instance: token + // auth may change the username, and credentials auth may too. + var resolvedUsername = instance.username if authMode == .token, !apiToken.isEmpty { try await performLogin() - instance.username = authService.currentUser?.login ?? username + resolvedUsername = authService.currentUser?.login ?? username } else if authMode == .credentials, !password.isEmpty { try await performLogin() - instance.username = username + resolvedUsername = username } + let normalizedURL = ForgejoClient.normalizeServerURL(serverURL) + + // Editing must not produce a server+username that already belongs to + // another account, since that collides on the same sourceKey. + if rejectDuplicateAccount( + serverURL: normalizedURL, username: resolvedUsername, excluding: instance, + ) { + return + } + + instance.username = resolvedUsername instance.name = name - instance.serverURL = ForgejoClient.normalizeServerURL(serverURL) + instance.serverURL = normalizedURL instance.allowSelfSignedCertificates = allowSelfSignedCertificates instance.useTokenAuth = authMode == .token @@ -328,6 +351,36 @@ struct InstanceFormView: View { isLoading = false } } + + /// Sets the duplicate-account error and returns true if another stored account + /// already uses this server+username. `excluding` skips the instance being edited. + private func rejectDuplicateAccount( + serverURL normalizedURL: String, + username: String, + excluding: ForgejoInstance? = nil, + ) -> Bool { + guard Self.accountAlreadyExists( + serverURL: normalizedURL, username: username, in: instances, excluding: excluding, + ) else { return false } + errorMessage = "An account for \"\(username)\" on \(normalizedURL) already exists." + showError = true + isLoading = false + return true + } + + /// True if `instances` already contains an account with this server+username, + /// ignoring `excluding` (the instance being edited). Accounts that share a + /// server+username collide on the same sourceKey, which crashes merged overviews. + static func accountAlreadyExists( + serverURL normalizedURL: String, + username: String, + in instances: [ForgejoInstance], + excluding: ForgejoInstance? = nil, + ) -> Bool { + instances.contains { inst in + inst !== excluding && inst.serverURL == normalizedURL && inst.username == username + } + } } #if DEBUG diff --git a/Forji/ForjiTests/InstanceFormViewTests.swift b/Forji/ForjiTests/InstanceFormViewTests.swift new file mode 100644 index 0000000..7b7964f --- /dev/null +++ b/Forji/ForjiTests/InstanceFormViewTests.swift @@ -0,0 +1,62 @@ +import Foundation +import Testing +@testable import Forji + +@MainActor +struct InstanceFormViewTests { + private func makeInstance( + server: String = "https://a.com", + username: String = "user", + name: String = "Account", + ) -> ForgejoInstance { + ForgejoInstance(serverURL: server, username: username, name: name) + } + + // MARK: - accountAlreadyExists + + @Test func detectsDuplicateServerAndUsername() { + let existing = [makeInstance(server: "https://a.com", username: "user", name: "Work")] + #expect(InstanceFormView.accountAlreadyExists( + serverURL: "https://a.com", username: "user", in: existing, + )) + } + + @Test func differentServerIsNotDuplicate() { + let existing = [makeInstance(server: "https://a.com", username: "user")] + #expect(!InstanceFormView.accountAlreadyExists( + serverURL: "https://b.com", username: "user", in: existing, + )) + } + + @Test func differentUsernameIsNotDuplicate() { + let existing = [makeInstance(server: "https://a.com", username: "user")] + #expect(!InstanceFormView.accountAlreadyExists( + serverURL: "https://a.com", username: "other", in: existing, + )) + } + + @Test func emptyListIsNeverDuplicate() { + #expect(!InstanceFormView.accountAlreadyExists( + serverURL: "https://a.com", username: "user", in: [], + )) + } + + // MARK: - excluding (edit path) + + @Test func editingInstanceWithoutChangingIdentityIsAllowed() { + // Editing only the name on an existing account must not flag itself as a duplicate. + let instance = makeInstance(server: "https://a.com", username: "user", name: "Old") + #expect(!InstanceFormView.accountAlreadyExists( + serverURL: "https://a.com", username: "user", in: [instance], excluding: instance, + )) + } + + @Test func editingIntoAnotherAccountsIdentityIsRejected() { + // Two distinct accounts; editing the second to match the first must be caught. + let first = makeInstance(server: "https://a.com", username: "user", name: "First") + let second = makeInstance(server: "https://b.com", username: "user", name: "Second") + #expect(InstanceFormView.accountAlreadyExists( + serverURL: "https://a.com", username: "user", in: [first, second], excluding: second, + )) + } +}