FINERACT-1977: 4-eye-principle, maker-checker permission

fineract-core/src/main/java/org/apache/fineract/batch/service/BatchApiServiceImpl.java

@@ -52,7 +52,6 @@
 import org.apache.fineract.batch.exception.ErrorInfo;
 import org.apache.fineract.batch.service.ResolutionHelper.BatchRequestNode;
 import org.apache.fineract.infrastructure.core.domain.BatchRequestContextHolder;
-import org.apache.fineract.infrastructure.core.exception.AbstractIdempotentCommandException;
 import org.apache.fineract.infrastructure.core.exception.ErrorHandler;
 import org.apache.fineract.infrastructure.core.filters.BatchCallHandler;
 import org.apache.fineract.infrastructure.core.filters.BatchFilter;
@@ -62,10 +61,8 @@
 import org.springframework.dao.NonTransientDataAccessException;
 import org.springframework.stereotype.Service;
 import org.springframework.transaction.PlatformTransactionManager;
-import org.springframework.transaction.TransactionDefinition;
 import org.springframework.transaction.TransactionException;
 import org.springframework.transaction.TransactionExecution;
-import org.springframework.transaction.TransactionStatus;
 import org.springframework.transaction.TransactionSystemException;
 import org.springframework.transaction.support.TransactionTemplate;
 
