From 9b9b33630d01260634fce472adb18ab422e962a0 Mon Sep 17 00:00:00 2001 From: sondermanish Date: Thu, 20 Feb 2025 13:35:43 +0530 Subject: [PATCH 1/2] added changes for bugs --- .../appsmith/git/files/FileUtilsCEImpl.java | 47 ++++++++++++++-- .../com/appsmith/git/files/FileUtilsImpl.java | 4 +- .../git/helpers/FileUtilsImplTest.java | 11 +++- .../server/domains/ce/ApplicationCE.java | 2 +- .../server/git/fs/GitFSServiceCEImpl.java | 54 ++++++++++--------- .../helpers/ce/CommonGitFileUtilsCE.java | 38 ++++++++----- 6 files changed, 111 insertions(+), 45 deletions(-) diff --git a/app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java b/app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java index 413720c5e483..297ac13544cc 100644 --- a/app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java +++ b/app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java @@ -6,6 +6,7 @@ import com.appsmith.external.git.FileInterface; import com.appsmith.external.git.GitExecutor; import com.appsmith.external.git.constants.GitSpan; +import com.appsmith.external.git.handler.FSGitHandler; import com.appsmith.external.git.models.GitResourceIdentity; import com.appsmith.external.git.models.GitResourceMap; import com.appsmith.external.git.models.GitResourceType; @@ -75,6 +76,7 @@ import static com.appsmith.git.constants.GitDirectories.DATASOURCE_DIRECTORY; import static com.appsmith.git.constants.GitDirectories.JS_LIB_DIRECTORY; import static com.appsmith.git.constants.GitDirectories.PAGE_DIRECTORY; +import static com.appsmith.git.constants.ce.CommonConstantsCE.DELIMITER_PATH; @Slf4j @Getter @@ -83,6 +85,7 @@ public class FileUtilsCEImpl implements FileInterface { private final GitServiceConfig gitServiceConfig; + protected final FSGitHandler fsGitHandler; private final GitExecutor gitExecutor; protected final FileOperations fileOperations; private final ObservationHelper observationHelper; @@ -101,11 +104,13 @@ public class FileUtilsCEImpl implements FileInterface { public FileUtilsCEImpl( GitServiceConfig gitServiceConfig, + FSGitHandler fsGitHandler, GitExecutor gitExecutor, FileOperations fileOperations, ObservationHelper observationHelper, ObjectMapper objectMapper) { this.gitServiceConfig = gitServiceConfig; + this.fsGitHandler = fsGitHandler; this.gitExecutor = gitExecutor; this.fileOperations = fileOperations; this.observationHelper = observationHelper; @@ -247,7 +252,7 @@ public Mono saveArtifactToGitRepo(Path baseRepoSuffix, GitResourceMap gitR // Repo path will be: // baseRepo : root/workspaceId/defaultAppId/repoName/{applicationData} // Checkout to mentioned branch if not already checked-out - return gitExecutor + return fsGitHandler .resetToLastCommit(baseRepoSuffix, branchName) .flatMap(isSwitched -> { Path baseRepo = Paths.get(gitServiceConfig.getGitRootPath()).resolve(baseRepoSuffix); @@ -263,12 +268,48 @@ public Mono saveArtifactToGitRepo(Path baseRepoSuffix, GitResourceMap gitR .subscribeOn(scheduler); } + protected Set getWhitelistedPaths() { + String pages = PAGE_DIRECTORY + DELIMITER_PATH; + String datasources = DATASOURCE_DIRECTORY + DELIMITER_PATH; + String themes = CommonConstants.THEME + JSON_EXTENSION; + String application = CommonConstants.APPLICATION + JSON_EXTENSION; + String metadata = CommonConstants.METADATA + JSON_EXTENSION; + String customJsLibs = JS_LIB_DIRECTORY + DELIMITER_PATH; + + return new HashSet<>(Set.of(pages, datasources, themes, application, metadata, customJsLibs)); + } + + protected Boolean isWhiteListedPath(Set whiteListedPaths, String relativePath) { + + // Not expecting the relative path to ever be empty. + // .git is internal file this shouldn't be whitelisted + if (!StringUtils.hasText(relativePath) || relativePath.contains(".git/")) { + return Boolean.FALSE; + } + + // cases where the path is a direct root config object + if (whiteListedPaths.contains(relativePath)) { + return Boolean.TRUE; + } + + String[] tokens = relativePath.strip().split(DELIMITER_PATH); + // it means that path is not a root config object and adheres to the given whitelisted path + if (tokens.length > 1 && whiteListedPaths.contains(tokens[0] + DELIMITER_PATH)) { + return Boolean.TRUE; + } + + return Boolean.FALSE; + } + protected Set getExistingFilesInRepo(Path baseRepo) throws IOException { + Set whiteListedPaths = getWhitelistedPaths(); try (Stream stream = Files.walk(baseRepo).parallel()) { return stream.filter(path -> { try { - return !path.toString().contains(".git" + File.separator) - && (Files.isRegularFile(path) || FileUtils.isEmptyDirectory(path.toFile())); + return (Files.isRegularFile(path) || FileUtils.isEmptyDirectory(path.toFile())) + && isWhiteListedPath( + whiteListedPaths, + baseRepo.relativize(path).toString()); } catch (IOException e) { log.error("Unable to find file details. Please check the file at file path: {}", path); log.error("Assuming that it does not exist for now ..."); diff --git a/app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsImpl.java b/app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsImpl.java index 34256a08096a..9122b5d7c3dc 100644 --- a/app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsImpl.java +++ b/app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsImpl.java @@ -2,6 +2,7 @@ import com.appsmith.external.git.FileInterface; import com.appsmith.external.git.GitExecutor; +import com.appsmith.external.git.handler.FSGitHandler; import com.appsmith.external.git.operations.FileOperations; import com.appsmith.external.helpers.ObservationHelper; import com.appsmith.git.configurations.GitServiceConfig; @@ -21,10 +22,11 @@ public class FileUtilsImpl extends FileUtilsCEImpl implements FileInterface { public FileUtilsImpl( GitServiceConfig gitServiceConfig, + FSGitHandler fsGitHandler, GitExecutor gitExecutor, FileOperations fileOperations, ObservationHelper observationHelper, ObjectMapper objectMapper) { - super(gitServiceConfig, gitExecutor, fileOperations, observationHelper, objectMapper); + super(gitServiceConfig, fsGitHandler, gitExecutor, fileOperations, observationHelper, objectMapper); } } diff --git a/app/server/appsmith-git/src/test/java/com/appsmith/git/helpers/FileUtilsImplTest.java b/app/server/appsmith-git/src/test/java/com/appsmith/git/helpers/FileUtilsImplTest.java index e222feff2a92..144fee83a010 100644 --- a/app/server/appsmith-git/src/test/java/com/appsmith/git/helpers/FileUtilsImplTest.java +++ b/app/server/appsmith-git/src/test/java/com/appsmith/git/helpers/FileUtilsImplTest.java @@ -1,5 +1,6 @@ package com.appsmith.git.helpers; +import com.appsmith.external.git.handler.FSGitHandler; import com.appsmith.external.git.operations.FileOperations; import com.appsmith.external.helpers.ObservationHelper; import com.appsmith.external.models.ApplicationGitReference; @@ -30,8 +31,9 @@ import static com.appsmith.git.constants.GitDirectories.PAGE_DIRECTORY; public class FileUtilsImplTest { - private FileUtilsImpl fileUtils; + private FileUtilsImpl fileUtils; + private FSGitHandler fsGitHandler; private GitExecutorImpl gitExecutor; private static final String localTestDirectory = "localTestDirectory"; @@ -44,7 +46,12 @@ public void setUp() { gitServiceConfig.setGitRootPath(localTestDirectoryPath.toString()); FileOperations fileOperations = new FileOperationsImpl(null, ObservationHelper.NOOP); fileUtils = new FileUtilsImpl( - gitServiceConfig, gitExecutor, fileOperations, ObservationHelper.NOOP, new ObjectMapper()); + gitServiceConfig, + fsGitHandler, + gitExecutor, + fileOperations, + ObservationHelper.NOOP, + new ObjectMapper()); } @AfterEach diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/ApplicationCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/ApplicationCE.java index e00580ef8831..787a85106810 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/ApplicationCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/ApplicationCE.java @@ -368,7 +368,7 @@ public void setApplicationDetail(ApplicationDetail applicationDetail) { } @Override - @JsonView({Views.Internal.class}) + @JsonView({Views.Public.class}) public ArtifactType getArtifactType() { return ArtifactType.APPLICATION; } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java index df6fc99f9afc..f4b6aa4daa93 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java @@ -215,13 +215,15 @@ public Mono updateImportedRepositoryDetails( String workspaceId = jsonTransformationDTO.getWorkspaceId(); String placeHolder = jsonTransformationDTO.getBaseArtifactId(); String repoName = jsonTransformationDTO.getRepoName(); - Path temporaryStorage = Path.of(workspaceId, placeHolder, repoName); + Path temporaryArtifactPath = Path.of(workspaceId, placeHolder); ArtifactType artifactType = baseArtifact.getArtifactType(); GitArtifactHelper gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType); - Path newPath = gitArtifactHelper.getRepoSuffixPath(workspaceId, baseArtifact.getId(), repoName); + Path absoluteArtifactPath = gitArtifactHelper + .getRepoSuffixPath(workspaceId, baseArtifact.getId(), repoName) + .getParent(); - return commonGitFileUtils.moveArtifactFromTemporaryToBaseArtifactIdRepository(temporaryStorage, newPath); + return commonGitFileUtils.moveRepositoryFromTemporaryStorage(temporaryArtifactPath, absoluteArtifactPath); } @Override @@ -237,29 +239,29 @@ public Mono fetchRemoteRepository( repoSuffix, gitConnectDTO.getRemoteUrl(), gitAuth.getPrivateKey(), gitAuth.getPublicKey()) .onErrorResume(error -> { log.error("Error while cloning the remote repo, {}", error.getMessage()); - return gitAnalyticsUtils - .addAnalyticsForGitOperation( - AnalyticsEvents.GIT_IMPORT, - artifact, - error.getClass().getName(), - error.getMessage(), - false) - .flatMap(user -> commonGitFileUtils - .deleteLocalRepo(repoSuffix) - .then(gitArtifactHelper.deleteArtifact(artifact.getId()))) - .flatMap(artifact1 -> { - if (error instanceof TransportException) { - return Mono.error( - new AppsmithException(AppsmithError.INVALID_GIT_SSH_CONFIGURATION)); - } else if (error instanceof InvalidRemoteException) { - return Mono.error( - new AppsmithException(AppsmithError.INVALID_PARAMETER, "remote url")); - } else if (error instanceof TimeoutException) { - return Mono.error(new AppsmithException(AppsmithError.GIT_EXECUTION_TIMEOUT)); - } - return Mono.error(new AppsmithException( - AppsmithError.GIT_ACTION_FAILED, "clone", error.getMessage())); - }); + + Mono deleteLocalRepoMono = commonGitFileUtils.deleteLocalRepo(repoSuffix); + Mono errorAnalyticsMono = gitAnalyticsUtils.addAnalyticsForGitOperation( + AnalyticsEvents.GIT_IMPORT, + artifact, + error.getClass().getName(), + error.getMessage(), + false); + + AppsmithException appsmithException; + + if (error instanceof TransportException) { + appsmithException = new AppsmithException(AppsmithError.INVALID_GIT_SSH_CONFIGURATION); + } else if (error instanceof InvalidRemoteException) { + appsmithException = new AppsmithException(AppsmithError.INVALID_PARAMETER, "remote url"); + } else if (error instanceof TimeoutException) { + appsmithException = new AppsmithException(AppsmithError.GIT_EXECUTION_TIMEOUT); + } else { + appsmithException = + new AppsmithException(AppsmithError.GIT_ACTION_FAILED, "clone", error.getMessage()); + } + + return deleteLocalRepoMono.zipWith(errorAnalyticsMono).then(Mono.error(appsmithException)); }); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java index bd0849e28971..cffb9b9b89c6 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java @@ -143,9 +143,9 @@ protected ArtifactGitFileUtils getArtifactBasedFileHelper(ArtifactType artifa * This method will save the complete application in the local repo directory. * Path to repo will be : ./container-volumes/git-repo/workspaceId/defaultApplicationId/repoName/{application_data} * - * @param baseRepoSuffix path suffix used to create a local repo path + * @param baseRepoSuffix path suffix used to create a local repo path * @param artifactExchangeJson application reference object from which entire application can be rehydrated - * @param branchName name of the branch for the current application + * @param branchName name of the branch for the current application * @return repo path where the application is stored */ public Mono saveArtifactToLocalRepo( @@ -662,9 +662,9 @@ && hasText(jsonTransformationDTO.getBaseArtifactId()) /** * Method to reconstruct the application from the local git repo * - * @param workspaceId To which workspace application needs to be rehydrated + * @param workspaceId To which workspace application needs to be rehydrated * @param baseArtifactId Root application for the current branched application - * @param branchName for which branch the application needs to rehydrate + * @param branchName for which branch the application needs to rehydrate * @param artifactType * @return application reference from which entire application can be rehydrated */ @@ -786,17 +786,31 @@ public Mono> reconstructMetadataFromRepo( }); } - public Mono moveArtifactFromTemporaryToBaseArtifactIdRepository( - Path currentRepositoryPath, Path newRepositoryPath) { - Path currentGitPath = Path.of(gitServiceConfig.getGitRootPath()).resolve(currentRepositoryPath); - Path targetPath = Path.of(gitServiceConfig.getGitRootPath()).resolve(newRepositoryPath); + public Mono moveRepositoryFromTemporaryStorage(Path temporaryPath, Path absoluteArtifactPath) { + + Path currentGitPath = Path.of(gitServiceConfig.getGitRootPath()).resolve(temporaryPath); + Path targetPath = Path.of(gitServiceConfig.getGitRootPath()).resolve(absoluteArtifactPath); return Mono.fromCallable(() -> { - if (!Files.exists(targetPath)) { - Files.createDirectories(targetPath); - } + try { + if (!Files.exists(targetPath)) { + Files.createDirectories(targetPath); + } - return Files.move(currentGitPath, targetPath, REPLACE_EXISTING); + return Files.move(currentGitPath, targetPath, REPLACE_EXISTING); + + } catch (IOException exception) { + log.error("File IO exception while moving repository. {}", exception.getMessage()); + throw new AppsmithException(AppsmithError.GIT_FILE_SYSTEM_ERROR, exception.getMessage()); + } + }) + .onErrorResume(error -> { + log.error( + "Error while moving repository from temporary storage {} to permanent storage {}", + currentGitPath, + targetPath, + error.getMessage()); + return Mono.error(error); }) .subscribeOn(Schedulers.boundedElastic()); } From 4477846d5e06577de8fdb80ae1ac5562497bdfa2 Mon Sep 17 00:00:00 2001 From: sondermanish Date: Thu, 20 Feb 2025 19:20:51 +0530 Subject: [PATCH 2/2] fixing test cases --- .../server/git/resourcemap/ExchangeJsonConversionTests.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/git/resourcemap/ExchangeJsonConversionTests.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/git/resourcemap/ExchangeJsonConversionTests.java index 9dfe3997401d..c42f8e4c3cce 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/git/resourcemap/ExchangeJsonConversionTests.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/git/resourcemap/ExchangeJsonConversionTests.java @@ -1,6 +1,6 @@ package com.appsmith.server.git.resourcemap; -import com.appsmith.external.git.GitExecutor; +import com.appsmith.external.git.handler.FSGitHandler; import com.appsmith.external.git.models.GitResourceIdentity; import com.appsmith.external.git.models.GitResourceMap; import com.appsmith.server.constants.ArtifactType; @@ -52,7 +52,7 @@ public class ExchangeJsonConversionTests { CommonGitFileUtils commonGitFileUtils; @SpyBean - GitExecutor gitExecutor; + FSGitHandler fsGitHandler; @TestTemplate public void testConvertArtifactJsonToGitResourceMap_whenArtifactIsFullyPopulated_returnsCorrespondingResourceMap( @@ -127,7 +127,7 @@ public void testSerializeArtifactExchangeJson_whenArtifactIsFullyPopulated_retur ExchangeJsonContext context) throws IOException, GitAPIException { ArtifactExchangeJson originalArtifactJson = createArtifactJson(context).block(); - Mockito.doReturn(Mono.just(true)).when(gitExecutor).resetToLastCommit(Mockito.any(), Mockito.anyString()); + Mockito.doReturn(Mono.just(true)).when(fsGitHandler).resetToLastCommit(Mockito.any(), Mockito.anyString()); Files.createDirectories(Path.of("./container-volumes/git-storage/test123"));