Merge pull request finos#1094 from 06kellyjac/trim_dot_git_correctly

fix: only trim trailing .git not any match

Author: jescalada

src/db/file/pushes.ts

@@ -2,7 +2,7 @@ import fs from 'fs';
 import _ from 'lodash';
 import Datastore from '@seald-io/nedb';
 import { Action } from '../../proxy/actions/Action';
-import { toClass } from '../helper';
+import { toClass, trimTrailingDotGit } from '../helper';
 import * as repo from './repo';
 import { PushQuery } from '../types';
 
@@ -138,7 +138,7 @@ export const canUserCancelPush = async (id: string, user: string) => {
       return;
     }
 
-    const repoName = pushDetail.repoName.replace('.git', '');
+    const repoName = trimTrailingDotGit(pushDetail.repoName);
     const isAllowed = await repo.isUserPushAllowed(repoName, user);
 
     if (isAllowed) {
@@ -156,7 +156,8 @@ export const canUserApproveRejectPush = async (id: string, user: string) => {
       resolve(false);
       return;
     }
-    const repoName = action.repoName.replace('.git', '');
+
+    const repoName = trimTrailingDotGit(action.repoName);
     const isAllowed = await repo.canUserApproveRejectPushRepo(repoName, user);
 
     resolve(isAllowed);

src/db/helper.ts

@@ -3,3 +3,21 @@ export const toClass = function (obj: any, proto: any) {
   obj.__proto__ = proto;
   return obj;
 };
+
+export const trimTrailingDotGit = (str: string): string => {
+  const target = '.git';
+  if (str.endsWith(target)) {
+    // extract string from 0 to the end minus the length of target
+    return str.slice(0, -target.length);
+  }
+  return str;
+};
+
+export const trimPrefixRefsHeads = (str: string): string => {
+  const target = 'refs/heads/';
+  if (str.startsWith(target)) {
+    // extract string from the end of the target to the end of str
+    return str.slice(target.length);
+  }
+  return str;
+};

src/db/mongo/pushes.ts

@@ -1,6 +1,6 @@
 import { connect, findDocuments, findOneDocument } from './helper';
 import { Action } from '../../proxy/actions';
-import { toClass } from '../helper';
+import { toClass, trimTrailingDotGit } from '../helper';
 import * as repo from './repo';
 import { Push, PushQuery } from '../types';
 
@@ -108,7 +108,7 @@ export const canUserApproveRejectPush = async (id: string, user: string) => {
       return;
     }
 
-    const repoName = action.repoName.replace('.git', '');
+    const repoName = trimTrailingDotGit(action.repoName);
     const isAllowed = await repo.canUserApproveRejectPushRepo(repoName, user);
 
     resolve(isAllowed);
@@ -123,7 +123,7 @@ export const canUserCancelPush = async (id: string, user: string) => {
       return;
     }
 
-    const repoName = pushDetail.repoName.replace('.git', '');
+    const repoName = trimTrailingDotGit(pushDetail.repoName);
     const isAllowed = await repo.isUserPushAllowed(repoName, user);
 
     if (isAllowed) {

src/proxy/processors/push-action/checkRepoInAuthorisedList.ts

@@ -1,6 +1,7 @@
 import { Action, Step } from '../../actions';
 import { getRepos } from '../../../db';
 import { Repo } from '../../../db/types';
+import { trimTrailingDotGit } from '../../../db/helper';
 
 // Execute if the repo is approved
 const exec = async (
@@ -14,8 +15,8 @@ const exec = async (
   console.log(list);
 
   const found = list.find((x: Repo) => {
-    const targetName = action.repo.replace('.git', '').toLowerCase();
-    const allowedName = `${x.project}/${x.name}`.replace('.git', '').toLowerCase();
+    const targetName = trimTrailingDotGit(action.repo.toLowerCase());
+    const allowedName = trimTrailingDotGit(`${x.project}/${x.name}`.toLowerCase());
     console.log(`${targetName} = ${allowedName}`);
     return targetName === allowedName;
   });

src/proxy/processors/push-action/checkUserPushPermission.ts

@@ -1,11 +1,20 @@
 import { Action, Step } from '../../actions';
 import { getUsers, isUserPushAllowed } from '../../../db';
+import { trimTrailingDotGit } from '../../../db/helper';
 
 // Execute if the repo is approved
 const exec = async (req: any, action: Action): Promise<Action> => {
   const step = new Step('checkUserPushPermission');
 
-  const repoName = action.repo.split('/')[1].replace('.git', '');
+  const repoSplit = trimTrailingDotGit(action.repo.toLowerCase()).split('/');
+  // we expect there to be exactly one / separating org/repoName
+  if (repoSplit.length != 2) {
+    step.setError('Server-side issue extracting repoName');
+    action.addStep(step);
+    return action;
+  }
+  // pull the 2nd value of the split for repoName
+  const repoName = repoSplit[1];
   let isUserAllowed = false;
   let user = action.user;
 

src/ui/views/OpenPushRequests/components/PushesTable.tsx

@@ -16,6 +16,7 @@ import { KeyboardArrowRight } from '@material-ui/icons';
 import Search from '../../../components/Search/Search';
 import Pagination from '../../../components/Pagination/Pagination';
 import { PushData } from '../../../../types/models';
+import { trimPrefixRefsHeads, trimTrailingDotGit } from '../../../../db/helper';
 
 interface PushesTableProps {
   [key: string]: any;
@@ -100,8 +101,8 @@ const PushesTable: React.FC<PushesTableProps> = (props) => {
           </TableHead>
           <TableBody>
             {[...currentItems].reverse().map((row) => {
-              const repoFullName = row.repo.replace('.git', '');
-              const repoBranch = row.branch.replace('refs/heads/', '');
+              const repoFullName = trimTrailingDotGit(row.repo);
+              const repoBranch = trimPrefixRefsHeads(row.branch);
               const commitTimestamp =
                 row.commitData[0]?.commitTs || row.commitData[0]?.commitTimestamp;
 

src/ui/views/PushDetails/PushDetails.tsx

@@ -23,6 +23,7 @@ import { CheckCircle, Visibility, Cancel, Block } from '@material-ui/icons';
 import Snackbar from '@material-ui/core/Snackbar';
 import Tooltip from '@material-ui/core/Tooltip';
 import { PushData } from '../../../types/models';
+import { trimPrefixRefsHeads, trimTrailingDotGit } from '../../../db/helper';
 
 const Dashboard: React.FC = () => {
   const { id } = useParams<{ id: string }>();
@@ -105,8 +106,8 @@ const Dashboard: React.FC = () => {
     };
   }
 
-  const repoFullName = data.repo.replace('.git', '');
-  const repoBranch = data.branch.replace('refs/heads/', '');
+  const repoFullName = trimTrailingDotGit(data.repo);
+  const repoBranch = trimPrefixRefsHeads(data.branch);
 
   const generateIcon = (title: string) => {
     switch (title) {

src/ui/views/RepoDetails/RepoDetails.tsx

@@ -20,6 +20,7 @@ import { useNavigate, useParams } from 'react-router-dom';
 import { UserContext } from '../../../context';
 import CodeActionButton from '../../components/CustomButtons/CodeActionButton';
 import { Box } from '@material-ui/core';
+import { trimTrailingDotGit } from '../../../db/helper';
 
 interface RepoData {
   project: string;
@@ -148,7 +149,7 @@ const RepoDetails: React.FC = () => {
                   <FormLabel component='legend'>URL</FormLabel>
                   <h4>
                     <a href={data.url} target='_blank' rel='noopener noreferrer'>
-                      {data.url.replace('.git', '')}
+                      {trimTrailingDotGit(data.url)}
                     </a>
                   </h4>
                 </GridItem>

test/db-helper.test.js

@@ -0,0 +1,63 @@
+const { expect } = require('chai');
+const { trimPrefixRefsHeads, trimTrailingDotGit } = require('../src/db/helper');
+
+describe('db helpers', () => {
+  describe('trimPrefixRefsHeads', () => {
+    it('removes `refs/heads/`', () => {
+      const res = trimPrefixRefsHeads('refs/heads/test');
+      expect(res).to.equal('test');
+    });
+
+    it('removes only one `refs/heads/`', () => {
+      const res = trimPrefixRefsHeads('refs/heads/refs/heads/');
+      expect(res).to.equal('refs/heads/');
+    });
+
+    it('removes only the first `refs/heads/`', () => {
+      const res = trimPrefixRefsHeads('refs/heads/middle/refs/heads/end/refs/heads/');
+      expect(res).to.equal('middle/refs/heads/end/refs/heads/');
+    });
+
+    it('handles empty string', () => {
+      const res = trimPrefixRefsHeads('');
+      expect(res).to.equal('');
+    });
+
+    it("doesn't remove `refs/heads`", () => {
+      const res = trimPrefixRefsHeads('refs/headstest');
+      expect(res).to.equal('refs/headstest');
+    });
+
+    it("doesn't remove `/refs/heads/`", () => {
+      const res = trimPrefixRefsHeads('/refs/heads/test');
+      expect(res).to.equal('/refs/heads/test');
+    });
+  });
+
+  describe('trimTrailingDotGit', () => {
+    it('removes `.git`', () => {
+      const res = trimTrailingDotGit('test.git');
+      expect(res).to.equal('test');
+    });
+
+    it('removes only one `.git`', () => {
+      const res = trimTrailingDotGit('.git.git');
+      expect(res).to.equal('.git');
+    });
+
+    it('removes only the last `.git`', () => {
+      const res = trimTrailingDotGit('.git-middle.git-end.git');
+      expect(res).to.equal('.git-middle.git-end');
+    });
+
+    it('handles empty string', () => {
+      const res = trimTrailingDotGit('');
+      expect(res).to.equal('');
+    });
+
+    it("doesn't remove just `git`", () => {
+      const res = trimTrailingDotGit('testgit');
+      expect(res).to.equal('testgit');
+    });
+  });
+});

test/testDb.test.js

@@ -1,6 +1,7 @@
 // This test needs to run first
 const chai = require('chai');
 const db = require('../src/db');
+const { trimTrailingDotGit } = require('../src/db/helper');
 
 const { expect } = chai;
 
@@ -46,6 +47,21 @@ const TEST_PUSH = {
   attestation: null,
 };
 
+const TEST_REPO_DOT_GIT = {
+  project: 'finos',
+  name: 'db.git-test-repo',
+  url: 'https://github.com/finos/db.git-test-repo.git',
+};
+
+// the same as TEST_PUSH but with .git somewhere valid within the name
+// to ensure a global replace isn't done when trimming, just to the end
+const TEST_PUSH_DOT_GIT = {
+  ...TEST_PUSH,
+  repoName: 'db.git-test-repo.git',
+  url: 'https://github.com/finos/db.git-test-repo.git',
+  repo: 'finos/db.git-test-repo.git',
+};
+
 /**
  * Clean up response data from the DB by removing an extraneous properties,
  * allowing comparison with expect.
@@ -563,7 +579,7 @@ describe('Database clients', async () => {
 
   it('should be able to check if a user can cancel push', async function () {
     let threwError = false;
-    const repoName = TEST_PUSH.repoName.replace('.git', '');
+    const repoName = trimTrailingDotGit(TEST_PUSH.repoName);
     try {
       // push does not exist yet, should return false
       let allowed = await db.canUserCancelPush(TEST_PUSH.id, TEST_USER.username);
@@ -588,34 +604,81 @@ describe('Database clients', async () => {
   });
 
   it('should be able to check if a user can approve/reject push', async function () {
-    let threwError = false;
-    const repoName = TEST_PUSH.repoName.replace('.git', '');
+    let allowed = undefined;
+    const repoName = trimTrailingDotGit(TEST_PUSH.repoName);
+
     try {
       // push does not exist yet, should return false
-      let allowed = await db.canUserApproveRejectPush(TEST_PUSH.id, TEST_USER.username);
+      allowed = await db.canUserApproveRejectPush(TEST_PUSH.id, TEST_USER.username);
       expect(allowed).to.be.false;
+    } catch (e) {
+      expect.fail(e);
+    }
 
+    try {
       // create the push - user should already exist and not authorised to push
       await db.writeAudit(TEST_PUSH);
       allowed = await db.canUserApproveRejectPush(TEST_PUSH.id, TEST_USER.username);
       expect(allowed).to.be.false;
+    } catch (e) {
+      expect.fail(e);
+    }
 
+    try {
       // authorise user and recheck
       await db.addUserCanAuthorise(repoName, TEST_USER.username);
       allowed = await db.canUserApproveRejectPush(TEST_PUSH.id, TEST_USER.username);
       expect(allowed).to.be.true;
     } catch (e) {
-      threwError = true;
+      expect.fail(e);
     }
-    expect(threwError).to.be.false;
+
     // clean up
     await db.deletePush(TEST_PUSH.id);
     await db.removeUserCanAuthorise(repoName, TEST_USER.username);
   });
 
+  it('should be able to check if a user can approve/reject push including .git within the repo name', async function () {
+    let allowed = undefined;
+    const repoName = trimTrailingDotGit(TEST_PUSH_DOT_GIT.repoName);
+
+    await db.createRepo(TEST_REPO_DOT_GIT);
+    try {
+      // push does not exist yet, should return false
+      allowed = await db.canUserApproveRejectPush(TEST_PUSH_DOT_GIT.id, TEST_USER.username);
+      expect(allowed).to.be.false;
+    } catch (e) {
+      expect.fail(e);
+    }
+
+    try {
+      // create the push - user should already exist and not authorised to push
+      await db.writeAudit(TEST_PUSH_DOT_GIT);
+      allowed = await db.canUserApproveRejectPush(TEST_PUSH_DOT_GIT.id, TEST_USER.username);
+      expect(allowed).to.be.false;
+    } catch (e) {
+      expect.fail(e);
+    }
+
+    try {
+      // authorise user and recheck
+      await db.addUserCanAuthorise(repoName, TEST_USER.username);
+      allowed = await db.canUserApproveRejectPush(TEST_PUSH_DOT_GIT.id, TEST_USER.username);
+      expect(allowed).to.be.true;
+    } catch (e) {
+      expect.fail(e);
+    }
+
+    // clean up
+    await db.deletePush(TEST_PUSH_DOT_GIT.id);
+    await db.removeUserCanAuthorise(repoName, TEST_USER.username);
+  });
+
   after(async function () {
     await db.deleteRepo(TEST_REPO.name);
+    await db.deleteRepo(TEST_REPO_DOT_GIT.name);
     await db.deleteUser(TEST_USER.username);
     await db.deletePush(TEST_PUSH.id);
+    await db.deletePush(TEST_PUSH_DOT_GIT.id);
   });
 });