diff --git a/plugins/alerts/api/api.js b/plugins/alerts/api/api.js index 6bf378e155e..96a2dc9d7a1 100644 --- a/plugins/alerts/api/api.js +++ b/plugins/alerts/api/api.js @@ -5,9 +5,10 @@ var Promise = require("bluebird"); const JOB = require('../../../api/parts/jobs'); const utils = require('./parts/utils.js'); const _ = require('lodash'); -const { validateCreate, validateRead, validateUpdate, hasCreateRight, getAdminApps, getUserAppsForFeaturePermission } = require('../../../api/utils/rights.js'); +const { validateCreate, validateRead, validateUpdate, hasCreateRight, hasUpdateRight, getAdminApps, getUserAppsForFeaturePermission } = require('../../../api/utils/rights.js'); const FEATURE_NAME = 'alerts'; const commonLib = require("./parts/common-lib.js"); +const { memberHasRightForAllApps } = require('./parts/app-authorization.js'); const moment = require('moment-timezone'); /** @@ -235,22 +236,47 @@ function getScheduleTextExpression(period, offset) { const id = alertConfig._id; delete alertConfig._id; alertConfig.createdBy = params.member._id; - return common.db.collection("alerts").findAndModify( - { _id: common.db.ObjectID(id) }, - {}, - {$set: alertConfig}, - function(err, result) { - if (!err) { - if (result && result.value) { - plugins.dispatch("/updateAlert", { method: "alertTrigger", alert: result.value }); + //Two things are missing on this branch and both are added here. + // + //The filter was the alert id alone, with no owner, so any member with + //alerts rights on any app could rewrite any alert in the system. master + //restricts this to the caller's own alerts and that never reached here. + var query = { _id: common.db.ObjectID(id) }; + if (params.member.global_admin !== true) { + query.createdBy = params.member._id; + } + //Owning an alert is not the same as being allowed to act on the apps it + //targets, and the guard above only saw the submitted selectedApps, which + //an update may leave out to keep whatever is stored. So authorize the + //apps the stored alert currently points at. + return common.db.collection("alerts").findOne({_id: common.db.ObjectID(id)}, function(findErr, existingAlert) { + if (findErr) { + common.returnMessage(params, 500, "Failed to save an alert"); + return; + } + if (existingAlert && params.member.global_admin !== true + && !memberHasRightForAllApps(hasUpdateRight, params.member, existingAlert.selectedApps)) { + log.d("Rejected alert update: alert " + id + " targets apps the caller may not update"); + common.returnMessage(params, 403, 'No alerts:update permission on the apps this alert targets'); + return; + } + return common.db.collection("alerts").findAndModify( + query, + {}, + {$set: alertConfig}, + function(err, result) { + if (!err) { + if (result && result.value) { + plugins.dispatch("/updateAlert", { method: "alertTrigger", alert: result.value }); + } + + common.returnOutput(params, result && result.value); } - - common.returnOutput(params, result && result.value); - } - else { - common.returnMessage(params, 500, "Failed to save an alert"); - } - }); + else { + common.returnMessage(params, 500, "Failed to save an alert"); + } + }); + }); } alertConfig.createdBy = params.member._id; return common.db.collection("alerts").insert( @@ -299,8 +325,20 @@ function getScheduleTextExpression(period, offset) { validateUpdate(params, FEATURE_NAME, function() { let alertID = params.qstring.alertID; try { + //the filter was the alert id alone, so any member with alerts rights on + //any app could delete any alert. master restricts this to the caller's own + //alerts and that never reached this branch. + // + //Deliberately not also requiring rights on the apps the alert targets: + //deleting only removes what the alert does, and somebody who has lost + //access to an app still needs to be able to remove the alert they own for + //it, or they are left holding one they can neither manage nor delete. + var deleteQuery = { "_id": common.db.ObjectID(alertID) }; + if (params.member.global_admin !== true) { + deleteQuery.createdBy = params.member._id; + } common.db.collection("alerts").remove( - { "_id": common.db.ObjectID(alertID) }, + deleteQuery, function(err, result) { log.d(err, result, "delete an alert"); if (!err) { @@ -337,13 +375,56 @@ function getScheduleTextExpression(period, offset) { plugins.register("/i/alert/status", function(ob) { let params = ob.params; - validateUpdate(params, FEATURE_NAME, function() { + validateUpdate(params, FEATURE_NAME, async function() { const statusList = JSON.parse(params.qstring.status); + + //Enabling an alert for an app the caller may no longer touch is what makes + //this exploitable, so authorize the apps each stored alert targets before + //switching it on. Switching one off stays allowed: it only reduces what the + //alert does, and refusing would leave somebody who lost access unable to stop + //mail they no longer want. + const enablingIds = Object.keys(statusList).filter(function(alertID) { + return statusList[alertID] === true || statusList[alertID] === "true"; + }); + if (enablingIds.length > 0 && params.member.global_admin !== true) { + let toEnable = []; + try { + //scoped to the caller's own alerts, as the update below is, so an id + //belonging to somebody else stays a silent no-op rather than a 403 + //that would confirm an alert with those apps exists + toEnable = await common.db.collection("alerts").find({ + _id: { + $in: enablingIds.map(function(a) { + return common.db.ObjectID(a); + }) + }, + createdBy: params.member._id + }, { projection: { selectedApps: 1 } }).toArray(); + } + catch (e) { + log.e("Failed to load alerts for a status change", e); + common.returnMessage(params, 500, "Failed to change alert status"); + return; + } + const unauthorized = toEnable.filter(function(alert) { + return !memberHasRightForAllApps(hasUpdateRight, params.member, alert.selectedApps); + }); + if (unauthorized.length > 0) { + log.d("Rejected alert status change: alert(s) target apps the caller may not update"); + common.returnMessage(params, 403, 'No alerts:update permission on the apps these alerts target'); + return; + } + } const batch = []; for (const appID in statusList) { + //the filter was the alert id alone, so any member could toggle any alert + var qquery = { _id: common.db.ObjectID(appID) }; + if (params.member.global_admin !== true) { + qquery.createdBy = params.member._id; + } batch.push( common.db.collection("alerts").findAndModify( - { _id: common.db.ObjectID(appID) }, + qquery, {}, { $set: { enabled: statusList[appID] } }, { new: true, upsert: false } diff --git a/plugins/alerts/api/jobs/monitor.js b/plugins/alerts/api/jobs/monitor.js index fab1bf99b65..8aaf03b96d1 100644 --- a/plugins/alerts/api/jobs/monitor.js +++ b/plugins/alerts/api/jobs/monitor.js @@ -4,7 +4,46 @@ const { TRIGGERED_BY_EVENT } = require('../parts/common-lib.js'); const { Job } = require('../../../../api/parts/jobs/job.js'), log = require('../../../../api/utils/log.js')('alert:monitor'), - common = require('../../../../api/utils/common.js'); + common = require('../../../../api/utils/common.js'), + { hasReadRight } = require('../../../../api/utils/rights.js'); + +const { memberMayReadApp } = require('../parts/app-authorization.js'); + +/** + * Whether the member who created an alert may still read the app it is about. + * + * The endpoints authorize a request, but nothing re-checked anything once an alert was + * enabled, so one created before access was revoked kept emailing that app's metrics + * indefinitely with no further action by anybody. + * + * @param {object} alert - the stored alert + * @param {string} appID - app the scheduled job is about + * @returns {Promise} true when the alert may still be sent + */ +async function ownerMayStillReadApp(alert, appID) { + if (!alert.createdBy) { + //alerts predating the field cannot be attributed, and refusing to send them would + //silently stop alerts that have worked for years, so they are reported and left be + log.w("Alert " + alert._id + " has no createdBy, so its owner's current access cannot be checked. Sending anyway."); + return true; + } + let owner; + try { + owner = await common.db.collection("members").findOne( + { _id: common.db.ObjectID(alert.createdBy + "") }, + { projection: { global_admin: 1, permission: 1, user_of: 1, admin_of: 1 } } + ); + } + catch (e) { + log.e("Could not resolve the owner of alert " + alert._id + ", not sending", e); + return false; + } + if (!owner) { + log.d("Owner of alert " + alert._id + " no longer exists, not sending"); + return false; + } + return memberMayReadApp(hasReadRight, owner, appID); +} const ALERT_MODULES = { "views": require("../alertModules/views.js"), @@ -43,6 +82,10 @@ class MonitorJob extends Job { _id: common.db.ObjectID(appID), }); log.d("alert job info:", this._json, alert, app); + if (alert && app && !await ownerMayStillReadApp(alert, appID)) { + log.d("Not sending alert " + alertID + " for app " + appID + ": its owner no longer has access to that app"); + return; + } if (!alert || !app) { throw new Error("Alert", alertID, "or App", appID, "couldn't be found"); } diff --git a/plugins/alerts/api/parts/app-authorization.js b/plugins/alerts/api/parts/app-authorization.js new file mode 100644 index 00000000000..02747b67b89 --- /dev/null +++ b/plugins/alerts/api/parts/app-authorization.js @@ -0,0 +1,95 @@ +/** + * @module plugins/alerts/api/parts/app-authorization + * @description Decides whether a member may act on the apps an alert targets. + * + * The alert endpoints authorize the caller against params.qstring.app_id, which says + * nothing about the apps the alert itself points at. Two further things have to be + * checked and were not: + * + * - the apps the *stored* alert targets, on any mutation addressed by _id. The + * submitted selectedApps is checked separately, but an update may leave that field + * out and keep whatever is stored, so the stored value needs its own check. + * - the owner's *current* access, at the moment an alert is sent. Nothing re-checked + * that once an alert was enabled, so one created before access was revoked kept + * emailing that app's metrics with no further action by anybody. + * + * createdBy is not a substitute for either. It says the member made the alert, not that + * they may still act on the apps it targets. + * + * This lives in its own module because the request path and the scheduled job both need + * it, including the legacy allowance below, and two copies of that would drift. + */ + +'use strict'; + +const FEATURE_NAME = 'alerts'; + +/** + * Apps a member reaches only through the legacy fields. + * + * Members created before the permission object have none, and the right helpers then + * fall through to admin_of alone, so somebody who is merely user_of an app looks + * unauthorized. Without this they would lose the ability to manage their own alerts, and + * their alerts would stop being delivered. /o/alert/list makes the same allowance; all + * three have to agree or an alert could be listed and not editable, or editable and not + * delivered. + * + * @param {object} member - member object + * @returns {string[]} app ids granted by the legacy fields, empty for modern members + */ +function legacyApps(member) { + if (!member || typeof member.permission !== "undefined") { + return []; + } + return Array.isArray(member.user_of) ? member.user_of.map(String) : []; +} + +/** + * Whether the member currently holds a right on every app in the list. + * + * An empty or missing list returns false: an alert that targets nothing is not something + * to authorize permissively. + * + * @param {function} rightFn - hasCreateRight / hasUpdateRight / hasReadRight + * @param {object} member - member object + * @param {Array} apps - app ids the alert targets + * @returns {boolean} true only when the member holds the right for every app + */ +function memberHasRightForAllApps(rightFn, member, apps) { + if (!member || !Array.isArray(apps) || apps.length === 0) { + return false; + } + if (member.global_admin) { + return true; + } + const legacy = legacyApps(member); + return apps.every(function(appId) { + return rightFn(FEATURE_NAME, appId + "", member) || legacy.indexOf(appId + "") > -1; + }); +} + +/** + * Whether the member may read one app's alerts. Used by the scheduled job, which deals + * with a single app rather than an alert's whole target list. + * + * @param {function} readRightFn - hasReadRight from rights.js + * @param {object} member - member object + * @param {string} appId - app id + * @returns {boolean} true when the member may read that app's alerts + */ +function memberMayReadApp(readRightFn, member, appId) { + if (!member) { + return false; + } + if (member.global_admin) { + return true; + } + return readRightFn(FEATURE_NAME, appId + "", member) || legacyApps(member).indexOf(appId + "") > -1; +} + +module.exports = { + FEATURE_NAME, + legacyApps, + memberHasRightForAllApps, + memberMayReadApp +}; diff --git a/test/unit-tests/plugins.alerts.app-authorization.js b/test/unit-tests/plugins.alerts.app-authorization.js new file mode 100644 index 00000000000..18e17db2fa8 --- /dev/null +++ b/test/unit-tests/plugins.alerts.app-authorization.js @@ -0,0 +1,109 @@ +require("should"); +var authz = require("../../plugins/alerts/api/parts/app-authorization.js"); +var rights = require("../../api/utils/rights.js"); + +// These decide whether a member may act on the apps an alert targets. Both directions +// matter equally: too permissive and an alert for a revoked app stays editable and keeps +// being delivered, too strict and members lose the ability to manage alerts they own. +// +// The legacy cases are the ones worth having tests for. Members created before the +// permission object reach apps through user_of, and the right helpers fall through to +// admin_of alone, so a strict reading would judge them unauthorized and both stop their +// alerts and lock them out of their own configuration. +describe("alerts app authorization", function() { + var modernMember = { + _id: "m1", + permission: { + _: {a: [], u: [["appA"]]}, + c: {appA: {all: false, allowed: {alerts: true}}}, + r: {appA: {all: false, allowed: {alerts: true}}}, + u: {appA: {all: false, allowed: {alerts: true}}}, + d: {} + } + }; + var legacyMember = {_id: "m2", user_of: ["appL"], admin_of: []}; + var legacyAdminMember = {_id: "m3", user_of: ["appL2"], admin_of: ["appL2"]}; + var globalAdmin = {_id: "m4", global_admin: true}; + + describe("legacyApps", function() { + it("returns nothing for a member with a permission object", function() { + authz.legacyApps(modernMember).should.eql([]); + }); + it("returns user_of for a member without one", function() { + authz.legacyApps(legacyMember).should.eql(["appL"]); + }); + it("tolerates a member with neither", function() { + authz.legacyApps({_id: "x"}).should.eql([]); + authz.legacyApps(null).should.eql([]); + }); + it("stringifies ids, since they may be stored as ObjectIds", function() { + var oid = { + toString: function() { + return "appZ"; + } + }; + authz.legacyApps({user_of: [oid]}).should.eql(["appZ"]); + }); + }); + + describe("memberHasRightForAllApps", function() { + it("allows an app the member holds the right on", function() { + authz.memberHasRightForAllApps(rights.hasUpdateRight, modernMember, ["appA"]).should.equal(true); + }); + it("refuses an app the member does not", function() { + authz.memberHasRightForAllApps(rights.hasUpdateRight, modernMember, ["appB"]).should.equal(false); + }); + it("requires every app, not just one of them", function() { + // an alert targeting two apps is only editable by someone who may touch both + authz.memberHasRightForAllApps(rights.hasUpdateRight, modernMember, ["appA", "appB"]).should.equal(false); + }); + it("allows a legacy member their user_of app", function() { + // without this a legacy member could not edit an alert they own + authz.memberHasRightForAllApps(rights.hasUpdateRight, legacyMember, ["appL"]).should.equal(true); + }); + it("refuses a legacy member an app outside user_of", function() { + authz.memberHasRightForAllApps(rights.hasUpdateRight, legacyMember, ["appOther"]).should.equal(false); + }); + it("allows a legacy admin_of member", function() { + authz.memberHasRightForAllApps(rights.hasUpdateRight, legacyAdminMember, ["appL2"]).should.equal(true); + }); + it("allows a global admin anything", function() { + authz.memberHasRightForAllApps(rights.hasUpdateRight, globalAdmin, ["anything", "else"]).should.equal(true); + }); + it("refuses an empty or missing target list", function() { + // an alert that targets nothing is not something to wave through + authz.memberHasRightForAllApps(rights.hasUpdateRight, modernMember, []).should.equal(false); + authz.memberHasRightForAllApps(rights.hasUpdateRight, modernMember, undefined).should.equal(false); + authz.memberHasRightForAllApps(rights.hasUpdateRight, modernMember, null).should.equal(false); + }); + it("refuses a missing member", function() { + authz.memberHasRightForAllApps(rights.hasUpdateRight, null, ["appA"]).should.equal(false); + }); + it("distinguishes the rights, so read access does not grant update", function() { + var readOnly = { + _id: "m5", + permission: {_: {a: [], u: [["appR"]]}, c: {}, r: {appR: {all: false, allowed: {alerts: true}}}, u: {}, d: {}} + }; + authz.memberHasRightForAllApps(rights.hasReadRight, readOnly, ["appR"]).should.equal(true); + authz.memberHasRightForAllApps(rights.hasUpdateRight, readOnly, ["appR"]).should.equal(false); + }); + }); + + describe("memberMayReadApp", function() { + it("allows the app a member can read", function() { + authz.memberMayReadApp(rights.hasReadRight, modernMember, "appA").should.equal(true); + }); + it("refuses an app the member cannot read", function() { + authz.memberMayReadApp(rights.hasReadRight, modernMember, "appB").should.equal(false); + }); + it("allows a legacy member their user_of app, so their alerts keep arriving", function() { + authz.memberMayReadApp(rights.hasReadRight, legacyMember, "appL").should.equal(true); + }); + it("allows a global admin", function() { + authz.memberMayReadApp(rights.hasReadRight, globalAdmin, "whatever").should.equal(true); + }); + it("refuses a missing member, which is how a deleted owner looks", function() { + authz.memberMayReadApp(rights.hasReadRight, null, "appA").should.equal(false); + }); + }); +});