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.

Sign up for GitHub

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

fix: init webhook validation check #117

Merged
merged 1 commit into from
Oct 10, 2024
Merged

fix: init webhook validation check #117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 30 additions & 17 deletions api/pkg/webhooks/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,9 @@ func parseAndValidateConfig(
if err := validateClients(allClients); err != nil {
return nil, err
}
if err := validateSyncClients(syncClients); err != nil {
return nil, err
}
syncClientMap[eventType] = syncClients
asyncClientMap[eventType] = asyncClients
}
Expand All @@ -153,22 +156,39 @@ func validateWebhookResponse(content []byte) error {
return fmt.Errorf("webhook response is not a valid json object and not empty")
}

// validateClients ensures that there are no duplicate webhook names
func validateClients(webhookClients []WebhookClient) error {
// ensure that only 1 sync client has finalResponse set to true
existingWebhooks := make(map[string]bool)
for _, client := range webhookClients {
if existingWebhooks[client.GetName()] {
return fmt.Errorf("duplicate webhook name")
}
existingWebhooks[client.GetName()] = true
}

return nil
}

// validateSyncClients ensures that only 1 sync client has finalResponse set to true;
// and for clients that use the response from another client, the used client is
// defined before the client that uses it
func validateSyncClients(webhookClients []WebhookClient) error {
if len(webhookClients) == 0 {
return nil
}

isFinalResponseSet := false
// Check for duplicate webhook names
webhookNames := make(map[string]int)
for idx, client := range webhookClients {
existingWebhooks := make(map[string]bool)

for _, client := range webhookClients {
if client.IsFinalResponse() {
if isFinalResponseSet {
return fmt.Errorf("only 1 sync client can have finalResponse set to true")
}
isFinalResponseSet = true
}
if _, ok := webhookNames[client.GetName()]; ok {
return fmt.Errorf("duplicate webhook name")
}
webhookNames[client.GetName()] = idx

existingWebhooks[client.GetName()] = true
// Ensure that if a client uses the response from another client, that client exists
// If a client uses the response from another client, it must be defined before it
if client.GetUseDataFrom() == "" {
Expand All @@ -177,17 +197,10 @@ func validateClients(webhookClients []WebhookClient) error {
// since the payload used will be the user's payload
continue
}
useIdx, ok := webhookNames[client.GetUseDataFrom()]
if !ok {

if client.GetName() == client.GetUseDataFrom() || !existingWebhooks[client.GetUseDataFrom()] {
return fmt.Errorf("webhook name %s not found", client.GetUseDataFrom())
}
if useIdx > idx {
return fmt.Errorf(
"webhook name %s must be defined before %s",
client.GetUseDataFrom(),
client.GetName(),
)
}
}
if !isFinalResponseSet {
return fmt.Errorf("at least 1 sync client must have finalResponse set to true")
Expand Down
84 changes: 78 additions & 6 deletions api/pkg/webhooks/manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ func TestInitializeWebhooks(t *testing.T) {
expectedError: true,
},
{
name: "Config enabled with multiple webhooks",
name: "Config enabled with multiple webhooks all sync",
cfg: &Config{
Enabled: true,
Config: map[EventType][]WebhookConfig{
Expand All @@ -152,6 +152,30 @@ func TestInitializeWebhooks(t *testing.T) {
eventList: []EventType{"event1"},
expectedError: false,
},
{
name: "Config enabled with multiple webhooks all async",
cfg: &Config{
Enabled: true,
Config: map[EventType][]WebhookConfig{
"event1": {
{
Name: "webhook1",
URL: "http://example.com",
Method: "POST",
Async: true,
},
{
Name: "webhook2",
URL: "http://example.com",
Method: "POST",
Async: true,
},
},
},
},
eventList: []EventType{"event1"},
expectedError: false,
},
{
name: "Config enabled with multiple webhooks fail (both set FinalResponse)",
cfg: &Config{
Expand Down Expand Up @@ -247,18 +271,41 @@ func TestInitializeWebhooks(t *testing.T) {
eventList: []EventType{"event1"},
expectedError: true,
},
{
name: "Config enabled with multiple webhooks with sync and async",
cfg: &Config{
Enabled: true,
Config: map[EventType][]WebhookConfig{
"event1": {
{
Name: "webhook2",
URL: "http://example.com",
Method: "POST",
Async: true,
},
{
Name: "webhook1",
URL: "http://example.com",
Method: "POST",
FinalResponse: true,
},
},
},
},
eventList: []EventType{"event1"},
expectedError: false,
},
{
name: "Config enabled with multiple webhooks with sync and async, fail with duplicate names",
cfg: &Config{
Enabled: true,
Config: map[EventType][]WebhookConfig{
"event1": {
{
Name: "webhook2",
URL: "http://example.com",
Method: "POST",
UseDataFrom: "webhook1",
Async: true,
Name: "webhook2",
URL: "http://example.com",
Method: "POST",
Async: true,
},
{
Name: "webhook1",
Expand All @@ -278,6 +325,31 @@ func TestInitializeWebhooks(t *testing.T) {
eventList: []EventType{"event1"},
expectedError: true,
},
{
name: "Config enabled with multiple webhooks fail (no final response set for sync webhook)",
cfg: &Config{
Enabled: true,
Config: map[EventType][]WebhookConfig{
"event1": {
{
Name: "webhook1",
URL: "http://example.com",
Method: "POST",
FinalResponse: true,
Async: true,
},
{
Name: "webhook2",
URL: "http://example.com",
Method: "POST",
Async: false,
},
},
},
},
eventList: []EventType{"event1"},
expectedError: true,
},
}

for _, test := range tests {
Expand Down
Loading