Add support for form URL-encoded data in GitHub requests

[dougbu]

Add a way for users to switch to match GitHub's default request content type.

Background

GitHub sends form URL-encoded data by default. However ASP.NET WebHooks supported only JSON data for GitHub and JSON has been the default choice in ASP.NET Core WebHooks since the original port. Since 730adf3128, ASP.NET Core WebHooks supports only JSON data. That is, ASP.NET Core WebHooks for GitHub is consistent with ASP.NET WebHooks for GitHub but neither can support the GitHub default content type.

Design

See 3. below. That is the most flexible approach, avoids runtime switches between content types, and doesn't introduce a new-for-WebHooks concept.

Possible approaches

1. Add a configuration setting to switch from JSON to form URL-encoded data. Metadata is already a service, making consuming IConfiguration straightforward. Users could then easily choose between the two content types globally -- with, say, an Azure application setting.
2. Add ASP.NET Core WebHooks' first IOptions<T> type and place the switch there. This would make it easier (than 1.) to change the configuration programmatically but harder to change it with an Azure application setting (unless we built something).
3. Add another receiver configuration. This is, basically, an attribute, a metadata class and a subclass of the existing GitHub-specific security verification filter. This approach is by far the most flexible since it allows use of both formats in the same application e.g. users configuring multiple repos differently. Its only downside would be naming and (thus) discoverability. Suggest [FormEncodedGitHubWebHook] for the attribute because that would show up in IntelliSense along with [GitHubWebHook] if the user types "[github" above an action. The receiver name i.e. one part of the configuration keys for secrets would be "formencodedgithub" (case-insensitive).

Details

GitHub still sends JSON in its content-type:"application/x-www-form-urlencoded" requests. The JSON event data is just pushed down into a payload field.

[WestDiscGolf]

Adding in another attribute for a body type seems a little strange to me. Does it not go against the rest of the webhooks api surface design?

Is the solution not to allow for the GithubWebHookAttribute to default to WebHookBodyType of Json but allow to change it in the constructor so the usages would be eg:

[GitHubWebHook]
public IActionResult GitHubHandler(string id, string @event, JObject data)

and

[GitHubWebHook(WebHookBodyType.Form)]
public IActionResult GitHubHandler(string id, string @event, JObject data)

Or

Didn't one of the webhooks in an earlier version have an option in its constructor to specify what the body type was. This could be specified in the configuration options and passed into the middleware when the webhook gets registered with the service? This way it can be driven by config, the webhooks wouldn't need to have it's own IOptions and nor would it be dependent on IConfiguration directly.

Something like this ...

public void ConfigureServices(IServiceCollection services)
{
    services
        .AddMvcCore()
        .AddGitHubWebHooks(opts =>
        {
            opts.ForceFormEncodingBody = true;
        });
}

The name needs work, but isn't naming always the hardest part :-)

We'd probably also need to be able to define it at the Attribute level as well (maybe an override?) as I mention at the beginning. Should all end points which use the GitHubWebHookAttribute in the same application be forced to use the same WebHookBodyType?

Just some thoughts :-)

[dougbu]

The concept of a receiver supporting multiple content types leads to some really fishy code. And, GitHub is the only sender I've seen that offers users a choice of content types. Lots of work for a unique case. That's why we removed this feature after a few iterations at it.

An IOptions<T> type (containing ForceFormEncodingBody) would fit fairly well in any framework layered on ASP.NET Core Mvc. However it's a new concept for ASP.NET Core WebHooks. I'd be more stoked about it if I was aware of similar needs elsewhere in this repo.

Thinking about [GitHubWebHook(WebHookBodyType.Form)] leads me to another option: Give [[GitHubWebHook(...)] that new constructor but switch the receiver name passed down to WebHookAttribute. The different receiver name would be visible to users in configuration keys and the listening URL. But, no new attribute and the change would cost less to implement. Thus, it's more likely we'd do it soon.

[WestDiscGolf]

The receiver name switch depending on the body type passed in seems reasonable to me.

Are you thinking a required parameter in the constructor so no default constructor or will json be set as default for the current implementation? I'm leaning towards the default as json at the moment personally.

[dougbu]

Yup, two constructors with JSON ("github" receiver name) being the default.

[WestDiscGolf]

Sounds good. And a "githubform" receiver?

[dougbu]

Possible names include githubform, githubformencoded, githuburlencoded, or any of those with github last e.g. urlencodedgithub. I don't have a strong leaning though I slightly prefer the variants with github last because it makes the added words seem like adjectives.

[WestDiscGolf]

If keeping github at the end is preferred then I'd lean towards "formencodedgithub", although I think I prefer the provider at the beginning of the name so from the configuration you can tell from first few characters which provider it is. So leaning toward"s "githubformencoded".

[aspnet-hello]

This issue was moved to aspnet/AspLabs#43