diff --git a/pass/de.lproj/Localizable.strings b/pass/de.lproj/Localizable.strings index 8177a35..355b200 100644 --- a/pass/de.lproj/Localizable.strings +++ b/pass/de.lproj/Localizable.strings @@ -72,6 +72,7 @@ "KeyImportError." = "Schlüssel kann nicht importiert werden."; "FileNotFoundError." = "Die Datei '%@' kann nicht gelesen werden."; "PasswordDuplicatedError." = "Passwort kann nicht hinzugefügt werden; es existiert bereits."; +"CannotDeleteNonEmptyDirectoryError." = "Ordner muss erst leer sein um gelöscht werden zu können."; "GitResetError." = "Der zuletzt synchronisierte Commit kann nicht identifiziert werden."; "GitCreateSignatureError." = "Es konnte keine valide Signatur für den Author/Committer angelegt werden."; "GitPushNotSuccessfulError." = "Die Übertragung der lokalen Änderungen war nicht erfolgreich. Stelle bitte sicher, dass auf dem Remote-Repository alle Änderungen commitet sind."; diff --git a/pass/en.lproj/Localizable.strings b/pass/en.lproj/Localizable.strings index 10d8b30..a4f74e3 100644 --- a/pass/en.lproj/Localizable.strings +++ b/pass/en.lproj/Localizable.strings @@ -73,7 +73,7 @@ "KeyImportError." = "Cannot import the key."; "FileNotFoundError." = "File '%@' cannot be read."; "PasswordDuplicatedError." = "Cannot add the password; password is duplicated."; -"CannotDeleteDirectoryError." = "Cannot delete directories; delete passwords instead."; +"CannotDeleteNonEmptyDirectoryError." = "Delete passwords from the directory before deleting the directory itself."; "GitResetError." = "Cannot identify the latest synced commit."; "GitCreateSignatureError." = "Cannot create a valid author/committer signature."; "GitPushNotSuccessfulError." = "Pushing local changes was not successful. Make sure there are no uncommitted changes on the remote repository."; diff --git a/passKit/Helpers/AppError.swift b/passKit/Helpers/AppError.swift index 8bcc84b..28ceb73 100644 --- a/passKit/Helpers/AppError.swift +++ b/passKit/Helpers/AppError.swift @@ -15,7 +15,7 @@ public enum AppError: Error, Equatable { case keyImport case readingFile(fileName: String) case passwordDuplicated - case cannotDeleteDirectory + case cannotDeleteNonEmptyDirectory case gitReset case gitCommit case gitCreateSignature diff --git a/passKit/Models/PasswordStore.swift b/passKit/Models/PasswordStore.swift index 78c92c4..b918ab4 100644 --- a/passKit/Models/PasswordStore.swift +++ b/passKit/Models/PasswordStore.swift @@ -273,13 +273,15 @@ public class PasswordStore { } public func delete(passwordEntity: PasswordEntity) throws { - if passwordEntity.isDir { - throw AppError.cannotDeleteDirectory + if !passwordEntity.children.isEmpty { + throw AppError.cannotDeleteNonEmptyDirectory } let deletedFileURL = passwordEntity.fileURL(in: storeURL) let deletedFilePath = passwordEntity.path - try gitRm(path: passwordEntity.path) + if !passwordEntity.isDir { + try gitRm(path: passwordEntity.path) + } try deletePasswordEntities(passwordEntity: passwordEntity) try deleteDirectoryTree(at: deletedFileURL) try gitCommit(message: "RemovePassword.".localize(deletedFilePath)) @@ -287,6 +289,11 @@ public class PasswordStore { } public func edit(passwordEntity: PasswordEntity, password: Password, keyID: String? = nil) throws -> PasswordEntity? { + guard !passwordEntity.isDir else { + // caller should ensure this, so this is not a user-facing error + throw AppError.other(message: "Cannot edit a directory") + } + var newPasswordEntity: PasswordEntity? = passwordEntity let url = passwordEntity.fileURL(in: storeURL) diff --git a/passKitTests/Fixtures/password-store-empty-dirs.git/HEAD b/passKitTests/Fixtures/password-store-empty-dirs.git/HEAD new file mode 100644 index 0000000..b870d82 --- /dev/null +++ b/passKitTests/Fixtures/password-store-empty-dirs.git/HEAD @@ -0,0 +1 @@ +ref: refs/heads/main diff --git a/passKitTests/Fixtures/password-store-empty-dirs.git/config b/passKitTests/Fixtures/password-store-empty-dirs.git/config new file mode 100644 index 0000000..e6da231 --- /dev/null +++ b/passKitTests/Fixtures/password-store-empty-dirs.git/config @@ -0,0 +1,6 @@ +[core] + repositoryformatversion = 0 + filemode = true + bare = true + ignorecase = true + precomposeunicode = true diff --git a/passKitTests/Fixtures/password-store-empty-dirs.git/description b/passKitTests/Fixtures/password-store-empty-dirs.git/description new file mode 100644 index 0000000..498b267 --- /dev/null +++ b/passKitTests/Fixtures/password-store-empty-dirs.git/description @@ -0,0 +1 @@ +Unnamed repository; edit this file 'description' to name the repository. diff --git a/passKitTests/Fixtures/password-store-empty-dirs.git/info/exclude b/passKitTests/Fixtures/password-store-empty-dirs.git/info/exclude new file mode 100644 index 0000000..a5196d1 --- /dev/null +++ b/passKitTests/Fixtures/password-store-empty-dirs.git/info/exclude @@ -0,0 +1,6 @@ +# git ls-files --others --exclude-from=.git/info/exclude +# Lines that start with '#' are comments. +# For a project mostly in C, the following would be a good set of +# exclude patterns (uncomment them if you want to use them): +# *.[oa] +# *~ diff --git a/passKitTests/Fixtures/password-store-empty-dirs.git/objects/44/73d8218dcffe837e18d56d74c240e565461aea b/passKitTests/Fixtures/password-store-empty-dirs.git/objects/44/73d8218dcffe837e18d56d74c240e565461aea new file mode 100644 index 0000000..6fa149a Binary files /dev/null and b/passKitTests/Fixtures/password-store-empty-dirs.git/objects/44/73d8218dcffe837e18d56d74c240e565461aea differ diff --git a/passKitTests/Fixtures/password-store-empty-dirs.git/objects/4c/f9f177c4c015836fca6a31f9c3917e89ae29ec b/passKitTests/Fixtures/password-store-empty-dirs.git/objects/4c/f9f177c4c015836fca6a31f9c3917e89ae29ec new file mode 100644 index 0000000..1c17f20 Binary files /dev/null and b/passKitTests/Fixtures/password-store-empty-dirs.git/objects/4c/f9f177c4c015836fca6a31f9c3917e89ae29ec differ diff --git a/passKitTests/Fixtures/password-store-empty-dirs.git/objects/50/96ac11d1376ea9b22ddedac1130f45ec618d11 b/passKitTests/Fixtures/password-store-empty-dirs.git/objects/50/96ac11d1376ea9b22ddedac1130f45ec618d11 new file mode 100644 index 0000000..5f9c0af Binary files /dev/null and b/passKitTests/Fixtures/password-store-empty-dirs.git/objects/50/96ac11d1376ea9b22ddedac1130f45ec618d11 differ diff --git a/passKitTests/Fixtures/password-store-empty-dirs.git/objects/ce/013625030ba8dba906f756967f9e9ca394464a b/passKitTests/Fixtures/password-store-empty-dirs.git/objects/ce/013625030ba8dba906f756967f9e9ca394464a new file mode 100644 index 0000000..6802d49 Binary files /dev/null and b/passKitTests/Fixtures/password-store-empty-dirs.git/objects/ce/013625030ba8dba906f756967f9e9ca394464a differ diff --git a/passKitTests/Fixtures/password-store-empty-dirs.git/objects/d5/64d0bc3dd917926892c55e3706cc116d5b165e b/passKitTests/Fixtures/password-store-empty-dirs.git/objects/d5/64d0bc3dd917926892c55e3706cc116d5b165e new file mode 100644 index 0000000..24279dc Binary files /dev/null and b/passKitTests/Fixtures/password-store-empty-dirs.git/objects/d5/64d0bc3dd917926892c55e3706cc116d5b165e differ diff --git a/passKitTests/Fixtures/password-store-empty-dirs.git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 b/passKitTests/Fixtures/password-store-empty-dirs.git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 new file mode 100644 index 0000000..7112238 Binary files /dev/null and b/passKitTests/Fixtures/password-store-empty-dirs.git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 differ diff --git a/passKitTests/Fixtures/password-store-empty-dirs.git/objects/fb/cbb5819e1c864ef33cfffa179a71387a5d90d0 b/passKitTests/Fixtures/password-store-empty-dirs.git/objects/fb/cbb5819e1c864ef33cfffa179a71387a5d90d0 new file mode 100644 index 0000000..7052504 Binary files /dev/null and b/passKitTests/Fixtures/password-store-empty-dirs.git/objects/fb/cbb5819e1c864ef33cfffa179a71387a5d90d0 differ diff --git a/passKitTests/Fixtures/password-store-empty-dirs.git/packed-refs b/passKitTests/Fixtures/password-store-empty-dirs.git/packed-refs new file mode 100644 index 0000000..025abc4 --- /dev/null +++ b/passKitTests/Fixtures/password-store-empty-dirs.git/packed-refs @@ -0,0 +1,2 @@ +# pack-refs with: peeled fully-peeled sorted +fbcbb5819e1c864ef33cfffa179a71387a5d90d0 refs/heads/main diff --git a/passKitTests/Models/PasswordStoreTest.swift b/passKitTests/Models/PasswordStoreTest.swift index de3f7b3..4fe2c80 100644 --- a/passKitTests/Models/PasswordStoreTest.swift +++ b/passKitTests/Models/PasswordStoreTest.swift @@ -176,7 +176,40 @@ final class PasswordStoreTest: XCTestCase { waitForExpectations(timeout: 1, handler: nil) } - func testDeleteDirectoryFails() throws { + func testDeletePasswordKeepsFileSystemFolderIfNotEmpty() throws { + try cloneRepository(.withGPGID) + + // /work contains .gpg-id in addition to a password file + let entity = passwordStore.fetchPasswordEntity(with: "work/github.com.gpg") + try passwordStore.delete(passwordEntity: entity!) + + XCTAssertFalse(FileManager.default.fileExists(atPath: localRepoURL.appendingPathComponent("work/github.com.gpg").path)) + XCTAssertNil(passwordStore.fetchPasswordEntity(with: "work/github.com.gpg")) + XCTAssertNil(passwordStore.fetchPasswordEntity(with: "work")) + XCTAssertTrue(FileManager.default.fileExists(atPath: localRepoURL.appendingPathComponent("work/.gpg-id").path)) + } + + func testDeleteEmptyDirectory() throws { + try cloneRepository(.emptyDirs) + let numCommitsBefore = passwordStore.numberOfCommits! + let numLocalCommitsBefore = passwordStore.numberOfLocalCommits + + expectation(forNotification: .passwordStoreUpdated, object: nil) + + // Note: the directory isn't truely empty since Git doesn't track empty directories, + // but it should be treated as empty by the app since it contains only hidden files + let entityToDelete = passwordStore.fetchPasswordEntity(with: "empty-dir") + XCTAssertNotNil(entityToDelete) + try passwordStore.delete(passwordEntity: entityToDelete!) + + XCTAssertNil(passwordStore.fetchPasswordEntity(with: "empty-dir")) + XCTAssertTrue(FileManager.default.fileExists(atPath: localRepoURL.appendingPathComponent("empty-dir/.gitkeep").path)) + XCTAssertEqual(passwordStore.numberOfCommits!, numCommitsBefore + 1) + XCTAssertEqual(passwordStore.numberOfLocalCommits, numLocalCommitsBefore + 1) + waitForExpectations(timeout: 1, handler: nil) + } + + func testDeleteNonEmptyDirectoryFails() throws { try cloneRepository(.withGPGID) let numCommitsBefore = passwordStore.numberOfCommits! let numLocalCommitsBefore = passwordStore.numberOfLocalCommits @@ -186,7 +219,7 @@ final class PasswordStoreTest: XCTestCase { let entity = passwordStore.fetchPasswordEntity(with: "personal") XCTAssertThrowsError(try passwordStore.delete(passwordEntity: entity!)) { error in XCTAssertTrue(error is AppError, "Unexpected error type: \(type(of: error))") - XCTAssertEqual(error as? AppError, .cannotDeleteDirectory) + XCTAssertEqual(error as? AppError, .cannotDeleteNonEmptyDirectory) } XCTAssertNotNil(passwordStore.fetchPasswordEntity(with: "personal/github.com.gpg")) @@ -240,6 +273,23 @@ final class PasswordStoreTest: XCTestCase { waitForExpectations(timeout: 1, handler: nil) } + func testEditDirectoryFails() throws { + try cloneRepository(.withGPGID) + try importSinglePGPKey() + let numCommitsBefore = passwordStore.numberOfCommits! + + let directoryEntity = passwordStore.fetchPasswordEntity(with: "personal")! + let editedPassword = Password(name: "new name", path: "new name", plainText: "") + editedPassword.changed = PasswordChange.path.rawValue + XCTAssertThrowsError(try passwordStore.edit(passwordEntity: directoryEntity, password: editedPassword)) { error in + XCTAssertTrue(error is AppError, "Unexpected error type: \(type(of: error))") + XCTAssertEqual(error as? AppError, .other(message: "Cannot edit a directory")) + } + + XCTAssertNotNil(passwordStore.fetchPasswordEntity(with: "personal")) + XCTAssertEqual(passwordStore.numberOfCommits!, numCommitsBefore) + } + func testReset() throws { try cloneRepository(.withGPGID) try importSinglePGPKey() @@ -292,12 +342,15 @@ final class PasswordStoreTest: XCTestCase { private enum RemoteRepo { case empty + case emptyDirs case withGPGID var url: URL { switch self { case .empty: Bundle(for: PasswordStoreTest.self).resourceURL!.appendingPathComponent("Fixtures/password-store-empty.git") + case .emptyDirs: + Bundle(for: PasswordStoreTest.self).resourceURL!.appendingPathComponent("Fixtures/password-store-empty-dirs.git") case .withGPGID: Bundle(for: PasswordStoreTest.self).resourceURL!.appendingPathComponent("Fixtures/password-store-with-gpgid.git") } @@ -307,6 +360,8 @@ final class PasswordStoreTest: XCTestCase { switch self { case .empty: "main" + case .emptyDirs: + "main" case .withGPGID: "master" }