fix(api): verify service ownership and reserve platform app names

internal/api/domains_teardown_reserved_test.go

@@ -0,0 +1,30 @@
+package api
+
+import (
+	"context"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/mctlhq/mctl-api/internal/auth"
+	"github.com/mctlhq/mctl-api/internal/domains"
+	"github.com/mctlhq/mctl-api/internal/operations"
+)
+
+// A domain row whose team/service pair is now reserved must still be torn
+// down: DeleteDomain keeps the row when teardown fails, so refusing it here
+// would make the row undeletable and leave its ingress and TLS in place.
+func TestTriggerRemoveCustomDomain_ReservedPairStillTearsDown(t *testing.T) {
+	exec := &fakeDomainExecutor{}
+	h := &Handlers{opts: Options{Executor: exec, Registry: operations.NewRegistry()}}
+	d := &domains.Domain{Team: "labs", Service: "oidc-zitadel", Domain: "x.example.com", Status: domains.StatusActive}
+	r := httptest.NewRequest(http.MethodDelete, "/api/v1/domains/x", nil)
+	r = r.WithContext(auth.WithUser(context.Background(), auth.NewGitHubUser("u1", []string{"labs"})))
+	_, outcome, err := h.triggerRemoveCustomDomain(r.Context(), r, d)
+	if err != nil || outcome != cleanupSubmitted {
+		t.Fatalf("outcome %q, err %v; want the teardown submitted", outcome, err)
+	}
+	if len(exec.submitted) != 1 {
+		t.Fatalf("submitted %d, want 1", len(exec.submitted))
+	}
+}

internal/api/handlers_ci_deploy_test.go

