From 6a5509a90ef0f0cf55c9e2075923fb5a3f453a06 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 00:59:44 +0300 Subject: [PATCH 1/3] fix(reports): authorize non-core report targets on create and update /i/reports/create and /i/reports/update authorized the target apps only when the effective report_type was "core". A non-core report_type names the plugin that owns the report, and those reports were inserted and updated without any authorization of the object they point at. The authorization path for this already existed but was unreachable. The dashboards plugin implements /report/authorize, which checks view access to the dashboard a report renders, while the reports-side caller that invoked it (validateNonCoreUser) had been commented out. Restores that caller and gates both create and update on its result. Update authorizes the merged stored-plus-payload report rather than the payload alone, so a partial update cannot leave an unauthorized target in place, and repointing an existing report at a different target is checked too. A copy is passed so the authorize flag, which is only how the dispatch returns its result, never reaches the written document. Fails closed, matching the original intent of the commented-out code: a report type whose plugin does not answer /report/authorize is not authorized. Only "dashboards" implements it today, in this repo and in countly-platform, so a plugin adding a new report type needs to implement the event for it. Also drops validateCoreUser, which was commented out and superseded by the inline per-app check that uses the reports feature permission rather than plain app membership. Co-Authored-By: Claude --- plugins/reports/api/api.js | 164 ++++++++++++++++++++++--------------- 1 file changed, 99 insertions(+), 65 deletions(-) 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); }); } - */ }()); From 9c5bf845d13b24b22badde30f40f23ee82ca4355 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 01:15:10 +0300 Subject: [PATCH 2/3] test(reports): cover authorization of non-core report targets Real end-to-end coverage through the HTTP endpoints with a real non-admin member, so the whole path is exercised rather than the branch in isolation: validateCreate, the report_type branch, the /report/authorize dispatch into the dashboards plugin, and the insert. - a member with reports rights but no view access to a private dashboard is refused when scheduling a report against it - the same member succeeds for a dashboard they own, which proves the authorize dispatch resolves rather than the request simply failing - the authorize flag does not appear on the stored report - a core report for an app the member has rights on still succeeds, so the restructuring did not change the core path Not executed locally: the harness needs COUNTLY_TEST_API_KEY_ADMIN and COUNTLY_TEST_APP_ID. CI runs them in test-api-plugins. Co-Authored-By: Claude --- CHANGELOG.md | 1 + plugins/reports/tests.js | 188 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 189 insertions(+) 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/tests.js b/plugins/reports/tests.js index b6d0db622b8..1c7b6891ade 100644 --- a/plugins/reports/tests.js +++ b/plugins/reports/tests.js @@ -316,3 +316,191 @@ 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 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 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() { + request.get('/i/dashboards/delete?api_key=' + API_KEY_ADMIN + '&dashboard_id=' + adminDashboardId) + .end(function() { + request.get('/i/dashboards/delete?api_key=' + API_KEY_ADMIN + '&dashboard_id=' + memberDashboardId) + .end(function() { + request.get('/i/users/delete?api_key=' + API_KEY_ADMIN + '&args=' + encodeURIComponent(JSON.stringify({user_ids: [memberUserId]}))) + .end(function() { + done(); + }); + }); + }); + } + 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(); + } + }); + }); + }); + }); +}); From 5fb1a6e2aac12fb4c063dc4872f1e1b508613890 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 17:57:28 +0300 Subject: [PATCH 3/3] test(reports): cover a dashboards report for a shared dashboard 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. The suite proved a member is refused a dashboard they cannot view, and allowed their own, but nothing covered the case in between, which is the one a tightening change could plausibly break. This uses the same admin-owned dashboard the member was already refused, changing only the share, so the test isolates the share as the deciding factor rather than app rights. Also generalised the cleanup to delete a list of dashboards rather than two hard-coded ids. Co-Authored-By: Claude --- plugins/reports/tests.js | 59 ++++++++++++++++++++++++++++++++++------ 1 file changed, 51 insertions(+), 8 deletions(-) diff --git a/plugins/reports/tests.js b/plugins/reports/tests.js index 1c7b6891ade..0d730d150e5 100644 --- a/plugins/reports/tests.js +++ b/plugins/reports/tests.js @@ -334,6 +334,7 @@ describe('Testing Reports non-core authorization', function() { var APP_ID = ""; var adminDashboardId = ""; var memberDashboardId = ""; + var sharedDashIdForCleanup = ""; var memberApiKey = ""; var memberUserId = ""; var uniq = Date.now(); @@ -436,6 +437,39 @@ describe('Testing Reports non-core authorization', function() { }); }); + 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) @@ -478,16 +512,25 @@ describe('Testing Reports non-core authorization', function() { * @returns {void} */ function cleanupRest() { - request.get('/i/dashboards/delete?api_key=' + API_KEY_ADMIN + '&dashboard_id=' + adminDashboardId) - .end(function() { - request.get('/i/dashboards/delete?api_key=' + API_KEY_ADMIN + '&dashboard_id=' + memberDashboardId) + 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() { - request.get('/i/users/delete?api_key=' + API_KEY_ADMIN + '&args=' + encodeURIComponent(JSON.stringify({user_ids: [memberUserId]}))) - .end(function() { - done(); - }); + 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();