Add support for incoming webhooks with a callback to the third-party server

[timabbott]

This is relevant for at least the Bitbucket Server (#10903) and (existing) Stripe integrations. Some third-party website's webhooks suck and don't provide the data you'd need to display them in a sensible way. E.g. Stripe only includes IDs, not human-readable names, for customers, plans and other objects in its webhooks.

This issue is a design for how we could handle these integrations well.

• In short, we'd have the Zulip view.py function for the integration do a callback to the third-party service, probably just via requests.get to get the data and render. We will want to use an initially small wrapper function in zerver/webhooks/common/ just so that we can do logging, HTTP options, timeouts, exception handling, etc. in one place as they are required. We can all this something like http_api_callback() or something. (I can also imagine future plumbing that farms this out to a queue processor for retries, etc.)
• In order to do that API callback, we'll often need some sort of credential. To get initial versions up and running, we should just include the credentials as a ? thing in the URL; then we can separate the infrastructure work from building a configuration UI (which we'll eventually be able to borrow the BotConfigData table for).
• For v2, we use BotConfigData to let users set the credentials in a more normal way.

[zulipbot]

Hello @zulip/server-integrations members, this issue was labeled with the "area: integrations" label, so you may want to check it out!

[rishig]

One note for the Stripe version of this is that the API key should always be optional. Even with the restricted permissions, it's kind of scary to give your Stripe credentials to a third party service.

[timabbott]

Sure, though we should also recommend they use https://stripe.com/docs/keys#limiting-access-with-restricted-api-keys.

[Hypro999]

Opened a WIP PR to get the ball rolling :)

[Hypro999]

@timabbott, for Stripe, shouldn't we just use the official python library stripe-python? The reason is because they tend to do a lot of initialization before contacting their own api, an example I can give from their source code would be:

headers = {
            "X-Stripe-Client-User-Agent": json.dumps(ua),
            "User-Agent": user_agent,
            "Authorization": "Bearer %s" % (api_key,),
        }

In which case we don't necessarily need the general callback method (for this integration anyhow) and we could simply just address the problem of how we want to allow users to configure their bots (in this example, how they would add a stripe API key) - this seems to be a bigger pre-requisite here either way.

[Hypro999]

@eeshangarg any comments?

[timabbott]

@Hypro999 we already have that library in the project, so it seems potentially reasonable to use it for Stripe in particular.

That said, there are other integrations where a callback either is or would be required, and I think our goal should be to figure out the correct way to do this in general (which could include an if statement that determines whether to using python-stripe or requests directly).

What makes sense depends a bit on whether we end up just needing ~10 lines of code from python-stripe for header initialization, or something else.

[Hypro999]

@timabbott why are we trying to generalize this? Each integration/service is unique in it's own way and I don't see the motive here anymore. Yes, there would be slight overlaps between certain integrations, but I'm not sure if it's worth forgoing flexibility by introducing a generalized method/function. In fact at this point, it even seems to be harmful. Due to this lack of understanding of the situation, I'm blocked on this issue.

[eeshangarg]

@timabbott: I would actually love to know your thoughts on whether we are doing this for all integrations or just specific ones (the former was my initial impression). In terms of quality, I kinda see Hemanth's point that an implementation tailored to specific integrations might be easier to use and more robust. Perhaps we should come up with a general framework that can be used to buttress specific implementations? :)

[timabbott]

I guess the two options here are as follows:

1. For each platform with a bad webhook API, integrate a third-party Python API bindings like python-stripe (if it exists). The advantage of this is that we get to use the hopefully nice syntax the library designers created for interacting with the third-party API, which can be nice in the case that we're going to hit a lot of endpoints in the third-party API. The downside is that we need to and deal with the maintenance and import time costs associated with doing so, and also developers working on Zulip's integrations need to be able to reason about the error-handling behavior of python-stripe (etc.) in order to ensure that Zulip's error handling is what we want. (E.g. we'll need to catch every version of networking error and authorization error raised by each library and turn it into what we want Zulip to do with networking and authorization errors, respectively, which we probably do want to be the same for all of our integrations with this model). We also will end up needing to figure out how to mock each third-party library for unit tests.
2. Build a generic/configurable system that makes JSON (or whatever) requests to the third party APIs using python-requests, which would do proper error handling for this sort of thing using python-requests just once. This has the advantage that the total complexity of doing 5 integrations with this model would probably be less for Zulip integrations maintainers to understand, and we can use the same mocking strategy (of using e.g. the httpretty library) for all of them. The downside is that each individual integration may , and we potentially need to deal with different annoying authentication models for different APIs (which in most cases will be ~5 lines of code, but there may be exceptions).

Which is more robust will depend on whether you think the third-party service is more likely to break their Python API or their HTTP API; I think it's pretty unclear :).

Ultimately, we may end up doing both models for different integrations. I think which model makes sense for Stripe in particular depends on (1) to what extent we'll end up needing to do dozens of requests back to the Stripe API as part of our integration, or only ~1 that we do on every request (which I can certainly imagine for some integration structures where they don't include some key single piece of data one needs to format every request). Does that make sense?

I think there's a valid strategy of doing this one with python-stripe (which we conveniently already have in the project as a dependency for our billing stuff) and then after looking at that result, we can talk about whether there's a good way to refactor it (or parts of it) in a reusable way with concrete code in front of us.

[Hypro999]

Perhaps we should come up with a general framework that can be used to buttress specific implementations?

@timabbott, @eeshangarg I agree with this. This would be a better idea IMHO. The reason is that after I began working on the Phabricator integration, I found that trying to formulate a general method or set of methods would be difficult and make me feel extremely restricted especially with APIs like Phabricator's where we would need to contact several endpoints in a manner that we can't tie to general rules we expect all incoming webhooks (which need to contact an API) to follow.

As for the disadvantages listed, we actually need to do the error handling one way or the other since we'd still be dealing multiple different third party APIs.

we'll need to catch every version of networking error and authorization error raised by each library and turn it into what we want Zulip to do with networking and authorization errors, respectively, which we probably do want to be the same for all of our integrations with this model

Point duly noted. But, the thing is each integration would present it's own set of networking and authorization errors which we would need to deal with anyways (and the person developing the integration should be aware of them). Using a library for an API would just add a few extra classes for these exceptions. Either way we would need to anticipate them and convert the message into a Zulip-style error message. What I'm trying to say here is that these cases would need to be considered anyways. Either way, I think that the benefits outweigh the costs here. But this is my opinions. @eeshangarg can we here your opinions on which approach you think is better?

Ultimately, we may end up doing both models for different integrations.

On a sidenote, this might not work because of subtle differences that easily break consistency. For example, if API A follows auth scheme X and API B follows the same auth scheme X but with a slight modification, then we would need to create an extra AuthStrategy just to accommodate for this (please quickly revisit/glance over the code attached to the related PR to see what I mean).

Also, please let me know if any of my points didn't make too much sense. I'm facing a bit of difficulty in expressing exactly what I'm thinking. I'll happily clarify any hazy points :).