- Notifications
You must be signed in to change notification settings - Fork1k
feat: implement standardized OAuth2 endpoints and token revocation#18809
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
feat: implement standardized OAuth2 endpoints and token revocation#18809
Conversation
ThomasK33 commentedJul 9, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
bc44107 toab73979Compareab73979 tob89c367Compareb89c367 tob73b71dCompare514f744 to386d77dCompare01f48f3 to9393060Compare9393060 to2c31819Compare4ad81e8 to12117a3Compared10afa7 tob436ec8Compare
aslilac left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
btw do we have any figma designs for these new pages? is there an easy way for me to take a look at them locally? at the very least I'd like to see some screenshots
| oauthTemplate*htmltemplate.Template | ||
| //go:embed static/oauth2device.html |
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.
this means all of these files will actually be embedded in the binary twice
the version copied by vite is practically useless since it will always be the unfilled template, but it will always be accessible at http:///oauth2device.html, and so on for the other similar html files.
maybe this means we should have a site/templates/ folder to put stuff like this in?
| } | ||
| // RenderOAuthDeviceData contains the variables that are found in | ||
| // site/static/oauth2device.html. |
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.
if we do move the files we should update this path
| } | ||
| // RenderOAuthDeviceResultData contains the variables that are found in | ||
| // site/static/oauth2device_success.html and site/static/oauth2device_denied.html. |
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.
and these
b436ec8 toaca2f6aCompare- Change /oauth2/tokens → /oauth2/token per RFC 6749 - Move token deletion to POST /oauth2/revoke per RFC 7009 - Update all endpoint URLs and documentation - Maintain backward compatibility in client librariesfeat: implement OAuth2 Device Authorization Grant (RFC 8628) - Add device authorization endpoint /oauth2/device/authorize - Add device verification UI at /oauth2/device - Support device_code grant type in token endpoint - Add database table for device codes with expiration - Implement polling interval and user authorization flow - Add comprehensive test coverage for device flowChange-Id: I7a7eebeb23a4f28718ebed2994d01dc21b49315bSigned-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: Ic232851727e683ab3d8b7ce970c505588da2f827Signed-off-by: Thomas Kosiewski <tk@coder.com>
aca2f6a to35d7f5aCompareThere 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.
I'm afraid this pr is large enough to bevery difficult to review 😅 any ideas on how we could break it up?
I would call out the size even for something half this line count. I think the ideal diff size for getting good reviews is under 1k.
| const ( | ||
| // ANSI color codes | ||
| colorReset="\033[0m" | ||
| colorRed="\033[31m" | ||
| colorGreen="\033[32m" | ||
| colorYellow="\033[33m" | ||
| colorBlue="\033[34m" | ||
| colorPurple="\033[35m" | ||
| colorCyan="\033[36m" | ||
| colorWhite="\033[37m" | ||
| ) |
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.
I see other places in the codebase where we use github.com/muesli/termenv for this, which will do a better job of not flooding the output with noise when stdout isn't a tty. it's maybe not a big deal for something in scripts/, but we already pull in the dependency. might as well use it.
| } | ||
| _=resp.Body.Close() | ||
| iferrorCode,ok:=result["error"].(string);ok { |
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.
this should probably usecodersdk.AsError
| // NOTE: OAuth2 client registration validation tests have been migrated to | ||
| // oauth2provider/validation_test.go for better separation of concerns |
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.
feels weird to leave this comment in the middle of such a huge file. maybe move it to the top actually?
| COMMENT ON TABLE oauth2_provider_device_codes IS'RFC 8628: OAuth2 Device Authorization Grant device codes'; | ||
| COMMENT ON COLUMN oauth2_provider_device_codes.device_code_hash IS'Hashed device code for security'; | ||
| COMMENT ON COLUMN oauth2_provider_device_codes.device_code_prefix IS'Device code prefix for lookup (first 8 chars)'; | ||
| COMMENT ON COLUMN oauth2_provider_device_codes.user_code IS'Human-readable code shown to user (6-8 characters)'; | ||
| COMMENT ON COLUMN oauth2_provider_device_codes.verification_uri IS'URI where user enters user_code'; | ||
| COMMENT ON COLUMN oauth2_provider_device_codes.verification_uri_complete IS'Optional complete URI with user_code embedded'; | ||
| COMMENT ON COLUMN oauth2_provider_device_codes.polling_interval IS'Minimum polling interval in seconds (RFC 8628)'; | ||
| COMMENT ON COLUMN oauth2_provider_device_codes.resource_uri IS'RFC 8707 resource parameter for audience restriction'; | ||
| COMMENT ON COLUMN oauth2_provider_device_codes.status IS'Current authorization status: pending (awaiting user action), authorized (user approved), or denied (user rejected)'; |
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.
I think we generally tend to be more conservative than this with SQL comments. some of these seem a bit overkill, likestatus which is an enum type and pretty self explanatory.
| client_id uuidNOT NULLREFERENCES oauth2_provider_apps(id)ON DELETE CASCADE, | ||
| user_id uuidREFERENCES users(id)ON DELETE CASCADE,-- NULL until authorized | ||
| -- Authorization state (using enum for better data integrity) |
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.
similarly here, I don't think this needs to be rationalized. enums are a great choice!
| @@ -0,0 +1,8 @@ | |||
| -- Remove OAuth2 Device Authorization Grant support (RFC 8628) | |||
| -- Remove constraints added for data integrity | |||
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.
this is also kinda weird. I imagine some agent inserted this? saying "this was added for data integrity" as you destroy it is just tonally odd.
@ThomasK33@Emyrk Sorry, somehow missed the first ping from two weeks ago. Where are we at on the PR? Do we just need another pass on the frontend? |
Uh oh!
There was an error while loading.Please reload this page.
feat: standardize OAuth2 endpoints and add token revocation
feat: implement OAuth2 Device Authorization Grant (RFC 8628)
chore: add OAuth2 device flow test scripts
Change-Id: Ic232851727e683ab3d8b7ce970c505588da2f827
Change-Id: I7a7eebeb23a4f28718ebed2994d01dc21b49315b
Signed-off-by: Thomas Kosiewskitk@coder.com
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Style
Chores