fix: checkUserPushPermission push failure on name

src/db/file/index.ts

@@ -17,6 +17,7 @@ export const {
 export const {
   getRepos,
   getRepo,
+  getRepoByUrl,
   createRepo,
   addUserCanPush,
   addUserCanAuthorise,

src/db/file/repo.ts

@@ -10,7 +10,8 @@ if (!fs.existsSync('./.data')) fs.mkdirSync('./.data');
 /* istanbul ignore if */
 if (!fs.existsSync('./.data/db')) fs.mkdirSync('./.data/db');
 
-const db = new Datastore({ filename: './.data/db/repos.db', autoload: true });
+// export for testing purposes
+export const db = new Datastore({ filename: './.data/db/repos.db', autoload: true });
 db.ensureIndex({ fieldName: 'name', unique: false });
 db.setAutocompactionInterval(COMPACTION_INTERVAL);
 
@@ -49,6 +50,20 @@ export const getRepo = async (name: string) => {
   });
 };
 
+export const getRepoByUrl = async (url: string) => {
+  return new Promise<Repo | null>((resolve, reject) => {
+    db.findOne({ url: url.toLowerCase() }, (err: Error | null, doc: Repo) => {
+      // ignore for code coverage as neDB rarely returns errors even for an invalid query
+      /* istanbul ignore if */
+      if (err) {
+        reject(err);
+      } else {
+        resolve(doc);
+      }
+    });
+  });
+};
+
 export const createRepo = async (repo: Repo) => {
   if (isBlank(repo.project)) {
     throw new Error('Project name cannot be empty');

src/db/index.ts

@@ -71,6 +71,7 @@ export const {
   updateUser,
   getRepos,
   getRepo,
+  getRepoByUrl,
   createRepo,
   addUserCanPush,
   addUserCanAuthorise,

src/db/mongo/index.ts

@@ -20,6 +20,7 @@ export const {
 export const {
   getRepos,
   getRepo,
+  getRepoByUrl,
   createRepo,
   addUserCanPush,
   addUserCanAuthorise,

src/db/mongo/repo.ts

@@ -18,6 +18,11 @@ export const getRepo = async (name: string) => {
   return collection.findOne({ name: { $eq: name } });
 };
 
+export const getRepoByUrl = async (url: string) => {
+  const collection = await connect(collectionName);
+  return collection.findOne({ url: { $eq: url.toLowerCase() } });
+};
+
 export const createRepo = async (repo: Repo) => {
   repo.name = repo.name.toLowerCase();
   console.log(`creating new repo ${JSON.stringify(repo)}`);

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

@@ -1,40 +1,22 @@
 import { Action, Step } from '../../actions';
-import { getRepos } from '../../../db';
-import { Repo } from '../../../db/types';
-import { trimTrailingDotGit } from '../../../db/helper';
+import { getRepoByUrl } from '../../../db';
 
 // Execute if the repo is approved
 const exec = async (
   req: any,
   action: Action,
-  authorisedList: () => Promise<Repo[]> = getRepos,
 ): Promise<Action> => {
   const step = new Step('checkRepoInAuthorisedList');
 
-  const list = await authorisedList();
-  console.log(list);
-
-  const found = list.find((x: Repo) => {
-    const targetName = trimTrailingDotGit(action.repo.toLowerCase());
-    const allowedName = trimTrailingDotGit(`${x.project}/${x.name}`.toLowerCase());
-    console.log(`${targetName} = ${allowedName}`);
-    return targetName === allowedName;
-  });
-
-  console.log(found);
-
-  if (!found) {
-    console.log('not found');
+  const found = await getRepoByUrl(action.url);
+  if (found) {
+    step.log(`repo ${action.repo} is in the authorisedList`);
+  } else {
     step.error = true;
-    step.log(`repo ${action.repo} is not in the authorisedList, ending`);
-    console.log('setting error');
-    step.setError(`Rejecting repo ${action.repo} not in the authorisedList`);
-    action.addStep(step);
-    return action;
+    step.log(`repo ${action.repo} is not in the authorised whitelist, ending`);
+    step.setError(`Rejecting repo ${action.repo} not in the authorised whitelist`);
   }
 
-  console.log('found');
-  step.log(`repo ${action.repo} is in the authorisedList`);
   action.addStep(step);
   return action;
 };

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

@@ -1,52 +1,31 @@
 import { Action, Step } from '../../actions';
-import { getUsers, isUserPushAllowed } from '../../../db';
-import { trimTrailingDotGit } from '../../../db/helper';
+import { getRepoByUrl, getUsers } from '../../../db';
+
+const getPushUser = async (action: Action): Promise<string | null> => {
+  const list = await getUsers({ gitAccount: action.user });
+  return list.length === 1 ? list[0].username : null;
+};
 
 // Execute if the repo is approved
 const exec = async (req: any, action: Action): Promise<Action> => {
   const step = new Step('checkUserPushPermission');
 
-  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;
-
-  // Find the user associated with this Git Account
-  const list = await getUsers({ gitAccount: action.user });
-
-  console.log(`Users for this git account: ${JSON.stringify(list)}`);
-
-  if (list.length == 1) {
-    user = list[0].username;
-    isUserAllowed = await isUserPushAllowed(repoName, user);
-  }
-
-  console.log(`User ${user} permission on Repo ${repoName} : ${isUserAllowed}`);
-
-  if (!isUserAllowed) {
-    console.log('User not allowed to Push');
-    step.error = true;
-    step.log(`User ${user} is not allowed to push on repo ${action.repo}, ending`);
-
-    console.log('setting error');
-
-    step.setError(
-      `Rejecting push as user ${action.user} ` +
-        `is not allowed to push on repo ` +
-        `${action.repo}`,
-    );
-    action.addStep(step);
-    return action;
+  const repo = await getRepoByUrl(action.url);
+  if (repo) {
+    const user = await getPushUser(action);
+    const isUserAllowed =
+      user && (repo.users.canPush.includes(user) || repo.users.canAuthorise.includes(user));
+    if (isUserAllowed) {
+      step.log(`User ${user} is allowed to push on repo ${action.repo}`);
+    } else {
+      step.error = true;
+      step.log(`User ${user} is not allowed to push on repo ${action.repo}, ending`);
+      step.setError(
+        `Rejecting push as user ${action.user} is not allowed to push on repo ${action.repo}`,
+      );
+    }
   }
 
-  step.log(`User ${user} is allowed to push on repo ${action.repo}`);
   action.addStep(step);
   return action;
 };

test/db/file/repo.test.js

@@ -0,0 +1,94 @@
+const { expect } = require('chai');
+const sinon = require('sinon');
+const repoModule = require('../../../src/db/file/repo');
+
+describe('File DB', () => {
+  let sandbox;
+
+  beforeEach(() => {
+    sandbox = sinon.createSandbox();
+  });
+
+  afterEach(() => {
+    sandbox.restore();
+  });
+
+  describe('getRepo', () => {
+    it('should get the repo using the name', async () => {
+      const repoData = {
+        name: 'sample',
+        users: { canPush: [] },
+        url: 'http://example.com/sample-repo.git',
+      };
+
+      sandbox.stub(repoModule.db, 'findOne').callsFake((query, cb) => cb(null, repoData));
+
+      const result = await repoModule.getRepo('Sample');
+      expect(result).to.equal(repoData);
+    });
+  });
+
+  describe('getRepoByUrl', () => {
+    it('should get the repo using the url', async () => {
+      const repoData = {
+        name: 'sample',
+        users: { canPush: [] },
+        url: 'https://github.com/finos/git-proxy.git',
+      };
+
+      sandbox.stub(repoModule.db, 'findOne').callsFake((query, cb) => cb(null, repoData));
+
+      const result = await repoModule.getRepoByUrl('https://github.com/finos/git-proxy.git');
+      expect(result).to.equal(repoData);
+    });
+
+    it('should get the repo using the url, stripping off the .git', async () => {
+      const repoData = {
+        name: 'sample',
+        users: { canPush: [] },
+        url: 'https://github.com/finos/git-proxy.git',
+      };
+
+      sandbox.stub(repoModule.db, 'findOne').callsFake((query, cb) => cb(null, repoData));
+
+      const result = await repoModule.getRepoByUrl('https://github.com/finos/git-proxy.git');
+
+      expect(repoModule.db.findOne.calledWith(sinon.match({ url: 'https://github.com/finos/git-proxy.git'}))).to.be.true;
+      expect(result).to.equal(repoData);
+    });
+
+    it('should get the repo using the url, ignoring the case', async () => {
+      const repoData = {
+        name: 'sample',
+        users: { canPush: [] },
+        url: 'https://github.com/finos/git-proxy.git',
+      };
+
+      sandbox.stub(repoModule.db, 'findOne').callsFake((query, cb) => cb(null, repoData));
+
+      const result = await repoModule.getRepoByUrl('https://github.com/Finos/Git-Proxy.git');
+      expect(result).to.equal(repoData);
+      expect(repoModule.db.findOne.calledWith(sinon.match({ url: 'https://github.com/finos/git-proxy.git' }))).to.be.true;
+    });
+
+    it('should return null if the repo is not found', async () => {
+      sandbox.stub(repoModule.db, 'findOne').callsFake((query, cb) => cb(null, null));
+
+      const result = await repoModule.getRepoByUrl('https://github.com/finos/missing-repo.git');
+      expect(result).to.be.null;
+      expect(repoModule.db.findOne.calledWith(sinon.match({ url: 'https://github.com/finos/missing-repo.git' })),
+      ).to.be.true;
+    });
+
+    it('should reject if the database returns an error', async () => {
+      sandbox.stub(repoModule.db, 'findOne').callsFake((query, cb) => cb(new Error('DB error')));
+
+      try {
+        await repoModule.getRepoByUrl('https://github.com/finos/git-proxy.git');
+        expect.fail('Expected promise to be rejected');
+      } catch (err) {
+        expect(err.message).to.equal('DB error');
+      }
+    });
+  });
+});

test/db/mongo/repo.test.js

@@ -0,0 +1,69 @@
+const { expect } = require('chai');
+const sinon = require('sinon');
+const proxyqquire = require('proxyquire');
+
+const repoCollection = {
+  findOne: sinon.stub(),
+};
+
+const connectionStub = sinon.stub().returns(repoCollection);
+
+const { getRepo, getRepoByUrl } = proxyqquire('../../../src/db/mongo/repo', {
+  './helper': { connect: connectionStub },
+});
+
+describe('MongoDB', () => {
+  afterEach(function () {
+    sinon.restore();
+  });
+
+  describe('getRepo', () => {
+    it('should get the repo using the name', async () => {
+      const repoData = {
+        name: 'sample',
+        users: { canPush: [] },
+        url: 'http://example.com/sample-repo.git',
+      };
+      repoCollection.findOne.resolves(repoData);
+
+      const result = await getRepo('Sample');
+      expect(result).to.equal(repoData);
+      expect(connectionStub.calledWith('repos')).to.be.true;
+      expect(repoCollection.findOne.calledWith({ name: { $eq: 'sample' } })).to.be.true;
+    });
+  });
+
+  describe('getRepoByUrl', () => {
+    it('should get the repo using the url', async () => {
+      const repoData = {
+        name: 'sample',
+        users: { canPush: [] },
+        url: 'https://github.com/finos/git-proxy.git',
+      };
+      repoCollection.findOne.resolves(repoData);
+
+      const result = await getRepoByUrl('https://github.com/finos/git-proxy.git');
+      expect(result).to.equal(repoData);
+      expect(connectionStub.calledWith('repos')).to.be.true;
+      expect(
+        repoCollection.findOne.calledWith({ url: { $eq: 'https://github.com/finos/git-proxy.git' } }),
+      ).to.be.true;
+    });
+
+    it('should get the repo using the url, ignoring the case', async () => {
+      const repoData = {
+        name: 'sample',
+        users: { canPush: [] },
+        url: 'https://github.com/finos/git-proxy.git',
+      };
+      repoCollection.findOne.resolves(repoData);
+
+      const result = await getRepoByUrl('https://github.com/Finos/Git-Proxy.git');
+      expect(result).to.equal(repoData);
+      expect(connectionStub.calledWith('repos')).to.be.true;
+      expect(
+        repoCollection.findOne.calledWith({ url: { $eq: 'https://github.com/finos/git-proxy.git' } }),
+      ).to.be.true;
+    });
+  });
+});