Merge pull request #6917 from jmue/fixStaleFileContents

DocumentsStorageProvider: prevent opening stale file contents

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))
+    }
 }

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,6 +63,7 @@ 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.CheckEtagRemoteOperation;
 import com.owncloud.android.lib.resources.files.UploadFileRemoteOperation;
 import com.owncloud.android.operations.CopyFileOperation;
 import com.owncloud.android.operations.CreateFolderOperation;
@@ -72,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;
@@ -189,7 +188,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);
@@ -197,41 +196,27 @@ 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) {
+        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
-                Log_OC.w(TAG, "Conflict found: " + result);
-            } else if (!result.isSuccess()) {
-                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 {
-            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());
+                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());
-        int accessMode = ParcelFileDescriptor.parseMode(mode);
-        boolean isWrite = accessMode != ParcelFileDescriptor.MODE_READ_ONLY;
 
-        if (isWrite) {
+        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.
             Handler handler = new Handler(context.getMainLooper());
@@ -264,6 +249,23 @@ public class DocumentsStorageProvider extends DocumentsProvider {
         }
     }
 
+    private boolean hasServerChange(Document document) throws FileNotFoundException {
+        Context context = getNonNullContext();
+        OCFile ocFile = document.getFile();
+        RemoteOperationResult result = new CheckEtagRemoteOperation(ocFile.getRemotePath(), ocFile.getEtag())
+            .execute(document.getAccount(), 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.
      *