-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Initial version of OAuth server to manage access to Gitpod workspaces #4222
Conversation
What does this PR do?
What this PR does not do?
TestingInstalling local app
|
/werft run 👍 started the job as gitpod-build-rl-gplctl-oauth-server.2 |
/werft run 👍 started the job as gitpod-build-rl-gplctl-oauth-server.3 |
@gtsiolis can you help with the design of this page? |
@svenefftinge Yes! Although not needed, how could I see this page in aciton? I'd like to also see the interaction for this (if the tab is closing, etc). I tried following the steps in #4222 (comment) but only see the following then opening port |
You need to download the local-app in that screen and keep on with the directions. |
@gtsiolis https://www.loom.com/share/9442ca1e2ab6408c93ce8c3390cf20e4 Note that the gitpod.io page is only there to keep chrome on the screen during the recording, it plays no part. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the generous display of logging statements the code LGTM. I've gone through the testing instructions and things work as advertised. Pending @gtsiolis's comments this could land in main I reckon :)
Thanks for clarifying @rl-gitpod! Here're two variations we could use in case one of these two makes more sense, following two relevant existing designs in the product. Feel free to adjust text per need. A couple of notes or things to consider:
See also relevant specs and relevant existing implementations in #3704 and #3693. |
yarn.lock
Outdated
version "5.0.1" | ||
resolved "https://registry.yarnpkg.com/vscode-jsonrpc/-/vscode-jsonrpc-5.0.1.tgz#9bab9c330d89f43fc8c1e8702b5c36e058a01794" | ||
integrity sha512-JvONPptw3GAQGXlVV2utDcHx0BiY34FupW/kI6mZ5x06ER5DdPG/tXWMVHjTNULF5uKPOUUD0SaXg5QaubJL0A== | ||
|
||
[email protected]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
surprising!
public issueAuthCode(client: OAuthClient, user: OAuthUser | undefined, scopes: OAuthScope[]): OAuthAuthCode { | ||
const code = crypto.randomBytes(30).toString('hex'); | ||
log.info(`issueAuthCode: ${JSON.stringify(client)}, ${JSON.stringify(user)}, ${JSON.stringify(scopes)}, ${code}`); | ||
// NOTE: caller (@jmondi/oauth2-server) is responsible for adding the remaining items, PKCE params, redirect URL, etc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given, that this is called by the oauth2 server framework, I'd suggest to extract and move it to the server component. it seems odd to implement the missing bits for the framework within gitpod-db
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That requires moving the entire AuthCodeRepositoryDB (OAuthAuthCodeRepository) implementation to server and since it requires storage, then adding a db service + implementation for it to use.
I agree it meets the current, very structured implementation approach for most parts of the system that interact with the db and will take a look at it tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That requires moving the entire AuthCodeRepositoryDB (OAuthAuthCodeRepository) implementation to server
no it doesn't.
this pattern is used in may cases here. the DB
implementation lives in gitpod-db
and is injected into services from server
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
odd to implement the missing bits for the framework within gitpod-db.
Not so much a missing bit but the OAuthAuthCodeRepository interface needs to be implemented for the framework to use. Moving 1 requires moving the rest (yes - there are other, less readable ways to do so).
this pattern is used in may cases here. the
DB
implementation lives ingitpod-db
and is injected into services fromserver
.
Agreed. That's what I was referring to with "then adding a db service + implementation for it to use"
components/server/src/oauth-server/oauth-authorization-server.ts
Outdated
Show resolved
Hide resolved
Thanks @gtsiolis!
What we are doing here is the same as the 'Consent form' for any/all OAuth implementations e.g. here or here Ultimately we will want to itemise the types of access (scopes) they are approving, but we don't have a good sense of what they should be at this point so we are asking for a general access approval.
It is for all workspaces atm. That will likely change in the future, but not for the first iteration and possibly more.
The 'Consent form' in other systems tends to be modal. It is also simpler to implement :-) If it is not modal, having the ability to jump to the dashboard/account/etc without completing an action would need to be handled. If we assume that it doesn't mean either yes or no (and therefore we don't need to update the db to reflect that) it could just be a timeout i.e. easy.
If we have a decision on A vs B, I'll take a look tomorrow. |
/werft run 👍 started the job as gitpod-build-rl-gplctl-oauth-server.10 |
9288d08
to
25a6e47
Compare
/werft run 👍 started the job as gitpod-build-rl-gplctl-oauth-server.16 |
/werft run 👍 started the job as gitpod-build-rl-gplctl-oauth-server.17 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, a little chicken/egg :-( That scope is valid but insufficient (only a single workspace) so it was changed to 'getWorkspaces' in the backend. You need to build a local app using the commit I linked to in our discussion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's still a bunch of log statements which I'd fret would be too loud in production. Many (most?) of them should either be on debug level, or be removed.
Other than that, after some commit history authoring this change would be ready for main.
Thank you very much for adding this! OAuth in Gitpod enables a whole host of new use-cases.
to manage client application access to users Gitpod workspaces
b7b4a5d
to
8de1006
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…io#4222) to manage client application access to users Gitpod workspaces
…io#4222) to manage client application access to users Gitpod workspaces
No description provided.