diff --git a/src/frontend/apiserver/controllers/hosts.go b/src/frontend/apiserver/controllers/hosts.go index 152014f2..a15975ab 100644 --- a/src/frontend/apiserver/controllers/hosts.go +++ b/src/frontend/apiserver/controllers/hosts.go @@ -422,17 +422,17 @@ func (h *HostHandler) ControlHost(ctx *gin.Context) { successHosts := []map[string]interface{}{} failedHosts := []map[string]interface{}{} - validatedSuccessHosts, validatedFailedHosts := validateHost(ctx, actionType, req.Hostname) - if ctx.IsAborted() { + hostnames := req.Hostname + if allFlag { + hostnames = store.GetAllHostNames() + } + validatedSuccessHosts, validatedFailedHosts, err := validateHost(ctx, actionType, hostnames) + if err != nil { + // validateHost rejected the request: answer once and stop (bug-1961). ReturnError(ctx, common.ERROR_AUTH_CODE, common.ERROR_ACCESS_FORBIDDEN_MSG, nil) return } - if allFlag { - hostnames := store.GetAllHostNames() - validatedSuccessHosts, validatedFailedHosts = validateHost(ctx, actionType, hostnames) - } - for _, hostname := range validatedSuccessHosts { successHosts = append(successHosts, map[string]interface{}{ "host_name": hostname, @@ -471,16 +471,27 @@ func (h *HostHandler) ControlHost(ctx *gin.Context) { return } -// validateHost validates the hostname in the request -func validateHost(ctx *gin.Context, control int, hostnames []string) (successHosts []string, failedHosts map[string]string) { +// validateHost validates the hostnames in the request. It writes no +// response: a non-nil error means the caller lacks host:control (or its +// token is invalid) and the caller must reject the whole request (bug-1961). +func validateHost(ctx *gin.Context, control int, hostnames []string) (successHosts []string, failedHosts map[string]string, err error) { logger.Debug("Validating hostnames: %v", zap.Any("hostnames", hostnames)) // get logon user token := ctx.GetHeader("Authorization") logonUser, err := GetUsernameFromToken(token) if err != nil { - ReturnError(ctx, common.ERROR_CODE, err.Error(), nil) - return + logger.Error(err.Error()) + return nil, nil, err + } + + hostPermission, err := GetRequireResourcePermission(logonUser, "host:control") + if err != nil { + logger.Error(err.Error(), zap.String("user", logonUser)) + return nil, nil, err + } + if hostPermission.DomainType == models.DomainTypeHostGroup { + // TODO: check hostgroup values } successHosts = []string{} @@ -497,16 +508,6 @@ func validateHost(ctx *gin.Context, control int, hostnames []string) (successHos continue } - hostPermission, err := GetRequireResourcePermission(logonUser, "host:control") - if err != nil { - logger.Error(err.Error()) - ReturnError(ctx, common.ERROR_AUTH_CODE, common.ERROR_ACCESS_FORBIDDEN_MSG, nil) - return - } - if hostPermission.DomainType == models.DomainTypeHostGroup { - // TODO: check hostgroup values - } - if store.Hosts.Has(hostname) { logger.Debug("Found hostname in store", zap.String("hostname", hostname)) successHosts = append(successHosts, hostname) diff --git a/src/frontend/apiserver/controllers/hosts_handler_test.go b/src/frontend/apiserver/controllers/hosts_handler_test.go new file mode 100644 index 00000000..8a2e8f3c --- /dev/null +++ b/src/frontend/apiserver/controllers/hosts_handler_test.go @@ -0,0 +1,68 @@ +//go:build !cgo + +package controllers + +import ( + "encoding/json" + "io" + "net/http" + "strings" + "testing" +) + +// RBAC fixture for bug-1961: viewer has no host:control grant at all. +const hostControlRBAC = ` +permissions: + - host:control + - host:view +roles: + host_viewer: + permissions: + - action: host:view + domain: + - type: all +users: + viewer: + roles: + - name: host_viewer +` + +// countJSONValues returns how many JSON documents body holds back to back: +// a handler that writes a second response after an error shows up as 2. +func countJSONValues(t *testing.T, body string) int { + t.Helper() + dec := json.NewDecoder(strings.NewReader(body)) + n := 0 + for { + var v interface{} + if err := dec.Decode(&v); err == io.EOF { + return n + } else if err != nil { + t.Fatalf("decoding response body %q: %v", body, err) + } + n++ + } +} + +// bug-1961: when validateHost rejects the request (no host:control grant), +// ControlHost must stop with a single 403 response. The route is mounted +// without RequirePermission so the handler's own check is what is tested. +func TestControlHost_RejectedWritesSingleResponse(t *testing.T) { + for _, hosts := range []string{`["node01"]`, `["all"]`} { + t.Run(hosts, func(t *testing.T) { + setupRBACTest(t, hostControlRBAC) + r := newTestRouter() + h := &HostHandler{} + r.POST("/xce/v1/hosts/control", h.ControlHost) + + w := doRoleRequest(r, http.MethodPost, "/xce/v1/hosts/control", "viewer", + `{"action":"close","host_name":`+hosts+`}`) + if n := countJSONValues(t, w.Body.String()); n != 1 { + t.Errorf("handler wrote %d responses, want 1: %s", n, w.Body.String()) + } + if w.Code != http.StatusForbidden { + t.Errorf("status = %d, want 403 (%s)", w.Code, w.Body.String()) + } + }) + } +} diff --git a/src/frontend/apiserver/controllers/integration_test.go b/src/frontend/apiserver/controllers/integration_test.go index 1d965315..05cac6e9 100644 --- a/src/frontend/apiserver/controllers/integration_test.go +++ b/src/frontend/apiserver/controllers/integration_test.go @@ -189,7 +189,10 @@ users: r := gin.New() r.POST("/test", func(ctx *gin.Context) { - successHosts, failedHosts := validateHost(ctx, 1, []string{"node01", "nonexistent"}) + successHosts, failedHosts, err := validateHost(ctx, 1, []string{"node01", "nonexistent"}) + if err != nil { + t.Fatalf("validateHost: %v", err) + } foundNode01 := false for _, h := range successHosts { if h == "node01" {