feat(webhook): add verification expression support

[igpetrov]

feat(webhook): add verification expression support

[igpetrov]

@sbuettner do you consider to put it into 8.3.1? If so, I will go ahead, and create a QA ticket.

[sbuettner]

I am curious by the way why we went for a dedicated interface public interface VerifiableWebhook and extend it automatically in the WebhookConnectorExecutable?

If it would be optional to the implementor we could remove the default implementation etc.

[igpetrov]

I am curious by the way why we went for a dedicated interface public interface VerifiableWebhook and extend it automatically in the WebhookConnectorExecutable?

If it would be optional to the implementor we could remove the default implementation etc.

I prefer to work with smaller interfaces with explicitly defined purpose. In that case, verification expression is covered by a dedicated interface.

[sbuettner]

explicitly defined purpose

Agreed, but why extend it in the default WebhookConnectorExecutable? You could get rid of the whole default behaviour if its optional for users to implement.

[igpetrov]

explicitly defined purpose

Agreed, but why extend it in the default WebhookConnectorExecutable? You could get rid of the whole default behaviour if its optional for users to implement.

Sorry, maybe I am not getting your concern right. WebhookConnectorExecutable is a core interface, which is used in many places, like InboundConnectorManager and InboundConnectorRestController. By extending WebhookConnectorExecutable with VerifiableWebhook I am implicitly defining relation WebhookConnectorExecutable-[is-a]-VerifiableWebhook which several instanceof calls. Or maybe I misunderstand your concern 🤔 .

[sbuettner]

@igpetrov Usually one would extends the VerifiableWebhook with the WebhookConnectorExecutable. Meaning that its an extra capability that one can implement optionally. If you implement a VerifiableWebhook it verifies but its also executable. You implemented in a way that every WebhookConnectorExecutable is verifiable. I hope that makes it clearer.

[igpetrov]

@igpetrov Usually one would extends the VerifiableWebhook with the WebhookConnectorExecutable. Meaning that its an extra capability that one can implement optionally. If you implement a VerifiableWebhook it verifies but its also executable. You implemented in a way that every WebhookConnectorExecutable is verifiable. I hope that makes it clearer.

I guess, I understand what you mean and it makes sense. What I am trying to achieve with it are two things: (1) VerifiableWebhook is more like an extention to make code clearer, I think we agreed on that; (2) we currently heavily rely on WebhookConnectorExecutable interface in runtime. If I make VerifiableWebhook extending WebhookConnectorExecutable, then I may need to perform a bit of refactoring.

So extending WebhookConnectorExecutable with VerifiableWebhook is just more practical. In addition to that, VerifiableWebhook has just a verify method. Potentially, we could be able to extend with it other interfaces, such as HttpPollingConnector.
Maybe we should consider the following: (1) give it better naming (Verifiable, VerifiableInboundConnector?), and (2) rework it in such a way, that it fits not only webhooks? WDYT?

[sbuettner]

Not sure if there is a use-case for a VerifiableInboundConnector but having a simple interface could allow this. Although I would be careful as we return http related information (status) with the verification here which doesnt apply for other connectors like kafka for example.

we currently heavily rely on WebhookConnectorExecutable interface in runtime.

Exactly and its totally fine. The nice thing with having a simple Verifiable or VerifiableWebhook is that you can just check whether a webhook implements that interface and execute the verification if its the case. We could remove the whole default behaviour this way and keep the interfaces clean, simple and combinable.

[sbuettner]

@igpetrov Please take a look at this proposal to make it more clear: https://github.com/camunda/connectors/tree/webhook-verification-expression-idea

[igpetrov]

@igpetrov Please take a look at this proposal to make it more clear: https://github.com/camunda/connectors/tree/webhook-verification-expression-idea

That works for me. I thought it to be more cohesive but that works for me as well. Let me think about it a bit first.

[sbuettner]

@igpetrov Of course, no need to rush here and I could live with both approaches.

Also chiming in @chillleader for a second pair of eyes.

[igpetrov]

@igpetrov Of course, no need to rush here and I could live with both approaches.

Also chiming in @chillleader for a second pair of eyes.

So, after thinking about the proposal, it seems to me alright, my only concern now is that previously customer had to implement an interface WebhookConnectorExecutable which was essentially a single one they need to know about when talking about webhook implementation. Now they need to learn a new one VerifiableWebhook. This may theoretically cause some confusion.

But overall, I like the approach. Happy to discuss it with the team. WDYT @sbuettner @chillleader @markfarkas-camunda @Oleksiivanov?

[sbuettner]

@igpetrov

it seems to me alright, my only concern now is that previously customer had to implement an interface WebhookConnectorExecutable which was essentially a single one

Valid point, lets continue as it is right now

[chillleader]

/backport

[github-actions]

Successfully created backport PR for release/8.3:

• [Backport release/8.3] feat(webhook): add verification expression support #1596