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.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 12, 9:59 PM
Unknown Object (File)
Fri, Mar 1, 8:57 PM
Unknown Object (File)
Feb 8 2024, 6:50 AM
Unknown Object (File)
Feb 3 2024, 8:21 PM
Unknown Object (File)
Jan 31 2024, 4:48 PM
Unknown Object (File)
Jan 31 2024, 4:41 PM
Unknown Object (File)
Jan 31 2024, 3:22 PM
Unknown Object (File)
Jan 25 2024, 1:37 AM
Subscribers
None

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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

  • Better ordering for capability checks?
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.

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.