From 6bf6eabc2e9ca205a87e5ada5c60a5353aabcf4d Mon Sep 17 00:00:00 2001 From: pikann22 Date: Fri, 23 Oct 2026 03:54:35 +0000 Subject: [PATCH 1/2] feat: refactor notification handling to use AccountID and enhance IAM principal mapping for members --- .../internal/service/notification/service.go | 8 ++++---- .../service/notification/service_test.go | 3 +++ .../transport/http/handler/project_handler.go | 12 ++++++++---- .../transport/http/middleware/authz_attrs.go | 11 +++++------ .../internal/transport/triggergate/gate.go | 19 +++++++++++++++---- .../transport/triggergate/gate_test.go | 13 ++++++++----- 6 files changed, 43 insertions(+), 23 deletions(-) diff --git a/services/api/internal/service/notification/service.go b/services/api/internal/service/notification/service.go index c3976133f..e1a72d34e 100644 --- a/services/api/internal/service/notification/service.go +++ b/services/api/internal/service/notification/service.go @@ -206,7 +206,7 @@ func (s *Svc) NotifyAssigned(ctx context.Context, in notificationdom.NotifyAssig } // Skip self-assignment (actor is the same user as the new assignee). - if assigneeMember.UserID == in.ActorID { + if assigneeMember.AccountID == in.ActorID { return nil } @@ -221,7 +221,7 @@ func (s *Svc) NotifyAssigned(ctx context.Context, in notificationdom.NotifyAssig n := ¬ificationdom.Notification{ ID: uuid.New(), - RecipientAccountID: assigneeMember.UserID, + RecipientAccountID: assigneeMember.AccountID, Type: notificationdom.NotificationTypeAssigned, ProjectID: in.ProjectID, CreatedAt: time.Now(), @@ -236,13 +236,13 @@ func (s *Svc) NotifyAssigned(ctx context.Context, in notificationdom.NotifyAssig if err := s.repo.Create(ctx, n); err != nil { return err } - s.publishCreated(ctx, n, assigneeMember.UserID) + s.publishCreated(ctx, n, assigneeMember.AccountID) actorName := "" if actorMember != nil { actorName = actorMember.FullName } - s.publishNotificationEvent(ctx, events.TopicNotificationAssigned, assigneeMember.UserID, actorName, s.taskURL(in.ProjectID, taskID), s.taskTitle(ctx, taskID)) + s.publishNotificationEvent(ctx, events.TopicNotificationAssigned, assigneeMember.AccountID, actorName, s.taskURL(in.ProjectID, taskID), s.taskTitle(ctx, taskID)) return nil } diff --git a/services/api/internal/service/notification/service_test.go b/services/api/internal/service/notification/service_test.go index 18d920ed9..a7345bbd5 100644 --- a/services/api/internal/service/notification/service_test.go +++ b/services/api/internal/service/notification/service_test.go @@ -141,6 +141,9 @@ func (r *fakeMemberRepo) add(m *projectdom.ProjectMember) { r.mu.Lock() defer r.mu.Unlock() cp := *m + if cp.AccountID == uuid.Nil { + cp.AccountID = cp.UserID // a human member's account is its user + } r.byID[m.ID] = &cp r.byProj[m.ProjectID] = append(r.byProj[m.ProjectID], &cp) if m.AgentID != nil { diff --git a/services/api/internal/transport/http/handler/project_handler.go b/services/api/internal/transport/http/handler/project_handler.go index 46c407b6b..4cbfd6dda 100644 --- a/services/api/internal/transport/http/handler/project_handler.go +++ b/services/api/internal/transport/http/handler/project_handler.go @@ -195,7 +195,7 @@ func (h *ProjectHandler) ListProjects(w http.ResponseWriter, r *http.Request) { return } - hasGlobalRead, authzErr := h.hasPlatformProjectsRead(r, accountID) + hasGlobalRead, authzErr := h.hasPlatformProjectsRead(r) if authzErr != nil { presenter.Error(w, r, authzErr) return @@ -221,11 +221,15 @@ func (h *ProjectHandler) ListProjects(w http.ResponseWriter, r *http.Request) { // hasPlatformProjectsRead reports whether the caller may read every project: // projects:read on the project collection "project" (the platform-wide // projects.read). Judged as the token's subject, as before. -func (h *ProjectHandler) hasPlatformProjectsRead(r *http.Request, accountID uuid.UUID) (bool, error) { +func (h *ProjectHandler) hasPlatformProjectsRead(r *http.Request) (bool, error) { if h.authorizer == nil { return false, nil } - return iam.AllowedAll(r.Context(), h.authorizer, iam.User(accountID.String()), "project", iam.ActionProjectsRead) + principal, err := middleware.IAMPrincipalFrom(r) + if err != nil { + return false, err + } + return iam.AllowedAll(r.Context(), h.authorizer, principal, "project", iam.ActionProjectsRead) } // GetWorkspaceStats handles GET /projects/workspace-stats. @@ -240,7 +244,7 @@ func (h *ProjectHandler) GetWorkspaceStats(w http.ResponseWriter, r *http.Reques return } - hasGlobalRead, authzErr := h.hasPlatformProjectsRead(r, accountID) + hasGlobalRead, authzErr := h.hasPlatformProjectsRead(r) if authzErr != nil { presenter.Error(w, r, authzErr) return diff --git a/services/api/internal/transport/http/middleware/authz_attrs.go b/services/api/internal/transport/http/middleware/authz_attrs.go index 7fa42c17a..0a5f8c6be 100644 --- a/services/api/internal/transport/http/middleware/authz_attrs.go +++ b/services/api/internal/transport/http/middleware/authz_attrs.go @@ -193,14 +193,13 @@ func (l MemberRepoLookup) MemberPrincipalID(ctx context.Context, memberID uuid.U if err != nil { return "", err } - if m.AgentID != nil { - return m.AgentID.String(), nil - } - return m.UserID.String(), nil + // Policies key every principal kind (user, agent, service account) by + // account id; UserID is only set for human members. + return m.AccountID.String(), nil } -// MemberPrincipalLookup maps a project member id to the id of the user or -// agent behind it (what a policy's principal.id and task.assignee_id hold). An +// MemberPrincipalLookup maps a project member id to the id of the account +// (user, agent or service account) behind it (what a policy's principal.id and task.assignee_id hold). An // unknown member is an error: the request names someone who does not exist, and // the handler would reject it too. type MemberPrincipalLookup interface { diff --git a/services/api/internal/transport/triggergate/gate.go b/services/api/internal/transport/triggergate/gate.go index d09a1fdb4..21aa7cf50 100644 --- a/services/api/internal/transport/triggergate/gate.go +++ b/services/api/internal/transport/triggergate/gate.go @@ -80,10 +80,7 @@ func (g *Gate) allow(ctx context.Context, projectID, agentID uuid.UUID, memberID if member.ProjectID != projectID { return errForbidden } - principal := iam.User(member.UserID.String()) - if member.MemberType == "agent" && member.AgentID != nil { - principal = iam.Agent(member.AgentID.String()) - } + principal := principalOf(member) project := iam.ProjectResource(projectID.String()) ok, err := iam.AllowedAll(ctx, g.authz, principal, project+"/agent/"+agentID.String(), iam.ActionConversationsWrite) if err != nil { @@ -109,6 +106,20 @@ func (g *Gate) allow(ctx context.Context, projectID, agentID uuid.UUID, memberID return nil } +// principalOf is the member's account as an IAM principal. Grants are keyed +// by account id for every kind (user, agent, service account); member.UserID +// is only set for human members, so it must not be used here. +func principalOf(m *projectdom.ProjectMember) iam.Principal { + kind := iam.PrincipalUser + switch m.MemberType { + case "agent": + kind = iam.PrincipalAgent + case "service_account": + kind = iam.PrincipalServiceAccount + } + return iam.Principal{Type: kind, ID: m.AccountID.String()} +} + func memberOrNil(id *uuid.UUID) uuid.UUID { if id == nil { return uuid.Nil diff --git a/services/api/internal/transport/triggergate/gate_test.go b/services/api/internal/transport/triggergate/gate_test.go index 19511cd9c..f7e9a7e34 100644 --- a/services/api/internal/transport/triggergate/gate_test.go +++ b/services/api/internal/transport/triggergate/gate_test.go @@ -74,13 +74,14 @@ func deny(action, resource string) iam.Statement { func TestGate(t *testing.T) { project, agent, env := uuid.New(), uuid.New(), uuid.New() - userMember, agentMember := uuid.New(), uuid.New() - user, bot := uuid.New(), uuid.New() - u, a := iam.User(user.String()), iam.Agent(bot.String()) + userMember, agentMember, saMember := uuid.New(), uuid.New(), uuid.New() + user, bot, sa := uuid.New(), uuid.New(), uuid.New() + u, a, s := iam.User(user.String()), iam.Agent(bot.String()), iam.Principal{Type: iam.PrincipalServiceAccount, ID: sa.String()} p := "project/" + project.String() roster := members{ - userMember: {ID: userMember, ProjectID: project, UserID: user, MemberType: "human"}, - agentMember: {ID: agentMember, ProjectID: project, MemberType: "agent", AgentID: &bot}, + userMember: {ID: userMember, ProjectID: project, AccountID: user, UserID: user, MemberType: "human"}, + agentMember: {ID: agentMember, ProjectID: project, AccountID: bot, MemberType: "agent", AgentID: &bot}, + saMember: {ID: saMember, ProjectID: project, AccountID: sa, MemberType: "service_account"}, } type tc struct { @@ -100,6 +101,8 @@ func TestGate(t *testing.T) { {name: "Deny on the environment is irrelevant when the agent has no environment", grants: map[iam.Principal][]iam.Grant{u: {grant(project, allowUse(project), deny("environments:read", p+"/environment/"+env.String()+"/*"))}}, env: nil, member: userMember, wantRun: true}, {name: "agent member judged by its own grants", grants: map[iam.Principal][]iam.Grant{u: {grant(project, allowUse(project))}}, env: &env, member: agentMember, wantForbd: true}, {name: "agent member with its own grant proceeds", grants: map[iam.Principal][]iam.Grant{a: {grant(project, allowUse(project))}}, env: &env, member: agentMember, wantRun: true}, + {name: "service account member with its own grant proceeds", grants: map[iam.Principal][]iam.Grant{s: {grant(project, allowUse(project))}}, env: &env, member: saMember, wantRun: true}, + {name: "service account member without a grant is denied", grants: map[iam.Principal][]iam.Grant{u: {grant(project, allowUse(project))}}, env: &env, member: saMember, wantForbd: true}, {name: "authorizer error denies", err: errors.New("db down"), env: &env, member: userMember}, {name: "unknown member denies", grants: map[iam.Principal][]iam.Grant{u: {grant(project, allowUse(project))}}, env: &env, member: uuid.New()}, {name: "no actor is not checked (even with a failing authorizer)", err: errors.New("never consulted"), env: &env, member: uuid.Nil, wantRun: true}, From 2a636fe5cca403663faf1ff773e52f0b1f3575db Mon Sep 17 00:00:00 2001 From: pikann22 Date: Fri, 23 Oct 2026 04:02:40 +0000 Subject: [PATCH 2/2] feat: enhance IAM principal mapping by skipping notifications for agents and service accounts --- .../internal/service/notification/service.go | 6 +++ .../service/notification/service_test.go | 25 +++++++++ .../http/handler/project_handler_test.go | 52 +++++++++++++++++++ .../transport/http/middleware/authz_attrs.go | 7 +-- .../http/middleware/authz_attrs_test.go | 33 ++++++++++++ .../internal/transport/triggergate/gate.go | 4 ++ .../transport/triggergate/gate_test.go | 3 ++ 7 files changed, 127 insertions(+), 3 deletions(-) diff --git a/services/api/internal/service/notification/service.go b/services/api/internal/service/notification/service.go index e1a72d34e..3ec671724 100644 --- a/services/api/internal/service/notification/service.go +++ b/services/api/internal/service/notification/service.go @@ -205,6 +205,12 @@ func (s *Svc) NotifyAssigned(ctx context.Context, in notificationdom.NotifyAssig return nil } + // Agents and service accounts have no inbox, and NotifyMentioned skips + // them too, so only human assignees are notified. + if assigneeMember.MemberType == "agent" || assigneeMember.MemberType == "service_account" { + return nil + } + // Skip self-assignment (actor is the same user as the new assignee). if assigneeMember.AccountID == in.ActorID { return nil diff --git a/services/api/internal/service/notification/service_test.go b/services/api/internal/service/notification/service_test.go index a7345bbd5..2f40bb233 100644 --- a/services/api/internal/service/notification/service_test.go +++ b/services/api/internal/service/notification/service_test.go @@ -429,6 +429,31 @@ func TestNotifyAssigned_AgentActor_ResolvesAgentMember(t *testing.T) { } } +func TestNotifyAssigned_BotAssigneeSkipped(t *testing.T) { + for _, memberType := range []string{"agent", "service_account"} { + t.Run(memberType, func(t *testing.T) { + repo := newFakeNotificationRepo() + members := newFakeMemberRepo() + svc := New(repo, members, nil) + + projectID, accountID, memberID := uuid.New(), uuid.New(), uuid.New() + members.add(&projectdom.ProjectMember{ + ID: memberID, ProjectID: projectID, AccountID: accountID, MemberType: memberType, + }) + + err := svc.NotifyAssigned(context.Background(), notificationdom.NotifyAssignedInput{ + TaskID: uuid.New(), ProjectID: projectID, NewAssigneeMemberID: memberID, ActorID: uuid.New(), + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got := countNotifications(repo, accountID) + countNotifications(repo, uuid.Nil); got != 0 { + t.Fatalf("expected no notification for a %s assignee, got %d", memberType, got) + } + }) + } +} + func TestNotifyAssigned_SelfAssignmentSuppressed(t *testing.T) { ctx := context.Background() repo := newFakeNotificationRepo() diff --git a/services/api/internal/transport/http/handler/project_handler_test.go b/services/api/internal/transport/http/handler/project_handler_test.go index 4df228953..4cadae605 100644 --- a/services/api/internal/transport/http/handler/project_handler_test.go +++ b/services/api/internal/transport/http/handler/project_handler_test.go @@ -16,6 +16,7 @@ import ( "github.com/golang-jwt/jwt/v5" "github.com/google/uuid" + accountdom "github.com/Paca-AI/api/internal/domain/account" attachmentdom "github.com/Paca-AI/api/internal/domain/attachment" domainauth "github.com/Paca-AI/api/internal/domain/auth" projectdom "github.com/Paca-AI/api/internal/domain/project" @@ -1196,3 +1197,54 @@ func assertSameProjectIDs(t *testing.T, got []uuid.UUID, want []uuid.UUID) { } } } + +// agentOnlyStore grants projects:read on the project collection to agent +// principals only, so the answer shows which principal kind the handler judged. +type agentOnlyStore struct{} + +func (agentOnlyStore) ListGrants(_ context.Context, p iam.Principal) ([]iam.Grant, error) { + if p.Type != iam.PrincipalAgent { + return nil, nil + } + return (&fakeGlobalPermStore{globalPerms: []iam.Action{iam.ActionProjectsRead}}).ListGrants(context.Background(), p) +} + +func TestListProjects_JudgesRealPrincipalKind(t *testing.T) { + for _, tc := range []struct { + kind accountdom.Kind + wantGlobal bool + }{ + {accountdom.KindAgent, true}, + {accountdom.KindUser, false}, + {accountdom.KindServiceAccount, false}, + } { + t.Run(string(tc.kind), func(t *testing.T) { + var gotGlobal bool + svc := &mockProjectSvc{ + list: func(context.Context, int, int) ([]*projectdom.Project, int64, error) { + gotGlobal = true + return nil, 0, nil + }, + listAccessible: func(context.Context, uuid.UUID, int, int) ([]*projectdom.Project, int64, error) { + return nil, 0, nil + }, + } + h := handler.NewProjectHandler(svc, iam.NewAuthorizer(agentOnlyStore{}, iam.NewRegistry(), iam.NewAttributeSchema())) + r := chi.NewRouter() + r.Use(func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + ctx := httpmw.WithPrincipal(r.Context(), httpmw.Principal{AccountID: uuid.New(), Kind: tc.kind}) + next.ServeHTTP(w, r.WithContext(ctx)) + }) + }) + r.Get("/projects", h.ListProjects) + + if w := do(t, r, http.MethodGet, "/projects", nil); w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if gotGlobal != tc.wantGlobal { + t.Errorf("global read = %v, want %v", gotGlobal, tc.wantGlobal) + } + }) + } +} diff --git a/services/api/internal/transport/http/middleware/authz_attrs.go b/services/api/internal/transport/http/middleware/authz_attrs.go index 0a5f8c6be..425fab85d 100644 --- a/services/api/internal/transport/http/middleware/authz_attrs.go +++ b/services/api/internal/transport/http/middleware/authz_attrs.go @@ -199,9 +199,10 @@ func (l MemberRepoLookup) MemberPrincipalID(ctx context.Context, memberID uuid.U } // MemberPrincipalLookup maps a project member id to the id of the account -// (user, agent or service account) behind it (what a policy's principal.id and task.assignee_id hold). An -// unknown member is an error: the request names someone who does not exist, and -// the handler would reject it too. +// (user, agent or service account) behind it, which is what a policy's +// principal.id and task.assignee_id hold. An unknown member is an error: the +// request names someone who does not exist, and the handler would reject it +// too. type MemberPrincipalLookup interface { MemberPrincipalID(ctx context.Context, memberID uuid.UUID) (string, error) } diff --git a/services/api/internal/transport/http/middleware/authz_attrs_test.go b/services/api/internal/transport/http/middleware/authz_attrs_test.go index 2e49b99db..6e10531f0 100644 --- a/services/api/internal/transport/http/middleware/authz_attrs_test.go +++ b/services/api/internal/transport/http/middleware/authz_attrs_test.go @@ -11,6 +11,7 @@ import ( "github.com/go-chi/chi/v5" "github.com/google/uuid" + projectdom "github.com/Paca-AI/api/internal/domain/project" taskdom "github.com/Paca-AI/api/internal/domain/task" "github.com/Paca-AI/api/internal/platform/authz/iam" ) @@ -263,3 +264,35 @@ func TestViewCreateAndReorderGates(t *testing.T) { }) } } + +// stubMemberRepo serves one member; the embedded interface is never reached. +type stubMemberRepo struct { + projectdom.MemberRepository + m *projectdom.ProjectMember +} + +func (s stubMemberRepo) FindMemberByID(context.Context, uuid.UUID) (*projectdom.ProjectMember, error) { + return s.m, nil +} + +func TestMemberRepoLookup_UsesAccountID(t *testing.T) { + account, user := uuid.New(), uuid.New() + for _, tc := range []struct { + name string + m *projectdom.ProjectMember + }{ + {"human", &projectdom.ProjectMember{AccountID: account, UserID: account, MemberType: "human"}}, + {"agent", &projectdom.ProjectMember{AccountID: account, AgentID: &user, MemberType: "agent"}}, + {"service account (no UserID)", &projectdom.ProjectMember{AccountID: account, MemberType: "service_account"}}, + } { + t.Run(tc.name, func(t *testing.T) { + got, err := MemberRepoLookup{Repo: stubMemberRepo{m: tc.m}}.MemberPrincipalID(t.Context(), uuid.New()) + if err != nil { + t.Fatal(err) + } + if got != account.String() { + t.Errorf("got %s, want account id %s", got, account) + } + }) + } +} diff --git a/services/api/internal/transport/triggergate/gate.go b/services/api/internal/transport/triggergate/gate.go index 21aa7cf50..f176576ab 100644 --- a/services/api/internal/transport/triggergate/gate.go +++ b/services/api/internal/transport/triggergate/gate.go @@ -80,6 +80,10 @@ func (g *Gate) allow(ctx context.Context, projectID, agentID uuid.UUID, memberID if member.ProjectID != projectID { return errForbidden } + if member.AccountID == uuid.Nil { + // No account to judge: fail closed rather than check a nil-UUID principal. + return errForbidden + } principal := principalOf(member) project := iam.ProjectResource(projectID.String()) ok, err := iam.AllowedAll(ctx, g.authz, principal, project+"/agent/"+agentID.String(), iam.ActionConversationsWrite) diff --git a/services/api/internal/transport/triggergate/gate_test.go b/services/api/internal/transport/triggergate/gate_test.go index f7e9a7e34..ad5f4bcd7 100644 --- a/services/api/internal/transport/triggergate/gate_test.go +++ b/services/api/internal/transport/triggergate/gate_test.go @@ -78,7 +78,9 @@ func TestGate(t *testing.T) { user, bot, sa := uuid.New(), uuid.New(), uuid.New() u, a, s := iam.User(user.String()), iam.Agent(bot.String()), iam.Principal{Type: iam.PrincipalServiceAccount, ID: sa.String()} p := "project/" + project.String() + noAccount := uuid.New() roster := members{ + noAccount: {ID: noAccount, ProjectID: project, UserID: user, MemberType: "human"}, userMember: {ID: userMember, ProjectID: project, AccountID: user, UserID: user, MemberType: "human"}, agentMember: {ID: agentMember, ProjectID: project, AccountID: bot, MemberType: "agent", AgentID: &bot}, saMember: {ID: saMember, ProjectID: project, AccountID: sa, MemberType: "service_account"}, @@ -103,6 +105,7 @@ func TestGate(t *testing.T) { {name: "agent member with its own grant proceeds", grants: map[iam.Principal][]iam.Grant{a: {grant(project, allowUse(project))}}, env: &env, member: agentMember, wantRun: true}, {name: "service account member with its own grant proceeds", grants: map[iam.Principal][]iam.Grant{s: {grant(project, allowUse(project))}}, env: &env, member: saMember, wantRun: true}, {name: "service account member without a grant is denied", grants: map[iam.Principal][]iam.Grant{u: {grant(project, allowUse(project))}}, env: &env, member: saMember, wantForbd: true}, + {name: "member without an account id is denied", grants: map[iam.Principal][]iam.Grant{u: {grant(project, allowUse(project))}}, env: &env, member: noAccount, wantForbd: true}, {name: "authorizer error denies", err: errors.New("db down"), env: &env, member: userMember}, {name: "unknown member denies", grants: map[iam.Principal][]iam.Grant{u: {grant(project, allowUse(project))}}, env: &env, member: uuid.New()}, {name: "no actor is not checked (even with a failing authorizer)", err: errors.New("never consulted"), env: &env, member: uuid.Nil, wantRun: true},