fix: Let users open conversation history and send messages

config/milo/iam/resources/conversations.yaml

@@ -35,6 +35,14 @@ spec:
     - update
     - patch
     - delete
+  # Checked with no fallback to the base permissions above.
+  subresources:
+    - name: messages
+      permissions:
+        - get
+    - name: sendmessage
+      permissions:
+        - create
   # Conversations are scoped to a project: the SAR carries the project name as
   # the request namespace (internal/tenant treats the namespace as
   # authoritative). Organization is listed so an org-level grant is inherited

config/milo/iam/roles/conversation-creator.yaml

@@ -1,6 +1,6 @@
-# The grant that makes Patch usable. This single permission is what the
-# assistant's SubjectAccessReview asks for on every request; a subject without
-# it gets a 403 before the agent loop is ever entered.
+# The grant that makes Patch usable. These permissions are what A2A and
+# sendmessage requests ask for; a subject without them gets a 403 before the
+# agent loop is ever entered.
 apiVersion: iam.miloapis.com/v1alpha1
 kind: Role
 metadata:
@@ -13,3 +13,4 @@ spec:
   launchStage: Beta
   includedPermissions:
     - assistant.miloapis.com/conversations.create
+    - assistant.miloapis.com/conversations/sendmessage.create

config/milo/iam/roles/conversation-viewer.yaml

@@ -23,6 +23,7 @@ spec:
   includedPermissions:
     - assistant.miloapis.com/conversations.get
     - assistant.miloapis.com/conversations.list
+    - assistant.miloapis.com/conversations/messages.get
     - assistant.miloapis.com/capabilitygapreports.list
     - assistant.miloapis.com/capabilitygaps.list
     - assistant.miloapis.com/assistantendpoints.get

internal/apiserver/apiserver.go

