test: improve auth test coverage

src/service/passport/activeDirectory.js

@@ -43,8 +43,9 @@
               const message = `User it not a member of ${userGroup}`;
               return done(message, null);
             }
-          } catch (e) {
-            const message = `An error occurred while checking if the user is a member of the user group: ${JSON.stringify(e)}`;
+          } catch (err) {
+            console.log('ad test (isUser): e', err);
+            const message = `An error occurred while checking if the user is a member of the user group: ${err.message}`;
             return done(message, null);
           }
 
@@ -53,9 +54,9 @@
           try {
             isAdmin = await ldaphelper.isUserInAdGroup(req, profile, ad, domain, adminGroup);
 
-          } catch (e) {
-            const message = `An error occurred while checking if the user is a member of the admin group: ${JSON.stringify(e)}`;
-            console.error(message, e); // don't return an error for this case as you may still be a user
+          } catch (err) {
+            const message = `An error occurred while checking if the user is a member of the admin group: ${err.message}`;
+            console.error(message, err); // don't return an error for this case as you may still be a user
           }
 
           profile.admin = isAdmin;

src/service/routes/auth.js

@@ -66,29 +66,6 @@ router.get('/oidc/callback', (req, res, next) => {
   })(req, res, next);
 });
 
-// when login is successful, retrieve user info
-router.get('/success', (req, res) => {
-  console.log('authenticated' + JSON.stringify(req.user));
-  if (req.user) {
-    res.json({
-      success: true,
-      message: 'user has successfully authenticated',
-      user: req.user,
-      cookies: req.cookies,
-    });
-  } else {
-    res.status(401).end();
-  }
-});
-
-// when login failed, send failed msg
-router.get('failed', (req, res) => {
-  res.status(401).json({
-    success: false,
-    message: 'user failed to authenticate.',
-  });
-});
-
 router.post('/logout', (req, res, next) => {
   req.logout(req.user, (err) => {
     if (err) return next(err);
@@ -110,24 +87,28 @@ router.get('/profile', async (req, res) => {
 router.post('/gitAccount', async (req, res) => {
   if (req.user) {
     try {
-      let login =
+      let username =
         req.body.username == null || req.body.username == 'undefined'
           ? req.body.id
           : req.body.username;
+      username = username?.split('@')[0];
 
-      login = login.split('@')[0];
+      if (!username) {
+        res.status(400).send('Error: Missing username. Git account not updated').end();
+        return;
+      }
 
-      const user = await db.findUser(login);
+      const user = await db.findUser(username);
 
       console.log('Adding gitAccount' + req.body.gitAccount);
       user.gitAccount = req.body.gitAccount;
       db.updateUser(user);
       res.status(200).end();
-    } catch {
+    } catch (e) {
       res
         .status(500)
         .send({
-          message: 'An error occurred',
+          message: `Error updating git account: ${e.message}`,
         })
         .end();
     }

src/ui/services/git-push.js

@@ -9,21 +9,6 @@ const config = {
   withCredentials: true,
 };
 
-const getUser = async (setIsLoading, setData, setAuth, setIsError) => {
-  const url = new URL(`${location.origin}/api/auth/success`);
-  await axios(url.toString(), config)
-    .then((response) => {
-      const data = response.data;
-      setData(data);
-      setIsLoading(false);
-    })
-    .catch((error) => {
-      if (error.response && error.response.status === 401) setAuth(false);
-      else setIsError(true);
-      setIsLoading(false);
-    });
-};
-
 const getPush = async (id, setIsLoading, setData, setAuth, setIsError) => {
   const url = `${baseUrl}/push/${id}`;
   await axios(url, config)
@@ -126,4 +111,4 @@ const cancelPush = async (id, setAuth, setIsError) => {
     });
 };
 
-export { getPush, getPushes, authorisePush, rejectPush, cancelPush, getUser };
+export { getPush, getPushes, authorisePush, rejectPush, cancelPush };

test/testActiveDirectoryAuth.test.js

@@ -0,0 +1,150 @@
+const chai = require('chai');
+const sinon = require('sinon');
+const proxyquire = require('proxyquire');
+const expect = chai.expect;
+
+describe('ActiveDirectory auth method', () => {
+  let ldapStub;
+  let dbStub;
+  let passportStub;
+  let strategyCallback;
+
+  const newConfig = JSON.stringify({
+    authentication: [
+      {
+        type: 'ActiveDirectory',
+        enabled: true,
+        adminGroup: 'test-admin-group',
+        userGroup: 'test-user-group',
+        domain: 'test.com',
+        adConfig: {
+          url: 'ldap://test-url',
+          baseDN: 'dc=test,dc=com',
+          searchBase: 'ou=users,dc=test,dc=com',
+        },
+      },
+    ],
+  });
+
+  beforeEach(() => {
+    ldapStub = {
+      isUserInAdGroup: sinon.stub(),
+    };
+
+    dbStub = {
+      updateUser: sinon.stub(),
+    };
+
+    passportStub = {
+      use: sinon.stub(),
+      serializeUser: sinon.stub(),
+      deserializeUser: sinon.stub(),
+    };
+
+    const fsStub = {
+      existsSync: sinon.stub().returns(true),
+      readFileSync: sinon.stub().returns(newConfig),
+    };
+
+    const config = proxyquire('../src/config', {
+      fs: fsStub,
+    });
+
+    const { configure } = proxyquire('../src/service/passport/activeDirectory', {
+      './ldaphelper': ldapStub,
+      '../../db': dbStub,
+      '../../config': config,
+      'passport-activedirectory': function (options, callback) {
+        strategyCallback = callback;
+        return {
+          name: 'ActiveDirectory',
+          authenticate: () => {},
+        };
+      },
+    });
+
+    configure(passportStub);
+  });
+
+  it('should authenticate a valid user and mark them as admin', async () => {
+    const mockReq = {};
+    const mockProfile = {
+      _json: {
+        sAMAccountName: 'test-user',
+        mail: 'test@test.com',
+        userPrincipalName: 'test@test.com',
+        title: 'Test User',
+      },
+      displayName: 'Test User',
+    };
+
+    ldapStub.isUserInAdGroup
+      .onCall(0).resolves(true)
+      .onCall(1).resolves(true);
+
+    const done = sinon.spy();
+
+    await strategyCallback(mockReq, mockProfile, {}, done);
+
+    expect(done.calledOnce).to.be.true;
+    const [err, user] = done.firstCall.args;
+    expect(err).to.be.null;
+    expect(user).to.have.property('username', 'test-user');
+    expect(user).to.have.property('email', 'test@test.com');
+    expect(user).to.have.property('displayName', 'Test User');
+    expect(user).to.have.property('admin', true);
+    expect(user).to.have.property('title', 'Test User');
+
+    expect(dbStub.updateUser.calledOnce).to.be.true;
+  });
+
+  it('should fail if user is not in user group', async () => {
+    const mockReq = {};
+    const mockProfile = {
+      _json: {
+        sAMAccountName: 'bad-user',
+        mail: 'bad@test.com',
+        userPrincipalName: 'bad@test.com',
+        title: 'Bad User'
+      },
+      displayName: 'Bad User'
+    };
+
+    ldapStub.isUserInAdGroup.onCall(0).resolves(false);
+
+    const done = sinon.spy();
+
+    await strategyCallback(mockReq, mockProfile, {}, done);
+
+    expect(done.calledOnce).to.be.true;
+    const [err, user] = done.firstCall.args;
+    expect(err).to.include('not a member');
+    expect(user).to.be.null;
+
+    expect(dbStub.updateUser.notCalled).to.be.true;
+  });
+
+  it('should handle LDAP errors gracefully', async () => {
+    const mockReq = {};
+    const mockProfile = {
+      _json: {
+        sAMAccountName: 'error-user',
+        mail: 'err@test.com',
+        userPrincipalName: 'err@test.com',
+        title: 'Whoops'
+      },
+      displayName: 'Error User'
+    };
+
+    ldapStub.isUserInAdGroup.rejects(new Error('LDAP error'));
+
+    const done = sinon.spy();
+
+    await strategyCallback(mockReq, mockProfile, {}, done);
+
+    expect(done.calledOnce).to.be.true;
+    const [err, user] = done.firstCall.args;
+    expect(err).to.contain('LDAP error');
+    expect(user).to.be.null;
+  });
+});

test/testLogin.test.js

@@ -52,6 +52,27 @@ describe('auth', async () => {
       res.should.have.status(200);
     });
 
+    it('should be able to set the git account', async function () {
+      console.log(`cookie: ${cookie}`);
+      const res = await chai.request(app).post('/api/auth/gitAccount')
+        .set('Cookie', `${cookie}`)
+        .send({
+          username: 'admin',
+          gitAccount: 'new-account',
+        });
+      res.should.have.status(200);
+    });
+
+    it('should throw an error if the username is not provided when setting the git account', async function () {
+      const res = await chai.request(app).post('/api/auth/gitAccount')
+        .set('Cookie', `${cookie}`)
+        .send({
+          gitAccount: 'new-account',
+        });
+      console.log(`res: ${JSON.stringify(res)}`);
+      res.should.have.status(400);
+    });
+
     it('should now be able to logout', async function () {
       const res = await chai.request(app).post('/api/auth/logout').set('Cookie', `${cookie}`);
       res.should.have.status(200);
@@ -78,6 +99,27 @@ describe('auth', async () => {
       });
       res.should.have.status(401);
     });
+
+    it('should fail to set the git account if the user is not logged in', async function () {
+      const res = await chai.request(app).post('/api/auth/gitAccount').send({
+        username: 'admin',
+        gitAccount: 'new-account',
+      });
+      res.should.have.status(401);
+    });
+
+    it('should fail to get the current user metadata if not logged in', async function () {
+      const res = await chai.request(app).get('/api/auth/me');
+      res.should.have.status(401);
+    });
+
+    it('should fail to login with invalid credentials', async function () {
+      const res = await chai.request(app).post('/api/auth/login').send({
+        username: 'admin',
+        password: 'invalid',
+      });
+      res.should.have.status(401);
+    });
   });
 
   after(async function () {