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. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
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 | ||
| // } |
There was a problem hiding this comment.
The test environment logic in the Generate method is commented out, rendering the isTestEnv and testAnswer parameters in NewCaptchaImpl effectively useless. If test mode support is required, this logic should be implemented and active.
| import ( | ||
| "encoding/json" | ||
| "fmt" | ||
| "log" |
There was a problem hiding this comment.
The standard log package is used here, but the rest of the project consistently uses github.com/beego/beego/v2/core/logs. Please switch to the project's preferred logging utility for consistency.
| "log" | |
| "github.com/beego/beego/v2/core/logs" |
|
|
||
| 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() { |
There was a problem hiding this comment.
This goroutine lacks a recovery mechanism. If toCache or json.Marshal were to panic, it would cause the entire application to crash. It is safer to wrap the goroutine logic in a defer recover() block or use a centralized safe goroutine utility.
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.
Using an unescaped user-provided string directly in a MongoDB $regex query can lead to performance issues (such as ReDoS) or unexpected query results if the input contains special regex characters (e.g., .*). It is recommended to escape the searchQuery before incorporating it into the regex filter.
| 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.
The intPage and intPageSize parameters are used in a calculation for $skip without prior validation. If intPage is less than 1, the resulting skip value will be negative, which will cause the MongoDB aggregation to fail. It is safer to ensure the skip value is non-negative.
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. 👍 |
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 participants
描述
为社区管理员登录增加图形验证码功能，防止暴力破解。当登录失败达到阈值次数后，下次登录需要输入验证码。
主要变更：
新增验证码服务 (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
变更类型