Page MenuHomePhabricator

Allow Herald to "Queue Call to Webhook: ..."
Closed, ResolvedPublic

Description

We have a number of external services that link users to custom maniphest forms to fill out tickets. The built in feature set for custom forms is strong enough that many developers elect to use those forms instead of making API calls. The one downside is that external services which do this have no way to then react to the user creating the ticket. It would be nice if external services could simply link to a custom form that has a configured (or URL parameter specified) redirect or callback url.

Event Timeline

yelirekim updated the task description. (Show Details)

Some general sorts of thoughts:

  • Redirecting has some security implications with sneaky/spooky OAuth token theft. Phurl pretty much means we accept this (T9744 was the last time it came up) and we could roadblock users explicitly ("All done, return to your thing? [ Stay Here ] [ Open Redirect to Danger ]") so this is likely not much of a real issue, but requires at least some care to navigate safely.
  • Currently, we don't retain any information about which form you used to complete the workflow at the end of the workflow or on the object itself. There are some adjacent reasons why we possibly should. In particular, if you use form "X" to create an object, it might be useful to default to that same form when editing it, to compartmentalize types of objects better and let a rampant jungle of fields bloom and flourish freely. This could motivate persisting which form was used instead of just passing it around in the URL.
  • Running the workflow through Nuance and then having Nuance dump into Maniphest might be another approach here, but probably more complicated for ad-hoc forms and some ways away.

I guess this is customfieldable already, isn't it? There will be weirdness with checking state in the field / the interface doesn't explicitly support it, but I'm pretty sure it's doable.

Would updating the customfield/editengine interfaces to make these kinds of things non-awkward be a better idea?

You could write a CustomField that was hidden and had some static prefilled value for each form, but I think you might have some amount of trouble getting the UX to work well after the task was created (e.g., you can put some "link back to the thing you came from" in the properties, but not easily render a banner at the top of the page with an obvious "continue" button, nor easily detect that the user just created the object and seamlessly redirect).

I guess you could do something sketchy like: if the "redirect" field is nonempty and the task was created in the last 10 seconds by the current viewer, emit a <script> tag to window.location them away. That will stop working after we implement CSP (T4340), although you could move it to a behavior after that.

I was thinking custom field that just pings a callback url was what I was thinking.

Ohhh, sorry, I missed that this is a service call thing rather than a redirect/workflow thing.

If you make calls from within a CustomField they'll block the user, and can't be retried if they fail, etc. T5462 has the more-suitable mechanisms, but none are great fits. Doorkeeper may become more approachable after T10363.

You could subclass FeedPushWorker and make FeedPublisherWorker queue it, like we do with HTTP workers.

Ok cool.

Yeah the redirect is just an alternative / potentially less brittle form of API call. The only use case is just "let external system know you filed a task".

If we just added an action to Herald like Queue Call to Webhook: http://whatever.derp/, would that cover this? I think that's often kind-of-a-bit-hacky, but probably makes a lot of these soft use cases easier.

We can get around most of the issues in T5462 by making this just queue the call into the daemon queue instead of executing it inline.

I'd want to do T10363 first and adjust the payloads to disclose less information (some discussion T5726#88706) since this lets anyone with access to Global Herald rules write policy-bypassing hook rules, versus only users with CLI access to Phabricator to adjust feed.http-hooks. So the receiving end of the hook will pretty much need to have a Conduit client and call back to Phabricator to figure out what happened, but you'd need this with a callback/redirect URL anyway.

Yes that would do it.

We can build this first, then backport it to Doorkeeper and feed.http-hooks, so this doesn't need to be an enormous mess.

However, I imagine publishing something fairly bare-bones (in particular: as nondisclosing as possible), like:

{
  "origin": {
    "type": "herald",
    "phid": "PHID-HRUL-abc"
  },
  "object": {
    "phid": "PHID-TASK-xyz"
  },
  "transactions": [
    {
      "phid": "PHID-TASK-XACT-uuu",
    },
    {
      "phid": "PHID-TASK-XACT-vvv",
    }
  ]
}

That's probably fine for this use case, but not rich enough for most existing feed.http-hooks use cases, particularly in the absence of API methods for querying transaction details in a generic way (T5873).

J5lx added a subscriber: J5lx.Dec 3 2016, 10:18 AM
arrowd added a subscriber: arrowd.Feb 9 2018, 2:06 PM
ftdysa added a subscriber: ftdysa.Feb 9 2018, 7:49 PM

I'm not entirely sure this resolves the original request, but we have webhooks now. See also PHI348 and PHI349. There's some documentation that should publish shortly and the changelog should have more details in the next couple hours.

witrin added a subscriber: witrin.EditedFeb 11 2018, 1:45 AM

@epriestley

My use case for a Webhook: Trigger a stage build when a commit is pushed to an untracked topic branch.

Here is what I did:

  • created a new Webhook using https://requestb.in/, for debugging the request
  • created a Herald rule for Commit Hook: Commit Content, which calls the Webhook
  • pushed a commit into an untracked topic branch
  • checked the Transcript

Transcript told me my push passed the new Herald rule and the new Webhook has been called. But there was no request listed in the Webhook details, also not in requestb.in. When I tested the Webhook it works.

Assuming this would work, how could I get the commit hash? In transcript there is nothing helpful listed and in the test run even less; only transactions recarding Herald but no further PHIDS.

I'm using ffc5c95c2f3dca03a62db25130e89408b707538c.

Please use the Discourse community forum or your organization's Support Pact to get help with support questions.

@epriestley Ah okay sorry, I'll do that! But I think the first part is a bug.