[MM-51404]: Added logic to generate unique webhook secret for subscri…

…ptions (#152)

Co-authored-by: Abhishek Verma <[email protected]>
Co-authored-by: raghavaggarwal2308 <[email protected]>

go.mod

@@ -5,6 +5,7 @@ go 1.16
 require (
 	bou.ke/monkey v1.0.2
 	github.com/golang/mock v1.6.0
+	github.com/google/uuid v1.3.0
 	github.com/gorilla/mux v1.8.0
 	github.com/mattermost/mattermost-plugin-api v0.0.27
 	github.com/mattermost/mattermost-server/v5 v5.37.9

plugin.json

@@ -47,12 +47,6 @@
                 "display_name": "Encryption Secret:",
                 "type": "generated",
                 "help_text": "The secret key used to encrypt and decrypt the OAuth token.\nRegenerating the secret will require all users to re-connect their accounts to Azure DevOps."
-            },
-            {
-                "key": "WebhookSecret",
-                "display_name": "Webhook Secret:",
-                "type": "generated",
-                "help_text": "The secret key used to authenticate incoming requests to notification webhook used for subscriptions.\nRegenerating the secret will require all users to create new subscriptions as existing ones will no longer be authenticated."
             }
         ]
     }

server/config/config.go

@@ -23,7 +23,6 @@ type Configuration struct {
 	AzureDevopsOAuthAppID        string `json:"azureDevopsOAuthAppID"`
 	AzureDevopsOAuthClientSecret string `json:"azureDevopsOAuthClientSecret"`
 	EncryptionSecret             string `json:"EncryptionSecret"`
-	WebhookSecret                string `json:"WebhookSecret"`
 	MattermostSiteURL            string
 }
 
@@ -40,7 +39,6 @@ func (c *Configuration) ProcessConfiguration() error {
 	c.AzureDevopsOAuthAppID = strings.TrimSpace(c.AzureDevopsOAuthAppID)
 	c.AzureDevopsOAuthClientSecret = strings.TrimSpace(c.AzureDevopsOAuthClientSecret)
 	c.EncryptionSecret = strings.TrimSpace(c.EncryptionSecret)
-	c.WebhookSecret = strings.TrimSpace(c.WebhookSecret)
 
 	return nil
 }
@@ -59,9 +57,6 @@ func (c *Configuration) IsValid() error {
 	if c.EncryptionSecret == "" {
 		return errors.New(constants.EmptyEncryptionSecretError)
 	}
-	if c.WebhookSecret == "" {
-		return errors.New(constants.EmptyWebhookSecretError)
-	}
 
 	return nil
 }

server/config/config_test.go

@@ -22,7 +22,6 @@ func TestIsValid(t *testing.T) {
 				AzureDevopsOAuthAppID:        "mockAzureDevopsOAuthAppID",
 				AzureDevopsOAuthClientSecret: "mockAzureDevopsOAuthClientSecret",
 				EncryptionSecret:             "mockEncryptionSecret",
-				WebhookSecret:                "mockWebhookSecret",
 			},
 		},
 		{
@@ -32,7 +31,6 @@ func TestIsValid(t *testing.T) {
 				AzureDevopsOAuthAppID:        "mockAzureDevopsOAuthAppID",
 				AzureDevopsOAuthClientSecret: "mockAzureDevopsOAuthClientSecret",
 				EncryptionSecret:             "mockEncryptionSecret",
-				WebhookSecret:                "mockWebhookSecret",
 			},
 			errMsg: constants.EmptyAzureDevopsAPIBaseURLError,
 		},
@@ -43,7 +41,6 @@ func TestIsValid(t *testing.T) {
 				AzureDevopsOAuthAppID:        "",
 				AzureDevopsOAuthClientSecret: "mockAzureDevopsOAuthClientSecret",
 				EncryptionSecret:             "mockEncryptionSecret",
-				WebhookSecret:                "mockWebhookSecret",
 			},
 			errMsg: constants.EmptyAzureDevopsOAuthAppIDError,
 		},
@@ -54,7 +51,6 @@ func TestIsValid(t *testing.T) {
 				AzureDevopsOAuthAppID:        "mockAzureDevopsOAuthAppID",
 				AzureDevopsOAuthClientSecret: "",
 				EncryptionSecret:             "mockEncryptionSecret",
-				WebhookSecret:                "mockWebhookSecret",
 			},
 			errMsg: constants.EmptyAzureDevopsOAuthClientSecretError,
 		},
@@ -65,21 +61,9 @@ func TestIsValid(t *testing.T) {
 				AzureDevopsOAuthAppID:        "mockAzureDevopsOAuthAppID",
 				AzureDevopsOAuthClientSecret: "mockAzureDevopsOAuthClientSecret",
 				EncryptionSecret:             "",
-				WebhookSecret:                "mockWebhookSecret",
 			},
 			errMsg: constants.EmptyEncryptionSecretError,
 		},
