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
Priority
Low - OAuth works correctly with current hardcoded defaults. These are improvements for:
- Tenant configurability (DB settings)
- Code quality (signup detection)
Summary
Tech debt items from OAuth PR #1141 / #1220 review that were not addressed in #1220.
Review Status Overview
Pyramation Inline Review (7 条)
encryptedSecretsLoadergetNodeEnv()express-rate-limitwithPgClient()userAuth.schemaNameDevin DB Settings 建议 (5/23 评论)
oauth_state_max_age从 DB 读取oauth_require_verified_email从 DB 读取oauth_error_redirect_path从 DB 读取allow_identity_sign_up从 DB 读取TODO Item 1: Integrate OAuth DB Settings
PR #1301 (constructive-db, merged) added three OAuth settings to
app_settings_auth:oauth_state_max_ageinterval'10 minutes'oauth_require_verified_emailbooleantrueoauth_error_redirect_pathtext'/auth/error'Changes needed
1. Update
authSettingsLoader(packages/express-context/src/loaders/auth-settings.ts):2. Update
oauth.tsto read fromauthSettings:3. Update state cookie to use
authSettings:TODO Item 2: Improve Signup Detection Logic
Current flow relies on catching
IDENTITY_ACCOUNT_NOT_FOUNDerror to trigger signup:Issues
Possible alternatives
sign_in_or_create_identity()在 DB 层处理INSERT ... ON CONFLICT思路Related
Priority
Low - OAuth works correctly with current hardcoded defaults. These are improvements for: