Added IntegrationProvider, IntegrationToken tables, GitLab auth, migrated Mattermost to the new data format #5805

BartoszJarocki · 2021-12-17T09:16:01Z

This PR adds two new tables: IntegrationProvider and IntegrationToken, migrates existing Mattermost integration to a new type, and adds a GitLab auth integration. See more information here.

Base PR for reference: #5594
Base RFC: #5567

The main goal of this PR was to remove provider-specific columns from IntegrationProvider and IntegrationToken tables, fix the TS errors, and do some code cleanup.

Leftovers

remove authorizeOAuth2 helper method, as we continue to improve our integrations the differences between GitLab/GitHub/Atlassian OAuth2 grow
Figure out a better table name for IntegrationToken, maybe UserIntegration?

@jordanh's Loom about new tables: https://www.loom.com/share/22037d51a1404fa5aab5d6ba10c98778

Self-review, my first Loom so I'm sorry for being chaotic! https://www.loom.com/share/9435223bbeeb47da977343d65d6c95bc

- TODO: add userId to MattermostAuth, query best auth for team

Before, when we did an "import someImage 'static/some.png'", file-loader would give us a path relative to the root like 'dist/[hash].png'. Now, we'll: - Use the CDN if it's configured - Serve up the assets via `static/` otherwise Also: - makeMattermostAttachments fixed to server assets from CDN

Also: - Fixed bugs in (Add|Update)IntegrationProvier mutations where we were returning the wrong error type (and sometimes the incorrect error variable!) - Added comments and TODOs to enhance AddIntegrationProvider when an oauth token is specified to work like AddIntegrationToken and, - validate that new tokens are functional by loading the appropriate ServerManager and running a method that tests that ServerManager's connection

jordanh · 2021-12-17T16:45:23Z

Align client mutations and data types to the new structure (will we ever get GraphQL input union types? 😢)

I felt this too, so hard, when I was coding this set of features

packages/server/integrations/IntegrationServerManager.ts

packages/server/postgres/migrations/1635354572171_addIntegrationProviderAndToken.ts

packages/server/postgres/migrations/1636229261909_mattermostToIntegrationsTables.ts

BartoszJarocki · 2021-12-21T18:12:16Z

packages/server/integrations/helpers/authorizeOAuth2.ts

