Fixed issue: Comment notification not working after implementing OAut…

…h2 and Added PKCE code for OAuth2 (#953) * Migrate Docs from GitBook (#948) * Migrate Docs from GitBook * Update readme.md * Update readme.md Co-authored-by: Katie Wiersgalla <[email protected]> * Update readme.md Co-authored-by: Katie Wiersgalla <[email protected]> * Update readme.md Co-authored-by: Katie Wiersgalla <[email protected]> --------- Co-authored-by: Katie Wiersgalla <[email protected]> * [MI-3153] Fixed issue: comment webhook notification not working * [MI-3153] Added PKCE along with authorization code for getting the access token * [MI-3153] Review fixes * [MI-3159] Review fixes of comments given by Kshitij * [MI-3159] Review fixes given by Ayush * [MI-3159] Removed the code which was forcing the template window to close after user is connected * [MI-3153] Review fixes * [MI-3159] Review fixes * [MI-3159] Review fix * [MI-3282] Added logic to expand issue and issue with DM notification for comment webhook notification (#58) * [MI-3282] Added logic to expand issue in comment webhook notification * [MI-3282] Fix lint errors * [MI-3315] Fixed issue: comment DM notification not working due to invalid API call * [MI-3282] Review fixes * [MI-3282] Review fix * [MI-3331] Review fixes on PR #953 (Comment notification issue) (#60) * [MI-3331] Review fixes on PR #953 (Comment notification issue) * [MI-3331] Added separate condition to check if the instance is a cloud instance or not * [MI-3335] Review fixes on Jira PR #953(Comment notification issue) (#62) 1. Added conditional for separation out the logic for jira cloud and server --------- Co-authored-by: Carrie Warner (Mattermost) <[email protected]> Co-authored-by: Katie Wiersgalla <[email protected]>
mattermost · Aug 11, 2023 · 6df75bc · 6df75bc
1 parent dc58dd5
commit 6df75bc
Show file tree

Hide file tree

Showing 13 changed files with 637 additions and 144 deletions.
diff --git a/assets/templates/oauth2/complete.html b/assets/templates/oauth2/complete.html
@@ -1,10 +1,6 @@
 <!DOCTYPE html>
 <html lang="en">
 	<head>
-		<script>
-			window.open('','_parent','');
-			window.close();
-		</script>
 		<style>
 			body {
 				color: rgb(23, 43, 77);
@@ -18,11 +14,11 @@
 			.btn {
 				-webkit-transition: all 0.15s ease;
 				-webkit-transition-delay: 0s;
+				transition-delay: 0s;
 				-moz-transition: all 0.15s ease;
 				-o-transition: all 0.15s ease;
 				transition: all 0.15s ease false;
-				padding-right: 1em;
-				padding-left: 1em;
+				padding-right: 0 1em;
 				font-size: inherit;
 				border: none;
 				height: 2.4em;
@@ -41,14 +37,14 @@
 			}
 
 			.btn-link {
-				color: #505f79;
-				background: #f4f5f7;
+				color: rgb(80, 95, 121);
+				background: rgb(244, 245, 247);
 				padding: 10px;
 			}
 
 			.btn-link:hover,
 			.btn-link:active {
-				background: #ebecf0;
+				background: rgb(235, 236, 240);
 			}
 
 			.accounts-container {

diff --git a/readme.md b/readme.md
diff --git a/server/client.go b/server/client.go
@@ -310,12 +310,12 @@ func (client JiraClient) GetSelf() (*jira.User, error) {
 // MakeCreateIssueURL makes a URL that would take a browser to a pre-filled form
 // to file a new issue in Jira.
 func MakeCreateIssueURL(instance Instance, project *jira.Project, issue *jira.Issue) string {
-	u, err := url.Parse(fmt.Sprintf("%v/secure/CreateIssueDetails!init.jspa", instance.GetJiraBaseURL()))
+	url, err := url.Parse(fmt.Sprintf("%v/secure/CreateIssueDetails!init.jspa", instance.GetJiraBaseURL()))
 	if err != nil {
 		return ""
 	}
 
-	q := u.Query()
+	q := url.Query()
 	q.Add("pid", project.ID)
 	q.Add("issuetype", issue.Fields.Type.ID)
 	q.Add("summary", issue.Fields.Summary)
@@ -344,8 +344,8 @@ func MakeCreateIssueURL(instance Instance, project *jira.Project, issue *jira.Is
 		}
 	}
 
-	u.RawQuery = q.Encode()
-	return u.String()
+	url.RawQuery = q.Encode()
+	return url.String()
 }
 
 // SearchUsersAssignableToIssue finds all users that can be assigned to an issue.

diff --git a/server/command.go b/server/command.go
@@ -784,7 +784,7 @@ func authorizedSysAdmin(p *Plugin, userID string) (bool, error) {
 func executeInstanceInstallCloud(p *Plugin, c *plugin.Context, header *model.CommandArgs, args ...string) *model.CommandResponse {
 	authorized, err := authorizedSysAdmin(p, header.UserId)
 	if err != nil {
-		return p.responsef(header, "%v", err)
+		return p.responsef(header, err.Error())
 	}
 	if !authorized {
 		return p.responsef(header, "`/jira install` can only be run by a system administrator.")
@@ -808,7 +808,7 @@ func executeInstanceInstallCloud(p *Plugin, c *plugin.Context, header *model.Com
 func executeInstanceInstallCloudOAuth(p *Plugin, c *plugin.Context, header *model.CommandArgs, args ...string) *model.CommandResponse {
 	authorized, err := authorizedSysAdmin(p, header.UserId)
 	if err != nil {
-		return p.responsef(header, "%v", err)
+		return p.responsef(header, err.Error())
 	}
 	if !authorized {
 		return p.responsef(header, "`/jira install` can only be run by a Mattermost system administrator.")
@@ -830,12 +830,14 @@ func executeInstanceInstallCloudOAuth(p *Plugin, c *plugin.Context, header *mode
 		keyConnectURL:       p.GetPluginURL() + instancePath(routeUserConnect, types.ID(jiraURL)),
 	}
 
-	err = p.oauth2Flow.ForUser(header.UserId).Start(state)
-	if err != nil {
+	if err = p.oauth2Flow.ForUser(header.UserId).Start(state); err != nil {
 		return p.responsef(header, err.Error())
 	}
 
-	channel, _ := p.client.Channel.GetDirect(header.UserId, p.conf.botUserID)
+	channel, err := p.client.Channel.GetDirect(header.UserId, p.conf.botUserID)
+	if err != nil {
+		return p.responsef(header, err.Error())
+	}
 	if channel != nil && channel.Id != header.ChannelId {
 		return p.responsef(header, "continue in the direct conversation with @jira bot.")
 	}
@@ -846,7 +848,7 @@ func executeInstanceInstallCloudOAuth(p *Plugin, c *plugin.Context, header *mode
 func executeInstanceInstallServer(p *Plugin, c *plugin.Context, header *model.CommandArgs, args ...string) *model.CommandResponse {
 	authorized, err := authorizedSysAdmin(p, header.UserId)
 	if err != nil {
-		return p.responsef(header, "%v", err)
+		return p.responsef(header, err.Error())
 	}
 	if !authorized {
 		return p.responsef(header, "`/jira install` can only be run by a system administrator.")
@@ -876,7 +878,7 @@ func executeInstanceInstallServer(p *Plugin, c *plugin.Context, header *model.Co
 func executeInstanceUninstall(p *Plugin, c *plugin.Context, header *model.CommandArgs, args ...string) *model.CommandResponse {
 	authorized, err := authorizedSysAdmin(p, header.UserId)
 	if err != nil {
-		return p.responsef(header, "%v", err)
+		return p.responsef(header, err.Error())
 	}
 	if !authorized {
 		return p.responsef(header, "`/jira uninstall` can only be run by a System Administrator.")
@@ -1138,12 +1140,14 @@ func executeSetup(p *Plugin, c *plugin.Context, header *model.CommandArgs, args
 		return p.responsef(header, "`/jira setup` can only be run by a system administrator.")
 	}
 
-	err = p.setupFlow.ForUser(header.UserId).Start(nil)
-	if err != nil {
+	if err = p.setupFlow.ForUser(header.UserId).Start(nil); err != nil {
 		return p.responsef(header, errors.Wrap(err, "Failed to start setup wizard").Error())
 	}
 
-	channel, _ := p.client.Channel.GetDirect(header.UserId, p.conf.botUserID)
+	channel, err := p.client.Channel.GetDirect(header.UserId, p.conf.botUserID)
+	if err != nil {
+		return p.responsef(header, err.Error())
+	}
 	if channel != nil && channel.Id != header.ChannelId {
 		return p.responsef(header, "continue in the direct conversation with @jira bot.")
 	}

diff --git a/server/instance_cloud_oauth.go b/server/instance_cloud_oauth.go
@@ -20,13 +20,15 @@ import (
 type cloudOAuthInstance struct {
 	*InstanceCommon
 
-	// The SiteURL may change as we go, so we store the PluginKey when as it was installed
+	// The SiteURL may change as we go, so we store the PluginKey when it was installed
 	MattermostKey string
 
 	JiraResourceID   string
 	JiraClientID     string
 	JiraClientSecret string
 	JiraBaseURL      string
+	CodeVerifier     string
+	CodeChallenge    string
 }
 
 type CloudOAuthConfigure struct {
@@ -39,22 +41,33 @@ type JiraAccessibleResources []struct {
 	ID string
 }
 
+type PKCEParams struct {
+	CodeVerifier  string
+	CodeChallenge string
+}
+
 var _ Instance = (*cloudOAuthInstance)(nil)
 
 const (
-	JiraScopes        = "read:jira-user,read:jira-work,write:jira-work"
-	JiraScopesOffline = JiraScopes + ",offline_access"
-	JiraResponseType  = "code"
-	JiraConsent       = "consent"
+	JiraScopes          = "read:jira-user,read:jira-work,write:jira-work"
+	JiraScopesOffline   = JiraScopes + ",offline_access"
+	JiraResponseType    = "code"
+	JiraConsent         = "consent"
+	PKCEByteArrayLength = 32
 )
 
-func (p *Plugin) installCloudOAuthInstance(rawURL string, clientID string, clientSecret string) (string, *cloudOAuthInstance, error) {
+func (p *Plugin) installCloudOAuthInstance(rawURL, clientID, clientSecret string) (string, *cloudOAuthInstance, error) {
 	jiraURL, err := utils.CheckJiraURL(p.GetSiteURL(), rawURL, false)
 	if err != nil {
 		return "", nil, err
 	}
 	if !utils.IsJiraCloudURL(jiraURL) {
-		return "", nil, errors.Errorf("`%s` is a Jira server URL, not a Jira Cloud", jiraURL)
+		return "", nil, errors.Errorf("`%s` is a Jira server URL instead of a Jira Cloud URL", jiraURL)
+	}
+
+	params, err := getS256PKCEParams()
+	if err != nil {
+		return "", nil, err
 	}
 
 	instance := &cloudOAuthInstance{
@@ -63,10 +76,11 @@ func (p *Plugin) installCloudOAuthInstance(rawURL string, clientID string, clien
 		JiraClientID:     clientID,
 		JiraClientSecret: clientSecret,
 		JiraBaseURL:      rawURL,
+		CodeVerifier:     params.CodeVerifier,
+		CodeChallenge:    params.CodeChallenge,
 	}
 
-	err = p.InstallInstance(instance)
-	if err != nil {
+	if err = p.InstallInstance(instance); err != nil {
 		return "", nil, err
 	}
 
@@ -76,7 +90,7 @@ func (p *Plugin) installCloudOAuthInstance(rawURL string, clientID string, clien
 func (ci *cloudOAuthInstance) GetClient(connection *Connection) (Client, error) {
 	client, _, err := ci.getClientForConnection(connection)
 	if err != nil {
-		return nil, errors.WithMessage(err, "failed to get Jira client for user "+connection.DisplayName)
+		return nil, errors.WithMessage(err, fmt.Sprintf("failed to get Jira client for the user %s", connection.DisplayName))
 	}
 	return newCloudClient(client), nil
 }
@@ -87,24 +101,23 @@ func (ci *cloudOAuthInstance) getClientForConnection(connection *Connection) (*j
 	tokenSource := oauth2Conf.TokenSource(ctx, connection.OAuth2Token)
 	client := oauth2.NewClient(ctx, tokenSource)
 
-	// Get a new token if Access Token has expired
+	// Get a new token, if Access Token has expired
 	currentToken := connection.OAuth2Token
 	updatedToken, err := tokenSource.Token()
 	if err != nil {
-		return nil, nil, errors.Wrap(err, "error getting token from token source")
+		return nil, nil, errors.Wrap(err, "error in getting token from token source")
 	}
 
 	if updatedToken.RefreshToken != currentToken.RefreshToken {
 		connection.OAuth2Token = updatedToken
 
 		// Store this new access token & refresh token to get a new access token in future when it has expired
-		err = ci.Plugin.userStore.StoreConnection(ci.Common().InstanceID, connection.MattermostUserID, connection)
-		if err != nil {
+		if err = ci.Plugin.userStore.StoreConnection(ci.Common().InstanceID, connection.MattermostUserID, connection); err != nil {
 			return nil, nil, err
 		}
 	}
 
-	// TODO Get resource ID if not in the KV Store?
+	// TODO: Get resource ID if not in the KV Store?
 	jiraID, err := ci.getJiraCloudResourceID(*client)
 	ci.JiraResourceID = jiraID
 	if err != nil {
@@ -130,6 +143,8 @@ func (ci *cloudOAuthInstance) GetUserConnectURL(mattermostUserID string) (string
 		oauth2.SetAuthURLParam("state", state),
 		oauth2.SetAuthURLParam("response_type", "code"),
 		oauth2.SetAuthURLParam("prompt", "consent"),
+		oauth2.SetAuthURLParam("code_challenge_method", "S256"),
+		oauth2.SetAuthURLParam("code_challenge", ci.CodeChallenge),
 	)
 	if err := ci.Plugin.otsStore.StoreOneTimeSecret(mattermostUserID, state); err != nil {
 		return "", nil, err
@@ -159,12 +174,10 @@ func (ci *cloudOAuthInstance) GetJiraBaseURL() string {
 }
 
 func (ci *cloudOAuthInstance) GetManageAppsURL() string {
-	// TODO
 	return fmt.Sprintf("%s/plugins/servlet/applinks/listApplicationLinks", ci.GetURL())
 }
 
 func (ci *cloudOAuthInstance) GetManageWebhooksURL() string {
-	// TODO
 	return fmt.Sprintf("%s/plugins/servlet/webhooks", ci.GetURL())
 }
 
@@ -174,29 +187,27 @@ func (ci *cloudOAuthInstance) GetMattermostKey() string {
 
 func (ci *cloudOAuthInstance) getJiraCloudResourceID(client http.Client) (string, error) {
 	request, err := http.NewRequest(
-		"GET",
+		http.MethodGet,
 		"https://api.atlassian.com/oauth/token/accessible-resources",
 		nil,
 	)
 	if err != nil {
-		return "", fmt.Errorf("failed getting request")
+		return "", fmt.Errorf("failed to get the request")
 	}
 
 	response, err := client.Do(request)
 	if err != nil {
-		return "", fmt.Errorf("failed getting accessible resources: %s", err.Error())
+		return "", fmt.Errorf("failed to get the accessible resources: %s", err.Error())
 	}
 
 	defer response.Body.Close()
 	contents, err := io.ReadAll(response.Body)
 	if err != nil {
-		return "", fmt.Errorf("failed read accesible resources response: %s", err.Error())
+		return "", fmt.Errorf("failed to read accessible resources response: %s", err.Error())
 	}
 
 	var resources JiraAccessibleResources
-	err = json.Unmarshal(contents, &resources)
-
-	if err != nil {
+	if err = json.Unmarshal(contents, &resources); err != nil {
 		return "", errors.Wrap(err, "failed to unmarshal JiraAccessibleResources")
 	}
 

diff --git a/server/instances.go b/server/instances.go
@@ -115,7 +115,7 @@ func (instances Instances) isAliasUnique(instanceID types.ID, alias string) (boo
 	return true, ""
 }
 
-// checkIfExists returns true if the specified instance id already exists
+// checkIfExists returns true if the specified instance ID already exists
 func (instances Instances) checkIfExists(instanceID types.ID) bool {
 	for _, id := range instances.IDs() {
 		if id == instanceID {

diff --git a/server/issue.go b/server/issue.go
@@ -690,32 +690,6 @@ func getPermaLink(instance Instance, postID string, currentTeam string) string {
 	return fmt.Sprintf("%v/%v/pl/%v", instance.Common().Plugin.GetSiteURL(), currentTeam, postID)
 }
 
-func (p *Plugin) getIssueDataForCloudWebhook(instance Instance, issueKey string) (*jira.Issue, error) {
-	ci, ok := instance.(*cloudInstance)
-	if !ok {
-		return nil, errors.Errorf("Must be a JIRA Cloud instance, is %s", instance.Common().Type)
-	}
-
-	jiraClient, err := ci.getClientForBot()
-	if err != nil {
-		return nil, err
-	}
-
-	issue, resp, err := jiraClient.Issue.Get(issueKey, nil)
-	if err != nil {
-		switch {
-		case resp == nil:
-			return nil, errors.WithMessage(userFriendlyJiraError(nil, err),
-				"request to Jira failed")
-
-		case resp.StatusCode == http.StatusNotFound || resp.StatusCode == http.StatusUnauthorized:
-			return nil, errors.New(`we couldn't find the issue key, or the cloud "bot" client does not have the appropriate permissions to view the issue`)
-		}
-	}
-
-	return issue, nil
-}
-
 func getIssueCustomFieldValue(issue *jira.Issue, key string) StringSet {
 	m, exists := issue.Fields.Unknowns.Value(key)
 	if !exists || m == nil {
@@ -762,6 +736,30 @@ func getIssueCustomFieldValue(issue *jira.Issue, key string) StringSet {
 	return nil
 }
 
+func (p *Plugin) getIssueDataForCloudWebhook(instance Instance, issueKey string) (*jira.Issue, error) {
+	ci, ok := instance.(*cloudInstance)
+	if !ok {
+		return nil, errors.Errorf("must be a Jira cloud instance, is %s", instance.Common().Type)
+	}
+
+	jiraClient, err := ci.getClientForBot()
+	if err != nil {
+		return nil, err
+	}
+
+	issue, resp, err := jiraClient.Issue.Get(issueKey, nil)
+	if err != nil {
+		switch {
+		case resp == nil:
+			return nil, errors.WithMessage(userFriendlyJiraError(nil, err), "request to Jira failed")
+		case resp.StatusCode == http.StatusNotFound || resp.StatusCode == http.StatusUnauthorized:
+			return nil, errors.New(`we couldn't find the issue key, or the cloud "bot" client does not have the appropriate permissions to view the issue`)
+		}
+	}
+
+	return issue, nil
+}
+
 func getIssueFieldValue(issue *jira.Issue, key string) StringSet {
 	key = strings.ToLower(key)
 	switch key {