-		{
-			description: "configuration: empty WebhookSecret",
-			config: &Configuration{
-				AzureDevopsAPIBaseURL:        "mockAzureDevopsAPIBaseURL",
-				AzureDevopsOAuthAppID:        "mockAzureDevopsOAuthAppID",
-				AzureDevopsOAuthClientSecret: "mockAzureDevopsOAuthClientSecret",
-				EncryptionSecret:             "mockEncryptionSecret",
-				WebhookSecret:                "",
-			},
-			errMsg: constants.EmptyWebhookSecretError,
-		},
 	} {
 		t.Run(testCase.description, func(t *testing.T) {
 			err := testCase.config.IsValid()

server/constants/messages.go

@@ -35,7 +35,6 @@ const (
 	// #nosec G101 -- This is a false positive. The below line is not a hardcoded credential
 	EmptyAzureDevopsOAuthClientSecretError = "azure devops OAuth client secret should not be empty"
 	EmptyEncryptionSecretError             = "encryption secret should not be empty"
-	EmptyWebhookSecretError                = "webhook secret should not be empty"
 	ProjectIDRequired                      = "project ID is required"
 	FiltersRequired                        = "filters required"
 )

server/plugin/api.go

@@ -12,6 +12,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/google/uuid"
 	"github.com/gorilla/mux"
 	"github.com/mattermost/mattermost-server/v5/model"
 	"golang.org/x/text/cases"
@@ -318,13 +319,20 @@ func (p *Plugin) handleCreateSubscription(w http.ResponseWriter, r *http.Request
 		return
 	}
 
-	subscription, statusCode, err := p.Client.CreateSubscription(body, project, body.ChannelID, p.GetPluginURL(), mattermostUserID)
+	uniqueWebhookSecret := uuid.New().String()
+	subscription, statusCode, err := p.Client.CreateSubscription(body, project, body.ChannelID, p.GetPluginURL(), mattermostUserID, uniqueWebhookSecret)
 	if err != nil {
 		p.API.LogError(constants.CreateSubscriptionError, "Error", err.Error())
 		p.handleError(w, r, &serializers.Error{Code: statusCode, Message: err.Error()})
 		return
 	}
 
+	if err := p.Store.StoreSubscriptionAndChannelIDMap(subscription.ID, uniqueWebhookSecret, body.ChannelID); err != nil {
+		p.API.LogError("Error storing channel ID for subscription", "Error", err.Error())
+		p.handleError(w, r, &serializers.Error{Code: http.StatusInternalServerError, Message: err.Error()})
+		return
+	}
+
 	channel, channelErr := p.API.GetChannel(body.ChannelID)
 	if channelErr != nil {
 		p.API.LogError(constants.GetChannelError, "Error", channelErr.Error())
@@ -560,30 +568,24 @@ func (p *Plugin) getPipelineReleaseEnvironmentList(environments []*serializers.E
 }
 
 func (p *Plugin) handleSubscriptionNotifications(w http.ResponseWriter, r *http.Request) {
-	webhookSecret := r.URL.Query().Get(constants.AzureDevopsQueryParamWebhookSecret)
-	if status, err := p.VerifyWebhookSecret(webhookSecret); err != nil {
-		p.API.LogError(constants.ErrorUnauthorisedSubscriptionsWebhookRequest, "Error", err.Error())
-		p.handleError(w, r, &serializers.Error{Code: status, Message: constants.ErrorUnauthorisedSubscriptionsWebhookRequest})
-		return
-	}
-
 	body, err := serializers.SubscriptionNotificationFromJSON(r.Body)
 	if err != nil {
 		p.API.LogError("Error in decoding the body for listening notifications", "Error", err.Error())
 		p.handleError(w, r, &serializers.Error{Code: http.StatusBadRequest, Message: err.Error()})
 		return
 	}
 
-	channelID := r.URL.Query().Get(constants.AzureDevopsQueryParamChannelID)
-	if channelID == "" {
-		p.API.LogError(constants.ChannelIDRequired)
-		p.handleError(w, r, &serializers.Error{Code: http.StatusBadRequest, Message: constants.ChannelIDRequired})
+	webhookSecret := r.URL.Query().Get(constants.AzureDevopsQueryParamWebhookSecret)
+	if webhookSecret == "" {
+		p.API.LogError(constants.ErrorUnauthorisedSubscriptionsWebhookRequest)
+		p.handleError(w, r, &serializers.Error{Code: http.StatusUnauthorized, Message: constants.ErrorUnauthorisedSubscriptionsWebhookRequest})
 		return
 	}
 
-	if !model.IsValidId(channelID) {
-		p.API.LogError(constants.InvalidChannelID)
-		p.handleError(w, r, &serializers.Error{Code: http.StatusBadRequest, Message: constants.InvalidChannelID})
+	channelID, status, err := p.VerifySubscriptionWebhookSecretAndGetChannelID(body.SubscriptionID, webhookSecret)
+	if err != nil {
+		p.API.LogError("Unable to verify webhook secret for subscription", "Error", err.Error())
+		p.handleError(w, r, &serializers.Error{Code: status, Message: err.Error()})
 		return
 	}
 

server/plugin/api_test.go

@@ -21,7 +21,6 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/mattermost/mattermost-plugin-azure-devops/mocks"
-	"github.com/mattermost/mattermost-plugin-azure-devops/server/config"
 	"github.com/mattermost/mattermost-plugin-azure-devops/server/constants"
 	"github.com/mattermost/mattermost-plugin-azure-devops/server/serializers"
 	"github.com/mattermost/mattermost-plugin-azure-devops/server/testutils"
@@ -41,10 +40,6 @@ func setupMockPlugin(api *plugintest.API, store *mocks.MockKVStore, client *mock
 		p.Store = store
 	}
 
-	p.setConfiguration(&config.Configuration{
-		WebhookSecret: "mockWebhookSecret",
-	})
-
 	if client != nil {
 		p.Client = client
 	}
@@ -366,6 +361,7 @@ func TestHandleDeleteAllSubscriptions(t *testing.T) {
 				mockedClient.EXPECT().DeleteSubscription(gomock.Any(), gomock.Any(), gomock.Any()).Return(testCase.statusCode, testCase.err)
 				if testCase.err == nil {
 					mockedStore.EXPECT().DeleteSubscription(gomock.Any()).Return(nil)
+					mockedStore.EXPECT().DeleteSubscriptionAndChannelIDMap(gomock.Any()).Return(nil)
 				}
 			}
 
@@ -680,12 +676,13 @@ func TestHandleCreateSubscriptions(t *testing.T) {
 			})
 
 			if testCase.statusCode == http.StatusOK {
-				mockedClient.EXPECT().CreateSubscription(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(&serializers.SubscriptionValue{
+				mockedClient.EXPECT().CreateSubscription(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(&serializers.SubscriptionValue{
 					ID: testutils.MockSubscriptionID,
 				}, testCase.statusCode, testCase.err)
 				mockedStore.EXPECT().GetAllProjects(testutils.MockMattermostUserID).Return(testCase.projectList, nil)
 				mockedStore.EXPECT().GetAllSubscriptions(testutils.MockMattermostUserID).Return(testCase.subscriptionList, nil)
 				mockedStore.EXPECT().StoreSubscription(testCase.subscription).Return(nil)
+				mockedStore.EXPECT().StoreSubscriptionAndChannelIDMap(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
 			}
 
 			req := httptest.NewRequest(http.MethodPost, "/subscriptions", bytes.NewBufferString(testCase.body))
@@ -839,17 +836,6 @@ func TestHandleSubscriptionNotifications(t *testing.T) {
 			isValidChannelID: true,
 			webhookSecret:    "mockWebhookSecret",
 		},
-		{
-			description: "SubscriptionNotifications: without channelID",
-			body: `{
-				"detailedMessage": {
-					"markdown": "mockMarkdown"
-					}
-				}`,
-			statusCode:       http.StatusBadRequest,
-			isValidChannelID: true,
-			webhookSecret:    "mockWebhookSecret",
-		},
 		{
 			description: "SubscriptionNotifications: eventType pull request created",
 			body: `{
@@ -1061,8 +1047,8 @@ func TestHandleSubscriptionNotifications(t *testing.T) {
 				return time.Time{}, testCase.parseTimeError
 			})
 
-			monkey.PatchInstanceMethod(reflect.TypeOf(p), "VerifyWebhookSecret", func(_ *Plugin, _ string) (int, error) {
-				return testCase.statusCode, testCase.err
+			monkey.PatchInstanceMethod(reflect.TypeOf(p), "VerifySubscriptionWebhookSecretAndGetChannelID", func(_ *Plugin, _, _ string) (string, int, error) {
+				return testCase.channelID, testCase.statusCode, testCase.err
 			})
 
 			req := httptest.NewRequest(http.MethodPost, fmt.Sprintf("%s?%s=%s&%s=%s", constants.PathSubscriptionNotifications, constants.AzureDevopsQueryParamChannelID, testCase.channelID, constants.AzureDevopsQueryParamWebhookSecret, testCase.webhookSecret), bytes.NewBufferString(testCase.body))
@@ -1138,6 +1124,7 @@ func TestHandleDeleteSubscriptions(t *testing.T) {
 				mockedClient.EXPECT().DeleteSubscription(gomock.Any(), gomock.Any(), gomock.Any()).Return(testCase.statusCode, testCase.err)
 				mockedStore.EXPECT().GetAllSubscriptions(testutils.MockMattermostUserID).Return(testCase.subscriptionList, nil)
 				mockedStore.EXPECT().DeleteSubscription(gomock.Any()).Return(nil)
+				mockedStore.EXPECT().DeleteSubscriptionAndChannelIDMap(gomock.Any()).Return(nil)
 			}
 
 			req := httptest.NewRequest(http.MethodDelete, "/subscriptions", bytes.NewBufferString(testCase.body))

server/plugin/client.go

@@ -22,7 +22,7 @@ type Client interface {
 	GetTask(organization, taskID, projectName, mattermostUserID string) (*serializers.TaskValue, int, error)
 	GetPullRequest(organization, pullRequestID, projectName, mattermostUserID string) (*serializers.PullRequest, int, error)
 	Link(body *serializers.LinkRequestPayload, mattermostUserID string) (*serializers.Project, int, error)
-	CreateSubscription(body *serializers.CreateSubscriptionRequestPayload, project *serializers.ProjectDetails, channelID, pluginURL, mattermostUserID string) (*serializers.SubscriptionValue, int, error)
+	CreateSubscription(body *serializers.CreateSubscriptionRequestPayload, project *serializers.ProjectDetails, channelID, pluginURL, mattermostUserID, uuid string) (*serializers.SubscriptionValue, int, error)
 	DeleteSubscription(organization, subscriptionID, mattermostUserID string) (int, error)
 	UpdatePipelineApprovalRequest(pipelineApproveRequestPayload *serializers.PipelineApproveRequest, organization, projectName, mattermostUserID string, approvalID int) (int, error)
 	UpdatePipelineRunApprovalRequest(pipelineApproveRequestPayload []*serializers.PipelineApproveRequest, organization, projectID, mattermostUserID string) (*serializers.PipelineRunApproveResponse, int, error)
@@ -226,15 +226,16 @@ var publisherID = map[string]string{
 	constants.SubscriptionEventRunStateChanged:                    constants.PublisherIDPipelines,
 }
 
-func (c *client) CreateSubscription(body *serializers.CreateSubscriptionRequestPayload, project *serializers.ProjectDetails, channelID, pluginURL, mattermostUserID string) (*serializers.SubscriptionValue, int, error) {
+func (c *client) CreateSubscription(body *serializers.CreateSubscriptionRequestPayload, project *serializers.ProjectDetails, channelID, pluginURL, mattermostUserID, uuid string) (*serializers.SubscriptionValue, int, error) {
 	if statusCode, err := c.plugin.SanitizeURLPaths(body.Organization, "", ""); err != nil {
 		return nil, statusCode, err
 	}
 	createSubscriptionPath := fmt.Sprintf(constants.CreateSubscription, body.Organization)
 
-	webhookSecret := url.QueryEscape(c.plugin.getConfiguration().WebhookSecret)
+	uniqueWebhookSecret := url.QueryEscape(uuid)
+
 	consumerInputs := serializers.ConsumerInputs{
-		URL: fmt.Sprintf("%s%s?%s=%s&%s=%s", strings.TrimRight(pluginURL, "/"), constants.PathSubscriptionNotifications, constants.AzureDevopsQueryParamChannelID, channelID, constants.AzureDevopsQueryParamWebhookSecret, webhookSecret),
+		URL: fmt.Sprintf("%s%s?%s=%s", strings.TrimRight(pluginURL, "/"), constants.PathSubscriptionNotifications, constants.AzureDevopsQueryParamWebhookSecret, uniqueWebhookSecret),
 	}
 
 	payload := serializers.CreateSubscriptionBodyPayload{

server/plugin/client_test.go

@@ -311,7 +311,7 @@ func TestCreateSubscription(t *testing.T) {
 				return nil, testCase.statusCode, testCase.err
 			})
 
-			_, statusCode, err := p.Client.CreateSubscription(&serializers.CreateSubscriptionRequestPayload{}, &serializers.ProjectDetails{}, testutils.MockChannelID, "mockPluginURL", testutils.MockMattermostUserID)
+			_, statusCode, err := p.Client.CreateSubscription(&serializers.CreateSubscriptionRequestPayload{}, &serializers.ProjectDetails{}, testutils.MockChannelID, "mockPluginURL", testutils.MockMattermostUserID, "mockUUID")
 
 			if testCase.err != nil {
 				assert.Error(t, err)