-
Notifications
You must be signed in to change notification settings - Fork 961
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 our terms of service and privacy 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
bc44107
to
ab73979
Compare
ab73979
to
b89c367
Compare
b89c367
to
b73b71d
Compare
514f744
to
386d77d
Compare
01f48f3
to
9393060
Compare
9393060
to
2c31819
Compare
8342301
to
3c0603e
Compare
coderd/database/migrations/testdata/fixtures/000350_oauth2_device_codes.up.sql
Outdated
Show resolved
Hide resolved
3c0603e
to
9dfcc8a
Compare
4ad81e8
to
12117a3
Compare
d10afa7
to
b436ec8
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.
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
@@ -58,6 +58,21 @@ var ( | |||
oauthHTML string | |||
|
|||
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?
@@ -1149,3 +1179,56 @@ func RenderOAuthAllowPage(rw http.ResponseWriter, r *http.Request, data RenderOA | |||
return | |||
} | |||
} | |||
|
|||
// 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
to
aca2f6a
Compare
- 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 libraries feat: 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 flow Change-Id: I7a7eebeb23a4f28718ebed2994d01dc21b49315b Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: Ic232851727e683ab3d8b7ce970c505588da2f827 Signed-off-by: Thomas Kosiewski <tk@coder.com>
aca2f6a
to
35d7f5a
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.
I'm afraid this pr is large enough to be very 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() | ||
|
||
if errorCode, 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 use codersdk.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, like status
which is an enum type and pretty self explanatory.
client_id uuid NOT NULL REFERENCES oauth2_provider_apps(id) ON DELETE CASCADE, | ||
user_id uuid REFERENCES 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.
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 Kosiewski tk@coder.com
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Style
Chores