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

Filter on PartnerID #181

Closed
kcajmagic opened this issue Feb 10, 2020 · 6 comments
Closed

Filter on PartnerID #181

kcajmagic opened this issue Feb 10, 2020 · 6 comments
Labels
argus-webhooks Securing webhooks for the switch to argus enhancement partner-id

Comments

@kcajmagic
Copy link
Member

Add the ability to filter events by partnerID.

@joe94 joe94 added the argus-webhooks Securing webhooks for the switch to argus label May 20, 2020
@joe94
Copy link
Member

joe94 commented May 20, 2020

This should be a configurable feature. Off by default.

@kristinapathak
Copy link
Contributor

kristinapathak commented Aug 14, 2020

AFAIK, this requires modifying the webhook struct, so that caduceus knows what partner id registered for a webhook:
https:/xmidt-org/webpa-common/blob/main/webhook/webhook.go

If we want to add a field to this struct that is a part of our api, this could be a good time, since we need to add a field for this issue:
https:/xmidt-org/webpa-common/issues/514
Then we can just increase the api version once.

We do already have fields that can be sent in the request that we don't modify:
https:/xmidt-org/webpa-common/blob/main/webhook/webhook.go#L115
But this could also be a good time to diverge the two - store a different struct than the one we allow as a part of the request to register a webhook. Then we can store information that we add in addition to information sent as a part of the registration request.

@kristinapathak kristinapathak added needs discussion and removed Good first issue Good for newcomers labels Aug 14, 2020
@kristinapathak
Copy link
Contributor

kristinapathak commented Aug 21, 2020

Based on discussion, we agreed that struct passed to sns/argus will be changed to something similar to:

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

When a new webhook is created, the partner id will be determined from the JWT provided. If basic auth is provided, we'll pull them from a header (@joe94 knows more about this). Issue related to this: https:/xmidt-org/webpa-common/issues/517

Caduceus will only send an event to a webhook if one of the device's partner ID is contained in the webhook partner IDs list (or if the wildcard is present in the webhook partner ID list). It should be configurable whether to monitor the events for device ID or actually enforce it.

We should have a metric for this so we know the partner id of the events we are sending to each webhook (and if an event isn't sent because the webhook isn't authorized to receive it). This can probably be added to a metric we already have.

@joe94
Copy link
Member

joe94 commented Aug 21, 2020

If basic auth is provided, the partnerIDs should be fetched from the HTTP headers like this: https:/xmidt-org/tr1d1um/blob/main/translation/transport.go#L75. This might be a good opportunity to move this shared logic into wrp-go/wrphttp or somewhere more fitting.

@joe94
Copy link
Member

joe94 commented Aug 21, 2020

Two quick things:

  1. Any better names than WebhookWrapper? like InternalWebhook maybe? A better name to convey that this is a struct that's internal to XMiDT to filter events.
  2. Should we make PartnerIDs of type map[string]bool? That way to make it clear we want the check to be O(N) for N=size of the partners list in the WRP

@kristinapathak
Copy link
Contributor

Fixed in #276.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
argus-webhooks Securing webhooks for the switch to argus enhancement partner-id
Projects
None yet
Development

No branches or pull requests

3 participants