Page MenuHomePhabricator
Differential D20340

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

Authored by epriestley on Mar 28 2019, 10:35 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Jan 2, 1:41 AM
Unknown Object (File)
Sat, Dec 21, 6:45 AM
Unknown Object (File)
Sat, Dec 14, 2:54 AM
Unknown Object (File)
Dec 9 2024, 5:55 AM
Unknown Object (File)
Nov 27 2024, 2:19 AM
Unknown Object (File)
Nov 26 2024, 6:25 AM
Unknown Object (File)
Nov 26 2024, 6:25 AM
Unknown Object (File)
Nov 26 2024, 6:25 AM
View All Files
Subscribers
None

Details

Reviewers
amckinley
Commits
rP15cc475cbd8c: When a comment was signed with MFA, require MFA to edit it
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

epriestley created this revision.Mar 28 2019, 10:35 PM
Harbormaster completed remote builds in B22417: Diff 48546.Mar 28 2019, 10:37 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.
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
epriestley added inline comments.Mar 28 2019, 10:54 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.

Closed by commit rP15cc475cbd8c: When a comment was signed with MFA, require MFA to edit it (authored by epriestley). · Explain WhyMar 28 2019, 10:55 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 48547

View Options

src/applications/transactions/controller/PhabricatorApplicationTransactionCommentEditController.php

Loading...
View Options

src/applications/transactions/controller/PhabricatorApplicationTransactionCommentRemoveController.php

Loading...
View Options

src/applications/transactions/editor/PhabricatorApplicationTransactionCommentEditor.php

Loading...
View Options

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

Loading...
Phabricator · Built by Phacility