diff --git a/src/db/file/index.ts b/src/db/file/index.ts index 6ac1c2088..18c36a06b 100644 --- a/src/db/file/index.ts +++ b/src/db/file/index.ts @@ -17,6 +17,7 @@ export const { export const { getRepos, getRepo, + getRepoByUrl, createRepo, addUserCanPush, addUserCanAuthorise, diff --git a/src/db/file/repo.ts b/src/db/file/repo.ts index fd7218c15..d07e1a2d0 100644 --- a/src/db/file/repo.ts +++ b/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((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'); diff --git a/src/db/index.ts b/src/db/index.ts index ff1189f1b..19a79b098 100644 --- a/src/db/index.ts +++ b/src/db/index.ts @@ -71,6 +71,7 @@ export const { updateUser, getRepos, getRepo, + getRepoByUrl, createRepo, addUserCanPush, addUserCanAuthorise, diff --git a/src/db/mongo/index.ts b/src/db/mongo/index.ts index a6d7ce6b2..b190bed49 100644 --- a/src/db/mongo/index.ts +++ b/src/db/mongo/index.ts @@ -20,6 +20,7 @@ export const { export const { getRepos, getRepo, + getRepoByUrl, createRepo, addUserCanPush, addUserCanAuthorise, diff --git a/src/db/mongo/repo.ts b/src/db/mongo/repo.ts index f299f907a..ed1c26535 100644 --- a/src/db/mongo/repo.ts +++ b/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)}`); diff --git a/src/proxy/processors/push-action/checkRepoInAuthorisedList.ts b/src/proxy/processors/push-action/checkRepoInAuthorisedList.ts index c3768888b..5f67f4e50 100644 --- a/src/proxy/processors/push-action/checkRepoInAuthorisedList.ts +++ b/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 = getRepos, ): Promise => { 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; }; diff --git a/src/proxy/processors/push-action/checkUserPushPermission.ts b/src/proxy/processors/push-action/checkUserPushPermission.ts index e1b7c7cef..6f378f760 100644 --- a/src/proxy/processors/push-action/checkUserPushPermission.ts +++ b/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 => { + 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 => { 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; }; diff --git a/test/db/file/repo.test.js b/test/db/file/repo.test.js new file mode 100644 index 000000000..d96e37cc4 --- /dev/null +++ b/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'); + } + }); + }); +}); diff --git a/test/db/mongo/repo.test.js b/test/db/mongo/repo.test.js new file mode 100644 index 000000000..187792686 --- /dev/null +++ b/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; + }); + }); +}); diff --git a/test/processors/checkUserPushPermission.test.js b/test/processors/checkUserPushPermission.test.js index b140d383b..a791aa489 100644 --- a/test/processors/checkUserPushPermission.test.js +++ b/test/processors/checkUserPushPermission.test.js @@ -1,7 +1,7 @@ const chai = require('chai'); const sinon = require('sinon'); const proxyquire = require('proxyquire'); -const { Action, Step } = require('../../src/proxy/actions'); +const { Action } = require('../../src/proxy/actions'); chai.should(); const expect = chai.expect; @@ -9,19 +9,16 @@ const expect = chai.expect; describe('checkUserPushPermission', () => { let exec; let getUsersStub; - let isUserPushAllowedStub; - let logStub; + let getRepoByUrl; beforeEach(() => { - logStub = sinon.stub(console, 'log'); - getUsersStub = sinon.stub(); - isUserPushAllowedStub = sinon.stub(); + getRepoByUrl = sinon.stub(); const checkUserPushPermission = proxyquire('../../src/proxy/processors/push-action/checkUserPushPermission', { '../../../db': { getUsers: getUsersStub, - isUserPushAllowed: isUserPushAllowedStub + getRepoByUrl: getRepoByUrl } }); @@ -35,7 +32,6 @@ describe('checkUserPushPermission', () => { describe('exec', () => { let action; let req; - let stepSpy; beforeEach(() => { req = {}; @@ -46,47 +42,78 @@ describe('checkUserPushPermission', () => { 1234567890, 'test/repo.git' ); - action.user = 'git-user'; - stepSpy = sinon.spy(Step.prototype, 'log'); + action.user = 'a-git-user'; }); it('should allow push when user has permission', async () => { - getUsersStub.resolves([{ username: 'db-user', gitAccount: 'git-user' }]); - isUserPushAllowedStub.resolves(true); + getRepoByUrl.resolves({ + name: 'repo', + project: 'test', + url: 'test/repo.git', + users: { + canPush: ['a-user'], canAuthorise: ['zohar'] + } + }); + + getUsersStub.resolves([{ username: 'a-user', gitAccount: 'a-git-user' }]); const result = await exec(req, action); + expect(result.error).to.be.false; expect(result.steps).to.have.lengthOf(1); expect(result.steps[0].error).to.be.false; - expect(stepSpy.calledWith('User db-user is allowed to push on repo test/repo.git')).to.be.true; - expect(logStub.calledWith('User db-user permission on Repo repo : true')).to.be.true; + expect(result.steps[0].logs[0]).to.eq('checkUserPushPermission - User a-user is allowed to push on repo test/repo.git'); }); it('should reject push when user has no permission', async () => { - getUsersStub.resolves([{ username: 'db-user', gitAccount: 'git-user' }]); - isUserPushAllowedStub.resolves(false); + getRepoByUrl.resolves({ + name: 'repo', + project: 'test', + url: 'test/repo.git', + users: { + canPush: ['diff-user'], canAuthorise: ['zohar'] + } + }); + + getUsersStub.resolves([{ username: 'a-user', gitAccount: 'a-git-user' }]); const result = await exec(req, action); - expect(result.steps).to.have.lengthOf(1); + expect(result.error).to.be.true; expect(result.steps[0].error).to.be.true; - expect(stepSpy.calledWith('User db-user is not allowed to push on repo test/repo.git, ending')).to.be.true; - expect(result.steps[0].errorMessage).to.include('Rejecting push as user git-user'); - expect(logStub.calledWith('User not allowed to Push')).to.be.true; + expect(result.steps[0].errorMessage).to.equal('Rejecting push as user a-git-user is not allowed to push on repo test/repo.git'); + expect(result.steps[0].logs[0]).to.eq('checkUserPushPermission - User a-user is not allowed to push on repo test/repo.git, ending'); }); it('should reject push when no user found for git account', async () => { + getRepoByUrl.resolves({ + name: 'repo', + project: 'test', + url: 'test/repo.git', + users: { + canPush: ['diff-user'], canAuthorise: ['zohar'] + } + }); + getUsersStub.resolves([]); const result = await exec(req, action); - expect(result.steps).to.have.lengthOf(1); + expect(result.error).to.be.true; expect(result.steps[0].error).to.be.true; - expect(stepSpy.calledWith('User git-user is not allowed to push on repo test/repo.git, ending')).to.be.true; - expect(result.steps[0].errorMessage).to.include('Rejecting push as user git-user'); + expect(result.steps[0].errorMessage).to.equal('Rejecting push as user a-git-user is not allowed to push on repo test/repo.git'); }); it('should handle multiple users for git account by rejecting push', async () => { + getRepoByUrl.resolves({ + name: 'repo', + project: 'test', + url: 'test/repo.git', + users: { + canPush: ['diff-user'], canAuthorise: ['zohar'] + } + }); + getUsersStub.resolves([ { username: 'user1', gitAccount: 'git-user' }, { username: 'user2', gitAccount: 'git-user' } @@ -94,9 +121,9 @@ describe('checkUserPushPermission', () => { const result = await exec(req, action); - expect(result.steps).to.have.lengthOf(1); + expect(result.error).to.be.true; expect(result.steps[0].error).to.be.true; - expect(logStub.calledWith('Users for this git account: [{"username":"user1","gitAccount":"git-user"},{"username":"user2","gitAccount":"git-user"}]')).to.be.true; + expect(result.steps[0].errorMessage).to.equal('Rejecting push as user a-git-user is not allowed to push on repo test/repo.git'); }); }); }); diff --git a/test/processors/testCheckRepoInAuthList.test.js b/test/processors/testCheckRepoInAuthList.test.js new file mode 100644 index 000000000..f280e844a --- /dev/null +++ b/test/processors/testCheckRepoInAuthList.test.js @@ -0,0 +1,34 @@ +const chai = require('chai'); +const sinon = require('sinon'); +const actions = require('../../src/proxy/actions/Action'); +const processor = require('../../src/proxy/processors/push-action/checkRepoInAuthorisedList'); +const expect = chai.expect; +const db = require('../../src/db'); + +describe('Check a Repo is in the authorised list', async () => { + afterEach(() => { + sinon.restore(); + }); + + it('accepts the action if the repository is whitelisted in the db', async () => { + sinon.stub(db, 'getRepoByUrl').resolves({ + name: 'repo-is-ok', + project: 'thisproject', + url: 'https://github.com/thisproject/repo-is-ok', + }); + + const action = new actions.Action('123', 'type', 'get', 1234, 'thisproject/repo-is-ok'); + const result = await processor.exec(null, action); + expect(result.error).to.be.false; + expect(result.steps[0].logs[0]).to.eq('checkRepoInAuthorisedList - repo thisproject/repo-is-ok is in the authorisedList'); + }); + + it('rejects the action if repository not in the db', async () => { + sinon.stub(db, 'getRepoByUrl').resolves(null); + + const action = new actions.Action('123', 'type', 'get', 1234, 'thisproject/repo-is-not-ok'); + const result = await processor.exec(null, action); + expect(result.error).to.be.true; + expect(result.steps[0].logs[0]).to.eq('checkRepoInAuthorisedList - repo thisproject/repo-is-not-ok is not in the authorised whitelist, ending'); + }); +}); diff --git a/test/testCheckRepoInAuthList.test.js b/test/testCheckRepoInAuthList.test.js deleted file mode 100644 index 19d161c12..000000000 --- a/test/testCheckRepoInAuthList.test.js +++ /dev/null @@ -1,27 +0,0 @@ -const chai = require('chai'); -const actions = require('../src/proxy/actions/Action'); -const processor = require('../src/proxy/processors/push-action/checkRepoInAuthorisedList'); -const expect = chai.expect; - -const authList = () => { - return [ - { - name: 'repo-is-ok', - project: 'thisproject', - }, - ]; -}; - -describe('Check a Repo is in the authorised list', async () => { - it('Should set ok=true if repo in whitelist', async () => { - const action = new actions.Action('123', 'type', 'get', 1234, 'thisproject/repo-is-ok'); - const result = await processor.exec(null, action, authList); - expect(result.error).to.be.false; - }); - - it('Should set ok=false if not in authorised', async () => { - const action = new actions.Action('123', 'type', 'get', 1234, 'thisproject/repo-is-not-ok'); - const result = await processor.exec(null, action, authList); - expect(result.error).to.be.true; - }); -});