Skip to content

fix: Restore archived ciphers through the unarchive endpoint #1565

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mvanhorn wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: Restore archived ciphers through the unarchive endpoint #1565

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 23 additions & 7 deletions ...n/kotlin/com/artemchep/keyguard/provider/bitwarden/sync/v2/bitwarden/ops/CipherSyncOps.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import com.artemchep.keyguard.provider.bitwarden.api.builder.put
import com.artemchep.keyguard.provider.bitwarden.api.builder.renew
import com.artemchep.keyguard.provider.bitwarden.api.builder.restore
import com.artemchep.keyguard.provider.bitwarden.api.builder.trash
import com.artemchep.keyguard.provider.bitwarden.api.builder.unarchive
import com.artemchep.keyguard.provider.bitwarden.api.merge
import com.artemchep.keyguard.provider.bitwarden.crypto.BitwardenCr
import com.artemchep.keyguard.provider.bitwarden.crypto.BitwardenCrCta
Expand All @@ -58,6 +59,7 @@ import com.artemchep.keyguard.provider.bitwarden.entity.request.CipherAttachment
import com.artemchep.keyguard.provider.bitwarden.entity.request.CipherDeleteRequest
import com.artemchep.keyguard.provider.bitwarden.entity.request.CipherRestoreRequest
import com.artemchep.keyguard.provider.bitwarden.entity.request.CipherUpdate
import com.artemchep.keyguard.provider.bitwarden.entity.request.CipherUnarchiveRequest
import com.artemchep.keyguard.provider.bitwarden.entity.request.of
import com.artemchep.keyguard.provider.bitwarden.sync.v2.CipherConflictResolution
import com.artemchep.keyguard.provider.bitwarden.sync.v2.bitwarden.BitwardenSyncDiagnostics
Expand Down Expand Up @@ -91,11 +93,12 @@ import kotlin.time.Instant
* **three-way merge** via [resolveCipherConflict],
* and **bulk server operations** ([BulkRemoteOps]).
*
* **Push flow** for modifications (restore → PUT → trash → GET):
* **Push flow** for modifications (restore → PUT → unarchive → trash → GET):
* 1. If the cipher was trashed, restore it first.
* 2. If data changed, PUT the updated cipher.
* 3. If the cipher should be trashed, trash it.
* 4. Final GET to refresh local state.
* 3. If the cipher should be unarchived, unarchive it.
* 4. If the cipher should be trashed, trash it.
* 5. Final GET to refresh local state.
*
* Intermediate server responses are captured as [partialRemoteLocal]
* so that metadata can be preserved even if a later step fails.
Expand Down Expand Up @@ -335,6 +338,7 @@ class CipherSyncOps(
pushCipherUpdate(
update = update,
local = local,
server = server,
force = force,
updatePartialRemoteLocal = { partialRemoteLocal = it },
)
Expand Down Expand Up @@ -434,6 +438,7 @@ class CipherSyncOps(
private suspend fun pushCipherUpdate(
update: CipherUpdate,
local: BitwardenCipher,
server: CipherEntity?,
force: Boolean,
updatePartialRemoteLocal: (BitwardenCipher) -> Unit,
): CipherEntity =
Expand All @@ -442,6 +447,7 @@ class CipherSyncOps(
pushModifiedCipher(
update = update,
local = local,
server = server,
force = force,
updatePartialRemoteLocal = updatePartialRemoteLocal,
)
Expand All @@ -456,6 +462,7 @@ class CipherSyncOps(
private suspend fun pushModifiedCipher(
update: CipherUpdate.Modify,
local: BitwardenCipher,
server: CipherEntity?,
force: Boolean,
updatePartialRemoteLocal: (BitwardenCipher) -> Unit,
): CipherEntity {
Expand All @@ -477,8 +484,9 @@ class CipherSyncOps(

val isTrashed = update.source.deletedDate != null
val wasTrashed = update.source.service.remote?.deletedDate != null
val shouldUnarchive = server?.archivedDate != null && update.source.archivedDate == null
val hasChanged = update.hasChanged(force)
if (isTrashed == wasTrashed && !hasChanged) {
if (isTrashed == wasTrashed && !shouldUnarchive && !hasChanged) {
return getCipher(cipherApi)
}

Expand All @@ -505,10 +513,18 @@ class CipherSyncOps(
null
}

if (putCipher != null && (shouldUnarchive || isTrashed)) {
handleIntermediateResponse(putCipher)
}
if (shouldUnarchive) {
ciphersApi.unarchive(
httpClient = httpClient,
env = env,
token = token,
body = CipherUnarchiveRequest(ids = listOf(update.cipherId)),
)
}
if (isTrashed) {
if (putCipher != null) {
handleIntermediateResponse(putCipher)
}
trashCipher(cipherApi)
}
return getCipher(cipherApi)
Expand Down
195 changes: 195 additions & 0 deletions ...in/com/artemchep/keyguard/provider/bitwarden/sync/v2/SyncV2CipherUploadIntegrationTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import com.artemchep.keyguard.provider.bitwarden.entity.AttachmentEntity
import com.artemchep.keyguard.provider.bitwarden.entity.CipherEntity
import com.artemchep.keyguard.provider.bitwarden.entity.request.CipherAttachmentCreateRequest
import com.artemchep.keyguard.provider.bitwarden.entity.request.CipherRequest
import com.artemchep.keyguard.provider.bitwarden.entity.request.CipherUnarchiveRequest
import com.artemchep.keyguard.provider.bitwarden.sync.v2.bitwarden.SyncByBitwardenTokenV2Impl
import com.artemchep.keyguard.provider.bitwarden.sync.v2.core.EntityTypeOutcome
import com.artemchep.keyguard.provider.bitwarden.sync.v2.bitwarden.ops.CipherSyncOps
Expand Down Expand Up @@ -2458,6 +2459,200 @@ class SyncV2CipherUploadIntegrationTest {
assertEquals(emptyList(), fixture.database.cipherQueries.getByAccountId(ACCOUNT_ID).executeAsList())
}

@Test
fun `production CipherSyncOps restores archived cipher through unarchive endpoint`() = runTest {
val server = UploadTestServer()
val fixture = createProductionCipherOpsFixture(server)
val remote =
testCipher(
localId = "cipher-local-1",
remoteId = "cipher-remote-1",
localRevisionDate = T0,
remoteRevisionDate = T0,
attachments = emptyList(),
).copy(
keyBase64 = fixture.cipherKeyBase64(),
archivedDate = T0,
)
val local = remote.copy(archivedDate = null)
server.seedCipher(
remote.toEncryptedCipherEntity(
crypto = fixture.crypto,
base64Service = fixture.base64Service,
),
)

val outcome = assertIs<RemoteWriteOutcome.Upsert<BitwardenCipher>>(
fixture.ops.pushToServer(
local = local,
server = server.ciphers.getValue("cipher-remote-1"),
force = false,
),
)

assertNull(outcome.local.archivedDate)
val unarchiveRequest = server.requests.first { it.path == "/api/ciphers/unarchive" }
assertEquals(
listOf("cipher-remote-1"),
UploadTestServer.json.decodeFromString<CipherUnarchiveRequest>(unarchiveRequest.body).ids,
)
assertEquals(
listOf(
HttpMethod.Put to "/api/ciphers/unarchive",
HttpMethod.Get to "/api/ciphers/cipher-remote-1",
),
server.requests.map { it.method to it.path },
)
}

@Test
fun `production CipherSyncOps puts changed cipher before unarchiving`() = runTest {
val server = UploadTestServer()
server.cipherPutAppliesRequestBody = true
val fixture = createProductionCipherOpsFixture(server)
val remote =
testCipher(
localId = "cipher-local-1",
remoteId = "cipher-remote-1",
localRevisionDate = T0,
remoteRevisionDate = T0,
attachments = emptyList(),
).copy(
keyBase64 = fixture.cipherKeyBase64(),
name = "Archived Cipher",
archivedDate = T0,
)
val local = remote.copy(
name = "Restored Cipher",
revisionDate = T2,
archivedDate = null,
)
server.seedCipher(
remote.toEncryptedCipherEntity(
crypto = fixture.crypto,
base64Service = fixture.base64Service,
),
)

val outcome = assertIs<RemoteWriteOutcome.Upsert<BitwardenCipher>>(
fixture.ops.pushToServer(
local = local,
server = server.ciphers.getValue("cipher-remote-1"),
force = false,
),
)

assertEquals("Restored Cipher", outcome.local.name)
assertNull(outcome.local.archivedDate)
val unarchiveRequest = server.requests.first { it.path == "/api/ciphers/unarchive" }
assertEquals(
listOf("cipher-remote-1"),
UploadTestServer.json.decodeFromString<CipherUnarchiveRequest>(unarchiveRequest.body).ids,
)
assertEquals(
listOf(
HttpMethod.Put to "/api/ciphers/cipher-remote-1",
HttpMethod.Put to "/api/ciphers/unarchive",
HttpMethod.Get to "/api/ciphers/cipher-remote-1",
),
server.requests.map { it.method to it.path },
)
}

@Test
fun `production CipherSyncOps skips unarchive when archive states match`() = runTest {
val server = UploadTestServer()
val fixture = createProductionCipherOpsFixture(server)
val local =
testCipher(
localId = "cipher-local-1",
remoteId = "cipher-remote-1",
localRevisionDate = T0,
remoteRevisionDate = T0,
attachments = emptyList(),
).copy(
keyBase64 = fixture.cipherKeyBase64(),
archivedDate = T0,
)
server.seedCipher(
local.toEncryptedCipherEntity(
crypto = fixture.crypto,
base64Service = fixture.base64Service,
),
)

val outcome = assertIs<RemoteWriteOutcome.Upsert<BitwardenCipher>>(
fixture.ops.pushToServer(
local = local,
server = server.ciphers.getValue("cipher-remote-1"),
force = false,
),
)

assertEquals(T0, outcome.local.archivedDate)
assertEquals(
listOf(HttpMethod.Get to "/api/ciphers/cipher-remote-1"),
server.requests.map { it.method to it.path },
)
}

@Test
fun `production CipherSyncOps keeps put partial when unarchive fails`() = runTest {
val server = UploadTestServer()
val fixture = createProductionCipherOpsFixture(server)
val remote =
testCipher(
localId = "cipher-local-1",
remoteId = "cipher-remote-1",
localRevisionDate = T0,
remoteRevisionDate = T0,
attachments = emptyList(),
).copy(
keyBase64 = fixture.cipherKeyBase64(),
name = "Archived Cipher",
archivedDate = T0,
)
val local = remote.copy(
name = "Restored Cipher",
revisionDate = T2,
archivedDate = null,
)
server.seedCipher(
remote.toEncryptedCipherEntity(
crypto = fixture.crypto,
base64Service = fixture.base64Service,
),
)
server.nextCipherUnarchiveFailure = HttpStatusCode.InternalServerError

val error = assertCipherFailure {
fixture.ops.pushToServer(
local = local,
server = server.ciphers.getValue("cipher-remote-1"),
force = false,
)
}

val partial = assertIs<BitwardenCipher>(error.partialRemoteLocal)
assertEquals(T4, partial.service.remote?.revisionDate)
assertEquals(T0, partial.archivedDate)
assertEquals(T0, server.ciphers.getValue("cipher-remote-1").archivedDate)
val httpException = assertIs<HttpException>(error.cause)
assertEquals(HttpStatusCode.InternalServerError, httpException.statusCode)
val unarchiveRequest = server.requests.first { it.path == "/api/ciphers/unarchive" }
assertEquals(
listOf("cipher-remote-1"),
UploadTestServer.json.decodeFromString<CipherUnarchiveRequest>(unarchiveRequest.body).ids,
)
assertEquals(
listOf(
HttpMethod.Put to "/api/ciphers/cipher-remote-1",
HttpMethod.Put to "/api/ciphers/unarchive",
),
server.requests.map { it.method to it.path },
)
}

@Test
fun `production CipherSyncOps restores legacy trashed cipher before uploading generated item key`() = runTest {
val server = UploadTestServer()
Expand Down
24 changes: 24 additions & 0 deletions ...Test/kotlin/com/artemchep/keyguard/provider/bitwarden/sync/v2/SyncV2UploadTestFixtures.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ import com.artemchep.keyguard.provider.bitwarden.crypto.transform
import com.artemchep.keyguard.provider.bitwarden.entity.AttachmentEntity
import com.artemchep.keyguard.provider.bitwarden.entity.CipherAttachmentUploadEntity
import com.artemchep.keyguard.provider.bitwarden.entity.CipherEntity
import com.artemchep.keyguard.provider.bitwarden.entity.CipherListEntity
import com.artemchep.keyguard.provider.bitwarden.entity.CipherTypeEntity
import com.artemchep.keyguard.provider.bitwarden.entity.CollectionEntity
import com.artemchep.keyguard.provider.bitwarden.entity.ConnectTokenResponse
Expand All @@ -108,6 +109,7 @@ import com.artemchep.keyguard.provider.bitwarden.entity.request.CipherAttachment
import com.artemchep.keyguard.provider.bitwarden.entity.request.CipherCreateRequest
import com.artemchep.keyguard.provider.bitwarden.entity.request.CipherDeleteRequest
import com.artemchep.keyguard.provider.bitwarden.entity.request.CipherRequest
import com.artemchep.keyguard.provider.bitwarden.entity.request.CipherUnarchiveRequest
import com.artemchep.keyguard.provider.bitwarden.entity.request.SendRequest
import com.artemchep.keyguard.provider.bitwarden.upload.FailingPendingUploadCoordinator
import com.artemchep.keyguard.provider.bitwarden.upload.PendingUploadCoordinator
Expand Down Expand Up @@ -169,6 +171,7 @@ internal class UploadTestServer {
var nextCipherGetFailure: HttpStatusCode? = null
var cipherGetFailureAfterSuccessfulGets: Int? = null
var nextCipherPutFailure: HttpStatusCode? = null
var nextCipherUnarchiveFailure: HttpStatusCode? = null
var nextCipherTrashFailure: HttpStatusCode? = null
var nextCipherBulkDeleteFailure: HttpStatusCode? = null
var nextCipherTrashException: Throwable? = null
Expand Down Expand Up @@ -296,6 +299,27 @@ internal class UploadTestServer {
respondText("")
}

request.method == HttpMethod.Put &&
request.path == "/api/ciphers/unarchive" -> {
val failure = nextCipherUnarchiveFailure
if (failure != null) {
nextCipherUnarchiveFailure = null
respondApiError(
status = failure,
description = "cipher unarchive failed",
)
} else {
val body = json.decodeFromString<CipherUnarchiveRequest>(request.body)
val unarchived = body.ids.map { cipherId ->
requireNotNull(ciphers[cipherId]).copy(
revisionDate = T4,
archivedDate = null,
).also { ciphers[cipherId] = it }
}
respondJson(CipherListEntity(data = unarchived))
}
}

request.method == HttpMethod.Delete &&
request.path == "/api/ciphers/" -> {
val failure = nextCipherBulkDeleteFailure
Expand Down
Loading