diff --git a/app/controllers/team.controller.js b/app/controllers/team.controller.js index 6cf5898..97c8590 100644 --- a/app/controllers/team.controller.js +++ b/app/controllers/team.controller.js @@ -6,6 +6,8 @@ const { ConflictError, NotFoundError, ForbiddenError, handleError, } = require('../exceptions/errors.js'); const authMiddleware = require('../utils/auth-middleware.js'); +const validationUtils = require('../utils/validation-utils.js'); +const { getCoreAdminUsername } = require('../utils/admin-seed-service.js'); const log = debug('team:controller'); @@ -153,14 +155,6 @@ exports.delete = (req, res) => { */ const MEMBER_FIELDS = '_id username role authProvider'; -/** - * Team leads manage plain users only. An admin's access is global, and a team lead's - * `team_lead` role is what grants lead powers over every team they belong to - so letting - * a lead add or remove another lead would let them hand out (or take away) lead access. - * Those memberships stay with admins. - */ -const canManageMembership = (caller, targetUser) => caller.role === 'admin' || targetUser.role === 'user'; - exports.getMembers = async (req, res) => { const errors = validationResult(req); if (!errors.isEmpty()) { @@ -177,7 +171,11 @@ exports.getMembers = async (req, res) => { throw new NotFoundError(`Team not found with id ${teamId}`); } const members = await User.find({ teams: teamId }, MEMBER_FIELDS).sort('username').lean(); - return res.status(200).send(members); + // Flag the core admin so a client can show it as not removable by a team lead. + const coreAdmin = getCoreAdminUsername(); + return res.status(200).send(members.map((member) => ({ + ...member, isCoreAdmin: member.username === coreAdmin, + }))); } catch (err) { return handleError(err, res); } @@ -203,8 +201,8 @@ exports.addMember = async (req, res) => { if (!user) { throw new NotFoundError(`User "${username}" not found.`); } - if (!canManageMembership(req.user, user)) { - throw new ForbiddenError('Forbidden. Only an admin can add team leads or admins to a team.'); + if (req.user.role !== 'admin' && user.username === getCoreAdminUsername()) { + throw new ForbiddenError('Forbidden. The core admin account cannot be added to a team by a team lead.'); } if (user.teams.some((id) => id.toString() === teamId)) { throw new ConflictError(`User "${username}" is already a member of this team.`); @@ -243,8 +241,8 @@ exports.removeMember = async (req, res) => { if (req.user.role !== 'admin' && req.user._id.toString() === userId) { throw new ForbiddenError('Forbidden. You cannot remove yourself from a team you lead; ask an admin.'); } - if (!canManageMembership(req.user, user)) { - throw new ForbiddenError('Forbidden. Only an admin can remove team leads or admins from a team.'); + if (req.user.role !== 'admin' && user.username === getCoreAdminUsername()) { + throw new ForbiddenError('Forbidden. The core admin account cannot be removed from a team by a team lead.'); } await User.updateOne({ _id: user._id }, { $pull: { teams: teamId } }); log(`Removed user "${user.username}" from team "${team.name}" (${teamId}).`); @@ -262,9 +260,9 @@ exports.searchMemberCandidates = async (req, res) => { return res.status(422).json({ errors: errors.array() }); } const { teamId } = req.params; - // The validator limits q to [A-Za-z0-9-_], none of which is a regex metacharacter, so it - // can go into the pattern as is. Usernames are stored lowercase. - const term = req.query.q.trim().toLowerCase(); + // Usernames are stored lowercase. The term can contain regex metacharacters (. + in an + // email address), so it is escaped and matched literally. + const term = validationUtils.escapeRegex(req.query.q.trim().toLowerCase()); try { if (!authMiddleware.hasTeamLeadAccess(req.user, teamId)) { @@ -274,14 +272,12 @@ exports.searchMemberCandidates = async (req, res) => { if (!team) { throw new NotFoundError(`Team not found with id ${teamId}`); } + // Everyone except people already in the team and the core admin, whatever their role or + // how they sign in (local, LDAP, SSO). const filter = { - username: new RegExp(term), + username: { $regex: term, $ne: getCoreAdminUsername() }, teams: { $ne: teamId }, }; - // Same rule as addMember: only an admin can add team leads or admins. - if (req.user.role !== 'admin') { - filter.role = 'user'; - } const candidates = await User.find(filter, '_id username role') .sort('username') .limit(CANDIDATE_LIMIT) diff --git a/app/routes/team.routes.js b/app/routes/team.routes.js index 9765dd7..54c7c8f 100644 --- a/app/routes/team.routes.js +++ b/app/routes/team.routes.js @@ -2,6 +2,11 @@ const { check, param, query } = require('express-validator'); const teamController = require('../controllers/team.controller.js'); const authMiddleware = require('../utils/auth-middleware.js'); +// What a username can contain when it is looked up for team membership. Local accounts are +// letters, digits, hyphens and underscores, but accounts created by a single sign-on login +// take the identifier the provider reports, which is often an email address. +const USERNAME_CHARACTERS = /^[A-Za-z0-9._@+-]+$/; + module.exports = (app, path) => { app.post(`${path}/team`, authMiddleware.authorizeAdmin, [ check('name') @@ -54,18 +59,18 @@ module.exports = (app, path) => { // Predictive search for the "add member" box. Deliberately narrow, because it lets a team // lead look up accounts they otherwise cannot list: it needs at least three characters of - // the username, returns a handful of matches, and (for a team lead) only users they could - // add anyway - never admins, team leads, or people already in the team. + // the username and returns a handful of matches, never the core admin account or people + // already in the team. app.get(`${path}/team/:teamId/members/candidates`, authMiddleware.preventTokenAuth, [ param('teamId').isMongoId(), query('q') .exists({ checkFalsy: true }) .isString() .trim() - .isLength({ min: 3, max: 50 }) - .withMessage('q must be between 3 and 50 characters.') - .matches(/^[A-Za-z0-9-_]+$/) - .withMessage('q must only contain letters, numbers, hyphens or underscores.'), + .isLength({ min: 3, max: 254 }) + .withMessage('q must be between 3 and 254 characters.') + .matches(USERNAME_CHARACTERS) + .withMessage('q must only contain letters, numbers and . _ @ + - characters.'), ], teamController.searchMemberCandidates); app.post(`${path}/team/:teamId/members`, authMiddleware.preventTokenAuth, [ @@ -73,8 +78,9 @@ module.exports = (app, path) => { check('username') .exists({ checkFalsy: true }) .isString() - .matches(/^[A-Za-z0-9-_]{2,50}$/) - .withMessage('Username must only contain letters, numbers, hyphens or underscores (2–50 characters).'), + .isLength({ min: 2, max: 254 }) + .matches(USERNAME_CHARACTERS) + .withMessage('Username must be 2 to 254 characters: letters, numbers and . _ @ + - only.'), ], teamController.addMember); app.delete(`${path}/team/:teamId/members/:userId`, authMiddleware.preventTokenAuth, [ diff --git a/app/utils/admin-seed-service.js b/app/utils/admin-seed-service.js index 947ba38..2e1b83f 100644 --- a/app/utils/admin-seed-service.js +++ b/app/utils/admin-seed-service.js @@ -20,9 +20,17 @@ const BCRYPT_ROUNDS = 10; * 'no-password' (env var unset), 'weak-password' (fails the strength policy, with the unmet * requirements in violations), or 'exists' (admin already present). */ +/** + * Username of the core admin: the account seeded at first run (ANGLES_ADMIN_USERNAME, + * default 'admin'). It is the deployment's break-glass account, so it is kept out of the + * team-membership features that team leads can use. + * @returns {string} + */ +const getCoreAdminUsername = () => (process.env.ANGLES_ADMIN_USERNAME || 'admin').toLowerCase(); + const ensureAdminUser = async () => { const password = process.env.ANGLES_ADMIN_PASSWORD; - const username = (process.env.ANGLES_ADMIN_USERNAME || 'admin').toLowerCase(); + const username = getCoreAdminUsername(); if (!password) { log('ANGLES_ADMIN_PASSWORD not set; skipping admin seed.'); @@ -56,4 +64,5 @@ const ensureAdminUser = async () => { module.exports = { ensureAdminUser, + getCoreAdminUsername, }; diff --git a/swagger/swagger.json b/swagger/swagger.json index 428c5ea..722c942 100644 --- a/swagger/swagger.json +++ b/swagger/swagger.json @@ -412,7 +412,7 @@ "Team" ], "summary": "Add a user to a team", - "description": "Session authentication only - API tokens (x-api-key) are rejected on this endpoint with a 403. Callable by an admin or a team lead of this team. A team lead can only add users with the 'user' role; adding team leads or admins is admin-only.", + "description": "Session authentication only - API tokens (x-api-key) are rejected on this endpoint with a 403. Callable by an admin or a team lead of this team. A team lead can add any existing user except the core admin account (the seeded ANGLES_ADMIN_USERNAME account); an admin can add anyone. The username is matched case-insensitively and can be an email address.", "requestBody": { "required": true, "content": { @@ -424,7 +424,7 @@ "properties": { "username": { "type": "string", - "description": "Username of the existing user to add (case-insensitive)." + "description": "Username of the existing user to add (case-insensitive). Letters, numbers and . _ @ + - characters, 2 to 254 characters, so email-style usernames work." } } } @@ -443,7 +443,7 @@ } }, "403": { - "description": "Caller may not manage this team, or may not add a user with that role", + "description": "Caller may not manage this team, or a team lead tried to add the core admin account", "content": { "application/json": { "schema": { @@ -501,17 +501,17 @@ "Team" ], "summary": "Search for users that can be added to a team", - "description": "Session authentication only - API tokens (x-api-key) are rejected on this endpoint with a 403. Callable by an admin or a team lead of this team. Powers the type-ahead on the add-member box. Matches part of the username (case-insensitive), returns at most 10 users ordered by username, and leaves out users who are already in the team. For a team lead only users with the 'user' role are returned, since those are the only ones a lead can add; an admin also sees team leads and admins.", + "description": "Session authentication only - API tokens (x-api-key) are rejected on this endpoint with a 403. Callable by an admin or a team lead of this team. Powers the type-ahead on the add-member box. Matches part of the username (case-insensitive, literal - characters such as . and + are not wildcards), returns at most 10 users ordered by username, and leaves out users who are already in the team and the core admin account. Every other user is offered, whatever their role or whether they sign in locally, with LDAP or with SSO.", "parameters": [ { "name": "q", "in": "query", "required": true, - "description": "Part of the username: 3 to 50 letters, numbers, hyphens or underscores.", + "description": "Part of the username: 3 to 254 letters, numbers and . _ @ + - characters (email-style usernames are supported).", "schema": { "type": "string", "minLength": 3, - "maxLength": 50 + "maxLength": 254 } } ], @@ -601,7 +601,7 @@ "Team" ], "summary": "Remove a user from a team", - "description": "Session authentication only - API tokens (x-api-key) are rejected on this endpoint with a 403. Callable by an admin or a team lead of this team. A team lead can only remove users with the 'user' role, and cannot remove themselves; removing team leads or admins is admin-only. The user's membership of other teams is unaffected.", + "description": "Session authentication only - API tokens (x-api-key) are rejected on this endpoint with a 403. Callable by an admin or a team lead of this team. A team lead can remove any member except themselves and the core admin account; an admin can remove anyone. The user's membership of other teams is unaffected.", "responses": { "200": { "description": "The user was removed from the team", @@ -614,7 +614,7 @@ } }, "403": { - "description": "Caller may not manage this team, or may not remove that user", + "description": "Caller may not manage this team, or a team lead tried to remove themselves or the core admin account", "content": { "application/json": { "schema": { @@ -10038,6 +10038,10 @@ "authProvider": { "type": "string", "description": "'local' or the id of the SSO provider that authenticates the user." + }, + "isCoreAdmin": { + "type": "boolean", + "description": "Present in the member list: true for the core admin account, which a team lead cannot remove from a team." } } } diff --git a/test/team-members.tests.js b/test/team-members.tests.js index 8ae9ee7..3b39396 100644 --- a/test/team-members.tests.js +++ b/test/team-members.tests.js @@ -29,6 +29,10 @@ describe('Team members, last login and build trigger Tests', () => { let candidate; let teamLead2; let outsider; + let coreAdmin; + let localAdmin; + let ssoUser; + let previousCoreAdminName; let leadToken; const login = (username) => new Promise((resolve, reject) => { @@ -71,6 +75,16 @@ describe('Team members, last login and build trigger Tests', () => { candidate = await createUser('candidate', 'user'); teamLead2 = await createUser('lead-two', 'team_lead', [team._id]); outsider = await createUser('outsider', 'user'); + // The core admin is the seeded account (ANGLES_ADMIN_USERNAME); point it at a test account + // so the suite does not depend on an 'admin' user existing. + previousCoreAdminName = process.env.ANGLES_ADMIN_USERNAME; + process.env.ANGLES_ADMIN_USERNAME = `${PREFIX}-core`; + coreAdmin = await createUser('core', 'admin'); + localAdmin = await createUser('local-admin', 'admin'); + // An SSO account: no password, and a username that is an email address. + ssoUser = await new User({ + username: `${PREFIX}-sso.user+qa@example.com`, role: 'user', authProvider: 'oidc-test', + }).save(); adminAgent = await testUtils.getAdminAgent(); leadAgent = await login(lead.username); @@ -84,6 +98,8 @@ describe('Team members, last login and build trigger Tests', () => { }); after(async () => { + if (previousCoreAdminName === undefined) delete process.env.ANGLES_ADMIN_USERNAME; + else process.env.ANGLES_ADMIN_USERNAME = previousCoreAdminName; await User.deleteMany({ username: new RegExp(`^${PREFIX}`) }); await Team.deleteMany({ name: new RegExp(`^${PREFIX}`) }); await Environment.deleteMany({ name: new RegExp(`^${PREFIX}`) }); @@ -149,23 +165,37 @@ describe('Team members, last login and build trigger Tests', () => { res.body.map((u) => u.username).should.eql([outsider.username]); }); - it('offers a team lead only plain users who are not already in the team', async () => { + it('offers a team lead every other user, whatever their role or how they sign in', async () => { const res = await search(leadAgent, team._id, `${PREFIX}`).expect(200); const names = res.body.map((u) => u.username); - names.should.containDeep([candidate.username, outsider.username]); - // already a member / a team lead of this team / a team lead elsewhere + // plain users, a team lead of another team, an admin, a local and an SSO account + names.should.containDeep([ + candidate.username, outsider.username, otherLead.username, + localAdmin.username, ssoUser.username, + ]); + // already in the team names.should.not.containEql(member.username); names.should.not.containEql(lead.username); names.should.not.containEql(teamLead2.username); - names.should.not.containEql(otherLead.username); - res.body.forEach((u) => u.role.should.equal('user')); }); - it('also offers team leads to an admin, who may add them', async () => { - const res = await search(adminAgent, team._id, `${PREFIX}`).expect(200); - const names = res.body.map((u) => u.username); - names.should.containDeep([candidate.username, outsider.username, otherLead.username]); - names.should.not.containEql(member.username); + it('never offers the core admin account, to a team lead or an admin', async () => { + await Promise.all([leadAgent, adminAgent].map(async (agent) => { + const res = await search(agent, team._id, `${PREFIX}`).expect(200); + res.body.map((u) => u.username).should.not.containEql(coreAdmin.username); + const exact = await search(agent, team._id, coreAdmin.username).expect(200); + exact.body.should.eql([]); + })); + }); + + it('finds email-style usernames, treating . and + literally', async () => { + await Promise.all(['@example.com', 'sso.user+qa', 'user+qa@exa'].map(async (q) => { + const res = await search(leadAgent, team._id, q).expect(200); + res.body.map((u) => u.username).should.eql([ssoUser.username]); + })); + // '.' must not act as a wildcard: 'sso-user' would match 'sso.user' if it did + (await search(leadAgent, team._id, 'sso-user').expect(200)).body.should.eql([]); + (await search(leadAgent, team._id, 'user+').expect(200)).body.should.have.length(1); }); it('returns an empty list when nothing matches', async () => { @@ -186,7 +216,8 @@ describe('Team members, last login and build trigger Tests', () => { it('respond with 422 for fewer than three characters, or characters outside a username', async () => { await search(leadAgent, team._id, 'ca').expect(422); await search(leadAgent, team._id, '').expect(422); - await search(leadAgent, team._id, 'a.*').expect(422); + await search(leadAgent, team._id, 'a b!').expect(422); + await search(leadAgent, team._id, 'a*(').expect(422); await leadAgent.get(`${baseUrl}team/${team._id}/members/candidates`).expect(422); }); @@ -257,19 +288,39 @@ describe('Team members, last login and build trigger Tests', () => { .expect(403); }); - it('respond with 403 when a team lead tries to add another team lead, but an admin can', async () => { + it('lets a team lead add team leads and admins too', async () => { + await Promise.all([otherLead, localAdmin].map(async (target) => { + await leadAgent + .post(`${baseUrl}team/${team._id}/members`) + .send({ username: target.username }) + .expect(201); + (await User.findById(target._id).lean()).teams.map(String) + .should.containEql(String(team._id)); + })); + }); + + it('lets a team lead add a user whose username is an email address', async () => { + const res = await leadAgent + .post(`${baseUrl}team/${team._id}/members`) + .send({ username: ssoUser.username.toUpperCase() }) + .expect(201); + res.body.username.should.equal(ssoUser.username); + (await members(leadAgent, team._id)).map((u) => u.username) + .should.containEql(ssoUser.username); + }); + + it('respond with 403 when a team lead tries to add the core admin, but an admin can', async () => { await leadAgent .post(`${baseUrl}team/${team._id}/members`) - .send({ username: otherLead.username }) + .send({ username: coreAdmin.username }) .expect(403); - (await User.findById(otherLead._id).lean()).teams.map(String) - .should.not.containEql(String(team._id)); + (await User.findById(coreAdmin._id).lean()).teams.should.have.length(0); await adminAgent .post(`${baseUrl}team/${team._id}/members`) - .send({ username: otherLead.username }) + .send({ username: coreAdmin.username }) .expect(201); - (await User.findById(otherLead._id).lean()).teams.map(String) + (await User.findById(coreAdmin._id).lean()).teams.map(String) .should.containEql(String(team._id)); }); @@ -300,10 +351,23 @@ describe('Team members, last login and build trigger Tests', () => { (await User.findById(lead._id).lean()).teams.map(String).should.containEql(String(team._id)); }); - it('respond with 403 when a team lead removes another team lead, but an admin can', async () => { - await leadAgent.delete(`${baseUrl}team/${team._id}/members/${teamLead2._id}`).expect(403); - await adminAgent.delete(`${baseUrl}team/${team._id}/members/${teamLead2._id}`).expect(200); + it('lets a team lead remove a team lead or an admin they added', async () => { + await leadAgent.delete(`${baseUrl}team/${team._id}/members/${teamLead2._id}`).expect(200); (await User.findById(teamLead2._id).lean()).teams.should.have.length(0); + await leadAgent.delete(`${baseUrl}team/${team._id}/members/${localAdmin._id}`).expect(200); + }); + + it('shows the core admin as such in the member list, and only an admin can remove it', async () => { + // the previous describe left the core admin in the team, added by the admin + const list = await members(leadAgent, team._id); + list.find((u) => u.username === coreAdmin.username).isCoreAdmin.should.equal(true); + list.filter((u) => u.username !== coreAdmin.username) + .forEach((u) => u.isCoreAdmin.should.equal(false)); + + await leadAgent.delete(`${baseUrl}team/${team._id}/members/${coreAdmin._id}`).expect(403); + (await User.findById(coreAdmin._id).lean()).teams.map(String) + .should.containEql(String(team._id)); + await adminAgent.delete(`${baseUrl}team/${team._id}/members/${coreAdmin._id}`).expect(200); }); it('respond with 403 when a team lead removes a member from a team they do not lead', async () => {