GH Issue #1444: Improve permission checking on secure message replies

[labkey-adam]

Rationale

Prevent users who aren't on a secure thread's member list from replying to it, and stop API replies and edits from clearing a thread's member list. See GH Issue 1444.

Changes

• Replies through the UI and API now require permission to respond to the parent thread
• Replies must target the thread itself, not one of its responses
• API replies and edits keep the thread's existing member list
• Integration tests

[labkey-adam]

For manual testing, attempt to POST to this URL, changing the parent value to an existing secure message's GUID. It should reject users who can't read the thread and accept users who can.

announcements-createThread.view?reply=true&thread.title=Reply&thread.body=reply%20body&thread.parent=87ac235b-a0e6-103f-a1e7-a9bb95f66820

Looks like this action requires a non-JSON post, which the query-apiTest.view page doesn't support. To test, I commented out the line that sets content-type to application/json (line 242) in apiTest.jsp.

[labkey-bpatel]

Manual test findings:

Note: These findings are all around the message board APIs (createThread.api and updateThread.api). Since users can call these APIs directly, perhaps they should follow the same rules as the UI.

Test findings 1 and 2 below show places where the API behaves differently from the UI. Test finding 3 is a side effect where an API edit unexpectedly changes the saved notify list.

Setup

1. Create three users: Jim, Michael and Creed.
2. Create two folders, each with a Messages web part. Configure each one in Messages › Admin (Customize):
   a) SecureBoard: Security = On without email; Include Notify List checked. Roles: Jim and Michael are Message Board Contributors.
   b) OpenBoard: Security = Off; Include Notify List checked. Roles: Jim and Creed are Message Board Contributors.
3. API calls are made from the browser devtools console on a page in the step's folder, as the user the step names. Paste this helper again after each page load or impersonation change:
   function call(action, params) {
   LABKEY.Ajax.request({
   url: LABKEY.ActionURL.buildURL('announcements', action + '.api'),
   method: 'POST', params,
   success: r => console.log(r.status, r.responseText),
   failure: r => console.log(r.status, r.responseText)
   });
   }
4. Use this query to see each post's RowId, EntityId and saved notify list:
   SELECT a.RowId, t.Title AS thread,
   CASE WHEN a.Parent IS NULL THEN 'original' ELSE 'response' END AS post,
   a.EntityId, a.Created,
   string_agg(p.Name, ', ' ORDER BY p.Name) AS notify_list
   FROM comm.Announcements a
   JOIN comm.Announcements t ON t.EntityId = COALESCE(a.Parent, a.EntityId)
   LEFT JOIN comm.UserList u ON u.MessageId = a.RowId
   LEFT JOIN core.Principals p ON p.UserId = u.UserId
   GROUP BY a.RowId, t.Title
   ORDER BY a.RowId;

Test findings

1. createThread.api doesn't check that you're replying to the original message, like the UI does
   Folder: SecureBoard

1. As admin, create Message 1 and notify Jim.
2. As Jim, reply to Message 1 through the API:
   call('createThread', {reply:true, 'thread.parent':'<Message 1 EntityId>', 'thread.title':'x', 'thread.body':'x'})
   Result: 200. Jim's reply shows notify list = Jim.
3. As admin, respond to Message 1 with Notify cleared. The admin's response has an empty notify list. Jim's reply still lists Jim, but that list is now out of date.
4. As Jim, try to reply:
   • UI, with Jim's reply as the parent: announcements-respond.view?parentId=<Jim's reply EntityId> returns 404, "Could not find message."
   • UI, with Message 1 as the parent: announcements-respond.view?parentId=<Message 1 EntityId> returns a permission error.
   • API, with Jim's reply as the parent: call('createThread', {reply:true, 'thread.parent':'<Jim's reply EntityId>', 'thread.title':'x', 'thread.body':'x'}) returns 400, "Failed to reply to thread. Could not locate most recent response for thread …"
5. SELECT RowId, LatestId FROM comm.Threads WHERE RowId IN (<Message 1 RowId>, <Jim's reply RowId>);
   This returns only Message 1, with the admin's response as its LatestId. There is no row for Jim's reply, which is the only reason the API call in step 4 fails.

Expected: The API explicitly rejects Jim's reply as a reply parent, the same way the UI does (404, "Could not find message").

2) API replies drop notify-list members who temporarily can't read the thread, permanently and without saying so
Folder: SecureBoard

1. As admin, create Message 2 and notify Jim and Michael. The query shows notify list = Jim, Michael.
2. As admin, deactivate Michael (Admin › Site › Site Users).
3. UI: As Jim, respond to Message 2, leave the prefilled Notify list as it is, and submit.
   Result: rejected with "Can't add Michael to the member list: This user doesn't have permission to read the thread." Nothing is saved.
4. API: As Jim, reply to Message 2:
   call('createThread', {reply:true, 'thread.parent':'<Message 2 EntityId>', 'thread.title':'api', 'thread.body':'api'})
   Result: 200, and nothing in the response mentions Michael.
5. Run the query. The new response's notify list = Jim. Michael was removed.
6. As admin, reactivate Michael.
7. As Michael, open SecureBoard. No messages are listed. SELECT RowId, LatestId FROM comm.Threads WHERE RowId = <Message 2 RowId>; shows that Message 2's latest post is Jim's API reply, whose notify list doesn't include Michael.

Expected: The UI and API handle a member who can't read the thread the same way. The API doesn't remove someone without saying so.

3. API edit overwrites the original message's saved notify list with the latest response's list

Folder: OpenBoard

1. As admin, create Message 3 and notify Jim and Creed. The query shows original notify list = Jim, Creed.
2. As admin, respond to Message 3 and notify Jim only. The query shows response notify list = Jim.
3. As admin, edit only the original message's body through the API:
   call('updateThread', {'thread.rowId': <Message 3 RowId>, 'thread.body': 'edited via api'})
   Result: 200, and only the body changed.
4. Run the query again:
   • The original Message 3 changed from Jim, Creed to Jim.
   • The response is unchanged (Jim).

Expected: Editing only the body leaves the original message's notify list unchanged (Jim, Creed).