Skip to content

Implement admin signature handling and pinning functionality for post… - #387

Open
dhyeymistri wants to merge 1 commit into
RetroShare:masterfrom
dhyeymistri:posted-admin-edit-pins
Open

dhyeymistri wants to merge 1 commit into
RetroShare:masterfrom
dhyeymistri:posted-admin-edit-pins

Conversation

@dhyeymistri

Copy link
Copy Markdown

…ed items

@jolavillette

Copy link
Copy Markdown
Contributor

Thanks for this — I went through the whole diff (signature construction/verification, wire format, the blocking APIs, the GUI call sites on master, the JSON API generator, the tests). The core mechanism is sound, but before you spend more time on it I think the maintainers need to take a design decision, and one thing currently breaks the build. Details below, roughly in order of importance.

1. Build blocker (trivial to fix): the JSON API generator aborts. setPostPinned() is tagged @jsonapi{development} but has no @param lines, and src/jsonapi/jsonapi-generator.py exits on any @jsonapi parameter without a Doxygen direction. Reproduced with doxygen + the generator on this branch's rsposted.h (master's header passes):

ERROR Parameter: boardId of: /rsPosted/setPostPinned declared in: retroshare/rsposted.h miss doxygen parameter direction attribute!

RS_JSON_API is ON by default for the GUI, is the point of retroshare-service (and Android), and is enabled in 8 of the 11 root build workflows, so this fails everywhere except the lib's own CI (which has the option OFF and has not run on this PR yet). Fix: @param[in] boardId, @param[in] postId, @param[in] pinned, @param[out] errorMessage. The defaulted origPostId on createPostV2 is fine (forums already do the same on a @jsonapi method).

2. Design decision for @csoler, before any further work on the editing part. The PR makes editing an admin-key operation: only the node that created the board can edit any post, the author cannot fix their own typo, and a board whose creator is gone is frozen. The two precedents in the lib are different: channels let any publish-key holder edit and keep older versions in mOlderVersions; forums let the author or a moderator from mAdminList edit, with a lib-side check at read time (p3gxsforums.cc:1335-1360, IS_FORUM_MSG_MODERATION + canEditPosts). Your receipt-time admin signature is actually stronger than either (forged revisions are rejected and never stored), which is nice — the question is only which authority it should be tied to (node admin key vs identities in a moderator list, as forums do), whether the revision's author should be surfaced (resolve() keeps the original author and timestamp on the post while replacing its text, so an admin rewrite reads as if the author wrote it), and whether older versions should be kept like channels do. If the answer is "moderator list", the core of the editing part changes, so I'd rather not ask you to polish it before that call.

3. Performance regression on the existing GUI, independent of the design call. getBoardContent() now calls getBoardPostSummaries() — a full metadata load of the board — on every call, to find revisions of the requested ids. The Qt GUI calls it with a single id from every board item in the News feed (BoardsPostItem.cpp:269, BoardsCommentsItem.cpp:183,245, PostedItem.cpp:176,237), from the per-post refresh in the board view (PostedPostsModel.cpp:122) and from IdDialog.cpp:2100. N feed items become N full-board metadata scans, and the summary call is behind the pre-existing 5 s waitToken cap while the content request has 20 s, so on a big board getBoardContent can now fail where it used to succeed. Any variant of the feature needs this bounded (e.g. request only messages whose mOrigMsgId is in the requested set, or skip the expansion when explicit ids are given).

4. A few things the eventual lib/GUI pair must handle (noting, not asking for now):

  • An edited post gets stuck at 1 unread on receiving nodes: getGroupStatistic treats the original as obsolete once a revision exists and counts the revision instead, but resolve() hides the revision so mark-as-read never touches it. Easy lib-side fix in setMessageReadStatus.
  • Old clients will show an admin revision as a second, admin-authored post next to the original (the version-collapsing code in PostedPostsModel.cpp is under #ifdef TODO), and comments they attach to that copy are invisible on new clients. Old clients also drop the pin list when they edit the board (same as forums).
  • resolve() hides a revision whose mPublishTs is older than the original's, i.e. clock skew between author and admin silently disables editing.
  • Originals expire on their own timestamp, so an old post's revision can outlive it and become an orphan.
  • The README describes GUI actions (Edit Post, Pin Post, pinned-first sorting) that don't exist anywhere I can find — is there a GUI branch? A PR description would also help a lot: this is three features plus a real fix.

5. Suggested split. (a) The ctx.mSizectx.mOffset change in createPostV2 is a genuine bug fix (the 200 kB limit was never enforced) and could be merged on its own today. (b) Pins are cheap, mirror forums exactly and are wire-compatible (old clients ignore the trailing TLV, unpinned groups keep the old byte layout) — near-mergeable once item 1 is fixed. (c) Revisions + admin signature: hold for the design call in item 2.

I did not find problems with the crypto: the admin signature covers the same buffer as the publish/author signatures (body + meta with mMsgId and signSet cleared, mOrigMsgId included), is verified against the group's public admin key that every node has, and the scope (Posted, top-level posts with mOrigMsgId ≠ mMsgId) leaves comments, votes and other services untouched.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants