search

xmidt-org / ancla

Ancla provides event webhook registry capabilities to XMiDT services.
Apache License 2.0
1 stars 2 forks source link

Store Partner IDs associated with webhook #16

Closed kristinapathak closed 3 years ago

kristinapathak commented 4 years ago

Create a new struct that is stored in sns/argus. It should look something like this:

type WebhookWrapper struct {
  PartnerIDs []string
  Webhook webhook.W
}

Then we store the webhook registered as well as the partner IDs associated with it, allowing us to only send wrp Messages given a partner ID match.

The partner ID can be determined in two ways:

  1. From a field in the payload of a JWT.
  2. From a header, only when the Authorization provided is Basic.
kristinapathak commented 3 years ago

Related to: https://github.com/xmidt-org/caduceus/issues/181 https://github.com/xmidt-org/scytale/issues/99

kristinapathak commented 3 years ago

More details on this....

  1. In order to add the webhookWrapper everywhere needed, anything that uses the chrysom client should expect the wrapper instead of the webhook itself.
  2. when handling the endpoint to register the webhook, as a part of decoding, we want to pull the partner IDs from the request and then add them to a new field in the addWebhookRequest struct.
  3. Pulling from the request depends on the type of bascule token. First, we need to pull the token from the request context. If it's a jwt token, we should get the values from the token's attributes using the partner keys, and then convert that interface{} into a []string. If it's a basic token, we should get the values found at a specific header.
  4. For getting webhooks, after receiving the list of webhook wrappers from the chrysom client, we need to create a new list of only webhooks to return as the response body.

Remaining questions:

schmidtw commented 3 years ago 
-What should the name of the header be? Should it be configurable?

Probably configurable gives us the most flexibility unless that's a challenge.

joe94 commented 3 years ago 
-What should the name of the header be? Should it be configurable?

Probably configurable gives us the most flexibility unless that's a challenge.

Yeah we can definitely make it configurable or match the behavior from services like Tr1d1um https://github.com/xmidt-org/tr1d1um/blob/683f4c0f20bc86be8bf43d5d7ee9ef187078f40a/translation/transport.go#L74

kristinapathak commented 3 years ago

Got confirmation from @joe94 and @schmidtw to convert when encoding the response.

kristinapathak commented 3 years ago

Do we want partner ID filtering to be configurable for our services? This would mean:

  1. tr1d1um would be configured whether or not to allow no partner IDs for a webhook.
  2. caduceus would be configured whether or not to only send events if there's a partner ID in common between the wrp and webhook.
joe94 commented 3 years ago

For 1), this would essentially control whether the field is required, right? If we end up adding this option, we might want to update Tr1d1um with a similar option since today the partners are left as nil when not defined -> https://github.com/xmidt-org/tr1d1um/blob/683f4c0f20bc86be8bf43d5d7ee9ef187078f40a/translation/transport.go#L76

For 2) we could follow the monitor/enforce pattern we have for some of our other checks (ex: https://github.com/xmidt-org/talaria/blob/6cd8ee52a0a29bcb601a6cff7bd0cec460bddeee/talaria.yaml#L189)

Side question: what should caduceus do when enforcement of 2) is enabled and the partner_ids WRP event field wrp is empty? Should caduceus allow such event being sent to listeners regardless of what's in their webhook registration partners list?

kristinapathak commented 3 years ago

If caduceus was enforcing partner ID checks and had a webhook with no partners, that webhook would never receive any events.

joe94 commented 3 years ago

^ that def makes sense under the assumption the WRP partner_ids field is never empty. Can we make that assumption?

kristinapathak commented 3 years ago

Oops, I misread your original question. Ideally I think talaria would add the partner ID information to every event, and there would be at least partner there. I believe if the partner ID isn't there currently, talaria or themis set it to a default, don't they?

If we can't ensure there is a partner ID from upstream of caduceus, I think we might as well consider no partner as can be sent to any webhook. The alternative is not send the event anywhere, which doesn't seem as backwards compatible. @schmidtw, what do you think?

schmidtw commented 3 years ago

If the partner_ids is missing or empty, it seems like we probably will want to provide the following configuration options for users:

  1. Empty will be sent to all listeners.
  2. Empty will be sent to no listeners.
  3. Empty is assumed to be a specified partner_id.

This should give folks upgrade options as well as a fair bit of control.

In all cases, we should make sure a metric accounts for the presence of empty and the mechanism of dispatch.

mtrinh11 commented 3 years ago

https://github.com/xmidt-org/ancla/pull/79