NO-JIRA: test: add RBAC e2e tests for create and delete endpoints

internal/managementrouter/router.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/gorilla/mux"
 	"github.com/sirupsen/logrus"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 
 	"github.com/openshift/monitoring-plugin/pkg/k8s"
 	"github.com/openshift/monitoring-plugin/pkg/management"
@@ -62,6 +63,7 @@ func authMiddleware(next http.Handler) http.Handler {
 	})
 }
 
+// writeError sends a JSON {"error": message} response with the given status code.
 func writeError(w http.ResponseWriter, statusCode int, message string) {
 	w.Header().Set("Content-Type", "application/json")
 	w.WriteHeader(statusCode)
@@ -75,28 +77,38 @@ func writeError(w http.ResponseWriter, statusCode int, message string) {
 	}
 }
 
+// handleError maps err to an HTTP status via parseError and writes the response.
 func handleError(w http.ResponseWriter, err error) {
 	status, message := parseError(err)
 	writeError(w, status, message)
 }
 
+// parseError inspects err and returns a (statusCode, userMessage) pair.
+// Kubernetes auth errors are checked first to prevent information leakage;
+// domain errors are then mapped to 4xx codes.
 func parseError(err error) (int, string) {
-	var nf *management.NotFoundError
-	if errors.As(err, &nf) {
+	var (
+		nf *management.NotFoundError
+		ve *management.ValidationError
+		na *management.NotAllowedError
+		ce *management.ConflictError
+	)
+
+	switch {
+	case apierrors.IsUnauthorized(err):
+		return http.StatusUnauthorized, "authentication failed"
+	case apierrors.IsForbidden(err):
+		return http.StatusForbidden, "insufficient permissions"
+	case errors.As(err, &nf):
 		return http.StatusNotFound, err.Error()
-	}
-	var ve *management.ValidationError
-	if errors.As(err, &ve) {
+	case errors.As(err, &ve):
 		return http.StatusBadRequest, err.Error()
-	}
-	var na *management.NotAllowedError
-	if errors.As(err, &na) {
+	case errors.As(err, &na):
 		return http.StatusMethodNotAllowed, err.Error()
-	}
-	var ce *management.ConflictError
-	if errors.As(err, &ce) {
+	case errors.As(err, &ce):
 		return http.StatusConflict, err.Error()
+	default:
+		log.WithError(err).Error("unexpected management API error")
+		return http.StatusInternalServerError, "An unexpected error occurred"
 	}
-	log.WithError(err).Error("unexpected management API error")
-	return http.StatusInternalServerError, "An unexpected error occurred"
 }

internal/managementrouter/router_test.go

@@ -0,0 +1,83 @@
+package managementrouter
+
+import (
+	"fmt"
+	"net/http"
+	"testing"
+
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+
+	"github.com/openshift/monitoring-plugin/pkg/management"
+)
+
+func TestParseError(t *testing.T) {
+	tests := []struct {
+		name           string
+		err            error
+		expectedStatus int
+		expectedMsg    string
+	}{
+		{
+			name:           "NotFoundError",
+			err:            &management.NotFoundError{Resource: "AlertRule", Id: "abc"},
+			expectedStatus: http.StatusNotFound,
+		},
+		{
+			name:           "ValidationError",
+			err:            &management.ValidationError{Message: "bad input"},
+			expectedStatus: http.StatusBadRequest,
+		},
+		{
+			name:           "NotAllowedError",
+			err:            &management.NotAllowedError{Message: "not allowed"},
+			expectedStatus: http.StatusMethodNotAllowed,
+		},
+		{
+			name:           "ConflictError",
+			err:            &management.ConflictError{Message: "conflict"},
+			expectedStatus: http.StatusConflict,
+		},
+		{
+			name: "Kubernetes Forbidden",
+			err: apierrors.NewForbidden(schema.GroupResource{
+				Group: "monitoring.coreos.com", Resource: "prometheusrules",
+			}, "test-pr", fmt.Errorf("access denied")),
+			expectedStatus: http.StatusForbidden,
+			expectedMsg:    "insufficient permissions",
+		},
+		{
+			name: "Kubernetes Forbidden wrapped",
+			err: fmt.Errorf("failed to get PrometheusRule: %w",
+				apierrors.NewForbidden(schema.GroupResource{
+					Group: "monitoring.coreos.com", Resource: "prometheusrules",
+				}, "test-pr", fmt.Errorf("access denied"))),
+			expectedStatus: http.StatusForbidden,
+			expectedMsg:    "insufficient permissions",
+		},
+		{
+			name:           "Kubernetes Unauthorized",
+			err:            apierrors.NewUnauthorized("token expired"),
+			expectedStatus: http.StatusUnauthorized,
+			expectedMsg:    "authentication failed",
+		},
+		{
+			name:           "unknown error",
+			err:            fmt.Errorf("something unexpected"),
+			expectedStatus: http.StatusInternalServerError,
+			expectedMsg:    "An unexpected error occurred",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			status, msg := parseError(tt.err)
+			if status != tt.expectedStatus {
+				t.Errorf("expected status %d, got %d", tt.expectedStatus, status)
+			}
+			if tt.expectedMsg != "" && msg != tt.expectedMsg {
+				t.Errorf("expected message %q, got %q", tt.expectedMsg, msg)
+			}
+		})
+	}
+}

