feat: add team member role for team-level permission control#1023
Merged
feat: add team member role for team-level permission control#1023
Conversation
…ontrol Cherry-pick from PR #1021 (feat/user-team-api branch). - Add `role` field to TeamMember entity/model (owner or member, default member) - Team creator is auto-added as team owner - Team write operations now require team owner, org owner, or admin - Add private API `GET /-/team/:org/:team/member` returning [{user, role}] - Add private API `PATCH /-/team/:org/:team/member/:username` for role updates - Keep npm compatible endpoints unchanged - Update docs (Chinese + English) Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Contributor
📝 WalkthroughWalkthroughThis pull request introduces a team member role system that extends the existing Team infrastructure. It adds a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TeamController
participant TeamService
participant TeamRepository
participant Database
Client->>TeamController: POST /team (createTeam with creatorUserId)
TeamController->>TeamService: createTeam(orgId, name, description, creatorUserId)
TeamService->>TeamRepository: createTeam(team)
TeamRepository->>Database: INSERT team
Database-->>TeamRepository: team_id
TeamRepository-->>TeamService: team
Note over TeamService: Creator auto-added as owner
TeamService->>TeamRepository: addMember(teamId, creatorUserId, role='owner')
TeamRepository->>Database: INSERT team_member (role='owner')
Database-->>TeamRepository: member record
TeamRepository-->>TeamService: TeamMember
TeamService-->>TeamController: Team
TeamController-->>Client: 201 Team created
sequenceDiagram
participant Client
participant TeamController
participant TeamRepository
participant TeamService
participant Database
Client->>TeamController: PATCH /-/team/:orgName/:teamName/member/:username
activate TeamController
TeamController->>TeamController: Verify admin/org-owner/team-owner
TeamController->>TeamRepository: findTeamMemberByUserId(teamId, userId)
Database-->>TeamRepository: existing member
TeamRepository-->>TeamController: TeamMember record
Note over TeamController: Validate new role, verify user exists
TeamController->>TeamService: addMember(teamId, userId, role='owner')
TeamService->>TeamRepository: addMember with existing member.id
TeamRepository->>Database: UPDATE team_member SET role='owner'
Database-->>TeamRepository: updated member
TeamRepository-->>TeamService: TeamMember
TeamService-->>TeamController: TeamMember
deactivate TeamController
TeamController-->>Client: 200 { user, role }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a role-based access control (RBAC) system for teams, adding 'owner' and 'member' roles to team memberships. Key changes include updating the TeamMember entity and database schema, modifying the TeamService to handle role assignments (automatically making creators 'owners'), and implementing new private API endpoints in the TeamController for role management. The permission logic has been refactored to allow Admins, Org Owners, and Team Owners to perform write operations, and comprehensive documentation and tests have been added. Feedback focuses on improving the clarity of error messages regarding authorized roles and ensuring consistency when listing team members by filtering out records for missing users.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1023 +/- ##
==========================================
+ Coverage 94.12% 94.30% +0.17%
==========================================
Files 211 211
Lines 8619 8675 +56
Branches 1701 1690 -11
==========================================
+ Hits 8113 8181 +68
+ Misses 506 494 -12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a role (owner / member) to team membership so team-level write operations can be restricted to team owners (plus org owners/admin), while keeping npm-compatible endpoints stable and introducing private endpoints for role-aware membership management.
Changes:
- Introduces
TeamMember.rolewith DB migrations (MySQL/PostgreSQL) and updates entity/model/service/repository layers accordingly. - Tightens authorization for team write operations via team-owner checks and auto-adds the team creator as an owner.
- Adds private APIs to list members with roles and to update a member’s role; expands tests and updates docs (ZH + new EN doc).
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
app/core/entity/TeamMember.ts |
Adds role field to the TeamMember domain entity. |
app/repository/model/TeamMember.ts |
Persists role on the TeamMember ORM model. |
sql/mysql/4.32.0.sql |
Adds role column to team_members (MySQL). |
sql/postgresql/4.32.0.sql |
Adds role column to team_members (PostgreSQL). |
app/core/service/TeamService.ts |
Auto-add creator as owner; supports role updates via addMember(). |
app/core/service/OrgService.ts |
Ensures developers-team membership uses explicit team roles on org creation/member add. |
app/repository/TeamRepository.ts |
Returns team role from membership in listTeamsByUserIdAndOrgId(); supports member updates when id exists. |
app/port/controller/TeamController.ts |
Adds/uses requireTeamWriteAccess() with team-owner checks; adds private member-role endpoints. |
app/port/controller/OrgController.ts |
Includes role in “list user’s teams in org” response. |
test/port/controller/TeamController/index.test.ts |
Expands test coverage for role behavior, private endpoints, and permission paths. |
test/port/controller/package/ReadAccessAuth.test.ts |
Updates fixture setup to include TeamMember.role. |
docs/org-team.md |
Updates Chinese docs to describe npm-compatible vs private APIs and role semantics. |
docs/org-team.en.md |
Adds English documentation covering the same protocol/role behavior and endpoint tables. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/core/service/TeamService.ts (2)
22-39:⚠️ Potential issue | 🟠 MajorMake the team + creator-owner write atomic.
createTeam()now owns a two-table invariant, butsaveTeam()commits before the owner row is inserted. If the second write fails, the request errors while the team remains persisted, and retries can only hit "already exists" with no owner attached. You already handle the delete-side invariant atomically viaremoveTeamCascade().💡 One way to keep the invariant atomic
- await this.teamRepository.saveTeam(team); - - // Auto-add creator as team owner - if (creatorUserId) { - const member = TeamMember.create({ teamId: team.teamId, userId: creatorUserId, role: 'owner' }); - await this.teamRepository.addMember(member); - } + await this.teamRepository.createTeamWithCreator(team, creatorUserId);
createTeamWithCreator()should wrap both inserts in the same transaction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/core/service/TeamService.ts` around lines 22 - 39, createTeam currently persists the Team via teamRepository.saveTeam and then separately adds the creator via teamRepository.addMember, which can leave a team without an owner if the second write fails; wrap both operations in a single DB transaction so they commit or rollback together. In TeamService.createTeam (or by adding a new createTeamWithCreator called from createTeam when creatorUserId is present) obtain a transactional context from the repository/ORM (e.g., a transaction callback, EntityManager, or teamRepository.runInTransaction), then call teamRepository.saveTeam(transactionalContext, team) and teamRepository.addMember(transactionalContext, member) inside that transaction; ensure failures roll back and propagate the error, and keep removeTeamCascade behavior unchanged.
64-90:⚠️ Potential issue | 🟠 MajorKeep the npm-compatible add-member path idempotent.
Line 267 in
app/port/controller/TeamController.tscalls this without a role forPUT /-/team/:orgName/:teamName/user. With the new default'member', re-adding an existing owner now demotes them tomemberinstead of being a no-op, and that can leave the team with zero owners. Only mutateexisting.rolewhen a caller explicitly requested a role change, and protect the last-owner path here and inremoveMember().🛡️ Safer split between add and role-change
- async addMember(teamId: string, userId: string, role: 'owner' | 'member' = 'member'): Promise<TeamMember> { + async addMember(teamId: string, userId: string, role?: 'owner' | 'member'): Promise<TeamMember> { @@ const existing = await this.teamRepository.findMember(teamId, userId); if (existing) { - // Update role if changed - if (existing.role !== role) { + if (role && existing.role !== role) { + await this.ensureOwnerWillRemain(teamId, existing, role); existing.role = role; await this.teamRepository.addMember(existing); } return existing; } - const member = TeamMember.create({ teamId, userId, role }); + const member = TeamMember.create({ teamId, userId, role: role ?? 'member' });Call the same
ensureOwnerWillRemain()guard fromremoveMember()so the last owner cannot be removed either.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/core/service/TeamService.ts` around lines 64 - 90, The addMember path is currently idempotent-breaking because the function signature default role = 'member' will demote an existing owner when callers omit role; change addMember to accept role as optional (role?: 'owner' | 'member') and only mutate existing.role when role is explicitly provided and different, calling the existing ensureOwnerWillRemain guard before demoting an owner; also call ensureOwnerWillRemain from removeMember to protect the last-owner path. Locate addMember, removeMember, teamRepository.findMember and teamRepository.addMember and update the logic so implicit calls (no role passed) are no-ops for existing members while explicit role changes use ensureOwnerWillRemain to prevent removing the last owner.
🧹 Nitpick comments (3)
app/core/entity/TeamMember.ts (1)
4-25: Make the default part ofcreate()and narrowroleto the supported literals.Right now this is a two-value domain field, but the entity re-opens it to arbitrary strings and silently coerces falsy values to
'member'. A shared'owner' | 'member'type plus a create-time default gives you a tighter contract and makes the default visible to typed callers.If external input will be validated against this type, I’d also add the usual `isTeamMemberRole()` guard alongside the union. As per coding guidelines, "Use strict TypeScript with comprehensive type definitions; avoid `any` types", "Export types and interfaces for reusability across modules", and "Implement type guards for union types in entity files (e.g., `isGranularToken()`)".♻️ Suggested change
+export type TeamMemberRole = 'owner' | 'member'; + interface TeamMemberData extends EntityData { teamMemberId: string; teamId: string; userId: string; - role: string; + role: TeamMemberRole; } -export type CreateTeamMemberData = Omit<EasyData<TeamMemberData, 'teamMemberId'>, 'id'>; +export type CreateTeamMemberData = + Omit<EasyData<TeamMemberData, 'teamMemberId'>, 'id' | 'role'> & { + role?: TeamMemberRole; + }; @@ - role: string; + role: TeamMemberRole; @@ - this.role = data.role || 'member'; + this.role = data.role; } static create(data: CreateTeamMemberData): TeamMember { - const newData = EntityUtil.defaultData(data, 'teamMemberId'); + const newData = EntityUtil.defaultData({ + role: 'member', + ...data, + }, 'teamMemberId'); return new TeamMember(newData); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/core/entity/TeamMember.ts` around lines 4 - 25, The TeamMember entity currently allows any string for role and coerces falsy values in the constructor; change the domain to a strict union and apply the default at creation time: introduce and export type TeamMemberRole = 'owner' | 'member', update TeamMemberData.role to TeamMemberRole, make CreateTeamMemberData.role optional so callers can omit it, ensure the creation path (the static create() factory or caller that constructs TeamMember) sets role = 'member' when undefined (not by coercing falsy in the TeamMember constructor), remove the truthy/coercion logic from the TeamMember constructor so it assumes a valid TeamMemberRole, and add an exported type guard isTeamMemberRole(role: string): role is TeamMemberRole to validate external input before creating TeamMember.app/port/controller/TeamController.ts (1)
225-245: Move therolecontract into request validation.This endpoint calls
requireTeamWriteAccess()before validatingbody.role, so malformed requests bypass the controller's schema-validation path and return auth/authorization errors before 422. Defining a typed schema for{ role: 'owner' | 'member' }here would also remove the cast at Line 244.As per coding guidelines, "Use
@eggjs/typebox-validatefor type-safe request validation in controllers" and "Validate requests in exact order: Parameter Validation (using@eggjs/typebox-validate) → Authentication → Authorization".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/port/controller/TeamController.ts` around lines 225 - 245, The updateTeamMemberRole handler currently validates body.role after calling requireTeamWriteAccess; move the role contract into request validation by adding a TypeBox schema (e.g., Type.Object({ role: Type.Union([Type.Literal('owner'), Type.Literal('member')]) })) and applying the `@HTTPBody/`@Validate decorator so the controller validates the body before calling requireTeamWriteAccess; update the method signature to use the typed body (no runtime string checks) and remove the unnecessary cast at the teamService.addMember(...) call so it accepts the typed 'owner' | 'member' value directly.test/port/controller/TeamController/index.test.ts (1)
575-597: Add the owner-preservation idempotency case.This test only proves that re-adding an existing
memberdoesn't duplicate the row. The new service logic is riskier for existingowners, so a second case that promotes the user first, replaysPUT /user, and asserts the role staysownerwould catch the real regression.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/port/controller/TeamController/index.test.ts` around lines 575 - 597, Add a new test alongside "should be idempotent when adding same user twice" that covers owner-preservation: first use the same PUT '/-/team/teamorg/add-team/user' endpoint (with adminUser authorization) to promote normalUser to role "owner", then replay the identical PUT call that previously re-added the member, and finally GET the members (GET '/-/team/teamorg/add-team/user' using normalUser.authorization or adminUser) and assert that normalUser's role is still "owner" (not downgraded/duplicated). Ensure the test names the scenario clearly (e.g., "should preserve owner role when re-adding same user") and references normalUser, adminUser and the PUT/GET endpoints used in the existing test so it lives next to the current idempotency case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/repository/TeamRepository.ts`:
- Around line 94-100: The addMember method silently returns when member.id is
present but no DB row exists, hiding stale/concurrent-delete failures; change
behavior to throw an error if member.id exists but this.TeamMember.findOne({ id:
member.id }) returns null, and then handle update path normally (call
ModelConvertor.saveEntityToModel and persist), and rename the method to
saveMember to match repository naming conventions; update any call sites
accordingly and ensure the method signature and thrown error type/message are
consistent with other repository save* methods.
In `@docs/org-team.en.md`:
- Around line 311-324: The fenced code block showing the request flow is missing
a language tag (MD040); update the triple-backtick fence that precedes the flow
text to include a language (e.g., "text" or "markdown") so the block becomes
```text (or ```markdown) and leave the closing ``` unchanged; locate the block
in docs/org-team.en.md that starts with the Request GET /@scope/name flow and
add the language tag to the opening fence.
---
Outside diff comments:
In `@app/core/service/TeamService.ts`:
- Around line 22-39: createTeam currently persists the Team via
teamRepository.saveTeam and then separately adds the creator via
teamRepository.addMember, which can leave a team without an owner if the second
write fails; wrap both operations in a single DB transaction so they commit or
rollback together. In TeamService.createTeam (or by adding a new
createTeamWithCreator called from createTeam when creatorUserId is present)
obtain a transactional context from the repository/ORM (e.g., a transaction
callback, EntityManager, or teamRepository.runInTransaction), then call
teamRepository.saveTeam(transactionalContext, team) and
teamRepository.addMember(transactionalContext, member) inside that transaction;
ensure failures roll back and propagate the error, and keep removeTeamCascade
behavior unchanged.
- Around line 64-90: The addMember path is currently idempotent-breaking because
the function signature default role = 'member' will demote an existing owner
when callers omit role; change addMember to accept role as optional (role?:
'owner' | 'member') and only mutate existing.role when role is explicitly
provided and different, calling the existing ensureOwnerWillRemain guard before
demoting an owner; also call ensureOwnerWillRemain from removeMember to protect
the last-owner path. Locate addMember, removeMember, teamRepository.findMember
and teamRepository.addMember and update the logic so implicit calls (no role
passed) are no-ops for existing members while explicit role changes use
ensureOwnerWillRemain to prevent removing the last owner.
---
Nitpick comments:
In `@app/core/entity/TeamMember.ts`:
- Around line 4-25: The TeamMember entity currently allows any string for role
and coerces falsy values in the constructor; change the domain to a strict union
and apply the default at creation time: introduce and export type TeamMemberRole
= 'owner' | 'member', update TeamMemberData.role to TeamMemberRole, make
CreateTeamMemberData.role optional so callers can omit it, ensure the creation
path (the static create() factory or caller that constructs TeamMember) sets
role = 'member' when undefined (not by coercing falsy in the TeamMember
constructor), remove the truthy/coercion logic from the TeamMember constructor
so it assumes a valid TeamMemberRole, and add an exported type guard
isTeamMemberRole(role: string): role is TeamMemberRole to validate external
input before creating TeamMember.
In `@app/port/controller/TeamController.ts`:
- Around line 225-245: The updateTeamMemberRole handler currently validates
body.role after calling requireTeamWriteAccess; move the role contract into
request validation by adding a TypeBox schema (e.g., Type.Object({ role:
Type.Union([Type.Literal('owner'), Type.Literal('member')]) })) and applying the
`@HTTPBody/`@Validate decorator so the controller validates the body before
calling requireTeamWriteAccess; update the method signature to use the typed
body (no runtime string checks) and remove the unnecessary cast at the
teamService.addMember(...) call so it accepts the typed 'owner' | 'member' value
directly.
In `@test/port/controller/TeamController/index.test.ts`:
- Around line 575-597: Add a new test alongside "should be idempotent when
adding same user twice" that covers owner-preservation: first use the same PUT
'/-/team/teamorg/add-team/user' endpoint (with adminUser authorization) to
promote normalUser to role "owner", then replay the identical PUT call that
previously re-added the member, and finally GET the members (GET
'/-/team/teamorg/add-team/user' using normalUser.authorization or adminUser) and
assert that normalUser's role is still "owner" (not downgraded/duplicated).
Ensure the test names the scenario clearly (e.g., "should preserve owner role
when re-adding same user") and references normalUser, adminUser and the PUT/GET
endpoints used in the existing test so it lives next to the current idempotency
case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59bd3553-29c9-4ba4-958e-d4aae5f76472
📒 Files selected for processing (13)
app/core/entity/TeamMember.tsapp/core/service/OrgService.tsapp/core/service/TeamService.tsapp/port/controller/OrgController.tsapp/port/controller/TeamController.tsapp/repository/TeamRepository.tsapp/repository/model/TeamMember.tsdocs/org-team.en.mddocs/org-team.mdsql/mysql/4.32.0.sqlsql/postgresql/4.32.0.sqltest/port/controller/TeamController/index.test.tstest/port/controller/package/ReadAccessAuth.test.ts
fengmk2
approved these changes
Apr 7, 2026
fengmk2
pushed a commit
that referenced
this pull request
Apr 8, 2026
[skip ci] ## 4.32.0 (2026-04-08) * feat: add team member role for team-level permission control (#1023) ([d2fd3c5](d2fd3c5)), closes [#1023](#1023) [#1021](#1021) * chore(deps): update dependency oxfmt to ^0.44.0 (#1025) ([2d5df34](2d5df34)), closes [#1025](#1025) * chore(deps): update dependency oxlint-tsgolint to ^0.20.0 (#1024) ([407d659](407d659)), closes [#1024](#1024)
|
🎉 This PR is included in version 4.32.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Cherry-pick from #1021 (
feat/user-team-apibranch).rolefield (owner/member) toTeamMember, enabling team-level permission controlGET /-/team/:org/:team/memberreturning[{user, role}]PATCH /-/team/:org/:team/member/:usernamefor updating member role4.32.0.sqladdsrolecolumn toteam_membersTest plan
npm run test:local test/port/controller/TeamController/index.test.ts🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation