-
Notifications
You must be signed in to change notification settings - Fork 21
HYPERFLEET-492 - refactor: replace OCM auth client with JWT-based auth #120
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
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
|
|
@@ -9,9 +9,9 @@ import ( | |
| "time" | ||
|
|
||
| gorillahandlers "github.com/gorilla/handlers" | ||
| "github.com/openshift-online/ocm-sdk-go/authentication" | ||
|
|
||
| "github.com/openshift-hyperfleet/hyperfleet-api/cmd/hyperfleet-api/environments" | ||
| "github.com/openshift-hyperfleet/hyperfleet-api/pkg/auth" | ||
| "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" | ||
| ) | ||
|
|
||
|
|
@@ -34,30 +34,28 @@ func NewAPIServer(tracingEnabled bool) Server { | |
| var mainHandler http.Handler = mainRouter | ||
|
|
||
| if env().Config.Server.JWT.Enabled { | ||
| // Create the logger for the authentication handler using slog bridge | ||
| authnLogger := logger.NewOCMLoggerBridge() | ||
|
|
||
| // Create the handler that verifies that tokens are valid: | ||
| var err error | ||
|
Contributor
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. Warning Blocking Category: Bug
The old OCM client had Suggested approach:
type jwtHandler struct {
keyfunc keyfunc.Keyfunc
parser *jwt.Parser
publicPatterns []*regexp.Regexp
next http.Handler
cancel context.CancelFunc // add this
}
func (h *jwtHandler) Close() {
if h.cancel != nil {
h.cancel()
}
}And in ctx, cancel := context.WithCancel(serverCtx) // not context.Background()
mainHandler, err := auth.NewJWTHandler(ctx, auth.JWTHandlerConfig{...})
// store cancel or handler for shutdown |
||
| mainHandler, err = authentication.NewHandler(). | ||
| Logger(authnLogger). | ||
| KeysFile(env().Config.Server.JWK.CertFile). | ||
| KeysURL(env().Config.Server.JWK.CertURL). | ||
| ACLFile(env().Config.Server.ACL.File). | ||
| Public("^/api/hyperfleet/?$"). | ||
| Public("^/api/hyperfleet/v1/?$"). | ||
| Public("^/api/hyperfleet/v1/openapi/?$"). | ||
| Public("^/api/hyperfleet/v1/openapi.html/?$"). | ||
| Public("^/api/hyperfleet/v1/errors(/.*)?$"). | ||
| Next(mainHandler). | ||
| Build() | ||
| check(err, "Unable to create authentication handler") | ||
| mainHandler, err = auth.NewJWTHandler(context.Background(), auth.JWTHandlerConfig{ | ||
| KeysFile: env().Config.Server.JWK.CertFile, | ||
| KeysURL: env().Config.Server.JWK.CertURL, | ||
| IssuerURL: env().Config.Server.JWT.IssuerURL, | ||
| Audience: env().Config.Server.JWT.Audience, | ||
| PublicPaths: []string{ | ||
| "^/api/hyperfleet/?$", | ||
| "^/api/hyperfleet/v1/?$", | ||
| "^/api/hyperfleet/v1/openapi/?$", | ||
| "^/api/hyperfleet/v1/openapi.html/?$", | ||
| "^/api/hyperfleet/v1/errors(/.*)?$", | ||
| }, | ||
| Next: mainHandler, | ||
| }) | ||
| check(err, "Unable to create JWT authentication handler") | ||
| } | ||
|
|
||
| // Configure CORS for Red Hat console and API access | ||
| mainHandler = gorillahandlers.CORS( | ||
| gorillahandlers.AllowedOrigins([]string{ | ||
| // OCM UI local development URLs | ||
| // Console local development URLs | ||
| "https://qa.foo.redhat.com:1337", | ||
| "https://prod.foo.redhat.com:1337", | ||
| "https://ci.foo.redhat.com:1337", | ||
|
|
||
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.
Tip
nit — non-blocking suggestion
Category: Architecture
After removing the OCM client, several abstractions are now vestigial:
Clientsstruct is empty —OverrideClientsacross all environment impls does nothingAuthorizationMiddlewareinterface has no real implementation (only the passthrough mock). It's still threaded throughRouteRegistrationFuncand plugin registrations as dead weightConfigDefaultsstruct appears unreferencedConsider a follow-up PR to clean up this dead plumbing, or track it as tech debt.