fix fileservice util to support correctly immutible files (#7734)

Signed-off-by: Lev Povolotsky <[email protected]>
hashgraph · Aug 2, 2023 · f1e0bed · f1e0bed
1 parent 18d681a
commit f1e0bed
Show file tree

Hide file tree

Showing 14 changed files with 485 additions and 123 deletions.
diff --git a/hedera-node/hedera-file-service-impl/build.gradle.kts b/hedera-node/hedera-file-service-impl/build.gradle.kts
@@ -23,6 +23,7 @@ dependencies {
         annotationProcessor(gav("dagger.compiler"))
 
         testImplementation(project(":app-service-token"))
+        testImplementation(project(":app"))
         testImplementation(testFixtures(project(":app-service-mono")))
         testImplementation(testFixtures(project(":config")))
         testImplementation(testFixtures(project(":app-spi")))

diff --git a/...-impl/src/main/java/com/hedera/node/app/service/file/impl/handlers/FileAppendHandler.java b/...-impl/src/main/java/com/hedera/node/app/service/file/impl/handlers/FileAppendHandler.java
@@ -19,9 +19,11 @@
 import static com.hedera.hapi.node.base.ResponseCodeEnum.FILE_CONTENT_EMPTY;
 import static com.hedera.hapi.node.base.ResponseCodeEnum.FILE_DELETED;
 import static com.hedera.hapi.node.base.ResponseCodeEnum.INVALID_FILE_ID;
+import static com.hedera.hapi.node.base.ResponseCodeEnum.UNAUTHORIZED;
 import static com.hedera.node.app.service.file.impl.utils.FileServiceUtils.preValidate;
 import static com.hedera.node.app.service.file.impl.utils.FileServiceUtils.validateAndAddRequiredKeys;
 import static com.hedera.node.app.service.file.impl.utils.FileServiceUtils.validateContent;
+import static com.hedera.node.app.spi.workflows.HandleException.validateFalse;
 import static java.util.Objects.requireNonNull;
 
 import com.hedera.hapi.node.base.HederaFunctionality;
@@ -72,9 +74,10 @@ public void preHandle(@NonNull final PreHandleContext context) throws PreCheckEx
 
         final var transactionBody = context.body().fileAppendOrThrow();
         final var fileStore = context.createStore(ReadableFileStore.class);
-        final var fileMeta = preValidate(transactionBody.fileID(), fileStore, context, false);
+        preValidate(transactionBody.fileID(), fileStore, context, false);
 
-        validateAndAddRequiredKeys(fileMeta.keys(), context, true);
+        var file = fileStore.getFileLeaf(transactionBody.fileID());
+        validateAndAddRequiredKeys(file.orElse(null), null, context);
     }
 
     @Override
@@ -107,6 +110,11 @@ public void handle(@NonNull final HandleContext handleContext) throws HandleExce
         }
         final var file = optionalFile.get();
 
+        // TODO: skip at least the mutability check for privileged "payer" accounts
+
+        // First validate this file is mutable; and the pending mutations are allowed
+        validateFalse(file.keys() == null, UNAUTHORIZED);
+
         if (file.deleted()) {
             throw new HandleException(FILE_DELETED);
         }

diff --git a/...-impl/src/main/java/com/hedera/node/app/service/file/impl/handlers/FileCreateHandler.java b/...-impl/src/main/java/com/hedera/node/app/service/file/impl/handlers/FileCreateHandler.java
@@ -27,6 +27,7 @@
 import com.hedera.hapi.node.base.AccountID;
 import com.hedera.hapi.node.base.FileID;
 import com.hedera.hapi.node.base.HederaFunctionality;
+import com.hedera.hapi.node.base.KeyList;
 import com.hedera.hapi.node.state.file.File;
 import com.hedera.node.app.service.file.impl.WritableFileStore;
 import com.hedera.node.app.service.file.impl.records.CreateFileRecordBuilder;
@@ -66,7 +67,7 @@ public void preHandle(@NonNull final PreHandleContext context) throws PreCheckEx
 
         final var transactionBody = context.body().fileCreateOrThrow();
 
-        validateAndAddRequiredKeys(transactionBody.keys(), context, false);
+        validateAndAddRequiredKeys(null, transactionBody.keys(), context);
 
         if (!transactionBody.hasExpirationTime()) {
             throw new PreCheckException(INVALID_EXPIRATION_TIME);
@@ -81,8 +82,11 @@ public void handle(@NonNull final HandleContext handleContext) throws HandleExce
         final var fileServiceConfig = handleContext.configuration().getConfigData(FilesConfig.class);
 
         final var fileCreateTransactionBody = handleContext.body().fileCreateOrThrow();
+
+        // TODO: skip at least the mutability check for privileged "payer" accounts
         if (fileCreateTransactionBody.hasKeys()) {
-            builder.keys(fileCreateTransactionBody.keys());
+            KeyList transactionKeyList = fileCreateTransactionBody.keys();
+            builder.keys(transactionKeyList);
         }
 
         /* Validate if the current file can be created */

diff --git a/...-impl/src/main/java/com/hedera/node/app/service/file/impl/handlers/FileDeleteHandler.java b/...-impl/src/main/java/com/hedera/node/app/service/file/impl/handlers/FileDeleteHandler.java
@@ -17,14 +17,16 @@
 package com.hedera.node.app.service.file.impl.handlers;
 
 import static com.hedera.hapi.node.base.ResponseCodeEnum.INVALID_FILE_ID;
+import static com.hedera.hapi.node.base.ResponseCodeEnum.UNAUTHORIZED;
 import static com.hedera.node.app.service.file.impl.utils.FileServiceUtils.preValidate;
 import static com.hedera.node.app.service.file.impl.utils.FileServiceUtils.validateAndAddRequiredKeys;
 import static com.hedera.node.app.service.file.impl.utils.FileServiceUtils.verifyNotSystemFile;
+import static com.hedera.node.app.spi.workflows.HandleException.validateFalse;
 import static java.util.Objects.requireNonNull;
 
 import com.hedera.hapi.node.base.HederaFunctionality;
 import com.hedera.hapi.node.state.file.File;
-import com.hedera.node.app.service.file.impl.ReadableFileStoreImpl;
+import com.hedera.node.app.service.file.ReadableFileStore;
 import com.hedera.node.app.service.file.impl.WritableFileStore;
 import com.hedera.node.app.spi.workflows.HandleContext;
 import com.hedera.node.app.spi.workflows.HandleException;
@@ -61,10 +63,11 @@ public void preHandle(@NonNull final PreHandleContext context) throws PreCheckEx
         requireNonNull(context);
 
         final var transactionBody = context.body().fileDeleteOrThrow();
-        final var fileStore = context.createStore(ReadableFileStoreImpl.class);
-        final var fileMeta = preValidate(transactionBody.fileID(), fileStore, context, true);
+        final var fileStore = context.createStore(ReadableFileStore.class);
+        preValidate(transactionBody.fileID(), fileStore, context, true);
 
-        validateAndAddRequiredKeys(fileMeta.keys(), context, true);
+        var file = fileStore.getFileLeaf(transactionBody.fileID());
+        validateAndAddRequiredKeys(file.orElse(null), null, context);
     }
 
     @Override
@@ -82,6 +85,9 @@ public void handle(@NonNull final HandleContext handleContext) throws HandleExce
 
         final File file = verifyNotSystemFile(ledgerConfig, fileStore, fileId);
 
+        // First validate this file is mutable; and the pending mutations are allowed
+        validateFalse(file.keys() == null, UNAUTHORIZED);
+
         /* Copy part of the fields from existing, delete the file content and set the deleted flag  */
         final var fileBuilder = new File.Builder()
                 .fileId(file.fileId())

diff --git a/...src/main/java/com/hedera/node/app/service/file/impl/handlers/FileSystemDeleteHandler.java b/...src/main/java/com/hedera/node/app/service/file/impl/handlers/FileSystemDeleteHandler.java
@@ -25,7 +25,7 @@
 import com.hedera.hapi.node.base.HederaFunctionality;
 import com.hedera.hapi.node.base.TimestampSeconds;
 import com.hedera.hapi.node.state.file.File;
