mattermost · mickmister · Aug 11, 2023 · Jun 6, 2023 · Jun 9, 2023 · Jun 9, 2023
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 {

diff --git a/server/kv.go b/server/kv.go
@@ -470,40 +470,35 @@ func (store *store) LoadInstance(instanceID types.ID) (Instance, error) {
 
 func (store *store) LoadInstanceFullKey(fullkey string) (Instance, error) {
 	var data []byte
-	err := store.plugin.client.KV.Get(fullkey, &data)
-	if err != nil {
+	if err := store.plugin.client.KV.Get(fullkey, &data); err != nil {
 		return nil, err
 	}
 	if data == nil {
 		return nil, errors.Wrap(kvstore.ErrNotFound, fullkey)
 	}
 
 	si := serverInstance{}
-	err = json.Unmarshal(data, &si)
-	if err != nil {
+	if err := json.Unmarshal(data, &si); err != nil {
 		return nil, err
 	}
 	switch si.Type {
 	case CloudInstanceType:
 		ci := cloudInstance{}
-		err = json.Unmarshal(data, &ci)
-		if err != nil {
-			return nil, errors.WithMessage(err, "failed to unmarshal stored Instance "+fullkey)
+		if err := json.Unmarshal(data, &ci); err != nil {
+			return nil, errors.WithMessage(err, fmt.Sprintf("failed to unmarshal stored instance %s", fullkey))
 		}
 		if len(ci.RawAtlassianSecurityContext) > 0 {
-			err = json.Unmarshal([]byte(ci.RawAtlassianSecurityContext), &ci.AtlassianSecurityContext)
-			if err != nil {
-				return nil, errors.WithMessage(err, "failed to unmarshal stored Instance "+fullkey)
+			if err := json.Unmarshal([]byte(ci.RawAtlassianSecurityContext), &ci.AtlassianSecurityContext); err != nil {
+				return nil, errors.WithMessage(err, fmt.Sprintf("failed to unmarshal stored instance %s", fullkey))
 			}
 		}
 		ci.Plugin = store.plugin
 		return &ci, nil
 
 	case CloudOAuthInstanceType:
 		ci := cloudOAuthInstance{}
-		err = json.Unmarshal(data, &ci)
-		if err != nil {
-			return nil, errors.WithMessage(err, "failed to unmarshal stored Instance "+fullkey)
+		if err := json.Unmarshal(data, &ci); err != nil {
+			return nil, errors.WithMessage(err, fmt.Sprintf("failed to unmarshal stored instance %s", fullkey))
 		}
 		ci.Plugin = store.plugin
 		return &ci, nil