test(auth): add TC-30 subsystem test for auth disabled ignoring garbage headers

[vkolodny]

Summary

• Add TC-30: verify AUTH_DISABLED=true ignores garbage auth headers on protected
  endpoints (/providers, /catalog-items). Placed in the catalog subsystem suite
  which already runs with AUTH_DISABLED=true.

Unlike the unit test in internal/auth/middleware_test.go, this exercises the
full HTTP stack with a real compose config.

Covers FLPATH-3254 test plan item TC-30 (P2).

Note: TC-25 (concurrent JIT for different users) was originally in this PR
but moved to dcm-utilities E2E suite per review feedback — subsystem is not
the right tier for concurrency/deadlock assertions.

Test plan

• make catalog-subsystem-test — 41 specs passed (4 new from TC-30)

[qodo-code-review]

PR Summary by Qodo

Add subsystem tests for auth-disabled header handling and concurrent JIT provisioning

🧪 Tests 🕐 20-40 Minutes

AI Description

• Add TC-30 subsystem coverage to ensure AUTH_DISABLED ignores malformed auth headers.
• Add TC-25 concurrency coverage to validate JIT provisioning across distinct users.
• Verify protected endpoints remain accessible and actor records are created exactly once.

Diagram

graph TD
  A(["Catalog subsystem tests"]) --> B["Catalog Manager API"] --> C["Auth middleware"]
  D(["Auth subsystem tests"]) --> B --> E["JIT provisioning"] --> F["Actor store"]
  E --> G["Keycloak"]

  subgraph Legend
    direction LR
    _t([Test suite]) ~~~ _api([HTTP API]) ~~~ _svc([Service/Logic]) ~~~ _db[(Database)]
  end

Loading

High-Level Assessment

The PR’s approach is appropriate: TC-30 is best validated as an end-to-end subsystem test under AUTH_DISABLED to cover the full HTTP/middleware stack, and TC-25’s concurrency risk is realistically exercised via parallel requests. A unit-test-only approach would miss integration and configuration-driven behavior.

Files changed (2) +117 / -0

Tests (2) +117 / -0

race_test.goAdd TC-25 concurrent JIT provisioning test across distinct users +44/-0

Add TC-25 concurrent JIT provisioning test across distinct users

• Introduces a new Ginkgo test that creates 10 Keycloak users, fires concurrent requests to a protected endpoint, and asserts all requests succeed. Verifies that each external subject results in exactly one actor record, guarding against deadlocks, duplication, or dropped provisioning under concurrency.

test/subsystem/auth/race_test.go

auth_disabled_test.goAdd TC-30 auth-disabled subsystem tests for garbage auth headers +73/-0

Add TC-30 auth-disabled subsystem tests for garbage auth headers

• Adds a new subsystem test file that exercises protected catalog endpoints with malformed/irrelevant auth headers while AUTH_DISABLED is enabled via the subsystem environment. Ensures requests still return HTTP 200 and produce a valid response, covering multiple malformed header combinations.

test/subsystem/catalog/auth_disabled_test.go

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Fragile base URL join ✓ Resolved 🐞 Bug ☼ Reliability

Description

auth_disabled_test.go constructs baseURL by concatenating CATALOG_MANAGER_URL with "/api/v1alpha1",
which can produce "//api/v1alpha1" (trailing slash) or duplicate the API path (if env already
includes it), causing environment-dependent subsystem test failures.

Code

test/subsystem/catalog/auth_disabled_test.go[18]

+	baseURL := envOrDefault("CATALOG_MANAGER_URL", "http://localhost:28080") + "/api/v1alpha1"

Evidence

The new test file uses raw string concatenation for the API base URL. Elsewhere in the same
subsystem suite, the code uses client.APIBaseURL() which explicitly trims trailing slashes and
joins path segments safely, showing the repo’s intended robust approach and highlighting the new
test’s fragility to environment formatting.

test/subsystem/catalog/auth_disabled_test.go[17-23]
test/subsystem/catalog/suite_test.go[30-45]
pkg/catalog/client/http.go[21-28]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`test/subsystem/catalog/auth_disabled_test.go` builds `baseURL` via string concatenation (`CATALOG_MANAGER_URL + "/api/v1alpha1"`). This is sensitive to whether `CATALOG_MANAGER_URL` has a trailing slash or already includes the API path, which can yield malformed/duplicated paths and flaky failures depending on environment configuration.

## Issue Context
The catalog subsystem suite already uses a helper (`client.APIBaseURL`) that safely appends `api/v1alpha1` via `url.JoinPath` and trims trailing slashes.

## Fix Focus Areas
- test/subsystem/catalog/auth_disabled_test.go[17-23]
- test/subsystem/catalog/suite_test.go[30-45]
- pkg/catalog/client/http.go[21-28]

## Suggested change
In `auth_disabled_test.go`, compute the API base URL using `client.APIBaseURL(envOrDefault(...))` (or `url.JoinPath(strings.TrimRight(...), "api", "v1alpha1")`) and `Expect(err).NotTo(HaveOccurred())` before issuing requests.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can route each action level your way: inline, summary, both, or drop

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[chadcrum]

Small metadata nit now that TC-25 was dropped from this PR (per the thread - moving it to dcm-utilities E2E makes sense).

The title, summary, and test plan still read like both TC-25 and TC-30 ship here:

• Title: "TC-25 and TC-30"
• Summary bullet for TC-25 / race_test.go
• Test plan checkbox: "make auth-subsystem-test — TC-25 passed"

Current diff is only auth_disabled_test.go (TC-30). Worth updating those before merge so QE tracking for FLPATH-3254 does not mark TC-25 done off this PR alone.

[jordigilh]

Heads-up: the TC-30 specs added here are failing catalog-subsystem / blackbox on main since this merged — confirmed on main's last 2 runs (unrelated to any other PR's changes). GET /providers returns 404 instead of 200 while /catalog-items in the same spec passes, so it looks like an environment/wiring gap for the SP-provider routes in that compose stack rather than an auth bug. It's currently blocking the required check on every open PR (including ours, #52). Could you take a look?

[vkolodny]

@jordigilh PR #51 removed /providers after this merged. Fix is up in #54 (replaces with /service-types).