fix: added clientID/secret based access #160
Conversation
On-behalf-of: SAP [email protected]
WalkthroughSwitch Keycloak auth from password-based OAuth2 to client-credentials flow, add two indirect go.mod deps, replace Keycloak password fields with a client secret in InviteConfig, remove runtime password-file reading, and update New() signatures and call sites. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
On-behalf-of: SAP [email protected]
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmd/operator.go (1)
8-8: Remove unused import.The
ospackage is imported but doesn't appear to be used in this file.Apply this diff:
- "os"internal/controller/invite_controller.go (1)
24-27: Consider making namespace configurable.The namespace
platform-mesh-systemis hardcoded, which reduces flexibility. If there's a possibility of deploying to different namespaces, consider making this configurable through the Config struct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
cmd/operator.go(3 hunks)go.mod(2 hunks)internal/config/config.go(1 hunks)internal/controller/invite_controller.go(2 hunks)internal/subroutine/invite/subroutine.go(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: aaronschweig
Repo: platform-mesh/security-operator PR: 139
File: internal/subroutine/invite/subroutine.go:182-188
Timestamp: 2025-10-27T08:44:10.551Z
Learning: In the platform-mesh/security-operator repository, when creating Keycloak realms, a client is created with the same identifier as the realm name. Therefore, using the realm name as the `client_id` parameter in Keycloak API calls (such as execute-actions-email) is correct and intentional.
📚 Learning: 2025-10-24T06:20:22.789Z
Learnt from: aaronschweig
Repo: platform-mesh/security-operator PR: 136
File: internal/subroutine/authorization_model.go:74-81
Timestamp: 2025-10-24T06:20:22.789Z
Learning: In internal/subroutine/authorization_model.go, the NewDiscoveryClientFunc factory does not need to return an error because the *rest.Config passed to it is copied from an already validated manager config (a.mgr.GetLocalManager().GetConfig()), so the config is guaranteed to be valid at that point.
Applied to files:
cmd/operator.gointernal/subroutine/invite/subroutine.go
🧬 Code graph analysis (2)
cmd/operator.go (2)
internal/subroutine/invite/subroutine.go (1)
New(51-73)internal/controller/invite_controller.go (1)
NewInviteReconciler(33-57)
internal/controller/invite_controller.go (2)
internal/config/config.go (1)
Config(11-45)internal/subroutine/invite/subroutine.go (1)
New(51-73)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: pipe / lint / lint
- GitHub Check: pipe / dockerBuild / docker
- GitHub Check: pipe / testSource / test
🔇 Additional comments (5)
internal/config/config.go (1)
5-7: Configuration changes align with client credentials flow.The default changes properly reflect the switch from admin CLI authentication to service client credentials. The
KeycloakClientSecretfield correctly stores the Kubernetes Secret name rather than the secret value itself.cmd/operator.go (2)
151-156: In-cluster client creation looks good.The runtime client is properly initialized with error handling. Using
ctrl.GetConfigOrDie()is appropriate for the operator initialization context.
169-169: Reconciler initialization properly updated.The call to
NewInviteReconcilercorrectly includes the newruntimeClientparameter, matching the updated function signature.internal/subroutine/invite/subroutine.go (1)
51-72: OAuth2 client credentials flow properly implemented.The switch from password credentials to client credentials flow is correctly implemented. The
clientcredentials.Configwill automatically handle token acquisition and refresh for all HTTP requests made with the returned client.internal/controller/invite_controller.go (1)
33-46: Secret loaded at startup only.Note that the Keycloak client secret is loaded once during operator initialization. If the Secret is updated in Kubernetes, the operator will need to be restarted to pick up the new value. This is typical behavior for service credentials.
Confirm this startup-time loading behavior aligns with your operational requirements for credential rotation.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/operator.go(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: aaronschweig
Repo: platform-mesh/security-operator PR: 139
File: internal/subroutine/invite/subroutine.go:182-188
Timestamp: 2025-10-27T08:44:10.551Z
Learning: In the platform-mesh/security-operator repository, when creating Keycloak realms, a client is created with the same identifier as the realm name. Therefore, using the realm name as the `client_id` parameter in Keycloak API calls (such as execute-actions-email) is correct and intentional.
📚 Learning: 2025-10-24T06:20:22.789Z
Learnt from: aaronschweig
Repo: platform-mesh/security-operator PR: 136
File: internal/subroutine/authorization_model.go:74-81
Timestamp: 2025-10-24T06:20:22.789Z
Learning: In internal/subroutine/authorization_model.go, the NewDiscoveryClientFunc factory does not need to return an error because the *rest.Config passed to it is copied from an already validated manager config (a.mgr.GetLocalManager().GetConfig()), so the config is guaranteed to be valid at that point.
Applied to files:
cmd/operator.go
🧬 Code graph analysis (1)
cmd/operator.go (2)
internal/subroutine/invite/subroutine.go (1)
New(51-73)internal/controller/invite_controller.go (1)
NewInviteReconciler(33-57)
🔇 Additional comments (1)
cmd/operator.go (1)
168-168: LGTM!The
runtimeClientis correctly passed toNewInviteReconcileras the third parameter, matching the updated constructor signature. This enables the reconciler to fetch the Keycloak client secret from a Kubernetes Secret.
On-behalf-of: SAP [email protected]
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/config/config.go (1)
7-7: Consider validation for required KeycloakClientSecret.The
KeycloakClientSecretis now essential for authentication but has no default value or validation. If this field is empty at runtime, authentication will fail silently.Consider adding validation in the config loading process or at the point where
invite.New()is called to ensureKeycloakClientSecretis non-empty.internal/subroutine/invite/subroutine_test.go (2)
350-356: Consider updating test client ID to match new default.The test uses
"admin-cli"as theKeycloakClientID, but the new default ininternal/config/config.goline 5 is"security-operator". While this doesn't affect test functionality (due to mocking), aligning tests with production defaults improves consistency.Apply this diff to align with the new default:
s, err := invite.New(ctx, &config.Config{ Invite: config.InviteConfig{ KeycloakBaseURL: srv.URL, - KeycloakClientID: "admin-cli", + KeycloakClientID: "security-operator", }, BaseDomain: "portal.dev.local:8443", }, mgr)
382-387: Consider updating test client ID to match new default.Same as the previous test case, this test uses
"admin-cli"instead of the new default"security-operator".Apply this diff:
s, err := invite.New(ctx, &config.Config{ Invite: config.InviteConfig{ KeycloakBaseURL: srv.URL, - KeycloakClientID: "admin-cli", + KeycloakClientID: "security-operator", }, }, nil)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/config/config.go(1 hunks)internal/controller/invite_controller.go(1 hunks)internal/subroutine/invite/subroutine.go(2 hunks)internal/subroutine/invite/subroutine_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/subroutine/invite/subroutine.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: aaronschweig
Repo: platform-mesh/security-operator PR: 139
File: internal/subroutine/invite/subroutine.go:182-188
Timestamp: 2025-10-27T08:44:10.551Z
Learning: In the platform-mesh/security-operator repository, when creating Keycloak realms, a client is created with the same identifier as the realm name. Therefore, using the realm name as the `client_id` parameter in Keycloak API calls (such as execute-actions-email) is correct and intentional.
📚 Learning: 2025-10-24T06:20:22.789Z
Learnt from: aaronschweig
Repo: platform-mesh/security-operator PR: 136
File: internal/subroutine/authorization_model.go:74-81
Timestamp: 2025-10-24T06:20:22.789Z
Learning: In internal/subroutine/authorization_model.go, the NewDiscoveryClientFunc factory does not need to return an error because the *rest.Config passed to it is copied from an already validated manager config (a.mgr.GetLocalManager().GetConfig()), so the config is guaranteed to be valid at that point.
Applied to files:
internal/subroutine/invite/subroutine_test.go
🧬 Code graph analysis (1)
internal/controller/invite_controller.go (1)
internal/subroutine/invite/subroutine.go (1)
New(51-73)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: pipe / dockerBuild / docker
- GitHub Check: pipe / lint / lint
🔇 Additional comments (1)
internal/controller/invite_controller.go (1)
25-40: LGTM! Simplified initialization.The removal of password file reading and the simplified call to
invite.New(ctx, cfg, mgr)is a good improvement. The authentication is now handled through client credentials flow configured viacfg.Invite.KeycloakClientSecret.Note: The past review comment about validating Secret data keys doesn't appear to apply to the current code, as there's no Kubernetes Secret reading in this file.
On-behalf-of: SAP [email protected]
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/config/config.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: aaronschweig
Repo: platform-mesh/security-operator PR: 139
File: internal/subroutine/invite/subroutine.go:182-188
Timestamp: 2025-10-27T08:44:10.551Z
Learning: In the platform-mesh/security-operator repository, when creating Keycloak realms, a client is created with the same identifier as the realm name. Therefore, using the realm name as the `client_id` parameter in Keycloak API calls (such as execute-actions-email) is correct and intentional.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: pipe / dockerBuild / docker
- GitHub Check: pipe / lint / lint
🔇 Additional comments (1)
internal/config/config.go (1)
3-7: LGTM! Client secret is securely handled.The struct changes migrate cleanly to client credentials flow:
- Removed password-based fields (
KeycloakUser,KeycloakPasswordFile)- Added
KeycloakClientSecretwith no default value (correct security practice)Verification confirms secure handling:
- Secret is passed directly to the oauth2
clientcredentialslibrary and never logged- HTTP client is created once and cached; secret is not repeatedly exposed
- Error logging only captures specific safe fields (email, user ID, client ID), never the config or secret
- No
String()orMarshalJSON()methods exist that could serialize the secret
On-behalf-of: SAP [email protected]
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/subroutine/invite/subroutine_test.go (1)
41-50: Consider validating grant_type in the mock token endpoint.The mock token endpoint returns a token without validating the
grant_typeparameter. Since the PR switches from password flow to client credentials flow, consider adding validation to ensure the correct grant type is being used.For example:
mux.HandleFunc("/realms/master/protocol/openid-connect/token", func(w http.ResponseWriter, r *http.Request) { + err := r.ParseForm() + assert.NoError(t, err) + assert.Equal(t, "client_credentials", r.FormValue("grant_type")) w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusOK) err := json.NewEncoder(w).Encode(&map[string]string{ "access_token": "token", }) assert.NoError(t, err) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/subroutine/invite/subroutine_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: aaronschweig
Repo: platform-mesh/security-operator PR: 139
File: internal/subroutine/invite/subroutine.go:182-188
Timestamp: 2025-10-27T08:44:10.551Z
Learning: In the platform-mesh/security-operator repository, when creating Keycloak realms, a client is created with the same identifier as the realm name. Therefore, using the realm name as the `client_id` parameter in Keycloak API calls (such as execute-actions-email) is correct and intentional.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pipe / dockerBuild / docker
🔇 Additional comments (2)
internal/subroutine/invite/subroutine_test.go (2)
382-387: Same verification needed for KeycloakClientSecret.Similar to the previous test case, this configuration also doesn't specify
KeycloakClientSecret. While this test only exercises helper functions (GetName,Finalizers,Finalize) and may not trigger authentication, it's worth confirming the intended behavior for consistency.
350-356: No action required—KeycloakClientSecret handling in test is correct.Both test cases consistently omit
KeycloakClientSecretbecause the mocked OIDC provider doesn't validate credentials—it returns a hardcoded token regardless of the secret value. This is intentional and correct for unit testing with mocks. Providing a dummy secret is unnecessary.
Yes, it's already set up here client roles and client |
On-behalf-of: SAP [email protected]
Summary by CodeRabbit
Chores
Configuration
Tests