Add built-in system project support#647
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the optional built-in "System Project" workspace, providing Hub administrators with elevated management capabilities through ancestry-based admin delegation, startup registration, and reserved slug/label enforcement. The review feedback highlights critical areas for improvement, including caching database lookups on authorization checks to prevent performance bottlenecks, validating user status before granting delegated admin permissions, resolving usability issues with auto-generated reserved slugs for non-admins, and ensuring workspace paths are updated for existing providers.
| func (a *AuthzService) isSystemProjectAgent(ctx context.Context, agent AgentIdentity) bool { | ||
| projectID := agent.ProjectID() | ||
| if projectID == "" { | ||
| return false | ||
| } | ||
| project, err := a.store.GetProject(ctx, projectID) | ||
| if err != nil || project == nil { | ||
| return false | ||
| } | ||
| return project.Labels[projectcompat.LabelSystemProject] == "true" | ||
| } |
There was a problem hiding this comment.
Performance Bottleneck: Database Query on Every Authorization Check
The isSystemProjectAgent method is called at the beginning of checkAccessForAgent for every single authorization check across all agents. Performing a database query (a.store.GetProject) on every check will introduce a significant performance bottleneck, especially under high load or during batch policy evaluations.
Since the system project's ID and labels are static after startup, we should cache this lookup (e.g., using a thread-safe cache like sync.Map or a cache field on the AuthzService struct) to avoid redundant database roundtrips.
| func (a *AuthzService) originUserIsCurrentAdmin(ctx context.Context, agent AgentIdentity) bool { | ||
| originUserID := agent.OriginUserID() | ||
| if originUserID == "" { | ||
| return false | ||
| } | ||
| user, err := a.store.GetUser(ctx, originUserID) | ||
| if err != nil || user == nil { | ||
| return false | ||
| } | ||
| return user.Role == "admin" | ||
| } |
There was a problem hiding this comment.
Security Vulnerability: Missing User Status Check for Admin Delegation
The originUserIsCurrentAdmin method checks if the origin user has the "admin" role, but it does not verify if the user's status is active. If an admin user is suspended, deactivated, or deleted, their associated system-project agents would still retain elevated admin-equivalent permissions.
To prevent unauthorized access, ensure that the user's status is also validated (e.g., user.Status == "active") before granting delegated admin permissions.
| func (a *AuthzService) originUserIsCurrentAdmin(ctx context.Context, agent AgentIdentity) bool { | |
| originUserID := agent.OriginUserID() | |
| if originUserID == "" { | |
| return false | |
| } | |
| user, err := a.store.GetUser(ctx, originUserID) | |
| if err != nil || user == nil { | |
| return false | |
| } | |
| return user.Role == "admin" | |
| } | |
| func (a *AuthzService) originUserIsCurrentAdmin(ctx context.Context, agent AgentIdentity) bool { | |
| originUserID := agent.OriginUserID() | |
| if originUserID == "" { | |
| return false | |
| } | |
| user, err := a.store.GetUser(ctx, originUserID) | |
| if err != nil || user == nil { | |
| return false | |
| } | |
| return user.Role == "admin" && user.Status == "active" | |
| } |
| if baseSlug == "" { | ||
| baseSlug = api.Slugify(req.Name) | ||
| } | ||
| if s.rejectReservedProjectSlugForNonAdmin(w, ctx, baseSlug) { | ||
| return | ||
| } |
There was a problem hiding this comment.
Usability Issue: Auto-Generated Reserved Slugs Rejected for Non-Admins
If a non-admin user creates a project named "System" or "Global" without explicitly specifying a slug, baseSlug is auto-generated as "system" or "global". This triggers rejectReservedProjectSlugForNonAdmin and rejects the request with a 403 Forbidden error, preventing non-admins from using these common names.
To improve usability, we should automatically append a suffix (e.g., "-project") to the auto-generated slug if it conflicts with a reserved slug, while still rejecting explicitly requested reserved slugs.
| if baseSlug == "" { | |
| baseSlug = api.Slugify(req.Name) | |
| } | |
| if s.rejectReservedProjectSlugForNonAdmin(w, ctx, baseSlug) { | |
| return | |
| } | |
| if baseSlug == "" { | |
| baseSlug = api.Slugify(req.Name) | |
| if projectcompat.IsReservedProjectSlug(baseSlug) { | |
| baseSlug += "-project" | |
| } | |
| } else if s.rejectReservedProjectSlugForNonAdmin(w, ctx, baseSlug) { | |
| return | |
| } |
| baseSlug := api.Slugify(req.Name) | ||
| if s.rejectReservedProjectSlugForNonAdmin(w, ctx, baseSlug) { | ||
| return | ||
| } |
There was a problem hiding this comment.
Usability Issue: Auto-Generated Reserved Slugs Rejected in Project Registration
Similar to the project creation endpoint, registering a project named "System" or "Global" will auto-generate a reserved slug and reject the request for non-admin users.
We should automatically append a suffix to the auto-generated slug for non-admin users to allow them to register projects with these names.
| baseSlug := api.Slugify(req.Name) | |
| if s.rejectReservedProjectSlugForNonAdmin(w, ctx, baseSlug) { | |
| return | |
| } | |
| baseSlug := api.Slugify(req.Name) | |
| if projectcompat.IsReservedProjectSlug(baseSlug) { | |
| if user := GetUserIdentityFromContext(ctx); user == nil || user.Role() != "admin" { | |
| baseSlug += "-project" | |
| } | |
| } | |
| if s.rejectReservedProjectSlugForNonAdmin(w, ctx, baseSlug) { | |
| return | |
| } |
| provider := &store.ProjectProvider{ | ||
| ProjectID: project.ID, | ||
| BrokerID: brokerID, | ||
| BrokerName: brokerName, | ||
| LocalPath: workspacePath, | ||
| Status: store.BrokerStatusOnline, | ||
| LastSeen: time.Now(), | ||
| } | ||
| if err := s.AddProjectProvider(ctx, provider); err != nil { | ||
| if !errors.Is(err, store.ErrAlreadyExists) { | ||
| return fmt.Errorf("failed to add system project provider: %w", err) | ||
| } | ||
| if err := s.UpdateProviderStatus(ctx, project.ID, brokerID, store.BrokerStatusOnline); err != nil { | ||
| log.Printf("Warning: failed to update system project provider status: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential Configuration Mismatch: Workspace Path Not Updated for Existing Providers
If the system project provider already exists, s.AddProjectProvider fails with store.ErrAlreadyExists and we only update the status to store.BrokerStatusOnline. However, if the administrator has changed the workspace_path in the configuration, the stored LocalPath in the database will remain outdated.
Consider updating the provider's LocalPath if it already exists, or logging a warning if the configured workspace path differs from the one stored in the database.
ptone
force-pushed
the
scion/system-project
branch
from
d06b452 to
26d2d97
Compare
Add built-in system project for hub administration with workspace management, authorization rules, and runtime broker integration.
ptone
force-pushed
the
scion/system-project
branch
from
a64b697 to
bdaa18f
Compare
…vider path, tests, slug guard) - authz.go: replace sync.Once with sync.Mutex + success flag so failed system-project lookups are retried on next call - controlchannel.go: move connected=true to after waitForConnected() succeeds, fixing regression where connected was set before handshake - server_broker.go: replace non-atomic RemoveProjectProvider+AddProjectProvider with error-level log when workspace path differs - authz_test.go: add test verifying suspended admin user is denied system-project delegation - handlers_projects_core.go: remove reserved-slug guard from existing-project re-registration path so idempotent re-registration works
1 participant
Summary
Adds an optional, built-in "system" project that gives Hub admins a dedicated helper-assistant workspace with elevated permissions.
Phases implemented:
systemslug, idempotent project registration at startup, configurable workspace path, members group + policy bindings, private visibility with system labelsSCION_CLI_MODE=assistanton all dispatch paths (create, start, restart)