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

[ID-1051] Add GitHub account linking #165

Closed
wants to merge 2 commits into from

Conversation

samanehsan
Copy link
Contributor

@samanehsan samanehsan commented Feb 12, 2024

Jira:

What:
Add github as a provider option for the following endpoints:

  • GET /api/oidc/v1/{provider}
  • DELETE /api/oidc/v1/{provider}
  • GET /api/oidc/v1/{provider}/authorization-url
  • POST /api/oidc/v1/{provider}/oauthcode

The only endpoint changed in this PR is the POST endpoint. If the provider is github, the createLink method for token providers will be called (not passport providers).

Why:
This PR enables a user to link their github account, which will allow them to import private workflows into their Terra workspace.

How:

  • Add a new TokenProviderService with a createLink method that handles account linking for any provider that uses access tokens.

Note: This is blocked by https://broadworkbench.atlassian.net/browse/ID-1044 (adding github to the ECM provider configuration)

Copy link

@samanehsan samanehsan changed the title [ID-1501] Add GitHub account linking [ID-1051] Add GitHub account linking Feb 13, 2024
Copy link
Contributor

@tlangs tlangs left a comment

Choose a reason for hiding this comment

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

We should try and use the Provider enum as much as we can in the codebase. Passing magic strings around is bound to lead to confusion if we keep adding providers.

}

public Optional<LinkedAccount> createLink(
String providerName,
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a Provider, not a String. That way, we can do validation on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change it to be consistent with #168. I'm not sure that we need to check if this is a valid provider here since that already happens upstream. Unless you mean validating that it is a token-based provider?

@@ -76,6 +80,7 @@ public ResponseEntity<LinkInfo> createLink(
.userId(samUser.getSubjectId())
.clientIP(request.getRemoteAddr());

Optional<LinkInfo> linkInfo;
try {
if (providerName.equals(Provider.RAS)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since providerName is an enum value, we can replace this if/else with a switch statement

@samanehsan samanehsan closed this Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants