[RFC] Add GitLab auth #5594

jordanh · 2021-11-01T20:15:29Z

No description provided.

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

jordanh · 2021-11-12T04:53:04Z

packages/client/hooks/useMenu.ts

@@ -30,7 +30,15 @@ const useMenu = <T extends HTMLElement = HTMLButtonElement>(
  if (originCoords) {
    ;(originRef as any).current = {getBoundingClientRect: () => originCoords} as RectElement
  }
-  const {portal, closePortal, togglePortal, portalStatus, setPortalStatus, openPortal} = usePortal({
+  const {


Changes here are only to return terminatePortal so it may be used

jordanh

Self-review pass one: will make some small fixups

jordanh · 2021-11-12T04:53:54Z

packages/client/modules/demo/DemoUser.ts

@@ -8,7 +8,8 @@ export default class DemoUser {
  createdAt = new Date().toJSON()
  email: string
  featureFlags = {
-    jira: false


I presume it is ok to remove this flag of yesteryear...

packages/client/types/ClientActionVarsT.ts

packages/client/types/constEnums.ts

packages/server/graphql/intranetSchema/mutations/runScheduledJobs.ts

packages/server/graphql/types/GitHubIntegration.ts

packages/server/integrations/MakeIntegrationServerManager.ts

packages/server/postgres/migrations/1634351004998_addMattermostAuth.ts

scripts/toolboxSrc/getClientActionVars.ts

jordanh · 2021-11-13T23:48:02Z

👀 **Watch the demo & overview 👇 **

Specific requests from reviewers:

Are these tables IntegrationProvider and IntegrationToken architected well for our needs?
Are the introduced GraphQL types/query patterns roughly correct? What might need to change now to make this safe to try?
Are the introduced mutations roughly correct? What might need to change now to make this safe to try?
Is the postDeploy utility function to populate global providers safe to try?
What would you change about the GitLab auth implementation? (Or, is it safe enough to try?)
What might you change about the Mattermost auth implementation? (Or, is it safe enough to try?)
Is the IntegrationServerManager pattern in the right direction? What might need to change now?
Are the minor refactoring (e.g. separating some dataLoaders into their own files) beneficial enough to include (or should these changes be reverted)?
Anything else you notice?

BartoszJarocki · 2021-11-15T15:21:48Z

Great work @jordanh!

Regarding the database schema, I'd avoid having separate columns for things that are related only to a particular provider type. For example, when the provider type is different than OAuth we'll have all OAuth columns empty. It'd be better to keep provider data in a separate JSONB column. This way we can deserialize data into a proper type when querying and no matter what provider type we add, we won't need to change the DB schema. And we avoid having provider-specific columns in a generic table. Then, the table would look like

    CREATE TABLE IF NOT EXISTS "IntegrationProvider" (
      "id" INT GENERATED BY DEFAULT AS IDENTITY PRIMARY KEY,
      "createdAt" TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP NOT NULL,
      "updatedAt" TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP NOT NULL,
      "type" "IntegrationProviderTypesEnum" NOT NULL,
      "tokenType" "IntegrationProviderTokenTypeEnum" NOT NULL,
      "scope" "IntegrationProviderScopesEnum" NOT NULL,
      "orgId" VARCHAR(100),
      "teamId" VARCHAR(100),
      "isActive" BOOLEAN DEFAULT TRUE NOT NULL,
      "name" VARCHAR(250) NOT NULL,
      "metadata" JSONB NOT NULL DEFAULT '{}', // here we'd store all the oauth/whatever data
       ...
    );

Depending on what differences are in IntegrationToken table between providers I'd do the same there (if depending on provider type authentication is or could be different), so instead of columns, just use JSONB.

tianrunhe

Great work! I second Bartosz's suggestion and made some minor suggestions

tianrunhe · 2021-11-17T23:23:46Z

packages/server/integrations/IntegrationServerManager.ts

+      'Content-Type': 'application/json'
+    }
+
+    const uri = `${this.provider.serverBaseUri}/oauth/token?${stringify(queryParams)}`


Does most of the services have OAuth token request URI like this?

This will almost certainly need to be parameterized

tianrunhe · 2021-11-17T23:34:28Z

scripts/toolboxSrc/postDeploy.ts

  try {
    const r = await getRethink()
    await flushSocketConnections()
    await storePersistedQueries()
    await r.getPoolMaster().drain()
+
+    const pg = getPg()
+    await upsertGlobalIntegrationProvidersFromEnv()


Would there be delay between inserting the rows and app starting?

The pool is drained, so these row(s) will be updated before the app starts

tianrunhe · 2021-11-17T23:39:43Z

packages/server/graphql/mutations/addIntegrationProvider.ts

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


Seems like the subscription level should be depend on the scope.

Indeed. If/when we update the Org view to manage integration providers, this logic will need to be added here (and the subscriptions return types will need to be updated)

Dschoordsch · 2021-11-16T09:48:28Z

packages/server/graphql/types/IntegrationProvider.ts

+      description: "The provider's unique identifier",
+      resolve: ({id}) => IntegrationProviderId.join(id)
+    },
+    type: {


Do we have cases where we expect to have more than one token type per integration provider?

It's certainly possible! For Mattermost we could support Oauth2, PATs, and Webhooks if we wanted to as Mattermost supports each of these modes

Dschoordsch · 2021-11-16T10:22:25Z

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

+      "oauthClientSecret" VARCHAR(2600),
+      "createdAt" TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP NOT NULL,
+      "updatedAt" TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP NOT NULL,
+      UNIQUE("scopeGlobal", "type"),


Dschoordsch · 2021-11-16T10:25:28Z

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

+      "expiresAt" TIMESTAMP WITH TIME ZONE,
+      "oauthRefreshToken" VARCHAR(2600),
+      "oauthScopes" VARCHAR(100)[],
+      "attributes" JSONB,


Seems rather vague, if we follow Bartosz's recommendation we don't need that. Is this meant to store integration specific settings like your last github search?

Yeah, I agree with you and @BartoszJarocki, let's move these oauth* vars to the attributes column.

If an implementer wanted to, we could use attributes to store things like your last github search, although, personally I think these should likely belong on their own table. I mostly intended these abstracts to only handle auth. The fact we conflate auth and state is a bit hacky IMO.

Dschoordsch · 2021-11-16T10:59:20Z

packages/server/postgres/queries/getGitHubAuthByUserIdTeamId.ts

-// this table has a composite primary key (userId, teamId),
-// which cannot use the index with a WHERE IN or JOIN on VALUES
-// so if we want to query multiple userIds/teamIds, just call this multiple times
+// if we want to query multiple userIds/teamIds, just call this multiple times


I know this is out of scope of this RFC, but you removed the answer to 'why?' from the comment, which is the important bit. If I would look at this in a month I wouldn't know why we wouldn't just implement it for multiple userId, teamId pairs.

This comment was originally written by @mattkrick. He tells me this is no longer true (or perhaps was never true) when we spoke sync about this

Dschoordsch · 2021-11-19T15:22:58Z

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

+    CREATE INDEX IF NOT EXISTS "idx_IntegrationProvider_orgId" ON "IntegrationProvider"("orgId");
+    CREATE INDEX IF NOT EXISTS "idx_IntegrationProvider_teamId" ON "IntegrationProvider"("teamId");
+    CREATE TABLE IF NOT EXISTS "IntegrationToken" (
+      "teamId" VARCHAR(100) NOT NULL,


Some thoughts on whether or not we should have the IntegrationToken per User, Team, Provider (1, current solution) or just User, Provider and then a separate relation TeamMemberIntegration (2).

Pro

easy, it's what we have now

no joins
Con

updates to the token need to be propagated to all rows

Pro

it would simplify refreshing a token and similar operations on the token itself, because it would not require to update multiple rows.

integration specific settings could be stored on TeamMemberIntegration and thus could also be stored without having a token
Con

additional join for retrieving the token

the key User,IntegrationProvider might not work. While I think it is unlikely that someone wants to have access to Jira with 2 different accounts in 1 team, the likelyhood increases when it's spread over multiple teams. This is probably still not a use case, only wanted to mention it.

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

BartoszJarocki · 2021-12-22T17:55:44Z

Closing in favor of #5805

jordanh changed the title ~~[WIP] Add gitlab auth~~ [WIP] Add gitlab auth [base: add-mattermost] Nov 1, 2021

jordanh force-pushed the add-gitlab-auth branch from fa3dbd8 to ac9f74c Compare November 3, 2021 18:25

jordanh changed the base branch from master to add-mattermost November 6, 2021 22:03

jordanh changed the base branch from add-mattermost to master November 6, 2021 22:04

jordanh and others added 10 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

jordanh force-pushed the add-gitlab-auth branch from 6736ac2 to 298dc22 Compare November 12, 2021 04:48

jordanh changed the base branch from master to add-mattermost November 12, 2021 04:50

jordanh commented Nov 12, 2021

View reviewed changes

jordanh added 2 commits November 11, 2021 20:58

remove notifyMattermost okResponse

9352749

refactor/rename field names

b9a6fe5

jordanh commented Nov 12, 2021

View reviewed changes

jordanh changed the title ~~[WIP] Add gitlab auth [base: add-mattermost]~~ [RFC] Add gitlab auth [base: add-mattermost] Nov 13, 2021

jordanh changed the base branch from add-mattermost to master November 13, 2021 23:37

jordanh mentioned this pull request Nov 13, 2021

Design: supporting self-hosted services #5567

Closed

self-review fixups

450baf3

jordanh force-pushed the add-gitlab-auth branch from 15ca521 to 450baf3 Compare November 13, 2021 23:49

Merge branch master into add-mattermost

64c5db9

tianrunhe reviewed Nov 17, 2021

View reviewed changes

Dschoordsch reviewed Nov 19, 2021

View reviewed changes

jordanh added 5 commits December 3, 2021 15:00

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

jordanh mentioned this pull request Dec 7, 2021

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

Closed

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

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

Closed

BartoszJarocki self-assigned this Dec 7, 2021

BartoszJarocki added 6 commits December 13, 2021 08:36

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

BartoszJarocki changed the title ~~[RFC] Add gitlab auth [base: add-mattermost]~~ [RFC] Add Gitlab auth Dec 13, 2021

BartoszJarocki changed the title ~~[RFC] Add Gitlab auth~~ [RFC] Add GitLab auth Dec 13, 2021

Dschoordsch mentioned this pull request Dec 13, 2021

Jira Server Architectural Spec Written #5563

Closed

BartoszJarocki and others added 3 commits December 13, 2021 18:49

fix: mattermost panel component types

59eaa56

refactor: lowercase integration provider enums, various fixes

4e05e68

tiffanyhan mentioned this pull request Dec 16, 2021

Create abstract NotificationManager class #5748

Closed

3 tasks

Merge branch 'master' into add-gitlab-auth

130c02c

BartoszJarocki mentioned this pull request Dec 22, 2021

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

Closed

BartoszJarocki closed this Dec 22, 2021

mattkrick deleted the add-gitlab-auth branch June 3, 2022 23:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Add GitLab auth #5594

[RFC] Add GitLab auth #5594

jordanh commented Nov 1, 2021

jordanh Nov 12, 2021

jordanh left a comment

jordanh Nov 12, 2021

jordanh commented Nov 13, 2021

BartoszJarocki commented Nov 15, 2021

tianrunhe left a comment

tianrunhe Nov 17, 2021

jordanh Dec 7, 2021

tianrunhe Nov 17, 2021

jordanh Dec 7, 2021

tianrunhe Nov 17, 2021

jordanh Dec 7, 2021 •

edited

Loading

Dschoordsch Nov 16, 2021

jordanh Dec 7, 2021

Dschoordsch Nov 16, 2021

jordanh Dec 7, 2021

Dschoordsch Nov 16, 2021

jordanh Dec 7, 2021

Dschoordsch Nov 16, 2021

jordanh Dec 7, 2021 •

edited

Loading

Dschoordsch Nov 19, 2021

BartoszJarocki commented Dec 22, 2021

[RFC] Add GitLab auth #5594

[RFC] Add GitLab auth #5594

Conversation

jordanh commented Nov 1, 2021

Choose a reason for hiding this comment

jordanh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordanh commented Nov 13, 2021

BartoszJarocki commented Nov 15, 2021

tianrunhe 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

jordanh Dec 7, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordanh Dec 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BartoszJarocki commented Dec 22, 2021

jordanh Dec 7, 2021 •

edited

Loading

jordanh Dec 7, 2021 •

edited

Loading