fix(auth): the browser-JWT branch now carries username, email and role

backend/__tests__/service/githubBrowserJwtAuth.test.js

@@ -0,0 +1,89 @@
+const request = require('supertest');
+const express = require('express');
+const User = require('../../models/User');
+const githubRoutes = require('../../routes/github');
+const GitHubAppService = require('../../services/githubAppService');
+const auth = require('../../middleware/auth');
+const {
+  setupMongoDb, closeMongoDb, clearMongoDb, generateTestToken,
+} = require('../utils/testUtils');
+
+// Drive the real GitHub route through the real auth middleware and a real
+// projected Mongo row. A route test that replaces auth with a fabricated
+// req.user cannot catch browser JWTs losing role or other identity fields.
+describe('GitHub admin route with a browser JWT', () => {
+  let app;
+  let seq = 0;
+
+  beforeAll(async () => {
+    await setupMongoDb();
+    process.env.JWT_SECRET = 'github-browser-jwt-test-secret';
+    app = express();
+    app.use('/api/github', githubRoutes);
+  });
+
+  afterAll(async () => {
+    await closeMongoDb();
+  });
+
+  afterEach(async () => {
+    await clearMongoDb();
+    jest.restoreAllMocks();
+  });
+
+  const createUser = async (role, overrides = {}) => {
+    seq += 1;
+    return User.create({
+      username: `github-jwt-${seq}`,
+      email: `github-jwt-${seq}@example.com`,
+      password: 'hashed-pass',
+      role,
+      ...overrides,
+    });
+  };
+
+  it('admits a real admin browser session and denies a non-admin', async () => {
+    jest.spyOn(GitHubAppService, 'isPatConfigured').mockReturnValue(true);
+    const admin = await createUser('admin');
+    const member = await createUser('user');
+
+    const adminResponse = await request(app)
+      .get('/api/github/status')
+      .set('Authorization', `Bearer ${generateTestToken(String(admin._id))}`);
+    expect(adminResponse.status).toBe(200);
+    expect(adminResponse.body).toEqual({ mode: 'pat', configured: true });
+
+    const memberResponse = await request(app)
+      .get('/api/github/status')
+      .set('Authorization', `Bearer ${generateTestToken(String(member._id))}`);
+    expect(memberResponse.status).toBe(403);
+    expect(memberResponse.body).toEqual({ message: 'Admin only' });
+  });
+
+  it('carries username, email and role from the live row', async () => {
+    const admin = await createUser('admin');
+    const token = generateTestToken(String(admin._id));
+    const req = { header: (name) => (name === 'Authorization' ? `Bearer ${token}` : undefined) };
+    const res = { status: jest.fn().mockReturnThis(), json: jest.fn() };
+    const next = jest.fn();
+
+    await auth(req, res, next);
+    expect(next).toHaveBeenCalledTimes(1);
+    expect(req.user).toEqual({
+      id: String(admin._id),
+      username: admin.username,
+      email: admin.email,
+      role: 'admin',
+    });
+  });
+
+  it('still refuses an agent row before reaching the GitHub route', async () => {
+    const bot = await createUser('admin', { isBot: true });
+
+    const response = await request(app)
+      .get('/api/github/status')
+      .set('Authorization', `Bearer ${generateTestToken(String(bot._id))}`);
+    expect(response.status).toBe(403);
+    expect(response.body.msg).toMatch(/Agent accounts authenticate/);
+  });
+});

backend/__tests__/unit/middleware/authUserShapeParity.test.js

