Add full OAuth2 server for gitpod local control to use #4164

rl-gitpod · 2021-05-07T00:13:24Z

No description provided.

rl-gitpod · 2021-05-07T10:17:02Z

/werft run

👍 started the job as gitpod-build-rl-gplctl-oauth2-server.9

rl-gitpod · 2021-05-08T22:42:40Z

/werft run

👍 started the job as gitpod-build-rl-gplctl-oauth2-server.10

rl-gitpod · 2021-05-09T21:59:01Z

/werft run

👍 started the job as gitpod-build-rl-gplctl-oauth2-server.21

rl-gitpod · 2021-05-10T10:36:12Z

/werft run

👍 started the job as gitpod-build-rl-gplctl-oauth2-server.32

rl-gitpod · 2021-05-10T22:28:24Z

/werft run

👍 started the job as gitpod-build-rl-gplctl-oauth2-server.33

rl-gitpod · 2021-05-11T08:20:32Z

/werft run

👍 started the job as gitpod-build-rl-gplctl-oauth2-server.37

Greeneisgood

ping

…pes as required.

… created 2 entries.

AlexTugarev · 2021-05-18T12:00:25Z

components/gitpod-db/src/typeorm/auth-code-repository-db.ts

+            user,
+            client,
+            redirectUri: "",
+            codeChallenge: undefined,


I thought the plan was to use PKCE, no? This looks like it's disabled.

It is a somewhat odd approach, but the oauth lib populates it, along with the redirectUri, scopes, etc as part of its flow

That said, I've added a comment, removed the confusing params and added code in the oauth server constructor to make the PKCE-side more explicit:

