Page MenuHomePhabricator

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

Authored by epriestley on Mar 28 2019, 10:35 PM.



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

rP Phabricator
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Mar 28 2019, 10:35 PM
epriestley requested review of this revision.Mar 28 2019, 10:37 PM
amckinley accepted this revision.Mar 28 2019, 10:51 PM
amckinley added inline comments.

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.


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" ๐Ÿ˜‚


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
epriestley added inline comments.Mar 28 2019, 10:54 PM

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


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.)


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.