From ed49e2997ea9e6d1622f518a583455b1ee0437f8 Mon Sep 17 00:00:00 2001 From: Anagh Hegde Date: Mon, 18 Sep 2023 16:19:33 +0530 Subject: [PATCH] feat: Add branch protection rules changes (#27246) ## Description This PR adds check for handling the branch protection rules before performing the git commit, pull and delete ops. This is fall back for CE behaviour. This is paid feature and to keep the downgrade experience smoother, this change set is added. #### PR fixes following issue(s) Fixes #26877 #### Type of change - New feature (non-breaking change which adds functionality) ## Testing #### How Has This Been Tested? - [ ] Manual - [ ] JUnit #### Test Plan > Add Testsmith test cases links that relate to this PR > > #### Issues raised during DP testing > Link issues raised during DP testing for better visiblity and tracking (copy link from comments dropped on this PR) > > > ## Checklist: #### Dev activity - [ ] My code follows the style guidelines of this project - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [ ] PR is being merged under a feature flag #### QA activity: - [ ] [Speedbreak features](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#speedbreakers-) have been covered - [ ] Test plan covers all impacted features and [areas of interest](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#areas-of-interest-) - [ ] Test plan has been peer reviewed by project stakeholders and other QA members - [ ] Manually tested functionality on DP - [ ] We had an implementation alignment call with stakeholders post QA Round 2 - [ ] Cypress test cases have been added and approved by SDET/manual QA - [ ] Added `Test Plan Approved` label after Cypress tests were reviewed - [ ] Added `Test Plan Approved` label after JUnit tests were reviewed --- .../server/services/ce/GitServiceCE.java | 2 + .../server/services/ce/GitServiceCEImpl.java | 48 +++++++++++++++++-- .../server/services/ce/GitServiceCETest.java | 26 ++++++++++ 3 files changed, 71 insertions(+), 5 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/GitServiceCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/GitServiceCE.java index a0f7d0e57f..ac6f238330 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/GitServiceCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/GitServiceCE.java @@ -83,4 +83,6 @@ public interface GitServiceCE { Mono fetchRemoteChanges(String defaultApplicationId, String branchName, boolean isFileLock); Mono autoCommitDSLMigration(String defaultApplicationId, String branchName); + + Mono isProtectedBranch(String branchName, GitApplicationMetadata gitApplicationMetadata); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/GitServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/GitServiceCEImpl.java index ef9b9cfa64..563495ac7e 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/GitServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/GitServiceCEImpl.java @@ -145,7 +145,7 @@ public class GitServiceCEImpl implements GitServiceCE { private final WorkspaceService workspaceService; private final RedisUtils redisUtils; private final ObservationRegistry observationRegistry; - private final GitPrivateRepoHelper gitPrivateRepoCountHelper; + private final GitPrivateRepoHelper gitPrivateRepoHelper; private static final Duration RETRY_DELAY = Duration.ofSeconds(1); private static final Integer MAX_RETRIES = 20; @@ -427,6 +427,17 @@ public class GitServiceCEImpl implements GitServiceCE { boolean isSystemGenerated = isSystemGeneratedTemp; Mono commitMono = this.getApplicationById(defaultApplicationId) + .flatMap(application -> isProtectedBranch(branchName, application.getGitApplicationMetadata()) + .flatMap(status -> { + if (Boolean.TRUE.equals(status)) { + return Mono.error(new AppsmithException( + AppsmithError.GIT_ACTION_FAILED, + "commit", + "Cannot commit to protected branch" + branchName)); + } else { + return Mono.just(application); + } + })) .flatMap(application -> { GitApplicationMetadata gitData = application.getGitApplicationMetadata(); if (Boolean.TRUE.equals(isFileLock)) { @@ -454,7 +465,7 @@ public class GitServiceCEImpl implements GitServiceCE { .save(defaultApplication) // Check if the private repo count is less than the allowed repo count .flatMap(application -> - gitPrivateRepoCountHelper.isRepoLimitReached(workspaceId, false)) + gitPrivateRepoHelper.isRepoLimitReached(workspaceId, false)) .flatMap(isRepoLimitReached -> { if (Boolean.FALSE.equals(isRepoLimitReached)) { return Mono.just(defaultApplication); @@ -739,7 +750,7 @@ public class GitServiceCEImpl implements GitServiceCE { return Mono.just(application); } // Check the limit for number of private repo - return gitPrivateRepoCountHelper + return gitPrivateRepoHelper .isRepoLimitReached(application.getWorkspaceId(), true) .flatMap(isRepoLimitReached -> { if (Boolean.FALSE.equals(isRepoLimitReached)) { @@ -1560,7 +1571,7 @@ public class GitServiceCEImpl implements GitServiceCE { return getApplicationById(applicationId); } - Mono getApplicationById(String applicationId) { + public Mono getApplicationById(String applicationId) { return applicationService .findById(applicationId, applicationPermission.getEditPermission()) .switchIfEmpty(Mono.error(new AppsmithException( @@ -1586,6 +1597,17 @@ public class GitServiceCEImpl implements GitServiceCE { * */ Mono pullMono = getApplicationById(defaultApplicationId) + .flatMap(application -> isProtectedBranch(branchName, application.getGitApplicationMetadata()) + .flatMap(status -> { + if (Boolean.TRUE.equals(status)) { + return Mono.error(new AppsmithException( + AppsmithError.GIT_ACTION_FAILED, + "commit", + "Cannot commit to protected branch" + branchName)); + } else { + return Mono.just(application); + } + })) .flatMap(application -> { GitApplicationMetadata gitData = application.getGitApplicationMetadata(); return addFileLock(gitData.getDefaultApplicationId()).then(Mono.just(application)); @@ -2443,7 +2465,7 @@ public class GitServiceCEImpl implements GitServiceCE { if (!isRepoPrivate) { return Mono.just(gitAuth).zipWith(applicationMono); } - return gitPrivateRepoCountHelper + return gitPrivateRepoHelper .isRepoLimitReached(workspaceId, true) .flatMap(isRepoLimitReached -> { if (Boolean.FALSE.equals(isRepoLimitReached)) { @@ -2693,6 +2715,17 @@ public class GitServiceCEImpl implements GitServiceCE { @Override public Mono deleteBranch(String defaultApplicationId, String branchName) { Mono deleteBranchMono = getApplicationById(defaultApplicationId) + .flatMap(application -> isProtectedBranch(branchName, application.getGitApplicationMetadata()) + .flatMap(status -> { + if (Boolean.TRUE.equals(status)) { + return Mono.error(new AppsmithException( + AppsmithError.GIT_ACTION_FAILED, + "delete", + "Cannot delete protected branch" + branchName)); + } else { + return Mono.just(application); + } + })) .flatMap(application -> addFileLock(defaultApplicationId).map(status -> application)) .flatMap(application -> { GitApplicationMetadata gitApplicationMetadata = application.getGitApplicationMetadata(); @@ -2933,6 +2966,11 @@ public class GitServiceCEImpl implements GitServiceCE { }); } + @Override + public Mono isProtectedBranch(String branchName, GitApplicationMetadata gitApplicationMetadata) { + return Mono.just(Boolean.FALSE); + } + private Mono deleteApplicationCreatedFromGitImport( String applicationId, String workspaceId, String repoName) { Path repoSuffix = Paths.get(workspaceId, applicationId, repoName); diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/GitServiceCETest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/GitServiceCETest.java index d1b7db740c..40cf58f723 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/GitServiceCETest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/GitServiceCETest.java @@ -116,6 +116,7 @@ import static com.appsmith.server.acl.AclPermission.READ_PAGES; import static com.appsmith.server.constants.FieldName.DEFAULT_PAGE_LAYOUT; import static java.lang.Boolean.TRUE; import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; @@ -4094,4 +4095,29 @@ public class GitServiceCETest { }) .verifyComplete(); } + + // The branch can be configured in free trial but when the user downgrades the plan, + // behaviour will be not same as the paid version. + @Test + @WithUserDetails(value = "api_user") + public void isProtectedBranch_branchNotConfiguredInPaidVersion_notProtected() { + GitApplicationMetadata gitApplicationMetadata = new GitApplicationMetadata(); + List protectedBranches = List.of("master", "feature"); + gitApplicationMetadata.setBranchProtectionRules(protectedBranches); + StepVerifier.create(gitService.isProtectedBranch("feature", gitApplicationMetadata)) + .assertNext(aBoolean -> assertEquals(false, aBoolean)) + .verifyComplete(); + } + + // Branch not configured is empty and user is in free version + @Test + @WithUserDetails(value = "api_user") + public void isProtectedBranch_branchNotConfiguredInFreeVersion_notProtected() { + GitApplicationMetadata gitApplicationMetadata = new GitApplicationMetadata(); + List protectedBranches = List.of(); + gitApplicationMetadata.setBranchProtectionRules(protectedBranches); + StepVerifier.create(gitService.isProtectedBranch("feature", gitApplicationMetadata)) + .assertNext(aBoolean -> assertEquals(false, aBoolean)) + .verifyComplete(); + } }