[MM-51253][MM-51275]: Added check for full name and refactored error …

…logs. (#148) * [MI-2504][server+webapp]: Generated manifest files and Changed "Hide" to "Close" on filter popover. (#17) (#21) * [MI-2504][webapp]: Changed Hide to Close on filter popover * [MI-2504][server]: Generated manifest files * [MI-2504][server]: Updated version in manifest Co-authored-by: Abhishek Verma <[email protected]> Co-authored-by: Abhishek Verma <[email protected]> * [MI-2505]: Added logic to protect subscriptions notification webhook API and fixed Boards update subscription. (#22) * [MI-2505][server]: Added logic to protect subscriptions notification webhook API and fixed Boards update subscription. * [MI-2505][MI-2518] Fix failing testcases * [MI-2505]:Added webhook secret encoding and review fixes * [MI-2505]:Added webhook secret encryption * [MI-2505]: Fixed CI * [MI-2505]: Reverted change of auth scopes * [MI-2505]: Fixed CI * [MI-2505][MI-2603] Fixed testcases * [MI-2505]: Used constant for path * [MI-2505]: Refinded message * [MI-2505]: Minor review fixes * [MI-2505][MI-2603] Review fix Co-authored-by: Abhishek Verma <[email protected]> Co-authored-by: raghavaggarwal2308 <[email protected]> * [Fix]: Updated plugin version in manifest file * [MM-51253]: Add check for showing user's fullname on subscription details * [MM-51253]: Modified test case * [MM-51275]: modified flow * [MM-51253]: Review fix * [MM-51253]: Added check for user id * [MI-2858]: Modified test cases --------- Co-authored-by: Abhishek Verma <[email protected]> Co-authored-by: raghavaggarwal2308 <[email protected]>
mattermost-community · Mar 28, 2023 · c9399d6 · c9399d6
1 parent 51e31f9
commit c9399d6
Show file tree

Hide file tree

Showing 4 changed files with 41 additions and 41 deletions.
diff --git a/server/plugin/api.go b/server/plugin/api.go
@@ -38,8 +38,8 @@ func (p *Plugin) InitRoutes() {
 	s := p.router.PathPrefix(constants.APIPrefix).Subrouter()
 
 	// OAuth
-	s.HandleFunc(constants.PathOAuthConnect, p.OAuthConnect).Methods(http.MethodGet)
-	s.HandleFunc(constants.PathOAuthCallback, p.OAuthComplete).Methods(http.MethodGet)
+	s.HandleFunc(constants.PathOAuthConnect, p.handleAuthRequired(p.OAuthConnect)).Methods(http.MethodGet)
+	s.HandleFunc(constants.PathOAuthCallback, p.handleAuthRequired(p.OAuthComplete)).Methods(http.MethodGet)
 	// Plugin APIs
 	s.HandleFunc(constants.PathCreateTasks, p.handleAuthRequired(p.checkOAuth(p.handleCreateTask))).Methods(http.MethodPost)
 	s.HandleFunc(constants.PathLinkProject, p.handleAuthRequired(p.checkOAuth(p.handleLink))).Methods(http.MethodPost)
@@ -339,10 +339,15 @@ func (p *Plugin) handleCreateSubscription(w http.ResponseWriter, r *http.Request
 		return
 	}
 
-	createdByDisplayName := fmt.Sprintf("%s %s", user.FirstName, user.LastName)
-	if len(strings.TrimSpace(createdByDisplayName)) == 0 {
-		createdByDisplayName = user.Username // If user's first/last name doesn't exist then show username as fallback
+	createdByDisplayName := user.Username
+
+	showFullName := p.API.GetConfig().PrivacySettings.ShowFullName
+	// If "PrivacySettings.ShowFullName" is true then show the user's first/last name
+	// If the user's first/last name doesn't exist then show the username as fallback
+	if showFullName != nil && *showFullName && (user.FirstName != "" || user.LastName != "") {
+		createdByDisplayName = fmt.Sprintf("%s %s", user.FirstName, user.LastName)
 	}
+
 	if storeErr := p.Store.StoreSubscription(&serializers.SubscriptionDetails{
 		MattermostUserID: mattermostUserID,
 		ProjectName:      body.Project,

diff --git a/server/plugin/api_test.go b/server/plugin/api_test.go
@@ -662,6 +662,10 @@ func TestHandleCreateSubscriptions(t *testing.T) {
 				FirstName: "mockCreatedBy",
 			}, nil)
 
+			showFullName := true
+			privacySettings := model.PrivacySettings{ShowFullName: &showFullName}
+			mockAPI.On("GetConfig", mock.AnythingOfType("string")).Return(&model.Config{PrivacySettings: privacySettings}, nil)
+
 			monkey.Patch(json.Marshal, func(interface{}) ([]byte, error) {
 				return []byte{}, testCase.marshalError
 			})

diff --git a/server/plugin/oAuth.go b/server/plugin/oAuth.go
@@ -67,11 +67,6 @@ func (p *Plugin) GenerateOAuthConnectURL(mattermostUserID string) string {
 // OAuthConnect redirects to the OAuth authorization URL
 func (p *Plugin) OAuthConnect(w http.ResponseWriter, r *http.Request) {
 	mattermostUserID := r.Header.Get(constants.HeaderMattermostUserID)
-	// TODO: use checkAuth middleware for this
-	if mattermostUserID == "" {
-		http.Error(w, "Not authorized", http.StatusUnauthorized)
-		return
-	}
 
 	if isConnected := p.MattermostUserAlreadyConnected(mattermostUserID); isConnected {
 		p.CloseBrowserWindowWithHTTPResponse(w)
@@ -107,7 +102,8 @@ func (p *Plugin) OAuthComplete(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	if err := p.GenerateOAuthToken(code, state); err != nil {
+	mattermostUserID := r.Header.Get(constants.HeaderMattermostUserID)
+	if err := p.GenerateOAuthToken(code, state, mattermostUserID); err != nil {
 		if strings.Contains(err.Error(), "already connected") {
 			p.API.LogError(constants.UnableToCompleteOAuth, "Error", constants.ErrorMessageAzureDevopsAccountAlreadyConnected)
 			http.Error(w, err.Error(), http.StatusForbidden)
@@ -122,13 +118,14 @@ func (p *Plugin) OAuthComplete(w http.ResponseWriter, r *http.Request) {
 }
 
 // GenerateOAuthToken generates OAuth token after successful authorization
-func (p *Plugin) GenerateOAuthToken(code, state string) error {
+func (p *Plugin) GenerateOAuthToken(code, state, authenticatedMattermostUserID string) error {
 	mattermostUserID := strings.Split(state, "_")[1]
 
+	if mattermostUserID != authenticatedMattermostUserID {
+		return errors.New("failed to complete oAuth, mattermost user is not authenticated")
+	}
+
 	if err := p.Store.VerifyOAuthState(mattermostUserID, state); err != nil {
-		if _, DMErr := p.DM(mattermostUserID, constants.GenericErrorMessage, false); DMErr != nil {
-			return DMErr
-		}
 		return errors.Wrap(err, "failed to verify oAuth state")
 	}
 

diff --git a/server/plugin/oAuth_test.go b/server/plugin/oAuth_test.go
@@ -3,6 +3,7 @@ package plugin
 import (
 	"bytes"
 	"errors"
+	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
@@ -31,33 +32,25 @@ func TestOAuthConnect(t *testing.T) {
 	mockAPI := &plugintest.API{}
 	p.API = mockAPI
 	for _, testCase := range []struct {
-		description      string
-		isConnected      bool
-		mattermostUserID string
-		DMErr            error
-		statusCode       int
+		description string
+		isConnected bool
+		DMErr       error
+		statusCode  int
 	}{
 		{
-			description:      "OAuthConnect: valid",
-			mattermostUserID: testutils.MockMattermostUserID,
-			statusCode:       http.StatusFound,
+			description: "OAuthConnect: valid",
+			statusCode:  http.StatusFound,
 		},
 		{
-			description: "OAuthConnect: without mattermostUserID",
-			statusCode:  http.StatusUnauthorized,
-		},
-		{
-			description:      "OAuthConnect: user already connected",
-			isConnected:      true,
-			mattermostUserID: testutils.MockMattermostUserID,
-			statusCode:       http.StatusBadRequest,
+			description: "OAuthConnect: user already connected",
+			isConnected: true,
+			statusCode:  http.StatusBadRequest,
 		},
 		{
-			description:      "OAuthConnect: user already connected and failed to DM",
-			isConnected:      true,
-			DMErr:            &model.AppError{},
-			mattermostUserID: testutils.MockMattermostUserID,
-			statusCode:       http.StatusInternalServerError,
+			description: "OAuthConnect: user already connected and failed to DM",
+			isConnected: true,
+			DMErr:       &model.AppError{},
+			statusCode:  http.StatusInternalServerError,
 		},
 	} {
 		t.Run(testCase.description, func(t *testing.T) {
@@ -73,7 +66,6 @@ func TestOAuthConnect(t *testing.T) {
 			})
 
 			req := httptest.NewRequest(http.MethodGet, "/oauth/connect", bytes.NewBufferString(`{}`))
-			req.Header.Add(constants.HeaderMattermostUserID, testCase.mattermostUserID)
 
 			res := httptest.NewRecorder()
 
@@ -139,7 +131,7 @@ func TestOAuthComplete(t *testing.T) {
 		},
 	} {
 		t.Run(testCase.description, func(t *testing.T) {
-			monkey.PatchInstanceMethod(reflect.TypeOf(&p), "GenerateOAuthToken", func(_ *Plugin, _, _ string) error {
+			monkey.PatchInstanceMethod(reflect.TypeOf(&p), "GenerateOAuthToken", func(_ *Plugin, _, _, _ string) error {
 				return testCase.oAuthTokenErr
 			})
 			monkey.PatchInstanceMethod(reflect.TypeOf(&p), "CloseBrowserWindowWithHTTPResponse", func(_ *Plugin, _ http.ResponseWriter) {})
@@ -170,14 +162,16 @@ func TestGenerateOAuthToken(t *testing.T) {
 		description      string
 		code             string
 		state            string
+		mmuserID         string
 		verifyOAuthError error
 		expectedError    string
 		DMError          error
 	}{
 		{
 			description: "GenerateOAuthToken: valid",
 			code:        "mockCode",
-			state:       "mock_state",
+			state:       fmt.Sprintf("mockState_%s", testutils.MockMattermostUserID),
+			mmuserID:    testutils.MockMattermostUserID,
 		},
 	} {
 		t.Run(testCase.description, func(t *testing.T) {
@@ -191,10 +185,10 @@ func TestGenerateOAuthToken(t *testing.T) {
 			})
 
 			if testCase.expectedError == "" {
-				mockedStore.EXPECT().VerifyOAuthState("state", testCase.state).Return(testCase.verifyOAuthError)
+				mockedStore.EXPECT().VerifyOAuthState(testCase.mmuserID, testCase.state).Return(testCase.verifyOAuthError)
 			}
 
-			err := p.GenerateOAuthToken(testCase.code, testCase.state)
+			err := p.GenerateOAuthToken(testCase.code, testCase.state, testCase.mmuserID)
 			if testCase.expectedError != "" {
 				assert.NotNil(t, err)
 				return