feat: add audit logging to SetOrganizationMemberRole RPC#1519
feat: add audit logging to SetOrganizationMemberRole RPC#1519whoAbhishekSah merged 4 commits intomainfrom
Conversation
SetOrganizationMemberRole had no audit logging unlike AddMember and RemoveUsers. This adds both structured audit records and event-based audit logging consistent with other org membership operations. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughRenamed two audit event constants for member role changes and added audit recording when an organization member's role is set: service now creates an audit record and emits an auditor log after replacing org-level policies; tests and a project handler audit call were updated to use the new event names. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
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/organization/service.go (2)
396-405: Redundant database lookups for organization and user.Both
organduserare already fetched during validation invalidateSetMemberRoleRequest(lines 520, 525). Consider refactoring to pass these entities through the call chain to avoid duplicate database queries.♻️ Suggested approach
Modify
SetMemberRoleto return the validated entities fromvalidateSetMemberRoleRequest, or restructure so validation results are reusable for audit logging.-func (s Service) validateSetMemberRoleRequest(ctx context.Context, orgID, userID, newRoleID string) error { +func (s Service) validateSetMemberRoleRequest(ctx context.Context, orgID, userID, newRoleID string) (Organization, user.User, error) { org, err := s.Get(ctx, orgID) if err != nil { - return err + return Organization{}, user.User{}, err } - _, err = s.userService.GetByID(ctx, userID) + usr, err := s.userService.GetByID(ctx, userID) if err != nil { - return err + return Organization{}, user.User{}, err } // ... rest of validation - return nil + return org, usr, nil }
430-433: Consider usingLogWithAttrsfor richer event logging.The project handler for
SetProjectMemberRole(seeinternal/api/v1beta1connect/project.go:400-404) usesLogWithAttrsto include metadata likerole_id. For consistency, consider doing the same here:♻️ Add attributes to audit event
- audit.GetAuditor(ctx, orgID).Log(audit.OrgMemberRoleSetEvent, audit.Target{ + audit.GetAuditor(ctx, orgID).LogWithAttrs(audit.OrgMemberRoleSetEvent, audit.Target{ ID: userID, Type: schema.UserPrincipal, + }, map[string]string{ + "role_id": newRoleID, })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 94310a44-78d2-43cb-a439-f539a6c8da85
📒 Files selected for processing (3)
core/audit/audit.gocore/organization/service.gopkg/auditrecord/consts.go
Test ResultsSteps
ResultsTwo {
"event": "organization.role_changed",
"actor": {
"id": "08006078-ed38-4aca-bea3-9c6dff865116",
"type": "app/user",
"name": "alice_raystack_org"
},
"resource": {
"id": "a9a62b93-ae6f-4aba-9c32-73281c49da73",
"type": "organization",
"name": "PR 1519 Test Org"
},
"target": {
"id": "998d7ebe-cd2c-4f0c-a560-7dd2a7d1a62c",
"type": "user",
"metadata": {
"email": "bob@raystack.org",
"role_id": "e57e1ba4-21fd-43a4-8aca-aa560afb32cf"
}
},
"org_id": "a9a62b93-ae6f-4aba-9c32-73281c49da73",
"org_name": "PR 1519 Test Org"
}{
"event": "organization.role_changed",
"actor": {
"id": "08006078-ed38-4aca-bea3-9c6dff865116",
"type": "app/user",
"name": "alice_raystack_org"
},
"resource": {
"id": "a9a62b93-ae6f-4aba-9c32-73281c49da73",
"type": "organization",
"name": "PR 1519 Test Org"
},
"target": {
"id": "6a51c542-7ca9-4b23-8709-74145013d919",
"type": "user",
"metadata": {
"email": "charlie@raystack.org",
"role_id": "afe94f10-1508-4379-88b1-2c328cb2b769"
}
},
"org_id": "a9a62b93-ae6f-4aba-9c32-73281c49da73",
"org_name": "PR 1519 Test Org"
}Verified
|
Fix gofmt alignment in metadata map literal and update SetMemberRole test cases to mock the new audit record repository and user/org GetByID calls added by audit logging. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6f85ab42-bcb2-46e7-be3f-f598d3fbd7e3
📒 Files selected for processing (2)
core/organization/service.gocore/organization/service_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- core/organization/service.go
Coverage Report for CI Build 24170478234Coverage increased (+0.04%) to 41.189%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Replace mock.Anything with mock.MatchedBy to assert event type, resource ID, target ID, email, role_id, and org_id in audit records. Populate user mock with title and email for realistic assertions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/organization/service_test.go (1)
490-536: Consider adding org title/name verification to audit assertions.The audit assertions comprehensively cover
Event,Resource.ID,Target.ID, email, role_id, andOrgID. However, per the PR objectives, the audit record should also captureorg_nameand org title inresource.name.The org mock currently only sets
IDandState, and themock.MatchedByclosure doesn't verifyar.OrgNameorar.Resource.Name. Adding these would ensure full coverage of the audit contract.🔧 Suggested enhancement
- repo.EXPECT().GetByID(ctx, orgID).Return(organization.Organization{ID: orgID, State: organization.Enabled}, nil).Times(2) + repo.EXPECT().GetByID(ctx, orgID).Return(organization.Organization{ + ID: orgID, + Name: "test-org", + Title: "Test Organization", + State: organization.Enabled, + }, nil).Times(2)And in the
mock.MatchedByassertion:return ar.Event == pkgAuditRecord.OrganizationMemberRoleChangedEvent && ar.Resource.ID == orgID && + ar.Resource.Name == "Test Organization" && ar.Target.ID == userID && ar.Target.Metadata["email"] == "test-user@acme.dev" && ar.Target.Metadata["role_id"] == memberRoleID && - ar.OrgID == orgID + ar.OrgID == orgID && + ar.OrgName == "test-org"
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 887b31b6-efec-45e5-835b-ca81d7189c67
📒 Files selected for processing (1)
core/organization/service_test.go
…harden test assertions - Use LogWithAttrs with role_id attribute for richer event logging - Rename event to app.organization.member.role_changed to match structured audit record naming convention - Align project event from app.project.member.role.set to app.project.member.role_changed for consistency - Add org Name/Title to mock and verify Resource.Name in audit assertions Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/organization/service.go (1)
397-405: Consider reusing org/user from validation to reduce duplicate fetches.Both
organduserare already fetched invalidateSetMemberRoleRequest(lines 522, 527). Returning them from validation or caching in the method could eliminate two extra database calls per role change.♻️ Potential approach
Modify
validateSetMemberRoleRequestto return the fetched entities:func (s Service) validateSetMemberRoleRequest(ctx context.Context, orgID, userID, newRoleID string) (Organization, user.User, error) { org, err := s.Get(ctx, orgID) if err != nil { return Organization{}, user.User{}, err } usr, err := s.userService.GetByID(ctx, userID) if err != nil { return Organization{}, user.User{}, err } // ... role validation ... return org, usr, nil }Then use the returned values in
SetMemberRolefor audit logging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6c342840-67ce-42b1-8c6c-b7909f7769aa
📒 Files selected for processing (4)
core/audit/audit.gocore/organization/service.gocore/organization/service_test.gointernal/api/v1beta1connect/project.go
✅ Files skipped from review due to trivial changes (1)
- core/organization/service_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- core/audit/audit.go
3 participants
Summary
organization.role_changed) and event-based audit (app.organization.member.role.set) toSetOrganizationMemberRoleAddMemberandRemoveUsersorg membership operationsTest plan
app.organization.member.role.set🤖 Generated with Claude Code