pkg/k8s/user_scoped_client.go

@@ -13,14 +13,21 @@ type userScopedClientsets struct {
 	osmV1        *osmv1client.Clientset
 }
 
+// buildUserScopedConfig creates a rest.Config that authenticates exclusively
+// with the given bearer token. It uses AnonymousClientConfig to strip all
+// existing auth (certs, basic auth, auth/exec providers, impersonation) while
+// preserving the server connection settings (host, TLS CA, proxy).
+func buildUserScopedConfig(baseConfig *rest.Config, userToken string) *rest.Config {
+	cfg := rest.AnonymousClientConfig(baseConfig)
+	cfg.BearerToken = userToken
+	return cfg
+}
+
 // newUserScopedClientsets creates clientsets that carry the supplied bearer
 // token so that Kubernetes RBAC is enforced for the requesting user on all
 // mutating API calls.
 func newUserScopedClientsets(baseConfig *rest.Config, userToken string) (*userScopedClientsets, error) {
-	cfg := rest.CopyConfig(baseConfig)
-	// Override any SA token loaded from the file system with the user's token.
-	cfg.BearerToken = userToken
-	cfg.BearerTokenFile = ""
+	cfg := buildUserScopedConfig(baseConfig, userToken)
 
 	monClient, err := monitoringv1client.NewForConfig(cfg)
 	if err != nil {

pkg/k8s/user_scoped_client_test.go

@@ -0,0 +1,64 @@
+package k8s
+
+import (
+	"testing"
+
+	"k8s.io/client-go/rest"
+)
+
+func TestBuildUserScopedConfig(t *testing.T) {
+	base := &rest.Config{
+		Host:            "https://api.example.com:6443",
+		BearerToken:     "sa-token",
+		BearerTokenFile: "/var/run/secrets/kubernetes.io/serviceaccount/token",
+		TLSClientConfig: rest.TLSClientConfig{
+			Insecure: true,
+			CertData: []byte("admin-cert"),
+			KeyData:  []byte("admin-key"),
+			CertFile: "/path/to/cert",
+			KeyFile:  "/path/to/key",
+		},
+	}
+
+	cfg := buildUserScopedConfig(base, "user-token")
+
+	// Derived config uses the user token exclusively.
+	if cfg.BearerToken != "user-token" {
+		t.Errorf("derived BearerToken = %q, want %q", cfg.BearerToken, "user-token")
+	}
+	if cfg.BearerTokenFile != "" {
+		t.Errorf("derived BearerTokenFile = %q, want empty", cfg.BearerTokenFile)
+	}
+	if cfg.CertData != nil {
+		t.Error("derived CertData should be nil")
+	}
+	if cfg.KeyData != nil {
+		t.Error("derived KeyData should be nil")
+	}
+	if cfg.CertFile != "" {
+		t.Errorf("derived CertFile = %q, want empty", cfg.CertFile)
+	}
+	if cfg.KeyFile != "" {
+		t.Errorf("derived KeyFile = %q, want empty", cfg.KeyFile)
+	}
+	if !cfg.Insecure {
+		t.Error("derived Insecure should be preserved as true")
+	}
+	if cfg.Host != base.Host {
+		t.Errorf("derived Host = %q, want %q", cfg.Host, base.Host)
+	}
+
+	// Base config must not be mutated.
+	if base.CertData == nil {
+		t.Error("base CertData was mutated")
+	}
+	if base.KeyData == nil {
+		t.Error("base KeyData was mutated")
+	}
+	if base.BearerToken != "sa-token" {
+		t.Errorf("base BearerToken = %q, want %q", base.BearerToken, "sa-token")
+	}
+	if base.BearerTokenFile != "/var/run/secrets/kubernetes.io/serviceaccount/token" {
+		t.Errorf("base BearerTokenFile = %q, was mutated", base.BearerTokenFile)
+	}
+}

test/e2e/create_alert_rule_test.go

@@ -3,9 +3,14 @@
 package e2e
 
 import (
+	"bytes"
 	"context"
+	"encoding/json"
 	"errors"
 	"fmt"
+	"io"
+	"net/http"
+	"net/url"
 	"testing"
 	"time"
 
@@ -87,3 +92,114 @@ func TestCreateUserDefinedAlertRule(t *testing.T) {
 	})
 	require.NoError(t, err)
 }
+
+// TestRBAC_CreateAlertRule verifies that the create endpoint enforces Kubernetes
+// RBAC across three user profiles: anonymous (403), namespace-scoped (201 in
+// own namespace, 403 elsewhere), and cluster-admin (201 everywhere).
+func TestRBAC_CreateAlertRule(t *testing.T) {
+	f, err := framework.New()
+	if err != nil {
+		t.Fatalf("Failed to create framework: %v", err)
+	}
+
+	ctx := context.Background()
+
+	nsY, cleanupY, err := f.CreateUserNamespace(ctx, "test-rbac-create-y")
+	if err != nil {
+		t.Fatalf("Failed to create namespace Y: %v", err)
+	}
+	defer func() { _ = cleanupY() }()
+
+	nsZ, cleanupZ, err := f.CreateUserNamespace(ctx, "test-rbac-create-z")
+	if err != nil {
+		t.Fatalf("Failed to create namespace Z: %v", err)
+	}
+	defer func() { _ = cleanupZ() }()
+
+	anonymousUser, err := f.CreateAnonymousUser(ctx, "e2e-rbac-user-a", "default")
+	if err != nil {
+		t.Fatalf("Failed to create anonymous user: %v", err)
+	}
+	defer func() { _ = anonymousUser.Cleanup() }()
+
+	userScopedToNamespaceY, err := f.CreateScopedUser(ctx, "e2e-rbac-user-b", nsY,
+		"monitoring.coreos.com", []string{"prometheusrules"}, []string{"get", "create", "update", "patch"})
+	if err != nil {
+		t.Fatalf("Failed to create scoped user for namespace Y: %v", err)
+	}
+	defer func() { _ = userScopedToNamespaceY.Cleanup() }()
+
+	cases := []struct {
+		name       string
+		token      string
+		namespace  string
+		alertName  string
+		wantStatus int
+	}{
+		{"AnonymousUser_FailsNamespaceY", anonymousUser.Token, nsY, "RBACAlertA", http.StatusForbidden},
+		{"ScopedUser_SucceedsNamespaceY", userScopedToNamespaceY.Token, nsY, "RBACAlertBY", http.StatusCreated},
+		{"ScopedUser_FailsNamespaceZ", userScopedToNamespaceY.Token, nsZ, "RBACAlertBZ", http.StatusForbidden},
+		{"ClusterAdmin_SucceedsNamespaceY", f.BearerToken, nsY, "RBACAlertCY", http.StatusCreated},
+		{"ClusterAdmin_SucceedsNamespaceZ", f.BearerToken, nsZ, "RBACAlertCZ", http.StatusCreated},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			status := createAlertRuleWithToken(t, f, ctx, tc.token, tc.namespace, tc.alertName)
+			if status != tc.wantStatus {
+				t.Fatalf("Expected status %d, got %d", tc.wantStatus, status)
+			}
+		})
+	}
+}
+
+// createAlertRuleWithToken sends a create alert rule request using the given
+// bearer token and returns the HTTP status code.
+func createAlertRuleWithToken(t *testing.T, f *framework.Framework, ctx context.Context, token, namespace, alertName string) int {
+	t.Helper()
+
+	expr := fmt.Sprintf("absent(nonexistent{e2e_rbac_create=%q})", alertName)
+	payload := managementrouter.CreateAlertRuleRequest{
+		AlertingRule: &managementrouter.AlertRuleSpec{
+			Alert: &alertName,
+			Expr:  &expr,
+			Labels: &map[string]string{
+				"severity": "info",
+			},
+		},
+		PrometheusRule: &managementrouter.PrometheusRuleTarget{
+			PrometheusRuleName:      "e2e-rbac-pr",
+			PrometheusRuleNamespace: namespace,
+		},
+	}
+
+	reqBody, err := json.Marshal(payload)
+	if err != nil {
+		t.Fatalf("Failed to marshal create request: %v", err)
+	}
+
+	createURL, err := url.JoinPath(f.PluginURL, "api/v1/alerting/rules")
+	if err != nil {
+		t.Fatalf("Failed to build URL: %v", err)
+	}
+
+	req, err := http.NewRequestWithContext(ctx, http.MethodPost, createURL, bytes.NewBuffer(reqBody))
+	if err != nil {
+		t.Fatalf("Failed to create HTTP request: %v", err)
+	}
+	req.Header.Set("Content-Type", "application/json")
+	req.Header.Set("Authorization", "Bearer "+token)
+
+	resp, err := f.HTTPClient().Do(req)
+	if err != nil {
+		t.Fatalf("Failed to make create request: %v", err)
+	}
+	defer resp.Body.Close()
+
+	if resp.StatusCode != http.StatusCreated {
+		body, _ := io.ReadAll(resp.Body)
+		t.Logf("Create %s in %s: status %d, body: %s", alertName, namespace, resp.StatusCode, string(body))
+	}
+
+	return resp.StatusCode
+}