HYPERFLEET-492 - refactor: replace OCM auth client with JWT-based auth#120
HYPERFLEET-492 - refactor: replace OCM auth client with JWT-based auth#120kuudori wants to merge 3 commits intoopenshift-hyperfleet:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR removes OCM/ocm-sdk-go integration (clients, mocks, logger bridge, flags) and replaces OCM-based authentication with a project-provided JWT middleware that validates RS256 tokens using JWKS (file and/or URL) with issuer/audience checks and JWKS rotation support (github.com/golang-jwt/jwt/v5, MicahParks/keyfunc/jwkset). It adds JWT configuration, flags and validation, updates config loading/dumping and server wiring to use the new JWT handler (with public-path allowlist and graceful Close), replaces test OCM account types with a local TestAccount and JWT helpers, removes OCM test mocks/benchmarks, renames the environment selector to HYPERFLEET_ENV across env handling, Makefile and docs, and updates documentation and CHANGELOG accordingly. Sequence Diagram(s)sequenceDiagram
participant Client
participant JWTHandler as JWT Handler
participant Parser as JWT Parser
participant JWKS as JWKS Provider (file/HTTP)
participant MainHandler
Client->>JWTHandler: HTTP Request (Authorization: Bearer <token>)
alt Public Path (regex match)
JWTHandler->>MainHandler: Forward Request
MainHandler->>Client: Response
else Protected Path
JWTHandler->>JWTHandler: Check Authorization header format
alt Missing/Malformed
JWTHandler->>Client: 401 Unauthorized
else Well-formed Bearer
JWTHandler->>Parser: Parse & validate token (RS256, exp, issuer, audience)
Parser->>JWKS: Resolve key by kid (read file or fetch/refresh from URL)
alt Key resolution or parse error
Parser->>JWTHandler: Validation Error
JWTHandler->>Client: 401 Unauthorized
else Validation success
JWTHandler->>JWTHandler: Store *jwt.Token in request context
JWTHandler->>MainHandler: Forward Request (with token context)
MainHandler->>Client: Response
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/logging.md (1)
535-538:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove stale OCM environment variable references.
The test examples still reference
OCM_ENV=integration_testing, but the OCM integration has been removed from the codebase per this PR's objectives. These environment variable references should be removed to align with the JWT-based authentication changes and prevent user confusion.📝 Proposed fix to remove OCM_ENV references
# Run tests with debug logging -HYPERFLEET_LOGGING_LEVEL=debug OCM_ENV=integration_testing go test ./test/integration/... +HYPERFLEET_LOGGING_LEVEL=debug go test ./test/integration/... # Run tests without OTel -HYPERFLEET_TRACING_ENABLED=false OCM_ENV=integration_testing go test ./... +HYPERFLEET_TRACING_ENABLED=false go test ./...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/logging.md` around lines 535 - 538, The doc snippets still export the removed environment variable OCM_ENV (e.g., "OCM_ENV=integration_testing") in the test command examples; remove all occurrences of the OCM_ENV=integration_testing token from the examples in docs/logging.md (and any other similar command examples) so the lines read only the remaining env vars (e.g., HYPERFLEET_LOGGING_LEVEL=debug go test ... and HYPERFLEET_TRACING_ENABLED=false go test ...); search for the symbol OCM_ENV in the file and delete those assignments to avoid referencing the removed OCM integration.
🧹 Nitpick comments (1)
test/helper.go (1)
577-589: ⚡ Quick winInclude audience claim in helper JWTs when configured.
Line 579 mirrors configured issuer, but helper tokens ignore configured audience. When
server.jwt.audienceis set, these tokens won’t match middleware expectations.Proposed update
func (helper *Helper) CreateJWTString(account *TestAccount) string { claims := jwt.MapClaims{ "iss": helper.Env().Config.Server.JWT.IssuerURL, "username": strings.ToLower(account.Username), "first_name": account.FirstName, "last_name": account.LastName, "typ": "Bearer", "iat": time.Now().Unix(), "exp": time.Now().Add(1 * time.Hour).Unix(), } + if aud := helper.Env().Config.Server.JWT.Audience; aud != "" { + claims["aud"] = aud + } if account.Email != "" { claims["email"] = account.Email }As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/helper.go` around lines 577 - 589, The CreateJWTString helper currently sets "iss" but omits the configured audience; update Helper.CreateJWTString to read the configured audience (helper.Env().Config.Server.JWT.Audience) and, when non-empty, set claims["aud"] = that value so generated test JWTs include the expected audience claim and will match middleware validation; locate this change inside the CreateJWTString method where claims are built and add the conditional audience assignment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 12-13: Update the placeholder PR links in CHANGELOG.md that point
to "/pull/TBD" (e.g., the JWT auth entry and the Hard deletion entry and the
other occurrences at lines referenced) by replacing "TBD" with the actual PR
numbers for those changes; search for the "/pull/TBD" pattern in CHANGELOG.md
and substitute each with the correct numeric PR identifier so the links resolve
correctly and repeat for the occurrences noted (lines ~32-33 and ~45).
In `@docs/development.md`:
- Around line 121-125: Split the single blocking code block into two separate
terminal examples so readers can run the server and then call the API: create a
"Terminal 1" code block containing only the blocking command `make run` to start
the server, and create a "Terminal 2" code block containing the `curl -H
"Authorization: Bearer ${TOKEN}"
http://localhost:8000/api/hyperfleet/v1/clusters` command; ensure the text
labels clarify that Terminal 1 must be running before executing Terminal 2.
In `@pkg/config/flags.go`:
- Around line 32-33: Add documentation entries for the two new CLI flags added
via cmd.Flags().String for "--server-jwt-issuer-url" and "--server-jwt-audience"
to the configuration reference (docs/config.md): describe each flag, show the
default values (defaults.JWT.IssuerURL and defaults.JWT.Audience), indicate that
issuer URL is used for token validation and audience is optional, and add them
to the CLI flags table so operators can discover and configure server JWT
validation.
---
Outside diff comments:
In `@docs/logging.md`:
- Around line 535-538: The doc snippets still export the removed environment
variable OCM_ENV (e.g., "OCM_ENV=integration_testing") in the test command
examples; remove all occurrences of the OCM_ENV=integration_testing token from
the examples in docs/logging.md (and any other similar command examples) so the
lines read only the remaining env vars (e.g., HYPERFLEET_LOGGING_LEVEL=debug go
test ... and HYPERFLEET_TRACING_ENABLED=false go test ...); search for the
symbol OCM_ENV in the file and delete those assignments to avoid referencing the
removed OCM integration.
---
Nitpick comments:
In `@test/helper.go`:
- Around line 577-589: The CreateJWTString helper currently sets "iss" but omits
the configured audience; update Helper.CreateJWTString to read the configured
audience (helper.Env().Config.Server.JWT.Audience) and, when non-empty, set
claims["aud"] = that value so generated test JWTs include the expected audience
claim and will match middleware validation; locate this change inside the
CreateJWTString method where claims are built and add the conditional audience
assignment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: a150046b-e9c4-44a3-97cd-a4d7d090571c
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (35)
CHANGELOG.mdMakefilePREREQUISITES.mdcmd/hyperfleet-api/environments/e_development.gocmd/hyperfleet-api/environments/e_integration_testing.gocmd/hyperfleet-api/environments/e_production.gocmd/hyperfleet-api/environments/e_unit_testing.gocmd/hyperfleet-api/environments/framework.gocmd/hyperfleet-api/environments/framework_test.gocmd/hyperfleet-api/environments/types.gocmd/hyperfleet-api/server/api_server.goconfigs/config.yaml.exampledocs/authentication.mddocs/config.mddocs/development.mddocs/logging.mdgo.modpkg/auth/authz_middleware.gopkg/auth/context.gopkg/auth/jwt_handler.gopkg/auth/jwt_handler_test.gopkg/client/ocm/authorization.gopkg/client/ocm/authorization_mock.gopkg/client/ocm/client.gopkg/config/config.gopkg/config/dump.gopkg/config/flags.gopkg/config/helpers_test.gopkg/config/loader.gopkg/config/loader_test.gopkg/config/ocm.gopkg/config/server.gopkg/logger/ocm_bridge.gotest/helper.gotest/mocks/ocm.go
💤 Files with no reviewable changes (16)
- pkg/config/helpers_test.go
- PREREQUISITES.md
- configs/config.yaml.example
- pkg/client/ocm/authorization_mock.go
- pkg/auth/authz_middleware.go
- pkg/logger/ocm_bridge.go
- cmd/hyperfleet-api/environments/e_integration_testing.go
- pkg/config/ocm.go
- cmd/hyperfleet-api/environments/e_development.go
- cmd/hyperfleet-api/environments/e_unit_testing.go
- cmd/hyperfleet-api/environments/framework_test.go
- pkg/config/config.go
- pkg/config/loader_test.go
- pkg/client/ocm/client.go
- pkg/client/ocm/authorization.go
- test/mocks/ocm.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/authentication.md`:
- Around line 35-52: The docs still reference JWT_ISSUER / JWT_AUDIENCE and
AUTH_ENABLED but the app loader expects HYPERFLEET_SERVER_JWT_* env vars; update
the Production Mode section and examples to use HYPERFLEET_SERVER_JWT_ENABLED,
HYPERFLEET_SERVER_JWT_ISSUER_URL, and HYPERFLEET_SERVER_JWT_AUDIENCE (and
replace any AUTH_ENABLED mentions with HYPERFLEET_SERVER_JWT_ENABLED), ensure
the example curl/launch commands and the "JWT Authentication" paragraph reflect
these exact env var names and mention RS256 verification remains in use so
operators can configure auth correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 98e77616-8752-421c-a705-3ede03ff6493
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (35)
CHANGELOG.mdMakefilePREREQUISITES.mdcmd/hyperfleet-api/environments/e_development.gocmd/hyperfleet-api/environments/e_integration_testing.gocmd/hyperfleet-api/environments/e_production.gocmd/hyperfleet-api/environments/e_unit_testing.gocmd/hyperfleet-api/environments/framework.gocmd/hyperfleet-api/environments/framework_test.gocmd/hyperfleet-api/environments/types.gocmd/hyperfleet-api/server/api_server.goconfigs/config.yaml.exampledocs/authentication.mddocs/config.mddocs/development.mddocs/logging.mdgo.modpkg/auth/authz_middleware.gopkg/auth/context.gopkg/auth/jwt_handler.gopkg/auth/jwt_handler_test.gopkg/client/ocm/authorization.gopkg/client/ocm/authorization_mock.gopkg/client/ocm/client.gopkg/config/config.gopkg/config/dump.gopkg/config/flags.gopkg/config/helpers_test.gopkg/config/loader.gopkg/config/loader_test.gopkg/config/ocm.gopkg/config/server.gopkg/logger/ocm_bridge.gotest/helper.gotest/mocks/ocm.go
💤 Files with no reviewable changes (16)
- cmd/hyperfleet-api/environments/e_unit_testing.go
- cmd/hyperfleet-api/environments/e_integration_testing.go
- pkg/client/ocm/authorization_mock.go
- cmd/hyperfleet-api/environments/e_development.go
- PREREQUISITES.md
- cmd/hyperfleet-api/environments/framework_test.go
- pkg/config/loader_test.go
- pkg/logger/ocm_bridge.go
- pkg/client/ocm/authorization.go
- pkg/config/helpers_test.go
- configs/config.yaml.example
- pkg/config/ocm.go
- pkg/client/ocm/client.go
- test/mocks/ocm.go
- pkg/auth/authz_middleware.go
- pkg/config/config.go
✅ Files skipped from review due to trivial changes (2)
- cmd/hyperfleet-api/environments/e_production.go
- docs/logging.md
🚧 Files skipped from review as they are similar to previous changes (3)
- Makefile
- cmd/hyperfleet-api/environments/framework.go
- CHANGELOG.md
| return &jwtHandler{ | ||
| keyfunc: kf, | ||
| parser: jwt.NewParser(parserOpts...), | ||
| publicPatterns: publicPatterns, |
There was a problem hiding this comment.
Warning
Blocking
Category: Bug
If buildKeyfunc succeeds (potentially starting a JWKS refresh goroutine) but a subsequent regexp.Compile for public paths fails, NewJWTHandler returns an error without cancelling the keyfunc's background goroutine. This leaks the goroutine.
Fix: use a cancellable context and cancel on error:
ctx, cancel := context.WithCancel(ctx)
kf, err := buildKeyfunc(ctx, cfg)
if err != nil {
cancel()
return nil, fmt.Errorf("failed to build JWKS keyfunc: %w", err)
}
// ... compile regexes ...
for _, pattern := range cfg.PublicPaths {
re, err := regexp.Compile(pattern)
if err != nil {
cancel() // stop the keyfunc goroutine
return nil, fmt.Errorf("failed to compile public path pattern %q: %w", pattern, err)
}
publicPatterns = append(publicPatterns, re)
}
// Store cancel on the handler for later shutdown
return &jwtHandler{
keyfunc: kf,
parser: parser,
publicPatterns: publicPatterns,
next: cfg.Next,
cancel: cancel,
}, nilThere was a problem hiding this comment.
Good catch, fixed. cancel() on all error paths.
|
|
||
| // ACLConfig holds access control list configuration | ||
| // Deprecated: ACLConfig is kept for Helm values.yaml backward compatibility. | ||
| // ACL checking was provided by the OCM SDK handler and is no longer functional. |
There was a problem hiding this comment.
Warning
Blocking
Category: Security
The old OCM handler consumed ACLConfig.File via .ACLFile() for access control enforcement. The new JWT handler has no ACL support — the config value is never read.
The --server-acl-file flag and SERVER_ACL_FILE env var are still registered and accepted without error, but they silently do nothing. If any deployment relied on ACL files for access control, that restriction is now gone with no warning.
Suggested actions:
- Verify no production deployment uses ACL files
- Either remove the
ACLConfigstruct, flag, and env var entirely, or add a startup warning whenserver.acl.fileis configured:"ACL file is configured but no longer supported after OCM removal" - Update
docs/config.mdto mark the ACL config as deprecated/non-functional
There was a problem hiding this comment.
Thanks, added startup warning + marked deprecated in docs. Kept struct for Helm compat.
| w.Header().Set("Content-Type", "application/json") | ||
| fmt.Fprintf(w, `{"keys":[%s]}`, string(jwkBytes)) | ||
| })) | ||
| } |
There was a problem hiding this comment.
Warning
Blocking
Category: Pattern
All tests use the URL-only JWKS path via newJWKSServer(). Two code paths in buildKeyfunc have zero test coverage:
- File-only path (
KeysFileset,KeysURLempty) — reads JWKS from disk viaos.ReadFile+keyfunc.NewJWKSetJSON - Combined path (both set) — creates
jwkset.NewHTTPClientwith merged storage, the most complex branch
Also, JWTConfig.Validate() has no dedicated tests (JWT disabled → nil, JWT enabled + empty IssuerURL → error).
Suggested test additions:
func TestBuildKeyfunc_FileOnly(t *testing.T) {
// Write a JWKS JSON file to t.TempDir()
// Call NewJWTHandler with KeysFile set, KeysURL empty
// Verify tokens signed with the file's key are accepted
}
func TestBuildKeyfunc_Combined(t *testing.T) {
// Set up both a JWKS file and a JWKS HTTP server
// Call NewJWTHandler with both KeysFile and KeysURL
// Verify tokens signed with either key are accepted
}
func TestJWTConfig_Validate(t *testing.T) {
// Enabled=false → nil
// Enabled=true, IssuerURL="" → error
// Enabled=true, IssuerURL="https://..." → nil
}There was a problem hiding this comment.
Good point, added tests for file-only, combined, Validate(), Close().
| RegisterTestingT(t) | ||
| token := signToken(t, privateKey, jwt.MapClaims{ | ||
| "iss": "https://test-issuer.example.com", | ||
| "exp": time.Now().Add(-time.Hour).Unix(), |
There was a problem hiding this comment.
Warning
Blocking
Category: Pattern
All 401 tests only check rr.Code but never verify the response body. The distinct error codes (CodeAuthNoCredentials, CodeAuthExpiredToken, CodeAuthInvalidCredentials) and RFC 9457 Problem Details format are untested.
A regression that changes the response body format or returns wrong error codes would not be caught.
Suggestion — at minimum, assert on the error code for each case:
t.Run("expired token returns 401 with expired code", func(t *testing.T) {
// ... existing setup ...
rr := serve(handler, "/protected", "Bearer "+token)
Expect(rr.Code).To(Equal(http.StatusUnauthorized))
Expect(rr.Header().Get("Content-Type")).To(ContainSubstring("application/problem+json"))
Expect(rr.Body.String()).To(ContainSubstring("HYPERFLEET-AUTH-EXPIRED"))
})
t.Run("missing header returns 401 with no-credentials code", func(t *testing.T) {
rr := serve(handler, "/protected", "")
Expect(rr.Code).To(Equal(http.StatusUnauthorized))
Expect(rr.Body.String()).To(ContainSubstring("HYPERFLEET-AUTH-NO-CREDENTIALS"))
})There was a problem hiding this comment.
Agreed, added response body assertions for all 401 scenarios (Content-Type, error codes, status).
|
Tip nit — non-blocking suggestion Category: Pattern —
// EnvironmentStringKey is the env var used to select the runtime environment.
// Named "OCM_ENV" for backward compatibility with existing deployments.
EnvironmentStringKey = "OCM_ENV"Or consider a follow-up ticket to rename it with a deprecation period. |
|
Tip nit — non-blocking suggestion Category: Improvement —
Since ACL enforcement was removed with the OCM handler, these silently do nothing. Consider:
|
Remove the OCM client dependency and replace authentication with a self-contained JWT handler using JWKS endpoint validation. - Add pkg/auth/jwt_handler.go with RS256 JWT validation via JWKS - Add pkg/auth/jwt_handler_test.go with full handler test coverage - Remove pkg/client/ocm/ (authorization, authorization_mock, client) - Remove pkg/config/ocm.go and OCM-specific config flags - Remove pkg/logger/ocm_bridge.go (OCM SDK log adapter) - Remove test/mocks/ocm.go (no longer needed) - Update pkg/auth/authz_middleware.go to use new JWT handler - Update pkg/auth/context.go to remove OCM identity types - Update environments to drop OCM env var requirements - Update go.mod/go.sum: add MicahParks/jwkset, keyfunc; remove ocm-sdk-go - Update docs/authentication.md, docs/config.md to reflect new auth setup - Update configs/config.yaml.example with JWT/JWKS config fields
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/hyperfleet-api/environments/framework.go`:
- Around line 69-70: SetEnvironmentDefaults currently uses the possibly stale
e.Name captured earlier; before calling
environments[e.Name].EnvironmentDefaults() refresh e.Name from the real env var
(e.g. read os.Getenv("HYPERFLEET_ENV") or call the existing Initialize helper
that re-reads the env) so the defaults come from the actual current environment;
update e.Name first, then call setFlagDefaults(flags,
environments[e.Name].EnvironmentDefaults()).
In `@cmd/hyperfleet-api/environments/types.go`:
- Line 17: The code currently defines EnvironmentStringKey = "HYPERFLEET_ENV"
and will drop support for existing OCM_ENV users; change the environment lookup
to accept both keys: keep EnvironmentStringKey as "HYPERFLEET_ENV", add a new
constant (e.g., OCMEnvironmentStringKey = "OCM_ENV"), and update the
environment-parsing logic (wherever EnvironmentStringKey is read) to first read
EnvironmentStringKey, then if empty read OCMEnvironmentStringKey and emit a
warning log about the deprecated key being used; ensure the preferred key wins
when both are set and that the warning is only logged when falling back to
OCMEnvironmentStringKey.
In `@pkg/auth/context.go`:
- Around line 56-64: GetAuthPayloadFromContext currently builds a Payload from
JWT claims but never sets Payload.Issuer, so populate payload.Issuer from
claims["iss"] before returning; locate GetAuthPayloadFromContext and after
parsing claims (jwt.MapClaims) extract the "iss" claim (ensure it exists and
cast to string safely) and assign it to the Payload.Issuer field so callers
receive the token issuer.
In `@pkg/config/loader.go`:
- Around line 179-181: The JWT validation currently only calls
config.Server.JWT.Validate(); after that, when server.jwt.enabled is true you
must also validate that a JWKS source is configured: check the JWK settings
(e.g., config.Server.JWK.CertFile and config.Server.JWK.CertURL) and return a
descriptive error if both are empty, so loading fails early instead of letting
buildKeyfunc in pkg/auth/jwt_handler.go reject the config at runtime. Ensure the
check runs after calling config.Server.JWT.Validate() and use the same
error-wrapping style as the existing return.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 4033af95-d5f6-4121-83ce-61bdbc17bb0e
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (41)
AGENTS.mdCHANGELOG.mdCLAUDE.mdCONTRIBUTING.mdMakefilePREREQUISITES.mdcmd/hyperfleet-api/environments/e_development.gocmd/hyperfleet-api/environments/e_integration_testing.gocmd/hyperfleet-api/environments/e_production.gocmd/hyperfleet-api/environments/e_unit_testing.gocmd/hyperfleet-api/environments/framework.gocmd/hyperfleet-api/environments/framework_test.gocmd/hyperfleet-api/environments/types.gocmd/hyperfleet-api/servecmd/cmd.gocmd/hyperfleet-api/server/api_server.goconfigs/config.yaml.exampledocs/authentication.mddocs/config.mddocs/development.mddocs/logging.mdgo.modpkg/auth/authz_middleware.gopkg/auth/context.gopkg/auth/jwt_handler.gopkg/auth/jwt_handler_test.gopkg/client/ocm/authorization.gopkg/client/ocm/authorization_mock.gopkg/client/ocm/client.gopkg/config/config.gopkg/config/dump.gopkg/config/flags.gopkg/config/helpers_test.gopkg/config/loader.gopkg/config/loader_test.gopkg/config/ocm.gopkg/config/server.gopkg/config/server_test.gopkg/logger/ocm_bridge.gotest/CLAUDE.mdtest/helper.gotest/mocks/ocm.go
💤 Files with no reviewable changes (15)
- pkg/config/config.go
- pkg/config/loader_test.go
- pkg/client/ocm/authorization_mock.go
- cmd/hyperfleet-api/environments/e_development.go
- cmd/hyperfleet-api/environments/e_integration_testing.go
- pkg/client/ocm/client.go
- cmd/hyperfleet-api/environments/e_unit_testing.go
- pkg/logger/ocm_bridge.go
- pkg/auth/authz_middleware.go
- pkg/config/helpers_test.go
- pkg/config/ocm.go
- test/mocks/ocm.go
- PREREQUISITES.md
- configs/config.yaml.example
- pkg/client/ocm/authorization.go
✅ Files skipped from review due to trivial changes (1)
- CONTRIBUTING.md
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/auth/jwt_handler.go`:
- Around line 108-110: The Authorization Bearer check in jwt_handler.go
currently uses strings.CutPrefix(authHeader, "Bearer ") which rejects case
variants; update the logic in the handler that reads authHeader (the
tokenString/found branch) to perform a case-insensitive prefix check (e.g.,
compare the first 7 characters with "Bearer " using a case-insensitive
comparison or normalize authHeader to a consistent case before cutting) so that
"bearer", "BEARER", etc. are accepted and still call handleError(r.Context(), w,
r, hferrors.CodeAuthInvalidCredentials, ...) when no valid Bearer prefix is
present.
- Around line 60-67: The JWT handler currently skips issuer/audience checks when
cfg.IssuerURL or cfg.Audience are empty, weakening authentication; change this
to fail startup instead of warning: in the JWT initialization code that builds
parserOpts (references cfg.IssuerURL, cfg.Audience, parserOpts, jwt.WithIssuer,
jwt.WithAudience and logger.Warn), return a configuration/initialization error
(or log and exit) if either IssuerURL or Audience is empty so parserOpts always
includes jwt.WithIssuer and jwt.WithAudience; update the function signature to
propagate the error back to the caller (or cause process failure) so missing
values are rejected at startup rather than disabling validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 1e124f1f-c3ef-42dc-a1f6-0ab9e34d7b2d
📒 Files selected for processing (11)
AGENTS.mdCLAUDE.mdCONTRIBUTING.mdMakefilecmd/hyperfleet-api/environments/framework.gocmd/hyperfleet-api/environments/framework_test.gocmd/hyperfleet-api/environments/types.gopkg/auth/context.gopkg/auth/jwt_handler.gopkg/config/loader.gotest/CLAUDE.md
✅ Files skipped from review due to trivial changes (2)
- test/CLAUDE.md
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (1)
- CONTRIBUTING.md
Last OCM reference in the codebase. Safe to rename now since no long-running deployments consume the old variable.
AuthorizationMiddleware was never enabled (mock passthrough only). Removed: interface, mock, Clients struct, ConfigDefaults struct, AuthzConfig, authz error codes, and all threading through routes, plugins, and environments. Updated Helm chart, docs, and Makefile.
2 participants
Summary
Test Plan
make test-allpassesmake lintpassesmake test-helm(if applicable)Summary by CodeRabbit
New Features
Removed
Documentation
Tests
Chores