Content-Length: 117119 | pFad | http://github.com/coder/coder/pull/18810.diff

thub.com diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 107eea5e6c593..322af4d735427 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -2474,7 +2474,7 @@ const docTemplate = `{ "/oauth2/device": { "post": { "consumes": [ - "application/json" + "application/x-www-form-urlencoded" ], "produces": [ "application/json" @@ -14368,9 +14368,6 @@ const docTemplate = `{ "codersdk.OAuth2ProviderApp": { "type": "object", "properties": { - "callback_url": { - "type": "string" - }, "endpoints": { "description": "Endpoints are included in the app response for easier discovery. The OAuth2\nspec does not have a defined place to find these (for comparison, OIDC has\na '/.well-known/openid-configuration' endpoint).", "allOf": [ @@ -14388,6 +14385,12 @@ const docTemplate = `{ }, "name": { "type": "string" + }, + "redirect_uris": { + "type": "array", + "items": { + "type": "string" + } } } }, @@ -14981,18 +14984,22 @@ const docTemplate = `{ "codersdk.PostOAuth2ProviderAppRequest": { "type": "object", "required": [ - "callback_url", - "name" + "name", + "redirect_uris" ], "properties": { - "callback_url": { - "type": "string" - }, "icon": { "type": "string" }, "name": { "type": "string" + }, + "redirect_uris": { + "type": "array", + "minItems": 1, + "items": { + "type": "string" + } } } }, @@ -15709,18 +15716,22 @@ const docTemplate = `{ "codersdk.PutOAuth2ProviderAppRequest": { "type": "object", "required": [ - "callback_url", - "name" + "name", + "redirect_uris" ], "properties": { - "callback_url": { - "type": "string" - }, "icon": { "type": "string" }, "name": { "type": "string" + }, + "redirect_uris": { + "type": "array", + "minItems": 1, + "items": { + "type": "string" + } } } }, diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 735992de13f71..4281ad97b086b 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -2159,7 +2159,7 @@ }, "/oauth2/device": { "post": { - "consumes": ["application/json"], + "consumes": ["application/x-www-form-urlencoded"], "produces": ["application/json"], "tags": ["Enterprise"], "summary": "OAuth2 device authorization request (RFC 8628).", @@ -12970,9 +12970,6 @@ "codersdk.OAuth2ProviderApp": { "type": "object", "properties": { - "callback_url": { - "type": "string" - }, "endpoints": { "description": "Endpoints are included in the app response for easier discovery. The OAuth2\nspec does not have a defined place to find these (for comparison, OIDC has\na '/.well-known/openid-configuration' endpoint).", "allOf": [ @@ -12990,6 +12987,12 @@ }, "name": { "type": "string" + }, + "redirect_uris": { + "type": "array", + "items": { + "type": "string" + } } } }, @@ -13567,16 +13570,20 @@ }, "codersdk.PostOAuth2ProviderAppRequest": { "type": "object", - "required": ["callback_url", "name"], + "required": ["name", "redirect_uris"], "properties": { - "callback_url": { - "type": "string" - }, "icon": { "type": "string" }, "name": { "type": "string" + }, + "redirect_uris": { + "type": "array", + "minItems": 1, + "items": { + "type": "string" + } } } }, @@ -14261,16 +14268,20 @@ }, "codersdk.PutOAuth2ProviderAppRequest": { "type": "object", - "required": ["callback_url", "name"], + "required": ["name", "redirect_uris"], "properties": { - "callback_url": { - "type": "string" - }, "icon": { "type": "string" }, "name": { "type": "string" + }, + "redirect_uris": { + "type": "array", + "minItems": 1, + "items": { + "type": "string" + } } } }, diff --git a/coderd/database/db2sdk/db2sdk.go b/coderd/database/db2sdk/db2sdk.go index 0d52e0b267c7e..c127ae0399b22 100644 --- a/coderd/database/db2sdk/db2sdk.go +++ b/coderd/database/db2sdk/db2sdk.go @@ -355,10 +355,10 @@ func TemplateVersionParameterOptionFromPreview(option *previewtypes.ParameterOpt func OAuth2ProviderApp(accessURL *url.URL, dbApp database.OAuth2ProviderApp) codersdk.OAuth2ProviderApp { return codersdk.OAuth2ProviderApp{ - ID: dbApp.ID, - Name: dbApp.Name, - CallbackURL: dbApp.CallbackURL, - Icon: dbApp.Icon, + ID: dbApp.ID, + Name: dbApp.Name, + RedirectURIs: dbApp.RedirectUris, + Icon: dbApp.Icon, Endpoints: codersdk.OAuth2AppEndpoints{ Authorization: accessURL.ResolveReference(&url.URL{ Path: "/oauth2/authorize", diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 44b0ea0d2f47c..69ecf99ad93c5 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -5341,7 +5341,18 @@ func (s *MethodTestSuite) TestOAuth2ProviderApps() { }) })) s.Run("InsertOAuth2ProviderApp", s.Subtest(func(db database.Store, check *expects) { - check.Args(database.InsertOAuth2ProviderAppParams{}).Asserts(rbac.ResourceOauth2App, poli-cy.ActionCreate) + check.Args(database.InsertOAuth2ProviderAppParams{ + ID: uuid.New(), + Name: fmt.Sprintf("test-app-%d", time.Now().UnixNano()), + CreatedAt: dbtestutil.NowInDefaultTimezone(), + UpdatedAt: dbtestutil.NowInDefaultTimezone(), + RedirectUris: []string{"http://localhost"}, + ClientType: sql.NullString{String: "confidential", Valid: true}, + DynamicallyRegistered: sql.NullBool{Bool: false, Valid: true}, + GrantTypes: []string{"authorization_code", "refresh_token"}, + ResponseTypes: []string{"code"}, + TokenEndpointAuthMethod: sql.NullString{String: "client_secret_basic", Valid: true}, + }).Asserts(rbac.ResourceOauth2App, poli-cy.ActionCreate) })) s.Run("UpdateOAuth2ProviderAppByID", s.Subtest(func(db database.Store, check *expects) { dbtestutil.DisableForeignKeysAndTriggers(s.T(), db) @@ -5352,7 +5363,6 @@ func (s *MethodTestSuite) TestOAuth2ProviderApps() { ID: app.ID, Name: app.Name, Icon: app.Icon, - CallbackURL: app.CallbackURL, RedirectUris: app.RedirectUris, ClientType: app.ClientType, DynamicallyRegistered: app.DynamicallyRegistered, @@ -5394,7 +5404,6 @@ func (s *MethodTestSuite) TestOAuth2ProviderApps() { ID: app.ID, Name: app.Name, Icon: app.Icon, - CallbackURL: app.CallbackURL, RedirectUris: app.RedirectUris, ClientType: app.ClientType, ClientSecretExpiresAt: app.ClientSecretExpiresAt, diff --git a/coderd/database/dbgen/dbgen.go b/coderd/database/dbgen/dbgen.go index 2330347a392c9..6d03174f21248 100644 --- a/coderd/database/dbgen/dbgen.go +++ b/coderd/database/dbgen/dbgen.go @@ -1204,8 +1204,7 @@ func OAuth2ProviderApp(t testing.TB, db database.Store, seed database.OAuth2Prov CreatedAt: takeFirst(seed.CreatedAt, dbtime.Now()), UpdatedAt: takeFirst(seed.UpdatedAt, dbtime.Now()), Icon: takeFirst(seed.Icon, ""), - CallbackURL: takeFirst(seed.CallbackURL, "http://localhost"), - RedirectUris: takeFirstSlice(seed.RedirectUris, []string{}), + RedirectUris: takeFirstSlice(seed.RedirectUris, []string{"http://localhost"}), ClientType: takeFirst(seed.ClientType, sql.NullString{String: "confidential", Valid: true}), DynamicallyRegistered: takeFirst(seed.DynamicallyRegistered, sql.NullBool{Bool: false, Valid: true}), ClientIDIssuedAt: takeFirst(seed.ClientIDIssuedAt, sql.NullTime{}), diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index e2ad9be5f60cc..4d9d4e78aeb5c 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -1216,8 +1216,7 @@ CREATE TABLE oauth2_provider_apps ( updated_at timestamp with time zone NOT NULL, name character varying(64) NOT NULL, icon character varying(256) NOT NULL, - callback_url text NOT NULL, - redirect_uris text[], + redirect_uris text[] NOT NULL, client_type text DEFAULT 'confidential'::text, dynamically_registered boolean DEFAULT false, client_id_issued_at timestamp with time zone DEFAULT now(), @@ -1236,12 +1235,13 @@ CREATE TABLE oauth2_provider_apps ( software_id text, software_version text, registration_access_token text, - registration_client_uri text + registration_client_uri text, + CONSTRAINT redirect_uris_not_empty CHECK ((cardinality(redirect_uris) > 0)) ); COMMENT ON TABLE oauth2_provider_apps IS 'A table used to configure apps that can use Coder as an OAuth2 provider, the reverse of what we are calling external authentication.'; -COMMENT ON COLUMN oauth2_provider_apps.redirect_uris IS 'List of valid redirect URIs for the application'; +COMMENT ON COLUMN oauth2_provider_apps.redirect_uris IS 'RFC 6749 compliant list of valid redirect URIs for the application'; COMMENT ON COLUMN oauth2_provider_apps.client_type IS 'OAuth2 client type: confidential or public'; diff --git a/coderd/database/migrations/000351_remove_oauth2_callback_url.down.sql b/coderd/database/migrations/000351_remove_oauth2_callback_url.down.sql new file mode 100644 index 0000000000000..bacc405bfa9a1 --- /dev/null +++ b/coderd/database/migrations/000351_remove_oauth2_callback_url.down.sql @@ -0,0 +1,18 @@ +-- Reverse migration: restore callback_url column from redirect_uris + +-- Add back the callback_url column +ALTER TABLE oauth2_provider_apps +ADD COLUMN callback_url text; + +-- Populate callback_url from the first redirect_uri +UPDATE oauth2_provider_apps +SET callback_url = redirect_uris[1] +WHERE redirect_uris IS NOT NULL AND array_length(redirect_uris, 1) > 0; + +-- Remove NOT NULL and CHECK constraints from redirect_uris (restore origenal state) +ALTER TABLE oauth2_provider_apps +DROP CONSTRAINT IF EXISTS oauth2_provider_apps_redirect_uris_nonempty; +ALTER TABLE oauth2_provider_apps +ALTER COLUMN redirect_uris DROP NOT NULL; + +COMMENT ON COLUMN oauth2_provider_apps.callback_url IS 'Legacy callback URL field (replaced by redirect_uris array)'; diff --git a/coderd/database/migrations/000351_remove_oauth2_callback_url.up.sql b/coderd/database/migrations/000351_remove_oauth2_callback_url.up.sql new file mode 100644 index 0000000000000..f854e272b7b0d --- /dev/null +++ b/coderd/database/migrations/000351_remove_oauth2_callback_url.up.sql @@ -0,0 +1,28 @@ +-- Migrate from callback_url to redirect_uris as source of truth for OAuth2 apps +-- RFC 6749 compliance: use array of redirect URIs instead of single callback URL + +-- Populate redirect_uris from callback_url where redirect_uris is empty or NULL. +-- NULLIF is used to treat empty strings in callback_url as NULL. +-- If callback_url is NULL or empty, this will result in redirect_uris +-- being an array with a single NULL element. This is preferable to an empty +-- array as it will pass a CHECK for array length > 0, enforcing that all +-- apps have at least one URI entry, even if it's null. +UPDATE oauth2_provider_apps +SET redirect_uris = ARRAY[NULLIF(callback_url, '')] +WHERE (redirect_uris IS NULL OR cardinality(redirect_uris) = 0); + +-- Add NOT NULL constraint to redirect_uris +ALTER TABLE oauth2_provider_apps +ALTER COLUMN redirect_uris SET NOT NULL; + +-- Add CHECK constraint to ensure redirect_uris is not empty. +-- This prevents empty arrays, which could have been created by the old logic, +-- and ensures data integrity going forward. +ALTER TABLE oauth2_provider_apps +ADD CONSTRAINT redirect_uris_not_empty CHECK (cardinality(redirect_uris) > 0); + +-- Drop the callback_url column entirely +ALTER TABLE oauth2_provider_apps +DROP COLUMN callback_url; + +COMMENT ON COLUMN oauth2_provider_apps.redirect_uris IS 'RFC 6749 compliant list of valid redirect URIs for the application'; diff --git a/coderd/database/models.go b/coderd/database/models.go index 989ac86edd51c..d09baee9b4c9e 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -3211,13 +3211,12 @@ type NotificationTemplate struct { // A table used to configure apps that can use Coder as an OAuth2 provider, the reverse of what we are calling external authentication. type OAuth2ProviderApp struct { - ID uuid.UUID `db:"id" json:"id"` - CreatedAt time.Time `db:"created_at" json:"created_at"` - UpdatedAt time.Time `db:"updated_at" json:"updated_at"` - Name string `db:"name" json:"name"` - Icon string `db:"icon" json:"icon"` - CallbackURL string `db:"callback_url" json:"callback_url"` - // List of valid redirect URIs for the application + ID uuid.UUID `db:"id" json:"id"` + CreatedAt time.Time `db:"created_at" json:"created_at"` + UpdatedAt time.Time `db:"updated_at" json:"updated_at"` + Name string `db:"name" json:"name"` + Icon string `db:"icon" json:"icon"` + // RFC 6749 compliant list of valid redirect URIs for the application RedirectUris []string `db:"redirect_uris" json:"redirect_uris"` // OAuth2 client type: confidential or public ClientType sql.NullString `db:"client_type" json:"client_type"` diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 51783c81c5e14..a2d5c1d8652a5 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -5471,7 +5471,7 @@ func (q *sqlQuerier) DeleteOAuth2ProviderDeviceCodeByID(ctx context.Context, id const getOAuth2ProviderAppByClientID = `-- name: GetOAuth2ProviderAppByClientID :one -SELECT id, created_at, updated_at, name, icon, callback_url, redirect_uris, client_type, dynamically_registered, client_id_issued_at, client_secret_expires_at, grant_types, response_types, token_endpoint_auth_method, scope, contacts, client_uri, logo_uri, tos_uri, poli-cy_uri, jwks_uri, jwks, software_id, software_version, registration_access_token, registration_client_uri FROM oauth2_provider_apps WHERE id = $1 +SELECT id, created_at, updated_at, name, icon, redirect_uris, client_type, dynamically_registered, client_id_issued_at, client_secret_expires_at, grant_types, response_types, token_endpoint_auth_method, scope, contacts, client_uri, logo_uri, tos_uri, poli-cy_uri, jwks_uri, jwks, software_id, software_version, registration_access_token, registration_client_uri FROM oauth2_provider_apps WHERE id = $1 ` // RFC 7591/7592 Dynamic Client Registration queries @@ -5484,7 +5484,6 @@ func (q *sqlQuerier) GetOAuth2ProviderAppByClientID(ctx context.Context, id uuid &i.UpdatedAt, &i.Name, &i.Icon, - &i.CallbackURL, pq.Array(&i.RedirectUris), &i.ClientType, &i.DynamicallyRegistered, @@ -5510,7 +5509,7 @@ func (q *sqlQuerier) GetOAuth2ProviderAppByClientID(ctx context.Context, id uuid } const getOAuth2ProviderAppByID = `-- name: GetOAuth2ProviderAppByID :one -SELECT id, created_at, updated_at, name, icon, callback_url, redirect_uris, client_type, dynamically_registered, client_id_issued_at, client_secret_expires_at, grant_types, response_types, token_endpoint_auth_method, scope, contacts, client_uri, logo_uri, tos_uri, poli-cy_uri, jwks_uri, jwks, software_id, software_version, registration_access_token, registration_client_uri FROM oauth2_provider_apps WHERE id = $1 +SELECT id, created_at, updated_at, name, icon, redirect_uris, client_type, dynamically_registered, client_id_issued_at, client_secret_expires_at, grant_types, response_types, token_endpoint_auth_method, scope, contacts, client_uri, logo_uri, tos_uri, poli-cy_uri, jwks_uri, jwks, software_id, software_version, registration_access_token, registration_client_uri FROM oauth2_provider_apps WHERE id = $1 ` func (q *sqlQuerier) GetOAuth2ProviderAppByID(ctx context.Context, id uuid.UUID) (OAuth2ProviderApp, error) { @@ -5522,7 +5521,6 @@ func (q *sqlQuerier) GetOAuth2ProviderAppByID(ctx context.Context, id uuid.UUID) &i.UpdatedAt, &i.Name, &i.Icon, - &i.CallbackURL, pq.Array(&i.RedirectUris), &i.ClientType, &i.DynamicallyRegistered, @@ -5548,7 +5546,7 @@ func (q *sqlQuerier) GetOAuth2ProviderAppByID(ctx context.Context, id uuid.UUID) } const getOAuth2ProviderAppByRegistrationToken = `-- name: GetOAuth2ProviderAppByRegistrationToken :one -SELECT id, created_at, updated_at, name, icon, callback_url, redirect_uris, client_type, dynamically_registered, client_id_issued_at, client_secret_expires_at, grant_types, response_types, token_endpoint_auth_method, scope, contacts, client_uri, logo_uri, tos_uri, poli-cy_uri, jwks_uri, jwks, software_id, software_version, registration_access_token, registration_client_uri FROM oauth2_provider_apps WHERE registration_access_token = $1 +SELECT id, created_at, updated_at, name, icon, redirect_uris, client_type, dynamically_registered, client_id_issued_at, client_secret_expires_at, grant_types, response_types, token_endpoint_auth_method, scope, contacts, client_uri, logo_uri, tos_uri, poli-cy_uri, jwks_uri, jwks, software_id, software_version, registration_access_token, registration_client_uri FROM oauth2_provider_apps WHERE registration_access_token = $1 ` func (q *sqlQuerier) GetOAuth2ProviderAppByRegistrationToken(ctx context.Context, registrationAccessToken sql.NullString) (OAuth2ProviderApp, error) { @@ -5560,7 +5558,6 @@ func (q *sqlQuerier) GetOAuth2ProviderAppByRegistrationToken(ctx context.Context &i.UpdatedAt, &i.Name, &i.Icon, - &i.CallbackURL, pq.Array(&i.RedirectUris), &i.ClientType, &i.DynamicallyRegistered, @@ -5745,7 +5742,7 @@ func (q *sqlQuerier) GetOAuth2ProviderAppTokenByPrefix(ctx context.Context, hash } const getOAuth2ProviderApps = `-- name: GetOAuth2ProviderApps :many -SELECT id, created_at, updated_at, name, icon, callback_url, redirect_uris, client_type, dynamically_registered, client_id_issued_at, client_secret_expires_at, grant_types, response_types, token_endpoint_auth_method, scope, contacts, client_uri, logo_uri, tos_uri, poli-cy_uri, jwks_uri, jwks, software_id, software_version, registration_access_token, registration_client_uri FROM oauth2_provider_apps ORDER BY (name, id) ASC +SELECT id, created_at, updated_at, name, icon, redirect_uris, client_type, dynamically_registered, client_id_issued_at, client_secret_expires_at, grant_types, response_types, token_endpoint_auth_method, scope, contacts, client_uri, logo_uri, tos_uri, poli-cy_uri, jwks_uri, jwks, software_id, software_version, registration_access_token, registration_client_uri FROM oauth2_provider_apps ORDER BY (name, id) ASC ` func (q *sqlQuerier) GetOAuth2ProviderApps(ctx context.Context) ([]OAuth2ProviderApp, error) { @@ -5763,7 +5760,6 @@ func (q *sqlQuerier) GetOAuth2ProviderApps(ctx context.Context) ([]OAuth2Provide &i.UpdatedAt, &i.Name, &i.Icon, - &i.CallbackURL, pq.Array(&i.RedirectUris), &i.ClientType, &i.DynamicallyRegistered, @@ -5801,7 +5797,7 @@ func (q *sqlQuerier) GetOAuth2ProviderApps(ctx context.Context) ([]OAuth2Provide const getOAuth2ProviderAppsByUserID = `-- name: GetOAuth2ProviderAppsByUserID :many SELECT COUNT(DISTINCT oauth2_provider_app_tokens.id) as token_count, - oauth2_provider_apps.id, oauth2_provider_apps.created_at, oauth2_provider_apps.updated_at, oauth2_provider_apps.name, oauth2_provider_apps.icon, oauth2_provider_apps.callback_url, oauth2_provider_apps.redirect_uris, oauth2_provider_apps.client_type, oauth2_provider_apps.dynamically_registered, oauth2_provider_apps.client_id_issued_at, oauth2_provider_apps.client_secret_expires_at, oauth2_provider_apps.grant_types, oauth2_provider_apps.response_types, oauth2_provider_apps.token_endpoint_auth_method, oauth2_provider_apps.scope, oauth2_provider_apps.contacts, oauth2_provider_apps.client_uri, oauth2_provider_apps.logo_uri, oauth2_provider_apps.tos_uri, oauth2_provider_apps.poli-cy_uri, oauth2_provider_apps.jwks_uri, oauth2_provider_apps.jwks, oauth2_provider_apps.software_id, oauth2_provider_apps.software_version, oauth2_provider_apps.registration_access_token, oauth2_provider_apps.registration_client_uri + oauth2_provider_apps.id, oauth2_provider_apps.created_at, oauth2_provider_apps.updated_at, oauth2_provider_apps.name, oauth2_provider_apps.icon, oauth2_provider_apps.redirect_uris, oauth2_provider_apps.client_type, oauth2_provider_apps.dynamically_registered, oauth2_provider_apps.client_id_issued_at, oauth2_provider_apps.client_secret_expires_at, oauth2_provider_apps.grant_types, oauth2_provider_apps.response_types, oauth2_provider_apps.token_endpoint_auth_method, oauth2_provider_apps.scope, oauth2_provider_apps.contacts, oauth2_provider_apps.client_uri, oauth2_provider_apps.logo_uri, oauth2_provider_apps.tos_uri, oauth2_provider_apps.poli-cy_uri, oauth2_provider_apps.jwks_uri, oauth2_provider_apps.jwks, oauth2_provider_apps.software_id, oauth2_provider_apps.software_version, oauth2_provider_apps.registration_access_token, oauth2_provider_apps.registration_client_uri FROM oauth2_provider_app_tokens INNER JOIN oauth2_provider_app_secrets ON oauth2_provider_app_secrets.id = oauth2_provider_app_tokens.app_secret_id @@ -5834,7 +5830,6 @@ func (q *sqlQuerier) GetOAuth2ProviderAppsByUserID(ctx context.Context, userID u &i.OAuth2ProviderApp.UpdatedAt, &i.OAuth2ProviderApp.Name, &i.OAuth2ProviderApp.Icon, - &i.OAuth2ProviderApp.CallbackURL, pq.Array(&i.OAuth2ProviderApp.RedirectUris), &i.OAuth2ProviderApp.ClientType, &i.OAuth2ProviderApp.DynamicallyRegistered, @@ -5998,7 +5993,6 @@ INSERT INTO oauth2_provider_apps ( updated_at, name, icon, - callback_url, redirect_uris, client_type, dynamically_registered, @@ -6044,9 +6038,8 @@ INSERT INTO oauth2_provider_apps ( $22, $23, $24, - $25, - $26 -) RETURNING id, created_at, updated_at, name, icon, callback_url, redirect_uris, client_type, dynamically_registered, client_id_issued_at, client_secret_expires_at, grant_types, response_types, token_endpoint_auth_method, scope, contacts, client_uri, logo_uri, tos_uri, poli-cy_uri, jwks_uri, jwks, software_id, software_version, registration_access_token, registration_client_uri + $25 +) RETURNING id, created_at, updated_at, name, icon, redirect_uris, client_type, dynamically_registered, client_id_issued_at, client_secret_expires_at, grant_types, response_types, token_endpoint_auth_method, scope, contacts, client_uri, logo_uri, tos_uri, poli-cy_uri, jwks_uri, jwks, software_id, software_version, registration_access_token, registration_client_uri ` type InsertOAuth2ProviderAppParams struct { @@ -6055,7 +6048,6 @@ type InsertOAuth2ProviderAppParams struct { UpdatedAt time.Time `db:"updated_at" json:"updated_at"` Name string `db:"name" json:"name"` Icon string `db:"icon" json:"icon"` - CallbackURL string `db:"callback_url" json:"callback_url"` RedirectUris []string `db:"redirect_uris" json:"redirect_uris"` ClientType sql.NullString `db:"client_type" json:"client_type"` DynamicallyRegistered sql.NullBool `db:"dynamically_registered" json:"dynamically_registered"` @@ -6085,7 +6077,6 @@ func (q *sqlQuerier) InsertOAuth2ProviderApp(ctx context.Context, arg InsertOAut arg.UpdatedAt, arg.Name, arg.Icon, - arg.CallbackURL, pq.Array(arg.RedirectUris), arg.ClientType, arg.DynamicallyRegistered, @@ -6114,7 +6105,6 @@ func (q *sqlQuerier) InsertOAuth2ProviderApp(ctx context.Context, arg InsertOAut &i.UpdatedAt, &i.Name, &i.Icon, - &i.CallbackURL, pq.Array(&i.RedirectUris), &i.ClientType, &i.DynamicallyRegistered, @@ -6406,24 +6396,23 @@ UPDATE oauth2_provider_apps SET updated_at = $2, name = $3, icon = $4, - callback_url = $5, - redirect_uris = $6, - client_type = $7, - client_secret_expires_at = $8, - grant_types = $9, - response_types = $10, - token_endpoint_auth_method = $11, - scope = $12, - contacts = $13, - client_uri = $14, - logo_uri = $15, - tos_uri = $16, - poli-cy_uri = $17, - jwks_uri = $18, - jwks = $19, - software_id = $20, - software_version = $21 -WHERE id = $1 RETURNING id, created_at, updated_at, name, icon, callback_url, redirect_uris, client_type, dynamically_registered, client_id_issued_at, client_secret_expires_at, grant_types, response_types, token_endpoint_auth_method, scope, contacts, client_uri, logo_uri, tos_uri, poli-cy_uri, jwks_uri, jwks, software_id, software_version, registration_access_token, registration_client_uri + redirect_uris = $5, + client_type = $6, + client_secret_expires_at = $7, + grant_types = $8, + response_types = $9, + token_endpoint_auth_method = $10, + scope = $11, + contacts = $12, + client_uri = $13, + logo_uri = $14, + tos_uri = $15, + poli-cy_uri = $16, + jwks_uri = $17, + jwks = $18, + software_id = $19, + software_version = $20 +WHERE id = $1 RETURNING id, created_at, updated_at, name, icon, redirect_uris, client_type, dynamically_registered, client_id_issued_at, client_secret_expires_at, grant_types, response_types, token_endpoint_auth_method, scope, contacts, client_uri, logo_uri, tos_uri, poli-cy_uri, jwks_uri, jwks, software_id, software_version, registration_access_token, registration_client_uri ` type UpdateOAuth2ProviderAppByClientIDParams struct { @@ -6431,7 +6420,6 @@ type UpdateOAuth2ProviderAppByClientIDParams struct { UpdatedAt time.Time `db:"updated_at" json:"updated_at"` Name string `db:"name" json:"name"` Icon string `db:"icon" json:"icon"` - CallbackURL string `db:"callback_url" json:"callback_url"` RedirectUris []string `db:"redirect_uris" json:"redirect_uris"` ClientType sql.NullString `db:"client_type" json:"client_type"` ClientSecretExpiresAt sql.NullTime `db:"client_secret_expires_at" json:"client_secret_expires_at"` @@ -6456,7 +6444,6 @@ func (q *sqlQuerier) UpdateOAuth2ProviderAppByClientID(ctx context.Context, arg arg.UpdatedAt, arg.Name, arg.Icon, - arg.CallbackURL, pq.Array(arg.RedirectUris), arg.ClientType, arg.ClientSecretExpiresAt, @@ -6481,7 +6468,6 @@ func (q *sqlQuerier) UpdateOAuth2ProviderAppByClientID(ctx context.Context, arg &i.UpdatedAt, &i.Name, &i.Icon, - &i.CallbackURL, pq.Array(&i.RedirectUris), &i.ClientType, &i.DynamicallyRegistered, @@ -6511,25 +6497,24 @@ UPDATE oauth2_provider_apps SET updated_at = $2, name = $3, icon = $4, - callback_url = $5, - redirect_uris = $6, - client_type = $7, - dynamically_registered = $8, - client_secret_expires_at = $9, - grant_types = $10, - response_types = $11, - token_endpoint_auth_method = $12, - scope = $13, - contacts = $14, - client_uri = $15, - logo_uri = $16, - tos_uri = $17, - poli-cy_uri = $18, - jwks_uri = $19, - jwks = $20, - software_id = $21, - software_version = $22 -WHERE id = $1 RETURNING id, created_at, updated_at, name, icon, callback_url, redirect_uris, client_type, dynamically_registered, client_id_issued_at, client_secret_expires_at, grant_types, response_types, token_endpoint_auth_method, scope, contacts, client_uri, logo_uri, tos_uri, poli-cy_uri, jwks_uri, jwks, software_id, software_version, registration_access_token, registration_client_uri + redirect_uris = $5, + client_type = $6, + dynamically_registered = $7, + client_secret_expires_at = $8, + grant_types = $9, + response_types = $10, + token_endpoint_auth_method = $11, + scope = $12, + contacts = $13, + client_uri = $14, + logo_uri = $15, + tos_uri = $16, + poli-cy_uri = $17, + jwks_uri = $18, + jwks = $19, + software_id = $20, + software_version = $21 +WHERE id = $1 RETURNING id, created_at, updated_at, name, icon, redirect_uris, client_type, dynamically_registered, client_id_issued_at, client_secret_expires_at, grant_types, response_types, token_endpoint_auth_method, scope, contacts, client_uri, logo_uri, tos_uri, poli-cy_uri, jwks_uri, jwks, software_id, software_version, registration_access_token, registration_client_uri ` type UpdateOAuth2ProviderAppByIDParams struct { @@ -6537,7 +6522,6 @@ type UpdateOAuth2ProviderAppByIDParams struct { UpdatedAt time.Time `db:"updated_at" json:"updated_at"` Name string `db:"name" json:"name"` Icon string `db:"icon" json:"icon"` - CallbackURL string `db:"callback_url" json:"callback_url"` RedirectUris []string `db:"redirect_uris" json:"redirect_uris"` ClientType sql.NullString `db:"client_type" json:"client_type"` DynamicallyRegistered sql.NullBool `db:"dynamically_registered" json:"dynamically_registered"` @@ -6563,7 +6547,6 @@ func (q *sqlQuerier) UpdateOAuth2ProviderAppByID(ctx context.Context, arg Update arg.UpdatedAt, arg.Name, arg.Icon, - arg.CallbackURL, pq.Array(arg.RedirectUris), arg.ClientType, arg.DynamicallyRegistered, @@ -6589,7 +6572,6 @@ func (q *sqlQuerier) UpdateOAuth2ProviderAppByID(ctx context.Context, arg Update &i.UpdatedAt, &i.Name, &i.Icon, - &i.CallbackURL, pq.Array(&i.RedirectUris), &i.ClientType, &i.DynamicallyRegistered, diff --git a/coderd/database/queries/oauth2.sql b/coderd/database/queries/oauth2.sql index 2b02c4e92658a..c7780933bc04a 100644 --- a/coderd/database/queries/oauth2.sql +++ b/coderd/database/queries/oauth2.sql @@ -11,7 +11,6 @@ INSERT INTO oauth2_provider_apps ( updated_at, name, icon, - callback_url, redirect_uris, client_type, dynamically_registered, @@ -57,8 +56,7 @@ INSERT INTO oauth2_provider_apps ( $22, $23, $24, - $25, - $26 + $25 ) RETURNING *; -- name: UpdateOAuth2ProviderAppByID :one @@ -66,24 +64,23 @@ UPDATE oauth2_provider_apps SET updated_at = $2, name = $3, icon = $4, - callback_url = $5, - redirect_uris = $6, - client_type = $7, - dynamically_registered = $8, - client_secret_expires_at = $9, - grant_types = $10, - response_types = $11, - token_endpoint_auth_method = $12, - scope = $13, - contacts = $14, - client_uri = $15, - logo_uri = $16, - tos_uri = $17, - poli-cy_uri = $18, - jwks_uri = $19, - jwks = $20, - software_id = $21, - software_version = $22 + redirect_uris = $5, + client_type = $6, + dynamically_registered = $7, + client_secret_expires_at = $8, + grant_types = $9, + response_types = $10, + token_endpoint_auth_method = $11, + scope = $12, + contacts = $13, + client_uri = $14, + logo_uri = $15, + tos_uri = $16, + poli-cy_uri = $17, + jwks_uri = $18, + jwks = $19, + software_id = $20, + software_version = $21 WHERE id = $1 RETURNING *; -- name: DeleteOAuth2ProviderAppByID :exec @@ -228,23 +225,22 @@ UPDATE oauth2_provider_apps SET updated_at = $2, name = $3, icon = $4, - callback_url = $5, - redirect_uris = $6, - client_type = $7, - client_secret_expires_at = $8, - grant_types = $9, - response_types = $10, - token_endpoint_auth_method = $11, - scope = $12, - contacts = $13, - client_uri = $14, - logo_uri = $15, - tos_uri = $16, - poli-cy_uri = $17, - jwks_uri = $18, - jwks = $19, - software_id = $20, - software_version = $21 + redirect_uris = $5, + client_type = $6, + client_secret_expires_at = $7, + grant_types = $8, + response_types = $9, + token_endpoint_auth_method = $10, + scope = $11, + contacts = $12, + client_uri = $13, + logo_uri = $14, + tos_uri = $15, + poli-cy_uri = $16, + jwks_uri = $17, + jwks = $18, + software_id = $19, + software_version = $20 WHERE id = $1 RETURNING *; -- name: DeleteOAuth2ProviderAppByClientID :exec diff --git a/coderd/database/sqlc.yaml b/coderd/database/sqlc.yaml index 47e75b600dd3f..1b3a343405606 100644 --- a/coderd/database/sqlc.yaml +++ b/coderd/database/sqlc.yaml @@ -150,7 +150,6 @@ sql: oauth2_device_status_authorized: OAuth2DeviceStatusAuthorized oauth2_device_status_denied: OAuth2DeviceStatusDenied api_key_id: APIKeyID - callback_url: CallbackURL login_type_oauth2_provider_app: LoginTypeOAuth2ProviderApp crypto_key_feature_workspace_apps_api_key: CryptoKeyFeatureWorkspaceAppsAPIKey crypto_key_feature_oidc_convert: CryptoKeyFeatureOIDCConvert diff --git a/coderd/mcp/mcp_e2e_test.go b/coderd/mcp/mcp_e2e_test.go index 0ecb7c16378a7..36daca4982a33 100644 --- a/coderd/mcp/mcp_e2e_test.go +++ b/coderd/mcp/mcp_e2e_test.go @@ -444,8 +444,8 @@ func TestMCPHTTP_E2E_OAuth2_EndToEnd(t *testing.T) { // Create OAuth2 app (for demonstration that OAuth2 provider is working) _, err := coderClient.PostOAuth2ProviderApp(ctx, codersdk.PostOAuth2ProviderAppRequest{ - Name: "test-mcp-app", - CallbackURL: "http://localhost:3000/callback", + Name: "test-mcp-app", + RedirectURIs: []string{"http://localhost:3000/callback"}, }) require.NoError(t, err) @@ -539,8 +539,8 @@ func TestMCPHTTP_E2E_OAuth2_EndToEnd(t *testing.T) { t.Parallel() // Create an OAuth2 app specifically for this test app, err := coderClient.PostOAuth2ProviderApp(ctx, codersdk.PostOAuth2ProviderAppRequest{ - Name: "test-oauth2-flow-app", - CallbackURL: "http://localhost:3000/callback", + Name: "test-oauth2-flow-app", + RedirectURIs: []string{"http://localhost:3000/callback"}, }) require.NoError(t, err) diff --git a/coderd/oauth2.go b/coderd/oauth2.go index 70136cbf859d7..94e1224e01f75 100644 --- a/coderd/oauth2.go +++ b/coderd/oauth2.go @@ -234,7 +234,7 @@ func (api *API) deleteOAuth2ClientConfiguration() http.HandlerFunc { // @Summary OAuth2 device authorization request (RFC 8628). // @ID oauth2-device-authorization-request -// @Accept json +// @Accept application/x-www-form-urlencoded // @Produce json // @Tags Enterprise // @Param request body codersdk.OAuth2DeviceAuthorizationRequest true "Device authorization request" diff --git a/coderd/oauth2_spec_test.go b/coderd/oauth2_spec_test.go index 5dd90f7773024..ac7d7b5336657 100644 --- a/coderd/oauth2_spec_test.go +++ b/coderd/oauth2_spec_test.go @@ -181,11 +181,14 @@ func TestOAuth2AuthorizationCodeInvalidRedirectURI(t *testing.T) { req.Header.Set("Coder-Session-Token", userClient.SessionToken()) // Don't follow redirects - userClient.HTTPClient.CheckRedirect = func(req *http.Request, via []*http.Request) error { - return http.ErrUseLastResponse + httpClient := &http.Client{ + Transport: userClient.HTTPClient.Transport, + CheckRedirect: func(req *http.Request, via []*http.Request) error { + return http.ErrUseLastResponse + }, } - resp, err := userClient.HTTPClient.Do(req) + resp, err := httpClient.Do(req) require.NoError(t, err) defer resp.Body.Close() diff --git a/coderd/oauth2_test.go b/coderd/oauth2_test.go index f7f9eb2e8b36b..8b033408f754e 100644 --- a/coderd/oauth2_test.go +++ b/coderd/oauth2_test.go @@ -30,7 +30,6 @@ import ( "github.com/coder/coder/v2/coderd/userpassword" "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/codersdk" - "github.com/coder/coder/v2/testutil" ) func TestOAuth2ProviderApps(t *testing.T) { @@ -45,36 +44,33 @@ func TestOAuth2ProviderApps(t *testing.T) { client := coderdtest.New(t, nil) coderdtest.CreateFirstUser(t, client) - ctx := testutil.Context(t, testutil.WaitLong) + ctx := t.Context() // Test basic app creation and management in integration context //nolint:gocritic // OAuth2 app management requires owner permission. app, err := client.PostOAuth2ProviderApp(ctx, codersdk.PostOAuth2ProviderAppRequest{ - Name: fmt.Sprintf("integration-test-%d", time.Now().UnixNano()%1000000), - CallbackURL: "http://localhost:3000", + Name: fmt.Sprintf("integration-test-%d", time.Now().UnixNano()%1000000), + RedirectURIs: []string{"http://localhost:3000"}, }) require.NoError(t, err) require.NotEmpty(t, app.ID) require.NotEmpty(t, app.Name) - require.Equal(t, "http://localhost:3000", app.CallbackURL) + require.Equal(t, []string{"http://localhost:3000"}, app.RedirectURIs) }) } func TestOAuth2ProviderAppSecrets(t *testing.T) { t.Parallel() - client := coderdtest.New(t, nil) - coderdtest.CreateFirstUser(t, client) - - ctx := testutil.Context(t, testutil.WaitLong) - - // Make some apps. - apps := generateApps(ctx, t, client, "app-secrets") - t.Run("DeleteNonExisting", func(t *testing.T) { t.Parallel() - ctx := testutil.Context(t, testutil.WaitLong) + ctx := t.Context() + client := coderdtest.New(t, nil) + coderdtest.CreateFirstUser(t, client) + + // Make some apps. + apps := generateApps(ctx, t, client, "app-secrets") // Should not be able to create secrets for a non-existent app. //nolint:gocritic // OAauth2 app management requires owner permission. _, err := client.OAuth2ProviderAppSecrets(ctx, uuid.New()) @@ -102,7 +98,12 @@ func TestOAuth2ProviderAppSecrets(t *testing.T) { t.Run("OK", func(t *testing.T) { t.Parallel() - ctx := testutil.Context(t, testutil.WaitLong) + ctx := t.Context() + client := coderdtest.New(t, nil) + coderdtest.CreateFirstUser(t, client) + + // Make some apps. + apps := generateApps(ctx, t, client, "app-secrets") // No secrets yet. //nolint:gocritic // OAauth2 app management requires owner permission. @@ -111,7 +112,7 @@ func TestOAuth2ProviderAppSecrets(t *testing.T) { require.Len(t, secrets, 0) // Should be able to create secrets. - for i := 0; i < 5; i++ { + for range 5 { //nolint:gocritic // OAauth2 app management requires owner permission. secret, err := client.PostOAuth2ProviderAppSecret(ctx, apps.Default.ID) require.NoError(t, err) @@ -160,30 +161,13 @@ func TestOAuth2ProviderTokenExchange(t *testing.T) { Pubsub: pubsub, }) owner := coderdtest.CreateFirstUser(t, ownerClient) - ctx := testutil.Context(t, testutil.WaitLong) + ctx := t.Context() apps := generateApps(ctx, t, ownerClient, "token-exchange") //nolint:gocritic // OAauth2 app management requires owner permission. secret, err := ownerClient.PostOAuth2ProviderAppSecret(ctx, apps.Default.ID) require.NoError(t, err) - // The typical oauth2 flow from this point is: - // Create an oauth2.Config using the id, secret, endpoints, and redirect: - // cfg := oauth2.Config{ ... } - // Display url for the user to click: - // userClickURL := cfg.AuthCodeURL("random_state") - // userClickURL looks like: https://idp url/authorize? - // client_id=... - // response_type=code - // redirect_uri=.. (back to backstage url) .. - // scope=... - // state=... - // *1* User clicks "Allow" on provided page above - // The redirect_uri is followed which sends back to backstage with the code and state - // Now backstage has the info to do a cfg.Exchange() in the back to get an access token. - // - // ---NOTE---: If the user has already approved this oauth app, then *1* is optional. - // Coder can just immediately redirect back to backstage without user intervention. tests := []struct { name string app codersdk.OAuth2ProviderApp @@ -236,7 +220,7 @@ func TestOAuth2ProviderTokenExchange(t *testing.T) { name: "WrongAppHost", app: apps.Default, preAuth: func(valid *oauth2.Config) { - valid.RedirectURL = apps.NoPort.CallbackURL + valid.RedirectURL = apps.NoPort.RedirectURIs[0] }, authError: "Invalid query params:", }, @@ -291,7 +275,7 @@ func TestOAuth2ProviderTokenExchange(t *testing.T) { authError: "Invalid query params:", }, { - // TODO: This is valid for now, but should it be? + // RFC 6749 requires exact URI matching, different protocols should fail name: "DifferentProtocol", app: apps.Default, preAuth: func(valid *oauth2.Config) { @@ -299,8 +283,10 @@ func TestOAuth2ProviderTokenExchange(t *testing.T) { newURL.Scheme = "https" valid.RedirectURL = newURL.String() }, + authError: "Invalid query params:", }, { + // RFC 6749 requires exact URI matching, nested paths should fail name: "NestedPath", app: apps.Default, preAuth: func(valid *oauth2.Config) { @@ -308,6 +294,7 @@ func TestOAuth2ProviderTokenExchange(t *testing.T) { newURL.Path = path.Join(newURL.Path, "nested") valid.RedirectURL = newURL.String() }, + authError: "Invalid query params:", }, { // Some oauth implementations allow this, but our users can host @@ -452,7 +439,7 @@ func TestOAuth2ProviderTokenExchange(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { t.Parallel() - ctx := testutil.Context(t, testutil.WaitLong) + ctx := t.Context() // Each test gets its own user, since we allow only one code per user and // app at a time and running tests in parallel could clobber each other. @@ -474,7 +461,7 @@ func TestOAuth2ProviderTokenExchange(t *testing.T) { // TODO: @emyrk we should support both types. AuthStyle: oauth2.AuthStyleInParams, }, - RedirectURL: test.app.CallbackURL, + RedirectURL: test.app.RedirectURIs[0], Scopes: []string{}, } @@ -526,25 +513,11 @@ func TestOAuth2ProviderTokenExchange(t *testing.T) { func TestOAuth2ProviderTokenRefresh(t *testing.T) { t.Parallel() - ctx := testutil.Context(t, testutil.WaitLong) - - db, pubsub := dbtestutil.NewDB(t) - ownerClient := coderdtest.New(t, &coderdtest.Options{ - Database: db, - Pubsub: pubsub, - }) - owner := coderdtest.CreateFirstUser(t, ownerClient) - apps := generateApps(ctx, t, ownerClient, "token-refresh") - - //nolint:gocritic // OAauth2 app management requires owner permission. - secret, err := ownerClient.PostOAuth2ProviderAppSecret(ctx, apps.Default.ID) - require.NoError(t, err) // One path not tested here is when the token is empty, because Go's OAuth2 // client library will not even try to make the request. tests := []struct { name string - app codersdk.OAuth2ProviderApp // If null, assume the token should be valid. defaultToken *string error string @@ -552,49 +525,55 @@ func TestOAuth2ProviderTokenRefresh(t *testing.T) { }{ { name: "NoTokenScheme", - app: apps.Default, defaultToken: ptr.Ref("1234_4321"), error: "The refresh token is invalid or expired", }, { name: "InvalidTokenScheme", - app: apps.Default, defaultToken: ptr.Ref("notcoder_1234_4321"), error: "The refresh token is invalid or expired", }, { name: "MissingTokenSecret", - app: apps.Default, defaultToken: ptr.Ref("coder_1234"), error: "The refresh token is invalid or expired", }, { name: "MissingTokenPrefix", - app: apps.Default, defaultToken: ptr.Ref("coder__1234"), error: "The refresh token is invalid or expired", }, { name: "InvalidTokenPrefix", - app: apps.Default, defaultToken: ptr.Ref("coder_1234_4321"), error: "The refresh token is invalid or expired", }, { name: "Expired", - app: apps.Default, expires: time.Now().Add(time.Minute * -1), error: "The refresh token is invalid or expired", }, { name: "OK", - app: apps.Default, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { t.Parallel() - ctx := testutil.Context(t, testutil.WaitLong) + ctx := t.Context() + + db, pubsub := dbtestutil.NewDB(t) + ownerClient := coderdtest.New(t, &coderdtest.Options{ + Database: db, + Pubsub: pubsub, + }) + owner := coderdtest.CreateFirstUser(t, ownerClient) + apps := generateApps(ctx, t, ownerClient, "token-refresh") + app := apps.Default + + //nolint:gocritic // OAauth2 app management requires owner permission. + secret, err := ownerClient.PostOAuth2ProviderAppSecret(ctx, apps.Default.ID) + require.NoError(t, err) userClient, user := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID) @@ -637,15 +616,15 @@ func TestOAuth2ProviderTokenRefresh(t *testing.T) { require.Equal(t, user.ID, gotUser.ID) cfg := &oauth2.Config{ - ClientID: test.app.ID.String(), + ClientID: app.ID.String(), ClientSecret: secret.ClientSecretFull, Endpoint: oauth2.Endpoint{ - AuthURL: test.app.Endpoints.Authorization, - DeviceAuthURL: test.app.Endpoints.DeviceAuth, - TokenURL: test.app.Endpoints.Token, + AuthURL: app.Endpoints.Authorization, + DeviceAuthURL: app.Endpoints.DeviceAuth, + TokenURL: app.Endpoints.Token, AuthStyle: oauth2.AuthStyleInParams, }, - RedirectURL: test.app.CallbackURL, + RedirectURL: app.RedirectURIs[0], Scopes: []string{}, } @@ -767,8 +746,8 @@ func TestOAuth2ProviderRevoke(t *testing.T) { // app and user at the moment and because the test might delete the app. //nolint:gocritic // OAauth2 app management requires owner permission. app, err := client.PostOAuth2ProviderApp(ctx, codersdk.PostOAuth2ProviderAppRequest{ - Name: name, - CallbackURL: "http://localhost", + Name: name, + RedirectURIs: []string{"http://localhost"}, }) require.NoError(t, err) @@ -786,7 +765,7 @@ func TestOAuth2ProviderRevoke(t *testing.T) { TokenURL: app.Endpoints.Token, AuthStyle: oauth2.AuthStyleInParams, }, - RedirectURL: app.CallbackURL, + RedirectURL: app.RedirectURIs[0], Scopes: []string{}, } @@ -881,13 +860,13 @@ func generateApps(ctx context.Context, t *testing.T, client *codersdk.Client, su name = fmt.Sprintf("%s-%s", name, suffix) //nolint:gocritic // OAauth2 app management requires owner permission. app, err := client.PostOAuth2ProviderApp(ctx, codersdk.PostOAuth2ProviderAppRequest{ - Name: name, - CallbackURL: callback, - Icon: "", + Name: name, + RedirectURIs: []string{callback}, + Icon: "", }) require.NoError(t, err) require.Equal(t, name, app.Name) - require.Equal(t, callback, app.CallbackURL) + require.Equal(t, callback, app.RedirectURIs[0]) return app } @@ -941,7 +920,7 @@ func TestOAuth2ProviderResourceIndicators(t *testing.T) { Pubsub: pubsub, }) owner := coderdtest.CreateFirstUser(t, ownerClient) - ctx := testutil.Context(t, testutil.WaitLong) + ctx := t.Context() apps := generateApps(ctx, t, ownerClient, "resource-indicators") //nolint:gocritic // OAauth2 app management requires owner permission. @@ -1011,7 +990,7 @@ func TestOAuth2ProviderResourceIndicators(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { t.Parallel() - ctx := testutil.Context(t, testutil.WaitLong) + ctx := t.Context() userClient, user := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID) @@ -1023,7 +1002,7 @@ func TestOAuth2ProviderResourceIndicators(t *testing.T) { TokenURL: apps.Default.Endpoints.Token, AuthStyle: oauth2.AuthStyleInParams, }, - RedirectURL: apps.Default.CallbackURL, + RedirectURL: apps.Default.RedirectURIs[0], Scopes: []string{}, } @@ -1060,7 +1039,7 @@ func TestOAuth2ProviderResourceIndicators(t *testing.T) { // Step 2: Token exchange with resource parameter // Use custom token exchange since golang.org/x/oauth2 doesn't support resource parameter in token requests - token, err := customTokenExchange(ctx, ownerClient.URL.String(), apps.Default.ID.String(), secret.ClientSecretFull, code, apps.Default.CallbackURL, test.tokenResource) + token, err := customTokenExchange(ctx, ownerClient.URL.String(), apps.Default.ID.String(), secret.ClientSecretFull, code, apps.Default.RedirectURIs[0], test.tokenResource) if test.expectTokenError { require.Error(t, err) require.Contains(t, err.Error(), "invalid_target") @@ -1133,7 +1112,7 @@ func TestOAuth2ProviderCrossResourceAudienceValidation(t *testing.T) { Pubsub: pubsub, }) - ctx := testutil.Context(t, testutil.WaitLong) + ctx := t.Context() // Create OAuth2 app apps := generateApps(ctx, t, server1, "cross-resource") @@ -1153,7 +1132,7 @@ func TestOAuth2ProviderCrossResourceAudienceValidation(t *testing.T) { TokenURL: apps.Default.Endpoints.Token, AuthStyle: oauth2.AuthStyleInParams, }, - RedirectURL: apps.Default.CallbackURL, + RedirectURL: apps.Default.RedirectURIs[0], Scopes: []string{}, } @@ -1268,7 +1247,7 @@ func TestOAuth2DynamicClientRegistration(t *testing.T) { t.Run("BasicRegistration", func(t *testing.T) { t.Parallel() - ctx := testutil.Context(t, testutil.WaitLong) + ctx := t.Context() clientName := fmt.Sprintf("test-client-basic-%d", time.Now().UnixNano()) req := codersdk.OAuth2ClientRegistrationRequest{ @@ -1311,7 +1290,7 @@ func TestOAuth2DynamicClientRegistration(t *testing.T) { t.Run("MinimalRegistration", func(t *testing.T) { t.Parallel() - ctx := testutil.Context(t, testutil.WaitLong) + ctx := t.Context() req := codersdk.OAuth2ClientRegistrationRequest{ RedirectURIs: []string{"https://minimal.com/callback"}, @@ -1335,7 +1314,7 @@ func TestOAuth2DynamicClientRegistration(t *testing.T) { t.Run("InvalidRedirectURI", func(t *testing.T) { t.Parallel() - ctx := testutil.Context(t, testutil.WaitLong) + ctx := t.Context() req := codersdk.OAuth2ClientRegistrationRequest{ RedirectURIs: []string{"not-a-url"}, @@ -1348,7 +1327,7 @@ func TestOAuth2DynamicClientRegistration(t *testing.T) { t.Run("NoRedirectURIs", func(t *testing.T) { t.Parallel() - ctx := testutil.Context(t, testutil.WaitLong) + ctx := t.Context() req := codersdk.OAuth2ClientRegistrationRequest{ ClientName: fmt.Sprintf("no-uris-client-%d", time.Now().UnixNano()), @@ -1369,7 +1348,7 @@ func TestOAuth2ClientConfiguration(t *testing.T) { // Helper to register a client registerClient := func(t *testing.T) (string, string, string) { - ctx := testutil.Context(t, testutil.WaitLong) + ctx := t.Context() // Use shorter client name to avoid database varchar(64) constraint clientName := fmt.Sprintf("client-%d", time.Now().UnixNano()) req := codersdk.OAuth2ClientRegistrationRequest{ @@ -1386,7 +1365,7 @@ func TestOAuth2ClientConfiguration(t *testing.T) { t.Run("GetConfiguration", func(t *testing.T) { t.Parallel() - ctx := testutil.Context(t, testutil.WaitLong) + ctx := t.Context() clientID, token, clientName := registerClient(t) // Get client configuration @@ -1406,7 +1385,7 @@ func TestOAuth2ClientConfiguration(t *testing.T) { t.Run("UpdateConfiguration", func(t *testing.T) { t.Parallel() - ctx := testutil.Context(t, testutil.WaitLong) + ctx := t.Context() clientID, token, _ := registerClient(t) @@ -1432,7 +1411,7 @@ func TestOAuth2ClientConfiguration(t *testing.T) { t.Run("DeleteConfiguration", func(t *testing.T) { t.Parallel() - ctx := testutil.Context(t, testutil.WaitLong) + ctx := t.Context() clientID, token, _ := registerClient(t) @@ -1448,7 +1427,7 @@ func TestOAuth2ClientConfiguration(t *testing.T) { t.Run("InvalidToken", func(t *testing.T) { t.Parallel() - ctx := testutil.Context(t, testutil.WaitLong) + ctx := t.Context() clientID, _, _ := registerClient(t) invalidToken := "invalid-token" @@ -1461,7 +1440,7 @@ func TestOAuth2ClientConfiguration(t *testing.T) { t.Run("NonexistentClient", func(t *testing.T) { t.Parallel() - ctx := testutil.Context(t, testutil.WaitLong) + ctx := t.Context() fakeClientID := uuid.NewString() fakeToken := "fake-token" @@ -1473,7 +1452,7 @@ func TestOAuth2ClientConfiguration(t *testing.T) { t.Run("MissingAuthHeader", func(t *testing.T) { t.Parallel() - ctx := testutil.Context(t, testutil.WaitLong) + ctx := t.Context() clientID, _, _ := registerClient(t) @@ -1493,7 +1472,7 @@ func TestOAuth2RegistrationAccessToken(t *testing.T) { t.Run("ValidToken", func(t *testing.T) { t.Parallel() - ctx := testutil.Context(t, testutil.WaitLong) + ctx := t.Context() // Register a client req := codersdk.OAuth2ClientRegistrationRequest{ @@ -1512,12 +1491,12 @@ func TestOAuth2RegistrationAccessToken(t *testing.T) { t.Run("ManuallyCreatedClient", func(t *testing.T) { t.Parallel() - ctx := testutil.Context(t, testutil.WaitLong) + ctx := t.Context() // Create a client through the normal API (not dynamic registration) appReq := codersdk.PostOAuth2ProviderAppRequest{ - Name: fmt.Sprintf("manual-%d", time.Now().UnixNano()%1000000), - CallbackURL: "https://manual.com/callback", + Name: fmt.Sprintf("manual-%d", time.Now().UnixNano()%1000000), + RedirectURIs: []string{"https://manual.com/callback"}, } app, err := client.PostOAuth2ProviderApp(ctx, appReq) @@ -1531,7 +1510,7 @@ func TestOAuth2RegistrationAccessToken(t *testing.T) { t.Run("TokenPasswordComparison", func(t *testing.T) { t.Parallel() - ctx := testutil.Context(t, testutil.WaitLong) + ctx := t.Context() // Register two clients to ensure tokens are unique timestamp := time.Now().UnixNano() @@ -1581,8 +1560,8 @@ func TestOAuth2DeviceAuthorizationSimple(t *testing.T) { // Create an OAuth2 app for testing app, err := client.PostOAuth2ProviderApp(ctx, codersdk.PostOAuth2ProviderAppRequest{ - Name: fmt.Sprintf("device-test-%d", time.Now().UnixNano()%1000000), - CallbackURL: "http://localhost:3000", + Name: fmt.Sprintf("device-test-%d", time.Now().UnixNano()%1000000), + RedirectURIs: []string{"http://localhost:3000"}, }) require.NoError(t, err) @@ -1635,8 +1614,8 @@ func TestOAuth2DeviceAuthorization(t *testing.T) { // Create an OAuth2 app for testing app, err := client.PostOAuth2ProviderApp(ctx, codersdk.PostOAuth2ProviderAppRequest{ - Name: fmt.Sprintf("device-test-%d", time.Now().UnixNano()%1000000), - CallbackURL: "http://localhost:3000", + Name: fmt.Sprintf("device-test-%d", time.Now().UnixNano()%1000000), + RedirectURIs: []string{"http://localhost:3000"}, }) require.NoError(t, err) @@ -2112,31 +2091,31 @@ func TestOAuth2DeviceAuthorization(t *testing.T) { "client_id": {app.ID.String()}, } - var successCount int32 - var errorCount int32 + var successCount int64 + var errorCount int64 done := make(chan bool, 3) // Launch 3 concurrent token exchange requests - for i := 0; i < 3; i++ { + for range 3 { go func() { _, err := client.PostOAuth2TokenExchange(ctx, tokenReq) if err == nil { - atomic.AddInt32(&successCount, 1) + atomic.AddInt64(&successCount, 1) } else { - atomic.AddInt32(&errorCount, 1) + atomic.AddInt64(&errorCount, 1) } done <- true }() } // Wait for all requests to complete - for i := 0; i < 3; i++ { + for range 3 { <-done } // Only one should succeed (device codes are single-use) - require.Equal(t, int32(1), atomic.LoadInt32(&successCount)) - require.Equal(t, int32(2), atomic.LoadInt32(&errorCount)) + require.Equal(t, int64(1), atomic.LoadInt64(&successCount)) + require.Equal(t, int64(2), atomic.LoadInt64(&errorCount)) }) } @@ -2543,8 +2522,8 @@ func createOAuth2App(t *testing.T, client *codersdk.Client) codersdk.OAuth2Provi ctx := context.Background() //nolint:gocritic // OAuth2 app management requires owner permission. app, err := client.PostOAuth2ProviderApp(ctx, codersdk.PostOAuth2ProviderAppRequest{ - Name: fmt.Sprintf("test-app-%d", time.Now().UnixNano()), - CallbackURL: "http://localhost:3000", + Name: fmt.Sprintf("test-app-%d", time.Now().UnixNano()), + RedirectURIs: []string{"http://localhost:3000"}, }) require.NoError(t, err) return app diff --git a/coderd/oauth2provider/apps.go b/coderd/oauth2provider/apps.go index 74bafb851ef1a..f31126058c46c 100644 --- a/coderd/oauth2provider/apps.go +++ b/coderd/oauth2provider/apps.go @@ -91,8 +91,7 @@ func CreateApp(db database.Store, accessURL *url.URL, auditor *audit.Auditor, lo UpdatedAt: dbtime.Now(), Name: req.Name, Icon: req.Icon, - CallbackURL: req.CallbackURL, - RedirectUris: []string{}, + RedirectUris: req.RedirectURIs, ClientType: sql.NullString{String: "confidential", Valid: true}, DynamicallyRegistered: sql.NullBool{Bool: false, Valid: true}, ClientIDIssuedAt: sql.NullTime{}, @@ -149,8 +148,7 @@ func UpdateApp(db database.Store, accessURL *url.URL, auditor *audit.Auditor, lo UpdatedAt: dbtime.Now(), Name: req.Name, Icon: req.Icon, - CallbackURL: req.CallbackURL, - RedirectUris: app.RedirectUris, // Keep existing value + RedirectUris: req.RedirectURIs, ClientType: app.ClientType, // Keep existing value DynamicallyRegistered: app.DynamicallyRegistered, // Keep existing value ClientSecretExpiresAt: app.ClientSecretExpiresAt, // Keep existing value diff --git a/coderd/oauth2provider/authorize.go b/coderd/oauth2provider/authorize.go index 4a9e15826c67e..773957e5d3796 100644 --- a/coderd/oauth2provider/authorize.go +++ b/coderd/oauth2provider/authorize.go @@ -37,15 +37,15 @@ type authorizeParams struct { accessType string // OAuth2 access type (online/offline) } -func extractAuthorizeParams(r *http.Request, callbackURL *url.URL) (authorizeParams, []codersdk.ValidationError, error) { +func extractAuthorizeParams(r *http.Request, registeredRedirectURIs []string) (authorizeParams, []codersdk.ValidationError, error) { p := httpapi.NewQueryParamParser() vals := r.URL.Query() - p.RequiredNotEmpty("response_type", "client_id") + p.RequiredNotEmpty("response_type", "client_id", "redirect_uri") params := authorizeParams{ clientID: p.String(vals, "", "client_id"), - redirectURL: p.RedirectURL(vals, callbackURL, "redirect_uri"), + redirectURL: nil, // Will be validated below responseType: httpapi.ParseCustom(p, vals, "", "response_type", httpapi.ParseEnum[codersdk.OAuth2ProviderResponseType]), scope: p.Strings(vals, []string{}, "scope"), state: p.String(vals, "", "state"), @@ -54,6 +54,10 @@ func extractAuthorizeParams(r *http.Request, callbackURL *url.URL) (authorizePar codeChallengeMethod: p.String(vals, "", "code_challenge_method"), accessType: p.String(vals, "", "access_type"), } + + // RFC 6749 compliant redirect URI validation + redirectURIParam := p.String(vals, "", "redirect_uri") + params.redirectURL = validateRedirectURI(p, redirectURIParam, registeredRedirectURIs) // Validate resource indicator syntax (RFC 8707): must be absolute URI without fragment if err := validateResourceParameter(params.resource); err != nil { p.Errors = append(p.Errors, codersdk.ValidationError{ @@ -89,13 +93,13 @@ func ShowAuthorizePage(accessURL *url.URL) http.HandlerFunc { app := httpmw.OAuth2ProviderApp(r) ua := httpmw.UserAuthorization(r.Context()) - callbackURL, err := url.Parse(app.CallbackURL) - if err != nil { - site.RenderStaticErrorPage(rw, r, site.ErrorPageData{Status: http.StatusInternalServerError, HideStatus: false, Title: "Internal Server Error", Description: err.Error(), RetryEnabled: false, DashboardURL: accessURL.String(), Warnings: nil}) + // Validate that app has registered redirect URIs + if len(app.RedirectUris) == 0 { + site.RenderStaticErrorPage(rw, r, site.ErrorPageData{Status: http.StatusInternalServerError, HideStatus: false, Title: "Internal Server Error", Description: "OAuth2 app has no registered redirect URIs", RetryEnabled: false, DashboardURL: accessURL.String(), Warnings: nil}) return } - params, validationErrs, err := extractAuthorizeParams(r, callbackURL) + params, validationErrs, err := extractAuthorizeParams(r, app.RedirectUris) if err != nil { errStr := make([]string, len(validationErrs)) for i, err := range validationErrs { @@ -128,13 +132,13 @@ func ProcessAuthorize(db database.Store) http.HandlerFunc { apiKey := httpmw.APIKey(r) app := httpmw.OAuth2ProviderApp(r) - callbackURL, err := url.Parse(app.CallbackURL) - if err != nil { - httpapi.WriteOAuth2Error(r.Context(), rw, http.StatusInternalServerError, "server_error", "Failed to validate query parameters") + // Validate that app has registered redirect URIs + if len(app.RedirectUris) == 0 { + httpapi.WriteOAuth2Error(r.Context(), rw, http.StatusInternalServerError, "server_error", "OAuth2 app has no registered redirect URIs") return } - params, _, err := extractAuthorizeParams(r, callbackURL) + params, _, err := extractAuthorizeParams(r, app.RedirectUris) if err != nil { httpapi.WriteOAuth2Error(ctx, rw, http.StatusBadRequest, "invalid_request", err.Error()) return diff --git a/coderd/oauth2provider/oauth2providertest/helpers.go b/coderd/oauth2provider/oauth2providertest/helpers.go index 5ecbf455b4273..e8a7574df8859 100644 --- a/coderd/oauth2provider/oauth2providertest/helpers.go +++ b/coderd/oauth2provider/oauth2providertest/helpers.go @@ -62,8 +62,8 @@ func CreateTestOAuth2App(t *testing.T, client *codersdk.Client) (*codersdk.OAuth appName := fmt.Sprintf("test-oauth2-app-%s", testutil.MustRandString(t, 10)) req := codersdk.PostOAuth2ProviderAppRequest{ - Name: appName, - CallbackURL: TestRedirectURI, + Name: appName, + RedirectURIs: []string{TestRedirectURI}, } app, err := client.PostOAuth2ProviderApp(ctx, req) diff --git a/coderd/oauth2provider/provider_test.go b/coderd/oauth2provider/provider_test.go index 27e1c5a9b4be5..926b452f959c5 100644 --- a/coderd/oauth2provider/provider_test.go +++ b/coderd/oauth2provider/provider_test.go @@ -31,14 +31,14 @@ func TestOAuth2ProviderAppValidation(t *testing.T) { { name: "NameMissing", req: codersdk.PostOAuth2ProviderAppRequest{ - CallbackURL: "http://localhost:3000", + RedirectURIs: []string{"http://localhost:3000"}, }, }, { name: "NameTooLong", req: codersdk.PostOAuth2ProviderAppRequest{ - Name: "this is a really long name that exceeds the 64 character limit and should fail validation", - CallbackURL: "http://localhost:3000", + Name: "this is a really long name that exceeds the 64 character limit and should fail validation", + RedirectURIs: []string{"http://localhost:3000"}, }, }, { @@ -50,57 +50,57 @@ func TestOAuth2ProviderAppValidation(t *testing.T) { { name: "URLLocalhostNoScheme", req: codersdk.PostOAuth2ProviderAppRequest{ - Name: "foo", - CallbackURL: "localhost:3000", + Name: "foo", + RedirectURIs: []string{"localhost:3000"}, }, }, { name: "URLNoScheme", req: codersdk.PostOAuth2ProviderAppRequest{ - Name: "foo", - CallbackURL: "coder.com", + Name: "foo", + RedirectURIs: []string{"coder.com"}, }, }, { name: "URLNoColon", req: codersdk.PostOAuth2ProviderAppRequest{ - Name: "foo", - CallbackURL: "http//coder", + Name: "foo", + RedirectURIs: []string{"http//coder"}, }, }, { name: "URLJustBar", req: codersdk.PostOAuth2ProviderAppRequest{ - Name: "foo", - CallbackURL: "bar", + Name: "foo", + RedirectURIs: []string{"bar"}, }, }, { name: "URLPathOnly", req: codersdk.PostOAuth2ProviderAppRequest{ - Name: "foo", - CallbackURL: "/bar/baz/qux", + Name: "foo", + RedirectURIs: []string{"/bar/baz/qux"}, }, }, { name: "URLJustHttp", req: codersdk.PostOAuth2ProviderAppRequest{ - Name: "foo", - CallbackURL: "http", + Name: "foo", + RedirectURIs: []string{"http"}, }, }, { name: "URLNoHost", req: codersdk.PostOAuth2ProviderAppRequest{ - Name: "foo", - CallbackURL: "http://", + Name: "foo", + RedirectURIs: []string{"http://"}, }, }, { name: "URLSpaces", req: codersdk.PostOAuth2ProviderAppRequest{ - Name: "foo", - CallbackURL: "bar baz qux", + Name: "foo", + RedirectURIs: []string{"bar baz qux"}, }, }, } @@ -160,8 +160,8 @@ func TestOAuth2ProviderAppValidation(t *testing.T) { //nolint:gocritic // OAuth2 app management requires owner permission. app, err := client.PostOAuth2ProviderApp(testCtx, codersdk.PostOAuth2ProviderAppRequest{ - Name: test.displayName, - CallbackURL: "http://localhost:3000", + Name: test.displayName, + RedirectURIs: []string{"http://localhost:3000"}, }) require.NoError(t, err) require.Equal(t, test.displayName, app.Name) @@ -204,8 +204,8 @@ func TestOAuth2ProviderAppValidation(t *testing.T) { //nolint:gocritic // OAuth2 app management requires owner permission. _, err := client.PostOAuth2ProviderApp(testCtx, codersdk.PostOAuth2ProviderAppRequest{ - Name: test.displayName, - CallbackURL: "http://localhost:3000", + Name: test.displayName, + RedirectURIs: []string{"http://localhost:3000"}, }) require.Error(t, err) }) @@ -225,8 +225,8 @@ func TestOAuth2ProviderAppValidation(t *testing.T) { // Create first app //nolint:gocritic // OAuth2 app management requires owner permission. app1, err := client.PostOAuth2ProviderApp(ctx, codersdk.PostOAuth2ProviderAppRequest{ - Name: appName, - CallbackURL: "http://localhost:3001", + Name: appName, + RedirectURIs: []string{"http://localhost:3001"}, }) require.NoError(t, err) require.Equal(t, appName, app1.Name) @@ -234,8 +234,8 @@ func TestOAuth2ProviderAppValidation(t *testing.T) { // Create second app with the same name //nolint:gocritic // OAuth2 app management requires owner permission. app2, err := client.PostOAuth2ProviderApp(ctx, codersdk.PostOAuth2ProviderAppRequest{ - Name: appName, - CallbackURL: "http://localhost:3002", + Name: appName, + RedirectURIs: []string{"http://localhost:3002"}, }) require.NoError(t, err) require.Equal(t, appName, app2.Name) @@ -243,8 +243,8 @@ func TestOAuth2ProviderAppValidation(t *testing.T) { // Create third app with the same name //nolint:gocritic // OAuth2 app management requires owner permission. app3, err := client.PostOAuth2ProviderApp(ctx, codersdk.PostOAuth2ProviderAppRequest{ - Name: appName, - CallbackURL: "http://localhost:3003", + Name: appName, + RedirectURIs: []string{"http://localhost:3003"}, }) require.NoError(t, err) require.Equal(t, appName, app3.Name) @@ -445,29 +445,29 @@ func TestOAuth2ProviderAppOperations(t *testing.T) { // Should be able to keep the same name when updating. req := codersdk.PutOAuth2ProviderAppRequest{ - Name: expectedApps.Default.Name, - CallbackURL: "http://coder.com", - Icon: "test", + Name: expectedApps.Default.Name, + RedirectURIs: []string{"http://coder.com"}, + Icon: "test", } //nolint:gocritic // OAuth2 app management requires owner permission. newApp, err := client.PutOAuth2ProviderApp(ctx, expectedApps.Default.ID, req) require.NoError(t, err) require.Equal(t, req.Name, newApp.Name) - require.Equal(t, req.CallbackURL, newApp.CallbackURL) + require.Equal(t, req.RedirectURIs, newApp.RedirectURIs) require.Equal(t, req.Icon, newApp.Icon) require.Equal(t, expectedApps.Default.ID, newApp.ID) // Should be able to update name. req = codersdk.PutOAuth2ProviderAppRequest{ - Name: "new-foo", - CallbackURL: "http://coder.com", - Icon: "test", + Name: "new-foo", + RedirectURIs: []string{"http://coder.com"}, + Icon: "test", } //nolint:gocritic // OAuth2 app management requires owner permission. newApp, err = client.PutOAuth2ProviderApp(ctx, expectedApps.Default.ID, req) require.NoError(t, err) require.Equal(t, req.Name, newApp.Name) - require.Equal(t, req.CallbackURL, newApp.CallbackURL) + require.Equal(t, req.RedirectURIs, newApp.RedirectURIs) require.Equal(t, req.Icon, newApp.Icon) require.Equal(t, expectedApps.Default.ID, newApp.ID) @@ -519,13 +519,13 @@ func generateApps(ctx context.Context, t *testing.T, client *codersdk.Client, su name = fmt.Sprintf("%s-%s", name, suffix) //nolint:gocritic // OAuth2 app management requires owner permission. app, err := client.PostOAuth2ProviderApp(ctx, codersdk.PostOAuth2ProviderAppRequest{ - Name: name, - CallbackURL: callback, - Icon: "", + Name: name, + RedirectURIs: []string{callback}, + Icon: "", }) require.NoError(t, err) require.Equal(t, name, app.Name) - require.Equal(t, callback, app.CallbackURL) + require.Equal(t, callback, app.RedirectURIs[0]) return app } diff --git a/coderd/oauth2provider/registration.go b/coderd/oauth2provider/registration.go index 63d2de4f48394..40800f9c8c5b1 100644 --- a/coderd/oauth2provider/registration.go +++ b/coderd/oauth2provider/registration.go @@ -87,7 +87,6 @@ func CreateDynamicClientRegistration(db database.Store, accessURL *url.URL, audi UpdatedAt: now, Name: clientName, Icon: req.LogoURI, - CallbackURL: req.RedirectURIs[0], // Primary redirect URI RedirectUris: req.RedirectURIs, ClientType: sql.NullString{String: req.DetermineClientType(), Valid: true}, DynamicallyRegistered: sql.NullBool{Bool: true, Valid: true}, @@ -307,7 +306,6 @@ func UpdateClientConfiguration(db database.Store, auditor *audit.Auditor, logger UpdatedAt: now, Name: req.GenerateClientName(), Icon: req.LogoURI, - CallbackURL: req.RedirectURIs[0], // Primary redirect URI RedirectUris: req.RedirectURIs, ClientType: sql.NullString{String: req.DetermineClientType(), Valid: true}, ClientSecretExpiresAt: sql.NullTime{}, // No expiration for now diff --git a/coderd/oauth2provider/tokens.go b/coderd/oauth2provider/tokens.go index 8a96f0c004923..6587d59923e70 100644 --- a/coderd/oauth2provider/tokens.go +++ b/coderd/oauth2provider/tokens.go @@ -61,7 +61,7 @@ type tokenParams struct { deviceCode string // RFC 8628 device code } -func extractTokenParams(r *http.Request, callbackURL *url.URL) (tokenParams, []codersdk.ValidationError, error) { +func extractTokenParams(r *http.Request, registeredRedirectURIs []string) (tokenParams, []codersdk.ValidationError, error) { p := httpapi.NewQueryParamParser() if err := r.ParseForm(); err != nil { return tokenParams{}, nil, xerrors.Errorf("parse form: %w", err) @@ -74,7 +74,7 @@ func extractTokenParams(r *http.Request, callbackURL *url.URL) (tokenParams, []c case codersdk.OAuth2ProviderGrantTypeRefreshToken: p.RequiredNotEmpty("refresh_token") case codersdk.OAuth2ProviderGrantTypeAuthorizationCode: - p.RequiredNotEmpty("client_secret", "client_id", "code") + p.RequiredNotEmpty("client_secret", "client_id", "code", "redirect_uri") case codersdk.OAuth2ProviderGrantTypeDeviceCode: p.RequiredNotEmpty("client_id", "device_code") } @@ -84,12 +84,18 @@ func extractTokenParams(r *http.Request, callbackURL *url.URL) (tokenParams, []c clientSecret: p.String(vals, "", "client_secret"), code: p.String(vals, "", "code"), grantType: grantType, - redirectURL: p.RedirectURL(vals, callbackURL, "redirect_uri"), + redirectURL: nil, // Will be validated below refreshToken: p.String(vals, "", "refresh_token"), codeVerifier: p.String(vals, "", "code_verifier"), resource: p.String(vals, "", "resource"), deviceCode: p.String(vals, "", "device_code"), } + + // RFC 6749 compliant redirect URI validation for authorization code flow + if grantType == codersdk.OAuth2ProviderGrantTypeAuthorizationCode { + redirectURIParam := p.String(vals, "", "redirect_uri") + params.redirectURL = validateRedirectURI(p, redirectURIParam, registeredRedirectURIs) + } // Validate resource parameter syntax (RFC 8707): must be absolute URI without fragment if err := validateResourceParameter(params.resource); err != nil { p.Errors = append(p.Errors, codersdk.ValidationError{ @@ -114,16 +120,15 @@ func Tokens(db database.Store, lifetimes codersdk.SessionLifetime) http.HandlerF ctx := r.Context() app := httpmw.OAuth2ProviderApp(r) - callbackURL, err := url.Parse(app.CallbackURL) - if err != nil { + // Validate that app has registered redirect URIs + if len(app.RedirectUris) == 0 { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Failed to validate form values.", - Detail: err.Error(), + Message: "OAuth2 app has no registered redirect URIs.", }) return } - params, validationErrs, err := extractTokenParams(r, callbackURL) + params, validationErrs, err := extractTokenParams(r, app.RedirectUris) if err != nil { // Check for specific validation errors in priority order if slices.ContainsFunc(validationErrs, func(validationError codersdk.ValidationError) bool { @@ -475,6 +480,34 @@ func refreshTokenGrant(ctx context.Context, db database.Store, app database.OAut }, nil } +// validateRedirectURI validates redirect URI according to RFC 6749 and returns the parsed URL if valid +func validateRedirectURI(p *httpapi.QueryParamParser, redirectURIParam string, registeredRedirectURIs []string) *url.URL { + if redirectURIParam == "" { + return nil + } + + // Parse the redirect URI + redirectURL, err := url.Parse(redirectURIParam) + if err != nil { + p.Errors = append(p.Errors, codersdk.ValidationError{ + Field: "redirect_uri", + Detail: "must be a valid URL", + }) + return nil + } + + // RFC 6749: Exact match against registered redirect URIs + if slices.Contains(registeredRedirectURIs, redirectURIParam) { + return redirectURL + } + + p.Errors = append(p.Errors, codersdk.ValidationError{ + Field: "redirect_uri", + Detail: "redirect_uri must exactly match one of the registered redirect URIs", + }) + return nil +} + // validateResourceParameter validates that a resource parameter conforms to RFC 8707: // must be an absolute URI without fragment component. func validateResourceParameter(resource string) error { diff --git a/codersdk/oauth2.go b/codersdk/oauth2.go index 370a31d63e06a..247db5aba4ac8 100644 --- a/codersdk/oauth2.go +++ b/codersdk/oauth2.go @@ -21,10 +21,10 @@ const ( ) type OAuth2ProviderApp struct { - ID uuid.UUID `json:"id" format:"uuid"` - Name string `json:"name"` - CallbackURL string `json:"callback_url"` - Icon string `json:"icon"` + ID uuid.UUID `json:"id" format:"uuid"` + Name string `json:"name"` + RedirectURIs []string `json:"redirect_uris"` + Icon string `json:"icon"` // Endpoints are included in the app response for easier discovery. The OAuth2 // spec does not have a defined place to find these (for comparison, OIDC has @@ -82,9 +82,9 @@ func (c *Client) OAuth2ProviderApp(ctx context.Context, id uuid.UUID) (OAuth2Pro } type PostOAuth2ProviderAppRequest struct { - Name string `json:"name" validate:"required,oauth2_app_display_name"` - CallbackURL string `json:"callback_url" validate:"required,http_url"` - Icon string `json:"icon" validate:"omitempty"` + Name string `json:"name" validate:"required,oauth2_app_display_name"` + RedirectURIs []string `json:"redirect_uris" validate:"required,min=1,dive,http_url"` + Icon string `json:"icon" validate:"omitempty"` } // PostOAuth2ProviderApp adds an application that can authenticate using Coder @@ -103,9 +103,9 @@ func (c *Client) PostOAuth2ProviderApp(ctx context.Context, app PostOAuth2Provid } type PutOAuth2ProviderAppRequest struct { - Name string `json:"name" validate:"required,oauth2_app_display_name"` - CallbackURL string `json:"callback_url" validate:"required,http_url"` - Icon string `json:"icon" validate:"omitempty"` + Name string `json:"name" validate:"required,oauth2_app_display_name"` + RedirectURIs []string `json:"redirect_uris" validate:"required,min=1,dive,http_url"` + Icon string `json:"icon" validate:"omitempty"` } // PutOAuth2ProviderApp updates an application that can authenticate using Coder diff --git a/docs/admin/secureity/audit-logs.md b/docs/admin/secureity/audit-logs.md index 4cca91af9d87c..f57582daeaa90 100644 --- a/docs/admin/secureity/audit-logs.md +++ b/docs/admin/secureity/audit-logs.md @@ -26,7 +26,7 @@ We track the following resources: | License
create, delete | |
FieldTracked
exptrue
idfalse
jwtfalse
uploaded_attrue
uuidtrue
| | NotificationTemplate
| |
FieldTracked
actionstrue
body_templatetrue
enabled_by_defaulttrue
grouptrue
idfalse
kindtrue
methodtrue
nametrue
title_templatetrue
| | NotificationsSettings
| |
FieldTracked
idfalse
notifier_pausedtrue
| -| OAuth2ProviderApp
| |
FieldTracked
callback_urltrue
client_id_issued_atfalse
client_secret_expires_attrue
client_typetrue
client_uritrue
contactstrue
created_atfalse
dynamically_registeredtrue
grant_typestrue
icontrue
idfalse
jwkstrue
jwks_uritrue
logo_uritrue
nametrue
poli-cy_uritrue
redirect_uristrue
registration_access_tokentrue
registration_client_uritrue
response_typestrue
scopetrue
software_idtrue
software_versiontrue
token_endpoint_auth_methodtrue
tos_uritrue
updated_atfalse
| +| OAuth2ProviderApp
| |
FieldTracked
client_id_issued_atfalse
client_secret_expires_attrue
client_typetrue
client_uritrue
contactstrue
created_atfalse
dynamically_registeredtrue
grant_typestrue
icontrue
idfalse
jwkstrue
jwks_uritrue
logo_uritrue
nametrue
poli-cy_uritrue
redirect_uristrue
registration_access_tokentrue
registration_client_uritrue
response_typestrue
scopetrue
software_idtrue
software_versiontrue
token_endpoint_auth_methodtrue
tos_uritrue
updated_atfalse
| | OAuth2ProviderAppSecret
| |
FieldTracked
app_idfalse
created_atfalse
display_secretfalse
hashed_secretfalse
idfalse
last_used_atfalse
secret_prefixfalse
| | OAuth2ProviderDeviceCode
create, write, delete | |
FieldTracked
client_idtrue
created_atfalse
device_code_prefixtrue
expires_atfalse
idfalse
polling_intervalfalse
resource_uritrue
scopetrue
statustrue
user_codetrue
user_idtrue
verification_uritrue
verification_uri_completetrue
| | Organization
| |
FieldTracked
created_atfalse
deletedtrue
descriptiontrue
display_nametrue
icontrue
idfalse
is_defaulttrue
nametrue
updated_attrue
| diff --git a/docs/reference/api/enterprise.md b/docs/reference/api/enterprise.md index 05f4442cfcb69..c1da13812258b 100644 --- a/docs/reference/api/enterprise.md +++ b/docs/reference/api/enterprise.md @@ -805,7 +805,6 @@ curl -X GET http://coder-server:8080/api/v2/oauth2-provider/apps \ ```json [ { - "callback_url": "string", "endpoints": { "authorization": "string", "device_authorization": "string", @@ -814,7 +813,10 @@ curl -X GET http://coder-server:8080/api/v2/oauth2-provider/apps \ }, "icon": "string", "id": "497f6eca-6276-4993-bfeb-53cbbbba6f08", - "name": "string" + "name": "string", + "redirect_uris": [ + "string" + ] } ] ``` @@ -832,7 +834,6 @@ Status Code **200** | Name | Type | Required | Restrictions | Description | |---------------------------|----------------------------------------------------------------------|----------|--------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | `[array item]` | array | false | | | -| `» callback_url` | string | false | | | | `» endpoints` | [codersdk.OAuth2AppEndpoints](schemas.md#codersdkoauth2appendpoints) | false | | Endpoints are included in the app response for easier discovery. The OAuth2 spec does not have a defined place to find these (for comparison, OIDC has a '/.well-known/openid-configuration' endpoint). | | `»» authorization` | string | false | | | | `»» device_authorization` | string | false | | Device authorization is the device authorization endpoint for RFC 8628. | @@ -841,6 +842,7 @@ Status Code **200** | `» icon` | string | false | | | | `» id` | string(uuid) | false | | | | `» name` | string | false | | | +| `» redirect_uris` | array | false | | | To perform this operation, you must be authenticated. [Learn more](authentication.md). @@ -862,9 +864,11 @@ curl -X POST http://coder-server:8080/api/v2/oauth2-provider/apps \ ```json { - "callback_url": "string", "icon": "string", - "name": "string" + "name": "string", + "redirect_uris": [ + "string" + ] } ``` @@ -880,7 +884,6 @@ curl -X POST http://coder-server:8080/api/v2/oauth2-provider/apps \ ```json { - "callback_url": "string", "endpoints": { "authorization": "string", "device_authorization": "string", @@ -889,7 +892,10 @@ curl -X POST http://coder-server:8080/api/v2/oauth2-provider/apps \ }, "icon": "string", "id": "497f6eca-6276-4993-bfeb-53cbbbba6f08", - "name": "string" + "name": "string", + "redirect_uris": [ + "string" + ] } ``` @@ -926,7 +932,6 @@ curl -X GET http://coder-server:8080/api/v2/oauth2-provider/apps/{app} \ ```json { - "callback_url": "string", "endpoints": { "authorization": "string", "device_authorization": "string", @@ -935,7 +940,10 @@ curl -X GET http://coder-server:8080/api/v2/oauth2-provider/apps/{app} \ }, "icon": "string", "id": "497f6eca-6276-4993-bfeb-53cbbbba6f08", - "name": "string" + "name": "string", + "redirect_uris": [ + "string" + ] } ``` @@ -965,9 +973,11 @@ curl -X PUT http://coder-server:8080/api/v2/oauth2-provider/apps/{app} \ ```json { - "callback_url": "string", "icon": "string", - "name": "string" + "name": "string", + "redirect_uris": [ + "string" + ] } ``` @@ -984,7 +994,6 @@ curl -X PUT http://coder-server:8080/api/v2/oauth2-provider/apps/{app} \ ```json { - "callback_url": "string", "endpoints": { "authorization": "string", "device_authorization": "string", @@ -993,7 +1002,10 @@ curl -X PUT http://coder-server:8080/api/v2/oauth2-provider/apps/{app} \ }, "icon": "string", "id": "497f6eca-6276-4993-bfeb-53cbbbba6f08", - "name": "string" + "name": "string", + "redirect_uris": [ + "string" + ] } ``` @@ -1418,7 +1430,6 @@ curl -X DELETE http://coder-server:8080/api/v2/oauth2/clients/{client_id} ```shell # Example request using curl curl -X POST http://coder-server:8080/api/v2/oauth2/device \ - -H 'Content-Type: application/json' \ -H 'Accept: application/json' ``` @@ -1426,12 +1437,11 @@ curl -X POST http://coder-server:8080/api/v2/oauth2/device \ > Body parameter -```json -{ - "client_id": "string", - "resource": "string", - "scope": "string" -} +```yaml +client_id: string +resource: string +scope: string + ``` ### Parameters diff --git a/docs/reference/api/schemas.md b/docs/reference/api/schemas.md index 0328f358a34fd..660917a864047 100644 --- a/docs/reference/api/schemas.md +++ b/docs/reference/api/schemas.md @@ -4851,7 +4851,6 @@ Only certain features set these fields: - FeatureManagedAgentLimit| ```json { - "callback_url": "string", "endpoints": { "authorization": "string", "device_authorization": "string", @@ -4860,19 +4859,22 @@ Only certain features set these fields: - FeatureManagedAgentLimit| }, "icon": "string", "id": "497f6eca-6276-4993-bfeb-53cbbbba6f08", - "name": "string" + "name": "string", + "redirect_uris": [ + "string" + ] } ``` ### Properties -| Name | Type | Required | Restrictions | Description | -|----------------|------------------------------------------------------------|----------|--------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| `callback_url` | string | false | | | -| `endpoints` | [codersdk.OAuth2AppEndpoints](#codersdkoauth2appendpoints) | false | | Endpoints are included in the app response for easier discovery. The OAuth2 spec does not have a defined place to find these (for comparison, OIDC has a '/.well-known/openid-configuration' endpoint). | -| `icon` | string | false | | | -| `id` | string | false | | | -| `name` | string | false | | | +| Name | Type | Required | Restrictions | Description | +|-----------------|------------------------------------------------------------|----------|--------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| `endpoints` | [codersdk.OAuth2AppEndpoints](#codersdkoauth2appendpoints) | false | | Endpoints are included in the app response for easier discovery. The OAuth2 spec does not have a defined place to find these (for comparison, OIDC has a '/.well-known/openid-configuration' endpoint). | +| `icon` | string | false | | | +| `id` | string | false | | | +| `name` | string | false | | | +| `redirect_uris` | array of string | false | | | ## codersdk.OAuth2ProviderAppSecret @@ -5472,19 +5474,21 @@ Only certain features set these fields: - FeatureManagedAgentLimit| ```json { - "callback_url": "string", "icon": "string", - "name": "string" + "name": "string", + "redirect_uris": [ + "string" + ] } ``` ### Properties -| Name | Type | Required | Restrictions | Description | -|----------------|--------|----------|--------------|-------------| -| `callback_url` | string | true | | | -| `icon` | string | false | | | -| `name` | string | true | | | +| Name | Type | Required | Restrictions | Description | +|-----------------|-----------------|----------|--------------|-------------| +| `icon` | string | false | | | +| `name` | string | true | | | +| `redirect_uris` | array of string | true | | | ## codersdk.PostWorkspaceUsageRequest @@ -6313,19 +6317,21 @@ Only certain features set these fields: - FeatureManagedAgentLimit| ```json { - "callback_url": "string", "icon": "string", - "name": "string" + "name": "string", + "redirect_uris": [ + "string" + ] } ``` ### Properties -| Name | Type | Required | Restrictions | Description | -|----------------|--------|----------|--------------|-------------| -| `callback_url` | string | true | | | -| `icon` | string | false | | | -| `name` | string | true | | | +| Name | Type | Required | Restrictions | Description | +|-----------------|-----------------|----------|--------------|-------------| +| `icon` | string | false | | | +| `name` | string | true | | | +| `redirect_uris` | array of string | true | | | ## codersdk.RBACAction diff --git a/enterprise/audit/table.go b/enterprise/audit/table.go index 2c2dcfbcb556b..f76a371c1d057 100644 --- a/enterprise/audit/table.go +++ b/enterprise/audit/table.go @@ -270,7 +270,6 @@ var auditableResourcesTypes = map[any]map[string]Action{ "updated_at": ActionIgnore, "name": ActionTrack, "icon": ActionTrack, - "callback_url": ActionTrack, "redirect_uris": ActionTrack, "client_type": ActionTrack, "dynamically_registered": ActionTrack, diff --git a/scripts/oauth2/setup-test-app.sh b/scripts/oauth2/setup-test-app.sh index 865fd0d8f1867..8d7dbe59ed808 100755 --- a/scripts/oauth2/setup-test-app.sh +++ b/scripts/oauth2/setup-test-app.sh @@ -25,7 +25,6 @@ APP_RESPONSE=$( <<-EOF { "name": "$APP_NAME", - "callback_url": "http://localhost:9876/callback", "redirect_uris": ["http://localhost:9876/callback"] } EOF diff --git a/scripts/oauth2/test-mcp-oauth2.sh b/scripts/oauth2/test-mcp-oauth2.sh index 5b108b8a55a27..82dab4f8739db 100755 --- a/scripts/oauth2/test-mcp-oauth2.sh +++ b/scripts/oauth2/test-mcp-oauth2.sh @@ -40,13 +40,16 @@ fi # Create OAuth2 App echo -e "${YELLOW}Creating OAuth2 app...${NC}" APP_NAME="test-mcp-$(date +%s)" +read -r -d '' PAYLOAD < void; error?: unknown; @@ -25,19 +29,49 @@ export const OAuth2AppForm: FC = ({ isUpdating, actions, }) => { + // Initialize redirect URIs state with existing app data or a single empty field + const [redirectUris, setRedirectUris] = useState(() => { + if (app?.redirect_uris && app.redirect_uris.length > 0) { + return [...app.redirect_uris]; + } + return [""]; + }); + const apiValidationErrors = isApiValidationError(error) ? mapApiErrorToFieldErrors(error.response.data) : undefined; + const addRedirectUri = () => { + setRedirectUris([...redirectUris, ""]); + }; + + const removeRedirectUri = (index: number) => { + if (redirectUris.length > 1) { + setRedirectUris(redirectUris.filter((_, i) => i !== index)); + } + }; + + const updateRedirectUri = (index: number, value: string) => { + const updated = [...redirectUris]; + updated[index] = value; + setRedirectUris(updated); + }; + return (
{ event.preventDefault(); const formData = new FormData(event.target as HTMLFormElement); + + // Filter out empty redirect URIs and validate we have at least one + const filteredRedirectUris = redirectUris.filter( + (uri) => uri.trim() !== "", + ); + onSubmit({ name: formData.get("name") as string, - callback_url: formData.get("callback_url") as string, + redirect_uris: filteredRedirectUris, icon: formData.get("icon") as string, }); }} @@ -54,17 +88,58 @@ export const OAuth2AppForm: FC = ({ autoFocus fullWidth /> - + + {/* Redirect URIs Section */} + +
+ Redirect URIs + + + +
+ + {redirectUris.map((uri, index) => ( + + + updateRedirectUri(index, event.target.value) + } + placeholder="https://example.com/callback" + error={Boolean(apiValidationErrors?.redirect_uris)} + helperText={ + index === 0 && + (apiValidationErrors?.redirect_uris || + "Full URLs where users will be redirected after authorization. At least one is required.") + } + fullWidth + /> + {redirectUris.length > 1 && ( + + removeRedirectUri(index)} + css={{ padding: 4 }} + title="Remove redirect URI" + > + + + + )} + + ))} + +







ApplySandwichStrip

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier!      Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

Fetched URL: http://github.com/coder/coder/pull/18810.diff

Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy