refactor(core): replace update helpers with CRUD verbs

design/EP-1270-scoped-authorization.md

@@ -48,11 +48,13 @@ Unsupported or invalid authorization decisions fail closed.
 
 ## Resource identity
 
-Authorization uses identity derived from stored or validated resource data. A request reference identifies what to load; it is not trusted evidence about the resource itself.
+Authorization uses namespace and name, the only attributes in scope. Each decision is made before the operation it authorizes.
 
-- Reads and deletes are decided from the stored resource.
+- Reads and deletes are decided from their references before the read, so denial does not reveal whether the resource exists.
 - Creates are decided from the validated proposed resource.
-- Updates are decided from the stored resource and preserve its namespace and name.
+- Updates authorize get before the read and update before the write. Generic updates use the identity of the loaded resource.
+
+A service that writes through its own client authorizes update from the request reference before its first side effect.
 
 ## Collection behavior
 

go/core/internal/grpcserver/agenttemplate.go

@@ -82,13 +82,13 @@ func (s *agentTemplateServer) UpdateAgentTemplate(ctx context.Context, request *
 	if err := s.decodeResource(request.GetRef(), request.GetResource(), incoming); err != nil {
 		return nil, err
 	}
-	existing, err := s.service.GetForUpdate(ctx, ref)
+	existing, err := s.service.Get(ctx, ref)
 	if err != nil {
 		return nil, err
 	}
 	existing.Spec = *incoming.Spec.DeepCopy()
 	existing.Labels = maps.Clone(incoming.Labels)
-	result, err := s.service.SaveUpdate(ctx, existing)
+	result, err := s.service.Update(ctx, existing)
 	if err != nil {
 		return nil, err
 	}

go/core/internal/service/kubecrud/service.go

@@ -78,10 +78,7 @@ func (s *Service[T, L]) List(ctx context.Context, namespace string) ([]T, error)
 
 func (s *Service[T, L]) Get(ctx context.Context, ref types.NamespacedName) (T, error) {
 	var zero T
-	if err := s.validateRef(ref); err != nil {
-		return zero, err
-	}
-	if err := s.authorize(ctx, auth.VerbGet, ref); err != nil {
+	if err := s.Authorize(ctx, auth.VerbGet, ref); err != nil {
 		return zero, err
 	}
 	return s.get(ctx, ref)
@@ -113,21 +110,16 @@ func (s *Service[T, L]) Create(ctx context.Context, object T) (T, error) {
 	return object, nil
 }
 
-// GetForUpdate authorizes an update and loads the live object that owns metadata and status.
-func (s *Service[T, L]) GetForUpdate(ctx context.Context, ref types.NamespacedName) (T, error) {
+// Update persists an object read from the cluster, authorized by the identity it is about to write.
+func (s *Service[T, L]) Update(ctx context.Context, object T) (T, error) {
 	var zero T
-	if err := s.validateRef(ref); err != nil {
-		return zero, err
+	if object == zero {
+		return zero, serviceerrors.NewInvalidArgument(s.resource+" resource is required", nil)
 	}
-	if err := s.authorize(ctx, auth.VerbUpdate, ref); err != nil {
+	ref := types.NamespacedName{Namespace: object.GetNamespace(), Name: object.GetName()}
+	if err := s.Authorize(ctx, auth.VerbUpdate, ref); err != nil {
 		return zero, err
 	}
-	return s.get(ctx, ref)
-}
-
-// SaveUpdate persists an object returned by GetForUpdate after its spec is changed.
-func (s *Service[T, L]) SaveUpdate(ctx context.Context, object T) (T, error) {
-	var zero T
 	if err := s.client.Update(ctx, object); err != nil {
 		if apierrors.IsInvalid(err) {
 			return zero, serviceerrors.NewInvalidArgument("Invalid "+s.resource, err)
@@ -138,10 +130,7 @@ func (s *Service[T, L]) SaveUpdate(ctx context.Context, object T) (T, error) {
 }
 
 func (s *Service[T, L]) Delete(ctx context.Context, ref types.NamespacedName) error {
-	if err := s.validateRef(ref); err != nil {
-		return err
-	}
-	if err := s.authorize(ctx, auth.VerbDelete, ref); err != nil {
+	if err := s.Authorize(ctx, auth.VerbDelete, ref); err != nil {
 		return err
 	}
 	object, err := s.get(ctx, ref)
@@ -178,7 +167,14 @@ func (s *Service[T, L]) get(ctx context.Context, ref types.NamespacedName) (T, e
 	return object, nil
 }
 
-// authorize decides a single operation before any read, so a denial never depends on the object existing.
+// Authorize validates a reference and checks one operation without accessing Kubernetes.
+func (s *Service[T, L]) Authorize(ctx context.Context, verb auth.Verb, ref types.NamespacedName) error {
+	if err := s.validateRef(ref); err != nil {
+		return err
+	}
+	return s.authorize(ctx, verb, ref)
+}
+
 func (s *Service[T, L]) authorize(ctx context.Context, verb auth.Verb, ref types.NamespacedName) error {
 	session, ok := auth.AuthSessionFrom(ctx)
 	if !ok || session == nil {
@@ -198,10 +194,8 @@ func (s *Service[T, L]) validateRef(ref types.NamespacedName) error {
 	return nil
 }
 
+// validateNewRef rejects a ref Kubernetes would not accept; an empty value is not a valid subdomain either.
 func (s *Service[T, L]) validateNewRef(ref types.NamespacedName) error {
-	if err := s.validateRef(ref); err != nil {
-		return err
-	}
 	if len(utilvalidation.IsDNS1123Subdomain(ref.Namespace)) > 0 {
 		return serviceerrors.NewInvalidArgument("namespace must be a valid DNS subdomain", nil)
 	}

go/core/internal/service/kubecrud/service_test.go

@@ -88,20 +88,20 @@ func TestServiceFiltersBeforeSortingAndUsesTrustedAttributes(t *testing.T) {
 		t.Fatalf("Create() error = %v", err)
 	}
 	mutableRef := types.NamespacedName{Namespace: "team", Name: "mutable"}
-	mutable, err := service.GetForUpdate(ctx, mutableRef)
+	mutable, err := service.Get(ctx, mutableRef)
 	if err != nil {
-		t.Fatalf("GetForUpdate() error = %v", err)
+		t.Fatalf("Get() error = %v", err)
 	}
 	mutable.Spec.Description = "updated"
-	if _, err := service.SaveUpdate(ctx, mutable); err != nil {
-		t.Fatalf("SaveUpdate() error = %v", err)
+	if _, err := service.Update(ctx, mutable); err != nil {
+		t.Fatalf("Update() error = %v", err)
 	}
 	if err := service.Delete(ctx, types.NamespacedName{Namespace: "team", Name: "b"}); err != nil {
 		t.Fatalf("Delete() error = %v", err)
 	}
 
-	wantVerbs := []auth.Verb{auth.VerbGet, auth.VerbCreate, auth.VerbUpdate, auth.VerbDelete}
-	wantNames := []string{"a", "created", "mutable", "b"}
+	wantVerbs := []auth.Verb{auth.VerbGet, auth.VerbCreate, auth.VerbGet, auth.VerbUpdate, auth.VerbDelete}
+	wantNames := []string{"a", "created", "mutable", "mutable", "b"}
 	if len(authorizer.checkCalls) != len(wantVerbs) {
 		t.Fatalf("Check() calls = %d, want %d", len(authorizer.checkCalls), len(wantVerbs))
 	}
@@ -171,17 +171,23 @@ func TestServiceRejectsInvalidScope(t *testing.T) {
 	}
 }
 
-// readRecordingClient counts the reads that reach Kubernetes.
+// readRecordingClient counts the reads and updates that reach Kubernetes.
 type readRecordingClient struct {
 	client.Client
-	gets int
+	gets    int
+	updates int
 }
 
 func (c *readRecordingClient) Get(ctx context.Context, key client.ObjectKey, object client.Object, options ...client.GetOption) error {
 	c.gets++
 	return c.Client.Get(ctx, key, object, options...)
 }
 
+func (c *readRecordingClient) Update(ctx context.Context, object client.Object, options ...client.UpdateOption) error {
+	c.updates++
+	return c.Client.Update(ctx, object, options...)
+}
+
 // A denied caller must not be able to tell an existing object from a missing one.
 func TestDeniedSingleResourceOperationsDoNotRevealExistence(t *testing.T) {
 	scheme := runtime.NewScheme()
@@ -197,10 +203,6 @@ func TestDeniedSingleResourceOperationsDoNotRevealExistence(t *testing.T) {
 			_, err := s.Get(ctx, ref)
 			return err
 		},
-		"GetForUpdate": func(s *kubecrud.Service[*v1alpha3.AgentTemplate, *v1alpha3.AgentTemplateList], ctx context.Context, ref types.NamespacedName) error {
-			_, err := s.GetForUpdate(ctx, ref)
-			return err
-		},
 		"Delete": func(s *kubecrud.Service[*v1alpha3.AgentTemplate, *v1alpha3.AgentTemplateList], ctx context.Context, ref types.NamespacedName) error {
 			return s.Delete(ctx, ref)
 		},
@@ -227,6 +229,25 @@ func TestDeniedSingleResourceOperationsDoNotRevealExistence(t *testing.T) {
 	}
 }
 
+func TestDeniedUpdateDoesNotWrite(t *testing.T) {
+	scheme := runtime.NewScheme()
+	if err := v1alpha3.AddToScheme(scheme); err != nil {
+		t.Fatal(err)
+	}
+	kubeClient := &readRecordingClient{Client: fake.NewClientBuilder().WithScheme(scheme).Build()}
+	authorizer := &recordingAuthorizer{checkErr: errors.New("denied")}
+	service := kubecrud.NewService(kubeClient, authorizer, &v1alpha3.AgentTemplate{}, &v1alpha3.AgentTemplateList{}, "AgentTemplate")
+	ctx := auth.AuthSessionTo(t.Context(), testSession{})
+
+	_, err := service.Update(ctx, &v1alpha3.AgentTemplate{ObjectMeta: metav1.ObjectMeta{Namespace: "team", Name: "existing"}})
+	if !serviceerrors.IsCode(err, serviceerrors.CodePermissionDenied) {
+		t.Fatalf("Update() error = %v, want permission denied", err)
+	}
+	if kubeClient.updates != 0 {
+		t.Fatalf("Update() denied the caller but wrote Kubernetes %d times", kubeClient.updates)
+	}
+}
+
 // A cluster-wide list must order same-named objects deterministically.
 func TestListOrdersAcrossNamespaces(t *testing.T) {
 	scheme := runtime.NewScheme()

go/core/internal/service/model/service.go

@@ -145,9 +145,12 @@ func (s *Service) createOwnedSecrets(ctx context.Context, modelConfig *v1alpha3.
 }
 
 // Update keeps its own write because the owned Secrets must land between the
-// authorized read and the retrying ModelConfig write.
+// authorized read and the retrying ModelConfig write; the update is authorized first.
 func (s *Service) Update(ctx context.Context, request UpdateRequest) (*v1alpha3.ModelConfig, error) {
-	modelConfig, err := s.modelConfigs.GetForUpdate(ctx, request.Ref)
+	if err := s.modelConfigs.Authorize(ctx, auth.VerbUpdate, request.Ref); err != nil {
+		return nil, err
+	}
+	modelConfig, err := s.modelConfigs.Get(ctx, request.Ref)
 	if err != nil {
 		return nil, err
 	}

go/core/internal/service/model/service_test.go

@@ -335,27 +335,6 @@ func TestServiceCRUDAndValidation(t *testing.T) {
 		assert.Error(t, err)
 	})
 
-	t.Run("update permission denied before write", func(t *testing.T) {
-		config := &v1alpha3.ModelConfig{
-			ObjectMeta: metav1.ObjectMeta{Name: "cfg", Namespace: "default"},
-			Spec:       v1alpha3.ModelConfigSpec{Model: "original", Provider: v1alpha3.ModelProviderOpenAI},
-		}
-		authorizer := &recordingAuthorizer{denyCheck: 1}
-		service, kubeClient, ctx := newService(authorizer, config)
-
-		_, err := service.Update(ctx, model.UpdateRequest{
-			Ref:  types.NamespacedName{Namespace: "default", Name: "cfg"},
-			Spec: v1alpha3.ModelConfigSpec{Model: "updated", Provider: v1alpha3.ModelProviderOpenAI},
-		})
-		require.Error(t, err)
-		assert.True(t, serviceerrors.IsCode(err, serviceerrors.CodePermissionDenied))
-		require.Len(t, authorizer.checkCalls, 1)
-
-		stored := &v1alpha3.ModelConfig{}
-		require.NoError(t, kubeClient.Get(ctx, ctrlclient.ObjectKey{Namespace: "default", Name: "cfg"}, stored))
-		assert.Equal(t, "original", stored.Spec.Model)
-	})
-
 	t.Run("denied collection is empty and denied item is rejected", func(t *testing.T) {
 		service, _, ctx := newService(denyAuthorizer{}, &v1alpha3.ModelConfig{
 			ObjectMeta: metav1.ObjectMeta{Name: "cfg", Namespace: "default"},
@@ -443,8 +422,8 @@ func TestModelConfigCRUDUsesTrustedAttributes(t *testing.T) {
 		t.Fatalf("Delete() error = %v", err)
 	}
 
-	wantVerbs := []pkgauth.Verb{pkgauth.VerbGet, pkgauth.VerbCreate, pkgauth.VerbUpdate, pkgauth.VerbDelete}
-	wantNames := []string{"existing", "created", "existing", "existing"}
+	wantVerbs := []pkgauth.Verb{pkgauth.VerbGet, pkgauth.VerbCreate, pkgauth.VerbUpdate, pkgauth.VerbGet, pkgauth.VerbDelete}
+	wantNames := []string{"existing", "created", "existing", "existing", "existing"}
 	require.Len(t, authorizer.checkCalls, len(wantVerbs))
 	for index, call := range authorizer.checkCalls {
 		assert.Equal(t, wantVerbs[index], call.verb)
@@ -454,6 +433,41 @@ func TestModelConfigCRUDUsesTrustedAttributes(t *testing.T) {
 	}
 }
 
+// A caller who may read but not update must not have its API key written on the way to the denial.
+func TestDeniedUpdateWritesNothing(t *testing.T) {
+	scheme := runtime.NewScheme()
+	require.NoError(t, v1alpha3.AddToScheme(scheme))
+	require.NoError(t, corev1.AddToScheme(scheme))
+	ref := types.NamespacedName{Namespace: "team", Name: "existing"}
+	kubeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&v1alpha3.ModelConfig{
+		ObjectMeta: metav1.ObjectMeta{Namespace: ref.Namespace, Name: ref.Name},
+		Spec:       v1alpha3.ModelConfigSpec{Model: "old", Provider: v1alpha3.ModelProviderOpenAI},
+	}).Build()
+	authorizer := &recordingAuthorizer{scope: apiauthorization.AuthorizationScope{Kind: apiauthorization.ScopeAll}, denyCheck: 1}
+	service := model.NewService(kubeClient, authorizer, "default")
+	ctx := pkgauth.AuthSessionTo(context.Background(), &authimpl.SimpleSession{P: pkgauth.Principal{User: pkgauth.User{ID: "test-user"}}})
+
+	apiKey := "api-key-value"
+	_, err := service.Update(ctx, model.UpdateRequest{
+		Ref:    ref,
+		APIKey: &apiKey,
+		Spec:   v1alpha3.ModelConfigSpec{Model: "updated", Provider: v1alpha3.ModelProviderOpenAI},
+	})
+	require.Error(t, err)
+	assert.Truef(t, serviceerrors.IsCode(err, serviceerrors.CodePermissionDenied), "got %v", err)
+
+	// The update decision is the only one reached: a read before it would record a get first.
+	assert.Equal(t, []authorizationCall{
+		{verb: pkgauth.VerbUpdate, resource: pkgauth.Resource{Type: "ModelConfig", Namespace: ref.Namespace, Name: ref.Name}},
+	}, authorizer.checkCalls)
+
+	err = kubeClient.Get(ctx, ref, &corev1.Secret{})
+	assert.True(t, apierrors.IsNotFound(err), "denied update wrote its API key secret: %v", err)
+	stored := &v1alpha3.ModelConfig{}
+	require.NoError(t, kubeClient.Get(ctx, ref, stored))
+	assert.Equal(t, "old", stored.Spec.Model)
+}
+
 // A create that cannot finish its Secrets must not leave the ModelConfig behind.
 func TestCreateRollsBackWhenSecretWriteFails(t *testing.T) {
 	scheme := runtime.NewScheme()