@@ -62,9 +62,12 @@ func newCIFixture(t *testing.T, repos *fakeSourceRepos) *ciFixture {
 	opts := mctlapi.Options{
 		Registry: operations.NewRegistry(),
 		// No members at all: a CI deploy must not depend on tenant roles.
-		GitReader: &fakeGitReader{tenants: []gitops.Tenant{{Name: "labs"}}},
-		AuditLog:  f.audit,
-		Executor:  f.exec,
+		GitReader: &fakeGitReader{
+			tenants:  []gitops.Tenant{{Name: "labs"}},
+			services: []gitops.Service{{Team: "labs", Name: "mctl-telegram"}},
+		},
+		AuditLog: f.audit,
+		Executor: f.exec,
 	}
 	if repos != nil {
 		opts.ComponentSourceRepos = repos

internal/api/handlers_write.go

@@ -262,6 +262,46 @@ func (h *Handlers) ExecuteOperation(w http.ResponseWriter, r *http.Request) {
 		}
 	}
 
+	// A new tenant name must not share an Application name space with an
+	// existing tenant: "labs-x" next to "labs" (or the reverse) would let one
+	// tenant's <tenant>-<service> spell the other's Application. This applies
+	// to admins too; re-running create-tenant for the same name is not a
+	// collision. A tenant list that cannot be read refuses.
+	if opName == "create-tenant" {
+		if status, msg := h.checkTenantNameCollision(tenantParam); status != 0 {
+			h.logAudit(r, audit.Entry{
+				UserID:     user.ID,
+				Operation:  opName,
+				Parameters: auditParams,
+				Status:     "denied",
+				RiskLevel:  string(op.RiskLevel),
+				Message:    msg,
+			})
+			writeError(w, status, msg)
+			return
+		}
+	}
+
+	// An operation on an existing service must name one that exists for this
+	// team in gitops. The workflows build the Argo CD Application name as
+	// <team>-<component> and act on it, so without this check the pair is
+	// only as safe as the caller's choice of it. A checkout that cannot
+	// answer refuses: "could not check" is not "exists".
+	if team, component, ok := operations.ServiceTarget(op, input); ok && requiresExistingService(opName, input) {
+		if status, msg := h.checkServiceExists(team, component); status != 0 {
+			h.logAudit(r, audit.Entry{
+				UserID:     user.ID,
+				Operation:  opName,
+				Parameters: auditParams,
+				Status:     "denied",
+				RiskLevel:  string(op.RiskLevel),
+				Message:    msg,
+			})
+			writeError(w, status, msg)
+			return
+		}
+	}
+
 	// For create-tenant: also notify Backstage for immediate catalog sync.
 	if opName == "create-tenant" && h.opts.BackstageURL != "" {
 		go h.notifyBackstage(input)
@@ -281,7 +321,7 @@ func (h *Handlers) ExecuteOperation(w http.ResponseWriter, r *http.Request) {
 		// Executor.Submit refuses a reserved tenant name on its own. The
 		// validation above normally gets there first; mapping it here keeps
 		// the backstop from masquerading as a platform fault.
-		if errors.Is(err, operations.ErrReservedTenantName) {
+		if errors.Is(err, operations.ErrReservedTenantName) || errors.Is(err, operations.ErrReservedServiceApplication) {
 			writeError(w, http.StatusBadRequest, err.Error())
 			return
 		}
@@ -514,3 +554,57 @@ func (h *Handlers) checkTenantIsNew(tenant string) (status int, msg string) {
 	}
 	return 0, ""
 }
+
+// requiresExistingService reports whether opName, which names a service,
+// acts on one that must already exist. It is fail-closed: every such
+// operation is gated unless listed here. deploy-service's onboard action
+// creates the service; provision-database may run before a service is
+// onboarded and preview-delete must still clean up after one is retired, and
+// neither acts on the <team>-<component> Application.
+func requiresExistingService(opName string, input map[string]string) bool {
+	switch opName {
+	case "provision-database", "preview-delete":
+		return false
+	case "deploy-service":
+		return input["action"] != "onboard"
+	}
+	return true
+}
+
+// checkServiceExists returns 0 when team/component is a service in the
+// gitops checkout, 404 when the checkout says it is not, and 503 when the
+// checkout cannot tell.
+func (h *Handlers) checkServiceExists(team, component string) (status int, msg string) {
+	if h.opts.GitReader == nil {
+		return http.StatusServiceUnavailable, "cannot check whether service \"" + team + "/" + component + "\" exists: gitops reader not configured"
+	}
+	exists, err := h.opts.GitReader.ServiceExists(team, component)
+	if err != nil {
+		// The error can carry checkout paths; it goes to the log, not the
+		// caller.
+		slog.Error("service existence check failed", "team", team, "component", component, "error", err)
+		return http.StatusServiceUnavailable, "cannot check whether service \"" + team + "/" + component + "\" exists right now; try again later"
+	}
+	if !exists {
+		return http.StatusNotFound, "service \"" + component + "\" not found in workspace \"" + team + "\" (a service created in the last minute may not be visible yet)"
+	}
+	return 0, ""
+}
+
+// checkTenantNameCollision returns 0 when tenant shares no hyphen prefix with
+// an existing tenant, 400 when it does, and 503 when the tenant list cannot
+// be read.
+func (h *Handlers) checkTenantNameCollision(tenant string) (status int, msg string) {
+	if h.opts.GitReader == nil {
+		return http.StatusServiceUnavailable, "cannot check workspace name \"" + tenant + "\": gitops reader not configured"
+	}
+	names, err := h.opts.GitReader.TenantNames()
+	if err != nil {
+		slog.Error("create-tenant name collision check failed", "tenant", tenant, "error", err)
+		return http.StatusServiceUnavailable, "cannot check workspace name \"" + tenant + "\" right now; try again later"
+	}
+	if other, collides := operations.TenantNameCollision(tenant, names); collides {
+		return http.StatusBadRequest, "tenant_name: \"" + tenant + "\" overlaps the name of existing workspace \"" + other + "\"; choose a name that neither extends nor is extended by another workspace name"
+	}
+	return 0, ""
+}

internal/api/interfaces.go

@@ -38,6 +38,13 @@ type GitReader interface {
 	ListTenants() ([]gitops.Tenant, error)
 	GetTenant(name string) (*gitops.Tenant, error)
 	TenantExists(name string) (bool, error)
+	// TenantNames lists every tenant name in the checkout; any read failure
+	// is an error, never a shorter list.
+	TenantNames() ([]string, error)
+	// ServiceExists reports whether services/<team>/<component> is a
+	// service in the checkout. Only a missing path is (false, nil); a read
+	// that failed is an error.
+	ServiceExists(team, component string) (bool, error)
 	// MemberRoles returns the members[].role values under which login is a
 	// member of namespace, as written, one list per members list naming the
 	// login. Empty with a nil error is "read, and not a member"; anything it