feat: enforce SSH key uniqueness to prevent duplicate keys across users

Author: fabiovincenzi

src/db/file/users.ts

@@ -2,6 +2,7 @@ import fs from 'fs';
 import Datastore from '@seald-io/nedb';
 
 import { User, UserQuery } from '../types';
+import { DuplicateSSHKeyError, UserNotFoundError } from '../../errors/DatabaseErrors';
 
 const COMPACTION_INTERVAL = 1000 * 60 * 60 * 24; // once per day
 
@@ -182,10 +183,20 @@ export const getUsers = (query: Partial<UserQuery> = {}): Promise<User[]> => {
 
 export const addPublicKey = (username: string, publicKey: string): Promise<void> => {
   return new Promise((resolve, reject) => {
-    findUser(username)
+    // Check if this key already exists for any user
+    findUserBySSHKey(publicKey)
+      .then((existingUser) => {
+        if (existingUser && existingUser.username.toLowerCase() !== username.toLowerCase()) {
+          reject(new DuplicateSSHKeyError(existingUser.username));
+          return;
+        }
+
+        // Key doesn't exist for other users
+        return findUser(username);
+      })
       .then((user) => {
         if (!user) {
-          reject(new Error('User not found'));
+          reject(new UserNotFoundError(username));
           return;
         }
         if (!user.publicKeys) {

src/db/mongo/users.ts

@@ -3,6 +3,7 @@ import { toClass } from '../helper';
 import { User } from '../types';
 import { connect } from './helper';
 import _ from 'lodash';
+import { DuplicateSSHKeyError } from '../../errors/DatabaseErrors';
 const collectionName = 'users';
 
 export const findUser = async function (username: string): Promise<User | null> {
@@ -71,6 +72,14 @@ export const updateUser = async (user: Partial<User>): Promise<void> => {
 };
 
 export const addPublicKey = async (username: string, publicKey: string): Promise<void> => {
+  // Check if this key already exists for any user
+  const existingUser = await findUserBySSHKey(publicKey);
+
+  if (existingUser && existingUser.username.toLowerCase() !== username.toLowerCase()) {
+    throw new DuplicateSSHKeyError(existingUser.username);
+  }
+
+  // Key doesn't exist for other users
   const collection = await connect(collectionName);
   await collection.updateOne(
     { username: username.toLowerCase() },

src/errors/DatabaseErrors.ts

@@ -0,0 +1,26 @@
+/**
+ * Custom error classes for database operations
+ * These provide type-safe error handling and better maintainability
+ */
+
+/**
+ * Thrown when attempting to add an SSH key that is already in use by another user
+ */
+export class DuplicateSSHKeyError extends Error {
+  constructor(public readonly existingUsername: string) {
+    super(`SSH key already in use by user '${existingUsername}'`);
+    this.name = 'DuplicateSSHKeyError';
+    Object.setPrototypeOf(this, DuplicateSSHKeyError.prototype);
+  }
+}
+
+/**
+ * Thrown when a user is not found in the database
+ */
+export class UserNotFoundError extends Error {
+  constructor(public readonly username: string) {
+    super(`User not found`);
+    this.name = 'UserNotFoundError';
+    Object.setPrototypeOf(this, UserNotFoundError.prototype);
+  }
+}

src/service/routes/users.ts

@@ -3,6 +3,7 @@ import { utils } from 'ssh2';
 
 import * as db from '../../db';
 import { toPublicUser } from './publicApi';
+import { DuplicateSSHKeyError, UserNotFoundError } from '../../errors/DatabaseErrors';
 
 const router = express.Router();
 const parseKey = utils.parseKey;
@@ -65,7 +66,19 @@ router.post('/:username/ssh-keys', async (req: Request, res: Response) => {
     res.status(201).json({ message: 'SSH key added successfully' });
   } catch (error) {
     console.error('Error adding SSH key:', error);
-    res.status(500).json({ error: 'Failed to add SSH key' });
+
+    if (error instanceof DuplicateSSHKeyError) {
+      res.status(409).json({ error: error.message });
+      return;
+    }
+
+    if (error instanceof UserNotFoundError) {
+      res.status(404).json({ error: error.message });
+      return;
+    }
+
+    const errorMessage = error instanceof Error ? error.message : 'Unknown error';
+    res.status(500).json({ error: `Failed to add SSH key: ${errorMessage}` });
   }
 });
 

test/services/routes/users.test.js

@@ -2,8 +2,11 @@ const chai = require('chai');
 const chaiHttp = require('chai-http');
 const sinon = require('sinon');
 const express = require('express');
+const fs = require('fs');
+const path = require('path');
 const usersRouter = require('../../../src/service/routes/users').default;
 const db = require('../../../src/db');
+const { DuplicateSSHKeyError, UserNotFoundError } = require('../../../src/errors/DatabaseErrors');
 
 const { expect } = chai;
 chai.use(chaiHttp);
@@ -64,4 +67,89 @@ describe('Users API', function () {
       admin: false,
     });
   });
+
+  describe('POST /users/:username/ssh-keys', function () {
+    let authenticatedApp;
+    const validPublicKey = fs
+      .readFileSync(path.join(__dirname, '../../.ssh/host_key.pub'), 'utf8')
+      .trim();
+
+    before(function () {
+      authenticatedApp = express();
+      authenticatedApp.use(express.json());
+      authenticatedApp.use((req, res, next) => {
+        req.user = { username: 'alice', admin: true };
+        next();
+      });
+      authenticatedApp.use('/users', usersRouter);
+    });
+
+    it('should return 409 when SSH key is already used by another user', async function () {
+      const publicKey = validPublicKey;
+
+      sinon.stub(db, 'addPublicKey').rejects(new DuplicateSSHKeyError('bob'));
+
+      const res = await chai
+        .request(authenticatedApp)
+        .post('/users/alice/ssh-keys')
+        .send({ publicKey });
+
+      expect(res).to.have.status(409);
+      expect(res.body).to.have.property('error');
+      expect(res.body.error).to.include("already in use by user 'bob'");
+    });
+
+    it('should return 404 when user not found', async function () {
+      const publicKey = validPublicKey;
+
+      sinon.stub(db, 'addPublicKey').rejects(new UserNotFoundError('nonexistent'));
+
+      const res = await chai
+        .request(authenticatedApp)
+        .post('/users/nonexistent/ssh-keys')
+        .send({ publicKey });
+
+      expect(res).to.have.status(404);
+      expect(res.body).to.have.property('error');
+      expect(res.body.error).to.include('User not found');
+    });
+
+    it('should return 201 when SSH key is added successfully', async function () {
+      const publicKey = validPublicKey;
+
+      sinon.stub(db, 'addPublicKey').resolves();
+
+      const res = await chai
+        .request(authenticatedApp)
+        .post('/users/alice/ssh-keys')
+        .send({ publicKey });
+
+      expect(res).to.have.status(201);
+      expect(res.body).to.have.property('message');
+      expect(res.body.message).to.equal('SSH key added successfully');
+    });
+
+    it('should return 400 when public key is missing', async function () {
+      const res = await chai.request(authenticatedApp).post('/users/alice/ssh-keys').send({});
+
+      expect(res).to.have.status(400);
+      expect(res.body).to.have.property('error');
+      expect(res.body.error).to.include('Public key is required');
+    });
+
+    it('should return 500 for unexpected errors', async function () {
+      const publicKey = validPublicKey;
+
+      sinon.stub(db, 'addPublicKey').rejects(new Error('Database connection failed'));
+
+      const res = await chai
+        .request(authenticatedApp)
+        .post('/users/alice/ssh-keys')
+        .send({ publicKey });
+
+      expect(res).to.have.status(500);
+      expect(res.body).to.have.property('error');
+      expect(res.body.error).to.include('Failed to add SSH key');
+    });
+  });
 });

test/testDb.test.js

@@ -574,6 +574,100 @@ describe('Database clients', async () => {
     // leave user in place for next test(s)
   });
 
+  it('should be able to add a public SSH key to a user', async function () {
+    const testKey = 'ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQC [email protected]';
+
+    await db.addPublicKey(TEST_USER.username, testKey);
+
+    const user = await db.findUser(TEST_USER.username);
+    expect(user.publicKeys).to.include(testKey);
+  });
+
+  it('should not add duplicate SSH key to same user', async function () {
+    const testKey = 'ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQC [email protected]';
+
+    // Add same key again - should not throw error but also not duplicate
+    await db.addPublicKey(TEST_USER.username, testKey);
+
+    const user = await db.findUser(TEST_USER.username);
+    const keyCount = user.publicKeys.filter((k) => k === testKey).length;
+    expect(keyCount).to.equal(1);
+  });
+
+  it('should throw DuplicateSSHKeyError when adding key already used by another user', async function () {
+    const testKey = 'ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQC [email protected]';
+    const otherUser = {
+      username: 'other-user',
+      password: 'password',
+      email: '[email protected]',
+      gitAccount: 'other-git',
+      admin: false,
+      publicKeys: [],
+    };
+
+    // Create another user
+    await db.createUser(
+      otherUser.username,
+      otherUser.password,
+      otherUser.email,
+      otherUser.gitAccount,
+      otherUser.admin,
+    );
+
+    let threwError = false;
+    let errorType = null;
+    try {
+      // Try to add the same key to another user
+      await db.addPublicKey(otherUser.username, testKey);
+    } catch (e) {
+      threwError = true;
+      errorType = e.constructor.name;
+    }
+
+    expect(threwError).to.be.true;
+    expect(errorType).to.equal('DuplicateSSHKeyError');
+
+    // Cleanup
+    await db.deleteUser(otherUser.username);
+  });
+
+  it('should be able to find user by SSH key', async function () {
+    const testKey = 'ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQC [email protected]';
+
+    const user = await db.findUserBySSHKey(testKey);
+    expect(user).to.not.be.null;
+    expect(user.username).to.equal(TEST_USER.username);
+  });
+
+  it('should return null when finding user by non-existent SSH key', async function () {
+    const nonExistentKey = 'ssh-rsa NONEXISTENT';
+
+    const user = await db.findUserBySSHKey(nonExistentKey);
+    expect(user).to.be.null;
+  });
+
+  it('should be able to remove a public SSH key from a user', async function () {
+    const testKey = 'ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQC [email protected]';
+
+    await db.removePublicKey(TEST_USER.username, testKey);
+
+    const user = await db.findUser(TEST_USER.username);
+    expect(user.publicKeys).to.not.include(testKey);
+  });
+
+  it('should not throw error when removing non-existent SSH key', async function () {
+    const nonExistentKey = 'ssh-rsa NONEXISTENT';
+
+    let threwError = false;
+    try {
+      await db.removePublicKey(TEST_USER.username, nonExistentKey);
+    } catch (e) {
+      threwError = true;
+    }
+
+    expect(threwError).to.be.false;
+  });
+
   it('should throw an error when authorising a user to push on non-existent repo', async function () {
     let threwError = false;
     try {