Page MenuHomePhabricator

When a comment was signed with MFA, require MFA to edit it
ClosedPublic

Authored by epriestley on Mar 28 2019, 10:35 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 26, 10:10 AM
Unknown Object (File)
Feb 11 2024, 7:11 AM
Unknown Object (File)
Jan 5 2024, 1:08 PM
Unknown Object (File)
Jan 5 2024, 10:42 AM
Unknown Object (File)
Nov 30 2023, 5:27 AM
Unknown Object (File)
Nov 18 2023, 3:53 PM
Unknown Object (File)
Nov 1 2023, 12:23 PM
Unknown Object (File)
Oct 28 2023, 4:07 AM
Subscribers
None

Details

Summary

Ref PHI1173. Currently, you can edit an MFA'd comment without redoing MFA. This is inconsistent with the intent of the MFA badge, since it means an un-MFA'd comment may have an "MFA" badge on it.

Instead, implement these rules:

  • If a comment was signed with MFA, you MUST MFA to edit it.
  • When removing a comment, add an extra MFA prompt if the user has MFA. This one isn't strictly required, this action is just very hard to undo and seems reasonable to MFA.
Test Plan
  • Made normal comments and MFA comments.
  • Edited normal comments and MFA comments (got prompted).
  • Removed normal comments and MFA comments (prompted in both cases).
  • Tried to edit an MFA comment without MFA on my account, got a hard "MFA absolutely required" failure.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

amckinley added inline comments.
src/applications/transactions/editor/PhabricatorApplicationTransactionCommentEditor.php
203โ€“205

I'm assuming you know these flows well enough that you're sure PhabricatorApplicationTransactionEditor->applyTransactions() is the only place that needs to call setIsNewComment()? I'm just wondering if setting a default for isNewComment would be useful at all.

257

As soon as I saw this diff I thought "ooooh, I bet @epriestley didn't consider the case where the user lost all their MFA factors" ๐Ÿ˜‚

278

Aren't these already globally unique? Not a big deal either way.

This revision is now accepted and ready to land.Mar 28 2019, 10:51 PM
src/applications/transactions/controller/PhabricatorApplicationTransactionCommentRemoveController.php
57

(This 'anchor' stuff was some long-ago legacy thing that isn't used anymore as far as I can tell.)

src/applications/transactions/editor/PhabricatorApplicationTransactionCommentEditor.php
203โ€“205

Yeah, there are only three invocations of CommentEditor in the codebase: new, edit, remove.

I didn't make isNewComment() the default since it's sort of more dangerous (it effectively disables these MFA checks).

(I initially did this test as $is_new = !$xaction->getComment()->getID(); but that felt very fragile.)

278

The <phid, id> pair is globally unique. The <id> alone wouldn't be, since Maniphest has a comment #6 and Differential also has a comment #6.

This revision was automatically updated to reflect the committed changes.