feat: add OAuth2 applications #11197

code-asher · 2023-12-13T23:17:16Z

This is part 1 of #11084.

It adds the oauth application configuration part. The actual token exchange is not implemented yet, probably will use golang/oauth2. As a consequence I have not linked the new page in the sidebar, but it can be accessed by going to /deployment/oauth2-apps. The endpoints are restricted to dev.

This is big enough that I felt like it made sense to get a review on it now even though the token exchange is missing. It is just a bunch of CRUD, so mostly tedious work like queries and a frontend for those queries.

Two areas in particular I am not sure about:

~~Did I do the RBAC part right?~~
I am not sure I am using features/entitlements correctly. ~~I am not actually preventing adding/editing applications but maybe I should? I was going to prevent the token exchange though.~~ I am doing this now.

Relying on backend validation right now but maybe should do it on the frontend since the backend errors are a bit hard to read. Also could add that emoji picker to the icon field.

These are applications that will be able to use OAuth2 to get an API key from Coder.

These let you add, update, and remove OAuth2 applications.

code-asher · 2023-12-14T03:23:47Z

Not sure about the race tests...will look into it later.

Emyrk · 2023-12-14T16:04:02Z

Github allows any user to make an Oauth2 app. Do we expect this to be the case for Coder?

Do we want Coder to not allow personal Oauth2 apps, and only allow global oauth apps?

Should apps be scoped to an org?

Emyrk · 2023-12-14T16:07:06Z

coderd/database/dbauthz/dbauthz.go

+func (q *querier) InsertOAuth2AppSecret(ctx context.Context, arg database.InsertOAuth2AppSecretParams) (database.OAuth2AppSecret, error) {
+	if err := q.authorizeContext(ctx, rbac.ActionCreate, rbac.ResourceOAuth2AppSecret); err != nil {
+		return database.OAuth2AppSecret{}, err
+	}
+	return q.db.InsertOAuth2AppSecret(ctx, arg)
+}


We should incorporate this in the encrypted db layer.

Parkreiner

TypeScript looks good! There's a lot of files, though, so I'll do a second review pass later today to check the stories and pull the repo down to kick the tires a bit

docs/api/enterprise.md

site/src/api/queries/oauth2.ts

site/src/pages/DeploySettingsPage/OAuth2AppsSettingsPage/CreateOAuth2AppPage.tsx

site/src/pages/DeploySettingsPage/OAuth2AppsSettingsPage/EditOAuth2AppPageView.tsx

Parkreiner · 2023-12-14T17:00:39Z

site/src/pages/DeploySettingsPage/OAuth2AppsSettingsPage/EditOAuth2AppPageView.tsx

