Improvements to webhooks endpoints #142

j616 · 2025-07-25T14:22:19Z

Details

While developing fine-grained auth approaches in #115 , several deficiencies were noticed with the Webhooks endpoints:

The CRUD patterns on the Webhooks endpoint do not match the rest of the API
The use of a secret attribute for auth on this endpoint is vulnerable to the loss of that secret. This would prevent editing or deleting of a webhook.
The BBC R&D (internal experimental) implementation and AWS implementation of TAMS differed in their interpretation of what is the primary key
Strict interpretation of the spec, and hiding of the secret api_key_value in the Webhooks listing, could result in multiple identical looking Webhooks being returned
The existing approach prevents "matrixing" of event types and filters (e.g. to receive all flows/created events, but only flows/segments_added for specific flows)

This PR proposes changes to the Webhooks endpoint to fix these issues.

Jira Issue (if relevant)

Jira URL: https://jira.dev.bbc.co.uk/browse/CLOUDFIT-5461

Related PRs

If merged before #115, add the new endpoints to the auth logic listings in that PR. If merged after, add that logic in this PR

Submitter PR Checks

(tick as appropriate)

PR completes task/fixes bug
API version has been incremented if necessary
ADR status has been updated, and ADR implementation has been recorded
Documentation updated (README, etc.)
PR added to Jira Issue (if relevant)
Follow-up stories added to Jira

Reviewer PR Checks

(tick as appropriate)

PR completes task/fixes bug
Design makes sense, and fits with our current code base
Code is easy to follow
PR size is sensible
Commit history is sensible and tidy

Info on PRs

The checks above are guidelines. They don't all have to be ticked, but they should all have been considered.

samdbmg

Changes LGTM - couple of docs nits inlined

api/schemas/webhook-with-id.json

samdbmg · 2025-08-18T16:39:35Z

api/TimeAddressableMediaStore.yaml

+        Register to receive event notifications as webhooks on a specified URL. Webhook messages will conform to the
+        format in the `webhooks` section of the API docs, depending on the event type (as defined in the same section).
+        Availability of this endpoint is indicated by the name "webhooks" appearing in the `event_stream_mechanisms`
+        list on the service endpoint.
+
+        HTTP requests from the service SHOULD include a `api_key_name` header with the 'api_key_value' value. Clients SHOULD verify this against the value they provided when registering the webhook.
+
+        API implementations MAY partially support event filtering and transformations.
+        API implementations SHALL return a 400 response code if the filtering or transformation specified in the request is not supported.
+
+        API implementations SHOULD consider the security implementations of providing webhooks, and include appropriate mitigations against Server Side Request Forgery (SSRF) attacks and similar. API implementations SHOULD take appropriate steps to authorize the modification of existing webhooks. This may take the form of RBAC, or ABAC.


I think this might need a tweak to match this being an update endpoint now

Tweaked in fdcd744

api/TimeAddressableMediaStore.yaml

api/schemas/webhook.json

api/TimeAddressableMediaStore.yaml

danjhd · 2025-09-02T11:37:28Z

Something that came up in a conversation today and may be something to include in this PR. Should we be supplying more "data" in the deleted events payload, for flows and sources? We had a use case where we wanted to be notified if a "multi" Source is deleted. Since the payload for deleted events only has the id this is not currently easy/possible.

j616 · 2025-09-02T12:56:02Z

Something that came up in a conversation today and may be something to include in this PR. Should we be supplying more "data" in the deleted events payload, for flows and sources? We had a use case where we wanted to be notified if a "multi" Source is deleted. Since the payload for deleted events only has the id this is not currently easy/possible.

I think that sounds reasonable. Would you be able to provide some user stories/use cases we can use to inform the change? I'm mainly wondering if that data should be the whole Source/Flow metadata, or just a subset. I wonder if it should be a separate ADR and/or PR to avoid adding even more to this very large change set!

danjhd · 2025-09-02T13:08:17Z

Something that came up in a conversation today and may be something to include in this PR. Should we be supplying more "data" in the deleted events payload, for flows and sources? We had a use case where we wanted to be notified if a "multi" Source is deleted. Since the payload for deleted events only has the id this is not currently easy/possible.

I think that sounds reasonable. Would you be able to provide some user stories/use cases we can use to inform the change? I'm mainly wondering if that data should be the whole Source/Flow metadata, or just a subset. I wonder if it should be a separate ADR and/or PR to avoid adding even more to this very large change set!

Separate PR is best plan. I just figured it was worth "sounding out" the idea here first in case it was considered "silly" ;)

api/TimeAddressableMediaStore.yaml

danjhd · 2025-09-03T06:37:49Z

api/TimeAddressableMediaStore.yaml

+        "204":
+          description: No content. The webhook has been deleted.
+        "403":
+          description: Forbidden. You do not have permission to modify this webhook.


Since we don't have a read-only field on the webhook, like we do on Flows i was just wondering if this 403 is needed explicitly? Since i guess this will be produced by the RBAC/ABAC/Coarse access rules? Unless i have missed something. Feels like if we are including a 403 here we should go back and add 403 to all methods?

Yes. This was added for access control use cases. We should make sure we're consistent with this.

danjhd · 2025-09-18T16:03:05Z

I think it might be a good idea to have a status field (values of enabled and disabled) on a webhook. This would serve 2 purposes:

It would be updateable on the PUT endpoint so that a client can disable a webhook for a period if needed without deleting it and without the need to alter it in any other way.
It would allow the implementation to automatically disable webhooks after a defined number of failures so avoid continuously loading the system with POSTs that will fail. The addition of an optional error field (readonly) would be useful to give feedback to the client on the reason for the disablement if it was done by the system rather than the client.

