feat: improve system status graph and add developer role#1664
Conversation
📝 WalkthroughWalkthroughThis PR introduces a new "developer" user role throughout the system and propagates it from authentication through agent session management, tool execution, and API endpoints. It adds role-based permission checks on tools and endpoints, extends resource monitoring with disk/memory total metrics, and updates the frontend UI to reflect the new authorization hierarchy. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
internal/service/frontend/api/v1/webhooks.go (1)
29-31:⚠️ Potential issue | 🟡 MinorStale doc comments still say "Requires admin role".
The
requireWebhookManagementhelper now enforces developer-or-above (Line 396), but the GoDoc comments onListWebhooks,GetDAGWebhook,CreateDAGWebhook,DeleteDAGWebhook,RegenerateDAGWebhookToken, andToggleDAGWebhookstill say "Requires admin role." Update them to match the actual policy.Also applies to: 54-56, 81-83, 132-134, 169-171, 205-207
🤖 Fix all issues with AI agents
In `@internal/service/frontend/api/v1/resources.go`:
- Around line 15-17: GetResourceHistory is currently gated by a call to
requireDeveloperOrAbove which blocks operators/viewers and breaks the System
Status page; change the access check so non-developer roles can still read
resource history (either replace requireDeveloperOrAbove with a read-level guard
such as requireViewerOrAbove/requireOperatorOrAbove if available, or introduce a
new lighter-weight check like requireAuthenticatedOrReadOnly and use that in
GetResourceHistory) or alternatively return a 200 with an empty/partial payload
and let the UI hide charts for insufficient roles; update the check in the
GetResourceHistory handler (the call to requireDeveloperOrAbove) accordingly so
the frontend /services/resources/history request succeeds for intended roles.
In `@ui/src/lib/formatBytes.ts`:
- Around line 3-9: The function formatBytes currently computes Math.log(bytes)
which returns NaN for negative inputs leading to outputs like "NaN undefined";
update formatBytes so it guards against negative values (treat bytes <= 0 the
same as zero) by returning "0 B" early, ensuring the rest of the logic (use of
units and k) only runs for positive byte values; keep the existing behavior for
bytes === 0 and decimals parameter and refer to the formatBytes function and
units array when making the change.
🧹 Nitpick comments (10)
ui/src/features/system-status/components/ResourceChart.tsx (1)
132-132: Tooltip formatter type annotation may not match recharts' actual signature.The recharts
Tooltipformattercallback receives(value: number | string | Array<number | string>, ...). Typing the parameter asnumberwill work at runtime but could cause a TypeScript error depending on your recharts version and strict settings.internal/service/frontend/api/v1/workers.go (1)
16-19: Log statement fires before authorization check.
logger.Info(ctx, "GetWorkers called")on Line 16 executes before the auth gate on Line 17. This means unauthorized requests also produce log entries. Consider moving the log after the auth check, or demoting it to debug level, to reduce noise from unauthorized callers.internal/agent/patch.go (1)
90-92: Consider using a named constant or method for empty-role checks.The empty-string sentinel
ctx.Role != auth.Role("")appears in three places:patch.go(line 90),navigate.go(line 66), andbash.go(line 145). A helper likeRole.IsSet()or aRoleNoneconstant would be more readable and maintainable if the sentinel value ever changes.Example approach in internal/auth/role.go
// Add to role.go const RoleNone Role = "" func (r Role) IsSet() bool { return r != RoleNone }Then usage becomes:
-if ctx.Role != auth.Role("") && !ctx.Role.CanWrite() { +if ctx.Role.IsSet() && !ctx.Role.CanWrite() {internal/agent/system_prompt_test.go (1)
22-27: Good addition of role-based assertion.The test correctly verifies the developer role appears in the generated prompt. Consider adding similar role-content assertions in the admin and viewer test cases (lines 38, 47) to verify that different roles produce different capability sections.
internal/agent/session.go (1)
381-403: Note:createLoopreadssm.role(and other fields) without holdingsm.mu.This follows the existing pattern for
sm.username,sm.ipAddress,sm.environment, etc., so it's not a new issue. However, ifUpdateUserContextis called concurrently withensureLoop, there's a theoretical data race on these fields sinceensureLoopreleases the lock at line 361 before callingcreateLoop. This is a pre-existing concern.internal/auth/role.go (1)
73-75: Duplicate doc comment onParseRole.Line 73-74 has two
// ParseRole converts...lines with slightly different wording.Suggested fix
// ParseRole converts a string to a Role. -// ParseRole converts the input string to a Role and verifies it is one of the known roles. // If the input is not "admin", "manager", "developer", "operator", or "viewer", it returns an error describing the valid options.internal/service/frontend/api/v1/audit_permissions_test.go (2)
13-23: Consider reusingsetupWebhookTestServerinstead of duplicating setup logic.
setupAuditTestServeris nearly identical tosetupWebhookTestServerinwebhooks_test.go(lines 66-75). The only addition iscfg.Server.Audit.Enabled = true. You could compose this by callingsetupWebhookTestServerwith an additional config mutator, or extract a shared base helper that accepts variadic config mutators, to avoid duplicating the auth config block.♻️ Suggested approach
Extract a shared helper (e.g., in a
helpers_test.go):func setupBaseTestServer(t *testing.T, extraMutators ...func(*config.Config)) test.Server { t.Helper() return test.SetupServer(t, test.WithConfigMutator(func(cfg *config.Config) { cfg.Server.Auth.Mode = config.AuthModeBuiltin cfg.Server.Auth.Builtin.Admin.Username = "admin" cfg.Server.Auth.Builtin.Admin.Password = "adminpass" cfg.Server.Auth.Builtin.Token.Secret = "jwt-secret-key" cfg.Server.Auth.Builtin.Token.TTL = 24 * time.Hour for _, m := range extraMutators { m(cfg) } })) }Then in this file:
func setupAuditTestServer(t *testing.T) test.Server { t.Helper() - return test.SetupServer(t, test.WithConfigMutator(func(cfg *config.Config) { - cfg.Server.Auth.Mode = config.AuthModeBuiltin - cfg.Server.Auth.Builtin.Admin.Username = "admin" - cfg.Server.Auth.Builtin.Admin.Password = "adminpass" - cfg.Server.Auth.Builtin.Token.Secret = "jwt-secret-key" - cfg.Server.Auth.Builtin.Token.TTL = 24 * time.Hour - cfg.Server.Audit.Enabled = true - })) + return setupBaseTestServer(t, func(cfg *config.Config) { + cfg.Server.Audit.Enabled = true + }) }
25-65: Consider expanding role coverage for the audit endpoint.The test verifies that manager is allowed and developer is forbidden, which validates the boundary between these two adjacent roles. However, it doesn't cover operator or viewer (also expected forbidden). Adding these cases—ideally via a table-driven approach—would strengthen confidence that the
requireManagerOrAbovecheck works correctly across the full role hierarchy, consistent with the approach taken inTestWebhooks_RequiresDeveloperOrAbovewhich tests both operator and viewer.internal/service/frontend/api/v1/webhooks_test.go (2)
133-207: Good role-based coverage; consider also verifying developer access to the webhook create endpoint.The test thoroughly covers operator and viewer being forbidden for both
GET /api/v1/webhooksandPOST /api/v1/dags/.../webhook, but for the developer role it only verifiesGET /api/v1/webhooks(line 204-206). Adding a developer-access check for the webhook create POST (after creating a test DAG) would ensure both webhook management endpoints are verified for the new role boundary.♻️ Suggested addition after line 206
// Developer can access webhook management endpoints. server.Client().Get("/api/v1/webhooks"). WithBearerToken(developerLogin.Token). ExpectStatus(http.StatusOK).Send(t) + + // Developer can also create webhooks. + dagName := "webhook_dev_access_test" + createTestDAG(t, server, adminToken, dagName) + server.Client().Post("/api/v1/dags/"+dagName+"/webhook", nil). + WithBearerToken(developerLogin.Token). + ExpectStatus(http.StatusCreated).Send(t)
186-201: Webhook POST tests for operator/viewer rely on a non-existent DAG — intentional but fragile.Lines 191-193 and 199-201 POST to
/api/v1/dags/test-dag/webhookwheretest-dagdoesn't exist. This works because the authorization middleware rejects before DAG lookup, but if middleware ordering ever changes, these assertions could break (returning 404 instead of 403). The existingTestWebhooks_RequiresAuth(line 129-130) uses the same pattern, so this is consistent with the current codebase convention.
|
What is the difference between developer and exusting manager role? |
|
@ghansham It's same at the moment (except for access to audit log). |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1664 +/- ##
==========================================
+ Coverage 70.18% 70.21% +0.02%
==========================================
Files 345 345
Lines 38664 38736 +72
==========================================
+ Hits 27135 27197 +62
- Misses 9361 9368 +7
- Partials 2168 2171 +3
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
2 participants
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
UI/UX Improvements