From e15f8fbc6c0251b82a7c4ec9af8fa5a5a9fcfdbf Mon Sep 17 00:00:00 2001 From: Jens Mueller Date: Wed, 9 Sep 2020 20:27:46 +0200 Subject: [PATCH 1/6] DocumentsStorageProvider: prevent opening stale file contents fix #6883 Signed-off-by: Jens Mueller --- .../providers/DocumentsStorageProvider.java | 47 ++++++++++--------- 1 file changed, 26 insertions(+), 21 deletions(-) diff --git a/src/main/java/com/owncloud/android/providers/DocumentsStorageProvider.java b/src/main/java/com/owncloud/android/providers/DocumentsStorageProvider.java index 3ed6d8ab20..20d885d3ec 100644 --- a/src/main/java/com/owncloud/android/providers/DocumentsStorageProvider.java +++ b/src/main/java/com/owncloud/android/providers/DocumentsStorageProvider.java @@ -64,7 +64,9 @@ import com.owncloud.android.lib.common.OwnCloudClientManagerFactory; import com.owncloud.android.lib.common.accounts.AccountUtils.AccountNotFoundException; import com.owncloud.android.lib.common.operations.RemoteOperationResult; import com.owncloud.android.lib.common.utils.Log_OC; +import com.owncloud.android.lib.resources.files.ReadFileRemoteOperation; import com.owncloud.android.lib.resources.files.UploadFileRemoteOperation; +import com.owncloud.android.lib.resources.files.model.RemoteFile; import com.owncloud.android.operations.CopyFileOperation; import com.owncloud.android.operations.CreateFolderOperation; import com.owncloud.android.operations.DownloadFileOperation; @@ -187,7 +189,7 @@ public class DocumentsStorageProvider extends DocumentsProvider { @SuppressLint("LongLogTag") @Override public ParcelFileDescriptor openDocument(String documentId, String mode, CancellationSignal cancellationSignal) - throws FileNotFoundException { + throws FileNotFoundException { Log.d(TAG, "openDocument(), id=" + documentId); Document document = toDocument(documentId); @@ -196,27 +198,32 @@ public class DocumentsStorageProvider extends DocumentsProvider { OCFile ocFile = document.getFile(); Account account = document.getAccount(); - final User user = accountManager.getUser(account.name).orElseThrow(RuntimeException::new); // should exist - if (ocFile.isDown()) { - RemoteOperationResult result; - try { - result = new SynchronizeFileOperation(ocFile, null, user, true, context) - .execute(document.getClient(), document.getStorageManager()); - } catch (Exception e) { - throw getFileNotFoundExceptionWithCause("Error synchronizing file: " + ocFile.getFileName(), e); - } - if (result.getCode() == RemoteOperationResult.ResultCode.SYNC_CONFLICT) { - // TODO show a conflict notification with a pending intent that shows a ConflictResolveDialog - Log_OC.w(TAG, "Conflict found: " + result); - } else if (!result.isSuccess()) { + int accessMode = ParcelFileDescriptor.parseMode(mode); + + boolean needsDownload = (accessMode != ParcelFileDescriptor.MODE_WRITE_ONLY); + if (needsDownload && ocFile.isDown()) { + RemoteOperationResult result = new ReadFileRemoteOperation(ocFile.getRemotePath()) + .execute(document.getClient()); + if (result.isSuccess()) { + OCFile serverFile = FileStorageUtils.fillOCFile((RemoteFile) result.getData().get(0)); + boolean serverChanged = !serverFile.getEtag().equals(ocFile.getEtag()); + boolean localChanged = ocFile.getLocalModificationTimestamp() > ocFile.getLastSyncDateForData(); + if (localChanged && serverChanged) { + // TODO show a conflict notification with a pending intent that shows a ConflictResolveDialog + Log_OC.w(TAG, "Conflict found: " + result); + // open local version for now + needsDownload = false; + } else { + needsDownload = serverChanged; + } + } else if (result.getCode() == RemoteOperationResult.ResultCode.FILE_NOT_FOUND) { Log_OC.e(TAG, result.toString()); throw new FileNotFoundException("Error synchronizing file: " + ocFile.getFileName()); } - // TODO test if this needed here - // block thread until file is saved - FileStorageUtils.checkIfFileFinishedSaving(ocFile); - } else { + } + + if (needsDownload) { DownloadFileOperation downloadFileOperation = new DownloadFileOperation(account, ocFile, context); RemoteOperationResult result = downloadFileOperation.execute(document.getClient()); if (!result.isSuccess()) { @@ -227,10 +234,8 @@ public class DocumentsStorageProvider extends DocumentsProvider { } File file = new File(ocFile.getStoragePath()); - int accessMode = ParcelFileDescriptor.parseMode(mode); - boolean isWrite = accessMode != ParcelFileDescriptor.MODE_READ_ONLY; - if (isWrite) { + if (accessMode != ParcelFileDescriptor.MODE_READ_ONLY) { // The calling thread is not guaranteed to have a Looper, so we can't block it with the OnCloseListener. // Thus, we are unable to do a synchronous upload and have to start an asynchronous one. Handler handler = new Handler(context.getMainLooper()); From cae0e77267989d5177ede1624c0a217e70888658 Mon Sep 17 00:00:00 2001 From: Jens Mueller Date: Thu, 10 Sep 2020 21:12:03 +0200 Subject: [PATCH 2/6] fix codacy comments Signed-off-by: Jens Mueller --- .../owncloud/android/providers/DocumentsStorageProvider.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/owncloud/android/providers/DocumentsStorageProvider.java b/src/main/java/com/owncloud/android/providers/DocumentsStorageProvider.java index 20d885d3ec..54f1c4256f 100644 --- a/src/main/java/com/owncloud/android/providers/DocumentsStorageProvider.java +++ b/src/main/java/com/owncloud/android/providers/DocumentsStorageProvider.java @@ -201,7 +201,7 @@ public class DocumentsStorageProvider extends DocumentsProvider { int accessMode = ParcelFileDescriptor.parseMode(mode); - boolean needsDownload = (accessMode != ParcelFileDescriptor.MODE_WRITE_ONLY); + boolean needsDownload = accessMode != ParcelFileDescriptor.MODE_WRITE_ONLY; if (needsDownload && ocFile.isDown()) { RemoteOperationResult result = new ReadFileRemoteOperation(ocFile.getRemotePath()) .execute(document.getClient()); From c00e7b3dbb4c75e7183307073d5d2310d84635d1 Mon Sep 17 00:00:00 2001 From: Jens Mueller Date: Fri, 11 Sep 2020 22:21:09 +0200 Subject: [PATCH 3/6] fix review comments Signed-off-by: Jens Mueller --- .../providers/DocumentsStorageProvider.java | 61 +++++++++---------- 1 file changed, 29 insertions(+), 32 deletions(-) diff --git a/src/main/java/com/owncloud/android/providers/DocumentsStorageProvider.java b/src/main/java/com/owncloud/android/providers/DocumentsStorageProvider.java index 54f1c4256f..27917d2d47 100644 --- a/src/main/java/com/owncloud/android/providers/DocumentsStorageProvider.java +++ b/src/main/java/com/owncloud/android/providers/DocumentsStorageProvider.java @@ -44,7 +44,6 @@ import android.provider.DocumentsProvider; import android.util.Log; import android.util.SparseArray; -import com.nextcloud.client.account.User; import com.nextcloud.client.account.UserAccountManager; import com.nextcloud.client.account.UserAccountManagerImpl; import com.nextcloud.client.files.downloader.DownloadTask; @@ -64,9 +63,8 @@ import com.owncloud.android.lib.common.OwnCloudClientManagerFactory; import com.owncloud.android.lib.common.accounts.AccountUtils.AccountNotFoundException; import com.owncloud.android.lib.common.operations.RemoteOperationResult; import com.owncloud.android.lib.common.utils.Log_OC; -import com.owncloud.android.lib.resources.files.ReadFileRemoteOperation; +import com.owncloud.android.lib.resources.files.CheckEtagRemoteOperation; import com.owncloud.android.lib.resources.files.UploadFileRemoteOperation; -import com.owncloud.android.lib.resources.files.model.RemoteFile; import com.owncloud.android.operations.CopyFileOperation; import com.owncloud.android.operations.CreateFolderOperation; import com.owncloud.android.operations.DownloadFileOperation; @@ -74,7 +72,6 @@ import com.owncloud.android.operations.MoveFileOperation; import com.owncloud.android.operations.RefreshFolderOperation; import com.owncloud.android.operations.RemoveFileOperation; import com.owncloud.android.operations.RenameFileOperation; -import com.owncloud.android.operations.SynchronizeFileOperation; import com.owncloud.android.ui.activity.SettingsActivity; import com.owncloud.android.utils.FileStorageUtils; import com.owncloud.android.utils.MimeTypeUtil; @@ -201,36 +198,20 @@ public class DocumentsStorageProvider extends DocumentsProvider { int accessMode = ParcelFileDescriptor.parseMode(mode); - boolean needsDownload = accessMode != ParcelFileDescriptor.MODE_WRITE_ONLY; - if (needsDownload && ocFile.isDown()) { - RemoteOperationResult result = new ReadFileRemoteOperation(ocFile.getRemotePath()) - .execute(document.getClient()); - if (result.isSuccess()) { - OCFile serverFile = FileStorageUtils.fillOCFile((RemoteFile) result.getData().get(0)); - boolean serverChanged = !serverFile.getEtag().equals(ocFile.getEtag()); - boolean localChanged = ocFile.getLocalModificationTimestamp() > ocFile.getLastSyncDateForData(); - if (localChanged && serverChanged) { - // TODO show a conflict notification with a pending intent that shows a ConflictResolveDialog - Log_OC.w(TAG, "Conflict found: " + result); - // open local version for now - needsDownload = false; - } else { - needsDownload = serverChanged; - } - } else if (result.getCode() == RemoteOperationResult.ResultCode.FILE_NOT_FOUND) { - Log_OC.e(TAG, result.toString()); - throw new FileNotFoundException("Error synchronizing file: " + ocFile.getFileName()); - } - } - + boolean needsDownload = (accessMode != ParcelFileDescriptor.MODE_WRITE_ONLY) && (!ocFile.isDown() || hasServerChange(ocFile, account)); if (needsDownload) { - DownloadFileOperation downloadFileOperation = new DownloadFileOperation(account, ocFile, context); - RemoteOperationResult result = downloadFileOperation.execute(document.getClient()); - if (!result.isSuccess()) { - Log_OC.e(TAG, result.toString()); - throw new FileNotFoundException("Error downloading file: " + ocFile.getFileName()); + if (ocFile.getLocalModificationTimestamp() > ocFile.getLastSyncDateForData()) { + // TODO show a conflict notification with a pending intent that shows a ConflictResolveDialog + Log_OC.w(TAG, "Conflict found!"); + } else { + DownloadFileOperation downloadFileOperation = new DownloadFileOperation(account, ocFile, context); + RemoteOperationResult result = downloadFileOperation.execute(document.getClient()); + if (!result.isSuccess()) { + Log_OC.e(TAG, result.toString()); + throw new FileNotFoundException("Error downloading file: " + ocFile.getFileName()); + } + saveDownloadedFile(document.getStorageManager(), downloadFileOperation, ocFile); } - saveDownloadedFile(document.getStorageManager(), downloadFileOperation, ocFile); } File file = new File(ocFile.getStoragePath()); @@ -268,6 +249,22 @@ public class DocumentsStorageProvider extends DocumentsProvider { } } + private boolean hasServerChange(OCFile ocFile, Account account) throws FileNotFoundException { + Context context = getNonNullContext(); + RemoteOperationResult result = new CheckEtagRemoteOperation(ocFile.getRemotePath(), ocFile.getEtag()) + .execute(account, context); + switch (result.getCode()) { + case ETAG_CHANGED: + return true; + case ETAG_UNCHANGED: + return false; + case FILE_NOT_FOUND: + default: + Log_OC.e(TAG, result.toString()); + throw new FileNotFoundException("Error synchronizing file: " + ocFile.getFileName()); + } + } + /** * Updates the OC File after a successful download. * From f9a82f9ed95ba83ff346f35812df54e5fe18608e Mon Sep 17 00:00:00 2001 From: Jens Mueller Date: Sat, 12 Sep 2020 00:09:51 +0200 Subject: [PATCH 4/6] add integration test Signed-off-by: Jens Mueller --- .../providers/DocumentsStorageProviderIT.kt | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/androidTest/java/com/owncloud/android/providers/DocumentsStorageProviderIT.kt b/src/androidTest/java/com/owncloud/android/providers/DocumentsStorageProviderIT.kt index d715a56807..e42af8a130 100644 --- a/src/androidTest/java/com/owncloud/android/providers/DocumentsStorageProviderIT.kt +++ b/src/androidTest/java/com/owncloud/android/providers/DocumentsStorageProviderIT.kt @@ -6,6 +6,7 @@ import androidx.documentfile.provider.DocumentFile import com.owncloud.android.AbstractOnServerIT import com.owncloud.android.R import com.owncloud.android.datamodel.OCFile.ROOT_PATH +import com.owncloud.android.lib.common.network.WebdavUtils import com.owncloud.android.providers.DocumentsProviderUtils.assertExistsOnServer import com.owncloud.android.providers.DocumentsProviderUtils.assertListFilesEquals import com.owncloud.android.providers.DocumentsProviderUtils.assertReadEquals @@ -18,6 +19,9 @@ import com.owncloud.android.providers.DocumentsProviderUtils.listFilesBlocking import com.owncloud.android.providers.DocumentsStorageProvider.DOCUMENTID_SEPARATOR import kotlinx.coroutines.runBlocking import net.bytebuddy.utility.RandomString +import org.apache.commons.httpclient.HttpStatus +import org.apache.commons.httpclient.methods.ByteArrayRequestEntity +import org.apache.commons.httpclient.methods.PutMethod import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse @@ -172,4 +176,32 @@ class DocumentsStorageProviderIT : AbstractOnServerIT() { assertFalse(file1.exists()) assertExistsOnServer(client, ocFile1.remotePath, false) } + + @Test + fun testServerChangedFileContent() { + // create random file + val file1 = rootDir.createFile("text/plain", RandomString.make())!! + file1.assertRegularFile(size = 0L) + + val content1 = "initial content".toByteArray(); + + // write content bytes to file + contentResolver.openOutputStream(file1.uri, "wt").use { + it!!.write(content1) + } + + val remotePath = file1.getOCFile(storageManager)!!.remotePath; + + val content2 = "new content".toByteArray(); + + // modify content on server side + val putMethod = PutMethod(client.webdavUri.toString() + WebdavUtils.encodePath(remotePath)); + putMethod.setRequestEntity(ByteArrayRequestEntity(content2)); + assertEquals(HttpStatus.SC_NO_CONTENT, client.executeMethod(putMethod)); + client.exhaustResponse(putMethod.responseBodyAsStream); + putMethod.releaseConnection(); // let the connection available for other methods + + // read back content bytes + assertReadEquals(content2, contentResolver.openInputStream(file1.uri)) + } } From a4b0e29d80d05a3ff3051cfe19bdc68ed69759c5 Mon Sep 17 00:00:00 2001 From: Jens Mueller Date: Mon, 14 Sep 2020 20:35:20 +0200 Subject: [PATCH 5/6] fix review comments Signed-off-by: Jens Mueller --- .../android/providers/DocumentsStorageProvider.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/main/java/com/owncloud/android/providers/DocumentsStorageProvider.java b/src/main/java/com/owncloud/android/providers/DocumentsStorageProvider.java index 27917d2d47..7b278003a5 100644 --- a/src/main/java/com/owncloud/android/providers/DocumentsStorageProvider.java +++ b/src/main/java/com/owncloud/android/providers/DocumentsStorageProvider.java @@ -196,9 +196,7 @@ public class DocumentsStorageProvider extends DocumentsProvider { OCFile ocFile = document.getFile(); Account account = document.getAccount(); - int accessMode = ParcelFileDescriptor.parseMode(mode); - - boolean needsDownload = (accessMode != ParcelFileDescriptor.MODE_WRITE_ONLY) && (!ocFile.isDown() || hasServerChange(ocFile, account)); + boolean needsDownload = !ocFile.isDown() || hasServerChange(document); if (needsDownload) { if (ocFile.getLocalModificationTimestamp() > ocFile.getLastSyncDateForData()) { // TODO show a conflict notification with a pending intent that shows a ConflictResolveDialog @@ -216,6 +214,7 @@ public class DocumentsStorageProvider extends DocumentsProvider { File file = new File(ocFile.getStoragePath()); + int accessMode = ParcelFileDescriptor.parseMode(mode); if (accessMode != ParcelFileDescriptor.MODE_READ_ONLY) { // The calling thread is not guaranteed to have a Looper, so we can't block it with the OnCloseListener. // Thus, we are unable to do a synchronous upload and have to start an asynchronous one. @@ -249,10 +248,11 @@ public class DocumentsStorageProvider extends DocumentsProvider { } } - private boolean hasServerChange(OCFile ocFile, Account account) throws FileNotFoundException { + private boolean hasServerChange(Document document) throws FileNotFoundException { Context context = getNonNullContext(); + OCFile ocFile = document.getFile(); RemoteOperationResult result = new CheckEtagRemoteOperation(ocFile.getRemotePath(), ocFile.getEtag()) - .execute(account, context); + .execute(document.getAccount(), context); switch (result.getCode()) { case ETAG_CHANGED: return true; From 69f3f804e29efe9fcb008ed96e54e13b2a6b09ee Mon Sep 17 00:00:00 2001 From: Jens Mueller Date: Tue, 15 Sep 2020 05:36:32 +0200 Subject: [PATCH 6/6] fix ktlint Signed-off-by: Jens Mueller --- .../providers/DocumentsStorageProviderIT.kt | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/androidTest/java/com/owncloud/android/providers/DocumentsStorageProviderIT.kt b/src/androidTest/java/com/owncloud/android/providers/DocumentsStorageProviderIT.kt index e42af8a130..93423ce688 100644 --- a/src/androidTest/java/com/owncloud/android/providers/DocumentsStorageProviderIT.kt +++ b/src/androidTest/java/com/owncloud/android/providers/DocumentsStorageProviderIT.kt @@ -183,23 +183,23 @@ class DocumentsStorageProviderIT : AbstractOnServerIT() { val file1 = rootDir.createFile("text/plain", RandomString.make())!! file1.assertRegularFile(size = 0L) - val content1 = "initial content".toByteArray(); + val content1 = "initial content".toByteArray() // write content bytes to file contentResolver.openOutputStream(file1.uri, "wt").use { it!!.write(content1) } - val remotePath = file1.getOCFile(storageManager)!!.remotePath; + val remotePath = file1.getOCFile(storageManager)!!.remotePath - val content2 = "new content".toByteArray(); + val content2 = "new content".toByteArray() // modify content on server side - val putMethod = PutMethod(client.webdavUri.toString() + WebdavUtils.encodePath(remotePath)); - putMethod.setRequestEntity(ByteArrayRequestEntity(content2)); - assertEquals(HttpStatus.SC_NO_CONTENT, client.executeMethod(putMethod)); - client.exhaustResponse(putMethod.responseBodyAsStream); - putMethod.releaseConnection(); // let the connection available for other methods + val putMethod = PutMethod(client.webdavUri.toString() + WebdavUtils.encodePath(remotePath)) + putMethod.setRequestEntity(ByteArrayRequestEntity(content2)) + assertEquals(HttpStatus.SC_NO_CONTENT, client.executeMethod(putMethod)) + client.exhaustResponse(putMethod.responseBodyAsStream) + putMethod.releaseConnection() // let the connection available for other methods // read back content bytes assertReadEquals(content2, contentResolver.openInputStream(file1.uri))