feat: add membership package and AddOrganizationMembers RPC#1537
feat: add membership package and AddOrganizationMembers RPC#1537whoAbhishekSah merged 7 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 14 minutes and 41 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (17)
📝 WalkthroughWalkthroughA new membership service is introduced for managing organization members. It includes error definitions, a Service implementation with dependency injection for policy, relation, role, organization, user, and audit services, and comprehensive test coverage. Auto-generated mocks are configured and created for all service dependencies. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
core/membership/service_test.go (2)
32-40: Add test coverage for remaining service guards.Line 231 always passes
schema.UserPrincipal, so the “principal must be user” branch is not validated. Also, there’s no explicit org-disabled case in the table. Please add both scenarios to lock down the contract from this PR.Also applies to: 231-231
126-126: Audit assertions are too broad in success paths.Using
mock.AnythingforauditRepo.Createcan hide payload regressions. Prefer matching key fields (resource/principal/action) so these tests fail on incorrect audit records.Also applies to: 149-149, 165-165
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 19e33b2d-b936-40f2-849b-4bc39105b736
📒 Files selected for processing (10)
.mockery.yamlcore/membership/errors.gocore/membership/mocks/audit_record_repository.gocore/membership/mocks/org_service.gocore/membership/mocks/policy_service.gocore/membership/mocks/relation_service.gocore/membership/mocks/role_service.gocore/membership/mocks/user_service.gocore/membership/service.gocore/membership/service_test.go
Coverage Report for CI Build 24438924492Coverage increased (+0.1%) to 41.727%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
whoAbhishekSah
changed the title
Introduce core/membership package that manages policy + relation together for org membership operations. AddOrganizationMember validates org/user/role, creates policy + explicit relation, and emits audit records. - Validates org exists and is enabled, user exists and is enabled, role is org-scoped (global or org-specific) - Rejects if user is already a member - Creates policy and matching explicit relation (owner role -> owner relation, everything else -> member relation) - Audit logging via both structured records and event auditor - 12 unit tests covering all error and success paths Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
If createRelation fails after createPolicy succeeds, attempt to delete the orphaned policy. Log a warning if the cleanup also fails so the orphan can be investigated. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Validate principalType is app/user before any service calls. Prevents invalid membership data if a caller passes a serviceuser or group principal type. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Rohil pointed out that the previous check let custom roles created for an org pass even if they weren't scoped to the org namespace. Now we check schema.OrganizationNamespace scope first, before deciding if the role is platform-wide or org-specific. Also remove stale 'replacePolicy deletes...' comment on createPolicy. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
whoAbhishekSah
force-pushed
the
feat/membership-service
branch
from
a15c20f to
a9c4f78
Compare
Test ResultsTested end-to-end on
Verified
|
3 participants
Summary
Introduces
core/membershippackage as the foundation for centralizing all member access management, and wires it into the newAddOrganizationMembersAdminService RPC. Service and handler PRs were initially split but merged into this single PR.What's in this PR
core/membership/packageAddOrganizationMember(ctx, orgID, principalID, principalType, roleID)— adds a user to an org with an explicit roleownerrelation, else →memberrelation)AddOrganizationMembersAdminService RPC{user_id, role_id}pairs{user_id, role_id, success, error}for partial failure handlingIsSuperUser(AdminService)Edge cases handled
app/user(rejected upfront — serviceusers are bound to orgs at creation, not added later)orgService.Get)Design decisions
orgRoleToRelationis a pure function — checks role name, no DB callvalidateOrgRolereturns the fetched role to avoid redundant lookupsvalidateOrgRolealways checksOrganizationNamespacescope first, then decides platform-wide vs org-specificcreatePolicy/createRelationas separate private helpers (no deletes needed for Add)toClientErrorto mask internalsRelated
Test plan
🤖 Generated with Claude Code