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..241dd2852 100644 --- a/agentteams-controller/internal/controller/human_controller_test.go +++ b/agentteams-controller/internal/controller/human_controller_test.go @@ -3,7 +3,9 @@ package controller import ( "context" "errors" + "fmt" "sort" + "sync/atomic" "testing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -14,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" ) @@ -341,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 @@ -635,3 +714,378 @@ 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, actorToken, selfToken string) 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) + } +} + +// 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) + } +} + +// 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 fabc61475..05e4bb54f 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" ) @@ -44,48 +47,173 @@ 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...) - for rid := range desired { + powerLevel := humanRoomPowerLevel(h.Spec.PermissionLevel) + + for rid, origin := 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. + // + // 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 err := r.Provisioner.JoinRoomAs(ctx, rid, token); err != nil { - logger.Error(err, "failed to join room as human", "room", rid) - continue + if grantErr != nil { + logger.Error(grantErr, "failed to ensure human power level", "room", rid, "level", powerLevel) } - next = append(next, rid) } - // 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. + 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 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 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 { + 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 +// 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/controller/human_scope.go b/agentteams-controller/internal/controller/human_scope.go index b693b313f..78818660a 100644 --- a/agentteams-controller/internal/controller/human_scope.go +++ b/agentteams-controller/internal/controller/human_scope.go @@ -56,34 +56,130 @@ 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. +// 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. // -// 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} + } + } + // 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} + } } } - 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 { + 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 team.Status.TeamRoomID != "" { - desired[team.Status.TeamRoomID] = struct{}{} + 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 + } + } } } - return desired + 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/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 2b13a7f45..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" @@ -25,6 +26,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 { @@ -60,6 +102,16 @@ 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 + // (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 @@ -139,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. @@ -175,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 @@ -182,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: + // 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. @@ -196,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) } @@ -263,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), @@ -286,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 @@ -299,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, @@ -314,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{ @@ -337,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 } @@ -375,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") @@ -413,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{ @@ -441,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 } @@ -449,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 @@ -747,12 +900,48 @@ 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, 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 + // 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: %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 + // 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, @@ -784,7 +973,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 } @@ -1006,19 +1199,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 204a0f97f..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" @@ -548,6 +549,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) { @@ -1052,3 +1105,383 @@ 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) + } +} + +// 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/interfaces.go b/agentteams-controller/internal/service/interfaces.go index 54ba1ac2f..8aa13ba7a 100644 --- a/agentteams-controller/internal/service/interfaces.go +++ b/agentteams-controller/internal/service/interfaces.go @@ -57,6 +57,27 @@ 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). 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 @@ -235,9 +256,39 @@ 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). 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, 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 // 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..2446b7457 100644 --- a/agentteams-controller/internal/service/provisioner.go +++ b/agentteams-controller/internal/service/provisioner.go @@ -1074,6 +1074,72 @@ 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). 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. +// +// 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) + } + // 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 + 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` // (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_human.go b/agentteams-controller/internal/service/provisioner_human.go index 256716467..0c00a1d7c 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 @@ -199,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_power_test.go b/agentteams-controller/internal/service/provisioner_power_test.go new file mode 100644 index 000000000..30afe8136 --- /dev/null +++ b/agentteams-controller/internal/service/provisioner_power_test.go @@ -0,0 +1,371 @@ +package service + +import ( + "context" + "errors" + "testing" + + "github.com/agentscope-ai/AgentTeams/agentteams-controller/internal/matrix" +) + +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. +// 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{}{ + "@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} + 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, "", "alice-token"); err != nil { + t.Fatalf("EnsureRoomPowerLevel: %v", err) + } + 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 { + 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) + } +} + +// 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}, + } + 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..1fd62d931 100644 --- a/agentteams-controller/internal/service/provisioner_team_test.go +++ b/agentteams-controller/internal/service/provisioner_team_test.go @@ -2,9 +2,11 @@ package service import ( "context" + "encoding/json" "errors" "reflect" "sort" + "strconv" "strings" "testing" "time" @@ -31,6 +33,29 @@ 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 + + // --- 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 { @@ -58,6 +83,7 @@ func newFakeTeamMatrix() *fakeTeamMatrix { listErrs: make(map[string]error), tokenUsers: make(map[string]string), created: true, + adminUser: "@admin:localhost", } } @@ -85,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"}) @@ -118,9 +156,64 @@ func (f *fakeTeamMatrix) SetRoomState(_ context.Context, roomID, eventType, stat content: content, 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) + 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, 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 + } + if f.powerStates == nil { + return nil, nil // legacy room: state never set + } + st, ok := f.powerStates[roomID] + if !ok { + return nil, 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 { f.joins = append(f.joins, roomUserCall{roomID: roomID, userID: token}) if userID := f.tokenUsers[token]; userID != "" { @@ -131,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 } @@ -140,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 { @@ -164,12 +268,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 } @@ -179,28 +289,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 } @@ -507,6 +619,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", @@ -602,6 +715,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", @@ -712,6 +826,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", @@ -740,6 +855,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, @@ -763,6 +882,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{ @@ -810,6 +937,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, @@ -828,12 +959,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) @@ -877,3 +1009,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 cfd7eab6f..3541383f3 100644 --- a/agentteams-controller/test/testutil/mocks/human_provisioner.go +++ b/agentteams-controller/test/testutil/mocks/human_provisioner.go @@ -26,13 +26,16 @@ 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 + 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 // AppServiceEnabled toggles MatrixAppServiceEnabled() — needed by // the legacy_password identity source to choose between AS and @@ -51,11 +54,26 @@ type MockHumanProvisioner struct { InviteToRoom []RoomMembershipCall JoinRoomAs []JoinRoomAsCall KickFromRoom []KickFromRoomCall + KickFromRoomAs []KickFromRoomCall + LeaveRoomAs []JoinRoomAsCall 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 + // 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. type LoginAsHumanCall struct { Name string @@ -130,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 @@ -156,8 +176,11 @@ func (m *MockHumanProvisioner) clearCallsLocked() { InviteToRoom []RoomMembershipCall JoinRoomAs []JoinRoomAsCall KickFromRoom []KickFromRoomCall + KickFromRoomAs []KickFromRoomCall + LeaveRoomAs []JoinRoomAsCall ForceLeaveRoom []ForceLeaveRoomCall DeactivateHumanUser []string + EnsureRoomPowerLevel []EnsureRoomPowerLevelCall }{} } @@ -329,6 +352,39 @@ func (m *MockHumanProvisioner) DeactivateHumanUser(ctx context.Context, userID s return nil } +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, ActorToken: actorToken, SelfToken: selfToken}) + fn := m.EnsureRoomPowerLevelFn + m.mu.Unlock() + if fn != nil { + 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 +} + 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..4f509d640 100644 --- a/agentteams-controller/test/testutil/mocks/provisioner.go +++ b/agentteams-controller/test/testutil/mocks/provisioner.go @@ -44,6 +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 + 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) @@ -85,6 +88,7 @@ type MockProvisioner struct { InviteToRoom []roomMembershipCall JoinRoomAs []joinRoomAsCall KickFromRoom []kickFromRoomCall + EnsureRoomPowerLevel []ensureRoomPowerLevelCall ForceLeaveRoom []roomMembershipCall DeactivateHumanUser []string ProvisionTeamRooms []service.TeamRoomRequest @@ -120,6 +124,14 @@ type remoteNamespaceCall struct { Namespace string } +type ensureRoomPowerLevelCall struct { + RoomID string + UserID string + Level int + ActorToken string + SelfToken string +} + type userPasswordCall struct { UserID string Password string @@ -186,6 +198,9 @@ 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 m.ProvisionTeamRoomsFn = nil @@ -232,6 +247,7 @@ func (m *MockProvisioner) clearCallsLocked() { InviteToRoom []roomMembershipCall JoinRoomAs []joinRoomAsCall KickFromRoom []kickFromRoomCall + EnsureRoomPowerLevel []ensureRoomPowerLevelCall ForceLeaveRoom []roomMembershipCall DeactivateHumanUser []string ProvisionTeamRooms []service.TeamRoomRequest @@ -617,6 +633,36 @@ func (m *MockProvisioner) KickFromRoom(ctx context.Context, roomID, userID, reas return nil } +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, ActorToken: actorToken, SelfToken: selfToken}) + fn := m.EnsureRoomPowerLevelFn + m.mu.Unlock() + if fn != nil { + return fn(ctx, roomID, userID, level, actorToken, selfToken) + } + 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..04ce9e160 --- /dev/null +++ b/docs/design/room-power-levels.md @@ -0,0 +1,259 @@ +# 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 — 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 + 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. + +## 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. +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. +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`. +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): + +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). +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). +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 + origin tracking lands in status. + +## 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. `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`, + `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 + 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), + `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), + `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). 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"