Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Store Partner IDs associated with webhook #16

Closed
kristinapathak opened this issue Aug 21, 2020 · 12 comments
Closed

Store Partner IDs associated with webhook #16

kristinapathak opened this issue Aug 21, 2020 · 12 comments
Assignees
Labels
enhancement improvement or small functionality added to an existing feature

Comments

@kristinapathak
Copy link
Contributor

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.
@joe94 joe94 transferred this issue from xmidt-org/webpa-common Feb 18, 2021
@kristinapathak kristinapathak added the enhancement improvement or small functionality added to an existing feature label Sep 10, 2021
@kristinapathak
Copy link
Contributor Author

Related to:
xmidt-org/caduceus#181
xmidt-org/scytale#99

@kristinapathak
Copy link
Contributor Author

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:

  • What should the name of the header be? Should it be configurable?
  • Where do we want to convert our []WebhookWrapper -> []Webhook? Given that Caduceus also uses ancla and wants that wrapper information, I think the ancla service has to continue to provide the []WebhookWrapper. We could convert when encoding the response? Or is there a better place for it?

@schmidtw
Copy link
Member

-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
Copy link
Member

joe94 commented Sep 14, 2021

-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
Copy link
Contributor Author

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

@kristinapathak
Copy link
Contributor Author

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
Copy link
Member

joe94 commented Sep 15, 2021

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
Copy link
Contributor Author

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

@joe94
Copy link
Member

joe94 commented Sep 15, 2021

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

@kristinapathak
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor

mtrinh11 commented Oct 6, 2021

#79

@mtrinh11 mtrinh11 closed this as completed Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement improvement or small functionality added to an existing feature
Projects
None yet
Development

No branches or pull requests

4 participants