1. 29 Mar, 2020 4 commits
    • Daniel Stone's avatar
      HACK: Reverse add/remove transaction application order · 661572ef
      Daniel Stone authored
      PhabricatorApplicationTransactionEditor contains logic (inside
      combineTransactions -> mergeTransactions ->
      mergePHIDOrEdgeTransactions), which will combine two transactions of the
      same edge type and author, then apply the operations in a deterministic
      order.
      
      This breaks our change to remove dependencies when updating a
      Differential revision, since we (acting as the user who uploaded the
      revision) remove the DifferentialRevisionDependsOn edge, then have the
      Remarkup block parser add the dependencies from the commit message
      later.
      
      The two (simplified) transactions of:
      {
          "1-from-our-change-to-differential": {
              "type": "edge",
              "-": {
      	    "PHID-DREV-1234": [...], // remove previous dep
      	}
          },
          "2-from-remarkup-parsing": {
              "type": "edge",
              "+": {
      	    "PHID-DREV-1234": [...], // add dep from commit message
      	}
          }
      }
      
      get merged into:
      {
          "1-combined": {
              "type": "edge",
      	"-": {
      	    "PHID-DREV-1234": [...], // remove previous dep
      	},
      	"+": {
      	    "PHID-DREV-1234": [...], // add dep from commit message
      	}
          }
      }
      
      getPHIDTransactionNewValue() then returns an empty dictionary, because
      it always executes the add before the remove, regardless of ordering.
      The correct fix would be quite invasive to the transaction editor
      (making the combine function considerably less naïve, and always
      preserving order of operation WRT identical PHIDs); the quick fix for
      now (at least) is to just make add operations execute after remove, thus
      'fixing' it for the only case we really care about.
      
      The correct fix is more time than worthwhile to achieve, especially
      since it's extremely difficult to achieve without code modifications.
      661572ef
    • Daniel Stone's avatar
      WIP: Maniphest: Allow restricting custom fields to subtypes · bbc9a7d7
      Daniel Stone authored
      Add a 'subtype' parameter to custom fields, which means they will only
      be visible on (and validated with) tasks of a particular subtype.
      
      Suitable for upstream after much rework:
      https://secure.phabricator.com/D17593
      bbc9a7d7
    • Emanuele Aina's avatar
      Derive story/mention time from transactions · 745775ca
      Emanuele Aina authored
      By taking the story time from the transaction creation date we ensure that the times reported in the feed view match the ones reported in the item-specific change lists.
      745775ca
    • Daniel Stone's avatar
      Preserve silent and time when updating blocked tasks · 64fae39e
      Daniel Stone authored
      Ref T13042. Updating blocked tasks creates a new ManiphestTransactionEditor instance from within the current transaction application, which fails to carry over all of the current properties. Failing to update silent means that mails will be generated for updates to blocked tasks, regardless of the setting for the original transaction editor.
      
      Failing to preserve the time can also give large time deltas in corner cases, such as running an importer which pulls tasks from yesteryear.
      
      This equally applies to inverse-edge transactions, though I don't have a ready-made usecase for those.
      64fae39e
  2. 30 Jan, 2020 1 commit
    • epriestley's avatar
      When issuing a "no-op" MFA token because no MFA is configured, don't give the... · 12c33709
      epriestley authored
      When issuing a "no-op" MFA token because no MFA is configured, don't give the timeline story a badge
      
      Summary:
      Fixes T13475. Sometimes, we issue a "no op" / "default permit" / "unchallenged" MFA token, when a user with no MFA configured does something which is configured to attempt (but not strictly require) MFA.
      
      An example of this kind of action is changing a username: usernames may be changed even if MFA is not set up.
      
      (Some other operations, notably "Sign With MFA", strictly require that MFA actually be set up.)
      
      When a user with no MFA configured takes a "try MFA" action, we see that they have no factors configured and issue a token so they can continue. This is correct. However, this token causes the assocaited timeline story to get an MFA badge.
      
      This badge is incorrect or at least wildly misleading, since the technical assertion it currently makes ("the user answered any configured MFA challenge to do this, if one exists") isn't explained properly and isn't useful anyway.
      
      Instead, only badge the story if the user actually has MFA and actually responded to some kind of MFA challege. The badge now asserts "this user responded to an MFA challenge", which is expected/desired.
      
      Test Plan:
        - As a user with no MFA, renamed a user. Before patch: badged story. After patch: no badge.
        - As a user with MFA, renamed a user. Got badged stories in both cases.
      
      Maniphest Tasks: T13475
      
      Differential Revision: https://secure.phabricator.com/D20958
      12c33709
  3. 26 Aug, 2019 1 commit
  4. 23 Jul, 2019 1 commit
    • Arturas Moskvinas's avatar
      Allow users with no CAN_EDIT permissions to silence projects if they want to · cd449254
      Arturas Moskvinas authored
      Summary: Humble user cannot silence/mute project if he/she has no CAN_EDIT permissions in it. You can actually leave it but if project is locked - then you're scr*wed.
      
      Test Plan:
      1. On a testing phabricator instance created a dummy project
      2. Changed that project permissions CAN_EDIT to be by admin only
      3. Added poor soul with no CAN_EDIT permissions
      4. Logged it in with poor soul
      5. Tried to silence the project
      6. The Project is successfully silenced
      7. User is happy :)
      
      Reviewers: epriestley, #blessed_reviewers
      
      Reviewed By: epriestley, #blessed_reviewers
      
      Subscribers: Korvin, Pawka
      
      Differential Revision: https://secure.phabricator.com/D20675
      cd449254
  5. 18 Jun, 2019 1 commit
  6. 17 Jun, 2019 1 commit
  7. 31 May, 2019 1 commit
    • epriestley's avatar
      After reloading transactions for the recipient while building transaction... · 81134d7e
      epriestley authored
      After reloading transactions for the recipient while building transaction mail, put them in the input order
      
      Summary:
      Ref T13303. In D20525 I fixed an issue where transaction rendering could use cached values with the wrong viewer by reloading transactions.
      
      However, reloading transactions may also reorder them as a side effect, since `withPHIDs(...)` does not imply an order. This can make transaction rendering order in mail wrong/inconsistent.
      
      Instead, reorder the transactions before continuing so mail transaction order is consistent.
      
      Test Plan: Applied a group of transactions to a task, saw a more consistent rendering order in mail after the change.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13303
      
      Differential Revision: https://secure.phabricator.com/D20563
      81134d7e
  8. 30 May, 2019 1 commit
    • epriestley's avatar
      Add a "View Task" button to HTML mail from Maniphest · 9a32a563
      epriestley authored
      Summary:
      See downstream <https://phabricator.wikimedia.org/T1050>. Some time ago, we added a "View Revision" button to Differential mail. This hasn't created any problems and generally seems good / desirable.
      
      It isn't trivial to just add everywhere since we need a translation string in each case, but at least add it to Maniphest for now. Going forward, we can fill in more applications as they come up.
      
      Test Plan:
      Used `bin/mail show-outbound --id <x> --dump-html`:
      
      {F6470461}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Differential Revision: https://secure.phabricator.com/D20561
      9a32a563
  9. 24 May, 2019 1 commit
    • epriestley's avatar
      Fix a looping workflow when trying to submit a partially-effectless transaction group · ce6fc5be
      epriestley authored
      Summary:
      Ref T13289. If you do this:
      
        - Subscribe to a task (so we don't generate a subscribe side-effect later).
        - Prepare a transaction group: sign with MFA, change projects (don't make any changes), add a comment.
        - Submit the transaction group.
      
      ...you'll get prompted "Some actions don't have any effect (the non-change to projects), apply remaining effects?".
      
      If you confirm, you get MFA'd, but the MFA flow loses the "continue" confirmation, so you get trapped in a workflow loop of confirming and MFA'ing.
      
      Instead, retain the "continue" bit through the MFA.
      
      Also, don't show "You can't sign an empty transaction group" if there's a comment.
      
      See also T13295, since the amount of magic here can probably be reduced. There's likely little reason for "continue" or "hisec" to be magic nowadays.
      
      Test Plan:
        - Went through the workflow above.
        - Before: looping workflow.
        - After: "Continue" carries through the MFA gate.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13289
      
      Differential Revision: https://secure.phabricator.com/D20552
      ce6fc5be
  10. 22 May, 2019 3 commits
    • epriestley's avatar
      Use the same transaction group ID for transactions applied indirectly by a sub-editor · 31e623af
      epriestley authored
      Summary:
      Ref T13283. Currently, each Editor sets its own group ID, so if you create a revision and then Herald does some stuff, the two groups of transactions get different group IDs.
      
      This means the test console is slightly misleading (it will only pick up the Herald transactions). It's going to be misleading anyway (Herald obviously can't evaluate Herald transactions) but this is at least a little closer to reality and stops Herald actions from masking non-Herald actions.
      
      Test Plan:
        - Created a revision. Herald applied one transaction.
        - Used the test console.
        - Before: The test console only picked up the single most recent Herald transaction.
        - After: The test console picked up the whole transaction group.
      
      {F6464059}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13283
      
      Differential Revision: https://secure.phabricator.com/D20546
      31e623af
    • epriestley's avatar
      Prevent "Differential Revision: ..." from counting as a mention in commit messages · 1eff4fdc
      epriestley authored
      Summary:
      Ref T13290. Ref T13291. Now that a full URI is a "mention", the full URI in "Differential Revision: ..." also triggers a mention.
      
      Stop it from doing that, since these mentions are silly/redundant/unintended.
      
      The API here is also slightly odd; simplify it a little bit to get rid of doing "append" with "get + append + set".
      
      Test Plan: Used `bin/repository reparse --publish` to republish commits with "Differential Revision: ..." and verified that the revision PHID was properly dropped from the mention list.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13291, T13290
      
      Differential Revision: https://secure.phabricator.com/D20544
      1eff4fdc
    • epriestley's avatar
      Stabilize sorting of feed stories with similar strength · fa4dcaa3
      epriestley authored
      Summary:
      See PHI1222. When we publish several transactions to feed at once, we sort them by "action strength" to figure out which one gets to be the title story.
      
      This sort currently uses `msort()`, which uses `asort()`, which is not a stable sort and has inconsistent behavior across PHP versions:
      
      {F6463721}
      
      Switch to `msortv()`, which is a stable sort. Previously, see also T6861.
      
      If all transactions have the same strength, we'll now consistently pick the first one.
      
      This probably (?) does not impact anything in the upstream, but is good from a consistency point of view.
      
      Test Plan:
      Top story was published after this change and uses the chronologically first transaction as the title story.
      
      Bottom story was published before this change and uses the chronologically second transaction as the title story.
      
      Both stories have two transactions with the same strength ("create" + "add reviewer").
      
      {F6463722}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Differential Revision: https://secure.phabricator.com/D20540
      fa4dcaa3
  11. 21 May, 2019 1 commit
    • epriestley's avatar
      Fix an issue where handles could load with the incorrect viewer when building... · b8f6248e
      epriestley authored
      Fix an issue where handles could load with the incorrect viewer when building mail about changes to related objects
      
      Summary:
      See <https://phabricator.wikimedia.org/T179591>. Some time ago, all handle rendering preloaded handles: things emitted a list of PHIDs they'd need handles for, then later used only those PHIDs.
      
      Later, we introduced `HandlePool` and lazy/on-demand handle loading. Modern transactions mostly use this to render object PHIDs.
      
      When we build mail, many newer transactions use an on-demand load to fetch handles to render transactions. This on-demand load may use the original viewer (the acting user) instead of the correct viewer (the mail recipient): we fetch and reset handles using the correct viewer, but do not overwrite the active viewer for on-demand loading. This could cause mail to leak the titles of related objects to users who don't have permission to see them.
      
      Instead, just reload the transactions with the correct viewer when building mail instead of playing a bunch of `setViewer()` and `clone` games. Until we're 100% on modular transactions, several pieces of the stack cache viewer or state information.
      
      Test Plan:
        - Created task A (public) with subtask B (private).
        - Closed subtask B as a user with access to it.
        - Viewed mail sent to subscribers of task A who can not see subtask B.
          - Before change: mail discloses title of subtask B.
          - After change: mail properly labels subtask B as "Restricted Task".
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Differential Revision: https://secure.phabricator.com/D20525
      b8f6248e
  12. 17 May, 2019 1 commit
    • epriestley's avatar
      Label transaction groups with a "group ID" so Herald can reconstruct them faithfully · 167f06d3
      epriestley authored
      Summary:
      Ref T13283. See PHI1202. See D20519. When we apply a group of transactions, label all of them with the same "group ID".
      
      This allows other things, notably Herald, to figure out which transactions applied together in a faithful way rather than by guessing, even though the guess was probably pretty good most of the time.
      
      Also expose this to `transaction.search` in case callers want to do something similar. They get a list of transaction IDs from webhooks already anyway, but some callers use `transaction.search` outside of webhooks and this information may be useful.
      
      Test Plan:
        - Ran Herald Test Console, saw faithful selection of recent transactions.
        - Changed hard limit from 1000 to 1, saw exception. Users should be very hard-pressed to hit this normally (they'd have to add 990-ish custom fields, then edit every field at once, I think) so I'm just fataling rather than processing some subset of the transaction set.
        - Called `transaction.search`, saw group ID information available.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13283
      
      Differential Revision: https://secure.phabricator.com/D20524
      167f06d3
  13. 14 May, 2019 1 commit
    • epriestley's avatar
      Don't mark subscribes/mentions as "Lock Overridden" · 3e0391c5
      epriestley authored
      Summary:
      See PHI1209. When a task is in "Hard Lock" mode, it's still possible to apply some changes to it. Notably:
      
        - You can subscribe/unsubscribe.
        - You can mention it on another object.
        - You can add a relationship from some other object to it (e.g., select it as a "Parent Task" for some other task).
      
      Currently, these types of edits will show a "Lock Overridden" timeline emblem icon. However, they should not: you didn't override a lock to make these changes, they just bypass locks.
      
      For now, special case these cases (self subscribe/unsubscribe + inverse edge edits) so they don't get the little icon, since I think this list is exhaustive today.
      
      Some day we should modularize this, but we'd need code like this anyway (since TYPE_SUBSCRIBE is not modular yet), and this seems unlikely to cause problems even if it's a bit rough.
      
      Test Plan:
        - Hard-locked a task.
          - Subscribed/unsubscribed, mentioned, relationship'd it as a non-author. No timeline emblems.
        - Soft-locked a task.
          - Subscribed/unsubscribed, mentioned, relationship'd it, no timeline emblems.
          - Clicked "Edit", answered "yes" to the override prompt, edited it. Got a timeline emblem.
        - Added some comments and stuff to a normal non-locked task, no emblems.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Differential Revision: https://secure.phabricator.com/D20513
      3e0391c5
  14. 29 Apr, 2019 1 commit
  15. 24 Apr, 2019 1 commit
    • epriestley's avatar
      When applying transactions, acquire a read lock sooner · b3b1cc64
      epriestley authored
      Summary:
      Depends on D20461. Ref T13276. Ref T13054.
      
      Currently, we acquire the transaction read lock after populating "old values" in transactions and filtering transactions with no effect.
      
      This isn't early enough to prevent all weird chaotic races: if two processes try to apply a "close revision" transaction at the same time, this can happen:
      
      ```
      PROCESS A             PROCESS B
      Old Value = Open      Old Value = Open
      Transaction OK: Yes   Transaction OK: Yes
      Acquire Read Lock     Acquire Read Lock
      Got Read Lock!        Wait...
      Apply Transactions    Wait...
      New Value = Closed    Wait...
      Release Lock          Wait...
                            Got Read Lock!
                            Apply Transactions
                            New Value = Closed
                            Release Lock
      ```
      
      That's not great: both processes apply an "Open -> Closed" transaction since this was a valid transaction from the viewpoint of each process when it did the checks.
      
      We actually want this:
      
      ```
      PROCESS A             PROCESS B
      Acquire Read Lock     Acquire Read Lock
      Got Read Lock!        Wait...
      Old Value = Open      Wait...
      Transaction OK: Yes   Wait...
      Apply Transactions    Wait...
      New Value = Closed    Wait...
      Release Lock          Wait...
                            Got Read Lock!
                            >>> Old Value = Closed
                            >>> Transaction Has No Effect!
                            >>> Do Nothing / Abort
                            Release Lock
      ```
      
      Move the "lock" part up so we do that.
      
      This may cause some kind of weird second-order effects, but T13054 went through pretty cleanly and we have to do this to get correct behavior, so we can survive those if/when they arise.
      
      Test Plan:
        - Added a `sleep(10)` before the lock.
        - Ran `bin/repository message --reparse X` in two console windows, where X is a commit that closes revision Y and Y is open.
          - Before patch: both windows closed the revision and added duplicate transactions.
          - After patch: only one of the processes had an effect.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Subscribers: jmeador
      
      Maniphest Tasks: T13276, T13054
      
      Differential Revision: https://secure.phabricator.com/D20462
      b3b1cc64
  16. 28 Mar, 2019 1 commit
    • epriestley's avatar
      When a comment was signed with MFA, require MFA to edit it · 15cc475c
      epriestley authored
      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.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Differential Revision: https://secure.phabricator.com/D20340
      15cc475c
  17. 18 Mar, 2019 1 commit
    • epriestley's avatar
      When performing complex edits, pause sub-editors before they publish to... · a6fd8f04
      epriestley authored
      When performing complex edits, pause sub-editors before they publish to propagate "Must Encrypt" and other state
      
      Summary:
      See PHI1134. Previously, see T13082 and D19969 for some sort-of-related stuff.
      
      Currently, edits work roughly like this:
      
        - Suppose we're editing object X, and we're also going to edit some other object, Y, because X mentioned Y or the edit is making X a child or parent of Y, or unblocking Y.
        - Do the actual edit to X, including inverse edits ("alice mentioned Y on X.", "alice added a child revision: X", etc) which apply to Y.
        - Run Herald rules on X.
        - Publish the edit to X.
      
      The "inverse edits" currently do this whole process inline, in a sub-editor. So the flow expands like this:
      
        - Begin editing X.
        - Update properties on X.
          - Begin inverse-edge editing Y.
          - Update properties on Y.
          - Run (actually, skip) Herald rules on Y.
          - Publish edits to Y.
        - Run Herald rules on X.
        - Publish edits to X.
      
      Notably, the "Y" stuff publishes before the "X" Herald rules run. This creates potential problems:
      
        - Herald rules may change the name or visibility policy of "X", but we'll publish mail about it via the edits to Y before those edits apply. This is a problem only in theory, we don't ship any upstream rules like this today.
        - Herald rules may "Require Secure Mail", but we won't know that at the time we're building mail about the indirect change to "Y". This is a problem in practice.
      
      Instead, switch to this new flow, where we stop the sub-editors before they publish, then publish everything at the very end once all the edits are complete:
      
        - Begin editing X.
        - Update properties on X.
          - Begin inverse-edge editing Y.
          - Update properties on Y.
          - Skip Herald on Y.
        - Run Herald rules on X.
        - Publish X.
          - Publish all child-editors of X.
            - Publish Y.
      
      Test Plan:
        - Created "Must Encrypt" Herald rules for Tasks and Revisions.
        - Edited object "A", an object which the rules applied to directly, and set object "B" (a different object which the rules did not hit) as its parent/child and/or unblocked it.
        - In `bin/mail list-outbound`, saw:
          - Mail about object "A" all flagged as "Must Encrypt".
          - Normal mail from object B not flagged "Must Encrypt".
          - Mail from object B about changing relationships to object A flagged as "Must Encrypt".
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Differential Revision: https://secure.phabricator.com/D20283
      a6fd8f04
  18. 13 Mar, 2019 1 commit
    • epriestley's avatar
      Don't subscribe bots implicitly when they act on objects, or when they are mentioned · 04f9e72c
      epriestley authored
      Summary:
      See PHI1098. When users comment on objects, they are automatically subscribed. And when `@alice` mentions `@bailey` on a task, that usually subscribes `@bailey`.
      
      These rules make less sense if the user is a bot. There's generally no reason for a bot to automatically subscribe to objects it acts on (it's not going to read email and follow up later), and it can always subscribe itself pretty easily if it wants (since everything is `*.edit` now and supports subscribe transactions).
      
      Also, don't subscribe bots when they're mentioned for similar reasons. If users really want to subscribe bots, they can do so explicitly.
      
      These rules seem slightly like "bad implicit magic" since it's not immediately obvious why `@abc` subscribes that user but `@xyz` may not, but some of these rules are fairly complicated already (e.g., `@xyz` doesn't subscribe them if they unsubscribed or are implicitly subscribed) and this feels like it gets the right/desired result almost-always.
      
      Test Plan:
      On a fresh task:
      
        - Mentioned a bot in a comment with `@bot`.
          - Before patch: bot got CC'd.
          - After patch: no CC.
        - Called `maniphest.edit` via the API to add a comment as a bot.
          - Before patch: bot got CC'd.
          - After patch: no CC.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Differential Revision: https://secure.phabricator.com/D20284
      04f9e72c
  19. 28 Feb, 2019 1 commit
    • epriestley's avatar
      Add more type checking to transactions queued by Herald · 814e6d2d
      epriestley authored
      Summary:
      See PHI1096. Depends on D20213. An install is reporting a hard-to-reproduce issue where a non-transaction gets queued by Herald somehow. This might be in third-party code.
      
      Sprinkle the relevant parts of the code with `final` and type checking to try to catch the problem before it causes a fatal we can't pull a stack trace out of.
      
      Test Plan: Poked around locally (e.g., edited revisions to cause Herald to trigger), but hard to know if this will do what it's supposed to or not without deploying and seeing if it catches anything.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Differential Revision: https://secure.phabricator.com/D20214
      814e6d2d
  20. 19 Feb, 2019 1 commit
    • epriestley's avatar
      Make "Subscribe/Unsubscribe" require only "CAN_VIEW", not "CAN_INTERACT" · e44b40ca
      epriestley authored
      Summary:
      Ref T13249. See PHI1059. Currently, Subscribe/Unsubscribe require CAN_INTERACT via the web UI and no permissions (i.e., effectively CAN_VIEW) via the API.
      
      Weaken the requirements from the web UI so that you do not need "CAN_INTERACT". This is a product change to the effect that it's okay to subscribe/unsubscribe from anything you can see, even hard-locked tasks. This generally seems reasonable.
      
      Increase the requirements for the actual transaction, which mostly applies to API changes:
      
        - To remove subscribers other than yourself, require CAN_EDIT.
        - To add subscribers other than yourself, require CAN_EDIT or CAN_INTERACT. You may have CAN_EDIT but not CAN_INTERACT on "soft locked" tasks. It's okay to click "Edit" on these, click "Yes, override lock", then remove subscribers other than yourself.
      
      This technically plugs some weird, mostly theoretical holes in the API where "attackers" could sometimes make more subscription changes than they should have been able to. Now that we send you email when you're unsubscribed this could only really be used to be mildly mischievous, but no harm in making the policy enforcement more correct.
      
      Test Plan: Against normal, soft-locked, and hard-locked tasks: subscribed, unsubscribed, added and removed subscribers, overrode locks, edited via API. Everything worked like it should and I couldn't find any combination of lock state, policy state, and edit pathway that did anything suspicious.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13249
      
      Differential Revision: https://secure.phabricator.com/D20174
      e44b40ca
  21. 09 Feb, 2019 1 commit
    • epriestley's avatar
      When an edit overrides an object lock, note it in the transaction record · a20f1080
      epriestley authored
      Summary:
      Ref T13244. See PHI1059. When you lock a task, users who can edit the task can currently override the lock by using "Edit Task" if they confirm that they want to do this.
      
      Mark these edits with an emblem, similar to the "MFA" and "Silent" emblems, so it's clear that they may have bent the rules.
      
      Also, make the "MFA" and "Silent" emblems more easily visible.
      
      Test Plan:
      Edited a locked task, overrode the lock, got marked for it.
      
      {F6195005}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Subscribers: aeiser
      
      Maniphest Tasks: T13244
      
      Differential Revision: https://secure.phabricator.com/D20131
      a20f1080
  22. 07 Feb, 2019 1 commit
    • epriestley's avatar
      Make the default behavior of getApplicationTransactionCommentObject() "return... · 9f5e6bee
      epriestley authored
      Make the default behavior of getApplicationTransactionCommentObject() "return null" instead of "throw"
      
      Summary:
      Depends on D20115. See <https://discourse.phabricator-community.org/t/transaction-search-endpoint-does-not-work-on-differential-diffs/2369/>.
      
      Currently, `getApplicationTransactionCommentObject()` throws by default. Subclasses must override it to `return null` to indicate that they don't support comments.
      
      This is silly, and leads to a bunch of code that does a `try / catch` around it, and at least some code (here, `transaction.search`) which doesn't `try / catch` and gets the wrong behavior as a result.
      
      Just make it `return null` by default, meaning "no support for comments". Then remove the `try / catch` stuff and all the `return null` implementations.
      
      Test Plan:
        - Grepped for `getApplicationTransactionCommentObject()`, fixed each callsite / definition.
        - Called `transaction.search` on a diff with transactions (i.e., not a sourced-from-commit diff).
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Subscribers: jbrownEP
      
      Differential Revision: https://secure.phabricator.com/D20121
      9f5e6bee
  23. 01 Feb, 2019 1 commit
  24. 31 Jan, 2019 3 commits
    • epriestley's avatar
      Fix method name typo in new modular transaction text/html mail methods · b33333ab
      epriestley authored
      See PHI1039. See D20057.
      b33333ab
    • epriestley's avatar
      Allow modular transactions to override transaction title and body text in mail · 138c07f3
      epriestley authored
      Summary:
      Ref T12921. I'm moving Instances to modular transactions, and we have an "Alert" transaction type used to send notifications ("Your instance is going to be suspended for nonpayment.").
      
      Currently, there's no way to specifically customize mail rendering under modular transactions. Add crude support for it.
      
      Note that (per comment) this is fairly aspirational right now, since we actually always render everything as text (see T12921). But this API should (?) mostly survive intact when I fix this properly, and allows Instances to move to modular transactions so I can fix some more pressing issues in the meantime.
      
      Test Plan: See next diff for Instances.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T12921
      
      Differential Revision: https://secure.phabricator.com/D20057
      138c07f3
    • epriestley's avatar
      Update a factor query in TransactionEditor for providers · 93b512b6
      epriestley authored
      Summary: This query didn't get updated and could let you through an explicit "Sign with MFA" action if you have only disabled factors on your account.
      
      Test Plan:
        - Disabled all factors.
        - Used explicit "Sign With MFA".
          - Before: Went through.
          - After: Sensible error.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Differential Revision: https://secure.phabricator.com/D20072
      93b512b6
  25. 30 Jan, 2019 1 commit
    • epriestley's avatar
      Provide an Editor extension point for transaction validation · f7e8fa07
      epriestley authored
      Summary:
      Depends on D20040. Ref T13242. See PHI1039. See PHI873. Two reasonable cases have arisen recently where extending validation rules would help solve problems.
      
      We can do this in a pretty straightforward way with a standard extension pattern.
      
      Test Plan:
      Used this extension to keep ducks away from projects:
      
      ```lang=php
      <?php
      
      final class NoDucksEditorExtension
        extends PhabricatorEditorExtension {
      
        const EXTENSIONKEY = 'no.ducks';
      
        public function getExtensionName() {
          return pht('No Ducks!');
        }
      
        public function supportsObject(
          PhabricatorApplicationTransactionEditor $editor,
          PhabricatorApplicationTransactionInterface $object) {
          return ($object instanceof PhabricatorProject);
        }
      
        public function validateTransactions($object, array $xactions) {
          $errors = array();
      
          $name_type = PhabricatorProjectNameTransaction::TRANSACTIONTYPE;
      
          $old_value = $object->getName();
          foreach ($xactions as $xaction) {
            if ($xaction->getTransactionType() !== $name_type) {
              continue;
            }
      
            $new_value = $xaction->getNewValue();
            if ($old_value === $new_value) {
              continue;
            }
      
            if (preg_match('/duck/i', $new_value)) {
              $errors[] = $this->newInvalidTransactionError(
                $xaction,
                pht('Project names must not contain the substring "duck".'));
            }
          }
      
          return $errors;
        }
      
      }
      ```
      
      {F6162585}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13242
      
      Differential Revision: https://secure.phabricator.com/D20041
      f7e8fa07
  26. 28 Jan, 2019 1 commit
    • epriestley's avatar
      Convert "Rename User" from session MFA to one-shot MFA · d24e6672
      epriestley authored
      Summary:
      Depends on D20035. Ref T13222.
      
        - Allow individual transactions to request one-shot MFA if available.
        - Make "change username" request MFA.
      
      Test Plan:
        - Renamed a user, got prompted for MFA, provided it.
        - Saw that I no longer remain in high-security after performing the edit.
        - Grepped for other uses of `PhabricatorUserUsernameTransaction`, found none.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13222
      
      Differential Revision: https://secure.phabricator.com/D20036
      d24e6672
  27. 23 Jan, 2019 1 commit
    • epriestley's avatar
      Always require MFA to edit contact numbers · 587e9cea
      epriestley authored
      Summary:
      Depends on D20023. Ref T13222. Although I think this isn't strictly necessary from a pure security perspective (since you can't modify the primary number while you have MFA SMS), it seems like a generally good idea.
      
      This adds a slightly new MFA mode, where we want MFA if it's available but don't strictly require it.
      
      Test Plan: Disabled, enabled, primaried, unprimaried, and edited contact numbers. With MFA enabled, got prompted for MFA. With no MFA, no prompts.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13222
      
      Differential Revision: https://secure.phabricator.com/D20024
      587e9cea
  28. 21 Jan, 2019 1 commit
    • epriestley's avatar
      Apply inverse edge edits after committing primary object edits · afd2ace0
      epriestley authored
      Summary:
      Fixes T13082. When you create a revision (say, `D111`) with `Ref T222` in the body, we write a `D111 -> T222` edge ("revision 111 references task 222") and an inverse `T222 -> D111` edge ("task 222 is referenced by revision 111").
      
      We also apply a transaction to `D111` ("alice added a task: Txxx.") and an inverse transaction to `T222` ("alice added a revision: Dxxx").
      
      Currently, it appears that the inverse transaction can sometimes generate mail faster than `D111` actually commits its (database) transactions, so the mail says "alice added a revision: Unknown Object (Differential Revision)". See T13082 for evidence that this is true, and a reproduction case.
      
      To fix this, apply the inverse transaction (to `T222`) after we commit the main object (here, `D111`).
      
      This is tricky because when we apply transactions, the transaction editor automatically "fixes" them to be consistent with the database state. For example, if a task already has title "XYZ" and you set the title to "XYZ" (same title), we just no-op the transaction.
      
      It also fixes edge edits. The old sequence was:
      
        - Open (database) transaction.
        - Apply our transaction ("alice added a task").
        - Apply the inverse transaction ("alice added a revision").
        - Write the edges to the database.
        - Commit (database) transaction.
      
      Under this sequence, the inverse transaction was "correct" and didn't need to be fixed, so the fixing step didn't touch it.
      
      The new sequence is:
      
        - Open (database) transaction.
        - Apply our transaction ("alice added a task").
        - Write the edges.
        - Commit (database) transaction.
        - Apply the inverse transaction ("alice added a revision").
      
      Since the inverse transaction now happens after the database edge write, the fixing step detects that it's a no-op and throws it away if we do this naively.
      
      Instead, add some special cases around inverse edits to skip the correction/fixing logic, and just pass the "right" values in the first place.
      
      Test Plan:
      Added and removed related tasks from revisions, saw appropriate transactions render on both objects.
      
      (It's hard to be certain this completely fixes the issue since it only happened occasionally in the first place, but we can see if it happens any more on `secure`.)
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13082, T222
      
      Differential Revision: https://secure.phabricator.com/D19969
      afd2ace0
  29. 16 Jan, 2019 2 commits
    • epriestley's avatar
      Fix an issue where transactions in mail were always rendered as text · 0c0cbb1c
      epriestley authored
      Summary:
      Fixes T12921. Currently, we call `getTitleForHTMLMail()`, but that calls `getTitleForMail()` which forces us into text rendering mode.
      
      Instead, have `getTitleForHTML/TextMail()` force the rendering mode, then call `getTitleForMail()` with the desired rendering mode.
      
      This causes stories like "epriestely added dependent tasks: x, y." to appear as links in email instead of plain text.
      
      Test Plan: Used `bin/mail show-outbound --id ... --dump-html > out.html` to verify HTML mail.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T12921
      
      Differential Revision: https://secure.phabricator.com/D19968
      0c0cbb1c
    • epriestley's avatar
      Don't require "CAN_EDIT" to watch/unwatch a project · 966a9333
      epriestley authored
      Summary:
      See T1024. When "CAN_EDIT" became default in T13186, this was missed as an exception.
      
      Watching shouldn't require "CAN_EDIT", so exempt it.
      
      Test Plan:
        - Before change: tried to watch a project I could not edit, got a policy error.
        - After change: watched/unwatched a project I could not edit.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Differential Revision: https://secure.phabricator.com/D19977
      966a9333
  30. 28 Dec, 2018 3 commits
    • epriestley's avatar
      Remove the "willApplyTransactions()" hook from ApplicationTransactionEditor · 106e90dc
      epriestley authored
      Summary:
      Depends on D19908. Ref T13222. In D19897, I reordered some transaction code and affected the call order of `willApplyTransactions()`.
      
      It turns out that we use this call for only one thing, and that thing is pretty silly: naming the raw paste data file when editing paste content.
      
      This is only user-visible in the URL when you click "View Raw Paste" and seems exceptionally low-value, so remove the hook and pick a consistent name for the paste datafiles. (We could retain the name behavior in other ways, but it hardly seems worthwhile.)
      
      Test Plan:
        - Created and edited a paste.
        - Grepped for `willApplyTransactions()`.
      
      Note that `EditEngine` (vs `ApplicationTransacitonEditor`) still has a `willApplyTransactions()`, which has one callsite in Phabricator (in Calendar) and a couple in Instances. That's untouched and still works.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13222
      
      Differential Revision: https://secure.phabricator.com/D19909
      106e90dc
    • epriestley's avatar
      Allow "MFA Required" objects to be edited without MFA if the edit is only creating inverse edges · efb01bf3
      epriestley authored
      Summary:
      Depends on D19900. Ref T13222. See PHI873. When an object requires MFA, we currently require MFA for every transaction.
      
      This includes some ambiguous cases like "unsubscribe", but also includes "mention", which seems like clearly bad behavior.
      
      Allow an "MFA" object to be the target of mentions, "edit child tasks", etc.
      
      Test Plan:
        - Mentioned an MFA object elsewhere (no MFA prompt).
        - Made an MFA object a subtask of a non-MFA object (no MFA prompt).
        - Tried to edit an MFA object normally (still got an MFA prompt).
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13222
      
      Differential Revision: https://secure.phabricator.com/D19901
      efb01bf3
    • epriestley's avatar
      Allow objects to be put in an "MFA required for all interactions" mode, and... · d3c325c4
      epriestley authored
      Allow objects to be put in an "MFA required for all interactions" mode, and support "MFA required" statuses in Maniphest
      
      Summary:
      Depends on D19898. Ref T13222. See PHI873. Allow objects to opt into an "MFA is required for all edits" mode.
      
      Put tasks in this mode if they're in a status that specifies it is an `mfa` status.
      
      This is still a little rough for now:
      
        - There's no UI hint that you'll have to MFA. I'll likely add some hinting in a followup.
        - All edits currently require MFA, even subscribe/unsubscribe. We could maybe relax this if it's an issue.
      
      Test Plan:
        - Edited an MFA-required object via comments, edit forms, and most/all of the extensions. These prompted for MFA, then worked correctly.
        - Tried to edit via Conduit, failed with a reasonably comprehensible error.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13222
      
      Differential Revision: https://secure.phabricator.com/D19899
      d3c325c4