const authorizationServer = new GitpodAuthorizationServer( authCodeRepository, clientRepository, tokenRepository, scopeRepository, userRepository, jwtService, { // Be explicit, communicate intent. Default is true but let's not assume that requiresPKCE: true, }

AlexTugarev · 2021-05-18T12:02:29Z

components/gitpod-db/src/typeorm/auth-code-repository-db.ts

+            }
+        });
+    }
+    public issueAuthCode(client: OAuthClient, user: OAuthUser | undefined, scopes: OAuthScope[]): OAuthAuthCode {


We used to separate services and persistence. If applied, methods like issueAuthCode would go to a service module which in turn uses the repository implementation.

AlexTugarev · 2021-05-18T12:04:05Z

components/gitpod-db/src/typeorm/auth-code-repository-db.ts

+        authCodes = authCodes.filter(te => (new Date(te.expiresAt)).getTime() > Date.now());
+        log.info(`getByIdentifier post: ${JSON.stringify(authCodes)}`);
+        const authCode = authCodes.length > 0 ? authCodes[0] : undefined;
+        return new Promise<OAuthAuthCode>((resolve, reject) => {


in async functions you don't need to return a promise, instead:

if (!authCode) { throw new Error(...); } return authCode;

Right, that was cruft - removed.

AlexTugarev · 2021-05-18T12:05:32Z

components/gitpod-db/src/typeorm/auth-code-repository-db.ts

+        const authCodeRepo = await this.getOauthAuthCodeRepo();
+        authCodeRepo.save(authCode);
+    }
+    public async isRevoked(authCodeCode: string): Promise<boolean> {


looks like unused. please remove if true.
also the implementation looks more like isExpired

Def called by lib and is an important part of the required interface.

Agreed that it looks like isExpired because that is exactly what it is doing :-)
Given we are very unlikely to want to support long-lived auth tokens I didn't want to go to the trouble of extending the interface to add something more suitable. The intention is to add a cleanup phase to remove expired auth codes at a later date.

AlexTugarev · 2021-05-18T12:07:50Z

components/gitpod-db/src/typeorm/auth-code-repository-db.ts

+        if (authCode) {
+            log.info(`revoke auth ${authCodeCode} ${JSON.stringify(authCode)}`);
+            // Set date to earliest timestamp that MySQL allows
+            authCode.expiresAt = new Date(1000);


why expiring the authCode?

auth codes are short-living anyways

that's not used

why expiring the authCode?
See answer above.

auth codes are short-living anyways
Best practice - yes, but not always and we are using a general purpose lib so it allows for that more unsafe option.

that's not used
See answer above.

AlexTugarev · 2021-05-18T12:36:10Z

components/gitpod-db/src/typeorm/user-db-impl.ts

+            dbToken = {
+                tokenHash,
+                name: `local-app`,
+                type: GitpodTokenType.MACHINE_AUTH_TOKEN,


@csweichel, what's the meaning of MACHINE_AUTH_TOKEN? this sounds like a new type.

AlexTugarev · 2021-05-18T12:38:17Z

components/gitpod-db/src/typeorm/user-db-impl.ts

+        log.info(`persist access token ${JSON.stringify(accessToken)}`);
+        var scopes: string[] = [];
+        for (const scope of accessToken.scopes) {
+            scopes = scopes.concat(scope.name);


nit: shorter: const scopes = [...accessToken.scopes];

Thanks... that fails with a type mismatch further down, but it is now the more concise:

const scopes = accessToken.scopes.map((s) => s.name);

AlexTugarev · 2021-05-18T12:49:45Z

components/server/src/oauth-server/oauth-controller.ts

+                }
+
+                // Have they authorized the local-app?
+                const wasApproved = req.query['approved'] || '';


how is the approved param related to the authorization request? is that a shortcut for the interactive part, i.e. the web flow?

Not so much a shortcut, but yes it is part of the web flow, in particular the rejection side so it returns immediately.

It is probably better to keep the explicit rejection in the db as opposed to deleting the entry. That way we can differentiate between not having asked for approval and an explicit rejection. The downside is that if they've changed their mind after initially rejecting it we have no easy way for them to do that in the UI... yet. It can be done via an explicitly formed URL but is not user-friendly.

AlexTugarev · 2021-05-18T12:51:50Z

components/server/src/oauth-server/oauth-controller.ts

+                        res.redirect(redirectTo)
+                        return;
+                    } else {
+                        log.error(`/oauth/authorize unknown client id: "${clientID}"`)


nit: looks like unreachable

Yes, using the hard-coded client id.
It should, and now does, use the requested client id which can fail.

AlexTugarev · 2021-05-18T13:02:58Z

components/server/src/oauth-server/oauth-controller.ts

+            });
+
+            router.post("/oauth/token", async (req: express.Request, res: express.Response) => {
+                log.info(`TOKEN: ${JSON.stringify(req.body)}`);


nit: it's the code at this point

Not sure I understand the comment, can you elaborate pls?

rl-gitpod · 2021-05-19T03:25:16Z

@AlexTugarev - I just realised you had reviewed the old, many-commits-for-testing PR :-(
My bad for leaving it around.

The comments still apply to the new PR as it was a merge-squash to make it a little cleaner.

rl-gitpod force-pushed the rl/gplctl-oauth2-server branch 2 times, most recently from a39a280 to 9ffc0b6 Compare May 13, 2021 23:08

Greeneisgood reviewed May 14, 2021

View reviewed changes

rl-gitpod force-pushed the rl/gplctl-oauth2-server branch from 4621c07 to 3119a38 Compare May 15, 2021 09:03

rl-gitpod added 20 commits May 16, 2021 01:24

WIP: initial exploratory implementation

72f5d1c

Add license headers

c29da42

Remove breaking code

f2a3c6e

Add license headers - again, sigh

b51e727

Add dummy data

5d604ed

Add valid redirect URI

9a943e5

Add logging, move token endpoint to correct parent

ab7ea41

Use correct grant type

34f9f71

Override auth server to add logging

1d524a4

Express.request is circular?

6b47c5b

Always allow auth code... temp

d916616

More logging

147edeb

Default grant type

553fbfd

Temp remove user check

985b633

Need fixed port

dd5cc6a

WIP: hacking away

675846d

Add headers

c413a60

Log body

0f36465

Really log body

b6b68ba

Client secret is required (but marked as optional)

77c3e9e

rl-gitpod added 12 commits May 16, 2021 01:24

Clean seperation

ef4bafc

One more dependency

0dc9f27

Use lib not src

5045a0b

Connect auth code db via inversify

1f1ab9b

oauth2 token <-> gitpod token

dcbcd26

MySQL doesn't accept Date(0)

b757f0a

Ignore expire auth codes

36edafb

Actually ignore expired auth codes

d05b86c

Really, actually ignore expired auth codes

1a2bf8b

Use random code

49cf2b5

Move OAuth endpoints to own controller. Sundry tidying up.

641b8db

Update URLs

c67755e

rl-gitpod force-pushed the rl/gplctl-oauth2-server branch from 640390d to c67755e Compare May 16, 2021 01:25

rl-gitpod added 12 commits May 16, 2021 01:37

Add license

86fa14c

OAuth controller needs binding

eb0a864

Allow new OAuth path

71da0ff

Add feature flag for OAuth server

49f4ca7

Use a Gitpod token for OAuth. Remove in-memory token repo. Extend sco…

59f83db

…pes as required.

OAuth2 server calls issueToken & issueRefresh token immediately which…

1334bb1

… created 2 entries.

Store scopes correctly

c874ef0

Rename

478c0ab

Add all scopes to repository

cb48cea

Allow multiple, potentially longer, scopes in db

9e2ba37

Rename all OAuth2 => OAuth for consistency. Added README on gitpod-db.

5ed396a

Really rename all OAuth2 server-related => OAuth

717fd7b

AlexTugarev reviewed May 18, 2021

View reviewed changes

rl-gitpod closed this May 19, 2021

AlexTugarev mentioned this pull request May 19, 2021

Initial version of OAuth server to manage access to Gitpod workspaces #4222

Merged

rl-gitpod deleted the rl/gplctl-oauth2-server branch May 24, 2021 23:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add full OAuth2 server for gitpod local control to use #4164

Add full OAuth2 server for gitpod local control to use #4164

rl-gitpod commented May 7, 2021

rl-gitpod commented May 7, 2021 •

edited by werft-gitpod-dev-com bot

Loading

rl-gitpod commented May 8, 2021 •

edited by werft-gitpod-dev-com bot

Loading

rl-gitpod commented May 9, 2021 •

edited by werft-gitpod-dev-com bot

Loading

rl-gitpod commented May 10, 2021 •

edited by werft-gitpod-dev-com bot

Loading

rl-gitpod commented May 10, 2021 •

edited by werft-gitpod-dev-com bot

Loading

rl-gitpod commented May 11, 2021 •

edited by werft-gitpod-dev-com bot

Loading

Greeneisgood left a comment

AlexTugarev May 18, 2021

rl-gitpod May 19, 2021

AlexTugarev May 19, 2021

AlexTugarev May 18, 2021

AlexTugarev May 18, 2021

rl-gitpod May 19, 2021

AlexTugarev May 18, 2021

rl-gitpod May 19, 2021

AlexTugarev May 18, 2021

rl-gitpod May 19, 2021

AlexTugarev May 18, 2021

AlexTugarev May 18, 2021

rl-gitpod May 19, 2021 •

edited

Loading

AlexTugarev May 18, 2021

rl-gitpod May 19, 2021

AlexTugarev May 18, 2021

rl-gitpod May 19, 2021

AlexTugarev May 18, 2021

rl-gitpod May 19, 2021

rl-gitpod commented May 19, 2021

Add full OAuth2 server for gitpod local control to use #4164

Add full OAuth2 server for gitpod local control to use #4164

Conversation

rl-gitpod commented May 7, 2021

rl-gitpod commented May 7, 2021 • edited by werft-gitpod-dev-com bot Loading

rl-gitpod commented May 8, 2021 • edited by werft-gitpod-dev-com bot Loading

rl-gitpod commented May 9, 2021 • edited by werft-gitpod-dev-com bot Loading

rl-gitpod commented May 10, 2021 • edited by werft-gitpod-dev-com bot Loading

rl-gitpod commented May 10, 2021 • edited by werft-gitpod-dev-com bot Loading

rl-gitpod commented May 11, 2021 • edited by werft-gitpod-dev-com bot Loading

Greeneisgood 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

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

rl-gitpod May 19, 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

rl-gitpod commented May 19, 2021

rl-gitpod commented May 7, 2021 •

edited by werft-gitpod-dev-com bot

Loading

rl-gitpod commented May 8, 2021 •

edited by werft-gitpod-dev-com bot

Loading

rl-gitpod commented May 9, 2021 •

edited by werft-gitpod-dev-com bot

Loading

rl-gitpod commented May 10, 2021 •

edited by werft-gitpod-dev-com bot

Loading

rl-gitpod commented May 10, 2021 •

edited by werft-gitpod-dev-com bot

Loading

rl-gitpod commented May 11, 2021 •

edited by werft-gitpod-dev-com bot

Loading

rl-gitpod May 19, 2021 •

edited

Loading