feat(rbac): implement platform:admin role for global gateway access - #143
Conversation
bsquizz
left a comment
There was a problem hiding this comment.
Amber Code Review: Platform:Admin Role Specification
Overall Assessment: The core design is excellent—well-scoped, orthogonal, and thoughtfully limited to view + delete. However, found 1 blocker and 1 critical inconsistency that must be fixed before merge.
🔴 BLOCKER
Outdated Role Model Reference
Location: specs/security/rbac-enforcement.spec.md:465
Integration tests SHALL exercise RBAC enforcement with the new three-role model.❌ Issue: Still says "three-role model" but the PR introduces a four-role model.
Fix: Change to "four-role model"
Why it's a blocker: This directly contradicts the entire PR. Integration test implementers will be confused.
🟠 CRITICAL: Debounce Timing Inconsistency
Multiple locations — Gateway search debounce values conflict across the specs:
| Location | Line | Value |
|---|---|---|
rbac-enforcement.spec.md (new) |
356 | 300ms |
web-console/architecture.spec.md WEB-ARCH-02 (new) |
92 | 300ms |
web-console/architecture.spec.md WEB-DATA-01 (existing) |
281 | 250ms |
web-console/architecture.spec.md WEB-UI-03A (new) |
365 | 300ms |
Impact:
- API implementers will use one value, UI implementers another
- Tests will fail intermittently
- Inconsistent user experience
Root cause: WEB-DATA-01 predates this PR and specified 250ms. The new platform:admin requirements introduce 300ms without updating the existing requirement.
Recommended fix: Align all gateway search debounce to 300ms (it's in 3/4 locations and is a common UX pattern):
- Update
specs/web-console/architecture.spec.md:281— Change WEB-DATA-01 from 250ms → 300ms - Consider: placement search (lines 283, 433) may also need alignment
🟡 MAJOR: Duplicate UI Requirements
Location: specs/security/rbac-enforcement.spec.md:349-389
The RBAC spec includes detailed UI requirements for platform:admin (pagination, search, delete actions, etc.) that duplicate requirements in specs/web-console/architecture.spec.md WEB-UI-03A (lines 355-422).
Why this matters:
- Maintenance burden: Changes require updates in TWO places
- Drift risk: Specs can become inconsistent over time
- Unclear authority: Which spec is canonical?
Recommended fix: In the RBAC spec, replace detailed UI requirements with a cross-reference:
### Requirement: Platform Admin UI Experience
The web UI SHALL support platform:admin users as defined in
`web-console/architecture.spec.md` requirement WEB-UI-03A.
Platform administrators SHALL see all gateways in the gateway list, which SHALL use
API-backed pagination and search as specified in WEB-API-01.Keep ALL UI implementation details (page sizes, debounce, modals, badges) in the web-console spec only.
🟢 MINOR Issues
1. OpenShell Access Not Explicit
Location: specs/security/rbac-enforcement.spec.md:106
The permission matrix shows -- for platform:admin OpenShell mapping but doesn't explain why.
Suggested addition after line 120:
The `platform:admin` role provides visibility and lifecycle management through the
HyperShell web console but does NOT grant OpenShell CLI access to gateways. Platform
administrators who need to use the `openshell` CLI for a specific gateway must be
granted `gateway:owner` or `gateway:viewer` on that gateway.2. No Database Migration Mentioned
Location: specs/security/rbac-enforcement.spec.md:47-48
The spec says roles are seeded via migration but doesn't explicitly state a new migration is needed.
Suggested addition in "Production Rollout" section:
### Database Migration Requirement
A database migration SHALL seed the `platform:admin` role record with:
- `name: "platform:admin"`
- `display_name: "Platform Administrator"`
- `description: "Platform-wide view and delete access for all gateways"`
- `built_in: true`3. No Audit Logging for High-Risk Operations
Platform admin deletions are high-privilege operations that should be audited. Consider referencing audit requirements or adding:
Platform administrator actions SHALL be logged with:
- Action performed (view, delete)
- Actor identity (platform:admin user)
- Target resource (gateway ID and name)
- Timestamp and correlation ID4. Scope Beyond Gateways Unclear
Location: specs/security/rbac-enforcement.spec.md:276-290
The spec only describes platform:admin permissions for gateways. What about Fleets, GatewayNetworks, ManagedClusters, etc.?
Recommended: Add explicit scoping:
The `platform:admin` role is **gateway-scoped only**. It does NOT grant permissions to:
- View or modify Fleets, GatewayNetworks, GatewayReleases, ManagedClusters, or ManagedDatabases
- View or modify Users or RoleBindings
- Access platform-level configuration or system administration functions5. gRPC Authorization Scenarios Missing
Location: specs/security/rbac-enforcement.spec.md:390-397
gRPC enforcement is mentioned but no scenarios show platform:admin via gRPC. Does the control plane need this role, or is it HTTP-only (web console)?
Summary
| Severity | Count | Action Required |
|---|---|---|
| 🔴 Blocker | 1 | Must fix before merge |
| 🟠 Critical | 1 | Must fix before merge |
| 🟡 Major | 1 | Strongly recommended |
| 🟢 Minor | 5 | Nice-to-have clarifications |
Confidence: High (95%)
Reviewed by Amber — HyperShell codebase intelligence
bsquizz
left a comment
There was a problem hiding this comment.
Amber Re-Review: All Issues Addressed ✅
Reviewed commit fa5ff3f — Excellent work! All previous findings have been resolved.
Changes Verified ✅
🔴 BLOCKER - FIXED
Line 482 (was 465): Changed "three-role model" → "four-role model" ✅
🟠 CRITICAL - FIXED
Debounce timing now consistent at 300ms across all locations:
specs/web-console/architecture.spec.md:281— Gateway search: 250ms → 300ms ✅specs/web-console/architecture.spec.md:283— Placement search: 250ms → 300ms ✅- All four locations now aligned at 300ms ✅
🟡 MAJOR - FIXED
Duplicate UI requirements consolidated:
- Removed detailed UI requirements from
rbac-enforcement.spec.md✅ - Added proper cross-reference to
web-console/architecture.spec.md(WEB-ARCH-02, WEB-UI-03A, WEB-DATA-01) ✅ - Single source of truth established ✅
🟢 MINOR Issues - ALL ADDRESSED
-
OpenShell access clarification — Added after line 118 ✅
- Clearly explains platform:admin does NOT grant OpenShell CLI access
- Documents how to get OpenShell access (requires gateway:owner or gateway:viewer)
-
Database migration requirement — New section added ✅
- Explicitly documents the platform:admin role record migration
- Specifies all required fields (name, display_name, description, built_in)
- Notes it runs alongside existing role migrations
-
Audit logging requirement — New section added ✅
- Documents what must be logged (action, actor, target, timestamp, correlation ID)
- Specifies INFO level or higher for high-privilege operations
- Provides security audit trail
-
Resource scope clarification — Added after line 297 ✅
- Explicitly states platform:admin is gateway-scoped only
- Lists what it does NOT grant access to (Fleets, Networks, Releases, etc.)
- Prevents scope creep and misunderstanding
-
gRPC authorization scenarios — Two scenarios added ✅
- Scenario: Platform admin watches gateways via gRPC
- Scenario: Platform admin cannot modify via gRPC without ownership
- Covers gRPC enforcement equivalence to HTTP
Final Assessment
| Category | Status |
|---|---|
| Blockers | 0 ✅ |
| Critical Issues | 0 ✅ |
| Major Issues | 0 ✅ |
| Minor Issues | 0 ✅ |
| Spec Quality | Excellent |
| Implementation Readiness | Ready |
Recommendation: ✅ APPROVE — All issues resolved, spec is complete and consistent.
The specification is now:
- Internally consistent (no conflicting values)
- Properly scoped (clear boundaries)
- Well-documented (scenarios, requirements, rationale)
- Maintainable (single source of truth, proper cross-references)
- Implementation-ready (database migration, audit logging, gRPC coverage)
Confidence: High (95%)
Re-reviewed by Amber — HyperShell codebase intelligence
|
LGTM |
Define a new platform:admin RBAC role that grants platform-wide view and delete permissions for all gateways, regardless of per-gateway ownership bindings. Changes to specs/security/rbac-enforcement.spec.md: - Add platform:admin as fourth built-in role (Keycloak JWT-sourced) - Platform admins can view and delete any gateway across the platform - Platform admins cannot modify gateways (requires gateway:owner) - Platform admins cannot create gateways (requires gateway:creator) - Platform admins cannot grant role bindings (requires gateway:owner) - Role is orthogonal to gateway:creator (roles compose) - Add 7 scenarios for platform admin operations - Add UI requirements: pagination (20-100 per page), search with 300ms debounce, delete actions with confirmation modal - Update production rollout warnings to include platform:admin SSO setup Changes to specs/web-console/architecture.spec.md: - Update gateway visibility scoping to include platform:admin users - Mandate API-backed pagination for gateway list (not optional) - Add WEB-UI-03A requirement for platform admin gateway list experience - Add 4 scenarios for platform admin UI behavior - Update API contract requirements for pagination, search, and filtering - Specify page size defaults (20), max (100), search fields (name/ID/fleet) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Fix blocker, critical, and minor issues identified in code review: - Update integration test requirement from three-role to four-role model - Align all debounce timings to 300ms (gateway and placement search) - Replace duplicate UI requirements with cross-references to web-console spec - Add OpenShell access clarification for platform:admin - Add explicit database migration requirement section - Add audit logging requirement for high-privilege operations - Clarify platform:admin scope is gateway-only (does not apply to Fleets, etc.) - Add gRPC authorization scenarios for platform:admin Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Update language to reflect that the initial implementation of platform:admin is limited to gateway view/delete operations, with potential expansion to other resources (Fleets, GatewayNetworks, etc.) in future iterations. This addresses review feedback that "gateway-scoped only" was too restrictive a statement given planned future expansion of the role. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Implements the four-role RBAC model with platform:admin role providing platform-wide view and delete permissions for all gateways. Backend implementation: - Database migration seeds platform:admin role - JWT syncing for platform:admin from Keycloak (HTTP & gRPC) - Authorization logic: view all + delete any gateway - Blocks platform:admin from modify/create without additional roles - Blocks platform:admin from granting RoleBindings - Audit logging at INFO level for platform:admin actions - gRPC authorization matches HTTP authorization Testing: - Added 8 unit tests for platform:admin HTTP authorization - Added 5 unit tests for platform:admin gRPC authorization - All tests passing (13/13 new tests) UI: - Delete button already exists in gateway row actions dropdown - Backend enforces authorization; UI is authorization-agnostic Spec compliance: - Implements specs/security/rbac-enforcement.spec.md - Follows web-console/architecture.spec.md patterns - Four-role model: platform:admin, gateway:creator, gateway:owner, gateway:viewer Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Adds comprehensive E2E test coverage for the platform:admin role in the
Kind cluster test suite:
Positive assertions:
- Platform admin can list all gateways (GET /gateways returns 200)
- Platform admin can delete gateways they don't own (DELETE /gateways/{id}
returns 204 even without gateway:owner binding)
Negative assertions:
- Platform admin cannot create gateways without gateway:creator role
(POST /gateways returns 403)
- Verifies platform admin has NO gateway:owner binding before delete test
Infrastructure:
- Added assign_realm_role() function to drivers/kind.sh for assigning
realm roles (platform:admin, gateway:creator) via Keycloak Admin API
- Added E2E_PLATFORM_ADMIN_USERNAME and E2E_PLATFORM_ADMIN_PASSWORD
environment variables (default: platform-admin/platform-admin)
- Platform admin user setup includes realm role assignment and OIDC
token acquisition
Test coverage:
- Verifies four-role RBAC model enforcement
- Tests platform:admin permissions match rbac-enforcement.spec.md
- Validates separation of concerns (view/delete vs modify/create)
The test runs as step 10 after developer RBAC verification, ensuring
the gateway created by the admin user is available for platform admin
to delete without ownership.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Fixes E2E test failure where platform-admin user did not exist in Keycloak, causing "User not found" and "Invalid user credentials" errors. Changes: - Add platform:admin realm role to Keycloak realm configuration - Add platform-admin user with platform:admin role pre-assigned - Update E2E test to handle case where role is already assigned via realm import (best-effort role assignment instead of failing) User configuration: - Username: platform-admin - Password: platform-admin - Realm roles: hypershell-users, platform:admin - Email: platform-admin@hypershell.local The platform-admin user is now pre-created in the Keycloak realm import (similar to admin and developer users), ensuring E2E tests can immediately acquire tokens and verify platform:admin RBAC permissions. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Fixes E2E test failures where platform:admin authorization was denied (HTTP 403) because the platform:admin role didn't exist in the database. Root cause: The platform:admin role was added to the builtInRoleSeeds array, but the seed migration (ID: 2026081112000004) had already run in existing environments. Gormigrate migrations only run once, so the new role was never created in the database. Solution: Created a new dedicated migration (ID: 2026081812000001) that: - Checks if platform:admin role already exists (idempotent) - Creates the role with proper permissions (read, delete on gateways) - Runs independently of the initial role seeding migration This ensures the platform:admin role exists in both fresh deployments (via builtInRoleSeeds) and existing deployments (via this migration). Migration details: - ID: 2026081812000001 - Name: platform:admin - Display: Platform Administrator - Permissions: gateways: [read, delete] - Built-in: true The E2E tests should now pass as the JWT role sync will successfully find the platform:admin role and create the RoleBinding. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Adds a helper script for administrators to grant the platform:admin role to users via the Keycloak Admin REST API. Features: - Automated role assignment via Keycloak Admin API - Configurable for any Keycloak environment (Kind, production) - Validates user exists before assignment - Verifies role was successfully assigned - Clear success/error messages at each step Usage: ./docs/assign-platform-admin-role.sh <username> Environment variables: KEYCLOAK_URL - Keycloak base URL KEYCLOAK_REALM - Realm name (default: hypershell) KEYCLOAK_ADMIN_USER - Admin username KEYCLOAK_ADMIN_PASS - Admin password This script complements the UI-based assignment documented in the rbac-enforcement spec, providing an automation-friendly alternative for batch operations or CI/CD pipelines. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
97a5527 to
a576e72
Compare
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: openshift-online/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 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 |
Bring in tracing (#144), CI e2e gating (#152), and platform:admin RBAC (#143) from main. Resolve the e2e-openshell.sh conflict by keeping both new sections in run order: platform admin RBAC as section 10 (it deletes the gateway) and namespace garbage collection as section 11 (it validates the namespace disappears, with a fallback delete if the gateway remains). Repair the semantic conflict in the gRPC RBAC interceptor test, which must now pass the new JWTRoleSyncer argument (nil, as it does not exercise role syncing). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Kyle Squizzato <kysquizz@redhat.com>
Summary
Implements a new
platform:adminRBAC role that grants platform-wide view and delete permissions for all gateways, regardless of per-gateway ownership bindings.This PR includes specification, backend implementation, and E2E testing for the four-role RBAC model.
Platform:Admin Role
gateway:creator)gateway:owner), cannot create gateways (requiresgateway:creator), cannot grant role bindings (requiresgateway:owner)Key Design Decisions
platform:admin+gateway:creatorfor full capabilities/adminhierarchySpecification Changes
specs/security/rbac-enforcement.spec.md
platform:adminto built-in roles table and permission matrixspecs/web-console/architecture.spec.md
platform:adminusersImplementation
Backend (components/api-server/)
Database Migration
platform:adminrole in built-in roles migrationAuthorization Logic
platform:admin(HTTP & gRPC)GET /gateways/{id}succeeds for any IDDELETE /gateways/{id}succeeds for any IDgateway:ownerbindinggateway:owneron target gatewayAudit Logging
gRPC Authorization
Frontend (UI)
Existing UI Already Supports Platform:Admin
Testing
Unit Tests (13 new tests, all passing)
E2E Tests (Kind cluster)
Files Changed
Specification: 2 files
specs/security/rbac-enforcement.spec.mdspecs/web-console/architecture.spec.mdImplementation: 10 files (+307 lines)
components/api-server/pkg/rbac/authorization.gocomponents/api-server/pkg/rbac/authorization_test.gocomponents/api-server/pkg/rbac/grpc_interceptor.gocomponents/api-server/pkg/rbac/grpc_interceptor_test.gocomponents/api-server/pkg/rbac/user_provisioning.gocomponents/api-server/plugins/gateways/handler.gocomponents/api-server/plugins/rbac/grpc_init.gocomponents/api-server/plugins/roleBindings/service.gocomponents/api-server/plugins/roles/migration.gocomponents/api-server/plugins/roles/model.goE2E Tests: 3 files (+201 lines)
tests/e2e/drivers/kind.sh(addedassign_realm_role()function)tests/e2e/e2e-openshell.sh(added step 10: platform admin RBAC verification)tests/e2e/lib.sh(added platform admin env vars)Bootstrap
The first platform admin is created by:
platform:adminrealm role to a userProduction prerequisite: External SSO must define
platform:adminrealm role and emit it in JWT claims BEFORE enabling RBAC enforcement.Test Plan
platform:adminrole definitionplatform:adminfrom JWT claims (HTTP & gRPC)gateway:creator(unit + E2E tests)Verification
Build: ✅
go build ./...passesUnit Tests: ✅ All 13 new tests passing
Syntax: ✅ E2E bash scripts validated
Related
specs/security/rbac-enforcement.spec.md(four-role model)specs/web-console/architecture.spec.md(UI patterns)🤖 Generated with Claude Code