Base event payload fields in GHEventPayload #947
Description
GitHub event spec declares common properties: action, sender, repository, organization, installation. But current implementation of GHEventPayload has only one common field sender, other fields declared in subclasses. Most probably because it's based on real life examples. E.g. common field action is not present in event push, that's true.
But it raises a problem. When you implement webhook handler (e.g. webmvc controller) that validates payload signature (header X-Hub-Signature), you need to resolve the secret first. In trivial case secret is the same for an instance of controller, but if it depends on repository (or even on event type) it may differ.
Imagine the method that resolves secret:
/**
* Get secret to validate payload signature by repository and action.
*
* @return secret or empty in case when validation may be omitted
*/
Optional<String> getSecret(GHRepository repository, String action);
To call this method you should repeat extraction on each type of payload. Like this:
... postHook(
@RequestHeader("X-Hub-Signature") signature: String,
@RequestHeader("X-GitHub-Event") event: String,
@RequestBody payload: String
) {
if ("push".equals(event)) {
Push push = GitHub.offline().parseEventPayload(new StringReader(payload), Push.class);
validateSignature(push.getRepository(), null /*action*/, payload, signature);
return handlePush(push);
} else if ("pull_request".equals(event)) {
...
}
}
There are few ways to address it:
- Introduce base event in the controller project (not the library) that has all required fields (workaround):
package org.kohsuke.github;
@Getter
public class BaseEventPayload extends GHEventPayload {
private String action;
private GHRepository repository;
}
- Move all mentioned fields and getters into
GHEventPayload
It has a problem: it will be not that clear where the field/getter is nullable - Move only getters to
GHEventPayload(likeabstract getRepository()), almost the same as option above - Introduce interfaces like
public interface EventRepository {
GHRepository getRepository();
}
and mark event classes where applicable.
If this problem is addressed - there should be an enhancement: you can have a method that parses an event and validates it at once. Later the common class to handle events (some kind of http servlet doGet, doPost, etc.) may be suggested on base of this change.
What do you guys think about it?