+  if (queryParams) {
+    uri = uri.concat(`?${stringify(queryParams)}`)
+  }
+  const oauth2Response = await fetch(uri, {


Wrap in try/catch

BartoszJarocki · 2021-12-21T18:18:46Z

packages/server/integrations/helpers/authorizeOAuth2.ts

+  additonalHeaders?: Record<string, string>
+}
+
+export const authorizeOAuth2 = async ({


…er', 'tokenType' is now 'type'

BartoszJarocki · 2021-12-22T17:16:07Z

@Dschoordsch I'd really appreciate it if you could take a look, thank you!

mattkrick

I'll take a crack at these comments over winter break, i know there are a lot!

packages/client/modules/teamDashboard/components/ProviderRow/GitLabConfigMenu.tsx

packages/server/graphql/types/GitLabIntegration.ts

packages/server/graphql/types/IntegrationProvider.ts

packages/client/modules/teamDashboard/components/ProviderRow/GitLabProviderRow.tsx

packages/server/postgres/queries/src/upsertIntegrationTokenQuery.sql

packages/server/postgres/types/IntegrationTokenWithProvider.ts

packages/server/types/modules.d.ts

packages/server/utils/AtlassianServerManager.ts

packages/server/utils/MattermostServerManager.ts

packages/server/graphql/types/GitLabIntegration.ts

packages/server/postgres/migrations/1635354572171_addIntegrationProviderAndToken.ts

Dschoordsch

I'm not sure how this change relates to #5829 so I just have a few comments for now.

packages/client/modules/demo/DemoUser.ts

packages/server/postgres/migrations/1635354572171_addIntegrationProviderAndToken.ts

Dschoordsch · 2022-01-04T06:07:03Z

packages/server/postgres/migrations/1635354572171_addIntegrationProviderAndToken.ts

+      "teamId" VARCHAR(100),
+      "isActive" BOOLEAN DEFAULT TRUE NOT NULL,
+      "name" VARCHAR(250) NOT NULL,
+      "providerMetadata" JSONB NOT NULL DEFAULT '{}',


+1 why not call it just "metadata"?

i think that was chosen to differentiate between tokenMetadata & providerMetadata (it gets confusing when you're in the thick of it, especially because the types weren't originally very strict).

IMO i think we should just use columns, then we get types for free!

Dschoordsch · 2022-01-04T09:38:17Z

packages/server/graphql/mutations/addIntegrationToken.ts

+  return null
+}
+
+const addIntegrationToken = {


+1 maybe name it addUserIntegration after its purpose

i agree token referring to an entire DB row feels bad.
technically, it isn't related to the User, but the TeamMember.
maybe IntegrationTeamMemberAuth? IntegrationAuth? IntegrationAuthStrategy?

packages/server/postgres/queries/src/getIntegrationProvidersQuery.sql

Dschoordsch · 2022-01-04T10:06:32Z

packages/client/modules/teamDashboard/components/ProviderRow/GitLabProviderRow.tsx

+                name
+                updatedAt
+                providerMetadata {
+                  ... on OAuth2ProviderMetadata {


Going forward with this, does that mean if we have a integration provider which supports OAuth2 and PAT we would split it into 2 different providers?

packages/client/modules/teamDashboard/components/ProviderRow/MattermostProviderRow.tsx

Dschoordsch · 2022-01-04T10:47:27Z

packages/client/mutations/AddIntegrationTokenMutation.ts

+    $providerId: ID!
+    $oauthCodeOrPat: ID!
+    $teamId: ID!
+    $redirectUri: URL!


I don't see why we can't trust the redirect URI here? If we pass a wrong one, then adding the token will fail. It's not used for anything else. Since the client does the initial redirect itself, it should also know which redirect URI was used. The OAuth provider should also be set up to only allow the correct redirect URIs.

packages/client/mutations/AddIntegrationTokenMutation.ts

packages/client/shared/gqlIds/GitLabIntegrationId.ts

mattkrick · 2022-01-06T16:29:44Z

packages/server/graphql/mutations/addIntegrationToken.ts

+    })
+
+    const data = {userId: viewerId, teamId}
+    publish(SubscriptionChannel.TEAM, teamId, 'AddIntegrationToken', data, subOptions)


NOTIFICATION channel, not team. team doesn't get to know about 1 persons token

BartoszJarocki · 2022-01-10T07:28:42Z

closing in favor of #5829

jordanh and others added 28 commits November 8, 2021 21:00

initial commit, framing in mattermost

0dc385e

feature complete

99e4b43

self-review fixups

f91cffb

review changes

e8ba78c

remove autofocus

fb0bfd5

Majority of Maintainer review fixups

e575a8b

- TODO: add userId to MattermostAuth, query best auth for team

refactor: add userId to MattermostAuth

40f8fec

better scheduled jobs types

49b363c

address @mattkrick 2nd review comments

1577aa6

Refactor Mattermost, add GitLab w/new auth scheme

298dc22

remove notifyMattermost okResponse

9352749

refactor/rename field names

b9a6fe5

self-review fixups

450baf3

Merge branch master into add-mattermost

64c5db9

Merge branch 'master' into add-mattermost

a75833f

maintainer review round 3 fixups

adc7cef

fixed small nits

841afb8

Merge branch 'add-mattermost' into add-gitlab-auth

cedec5f

Merge branch 'master' into add-gitlab-auth

e47d6ba

fix: removed not needed mattermost queries

55a8ba1

fix: removed not needed mattermost queries

5fbfeae

fix: added integration auth loaders to CustomLoaderMakers

627a11c

fix: mattermost provider row missing variables

74bad44

fix: various merge fixes, type improvements

94ff90a

fix: mattermost panel component types

59eaa56

refactor: lowercase integration provider enums, various fixes

4e05e68

jordanh reviewed Dec 17, 2021

View reviewed changes

packages/server/integrations/IntegrationServerManager.ts Outdated Show resolved Hide resolved

BartoszJarocki added 3 commits December 21, 2021 08:35

refactor: code cleanup, more comments

18f2714

code cleanup

e26e7bd

fix: mattermost to integration token migration

7f99e88

BartoszJarocki mentioned this pull request Dec 21, 2021

Jira Server: Jira Server OAuth1 flow #5817

Closed

7 tasks

BartoszJarocki commented Dec 21, 2021

View reviewed changes

mattkrick changed the base branch from add-gitlab-auth to master December 21, 2021 18:36

fix: mattermost to integration providers table migration

c0813b7

BartoszJarocki force-pushed the feat/integration-providers branch from 8e8d5e1 to c0813b7 Compare December 22, 2021 14:22

BartoszJarocki added 2 commits December 22, 2021 17:26

refactor: integration providers columns rename: 'type' is now 'provid…

d27b81c

…er', 'tokenType' is now 'type'

fix: removed not needed noFallthroughCasesInSwitch from tsconfig

ce69790

BartoszJarocki linked an issue Dec 22, 2021 that may be closed by this pull request

GitLab auth shipped behind feature flag, Mattermost to new Auth tables #5750

Closed

BartoszJarocki marked this pull request as ready for review December 22, 2021 16:54

BartoszJarocki changed the title ~~Changed integration providers schema [base: add-gitlab-auth]~~ Added IntegrationProvider, IntegrationToken tables, GitLab auth, migrated Mattermost to the new data format Dec 22, 2021

BartoszJarocki requested a review from Dschoordsch December 22, 2021 17:15

BartoszJarocki mentioned this pull request Dec 22, 2021

[RFC] Add GitLab auth #5594

Closed

mattkrick requested changes Dec 23, 2021

View reviewed changes

tiffanyhan mentioned this pull request Dec 31, 2021

Create abstract NotificationManager class #5748

Closed

3 tasks

mattkrick reviewed Jan 3, 2022

View reviewed changes

packages/server/graphql/types/GitLabIntegration.ts Show resolved Hide resolved

mattkrick reviewed Jan 3, 2022

View reviewed changes

packages/server/graphql/types/GitLabIntegration.ts Show resolved Hide resolved

mattkrick reviewed Jan 3, 2022

View reviewed changes

packages/server/postgres/migrations/1635354572171_addIntegrationProviderAndToken.ts Show resolved Hide resolved

tiffanyhan mentioned this pull request Jan 4, 2022

Implement Create abstract NotificationManager class #5828

Closed

1 task

mattkrick mentioned this pull request Jan 4, 2022

Add GitLab #5829

Merged

9 tasks

Dschoordsch reviewed Jan 4, 2022

View reviewed changes

mattkrick reviewed Jan 6, 2022

View reviewed changes

BartoszJarocki closed this Jan 10, 2022

mattkrick deleted the feat/integration-providers branch June 3, 2022 23:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added IntegrationProvider, IntegrationToken tables, GitLab auth, migrated Mattermost to the new data format #5805

Added IntegrationProvider, IntegrationToken tables, GitLab auth, migrated Mattermost to the new data format #5805

BartoszJarocki commented Dec 17, 2021 •

edited

Loading

jordanh commented Dec 17, 2021

BartoszJarocki Dec 21, 2021

BartoszJarocki Dec 21, 2021

BartoszJarocki commented Dec 22, 2021

mattkrick left a comment

Dschoordsch left a comment

Dschoordsch Jan 4, 2022

mattkrick Jan 7, 2022

Dschoordsch Jan 4, 2022

mattkrick Jan 7, 2022

Dschoordsch Jan 4, 2022

Dschoordsch Jan 4, 2022

mattkrick Jan 6, 2022

BartoszJarocki commented Jan 10, 2022

Added IntegrationProvider, IntegrationToken tables, GitLab auth, migrated Mattermost to the new data format #5805

Added IntegrationProvider, IntegrationToken tables, GitLab auth, migrated Mattermost to the new data format #5805

Conversation

BartoszJarocki commented Dec 17, 2021 • edited Loading

jordanh commented Dec 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BartoszJarocki commented Dec 22, 2021

mattkrick left a comment

Choose a reason for hiding this comment

Dschoordsch 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

Choose a reason for hiding this comment

BartoszJarocki commented Jan 10, 2022

BartoszJarocki commented Dec 17, 2021 •

edited

Loading