From 40743a2629cd25a83d44926fa4dff0d71b5d01e8 Mon Sep 17 00:00:00 2001 From: LUOSENGWA Date: Mon, 31 Aug 2026 17:34:54 +0000 Subject: [PATCH 1/6] fix(controller): grant Matrix power levels to human members on room join Humans are invited into worker/team rooms by the human reconciler and into project rooms by the Manager, but nothing ever grants them a Matrix power level: they sit at the implicit 0 and 403 on every room operation. Make the grant declarative and self-healing. - Human room reconcile ensures the mapped level in every desired room (new + already-observed; the observed pass heals legacy rooms on the first cycle after deployment) - Level mapping from spec.permissionLevel: 1 -> 100 (co-owner), 2/3 -> 50 (Matrix default member authority; the level-50 kick/ban/ redact scope over sub-50 members is explicitly accepted and documented in docs/design/room-power-levels.md) - EnsureRoomPowerLevel reconciles only the target users entry to exactly the mapped level (a demotion 100 -> 50 actually lowers it) and writes back the complete existing content with only that entry mutated -- events/invite/notifications/extension fields survive; no write when already at exactly that level - matrix.Client.GetRoomState reads a state event's content with the admin identity, decoding the wire response directly (the state endpoint returns the content object, not an event envelope); 404 -> (nil, nil) - create-project.sh: optional --grant-admin lifts the given humans to level 100 in the project room's creation-time override, and the runtime-facing project-management reference now instructs the Manager to pass it; tests/check-create-project-grant-admin.sh (wired into helm-lint) keeps the flag implemented and documented Tests: wire-format GetRoomState mock, exact-match no-write, 100 -> 50 demotion/revocation, extension-field preservation, merge preserving other users, legacy room, read error, state without a users map, second grant preserving the first; controller-level mapping/healing/non-fatal cases. --- .github/workflows/helm-lint.yml | 5 + .../controller/human_controller_test.go | 96 ++++++++ .../controller/human_reconcile_rooms.go | 65 ++++-- .../internal/matrix/client.go | 39 ++++ .../internal/matrix/client_test.go | 52 +++++ .../internal/service/interfaces.go | 18 ++ .../internal/service/provisioner.go | 39 ++++ .../service/provisioner_power_test.go | 212 ++++++++++++++++++ .../internal/service/provisioner_team_test.go | 36 +++ .../test/testutil/mocks/human_provisioner.go | 36 ++- .../test/testutil/mocks/provisioner.go | 21 ++ docs/design/room-power-levels.md | 103 +++++++++ .../references/create-project.md | 12 + .../scripts/create-project.sh | 32 ++- tests/check-create-project-grant-admin.sh | 42 ++++ 15 files changed, 778 insertions(+), 30 deletions(-) create mode 100644 agentteams-controller/internal/service/provisioner_power_test.go create mode 100644 docs/design/room-power-levels.md create mode 100755 tests/check-create-project-grant-admin.sh diff --git a/.github/workflows/helm-lint.yml b/.github/workflows/helm-lint.yml index a667bb201..93196299d 100644 --- a/.github/workflows/helm-lint.yml +++ b/.github/workflows/helm-lint.yml @@ -25,6 +25,8 @@ on: - 'tests/check-windows-appservice-normalization.ps1' - 'tests/check-apply-wrapper-flags.sh' - 'tests/check-helm-agentteams.sh' + - 'tests/check-create-project-grant-admin.sh' + - 'manager/agent/skills/project-management/**' workflow_dispatch: ~ jobs: @@ -61,6 +63,9 @@ jobs: - name: Check declarative apply wrapper flags run: bash tests/check-apply-wrapper-flags.sh + - name: Check create-project grant-admin wiring + run: bash tests/check-create-project-grant-admin.sh + - name: Set up Helm uses: azure/setup-helm@v4 with: diff --git a/agentteams-controller/internal/controller/human_controller_test.go b/agentteams-controller/internal/controller/human_controller_test.go index b85f3020d..5ae3cf2be 100644 --- a/agentteams-controller/internal/controller/human_controller_test.go +++ b/agentteams-controller/internal/controller/human_controller_test.go @@ -3,6 +3,7 @@ package controller import ( "context" "errors" + "fmt" "sort" "testing" @@ -635,3 +636,98 @@ func sortedCopy(in []string) []string { // Silence unused import lint when the service package is referenced only // via the mock type alias in some future subtest. var _ = service.HumanCredentials{} + +// TestHumanReconciler_PowerLevelMapping checks that every room in the +// desired set — new and already-observed — gets EnsureRoomPowerLevel with +// the level derived from the Human CR's permissionLevel. +func TestHumanReconciler_PowerLevelMapping(t *testing.T) { + worker := newReadyWorker("w1", "!room-w1:localhost") + team := newReadyTeam("t1", "!room-t1:localhost") + human := newHuman("alice", v1beta1.HumanSpec{ + PermissionLevel: 1, // admin-equivalent → 100 + AccessibleWorkers: []string{"w1"}, + AccessibleTeams: []string{"t1"}, + }) + human.Status.MatrixUserID = "@alice:localhost" + human.Status.InitialPassword = "stored-pw" + human.Status.Rooms = []string{"!room-w1:localhost"} // already in the worker room + human.Status.Phase = "Active" + human.Finalizers = []string{finalizerName} + + rig := newHumanRig(t, human, worker, team) + out, _, err := rig.reconcile("alice") + if err != nil { + t.Fatalf("reconcile: %v", err) + } + + byRoom := map[string]int{} + for _, c := range rig.prov.Calls.EnsureRoomPowerLevel { + byRoom[c.RoomID] = c.Level + } + if byRoom["!room-w1:localhost"] != 100 { + t.Errorf("existing room heal: level=%d, want 100 (calls=%+v)", byRoom["!room-w1:localhost"], rig.prov.Calls.EnsureRoomPowerLevel) + } + if byRoom["!room-t1:localhost"] != 100 { + t.Errorf("new room: level=%d, want 100 (calls=%+v)", byRoom["!room-t1:localhost"], rig.prov.Calls.EnsureRoomPowerLevel) + } + for _, c := range rig.prov.Calls.EnsureRoomPowerLevel { + if c.UserID != "@alice:localhost" { + t.Errorf("power level granted to %s, want @alice:localhost", c.UserID) + } + } + if len(out.Status.Rooms) != 2 { + t.Errorf("Status.Rooms=%v, want both rooms", out.Status.Rooms) + } +} + +// Team-scoped humans (level 2) get the default member level (50), not room +// ownership. +func TestHumanReconciler_PowerLevelL2GetsDefault(t *testing.T) { + worker := newReadyWorker("w1", "!room-w1:localhost") + human := newHuman("bob", v1beta1.HumanSpec{ + PermissionLevel: 2, + AccessibleWorkers: []string{"w1"}, + }) + human.Status.MatrixUserID = "@bob:localhost" + human.Status.InitialPassword = "stored-pw" + human.Status.Phase = "Active" + human.Finalizers = []string{finalizerName} + + rig := newHumanRig(t, human, worker) + if _, _, err := rig.reconcile("bob"); err != nil { + t.Fatalf("reconcile: %v", err) + } + if len(rig.prov.Calls.EnsureRoomPowerLevel) != 1 { + t.Fatalf("power calls=%+v, want 1", rig.prov.Calls.EnsureRoomPowerLevel) + } + if got := rig.prov.Calls.EnsureRoomPowerLevel[0].Level; got != 50 { + t.Errorf("level=%d, want 50", got) + } +} + +// A power-level grant failure is non-fatal: the room is still recorded and +// the next cycle retries. +func TestHumanReconciler_PowerLevelErrorNonFatal(t *testing.T) { + worker := newReadyWorker("w1", "!room-w1:localhost") + human := newHuman("carol", v1beta1.HumanSpec{ + PermissionLevel: 2, + AccessibleWorkers: []string{"w1"}, + }) + human.Status.MatrixUserID = "@carol:localhost" + human.Status.InitialPassword = "stored-pw" + human.Status.Phase = "Active" + human.Finalizers = []string{finalizerName} + + rig := newHumanRig(t, human, worker) + rig.prov.EnsureRoomPowerLevelFn = func(ctx context.Context, roomID, userID string, level int) error { + return fmt.Errorf("matrix unavailable") + } + + out, _, err := rig.reconcile("carol") + if err != nil { + t.Fatalf("power-level failure must be non-fatal, got: %v", err) + } + if len(out.Status.Rooms) != 1 || out.Status.Rooms[0] != "!room-w1:localhost" { + t.Errorf("room not recorded despite power failure: %v", out.Status.Rooms) + } +} diff --git a/agentteams-controller/internal/controller/human_reconcile_rooms.go b/agentteams-controller/internal/controller/human_reconcile_rooms.go index fabc61475..d6c6e651a 100644 --- a/agentteams-controller/internal/controller/human_reconcile_rooms.go +++ b/agentteams-controller/internal/controller/human_reconcile_rooms.go @@ -44,29 +44,43 @@ func (r *HumanReconciler) reconcileHumanRooms(ctx context.Context, s *humanScope next := make([]string, 0, len(h.Status.Rooms)+len(desired)) next = append(next, h.Status.Rooms...) + powerLevel := humanRoomPowerLevel(h.Spec.PermissionLevel) + for rid := range desired { + alreadyMember := false if _, ok := observed[rid]; ok { - continue + alreadyMember = true } - if err := r.Provisioner.InviteToRoom(ctx, rid, matrixUserID); err != nil { - logger.Error(err, "failed to invite human to room", "room", rid) - continue + if !alreadyMember { + if err := r.Provisioner.InviteToRoom(ctx, rid, matrixUserID); err != nil { + logger.Error(err, "failed to invite human to room", "room", rid) + continue + } + // Acquire a user token lazily — only on the first new-room + // addition of this reconcile. Steady-state passes (desired == + // observed) and revoke-only passes never reach this call, so + // Matrix Login is not issued on every 5-minute requeue. + token := r.ensureUserToken(ctx, s) + if token == "" { + logger.V(1).Info("user token unavailable; invite-only this cycle", + "room", rid, "human", h.Name, "username", s.username) + continue + } + if err := r.Provisioner.JoinRoomAs(ctx, rid, token); err != nil { + logger.Error(err, "failed to join room as human", "room", rid) + continue + } + next = append(next, rid) } - // Acquire a user token lazily — only on the first new-room - // addition of this reconcile. Steady-state passes (desired == - // observed) and revoke-only passes never reach this call, so - // Matrix Login is not issued on every 5-minute requeue. - token := r.ensureUserToken(ctx, s) - if token == "" { - logger.V(1).Info("user token unavailable; invite-only this cycle", - "room", rid, "human", h.Name, "username", s.username) - continue + // Grant the human their power level in every room they should be + // in — new rooms and already-observed ones alike. The existing-room + // pass is the healing path: legacy rooms were created before power + // levels accounted for human members, leaving them at the implicit + // level 0 and 403 on room operations (rename, invite). Non-fatal + // per this file's error policy; the next cycle retries. + if err := r.Provisioner.EnsureRoomPowerLevel(ctx, rid, matrixUserID, powerLevel); err != nil { + logger.Error(err, "failed to ensure human power level", "room", rid, "level", powerLevel) } - if err := r.Provisioner.JoinRoomAs(ctx, rid, token); err != nil { - logger.Error(err, "failed to join room as human", "room", rid) - continue - } - next = append(next, rid) } // Removals: in-place filter. A failed kick keeps the room so the @@ -86,6 +100,21 @@ func (r *HumanReconciler) reconcileHumanRooms(ctx context.Context, s *humanScope h.Status.Rooms = kept } +// humanRoomPowerLevel maps the Human CR permission level to the Matrix power +// level granted in rooms the human belongs to. Level 1 (admin equivalent) +// co-owns the rooms (full control); levels 2/3 (team/worker scoped) get +// level 50 — Matrix's default member authority: rename, invite, kick, ban +// and redact (the homeserver defaults all sit at 50), but not power-level +// changes, and only against members strictly below 50 — the manager/leader +// at 100 can never be kicked or banned by the human. This authority is +// accepted and documented in docs/design/room-power-levels.md. +func humanRoomPowerLevel(permissionLevel int) int { + if permissionLevel == 1 { + return 100 + } + return 50 +} + // ensureUserToken returns a Matrix access token for the human, // acquiring one via Login on first call per reconcile and caching it // in the scope. Returns "" when login fails — callers degrade to diff --git a/agentteams-controller/internal/matrix/client.go b/agentteams-controller/internal/matrix/client.go index 2b13a7f45..93b2fce78 100644 --- a/agentteams-controller/internal/matrix/client.go +++ b/agentteams-controller/internal/matrix/client.go @@ -60,6 +60,13 @@ type Client interface { // it falls back to the homeserver-admin identity. SetRoomState(ctx context.Context, roomID, eventType, stateKey string, content map[string]interface{}, userToken string) error + // GetRoomState reads the content of a single state event from a room + // using the homeserver-admin identity (the event's `content` object, + // not the full event envelope). A room that has never had the event + // set (e.g. legacy rooms with no m.room.power_levels) yields (nil, nil) + // rather than an error; any other failure is returned. + GetRoomState(ctx context.Context, roomID, eventType, stateKey string) (map[string]interface{}, error) + // JoinRoom makes the user identified by token join the given room. JoinRoom(ctx context.Context, roomID, userToken string) error @@ -753,6 +760,38 @@ func (c *TuwunelClient) SetRoomState(ctx context.Context, roomID, eventType, sta return nil } +func (c *TuwunelClient) GetRoomState(ctx context.Context, roomID, eventType, stateKey string) (map[string]interface{}, error) { + token, err := c.ensureAdminToken(ctx) + if err != nil { + return nil, fmt.Errorf("get room state %s %s: %w", roomID, eventType, err) + } + encodedRoom := encodeRoomID(roomID) + // Always include the state-key segment (trailing slash when the key is + // empty) to match SetRoomState — some strict homeservers reject the + // segment-less form for empty-key events. + path := fmt.Sprintf("/_matrix/client/v3/rooms/%s/state/%s/%s", + encodedRoom, url.PathEscape(eventType), url.PathEscape(stateKey)) + statusCode, respBody, err := c.doJSON(ctx, http.MethodGet, path, token, nil, nil) + if err != nil { + return nil, fmt.Errorf("get room state %s %s: %w", roomID, eventType, err) + } + if statusCode == http.StatusNotFound { + return nil, nil // state event never set on this room + } + if statusCode != http.StatusOK { + return nil, fmt.Errorf("get room state %s %s: HTTP %d: %s", + roomID, eventType, statusCode, truncate(respBody, 500)) + } + // The state endpoint returns the state CONTENT object directly + // (e.g. {"users":{...},"ban":50}), not an event envelope — decode + // the wire response as-is. + var content map[string]interface{} + if err := json.Unmarshal(respBody, &content); err != nil { + return nil, fmt.Errorf("get room state %s %s: decode: %w", roomID, eventType, err) + } + return content, nil +} + func (c *TuwunelClient) JoinRoom(ctx context.Context, roomID, userToken string) error { encodedRoom := encodeRoomID(roomID) statusCode, respBody, err := c.doJSON(ctx, http.MethodPost, diff --git a/agentteams-controller/internal/matrix/client_test.go b/agentteams-controller/internal/matrix/client_test.go index 204a0f97f..8434958cc 100644 --- a/agentteams-controller/internal/matrix/client_test.go +++ b/agentteams-controller/internal/matrix/client_test.go @@ -548,6 +548,58 @@ func TestSetRoomState(t *testing.T) { } } +func TestGetRoomState(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/_matrix/client/v3/login": + adminLoginHandler(t, w) + case "/_matrix/client/v3/rooms/!room:d/state/m.room.power_levels/": + // Trailing slash: the empty state key is always included in the + // URL (aligned with SetRoomState, strict-homeserver safe). + if r.Method != http.MethodGet { + t.Errorf("method = %s, want GET", r.Method) + } + if auth := r.Header.Get("Authorization"); auth != "Bearer admin-token" { + t.Errorf("Authorization = %q, want Bearer admin-token", auth) + } + w.WriteHeader(http.StatusOK) + // A compliant homeserver returns the state CONTENT object + // directly — no event envelope (no type/state_key/sender, no + // "content" wrapper). + json.NewEncoder(w).Encode(map[string]interface{}{ + "users": map[string]interface{}{"@a:d": 100.0}, + }) + case "/_matrix/client/v3/rooms/!room:d/state/room.meta/": + w.WriteHeader(http.StatusNotFound) + w.Write([]byte(`{"errcode":"M_NOT_FOUND"}`)) + default: + t.Errorf("unexpected path: %s", r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + })) + defer server.Close() + + c := NewTuwunelClient(Config{ServerURL: server.URL, Domain: "d"}, server.Client()) + + st, err := c.GetRoomState(context.Background(), "!room:d", "m.room.power_levels", "") + if err != nil { + t.Fatalf("GetRoomState: %v", err) + } + users, ok := st["users"].(map[string]interface{}) + if !ok || users["@a:d"] != 100.0 { + t.Fatalf("users=%#v, want @a:d=100 (content, not event envelope)", st) + } + + // A room that never had the state set yields (nil, nil), not an error. + st, err = c.GetRoomState(context.Background(), "!room:d", "room.meta", "") + if err != nil { + t.Fatalf("missing state must not error: %v", err) + } + if st != nil { + t.Errorf("missing state = %#v, want nil", st) + } +} + // adminLoginHandler returns a handler that responds to admin login with a // fixed token, allowing tests that exercise admin-driven endpoints. func adminLoginHandler(t *testing.T, w http.ResponseWriter) { diff --git a/agentteams-controller/internal/service/interfaces.go b/agentteams-controller/internal/service/interfaces.go index 54ba1ac2f..bb47b54eb 100644 --- a/agentteams-controller/internal/service/interfaces.go +++ b/agentteams-controller/internal/service/interfaces.go @@ -57,6 +57,15 @@ type WorkerProvisioner interface { // KickFromRoom removes userID from roomID using the admin token. // Idempotent: returns nil when the user is not a member. KickFromRoom(ctx context.Context, roomID, userID, reason string) error + + // EnsureRoomPowerLevel reconciles userID's entry in the room's + // m.room.power_levels to exactly the given power level (raising or + // lowering it), using the admin token. The complete existing content is + // preserved (every other user, every non-user field); only the target + // users entry is mutated. The call is a no-op write when the user + // already has exactly that level. Rooms that never had power_levels set + // (legacy) are treated as starting from an empty users map. + EnsureRoomPowerLevel(ctx context.Context, roomID, userID string, level int) error // ForceLeaveRoom removes a user whose room power level prevents a normal // admin kick. ForceLeaveRoom(ctx context.Context, userID, roomID string) error @@ -238,6 +247,15 @@ type HumanProvisioner interface { // Idempotent: returns nil when the user is not a member. KickFromRoom(ctx context.Context, roomID, userID, reason string) error + // EnsureRoomPowerLevel reconciles userID's entry in the room's + // m.room.power_levels to exactly the given power level (raising or + // lowering it), using the admin token. The complete existing content is + // preserved (every other user, every non-user field); only the target + // users entry is mutated. The call is a no-op write when the user + // already has exactly that level. Rooms that never had power_levels set + // (legacy) are treated as starting from an empty users map. + EnsureRoomPowerLevel(ctx context.Context, roomID, userID string, level int) error + // ForceLeaveRoom asks the Tuwunel admin bot to force-leave userID out // of roomID via "!admin users force-leave-room". Fire-and-forget at // the bot layer, but the admin message delivery itself is confirmed. diff --git a/agentteams-controller/internal/service/provisioner.go b/agentteams-controller/internal/service/provisioner.go index f5a0ba41c..594494283 100644 --- a/agentteams-controller/internal/service/provisioner.go +++ b/agentteams-controller/internal/service/provisioner.go @@ -1074,6 +1074,45 @@ func (p *Provisioner) EnsureRoomNonMember(ctx context.Context, roomID, userID, r return p.matrix.KickFromRoom(ctx, roomID, userID, reason) } +// EnsureRoomPowerLevel reconciles userID's entry in the room's +// m.room.power_levels to EXACTLY `level` (raising or lowering it) via the +// admin token. The complete existing content is preserved — every other +// user and every non-user field (events, invite, notifications, +// users_default, state_default, ban, kick, redact, extension fields); only +// the target users entry is mutated. Idempotent: no write when the user +// already has exactly `level`. Rooms that never had power_levels set +// (legacy rooms) start from an empty users map. +func (p *Provisioner) EnsureRoomPowerLevel(ctx context.Context, roomID, userID string, level int) error { + cur, err := p.matrix.GetRoomState(ctx, roomID, "m.room.power_levels", "") + if err != nil { + return fmt.Errorf("read power levels %s: %w", roomID, err) + } + // Preserve the complete existing content and mutate only the target + // users entry — rebuilding the struct would drop fields we don't + // explicitly know about (events, invite, notifications, extensions). + content := map[string]interface{}{} + var users map[string]interface{} + if cur != nil { + for k, v := range cur { + content[k] = v + } + users, _ = cur["users"].(map[string]interface{}) + } + if users == nil { + users = map[string]interface{}{} + } + // Exact-target semantics: a demoted human (e.g. permissionLevel 1 → 2, + // 100 → 50) must actually be lowered, not kept at the old level. + if have, ok := users[userID]; ok { + if n, ok := have.(float64); ok && int(n) == level { + return nil // already at exactly the desired level — no write + } + } + users[userID] = float64(level) + content["users"] = users + return p.matrix.SetRoomState(ctx, roomID, "m.room.power_levels", "", content, "") +} + // ReconcileRoomMembership drives the membership of roomID to match `desired` // (a list of full Matrix user IDs). Users present in `desired` but not in // the room are invited; users in the room but not in `desired` are kicked. diff --git a/agentteams-controller/internal/service/provisioner_power_test.go b/agentteams-controller/internal/service/provisioner_power_test.go new file mode 100644 index 000000000..ebb952acb --- /dev/null +++ b/agentteams-controller/internal/service/provisioner_power_test.go @@ -0,0 +1,212 @@ +package service + +import ( + "context" + "errors" + "testing" +) + +func TestEnsureRoomPowerLevel_LegacyRoomGrantsLevel(t *testing.T) { + fake := newFakeTeamMatrix() // no powerStates: legacy room, state never set + p := NewProvisioner(ProvisionerConfig{ + Matrix: fake, + Creds: fakeCredentialStore{}, + OSSAdmin: &fakeStorageAdmin{}, + }) + + if err := p.EnsureRoomPowerLevel(context.Background(), "!r:hs", "@alice:hs", 100); err != nil { + t.Fatalf("EnsureRoomPowerLevel: %v", err) + } + calls := fake.roomStates + if len(calls) != 1 || calls[0].eventType != "m.room.power_levels" || calls[0].roomID != "!r:hs" { + t.Fatalf("room state calls=%+v, want single m.room.power_levels write", calls) + } + users, ok := calls[0].content["users"].(map[string]interface{}) + if !ok { + t.Fatalf("users not map[string]interface{}: %#v", calls[0].content["users"]) + } + if users["@alice:hs"] != 100.0 { + t.Errorf("alice level=%v, want 100", users["@alice:hs"]) + } +} + +func TestEnsureRoomPowerLevel_MergesExistingUsers(t *testing.T) { + existing := map[string]interface{}{ + "users": map[string]interface{}{ + "@manager:hs": 100.0, + "@alice:hs": 0.0, // human currently at 0 — the bug + "@worker:hs": 0.0, + }, + "users_default": 0.0, + "state_default": 50.0, + // Extension fields the write path must survive untouched. + "events": map[string]interface{}{"m.room.name": 50.0}, + "invite": 50.0, + "notifications": map[string]interface{}{"room": 50.0}, + } + fake := newFakeTeamMatrix() + fake.powerStates = map[string]map[string]interface{}{"!r:hs": existing} + p := NewProvisioner(ProvisionerConfig{ + Matrix: fake, + Creds: fakeCredentialStore{}, + OSSAdmin: &fakeStorageAdmin{}, + }) + + if err := p.EnsureRoomPowerLevel(context.Background(), "!r:hs", "@alice:hs", 50); err != nil { + t.Fatalf("EnsureRoomPowerLevel: %v", err) + } + users, ok := fake.powerStates["!r:hs"]["users"].(map[string]interface{}) + if !ok { + t.Fatalf("stored users not a map: %#v", fake.powerStates["!r:hs"]) + } + if users["@alice:hs"] != 50.0 { + t.Errorf("alice=%v, want 50", users["@alice:hs"]) + } + if users["@manager:hs"] != 100.0 || users["@worker:hs"] != 0.0 { + t.Errorf("existing users disturbed: %v", users) + } + // Non-user power-level settings preserved. + if _, ok := fake.powerStates["!r:hs"]["users_default"]; !ok { + t.Errorf("users_default dropped") + } + if _, ok := fake.powerStates["!r:hs"]["state_default"]; !ok { + t.Errorf("state_default dropped") + } + // Extension fields survive the write — only the target users entry + // may be mutated, never a rebuilt struct. + if ev, ok := fake.powerStates["!r:hs"]["events"].(map[string]interface{}); !ok || ev["m.room.name"] != 50.0 { + t.Errorf("events field dropped or altered: %#v", fake.powerStates["!r:hs"]["events"]) + } + if inv, ok := fake.powerStates["!r:hs"]["invite"].(float64); !ok || inv != 50.0 { + t.Errorf("invite field dropped or altered: %#v", fake.powerStates["!r:hs"]["invite"]) + } + if n, ok := fake.powerStates["!r:hs"]["notifications"].(map[string]interface{}); !ok || n["room"] != 50.0 { + t.Errorf("notifications field dropped or altered: %#v", fake.powerStates["!r:hs"]["notifications"]) + } +} + +// A demoted human must actually be lowered: a user sitting at 100 whose +// permissionLevel drops from 1 to 2 is written at 50, not kept at 100. +func TestEnsureRoomPowerLevel_DemotionRevokesLevel(t *testing.T) { + existing := map[string]interface{}{ + "users": map[string]interface{}{ + "@manager:hs": 100.0, + "@alice:hs": 100.0, // was L1, now being demoted to L2 + "@worker:hs": 0.0, + }, + "ban": 50.0, + "kick": 50.0, + } + fake := newFakeTeamMatrix() + fake.powerStates = map[string]map[string]interface{}{"!r:hs": existing} + p := NewProvisioner(ProvisionerConfig{ + Matrix: fake, + Creds: fakeCredentialStore{}, + OSSAdmin: &fakeStorageAdmin{}, + }) + + if err := p.EnsureRoomPowerLevel(context.Background(), "!r:hs", "@alice:hs", 50); err != nil { + t.Fatalf("EnsureRoomPowerLevel: %v", err) + } + if len(fake.roomStates) != 1 { + t.Fatalf("demotion must write, got %d writes", len(fake.roomStates)) + } + users, ok := fake.powerStates["!r:hs"]["users"].(map[string]interface{}) + if !ok { + t.Fatalf("stored users not a map: %#v", fake.powerStates["!r:hs"]) + } + if users["@alice:hs"] != 50.0 { + t.Errorf("alice=%v, want 50 (demoted from 100)", users["@alice:hs"]) + } + if users["@manager:hs"] != 100.0 || users["@worker:hs"] != 0.0 { + t.Errorf("other users disturbed: %v", users) + } +} + +func TestEnsureRoomPowerLevel_ExactMatchNoWrite(t *testing.T) { + existing := map[string]interface{}{ + "users": map[string]interface{}{"@alice:hs": 50.0, "@manager:hs": 100.0}, + } + fake := newFakeTeamMatrix() + fake.powerStates = map[string]map[string]interface{}{"!r:hs": existing} + p := NewProvisioner(ProvisionerConfig{ + Matrix: fake, + Creds: fakeCredentialStore{}, + OSSAdmin: &fakeStorageAdmin{}, + }) + + if err := p.EnsureRoomPowerLevel(context.Background(), "!r:hs", "@alice:hs", 50); err != nil { + t.Fatalf("EnsureRoomPowerLevel: %v", err) + } + if len(fake.roomStates) != 0 { + t.Errorf("expected no write when already at exactly the desired level, got %d writes", len(fake.roomStates)) + } +} + +func TestEnsureRoomPowerLevel_ReadErrorPropagates(t *testing.T) { + fake := newFakeTeamMatrix() + fake.powerStateErr = errors.New("matrix down") + p := NewProvisioner(ProvisionerConfig{ + Matrix: fake, + Creds: fakeCredentialStore{}, + OSSAdmin: &fakeStorageAdmin{}, + }) + + err := p.EnsureRoomPowerLevel(context.Background(), "!r:hs", "@alice:hs", 50) + if err == nil { + t.Fatal("expected error, got nil") + } + if len(fake.roomStates) != 0 { + t.Errorf("no write allowed after read failure, got %d", len(fake.roomStates)) + } +} + +// A second human granted later must not clobber the first human's level — +// the write path must merge against what the previous write stored (via the +// homeserver's JSON round-trip in production, mirrored in the fake). +func TestEnsureRoomPowerLevel_SecondGrantPreservesFirst(t *testing.T) { + fake := newFakeTeamMatrix() + p := NewProvisioner(ProvisionerConfig{ + Matrix: fake, + Creds: fakeCredentialStore{}, + OSSAdmin: &fakeStorageAdmin{}, + }) + + if err := p.EnsureRoomPowerLevel(context.Background(), "!r:hs", "@alice:hs", 100); err != nil { + t.Fatalf("first grant: %v", err) + } + if err := p.EnsureRoomPowerLevel(context.Background(), "!r:hs", "@bob:hs", 50); err != nil { + t.Fatalf("second grant: %v", err) + } + stored := fake.powerStates["!r:hs"] + users, ok := stored["users"].(map[string]interface{}) + if !ok { + t.Fatalf("users=%#v, want map after second grant", stored["users"]) + } + if users["@alice:hs"] != 100.0 || users["@bob:hs"] != 50.0 { + t.Errorf("users=%v, want alice=100 bob=50 (no clobber)", users) + } +} + +// EnsureRoomPowerLevel must also handle a power_levels state that exists +// without a users map (defensive: treat as empty users). +func TestEnsureRoomPowerLevel_StateWithoutUsersMap(t *testing.T) { + fake := newFakeTeamMatrix() + fake.powerStates = map[string]map[string]interface{}{ + "!r:hs": {"users_default": 0.0}, + } + p := NewProvisioner(ProvisionerConfig{ + Matrix: fake, + Creds: fakeCredentialStore{}, + OSSAdmin: &fakeStorageAdmin{}, + }) + + if err := p.EnsureRoomPowerLevel(context.Background(), "!r:hs", "@alice:hs", 50); err != nil { + t.Fatalf("EnsureRoomPowerLevel: %v", err) + } + stored := fake.powerStates["!r:hs"] + users, ok := stored["users"].(map[string]interface{}) + if !ok || users["@alice:hs"] != 50.0 { + t.Errorf("users=%#v, want alice=50", stored["users"]) + } +} diff --git a/agentteams-controller/internal/service/provisioner_team_test.go b/agentteams-controller/internal/service/provisioner_team_test.go index 7546290ad..df72e5624 100644 --- a/agentteams-controller/internal/service/provisioner_team_test.go +++ b/agentteams-controller/internal/service/provisioner_team_test.go @@ -2,6 +2,7 @@ package service import ( "context" + "encoding/json" "errors" "reflect" "sort" @@ -31,6 +32,11 @@ type fakeTeamMatrix struct { roomStates []roomStateCall tokenInvites []roomUserCall created bool + + // powerStates seeds m.room.power_levels reads per room; an absent room + // reports (nil, nil) — the legacy "state never set" case. + powerStates map[string]map[string]interface{} + powerStateErr error } type roomUserCall struct { @@ -118,9 +124,39 @@ func (f *fakeTeamMatrix) SetRoomState(_ context.Context, roomID, eventType, stat content: content, token: token, }) + if eventType == "m.room.power_levels" { + // JSON round-trip so stored state matches what the homeserver + // would return (numbers as float64 in map[string]interface{}). + data, _ := json.Marshal(content) + var rt map[string]interface{} + if err := json.Unmarshal(data, &rt); err != nil { + rt = content + } + if f.powerStates == nil { + f.powerStates = map[string]map[string]interface{}{} + } + f.powerStates[roomID] = rt + } return nil } +func (f *fakeTeamMatrix) GetRoomState(_ context.Context, roomID, eventType, _ string) (map[string]interface{}, error) { + if eventType != "m.room.power_levels" { + return nil, nil + } + if f.powerStateErr != nil { + return nil, f.powerStateErr + } + if f.powerStates == nil { + return nil, nil // legacy room: state never set + } + st, ok := f.powerStates[roomID] + if !ok { + return nil, nil + } + return st, nil +} + func (f *fakeTeamMatrix) JoinRoom(_ context.Context, roomID, token string) error { f.joins = append(f.joins, roomUserCall{roomID: roomID, userID: token}) if userID := f.tokenUsers[token]; userID != "" { diff --git a/agentteams-controller/test/testutil/mocks/human_provisioner.go b/agentteams-controller/test/testutil/mocks/human_provisioner.go index cfd7eab6f..4e74b23e3 100644 --- a/agentteams-controller/test/testutil/mocks/human_provisioner.go +++ b/agentteams-controller/test/testutil/mocks/human_provisioner.go @@ -26,13 +26,14 @@ type MockHumanProvisioner struct { LoginAppServiceUserFn func(ctx context.Context, name string) (string, error) LoginWithPasswordFn func(ctx context.Context, name, password string) (string, error) - MatrixUserIDFn func(name string) string - InviteToRoomFn func(ctx context.Context, roomID, userID string) error - JoinRoomAsFn func(ctx context.Context, roomID, userToken string) error - KickFromRoomFn func(ctx context.Context, roomID, userID, reason string) error - ForceLeaveRoomFn func(ctx context.Context, userID, roomID string) error - DeactivateHumanUserFn func(ctx context.Context, userID string) error - SetDisplayNameFn func(ctx context.Context, userID, accessToken, displayName string) error + MatrixUserIDFn func(name string) string + InviteToRoomFn func(ctx context.Context, roomID, userID string) error + JoinRoomAsFn func(ctx context.Context, roomID, userToken string) error + KickFromRoomFn func(ctx context.Context, roomID, userID, reason string) error + EnsureRoomPowerLevelFn func(ctx context.Context, roomID, userID string, level int) error + ForceLeaveRoomFn func(ctx context.Context, userID, roomID string) error + DeactivateHumanUserFn func(ctx context.Context, userID string) error + SetDisplayNameFn func(ctx context.Context, userID, accessToken, displayName string) error // AppServiceEnabled toggles MatrixAppServiceEnabled() — needed by // the legacy_password identity source to choose between AS and @@ -53,9 +54,18 @@ type MockHumanProvisioner struct { KickFromRoom []KickFromRoomCall ForceLeaveRoom []ForceLeaveRoomCall DeactivateHumanUser []string + EnsureRoomPowerLevel []EnsureRoomPowerLevelCall } } +// EnsureRoomPowerLevelCall records the (RoomID, UserID, Level) triple passed +// to EnsureRoomPowerLevel. +type EnsureRoomPowerLevelCall struct { + RoomID string + UserID string + Level int +} + // LoginAsHumanCall records the (name, password) pair passed to LoginAsHuman. type LoginAsHumanCall struct { Name string @@ -158,6 +168,7 @@ func (m *MockHumanProvisioner) clearCallsLocked() { KickFromRoom []KickFromRoomCall ForceLeaveRoom []ForceLeaveRoomCall DeactivateHumanUser []string + EnsureRoomPowerLevel []EnsureRoomPowerLevelCall }{} } @@ -329,6 +340,17 @@ func (m *MockHumanProvisioner) DeactivateHumanUser(ctx context.Context, userID s return nil } +func (m *MockHumanProvisioner) EnsureRoomPowerLevel(ctx context.Context, roomID, userID string, level int) error { + m.mu.Lock() + m.Calls.EnsureRoomPowerLevel = append(m.Calls.EnsureRoomPowerLevel, EnsureRoomPowerLevelCall{RoomID: roomID, UserID: userID, Level: level}) + fn := m.EnsureRoomPowerLevelFn + m.mu.Unlock() + if fn != nil { + return fn(ctx, roomID, userID, level) + } + return nil +} + func (m *MockHumanProvisioner) MatrixAppServiceEnabled() bool { return m.AppServiceEnabled } diff --git a/agentteams-controller/test/testutil/mocks/provisioner.go b/agentteams-controller/test/testutil/mocks/provisioner.go index 34d03236a..1bfae080a 100644 --- a/agentteams-controller/test/testutil/mocks/provisioner.go +++ b/agentteams-controller/test/testutil/mocks/provisioner.go @@ -44,6 +44,7 @@ type MockProvisioner struct { InviteToRoomFn func(ctx context.Context, roomID, userID string) error JoinRoomAsFn func(ctx context.Context, roomID, userToken string) error KickFromRoomFn func(ctx context.Context, roomID, userID, reason string) error + EnsureRoomPowerLevelFn func(ctx context.Context, roomID, userID string, level int) error ForceLeaveRoomFn func(ctx context.Context, userID, roomID string) error DeactivateHumanUserFn func(ctx context.Context, userID string) error ProvisionTeamRoomsFn func(ctx context.Context, req service.TeamRoomRequest) (*service.TeamRoomResult, error) @@ -85,6 +86,7 @@ type MockProvisioner struct { InviteToRoom []roomMembershipCall JoinRoomAs []joinRoomAsCall KickFromRoom []kickFromRoomCall + EnsureRoomPowerLevel []ensureRoomPowerLevelCall ForceLeaveRoom []roomMembershipCall DeactivateHumanUser []string ProvisionTeamRooms []service.TeamRoomRequest @@ -120,6 +122,12 @@ type remoteNamespaceCall struct { Namespace string } +type ensureRoomPowerLevelCall struct { + RoomID string + UserID string + Level int +} + type userPasswordCall struct { UserID string Password string @@ -186,6 +194,7 @@ func (m *MockProvisioner) Reset() { m.InviteToRoomFn = nil m.JoinRoomAsFn = nil m.KickFromRoomFn = nil + m.EnsureRoomPowerLevelFn = nil m.ForceLeaveRoomFn = nil m.DeactivateHumanUserFn = nil m.ProvisionTeamRoomsFn = nil @@ -232,6 +241,7 @@ func (m *MockProvisioner) clearCallsLocked() { InviteToRoom []roomMembershipCall JoinRoomAs []joinRoomAsCall KickFromRoom []kickFromRoomCall + EnsureRoomPowerLevel []ensureRoomPowerLevelCall ForceLeaveRoom []roomMembershipCall DeactivateHumanUser []string ProvisionTeamRooms []service.TeamRoomRequest @@ -617,6 +627,17 @@ func (m *MockProvisioner) KickFromRoom(ctx context.Context, roomID, userID, reas return nil } +func (m *MockProvisioner) EnsureRoomPowerLevel(ctx context.Context, roomID, userID string, level int) error { + m.mu.Lock() + m.Calls.EnsureRoomPowerLevel = append(m.Calls.EnsureRoomPowerLevel, ensureRoomPowerLevelCall{RoomID: roomID, UserID: userID, Level: level}) + fn := m.EnsureRoomPowerLevelFn + m.mu.Unlock() + if fn != nil { + return fn(ctx, roomID, userID, level) + } + return nil +} + func (m *MockProvisioner) ForceLeaveRoom(ctx context.Context, userID, roomID string) error { m.mu.Lock() m.Calls.ForceLeaveRoom = append(m.Calls.ForceLeaveRoom, roomMembershipCall{RoomID: roomID, UserID: userID}) diff --git a/docs/design/room-power-levels.md b/docs/design/room-power-levels.md new file mode 100644 index 000000000..da0a1c283 --- /dev/null +++ b/docs/design/room-power-levels.md @@ -0,0 +1,103 @@ +# Room Power Levels for Human Members + +Status: implemented +Surfaces: `m.room.power_levels` in team / worker / project rooms; `create-project.sh --grant-admin` + +## Problem + +Humans are invited into worker and team rooms by the human reconciler and +into project rooms by the Manager — but nothing ever grants them a Matrix +power level. Two consequences: + +1. **Rooms created before power levels accounted for humans** (and rooms + where the human joined later) carry a `m.room.power_levels` state that + lists only manager / leader / admin at 100 and workers at 0. The human + sits at the implicit level 0. +2. **Rooms that never had the state set at all** (legacy) fall back to the + homeserver's strict defaults. + +Either way, a human operator gets `403` on every room operation — renaming +the room, inviting a colleague, even housekeeping. The room works for the +bots that own it and is unusable for the person it is meant for. + +## Design + +**Declarative grant in the human room reconcile.** The human reconciler +already walks the full desired room set on every cycle (new rooms: invite + +join; observed rooms: skip). It now additionally ensures the human's power +level in *every* desired room — new and already-observed. The +observed-room pass is the healing path: legacy rooms are fixed on the first +reconcile after deployment without any manual backfill. + +- Level mapping (`humanRoomPowerLevel`): `permissionLevel 1` → 100 + (co-owner: full room control, matching the admin-equivalent scope); + levels 2/3 → 50 (Matrix's default member authority). +- **Level-50 authority — explicitly accepted.** Matrix homeserver defaults + gate `kick`, `ban`, and `redact` at 50, so an L2/L3 human at level 50 can + rename the room, invite members, kick/ban members strictly below 50 + (the workers at 0 — but never the manager/leader/admin at 100), and + redact any message in the room. They cannot change power levels (100) or + create the room. We accept this authority rather than raising the + `kick`/`ban`/`redact` thresholds to 100: + - The humans at 50 are operators scoped to those rooms; the room's + manager sits at 100 above them, so kick/ban cannot be turned against + the team's control plane. + - Workers are service accounts; a human kicking/banning a stuck worker + is reversible housekeeping (the reconciler re-invites membership on + the next cycle) and is a useful operator lever, not a security + escalation. + - Redact at 50 is message cleanup within a room the human is already a + member of; the alternative (raising thresholds in every room-creation + path) would change the security posture of all worker/team/project + rooms — a system-wide policy change out of scope for this PR. +- The grant is a **merge**, never a replace: `Provisioner. + EnsureRoomPowerLevel` reads the current `m.room.power_levels` + (`matrix.Client.GetRoomState`, new — admin identity, 404 → empty state), + adds/raises the human's entry in `users`, preserves every other user and + every non-user setting (`users_default`, `state_default`, `ban`, …), and + writes back only when the level actually changed. Steady state = one GET + per room per cycle, zero writes. +- Errors are non-fatal per the reconcile's existing error policy: a failed + grant is logged and retried on the next cycle; the room is still recorded + in `status.rooms`. + +**Project rooms.** The Controller never creates project rooms; the Manager +does, via `create-project.sh`, which already writes a +`power_level_content_override` (manager + admin at 100, workers at 0) but +has no way to lift a human operator. New optional flag: + +``` +create-project.sh --id p1 --title T --workers w1,w2 --grant-admin luo,sunzong +``` + +`--grant-admin` accepts local parts or full Matrix IDs and adds each user at +level 100 to the creation-time override. Rooms created before this change +are healed one-time by the Manager (a `PUT m.room.power_levels` per room) — +a one-off operations task, not part of this PR. + +## What is not changed + +- Worker / team / DM room creation keeps its existing power levels + (manager / admin / leader at 100, workers at 0). +- Worker service accounts still cannot manage rooms (level 0 unchanged). +- No CRD change; the mapping is derived from the existing + `spec.permissionLevel`. + +## Tests + +- `internal/matrix/client_test.go` — `TestGetRoomState`: returns the state + **content** (not the event envelope) with the admin token; missing state + → `(nil, nil)`, not an error. +- `internal/service/provisioner_power_test.go`: legacy room → write with + the user's level; existing users merged and untouched; extension fields + (`events`, `invite`, `notifications`) preserved through the write — only + the target users entry is mutated; exact-match level → no write; + **demotion revokes: a user at 100 granted 50 is lowered to 50**; read + error propagates with no write; state without a `users` map handled; + second grant preserves the first (JSON round-trip semantics). +- `internal/controller/human_controller_test.go`: + `TestHumanReconciler_PowerLevelMapping` (level 1 → 100 in both a new room + and an already-observed room; grant targets the human's Matrix ID), + `TestHumanReconciler_PowerLevelL2GetsDefault` (level 2 → 50), + `TestHumanReconciler_PowerLevelErrorNonFatal` (grant failure does not + block the reconcile; room still recorded). diff --git a/manager/agent/skills/project-management/references/create-project.md b/manager/agent/skills/project-management/references/create-project.md index fb18653e4..456c8b8b3 100644 --- a/manager/agent/skills/project-management/references/create-project.md +++ b/manager/agent/skills/project-management/references/create-project.md @@ -35,6 +35,18 @@ bash /opt/agentteams/agent/skills/project-management/scripts/create-project.sh \ The script handles: directory creation, meta.json, placeholder plan.md, Matrix room creation (with admin + all workers invited), Manager groupAllowFrom update, and MinIO sync. +**Room administration for human operators (`--grant-admin`).** Human users who are not the default admin join the project room at the implicit power level 0 and get 403 on room operations (rename, inviting a colleague). When the project involves additional human operators who must be able to administer the room, pass their Matrix local parts (or full Matrix IDs): + +```bash +bash /opt/agentteams/agent/skills/project-management/scripts/create-project.sh \ + --id "${PROJECT_ID}" \ + --title "" \ + --workers "worker1,worker2,worker3" \ + --grant-admin "sunzong" +``` + +Each user listed in `--grant-admin` co-owns the project room (power level 100) from creation. Only pass humans who genuinely need room administration; workers are always created at level 0. + After the script, **fill in the full plan.md** with phases, tasks, and assignments (see `references/plan-format.md` for format). ## Step 1c: Present plan (and confirm) diff --git a/manager/agent/skills/project-management/scripts/create-project.sh b/manager/agent/skills/project-management/scripts/create-project.sh index f74d0475f..c9e8b82e6 100755 --- a/manager/agent/skills/project-management/scripts/create-project.sh +++ b/manager/agent/skills/project-management/scripts/create-project.sh @@ -14,18 +14,20 @@ source /opt/agentteams/scripts/lib/agentteams-env.sh PROJECT_ID="" PROJECT_TITLE="" WORKERS_CSV="" +GRANT_ADMIN_CSV="" while [ $# -gt 0 ]; do case "$1" in - --id) PROJECT_ID="$2"; shift 2 ;; - --title) PROJECT_TITLE="$2"; shift 2 ;; - --workers) WORKERS_CSV="$2"; shift 2 ;; + --id) PROJECT_ID="$2"; shift 2 ;; + --title) PROJECT_TITLE="$2"; shift 2 ;; + --workers) WORKERS_CSV="$2"; shift 2 ;; + --grant-admin) GRANT_ADMIN_CSV="$2"; shift 2 ;; *) echo "Unknown option: $1"; exit 1 ;; esac done if [ -z "${PROJECT_ID}" ] || [ -z "${PROJECT_TITLE}" ] || [ -z "${WORKERS_CSV}" ]; then - echo "Usage: create-project.sh --id <PROJECT_ID> --title <TITLE> --workers <w1,w2,...>" + echo "Usage: create-project.sh --id <PROJECT_ID> --title <TITLE> --workers <w1,w2,...> [--grant-admin <u1,u2,...>]" exit 1 fi @@ -107,6 +109,7 @@ log "Step 2: Creating Matrix project room..." # Build invite list and worker power level overrides (all workers → level 0) INVITE_LIST="[\"@${ADMIN_USER}:${MATRIX_DOMAIN}\"" WORKER_POWER_LEVELS="" +GRANT_ADMIN_LEVELS="" IFS=',' read -ra WORKER_ARR <<< "${WORKERS_CSV}" for worker in "${WORKER_ARR[@]}"; do worker=$(echo "${worker}" | tr -d ' ') @@ -114,6 +117,25 @@ for worker in "${WORKER_ARR[@]}"; do INVITE_LIST="${INVITE_LIST},\"@${worker}:${MATRIX_DOMAIN}\"" WORKER_POWER_LEVELS="${WORKER_POWER_LEVELS},\"@${worker}:${MATRIX_DOMAIN}\": 0" done +# --grant-admin: extra users that should co-own the room (level 100) — e.g. +# human operators who otherwise join at the implicit level 0 and 403 on room +# operations (rename / invite). Accepts local parts or full Matrix IDs. +if [ -n "${GRANT_ADMIN_CSV}" ]; then + IFS=',' read -ra GRANT_ARR <<< "${GRANT_ADMIN_CSV}" + for ga in "${GRANT_ARR[@]}"; do + ga=$(echo "${ga}" | tr -d ' ') + [ -z "${ga}" ] && continue + case "${ga}" in + @*:*) grant_id="${ga}" ;; + @*) grant_id="@${ga#@}:${MATRIX_DOMAIN}" ;; + *) grant_id="@${ga}:${MATRIX_DOMAIN}" ;; + esac + # The grant only takes effect once the user is in the room — invite + # them at creation time (a grant without membership is a no-op). + INVITE_LIST="${INVITE_LIST},\"${grant_id}\"" + GRANT_ADMIN_LEVELS="${GRANT_ADMIN_LEVELS},\"${grant_id}\": 100" + done +fi INVITE_LIST="${INVITE_LIST}]" MANAGER_MATRIX_ID="@manager:${MATRIX_DOMAIN}" @@ -129,7 +151,7 @@ ROOM_RESP=$(curl -sf -X POST ${AGENTTEAMS_MATRIX_URL}/_matrix/client/v3/createRo "power_level_content_override": { "users": { "'"${MANAGER_MATRIX_ID}"'": 100, - "'"${ADMIN_MATRIX_ID}"'": 100'"${WORKER_POWER_LEVELS}"' + "'"${ADMIN_MATRIX_ID}"'": 100'"${WORKER_POWER_LEVELS}"''"${GRANT_ADMIN_LEVELS}"' } } }' 2>/dev/null) || _fail "Failed to create Matrix project room" diff --git a/tests/check-create-project-grant-admin.sh b/tests/check-create-project-grant-admin.sh new file mode 100755 index 000000000..b2790498c --- /dev/null +++ b/tests/check-create-project-grant-admin.sh @@ -0,0 +1,42 @@ +#!/usr/bin/env bash + +set -euo pipefail + +ROOT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +SCRIPT="${ROOT_DIR}/manager/agent/skills/project-management/scripts/create-project.sh" +REFERENCE="${ROOT_DIR}/manager/agent/skills/project-management/references/create-project.md" + +fail() { + echo "FAIL: $*" >&2 + exit 1 +} + +[ -f "${SCRIPT}" ] || fail "create-project.sh not found at ${SCRIPT}" +[ -f "${REFERENCE}" ] || fail "create-project.md reference not found at ${REFERENCE}" + +# 1. The script must parse the --grant-admin flag. +grep -q -- "--grant-admin) GRANT_ADMIN_CSV" "${SCRIPT}" || + fail "create-project.sh must parse --grant-admin into GRANT_ADMIN_CSV" + +# 2. The usage line must document the new optional flag. +grep -Fq -- "[--grant-admin <u1,u2,...>]" "${SCRIPT}" || + fail "create-project.sh usage line must document [--grant-admin <u1,u2,...>]" + +# 3. Granted users must land in the room-creation power level override at +# level 100 (co-owner), next to the admin and worker entries. +grep -Fq -- 'GRANT_ADMIN_LEVELS="${GRANT_ADMIN_LEVELS},\"${grant_id}\": 100"' "${SCRIPT}" || + fail "create-project.sh must add each --grant-admin user at power level 100" +grep -F -- '${WORKER_POWER_LEVELS}' "${SCRIPT}" | grep -Fq -- '${GRANT_ADMIN_LEVELS}' || + fail "power_level_content_override must splice in the granted admin levels" + +# 4. The runtime-facing project-management reference must instruct the +# Manager to pass --grant-admin — the script flag alone is dead code if +# the Manager is never told about it. +grep -Fq -- "--grant-admin" "${REFERENCE}" || + fail "create-project.md must document --grant-admin for the Manager" +grep -Fq -- "--grant-admin \"sunzong\"" "${REFERENCE}" || + fail "create-project.md must show a concrete --grant-admin usage example" +grep -qi -- "co-owns the project room" "${REFERENCE}" || + fail "create-project.md must explain what --grant-admin grants (level 100 co-ownership)" + +echo "PASS: create-project.sh --grant-admin is implemented and wired into the runtime reference" From 949c7dc137882cd5cd1d89c427bf8b355b350c29 Mon Sep 17 00:00:00 2001 From: LUOSENGWA <luosengwa@qq.com> Date: Fri, 11 Sep 2026 07:28:38 +0000 Subject: [PATCH 2/6] fix(controller): authorized demotion/revocation and TeamAdmin-room actor Address the two authorization blockers from re-review of 7ae66601: 1. Equal-level demotion and access revocation (spec v8 rules 9.6 / 4.5.4): - EnsureRoomPowerLevel takes actorToken + selfToken: on M_FORBIDDEN (the homeserver refuses to change another user whose level is not strictly below the sender's) it retries with the target's own token, whose own entry is exempt from the rule. The reconciler fetches that token lazily, so steady-state cycles still issue no Matrix Login. - Revocation chain: admin kick -> self-leave with the human's own token (always authorized, the only in-band path for an equal-level 100) -> Tuwunel admin-bot force-leave. - KickFromRoomWithToken no longer swallows a 403 'cannot kick' as an idempotent success; only a not-in-room answer is idempotent. Rejected writes surface as matrix.APIError (M_FORBIDDEN) via IsForbidden. - The fake matrix client now ENFORCES the spec auth rules (membership, required level, 9.3/9.4/9.5/9.6/9.7, kick 4.5.4, invite 4.4), so the demotion/revocation tests prove behavior against an authorization-aware double instead of a permissive one. 2. TeamAdmin-owned rooms: GetRoomState/SetRoomState accept an explicit token (admin fallback), and the human reconciler grants team rooms of teams with spec.admin as that TeamAdmin (shared resolveTeamAdminActor), since the homeserver admin is not a member there. Controller tests cover the actor selection, the 403 -> self-token retry (lazy login), the revocation fallbacks, and the no-login-on-success steady state. Design doc (docs/design/room-power-levels.md) gains an Authorization section with the spec rules, actor selection, and fallback chains. --- .../controller/human_controller_test.go | 212 ++++++++- .../controller/human_reconcile_rooms.go | 98 +++- .../internal/controller/human_scope.go | 31 +- .../internal/controller/team_admin_actor.go | 80 ++++ .../internal/controller/team_controller.go | 50 +- .../internal/matrix/client.go | 99 +++- .../internal/matrix/client_test.go | 159 ++++++- .../internal/service/interfaces.go | 55 ++- .../internal/service/provisioner.go | 39 +- .../internal/service/provisioner_human.go | 22 + .../service/provisioner_power_test.go | 179 ++++++- .../internal/service/provisioner_team_test.go | 440 +++++++++++++++++- .../test/testutil/mocks/human_provisioner.go | 42 +- .../test/testutil/mocks/provisioner.go | 39 +- docs/design/room-power-levels.md | 98 +++- 15 files changed, 1489 insertions(+), 154 deletions(-) create mode 100644 agentteams-controller/internal/controller/team_admin_actor.go diff --git a/agentteams-controller/internal/controller/human_controller_test.go b/agentteams-controller/internal/controller/human_controller_test.go index 5ae3cf2be..17c353bc1 100644 --- a/agentteams-controller/internal/controller/human_controller_test.go +++ b/agentteams-controller/internal/controller/human_controller_test.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "sort" + "sync/atomic" "testing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -15,6 +16,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" v1beta1 "github.com/agentscope-ai/AgentTeams/agentteams-controller/api/v1beta1" + "github.com/agentscope-ai/AgentTeams/agentteams-controller/internal/matrix" "github.com/agentscope-ai/AgentTeams/agentteams-controller/internal/service" "github.com/agentscope-ai/AgentTeams/agentteams-controller/test/testutil/mocks" ) @@ -719,7 +721,7 @@ func TestHumanReconciler_PowerLevelErrorNonFatal(t *testing.T) { human.Finalizers = []string{finalizerName} rig := newHumanRig(t, human, worker) - rig.prov.EnsureRoomPowerLevelFn = func(ctx context.Context, roomID, userID string, level int) error { + rig.prov.EnsureRoomPowerLevelFn = func(ctx context.Context, roomID, userID string, level int, actorToken, selfToken string) error { return fmt.Errorf("matrix unavailable") } @@ -731,3 +733,211 @@ func TestHumanReconciler_PowerLevelErrorNonFatal(t *testing.T) { t.Errorf("room not recorded despite power failure: %v", out.Status.Rooms) } } + +// A TeamAdmin-owned team room must be granted with the team admin's token +// (the homeserver admin is not a member of those rooms), while worker DM +// rooms keep the default admin actor. +func TestHumanReconciler_PowerGrantUsesTeamAdminActor(t *testing.T) { + worker := newReadyWorker("w1", "!room-w1:localhost") + team := newReadyTeam("t1", "!room-t1:localhost") + team.Spec.Admin = &v1beta1.TeamAdminSpec{Name: "ada", MatrixUserID: "@ada:localhost"} + adminHuman := newHuman("ada", v1beta1.HumanSpec{}) + adminHuman.Status.MatrixUserID = "@ada:localhost" + adminHuman.Status.InitialPassword = "ada-pw" + adminHuman.Status.Phase = "Active" + + human := newHuman("alice", v1beta1.HumanSpec{ + PermissionLevel: 2, + AccessibleWorkers: []string{"w1"}, + AccessibleTeams: []string{"t1"}, + }) + human.Status.MatrixUserID = "@alice:localhost" + human.Status.InitialPassword = "alice-pw" + human.Status.Rooms = []string{"!room-w1:localhost"} + human.Status.Phase = "Active" + human.Finalizers = []string{finalizerName} + + var adminLogins atomic.Int32 + rig := newHumanRig(t, human, worker, team, adminHuman) + rig.prov.LoginWithPasswordFn = func(ctx context.Context, name, password string) (string, error) { + switch name { + case "ada": + adminLogins.Add(1) + return "teamadmin-token", nil + case "alice": + return "alice-token", nil + } + return "", fmt.Errorf("unexpected login %s", name) + } + + out, _, err := rig.reconcile("alice") + if err != nil { + t.Fatalf("reconcile: %v", err) + } + byRoom := map[string]string{} + for _, c := range rig.prov.Calls.EnsureRoomPowerLevel { + byRoom[c.RoomID] = c.ActorToken + } + if got := byRoom["!room-t1:localhost"]; got != "teamadmin-token" { + t.Errorf("team room actor=%q, want teamadmin-token (calls=%+v)", got, rig.prov.Calls.EnsureRoomPowerLevel) + } + if got := byRoom["!room-w1:localhost"]; got != "" { + t.Errorf("worker room actor=%q, want default admin (\"\")", got) + } + if adminLogins.Load() == 0 { + t.Error("team admin token was never resolved via login") + } + if len(out.Status.Rooms) != 2 { + t.Errorf("Status.Rooms=%v, want both rooms", out.Status.Rooms) + } +} + +// Steady-state: an equal-level demotion (actor 403 on the strict-greater +// rule) must be retried with the human's OWN token — lazily, i.e. the +// login happens only after the 403. +func TestHumanReconciler_EqualLevelDemotionSelfWriteFallback(t *testing.T) { + worker := newReadyWorker("w1", "!room-w1:localhost") + human := newHuman("carol", v1beta1.HumanSpec{ + PermissionLevel: 2, + AccessibleWorkers: []string{"w1"}, + }) + human.Status.MatrixUserID = "@carol:localhost" + human.Status.InitialPassword = "stored-pw" + human.Status.Rooms = []string{"!room-w1:localhost"} // already a member + human.Status.Phase = "Active" + human.Finalizers = []string{finalizerName} + + var logins atomic.Int32 + rig := newHumanRig(t, human, worker) + rig.prov.EnsureRoomPowerLevelFn = func(ctx context.Context, roomID, userID string, level int, actorToken, selfToken string) error { + if selfToken == "" { + // The homeserver's strict-greater rule rejects the actor's + // equal-level demotion. + return &matrix.APIError{StatusCode: 403, ErrCode: "M_FORBIDDEN", Message: "target level not below sender"} + } + return nil + } + rig.prov.LoginWithPasswordFn = func(ctx context.Context, name, password string) (string, error) { + logins.Add(1) + return "carol-token", nil + } + + if _, _, err := rig.reconcile("carol"); err != nil { + t.Fatalf("reconcile: %v", err) + } + calls := rig.prov.Calls.EnsureRoomPowerLevel + if len(calls) != 2 { + t.Fatalf("power calls=%d, want 2 (actor + self), got %+v", len(calls), calls) + } + if calls[0].SelfToken != "" { + t.Errorf("first attempt must not carry a self token: %+v", calls[0]) + } + if calls[1].SelfToken != "carol-token" { + t.Errorf("retry must use the human's own token, got %q", calls[1].SelfToken) + } + if logins.Load() != 1 { + t.Errorf("logins=%d, want exactly 1 (lazy, after the 403)", logins.Load()) + } +} + +// Steady-state: when the grant never 403s, no login is issued. +func TestHumanReconciler_PowerGrantNoLoginOnSuccess(t *testing.T) { + worker := newReadyWorker("w1", "!room-w1:localhost") + human := newHuman("carol", v1beta1.HumanSpec{ + PermissionLevel: 2, + AccessibleWorkers: []string{"w1"}, + }) + human.Status.MatrixUserID = "@carol:localhost" + human.Status.InitialPassword = "stored-pw" + human.Status.Rooms = []string{"!room-w1:localhost"} + human.Status.Phase = "Active" + human.Finalizers = []string{finalizerName} + + var logins atomic.Int32 + rig := newHumanRig(t, human, worker) + rig.prov.LoginWithPasswordFn = func(ctx context.Context, name, password string) (string, error) { + logins.Add(1) + return "carol-token", nil + } + + if _, _, err := rig.reconcile("carol"); err != nil { + t.Fatalf("reconcile: %v", err) + } + if logins.Load() != 0 { + t.Errorf("logins=%d, want 0 (no 403, no login)", logins.Load()) + } +} + +// Revocation: when the admin kick is rejected (equal-power, or the admin +// is not a member), the human leaves with their own token and the room is +// dropped from status. +func TestHumanReconciler_RevocationSelfLeaveFallback(t *testing.T) { + human := newHuman("dave", v1beta1.HumanSpec{}) + human.Status.MatrixUserID = "@dave:localhost" + human.Status.InitialPassword = "stored-pw" + human.Status.Rooms = []string{"!room-gone:localhost"} // no longer desired + human.Status.Phase = "Active" + human.Finalizers = []string{finalizerName} + + var leaves []string + rig := newHumanRig(t, human) + rig.prov.KickFromRoomFn = func(ctx context.Context, roomID, userID, reason string) error { + return &matrix.APIError{StatusCode: 403, ErrCode: "M_FORBIDDEN", Message: "cannot kick: target power level not below kicker"} + } + rig.prov.LoginWithPasswordFn = func(ctx context.Context, name, password string) (string, error) { + return "dave-token", nil + } + rig.prov.LeaveRoomAsFn = func(ctx context.Context, roomID, userToken string) error { + leaves = append(leaves, roomID+"/"+userToken) + return nil + } + + out, _, err := rig.reconcile("dave") + if err != nil { + t.Fatalf("reconcile: %v", err) + } + if len(rig.prov.Calls.KickFromRoom) != 1 { + t.Fatalf("kick calls=%+v, want 1", rig.prov.Calls.KickFromRoom) + } + if len(leaves) != 1 || leaves[0] != "!room-gone:localhost/dave-token" { + t.Errorf("self-leave=%v, want [!room-gone:localhost/dave-token]", leaves) + } + if len(out.Status.Rooms) != 0 { + t.Errorf("Status.Rooms=%v, want empty (revoked)", out.Status.Rooms) + } +} + +// Revocation: with no usable human token, the admin-bot force-leave is +// the last resort. +func TestHumanReconciler_RevocationForceLeaveLastResort(t *testing.T) { + human := newHuman("erin", v1beta1.HumanSpec{}) + human.Status.MatrixUserID = "@erin:localhost" + human.Status.InitialPassword = "stale-pw" + human.Status.Rooms = []string{"!room-gone:localhost"} + human.Status.Phase = "Active" + human.Finalizers = []string{finalizerName} + + var forced []string + rig := newHumanRig(t, human) + rig.prov.KickFromRoomFn = func(ctx context.Context, roomID, userID, reason string) error { + return &matrix.APIError{StatusCode: 403, ErrCode: "M_FORBIDDEN", Message: "cannot kick: target power level not below kicker"} + } + rig.prov.LoginWithPasswordFn = func(ctx context.Context, name, password string) (string, error) { + return "", errors.New("password stale: M_FORBIDDEN from homeserver login") + } + rig.prov.ForceLeaveRoomFn = func(ctx context.Context, userID, roomID string) error { + forced = append(forced, roomID+"/"+userID) + return nil + } + + out, _, err := rig.reconcile("erin") + if err != nil { + t.Fatalf("reconcile: %v", err) + } + if len(forced) != 1 || forced[0] != "!room-gone:localhost/@erin:localhost" { + t.Errorf("force-leave=%v, want [!room-gone:localhost/@erin:localhost]", forced) + } + if len(out.Status.Rooms) != 0 { + t.Errorf("Status.Rooms=%v, want empty (revoked)", out.Status.Rooms) + } +} diff --git a/agentteams-controller/internal/controller/human_reconcile_rooms.go b/agentteams-controller/internal/controller/human_reconcile_rooms.go index d6c6e651a..47494700b 100644 --- a/agentteams-controller/internal/controller/human_reconcile_rooms.go +++ b/agentteams-controller/internal/controller/human_reconcile_rooms.go @@ -3,6 +3,9 @@ package controller import ( "context" + "github.com/agentscope-ai/AgentTeams/agentteams-controller/api/v1beta1" + "github.com/agentscope-ai/AgentTeams/agentteams-controller/internal/matrix" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" ) @@ -46,7 +49,7 @@ func (r *HumanReconciler) reconcileHumanRooms(ctx context.Context, s *humanScope powerLevel := humanRoomPowerLevel(h.Spec.PermissionLevel) - for rid := range desired { + for rid, origin := range desired { alreadyMember := false if _, ok := observed[rid]; ok { alreadyMember = true @@ -78,28 +81,109 @@ func (r *HumanReconciler) reconcileHumanRooms(ctx context.Context, s *humanScope // levels accounted for human members, leaving them at the implicit // level 0 and 403 on room operations (rename, invite). Non-fatal // per this file's error policy; the next cycle retries. - if err := r.Provisioner.EnsureRoomPowerLevel(ctx, rid, matrixUserID, powerLevel); err != nil { - logger.Error(err, "failed to ensure human power level", "room", rid, "level", powerLevel) + // + // The grant must run as an actor actually authorized in the room: + // TeamAdmin-owned rooms do not include the homeserver admin, so + // the default admin identity would be rejected with M_FORBIDDEN. + actorToken := r.roomActorToken(ctx, origin, h.Namespace) + grantErr := r.Provisioner.EnsureRoomPowerLevel(ctx, rid, matrixUserID, powerLevel, actorToken, "") + if matrix.IsForbidden(grantErr) { + // An M_FORBIDDEN on the actor write is the homeserver's + // strict-greater rule: the actor's level is not above the + // human's CURRENT level, i.e. an equal-level demotion (a + // former L1 human at 100 being lowered to 50). The sender's + // OWN entry is exempt from that rule, so retry with the + // human's own token — lazily, so steady-state cycles never + // issue a Matrix Login. + if token := r.ensureUserToken(ctx, s); token != "" { + if retryErr := r.Provisioner.EnsureRoomPowerLevel(ctx, rid, matrixUserID, powerLevel, actorToken, token); retryErr == nil { + grantErr = nil + } else { + logger.Error(retryErr, "failed to ensure human power level via self-write", "room", rid, "level", powerLevel) + } + } + } + if grantErr != nil { + logger.Error(grantErr, "failed to ensure human power level", "room", rid, "level", powerLevel) } } - // Removals: in-place filter. A failed kick keeps the room so the + // Removals: in-place filter. A failed revocation keeps the room so the // next reconcile retries, matching pre-refactor behavior. + // + // Revocation chain — each stage covers what the previous cannot: + // 1. kick as the homeserver admin: works for rooms the admin is a + // member of when the target's level is below the admin's; + // 2. self-leave with the human's own token: always authorized for a + // joined member regardless of power levels (spec room-auth rule: + // a user may leave their own room) — the only in-band revocation + // for an equal-level (100) human, and the only one that works in + // rooms the admin is not in (TeamAdmin-owned rooms); + // 3. the Tuwunel admin bot force-leave: last resort when the human + // token is unavailable (stale password). Like the team-reconcile + // usage, a confirmed command delivery is treated as resolved. kept := next[:0] for _, rid := range next { if _, ok := desired[rid]; ok { kept = append(kept, rid) continue } - if err := r.Provisioner.KickFromRoom(ctx, rid, matrixUserID, "access revoked"); err != nil { - logger.Error(err, "failed to kick human from room", "room", rid) - kept = append(kept, rid) + if err := r.Provisioner.KickFromRoom(ctx, rid, matrixUserID, "access revoked"); err == nil { + continue // kicked, or the user was already out + } else { + logger.V(1).Info("admin kick rejected; trying revocation fallbacks", "room", rid, "err", err.Error()) + } + removed := false + if token := r.ensureUserToken(ctx, s); token != "" { + if lerr := r.Provisioner.LeaveRoomAs(ctx, rid, token); lerr == nil { + removed = true + } else { + logger.Error(lerr, "self-leave failed", "room", rid) + } + } + if !removed { + if ferr := r.Provisioner.ForceLeaveRoom(ctx, matrixUserID, rid); ferr == nil { + removed = true + } else { + logger.Error(ferr, "force-leave failed", "room", rid) + } + } + if !removed { + kept = append(kept, rid) // keep for the next cycle's retry } } h.Status.Rooms = kept } +// roomActorToken returns the access token of the actor authorized to read +// and write state in the room described by origin. Teams with a TeamAdmin +// configured own their team room (the homeserver admin is deliberately not +// a member), so grants there must run as that admin; every other room +// (worker DM rooms, teams without an admin) keeps the default +// homeserver-admin actor (""). An unavailable actor (admin human not +// provisioned, login failed) degrades to the default — the grant then 403s +// and is retried next cycle rather than silently skipping. +func (r *HumanReconciler) roomActorToken(ctx context.Context, origin humanRoomOrigin, namespace string) string { + if origin.teamName == "" { + return "" + } + var team v1beta1.Team + if err := r.Client.Get(ctx, client.ObjectKey{Name: origin.teamName, Namespace: namespace}, &team); err != nil { + return "" + } + if team.Spec.Admin == nil { + return "" + } + actor, err := resolveTeamAdminActor(ctx, r.Client, r.Provisioner, &team) + if err != nil { + log.FromContext(ctx).V(1).Info("team admin actor unavailable; power grant uses the default admin and may be rejected", + "team", origin.teamName, "err", err.Error()) + return "" + } + return actor.Token +} + // humanRoomPowerLevel maps the Human CR permission level to the Matrix power // level granted in rooms the human belongs to. Level 1 (admin equivalent) // co-owns the rooms (full control); levels 2/3 (team/worker scoped) get diff --git a/agentteams-controller/internal/controller/human_scope.go b/agentteams-controller/internal/controller/human_scope.go index b693b313f..50092bd64 100644 --- a/agentteams-controller/internal/controller/human_scope.go +++ b/agentteams-controller/internal/controller/human_scope.go @@ -56,24 +56,35 @@ func computeHumanPhase(h *v1beta1.Human, reconcileErr error) string { return "Active" } +// humanRoomOrigin records where a desired room came from so the room phase +// can pick an actor authorized to write that room's state: a TeamAdmin +// owns the team room of a team that configures an Admin (the homeserver +// admin is deliberately not a member of those rooms), while worker DM +// rooms and teams without an Admin keep the default homeserver-admin actor. +type humanRoomOrigin struct { + teamName string // non-empty for team rooms (team.Status.TeamRoomID) + workerName string // non-empty for worker DM rooms (worker.Status.RoomID) +} + // buildDesiredHumanRooms resolves Spec.AccessibleWorkers / AccessibleTeams // into the set of Matrix room IDs the human should currently be a member -// of. Workers/Teams that don't exist or haven't finished provisioning -// (empty Status.RoomID / TeamRoomID) are simply skipped — they'll be -// picked up on a later reconcile once their rooms materialize. +// of, annotated with each room's origin. Workers/Teams that don't exist or +// haven't finished provisioning (empty Status.RoomID / TeamRoomID) are +// simply skipped — they'll be picked up on a later reconcile once their +// rooms materialize. // -// Returned as a set (map-to-empty-struct) rather than a slice because -// the reconciler does membership comparisons against the observed -// Status.Rooms set. -func buildDesiredHumanRooms(ctx context.Context, c client.Client, h *v1beta1.Human) map[string]struct{} { - desired := make(map[string]struct{}) +// Returned as a map (roomID -> origin) rather than a slice because the +// reconciler does membership comparisons against the observed Status.Rooms +// set and needs the origin to choose the room-state actor. +func buildDesiredHumanRooms(ctx context.Context, c client.Client, h *v1beta1.Human) map[string]humanRoomOrigin { + desired := make(map[string]humanRoomOrigin) for _, workerName := range h.Spec.AccessibleWorkers { var worker v1beta1.Worker if err := c.Get(ctx, client.ObjectKey{Name: workerName, Namespace: h.Namespace}, &worker); err != nil { continue } if worker.Status.RoomID != "" { - desired[worker.Status.RoomID] = struct{}{} + desired[worker.Status.RoomID] = humanRoomOrigin{workerName: workerName} } } for _, teamName := range h.Spec.AccessibleTeams { @@ -82,7 +93,7 @@ func buildDesiredHumanRooms(ctx context.Context, c client.Client, h *v1beta1.Hum continue } if team.Status.TeamRoomID != "" { - desired[team.Status.TeamRoomID] = struct{}{} + desired[team.Status.TeamRoomID] = humanRoomOrigin{teamName: teamName} } } return desired diff --git a/agentteams-controller/internal/controller/team_admin_actor.go b/agentteams-controller/internal/controller/team_admin_actor.go new file mode 100644 index 000000000..c7866a87a --- /dev/null +++ b/agentteams-controller/internal/controller/team_admin_actor.go @@ -0,0 +1,80 @@ +package controller + +import ( + "context" + "fmt" + "strings" + + "github.com/agentscope-ai/AgentTeams/agentteams-controller/api/v1beta1" + "github.com/agentscope-ai/AgentTeams/agentteams-controller/internal/controller/humanidentity" + "github.com/agentscope-ai/AgentTeams/agentteams-controller/internal/service" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// resolveTeamAdminActor resolves the Matrix identity and access token of +// the TeamAdmin (a Human CR) configured on a team. Shared by: +// +// - the team reconciler, which creates the team rooms and reconciles +// their membership AS that admin, and +// - the human reconciler, which must write room state (power levels) +// inside TeamAdmin-owned rooms — the homeserver admin is deliberately +// NOT a member of those rooms, so a read/write under the default +// admin identity is rejected with M_FORBIDDEN. +// +// Returns (teamAdminActor{}, nil) when the team has no Admin configured. +// The token comes from the identity source's EnsureUserToken (a Matrix +// login when the password is stored), so a TeamAdmin whose password is +// unavailable surfaces an error and the caller degrades (default actor / +// retry next cycle) instead of guessing. +func resolveTeamAdminActor(ctx context.Context, c client.Client, prov service.HumanProvisioner, t *v1beta1.Team) (teamAdminActor, error) { + if t.Spec.Admin == nil { + return teamAdminActor{}, nil + } + if strings.TrimSpace(t.Spec.Admin.Name) == "" { + return teamAdminActor{}, fmt.Errorf("team admin human name is required") + } + + var human v1beta1.Human + key := client.ObjectKey{Name: t.Spec.Admin.Name, Namespace: t.Namespace} + if err := c.Get(ctx, key, &human); err != nil { + return teamAdminActor{}, fmt.Errorf("load team admin human %s/%s: %w", key.Namespace, key.Name, err) + } + + identity, err := humanidentity.ResolveHuman(&human.Spec, human.Name, humanidentity.Deps{Provisioner: prov}) + if err != nil { + return teamAdminActor{}, fmt.Errorf("resolve team admin human %s/%s identity: %w", key.Namespace, key.Name, err) + } + matrixUserID := human.Status.MatrixUserID + if matrixUserID == "" { + if human.Spec.IdentitySource != nil { + return teamAdminActor{}, fmt.Errorf("team admin human %s/%s uses an external identity source but is not provisioned yet", + key.Namespace, key.Name) + } + matrixUserID = identity.MatrixUserID + } + if matrixUserID != identity.MatrixUserID { + return teamAdminActor{}, fmt.Errorf("team admin human %s/%s status.matrixUserID %q does not match resolved identity %q", + key.Namespace, key.Name, matrixUserID, identity.MatrixUserID) + } + if t.Spec.Admin.MatrixUserID != "" && t.Spec.Admin.MatrixUserID != matrixUserID { + return teamAdminActor{}, fmt.Errorf("team admin matrixUserId %q does not match Human %s/%s matrix user %q", + t.Spec.Admin.MatrixUserID, key.Namespace, key.Name, matrixUserID) + } + if identity.ManagesInitialPassword && !prov.MatrixAppServiceEnabled() && human.Status.InitialPassword == "" { + return teamAdminActor{}, fmt.Errorf("team admin human %s/%s has no initial password; cannot obtain Matrix token", + key.Namespace, key.Name) + } + + token, err := identity.Source.EnsureUserToken(ctx, &human.Spec, &human.Status, human.Name) + if err != nil { + return teamAdminActor{}, fmt.Errorf("login as team admin human %s/%s: %w", key.Namespace, key.Name, err) + } + if token == "" { + return teamAdminActor{}, fmt.Errorf("team admin human %s/%s has no Matrix token", key.Namespace, key.Name) + } + return teamAdminActor{ + MatrixUserID: matrixUserID, + Token: token, + Username: identity.MatrixLocalpart, + }, nil +} diff --git a/agentteams-controller/internal/controller/team_controller.go b/agentteams-controller/internal/controller/team_controller.go index 47f026b61..29bd29418 100644 --- a/agentteams-controller/internal/controller/team_controller.go +++ b/agentteams-controller/internal/controller/team_controller.go @@ -108,57 +108,11 @@ func (r *TeamReconciler) resolveTeamAdminActor(ctx context.Context, t *v1beta1.T if t.Spec.Admin == nil { return teamAdminActor{}, nil } - if strings.TrimSpace(t.Spec.Admin.Name) == "" { - return teamAdminActor{}, fmt.Errorf("team admin human name is required") - } - - var human v1beta1.Human - key := client.ObjectKey{Name: t.Spec.Admin.Name, Namespace: t.Namespace} - if err := r.Get(ctx, key, &human); err != nil { - return teamAdminActor{}, fmt.Errorf("load team admin human %s/%s: %w", key.Namespace, key.Name, err) - } - humanProv, ok := r.Provisioner.(service.HumanProvisioner) if !ok { - return teamAdminActor{}, fmt.Errorf("team admin human %s/%s requires HumanProvisioner support", key.Namespace, key.Name) - } - identity, err := humanidentity.ResolveHuman(&human.Spec, human.Name, humanidentity.Deps{Provisioner: humanProv}) - if err != nil { - return teamAdminActor{}, fmt.Errorf("resolve team admin human %s/%s identity: %w", key.Namespace, key.Name, err) - } - matrixUserID := human.Status.MatrixUserID - if matrixUserID == "" { - if human.Spec.IdentitySource != nil { - return teamAdminActor{}, fmt.Errorf("team admin human %s/%s uses an external identity source but is not provisioned yet", - key.Namespace, key.Name) - } - matrixUserID = identity.MatrixUserID - } - if matrixUserID != identity.MatrixUserID { - return teamAdminActor{}, fmt.Errorf("team admin human %s/%s status.matrixUserID %q does not match resolved identity %q", - key.Namespace, key.Name, matrixUserID, identity.MatrixUserID) - } - if t.Spec.Admin.MatrixUserID != "" && t.Spec.Admin.MatrixUserID != matrixUserID { - return teamAdminActor{}, fmt.Errorf("team admin matrixUserId %q does not match Human %s/%s matrix user %q", - t.Spec.Admin.MatrixUserID, key.Namespace, key.Name, matrixUserID) - } - if identity.ManagesInitialPassword && !r.Provisioner.MatrixAppServiceEnabled() && human.Status.InitialPassword == "" { - return teamAdminActor{}, fmt.Errorf("team admin human %s/%s has no initial password; cannot obtain Matrix token", - key.Namespace, key.Name) - } - - token, err := identity.Source.EnsureUserToken(ctx, &human.Spec, &human.Status, human.Name) - if err != nil { - return teamAdminActor{}, fmt.Errorf("login as team admin human %s/%s: %w", key.Namespace, key.Name, err) - } - if token == "" { - return teamAdminActor{}, fmt.Errorf("team admin human %s/%s has no Matrix token", key.Namespace, key.Name) + return teamAdminActor{}, fmt.Errorf("team admin human %s/%s requires HumanProvisioner support", t.Namespace, t.Spec.Admin.Name) } - return teamAdminActor{ - MatrixUserID: matrixUserID, - Token: token, - Username: identity.MatrixLocalpart, - }, nil + return resolveTeamAdminActor(ctx, r.Client, humanProv, t) } // deriveTeamWithResolvedIdentities returns a deep copy of t with the team diff --git a/agentteams-controller/internal/matrix/client.go b/agentteams-controller/internal/matrix/client.go index 93b2fce78..91ddb293d 100644 --- a/agentteams-controller/internal/matrix/client.go +++ b/agentteams-controller/internal/matrix/client.go @@ -25,6 +25,47 @@ import ( // instead of logging it as a hard error. var ErrAppServiceNotReady = errors.New("matrix appservice token not active yet") +// APIError is a non-2xx Matrix response whose body carried a decoded +// errcode (e.g. M_FORBIDDEN for an authorization rejection). Callers that +// need to react to a specific rejection (retry with a different actor, +// fall back to a self-operation, ...) should test with errors.As / +// IsForbidden instead of pattern-matching on error text. +type APIError struct { + StatusCode int + ErrCode string + Message string +} + +func (e *APIError) Error() string { + if e.ErrCode != "" { + return fmt.Sprintf("HTTP %d %s: %s", e.StatusCode, e.ErrCode, e.Message) + } + return fmt.Sprintf("HTTP %d", e.StatusCode) +} + +// IsForbidden reports whether err is (or wraps) a Matrix M_FORBIDDEN +// response — the homeserver's authorization rejection (insufficient power +// level, sender not a member of the room, equal-power kick/demotion, ...). +func IsForbidden(err error) bool { + var ae *APIError + return errors.As(err, &ae) && ae.StatusCode == http.StatusForbidden && ae.ErrCode == "M_FORBIDDEN" +} + +// apiErrorFromBody builds an *APIError from a non-2xx status line and the +// response body ({"errcode": "...", "error": "..."} when decodable). +func apiErrorFromBody(statusCode int, respBody []byte) *APIError { + ae := &APIError{StatusCode: statusCode} + var decoded struct { + ErrCode string `json:"errcode"` + Error string `json:"error"` + } + if err := json.Unmarshal(respBody, &decoded); err == nil { + ae.ErrCode = decoded.ErrCode + ae.Message = decoded.Error + } + return ae +} + // Client abstracts Matrix homeserver operations. // Implementations: TuwunelClient (current), future SynapseClient. type Client interface { @@ -61,11 +102,14 @@ type Client interface { SetRoomState(ctx context.Context, roomID, eventType, stateKey string, content map[string]interface{}, userToken string) error // GetRoomState reads the content of a single state event from a room - // using the homeserver-admin identity (the event's `content` object, - // not the full event envelope). A room that has never had the event - // set (e.g. legacy rooms with no m.room.power_levels) yields (nil, nil) - // rather than an error; any other failure is returned. - GetRoomState(ctx context.Context, roomID, eventType, stateKey string) (map[string]interface{}, error) + // (the event's `content` object, not the full event envelope). The + // read uses userToken when non-empty, otherwise the homeserver-admin + // identity — state reads are membership-scoped, so rooms the admin is + // not in (e.g. TeamAdmin-owned rooms) must be read with a token of a + // member. A room that has never had the event set (e.g. legacy rooms + // with no m.room.power_levels) yields (nil, nil) rather than an error; + // any other failure (including M_FORBIDDEN) is returned. + GetRoomState(ctx context.Context, roomID, eventType, stateKey, userToken string) (map[string]interface{}, error) // JoinRoom makes the user identified by token join the given room. JoinRoom(ctx context.Context, roomID, userToken string) error @@ -754,16 +798,20 @@ func (c *TuwunelClient) SetRoomState(ctx context.Context, roomID, eventType, sta return fmt.Errorf("set room state %s %s: %w", roomID, eventType, err) } if statusCode != http.StatusOK && statusCode != http.StatusCreated { - return fmt.Errorf("set room state %s %s: HTTP %d: %s", - roomID, eventType, statusCode, truncate(respBody, 500)) + return fmt.Errorf("set room state %s %s: %w", + roomID, eventType, apiErrorFromBody(statusCode, respBody)) } return nil } -func (c *TuwunelClient) GetRoomState(ctx context.Context, roomID, eventType, stateKey string) (map[string]interface{}, error) { - token, err := c.ensureAdminToken(ctx) - if err != nil { - return nil, fmt.Errorf("get room state %s %s: %w", roomID, eventType, err) +func (c *TuwunelClient) GetRoomState(ctx context.Context, roomID, eventType, stateKey, userToken string) (map[string]interface{}, error) { + token := userToken + if token == "" { + var err error + token, err = c.ensureAdminToken(ctx) + if err != nil { + return nil, fmt.Errorf("get room state %s %s: %w", roomID, eventType, err) + } } encodedRoom := encodeRoomID(roomID) // Always include the state-key segment (trailing slash when the key is @@ -779,8 +827,8 @@ func (c *TuwunelClient) GetRoomState(ctx context.Context, roomID, eventType, sta return nil, nil // state event never set on this room } if statusCode != http.StatusOK { - return nil, fmt.Errorf("get room state %s %s: HTTP %d: %s", - roomID, eventType, statusCode, truncate(respBody, 500)) + return nil, fmt.Errorf("get room state %s %s: %w", + roomID, eventType, apiErrorFromBody(statusCode, respBody)) } // The state endpoint returns the state CONTENT object directly // (e.g. {"users":{...},"ban":50}), not an event envelope — decode @@ -823,7 +871,11 @@ func (c *TuwunelClient) LeaveRoom(ctx context.Context, roomID, userToken string) return fmt.Errorf("leave room %s: %w", roomID, err) } if statusCode != http.StatusOK && statusCode != http.StatusCreated { - return fmt.Errorf("leave room %s: HTTP %d: %s", roomID, statusCode, truncate(respBody, 500)) + // Idempotent: the user is not (or no longer) in the room. + if statusCode == http.StatusNotFound { + return nil + } + return fmt.Errorf("leave room %s: %w", roomID, apiErrorFromBody(statusCode, respBody)) } return nil } @@ -1045,19 +1097,28 @@ func (c *TuwunelClient) KickFromRoomWithToken(ctx context.Context, roomID, userI if statusCode == http.StatusOK || statusCode == http.StatusCreated { return nil } - // Idempotent: user not in the room (or already left). + // Idempotent: target not in the room (or already left). Some servers + // answer this with a 403 message instead of a 404. if statusCode == http.StatusNotFound { return nil } if statusCode == http.StatusForbidden && resp.ErrCode == "M_FORBIDDEN" { lower := strings.ToLower(resp.Error) - if strings.Contains(lower, "not in") || strings.Contains(lower, "not a member") || - strings.Contains(lower, "cannot kick") { + if strings.Contains(lower, "not in") || strings.Contains(lower, "not a member") { return nil } } - return fmt.Errorf("kick %s from %s: HTTP %d %s %s: %s", - userID, roomID, statusCode, resp.ErrCode, resp.Error, truncate(respBody, 500)) + // Any other 403 is an authorization failure and is NEVER an idempotent + // success: the kicker is not a member of the room, or the homeserver + // rejected the kick because the target's power level is not strictly + // below the kicker's (spec room-auth rule: a kick requires the target's + // level to be less than the sender's). Silently returning nil here is + // what used to make equal-power kicks ("cannot kick ...") look + // successful — the caller then dropped the room from status while the + // user stayed in it. Callers must fall back (self-leave with the + // target's own token, or the admin-bot force-leave). + return fmt.Errorf("kick %s from %s: %w", + userID, roomID, apiErrorFromBody(statusCode, respBody)) } // ListJoinedRooms returns the room IDs joined by the user identified by diff --git a/agentteams-controller/internal/matrix/client_test.go b/agentteams-controller/internal/matrix/client_test.go index 8434958cc..c597ffe72 100644 --- a/agentteams-controller/internal/matrix/client_test.go +++ b/agentteams-controller/internal/matrix/client_test.go @@ -581,7 +581,7 @@ func TestGetRoomState(t *testing.T) { c := NewTuwunelClient(Config{ServerURL: server.URL, Domain: "d"}, server.Client()) - st, err := c.GetRoomState(context.Background(), "!room:d", "m.room.power_levels", "") + st, err := c.GetRoomState(context.Background(), "!room:d", "m.room.power_levels", "", "") if err != nil { t.Fatalf("GetRoomState: %v", err) } @@ -591,7 +591,7 @@ func TestGetRoomState(t *testing.T) { } // A room that never had the state set yields (nil, nil), not an error. - st, err = c.GetRoomState(context.Background(), "!room:d", "room.meta", "") + st, err = c.GetRoomState(context.Background(), "!room:d", "room.meta", "", "") if err != nil { t.Fatalf("missing state must not error: %v", err) } @@ -1104,3 +1104,158 @@ func TestGeneratePassword(t *testing.T) { t.Error("two generated passwords should not be equal") } } + +// A state read with an explicit user token must authenticate as that +// token, not the homeserver admin — TeamAdmin-owned rooms reject the +// admin (not a member) and require a member's token. +func TestGetRoomState_WithUserToken(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/_matrix/client/v3/rooms/!room:d/state/m.room.power_levels/": + if auth := r.Header.Get("Authorization"); auth != "Bearer teamadmin-token" { + t.Errorf("Authorization = %q, want Bearer teamadmin-token", auth) + } + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]interface{}{ + "users": map[string]interface{}{"@a:d": 100.0}, + }) + default: + t.Errorf("unexpected path: %s", r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + })) + defer server.Close() + + c := NewTuwunelClient(Config{ServerURL: server.URL, Domain: "d"}, server.Client()) + st, err := c.GetRoomState(context.Background(), "!room:d", "m.room.power_levels", "", "teamadmin-token") + if err != nil { + t.Fatalf("GetRoomState: %v", err) + } + if st["users"] == nil { + t.Fatalf("unexpected state: %#v", st) + } +} + +// A rejected state write (e.g. the strict-greater power-level rule, or a +// non-member actor) must surface as a decodable M_FORBIDDEN so callers +// can detect it with matrix.IsForbidden and fall back to an authorized +// actor / self-write. +func TestSetRoomState_Forbidden(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/_matrix/client/v3/login": + adminLoginHandler(t, w) + case "/_matrix/client/v3/rooms/!room:d/state/m.room.power_levels/": + w.WriteHeader(http.StatusForbidden) + json.NewEncoder(w).Encode(map[string]string{ + "errcode": "M_FORBIDDEN", + "error": "You don't have permission to modify that state.", + }) + default: + w.WriteHeader(http.StatusNotFound) + } + })) + defer server.Close() + + c := NewTuwunelClient(Config{ + ServerURL: server.URL, Domain: "d", AdminUser: "admin", AdminPassword: "pw", + }, server.Client()) + err := c.SetRoomState(context.Background(), "!room:d", "m.room.power_levels", "", + map[string]interface{}{"users": map[string]interface{}{"@a:d": 50.0}}, "") + if err == nil { + t.Fatal("expected M_FORBIDDEN, got nil") + } + if !IsForbidden(err) { + t.Fatalf("IsForbidden(%v) = false, want true", err) + } +} + +// A state read by a non-member actor (the homeserver admin in a +// TeamAdmin-owned room) surfaces M_FORBIDDEN rather than a generic error. +func TestGetRoomState_Forbidden(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/_matrix/client/v3/login": + adminLoginHandler(t, w) + case "/_matrix/client/v3/rooms/!room:d/state/m.room.power_levels/": + w.WriteHeader(http.StatusForbidden) + json.NewEncoder(w).Encode(map[string]string{ + "errcode": "M_FORBIDDEN", + "error": "You are not a member of that room.", + }) + default: + w.WriteHeader(http.StatusNotFound) + } + })) + defer server.Close() + + c := NewTuwunelClient(Config{ + ServerURL: server.URL, Domain: "d", AdminUser: "admin", AdminPassword: "pw", + }, server.Client()) + _, err := c.GetRoomState(context.Background(), "!room:d", "m.room.power_levels", "", "") + if err == nil { + t.Fatal("expected M_FORBIDDEN, got nil") + } + if !IsForbidden(err) { + t.Fatalf("IsForbidden(%v) = false, want true", err) + } +} + +// Regression: an equal-power kick rejection ("cannot kick ...", 403) must +// be returned as an error — the old message-sniffing branch treated it as +// idempotent success, which made the caller drop the room from status +// while the user stayed in it. +func TestKickFromRoom_EqualPowerForbidden(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/_matrix/client/v3/login": + adminLoginHandler(t, w) + case "/_matrix/client/v3/rooms/!room:d/kick": + w.WriteHeader(http.StatusForbidden) + json.NewEncoder(w).Encode(map[string]string{ + "errcode": "M_FORBIDDEN", + "error": "Cannot kick @alice:d: their power level is not below yours.", + }) + default: + w.WriteHeader(http.StatusNotFound) + } + })) + defer server.Close() + + c := NewTuwunelClient(Config{ + ServerURL: server.URL, Domain: "d", AdminUser: "admin", AdminPassword: "pw", + }, server.Client()) + err := c.KickFromRoom(context.Background(), "!room:d", "@alice:d", "access revoked") + if err == nil { + t.Fatal("equal-power kick rejection must be an error, got nil") + } + if !IsForbidden(err) { + t.Fatalf("IsForbidden(%v) = false, want true", err) + } +} + +// Leaving a room you are not (or no longer) in is idempotent. +func TestLeaveRoom_IdempotentNotFound(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/_matrix/client/v3/login": + adminLoginHandler(t, w) + case "/_matrix/client/v3/rooms/!room:d/leave": + w.WriteHeader(http.StatusNotFound) + json.NewEncoder(w).Encode(map[string]string{ + "errcode": "M_NOT_FOUND", + "error": "Not a member of that room.", + }) + default: + w.WriteHeader(http.StatusNotFound) + } + })) + defer server.Close() + + c := NewTuwunelClient(Config{ + ServerURL: server.URL, Domain: "d", AdminUser: "admin", AdminPassword: "pw", + }, server.Client()) + if err := c.LeaveRoom(context.Background(), "!room:d", ""); err != nil { + t.Errorf("expected nil for not-in-room, got %v", err) + } +} diff --git a/agentteams-controller/internal/service/interfaces.go b/agentteams-controller/internal/service/interfaces.go index bb47b54eb..8aa13ba7a 100644 --- a/agentteams-controller/internal/service/interfaces.go +++ b/agentteams-controller/internal/service/interfaces.go @@ -60,12 +60,24 @@ type WorkerProvisioner interface { // EnsureRoomPowerLevel reconciles userID's entry in the room's // m.room.power_levels to exactly the given power level (raising or - // lowering it), using the admin token. The complete existing content is - // preserved (every other user, every non-user field); only the target - // users entry is mutated. The call is a no-op write when the user - // already has exactly that level. Rooms that never had power_levels set - // (legacy) are treated as starting from an empty users map. - EnsureRoomPowerLevel(ctx context.Context, roomID, userID string, level int) error + // lowering it). The write uses actorToken when non-empty, otherwise the + // homeserver-admin identity — state reads/writes are membership-scoped, + // so TeamAdmin-owned rooms (where the admin is deliberately not a + // member) must be passed a token of an authorized member. The complete + // existing content is preserved (every other user, every non-user + // field); only the target users entry is mutated. The call is a no-op + // write when the user already has exactly that level. Rooms that never + // had power_levels set (legacy) are treated as starting from an empty + // users map. + // + // Homeserver authorization (spec room-auth rules) rejects a power-level + // change to another user whose current level is not strictly below the + // sender's — so an equal-level demotion (admin at 100 demoting a former + // L1 human at 100) fails with M_FORBIDDEN. When that happens and + // selfToken (the target users own access token) is non-empty, the write + // is retried with selfToken: the senders own entry is exempt from the + // strict-greater rule, making a self-demotion always authorized. + EnsureRoomPowerLevel(ctx context.Context, roomID, userID string, level int, actorToken, selfToken string) error // ForceLeaveRoom removes a user whose room power level prevents a normal // admin kick. ForceLeaveRoom(ctx context.Context, userID, roomID string) error @@ -244,17 +256,38 @@ type HumanProvisioner interface { JoinRoomAs(ctx context.Context, roomID, userToken string) error // KickFromRoom removes userID from roomID using the admin token. - // Idempotent: returns nil when the user is not a member. + // Idempotent: returns nil when the user is not a member. An + // authorization rejection (kicker not a member, or the target's power + // level not strictly below the kickers) is returned as an error — + // callers must fall back (KickFromRoomAs with an authorized actor, + // LeaveRoomAs with the target's own token, or ForceLeaveRoom). KickFromRoom(ctx context.Context, roomID, userID, reason string) error + // KickFromRoomAs is KickFromRoom with an explicit kicker: actorToken + // ("" = homeserver-admin identity). Same idempotency and error + // semantics as KickFromRoom. + KickFromRoomAs(ctx context.Context, roomID, userID, reason, actorToken string) error + + // LeaveRoomAs makes the user identified by userToken leave roomID + // (self-leave). Always authorized for a joined/invited member + // (spec room-auth rule: a user may always leave their own room), + // which is why it is the fallback when an equal-power kick is + // rejected. Idempotent: returns nil when the user is not a member. + LeaveRoomAs(ctx context.Context, roomID, userToken string) error + // EnsureRoomPowerLevel reconciles userID's entry in the room's // m.room.power_levels to exactly the given power level (raising or - // lowering it), using the admin token. The complete existing content is + // lowering it). actorToken ("" = homeserver-admin identity) performs + // the read and the write; on an M_FORBIDDEN rejection (the homeserver + // refuses to let a sender change another users level unless the senders + // level is strictly greater — an equal-level demotion) the write is + // retried with selfToken (the target users own token), whose own + // entries are exempt from that rule. The complete existing content is // preserved (every other user, every non-user field); only the target // users entry is mutated. The call is a no-op write when the user - // already has exactly that level. Rooms that never had power_levels set - // (legacy) are treated as starting from an empty users map. - EnsureRoomPowerLevel(ctx context.Context, roomID, userID string, level int) error + // already has exactly that level. Rooms that never had power_levels + // set (legacy) are treated as starting from an empty users map. + EnsureRoomPowerLevel(ctx context.Context, roomID, userID string, level int, actorToken, selfToken string) error // ForceLeaveRoom asks the Tuwunel admin bot to force-leave userID out // of roomID via "!admin users force-leave-room". Fire-and-forget at diff --git a/agentteams-controller/internal/service/provisioner.go b/agentteams-controller/internal/service/provisioner.go index 594494283..2446b7457 100644 --- a/agentteams-controller/internal/service/provisioner.go +++ b/agentteams-controller/internal/service/provisioner.go @@ -1075,15 +1075,27 @@ func (p *Provisioner) EnsureRoomNonMember(ctx context.Context, roomID, userID, r } // EnsureRoomPowerLevel reconciles userID's entry in the room's -// m.room.power_levels to EXACTLY `level` (raising or lowering it) via the -// admin token. The complete existing content is preserved — every other -// user and every non-user field (events, invite, notifications, +// m.room.power_levels to EXACTLY `level` (raising or lowering it). The +// read and the write use actorToken when non-empty, otherwise the +// homeserver-admin identity — state access is membership-scoped, so rooms +// the admin is not in (TeamAdmin-owned rooms) must be passed a token of an +// authorized member. The complete existing content is preserved — every +// other user and every non-user field (events, invite, notifications, // users_default, state_default, ban, kick, redact, extension fields); only // the target users entry is mutated. Idempotent: no write when the user // already has exactly `level`. Rooms that never had power_levels set // (legacy rooms) start from an empty users map. -func (p *Provisioner) EnsureRoomPowerLevel(ctx context.Context, roomID, userID string, level int) error { - cur, err := p.matrix.GetRoomState(ctx, roomID, "m.room.power_levels", "") +// +// Equal-level demotion: the homeserver refuses to let a sender change +// ANOTHER user's power level unless the sender's level is strictly +// greater than the target's current level (spec room-auth rule 9.6), so +// an admin at 100 cannot demote a former L1 human who sits at 100. The +// sender's OWN entry is exempt from that rule, so on an M_FORBIDDEN +// rejection the write is retried with selfToken (the target user's own +// access token) — a self-demotion is always authorized when the new level +// does not exceed the target's current one. +func (p *Provisioner) EnsureRoomPowerLevel(ctx context.Context, roomID, userID string, level int, actorToken, selfToken string) error { + cur, err := p.matrix.GetRoomState(ctx, roomID, "m.room.power_levels", "", actorToken) if err != nil { return fmt.Errorf("read power levels %s: %w", roomID, err) } @@ -1110,7 +1122,22 @@ func (p *Provisioner) EnsureRoomPowerLevel(ctx context.Context, roomID, userID s } users[userID] = float64(level) content["users"] = users - return p.matrix.SetRoomState(ctx, roomID, "m.room.power_levels", "", content, "") + if err := p.matrix.SetRoomState(ctx, roomID, "m.room.power_levels", "", content, actorToken); err != nil { + // M_FORBIDDEN on a power-level write to another user means the + // homeserver's strict-greater rule rejected the sender (typically + // an equal-level demotion). Retry with the target's own token: + // their own entry is exempt from that rule. + if matrix.IsForbidden(err) && selfToken != "" { + if selfErr := p.matrix.SetRoomState(ctx, roomID, "m.room.power_levels", "", content, selfToken); selfErr != nil { + return fmt.Errorf("write power levels %s: actor write rejected (%w); self-write also failed: %w", roomID, err, selfErr) + } + log.FromContext(ctx).Info("equal-level power demotion completed via self-write", + "room", roomID, "user", userID, "level", level) + return nil + } + return fmt.Errorf("write power levels %s: %w", roomID, err) + } + return nil } // ReconcileRoomMembership drives the membership of roomID to match `desired` diff --git a/agentteams-controller/internal/service/provisioner_human.go b/agentteams-controller/internal/service/provisioner_human.go index 256716467..17d9cb865 100644 --- a/agentteams-controller/internal/service/provisioner_human.go +++ b/agentteams-controller/internal/service/provisioner_human.go @@ -183,6 +183,28 @@ func (p *Provisioner) KickFromRoom(ctx context.Context, roomID, userID, reason s return p.matrix.KickFromRoom(ctx, roomID, userID, reason) } +// KickFromRoomAs is KickFromRoom with an explicit kicker actor ("" = +// homeserver-admin identity). Authorization failures (kicker not a member +// of the room, or the target's power level not strictly below the kicker's) +// are returned as errors for the caller to fall back on. +func (p *Provisioner) KickFromRoomAs(ctx context.Context, roomID, userID, reason, actorToken string) error { + if actorToken == "" { + return p.matrix.KickFromRoom(ctx, roomID, userID, reason) + } + return p.matrix.KickFromRoomWithToken(ctx, roomID, userID, reason, actorToken) +} + +// LeaveRoomAs makes the user identified by userToken leave roomID +// (self-leave). Always authorized for a joined/invited member, which makes +// it the fallback when an equal-power kick is rejected by the homeserver. +// Idempotent: nil when the user is not a member. +func (p *Provisioner) LeaveRoomAs(ctx context.Context, roomID, userToken string) error { + if userToken == "" { + return fmt.Errorf("leave room %s: empty user token", roomID) + } + return p.matrix.LeaveRoom(ctx, roomID, userToken) +} + // ForceLeaveRoom asks the Tuwunel admin bot to force-leave userID out of // roomID. Used by the Human delete flow where the controller no longer // holds a valid user token (password may be stale) and must rely on the diff --git a/agentteams-controller/internal/service/provisioner_power_test.go b/agentteams-controller/internal/service/provisioner_power_test.go index ebb952acb..30afe8136 100644 --- a/agentteams-controller/internal/service/provisioner_power_test.go +++ b/agentteams-controller/internal/service/provisioner_power_test.go @@ -4,6 +4,8 @@ import ( "context" "errors" "testing" + + "github.com/agentscope-ai/AgentTeams/agentteams-controller/internal/matrix" ) func TestEnsureRoomPowerLevel_LegacyRoomGrantsLevel(t *testing.T) { @@ -14,7 +16,7 @@ func TestEnsureRoomPowerLevel_LegacyRoomGrantsLevel(t *testing.T) { OSSAdmin: &fakeStorageAdmin{}, }) - if err := p.EnsureRoomPowerLevel(context.Background(), "!r:hs", "@alice:hs", 100); err != nil { + if err := p.EnsureRoomPowerLevel(context.Background(), "!r:hs", "@alice:hs", 100, "", ""); err != nil { t.Fatalf("EnsureRoomPowerLevel: %v", err) } calls := fake.roomStates @@ -52,7 +54,7 @@ func TestEnsureRoomPowerLevel_MergesExistingUsers(t *testing.T) { OSSAdmin: &fakeStorageAdmin{}, }) - if err := p.EnsureRoomPowerLevel(context.Background(), "!r:hs", "@alice:hs", 50); err != nil { + if err := p.EnsureRoomPowerLevel(context.Background(), "!r:hs", "@alice:hs", 50, "", ""); err != nil { t.Fatalf("EnsureRoomPowerLevel: %v", err) } users, ok := fake.powerStates["!r:hs"]["users"].(map[string]interface{}) @@ -87,6 +89,12 @@ func TestEnsureRoomPowerLevel_MergesExistingUsers(t *testing.T) { // A demoted human must actually be lowered: a user sitting at 100 whose // permissionLevel drops from 1 to 2 is written at 50, not kept at 100. +// The admin (creator, 100) CANNOT make this write directly — spec v8 +// rule 9.6 rejects changing another user whose current level (100) is not +// strictly below the sender's (100). The provisioner must therefore fall +// back to the human's OWN token, whose own entry is exempt from 9.6. The +// authorization-aware fake enforces exactly that, so this test proves the +// fallback path rather than trusting a permissive double. func TestEnsureRoomPowerLevel_DemotionRevokesLevel(t *testing.T) { existing := map[string]interface{}{ "users": map[string]interface{}{ @@ -99,17 +107,25 @@ func TestEnsureRoomPowerLevel_DemotionRevokesLevel(t *testing.T) { } fake := newFakeTeamMatrix() fake.powerStates = map[string]map[string]interface{}{"!r:hs": existing} + fake.members["!r:hs"] = []matrix.RoomMember{{UserID: "@alice:hs", Membership: "join"}} + fake.tokenUsers = map[string]string{"alice-token": "@alice:hs"} p := NewProvisioner(ProvisionerConfig{ Matrix: fake, Creds: fakeCredentialStore{}, OSSAdmin: &fakeStorageAdmin{}, }) - if err := p.EnsureRoomPowerLevel(context.Background(), "!r:hs", "@alice:hs", 50); err != nil { + if err := p.EnsureRoomPowerLevel(context.Background(), "!r:hs", "@alice:hs", 50, "", "alice-token"); err != nil { t.Fatalf("EnsureRoomPowerLevel: %v", err) } - if len(fake.roomStates) != 1 { - t.Fatalf("demotion must write, got %d writes", len(fake.roomStates)) + if len(fake.roomStates) != 2 { + t.Fatalf("expected actor attempt + self-write, got %d attempts: %+v", len(fake.roomStates), fake.roomStates) + } + if fake.roomStates[0].token != "" { + t.Errorf("first attempt should run as the default admin actor, got token %q", fake.roomStates[0].token) + } + if fake.roomStates[1].token != "alice-token" { + t.Errorf("self-write must use the human's own token, got %q", fake.roomStates[1].token) } users, ok := fake.powerStates["!r:hs"]["users"].(map[string]interface{}) if !ok { @@ -123,6 +139,149 @@ func TestEnsureRoomPowerLevel_DemotionRevokesLevel(t *testing.T) { } } +// Without the human's own token there is NO authorized demotion path for +// an equal-level user: the enforced 9.6 rejects the admin's write and the +// call must surface that error (no silent success, no state change) so +// the reconcile retries next cycle. +func TestEnsureRoomPowerLevel_EqualLevelDemotionWithoutSelfTokenFails(t *testing.T) { + existing := map[string]interface{}{ + "users": map[string]interface{}{ + "@manager:hs": 100.0, + "@alice:hs": 100.0, + }, + } + fake := newFakeTeamMatrix() + fake.powerStates = map[string]map[string]interface{}{"!r:hs": existing} + p := NewProvisioner(ProvisionerConfig{ + Matrix: fake, + Creds: fakeCredentialStore{}, + OSSAdmin: &fakeStorageAdmin{}, + }) + + err := p.EnsureRoomPowerLevel(context.Background(), "!r:hs", "@alice:hs", 50, "", "") + if err == nil { + t.Fatal("equal-level demotion without a self token must fail (spec 9.6)") + } + if !matrix.IsForbidden(err) { + t.Fatalf("expected M_FORBIDDEN from the homeserver rule, got: %v", err) + } + users := fake.powerStates["!r:hs"]["users"].(map[string]interface{}) + if users["@alice:hs"] != 100.0 { + t.Errorf("state must be unchanged after a rejected demotion, alice=%v", users["@alice:hs"]) + } +} + +// A simple grant (0 -> 50) is an ordinary actor write: the target's +// current level (0) is below the actor's (100), so no self-write is +// needed and exactly one attempt is made. +func TestEnsureRoomPowerLevel_SimpleGrantIsSingleActorWrite(t *testing.T) { + existing := map[string]interface{}{ + "users": map[string]interface{}{"@alice:hs": 0.0}, + } + fake := newFakeTeamMatrix() + fake.powerStates = map[string]map[string]interface{}{"!r:hs": existing} + p := NewProvisioner(ProvisionerConfig{ + Matrix: fake, + Creds: fakeCredentialStore{}, + OSSAdmin: &fakeStorageAdmin{}, + }) + + if err := p.EnsureRoomPowerLevel(context.Background(), "!r:hs", "@alice:hs", 50, "", ""); err != nil { + t.Fatalf("EnsureRoomPowerLevel: %v", err) + } + if len(fake.roomStates) != 1 { + t.Fatalf("expected exactly one write attempt, got %d", len(fake.roomStates)) + } + if fake.roomStates[0].token != "" { + t.Errorf("simple grant should run as the default admin actor, got token %q", fake.roomStates[0].token) + } + users := fake.powerStates["!r:hs"]["users"].(map[string]interface{}) + if users["@alice:hs"] != 50.0 { + t.Errorf("alice=%v, want 50", users["@alice:hs"]) + } +} + +// TeamAdmin-owned room (P1-2): the homeserver admin is deliberately NOT a +// member, so the default admin identity cannot even READ the room state — +// let alone write it. The grant must run with a token of an authorized +// member (here the team admin, the room's creator). +func TestEnsureRoomPowerLevel_TeamAdminOwnedRoom(t *testing.T) { + existing := map[string]interface{}{ + "users": map[string]interface{}{ + "@teamadmin:hs": 100.0, + "@manager:hs": 100.0, + "@alice:hs": 0.0, + }, + } + room := "!team-owned:hs" + fake := newFakeTeamMatrix() + fake.powerStates = map[string]map[string]interface{}{room: existing} + fake.roomCreators = map[string]string{room: "@teamadmin:hs"} + fake.adminIsMember = map[string]bool{} // admin is a member of NO room + fake.tokenUsers = map[string]string{"teamadmin-token": "@teamadmin:hs"} + p := NewProvisioner(ProvisionerConfig{ + Matrix: fake, + Creds: fakeCredentialStore{}, + OSSAdmin: &fakeStorageAdmin{}, + }) + + // The default admin identity is rejected on the READ (non-member). + if _, err := fake.GetRoomState(context.Background(), room, "m.room.power_levels", "", ""); !matrix.IsForbidden(err) { + t.Fatalf("admin read of a TeamAdmin-owned room must be M_FORBIDDEN, got %v", err) + } + + // The team-admin actor can read AND write the grant. + if err := p.EnsureRoomPowerLevel(context.Background(), room, "@alice:hs", 50, "teamadmin-token", ""); err != nil { + t.Fatalf("EnsureRoomPowerLevel as team admin: %v", err) + } + users := fake.powerStates[room]["users"].(map[string]interface{}) + if users["@alice:hs"] != 50.0 { + t.Errorf("alice=%v, want 50", users["@alice:hs"]) + } + if users["@teamadmin:hs"] != 100.0 || users["@manager:hs"] != 100.0 { + t.Errorf("other users disturbed: %v", users) + } +} + +// A team-admin actor in a room where the homeserver admin is not a member +// must still be able to demote an equal-level human — the actor (100) +// hits the same 9.6 wall as the admin would, so the self fallback runs as +// the human. +func TestEnsureRoomPowerLevel_TeamAdminRoomEqualLevelDemotion(t *testing.T) { + existing := map[string]interface{}{ + "users": map[string]interface{}{ + "@teamadmin:hs": 100.0, + "@alice:hs": 100.0, // former L1 human, now demoted + }, + } + room := "!team-owned:hs" + fake := newFakeTeamMatrix() + fake.powerStates = map[string]map[string]interface{}{room: existing} + fake.roomCreators = map[string]string{room: "@teamadmin:hs"} + fake.adminIsMember = map[string]bool{} + fake.members[room] = []matrix.RoomMember{{UserID: "@alice:hs", Membership: "join"}} + fake.tokenUsers = map[string]string{ + "teamadmin-token": "@teamadmin:hs", + "alice-token": "@alice:hs", + } + p := NewProvisioner(ProvisionerConfig{ + Matrix: fake, + Creds: fakeCredentialStore{}, + OSSAdmin: &fakeStorageAdmin{}, + }) + + if err := p.EnsureRoomPowerLevel(context.Background(), room, "@alice:hs", 50, "teamadmin-token", "alice-token"); err != nil { + t.Fatalf("EnsureRoomPowerLevel (team-admin actor + self fallback): %v", err) + } + if len(fake.roomStates) != 2 { + t.Fatalf("expected actor attempt + self-write, got %d attempts", len(fake.roomStates)) + } + users := fake.powerStates[room]["users"].(map[string]interface{}) + if users["@alice:hs"] != 50.0 { + t.Errorf("alice=%v, want 50", users["@alice:hs"]) + } +} + func TestEnsureRoomPowerLevel_ExactMatchNoWrite(t *testing.T) { existing := map[string]interface{}{ "users": map[string]interface{}{"@alice:hs": 50.0, "@manager:hs": 100.0}, @@ -135,7 +294,7 @@ func TestEnsureRoomPowerLevel_ExactMatchNoWrite(t *testing.T) { OSSAdmin: &fakeStorageAdmin{}, }) - if err := p.EnsureRoomPowerLevel(context.Background(), "!r:hs", "@alice:hs", 50); err != nil { + if err := p.EnsureRoomPowerLevel(context.Background(), "!r:hs", "@alice:hs", 50, "", ""); err != nil { t.Fatalf("EnsureRoomPowerLevel: %v", err) } if len(fake.roomStates) != 0 { @@ -152,7 +311,7 @@ func TestEnsureRoomPowerLevel_ReadErrorPropagates(t *testing.T) { OSSAdmin: &fakeStorageAdmin{}, }) - err := p.EnsureRoomPowerLevel(context.Background(), "!r:hs", "@alice:hs", 50) + err := p.EnsureRoomPowerLevel(context.Background(), "!r:hs", "@alice:hs", 50, "", "") if err == nil { t.Fatal("expected error, got nil") } @@ -172,10 +331,10 @@ func TestEnsureRoomPowerLevel_SecondGrantPreservesFirst(t *testing.T) { OSSAdmin: &fakeStorageAdmin{}, }) - if err := p.EnsureRoomPowerLevel(context.Background(), "!r:hs", "@alice:hs", 100); err != nil { + if err := p.EnsureRoomPowerLevel(context.Background(), "!r:hs", "@alice:hs", 100, "", ""); err != nil { t.Fatalf("first grant: %v", err) } - if err := p.EnsureRoomPowerLevel(context.Background(), "!r:hs", "@bob:hs", 50); err != nil { + if err := p.EnsureRoomPowerLevel(context.Background(), "!r:hs", "@bob:hs", 50, "", ""); err != nil { t.Fatalf("second grant: %v", err) } stored := fake.powerStates["!r:hs"] @@ -201,7 +360,7 @@ func TestEnsureRoomPowerLevel_StateWithoutUsersMap(t *testing.T) { OSSAdmin: &fakeStorageAdmin{}, }) - if err := p.EnsureRoomPowerLevel(context.Background(), "!r:hs", "@alice:hs", 50); err != nil { + if err := p.EnsureRoomPowerLevel(context.Background(), "!r:hs", "@alice:hs", 50, "", ""); err != nil { t.Fatalf("EnsureRoomPowerLevel: %v", err) } stored := fake.powerStates["!r:hs"] diff --git a/agentteams-controller/internal/service/provisioner_team_test.go b/agentteams-controller/internal/service/provisioner_team_test.go index df72e5624..52c9f9d51 100644 --- a/agentteams-controller/internal/service/provisioner_team_test.go +++ b/agentteams-controller/internal/service/provisioner_team_test.go @@ -6,6 +6,7 @@ import ( "errors" "reflect" "sort" + "strconv" "strings" "testing" "time" @@ -37,6 +38,24 @@ type fakeTeamMatrix struct { // reports (nil, nil) — the legacy "state never set" case. powerStates map[string]map[string]interface{} powerStateErr error + + // --- Matrix room-auth model (spec v8 rules) — the fake enforces + // authorization on power-level writes and kicks so that passing + // tests prove the controller survives a real homeserver's + // strict-greater rule, not just its own JSON round-trip. + + // adminUser is the user ID behind the "" (homeserver-admin) token. + adminUser string + // roomCreators maps roomID -> creator user ID. A creator that is not + // explicitly listed in the users map still holds the implicit level + // 100 (room versions 1-11); rooms without an entry are created by + // the admin. + roomCreators map[string]string + // adminIsMember, when non-nil, makes the admin's membership explicit + // per room (absent room = NOT a member) — the TeamAdmin-owned-room + // case, where the homeserver admin is deliberately excluded. When + // nil, the admin is a member of every room (legacy default). + adminIsMember map[string]bool } type roomUserCall struct { @@ -64,6 +83,7 @@ func newFakeTeamMatrix() *fakeTeamMatrix { listErrs: make(map[string]error), tokenUsers: make(map[string]string), created: true, + adminUser: "@admin:localhost", } } @@ -91,6 +111,18 @@ func (f *fakeTeamMatrix) CreateRoom(_ context.Context, req matrix.CreateRoomRequ if req.CreatorToken == "" && f.created { f.members[roomID] = []matrix.RoomMember{{UserID: "@admin:localhost", Membership: "join"}} } + if f.created && len(req.PowerLevels) > 0 { + // Wire-faithful: the homeserver records the creator-time power + // level overrides as the room's m.room.power_levels state. + if f.powerStates == nil { + f.powerStates = map[string]map[string]interface{}{} + } + users := map[string]interface{}{} + for uid, lvl := range req.PowerLevels { + users[uid] = float64(lvl) + } + f.powerStates[roomID] = map[string]interface{}{"users": users} + } if f.created { for _, userID := range req.Invite { f.members[roomID] = append(f.members[roomID], matrix.RoomMember{UserID: userID, Membership: "invite"}) @@ -125,6 +157,11 @@ func (f *fakeTeamMatrix) SetRoomState(_ context.Context, roomID, eventType, stat token: token, }) if eventType == "m.room.power_levels" { + // Authorization-aware: a rejected write stores nothing, exactly + // like a real homeserver would refuse the event. + if err := f.authorizePowerLevelWrite(roomID, token, content); err != nil { + return err + } // JSON round-trip so stored state matches what the homeserver // would return (numbers as float64 in map[string]interface{}). data, _ := json.Marshal(content) @@ -140,10 +177,18 @@ func (f *fakeTeamMatrix) SetRoomState(_ context.Context, roomID, eventType, stat return nil } -func (f *fakeTeamMatrix) GetRoomState(_ context.Context, roomID, eventType, _ string) (map[string]interface{}, error) { +func (f *fakeTeamMatrix) GetRoomState(_ context.Context, roomID, eventType, _ string, token string) (map[string]interface{}, error) { if eventType != "m.room.power_levels" { return nil, nil } + // State reads are membership-scoped: a non-member reader (the + // homeserver admin in a TeamAdmin-owned room) is rejected, exactly + // like the real endpoint. + if sender := f.senderFor(token); sender == "" { + return nil, fakeUnknownToken() + } else if !f.isMember(roomID, sender) { + return nil, fakeForbidden("not a member of room " + roomID) + } if f.powerStateErr != nil { return nil, f.powerStateErr } @@ -154,7 +199,19 @@ func (f *fakeTeamMatrix) GetRoomState(_ context.Context, roomID, eventType, _ st if !ok { return nil, nil } - return st, nil + // Wire-faithful round-trip: the real endpoint returns a fresh JSON + // decode, never a shared reference. Returning the stored map directly + // would let a caller's mutation leak into the "server" state before + // the write is authorized. + data, err := json.Marshal(st) + if err != nil { + return nil, err + } + var out map[string]interface{} + if err := json.Unmarshal(data, &out); err != nil { + return nil, err + } + return out, nil } func (f *fakeTeamMatrix) JoinRoom(_ context.Context, roomID, token string) error { @@ -167,6 +224,14 @@ func (f *fakeTeamMatrix) JoinRoom(_ context.Context, roomID, token string) error func (f *fakeTeamMatrix) LeaveRoom(_ context.Context, roomID, token string) error { f.leaves = append(f.leaves, roomID) + uid := f.senderFor(token) + if uid == "" { + return fakeUnknownToken() + } + if !f.hasMembership(roomID, uid) { + return nil // already out — idempotent (client maps 404 -> nil) + } + f.removeMember(roomID, uid) return nil } @@ -200,12 +265,18 @@ func (f *fakeTeamMatrix) ListRoomMembersWithToken(_ context.Context, roomID, _ s } func (f *fakeTeamMatrix) InviteToRoom(_ context.Context, roomID, userID string) error { + if err := f.authorizeInvite(roomID, "", userID); err != nil { + return err + } f.members[roomID] = append(f.members[roomID], matrix.RoomMember{UserID: userID, Membership: "invite"}) return nil } -func (f *fakeTeamMatrix) InviteToRoomWithToken(_ context.Context, roomID, userID, _ string) error { +func (f *fakeTeamMatrix) InviteToRoomWithToken(_ context.Context, roomID, userID, token string) error { f.tokenInvites = append(f.tokenInvites, roomUserCall{roomID: roomID, userID: userID}) + if err := f.authorizeInvite(roomID, token, userID); err != nil { + return err + } f.members[roomID] = append(f.members[roomID], matrix.RoomMember{UserID: userID, Membership: "invite"}) return nil } @@ -215,28 +286,30 @@ func (f *fakeTeamMatrix) KickFromRoom(_ context.Context, roomID, userID, _ strin if f.kickErr != nil { return f.kickErr } - next := f.members[roomID][:0] - for _, member := range f.members[roomID] { - if member.UserID != userID { - next = append(next, member) - } - } - f.members[roomID] = next - return nil + return f.enforceKick(roomID, "", userID) } -func (f *fakeTeamMatrix) KickFromRoomWithToken(_ context.Context, roomID, userID, _ string, _ string) error { +func (f *fakeTeamMatrix) KickFromRoomWithToken(_ context.Context, roomID, userID, _ string, token string) error { f.tokenKicks = append(f.tokenKicks, roomUserCall{roomID: roomID, userID: userID}) if f.kickErr != nil { return f.kickErr } - next := f.members[roomID][:0] - for _, member := range f.members[roomID] { - if member.UserID != userID { - next = append(next, member) + return f.enforceKick(roomID, token, userID) +} + +// enforceKick runs the spec room-auth rule 4.5.4 for a kick and performs +// it when authorized: the kicker must be a member with at least the kick +// level (default 50) and the target's level must be STRICTLY below the +// kicker's. A missing target maps to nil (the real client treats the 404 +// as idempotent success). +func (f *fakeTeamMatrix) enforceKick(roomID, token, userID string) error { + if err := f.authorizeKick(roomID, token, userID); err != nil { + if apiErr, ok := err.(*matrix.APIError); ok && apiErr.StatusCode == 404 { + return nil } + return err } - f.members[roomID] = next + f.removeMember(roomID, userID) return nil } @@ -543,6 +616,7 @@ func (f *fakeTeamMatrix) Whoami(_ context.Context, _ string) (string, error) { func TestProvisionTeamRoomsInvitesExplicitTeamAdminAndLeavesNewLeaderDM(t *testing.T) { matrixClient := newFakeTeamMatrix() + matrixClient.tokenUsers["team-admin-token"] = "@alice:example.com" p := NewProvisioner(ProvisionerConfig{ Matrix: matrixClient, AdminUser: "admin", @@ -638,6 +712,7 @@ func TestProvisionTeamRoomsInvitesExplicitTeamAdminAndLeavesNewLeaderDM(t *testi func TestProvisionTeamRoomsInvitesCoordinatorMembersLikeTeamAdmin(t *testing.T) { matrixClient := newFakeTeamMatrix() + matrixClient.tokenUsers["team-admin-token"] = "@alice:example.com" p := NewProvisioner(ProvisionerConfig{ Matrix: matrixClient, AdminUser: "admin", @@ -748,6 +823,7 @@ func TestProvisionTeamRoomsSkipsNewFallbackLeaderDMReconcileWithoutJoinedActor(t func TestProvisionTeamRoomsDerivesTeamAdminMatrixIDFromName(t *testing.T) { matrixClient := newFakeTeamMatrix() + matrixClient.tokenUsers["team-admin-token"] = "@alice:localhost" p := NewProvisioner(ProvisionerConfig{ Matrix: matrixClient, AdminUser: "admin", @@ -776,6 +852,10 @@ func TestProvisionTeamRoomsDerivesTeamAdminMatrixIDFromName(t *testing.T) { func TestProvisionTeamRoomsDoesNotLeaveExistingLeaderDM(t *testing.T) { matrixClient := newFakeTeamMatrix() + matrixClient.tokenUsers["team-admin-token"] = "@alice:localhost" + // Existing rooms: alice (the team admin) created and owns both. + matrixClient.seedOwnedRoom("!team:localhost", "@alice:localhost") + matrixClient.seedOwnedRoom("!leader-dm:localhost", "@alice:localhost") matrixClient.created = false p := NewProvisioner(ProvisionerConfig{ Matrix: matrixClient, @@ -799,6 +879,14 @@ func TestProvisionTeamRoomsDoesNotLeaveExistingLeaderDM(t *testing.T) { func TestProvisionTeamRoomsLeaderJoinsExistingFallbackLeaderDMBeforeReconcile(t *testing.T) { matrixClient := newFakeTeamMatrix() + // Existing leader DM was created with the fallback power levels. + matrixClient.powerStates = map[string]map[string]interface{}{ + "!leader-dm:localhost": {"users": map[string]interface{}{ + "@lead:localhost": 100.0, + "@admin:localhost": 100.0, + "@manager:localhost": 100.0, + }}, + } matrixClient.created = false matrixClient.tokenUsers["leader-token"] = "@lead:localhost" p := NewProvisioner(ProvisionerConfig{ @@ -846,6 +934,10 @@ func TestProvisionTeamRoomsRequiresTeamAdminActorToken(t *testing.T) { func TestProvisionTeamRoomsUsesTeamAdminTokenForExistingTeamRoom(t *testing.T) { matrixClient := newFakeTeamMatrix() + matrixClient.tokenUsers["team-admin-token"] = "@alice:localhost" + // Existing rooms: alice (the team admin) created and owns both. + matrixClient.seedOwnedRoom("!team:localhost", "@alice:localhost") + matrixClient.seedOwnedRoom("!leader-dm:localhost", "@alice:localhost") matrixClient.created = false p := NewProvisioner(ProvisionerConfig{ Matrix: matrixClient, @@ -864,12 +956,13 @@ func TestProvisionTeamRoomsUsesTeamAdminTokenForExistingTeamRoom(t *testing.T) { if err != nil { t.Fatalf("ProvisionTeamRooms: %v", err) } + // alice already owns (is a member of) both rooms, so the reconcile + // only invites the MISSING members — and does so with the team-admin + // token, never an admin kick. wantInvites := []roomUserCall{ - {roomID: "!team:localhost", userID: "@alice:localhost"}, {roomID: "!team:localhost", userID: "@lead:localhost"}, {roomID: "!team:localhost", userID: "@dev:localhost"}, {roomID: "!leader-dm:localhost", userID: "@lead:localhost"}, - {roomID: "!leader-dm:localhost", userID: "@alice:localhost"}, } if got := matrixClient.tokenInvites; !reflect.DeepEqual(got, wantInvites) { t.Fatalf("team room token invites=%v, want %v", got, wantInvites) @@ -913,3 +1006,312 @@ func requireRoomState(t *testing.T, matrixClient *fakeTeamMatrix, roomID string) t.Fatalf("room.meta state for %s not found in %+v", roomID, matrixClient.roomStates) return roomStateCall{} } + +// --- Authorization-aware fake helpers (spec v8 room-auth rules) --- + +func fakeForbidden(msg string) *matrix.APIError { + return &matrix.APIError{StatusCode: 403, ErrCode: "M_FORBIDDEN", Message: msg} +} + +func fakeUnknownToken() *matrix.APIError { + return &matrix.APIError{StatusCode: 401, ErrCode: "M_UNKNOWN_TOKEN", Message: "unknown token"} +} + +// senderFor resolves the Matrix user ID behind a client token ("" = the +// homeserver admin). Returns "" for unknown tokens. +func (f *fakeTeamMatrix) senderFor(token string) string { + if token == "" { + return f.adminUser + } + return f.tokenUsers[token] +} + +// isMember reports whether uid is a joined/invited member of roomID. The +// homeserver admin is a member of every room by default; when +// adminIsMember is set, its membership becomes explicit per room (absent +// room = not a member) — the TeamAdmin-owned-room case. +func (f *fakeTeamMatrix) isMember(roomID, uid string) bool { + if uid == "" { + return false + } + // The creator is always a joined member of their room. + if uid == f.creatorIn(roomID) { + return true + } + if uid == f.adminUser { + if f.adminIsMember != nil { + return f.adminIsMember[roomID] + } + return true + } + return f.hasMembership(roomID, uid) +} + +func (f *fakeTeamMatrix) hasMembership(roomID, uid string) bool { + for _, m := range f.members[roomID] { + if m.UserID == uid && (m.Membership == "join" || m.Membership == "invite") { + return true + } + } + return false +} + +func (f *fakeTeamMatrix) removeMember(roomID, uid string) { + next := f.members[roomID][:0] + for _, m := range f.members[roomID] { + if m.UserID != uid { + next = append(next, m) + } + } + f.members[roomID] = next +} + +// creatorIn returns the room's creator — an implicit level 100 in room +// versions 1-11 when not explicitly listed in the users map. +func (f *fakeTeamMatrix) creatorIn(roomID string) string { + if c := f.roomCreators[roomID]; c != "" { + return c + } + return f.adminUser +} + +// levelIn resolves a user's current power level: explicit users-map +// entry, else the implicit creator level 100, else users_default, else 0. +func (f *fakeTeamMatrix) levelIn(roomID, uid string) int { + if st, ok := f.powerStates[roomID]; ok { + if users, ok := st["users"].(map[string]interface{}); ok { + if v, ok := users[uid]; ok { + if n, ok := v.(float64); ok { + return int(n) + } + } + } + } + if uid == f.creatorIn(roomID) { + return 100 + } + if st, ok := f.powerStates[roomID]; ok { + if d, ok := st["users_default"].(float64); ok { + return int(d) + } + } + return 0 +} + +// authorizePowerLevelWrite enforces the spec v8 room-auth rule 9 for an +// m.room.power_levels write by the sender behind token: +// - the sender must be a member with at least the event's required +// level (default 100); +// - 9.3: scalar fields may not move above the sender's level; +// - 9.4/9.5: events/notifications entries likewise; +// - 9.6: no users entry OTHER than the sender's own may be changed or +// removed unless the sender's level is strictly greater than the +// entry's current level — an equal-level demotion (admin 100 vs +// human 100) is rejected: this is the deadlock the controller's +// self-demotion fallback exists for; +// - 9.7: any users entry may not be raised above the sender's level. +func (f *fakeTeamMatrix) authorizePowerLevelWrite(roomID, token string, content map[string]interface{}) error { + sender := f.senderFor(token) + if sender == "" { + return fakeUnknownToken() + } + if !f.isMember(roomID, sender) { + return fakeForbidden("not a member of room " + roomID) + } + prev := f.powerStates[roomID] + senderLevel := f.levelIn(roomID, sender) + + required := 100 + if prev != nil { + if ev, ok := prev["events"].(map[string]interface{}); ok { + if v, ok := ev["m.room.power_levels"].(float64); ok { + required = int(v) + } + } + } + if senderLevel < required { + return fakeForbidden("insufficient power level to send m.room.power_levels") + } + + var curUsers, newUsers map[string]interface{} + if prev != nil { + curUsers, _ = prev["users"].(map[string]interface{}) + } + newUsers, _ = content["users"].(map[string]interface{}) + + // Rule 9.3 — scalar fields. + for _, field := range []string{"users_default", "events_default", "state_default", "ban", "redact", "kick", "invite"} { + _, present := content[field] + if !present { + continue + } + cur := 0 + if prev != nil { + if v, ok := prev[field].(float64); ok { + cur = int(v) + } + } + if cur > senderLevel { + return fakeForbidden("cannot alter " + field + " above the sender's level") + } + if nv, ok := content[field].(float64); ok && int(nv) > senderLevel { + return fakeForbidden("cannot raise " + field + " above the sender's level") + } + } + + // Rule 9.4/9.5 — events / notifications entries. + for _, field := range []string{"events", "notifications"} { + var cur, nw map[string]interface{} + if prev != nil { + cur, _ = prev[field].(map[string]interface{}) + } + nw, _ = content[field].(map[string]interface{}) + for k, v := range nw { + nv, ok := v.(float64) + if !ok { + continue + } + cv, existed := cur[k] + cvi := 0 + if cf, ok := cv.(float64); ok { + cvi = int(cf) + } + if existed && cvi > senderLevel { + return fakeForbidden("cannot alter " + field + " " + k + " above the sender's level") + } + if int(nv) > senderLevel { + return fakeForbidden("cannot set " + field + " " + k + " above the sender's level") + } + } + for k, v := range cur { + if _, kept := nw[k]; kept { + continue + } + if cf, ok := v.(float64); ok && int(cf) > senderLevel { + return fakeForbidden("cannot remove " + field + " " + k + " above the sender's level") + } + } + } + + // Rule 9.6 — other users' entries: strict-greater on the CURRENT + // level (equal is rejected). The sender's own entry is exempt. + for k, cv := range curUsers { + cvi, ok := cv.(float64) + if !ok { + continue + } + nv, kept := newUsers[k] + nvi := 0 + if cf, ok := nv.(float64); ok { + nvi = int(cf) + } + changed := !kept || nvi != int(cvi) + if !changed { + continue + } + if k != sender && int(cvi) >= senderLevel { + return fakeForbidden("cannot change " + k + " from " + strconv.Itoa(int(cvi)) + + " to " + strconv.Itoa(nvi) + ": the target's level is not below the sender's " + + strconv.Itoa(senderLevel)) + } + } + + // Rule 9.7 — no entry (own or other) may be raised above the sender. + for k, v := range newUsers { + nv, ok := v.(float64) + if !ok { + continue + } + cv, existed := curUsers[k] + changedOrAdded := !existed + if !changedOrAdded { + if cf, ok := cv.(float64); ok && int(cf) != int(nv) { + changedOrAdded = true + } + } + if changedOrAdded && int(nv) > senderLevel { + return fakeForbidden("cannot grant " + k + " level " + strconv.Itoa(int(nv)) + + " above the sender's " + strconv.Itoa(senderLevel)) + } + } + return nil +} + +// authorizeKick enforces spec room-auth rule 4.5.4 for a kick: the kicker +// must be a member with at least the kick level (default 50), and the +// target's level must be STRICTLY below the kicker's. A missing target +// yields a 404 (the caller maps it to idempotent success). +func (f *fakeTeamMatrix) authorizeKick(roomID, token, targetID string) error { + kicker := f.senderFor(token) + if kicker == "" { + return fakeUnknownToken() + } + if !f.isMember(roomID, kicker) { + return fakeForbidden("not in room") + } + if !f.hasMembership(roomID, targetID) { + return &matrix.APIError{StatusCode: 404, ErrCode: "M_NOT_FOUND", Message: "not a member of that room"} + } + kickerLevel := f.levelIn(roomID, kicker) + targetLevel := f.levelIn(roomID, targetID) + kickLevel := 50 + if st := f.powerStates[roomID]; st != nil { + if k, ok := st["kick"].(float64); ok { + kickLevel = int(k) + } + } + if kickerLevel < kickLevel { + return fakeForbidden("insufficient power level to kick") + } + if targetLevel >= kickerLevel { + return fakeForbidden("cannot kick " + targetID + ": their power level " + + strconv.Itoa(targetLevel) + " is not below the kicker's " + strconv.Itoa(kickerLevel)) + } + return nil +} + +// authorizeInvite enforces spec room-auth rule 4.4 for an invite: the +// inviter must be a joined member at least at the invite level (default +// 50), and the target must not already be a joined/banned member. +func (f *fakeTeamMatrix) authorizeInvite(roomID, token, targetID string) error { + inviter := f.senderFor(token) + if inviter == "" { + return fakeUnknownToken() + } + joined := false + for _, m := range f.members[roomID] { + if m.UserID == inviter && m.Membership == "join" { + joined = true + break + } + } + if !joined && !f.isMember(roomID, inviter) { + return fakeForbidden("not a member of room " + roomID) + } + inviteLevel := 50 + if st := f.powerStates[roomID]; st != nil { + if k, ok := st["invite"].(float64); ok { + inviteLevel = int(k) + } + } + if f.levelIn(roomID, inviter) < inviteLevel { + return fakeForbidden("insufficient power level to invite") + } + for _, m := range f.members[roomID] { + if m.UserID == targetID && (m.Membership == "join" || m.Membership == "ban") { + return fakeForbidden("target is already a member") + } + } + return nil +} + +// seedOwnedRoom models an EXISTING room that uid created: uid is the +// creator (implicit level 100 in room versions 1-11) and a joined member. +// Used by tests for rooms created before this reconcile ran. +func (f *fakeTeamMatrix) seedOwnedRoom(roomID, uid string) { + if f.roomCreators == nil { + f.roomCreators = map[string]string{} + } + f.roomCreators[roomID] = uid + f.members[roomID] = append(f.members[roomID], matrix.RoomMember{UserID: uid, Membership: "join"}) +} diff --git a/agentteams-controller/test/testutil/mocks/human_provisioner.go b/agentteams-controller/test/testutil/mocks/human_provisioner.go index 4e74b23e3..3541383f3 100644 --- a/agentteams-controller/test/testutil/mocks/human_provisioner.go +++ b/agentteams-controller/test/testutil/mocks/human_provisioner.go @@ -30,7 +30,9 @@ type MockHumanProvisioner struct { InviteToRoomFn func(ctx context.Context, roomID, userID string) error JoinRoomAsFn func(ctx context.Context, roomID, userToken string) error KickFromRoomFn func(ctx context.Context, roomID, userID, reason string) error - EnsureRoomPowerLevelFn func(ctx context.Context, roomID, userID string, level int) error + KickFromRoomAsFn func(ctx context.Context, roomID, userID, reason, actorToken string) error + LeaveRoomAsFn func(ctx context.Context, roomID, userToken string) error + EnsureRoomPowerLevelFn func(ctx context.Context, roomID, userID string, level int, actorToken, selfToken string) error ForceLeaveRoomFn func(ctx context.Context, userID, roomID string) error DeactivateHumanUserFn func(ctx context.Context, userID string) error SetDisplayNameFn func(ctx context.Context, userID, accessToken, displayName string) error @@ -52,6 +54,8 @@ type MockHumanProvisioner struct { InviteToRoom []RoomMembershipCall JoinRoomAs []JoinRoomAsCall KickFromRoom []KickFromRoomCall + KickFromRoomAs []KickFromRoomCall + LeaveRoomAs []JoinRoomAsCall ForceLeaveRoom []ForceLeaveRoomCall DeactivateHumanUser []string EnsureRoomPowerLevel []EnsureRoomPowerLevelCall @@ -64,6 +68,10 @@ type EnsureRoomPowerLevelCall struct { RoomID string UserID string Level int + // ActorToken / SelfToken record which identities the grant ran as + // ("" = homeserver-admin default actor). + ActorToken string + SelfToken string } // LoginAsHumanCall records the (name, password) pair passed to LoginAsHuman. @@ -140,6 +148,8 @@ func (m *MockHumanProvisioner) Reset() { m.InviteToRoomFn = nil m.JoinRoomAsFn = nil m.KickFromRoomFn = nil + m.KickFromRoomAsFn = nil + m.LeaveRoomAsFn = nil m.ForceLeaveRoomFn = nil m.DeactivateHumanUserFn = nil m.SetDisplayNameFn = nil @@ -166,6 +176,8 @@ func (m *MockHumanProvisioner) clearCallsLocked() { InviteToRoom []RoomMembershipCall JoinRoomAs []JoinRoomAsCall KickFromRoom []KickFromRoomCall + KickFromRoomAs []KickFromRoomCall + LeaveRoomAs []JoinRoomAsCall ForceLeaveRoom []ForceLeaveRoomCall DeactivateHumanUser []string EnsureRoomPowerLevel []EnsureRoomPowerLevelCall @@ -340,13 +352,35 @@ func (m *MockHumanProvisioner) DeactivateHumanUser(ctx context.Context, userID s return nil } -func (m *MockHumanProvisioner) EnsureRoomPowerLevel(ctx context.Context, roomID, userID string, level int) error { +func (m *MockHumanProvisioner) EnsureRoomPowerLevel(ctx context.Context, roomID, userID string, level int, actorToken, selfToken string) error { m.mu.Lock() - m.Calls.EnsureRoomPowerLevel = append(m.Calls.EnsureRoomPowerLevel, EnsureRoomPowerLevelCall{RoomID: roomID, UserID: userID, Level: level}) + m.Calls.EnsureRoomPowerLevel = append(m.Calls.EnsureRoomPowerLevel, EnsureRoomPowerLevelCall{RoomID: roomID, UserID: userID, Level: level, ActorToken: actorToken, SelfToken: selfToken}) fn := m.EnsureRoomPowerLevelFn m.mu.Unlock() if fn != nil { - return fn(ctx, roomID, userID, level) + return fn(ctx, roomID, userID, level, actorToken, selfToken) + } + return nil +} + +func (m *MockHumanProvisioner) KickFromRoomAs(ctx context.Context, roomID, userID, reason, actorToken string) error { + m.mu.Lock() + m.Calls.KickFromRoomAs = append(m.Calls.KickFromRoomAs, KickFromRoomCall{RoomID: roomID, UserID: userID, Reason: reason}) + fn := m.KickFromRoomAsFn + m.mu.Unlock() + if fn != nil { + return fn(ctx, roomID, userID, reason, actorToken) + } + return nil +} + +func (m *MockHumanProvisioner) LeaveRoomAs(ctx context.Context, roomID, userToken string) error { + m.mu.Lock() + m.Calls.LeaveRoomAs = append(m.Calls.LeaveRoomAs, JoinRoomAsCall{RoomID: roomID, UserToken: userToken}) + fn := m.LeaveRoomAsFn + m.mu.Unlock() + if fn != nil { + return fn(ctx, roomID, userToken) } return nil } diff --git a/agentteams-controller/test/testutil/mocks/provisioner.go b/agentteams-controller/test/testutil/mocks/provisioner.go index 1bfae080a..4f509d640 100644 --- a/agentteams-controller/test/testutil/mocks/provisioner.go +++ b/agentteams-controller/test/testutil/mocks/provisioner.go @@ -44,7 +44,9 @@ type MockProvisioner struct { InviteToRoomFn func(ctx context.Context, roomID, userID string) error JoinRoomAsFn func(ctx context.Context, roomID, userToken string) error KickFromRoomFn func(ctx context.Context, roomID, userID, reason string) error - EnsureRoomPowerLevelFn func(ctx context.Context, roomID, userID string, level int) error + KickFromRoomAsFn func(ctx context.Context, roomID, userID, reason, actorToken string) error + LeaveRoomAsFn func(ctx context.Context, roomID, userToken string) error + EnsureRoomPowerLevelFn func(ctx context.Context, roomID, userID string, level int, actorToken, selfToken string) error ForceLeaveRoomFn func(ctx context.Context, userID, roomID string) error DeactivateHumanUserFn func(ctx context.Context, userID string) error ProvisionTeamRoomsFn func(ctx context.Context, req service.TeamRoomRequest) (*service.TeamRoomResult, error) @@ -123,9 +125,11 @@ type remoteNamespaceCall struct { } type ensureRoomPowerLevelCall struct { - RoomID string - UserID string - Level int + RoomID string + UserID string + Level int + ActorToken string + SelfToken string } type userPasswordCall struct { @@ -194,6 +198,8 @@ func (m *MockProvisioner) Reset() { m.InviteToRoomFn = nil m.JoinRoomAsFn = nil m.KickFromRoomFn = nil + m.KickFromRoomAsFn = nil + m.LeaveRoomAsFn = nil m.EnsureRoomPowerLevelFn = nil m.ForceLeaveRoomFn = nil m.DeactivateHumanUserFn = nil @@ -627,13 +633,32 @@ func (m *MockProvisioner) KickFromRoom(ctx context.Context, roomID, userID, reas return nil } -func (m *MockProvisioner) EnsureRoomPowerLevel(ctx context.Context, roomID, userID string, level int) error { +func (m *MockProvisioner) KickFromRoomAs(ctx context.Context, roomID, userID, reason, actorToken string) error { + m.mu.Lock() + m.Calls.KickFromRoom = append(m.Calls.KickFromRoom, kickFromRoomCall{RoomID: roomID, UserID: userID, Reason: reason}) + fn := m.KickFromRoomAsFn + m.mu.Unlock() + if fn != nil { + return fn(ctx, roomID, userID, reason, actorToken) + } + return nil +} + +func (m *MockProvisioner) LeaveRoomAs(ctx context.Context, roomID, userToken string) error { + fn := m.LeaveRoomAsFn + if fn != nil { + return fn(ctx, roomID, userToken) + } + return nil +} + +func (m *MockProvisioner) EnsureRoomPowerLevel(ctx context.Context, roomID, userID string, level int, actorToken, selfToken string) error { m.mu.Lock() - m.Calls.EnsureRoomPowerLevel = append(m.Calls.EnsureRoomPowerLevel, ensureRoomPowerLevelCall{RoomID: roomID, UserID: userID, Level: level}) + m.Calls.EnsureRoomPowerLevel = append(m.Calls.EnsureRoomPowerLevel, ensureRoomPowerLevelCall{RoomID: roomID, UserID: userID, Level: level, ActorToken: actorToken, SelfToken: selfToken}) fn := m.EnsureRoomPowerLevelFn m.mu.Unlock() if fn != nil { - return fn(ctx, roomID, userID, level) + return fn(ctx, roomID, userID, level, actorToken, selfToken) } return nil } diff --git a/docs/design/room-power-levels.md b/docs/design/room-power-levels.md index da0a1c283..d70123cd0 100644 --- a/docs/design/room-power-levels.md +++ b/docs/design/room-power-levels.md @@ -52,7 +52,7 @@ reconcile after deployment without any manual backfill. rooms — a system-wide policy change out of scope for this PR. - The grant is a **merge**, never a replace: `Provisioner. EnsureRoomPowerLevel` reads the current `m.room.power_levels` - (`matrix.Client.GetRoomState`, new — admin identity, 404 → empty state), + (`matrix.Client.GetRoomState`, new — actor token with admin fallback, 404 → empty state), adds/raises the human's entry in `users`, preserves every other user and every non-user setting (`users_default`, `state_default`, `ban`, …), and writes back only when the level actually changed. Steady state = one GET @@ -75,6 +75,56 @@ level 100 to the creation-time override. Rooms created before this change are healed one-time by the Manager (a `PUT m.room.power_levels` per room) — a one-off operations task, not part of this PR. +## Authorization: actor selection and the equal-level deadlock + +The homeserver enforces room-auth rules that a plain "write with the admin +token" cannot assume away (spec v8 rules; the fake in +`provisioner_team_test.go` enforces the same rules, so tests prove the +controller survives a real homeserver): + +- **Strict-greater on other users' entries (rule 9.6).** A sender may + change or remove another user's `users` entry only if the sender's level + is **strictly greater** than the target's CURRENT level. A sender at 100 + therefore cannot demote a former L1 human who sits at 100. The sender's + OWN entry is exempt — a self-demotion is always authorized (downward). + On room version 12+ the creator holds an infinite level and is never + blocked; production rooms are v1–11, where the deadlock is real. +- **Kicks (rule 4.5.4).** The kicker needs at least the `kick` level + (50) AND the target's level strictly below the kicker's — an equal-power + kick is rejected. A user may always leave their own room (4.5.1). +- **Membership-scoped state access.** Reading or writing room state as a + non-member is rejected — including the homeserver admin in + **TeamAdmin-owned rooms**, where `ProvisionTeamRooms` deliberately + creates and reconciles as the TeamAdmin and leaves the admin out. + +Consequences implemented by this PR: + +1. **Actor selection.** `GetRoomState` / `SetRoomState` accept an explicit + token ("" = homeserver admin, as before). The human reconciler + annotates each desired room with its origin; a team room of a team with + `spec.admin` is granted as that TeamAdmin (token resolved from the + admin Human via the shared `resolveTeamAdminActor`), every other room + keeps the default admin actor. +2. **Equal-level demotion → self-write fallback.** `EnsureRoomPowerLevel` + writes as the actor; on `M_FORBIDDEN` it retries with the human's own + token (`selfToken`), whose own entry is exempt from 9.6. The reconciler + fetches that token **lazily** (only after the 403), so steady-state + cycles still issue no Matrix Login. +3. **Revocation chain** (removal from the desired set), each stage covering + what the previous cannot: + 1. kick as the homeserver admin (rooms it is in, target below 100); + 2. **self-leave** with the human's own token (always authorized, any + room, any level — the only in-band path for an equal-level 100); + 3. the Tuwunel admin-bot force-leave (token unavailable / stale + password). A confirmed command delivery is treated as resolved, + matching the team-reconcile convention. +4. **Kick idempotency fix.** `KickFromRoomWithToken` used to swallow a + 403 `cannot kick ...` as success, which made an equal-power kick look + like a removal: the room was dropped from `status.rooms` while the + user stayed in it. Only a 404 / "not in room" answer is idempotent; + every other 403 is returned as a decodable `M_FORBIDDEN` + (`matrix.APIError` / `matrix.IsForbidden`) so callers can fall back. + ## What is not changed - Worker / team / DM room creation keeps its existing power levels @@ -87,17 +137,45 @@ a one-off operations task, not part of this PR. - `internal/matrix/client_test.go` — `TestGetRoomState`: returns the state **content** (not the event envelope) with the admin token; missing state - → `(nil, nil)`, not an error. -- `internal/service/provisioner_power_test.go`: legacy room → write with - the user's level; existing users merged and untouched; extension fields - (`events`, `invite`, `notifications`) preserved through the write — only - the target users entry is mutated; exact-match level → no write; - **demotion revokes: a user at 100 granted 50 is lowered to 50**; read - error propagates with no write; state without a `users` map handled; - second grant preserves the first (JSON round-trip semantics). + → `(nil, nil)`, not an error. `TestGetRoomState_WithUserToken` (explicit + token authenticates as that token, not the admin), + `TestGetRoomState_Forbidden` / `TestSetRoomState_Forbidden` (non-member / + rejected writes surface a decodable `M_FORBIDDEN` via + `matrix.IsForbidden`), `TestKickFromRoom_EqualPowerForbidden` (403 + `cannot kick` is an error, not a silent success — the old swallowing + branch is gone), `TestLeaveRoom_IdempotentNotFound`. +- `internal/service/provisioner_power_test.go` (run against the + **authorization-aware** fake, which enforces the spec rules above — a + permissive double would not have caught either P1): legacy room → write + with the user's level; existing users merged and untouched; extension + fields (`events`, `invite`, `notifications`) preserved through the write + — only the target users entry is mutated; exact-match level → no write; + read error propagates with no write; state without a `users` map + handled; second grant preserves the first (JSON round-trip semantics); + **equal-level demotion** — `TestEnsureRoomPowerLevel_DemotionRevokesLevel` + (actor 100 vs human 100: the actor write is rejected by enforced 9.6 and + the self-write with the human's own token completes the 100 → 50), + `TestEnsureRoomPowerLevel_EqualLevelDemotionWithoutSelfTokenFails` + (no self token → `M_FORBIDDEN` surfaced, state unchanged — no silent + success), `TestEnsureRoomPowerLevel_SimpleGrantIsSingleActorWrite` + (0 → 50 is a plain actor write, one attempt), + `TestEnsureRoomPowerLevel_TeamAdminOwnedRoom` (admin read of a + TeamAdmin-owned room → `M_FORBIDDEN`; team-admin actor reads and writes + the grant), `TestEnsureRoomPowerLevel_TeamAdminRoomEqualLevelDemotion` + (same 9.6 wall as the team-admin actor, self fallback completes it). - `internal/controller/human_controller_test.go`: `TestHumanReconciler_PowerLevelMapping` (level 1 → 100 in both a new room and an already-observed room; grant targets the human's Matrix ID), `TestHumanReconciler_PowerLevelL2GetsDefault` (level 2 → 50), `TestHumanReconciler_PowerLevelErrorNonFatal` (grant failure does not - block the reconcile; room still recorded). + block the reconcile; room still recorded), + `TestHumanReconciler_PowerGrantUsesTeamAdminActor` (team room granted + with the TeamAdmin's token, worker room with the default admin; admin + token resolved via login as the admin human), + `TestHumanReconciler_EqualLevelDemotionSelfWriteFallback` (403 → retry + with the human's own token; login issued lazily, exactly once), + `TestHumanReconciler_PowerGrantNoLoginOnSuccess` (no 403 → no login), + `TestHumanReconciler_RevocationSelfLeaveFallback` (kick rejected → + self-leave with the human token → room dropped), + `TestHumanReconciler_RevocationForceLeaveLastResort` (stale password, + no self token → admin-bot force-leave → room dropped). From bc950f80c2480f9a95a474b8b7bc57f77574b32d Mon Sep 17 00:00:00 2001 From: LUOSENGWA <luosengwa@qq.com> Date: Fri, 11 Sep 2026 07:45:36 +0000 Subject: [PATCH 3/6] docs(room-power-levels): document known authorization limitations Per round-2 re-review follow-up: spell out the four documented limits of the authorized demotion/revocation paths (per-cycle TeamAdmin token resolution, stuck demotion for a tokenless level-100 human, spec.admin removal on existing rooms, and why revocation starts from the homeserver-admin kick rather than an actor-scoped one). All non-fatal; retry or documented-stuck. --- docs/design/room-power-levels.md | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/docs/design/room-power-levels.md b/docs/design/room-power-levels.md index d70123cd0..42f16afcb 100644 --- a/docs/design/room-power-levels.md +++ b/docs/design/room-power-levels.md @@ -125,6 +125,31 @@ Consequences implemented by this PR: every other 403 is returned as a decodable `M_FORBIDDEN` (`matrix.APIError` / `matrix.IsForbidden`) so callers can fall back. +Known limitations (documented, all non-fatal / retry or documented-stuck): + +1. **TeamAdmin actor token is re-resolved every reconcile cycle** (no + cross-cycle cache): in steady state, each 5-minute cycle issues one + Matrix login per (human, team room of a team with `spec.admin`). + Deliberate: a fresh login self-heals immediately after a password + change; a TTL cache is a possible follow-up if this becomes load. + This matches the pre-existing team-reconcile behaviour, which also + resolves the TeamAdmin actor token on every team reconcile. +2. **A level-100 human whose Matrix password is unavailable cannot be + demoted.** The actor write is rejected (9.6, equal level) and there is + no self token, so the demotion is retried every cycle without effect. + Matrix provides no out-of-band equal-level demotion. *Removal* from + the room is unaffected (admin-bot force-leave still works). +3. **Removing `spec.admin` from a team whose room already exists** leaves + that room owned by the former TeamAdmin (the homeserver admin is not a + member): actor selection falls back to the admin identity, the grant + 403s, and is retried every cycle without effect. Revocation is + unaffected (self-leave / force-leave still work). +4. **The revocation chain starts from a homeserver-admin kick.** A + removed room is by definition no longer in the desired set, and its + origin is not recorded in `status`, so an actor-scoped kick + (`KickFromRoomAs`) cannot be chosen yet; it is in place for when + origin tracking lands in status. + ## What is not changed - Worker / team / DM room creation keeps its existing power levels From dff7ce134d949ee678384bdef9b3b4e5d309d459 Mon Sep 17 00:00:00 2001 From: LUOSENGWA <luosengwa@qq.com> Date: Fri, 11 Sep 2026 08:15:16 +0000 Subject: [PATCH 4/6] perf(matrix): cache /login tokens per user with 30-minute TTL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Steady-state actor resolution (the TeamAdmin token on every human room grant and every team reconcile) previously issued a Matrix Login on every 5-minute cycle. TuwunelClient now caches /login access tokens per user (password and AppService-impersonation logins alike) for 30 minutes. In-band token invalidators clear the entry: password reset (orphan recovery, SetPasswordAsAdmin) and account deactivation; out-of-band invalidation self-heals on TTL expiry. Logins that double as an account-liveness check (the existing-account fallbacks in EnsureUser/EnsureAppServiceUser, which drive orphan recovery) bypass the cache via loginFresh/loginAppServiceFresh, so a cached dead token can never short-circuit the recovery flow — locked in by TestEnsureUser_OrphanRecovery_IgnoresStaleCachedToken. Retires known limitation 1 from docs/design/room-power-levels.md and adds consequence #5 (login-token cache) plus the new test list. --- .../internal/matrix/client.go | 114 ++++++++- .../internal/matrix/client_test.go | 226 ++++++++++++++++++ .../internal/service/provisioner_human.go | 9 +- .../internal/service/provisioner_team_test.go | 3 + docs/design/room-power-levels.md | 34 ++- 5 files changed, 368 insertions(+), 18 deletions(-) diff --git a/agentteams-controller/internal/matrix/client.go b/agentteams-controller/internal/matrix/client.go index 91ddb293d..fd8a134d9 100644 --- a/agentteams-controller/internal/matrix/client.go +++ b/agentteams-controller/internal/matrix/client.go @@ -10,6 +10,7 @@ import ( "net/http" "net/url" "strings" + "sync" "sync/atomic" "time" @@ -190,6 +191,12 @@ type Client interface { // they can still log in via Element. SetPasswordAsAdmin(ctx context.Context, userID, password string) error + // InvalidateUserToken discards any cached login token for the user so + // the next Login issues a fresh one. Implementations that cache /login + // tokens must call it (or be called after) operations that can + // invalidate access tokens: password reset, account deactivation. + InvalidateUserToken(userID string) + // RegisterAppService registers an Application Service with the homeserver // via the admin bot command. Includes smoke-test-first idempotency and // unregister-before-register fallback for safe token rotation. @@ -226,6 +233,12 @@ type SyncMessagesResult struct { Events []MessageEvent } +// loginTokenEntry is one cached /login access token. +type loginTokenEntry struct { + token string + expiresAt time.Time +} + // TuwunelClient implements Client for Tuwunel (conduwuit) homeservers. type TuwunelClient struct { config Config @@ -233,6 +246,20 @@ type TuwunelClient struct { adminToken atomic.Value // cached admin access token (string) adminRoomID atomic.Value // cached admin room ID (string), resolved from #admins:<domain> + // userLoginCache caches /login access tokens per full Matrix user ID, + // so repeated actor resolutions (the TeamAdmin token on every human + // room grant and every team reconcile, the human's own token on + // every join) do not issue a Matrix Login on every cycle. The admin + // token is cached separately (adminToken). + userLoginCache map[string]loginTokenEntry + userLoginMu sync.Mutex + // loginTokenTTL bounds how long a cached login token is trusted. + // In-band invalidators (password reset, deactivation) call + // InvalidateUserToken; out-of-band invalidation (server-side revoke, + // logout-everywhere) self-heals on TTL expiry. Exposed as a field + // (not a const) so tests can collapse it. + loginTokenTTL time.Duration + // orphanRetryBaseDelay is the base backoff between Login retries // after issuing an admin reset-password command. Exposed as a field // (not a const) so tests can collapse the delay. @@ -247,10 +274,41 @@ func NewTuwunelClient(cfg Config, httpClient *http.Client) *TuwunelClient { return &TuwunelClient{ config: cfg, http: httpClient, + loginTokenTTL: 30 * time.Minute, orphanRetryBaseDelay: 500 * time.Millisecond, } } +// cachedLoginToken returns the cached /login token for userID if it has +// not expired. +func (c *TuwunelClient) cachedLoginToken(userID string) (string, bool) { + c.userLoginMu.Lock() + defer c.userLoginMu.Unlock() + e, ok := c.userLoginCache[userID] + if !ok || time.Now().After(e.expiresAt) { + return "", false + } + return e.token, true +} + +func (c *TuwunelClient) storeLoginToken(userID, token string) { + c.userLoginMu.Lock() + defer c.userLoginMu.Unlock() + if c.userLoginCache == nil { + c.userLoginCache = make(map[string]loginTokenEntry) + } + c.userLoginCache[userID] = loginTokenEntry{token: token, expiresAt: time.Now().Add(c.loginTokenTTL)} +} + +// InvalidateUserToken discards any cached /login token for the user so the +// next Login issues a fresh one. Call it after any operation that can +// invalidate access tokens (password reset, account deactivation). +func (c *TuwunelClient) InvalidateUserToken(userID string) { + c.userLoginMu.Lock() + defer c.userLoginMu.Unlock() + delete(c.userLoginCache, userID) +} + func (c *TuwunelClient) UserID(localpart string) string { return fmt.Sprintf("@%s:%s", localpart, c.config.Domain) } @@ -314,8 +372,10 @@ func (c *TuwunelClient) EnsureUser(ctx context.Context, req EnsureUserRequest) ( return nil, fmt.Errorf("register user %s: %s (%s)", req.Username, regResp.ErrCode, regResp.Error) } - // Registration failed with M_USER_IN_USE — try login - token, err := c.Login(ctx, req.Username, password) + // Registration failed with M_USER_IN_USE — try login. loginFresh: this + // login doubles as an account-liveness check; a cached (possibly dead) + // token must not short-circuit the orphan recovery below. + token, err := c.loginFresh(ctx, req.Username, password) if err == nil { return &UserCredentials{ UserID: c.UserID(req.Username), @@ -337,6 +397,9 @@ func (c *TuwunelClient) EnsureUser(ctx context.Context, req EnsureUserRequest) ( return nil, fmt.Errorf("user %s exists but login failed (%v) and orphan recovery failed: %w", req.Username, err, adminErr) } + // The password just changed: any cached token is suspect — force the + // retry loop below to go to the homeserver. + c.InvalidateUserToken(userID) const maxAttempts = 5 baseDelay := c.orphanRetryBaseDelay @@ -350,7 +413,7 @@ func (c *TuwunelClient) EnsureUser(ctx context.Context, req EnsureUserRequest) ( return nil, ctx.Err() case <-time.After(baseDelay * time.Duration(attempt)): } - token, lastErr = c.Login(ctx, req.Username, password) + token, lastErr = c.loginFresh(ctx, req.Username, password) if lastErr == nil { return &UserCredentials{ UserID: userID, @@ -365,6 +428,22 @@ func (c *TuwunelClient) EnsureUser(ctx context.Context, req EnsureUserRequest) ( } func (c *TuwunelClient) Login(ctx context.Context, username, password string) (string, error) { + // Cache hit: the token was obtained by a previous successful login for + // this user and is still within the TTL. No HTTP call is issued. + userID := c.UserID(username) + if token, ok := c.cachedLoginToken(userID); ok { + return token, nil + } + return c.loginFresh(ctx, username, password) +} + +// loginFresh always goes to the homeserver and stores the result in the +// cache. Callers whose login doubles as an account-liveness check (the +// EnsureUser orphan-recovery path) must use it directly: a cached token +// may be dead (account deactivated out-of-band) and must not short- +// circuit the recovery flow. +func (c *TuwunelClient) loginFresh(ctx context.Context, username, password string) (string, error) { + userID := c.UserID(username) body := map[string]interface{}{ "type": "m.login.password", "identifier": map[string]string{ @@ -388,6 +467,7 @@ func (c *TuwunelClient) Login(ctx context.Context, username, password string) (s if resp.AccessToken == "" { return "", fmt.Errorf("login %s: empty access token", username) } + c.storeLoginToken(userID, resp.AccessToken) return resp.AccessToken, nil } @@ -426,10 +506,12 @@ func (c *TuwunelClient) EnsureAppServiceUser(ctx context.Context, username strin }, nil } - // User already exists → fall back to AS login + // User already exists → fall back to AS login. loginAppServiceFresh: + // this login doubles as an account-liveness check; a cached (possibly + // dead) token must not short-circuit deactivation handling. if regResp.ErrCode == "M_USER_IN_USE" { logger.Info("Matrix account already exists; falling back to AppService login", "httpStatus", statusCode) - token, loginErr := c.LoginAppServiceUser(ctx, username) + token, loginErr := c.loginAppServiceFresh(ctx, username) if loginErr != nil { if errors.Is(loginErr, ErrAppServiceNotReady) { logger.Info("Matrix AppService token not active yet during login fallback; will retry") @@ -464,6 +546,18 @@ func (c *TuwunelClient) EnsureAppServiceUser(ctx context.Context, username strin // Service login flow. The as_token authenticates the request; no user password // is needed. func (c *TuwunelClient) LoginAppServiceUser(ctx context.Context, username string) (string, error) { + // Cache hit: same TTL semantics as the password Login — no HTTP call. + userID := c.UserID(username) + if token, ok := c.cachedLoginToken(userID); ok { + return token, nil + } + return c.loginAppServiceFresh(ctx, username) +} + +// loginAppServiceFresh is the AppService variant of loginFresh (see its +// doc for why liveness-checking callers must bypass the cache). +func (c *TuwunelClient) loginAppServiceFresh(ctx context.Context, username string) (string, error) { + userID := c.UserID(username) body := map[string]interface{}{ "type": "m.login.application_service", "identifier": map[string]string{ @@ -492,6 +586,7 @@ func (c *TuwunelClient) LoginAppServiceUser(ctx context.Context, username string if resp.AccessToken == "" { return "", fmt.Errorf("AS login %s: empty access token", username) } + c.storeLoginToken(userID, resp.AccessToken) return resp.AccessToken, nil } @@ -500,7 +595,14 @@ func (c *TuwunelClient) LoginAppServiceUser(ctx context.Context, username string // so they can still log in via Element with username/password. func (c *TuwunelClient) SetPasswordAsAdmin(ctx context.Context, userID, password string) error { cmd := fmt.Sprintf("!admin users reset-password %s %s", userID, password) - return c.AdminCommand(ctx, cmd) + if err := c.AdminCommand(ctx, cmd); err != nil { + return err + } + // A password reset may invalidate the user's existing access tokens + // (see the provisioner's "clear cached AS token" convention): drop any + // cached one so the next Login goes to the homeserver. + c.InvalidateUserToken(userID) + return nil } // doJSONWithASToken performs an HTTP request authenticated with the AppService diff --git a/agentteams-controller/internal/matrix/client_test.go b/agentteams-controller/internal/matrix/client_test.go index c597ffe72..eec819523 100644 --- a/agentteams-controller/internal/matrix/client_test.go +++ b/agentteams-controller/internal/matrix/client_test.go @@ -3,6 +3,7 @@ package matrix import ( "context" "encoding/json" + "fmt" "net/http" "net/http/httptest" "sync/atomic" @@ -1259,3 +1260,228 @@ func TestLeaveRoom_IdempotentNotFound(t *testing.T) { t.Errorf("expected nil for not-in-room, got %v", err) } } + +// Login caching: a successful login caches the token per user; a second +// login for the same user is served from the cache (no HTTP), while a +// different user still goes to the homeserver. +func TestLogin_TokenCachedPerUser(t *testing.T) { + var logins atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/_matrix/client/v3/login" { + w.WriteHeader(http.StatusNotFound) + return + } + n := logins.Add(1) + json.NewEncoder(w).Encode(map[string]string{"access_token": fmt.Sprintf("tok-%d", n)}) + })) + defer server.Close() + + c := NewTuwunelClient(Config{ServerURL: server.URL, Domain: "d"}, server.Client()) + + tok1, err := c.Login(context.Background(), "alice", "pw") + if err != nil { + t.Fatalf("login 1: %v", err) + } + if logins.Load() != 1 { + t.Fatalf("logins=%d, want 1", logins.Load()) + } + tok2, err := c.Login(context.Background(), "alice", "pw") + if err != nil { + t.Fatalf("login 2: %v", err) + } + if tok2 != tok1 { + t.Errorf("cached token = %q, want %q (no second login)", tok2, tok1) + } + if logins.Load() != 1 { + t.Errorf("logins=%d, want 1 (per-user cache)", logins.Load()) + } + if _, err := c.Login(context.Background(), "bob", "pw"); err != nil { + t.Fatalf("login bob: %v", err) + } + if logins.Load() != 2 { + t.Errorf("logins=%d, want 2 (different user)", logins.Load()) + } +} + +// Login caching: an expired cache entry goes back to the homeserver. +func TestLogin_TokenCacheExpires(t *testing.T) { + var logins atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/_matrix/client/v3/login" { + w.WriteHeader(http.StatusNotFound) + return + } + n := logins.Add(1) + json.NewEncoder(w).Encode(map[string]string{"access_token": fmt.Sprintf("tok-%d", n)}) + })) + defer server.Close() + + c := NewTuwunelClient(Config{ServerURL: server.URL, Domain: "d"}, server.Client()) + if _, err := c.Login(context.Background(), "alice", "pw"); err != nil { + t.Fatalf("login 1: %v", err) + } + // White-box: expire the cached entry (same package). + c.userLoginMu.Lock() + e := c.userLoginCache[c.UserID("alice")] + e.expiresAt = time.Now().Add(-time.Second) + c.userLoginCache[c.UserID("alice")] = e + c.userLoginMu.Unlock() + if _, err := c.Login(context.Background(), "alice", "pw"); err != nil { + t.Fatalf("login 2: %v", err) + } + if logins.Load() != 2 { + t.Errorf("logins=%d, want 2 after TTL expiry", logins.Load()) + } +} + +// Login caching: the AppService impersonation login caches the same way. +func TestLogin_AppServiceTokenCached(t *testing.T) { + var logins atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/_matrix/client/v3/login" { + w.WriteHeader(http.StatusNotFound) + return + } + n := logins.Add(1) + json.NewEncoder(w).Encode(map[string]string{"access_token": fmt.Sprintf("as-tok-%d", n)}) + })) + defer server.Close() + + c := NewTuwunelClient(Config{ServerURL: server.URL, Domain: "d", AppServiceToken: "as"}, server.Client()) + tok1, err := c.LoginAppServiceUser(context.Background(), "carol") + if err != nil { + t.Fatalf("AS login 1: %v", err) + } + tok2, err := c.LoginAppServiceUser(context.Background(), "carol") + if err != nil { + t.Fatalf("AS login 2: %v", err) + } + if tok2 != tok1 { + t.Errorf("cached AS token = %q, want %q", tok2, tok1) + } + if logins.Load() != 1 { + t.Errorf("logins=%d, want 1 (AS per-user cache)", logins.Load()) + } +} + +// InvalidateUserToken: an explicit invalidation forces a fresh login. +func TestInvalidateUserToken_FreshLogin(t *testing.T) { + var logins atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/_matrix/client/v3/login" { + w.WriteHeader(http.StatusNotFound) + return + } + n := logins.Add(1) + json.NewEncoder(w).Encode(map[string]string{"access_token": fmt.Sprintf("tok-%d", n)}) + })) + defer server.Close() + + c := NewTuwunelClient(Config{ServerURL: server.URL, Domain: "d"}, server.Client()) + if _, err := c.Login(context.Background(), "dave", "pw"); err != nil { + t.Fatalf("login 1: %v", err) + } + c.InvalidateUserToken(c.UserID("dave")) + if _, err := c.Login(context.Background(), "dave", "pw"); err != nil { + t.Fatalf("login 2: %v", err) + } + if logins.Load() != 2 { + t.Errorf("logins=%d, want 2 after invalidation", logins.Load()) + } +} + +// Regression: a STALE cached token (account deactivated out-of-band) must +// not short-circuit orphan recovery. EnsureUser must reach the homeserver, +// see the failed login, issue the reset-password command, and return a +// fresh token — not the cached dead one. +func TestEnsureUser_OrphanRecovery_IgnoresStaleCachedToken(t *testing.T) { + var ( + bobLoginCalls int32 + adminSendHit int32 + ) + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.URL.Path == "/_matrix/client/v3/register": + w.WriteHeader(http.StatusBadRequest) + json.NewEncoder(w).Encode(map[string]string{ + "errcode": "M_USER_IN_USE", + "error": "User ID already taken", + }) + + case r.URL.Path == "/_matrix/client/v3/login": + var body struct { + Identifier struct { + User string `json:"user"` + } `json:"identifier"` + } + _ = json.NewDecoder(r.Body).Decode(&body) + if body.Identifier.User == "admin" { + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]string{"access_token": "admin-token"}) + return + } + // bob: first login fails (stale password), retry succeeds. + n := atomic.AddInt32(&bobLoginCalls, 1) + if n <= 1 { + w.WriteHeader(http.StatusForbidden) + json.NewEncoder(w).Encode(map[string]string{ + "errcode": "M_FORBIDDEN", + "error": "Invalid password", + }) + return + } + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]string{"access_token": "fresh-token"}) + + case r.URL.Path == "/_matrix/client/v3/directory/room/#admins:test.domain": + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]string{"room_id": "!admins:test.domain"}) + + case r.Method == http.MethodPut && + len(r.URL.Path) > len("/_matrix/client/v3/rooms/") && + r.URL.Path[:len("/_matrix/client/v3/rooms/")] == "/_matrix/client/v3/rooms/": + atomic.AddInt32(&adminSendHit, 1) + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{"event_id":"$evt"}`)) + + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + })) + defer server.Close() + + c := NewTuwunelClient(Config{ + ServerURL: server.URL, + Domain: "test.domain", + RegistrationToken: "reg", + AdminUser: "admin", + AdminPassword: "adminpw", + }, server.Client()) + c.orphanRetryBaseDelay = time.Millisecond + + // A previously successful login left a cached token; the account was + // since deactivated, so that token is dead. + c.storeLoginToken(c.UserID("bob"), "stale-token") + + creds, err := c.EnsureUser(context.Background(), EnsureUserRequest{ + Username: "bob", + Password: "bobpw", + }) + if err != nil { + t.Fatalf("EnsureUser: %v", err) + } + if creds.AccessToken == "stale-token" { + t.Error("EnsureUser returned the cached dead token — orphan recovery was skipped") + } + if creds.AccessToken != "fresh-token" { + t.Errorf("AccessToken = %q, want fresh-token", creds.AccessToken) + } + if atomic.LoadInt32(&bobLoginCalls) < 2 { + t.Errorf("bob login calls=%d, want >=2 (fail then retry via orphan recovery)", bobLoginCalls) + } + if atomic.LoadInt32(&adminSendHit) == 0 { + t.Error("expected the reset-password admin command to be sent") + } +} diff --git a/agentteams-controller/internal/service/provisioner_human.go b/agentteams-controller/internal/service/provisioner_human.go index 17d9cb865..0c00a1d7c 100644 --- a/agentteams-controller/internal/service/provisioner_human.go +++ b/agentteams-controller/internal/service/provisioner_human.go @@ -221,5 +221,12 @@ func (p *Provisioner) ForceLeaveRoom(ctx context.Context, userID, roomID string) func (p *Provisioner) DeactivateHumanUser(ctx context.Context, userID string) error { cmd := fmt.Sprintf("!admin users deactivate %s", userID) log.FromContext(ctx).Info("sending tuwunel human deactivate admin command", "user", userID, "command", cmd) - return p.matrix.AdminCommand(ctx, cmd) + if err := p.matrix.AdminCommand(ctx, cmd); err != nil { + return err + } + // Deactivation kills the account's access tokens: drop any cached one + // so a later re-provisioning of the same username re-logins fresh + // (and hits orphan recovery if the account is gone). + p.matrix.InvalidateUserToken(userID) + return nil } diff --git a/agentteams-controller/internal/service/provisioner_team_test.go b/agentteams-controller/internal/service/provisioner_team_test.go index 52c9f9d51..1fd62d931 100644 --- a/agentteams-controller/internal/service/provisioner_team_test.go +++ b/agentteams-controller/internal/service/provisioner_team_test.go @@ -241,6 +241,9 @@ func (f *fakeTeamMatrix) SendMessageAsAdmin(context.Context, string, string) err func (f *fakeTeamMatrix) Login(context.Context, string, string) (string, error) { return "", nil } +// InvalidateUserToken is a no-op: the fake holds no login-token cache. +func (f *fakeTeamMatrix) InvalidateUserToken(string) {} + func (f *fakeTeamMatrix) SetDisplayName(context.Context, string, string, string) error { return nil } func (f *fakeTeamMatrix) AdminCommand(_ context.Context, cmd string) error { diff --git a/docs/design/room-power-levels.md b/docs/design/room-power-levels.md index 42f16afcb..36fe3a98e 100644 --- a/docs/design/room-power-levels.md +++ b/docs/design/room-power-levels.md @@ -124,27 +124,32 @@ Consequences implemented by this PR: user stayed in it. Only a 404 / "not in room" answer is idempotent; every other 403 is returned as a decodable `M_FORBIDDEN` (`matrix.APIError` / `matrix.IsForbidden`) so callers can fall back. +5. **Login-token cache (steady-state logins).** `TuwunelClient` caches + `/login` access tokens per user for 30 minutes (password and + AppService-impersonation logins alike), so the per-cycle TeamAdmin + actor resolution — and the human's own token resolution — issue no + Matrix Login in steady state. In-band token invalidators clear the + entry: password reset (orphan recovery, `SetPasswordAsAdmin`) and + account deactivation; out-of-band invalidation (server-side revoke, + logout-everywhere) self-heals on TTL expiry. Logins that double as an + account-liveness check (the existing-account fallbacks in + `EnsureUser` / `EnsureAppServiceUser`, which drive orphan recovery) + always go to the homeserver, so a cached dead token can never + short-circuit the recovery flow. Known limitations (documented, all non-fatal / retry or documented-stuck): -1. **TeamAdmin actor token is re-resolved every reconcile cycle** (no - cross-cycle cache): in steady state, each 5-minute cycle issues one - Matrix login per (human, team room of a team with `spec.admin`). - Deliberate: a fresh login self-heals immediately after a password - change; a TTL cache is a possible follow-up if this becomes load. - This matches the pre-existing team-reconcile behaviour, which also - resolves the TeamAdmin actor token on every team reconcile. -2. **A level-100 human whose Matrix password is unavailable cannot be +1. **A level-100 human whose Matrix password is unavailable cannot be demoted.** The actor write is rejected (9.6, equal level) and there is no self token, so the demotion is retried every cycle without effect. Matrix provides no out-of-band equal-level demotion. *Removal* from the room is unaffected (admin-bot force-leave still works). -3. **Removing `spec.admin` from a team whose room already exists** leaves +2. **Removing `spec.admin` from a team whose room already exists** leaves that room owned by the former TeamAdmin (the homeserver admin is not a member): actor selection falls back to the admin identity, the grant 403s, and is retried every cycle without effect. Revocation is unaffected (self-leave / force-leave still work). -4. **The revocation chain starts from a homeserver-admin kick.** A +3. **The revocation chain starts from a homeserver-admin kick.** A removed room is by definition no longer in the desired set, and its origin is not recorded in `status`, so an actor-scoped kick (`KickFromRoomAs`) cannot be chosen yet; it is in place for when @@ -168,7 +173,14 @@ Known limitations (documented, all non-fatal / retry or documented-stuck): rejected writes surface a decodable `M_FORBIDDEN` via `matrix.IsForbidden`), `TestKickFromRoom_EqualPowerForbidden` (403 `cannot kick` is an error, not a silent success — the old swallowing - branch is gone), `TestLeaveRoom_IdempotentNotFound`. + branch is gone), `TestLeaveRoom_IdempotentNotFound`, + `TestLogin_TokenCachedPerUser` (per-user cache: second login is served + from cache, no HTTP; different user still goes to the homeserver), + `TestLogin_TokenCacheExpires` (TTL expiry → fresh login), + `TestLogin_AppServiceTokenCached`, `TestInvalidateUserToken_FreshLogin`, + `TestEnsureUser_OrphanRecovery_IgnoresStaleCachedToken` (a stale cached + token does NOT short-circuit orphan recovery — liveness-check logins + bypass the cache and the reset-password flow completes). - `internal/service/provisioner_power_test.go` (run against the **authorization-aware** fake, which enforces the spec rules above — a permissive double would not have caught either P1): legacy room → write From 4b489238be584c44b3c8184d83bc03e2c2aec0c5 Mon Sep 17 00:00:00 2001 From: LUOSENGWA <luosengwa@qq.com> Date: Fri, 11 Sep 2026 13:07:32 +0000 Subject: [PATCH 5/6] fix(controller): keep team admins/members in their team room (test-19 join-403 deadlock) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit buildDesiredHumanRooms now includes the team rooms of teams where the human is spec.admin or a spec.humanMember, not just spec.accessibleTeams. syncTeamRoomHumanStatuses writes the team room into the admin's status.rooms without touching accessibleTeams, so the access-revocation path kicked the team admin out of their own team room; with the admin excluded from the team-room invite list (creator-join design), the team then failed on join (M_FORBIDDEN: cannot join a room that is not public) on every reconcile — permanent deadlock, CI test-19 4/5 shards. + 2 regression tests; design doc room-power-levels.md item 6. --- .../controller/human_controller_test.go | 76 +++++++++++++++++++ .../internal/controller/human_scope.go | 45 ++++++++--- docs/design/room-power-levels.md | 14 ++++ 3 files changed, 126 insertions(+), 9 deletions(-) diff --git a/agentteams-controller/internal/controller/human_controller_test.go b/agentteams-controller/internal/controller/human_controller_test.go index 17c353bc1..44a8e4641 100644 --- a/agentteams-controller/internal/controller/human_controller_test.go +++ b/agentteams-controller/internal/controller/human_controller_test.go @@ -344,6 +344,82 @@ func TestHumanReconciler_Update_RevokeRoom(t *testing.T) { } } +// TestHumanReconciler_TeamAdminRoomNotRevoked pins the CI test-19 +// regression (join-403 deadlock): the team reconciler's +// syncTeamRoomHumanStatuses writes the team room into the spec.admin's +// Status.Rooms without touching the admin's AccessibleTeams. The +// access-revocation path must therefore treat the admin's team room as +// desired — not "access revoked". Without the team-membership leg in +// buildDesiredHumanRooms the admin gets self-left out of their own team +// room, and the team then fails on join (M_FORBIDDEN: cannot join a room +// that is not public) on every reconcile because the admin is +// deliberately excluded from the team-room invite list. +func TestHumanReconciler_TeamAdminRoomNotRevoked(t *testing.T) { + team := newReadyTeam("t1", "!room-t1:localhost") + team.Spec.Admin = &v1beta1.TeamAdminSpec{Name: "alice"} + human := newHuman("alice", v1beta1.HumanSpec{}) + human.Status.MatrixUserID = "@alice:localhost" + human.Status.InitialPassword = "stored-pw" + // The team reconciler already synced the room into Status.Rooms. + human.Status.Rooms = []string{"!room-t1:localhost"} + human.Status.Phase = "Active" + human.Finalizers = []string{finalizerName} + + rig := newHumanRig(t, human, team) + + out, _, err := rig.reconcile("alice") + if err != nil { + t.Fatalf("reconcile: %v", err) + } + if len(rig.prov.Calls.KickFromRoom) != 0 { + t.Errorf("no admin kick expected for the admin's own team room, got %+v", rig.prov.Calls.KickFromRoom) + } + if len(rig.prov.Calls.KickFromRoomAs) != 0 { + t.Errorf("no actor kick expected, got %+v", rig.prov.Calls.KickFromRoomAs) + } + if len(rig.prov.Calls.LeaveRoomAs) != 0 { + t.Errorf("no self-leave expected, got %+v", rig.prov.Calls.LeaveRoomAs) + } + if len(rig.prov.Calls.ForceLeaveRoom) != 0 { + t.Errorf("no force-leave expected, got %+v", rig.prov.Calls.ForceLeaveRoom) + } + if len(out.Status.Rooms) != 1 || out.Status.Rooms[0] != "!room-t1:localhost" { + t.Errorf("Status.Rooms=%v, want [!room-t1:localhost] retained", out.Status.Rooms) + } +} + +// TestHumanReconciler_HumanMemberRoomNotRevoked is the humanMembers variant +// of TestHumanReconciler_TeamAdminRoomNotRevoked: a human listed in +// team.Spec.HumanMembers (and mirrored into the team's members by the +// team reconciler) must also keep their team room even when their own +// AccessibleTeams is empty. +func TestHumanReconciler_HumanMemberRoomNotRevoked(t *testing.T) { + team := newReadyTeam("t1", "!room-t1:localhost") + team.Spec.HumanMembers = []v1beta1.TeamMemberSpec{{Name: "bob", MatrixUserID: "@bob:localhost"}} + human := newHuman("bob", v1beta1.HumanSpec{}) + human.Status.MatrixUserID = "@bob:localhost" + human.Status.InitialPassword = "stored-pw" + human.Status.Rooms = []string{"!room-t1:localhost"} + human.Status.Phase = "Active" + human.Finalizers = []string{finalizerName} + + rig := newHumanRig(t, human, team) + + out, _, err := rig.reconcile("bob") + if err != nil { + t.Fatalf("reconcile: %v", err) + } + if len(rig.prov.Calls.KickFromRoom) != 0 { + t.Errorf("no kick expected for a humanMember's team room, got %+v", rig.prov.Calls.KickFromRoom) + } + if len(rig.prov.Calls.LeaveRoomAs) != 0 { + t.Errorf("no self-leave expected, got %+v", rig.prov.Calls.LeaveRoomAs) + } + if len(out.Status.Rooms) != 1 || out.Status.Rooms[0] != "!room-t1:localhost" { + t.Errorf("Status.Rooms=%v, want [!room-t1:localhost] retained", out.Status.Rooms) + } +} + // TestHumanReconciler_Update_PendingResource exercises the case where // an AccessibleWorker references a Worker CR whose Status.RoomID is not // yet populated (still provisioning). The reconciler must not invite diff --git a/agentteams-controller/internal/controller/human_scope.go b/agentteams-controller/internal/controller/human_scope.go index 50092bd64..858779d2f 100644 --- a/agentteams-controller/internal/controller/human_scope.go +++ b/agentteams-controller/internal/controller/human_scope.go @@ -67,8 +67,9 @@ type humanRoomOrigin struct { } // buildDesiredHumanRooms resolves Spec.AccessibleWorkers / AccessibleTeams -// into the set of Matrix room IDs the human should currently be a member -// of, annotated with each room's origin. Workers/Teams that don't exist or +// plus team membership (spec.admin / spec.humanMembers) into the set of +// Matrix room IDs the human should currently be a member of, annotated +// with each room's origin. Workers/Teams that don't exist or // haven't finished provisioning (empty Status.RoomID / TeamRoomID) are // simply skipped — they'll be picked up on a later reconcile once their // rooms materialize. @@ -87,13 +88,39 @@ func buildDesiredHumanRooms(ctx context.Context, c client.Client, h *v1beta1.Hum desired[worker.Status.RoomID] = humanRoomOrigin{workerName: workerName} } } - for _, teamName := range h.Spec.AccessibleTeams { - var team v1beta1.Team - if err := c.Get(ctx, client.ObjectKey{Name: teamName, Namespace: h.Namespace}, &team); err != nil { - continue - } - if team.Status.TeamRoomID != "" { - desired[team.Status.TeamRoomID] = humanRoomOrigin{teamName: teamName} + // Team rooms: a human belongs to a team's room when the team names + // them (spec.admin / spec.humanMembers) or the human names the team + // (spec.accessibleTeams). The team-membership leg is load-bearing: + // syncTeamRoomHumanStatuses writes the team room into the admin's and + // human members' Status.Rooms WITHOUT touching their AccessibleTeams, + // so a desired set built from AccessibleTeams alone would let the + // access-revocation path kick the team admin out of their own team + // room — and, because the admin is deliberately excluded from the + // team-room invite list (creator-join design in ProvisionTeamRooms), + // the team would then fail on join (M_FORBIDDEN: cannot join a room + // that is not public) on every reconcile. + var teams v1beta1.TeamList + if err := c.List(ctx, &teams, client.InNamespace(h.Namespace)); err == nil { + for i := range teams.Items { + tm := &teams.Items[i] + if tm.Status.TeamRoomID == "" { + continue + } + belongs := containsString(h.Spec.AccessibleTeams, tm.Name) + if !belongs && tm.Spec.Admin != nil && tm.Spec.Admin.Name == h.Name { + belongs = true + } + if !belongs { + for _, m := range tm.Spec.HumanMembers { + if m.Name == h.Name || (m.MatrixUserID != "" && m.MatrixUserID == h.Status.MatrixUserID) { + belongs = true + break + } + } + } + if belongs { + desired[tm.Status.TeamRoomID] = humanRoomOrigin{teamName: tm.Name} + } } } return desired diff --git a/docs/design/room-power-levels.md b/docs/design/room-power-levels.md index 36fe3a98e..c506185f9 100644 --- a/docs/design/room-power-levels.md +++ b/docs/design/room-power-levels.md @@ -136,6 +136,20 @@ Consequences implemented by this PR: `EnsureUser` / `EnsureAppServiceUser`, which drive orphan recovery) always go to the homeserver, so a cached dead token can never short-circuit the recovery flow. +6. **The human desired-room set recognizes team membership.** + `buildDesiredHumanRooms` includes, beyond `spec.accessibleTeams`, the + team rooms of teams where the human is `spec.admin` or appears in + `spec.humanMembers`. Load-bearing: `syncTeamRoomHumanStatuses` (team + reconciler) writes the team room into the admin's / members' + `status.rooms` WITHOUT touching their `spec.accessibleTeams`; a + human-side desired set built from `accessibleTeams` alone would let + the access-revocation path kick the team admin out of their own team + room, and the team would then fail on join + (`M_FORBIDDEN: cannot join a room that is not public` — the admin is + deliberately excluded from the team-room invite list by the + creator-join design) on every reconcile: a permanent deadlock. + Regression tests: `TestHumanReconciler_TeamAdminRoomNotRevoked` / + `TestHumanReconciler_HumanMemberRoomNotRevoked`. Known limitations (documented, all non-fatal / retry or documented-stuck): From bf6b27d397a676c8b08fd6b2fe3caf8ca3e9e27b Mon Sep 17 00:00:00 2001 From: LUOSENGWA <luosengwa@qq.com> Date: Sat, 12 Sep 2026 05:01:36 +0000 Subject: [PATCH 6/6] fix(controller): defer human-room kicks while a team room claim is unresolved (test-19 join-403, 2nd wave) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Residual window of the 4b489238 fix: right after team provisioning, syncTeamRoomHumanStatuses writes the new team room into the admin's status.rooms BEFORE the team's status.teamRoomID is visible in the human reconciler's cache (informer lag across objects). In that window the room is in status.rooms but no visible Team/Worker claims it — an UNKNOWN origin — and the revocation path kicked it anyway, evicting the team admin from their own team room. With the admin excluded from the invite list (creator-join design), every later team reconcile then failed on join (M_FORBIDDEN: cannot join a room that is not public): permanent deadlock, CI SHARD_C 4/5 on heads 4f0c7946 and 4b489238. teamRoomRevocationLag detects the window (human is spec.admin / spec.humanMembers of a team whose room is not yet visible) and the removal path defers the kick of unknown-origin rooms for one cycle — by then the team status is visible and the origin resolves: still belonging -> desired (kept), genuinely revoked -> kicked. Known-origin revocations stay prompt (control test). + 2 regression tests; design doc room-power-levels.md item 7. --- .../controller/human_controller_test.go | 72 +++++++++++++++++++ .../controller/human_reconcile_rooms.go | 15 ++++ .../internal/controller/human_scope.go | 58 +++++++++++++++ docs/design/room-power-levels.md | 29 +++++++- 4 files changed, 173 insertions(+), 1 deletion(-) diff --git a/agentteams-controller/internal/controller/human_controller_test.go b/agentteams-controller/internal/controller/human_controller_test.go index 44a8e4641..241dd2852 100644 --- a/agentteams-controller/internal/controller/human_controller_test.go +++ b/agentteams-controller/internal/controller/human_controller_test.go @@ -1017,3 +1017,75 @@ func TestHumanReconciler_RevocationForceLeaveLastResort(t *testing.T) { t.Errorf("Status.Rooms=%v, want empty (revoked)", out.Status.Rooms) } } + +// Regression (CI test-19 join-403 deadlock, 2nd wave): the team names the +// human as admin and its room is already in the human's Status.Rooms +// (written by syncTeamRoomHumanStatuses), but the team's +// Status.TeamRoomID is not yet visible in the cache (informer lag right +// after team provisioning). The room's origin is unresolved while the +// human holds a team membership claim -> the revocation path must DEFER +// the kick for one cycle. Kicking here evicts the team admin from their +// own team room; with the admin excluded from the invite list +// (creator-join design), every later team reconcile then fails on join +// (M_FORBIDDEN: cannot join a room that is not public) forever. +func TestHumanReconciler_RevocationDeferredWhileTeamRoomUnresolved(t *testing.T) { + team := &v1beta1.Team{ + ObjectMeta: metav1.ObjectMeta{Name: "team-alpha", Namespace: "default"}, + Spec: v1beta1.TeamSpec{Admin: &v1beta1.TeamAdminSpec{Name: "dave"}}, + // Status.TeamRoomID intentionally empty: room not yet visible. + } + human := newHuman("dave", v1beta1.HumanSpec{}) + human.Status.MatrixUserID = "@dave:localhost" + human.Status.InitialPassword = "stored-pw" + human.Status.Rooms = []string{"!team-new:localhost"} + human.Status.Phase = "Active" + human.Finalizers = []string{finalizerName} + + rig := newHumanRig(t, human, team) + out, _, err := rig.reconcile("dave") + if err != nil { + t.Fatalf("reconcile: %v", err) + } + if len(rig.prov.Calls.KickFromRoom) != 0 { + t.Fatalf("kick calls=%+v, want 0 (deferred while claim unresolved)", rig.prov.Calls.KickFromRoom) + } + if len(out.Status.Rooms) != 1 || out.Status.Rooms[0] != "!team-new:localhost" { + t.Errorf("Status.Rooms=%v, want [!team-new:localhost] (kept for next cycle)", out.Status.Rooms) + } +} + +// Control: the deferral must not mask a genuine revocation. The human +// holds an unresolved claim on team-new (room not visible) but the room +// in Status.Rooms belongs to team-old, whose Status.TeamRoomID IS +// visible and which no longer names the human -> known origin, not +// desired -> the kick proceeds immediately. +func TestHumanReconciler_RevocationProceedsForKnownOriginTeamRoom(t *testing.T) { + teamOld := &v1beta1.Team{ + ObjectMeta: metav1.ObjectMeta{Name: "team-old", Namespace: "default"}, + Spec: v1beta1.TeamSpec{}, // admin removed: human no longer belongs + Status: v1beta1.TeamStatus{TeamRoomID: "!old-room:localhost"}, + } + teamNew := &v1beta1.Team{ + ObjectMeta: metav1.ObjectMeta{Name: "team-new", Namespace: "default"}, + Spec: v1beta1.TeamSpec{Admin: &v1beta1.TeamAdminSpec{Name: "dave"}}, + // Status.TeamRoomID intentionally empty: room not yet visible. + } + human := newHuman("dave", v1beta1.HumanSpec{}) + human.Status.MatrixUserID = "@dave:localhost" + human.Status.InitialPassword = "stored-pw" + human.Status.Rooms = []string{"!old-room:localhost"} + human.Status.Phase = "Active" + human.Finalizers = []string{finalizerName} + + rig := newHumanRig(t, human, teamOld, teamNew) + out, _, err := rig.reconcile("dave") + if err != nil { + t.Fatalf("reconcile: %v", err) + } + if len(rig.prov.Calls.KickFromRoom) != 1 { + t.Fatalf("kick calls=%+v, want 1 (known-origin revocation stays prompt)", rig.prov.Calls.KickFromRoom) + } + if len(out.Status.Rooms) != 0 { + t.Errorf("Status.Rooms=%v, want empty (revoked)", out.Status.Rooms) + } +} diff --git a/agentteams-controller/internal/controller/human_reconcile_rooms.go b/agentteams-controller/internal/controller/human_reconcile_rooms.go index 47494700b..05e4bb54f 100644 --- a/agentteams-controller/internal/controller/human_reconcile_rooms.go +++ b/agentteams-controller/internal/controller/human_reconcile_rooms.go @@ -122,12 +122,27 @@ func (r *HumanReconciler) reconcileHumanRooms(ctx context.Context, s *humanScope // 3. the Tuwunel admin bot force-leave: last resort when the human // token is unavailable (stale password). Like the team-reconcile // usage, a confirmed command delivery is treated as resolved. + deferUnknown, knownRoomIDs := teamRoomRevocationLag(ctx, r.Client, h) kept := next[:0] for _, rid := range next { if _, ok := desired[rid]; ok { kept = append(kept, rid) continue } + if deferUnknown { + if _, known := knownRoomIDs[rid]; !known { + // Origin unresolved while the human holds a team membership + // claim whose room is not yet visible: this may be that + // team's brand-new room (status-lag window right after + // team provisioning). Defer the kick to the next cycle + // instead of evicting the team admin from their own team + // room — that deadlock would surface as join-403 on every + // later team reconcile (CI test-19). + logger.V(1).Info("deferring kick: room origin unresolved, team room claim pending", "room", rid) + kept = append(kept, rid) + continue + } + } if err := r.Provisioner.KickFromRoom(ctx, rid, matrixUserID, "access revoked"); err == nil { continue // kicked, or the user was already out } else { diff --git a/agentteams-controller/internal/controller/human_scope.go b/agentteams-controller/internal/controller/human_scope.go index 858779d2f..78818660a 100644 --- a/agentteams-controller/internal/controller/human_scope.go +++ b/agentteams-controller/internal/controller/human_scope.go @@ -125,3 +125,61 @@ func buildDesiredHumanRooms(ctx context.Context, c client.Client, h *v1beta1.Hum } return desired } + +// teamRoomRevocationLag reports whether the revocation path must defer +// kicks of rooms whose origin cannot currently be resolved, and returns +// the set of room IDs currently visible in the cache (team +// Status.TeamRoomID + worker Status.RoomID). +// +// Right after team provisioning, syncTeamRoomHumanStatuses writes the new +// team room into the admin's / human members' Status.Rooms BEFORE the +// team's Status.TeamRoomID is visible in this reconciler's cache +// (informer lag across objects). While that window is open, a room in +// Status.Rooms that no visible Team/Worker claims has an UNKNOWN origin: +// it is the new team's room, not an orphan. Kicking it would evict the +// team admin from their own team room — and, because the admin is +// deliberately excluded from the team-room invite list (creator-join +// design in ProvisionTeamRooms), every later team reconcile would then +// fail on join (M_FORBIDDEN: cannot join a room that is not public): a +// permanent deadlock (CI test-19). So while the human holds a team +// membership claim (spec.admin / spec.humanMembers) against a team whose +// room is not yet visible, unknown-origin rooms are kept for one more +// cycle. By then the team status is visible and the origin resolves: +// still belonging -> the room is desired (kept); genuinely revoked -> +// kicked as usual. Known-origin rooms (visible team/worker rooms the +// human no longer belongs to) are kicked immediately, even inside the +// window, so access revocation stays prompt. +func teamRoomRevocationLag(ctx context.Context, c client.Client, h *v1beta1.Human) (deferUnknown bool, knownRoomIDs map[string]struct{}) { + knownRoomIDs = make(map[string]struct{}) + var teams v1beta1.TeamList + if err := c.List(ctx, &teams, client.InNamespace(h.Namespace)); err != nil { + return false, knownRoomIDs + } + unresolvedClaim := false + for i := range teams.Items { + tm := &teams.Items[i] + if tm.Status.TeamRoomID != "" { + knownRoomIDs[tm.Status.TeamRoomID] = struct{}{} + continue + } + if tm.Spec.Admin != nil && tm.Spec.Admin.Name == h.Name { + unresolvedClaim = true + } else { + for _, m := range tm.Spec.HumanMembers { + if m.Name == h.Name || (m.MatrixUserID != "" && m.MatrixUserID == h.Status.MatrixUserID) { + unresolvedClaim = true + break + } + } + } + } + var workers v1beta1.WorkerList + if err := c.List(ctx, &workers, client.InNamespace(h.Namespace)); err == nil { + for i := range workers.Items { + if workers.Items[i].Status.RoomID != "" { + knownRoomIDs[workers.Items[i].Status.RoomID] = struct{}{} + } + } + } + return unresolvedClaim, knownRoomIDs +} diff --git a/docs/design/room-power-levels.md b/docs/design/room-power-levels.md index c506185f9..04ce9e160 100644 --- a/docs/design/room-power-levels.md +++ b/docs/design/room-power-levels.md @@ -150,6 +150,27 @@ Consequences implemented by this PR: creator-join design) on every reconcile: a permanent deadlock. Regression tests: `TestHumanReconciler_TeamAdminRoomNotRevoked` / `TestHumanReconciler_HumanMemberRoomNotRevoked`. +7. **The revocation path never kicks a room whose origin it cannot + resolve while a team membership claim is pending.** Item 6 closes the + steady-state case (room visible in the team status). A residual + status-lag window remained: right after team provisioning, + `syncTeamRoomHumanStatuses` writes the new team room into the admin's + `status.rooms` BEFORE the team's `status.teamRoomID` is visible in the + human reconciler's cache (informer lag across objects). In that window + the room is in `status.rooms` but no visible Team/Worker claims it — + an UNKNOWN origin — and the revocation path kicked it anyway, + evicting the team admin from their own team room; every later team + reconcile then failed on join (`M_FORBIDDEN: cannot join a room that + is not public`) until the 180s test-19 timeout (CI SHARD_C 4/5). + `teamRoomRevocationLag` detects the window (human is `spec.admin` / + `spec.humanMembers` of a team whose room is not yet visible) and + defers the kick of unknown-origin rooms for one cycle — by then the + team status is visible and the origin resolves: still belonging → + desired (kept), genuinely revoked → kicked. Known-origin revocations + (visible team/worker rooms the human no longer belongs to) stay + prompt. Regression tests: + `TestHumanReconciler_RevocationDeferredWhileTeamRoomUnresolved` / + `TestHumanReconciler_RevocationProceedsForKnownOriginTeamRoom`. Known limitations (documented, all non-fatal / retry or documented-stuck): @@ -229,4 +250,10 @@ Known limitations (documented, all non-fatal / retry or documented-stuck): `TestHumanReconciler_RevocationSelfLeaveFallback` (kick rejected → self-leave with the human token → room dropped), `TestHumanReconciler_RevocationForceLeaveLastResort` (stale password, - no self token → admin-bot force-leave → room dropped). + no self token → admin-bot force-leave → room dropped), + `TestHumanReconciler_RevocationDeferredWhileTeamRoomUnresolved` + (team names the human as admin, room in `status.rooms`, team + `status.teamRoomID` not yet visible → kick deferred, room kept), + `TestHumanReconciler_RevocationProceedsForKnownOriginTeamRoom` + (claim pending on another team, but the revoked room's origin IS + visible → kick proceeds immediately).