In our implementation i need to solve the problem of repeated failures and thought about adding a disabled flag but it think this might be useful in the main spec, and also worth adding as part of this PR as it is the same schema?

j616 · 2025-09-23T14:52:11Z

Rebased

j616 · 2025-09-23T14:53:29Z

I think it might be a good idea to have a status field (values of enabled and disabled) on a webhook. This would serve 2 purposes:

It would be updateable on the PUT endpoint so that a client can disable a webhook for a period if needed without deleting it and without the need to alter it in any other way.

It would allow the implementation to automatically disable webhooks after a defined number of failures so avoid continuously loading the system with POSTs that will fail. The addition of an optional error field (readonly) would be useful to give feedback to the client on the reason for the disablement if it was done by the system rather than the client.

In our implementation i need to solve the problem of repeated failures and thought about adding a disabled flag but it think this might be useful in the main spec, and also worth adding as part of this PR as it is the same schema?

Initial solution proposed in #150 was merged into this PR. This was expanded on in 087510c

j616 · 2025-09-24T09:45:35Z

Rebased

samdbmg

One question, but LGTM

api/schemas/webhook-get.json

…state will be omitted

…eld to Webhooks sem-ver: api-break

…changes

Co-authored-by: Sam Mesterton-Gibbons <sam.mesterton-gibbons@bbc.co.uk>

@srstrong

Thanks to @srstrong for identifying this in id3as#1

…ility

j616 · 2025-09-24T12:35:12Z

Rebasing again

j616 requested a review from a team as a code owner July 25, 2025 14:22

j616 mentioned this pull request Aug 7, 2025

appnote: Add appnote covering Authorisation #115

Closed

11 tasks

samdbmg approved these changes Aug 18, 2025

View reviewed changes

GeorginaShippey reviewed Aug 21, 2025

View reviewed changes

api/TimeAddressableMediaStore.yaml Outdated Show resolved Hide resolved

danjhd reviewed Sep 1, 2025

View reviewed changes

api/schemas/webhook.json Show resolved Hide resolved

danjhd reviewed Sep 2, 2025

View reviewed changes

api/TimeAddressableMediaStore.yaml Outdated Show resolved Hide resolved

danjhd reviewed Sep 3, 2025

View reviewed changes

api/TimeAddressableMediaStore.yaml Outdated Show resolved Hide resolved

danjhd reviewed Sep 3, 2025

View reviewed changes

danjhd mentioned this pull request Sep 19, 2025

add status and error fields to webhook records #150

Merged

11 tasks

j616 force-pushed the jamessa-webhooks branch from 210a94b to 087510c Compare September 23, 2025 14:51

j616 requested a review from samdbmg September 24, 2025 08:54

j616 force-pushed the jamessa-webhooks branch from 52b8493 to c051a0d Compare September 24, 2025 09:45

samdbmg approved these changes Sep 24, 2025

View reviewed changes

api/schemas/webhook-get.json Show resolved Hide resolved

j616 and others added 10 commits September 24, 2025 13:34

Remove api_key_value from GET webhook body schema, that we already …

1e0239f

…state will be omitted

Re-structure webhook endpoints, factor out examples, and add an ID fi…

ec2cf27

…eld to Webhooks sem-ver: api-break

Add ADR concerning webhook improvements

c8b1e04

Add ADR0037 to ADR listing

19b0bac

Add link to implementation of ADR0037

26f03fb

Allow creation of webhooks using existing method to reduce impact of …

d4f7a59

…changes

Update api/schemas/webhook-with-id.json

46a5b1e

Co-authored-by: Sam Mesterton-Gibbons <sam.mesterton-gibbons@bbc.co.uk>

Tweak description of webhook PUT

9e97ffa

Remove un-needed 204

436c6e2

Add paging to webhooks listing endpoint

08eb9ba

j616 and others added 9 commits September 24, 2025 13:34

Add enum to webhook event type array

e1dd3d1

Fix event type const for flows/segments_deleted

11a61c6

Thanks to @srstrong for identifying this in id3as#1

Fix typo

95b175c

add status and error fields to webhook records

a16732f

Add default value for webhook status

2f614be

Fix duplicate operation ID

afefad6

Update style to match that used in recent audit of documentation

c78f41f

Update webhook status to more closely match delete requests

d425195

Make webhook status optional on POST to maintain backwards compatib…

77f528b

…ility

j616 force-pushed the jamessa-webhooks branch from c051a0d to 77f528b Compare September 24, 2025 12:34

Mark ADR0037 as accepted

0cb376a

j616 merged commit 1e87170 into main Sep 24, 2025
8 checks passed

j616 deleted the jamessa-webhooks branch September 24, 2025 14:33

Improvements to webhooks endpoints #142

Improvements to webhooks endpoints #142

Uh oh!

Conversation

j616 commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Jira Issue (if relevant)

Related PRs

Submitter PR Checks

Reviewer PR Checks

Info on PRs

Uh oh!

samdbmg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

samdbmg Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

j616 Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danjhd commented Sep 2, 2025

Uh oh!

j616 commented Sep 2, 2025

Uh oh!

danjhd commented Sep 2, 2025

Uh oh!

Uh oh!

danjhd Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

j616 Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

danjhd commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

j616 commented Sep 23, 2025

Uh oh!

j616 commented Sep 23, 2025

Uh oh!

j616 commented Sep 24, 2025

Uh oh!

samdbmg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

j616 commented Sep 24, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

j616 commented Jul 25, 2025 •

edited

Loading

danjhd commented Sep 18, 2025 •

edited

Loading