diff --git a/src/constants/featureFlags.ts b/src/constants/featureFlags.ts index 2f1d1f82c..e68972fd6 100644 --- a/src/constants/featureFlags.ts +++ b/src/constants/featureFlags.ts @@ -1,3 +1,4 @@ export const FEATURE_FLAGS = { GGS_WHITELISTED_REPOS: "ggs_whitelisted_repos", + IS_GGS_WHITELISTED: "is_ggs_whitelisted", } as const diff --git a/src/middleware/routeHandler.js b/src/middleware/routeHandler.js index a52b48cc5..3cd8cb8be 100644 --- a/src/middleware/routeHandler.js +++ b/src/middleware/routeHandler.js @@ -25,14 +25,13 @@ const gitFileSystemService = new GitFileSystemService( new SimpleGit({ maxConcurrentProcesses: MAX_CONCURRENT_GIT_PROCESSES }) ) -const isRepoWhitelisted = (siteName, ggsWhitelistedRepos) => { +const isRepoWhitelisted = (siteName, growthbook) => { // TODO: adding log to simplify debugging, to be removed after stabilising - logger.info( - `Checking if ${siteName} is GGS whitelisted: ${ggsWhitelistedRepos.includes( - siteName - )}` - ) - ggsWhitelistedRepos.includes(siteName) + logger.info(`Evaluating if ${siteName} is GGS whitelisted.`) + // NOTE: Growthbook has to be optional + // as we cannot guarantee its presence on `sessionData`. + // It is present iff there is a `siteHandler` attached. + return !!growthbook?.getFeatureValue(FEATURE_FLAGS.IS_GGS_WHITELISTED, false) } const handleGitFileLock = async (repoName, next) => { @@ -74,23 +73,15 @@ const attachWriteRouteHandlerWrapper = (routeHandler) => async ( const { siteName } = req.params const { growthbook } = req - let ggsWhitelistedRepos = { repos: [] } - if (growthbook) { - ggsWhitelistedRepos = growthbook.getFeatureValue( - FEATURE_FLAGS.GGS_WHITELISTED_REPOS, - { - repos: [], - } - ) - } - let isGitAvailable = true + // only check git file lock if the repo is whitelisted - if (isRepoWhitelisted(siteName, ggsWhitelistedRepos.repos)) { + if (isRepoWhitelisted(siteName, growthbook)) { isGitAvailable = await handleGitFileLock(siteName, next) } if (!isGitAvailable) return + try { await lock(siteName) } catch (err) { @@ -102,6 +93,7 @@ const attachWriteRouteHandlerWrapper = (routeHandler) => async ( await unlock(siteName) next(err) }) + try { await unlock(siteName) } catch (err) { @@ -120,20 +112,7 @@ const attachRollbackRouteHandlerWrapper = (routeHandler) => async ( const { accessToken } = userSessionData const { growthbook } = req - let ggsWhitelistedRepos = { repos: [] } - if (growthbook) { - ggsWhitelistedRepos = growthbook.getFeatureValue( - FEATURE_FLAGS.GGS_WHITELISTED_REPOS, - { - repos: [], - } - ) - } - - const shouldUseGitFileSystem = isRepoWhitelisted( - siteName, - ggsWhitelistedRepos.repos - ) + const shouldUseGitFileSystem = isRepoWhitelisted(siteName, growthbook) const isGitAvailable = await handleGitFileLock(siteName, next) if (!isGitAvailable) return diff --git a/src/services/db/RepoService.ts b/src/services/db/RepoService.ts index 5a0fbd0c6..2777bbc60 100644 --- a/src/services/db/RepoService.ts +++ b/src/services/db/RepoService.ts @@ -38,27 +38,19 @@ export default class RepoService extends GitHubService { this.gitFileSystemService = gitFileSystemService } - getGgsWhitelistedRepos( - growthbook: GrowthBook | undefined - ): string[] { - if (!growthbook) return [] - - const whitelistedRepos = growthbook.getFeatureValue( - FEATURE_FLAGS.GGS_WHITELISTED_REPOS, - { repos: [] } - ) - return whitelistedRepos.repos - } - - isRepoWhitelisted(repoName: string, ggsWhitelistedRepos: string[]): boolean { + isRepoWhitelisted( + repoName: string, + growthbook?: GrowthBook + ): boolean { // TODO: Adding for initial debugging if required. Remove once stabilised - logger.info( - `Evaluating if ${repoName} is GGS whitelisted: ${ggsWhitelistedRepos.includes( - repoName - )}` + logger.info(`Evaluating if ${repoName} is GGS whitelisted.`) + // NOTE: Growthbook has to be optional + // as we cannot guarantee its presence on `sessionData`. + // It is present iff there is a `siteHandler` attached. + return !!growthbook?.getFeatureValue( + FEATURE_FLAGS.IS_GGS_WHITELISTED, + false ) - - return ggsWhitelistedRepos.includes(repoName) } getCommitDiff(siteName: string, base?: string, head?: string) { @@ -152,12 +144,7 @@ export default class RepoService extends GitHubService { isMedia?: boolean } ): Promise<{ sha: string }> { - if ( - this.isRepoWhitelisted( - sessionData.siteName, - this.getGgsWhitelistedRepos(sessionData.growthbook) - ) - ) { + if (this.isRepoWhitelisted(sessionData.siteName, sessionData.growthbook)) { logger.info( `Writing file to local Git file system - Site name: ${sessionData.siteName}, directory name: ${directoryName}, file name: ${fileName}` ) @@ -189,12 +176,7 @@ export default class RepoService extends GitHubService { sessionData: UserWithSiteSessionData, { fileName, directoryName }: { fileName: string; directoryName?: string } ): Promise { - if ( - this.isRepoWhitelisted( - sessionData.siteName, - this.getGgsWhitelistedRepos(sessionData.growthbook) - ) - ) { + if (this.isRepoWhitelisted(sessionData.siteName, sessionData.growthbook)) { logger.info("Reading file from local Git file system") const filePath = directoryName ? `${directoryName}/${fileName}` : fileName const result = await this.gitFileSystemService.read( @@ -224,12 +206,7 @@ export default class RepoService extends GitHubService { const { siteName } = sessionData // fetch from local disk - if ( - this.isRepoWhitelisted( - siteName, - this.getGgsWhitelistedRepos(sessionData.growthbook) - ) - ) { + if (this.isRepoWhitelisted(siteName, sessionData.growthbook)) { logger.info( `Reading media file from disk. Site name: ${siteName}, directory name: ${directoryName}, file name: ${fileName},` ) @@ -274,12 +251,7 @@ export default class RepoService extends GitHubService { sessionData: UserWithSiteSessionData, { directoryName }: { directoryName: string } ): Promise { - if ( - this.isRepoWhitelisted( - sessionData.siteName, - this.getGgsWhitelistedRepos(sessionData.growthbook) - ) - ) { + if (this.isRepoWhitelisted(sessionData.siteName, sessionData.growthbook)) { logger.info("Reading directory from local Git file system") const result = await this.gitFileSystemService.listDirectoryContents( sessionData.siteName, @@ -311,12 +283,7 @@ export default class RepoService extends GitHubService { (file.type === "file" || file.type === "dir") && file.name !== PLACEHOLDER_FILE_NAME - if ( - this.isRepoWhitelisted( - siteName, - this.getGgsWhitelistedRepos(sessionData.growthbook) - ) - ) { + if (this.isRepoWhitelisted(siteName, sessionData.growthbook)) { const result = await this.gitFileSystemService.listDirectoryContents( siteName, directoryName @@ -367,12 +334,7 @@ export default class RepoService extends GitHubService { directoryName?: string } ): Promise { - if ( - this.isRepoWhitelisted( - sessionData.siteName, - this.getGgsWhitelistedRepos(sessionData.growthbook) - ) - ) { + if (this.isRepoWhitelisted(sessionData.siteName, sessionData.growthbook)) { logger.info("Updating file in local Git file system") const filePath = directoryName ? `${directoryName}/${fileName}` : fileName const result = await this.gitFileSystemService.update( @@ -411,12 +373,7 @@ export default class RepoService extends GitHubService { githubSessionData: GithubSessionData } ): Promise { - if ( - this.isRepoWhitelisted( - sessionData.siteName, - this.getGgsWhitelistedRepos(sessionData.growthbook) - ) - ) { + if (this.isRepoWhitelisted(sessionData.siteName, sessionData.growthbook)) { logger.info( `Deleting directory in local Git file system for repo: ${sessionData.siteName}, directory name: ${directoryName}` ) @@ -475,12 +432,7 @@ export default class RepoService extends GitHubService { directoryName: string } ): Promise { - if ( - this.isRepoWhitelisted( - sessionData.siteName, - this.getGgsWhitelistedRepos(sessionData.growthbook) - ) - ) { + if (this.isRepoWhitelisted(sessionData.siteName, sessionData.growthbook)) { logger.info( `Deleting file in local Git file system for repo: ${sessionData.siteName}, directory name: ${directoryName}, file name: ${fileName}` ) @@ -518,12 +470,7 @@ export default class RepoService extends GitHubService { newPath: string, message?: string ): Promise { - if ( - this.isRepoWhitelisted( - sessionData.siteName, - this.getGgsWhitelistedRepos(sessionData.growthbook) - ) - ) { + if (this.isRepoWhitelisted(sessionData.siteName, sessionData.growthbook)) { logger.info("Renaming file/directory in local Git file system") const result = await this.gitFileSystemService.renameSinglePath( sessionData.siteName, @@ -608,12 +555,7 @@ export default class RepoService extends GitHubService { targetFiles: string[], message?: string ): Promise { - if ( - this.isRepoWhitelisted( - sessionData.siteName, - this.getGgsWhitelistedRepos(sessionData.growthbook) - ) - ) { + if (this.isRepoWhitelisted(sessionData.siteName, sessionData.growthbook)) { logger.info("Moving files in local Git file system") const result = await this.gitFileSystemService.moveFiles( sessionData.siteName, @@ -697,12 +639,7 @@ export default class RepoService extends GitHubService { branchName: string ): Promise { const { siteName } = sessionData - if ( - this.isRepoWhitelisted( - siteName, - this.getGgsWhitelistedRepos(sessionData.growthbook) - ) - ) { + if (this.isRepoWhitelisted(siteName, sessionData.growthbook)) { logger.info( `Getting latest commit of branch ${branchName} for site ${siteName} from local Git file system` ) @@ -747,12 +684,7 @@ export default class RepoService extends GitHubService { }: { commitSha: string; branchName?: string } ): Promise { const { siteName } = sessionData - if ( - this.isRepoWhitelisted( - siteName, - this.getGgsWhitelistedRepos(sessionData.growthbook) - ) - ) { + if (this.isRepoWhitelisted(siteName, sessionData.growthbook)) { logger.info( `Updating repo state for site ${siteName} to ${commitSha} on local Git file system` ) diff --git a/src/services/db/__tests__/RepoService.spec.ts b/src/services/db/__tests__/RepoService.spec.ts index eb52fb77c..a4c2fbbe3 100644 --- a/src/services/db/__tests__/RepoService.spec.ts +++ b/src/services/db/__tests__/RepoService.spec.ts @@ -66,35 +66,34 @@ describe("RepoService", () => { repos: ["fake-repo", mockSiteName], }, }, + is_ggs_whitelisted: { + defaultValue: true, + }, }) }) describe("isRepoWhitelisted", () => { - it("should indicate whitelisted repos as whitelisted correctly", () => { - const actual1 = RepoService.isRepoWhitelisted( - "fake-repo", - RepoService.getGgsWhitelistedRepos( - mockUserWithSiteSessionDataAndGrowthBook.growthbook - ) - ) + it("should indicate that repos are whitelisted by default", () => { + const actual1 = RepoService.isRepoWhitelisted("fake-repo", mockGrowthBook) expect(actual1).toBe(true) const actual2 = RepoService.isRepoWhitelisted( mockSiteName, - RepoService.getGgsWhitelistedRepos( - mockUserWithSiteSessionDataAndGrowthBook.growthbook - ) + mockGrowthBook ) expect(actual2).toBe(true) }) - it("should indicate non-whitelisted repos as non-whitelisted correctly", () => { + it("should indicate blacklisted repos as not being allowed", () => { + // NOTE: Mock a blacklisted repo + const gbSpy = jest.spyOn(mockGrowthBook, "getFeatureValue") + gbSpy.mockReturnValueOnce(false) + const actual = RepoService.isRepoWhitelisted( "not-whitelisted", - RepoService.getGgsWhitelistedRepos( - mockUserWithSiteSessionDataAndGrowthBook.growthbook - ) + mockGrowthBook ) + expect(actual).toBe(false) }) }) diff --git a/src/types/featureFlags.ts b/src/types/featureFlags.ts index eadcc622e..0446ceb58 100644 --- a/src/types/featureFlags.ts +++ b/src/types/featureFlags.ts @@ -3,6 +3,7 @@ // Note: key should mirror GrowthBook exactly as it is export interface FeatureFlags { ggs_whitelisted_repos: { repos: string[] } + is_ggs_whitelisted: boolean } // List of attributes we set in GrowthBook Instance in auth middleware