-
Notifications
You must be signed in to change notification settings - Fork 7
tech-debt(oauth): integrate DB settings + improve signup detection #1226
Open
Description
Summary
Tech debt items from OAuth PR #1141 / #1220 review that were not addressed in #1220.
Review Status Overview
Pyramation Inline Review (7 条)
| # | Line | 评论 | #1220 状态 |
|---|---|---|---|
| 1 | 135 | 旧版本 secrets? | ✅ 改用 encryptedSecretsLoader |
| 2 | 243 | 用 getNodeEnv() |
✅ 已改用 |
| 3 | 245 | rate limiting 冲突 | ✅ 移除 express-rate-limit |
| 4 | 399 | privateSchema 不代表 secrets/identity 位置 | ✅ 用 module loaders 动态解析 |
| 5 | 459 | 不应手动做 (set_config) | ✅ 改用 withPgClient() |
| 6 | 473 | 为何假设 sign_in_identity 在 privateSchema | ✅ 用 userAuth.schemaName |
| 7 | 517 | signup 只基于 signin 失败?有更好方式? | ❌ 未改 |
Devin DB Settings 建议 (5/23 评论)
| # | 建议 | #1220 状态 |
|---|---|---|
| 1 | oauth_state_max_age 从 DB 读取 |
❌ 未实现 |
| 2 | oauth_require_verified_email 从 DB 读取 |
❌ 未实现 |
| 3 | oauth_error_redirect_path 从 DB 读取 |
❌ 未实现 |
| 4 | allow_identity_sign_up 从 DB 读取 |
❌ 未实现 |
| 5 | State cookie 用 authSettings | ❌ 未实现 |
TODO Item 1: Integrate OAuth DB Settings
PR #1301 (constructive-db, merged) added three OAuth settings to app_settings_auth:
| Column | Type | Default | Purpose |
|---|---|---|---|
oauth_state_max_age |
interval |
'10 minutes' |
State token validity window |
oauth_require_verified_email |
boolean |
true |
Reject unverified email signup |
oauth_error_redirect_path |
text |
'/auth/error' |
Error redirect path |
Changes needed
1. Update authSettingsLoader (packages/express-context/src/loaders/auth-settings.ts):
const buildAuthSettingsQuery = (schemaName: string, tableName: string) => ` SELECT cookie_secure, cookie_samesite, // ... existing fields + oauth_state_max_age, + oauth_require_verified_email, + oauth_error_redirect_path, + allow_identity_sign_up FROM "${schemaName}"."${tableName}" LIMIT 1 `;
2. Update oauth.ts to read from authSettings:
// Replace hardcoded constants - const stateMaxAge = DEFAULT_OAUTH_STATE_MAX_AGE; + const stateMaxAge = modules.authSettings?.oauthStateMaxAge ?? DEFAULT_OAUTH_STATE_MAX_AGE; - if (!emailVerified) { + const requireVerified = modules.authSettings?.oauthRequireVerifiedEmail ?? true; + if (requireVerified && !emailVerified) { - DEFAULT_ERROR_REDIRECT_PATH + modules.authSettings?.oauthErrorRedirectPath ?? DEFAULT_ERROR_REDIRECT_PATH
3. Update state cookie to use authSettings:
res.cookie(OAUTH_STATE_COOKIE, state, { - httpOnly: true, - secure: isProduction, - sameSite: 'lax', + httpOnly: authSettings?.cookieHttponly ?? true, + secure: authSettings?.cookieSecure ?? isProduction, + sameSite: authSettings?.cookieSamesite ?? 'lax', maxAge: stateMaxAge, });
TODO Item 2: Improve Signup Detection Logic
Current flow relies on catching IDENTITY_ACCOUNT_NOT_FOUND error to trigger signup:
try { await sign_in_identity(...); } catch (err) { if (err.message.includes('IDENTITY_ACCOUNT_NOT_FOUND')) { await sign_up_identity(...); // fallback to signup } }
Issues
- Uses exception for flow control (anti-pattern)
- Every new user triggers an "error" log
- Extra failed DB call for new users
Possible alternatives
| 方案 | 做法 | 优缺点 |
|---|---|---|
| Query first | 先 SELECT 检查用户是否存在 | 多一次查询,但逻辑清晰 |
| Merged function | sign_in_or_create_identity() 在 DB 层处理 |
一次调用,DB 内部判断 |
| Upsert pattern | 类似 INSERT ... ON CONFLICT 思路 |
需要改 DB 函数设计 |
Related
- constructive-db PR #1301 (merged) - added DB fields
- constructive PR feat: OAuth middleware with module loaders for identity/secrets/auth config #1220 (closed) - OAuth refactor
- constructive PR feat(oauth): OAuth identity sign-in with cross-origin token exchange #1141 (superseded) - original OAuth implementation
- Pyramation review comment: "is signup only based on a failed signin?"
- Devin DB Settings comment
Priority
Low - OAuth works correctly with current hardcoded defaults. These are improvements for:
- Tenant configurability (DB settings)
- Code quality (signup detection)
Metadata
Metadata
Assignees
Labels
No labels
Type
Fields
Give feedbackNo fields configured for issues without a type.