AlexStocks commented on code in PR #1061:
URL: https://github.com/apache/dubbo-go-pixiu/pull/1061#discussion_r4172978815
##########
admin/controller/auth/auth_test.go:
##########
@@ -41,21 +57,129 @@ func TestParseTokenRejectsUnexpectedSigningMethod(t
*testing.T) {
tokenString, err := token.SignedString(jwt.UnsafeAllowNoneSignatureType)
require.NoError(t, err)
- _, err = NewJWT().ParseToken(tokenString)
+ _, err = testJWT(t).ParseToken(tokenString)
assert.ErrorIs(t, err, TokenInvalid)
}
func TestGetSignKeyUsesEnvironmentOverride(t *testing.T) {
- t.Setenv(jwtSignKeyEnv, "from-env")
+ t.Setenv(jwtSignKeyEnv, testSignKey)
+
+ key, err := GetSignKey()
+ require.NoError(t, err)
+ assert.Equal(t, testSignKey, key)
+}
+
+func TestEmptySigningKeyUsesProcessLocalRandomKey(t *testing.T) {
+ t.Setenv(jwtSignKeyEnv, "")
+ type result struct {
+ key string
+ err error
+ }
+ results := make(chan result, 16)
+ for i := 0; i < cap(results); i++ {
+ go func() {
+ key, err := GetSignKey()
+ results <- result{key: key, err: err}
+ }()
+ }
+ var key string
+ for i := 0; i < cap(results); i++ {
+ got := <-results
+ require.NoError(t, got.err)
+ if key == "" {
+ key = got.key
+ }
+ assert.Equal(t, key, got.key)
+ }
+ material, err := hex.DecodeString(key)
+ require.NoError(t, err)
+ assert.Len(t, material, minSignKeyLength)
+ assert.NotEqual(t, legacySignKey, key)
+
+ for _, blank := range []string{"", " "} {
+ t.Setenv(jwtSignKeyEnv, blank)
+ got, err := GetSignKey()
+ require.NoError(t, err)
+ assert.Equal(t, key, got)
+ }
+ t.Setenv(jwtSignKeyEnv, "temporary")
+ require.NoError(t, os.Unsetenv(jwtSignKeyEnv))
+ j, err := NewJWT()
+ require.NoError(t, err)
+ assert.Equal(t, key, string(j.SigningKey))
+ claims := CustomClaims{Username: "admin", StandardClaims:
jwt.StandardClaims{ExpiresAt: time.Now().Add(time.Hour).Unix()}}
Review Comment:
[P1] 这 4 处新增的显式弃用引用会让本 PR 自己的 Golang Lint 失败，并连带跳过全部测试
第 111、136、145、161 行都以显式形式写了 `StandardClaims:
jwt.StandardClaims{...}`。`golang-jwt/jwt/v4 v4.5.2` 的 `StandardClaims` 类型带 `//
Deprecated:`，staticcheck 会报 SA1019，而本仓库的 lint 门禁恰好会拦住它：
- `.golangci.yml` 的 enable 列表包含 `staticcheck`，并且设置了 `issues.new: true`；
- CI 的 `Golang Lint` job 用 `--new-from-rev=<base sha>` 运行 golangci-lint
v2.4.0，只报告本 PR 新增行上的问题。
在隔离副本上（未改动被审代码）用 CI 的同一版本、同一参数复现：
```
$ golangci-lint v2.4.0 run
--new-from-rev=ba018888affd631f5e7683813d78a88a54a2de32 ./...
admin/controller/auth/auth_test.go:111:60: SA1019: jwt.StandardClaims is
deprecated: Use RegisteredClaims instead ... (staticcheck)
admin/controller/auth/auth_test.go:136:60: SA1019: ...
admin/controller/auth/auth_test.go:145:60: SA1019: ...
→ exit 1（3 issues，staticcheck: 3）
```
默认 `max-same-issues: 3` 会把同文案的问题截断到 3 条；加 `--max-same-issues=0` 后可见第 161
行也在其中，共 4 条。
影响不止这一个 job：`unit-test` 与 `integration-test` 都声明了 `needs: golangci`，`Golang
Lint` 失败后它们会变成 SKIPPED，本 PR 在 CI 上仍然没有任何测试证据——和之前 import 格式门禁是同一类连带效应。
建议沿用 #1062 的做法，不显式引用该类型（经内嵌字段隐式赋值不触发 SA1019，那边已独立验证）：
```go
claims := CustomClaims{Username: "admin"}
claims.ExpiresAt = time.Now().Add(time.Hour).Unix()
```
若确要保留显式写法，则需 `//nolint:staticcheck` 加说明，与
`pkg/config/xds/admin_runtime_e2e_test.go:643` 保持一致。
同文件第 52、185、220、233、250、271、280 行是 base 已有的同类写法，不属于本 PR 新增行，未计入。
##########
admin/controller/account/guest.go:
##########
@@ -99,7 +99,11 @@ func Login(c *gin.Context) {
}
func generateToken(c *gin.Context, username string) {
- j := auth.NewJWT()
+ j, err := auth.NewJWT()
+ if err != nil {
+ c.JSON(http.StatusInternalServerError, config.WithError(err))
Review Comment:
[P2] 新增的错误分支把内部配置错误原样回传给了 HTTP 客户端，与同一个 PR 在中间件里的脱敏做法不一致
`auth.NewJWT()` 现在会返回错误，这里以及 `admin/controller/account/user.go:48` 都直接把原始
`err` 交给 `config.WithError`。这个错误的来源是 `admin/controller/auth/auth.go:145`：
```
set DUBBOGO_PIXIU_JWT_SIGN_KEY to a unique random key of at least 32 bytes
(the former default is not accepted)
```
也就是登录/登出接口会把环境变量名和内部校验规则回给调用方。而同一个 PR 在 `JWTAuth()`
中对同一类失败选择了脱敏（`admin/controller/auth/auth.go:62`）：
```go
c.JSON(http.StatusInternalServerError,
config.WithError(errors.New("authentication is not configured")))
```
`/login` 与 `/logout` 的这段路径对未认证调用方是可达的，所以这是一个新引入的对外响应面。
影响有限——该环境变量名本身就是文档公开信息，且只有服务端密钥配置错误时才会走到这个分支。收益主要是两点：保持同一个 PR
内部错误处理一致，以及不把内部配置细节回给调用方。
建议这两处也返回固定的对外文案，把原始错误写进服务端日志（登录接口在服务运行期被调用，此时 `global.LOG` 已就绪）。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