+          </TableHead>
+          <TableBody>
+            {isLoadingSecrets && <TableLoader />}
+            {!isLoadingSecrets && (!secrets || secrets?.length === 0) && (


Could the second case be simplified down to !secrets?.length? I think that would catch both the undefined and 0 cases

It definitely could, for some reason I like to always compare length to a number rather than make use of falsey checks.

Honestly, that's generally what I default to, too – I guess my main concern here was making sure the condition isn't too long, since there's not an easy way to name it without moving the value all the way above the return statement

site/src/pages/DeploySettingsPage/OAuth2AppsSettingsPage/EditOAuth2AppPageView.tsx

Parkreiner · 2023-12-14T17:17:46Z

site/src/pages/DeploySettingsPage/OAuth2AppsSettingsPage/OAuth2AppForm.tsx

+        event.preventDefault();
+        const formData = new FormData(event.target as HTMLFormElement);
+        onSubmit({
+          name: formData.get("name") as string,


I get that the type assertions are necessary to exclude the File type, but do you think we should still have null checks?

Honestly I should probably use Formik or whatever, and add the validations too. I just...dislike Formik haha

I figure null will be caught by validation, either on the frontend (if I add it) or the backend. But from a code complete perspective it does seem like adding null here is the right move.

Since the page is blocked from being used outside of development for now I am going to punt frontend validation for another PR, if that seems chill.

site/src/pages/DeploySettingsPage/OAuth2AppsSettingsPage/OAuth2AppsSettingsPageView.stories.tsx

code-asher · 2023-12-14T19:34:03Z

Github allows any user to make an Oauth2 app. Do we expect this to be the case for Coder?
Do we want Coder to not allow personal Oauth2 apps, and only allow global oauth apps?
Should apps be scoped to an org?

I asked and looks like we should be scoping them to an org for the future, and right now owner-only is fine (so I think we should leave the possibility this will change).

Also remove a ? since that condition would short-circuit if `secrets` was not set. We could just do a falsey check, but I like to compare length to a number.

I think this is right?

This is to make sure it is not mistaken as the actual secret.

Moved them to the queries directory and they all invalidate now.

Forgot to do this after adding a new comment on the apps table.

The endpoints are disabled until we can implement the token exchange.

Parkreiner · 2023-12-18T15:27:19Z

site/src/AppRouter.tsx

@@ -315,6 +333,15 @@ export const AppRouter: FC = () => {
                  path="external-auth"
                  element={<ExternalAuthSettingsPage />}
                />
+
+                <Route path="oauth2-provider">
+                  <Route path="apps">


One thing I noticed during testing: I think there should be a redirect or something when the user tries to go go the deployment/oauth2-provider/ URL path

Right now, it doesn't take you to a completely empty screen, but you only see the navbar and the Deployment options

I am hesitant to do that because one day we might have something else on that page (like for example a toggle for whether to enable the oauth2 provider) and I would not want folks getting confused learning they can go to just /oauth2-provider to get to the apps, if that makes sense.

That said, seems like it should at least show a proper 404. Mine gives this error:

"user" must be an existing uuid or username. queried user="deployment"

So I guess it is falling through to some other page.

Whoops I was on dev.coder.com, yeah you are right it shows a blank page with the deployment sidebar. That is pretty weird. Is there even a way for me to show a 404 here? Maybe a redirect is the only fix...

I added a route with NotFoundPage.

Ah but that is weird too because the sidebar is still there. At least the page says "not found" I guess.

Emyrk · 2023-12-18T15:29:08Z

I'll get a green checkmark on this today.

We should get this in soon to prevent merge conflicts on the moving base.

site/src/pages/DeploySettingsPage/OAuth2AppsSettingsPage/OAuth2AppsSettingsPageView.tsx

site/src/pages/DeploySettingsPage/OAuth2AppsSettingsPage/EditOAuth2AppPageView.tsx

Parkreiner

The UI worked really well when I was testing things out. Just had a few more comments

site/src/pages/DeploySettingsPage/OAuth2AppsSettingsPage/EditOAuth2AppPageView.tsx

This will work better on smaller screens. Also I had an extra useless Stack around the button.

- Make it a header - Add a copy icon

One day we might have other settings on this page but for now there is nothing to show.

This should not normally be possible because the modal would prevent hitting the delete button but you never know.

Emyrk

Lets get this in.

github-actions bot assigned code-asher Dec 13, 2023

code-asher force-pushed the asher/oauth-provider branch 9 times, most recently from 0706a37 to 2bdd363 Compare December 14, 2023 02:17

code-asher added 2 commits December 13, 2023 17:31

Add database tables for OAuth2 applications

c7d69e2

These are applications that will be able to use OAuth2 to get an API key from Coder.

Add endpoints for managing OAuth2 applications

3c096b2

These let you add, update, and remove OAuth2 applications.

code-asher force-pushed the asher/oauth-provider branch 2 times, most recently from d865c7f to 1ff12c8 Compare December 14, 2023 02:55

Add frontend for managing OAuth2 applications

430140e

code-asher force-pushed the asher/oauth-provider branch from 1ff12c8 to 430140e Compare December 14, 2023 02:59

code-asher marked this pull request as ready for review December 14, 2023 03:22

code-asher requested review from Emyrk, a team and Parkreiner and removed request for a team December 14, 2023 03:22

Emyrk reviewed Dec 14, 2023

View reviewed changes

Parkreiner reviewed Dec 14, 2023

View reviewed changes

code-asher added 4 commits December 14, 2023 10:43

Add types to oauth2 stories

35978ed

Remove redundant existence checks

7e2db88

Also remove a ? since that condition would short-circuit if `secrets` was not set. We could just do a falsey check, but I like to compare length to a number.

Remove unnecessary function wrappers

f224ebc

Add types to default meta storybook exports

c57b400

I think this is right?

code-asher added 11 commits December 14, 2023 14:28

Rename secret ID param

8035333

This is to make sure it is not mistaken as the actual secret.

Move db to sdk conversions to db2sdk.go

fd13c02

Be more helpful with app and secret mismatches

64ef328

Clarify new secret vs secret list

7c7fd7e

Remove invalid JSON from test entities

765ee0c

Remove redundant type

1e08e4d

Refactor mutations

3e69495

Moved them to the queries directory and they all invalidate now.

Merge remote-tracking branch 'github/main' into asher/oauth-provider

41ff886

Bump migration number

6e2ec5a

make gen

7c3b1b1

Forgot to do this after adding a new comment on the apps table.

Add dev and entitlement checks to oauth2 provider

7e7cda5

The endpoints are disabled until we can implement the token exchange.

code-asher requested a review from Emyrk December 15, 2023 22:14

Parkreiner reviewed Dec 18, 2023

View reviewed changes

site/src/pages/DeploySettingsPage/OAuth2AppsSettingsPage/OAuth2AppsSettingsPageView.tsx Outdated Show resolved Hide resolved

Parkreiner reviewed Dec 18, 2023

View reviewed changes

site/src/pages/DeploySettingsPage/OAuth2AppsSettingsPage/EditOAuth2AppPageView.tsx Show resolved Hide resolved

Parkreiner reviewed Dec 18, 2023

View reviewed changes

site/src/pages/DeploySettingsPage/OAuth2AppsSettingsPage/EditOAuth2AppPageView.tsx Outdated Show resolved Hide resolved

site/src/pages/DeploySettingsPage/OAuth2AppsSettingsPage/EditOAuth2AppPageView.tsx Outdated Show resolved Hide resolved

code-asher added 6 commits December 18, 2023 11:19

Move badges underneath header

1be8f15

This will work better on smaller screens. Also I had an extra useless Stack around the button.

Improve client ID markup

511850b

- Make it a header - Add a copy icon

Show a 404 page on /deployment/oauth2-provider/

e8e8a94

One day we might have other settings on this page but for now there is nothing to show.

Clear full new secret if deleted

c2934fb

This should not normally be possible because the modal would prevent hitting the delete button but you never know.

Merge remote-tracking branch 'github/main' into asher/oauth-provider

b9a6765

Bump migration number

d8927ca

Emyrk approved these changes Dec 20, 2023

View reviewed changes

code-asher enabled auto-merge (squash) December 21, 2023 21:20

Fix undefined error in edit oauth2 stories

bd8cab4

code-asher force-pushed the asher/oauth-provider branch from 665808e to bd8cab4 Compare December 21, 2023 21:21

code-asher merged commit 5cfa34b into main Dec 21, 2023

code-asher deleted the asher/oauth-provider branch December 21, 2023 21:38

github-actions bot locked and limited conversation to collaborators Dec 21, 2023

feat: add OAuth2 applications #11197

feat: add OAuth2 applications #11197

Uh oh!

Conversation

code-asher commented Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

code-asher commented Dec 14, 2023

Uh oh!

Emyrk commented Dec 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Parkreiner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

code-asher commented Dec 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Emyrk commented Dec 18, 2023

Uh oh!

Uh oh!

Uh oh!

Parkreiner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Emyrk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

code-asher commented Dec 13, 2023 •

edited

Loading

code-asher commented Dec 14, 2023 •

edited

Loading