diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d91cd35b8e..212756a28d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ ## Version 25.03.xx Fixes: +- [reports] Non-core reports, such as dashboard reports, now authorize the object they target on create and update - [events] Fix sum chart tooltip displaying the raw floating-point value instead of a rounded number ## Version 25.03.50 diff --git a/plugins/reports/api/api.js b/plugins/reports/api/api.js index 3496a56be04..f82d97bc919 100644 --- a/plugins/reports/api/api.js +++ b/plugins/reports/api/api.js @@ -265,6 +265,24 @@ const FEATURE_NAME = 'reports'; //report for arbitrary apps. props.report_type = props.report_type || "core"; + /** + * Insert the authorized report document + * @returns {void} + */ + function insertReport() { + common.db.collection('reports').insert(props, function(err0, result) { + result = result.ops; + if (err0) { + err0 = err0.err; + common.returnMessage(params, 200, err0); + } + else { + plugins.dispatch("/systemlogs", {params: params, action: "reports_create", data: result[0]}); + common.returnMessage(params, 200, "Success"); + } + }); + } + if (props.report_type === "core") { if (!props.apps || !Array.isArray(props.apps) || props.apps.length === 0) { common.returnMessage(params, 400, 'Invalid or missing apps'); @@ -284,19 +302,22 @@ const FEATURE_NAME = 'reports'; return common.returnMessage(params, 401, 'User does not have right to access this information'); } } + insertReport(); + } + else { + //a non-core report targets another plugin's object - a + //"dashboards" report renders the dashboard named in + //props.dashboards - and only that plugin knows whether the + //member may use it. Previously nothing checked this, so any + //member with reports-create rights could schedule a report + //against an arbitrary dashboard id. + validateNonCoreUser(params, props, function(authErr, authorized) { + if (!authorized) { + return common.returnMessage(params, 401, 'User does not have right to access this information'); + } + insertReport(); + }); } - - common.db.collection('reports').insert(props, function(err0, result) { - result = result.ops; - if (err0) { - err0 = err0.err; - common.returnMessage(params, 200, err0); - } - else { - plugins.dispatch("/systemlogs", {params: params, action: "reports_create", data: result[0]}); - common.returnMessage(params, 200, "Success"); - } - }); }); break; case 'update': @@ -345,35 +366,59 @@ const FEATURE_NAME = 'reports'; //treated as a non-core type to skip the per-app check. var effectiveType = props.report_type || report.report_type || "core"; - if (effectiveType === "core" && typeof props.apps !== "undefined") { - if (!Array.isArray(props.apps) || props.apps.length === 0) { - return common.returnMessage(params, 400, 'Invalid or missing apps'); - } - if (!params.member.global_admin) { - let allowedApps = (getAdminApps(params.member) || []) - .concat(getUserAppsForFeaturePermission(params.member, FEATURE_NAME, 'r') || []); - if (typeof params.member.permission === "undefined" && Array.isArray(params.member.user_of)) { - allowedApps = allowedApps.concat(params.member.user_of); + /** + * Apply the authorized update + * @returns {void} + */ + function applyUpdate() { + common.db.collection('reports').update(recordUpdateOrDeleteQuery(params, id), {$set: props}, function(err_update2) { + if (err_update2) { + err_update2 = err_update2.err; + common.returnMessage(params, 200, err_update2); } - let notPermitted = props.apps.some(function(appId) { - return allowedApps.indexOf(appId) === -1; - }); - if (notPermitted) { - return common.returnMessage(params, 401, 'User does not have right to access this information'); + else { + plugins.dispatch("/systemlogs", {params: params, action: "reports_edited", data: {_id: id, before: report, update: props}}); + common.returnMessage(params, 200, "Success"); } - } + }); } - common.db.collection('reports').update(recordUpdateOrDeleteQuery(params, id), {$set: props}, function(err_update2) { - if (err_update2) { - err_update2 = err_update2.err; - common.returnMessage(params, 200, err_update2); - } - else { - plugins.dispatch("/systemlogs", {params: params, action: "reports_edited", data: {_id: id, before: report, update: props}}); - common.returnMessage(params, 200, "Success"); + if (effectiveType === "core") { + if (typeof props.apps !== "undefined") { + if (!Array.isArray(props.apps) || props.apps.length === 0) { + return common.returnMessage(params, 400, 'Invalid or missing apps'); + } + if (!params.member.global_admin) { + let allowedApps = (getAdminApps(params.member) || []) + .concat(getUserAppsForFeaturePermission(params.member, FEATURE_NAME, 'r') || []); + if (typeof params.member.permission === "undefined" && Array.isArray(params.member.user_of)) { + allowedApps = allowedApps.concat(params.member.user_of); + } + let notPermitted = props.apps.some(function(appId) { + return allowedApps.indexOf(appId) === -1; + }); + if (notPermitted) { + return common.returnMessage(params, 401, 'User does not have right to access this information'); + } + } } - }); + applyUpdate(); + } + else { + //authorize the merged report, not just the payload: a + //partial update may leave the target (props.dashboards) + //in the stored document, and repointing an existing + //report at another dashboard must be checked too. A copy + //is passed so the authorize flag never reaches $set. + var mergedReport = Object.assign({}, report, props); + mergedReport.report_type = effectiveType; + validateNonCoreUser(params, mergedReport, function(authErr, authorized) { + if (!authorized) { + return common.returnMessage(params, 401, 'User does not have right to access this information'); + } + applyUpdate(); + }); + } }); }); break; @@ -623,40 +668,29 @@ const FEATURE_NAME = 'reports'; } /** - * validation function for verifing user have permission to access infomation or not for core type of report - * @param {object} params - request params object - * @param {object} props - report related props - * @param {func} cb - callback function - * @return {func} cb - callback function - - function validateCoreUser(params, props, cb) { - var userApps = getUserApps(params.member); - var apps = props.apps; - var isAppUser = apps.every(function(app) { - return userApps && userApps.indexOf(app) > -1; - }); - - if (!params.member.global_admin && !isAppUser) { - return cb(null, false); - } - else { - return cb(null, true); - } - - } - */ - - /** - * validation function for verifing user have permission to access infomation or not for not core type of report + * Verify the member may use a non-core report's target. + * + * A non-core report_type names the plugin that owns the report, and that + * plugin authorizes the target through the /report/authorize event: the + * dashboards plugin checks view access to report.dashboards there. This + * fails closed, so a report type whose plugin does not answer the event is + * not authorized. A plugin adding a new report type must implement + * /report/authorize for it. + * + * Core reports are authorized inline against the member's per-app reports + * permission instead, so they never reach this. + * * @param {object} params - request params object - * @param {object} props - report related props - * @param {func} cb - callback function - + * @param {object} props - report related props + * @param {function} cb - callback receiving (err, authorized) + */ function validateNonCoreUser(params, props, cb) { plugins.dispatch("/report/authorize", { params: params, report: props }, function() { var authorized = props.authorized || false; + //the flag is only how the dispatch reports its result back; it must + //not be persisted onto the report document + delete props.authorized; cb(null, authorized); }); } - */ }()); diff --git a/plugins/reports/tests.js b/plugins/reports/tests.js index b6d0db622b8..0d730d150e5 100644 --- a/plugins/reports/tests.js +++ b/plugins/reports/tests.js @@ -316,3 +316,234 @@ describe('Testing Reports', function() { }); }); + +// Regression tests for authorization of non-core report targets. +// +// A non-core report_type names the plugin that owns the report, and that plugin +// authorizes the target through /report/authorize: the dashboards plugin checks +// view access to the dashboard a "dashboards" report renders. That caller was +// commented out, so a member with reports rights could schedule a report against +// any dashboard id, including a private dashboard belonging to someone else. +// +// These go through the real HTTP endpoints with a real non-admin member, so the +// whole path is exercised: validateCreate, the report_type branch, the +// /report/authorize dispatch into the dashboards plugin, and the insert. + +describe('Testing Reports non-core authorization', function() { + var API_KEY_ADMIN = ""; + var APP_ID = ""; + var adminDashboardId = ""; + var memberDashboardId = ""; + var sharedDashIdForCleanup = ""; + var memberApiKey = ""; + var memberUserId = ""; + var uniq = Date.now(); + + /** + * Build a dashboards-type report config + * @param {string} dashboardId - dashboard the report renders + * @returns {object} report config + */ + function dashboardReport(dashboardId) { + return { + title: "noncore-authz-" + uniq, + report_type: "dashboards", + dashboards: dashboardId, + emails: ["a@abc.com"], + frequency: "daily", + timezone: "Europe/Tirane", + hour: 4, + minute: 0, + sendPdf: true + }; + } + + it('should create a private dashboard as admin', function(done) { + API_KEY_ADMIN = testUtils.get("API_KEY_ADMIN"); + APP_ID = testUtils.get("APP_ID"); + request.get('/i/dashboards/create?api_key=' + API_KEY_ADMIN + '&name=ReportsPrivateDash' + uniq + '&share_with=none') + .expect(200) + .end(function(err, res) { + if (err) { + return done(err); + } + adminDashboardId = JSON.parse(res.text); + should.exist(adminDashboardId); + done(); + }); + }); + + it('should create a non-admin member with reports rights on the test app', function(done) { + var permission = { + _: {a: [], u: [[APP_ID]]}, + c: {}, + r: {}, + u: {}, + d: {} + }; + permission.c[APP_ID] = {all: false, allowed: {reports: true}}; + permission.r[APP_ID] = {all: false, allowed: {reports: true}}; + permission.u[APP_ID] = {all: false, allowed: {reports: true}}; + var userParams = { + full_name: "reportsmember" + uniq, + username: "reportsmember" + uniq, + password: "p4ssw0rD!", + email: "reportsmember" + uniq + "@mail.test", + permission: permission + }; + request.get('/i/users/create?api_key=' + API_KEY_ADMIN + '&args=' + encodeURIComponent(JSON.stringify(userParams))) + .expect(200) + .end(function(err, res) { + if (err) { + return done(err); + } + memberApiKey = res.body.api_key; + memberUserId = res.body._id; + should.exist(memberApiKey); + done(); + }); + }); + + it('should reject a dashboards report for a dashboard the member cannot view', function(done) { + request.get('/i/reports/create?api_key=' + memberApiKey + '&app_id=' + APP_ID + + '&args=' + encodeURIComponent(JSON.stringify(dashboardReport(adminDashboardId)))) + .expect(401) + .end(function(err) { + if (err) { + return done(err); + } + done(); + }); + }); + + it('should allow a dashboards report for the member own dashboard', function(done) { + request.get('/i/dashboards/create?api_key=' + memberApiKey + '&name=MemberDash' + uniq + '&share_with=none') + .expect(200) + .end(function(err, res) { + if (err) { + return done(err); + } + memberDashboardId = JSON.parse(res.text); + request.get('/i/reports/create?api_key=' + memberApiKey + '&app_id=' + APP_ID + + '&args=' + encodeURIComponent(JSON.stringify(dashboardReport(memberDashboardId)))) + .expect(200) + .end(function(e, r) { + if (e) { + return done(e); + } + r.body.should.have.property('result', 'Success'); + done(); + }); + }); + }); + + it('should allow a dashboards report for a dashboard shared with the member', function(done) { + // Dashboard permissions are deliberately separate from app permissions: a + // dashboard can be shared with a member who has no access to the apps its + // widgets reference, and they are meant to be able to view it and schedule a + // report for it. This is the same admin-owned dashboard the member was + // refused above, so the only thing that changes here is the share, which is + // what proves authorization follows the share and not app rights. + var sharedDashboardId = ""; + request.get('/i/dashboards/create?api_key=' + API_KEY_ADMIN + + '&name=ReportsSharedDash' + uniq + + '&share_with=selected-users' + + '&shared_email_view=' + encodeURIComponent(JSON.stringify(["reportsmember" + uniq + "@mail.test"]))) + .expect(200) + .end(function(err, res) { + if (err) { + return done(err); + } + sharedDashboardId = JSON.parse(res.text); + should.exist(sharedDashboardId); + request.get('/i/reports/create?api_key=' + memberApiKey + '&app_id=' + APP_ID + + '&args=' + encodeURIComponent(JSON.stringify(dashboardReport(sharedDashboardId)))) + .expect(200) + .end(function(e, r) { + if (e) { + return done(e); + } + r.body.should.have.property('result', 'Success'); + sharedDashIdForCleanup = sharedDashboardId; + done(); + }); + }); + }); + + it('should not persist the authorize flag on the stored report', function(done) { + request.get('/o/reports/all?api_key=' + memberApiKey + '&app_id=' + APP_ID) + .expect(200) + .end(function(err, res) { + if (err) { + return done(err); + } + res.body.forEach(function(r) { + r.should.not.have.property('authorized'); + }); + done(); + }); + }); + + it('should still allow a core report for an app the member has rights on', function(done) { + var coreReport = Object.assign({}, newReport, {apps: [APP_ID], title: "noncore-core-control-" + uniq}); + request.get('/i/reports/create?api_key=' + memberApiKey + '&app_id=' + APP_ID + + '&args=' + encodeURIComponent(JSON.stringify(coreReport))) + .expect(200) + .end(function(err, res) { + if (err) { + return done(err); + } + res.body.should.have.property('result', 'Success'); + done(); + }); + }); + + after(function(done) { + // remove everything this block created: the reports it scheduled, both + // dashboards, and the member + request.get('/o/reports/all?api_key=' + API_KEY_ADMIN + '&app_id=' + APP_ID) + .end(function(listErr, listRes) { + var created = ((listRes && listRes.body) || []).filter(function(r) { + return typeof r.title === "string" && r.title.indexOf("" + uniq) > -1; + }); + var pending = created.length; + /** + * Delete the dashboards and the member once reports are gone + * @returns {void} + */ + function cleanupRest() { + var dashboards = [adminDashboardId, memberDashboardId, sharedDashIdForCleanup].filter(Boolean); + /** + * Delete the dashboards one at a time, then the member + * @param {number} i - index into dashboards + * @returns {void} + */ + function deleteDashboard(i) { + if (i >= dashboards.length) { + return request.get('/i/users/delete?api_key=' + API_KEY_ADMIN + '&args=' + encodeURIComponent(JSON.stringify({user_ids: [memberUserId]}))) + .end(function() { + done(); + }); + } + request.get('/i/dashboards/delete?api_key=' + API_KEY_ADMIN + '&dashboard_id=' + dashboards[i]) + .end(function() { + deleteDashboard(i + 1); + }); + } + deleteDashboard(0); + } + if (!pending) { + return cleanupRest(); + } + created.forEach(function(r) { + request.get('/i/reports/delete?api_key=' + API_KEY_ADMIN + '&app_id=' + APP_ID + '&args=' + encodeURIComponent(JSON.stringify({_id: r._id}))) + .end(function() { + pending--; + if (!pending) { + cleanupRest(); + } + }); + }); + }); + }); +});