From 4e2357b9d37e612e15cc020ec5f390538371c7a1 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 3 Oct 2026 14:43:08 +0000 Subject: [PATCH] fix(security): scope screenshots, baselines and metrics to the caller's team Several endpoints let an authenticated user of one team read or change another team's data. All were confirmed against a running instance as a plain user of a second team. Screenshots - GET /metrics/screenshot, /screenshot/grouped/platform and /screenshot/grouped/tag returned every team's screenshots (thumbnails, build ids and server paths included). They now only search builds of the caller's teams, as do the /screenshot/views and /screenshot/tags name lookups. - POST /screenshot accepted uploads into any team's build. It now requires access to the build's team and removes the uploaded file when rejected. Baselines - Baselines now record the team of their screenshot's build. Listing, reading and updating are limited to that team; a baseline can only be created from, or pointed at, a screenshot of the same team; and the "already exists" check only considers that team's baselines. - Comparing a screenshot against its baseline only uses the screenshot's own team's baseline. Before, a team using the same view name was compared against (and got a diff image of) another team's baseline. - GET /baseline takes an optional teamId. Admins, who can read every team, still see all teams' baselines without it. - Existing baselines are given their team on startup (idempotent backfill), so comparisons keep working after the upgrade. - GET /baseline/:id read the id from the query string and always returned 404; it now reads the path parameter. Other - POST /execution accepted executions into any team's build; it now requires access to the build's team. - GET /metrics/phase returned any team's metrics, test titles included; it now requires access to the team. - PUT /environment/:id and PUT /phase/:id were open to every user and API token. Environments and phases are shared, so renaming them is now admin-only, like creating and deleting them. - GET /screenshot/tags without a limit always failed with a 500 (MongoDB rejects $limit: 0); it now defaults to 10 like the view lookup. Adds test/team-scoping.tests.js (20 tests) and documents the 403 responses, the baseline team and the teamId filter in swagger.json. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KgQXSUjuLVXmLWxobnMfSf --- app/controllers/baseline.controller.js | 78 +++++- app/controllers/execution.controller.js | 3 + app/controllers/metrics.controller.js | 6 +- app/controllers/screenshot.controller.js | 117 ++++----- app/models/baseline.js | 11 + app/routes/baseline.routes.js | 3 + app/routes/environment.routes.js | 4 +- app/routes/phase.routes.js | 4 +- app/utils/baseline-utils.js | 60 ++++- server.js | 12 + swagger/swagger.json | 112 ++++++++- test/team-scoping.tests.js | 308 +++++++++++++++++++++++ 12 files changed, 642 insertions(+), 76 deletions(-) create mode 100644 test/team-scoping.tests.js diff --git a/app/controllers/baseline.controller.js b/app/controllers/baseline.controller.js index a3c408e..e9b34a2 100644 --- a/app/controllers/baseline.controller.js +++ b/app/controllers/baseline.controller.js @@ -1,6 +1,7 @@ const { validationResult } = require('express-validator'); const debug = require('debug'); const Baseline = require('../models/baseline.js'); +const Build = require('../models/build.js'); const Screenshot = require('../models/screenshot.js'); const validationUtils = require('../utils/validation-utils.js'); const baselineUtils = require('../utils/baseline-utils.js'); @@ -15,6 +16,22 @@ const { const log = debug('baseline:controller'); +// The team a screenshot belongs to, through its build. A screenshot whose build is gone +// cannot be attributed to a team, so it cannot back a baseline. +const teamOfScreenshot = async (screenshot) => { + const build = await Build.findById(screenshot.build).select('team').lean().exec(); + if (!build) { + throw new NotFoundError(`No build found for screenshot with id ${screenshot._id}`); + } + return build.team; +}; + +// A baseline is readable and editable by its team. One written before baselines recorded +// a team (and not yet backfilled) cannot be attributed, so only an admin may touch it. +const hasBaselineAccess = (user, baseline) => (baseline.team + ? authMiddleware.hasTeamAccess(user, baseline.team) + : Boolean(user && user.role === 'admin')); + // Create and save a new test execution exports.create = (req, res) => { // check the request is valid @@ -24,18 +41,21 @@ exports.create = (req, res) => { } const { screenshotId, view: requestView, ignoreBoxes } = req.body; - // determine if type is defined, otherwise set it to IMAGE (and handle default). - const promises = [ - Screenshot.findById(screenshotId).lean().exec(), - Baseline.find({ view: requestView }).lean().exec(), - ]; - return Promise.all(promises) - .then((results) => { - const screenshot = results[0]; - const baselinesFound = results[1]; + let team; + return Screenshot.findById(screenshotId).lean().exec() + .then(async (screenshot) => { if (!screenshot) { throw new NotFoundError(`No screenshot found with id ${screenshotId}`); } + team = await teamOfScreenshot(screenshot); + if (!authMiddleware.hasTeamAccess(req.user, team)) { + throw new ForbiddenError('You do not have access to this screenshot'); + } + // Only this team's baselines count: another team may use the same view name. + const baselinesFound = await Baseline.find({ team, view: requestView }).lean().exec(); + return { screenshot, baselinesFound }; + }) + .then(({ screenshot, baselinesFound }) => { const { view: screenshotView, platform } = screenshot; if (screenshotView !== requestView) { throw new InvalidRequestError(`The screenshot with id ${screenshotId} is not for the same view. Expected [${screenshotView}], Actual [${requestView}]`); @@ -61,7 +81,7 @@ exports.create = (req, res) => { throw new ConflictError(`Baseline for view [${requestView}], platform [${platformName}] and browser [${browserName}] with resolution [${width} x ${height}] already exists`); } } - const baseline = baselineUtils.createBaseline(requestView, screenshot, ignoreBoxes); + const baseline = baselineUtils.createBaseline(requestView, screenshot, ignoreBoxes, team); return baseline.save(); }) .then((savedBaseline) => { @@ -85,11 +105,23 @@ exports.findAll = (req, res) => { browserName, screenHeight, screenWidth, + teamId, } = req.query; const baseLineQuery = { view, 'platform.platformName': platformName, }; + // Baselines are per team. A named team must be one the caller can read; otherwise the + // result is limited to the caller's own teams (admins, who can read every team, are not + // limited, so an admin should name the team when view names collide across teams). + if (teamId) { + if (!authMiddleware.hasTeamAccess(req.user, teamId)) { + return handleError(new ForbiddenError('You do not have access to this team'), res); + } + baseLineQuery.team = teamId; + } else if (!req.user || req.user.role !== 'admin') { + baseLineQuery.team = { $in: (req.user && req.user.teams) || [] }; + } if (deviceName) baseLineQuery['platform.deviceName'] = deviceName; if (browserName) baseLineQuery['platform.browserName'] = browserName; if (screenHeight) baseLineQuery.screenHeight = screenHeight; @@ -106,12 +138,15 @@ exports.findOne = (req, res) => { if (!errors.isEmpty()) { return res.status(422).json({ errors: errors.array() }); } - const { baselineId } = req.query; + const { baselineId } = req.params; return Baseline.findById(baselineId).lean() .then((baseline) => { if (!baseline) { throw new NotFoundError(`Baseline not found with id ${baselineId}`); } + if (!hasBaselineAccess(req.user, baseline)) { + throw new ForbiddenError('You do not have access to this baseline'); + } return res.status(200).send(baseline); }).catch((err) => handleError(err, res)); }; @@ -133,7 +168,7 @@ exports.update = (req, res) => { Baseline.findById(baselineId).exec(), ]; return Promise.all(promises) - .then((results) => { + .then(async (results) => { const screenshot = results[0]; const baselineFound = results[1]; if (screenshotId && !screenshot) { @@ -142,6 +177,20 @@ exports.update = (req, res) => { if (!baselineFound) { throw new NotFoundError(`Baseline not found with id ${baselineId}`); } + if (!hasBaselineAccess(req.user, baselineFound)) { + throw new ForbiddenError('You do not have access to this baseline'); + } + // The new image has to come from the baseline's own team, or a baseline could be + // pointed at (and so expose) another team's screenshot. + if (screenshotId) { + const screenshotTeam = await teamOfScreenshot(screenshot); + const baselineTeam = baselineFound.team || screenshotTeam; + if (screenshotTeam.toString() !== baselineTeam.toString() + || !authMiddleware.hasTeamAccess(req.user, screenshotTeam)) { + throw new ForbiddenError('The screenshot must belong to the same team as the baseline'); + } + if (!baselineFound.team) baselineFound.team = screenshotTeam; + } if (screenshotId && screenshot.view !== baselineFound.view) { throw new InvalidRequestError(`The screenshot with id ${screenshotId} has a different view to the baseline and therefore can not be used for the requested baseline. Expected [${screenshot.view}], Actual [${baselineFound.view}].`); } @@ -174,7 +223,10 @@ exports.delete = (req, res) => { if (!baselineFound) { throw new NotFoundError(`Baseline not found with id ${baselineId}`); } - if (!authMiddleware.hasTeamLeadAccess(req.user, baselineFound.screenshot.build.team)) { + const team = baselineFound.team + || (baselineFound.screenshot && baselineFound.screenshot.build + && baselineFound.screenshot.build.team); + if (!team || !authMiddleware.hasTeamLeadAccess(req.user, team)) { throw new ForbiddenError('You do not have permission to delete this baseline'); } return Baseline.findByIdAndRemove(baselineId); diff --git a/app/controllers/execution.controller.js b/app/controllers/execution.controller.js index fc5e328..8fb84e7 100644 --- a/app/controllers/execution.controller.js +++ b/app/controllers/execution.controller.js @@ -38,6 +38,9 @@ exports.create = (req, res) => { if (!buildFound) { throw new NotFoundError(`No build found with id ${buildId}`); } + if (!authMiddleware.hasTeamAccess(req.user, buildFound.team)) { + throw new ForbiddenError('You do not have access to this build'); + } testExecution = buildMetricsUtils.createExecution(req, buildFound); return testExecution.save(); }) diff --git a/app/controllers/metrics.controller.js b/app/controllers/metrics.controller.js index 93d089b..5c8d411 100644 --- a/app/controllers/metrics.controller.js +++ b/app/controllers/metrics.controller.js @@ -7,7 +7,8 @@ const { Team } = require('../models/team.js'); // const Environment = require('../models/environment.js'); // const Screenshot = require('../models/screenshot.js'); const Execution = require('../models/execution.js'); -const { handleError, NotFoundError } = require('../exceptions/errors.js'); +const { handleError, NotFoundError, ForbiddenError } = require('../exceptions/errors.js'); +const authMiddleware = require('../utils/auth-middleware.js'); // const Baseline = require('../models/baseline.js'); // const Phase = require('../models/phase.js'); @@ -68,6 +69,9 @@ exports.retrieveMetricsPerPhase = (req, res) => { if (!teamFound) { throw new NotFoundError(`No team found with id ${teamId}`); } + if (!authMiddleware.hasTeamAccess(req.user, teamFound._id)) { + throw new ForbiddenError('You do not have access to this team'); + } const buildQuery = { team: teamFound._id }; // Applied to the build query rather than the execution aggregation: the executions // are already scoped to these builds, so narrowing here narrows both. Absent means diff --git a/app/controllers/screenshot.controller.js b/app/controllers/screenshot.controller.js index 2cca6e8..593fbe0 100644 --- a/app/controllers/screenshot.controller.js +++ b/app/controllers/screenshot.controller.js @@ -10,6 +10,7 @@ const Build = require('../models/build.js'); const Baseline = require('../models/baseline.js'); const validationUtils = require('../utils/validation-utils.js'); const imageUtils = require('../utils/image-utils.js'); +const baselineUtils = require('../utils/baseline-utils.js'); // Searches run on the image-engine worker pool; annotateMatches stays on the main // thread because it is I/O-bound rather than CPU-bound. const imageEngine = require('../image-engine/index.js'); @@ -76,6 +77,28 @@ const readableTeamIds = (user) => { return (user && user.teams) ? user.teams : []; }; +/** + * A `$match` stage restricting screenshots to builds of the caller's teams, for the + * aggregations that search across builds (metrics, view and tag lookups, the latest + * screenshot per platform). Empty for admins, who may read every team. + */ +const readableScreenshotsMatch = async (user) => { + const teamIds = readableTeamIds(user); + if (teamIds === null) return {}; + const buildIds = await Build.find({ team: { $in: teamIds } }).distinct('_id').exec(); + return { build: { $in: buildIds } }; +}; + +// An upload that is rejected after multer has written it must not be left on disk. +const removeUploadedFile = async (file) => { + if (!file || !file.path) return; + try { + await fs.promises.unlink(file.path); + } catch (error) { + if (error.code !== 'ENOENT') log(`Could not remove rejected upload ${file.path}: ${error.message}`); + } +}; + // Compare options arrive as strings; undefined fields mean "use the engine default" // (pixel algorithm, threshold 0.5, no regions). const parseCompareOptions = (query) => { @@ -99,11 +122,14 @@ exports.create = (req, res) => { view, tags, } = req.body; - return Build.findById(buildId).select('_id').lean() + return Build.findById(buildId).select('_id team').lean() .then((foundBuild) => { if (!foundBuild) { throw new NotFoundError(`No build found with id ${buildId}`); } + if (!authMiddleware.hasTeamAccess(req.user, foundBuild.team)) { + throw new ForbiddenError('You do not have access to this build'); + } build = foundBuild; return jimp.read(req.file.path) .then((image) => { @@ -148,7 +174,10 @@ exports.create = (req, res) => { log(`Created screenshot "${savedScreenshot.path}", view "${savedScreenshot.view}" build "${savedScreenshot.build}", with id: "${savedScreenshot._id}"`); return res.status(201).send(savedScreenshot); }) - .catch((error) => handleError(error, res)); + .catch(async (error) => { + await removeUploadedFile(req.file); + return handleError(error, res); + }); }; // Express only treats a middleware as an error handler when it declares four parameters, @@ -235,12 +264,12 @@ exports.findViewNames = (req, res) => { const queryLimit = parseInt(limit, 10) || 10; - return Screenshot.aggregate([ - { $match: { view: { $regex: `^${validationUtils.escapeRegex(partialView)}` } } }, + return readableScreenshotsMatch(req.user).then((readable) => Screenshot.aggregate([ + { $match: { ...readable, view: { $regex: `^${validationUtils.escapeRegex(partialView)}` } } }, { $group: { _id: '$view' } }, { $limit: queryLimit }, { $group: { _id: 0, views: { $push: '$_id' } } }, - ]) + ])) .then((resultArray) => { if (resultArray.length > 0) { const { views } = resultArray[0]; @@ -261,15 +290,18 @@ exports.findTagNames = (req, res) => { limit, } = req.query; - const queryLimit = parseInt(limit, 10) || 0; + // Defaults to 10 like the view lookup; MongoDB rejects `$limit: 0`, so the old default of + // 0 made every request without a limit fail with a 500. + const queryLimit = parseInt(limit, 10) || 10; - return Screenshot.aggregate([ + return readableScreenshotsMatch(req.user).then((readable) => Screenshot.aggregate([ + { $match: readable }, { $unwind: '$tags' }, { $match: { tags: { $regex: `^${validationUtils.escapeRegex(partialTag)}` } } }, { $group: { _id: '$tags' } }, { $limit: queryLimit }, { $group: { _id: 0, tagsArray: { $push: '$_id' } } }, - ]) + ])) .then((resultArray) => { if (resultArray.length > 0) { const { tagsArray } = resultArray[0]; @@ -380,11 +412,12 @@ exports.retrieveScreenshotMetrics = (req, res) => { ...aggregateTagsQuery, ]; } - const promises = [ - Screenshot.aggregate(aggregateViewQuery).exec(), - Screenshot.aggregate(aggregateTagsQuery).exec(), - ]; - return Promise.all(promises).then((results) => { + // Both pipelines group screenshots across every build, so they are restricted to the + // caller's teams first; the thumbnails they return would otherwise come from any team. + return readableScreenshotsMatch(req.user).then((readable) => Promise.all([ + Screenshot.aggregate([{ $match: readable }, ...aggregateViewQuery]).exec(), + Screenshot.aggregate([{ $match: readable }, ...aggregateTagsQuery]).exec(), + ])).then((results) => { const viewScreenshots = results[0]; const tagsScreenshots = results[1]; const result = { @@ -404,12 +437,12 @@ exports.findLatestForViewGroupedByPlatform = (req, res) => { const { view, numberOfDays } = req.query; const searchDate = new Date(); searchDate.setDate(searchDate.getDate() - numberOfDays); - return Screenshot.aggregate([ - { $match: { view, createdAt: { $gt: searchDate } } }, + return readableScreenshotsMatch(req.user).then((readable) => Screenshot.aggregate([ + { $match: { ...readable, view, createdAt: { $gt: searchDate } } }, { $sort: { _id: 1 } }, { $group: { _id: { view: '$view', platformId: '$platformId' }, lastId: { $last: '$_id' } } }, { $project: { _id: '$lastId' } }, - ]) + ])) .then((screenshotsIdsArray) => { const latestScreenshotIds = screenshotsIdsArray.map(({ _id }) => _id); return Screenshot.find({ _id: { $in: latestScreenshotIds } }).lean(); @@ -426,12 +459,12 @@ exports.findLatestForTagGroupedByView = (req, res) => { const { tag, numberOfDays } = req.query; const searchDate = new Date(); searchDate.setDate(searchDate.getDate() - numberOfDays); - return Screenshot.aggregate([ - { $match: { tags: { $in: [tag] }, createdAt: { $gt: searchDate } } }, + return readableScreenshotsMatch(req.user).then((readable) => Screenshot.aggregate([ + { $match: { ...readable, tags: { $in: [tag] }, createdAt: { $gt: searchDate } } }, { $sort: { view: 1, _id: 1 } }, { $group: { _id: { view: '$view', platformId: '$platformId' }, lastId: { $last: '$_id' } } }, { $project: { _id: '$lastId' } }, - ]) + ])) .then((screenshotsIdsArray) => { const latestScreenshotIds = screenshotsIdsArray.map(({ _id }) => _id); return Screenshot.find({ _id: { $in: latestScreenshotIds } }).lean(); @@ -718,29 +751,14 @@ exports.compareImageAgainstBaseline = (req, res) => { if (!screenshotFound) { throw new NotFoundError(`No screenshot found with id ${screenshotId}`); } - await assertScreenshotAccess(req.user, screenshotFound); + const build = await assertScreenshotAccess(req.user, screenshotFound); if (!screenshotFound.view) { throw new InvalidRequestError(`Screenshot with id ${screenshotId} does not have a view set, so can not be compared.`); } screenshot = screenshotFound; - const { - view, - platform, - height, - width, - } = screenshot; - // generate baseline query using screenshot details. - const baseLineQuery = { - view, - 'platform.platformName': platform.platformName, - }; - if (platform.deviceName) baseLineQuery['platform.deviceName'] = platform.deviceName; - if (platform.browserName) { - baseLineQuery['platform.browserName'] = platform.browserName; - baseLineQuery.screenHeight = height; - baseLineQuery.screenWidth = width; - } - return Baseline.findOne(baseLineQuery).populate('screenshot').lean(); + // Only the screenshot's own team's baseline: another team may use the same view name. + return Baseline.findOne(baselineUtils.baselineQueryForScreenshot(screenshot, build.team)) + .populate('screenshot').lean(); }) .then(async (baseline) => { // compare image with baseline and return result. @@ -787,28 +805,15 @@ exports.compareImageAgainstBaselineAndReturnImage = (req, res) => { if (!screenshot) { throw new NotFoundError(`No screenshot found with id ${screenshotId}`); } - await assertScreenshotAccess(req.user, screenshot); + const build = await assertScreenshotAccess(req.user, screenshot); if (!screenshot.view) { throw new InvalidRequestError(`Screenshot with id ${screenshotId} does not have a view set, so can not be compared.`); } screenshotToCompare = screenshot; - const { - view, - height, - width, - platform, - } = screenshotToCompare; - const baseLineQuery = { - view, - 'platform.platformName': platform.platformName, - }; - if (platform.deviceName) baseLineQuery['platform.deviceName'] = platform.deviceName; - if (platform.browserName) { - baseLineQuery['platform.browserName'] = platform.browserName; - baseLineQuery.screenHeight = height; - baseLineQuery.screenWidth = width; - } - return Baseline.findOne(baseLineQuery).populate('screenshot').lean(); + // Only the screenshot's own team's baseline: another team may use the same view name. + const baselineQuery = baselineUtils + .baselineQueryForScreenshot(screenshotToCompare, build.team); + return Baseline.findOne(baselineQuery).populate('screenshot').lean(); }) .then((baselineFound) => { if (!baselineFound) { diff --git a/app/models/baseline.js b/app/models/baseline.js index a729c1b..a2ccbed 100644 --- a/app/models/baseline.js +++ b/app/models/baseline.js @@ -43,6 +43,16 @@ const Platform = new Schema({ }, { _id: false }); const BaselineSchema = mongoose.Schema({ + // The team that owns the baseline: the team of the build its screenshot came from. View + // names are chosen freely by each team, so without this two teams using the same view + // name would compare against (and could edit) each other's baselines. Absent only on + // baselines written before teams were recorded until the startup backfill has run (see + // baselineUtils.backfillTeams); those are visible to admins only. + team: { + type: Schema.Types.ObjectId, + ref: 'Team', + required: false, + }, screenshot: { type: Schema.Types.ObjectId, ref: 'Screenshot', @@ -75,6 +85,7 @@ const BaselineSchema = mongoose.Schema({ }); BaselineSchema.index({ view: 1 }, { unique: false }); +BaselineSchema.index({ team: 1, view: 1 }, { unique: false }); BaselineSchema.index({ view: 1, 'platform.platformName': 1, diff --git a/app/routes/baseline.routes.js b/app/routes/baseline.routes.js index 19f7607..030e92b 100644 --- a/app/routes/baseline.routes.js +++ b/app/routes/baseline.routes.js @@ -39,6 +39,9 @@ module.exports = (app, path) => { query('screenWidth') .optional() .isNumeric(), + query('teamId') + .optional() + .isMongoId(), ], baselineController.findAll); app.get(`${path}/baseline/:baselineId`, [ diff --git a/app/routes/environment.routes.js b/app/routes/environment.routes.js index b750e12..2f4301b 100644 --- a/app/routes/environment.routes.js +++ b/app/routes/environment.routes.js @@ -16,7 +16,9 @@ module.exports = (app, path) => { param('environmentId').isMongoId(), ], environmentController.findOne); - app.put(`${path}/environment/:environmentId`, [ + // Environments are shared by every team, so renaming one is an admin action, the same as + // creating or deleting it. + app.put(`${path}/environment/:environmentId`, authMiddleware.authorizeAdmin, [ param('environmentId').isMongoId(), check('name') .exists({ checkFalsy: true }) diff --git a/app/routes/phase.routes.js b/app/routes/phase.routes.js index 5d34b5d..cb47f02 100644 --- a/app/routes/phase.routes.js +++ b/app/routes/phase.routes.js @@ -21,7 +21,9 @@ module.exports = (app, path) => { param('phaseId').isMongoId(), ], phaseController.findOne); - app.put(`${path}/phase/:phaseId`, [ + // Phases are shared by every team, so changing one is an admin action, the same as + // creating or deleting it. + app.put(`${path}/phase/:phaseId`, authMiddleware.authorizeAdmin, [ param('phaseId').isMongoId(), oneOf([ check('name') diff --git a/app/utils/baseline-utils.js b/app/utils/baseline-utils.js index 8a0bfb7..e18c236 100644 --- a/app/utils/baseline-utils.js +++ b/app/utils/baseline-utils.js @@ -1,8 +1,13 @@ +const debug = require('debug'); const Baseline = require('../models/baseline.js'); +const Build = require('../models/build.js'); +const Screenshot = require('../models/screenshot.js'); + +const log = debug('baseline:utils'); const baselineUtils = {}; -baselineUtils.createBaseline = (view, screenshot, ignoreBoxes) => { +baselineUtils.createBaseline = (view, screenshot, ignoreBoxes, team) => { // TODO: Handle type now for Image or Dynamic const { platform: { deviceName, platformName, browserName }, @@ -10,6 +15,7 @@ baselineUtils.createBaseline = (view, screenshot, ignoreBoxes) => { width, } = screenshot; const baseline = new Baseline({ + team, screenshot, view, platform: { @@ -47,4 +53,56 @@ baselineUtils.checkIfBaselineAlreadyExists = (requestView, baselinesFound, scree return matchingBaselines; }; +/* +Builds the query that finds the baseline a screenshot should be compared against: same +team, same view and same platform (device, or browser plus resolution). + */ +baselineUtils.baselineQueryForScreenshot = (screenshot, team) => { + const { + view, platform, height, width, + } = screenshot; + const query = { + team, + view, + 'platform.platformName': platform.platformName, + }; + if (platform.deviceName) query['platform.deviceName'] = platform.deviceName; + if (platform.browserName) { + query['platform.browserName'] = platform.browserName; + query.screenHeight = height; + query.screenWidth = width; + } + return query; +}; + +/* +Sets `team` on baselines written before it was recorded, from the build of the +baseline's screenshot. Baselines are now looked up per team, so one without a team is +never matched for a comparison; this restores them. + +Safe to run repeatedly: only baselines with no team are touched, and a baseline whose +screenshot or build is gone is left as it is (it cannot be attributed to a team). + */ +baselineUtils.backfillTeams = async () => { + const baselines = await Baseline.find({ team: { $exists: false } }) + .select('_id screenshot').lean().exec(); + let updated = 0; + // Baselines number in the tens or hundreds, so one at a time keeps this simple. + // eslint-disable-next-line no-restricted-syntax + for (const baseline of baselines) { + // eslint-disable-next-line no-await-in-loop + const screenshot = await Screenshot.findById(baseline.screenshot).select('build').lean().exec(); + // eslint-disable-next-line no-await-in-loop + const build = screenshot ? await Build.findById(screenshot.build).select('team').lean().exec() : null; + if (build && build.team) { + // eslint-disable-next-line no-await-in-loop + await Baseline.updateOne({ _id: baseline._id }, { $set: { team: build.team } }).exec(); + updated += 1; + } else { + log(`Baseline ${baseline._id} has no screenshot or build left; leaving it without a team`); + } + } + return { checked: baselines.length, updated }; +}; + module.exports = baselineUtils; diff --git a/server.js b/server.js index c9f4d90..92d3f62 100644 --- a/server.js +++ b/server.js @@ -115,6 +115,18 @@ mongoose.connect(mongoURL, { } catch (err) { logger.error('Could not load auth settings', err); } + // Baselines are looked up per team; give any written before that their team, or they + // would stop matching. Idempotent, and cheap once done (it only reads baselines that + // have no team). + try { + // eslint-disable-next-line global-require + const { checked, updated } = await require('./app/utils/baseline-utils.js').backfillTeams(); + if (checked > 0) { + logger.info('Baseline team backfill: %d of %d baseline(s) updated', updated, checked); + } + } catch (err) { + logger.error('Could not backfill baseline teams', err); + } // Load the persisted feature toggles onto the in-memory config the route guards read. // A failure here leaves the defaults in place (every feature on), which is the same // behaviour the instance had before toggles existed. diff --git a/swagger/swagger.json b/swagger/swagger.json index b2351cd..4abd435 100644 --- a/swagger/swagger.json +++ b/swagger/swagger.json @@ -556,8 +556,19 @@ } } } + }, + "403": { + "description": "Admin access required", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/DefaultResponse" + } + } + } } - } + }, + "description": "Admin only: environments and phases are shared by every team." } }, "/phase": { @@ -776,8 +787,19 @@ } } } + }, + "403": { + "description": "Admin access required", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/DefaultResponse" + } + } + } } - } + }, + "description": "Admin only: environments and phases are shared by every team." } }, "/build": { @@ -1340,6 +1362,16 @@ } } } + }, + "403": { + "description": "No access to the build's team", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/DefaultResponse" + } + } + } } } }, @@ -1734,6 +1766,16 @@ } } } + }, + "403": { + "description": "No access to the build's team", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/DefaultResponse" + } + } + } } } }, @@ -2953,6 +2995,16 @@ } } } + }, + "403": { + "description": "No access to the screenshot's team", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/DefaultResponse" + } + } + } } } }, @@ -3015,6 +3067,15 @@ "schema": { "type": "number" } + }, + { + "name": "teamId", + "in": "query", + "required": false, + "description": "Only return this team's baselines. The caller must have access to the team.", + "schema": { + "type": "string" + } } ], "responses": { @@ -3030,8 +3091,19 @@ } } } + }, + "403": { + "description": "No access to the named team", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/DefaultResponse" + } + } + } } - } + }, + "description": "Baselines belong to a team (the team of the build their screenshot came from). Without `teamId`, a non-admin only sees their own teams' baselines; admins see every team's, so they should pass `teamId` when view names are shared between teams." } }, "/baseline/{baselineId}": { @@ -3081,6 +3153,16 @@ } } } + }, + "403": { + "description": "No access to the baseline's team", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/DefaultResponse" + } + } + } } } }, @@ -3182,6 +3264,16 @@ } } } + }, + "403": { + "description": "No access to the baseline's team, or the new screenshot belongs to another team", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/DefaultResponse" + } + } + } } } } @@ -3249,6 +3341,16 @@ "responses": { "200": { "description": "OK" + }, + "403": { + "description": "No access to the team", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/DefaultResponse" + } + } + } } } } @@ -7899,6 +8001,10 @@ }, "screenshot": { "$ref": "#/components/schemas/Screenshot" + }, + "team": { + "type": "string", + "description": "The team that owns the baseline: the team of the build its screenshot came from." } } } diff --git a/test/team-scoping.tests.js b/test/team-scoping.tests.js new file mode 100644 index 0000000..36a2c1e --- /dev/null +++ b/test/team-scoping.tests.js @@ -0,0 +1,308 @@ +/** + * Team scoping of screenshots, baselines, metrics and writes into builds. + * + * Each test acts as a plain user of one team ("other") against data that belongs to + * another team ("owner"), and checks that it can neither read nor change it. These all + * returned the other team's data (or a 2xx) before the fix. + */ +const fs = require('fs'); +const path = require('path'); +const request = require('supertest'); +const should = require('should'); +const bcrypt = require('bcryptjs'); +const app = require('../server.js'); +const User = require('../app/models/user.js'); +const Build = require('../app/models/build.js'); +const Screenshot = require('../app/models/screenshot.js'); +const TestExecution = require('../app/models/execution.js'); +const Baseline = require('../app/models/baseline.js'); +const Environment = require('../app/models/environment.js'); +const Phase = require('../app/models/phase.js'); +const { Team } = require('../app/models/team.js'); +const buildUtils = require('../app/utils/build-utils.js'); +const baselineUtils = require('../app/utils/baseline-utils.js'); + +const baseUrl = '/rest/api/v1.0/'; +const IMAGE = './test/resources/angles_home_page.jpg'; +const OWNER_PASSWORD = 'unit-testing-TsOwner1!'; +const OTHER_PASSWORD = 'unit-testing-TsOther1!'; + +const SHARED_VIEW = 'unit-testing-ts-shared-view'; +const OWNER_ONLY_VIEW = 'unit-testing-ts-owner-only-view'; +const OWNER_TAG = 'unit-testing-ts-owner-tag'; + +describe('Team scoping Tests', () => { + let ownerAgent; + let otherAgent; + let ownerTeam; + let otherTeam; + let environment; + let phase; + let ownerBuild; + let otherBuild; + let ownerShot; + let ownerOnlyShot; + let otherShot; + let ownerBaseline; + + const login = (username, password) => new Promise((resolve, reject) => { + const agent = request.agent(app); + agent.post(`${baseUrl}auth/login`).send({ username, password }).end((err, res) => { + if (err) return reject(err); + if (res.status !== 200) return reject(new Error(`login failed for ${username}: ${res.status}`)); + return resolve(agent); + }); + }); + + const send = (agent, method, url, body) => new Promise((resolve, reject) => { + const req = agent[method](`${baseUrl}${url}`).set('Accept', 'application/json'); + if (body !== undefined) req.send(body); + req.end((err, res) => (err ? reject(err) : resolve(res))); + }); + + const uploadScreenshot = (agent, buildId, view, tags) => new Promise((resolve, reject) => { + const req = agent.post(`${baseUrl}screenshot`) + .field('buildId', buildId.toString()) + .field('timestamp', new Date().toISOString()) + .field('view', view) + .field('platformName', 'linux') + .field('browserName', 'chrome'); + if (tags) req.field('tags', JSON.stringify(tags)); + req.attach('screenshot', IMAGE).end((err, res) => (err ? reject(err) : resolve(res))); + }); + + const newBuild = (team, name) => new Build({ + name, + team, + environment, + status: buildUtils.executionStates[0], + component: team.components[0]._id, + suites: [], + result: new Map(buildUtils.defaultResultMap), + }).save(); + + const removeFixtures = async () => { + const teams = await Team.find({ name: /^unit-testing-ts-/ }).select('_id').lean(); + const teamIds = teams.map((t) => t._id); + const builds = await Build.find({ team: { $in: teamIds } }).select('_id').lean(); + const buildIds = builds.map((b) => b._id); + const screenshots = await Screenshot.find({ build: { $in: buildIds } }).lean(); + screenshots.forEach((s) => { try { fs.unlinkSync(s.path); } catch (e) { /* gone */ } }); + await Promise.all([ + Baseline.deleteMany({ $or: [{ team: { $in: teamIds } }, { view: /^unit-testing-ts-/ }] }).exec(), + Screenshot.deleteMany({ build: { $in: buildIds } }).exec(), + TestExecution.deleteMany({ build: { $in: buildIds } }).exec(), + Build.deleteMany({ _id: { $in: buildIds } }).exec(), + Environment.deleteMany({ name: /^unit-testing-ts-/ }).exec(), + Phase.deleteMany({ name: /^unit-testing-ts-/ }).exec(), + User.deleteMany({ username: /^unit-testing-ts-/ }).exec(), + Team.deleteMany({ _id: { $in: teamIds } }).exec(), + ]); + }; + + before(async () => { + await removeFixtures(); + ownerTeam = await new Team({ name: 'unit-testing-ts-owner', components: [{ name: 'ts-owner' }] }).save(); + otherTeam = await new Team({ name: 'unit-testing-ts-other', components: [{ name: 'ts-other' }] }).save(); + environment = await new Environment({ name: 'unit-testing-ts-env' }).save(); + phase = await new Phase({ name: 'unit-testing-ts-phase', orderNumber: 99 }).save(); + ownerBuild = await newBuild(ownerTeam, 'unit-testing-ts-owner-build'); + otherBuild = await newBuild(otherTeam, 'unit-testing-ts-other-build'); + const [ownerHash, otherHash] = await Promise.all([ + bcrypt.hash(OWNER_PASSWORD, 10), bcrypt.hash(OTHER_PASSWORD, 10), + ]); + await User.create([ + { + username: 'unit-testing-ts-owner', password: ownerHash, role: 'user', teams: [ownerTeam._id], + }, + { + username: 'unit-testing-ts-other', password: otherHash, role: 'user', teams: [otherTeam._id], + }, + ]); + [ownerAgent, otherAgent] = await Promise.all([ + login('unit-testing-ts-owner', OWNER_PASSWORD), + login('unit-testing-ts-other', OTHER_PASSWORD), + ]); + + ownerShot = (await uploadScreenshot(ownerAgent, ownerBuild._id, SHARED_VIEW, [OWNER_TAG])).body; + ownerOnlyShot = (await uploadScreenshot(ownerAgent, ownerBuild._id, OWNER_ONLY_VIEW)).body; + otherShot = (await uploadScreenshot(otherAgent, otherBuild._id, SHARED_VIEW)).body; + should(ownerShot._id).be.a.String(); + should(otherShot._id).be.a.String(); + + const res = await send(ownerAgent, 'post', 'baseline', { screenshotId: ownerShot._id, view: SHARED_VIEW }); + should(res.status).equal(201); + ownerBaseline = res.body; + }); + + after(removeFixtures); + + const ownerBuildIds = () => [ownerBuild._id.toString()]; + const buildsIn = (screenshots) => screenshots.map((s) => s.build.toString()); + + describe('Screenshot lookups across builds', () => { + it('GET /metrics/screenshot only returns the caller\'s teams\' screenshots', async () => { + const res = await send(otherAgent, 'get', 'metrics/screenshot?thumbnail=true&limit=1000'); + should(res.status).equal(200); + const shots = [...res.body.views, ...res.body.tags] + .flatMap((group) => group.platforms) + .map((platform) => platform.screenshot) + .filter(Boolean); + buildsIn(shots).forEach((buildId) => should(ownerBuildIds()).not.containEql(buildId)); + should(res.body.views.map((v) => v._id)).not.containEql(OWNER_ONLY_VIEW); + }); + + it('GET /screenshot/grouped/platform only returns the caller\'s teams\' screenshots', async () => { + const res = await send(otherAgent, 'get', `screenshot/grouped/platform?view=${SHARED_VIEW}&numberOfDays=1`); + should(res.status).equal(200); + should(res.body.length).equal(1); + should(res.body[0]._id).equal(otherShot._id); + }); + + it('GET /screenshot/grouped/tag only returns the caller\'s teams\' screenshots', async () => { + const res = await send(otherAgent, 'get', `screenshot/grouped/tag?tag=${OWNER_TAG}&numberOfDays=1`); + should(res.status).equal(200); + should(res.body).eql([]); + const own = await send(ownerAgent, 'get', `screenshot/grouped/tag?tag=${OWNER_TAG}&numberOfDays=1`); + should(own.body.map((s) => s._id)).eql([ownerShot._id]); + }); + + it('GET /screenshot/views and /screenshot/tags do not reveal another team\'s names', async () => { + const views = await send(otherAgent, 'get', 'screenshot/views?view=unit-testing-ts'); + should(views.status).equal(200); + should(views.body).not.containEql(OWNER_ONLY_VIEW); + should(views.body).containEql(SHARED_VIEW); + const tags = await send(otherAgent, 'get', 'screenshot/tags?tag=unit-testing-ts'); + should(tags.status).equal(200); + should(tags.body).not.containEql(OWNER_TAG); + const ownTags = await send(ownerAgent, 'get', 'screenshot/tags?tag=unit-testing-ts'); + should(ownTags.body).containEql(OWNER_TAG); + }); + }); + + describe('Baselines', () => { + const baselineQuery = `baseline?view=${SHARED_VIEW}&platformName=linux&browserName=chrome`; + + it('are created with the team of their screenshot', () => { + should(ownerBaseline.team).equal(ownerTeam._id.toString()); + }); + + it('are listed for their own team only', async () => { + const other = await send(otherAgent, 'get', baselineQuery); + should(other.status).equal(200); + should(other.body).eql([]); + const owner = await send(ownerAgent, 'get', baselineQuery); + should(owner.body.map((b) => b._id)).eql([ownerBaseline._id]); + }); + + it('cannot be listed for a named team the caller cannot read', async () => { + const res = await send(otherAgent, 'get', `${baselineQuery}&teamId=${ownerTeam._id}`); + should(res.status).equal(403); + }); + + it('GET /baseline/:id returns the baseline to its team and 403 to others', async () => { + const owner = await send(ownerAgent, 'get', `baseline/${ownerBaseline._id}`); + should(owner.status).equal(200); + should(owner.body._id).equal(ownerBaseline._id); + const other = await send(otherAgent, 'get', `baseline/${ownerBaseline._id}`); + should(other.status).equal(403); + }); + + it('cannot be changed by another team', async () => { + const res = await send(otherAgent, 'put', `baseline/${ownerBaseline._id}`, { + ignoreBoxes: [{ + left: 0, top: 0, right: 0, bottom: 0, + }], + }); + should(res.status).equal(403); + const stored = await Baseline.findById(ownerBaseline._id).lean(); + should(stored.ignoreBoxes).eql([]); + }); + + it('cannot be pointed at another team\'s screenshot', async () => { + const res = await send(ownerAgent, 'put', `baseline/${ownerBaseline._id}`, { screenshotId: otherShot._id }); + should(res.status).equal(403); + }); + + it('cannot be created from another team\'s screenshot', async () => { + const res = await send(otherAgent, 'post', 'baseline', { screenshotId: ownerShot._id, view: SHARED_VIEW }); + should(res.status).equal(403); + }); + + it('are never compared across teams', async () => { + // The other team has no baseline of its own yet; the owner's must not be used. + const res = await send(otherAgent, 'get', `screenshot/${otherShot._id}/baseline/compare`); + should(res.status).equal(404); + const image = await send(otherAgent, 'get', `screenshot/${otherShot._id}/baseline/compare/image`); + should(image.status).equal(404); + }); + + it('can be created per team for the same view, and each team compares against its own', async () => { + const created = await send(otherAgent, 'post', 'baseline', { screenshotId: otherShot._id, view: SHARED_VIEW }); + should(created.status).equal(201); + should(created.body.team).equal(otherTeam._id.toString()); + const compare = await send(otherAgent, 'get', `screenshot/${otherShot._id}/baseline/compare`); + should(compare.status).equal(200); + const owner = await send(ownerAgent, 'get', `screenshot/${ownerShot._id}/baseline/compare`); + should(owner.status).equal(200); + }); + + it('written before teams were recorded get their team from the startup backfill', async () => { + const legacy = await new Baseline({ + screenshot: ownerOnlyShot._id, + view: OWNER_ONLY_VIEW, + platform: { platformName: 'linux', browserName: 'chrome' }, + }).save(); + const result = await baselineUtils.backfillTeams(); + should(result.updated).be.aboveOrEqual(1); + const stored = await Baseline.findById(legacy._id).lean(); + should(stored.team.toString()).equal(ownerTeam._id.toString()); + }); + }); + + describe('Writing into another team\'s build', () => { + it('POST /execution returns 403 and stores nothing', async () => { + const res = await send(otherAgent, 'post', 'execution', { + title: 'unit-testing-ts injected', suite: 'unit-testing-ts suite', build: ownerBuild._id.toString(), + }); + should(res.status).equal(403); + should(await TestExecution.countDocuments({ title: 'unit-testing-ts injected' })).equal(0); + }); + + it('POST /screenshot returns 403 and leaves no file behind', async () => { + const directory = path.resolve(__dirname, '../screenshots', ownerBuild._id.toString()); + const before = fs.existsSync(directory) ? fs.readdirSync(directory).length : 0; + const res = await uploadScreenshot(otherAgent, ownerBuild._id, 'unit-testing-ts-injected'); + should(res.status).equal(403); + const after = fs.existsSync(directory) ? fs.readdirSync(directory).length : 0; + should(after).equal(before); + should(await Screenshot.countDocuments({ view: 'unit-testing-ts-injected' })).equal(0); + }); + }); + + describe('GET /metrics/phase', () => { + it('returns 403 for a team the caller cannot read', async () => { + const res = await send(otherAgent, 'get', `metrics/phase?teamId=${ownerTeam._id}`); + should(res.status).equal(403); + }); + + it('still works for the caller\'s own team', async () => { + const res = await send(ownerAgent, 'get', `metrics/phase?teamId=${ownerTeam._id}`); + should(res.status).equal(200); + }); + }); + + describe('Shared environments and phases', () => { + it('PUT /environment/:id is admin only', async () => { + const res = await send(otherAgent, 'put', `environment/${environment._id}`, { name: 'unit-testing-ts-renamed' }); + should(res.status).equal(403); + const stored = await Environment.findById(environment._id).lean(); + should(stored.name).equal('unit-testing-ts-env'); + }); + + it('PUT /phase/:id is admin only', async () => { + const res = await send(otherAgent, 'put', `phase/${phase._id}`, { orderNumber: 1 }); + should(res.status).equal(403); + }); + }); +});