@@ -113,15 +113,7 @@ func (c completedConfig) New() (*ConversationServer, error) {
 
 	apiGroupInfo := genericapiserver.NewDefaultAPIGroupInfo(v1alpha1.GroupName, Scheme, metav1.ParameterCodec, Codecs)
 
-	v1alpha1Storage := map[string]rest.Storage{
-		"conversations":             conversation.NewConversationREST(c.ExtraConfig.Conversations),
-		"conversations/messages":    conversation.NewMessagesREST(c.ExtraConfig.Conversations),
-		"conversations/sendmessage": conversation.NewSendMessageREST(c.ExtraConfig.Runner),
-		"capabilitygapreports":      capabilitygapreport.NewCapabilityGapReportREST(c.ExtraConfig.GapReports),
-		"capabilitygaps":            capabilitygapreport.NewCapabilityGapREST(c.ExtraConfig.GapReports),
-		"assistantendpoints":        endpoint.NewAssistantEndpointREST(c.ExtraConfig.PublicBaseURL),
-	}
-	apiGroupInfo.VersionedResourcesStorageMap["v1alpha1"] = v1alpha1Storage
+	apiGroupInfo.VersionedResourcesStorageMap["v1alpha1"] = v1alpha1Storage(c.ExtraConfig)
 
 	if err := s.GenericAPIServer.InstallAPIGroup(&apiGroupInfo); err != nil {
 		return nil, err
@@ -130,3 +122,14 @@ func (c completedConfig) New() (*ConversationServer, error) {
 	klog.Info("conversations apiserver initialized")
 	return s, nil
 }
+
+func v1alpha1Storage(extra *ExtraConfig) map[string]rest.Storage {
+	return map[string]rest.Storage{
+		"conversations":             conversation.NewConversationREST(extra.Conversations),
+		"conversations/messages":    conversation.NewMessagesREST(extra.Conversations),
+		"conversations/sendmessage": conversation.NewSendMessageREST(extra.Runner),
+		"capabilitygapreports":      capabilitygapreport.NewCapabilityGapReportREST(extra.GapReports),
+		"capabilitygaps":            capabilitygapreport.NewCapabilityGapREST(extra.GapReports),
+		"assistantendpoints":        endpoint.NewAssistantEndpointREST(extra.PublicBaseURL),
+	}
+}

internal/apiserver/iam_test.go

@@ -0,0 +1,184 @@
+package apiserver
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"path/filepath"
+	"sort"
+	"strings"
+	"testing"
+
+	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/apiserver/pkg/endpoints/request"
+	"sigs.k8s.io/yaml"
+
+	"github.com/milo-os/assistant/pkg/apis/assistant/v1alpha1"
+)
+
+const iamDir = "../../config/milo/iam"
+
+type protectedResource struct {
+	Spec struct {
+		ServiceRef struct {
+			Name string `json:"name"`
+		} `json:"serviceRef"`
+		Plural       string   `json:"plural"`
+		Permissions  []string `json:"permissions"`
+		Subresources []struct {
+			Name        string   `json:"name"`
+			Permissions []string `json:"permissions"`
+		} `json:"subresources"`
+	} `json:"spec"`
+}
+
+type role struct {
+	Metadata struct {
+		Name string `json:"name"`
+	} `json:"metadata"`
+	Spec struct {
+		IncludedPermissions []string `json:"includedPermissions"`
+	} `json:"spec"`
+}
+
+func readYAML(t *testing.T, path string, into any) {
+	t.Helper()
+	data, err := os.ReadFile(path)
+	if err != nil {
+		t.Fatalf("read %s: %v", path, err)
+	}
+	if err := yaml.Unmarshal(data, into); err != nil {
+		t.Fatalf("parse %s: %v", path, err)
+	}
+}
+
+func globYAML(t *testing.T, dir string) []string {
+	t.Helper()
+	files, err := filepath.Glob(filepath.Join(iamDir, dir, "*.yaml"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	var out []string
+	for _, f := range files {
+		if filepath.Base(f) != "kustomization.yaml" {
+			out = append(out, f)
+		}
+	}
+	if len(out) == 0 {
+		t.Fatalf("no manifests under %s/%s", iamDir, dir)
+	}
+	return out
+}
+
+func declaredPermissions(t *testing.T) sets.Set[string] {
+	t.Helper()
+	declared := sets.New[string]()
+	for _, f := range globYAML(t, "resources") {
+		var pr protectedResource
+		readYAML(t, f, &pr)
+		base := pr.Spec.ServiceRef.Name + "/" + pr.Spec.Plural
+		for _, verb := range pr.Spec.Permissions {
+			declared.Insert(base + "." + verb)
+		}
+		for _, sub := range pr.Spec.Subresources {
+			for _, verb := range sub.Permissions {
+				declared.Insert(base + "/" + sub.Name + "." + verb)
+			}
+		}
+	}
+	return declared
+}
+
+func roles(t *testing.T) []role {
+	t.Helper()
+	var out []role
+	for _, f := range globYAML(t, "roles") {
+		var r role
+		readYAML(t, f, &r)
+		out = append(out, r)
+	}
+	return out
+}
+
+func servedSubresourceRequests(t *testing.T) []*http.Request {
+	t.Helper()
+	srv, _ := newTestAPI(t)
+	var subresources []string
+	for path := range v1alpha1Storage(&ExtraConfig{}) {
+		if strings.Contains(path, "/") {
+			subresources = append(subresources, path)
+		}
+	}
+	if len(subresources) == 0 {
+		t.Fatal("the storage map serves no subresources")
+	}
+	sort.Strings(subresources)
+
+	var served []*http.Request
+	for _, path := range subresources {
+		resource, sub, _ := strings.Cut(path, "/")
+		url := "/apis/" + v1alpha1.GroupName + "/v1alpha1/namespaces/demo/" + resource + "/ctx-a/" + sub
+		routed := 0
+		for _, method := range []string{http.MethodGet, http.MethodPost, http.MethodPut, http.MethodPatch, http.MethodDelete} {
+			// Only 405 means unrouted; any other answer reached the storage.
+			if code, _ := do(t, srv, method, url, "application/json", "{}"); code == http.StatusMethodNotAllowed {
+				continue
+			}
+			served = append(served, httptest.NewRequest(method, url, nil))
+			routed++
+		}
+		if routed == 0 {
+			t.Fatalf("%s is in the storage map but no method reaches it", path)
+		}
+	}
+	return served
+}
+
+// A subresource permission belongs in every role granting the base verb.
+func TestSubresourcePermissionsAreDeclaredAndGranted(t *testing.T) {
+	declared := declaredPermissions(t)
+	allRoles := roles(t)
+	resolver := &request.RequestInfoFactory{
+		APIPrefixes:          sets.NewString("api", "apis"),
+		GrouplessAPIPrefixes: sets.NewString("api"),
+	}
+
+	for _, req := range servedSubresourceRequests(t) {
+		t.Run(req.Method+" "+req.URL.Path, func(t *testing.T) {
+			info, err := resolver.NewRequestInfo(req)
+			if err != nil {
+				t.Fatalf("resolve: %v", err)
+			}
+			if info.Subresource == "" {
+				t.Fatalf("resolved %+v, want a subresource", info)
+			}
+			base := info.APIGroup + "/" + info.Resource + "." + info.Verb
+			permission := info.APIGroup + "/" + info.Resource + "/" + info.Subresource + "." + info.Verb
+			if !declared.Has(permission) {
+				t.Errorf("Milo asks for %s, which no ProtectedResource declares", permission)
+			}
+			granted := false
+			for _, r := range allRoles {
+				has := sets.New(r.Spec.IncludedPermissions...)
+				if has.Has(base) && !has.Has(permission) {
+					t.Errorf("%s grants %s but not %s", r.Metadata.Name, base, permission)
+				}
+				granted = granted || has.Has(permission)
+			}
+			if !granted {
+				t.Errorf("no role grants %s", permission)
+			}
+		})
+	}
+}
+
+func TestRolesGrantOnlyDeclaredPermissions(t *testing.T) {
+	declared := declaredPermissions(t)
+	for _, r := range roles(t) {
+		for _, permission := range r.Spec.IncludedPermissions {
+			if !declared.Has(permission) {
+				t.Errorf("%s grants %s, which no ProtectedResource declares", r.Metadata.Name, permission)
+			}
+		}
+	}
+}