Implement OAuth client in VS Code #633
Conversation
EhabY
force-pushed
the
oauth-vs-code
branch
2 times, most recently
from
78db0ad to
b93e027
Compare
EhabY
force-pushed
the
oauth-vs-code
branch
6 times, most recently
from
8e5ad5d to
423742c
Compare
code-asher
left a comment
code-asher
left a comment
There was a problem hiding this comment.
I am I think maybe halfway through I need to log off but thought I would submit what I have so far.
code-asher
left a comment
code-asher
left a comment
There was a problem hiding this comment.
I have not tested if this was an existing issue, but when I log in with a token my workspaces are not populated until I manually refresh. Are you seeing the same thing?
I have not tested logging in with oauth yet because xdg-open opens the wrong thing, but that is an issue with my system configuration. 😛 But, could be cool if there was an option in the flow to manually paste in the callback URI as a fallback.
|
|
||
| this.refreshTimer = setTimeout(async () => { | ||
| try { | ||
| await this.refreshIfAlmostExpired(); |
There was a problem hiding this comment.
If we only refresh when almost expired, why not set a timer directly for that time rather than checking in a loop?
There was a problem hiding this comment.
So I've tried this approach just now but I'm thinking of reverting since it adds much more complexity, like we need to update on deployment change, update on token change, keep track of extra state as well..
| */ | ||
| private hasRequiredScopes(grantedScope: string | undefined): boolean { | ||
| if (!grantedScope) { | ||
| // TODO server always returns empty scopes |
There was a problem hiding this comment.
Yeah I think this was never implemented or at least not merged.
There was a problem hiding this comment.
Yup seems so, I'll keep this here for when it is added though we'll always return true for backward compatibility with older servers
| const DEFAULT_OAUTH_SCOPES = [ | ||
| "workspace:read", | ||
| "workspace:update", | ||
| "workspace:start", | ||
| "workspace:ssh", | ||
| "workspace:application_connect", | ||
| "template:read", | ||
| "user:read_personal", | ||
| ].join(" "); |
There was a problem hiding this comment.
Did you have a way to test if these scopes were sufficient? Not sure if there is a way right now, maybe if we try to generate a key manually with these scopes (I think this can be done on the account page) and paste it in to make sure everything works.
There was a problem hiding this comment.
I think this can be done on the account page
I just checked and no such way unless I am missing an extended accounts page :/
But yes that's a smart idea, we need a way to test these scopes
| @@ -0,0 +1,163 @@ | |||
| // OAuth 2.1 Grant Types | |||
| export type GrantType = | |||
There was a problem hiding this comment.
Is pulling these types from typesGenerated.ts an option?
There was a problem hiding this comment.
I upgraded the coder API package and it doesn't include all the types :/, it does include some though. So I'll try to replace those that are available
| @@ -0,0 +1,808 @@ | |||
| import { type AxiosInstance } from "axios"; | |||
There was a problem hiding this comment.
Have we considered using an oauth client library? I could see needing to add more parts of the spec in the future, or oidc or something, and a library could possibly handle it all for us.
There was a problem hiding this comment.
Yes, I wanted the bare minimum implementation first, but I suppose I can something like https://www.npmjs.com/package/openid-client
There was a problem hiding this comment.
I'm thinking of doing this one in a follow up so we can land something that works first (with tests) then use this library for more reliability
| await this.secretsManager.setOAuthTokens(deployment.label, tokens); | ||
| await this.secretsManager.setSessionAuth(deployment.label, { |
There was a problem hiding this comment.
From the outside looking in, it feels weird to me that we set oauth tokens and session auth separately. I think I would expect a single session auth object, and that object would have oauth information as well if it came from oauth.
Or to approach this another way, is there a use case for setting only oauth tokens but not the session auth?
There was a problem hiding this comment.
The session auth is simply URL + current token, while the OAuth tokens contains more information like expiry_timestamp and refresh_token (Also potentially scope). Let me try to consolidate this into an optional OAuth field to distinguish OAuth from non-OAuth
There was a problem hiding this comment.
Combined them in the following and you can see that we have certain racy calls that essentially do getSessionAuth then setSessionAuth which means we can overwrite with outdated information between multiple windows: 3d1440b
This is very unlikely to happen but it's possible and there's no way to guard against it unless we separate the fields or use file locks (inter-process communication). This actually makes me want to also separate hostname -> { url, token } into hostname -> url (memento since it's not sensitive) and hostname -> token because there are cases where we want to set the URL but not the token (currently we just pass an "" token)
2 participants
Closes #586