Details
- Reviewers
amckinley - Maniphest Tasks
- T13222: 2018 Week 48-51 Bonus Content
- Commits
- rP543f2b6bf156: Allow any transaction group to be signed with a one-shot "Sign With MFA" action
- 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
- Branch
- mfa15
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 21361 Build 29076: Run Core Tests Build 29075: arc lint + arc unit
Event Timeline
| 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. | |
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.