mirror of
https://github.com/coder/coder.git
synced 2026-09-21 20:51:01 +08:00
feat: constrain the OAuth2 client type column (#27931)
Extracted from #27873 so the schema change can be reviewed for migration safety on its own. #27873 will rebase onto this. `client_type` decides whether the token endpoint validates a client secret at all, and the column accepts any text: nullable, no `CHECK`, no enum. No Go path can write a bad value today, and `IsPublic` fails closed on anything unrecognized, so the read side is safe. What the schema still permits is the problem: a future migration writing `'public'` onto a row that holds a secret turns off client authentication for that app with nothing to catch it, no constraint, no log, no audit entry, no test. `000565` adds `CHECK (client_type IN ('confidential', 'public'))` and `NOT NULL`. The `UPDATE` ahead of it should touch zero rows, since migration `000344` added the column with a default of `'confidential'` and backfilled with `COALESCE`; it is there so `SET NOT NULL` cannot fail on an unexpected row. Both `ALTER`s take `ACCESS EXCLUSIVE` and scan a table holding one row per registered OAuth2 client, so the lock is brief. ## The second migration, and why it aligns the way it does Two columns describe the same fact and can currently contradict each other. `token_endpoint_auth_method` is the client's own declaration: registered client metadata under RFC 7591 §2, where `"none"` is defined to mean the client is public and has no secret. `client_type` is Coder's derived copy, and it is what the token endpoint enforces on. RFC 7591 defines no `client_type` metadata field; the column exists only as a denormalization. Registration used to persist the declaration verbatim while hardcoding `client_type` to `'confidential'`, so rows exist declaring `"none"` on a client stored confidential that was issued, and still requires, a real secret. A client that reads its own metadata and believes it is public will drop that secret and stop being able to exchange codes. `000566` aligns the declaration to what is enforced, not the reverse. Deriving enforcement from the declaration would reclassify every such client as public and stop requiring the secret it holds, which is a silent authentication downgrade. The down migration is deliberately empty: the previous values are not recorded, and restoring them would only reinstate metadata that tells a client to authenticate in a way the server rejects. ## Application changes `SET NOT NULL` changes the generated field from `sql.NullString` to `string`, so the three write sites are updated to match. That is the entire application diff and no behavior depends on it. Refs https://linear.app/codercom/issue/ENG-3029/oauth2-support-public-client --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
2f34e1abd0
commit
16c58770f8
Generated
+3
-2
@@ -2652,7 +2652,7 @@ CREATE TABLE oauth2_provider_apps (
|
||||
icon character varying(256) NOT NULL,
|
||||
callback_url text NOT NULL,
|
||||
redirect_uris text[],
|
||||
client_type text DEFAULT 'confidential'::text,
|
||||
client_type text DEFAULT 'confidential'::text NOT NULL,
|
||||
dynamically_registered boolean DEFAULT false,
|
||||
client_id_issued_at timestamp with time zone DEFAULT now(),
|
||||
client_secret_expires_at timestamp with time zone,
|
||||
@@ -2670,7 +2670,8 @@ CREATE TABLE oauth2_provider_apps (
|
||||
software_id text,
|
||||
software_version text,
|
||||
registration_access_token bytea,
|
||||
registration_client_uri text
|
||||
registration_client_uri text,
|
||||
CONSTRAINT oauth2_provider_apps_client_type_check CHECK ((client_type = ANY (ARRAY['confidential'::text, 'public'::text])))
|
||||
);
|
||||
|
||||
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.';
|
||||
|
||||
Reference in New Issue
Block a user