-import com.hedera.node.app.service.file.impl.ReadableFileStoreImpl;
+import com.hedera.node.app.service.file.ReadableFileStore;
 import com.hedera.node.app.service.file.impl.WritableFileStore;
 import com.hedera.node.app.spi.workflows.HandleContext;
 import com.hedera.node.app.spi.workflows.HandleException;
@@ -62,15 +62,18 @@ public void preHandle(@NonNull final PreHandleContext context) throws PreCheckEx
         requireNonNull(context);
 
         final var transactionBody = context.body().systemDeleteOrThrow();
-        final var fileStore = context.createStore(ReadableFileStoreImpl.class);
-        final var fileMeta = preValidate(transactionBody.fileID(), fileStore, context, true);
+        final var fileStore = context.createStore(ReadableFileStore.class);
+        preValidate(transactionBody.fileID(), fileStore, context, true);
 
-        validateAndAddRequiredKeys(fileMeta.keys(), context, true);
+        var file = fileStore.getFileLeaf(transactionBody.fileID());
+        validateAndAddRequiredKeys(file.orElse(null), null, context);
     }
 
     @Override
     public void handle(@NonNull final HandleContext handleContext) throws HandleException {
         requireNonNull(handleContext);
+        // TODO: check here that the "payer" is a privileged account.
+        //       a privileged account is always required for this transaction.
 
         final var systemDeleteTransactionBody = handleContext.body().systemDeleteOrThrow();
         if (!systemDeleteTransactionBody.hasFileID()) {

diff --git a/...c/main/java/com/hedera/node/app/service/file/impl/handlers/FileSystemUndeleteHandler.java b/...c/main/java/com/hedera/node/app/service/file/impl/handlers/FileSystemUndeleteHandler.java
@@ -23,7 +23,7 @@
 
 import com.hedera.hapi.node.base.HederaFunctionality;
 import com.hedera.hapi.node.state.file.File;
-import com.hedera.node.app.service.file.impl.ReadableFileStoreImpl;
+import com.hedera.node.app.service.file.ReadableFileStore;
 import com.hedera.node.app.service.file.impl.WritableFileStore;
 import com.hedera.node.app.service.file.impl.utils.FileServiceUtils;
 import com.hedera.node.app.spi.workflows.HandleContext;
@@ -61,15 +61,18 @@ public void preHandle(@NonNull final PreHandleContext context) throws PreCheckEx
         requireNonNull(context);
 
         final var transactionBody = context.body().systemUndeleteOrThrow();
-        final var fileStore = context.createStore(ReadableFileStoreImpl.class);
-        final var fileMeta = preValidate(transactionBody.fileID(), fileStore, context, true);
+        final var fileStore = context.createStore(ReadableFileStore.class);
+        preValidate(transactionBody.fileID(), fileStore, context, true);
 
-        validateAndAddRequiredKeys(fileMeta.keys(), context, true);
+        var file = fileStore.getFileLeaf(transactionBody.fileID());
+        validateAndAddRequiredKeys(file.orElse(null), null, context);
     }
 
     @Override
     public void handle(@NonNull final HandleContext handleContext) throws HandleException {
         requireNonNull(handleContext);
+        // TODO: check here that the "payer" is a privileged account.
+        //       a privileged account is always required for this transaction.
 
         final var systemUndeleteTransactionBody = handleContext.body().systemUndeleteOrThrow();
         if (!systemUndeleteTransactionBody.hasFileID()) {

diff --git a/...-impl/src/main/java/com/hedera/node/app/service/file/impl/handlers/FileUpdateHandler.java b/...-impl/src/main/java/com/hedera/node/app/service/file/impl/handlers/FileUpdateHandler.java
@@ -71,9 +71,10 @@ public void preHandle(@NonNull final PreHandleContext context) throws PreCheckEx
         requireNonNull(context);
         final var transactionBody = context.body().fileUpdateOrThrow();
         final var fileStore = context.createStore(ReadableFileStore.class);
-
         preValidate(transactionBody.fileID(), fileStore, context, false);
-        validateAndAddRequiredKeys(transactionBody.keys(), context, true);
+
+        var file = fileStore.getFileLeaf(transactionBody.fileID());
+        validateAndAddRequiredKeys(file.orElse(null), transactionBody.keys(), context);
     }
 
     @Override
@@ -105,7 +106,8 @@ public void handle(@NonNull final HandleContext handleContext) throws HandleExce
         validateFalse(file.deleted(), FILE_DELETED);
 
         // First validate this file is mutable; and the pending mutations are allowed
-        validateFalse(file.keys() == null && wantsToMutateNonExpiryField(fileUpdate), UNAUTHORIZED);
+        // TODO: add or condition for privilege accounts from context
+        validateFalse(file.keys() == null, UNAUTHORIZED);
 
         validateMaybeNewMemo(handleContext.attributeValidator(), fileUpdate);
 

diff --git a/...vice-impl/src/main/java/com/hedera/node/app/service/file/impl/utils/FileServiceUtils.java b/...vice-impl/src/main/java/com/hedera/node/app/service/file/impl/utils/FileServiceUtils.java
@@ -96,29 +96,26 @@ public static void validateContent(@NonNull byte[] content, @NonNull FilesConfig
     /**
      * The function validates the keys and adds them to the context.
      *
-     * @param listKeys the list of keys to validate and add to required keys in context
+     * @param file file that will be checked for required keys
+     * @param transactionKeys transaction keys that add to context for required keys.
      * @param context the prehandle context for the transaction.
-     * @param areKeysRequired create allows files to be created without additional keys. Therefore,
-     *     this flag is needed.
-     * @throws PreCheckException
      */
     public static void validateAndAddRequiredKeys(
-            @Nullable final KeyList listKeys, @NonNull final PreHandleContext context, final boolean areKeysRequired)
-            throws PreCheckException {
+            @Nullable final File file,
+            @Nullable final KeyList transactionKeys,
+            @NonNull final PreHandleContext context) {
+        if (file != null) {
+            KeyList fileKeyList = file.keys();
+
+            if (fileKeyList != null && fileKeyList.hasKeys()) {
+                for (final Key key : fileKeyList.keys()) {
+                    context.requireKey(key);
+                }
+            }
+        }
 
-        // TODO This logic is wrong. What we should be doing, is verifying whether the file in state has keys, and
-        //  if so, that all those keys are added to the required signing keys, and if not, then unless the user is
-        //  a super user, we should throw UNAUTHORIZED. But just because the update transaction itself (for example)
-        //  is missing keys DOES NOT mean that we should throw UNAUTHORIZED.
-        //        if (listKeys == null || !listKeys.hasKeys() || listKeys.keys().isEmpty()) {
-        //            // @todo('protobuf change needed') change to immutable file response code
-        //            if (areKeysRequired) {
-        //                throw new PreCheckException(UNAUTHORIZED);
-        //            }
-        //        }
-
-        if (listKeys != null && listKeys.hasKeys()) {
-            for (final Key key : listKeys.keys()) {
+        if (transactionKeys != null && transactionKeys.hasKeys()) {
+            for (final Key key : transactionKeys.keys()) {
                 context.requireKey(key);
             }
         }

diff --git a/...e-service-impl/src/test/java/com/hedera/node/app/service/file/impl/test/FileTestBase.java b/...e-service-impl/src/test/java/com/hedera/node/app/service/file/impl/test/FileTestBase.java
@@ -42,6 +42,7 @@
 import com.hedera.node.app.spi.fixtures.state.ListWritableQueueState;
 import com.hedera.node.app.spi.fixtures.state.MapReadableKVState;
 import com.hedera.node.app.spi.fixtures.state.MapWritableKVState;
+import com.hedera.node.app.spi.signatures.SignatureVerification;
 import com.hedera.node.app.spi.state.FilteredReadableStates;
 import com.hedera.node.app.spi.state.FilteredWritableStates;
 import com.hedera.node.app.spi.state.ReadableStates;
@@ -128,6 +129,9 @@ public class FileTestBase {
     @Mock(strictness = LENIENT)
     protected HandleContext handleContext;
 
+    @Mock(strictness = LENIENT)
+    protected SignatureVerification signatureVerification;
+
     protected MapReadableKVState<FileID, File> readableFileState;
     protected MapWritableKVState<FileID, File> writableFileState;