feat: BED-6674 add Auditor role #2016
Conversation
WalkthroughAdds an AuditLogRead permission, registers an Auditor role that includes it, updates the v2 audit endpoint to require AuditLogRead, and adds an SQL migration to insert the permission, role, and role→permission mappings. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as API Server
participant Auth as Auth Middleware
participant Audit as AuditHandler
rect rgb(245, 250, 255)
Client->>API: GET /api/v2/audit
API->>Auth: validate token & check permission
Auth->>Auth: requires "AuditLogRead"
alt has AuditLogRead
Auth-->>API: authorized
API->>Audit: handle audit request
Audit-->>API: audit data
API-->>Client: 200 OK + data
else missing permission
Auth-->>API: unauthorized
API-->>Client: 403 Forbidden
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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 |
There was a problem hiding this comment.
note: this is a pfc change that was missed in a previous PR, unrelated to my changes here
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/api/src/database/migration/migrations/v8.4.0.sql (1)
23-44: Consider refactoring JOIN...ON to use WHERE for more idiomatic SQL.The JOIN with row-filtering conditions in the ON clause is valid PostgreSQL but less idiomatic than using a WHERE clause. This makes the query less readable and suggests the intent is not a traditional join relationship.
Apply this diff for clearer SQL idiomaticity:
INSERT INTO roles_permissions (role_id, permission_id) SELECT r.id, p.id - FROM roles r - JOIN permissions p - ON ( - (r.name = 'Auditor' AND (p.authority, p.name) IN ( - ('app', 'ReadAppConfig'), - ('risks', 'GenerateReport'), - ('audit_log', 'Read'), - ('auth', 'CreateToken'), - ('auth', 'ManageSelf'), - ('auth', 'ReadUsers'), - ('graphdb', 'Read'), - ('saved_queries', 'Read'), - ('clients', 'Read') - )) - OR - (r.name = 'Administrator' AND (p.authority, p.name) IN ( - ('audit_log', 'Read') - )) - ) + FROM roles r + CROSS JOIN permissions p + WHERE ( + (r.name = 'Auditor' AND (p.authority, p.name) IN ( + ('app', 'ReadAppConfig'), + ('risks', 'GenerateReport'), + ('audit_log', 'Read'), + ('auth', 'CreateToken'), + ('auth', 'ManageSelf'), + ('auth', 'ReadUsers'), + ('graphdb', 'Read'), + ('saved_queries', 'Read'), + ('clients', 'Read') + )) + OR + (r.name = 'Administrator' AND (p.authority, p.name) IN ( + ('audit_log', 'Read') + )) + ) ON CONFLICT DO NOTHING;Using
CROSS JOINwithWHEREmakes it explicit that you're filtering a Cartesian product rather than joining on a relationship, and is the more standard pattern for this type of query.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/api/src/auth/role.go(2 hunks)cmd/api/src/database/migration/migrations/v8.4.0.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/api/src/auth/role.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: build-ui
- GitHub Check: run-tests
🔇 Additional comments (1)
cmd/api/src/database/migration/migrations/v8.4.0.sql (1)
17-21: Permission and role inserts look correct.The permission insert (line 18) and role insert (lines 20-21) are straightforward and syntactically correct. The use of ON CONFLICT DO NOTHING ensures idempotency. The description "Can read data and audit logs" accurately reflects the Auditor role's intended permissions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/api/src/auth/permission.go (1)
63-63: LGTM! Permission included in All() method.The
AuditLogReadpermission is correctly included in theAll()method.Optional: Consider verifying AuthAcceptEULA inclusion.
AuthAcceptEULAis defined inPermissions()at line 95 but doesn't appear in theAll()method. This may be intentional, but worth confirming:#!/bin/bash # Description: Check if AuthAcceptEULA is used elsewhere or if its omission from All() is intentional # Search for AuthAcceptEULA usage across the codebase rg -n "AuthAcceptEULA" --type go -C 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
cmd/api/src/test/integration/harnesses/citrixRDPHarness.svgis excluded by!**/*.svg
📒 Files selected for processing (4)
cmd/api/src/api/registration/v2.go(1 hunks)cmd/api/src/auth/permission.go(3 hunks)cmd/api/src/auth/role.go(2 hunks)cmd/api/src/database/migration/migrations/v8.4.0.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/api/src/auth/role.go
- cmd/api/src/database/migration/migrations/v8.4.0.sql
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/api/src/auth/permission.go (1)
cmd/api/src/model/auth.go (2)
Permission(35-40)NewPermission(42-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: run-tests
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: build-ui
🔇 Additional comments (3)
cmd/api/src/auth/permission.go (2)
30-31: LGTM! Permission field properly defined.The
AuditLogReadfield is correctly added to thePermissionSetstruct with the appropriate type and maintains alphabetical ordering.
93-94: LGTM! Permission properly initialized.The
AuditLogReadpermission is correctly initialized withNewPermission("audit_log", "Read"), following the established naming convention and pattern. The placement maintains alphabetical ordering.cmd/api/src/api/registration/v2.go (1)
149-149: Verification complete - all changes are correct and consistent.The migration in v8.4.0.sql properly assigns
AuditLogReadto both the Administrator and Auditor roles, ensuring existing administrators retain access while enabling the new Auditor role. The permission constant is correctly defined incmd/api/src/auth/permission.goand maps to("audit_log", "Read"), the Auditor role is properly defined incmd/api/src/auth/role.gowith the appropriate permissions, and the endpoint inv2.gocorrectly usespermissions.AuditLogRead.
mistahj67
left a comment
mistahj67
left a comment
There was a problem hiding this comment.
Code LGTM, local testing worked as well. Great job, 🚀
Description
Adds an Auditor role with read-only permissions, plus permissions to view the audit log. Includes a database migration to add the new permission and the new role.
Motivation and Context
Resolves BED-6674
This role has all of the permissions of the existing
Read Onlyrole, plus the ability to view the audit log (/api/v2/audit). Previously, only admins could view the audit log.How Has This Been Tested?
Auditorpermissionsauditendpoint with this new userScreenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Changes