Skip to content

Commit 084e113

Browse files
jsell-rhclaude
andcommitted
fix: address PR review feedback
- e2e-login: add AMBIENT_ENV=production guard alongside E2E_TEST_HELPERS to prevent accidental auth bypass in production - impersonation: align getUserSubjectFromContext claim order with buildImpersonatingClients (preferred_username > email > sub) so RoleBinding subjects match the impersonated identity - JWK_CERT_URL: fix priority to CLI flag > env var > default (standard convention, per review feedback) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 20c7b98 commit 084e113

4 files changed

Lines changed: 24 additions & 17 deletions

File tree

components/ambient-api-server/cmd/ambient-api-server/environments/e_production.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,15 +24,15 @@ const defaultJwkCertURL = "https://sso.redhat.com/auth/realms/redhat-external/pr
2424
func (e *ProductionEnvImpl) OverrideConfig(c *config.ApplicationConfig) error {
2525
c.Server.CORSAllowedHeaders = []string{"X-Ambient-Project"}
2626

27-
// Priority: env var > CLI flag > default.
27+
// Priority: CLI flag > env var > default.
2828
// The framework parses --jwk-cert-url before OverrideConfig runs,
2929
// so c.Auth.JwkCertURL already holds the flag value (or the flag's
3030
// built-in default if not explicitly set).
3131
switch {
32-
case os.Getenv("JWK_CERT_URL") != "":
33-
c.Auth.JwkCertURL = os.Getenv("JWK_CERT_URL")
3432
case c.Auth.JwkCertURL != "" && c.Auth.JwkCertURL != defaultJwkCertURL:
3533
// CLI flag was explicitly set to a non-default value; keep it.
34+
case os.Getenv("JWK_CERT_URL") != "":
35+
c.Auth.JwkCertURL = os.Getenv("JWK_CERT_URL")
3636
default:
3737
c.Auth.JwkCertURL = defaultJwkCertURL
3838
}

components/backend/handlers/projects.go

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -941,17 +941,20 @@ func getUserSubjectFromContext(c *gin.Context) (string, error) {
941941
return fmt.Sprintf("system:serviceaccount:%s:%s", ns, saName), nil
942942
}
943943

944-
// Prefer email — this matches the impersonation identity (Impersonate-User)
945-
// so RoleBindings created with this subject are effective under impersonation.
944+
// Must match the order in buildImpersonatingClients (sso.go):
945+
// preferred_username > email > sub. If these diverge, RoleBindings
946+
// created here won't match the impersonated identity.
947+
if userName, exists := c.Get("userName"); exists && userName != nil {
948+
if s := fmt.Sprintf("%v", userName); s != "" {
949+
return s, nil
950+
}
951+
}
946952
if email := c.GetString("userEmail"); email != "" {
947953
return email, nil
948954
}
949955
if userIDOrig := c.GetString("userIDOriginal"); userIDOrig != "" {
950956
return userIDOrig, nil
951957
}
952-
if userName, exists := c.Get("userName"); exists && userName != nil {
953-
return fmt.Sprintf("%v", userName), nil
954-
}
955958
if userID, exists := c.Get("userID"); exists && userID != nil {
956959
return fmt.Sprintf("%v", userID), nil
957960
}

components/frontend/src/app/api/auth/sso/e2e-login/route.ts

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,14 @@ import { NextRequest, NextResponse } from "next/server";
22
import { getSession } from "@/lib/session";
33

44
export async function POST(request: NextRequest) {
5-
// Gate on E2E_TEST_HELPERS rather than NODE_ENV so this route works in
6-
// Kind/CI where the Docker image sets NODE_ENV=production but E2E tests
7-
// still need programmatic session creation.
8-
if (process.env.E2E_TEST_HELPERS !== "true") {
5+
// Double guard: E2E_TEST_HELPERS must be explicitly set AND we must not
6+
// be in an environment that looks like production (has a public route).
7+
// This prevents accidental auth bypass if E2E_TEST_HELPERS leaks into
8+
// a production overlay.
9+
if (
10+
process.env.E2E_TEST_HELPERS !== "true" ||
11+
process.env.AMBIENT_ENV === "production"
12+
) {
913
return NextResponse.json({ error: "Not available" }, { status: 404 });
1014
}
1115

workflows/deploy-native-sso.md

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -268,12 +268,12 @@ data:
268268
```bash
269269
cd /path/to/platform
270270
271-
# Apply ClusterRoleBindings separately (kustomize's namespace transformer
272-
# would rewrite the ambient-code subject namespaces)
273-
oc apply -f components/manifests/overlays/hcmais/jsell-sso-poc/clusterrolebindings.yaml
274-
275-
# Apply everything else via kustomize
271+
# Apply kustomize first
276272
oc apply -k components/manifests/overlays/hcmais/jsell-sso-poc/
273+
274+
# Then apply CRBs AFTER kustomize to restore the ambient-code subjects
275+
# that kustomize's namespace transformer overwrites
276+
oc apply -f components/manifests/overlays/hcmais/jsell-sso-poc/clusterrolebindings.yaml
277277
```
278278

279279
Verify all deployments come up:

0 commit comments

Comments
 (0)