-
Notifications
You must be signed in to change notification settings - Fork 1.1k
cubeops: propagate database errors from the settings and user getters #1382
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
02b5972
24ecdf2
75d003d
4d6bbd2
78cb4b2
ce0338d
5a6b6cc
27b92a9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| // Copyright (c) 2026 Tencent Inc. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package auth_test | ||
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "strings" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/gin-gonic/gin" | ||
| "github.com/tencentcloud/CubeSandbox/CubeOps/internal/auth" | ||
| "github.com/tencentcloud/CubeSandbox/CubeOps/internal/service" | ||
| ) | ||
|
|
||
| const dbErrorDetail = "dial tcp 10.0.0.5:3306: connect: connection refused" | ||
|
|
||
| type outageUserStore struct{ fakeUserStore } | ||
|
|
||
| func (o *outageUserStore) GetUserPassword(_ context.Context, _ string) (string, error) { | ||
| return "", errors.New(dbErrorDetail) | ||
| } | ||
|
|
||
| func (o *outageUserStore) IsRefreshTokenRevoked(_ context.Context, _ string) (bool, error) { | ||
| return false, errors.New(dbErrorDetail) | ||
| } | ||
|
|
||
| func newOutageRouter(t *testing.T) *gin.Engine { | ||
| t.Helper() | ||
| jm := auth.NewJWTManager("test-secret-32-bytes-long-enough!", 15*time.Minute, 168*time.Hour) | ||
| svc := service.NewAuthService(&outageUserStore{}, jm) | ||
| h := auth.NewHandler(svc) | ||
|
|
||
| r := gin.New() | ||
| h.RegisterPublic(r.Group("/api/v1")) | ||
| h.RegisterAuthed(r.Group("/api/v1", auth.Middleware(jm))) | ||
| return r | ||
| } | ||
|
|
||
| func TestLoginOutageDoesNotLeakDatabaseDetailsToTheCaller(t *testing.T) { | ||
| r := newOutageRouter(t) | ||
|
|
||
| w := doRequest(t, r, "POST", "/api/v1/auth/login", | ||
| `{"username":"admin","password":"s3cret"}`, "") | ||
|
|
||
| if w.Code != 500 { | ||
| t.Fatalf("status = %d, want 500", w.Code) | ||
| } | ||
| body := w.Body.String() | ||
| for _, secret := range []string{dbErrorDetail, "10.0.0.5", "3306", "dial tcp", "connection refused"} { | ||
| if strings.Contains(body, secret) { | ||
| t.Errorf("500 body leaks %q to an unauthenticated caller: %s", secret, body) | ||
| } | ||
| } | ||
| if !strings.Contains(body, "internal server error") { | ||
| t.Errorf("500 body = %s, want a generic message", body) | ||
| } | ||
| if strings.Contains(body, "required") { | ||
| t.Errorf("the request never reached the database path: %s", body) | ||
| } | ||
| } | ||
|
|
||
| func TestChangePasswordOutageDoesNotLeakDatabaseDetailsToTheCaller(t *testing.T) { | ||
| jm := auth.NewJWTManager("test-secret-32-bytes-long-enough!", 15*time.Minute, 168*time.Hour) | ||
| token, err := jm.GenerateAccessToken("admin") | ||
| if err != nil { | ||
| t.Fatalf("GenerateAccessToken: %v", err) | ||
| } | ||
|
|
||
| r := newOutageRouter(t) | ||
| w := doRequest(t, r, "POST", "/api/v1/auth/change-password", | ||
| `{"old_password":"s3cret","new_password":"n3wpass"}`, token) | ||
|
|
||
| if w.Code != 500 { | ||
| t.Fatalf("status = %d, want 500 (body: %s)", w.Code, w.Body.String()) | ||
| } | ||
| body := w.Body.String() | ||
| for _, secret := range []string{dbErrorDetail, "10.0.0.5", "3306", "dial tcp", "connection refused"} { | ||
| if strings.Contains(body, secret) { | ||
| t.Errorf("500 body leaks %q to the caller: %s", secret, body) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func TestRefreshOutageDoesNotLeakDatabaseDetailsToTheCaller(t *testing.T) { | ||
| jm := auth.NewJWTManager("test-secret-32-bytes-long-enough!", 15*time.Minute, 168*time.Hour) | ||
| refresh, _, err := jm.GenerateRefreshToken("admin") | ||
| if err != nil { | ||
| t.Fatalf("GenerateRefreshToken: %v", err) | ||
| } | ||
|
|
||
| r := newOutageRouter(t) | ||
| w := doRequest(t, r, "POST", "/api/v1/auth/refresh", | ||
| `{"refreshToken":"`+refresh+`"}`, "") | ||
|
|
||
| if w.Code != 500 { | ||
| t.Fatalf("status = %d, want 500 (body: %s)", w.Code, w.Body.String()) | ||
| } | ||
| body := w.Body.String() | ||
| for _, secret := range []string{dbErrorDetail, "10.0.0.5", "3306", "dial tcp", "connection refused"} { | ||
| if strings.Contains(body, secret) { | ||
| t.Errorf("500 body leaks %q to the caller: %s", secret, body) | ||
| } | ||
| } | ||
| if !strings.Contains(body, "internal server error") { | ||
| t.Errorf("500 body = %s, want a generic message", body) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| // Copyright (c) 2026 Tencent Inc. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package service | ||
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "testing" | ||
| "time" | ||
| ) | ||
|
|
||
| type dbErrorStore struct { | ||
| err error | ||
| } | ||
|
|
||
| func (d dbErrorStore) GetUserPassword(context.Context, string) (string, error) { | ||
| return "", d.err | ||
| } | ||
| func (d dbErrorStore) SetUserPassword(context.Context, string, string) error { return nil } | ||
| func (d dbErrorStore) CreateRefreshToken(context.Context, string, string) error { return nil } | ||
| func (d dbErrorStore) IsRefreshTokenRevoked(context.Context, string) (bool, error) { return false, nil } | ||
| func (d dbErrorStore) RevokeRefreshToken(context.Context, string) error { return nil } | ||
| func (d dbErrorStore) RevokeAllRefreshTokensForUser(context.Context, string) error { return nil } | ||
|
|
||
| type stubIssuer struct{} | ||
|
|
||
| func (stubIssuer) GenerateAccessToken(string) (string, error) { return "a", nil } | ||
| func (stubIssuer) GenerateRefreshToken(string) (string, string, error) { return "r", "t", nil } | ||
| func (stubIssuer) VerifyRefreshToken(string) (*RefreshClaims, error) { return &RefreshClaims{}, nil } | ||
| func (stubIssuer) AccessTTL() time.Duration { return time.Minute } | ||
|
|
||
| func TestLoginSurfacesInfrastructureError(t *testing.T) { | ||
| dbDown := errors.New("dial tcp 10.0.0.5:3306: connect: connection refused") | ||
| svc := NewAuthService(dbErrorStore{err: dbDown}, stubIssuer{}) | ||
|
|
||
| _, err := svc.Login(context.Background(), "admin", "hunter2") | ||
| if err == nil { | ||
| t.Fatal("Login returned no error while the database was unreachable") | ||
| } | ||
| if errors.Is(err, ErrInvalidCredentials) { | ||
| t.Fatalf("Login masked a database outage as invalid credentials: %v", err) | ||
| } | ||
| if !errors.Is(err, dbDown) { | ||
| t.Fatalf("Login did not wrap the underlying error, got %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestLoginUnknownUserStillReportsInvalidCredentials(t *testing.T) { | ||
| svc := NewAuthService(dbErrorStore{err: nil}, stubIssuer{}) | ||
|
|
||
| _, err := svc.Login(context.Background(), "nobody", "hunter2") | ||
| if !errors.Is(err, ErrInvalidCredentials) { | ||
| t.Fatalf("unknown user should report invalid credentials, got %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestChangePasswordSurfacesInfrastructureError(t *testing.T) { | ||
| dbDown := errors.New("dial tcp 10.0.0.5:3306: connect: connection refused") | ||
| svc := NewAuthService(dbErrorStore{err: dbDown}, stubIssuer{}) | ||
|
|
||
| err := svc.ChangePassword(context.Background(), "admin", "old-pass", "new-pass") | ||
| if err == nil { | ||
| t.Fatal("ChangePassword returned no error while the database was unreachable") | ||
| } | ||
| if errors.Is(err, ErrInvalidOldPassword) { | ||
| t.Fatalf("ChangePassword masked a database outage as a bad old password: %v", err) | ||
| } | ||
| if !errors.Is(err, dbDown) { | ||
| t.Fatalf("ChangePassword discarded the underlying database error: %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestChangePasswordUnknownUserReportsBadOldPassword(t *testing.T) { | ||
| svc := NewAuthService(dbErrorStore{err: nil}, stubIssuer{}) | ||
|
|
||
| err := svc.ChangePassword(context.Background(), "nobody", "old-pass", "new-pass") | ||
| if !errors.Is(err, ErrInvalidOldPassword) { | ||
| t.Fatalf("unknown user should report a bad old password, got %v", err) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,14 +17,17 @@ const settingMasterKey = "secret_master_key" | |
|
|
||
| // GetSystemSetting retrieves a system-level setting value by key. | ||
| func (s *Store) GetSystemSetting(ctx context.Context, key string) (string, error) { | ||
| var val string | ||
| var val sql.NullString | ||
| err := s.db.WithContext(ctx).Raw( | ||
| "SELECT setting_value FROM t_system_setting WHERE setting_key = ? LIMIT 1", key, | ||
| ).Scan(&val).Error | ||
| if errors.Is(err, sql.ErrNoRows) || val == "" { | ||
| return "", nil | ||
| ).Row().Scan(&val) | ||
| if err != nil { | ||
| if errors.Is(err, sql.ErrNoRows) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This
This is harmless (behavior is correct) and even a forward-compatible safety net if GORM ever changes, so no change is strictly required — but the PR description presents this branch as the mechanism that separates "absent" from "error", which isn't what happens today. A one-line comment noting the empty-value fall-through is the real path would prevent a future reader from "fixing" it. Same applies to the identical branches in |
||
| return "", nil | ||
| } | ||
| return "", err | ||
| } | ||
| return val, err | ||
| return val.String, nil | ||
| } | ||
|
|
||
| // GetOrCreateSystemSetting atomically gets an existing system setting or | ||
|
|
@@ -52,14 +55,17 @@ func (s *Store) SetSystemSetting(ctx context.Context, key, value string) error { | |
|
|
||
| // GetSetting retrieves an AgentHub-level setting value by key. | ||
| func (s *Store) GetSetting(ctx context.Context, key string) (string, error) { | ||
| var val string | ||
| var val sql.NullString | ||
| err := s.db.WithContext(ctx).Raw( | ||
| "SELECT setting_value FROM t_agenthub_setting WHERE setting_key = ? LIMIT 1", key, | ||
| ).Scan(&val).Error | ||
| if errors.Is(err, sql.ErrNoRows) || val == "" { | ||
| return "", nil | ||
| ).Row().Scan(&val) | ||
| if err != nil { | ||
| if errors.Is(err, sql.ErrNoRows) { | ||
| return "", nil | ||
| } | ||
| return "", err | ||
| } | ||
| return val, err | ||
| return val.String, nil | ||
| } | ||
|
|
||
| // GetOrCreateSetting atomically gets an existing setting or creates it with the given value. | ||
|
|
@@ -104,14 +110,17 @@ func (s *Store) SetSettingsTx(ctx context.Context, kv map[string]string) error { | |
|
|
||
| // GetUserPassword retrieves the stored password hash for a user. | ||
| func (s *Store) GetUserPassword(ctx context.Context, username string) (string, error) { | ||
| var pwd string | ||
| var pwd sql.NullString | ||
| err := s.db.WithContext(ctx).Raw( | ||
| "SELECT password FROM t_system_user WHERE username = ? LIMIT 1", username, | ||
| ).Scan(&pwd).Error | ||
| if errors.Is(err, sql.ErrNoRows) || pwd == "" { | ||
| return "", nil | ||
| ).Row().Scan(&pwd) | ||
| if err != nil { | ||
| if errors.Is(err, sql.ErrNoRows) { | ||
| return "", nil | ||
| } | ||
| return "", err | ||
| } | ||
| return pwd, err | ||
| return pwd.String, nil | ||
| } | ||
|
|
||
| // SetUserPassword updates the password hash for a user. | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.