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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
aad0dbb
[MI-3620] Support Jira migration to Oauth: (#73)
raghavaggarwal2308 Oct 26, 2023
13dcb55
[MI-3676] Review fixes: Code formatting
raghavaggarwal2308 Oct 27, 2023
7e27ce0
[MI-3676] Review fixes:
raghavaggarwal2308 Oct 31, 2023
5614754
[MI-3676] Review fixes:
raghavaggarwal2308 Nov 1, 2023
1055626
move jwt backwards compatibility code to installCloudOAuthInstance
mickmister Nov 2, 2023
18f2deb
remove unnecessary verbose logging of new user connection
mickmister Nov 2, 2023
cdd9d2a
Merge branch 'master' into MM-971-refactor
mickmister Nov 2, 2023
cf7e02a
Review fix
raghavaggarwal2308 Nov 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions server/instance_cloud_oauth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,12 +98,12 @@ 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()
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")
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 👍

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)

// Get a new token, if Access Token has expired
Expand Down
51 changes: 28 additions & 23 deletions server/instances.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,44 +141,49 @@ 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()) {
if instances != nil && len(instances.IDs()) > 0 && !instances.checkIfExists(newInstance.GetID()) {
return errors.Errorf(licenseErrorString)
}
}

if instance.Common().Type == CloudOAuthInstanceType && len(instances.IDs()) > 0 && instances.checkIfExists(instance.GetID()) {
p.API.LogDebug("Getting the stored JWT instance data to store it inside the OAuth instance data.")
jwtInstance, err := p.instanceStore.LoadInstance(instance.GetID())
if err != nil {
p.API.LogError("Error occurred while fetching the instance", "ID", instance.GetID(), "Error", err.Error())
return err
}

// Storing the JWT instance data inside the OAuth instance data. We will use this to use the stored JWT token in case the user has not connected to OAuth yet.
p.API.LogDebug("Instance type stored in KV store", "ID", jwtInstance.GetID(), "Type", jwtInstance.Common().Type)
if jwtInstance.Common().Type == CloudInstanceType {
instance.(*cloudOAuthInstance).JWTInstance = jwtInstance.(*cloudInstance)
} else if jwtInstance.Common().Type == CloudOAuthInstanceType {
instance.(*cloudOAuthInstance).JWTInstance = jwtInstance.(*cloudOAuthInstance).JWTInstance
}

if instance.(*cloudOAuthInstance).JWTInstance != nil {
p.API.LogDebug("JWT instance is successfully stored inside cloud OAuth instance data.", "JWTInstance", instance.(*cloudOAuthInstance).JWTInstance)
if newInstance.Common().Type == CloudOAuthInstanceType && len(instances.IDs()) > 0 && instances.checkIfExists(newInstance.GetID()) {
oAuthInstance, ok := newInstance.(*cloudOAuthInstance)
if !ok {
p.API.LogError("Instance type is `cloud-oauth` but failed to assert instance.(*cloudOAuthInstance).", "ID", newInstance.GetID())
} else {
p.API.LogDebug("Failed to store JWT instance inside cloud OAuth instance data or there was no JWT instance installed previously.", "JWTInstance", instance.(*cloudOAuthInstance).JWTInstance)
p.API.LogDebug("Getting the stored JWT instance data to store it inside the OAuth instance data.")
previousInstance, err := p.instanceStore.LoadInstance(newInstance.GetID())
if err != nil {
p.API.LogError("Error occurred while fetching the instance", "ID", newInstance.GetID(), "Error", err.Error())
return err
}

// Storing the JWT instance data inside the OAuth instance data. We will use this to use the stored JWT token in case the user has not connected to OAuth yet.
p.API.LogDebug("Instance type stored in KV store", "ID", previousInstance.GetID(), "Type", previousInstance.Common().Type)
if previousInstance.Common().Type == CloudInstanceType {
oAuthInstance.JWTInstance = previousInstance.(*cloudInstance)
} else if previousInstance.Common().Type == CloudOAuthInstanceType {
oAuthInstance.JWTInstance = previousInstance.(*cloudOAuthInstance).JWTInstance
raghavaggarwal2308 marked this conversation as resolved.
Show resolved Hide resolved
}

if oAuthInstance.JWTInstance != nil {
p.API.LogDebug("JWT instance is successfully stored inside cloud OAuth instance data.", "JWTInstance", oAuthInstance.JWTInstance)
} else {
p.API.LogDebug("No JWT instance inside cloud OAuth instance data.", "JWTInstance", oAuthInstance.JWTInstance)
}
}
}

err := p.instanceStore.StoreInstance(instance)
err := p.instanceStore.StoreInstance(newInstance)
if err != nil {
return err
}
instances.Set(instance.Common())
instances.Set(newInstance.Common())
updated = instances
return nil
})
Expand Down
Loading