Page MenuHomePhabricator

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

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



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

rP Phabricator
Lint Not Applicable
Tests Not Applicable

Event Timeline

  • Better ordering for capability checks?

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


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.

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.

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.