fix(api): hide staff comment fields on public reads

api/aggregators/itemAggregator.js

@@ -28,7 +28,10 @@ exports.resolveParentGate = async (schemaName, roles) => {
   }
 
   if (schemaName === constants.COMMENT) {
-    return { unreadablePeriodIds: await parentRead.unreadablePeriodIds(roles) };
+    // The public sees a comment only under a period it can read; staff also see one whose period is gone.
+    return updateRules.isPublicCaller(roles)
+      ? { readablePeriodIds: await parentRead.readablePeriodIds(roles) }
+      : { unreadablePeriodIds: await parentRead.unreadablePeriodIds(roles) };
   }
 
   return {};
@@ -57,7 +60,8 @@ exports.createItemAggr = (itemId, schemaName, roles, gate) => {
 
   aggregation.push(
     {
-      '$match': { _id: new mongoose.Types.ObjectId(itemId) }
+      // Every model shares the epic collection: without the schema name, any allowed name reads any record.
+      '$match': { _id: new mongoose.Types.ObjectId(itemId), _schemaName: schemaName }
     },
     {
       $redact: {
@@ -95,8 +99,10 @@ exports.createItemAggr = (itemId, schemaName, roles, gate) => {
     aggregation.push(...parentRead.parentReadMatch(gate.unreadableParentIds));
   } else if (schemaName === constants.COMMENT) {
     // A comment hangs off a comment period, which hangs off a project. Both levels gate it, and
-    // unreadablePeriodIds already folds the project level into the period set.
-    aggregation.push(...parentRead.parentReadMatch(gate.unreadablePeriodIds, 'period'));
+    // both period sets already fold the project level in.
+    aggregation.push(...(gate.readablePeriodIds
+      ? parentRead.readablePeriodMatch(gate.readablePeriodIds)
+      : parentRead.parentReadMatch(gate.unreadablePeriodIds, 'period')));
   }
 
   if (schemaName === constants.INSPECTION) {

api/controllers/comment.js

@@ -3,35 +3,16 @@ var mongoose = require('mongoose');
 var Actions = require('../helpers/actions');
 var Utils = require('../helpers/utils');
 const demiPush = require('../helpers/demiPush');
+const parentRead = require('../helpers/parentRead');
 const { DateTime } = require('luxon');
-const { COMMENT_STAFF_FIELDS } = require('../helpers/constants');
-
-const ALLOWED_FIELDS = [
-  'author',
-  'comment',
-  'commentId',
-  'dateAdded',
-  'datePosted',
-  'dateUpdated',
-  'documents',
-  'eaoNotes',
-  'eaoStatus',
-  'submittedCAC',
-  'isAnonymous',
-  'location',
-  'period',
-  'proponentNotes',
-  'proponentStatus',
-  'publishedNotes',
-  'rejectedNotes',
-  'rejectedReason',
-  'valuedComponents',
-  'read',
-  'write',
-  'delete'
-];
-// Staff review fields stay off the public routes.
-const PUBLIC_FIELDS = ALLOWED_FIELDS.filter(f => !COMMENT_STAFF_FIELDS.includes(f));
+const { COMMENT_FIELDS: ALLOWED_FIELDS, COMMENT_PUBLIC_FIELDS: PUBLIC_FIELDS, PUBLIC_ROLES } = require('../helpers/constants');
+
+// A comment's own read[] stays Published when staff unpublish its period or project, so the
+// public routes keep only comments under a period the public can read, as dataset=Item does.
+const publicPeriodGate = async () => parentRead.readablePeriodMatch(await parentRead.readablePeriodIds(PUBLIC_ROLES));
+
+// Sorting on a field the reply hides would leak its order, so the public sorts on these only.
+const PUBLIC_SORT_FIELDS = ['commentId', 'dateAdded'];
 
 // Hiding an anonymous author needs isAnonymous: read it with author, drop it when the caller did not ask.
 const publicCommentQuery = fields => {
@@ -110,7 +91,8 @@ exports.publicHead = async function (args, res) {
     null, // sort
     null, // skip
     null, // limit
-    true); // count
+    true, // count
+    await publicPeriodGate()); // pre-query pipeline steps
   Utils.recordAction('Head', 'CommentPeriod', 'public');
   // /api/comment/ route, return 200 OK with 0 items if necessary
   if (!(args.swagger.params.period && args.swagger.params.period.value) || (data && data.length > 0)) {
@@ -133,13 +115,15 @@ exports.publicGet = async function (args, res) {
     if (args.swagger.params.period && args.swagger.params.period.value) {
       query = Utils.buildQuery('period', args.swagger.params.period.value, query);
     }
-    // Sort
     if (args.swagger.params.sortBy && args.swagger.params.sortBy.value) {
-      args.swagger.params.sortBy.value.forEach(function (value) {
-        var order_by = value.charAt(0) == '-' ? -1 : 1;
-        var sort_by = value.slice(1);
-        sort[sort_by] = order_by;
-      }, this);
+      args.swagger.params.sortBy.value.forEach(value => {
+        const sortBy = value.slice(1);
+        if (PUBLIC_SORT_FIELDS.includes(sortBy)) {
+          sort[sortBy] = value.charAt(0) === '-' ? -1 : 1;
+        } else {
+          defaultLog.debug('publicGet: ignoring sort on %s', sortBy);
+        }
+      });
     }
 
     var processedParameters = Utils.getSkipLimitParameters(args.swagger.params.pageSize, args.swagger.params.pageNum);
@@ -159,7 +143,8 @@ exports.publicGet = async function (args, res) {
     sort, // sort
     skip, // skip
     limit, // limit
-    true); // count
+    true, // count
+    await publicPeriodGate()); // pre-query pipeline steps
 
   const results = data[0] ? data[0].results : [];
   results.forEach(commentQuery.redact);
@@ -299,7 +284,7 @@ exports.protectedGet = async function (args, res) {
       limit, // limit
       count); // count
     Utils.recordAction('Get', 'Comment', args.swagger.params.auth_payload.preferred_username, args.swagger.params.commentId && args.swagger.params.commentId.value ? args.swagger.params.commentId.value : null);
-    defaultLog.info('Got comment(s):', data);
+    defaultLog.info('Got %d comment(s)', count ? (data[0] ? data[0].total_items : 0) : data.length);
 
     // This is to get the next pending comment information.
     if (args.swagger.params.populateNextComment && args.swagger.params.populateNextComment.value) {
@@ -332,7 +317,7 @@ exports.protectedGet = async function (args, res) {
 //  Create a new Comment
 exports.protectedPost = async function (args, res) {
   var obj = args.swagger.params.comment.value;
-  defaultLog.info('Incoming new comment:', obj);
+  defaultLog.info('Incoming new comment for period: %s', obj.period);
 
   var Comment = mongoose.model('Comment');
 
@@ -383,7 +368,7 @@ exports.protectedPost = async function (args, res) {
   try {
     var c = await comment.save();
     Utils.recordAction('Post', 'Comment', args.swagger.params.auth_payload.preferred_username, c._id);
-    defaultLog.info('Saved new comment object:', c);
+    defaultLog.info('Saved new comment object: %s', c._id);
     return Actions.sendMirrored(res, 200, c, demiPush.comment(c));
   } catch (e) {
     defaultLog.error(`Error: ${e.message}`);
@@ -401,7 +386,7 @@ async function getNextCommentIdCount(period) {
 exports.unProtectedPost = async function (args, res) {
   try {
     const obj = args.swagger.params.comment.value;
-    defaultLog.info('Incoming new object:', obj);
+    defaultLog.info('Incoming new comment for period: %s', obj.period);
     if (args.swagger.params.period && args.swagger.params.period.value && !mongoose.Types.ObjectId.isValid(args.swagger.params.period.value)) {
       return Actions.sendResponse(res, 400, { });
     }
@@ -411,7 +396,7 @@ exports.unProtectedPost = async function (args, res) {
     const commentPeriodModel = mongoose.model('CommentPeriod');
     const period = await commentPeriodModel.findOne({ _id: new mongoose.Types.ObjectId(obj.period) });
 
-    defaultLog.info('Period:', period);
+    defaultLog.info('Period: %s', period && period._id);
 
     if (DateTime.now() > DateTime.fromJSDate(new Date(period.dateCompleted))) {
       // Past the specified time
@@ -489,7 +474,7 @@ exports.protectedPut = async function (args, res) {
   };
   comment = setPermissionsFromEaoStatus(obj.eaoStatus, comment);
 
-  defaultLog.info('Incoming updated object:', comment);
+  defaultLog.info('Updating comment %s to status %s', objId, comment.eaoStatus);
 
   try {
     var c = await Comment.updateOne({ _id: objId }, { $set: comment });

api/controllers/search.js

@@ -6,7 +6,7 @@ const Actions = require('../helpers/actions');
 const Utils = require('../helpers/utils');
 const analytics = require('../helpers/analytics');
 const constants = require('../helpers/constants').schemaTypes;
-const { COMMENT_STAFF_FIELDS } = require('../helpers/constants');
+const { COMMENT_PUBLIC_FIELDS } = require('../helpers/constants');
 
 const documentAggregator = require('../aggregators/documentAggregator');
 const projectAggregator = require('../aggregators/projectAggregator');
@@ -34,6 +34,19 @@ const PROJECT_JOINED_FIELDS = ['proponent', 'currentPhaseName', 'eacDecision', '
 // Project fields still on the root before the page is cut; unwindProjectData copies them into default after it.
 const PROJECT_ROOT_SORT_FIELDS = ['_id', ...aggregateHelper.PROJECT_ROOT_FIELDS];
 
+const PUBLIC_COMMENT_ITEM_FIELDS = ['_id', '_schemaName', ...COMMENT_PUBLIC_FIELDS];
+
+// A public caller gets only the public comment fields; the author stays hidden unless the commenter chose to show it.
+const commentItemView = (item, isAuthenticated) => {
+  const view = isAuthenticated
+    ? { ...item }
+    : Object.fromEntries(PUBLIC_COMMENT_ITEM_FIELDS.filter(f => f in item).map(f => [f, item[f]]));
+  if (item.isAnonymous !== false) {
+    delete view.author;
+  }
+  return view;
+};
+
 const searchCollection = async function (roles, keywords, schemaName, pageNum, pageSize, project, projectLegislation, sortField = undefined, sortDirection = undefined, caseSensitive, populate = false, and, or, sortingValue, categorized, fuzzy, signedIn) {
   defaultLog.debug('collation:', aggregateHelper.AGGREGATE_COLLATION);
   defaultLog.debug('populate:', populate);
@@ -355,16 +368,6 @@ const executeQuery = async function (args, res) {
       );
     }
 
-    // TODO: this should be moved into the aggregation.
-    if (dataset === constants.COMMENT) {
-      // Filter
-      collectionData[0].searchResults.forEach(function (item) {
-        if (item.isAnonymous !== false) {
-          delete item.author;
-        }
-      });
-    }
-
     return Actions.sendResponse(res, 200, collectionData);
 
   } else if (dataset === constants.ITEM) {
@@ -379,26 +382,18 @@ const executeQuery = async function (args, res) {
     const collectionObj = mongoose.model(schemaNameVal);
     const gate = await itemAggregator.resolveParentGate(schemaNameVal, roles);
     const aggregation = itemAggregator.createItemAggr(args.swagger.params._id.value, schemaNameVal, roles, gate);
-    let data = await collectionObj.aggregate(aggregation).allowDiskUse(true);
+    const data = await collectionObj.aggregate(aggregation).allowDiskUse(true);
 
-    if (schemaNameVal === constants.COMMENT) {
-      // Filter
-      data.forEach(function (item) {
-        if (item.isAnonymous !== false) {
-          delete item.author;
-        }
-        if (!isAuthenticated) {
-          COMMENT_STAFF_FIELDS.forEach(field => delete item[field]);
-        }
-      });
-    }
+    // Keyed on each record's own schema as well as the requested one, in case a record of another
+    // schema ever gets past the $match.
+    const isSchema = (item, schemaName) => schemaNameVal === schemaName || item._schemaName === schemaName;
 
-    if (schemaNameVal === constants.PROJECT) {
-      // If we are a project, and we are not authed, we need to sanitize some fields.
-      data = Utils.filterData(schemaNameVal, data, roles);
-    }
+    // filterData strips fields in place.
+    Utils.filterData(constants.PROJECT, data.filter(item => isSchema(item, constants.PROJECT)), roles);
+
+    const results = data.map(item => (isSchema(item, constants.COMMENT) ? commentItemView(item, isAuthenticated) : item));
 
-    return Actions.sendResponse(res, 200, data);
+    return Actions.sendResponse(res, 200, results);
   } else {
     defaultLog.warn('search.js executeQuery: bad request — missing schemaName');
     return Actions.sendResponse(res, 400, {});

api/helpers/constants.js

@@ -20,8 +20,40 @@ exports.MAX_FEATURE_DOCS = 5;
 exports.PUBLIC_ROLES = ['public'];
 exports.SECURE_ROLES = ['sysadmin', 'staff'];
 
+// Comment fields the comment routes may return.
+const COMMENT_FIELDS = Object.freeze([
+  'author',
+  'comment',
+  'commentId',
+  'dateAdded',
+  'datePosted',
+  'dateUpdated',
+  'documents',
+  'eaoNotes',
+  'eaoStatus',
+  'submittedCAC',
+  'isAnonymous',
+  'location',
+  'period',
+  'proponentNotes',
+  'proponentStatus',
+  'publishedNotes',
+  'rejectedNotes',
+  'rejectedReason',
+  'valuedComponents',
+  'read',
+  'write',
+  'delete'
+]);
+exports.COMMENT_FIELDS = COMMENT_FIELDS;
+
 // Comment fields only signed-in staff may read.
-exports.COMMENT_STAFF_FIELDS = Object.freeze(['eaoNotes', 'eaoStatus', 'proponentNotes', 'rejectedNotes', 'rejectedReason']);
+const COMMENT_STAFF_FIELDS = Object.freeze(['eaoNotes', 'eaoStatus', 'proponentNotes', 'rejectedNotes', 'rejectedReason']);
+exports.COMMENT_STAFF_FIELDS = COMMENT_STAFF_FIELDS;
+
+// Comment fields the public routes may return. write and delete name the staff roles that may edit it.
+const COMMENT_HIDDEN_FROM_PUBLIC = [...COMMENT_STAFF_FIELDS, 'write', 'delete'];
+exports.COMMENT_PUBLIC_FIELDS = Object.freeze(COMMENT_FIELDS.filter(f => !COMMENT_HIDDEN_FROM_PUBLIC.includes(f)));
 
 // Legislation registry, keyed by year. A project keeps one sub-document per year under `legislation_<year>`.
 // Adding an Act: add its entry here and its year to the project POST and publish prose in swagger.yaml (a test checks).

api/helpers/parentRead.js

@@ -49,21 +49,33 @@ exports.unreadableParentIds = async (roles) => {
   return ids;
 };
 
+// A period hides its comments on its own read[] or because its project is unreadable.
+const hidesComments = (roles, unreadableProjects) => [unreadableBy(roles), { project: { $in: unreadableProjects } }];
+
 /**
  * Ids of the comment periods that hide their comments, either on their own `read[]` or because
  * their project hides them. Folding both levels in here keeps the comment gate a single $match.
  */
 exports.unreadablePeriodIds = async (roles) => {
   const parents = await exports.unreadableParentIds(roles);
-  const ids = await idsMatching({
-    _schemaName: 'CommentPeriod',
-    $or: [unreadableBy(roles), { project: { $in: parents } }]
-  });
+  const ids = await idsMatching({ _schemaName: 'CommentPeriod', $or: hidesComments(roles, parents) });
   defaultLog.debug('parent read gate: %d unreadable comment periods', ids.length);
 
   return ids;
 };
 
+/**
+ * Ids of the comment periods whose comments the caller may see: every period unreadablePeriodIds
+ * leaves out. Matching a comment against this set also hides one whose period is unset or gone.
+ */
+exports.readablePeriodIds = async (roles) => {
+  const parents = await exports.unreadableParentIds(roles);
+  const ids = await idsMatching({ _schemaName: 'CommentPeriod', $nor: hidesComments(roles, parents) });
+  defaultLog.debug('parent read gate: %d readable comment periods', ids.length);
+
+  return ids;
+};
+
 /**
  * The gate. $nin does not match a row whose parent field is absent or points at a parent that is
  * gone, so those keep only their own `read[]`, which the caller applies. Empty when the caller can
@@ -81,3 +93,14 @@ exports.parentReadMatch = (unreadableIds, parentField = 'project') => {
 
   return [{ $match: { [parentField]: { $nin: unreadableIds } } }];
 };
+
+/**
+ * The strict comment gate: keeps only comments whose period is in readablePeriodIds.
+ */
+exports.readablePeriodMatch = (readableIds) => {
+  if (!Array.isArray(readableIds)) {
+    throw new TypeError('comment period gate needs an array of readable ids from helpers/parentRead');
+  }
+
+  return [{ $match: { period: { $in: readableIds } } }];
+};

api/swagger/swagger.yaml

@@ -491,8 +491,6 @@ definitions:
       - 'publishedNotes'
       - 'valuedComponents'
       - 'read'
-      - 'write'
-      - 'delete'
   CommentSortByFields:
     type: string
     description: "Optional fields to sort by (prefixed with + or - for order)"
@@ -502,6 +500,14 @@ definitions:
       - -author
       - +dateAdded
       - -dateAdded
+  PublicCommentSortByFields:
+    type: string
+    description: "Fields the public comment routes sort by. Other fields are ignored."
+    enum: &publicCommentSortByFields
+      - +commentId
+      - -commentId
+      - +dateAdded
+      - -dateAdded
 
 ### Comment Period Definitions
   CommentStatus:
@@ -5387,12 +5393,12 @@ paths:
           description: "Number of results per page (if 'pageNum' is specified)."
         - in: query
           name: sortBy
-          description: "Comment fields to sort by."
+          description: "Comment fields to sort by: commentId or dateAdded. Other fields are ignored."
           required: false
           type: array
           items:
             type: string
-            enum: *commentSortByFields
+            enum: *publicCommentSortByFields
       responses:
         "200":
           description: "Success"