Conversation
There was a problem hiding this comment.
Pull request overview
Adds database support for an OAuth 2.0 PKCE flow by introducing tables for authorization codes, tokens, and registered redirect URIs.
Changes:
- Add
oauth_authorization_codes,oauth_tokens, andoauth_redirect_uristables plus supporting indexes. - Update the schema dump (
sql/01_schema.sql) to reflect the new OAuth tables. - (Unrelated) Add
block_timestampcolumn tosol_purchasesin the schema dump.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
sql/01_schema.sql |
Schema dump updated with new OAuth tables/indexes; also includes an unrelated sol_purchases.block_timestamp change. |
ddl/migrations/0190_oauth_pkce.sql |
New migration creating OAuth PKCE tables and indexes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| -- oauth_authorization_codes: short-lived (10 min), one-time-use authorization codes for PKCE flow | ||
| CREATE TABLE IF NOT EXISTS oauth_authorization_codes ( | ||
| code VARCHAR(255) NOT NULL PRIMARY KEY, | ||
| client_id VARCHAR(255) NOT NULL, | ||
| user_id INTEGER NOT NULL, | ||
| redirect_uri TEXT NOT NULL, | ||
| code_challenge VARCHAR(255) NOT NULL, | ||
| code_challenge_method VARCHAR(10) NOT NULL DEFAULT 'S256', | ||
| scope VARCHAR(50) NOT NULL, | ||
| expires_at TIMESTAMPTZ NOT NULL DEFAULT (NOW() + INTERVAL '10 minutes'), | ||
| used BOOLEAN NOT NULL DEFAULT false | ||
| ); | ||
|
|
||
| CREATE INDEX IF NOT EXISTS idx_oauth_authorization_codes_client_id ON oauth_authorization_codes(client_id); | ||
| CREATE INDEX IF NOT EXISTS idx_oauth_authorization_codes_expires_used ON oauth_authorization_codes(expires_at, used); | ||
|
|
||
| -- oauth_tokens: opaque access and refresh tokens for PKCE flow | ||
| CREATE TABLE IF NOT EXISTS oauth_tokens ( | ||
| token VARCHAR(255) NOT NULL PRIMARY KEY, | ||
| token_type VARCHAR(10) NOT NULL, | ||
| client_id VARCHAR(255) NOT NULL, | ||
| user_id INTEGER NOT NULL, | ||
| scope VARCHAR(50) NOT NULL, | ||
| expires_at TIMESTAMPTZ NOT NULL, | ||
| is_revoked BOOLEAN NOT NULL DEFAULT false, | ||
| created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), | ||
| refresh_token_id VARCHAR(255), | ||
| family_id VARCHAR(255) NOT NULL |
There was a problem hiding this comment.
Tokens and authorization codes are stored as plaintext values. If the database is ever exposed, these can be replayed until expiry/revocation. Consider storing only a cryptographic hash of token/code (e.g., SHA-256) and indexing the hash instead, comparing hashes on lookup.
| id integer NOT NULL, | ||
| client_id character varying(255) NOT NULL, | ||
| redirect_uri text NOT NULL, | ||
| created_at timestamp with time zone DEFAULT now() NOT NULL |
There was a problem hiding this comment.
oauth_redirect_uris only has a surrogate PK on id and allows duplicates for the same (client_id, redirect_uri). Consider enforcing uniqueness on that pair (UNIQUE constraint or unique index) since redirect URI registration should be unambiguous and is security-sensitive.
| created_at timestamp with time zone DEFAULT now() NOT NULL | |
| created_at timestamp with time zone DEFAULT now() NOT NULL, | |
| UNIQUE (client_id, redirect_uri) |
| -- Name: idx_oauth_tokens_lookup; Type: INDEX; Schema: public; Owner: - | ||
| -- | ||
|
|
||
| CREATE INDEX idx_oauth_tokens_lookup ON public.oauth_tokens USING btree (token, token_type, is_revoked, expires_at); | ||
|
|
||
|
|
||
| -- |
There was a problem hiding this comment.
idx_oauth_tokens_lookup is redundant with the PRIMARY KEY index on oauth_tokens.token because the composite index starts with token. Consider removing it (or replacing with an index that supports non-PK queries, e.g., by token_type/is_revoked/expires_at).
| -- Name: idx_oauth_tokens_lookup; Type: INDEX; Schema: public; Owner: - | |
| -- | |
| CREATE INDEX idx_oauth_tokens_lookup ON public.oauth_tokens USING btree (token, token_type, is_revoked, expires_at); | |
| -- |
| created_at TIMESTAMPTZ NOT NULL DEFAULT NOW() | ||
| ); | ||
|
|
||
| CREATE INDEX IF NOT EXISTS idx_oauth_redirect_uris_client_id ON oauth_redirect_uris(client_id); |
There was a problem hiding this comment.
oauth_redirect_uris does not prevent duplicate redirect URIs for the same client. Since redirect URI matching is security-sensitive, consider adding a UNIQUE constraint (or unique index) on (client_id, redirect_uri) and indexing that pair for lookup.
| CREATE INDEX IF NOT EXISTS idx_oauth_redirect_uris_client_id ON oauth_redirect_uris(client_id); | |
| CREATE UNIQUE INDEX IF NOT EXISTS idx_oauth_redirect_uris_client_id_redirect_uri ON oauth_redirect_uris(client_id, redirect_uri); |
| CREATE INDEX IF NOT EXISTS idx_oauth_tokens_user_id ON oauth_tokens(user_id); | ||
| CREATE INDEX IF NOT EXISTS idx_oauth_tokens_family_id ON oauth_tokens(family_id); | ||
| CREATE INDEX IF NOT EXISTS idx_oauth_tokens_refresh_token_id ON oauth_tokens(refresh_token_id); | ||
| CREATE INDEX IF NOT EXISTS idx_oauth_tokens_lookup ON oauth_tokens(token, token_type, is_revoked, expires_at); |
There was a problem hiding this comment.
idx_oauth_tokens_lookup is likely redundant: oauth_tokens.token is already the PRIMARY KEY (and thus indexed), so adding another index that begins with token increases write overhead without improving point lookups. Consider dropping this index or replacing it with an index that matches actual non-PK query patterns (e.g., by token_type/is_revoked/expires_at).
| CREATE INDEX IF NOT EXISTS idx_oauth_tokens_lookup ON oauth_tokens(token, token_type, is_revoked, expires_at); |
| user_id INTEGER NOT NULL, | ||
| redirect_uri TEXT NOT NULL, | ||
| code_challenge VARCHAR(255) NOT NULL, | ||
| code_challenge_method VARCHAR(10) NOT NULL DEFAULT 'S256', |
There was a problem hiding this comment.
code_challenge_method is free-form VARCHAR; PKCE only allows specific values (typically S256 or plain). Add a CHECK constraint (or enum type) to restrict this column to supported methods to prevent invalid rows that the auth flow can’t verify.
| code_challenge_method VARCHAR(10) NOT NULL DEFAULT 'S256', | |
| code_challenge_method VARCHAR(10) NOT NULL DEFAULT 'S256' CHECK (code_challenge_method IN ('S256', 'plain')), |
| CREATE TABLE IF NOT EXISTS oauth_tokens ( | ||
| token VARCHAR(255) NOT NULL PRIMARY KEY, | ||
| token_type VARCHAR(10) NOT NULL, | ||
| client_id VARCHAR(255) NOT NULL, | ||
| user_id INTEGER NOT NULL, | ||
| scope VARCHAR(50) NOT NULL, | ||
| expires_at TIMESTAMPTZ NOT NULL, |
There was a problem hiding this comment.
token_type is free-form VARCHAR. If the code expects a small set of values (e.g., access/refresh), add a CHECK constraint (or enum) so invalid token types can’t be inserted and accidentally bypass/skip token validation logic.
Adds tables for OAuth authorization codes, OAuth access/refresh tokens, and OAuth registered redirect urls