fix: init webhook validation check (#117)

caraml-dev · Oct 10, 2024 · e057352 · e057352
1 parent a98c895
commit e057352
Show file tree

Hide file tree

Showing 2 changed files with 108 additions and 23 deletions.
diff --git a/api/pkg/webhooks/manager.go b/api/pkg/webhooks/manager.go
@@ -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
 	}
@@ -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() == "" {
@@ -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")

diff --git a/api/pkg/webhooks/manager_test.go b/api/pkg/webhooks/manager_test.go
@@ -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{
@@ -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{
@@ -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",
@@ -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 {