Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GH-971] Support Jira migration to Oauth #995

Merged
merged 8 commits into from
Nov 6, 2023
2 changes: 1 addition & 1 deletion server/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -817,7 +817,7 @@ func executeInstanceInstallCloudOAuth(p *Plugin, c *plugin.Context, header *mode
return p.help(header)
}

jiraURL, instance, err := p.installCloudOAuthInstance(args[0], "", "")
jiraURL, instance, err := p.installCloudOAuthInstance(args[0])
if err != nil {
return p.responsef(header, err.Error())
}
Expand Down
63 changes: 51 additions & 12 deletions server/instance_cloud_oauth.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"golang.org/x/oauth2"

"github.com/mattermost/mattermost-plugin-jira/server/utils"
"github.com/mattermost/mattermost-plugin-jira/server/utils/kvstore"
"github.com/mattermost/mattermost-plugin-jira/server/utils/types"
)

Expand All @@ -29,6 +30,7 @@ type cloudOAuthInstance struct {
JiraBaseURL string
CodeVerifier string
CodeChallenge string
JWTInstance *cloudInstance
}

type CloudOAuthConfigure struct {
Expand Down Expand Up @@ -56,7 +58,7 @@ const (
PKCEByteArrayLength = 32
)

func (p *Plugin) installCloudOAuthInstance(rawURL, clientID, clientSecret string) (string, *cloudOAuthInstance, error) {
func (p *Plugin) installCloudOAuthInstance(rawURL string) (string, *cloudOAuthInstance, error) {
jiraURL, err := utils.CheckJiraURL(p.GetSiteURL(), rawURL, false)
if err != nil {
return "", nil, err
Expand All @@ -70,21 +72,51 @@ func (p *Plugin) installCloudOAuthInstance(rawURL, clientID, clientSecret string
return "", nil, err
}

instance := &cloudOAuthInstance{
InstanceCommon: newInstanceCommon(p, CloudOAuthInstanceType, types.ID(jiraURL)),
MattermostKey: p.GetPluginKey(),
JiraClientID: clientID,
JiraClientSecret: clientSecret,
Comment on lines -76 to -77

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these removed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@esarafianou This is because at the time of "installing" the cloud-oauth instance, we haven't gotten to the point of the setup where we provide the client id & client secret. That is instead in the second step of the setup process. Before this PR, we were calling installCloudOAuthInstance on that step too, though we have changed how that works by just setting the values on the saved instance rather than going through that same function call again

instance.JiraClientID = clientID
instance.JiraClientSecret = clientSecret
if err = p.instanceStore.StoreInstance(instance); err != nil {

JiraBaseURL: rawURL,
CodeVerifier: params.CodeVerifier,
CodeChallenge: params.CodeChallenge,
newInstance := &cloudOAuthInstance{
InstanceCommon: newInstanceCommon(p, CloudOAuthInstanceType, types.ID(jiraURL)),
MattermostKey: p.GetPluginKey(),
JiraBaseURL: rawURL,
CodeVerifier: params.CodeVerifier,
CodeChallenge: params.CodeChallenge,
}

if err = p.InstallInstance(instance); err != nil {
return "", nil, err
existingInstance, err := p.instanceStore.LoadInstance(types.ID(jiraURL))
if err != nil && !errors.Is(err, kvstore.ErrNotFound) {
return "", nil, errors.Wrapf(err, "failed to load existing jira instance. ID: %s", jiraURL)
}

return jiraURL, instance, err
// Handle backwards compatibility with existing JWT instance
if existingInstance != nil {
if existingInstance.Common().Type == CloudOAuthInstanceType {
oauthInstance, ok := existingInstance.(*cloudOAuthInstance)
if !ok {
return "", nil, errors.Wrapf(err, "failed to assert existing cloud-oauth instance as cloudOAuthInstance. ID: %s", jiraURL)
}

newInstance.JWTInstance = oauthInstance.JWTInstance
if newInstance.JWTInstance != nil {
p.API.LogDebug("Installing cloud-oauth over existing cloud-oauth instance. Carrying over existing saved JWT instance.")
} else {
p.API.LogDebug("Installing cloud-oauth over existing cloud-oauth instance. There exists no previous JWT instance to carry over.")
}
} else if existingInstance.Common().Type == CloudInstanceType {
jwtInstance, ok := existingInstance.(*cloudInstance)
if !ok {
return "", nil, errors.Wrapf(err, "failed to assert existing cloud instance as cloudInstance. ID: %s", jiraURL)
}

newInstance.JWTInstance = jwtInstance
p.API.LogDebug("Installing cloud-oauth over existing cloud JWT instance. Carrying over existing saved JWT instance.")
}
} else {
p.API.LogDebug("Installing new cloud-oauth instance. There exists no previous JWT instance to carry over.")
}

if err = p.InstallInstance(newInstance); err != nil {
return "", nil, errors.Wrapf(err, "failed to install cloud-oauth instance. ID: %s", jiraURL)
}

return jiraURL, newInstance, nil
}

func (ci *cloudOAuthInstance) GetClient(connection *Connection) (Client, error) {
Expand All @@ -98,6 +130,13 @@ func (ci *cloudOAuthInstance) GetClient(connection *Connection) (Client, error)
func (ci *cloudOAuthInstance) getClientForConnection(connection *Connection) (*jira.Client, *http.Client, error) {
oauth2Conf := ci.GetOAuthConfig()
ctx := context.Background()

// Checking if this user's connection is for a JWT instance
if ci.JWTInstance != nil && connection.OAuth2Token == nil {
ci.Plugin.API.LogDebug("Returning a JWT token client since the stored JWT instance is not nil and the user's oauth token is nil")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you thinking we should remove log statements like this in a following release?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mickmister Yes, we will remove these in a later release. These are just to check if the code executed as we expected it to.

return ci.JWTInstance.getClientForConnection(connection)
}

tokenSource := oauth2Conf.TokenSource(ctx, connection.OAuth2Token)
client := oauth2.NewClient(ctx, tokenSource)

Expand Down
19 changes: 11 additions & 8 deletions server/instances.go
Original file line number Diff line number Diff line change
Expand Up @@ -141,21 +141,24 @@ func (p *instancesArray) Resize(n int) {
*p = make(instancesArray, n)
}

func (p *Plugin) InstallInstance(instance Instance) error {
func (p *Plugin) InstallInstance(newInstance Instance) error {
var updated *Instances
err := UpdateInstances(p.instanceStore,
func(instances *Instances) error {
if !p.enterpriseChecker.HasEnterpriseFeatures() {
if instances != nil && len(instances.IDs()) > 0 && !instances.checkIfExists(instance.GetID()) {
return errors.Errorf(licenseErrorString)
}
if instances == nil {
return errors.New("received nil 'instances' in UpdateInstances callback")
}

if !p.enterpriseChecker.HasEnterpriseFeatures() && len(instances.IDs()) > 0 && !instances.checkIfExists(newInstance.GetID()) {
mickmister marked this conversation as resolved.
Show resolved Hide resolved
return errors.New(licenseErrorString)
}

err := p.instanceStore.StoreInstance(instance)
err := p.instanceStore.StoreInstance(newInstance)
if err != nil {
return err
return errors.Wrap(err, "failed to store new instance")
}
instances.Set(instance.Common())

instances.Set(newInstance.Common())
updated = instances
return nil
})
Expand Down
4 changes: 2 additions & 2 deletions server/kv.go
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,8 @@ func (store store) StoreConnection(instanceID, mattermostUserID types.ID, connec
return err
}

store.plugin.debugf("Stored: connection, keys:\n\t%s (%s): %+v\n\t%s (%s): %s",
keyWithInstanceID(instanceID, mattermostUserID), mattermostUserID, connection,
store.plugin.debugf("Stored: connection, keys:\n\t%s (%s): %s\n\t%s (%s): %s",
keyWithInstanceID(instanceID, mattermostUserID), mattermostUserID, connection.DisplayName,
keyWithInstanceID(instanceID, connection.JiraAccountID()), connection.JiraAccountID(), mattermostUserID)

return nil
Expand Down
17 changes: 14 additions & 3 deletions server/setup_flow.go
Original file line number Diff line number Diff line change
Expand Up @@ -590,7 +590,7 @@ func (p *Plugin) initCreateCloudOAuthInstance(f *flow.Flow, submission map[strin
jiraURL = fmt.Sprintf("https://%s.atlassian.net", jiraURL)
}

jiraURL, instance, err := p.installCloudOAuthInstance(jiraURL, "", "")
jiraURL, instance, err := p.installCloudOAuthInstance(jiraURL)
if err != nil {
return "", nil, nil, err
}
Expand Down Expand Up @@ -633,9 +633,20 @@ func (p *Plugin) submitCreateCloudOAuthInstance(f *flow.Flow, submission map[str
return "", nil, nil, errors.New("no Jira OAuth Client Secret is present in the request")
}

jiraURL, instance, err := p.installCloudOAuthInstance(jiraURL, clientID, clientSecret)
existingInstance, err := p.instanceStore.LoadInstance(types.ID(jiraURL))
if err != nil {
return "", nil, nil, err
return "", nil, nil, errors.Wrap(err, "failed to load existing cloud-oauth instance")
}

instance, ok := existingInstance.(*cloudOAuthInstance)
if !ok {
return "", nil, nil, errors.Errorf("existing instance is not a cloud-oauth instance. ID: %s", jiraURL)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the submitCreateCloudOAuthInstance assume that there's already an existing instance of cloud-oauth? Isn't the submitCreateCloudOAuthInstance what get's triggered once the admin has clicked the submit button? That should work regardless of whether there is an existing instance.

Copy link
Contributor

@mickmister mickmister Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

submitCreateCloudOAuthInstance should really be renamed submitCloudOAuthInstanceConfiguration or something similar. It's just a form that contains the client id & client secret. The instance is being initialized in the kv store here

func (p *Plugin) initCreateCloudOAuthInstance(f *flow.Flow, submission map[string]interface{}) (flow.Name, flow.State, map[string]string, error) {

}

instance.JiraClientID = clientID
instance.JiraClientSecret = clientSecret
if err = p.instanceStore.StoreInstance(instance); err != nil {
Comment on lines +646 to +648

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we storing the clientID and clientSecret of the new oauth instance to the existing one?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's not really a "new" instance here. There will always be an existing one in the kv store at this point of the setup. There are multiple stages to the setup flow and this is the second one

return "", nil, nil, errors.Wrap(err, "failed to store cloud-oauth instance")
}

return stepInstalledJiraApp, flow.State{
Expand Down
17 changes: 15 additions & 2 deletions server/webhook_jira.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,23 @@ func (jwh *JiraWebhook) expandIssue(p *Plugin, instanceID types.ID) error {
}

jwh.Issue = *issue
} else if _, ok := instance.(*cloudOAuthInstance); ok {
} else if instance, ok := instance.(*cloudOAuthInstance); ok {
mmUserID, err := p.userStore.LoadMattermostUserID(instanceID, jwh.Comment.Author.AccountID)
if err != nil {
return errors.Wrap(err, "Cannot create subscription posts for this comment as the Jira comment author is not connected to Mattermost.")
// User is not connected, so we try to fall back to JWT bot
if instance.JWTInstance == nil {
return errors.Wrap(err, "Cannot create subscription posts for this comment as the Jira comment author is not connected to Mattermost.")
}

// Fetch issue details with bot JWT bot
var issue *jira.Issue
issue, err = p.getIssueDataForCloudWebhook(instance.JWTInstance, jwh.Issue.ID)
if err != nil {
return errors.Wrap(err, "failed to getIssueDataForCloudWebhook using bot account")
}

jwh.Issue = *issue
return nil
}

conn, err := p.userStore.LoadConnection(instance.GetID(), mmUserID)
Expand Down
Loading