@@ -0,0 +1,132 @@
+const jwt = require('jsonwebtoken');
+const mongoose = require('mongoose');
+
+// `middleware/auth.ts` dispatches on the token prefix and, until this test,
+// the two branches left DIFFERENT shapes on `req.user`: the `cm_` API-token
+// path assigned `{ id, username, email, role }`, the browser-JWT path assigned
+// `{ id }`. Nothing errored — consumers just read `undefined`:
+//
+//   - `github.ts:146` refused genuine admins with `403 Admin only`
+//   - registry publish/install persisted `publisher.name: undefined` (#1211)
+//
+// The bug is invisible to the suites that cover those routes, because their
+// fake auth middleware injects `{ _id, role }` directly — i.e. the API-token
+// shape. Only a test that drives the real middleware over a real browser JWT
+// can see it, which is why it lives here rather than beside either consumer.
+
+let mockLiveUser = null;
+let mockApiTokenUser = null;
+
+// The mock HONOURS the projection string. A `select()` that ignores its
+// argument and hands back the whole row makes this suite fail open: narrowing
+// the middleware's projection back to `.select('banned')` then leaves all
+// seven tests green, because the fields arrive from the fixture rather than
+// from the query. Verified by mutation — with the naive mock, that narrowing
+// is invisible; with this one it reddens.
+const project = (row, fields) => {
+  if (!row) return row;
+  const keys = String(fields).split(/\s+/).filter(Boolean);
+  return Object.fromEntries(keys.filter((k) => k in row).map((k) => [k, row[k]]));
+};
+
+jest.mock('../../../models/User', () => ({
+  findOne: jest.fn(() => ({
+    select: async (fields) => (
+      mockApiTokenUser && { ...project(mockApiTokenUser, fields), _id: mockApiTokenUser._id }
+    ),
+  })),
+  findById: jest.fn(() => ({
+    select: (fields) => ({ lean: async () => project(mockLiveUser, fields) }),
+  })),
+  updateOne: jest.fn(() => Promise.resolve()),
+}));
+
+const authMiddleware = require('../../../middleware/auth');
+
+const ID = new mongoose.Types.ObjectId();
+
+const runWithJwt = async () => {
+  process.env.JWT_SECRET = 'test-jwt-secret';
+  const token = jwt.sign({ id: ID.toString() }, process.env.JWT_SECRET);
+  const req = { header: (h) => (h === 'Authorization' ? `Bearer ${token}` : null) };
+  const res = { status: jest.fn().mockReturnThis(), json: jest.fn() };
+  const next = jest.fn();
+  await authMiddleware(req, res, next);
+  return { req, res, next };
+};
+
+const runWithApiToken = async () => {
+  const req = { header: (h) => (h === 'Authorization' ? 'Bearer cm_testtoken' : null) };
+  const res = { status: jest.fn().mockReturnThis(), json: jest.fn() };
+  const next = jest.fn();
+  await authMiddleware(req, res, next);
+  return { req, res, next };
+};
+
+describe('auth middleware: both token branches leave the same req.user shape', () => {
+  beforeEach(() => {
+    mockLiveUser = {
+      banned: false, username: 'lily', email: 'lily@example.com', role: 'admin',
+    };
+    mockApiTokenUser = {
+      _id: ID, username: 'lily', email: 'lily@example.com', role: 'admin', banned: false,
+    };
+  });
+
+  it('the browser-JWT branch carries username, email and role', async () => {
+    const { req, next } = await runWithJwt();
+    expect(next).toHaveBeenCalled();
+    expect(req.user).toEqual({
+      id: ID.toString(), username: 'lily', email: 'lily@example.com', role: 'admin',
+    });
+  });
+
+  it('the two branches agree on the key set', async () => {
+    const jwtReq = (await runWithJwt()).req;
+    const apiReq = (await runWithApiToken()).req;
+    // Fails closed in BOTH directions: narrowing the JWT branch reintroduces
+    // the original defect, and widening only the API-token branch reintroduces
+    // it for whatever field gets added next.
+    expect(Object.keys(jwtReq.user).sort()).toEqual(Object.keys(apiReq.user).sort());
+    expect(jwtReq.user).toEqual(apiReq.user);
+  });
+
+  it('the github.ts:146 predicate now admits a browser-session admin', async () => {
+    const { req } = await runWithJwt();
+    // Verbatim the consumer's own test, not a paraphrase of it.
+    expect(req.user?.role !== 'admin').toBe(false);
+  });
+
+  it('CONTROL: the predicate still refuses a browser-session non-admin', async () => {
+    // Without this, the test above would pass just as well against a
+    // middleware that hardcoded `role: 'admin'` for everyone.
+    mockLiveUser = {
+      banned: false, username: 'mallory', email: 'm@example.com', role: 'member',
+    };
+    const { req } = await runWithJwt();
+    expect(req.user?.role !== 'admin').toBe(true);
+  });
+
+  it('a user row without a role leaves role undefined, not defaulted', async () => {
+    // Absence must stay absent: a default here would grant or deny on data the
+    // row does not contain.
+    mockLiveUser = { banned: false, username: 'nobody' };
+    const { req } = await runWithJwt();
+    expect(req.user.role).toBeUndefined();
+    expect(req.user.username).toBe('nobody');
+  });
+
+  it('the ban check still fires on the same widened read', async () => {
+    mockLiveUser = { banned: true, username: 'lily', role: 'admin' };
+    const { res, next } = await runWithJwt();
+    expect(next).not.toHaveBeenCalled();
+    expect(res.status).toHaveBeenCalledWith(403);
+  });
+
+  it('a deleted account is still refused', async () => {
+    mockLiveUser = null;
+    const { res, next } = await runWithJwt();
+    expect(next).not.toHaveBeenCalled();
+    expect(res.status).toHaveBeenCalledWith(401);
+  });
+});

backend/middleware/auth.ts

@@ -128,7 +128,12 @@ export default async function auth(req: Request, res: Response, next: NextFuncti
     }
 
     req.userId = id;
-    req.user = { id };
+    req.user = {
+      id,
+      username: account!.username,
+      email: account!.email,
+      role: account!.role,
+    };
     req.authType = 'jwt';
     touchLastActive(id);
     next();

backend/services/sessionAccountService.ts

@@ -30,10 +30,15 @@ export interface SessionAccount {
   _id?: unknown;
   banned?: boolean;
   isBot?: boolean;
+  username?: string;
+  email?: string;
+  role?: string;
 }
 
 export const loadSessionAccount = async (userId: string): Promise<SessionAccount | null> => {
-  const account = await User.findById(userId).select('banned isBot').lean();
+  // Express auth also needs identity fields for the same req.user shape as
+  // API-token auth. Keep one live-row read for all session verifiers.
+  const account = await User.findById(userId).select('banned isBot username email role').lean();
   return (account as SessionAccount | null) || null;
 };