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

raghavaggarwal2308 · 2023-10-26T10:03:32Z

Summary

While installing the oauth instance we are storing the previously stored JWT instance data inside the new oauth instance data.
While getting a client for making the API requests we are returning a JWT client in case the stored JWT instance is not nil and the user's oauth token is nil.
We have added several debug statements to keep track of how the code is working as there was no way for us to test this change properly. (Since we don't have a server with a JWT (cloud instance) setup)

What to test?

If an instance was previously connected to Jira using JWT (cloud instance) and the admin has installed a cloud-oauth instance then all the users who have not connected to oauth yet should be able to continue using the plugin.

Steps to reproduce:

Install a cloud-oauth instance on a server that already has a JWT (cloud instance) setup and certain users have connected to it.

Ticket Link

Fixes #971

* [MI-3620] Support Jira migration to Oauth: - Added logic to store the JWT instance data while installing the Oauth instance. - Added logic to fetch the client for JWT instance in case the user is not connected to Oauth. * [MI-3620] Review fixes: Updated log messages * [MI-3620] Review fixes

codecov-commenter · 2023-10-26T10:08:33Z

Codecov Report

Attention: 26 lines in your changes are missing coverage. Please review.

Comparison is base (d1e233a) 29.65% compared to head (13dcb55) 29.59%.
Report is 2 commits behind head on master.

❗ Current head 13dcb55 differs from pull request most recent head 7e27ce0. Consider uploading reports for the commit 7e27ce0 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #995      +/-   ##
==========================================
- Coverage   29.65%   29.59%   -0.06%     
==========================================
  Files          52       52              
  Lines        7794     7823      +29     
==========================================
+ Hits         2311     2315       +4     
- Misses       5287     5311      +24     
- Partials      196      197       +1

Files	Coverage Δ
server/instance_cloud_oauth.go	`0.00% <0.00%> (ø)`
server/instances.go	`31.06% <15.38%> (-2.17%)`	⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mickmister

Awesome work on this @raghavaggarwal2308 👍

In general LGTM, just some comments about variable names and avoiding panics

Another thing that comes to mind is the webhook events for subscriptions, particularly comment events. Does this PR make it so when there is a comment event and the instance is cloud-oauth, do we:

if comment author is connected, use their token to fetch the comment
else if JWTInstance != nil use the "bot" to fetch the comment

mickmister · 2023-10-26T17:13:31Z

server/instances.go

+				if jwtInstance.Common().Type == CloudInstanceType {
+					instance.(*cloudOAuthInstance).JWTInstance = jwtInstance.(*cloudInstance)
+				} else if jwtInstance.Common().Type == CloudOAuthInstanceType {
+					instance.(*cloudOAuthInstance).JWTInstance = jwtInstance.(*cloudOAuthInstance).JWTInstance


This seems a bit weird to me but also makes sense. Is the point of this line is to make sure we handle the case where we run /jira instance install cloud-oauth twice? We "carry over" the JWT instance embedded inside the existing one. This seems a bit error-prone since this is the only field we're carrying over. Are there any other fields we want to retain when "over-installing" a cloud-oauth over an existing cloud-oauth instance?

Maybe put a comment here stating this is here to make sure we carry it over when we install cloud-oauth twice

It seems like a null pointer is not possible here but still a little concerned about it. Ideally we always do the ok syntax to avoid type assertion panics

@mickmister This function "installInstance" is being called from the "installCloudOAuthInstance" function. After running the install command "installCloudOAuthInstance" is called twice during the flow:

When we run the install command.

When we submit the clientID and clientSecret modal. (To store the credentials in the instance data)

The instance is stored in the KV store after running the install command and the JWT instance data is stored inside it (Now the previous JWT instance data is overridden by the oauth instance data). When the function is called on submitting the modal, if we don't carry forward the JWT instance data then it will be lost.

When the function is called on submitting the modal, if we don't carry forward the JWT instance data then it will be lost.

Yeah when reviewing the OAuth implementation I thought it was weird how we call "install" when submitting the credential form. Being able to run /jira install on an existing instance was also introduced as a result, which also seemed off.

The fact that we have to manually add the JWTInstance here without making sure we copy everything over is a little concerning. If we add more fields to the cloud-oauth instance, we have to update this piece of code, and that seems error-prone.

Also I wonder if there are unwanted side effects to running /jira install server myurl after an existing Jira Server has been installed with that same URL. We may potentially lose information on the existing struct.

I think we can make the code more defensive by changing the "clientSecret modal" logic to be:

if existingInstance != nil { existingInstance.ClientID = clientID existingInstance.ClientSecret = clientSecret storeInstance(existingInstance) } else { newInstance := makeNewInstance() installInstance(newInstance) }

And then disallow calling installInstance with an existing instance, just as it was before the OAuth code was introduced. /jira install can just start a setup flow if an instance exists.

@raghavaggarwal2308 What do you think about my comment above?

@mickmister I agree with your suggestion. I have made the necessary changes according to the same.

Note: I have updated the condition to override the enterprise check to work only when a user is migrating from JWT to OAuth. Now if we try to run the "/jira install cloud-oauth ..." OR "/jira install server ..." command twice then we will get that message "You need a valid enterprise licence ....".

server/instance_cloud_oauth.go

server/instances.go

raghavaggarwal2308 · 2023-10-27T10:36:45Z

Another thing that comes to mind is the webhook events for subscriptions, particularly comment events. Does this PR make it so when there is a comment event and the instance is cloud-oauth, do we:

if the comment author is connected, use their token to fetch the comment

else if JWTInstance != nil use the "bot" to fetch the comment

@mickmister

If the user is connected to oauth it will use the token to fetch the comment.
If the user is not connected to oauth it will use the JWT token to fetch the comment.

In both cases the comment author needs to be connected. Do you want us to modify the logic to use the "bot" to fetch the comment in the second case?

mickmister · 2023-10-27T15:26:06Z

In both cases the comment author needs to be connected. Do you want us to modify the logic to use the "bot" to fetch the comment in the second case?

@raghavaggarwal2308 I'm thinking let's support a third case of "user is not connected to cloud-oauth or cloud, so fall back to using bot". That bot thing could stop working at any point, so it should be the last resort

mickmister · 2023-10-27T20:16:16Z

server/instance_cloud_oauth.go

 func (ci *cloudOAuthInstance) getClientForConnection(connection *Connection) (*jira.Client, *http.Client, error) {
 	oauth2Conf := ci.GetOAuthConfig()
 	ctx := context.Background()
-	tokenSource := oauth2Conf.TokenSource(ctx, connection.OAuth2Token)
-	if ci.JWTInstance != nil && tokenSource == nil {
-		ci.Plugin.API.LogDebug("Returning a JWT token client in case the stored JWT instance is not nil and the user's oauth token is nil")
+	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")


If I understand correctly, the connection variable here is ambiguous whether the user's connection is cloud or cloud-oauth. Maybe put a comment here that says "Checking if this user's connection is for a JWT instance"

Something else comes to mind. If the user disconnects their account, we should make sure we do proper cleanup of anything related to JWT instance type, in case that was their connection

Something else comes to mind. If the user disconnects their account, we should make sure we do proper cleanup of anything related to JWT instance type, in case that was their connection

@mickmister We are deleting the user's connection when a user is disconnecting. So, I think that should cover what you are suggesting here.
We cannot wipe the JWT instance data completely because even if a user has disconnected their account, there might be other users who are still using JWT.

We cannot wipe the JWT instance data completely because even if a user has disconnected their account, there might be other users who are still using JWT.

Right, I worded "we should make sure we do proper cleanup of anything related to JWT instance type" a bit misleadingly. I really meant cleaning up the connection itself, and not the centralized JWT instance data. LGTM 👍

server/instances.go

- Added fallback to use bot when comment author is not connected.

raghavaggarwal2308 · 2023-10-31T09:44:17Z

@raghavaggarwal2308 I'm thinking let's support a third case of "user is not connected to cloud-oauth or cloud, so fall back to using bot". That bot thing could stop working at any point, so it should be the last resort

@mickmister I have added the fallback. Just to avoid any confusion, the fallback will only work if there is stored JWT instance as the bot uses JWT for authentication.

hanzei · 2023-10-31T12:33:34Z

@mickmister I don't have much knowledge about the token handling in the JIRA plugin. Would you mind asking another person fro a review?

mickmister · 2023-10-31T16:41:22Z

server/instance_cloud_oauth.go

+
+	// 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")


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

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

mickmister · 2023-10-31T16:43:15Z

server/instances.go

+				if jwtInstance.Common().Type == CloudInstanceType {
+					instance.(*cloudOAuthInstance).JWTInstance = jwtInstance.(*cloudInstance)
+				} else if jwtInstance.Common().Type == CloudOAuthInstanceType {
+					instance.(*cloudOAuthInstance).JWTInstance = jwtInstance.(*cloudOAuthInstance).JWTInstance


@raghavaggarwal2308 What do you think about my comment above?

server/instances.go

server/webhook_jira.go

- Updated the code to not install the instance twice during setup. - Updated logic to override the enterprise check only when the user is migrating from JWT to OAuth.

raghavaggarwal2308 · 2023-11-01T14:03:36Z

@mickmister Fixed the review comments mentioned by you

mickmister · 2023-11-02T17:58:50Z

Hi @raghavaggarwal2308 I was going to request some changes, namely moving the backwards compatible oauthInstance.JWTInstance assignment from p.InstallInstance to p.installCloudOAuthInstance, though I just implemented them myself in the interest of time

For some reason, GitHub is not allowing me to add commits to your branch. Can you please merge the changes from https://github.com/mattermost/mattermost-plugin-jira/tree/MM-971-refactor into your branch? Please leave any comments you have on those changes on this PR. Thank you

server/instance_cloud_oauth.go

server/instances.go

raghavaggarwal2308 · 2023-11-06T04:55:39Z

@mickmister Added a few minor comments, otherwise LGTM.

server/instance_cloud_oauth.go

Co-authored-by: Michael Kochell <[email protected]>

esarafianou · 2023-11-08T11:41:37Z

server/setup_flow.go

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


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.

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

mattermost-plugin-jira/server/setup_flow.go

Line 583 in 06a02e8

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

esarafianou · 2023-11-08T11:42:32Z

server/setup_flow.go

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


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

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

esarafianou · 2023-11-08T11:42:55Z

server/instance_cloud_oauth.go

-		JiraClientID:     clientID,
-		JiraClientSecret: clientSecret,


Why are these removed?

@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

mattermost-plugin-jira/server/setup_flow.go

Lines 646 to 648 in 06a02e8

instance.JiraClientID = clientID

instance.JiraClientSecret = clientSecret

if err = p.instanceStore.StoreInstance(instance); err != nil {

raghavaggarwal2308 requested a review from mickmister as a code owner October 26, 2023 10:03

mickmister reviewed Oct 26, 2023

View reviewed changes

[MI-3676] Review fixes: Code formatting

13dcb55

mickmister reviewed Oct 27, 2023

View reviewed changes

server/instances.go Outdated Show resolved Hide resolved

mickmister added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Oct 27, 2023

mickmister requested a review from hanzei October 27, 2023 20:18

[MI-3676] Review fixes:

7e27ce0

- Added fallback to use bot when comment author is not connected.

raghavaggarwal2308 requested a review from mickmister October 31, 2023 09:44

hanzei removed their request for review October 31, 2023 12:33

mickmister reviewed Oct 31, 2023

View reviewed changes

server/instances.go Outdated Show resolved Hide resolved

server/webhook_jira.go Outdated Show resolved Hide resolved

mickmister requested review from Kshitij-Katiyar and AayushChaudhary0001 October 31, 2023 17:18

[MI-3676] Review fixes:

5614754

- Updated the code to not install the instance twice during setup. - Updated logic to override the enterprise check only when the user is migrating from JWT to OAuth.

raghavaggarwal2308 requested a review from mickmister November 1, 2023 14:03

mickmister added 3 commits November 2, 2023 13:49

move jwt backwards compatibility code to installCloudOAuthInstance

1055626

remove unnecessary verbose logging of new user connection

18f2deb

Merge branch 'master' into MM-971-refactor

cdd9d2a

raghavaggarwal2308 commented Nov 6, 2023

View reviewed changes

server/instance_cloud_oauth.go Outdated Show resolved Hide resolved

server/instances.go Show resolved Hide resolved

mickmister reviewed Nov 6, 2023

View reviewed changes

server/instance_cloud_oauth.go Outdated Show resolved Hide resolved

Review fix

cf7e02a

Co-authored-by: Michael Kochell <[email protected]>

mickmister approved these changes Nov 6, 2023

View reviewed changes

mickmister merged commit 06a02e8 into mattermost:master Nov 6, 2023
7 checks passed

mickmister mentioned this pull request Nov 6, 2023

Bump version to 4.1.0 #997

Merged

esarafianou reviewed Nov 8, 2023

View reviewed changes

avas27JTG added this to the v4.1.0 milestone Dec 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

raghavaggarwal2308 commented Oct 26, 2023 •

edited

Loading

codecov-commenter commented Oct 26, 2023 •

edited

Loading

mickmister left a comment

mickmister Oct 26, 2023

mickmister Oct 26, 2023

raghavaggarwal2308 Oct 27, 2023

mickmister Oct 27, 2023

mickmister Oct 31, 2023

raghavaggarwal2308 Nov 1, 2023

raghavaggarwal2308 commented Oct 27, 2023

mickmister commented Oct 27, 2023 •

edited

Loading

mickmister Oct 27, 2023

raghavaggarwal2308 Oct 31, 2023

mickmister Oct 31, 2023

raghavaggarwal2308 commented Oct 31, 2023

hanzei commented Oct 31, 2023

mickmister Oct 31, 2023

raghavaggarwal2308 Nov 1, 2023

mickmister Oct 31, 2023

raghavaggarwal2308 commented Nov 1, 2023

mickmister commented Nov 2, 2023

raghavaggarwal2308 commented Nov 6, 2023

esarafianou Nov 8, 2023

mickmister Nov 8, 2023 •

edited

Loading

esarafianou Nov 8, 2023

mickmister Nov 8, 2023

esarafianou Nov 8, 2023

mickmister Nov 8, 2023

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

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

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

Conversation

raghavaggarwal2308 commented Oct 26, 2023 • edited Loading

Summary

What to test?

Steps to reproduce:

Ticket Link

codecov-commenter commented Oct 26, 2023 • edited Loading

Codecov Report

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raghavaggarwal2308 commented Oct 27, 2023

mickmister commented Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raghavaggarwal2308 commented Oct 31, 2023

hanzei commented Oct 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raghavaggarwal2308 commented Nov 1, 2023

mickmister commented Nov 2, 2023

raghavaggarwal2308 commented Nov 6, 2023

Choose a reason for hiding this comment

mickmister Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raghavaggarwal2308 commented Oct 26, 2023 •

edited

Loading

codecov-commenter commented Oct 26, 2023 •

edited

Loading

mickmister commented Oct 27, 2023 •

edited

Loading

mickmister Nov 8, 2023 •

edited

Loading