@@ -104,8 +101,7 @@ public class BatchApiServiceImpl implements BatchApiService {
      */
     @Override
     public List<BatchResponse> handleBatchRequestsWithoutEnclosingTransaction(final List<BatchRequest> requestList, UriInfo uriInfo) {
-        BatchRequestContextHolder.setEnclosingTransaction(Optional.empty());
-        return handleBatchRequests(false, requestList, uriInfo);
+        return handleBatchRequests(requestList, uriInfo, false);
     }
 
     /**
@@ -117,7 +113,18 @@ public List<BatchResponse> handleBatchRequestsWithoutEnclosingTransaction(final
      */
     @Override
     public List<BatchResponse> handleBatchRequestsWithEnclosingTransaction(final List<BatchRequest> requestList, final UriInfo uriInfo) {
-        return callInTransaction(Function.identity()::apply, () -> handleBatchRequests(true, requestList, uriInfo));
+        return handleBatchRequests(requestList, uriInfo, true);
+    }
+
+    private List<BatchResponse> handleBatchRequests(final List<BatchRequest> requestList, final UriInfo uriInfo,
+            boolean enclosingTransaction) {
+        BatchRequestContextHolder.setIsEnclosingTransaction(enclosingTransaction);
+        try {
+            return enclosingTransaction ? callInTransaction(Function.identity()::apply, () -> handleRequestNodes(requestList, uriInfo))
+                    : handleRequestNodes(requestList, uriInfo);
+        } finally {
+            BatchRequestContextHolder.resetIsEnclosingTransaction();
+        }
     }
 
     /**
@@ -136,18 +143,15 @@ private List<BatchResponse> callInTransaction(Consumer<TransactionTemplate> tran
             TransactionTemplate transactionTemplate = new TransactionTemplate(transactionManager);
             transactionConfigurator.accept(transactionTemplate);
             return transactionTemplate.execute(status -> {
-                BatchRequestContextHolder.setEnclosingTransaction(Optional.of(status));
+                BatchRequestContextHolder.setEnclosingTransaction(status);
                 try {
                     responseList.addAll(request.get());
                     return responseList;
                 } catch (BatchExecutionException ex) {
-                    status.setRollbackOnly();
-                    return List.of(buildErrorResponse(ex.getCause(), ex.getRequest()));
-                } catch (RuntimeException ex) {
-                    status.setRollbackOnly();
-                    return buildErrorResponses(ex, responseList);
+                    responseList.add(buildErrorResponse(ex.getCause(), ex.getRequest()));
+                    return responseList;
                 } finally {
-                    BatchRequestContextHolder.setEnclosingTransaction(Optional.empty());
+                    BatchRequestContextHolder.resetTransaction();
                 }
             });
         } catch (TransactionException | NonTransientDataAccessException ex) {
@@ -163,24 +167,17 @@ private List<BatchResponse> callInTransaction(Consumer<TransactionTemplate> tran
      * @param uriInfo
      * @return {@code List<BatchResponse>}
      */
-    private List<BatchResponse> handleBatchRequests(boolean enclosingTransaction, final List<BatchRequest> requestList,
-            final UriInfo uriInfo) {
+    private List<BatchResponse> handleRequestNodes(final List<BatchRequest> requestList, final UriInfo uriInfo) {
         final List<BatchRequestNode> rootNodes;
         try {
             rootNodes = this.resolutionHelper.buildNodesTree(requestList);
         } catch (BatchReferenceInvalidException e) {
-            return List.of(buildErrorResponse(e));
+            return List.of(buildOrThrowErrorResponse(e, null));
         }
 
         final ArrayList<BatchResponse> responseList = new ArrayList<>(requestList.size());
         for (BatchRequestNode rootNode : rootNodes) {
-            if (enclosingTransaction) {
-                this.callRequestRecursive(rootNode.getRequest(), rootNode, responseList, uriInfo, enclosingTransaction);
-            } else {
-                ArrayList<BatchResponse> localResponseList = new ArrayList<>();
-                this.callRequestRecursive(rootNode.getRequest(), rootNode, localResponseList, uriInfo, enclosingTransaction);
-                responseList.addAll(localResponseList);
-            }
+            this.callRequestRecursive(rootNode.getRequest(), rootNode, responseList, uriInfo);
         }
         responseList.sort(Comparator.comparing(BatchResponse::getRequestId));
         return responseList;
@@ -197,18 +194,10 @@ private List<BatchResponse> handleBatchRequests(boolean enclosingTransaction, fi
      *            the collected responses
      * @return {@code BatchResponse}
      */
-    private void callRequestRecursive(BatchRequest request, BatchRequestNode requestNode, List<BatchResponse> responseList, UriInfo uriInfo,
-            boolean enclosingTransaction) {
+    private void callRequestRecursive(BatchRequest request, BatchRequestNode requestNode, List<BatchResponse> responseList,
+            UriInfo uriInfo) {
         // run current node
-        BatchResponse response;
-        if (enclosingTransaction) {
-            response = executeRequest(request, uriInfo);
-        } else {
-            List<BatchResponse> transactionResponse = callInTransaction(
-                    transactionTemplate -> transactionTemplate.setPropagationBehavior(TransactionDefinition.PROPAGATION_REQUIRES_NEW),
-                    () -> List.of(executeRequest(request, uriInfo)));
-            response = transactionResponse.get(0);
-        }
+        BatchResponse response = executeRequest(request, uriInfo);
         responseList.add(response);
         if (response.getStatusCode() != null && response.getStatusCode() == SC_OK) {
             // run child nodes
@@ -217,13 +206,10 @@ private void callRequestRecursive(BatchRequest request, BatchRequestNode request
                 BatchRequest resolvedChildRequest;
                 try {
                     resolvedChildRequest = this.resolutionHelper.resolveRequest(childRequest, response);
+                    callRequestRecursive(resolvedChildRequest, childNode, responseList, uriInfo);
                 } catch (JsonPathException jpex) {
-                    responseList.add(buildErrorResponse(jpex, childRequest));
-                    return;
-                } catch (RuntimeException ex) {
-                    throw new BatchExecutionException(childRequest, ex);
+                    responseList.add(buildOrThrowErrorResponse(jpex, childRequest));
                 }
-                callRequestRecursive(resolvedChildRequest, childNode, responseList, uriInfo, enclosingTransaction);
             });
         } else {
             responseList.addAll(parentRequestFailedRecursive(request, requestNode, response, null));
@@ -245,27 +231,24 @@ private BatchResponse executeRequest(BatchRequest request, UriInfo uriInfo) {
         log.debug("Batch request: method [{}], relative url [{}]", request.getMethod(), request.getRelativeUrl());
         Either<RuntimeException, BatchRequest> preprocessorResult = runPreprocessors(request);
         if (preprocessorResult.isLeft()) {
-            throw new BatchExecutionException(request, preprocessorResult.getLeft());
+            return buildOrThrowErrorResponse(preprocessorResult.getLeft(), request);
         } else {
             request = preprocessorResult.get();
         }
         try {
             BatchRequestContextHolder.setRequestAttributes(new HashMap<>(Optional.ofNullable(request.getHeaders())
                     .map(list -> list.stream().collect(Collectors.toMap(Header::getName, Header::getValue)))
                     .orElse(Collections.emptyMap())));
-            BatchCallHandler callHandler = new BatchCallHandler(this.batchFilters, commandStrategy::execute);
-            Optional<TransactionStatus> transaction = BatchRequestContextHolder.getEnclosingTransaction();
-            if (transaction.isPresent()) {
+            if (BatchRequestContextHolder.isEnclosingTransaction()) {
                 entityManager.flush();
             }
+            BatchCallHandler callHandler = new BatchCallHandler(this.batchFilters, commandStrategy::execute);
             final BatchResponse rootResponse = callHandler.serviceCall(request, uriInfo);
             log.debug("Batch response: status code [{}], method [{}], relative url [{}]", rootResponse.getStatusCode(), request.getMethod(),
                     request.getRelativeUrl());
             return rootResponse;
-        } catch (AbstractIdempotentCommandException idempotentException) {
-            return buildErrorResponse(idempotentException, request);
         } catch (RuntimeException ex) {
-            throw new BatchExecutionException(request, ex);
+            return buildOrThrowErrorResponse(ex, request);
         } finally {
             BatchRequestContextHolder.resetRequestAttributes();
         }
@@ -313,11 +296,6 @@ private List<BatchResponse> parentRequestFailedRecursive(@NotNull BatchRequest r
         return responseList;
     }
 
-    @NotNull
-    private BatchResponse buildErrorResponse(@NotNull Throwable ex) {
-        return buildErrorResponse(ex, null);
-    }
-
     /**
      * Return the response when any exception raised
      *
@@ -346,6 +324,15 @@ private BatchResponse buildErrorResponse(Throwable ex, BatchRequest request) {
         return buildErrorResponse(requestId, statusCode, body, headers);
     }
 
+    private BatchResponse buildOrThrowErrorResponse(RuntimeException ex, BatchRequest request) {
+        BatchResponse response = buildErrorResponse(ex, request);
+        if (response.getStatusCode() != SC_OK && BatchRequestContextHolder.isEnclosingTransaction()) {
+            BatchRequestContextHolder.getTransaction().ifPresent(TransactionExecution::setRollbackOnly);
+            throw new BatchExecutionException(request, ex);
+        }
+        return response;
+    }
+
     @NotNull
     private List<BatchResponse> buildErrorResponses(Throwable ex, @NotNull List<BatchResponse> responseList) {
         BatchResponse response = responseList.isEmpty() ? null
@@ -368,9 +355,10 @@ private List<BatchResponse> buildErrorResponses(Throwable ex, @NotNull List<Batc
         }
         if (response != null) {
             requestId = response.getRequestId();
-            if (response.getStatusCode() == null || response.getStatusCode() != SC_OK) {
-                if (response.getStatusCode() != null) {
-                    statusCode = response.getStatusCode();
+            Integer responseCode = response.getStatusCode();
+            if (responseCode == null || responseCode != SC_OK) {
+                if (responseCode != null) {
+                    statusCode = responseCode;
                 }
                 body = "Transaction is being rolled back. First erroneous request: \n" + new Gson().toJson(response);
             }

fineract-core/src/main/java/org/apache/fineract/commands/domain/CommandSource.java

@@ -202,6 +202,14 @@ public void setCommandJson(final String json) {
         this.commandAsJson = json;
     }
 
+    public AppUser getMaker() {
+        return maker;
+    }
+
+    public AppUser getChecker() {
+        return checker;
+    }
+
     public String getActionName() {
         return this.actionName;
     }

fineract-core/src/main/java/org/apache/fineract/commands/exception/RollbackTransactionNotApprovedException.java

@@ -0,0 +1,41 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.fineract.commands.exception;
+
+import org.apache.fineract.infrastructure.core.data.CommandProcessingResult;
+import org.apache.fineract.infrastructure.core.data.CommandProcessingResultBuilder;
+
+/**
+ * When maker-checker is configured globally and also for the current transaction. An initial save determines if there
+ * are any integrity rule or data problems. If there isn't... and the transaction is from a maker... then this roll back
+ * is issued and the commandSourceResult is used to write the audit entry.
+ */
+public class RollbackTransactionNotApprovedException extends RuntimeException {
+
+    private final CommandProcessingResult result;
+
+    public RollbackTransactionNotApprovedException(Long commandId, Long entityId) {
+        this.result = new CommandProcessingResultBuilder().withCommandId(commandId).withEntityId(entityId).setRollbackTransaction(true)
+                .build();
+    }
+
+    public CommandProcessingResult getResult() {
+        return result;
+    }
+}

fineract-core/src/main/java/org/apache/fineract/commands/exception/UnsupportedCommandException.java

@@ -30,6 +30,11 @@ public UnsupportedCommandException(final String unsupportedCommandName) {
         this.unsupportedCommandName = unsupportedCommandName;
     }
 
+    public UnsupportedCommandException(final String unsupportedCommandName, String message) {
+        super(message);
+        this.unsupportedCommandName = unsupportedCommandName;
+    }
+
     public String getUnsupportedCommandName() {
         return this.unsupportedCommandName;
     }

fineract-core/src/main/java/org/apache/fineract/commands/service/CommandProcessingService.java

@@ -18,7 +18,6 @@
  */
 package org.apache.fineract.commands.service;
 
-import org.apache.fineract.commands.domain.CommandSource;
 import org.apache.fineract.commands.domain.CommandWrapper;
 import org.apache.fineract.infrastructure.core.api.JsonCommand;
 import org.apache.fineract.infrastructure.core.data.CommandProcessingResult;
@@ -28,8 +27,6 @@ public interface CommandProcessingService {
 
     CommandProcessingResult executeCommand(CommandWrapper wrapper, JsonCommand command, boolean isApprovedByChecker);
 
-    CommandProcessingResult logCommand(CommandSource commandSourceResult);
-
-    boolean validateCommand(CommandWrapper commandWrapper, AppUser user);
+    boolean validateRollbackCommand(CommandWrapper commandWrapper, AppUser user);
 
 }

fineract-core/src/main/java/org/apache/fineract/commands/service/CommandSourceService.java

@@ -26,7 +26,10 @@
 import org.apache.fineract.commands.domain.CommandSourceRepository;
 import org.apache.fineract.commands.domain.CommandWrapper;
 import org.apache.fineract.commands.exception.CommandNotFoundException;
+import org.apache.fineract.commands.exception.RollbackTransactionNotApprovedException;
+import org.apache.fineract.commands.handler.NewCommandSourceHandler;
 import org.apache.fineract.infrastructure.core.api.JsonCommand;
+import org.apache.fineract.infrastructure.core.data.CommandProcessingResult;
 import org.apache.fineract.infrastructure.core.exception.ErrorHandler;
 import org.apache.fineract.useradministration.domain.AppUser;
 import org.jetbrains.annotations.NotNull;
@@ -95,19 +98,24 @@ public CommandSource findCommandSource(CommandWrapper wrapper, String idempotenc
                 idempotencyKey);
     }
 
-    @Transactional(propagation = Propagation.REQUIRED)
     public CommandSource getInitialCommandSource(CommandWrapper wrapper, JsonCommand jsonCommand, AppUser maker, String idempotencyKey) {
-        CommandSource commandSourceResult;
-        if (jsonCommand.commandId() != null) {
-            commandSourceResult = commandSourceRepository.findById(jsonCommand.commandId())
-                    .orElseThrow(() -> new CommandNotFoundException(jsonCommand.commandId()));
-            commandSourceResult.markAsChecked(maker);
-        } else {
-            commandSourceResult = CommandSource.fullEntryFrom(wrapper, jsonCommand, maker, idempotencyKey, UNDER_PROCESSING.getValue());
-        }
+        CommandSource commandSourceResult = CommandSource.fullEntryFrom(wrapper, jsonCommand, maker, idempotencyKey,
+                UNDER_PROCESSING.getValue());
         if (commandSourceResult.getCommandJson() == null) {
             commandSourceResult.setCommandJson("{}");
         }
         return commandSourceResult;
     }
+
+    @Transactional
+    public CommandProcessingResult processCommand(NewCommandSourceHandler handler, JsonCommand command, CommandSource commandSource,
+            AppUser user, boolean isApprovedByChecker, boolean isMakerChecker) {
+        final CommandProcessingResult result = handler.processCommand(command);
+        boolean isRollback = !isApprovedByChecker && !user.isCheckerSuperUser() && (isMakerChecker || result.isRollbackTransaction());
+        if (isRollback) {
+            commandSource.markAsAwaitingApproval();
+            throw new RollbackTransactionNotApprovedException(commandSource.getId(), commandSource.getResourceId());
+        }
+        return result;
+    }
 }