Conversation
CLA Signature PassKadenZhang3321, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
Linking Issue Notice@KadenZhang3321 , the pull request must be linked to at least one issue. |
There was a problem hiding this comment.
Code Review
This pull request introduces a graphic captcha system for corporation manager logins to protect against brute-force attacks, including new endpoints, domain logic, and a Redis-backed implementation. It also optimizes the corporation signing repository by implementing MongoDB index management, refactoring pagination queries to use a single aggregation pipeline, and adding a Redis-based cache decorator. Feedback highlights a regression in corporation name search logic, potential issues with error type preservation and Redis cache unmarshaling, and opportunities to improve logging consistency and goroutine safety.
| } else { | ||
| // 按企业名称搜索(模糊匹配) | ||
| filter[childField(fieldCorp, fieldName)] = bson.M{"$regex": searchQuery, "$options": "i"} | ||
| filter[childField(fieldCorp, fieldName)] = searchQuery |
There was a problem hiding this comment.
The search logic for corporation names has been changed from a case-insensitive fuzzy search ($regex) to an exact match. This is a regression in functionality as users can no longer search for corporations by partial names. If performance was the concern, consider using a text index or keeping the regex with appropriate constraints.
| filter[childField(fieldCorp, fieldName)] = searchQuery | |
| filter[childField(fieldCorp, fieldName)] = bson.M{"$regex": searchQuery, "$options": "i"} |
| } | ||
|
|
||
| if !c.captcha.Verify(id, answer, true) { | ||
| return errors.New(string(errCaptchaInvalid)) |
There was a problem hiding this comment.
Returning errors.New(string(errCaptchaInvalid)) creates a standard error that loses the ErrorCode() method defined on the domainError type. This will cause issues in the adapter layer where the error is checked against specific domain error codes. You should return the errCaptchaInvalid variable directly as it already implements the error interface and preserves the domain-specific metadata.
| return errors.New(string(errCaptchaInvalid)) | |
| return errCaptchaInvalid |
| var cached cachedCorpSigningPage | ||
| if err := c.cache.Get(key, &cached); err == nil { | ||
| return fromCache(cached), nil | ||
| } |
There was a problem hiding this comment.
The cache.Get method uses redisCli.Get(...).Scan(data) internally. Since cachedCorpSigningPage is a plain struct and does not implement redis.Scanner or encoding.BinaryUnmarshaler, the Scan call will fail to populate the struct from the JSON string stored in Redis. This results in a silent cache miss every time. You should retrieve the data as a string first and then use json.Unmarshal to populate the struct.
| var cached cachedCorpSigningPage | |
| if err := c.cache.Get(key, &cached); err == nil { | |
| return fromCache(cached), nil | |
| } | |
| var payload string | |
| if err := c.cache.Get(key, &payload); err == nil { | |
| if json.Unmarshal([]byte(payload), &cached) == nil { | |
| return fromCache(cached), nil | |
| } | |
| } |
| // if c.isTestEnv { | ||
| // id = uuid.New().String() | ||
| // if err = c.store.Set(id, c.testAnswer); err != nil { | ||
| // return | ||
| // } | ||
| // imageBase64 = "test_mode" | ||
| // return | ||
| // } |
| import ( | ||
| "encoding/json" | ||
| "fmt" | ||
| "log" |
|
|
||
| if clear { | ||
| // expire immediately | ||
| _ = s.dao.Expire(captchaKey(id), 0) |
There was a problem hiding this comment.
Using Expire(key, 0) to delete a key is less explicit and potentially less efficient than using the Del command. Since the Redis client already supports Del, you should add it to the local dao interface and use it here.
| _ = s.dao.Expire(captchaKey(id), 0) | |
| _ = s.dao.Del(captchaKey(id)) |
| } | ||
|
|
||
| // populate cache asynchronously so the caller is not blocked | ||
| go func() { |
CLA Signature PassKadenZhang3321, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
|
@gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements graphic captcha verification for corporation manager logins to protect against brute-force attacks. It introduces a new CaptchaService, updates the login flow to require captcha after a threshold of failed attempts, and adds a Redis-based caching layer for corporation signing pages. Additionally, the MongoDB repository was refactored to use aggregation for more efficient pagination. Feedback includes removing commented-out code in the captcha implementation, escaping user input in regex searches to prevent injection, validating pagination parameters to avoid negative skip values, and verifying the impact of changing the admin field filter from nil to $exists: false.
| // if c.isTestEnv { | ||
| // id = uuid.New().String() | ||
| // if err = c.store.Set(id, c.testAnswer); err != nil { | ||
| // return | ||
| // } | ||
| // imageBase64 = "test_mode" | ||
| // return | ||
| // } |
There was a problem hiding this comment.
The block of commented-out code should be removed to maintain code cleanliness. Additionally, the isTestEnv and testAnswer fields in the captchaImpl struct are currently unused, and the logic for a deterministic captcha in test environments is missing despite being passed in the constructor. Consider implementing the test mode logic or removing the unused parameters and fields.
| } else { | ||
| // 按企业名称搜索(模糊匹配) | ||
| // 按企业名称搜索(模糊匹配,case-insensitive) | ||
| filter[childField(fieldCorp, fieldName)] = bson.M{"$regex": searchQuery, "$options": "i"} |
There was a problem hiding this comment.
| filter["$or"] = []bson.M{ | ||
| {"admin.id": ""}, | ||
| {"admin": nil}, | ||
| {"admin": bson.M{"$exists": false}}, |
There was a problem hiding this comment.
Changing the filter from {"admin": nil} to {"admin": {"$exists": false}} alters the query behavior. In MongoDB, a query for field: null matches both documents where the field is explicitly set to null and documents where the field is missing. The $exists: false operator only matches documents where the field is missing. If there are existing records where admin is null, they will be excluded from the results. Ensure this change is intentional and won't lead to data inconsistency.
| bson.M{"$count": "n"}, | ||
| }, | ||
| "data": bson.A{ | ||
| bson.M{"$skip": int64((intPage - 1) * intPageSize)}, |
There was a problem hiding this comment.
CLA Signature PassKadenZhang3321, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
|
|||||||||||||||||||
|
|||||||||||||||||||
|
|||||||||||||||||||
|
|||||||||||||||||||
CLA Signature PassKadenZhang3321, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
2 similar comments
CLA Signature PassKadenZhang3321, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
CLA Signature PassKadenZhang3321, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
|
|||||||||||||||||||
|
|||||||||||||||||||
|
|||||||||||||||||||
- 添加登录图形验证码生成和验证接口 - 实现登录失败次数统计和验证码触发机制 - 登录成功后清除失败记录 - 修复 Code Review 相关问题
ffef0cd to
fa64fb1
Compare
CLA Signature PassKadenZhang3321, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
|
@gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a graphic captcha mechanism to protect against brute-force login attacks. It includes the necessary infrastructure for captcha generation and verification, updates the login flow to require captcha validation when a failure threshold is met, and adds relevant configuration and API endpoints. My review highlights a security concern regarding the fail-open behavior when the captcha requirement check fails, and suggests improvements for the captcha implementation's test mode and configuration flexibility.
| return | ||
| } | ||
| } else if needed, checkErr := s.ls.NeedCaptcha(lid); checkErr != nil { | ||
| logs.Warn("failed to check captcha requirement, err: %s", checkErr.Error()) |
There was a problem hiding this comment.
当前实现中,如果检查是否需要验证码的 s.ls.NeedCaptcha(lid) 调用失败,程序仅记录一条警告日志然后继续执行登录流程。这可能构成一个安全隐患,因为攻击者或许能通过让依赖的服务(如Redis)暂时不可用来绕过验证码。建议采用“失败即关闭”(fail-closed)的策略:当此检查失败时,应中断登录流程并返回错误,而不是继续尝试登录。
| logs.Warn("failed to check captcha requirement, err: %s", checkErr.Error()) | |
| logs.Error("failed to check captcha requirement for %s, err: %s", lid, checkErr.Error()) | |
| err = checkErr | |
| return |
| func NewCaptchaImpl(d dao, cfg *Config, isTestEnv bool, testAnswer string) *captchaImpl { | ||
| store := &captchaStore{dao: d, expiry: cfg.expiry()} | ||
|
|
||
| driver := base64Captcha.NewDriverDigit( | ||
| 80, // height | ||
| 240, // width | ||
| 6, // digit length | ||
| 0.7, // noise level | ||
| 80, // background circle count | ||
| ) | ||
|
|
||
| return &captchaImpl{ | ||
| captcha: base64Captcha.NewCaptcha(driver, store), | ||
| store: store, | ||
| isTestEnv: isTestEnv, | ||
| testAnswer: testAnswer, | ||
| } |
There was a problem hiding this comment.
描述
为社区管理员登录增加图形验证码功能,防止暴力破解。当登录失败达到阈值次数后,下次登录需要输入验证码。
主要变更:
新增验证码服务 (signing/domain/captchaservice/, signing/infrastructure/captchaimpl/)
登录失败计数和验证码触发逻辑 (signing/domain/login.go, signing/domain/loginservice/service.go)
登录接口返回 need_captcha 和 retry_num 字段
验证码存储使用 Redis
相关 Issue
https://github.com/opensourceways/backlog/issues/45
变更类型