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

Jira Server: Jira Server OAuth1 flow #5817

Closed
7 tasks
Dschoordsch opened this issue Dec 21, 2021 · 11 comments · Fixed by #5881
Closed
7 tasks

Jira Server: Jira Server OAuth1 flow #5817

Dschoordsch opened this issue Dec 21, 2021 · 11 comments · Fixed by #5881

Comments

@Dschoordsch
Copy link
Contributor

Dschoordsch commented Dec 21, 2021

Needs #5809

We could start making add...Auth mutations generic based on the type of service provider. Jira Server is the only one with OAuth1.0a, thus it would be different from existing ones in any case. Thus we're adding it as a separate mutation.

Integration row must be added when org/team has provider set up
Screenshot from 2021-12-13 09-18-58

OAuth1 has an additional first step to get a request token which then can be used for the OAuth flow on the client
OAuth1 flow

Flow

  • user presses connect
  • query getOAuth1RequestToken is called to get the authentication URL
  • popup with authentication URL is opened (just like OAuth2)
  • on callback from popup mutation addIntegrationToken is called with the access token

https://datatracker.ietf.org/doc/html/rfc5849

Acceptance criteria

  • new query getOAuth1RequestToken
    • input: integration provider id
    • output: authorization URL
  • modify mutation addIntegrationToken to support OAuth1 token
  • JiraServerManager which retrieves token and initializes OAuth1 encryption
    • get and post methods implemented including signatures from OAuth1
    • getRequestToken method
  • Jira Server integration row added if team/org has integration provider
  • Jira Server client manager which handles OAuth1 flow

#5818 contains UI draft and asset

@BartoszJarocki
Copy link
Contributor

@Dschoordsch any ideas how this mutation parameters would look like? It'd be great to be able to just use the one we're adding in #5805 I'm happy to sync on that if needed

@Dschoordsch
Copy link
Contributor Author

@BartoszJarocki I thought #5805 is about adding integration providers, #5809 is capturing that bit of work for JiraServer.

@BartoszJarocki
Copy link
Contributor

Oh perfect. So this one is actually authenticating users with Jira Server, I assume. If yes, maybe we can make it work with addIntegrationToken instead of creating custom mutation?

@Dschoordsch
Copy link
Contributor Author

I don't think that's beneficial since OAuth1.0a and OAuth2 are different enough to have 2 separate mutations (rather than a if/else in one mutation). We could name it more generically, but that's not important because we will probably never add another OAuth1.0a integration.

@jordanh jordanh changed the title Jira Server addJiraServerAuth mutation and UI Jira Server: Jira Server addJiraServerAuth mutation and UI Dec 28, 2021
@jordanh
Copy link
Contributor

jordanh commented Dec 28, 2021

@Dschoordsch you should be able to use the addIntegrationToken mutation instead of needing to implement addJiraServerAuth. For an oauth1 exchange, once our client gets the request token from Jira Server, you could pass that as the code param to the mutation. The addIntegrationToken mutation will call a Provider class implementation for Jira Server that knows the rest of the exchange will be oauth1, and not oauth2, and will use the request token stored in code to get the final access token (which you can then store in the IntegrationToken) table.

To make this implementation clean, all I'd suggest doing would be to add an oauth1 to the IntegrationProviderTokenTypeEnum postgres enum.

Doing this should be less work—or at least equivalent work—than needing to implement your own mutation. At the least, we should walk through it, to see if anything in our new generic authentication architecture should change...

@jordanh jordanh moved this to To Prioritize in Sprint Board Beta Dec 28, 2021
@jordanh jordanh added this to the Jira Server milestone Dec 28, 2021
@jordanh jordanh moved this from To Prioritize to Backlog in Sprint Board Beta Dec 28, 2021
@Dschoordsch
Copy link
Contributor Author

@jordanh you're right. With the current implementation in #5805 it would be adding another if to store the correct provider metadata.
I would have preferred to have 3 different methods

  • addIntegrationOAuth2Token
  • addIntegrationOAuth1Token
  • addIntegrationPAT
    since we already know on client side what we're adding and need to have different flows. However putting it all in one mutation reduces the amount of boilerplate we have to write for multiple mutations.

We need to have an additional query for OAuth1 though since it requires a request token for the client flow.

@Dschoordsch Dschoordsch changed the title Jira Server: Jira Server addJiraServerAuth mutation and UI Jira Server: Jira Server OAuth1 flow Jan 4, 2022
@Dschoordsch Dschoordsch self-assigned this Jan 17, 2022
@Dschoordsch Dschoordsch moved this from Backlog to In Progress in Sprint Board Beta Jan 17, 2022
@Dschoordsch Dschoordsch moved this from In Progress to Maintainer Review in Sprint Board Beta Feb 3, 2022
Repository owner moved this from Maintainer Review to Done in Sprint Board Beta Feb 4, 2022
@bobjac
Copy link
Contributor

bobjac commented Mar 1, 2022

@Dschoordsch - when extending the graphql schema to support Jira, I see AtlassianIntegration and associated types created in the schema.graphql file on the backend. Are you manually updating these types on both server and client? There appears to be some graphql types that you are getting directly from GitHub and GitLab.

I am looking to extend the schema to support ADO, and wanted to make sure I was working within whatever process you are using right now.

I was planning on

  1. Updating Server schema.graphql file with new types meant for ADO
  2. Creating new graphql fragments on the client and turn into generated types from a relay tools
  3. Import generated types to React components, etc.

@Dschoordsch
Copy link
Contributor Author

@bobjac You shouldn't need to modify any .graphql file manually.

  1. modifying and adding the GraphQL types on the server for ADO directly in the .ts files in packages/server/graphql directory. We're not manually writing a .graphql schema for the server.
  2. exactly, write the fragments and run yarn dev which will generate the schema files and types you need
  3. correct

@bobjac
Copy link
Contributor

bobjac commented Mar 1, 2022

@Dschoordsch Perfect. I saw all the types programmatically created, which is what I was sure to do, but got confused when I saw Atlassian types in schema.graphql. I didn't think Atlassian supported graphql natively, so I didn't think that was a pulled type from them. Thanks.

@bobjac
Copy link
Contributor

bobjac commented Mar 2, 2022

@Dschoordsch - Azure Active Directory / Azure DevOps, requires the PKCE flow for Oauth. This requires rolling your own pkce key and making an additional API call to get the auth token. I reviewed the code for the GitHub and Atlassian Managers that handle the authorization popups, and the code for the AzureDevOps Manager would need to include the PKCE flow implementation. Typically, we would recommend using msal (https://github.com/AzureAD/microsoft-authentication-library-for-js) for this. Are you OK with using this library? It would mean that Parabol would take an additional npm package as a dependency, but you wouldn't have to own and maintain identity protocol implementation in your AzureDevOpsManager implementation.

@Dschoordsch
Copy link
Contributor Author

@bobjac I'm not opposed against adding the library for the server since it's actively maintained and has no additional dependencies. For the client side I don't see a big benefit since it would increase client size and I would be afraid integrating it would be more effort than following existing patterns. Correct me if I'm wrong.

Adding PKCE to the OAuth flow client side shouldn't be much work. If I understand it correctly, adding it to Atlassian as an example, we would generate a code_challenge and pass it together with code_challenge_method here

const uri = `https://auth.atlassian.com/authorize?audience=api.atlassian.com&client_id=${

and then pass it along with the code here
AddAtlassianAuthMutation(atmosphere, {code, teamId}, {onError, onCompleted})

In addition the parameter needs to be passed on from the OAuth callback
return window.opener.postMessage({state, code}, window.location.origin)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants