Page MenuHomePhabricator

Allow any transaction group to be signed with a one-shot "Sign With MFA" action
ClosedPublic

Authored by epriestley on Dec 17 2018, 8:37 PM.

Details

Summary

Depends on D19896. Ref T13222. See PHI873. Add a core "Sign With MFA" transaction type which prompts you for MFA and marks your transactions as MFA'd.

This is a one-shot gate and does not keep you in MFA.

Test Plan
  • Used "Sign with MFA", got prompted for MFA, answered MFA, saw transactions apply with MFA metadata and markers.
  • Tried to sign alone, got appropriate errors.
  • Tried to sign no-op changes, got appropriate errors.

Diff Detail

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

Event Timeline

epriestley created this revision.Dec 17 2018, 8:37 PM
epriestley requested review of this revision.Dec 17 2018, 8:38 PM
epriestley updated this revision to Diff 47507.Dec 17 2018, 8:40 PM
  • Better ordering for capability checks?
epriestley added inline comments.Dec 17 2018, 8:45 PM
src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
853–855

Previously, we marked transactions as "MFA" if you were in an MFA session. Now, we only mark them if you "Sign With MFA".

997–1022

This may possibly cause some sort of behavioral change, although I'm not sure what. I've moved adjustTransactionValues(), applyCapabilityChecks(), and filterTransactions() up. I've removed the (~5-month old) hard safety check against requireCapabilities().

This means that these steps now happen before applyInitialEffects() and willApplyTransactions(). I think that's okay. It may cause some kind of problem I'm not anticipating, but if it does the meanings of "initial effects" and "will apply transactions" should probably be clarified and possibly these hooks should be removed. A cursory look at the implementations of these methods didn't reveal anything obvious.

amckinley accepted this revision.Dec 18 2018, 10:04 PM
This revision is now accepted and ready to land.Dec 18 2018, 10:04 PM

I've been probing this some more and applyInitialEffects() does actually do some weird stuff in Pholio, at least, and maybe Calendar/Ponder. Pretty sure I can sort it out but it may need a couple of diffs that look very unrelated to this stuff.

epriestley added a comment.EditedDec 20 2018, 10:01 PM

The parallel line of development culminating in D19926 should fix the applyInitialEffects stuff in Pholio.

It's still used by Calendar and Ponder:

  • Ponder should change to work like Pholio (create an un-attached "answer" first, then attach it with a transaction). I believe it doesn't actually interact negatively with this code, though, or at least I couldn't figure out how to break it after poking at it for a bit. I'll give it another try once the dust settles, or maybe try to fix it, but the Pholio fix already led me pretty far afield.
  • Calendar is just doing some basic setup and doesn't appear to interact here. The code could move to expandTransactions() for now, I think, but until the Ponder case is cleaned up that doesn't let us get rid of anything.
This revision was automatically updated to reflect the committed changes.