Repository navigation
[HYPERSHELL-262] feat(rbac): assign gateway:creator by default on user provisioning #263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
db3b999
a36d552
2d53d04
73d8716
e144c3c
5d9ea17
2e4bc50
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,10 +51,13 @@ func UserProvisioningMiddleware(provisioner UserProvisioner, syncer JWTRoleSynce | |
| jwtRoles := extractJWTRoles(r) | ||
| if len(jwtRoles) > 0 { | ||
| ctx = context.WithValue(ctx, ContextJWTRolesKey, jwtRoles) | ||
| if syncer != nil { | ||
| if syncErr := syncer.SyncJWTRoles(ctx, userID, jwtRoles); syncErr != nil { | ||
| glog.Warningf("JWT role sync failed for %q: %v", payload.Username, syncErr) | ||
| } | ||
| } | ||
| // Always sync even when jwtRoles is empty: SyncJWTRoles applies | ||
| // configured default roles (e.g. gateway:creator) so that users with | ||
| // no Keycloak realm roles still receive their initial bindings. | ||
| if syncer != nil { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Context] Always-sync even for empty JWT roles. This is a reasonable enabler for default-role assignment, but combined with the always-merge in |
||
| if syncErr := syncer.SyncJWTRoles(ctx, userID, jwtRoles); syncErr != nil { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Minor] Moving |
||
| glog.Warningf("JWT role sync failed for %q: %v", payload.Username, syncErr) | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,8 @@ package roleBindings | |
|
|
||
| import ( | ||
| "net/http" | ||
| "os" | ||
| "strings" | ||
|
|
||
| "github.com/gorilla/mux" | ||
| "google.golang.org/grpc" | ||
|
|
@@ -24,16 +26,38 @@ import ( | |
| type ServiceLocator func() RoleBindingService | ||
|
|
||
| func NewServiceLocator(env *environments.Env) ServiceLocator { | ||
| defaultRoles := defaultRolesFromEnv() | ||
| return func() RoleBindingService { | ||
| return NewRoleBindingService( | ||
| db.NewAdvisoryLockFactory(env.Database.SessionFactory), | ||
| NewRoleBindingDao(&env.Database.SessionFactory), | ||
| roles.NewRoleDao(&env.Database.SessionFactory), | ||
| events.Service(&env.Services), | ||
| defaultRoles, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| // defaultRolesFromEnv reads RBAC_DEFAULT_ROLES (comma-separated role names). | ||
| // Defaults to gateway:creator so all authenticated users can create gateways. | ||
| // Set RBAC_DEFAULT_ROLES= (explicit empty) to disable defaults entirely. | ||
| func defaultRolesFromEnv() []string { | ||
| val, set := os.LookupEnv("RBAC_DEFAULT_ROLES") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using |
||
| if !set { | ||
| return []string{roles.RoleGatewayCreator} | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Minor] With the default (empty env var) this grants
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Major] val, ok := os.LookupEnv("RBAC_DEFAULT_ROLES")
if !ok {
return []string{roles.RoleGatewayCreator}
}
// ok && val == "" -> no default roles
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Blocker/Config] Default is |
||
| } | ||
| if val == "" { | ||
| return nil | ||
| } | ||
| var result []string | ||
| for _, r := range strings.Split(val, ",") { | ||
| if trimmed := strings.TrimSpace(r); trimmed != "" { | ||
| result = append(result, trimmed) | ||
| } | ||
| } | ||
| return result | ||
| } | ||
|
|
||
| func Service(s *environments.Services) RoleBindingService { | ||
| if s == nil { | ||
| return nil | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ import ( | |
| "context" | ||
| "fmt" | ||
|
|
||
| "github.com/golang/glog" | ||
| "github.com/openshift-online/hypershell/components/api-server/pkg/rbac" | ||
| "github.com/openshift-online/hypershell/components/api-server/plugins/roles" | ||
| "github.com/openshift-online/rh-trex-ai/pkg/api" | ||
|
|
@@ -35,22 +36,30 @@ func NewRoleBindingService( | |
| rbDao RoleBindingDao, | ||
| roleDao roles.RoleDao, | ||
| events services.EventService, | ||
| defaultRoles []string, | ||
| ) RoleBindingService { | ||
| for _, r := range defaultRoles { | ||
| if !roles.JWTSyncedRoles[r] { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Minor] Silent degradation on misconfiguration. A default role that is not in |
||
| glog.Warningf("RBAC_DEFAULT_ROLES: role %q is not in JWTSyncedRoles and will be ignored; add it to JWTSyncedRoles to make it sync-eligible", r) | ||
| } | ||
| } | ||
| return &sqlRoleBindingService{ | ||
| lockFactory: lockFactory, | ||
| rbDao: rbDao, | ||
| roleDao: roleDao, | ||
| events: events, | ||
| lockFactory: lockFactory, | ||
| rbDao: rbDao, | ||
| roleDao: roleDao, | ||
| events: events, | ||
| defaultRoles: defaultRoles, | ||
| } | ||
| } | ||
|
|
||
| var _ RoleBindingService = &sqlRoleBindingService{} | ||
|
|
||
| type sqlRoleBindingService struct { | ||
| lockFactory db.LockFactory | ||
| rbDao RoleBindingDao | ||
| roleDao roles.RoleDao | ||
| events services.EventService | ||
| lockFactory db.LockFactory | ||
| rbDao RoleBindingDao | ||
| roleDao roles.RoleDao | ||
| events services.EventService | ||
| defaultRoles []string | ||
| } | ||
|
|
||
| func (s *sqlRoleBindingService) CreateGatewayOwnerBinding(ctx context.Context, userID string, gatewayID string) error { | ||
|
|
@@ -97,6 +106,16 @@ func (s *sqlRoleBindingService) SyncJWTRoles(ctx context.Context, userID string, | |
| jwtRoleSet[r] = true | ||
| } | ||
| } | ||
| // Default roles are always merged regardless of what the JWT carries. | ||
| // They represent the platform's baseline posture: every authenticated | ||
| // principal receives these capabilities unless explicitly disabled via | ||
| // RBAC_DEFAULT_ROLES=. Only roles in JWTSyncedRoles participate in the | ||
| // sync lifecycle (idempotent add, never revoked by JWT absence). | ||
| for _, r := range s.defaultRoles { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Blocker] Always-merge defeats Keycloak revocation and contradicts the RBAC spec. Because the default roles are re-applied on every sync regardless of JWT contents, the spec scenario "Keycloak admin revokes gateway:creator" (which requires the binding to be removed and the user to lose create ability) can never take effect for a user with no other synced roles: the next sync re-adds Either update the authoritative spec (Purpose, the two 403 scenarios, and the revoke scenario) with maintainer sign-off, or gate the merge so it cannot override an explicit Keycloak revocation. Note this is exactly the point where a sibling PR takes a different, incompatible approach (defaults only when
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Default roles are merged unconditionally here, so |
||
| if roles.JWTSyncedRoles[r] { | ||
| jwtRoleSet[r] = true | ||
| } | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Blocker] This default-role merge is correct, but it never executes for the target audience. Both callers gate the sync behind
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Blocker] Contradicts
The spec is not updated in this PR. Please either update |
||
|
|
||
| existing, err := s.rbDao.FindByUserID(ctx, userID) | ||
| if err != nil { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Major] Default role now granted to service accounts too.
provisionUserForGRPCruns before theisServiceAccountallowlist check (line 25/79), andSyncJWTRolesis now called even with emptyjwtRoles. Machine/service-account identities that previously had no bindings (empty JWT -> no sync) will now get a persisted globalgateway:creatorbinding. Please confirm whether service accounts should be excluded from default-role assignment, and document the effective "all authenticated users can create gateways" default in the RBAC spec.