Add option to configure authentication flows for new realm#126
Add option to configure authentication flows for new realm#126Prabhjot-Sethi wants to merge 2 commits into
Conversation
This currently is required for only enabling user session limits configuration and in future can be extended for other configurations Signed-off-by: Prabhjot Singh Sethi <prabhjot.sethi@gmail.com>
WalkthroughAdds Keycloak authentication-flow management and client updates: new flow constants, SetupReconciler.updateRealmAuthFlows and detectFormsSubflow to ensure/copy/link flows, create/configure user-session-limits executions and reposition them, integrate these steps into Reconcile, and add multiple Keycloak client methods for flow manipulation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Reconciler as SetupReconciler
participant Client as KeycloakClient
participant API as Keycloak Admin API
Reconciler->>Client: acquire token
Client->>API: admin token exchange
Reconciler->>Client: updateRealmAuthFlows(realm)
Client->>API: GET /admin/realms/{realm}/authentication/flows
API-->>Client: flows[]
alt Ensure IDP post-login flow
Reconciler->>Client: ensure or create "auth idp post login"
Client->>API: POST/GET /authentication/flows
API-->>Client: flow/execution
Reconciler->>Client: ConfigureUserSessionLimit(...)
Client->>API: PUT /authentication/executions/{id}/config
API-->>Client: 200
end
alt Ensure custom browser flow
Reconciler->>Client: ensure/copy "browser" -> "auth browser"
Client->>API: POST /authentication/flows/{src}/copy
API-->>Client: 201
Reconciler->>Client: update realm to use custom browser flow
Client->>API: PUT /realms/{realm}
API-->>Client: 200
Reconciler->>Client: detectFormsSubflow("auth browser")
Client->>API: GET /authentication/flows/{flow}/executions
API-->>Client: executions (including subflows)
Reconciler->>Client: LocateAuthenticationExecution(formsSubflow, "user-session-limits")
Client->>API: GET/POST /authentication/executions
API-->>Client: execution
Reconciler->>Client: ConfigureUserSessionLimit(...)
loop Reposition until last
Client->>API: POST /executions/{id}/lower-priority
API-->>Client: 204
end
end
Reconciler-->>Reconciler: success or requeue on error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
![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
🧹 Nitpick comments (3)
pkg/controller/tenant/setup.go (2)
114-118: Avoid brittle “len(list) < 4” check; rely on locating the execution instead.
The magic number is environment‑dependent and may cause unnecessary requeues.- list, err := r.ctrl.client.GetAuthenticationExecutions(ctx, token, realm, BrowserFlow+" forms") - if err != nil || len(list) < 4 { - log.Printf("waiting for copy operation for auth browser flow to complete, error: %s", err) - return errors.Wrapf(errors.Unknown, "waiting for copy operation for auth browser flow to complete, error :%s", err) - } + list, err := r.ctrl.client.GetAuthenticationExecutions(ctx, token, realm, BrowserFlow+" forms") + if err != nil { + log.Printf("failed to fetch executions for %q, error: %s", BrowserFlow+" forms", err) + return err + } + _ = list // presence not enforced; locating execution below is authoritative
125-129: Config parameters should be configurable (not hard‑coded).
Session limit “5” and behavior could come from tenant config or defaults.Would you like a follow‑up patch to read these from table.TenantEntry (with sane defaults)?
pkg/keycloak/client.go (1)
242-263: Improve error message; include src and dest, fix grammar.
Enhances observability when copy fails.- if resp == nil || resp.IsError() { - return errors.Wrapf(errors.Unknown, "failed to create of copy authentication flow %s, status code: %d", src, resp.StatusCode()) - } + if resp == nil || resp.IsError() { + return errors.Wrapf( + errors.Unknown, + "failed to copy authentication flow from %q to %q, status code: %d", + src, dest, resp.StatusCode(), + ) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
pkg/controller/tenant/setup.go(2 hunks)pkg/keycloak/client.go(2 hunks)
🔇 Additional comments (7)
pkg/controller/tenant/setup.go (5)
31-35: Good constant naming and scoping.
Clear aliases for custom flows; suitable for reuse.
82-93: Solid flow: locate/create execution, configure, then set requirement.
This sequence is correct and idempotent for the IDP post-login flow.
94-100: Good: copy built-in browser flow to custom alias if missing.
Keeps built‑ins intact and enables customization.
102-112: Realm update is appropriate.
Setting BrowserFlow to the custom alias is the right hook.
177-182: Requeue-on-failure integration looks good.
Failure path requeues after 5s; consistent with realm creation path.If we adopt the non‑panic change above, ensure reconcile propagates the error path properly.
pkg/keycloak/client.go (2)
265-308: Execution lookup/creation flow is sound.
Top‑level filtering and create‑if‑missing pattern is good.
350-363: Priority adjustment endpoint wrapper looks correct.
Endpoint path and error handling are appropriate.
| for _, f := range flows { | ||
| if *f.Alias == IDPPostLoginFlow { | ||
| idpPostLoginFlow = true | ||
| } | ||
| if *f.Alias == BrowserFlow { | ||
| browserFlow = true | ||
| } | ||
| } |
There was a problem hiding this comment.
Nil-pointer risk: f.Alias may be nil.
Guard before dereferencing to avoid panic.
- for _, f := range flows {
- if *f.Alias == IDPPostLoginFlow {
- idpPostLoginFlow = true
- }
- if *f.Alias == BrowserFlow {
- browserFlow = true
- }
- }
+ for _, f := range flows {
+ if f.Alias == nil {
+ continue
+ }
+ switch *f.Alias {
+ case IDPPostLoginFlow:
+ idpPostLoginFlow = true
+ case BrowserFlow:
+ browserFlow = true
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, f := range flows { | |
| if *f.Alias == IDPPostLoginFlow { | |
| idpPostLoginFlow = true | |
| } | |
| if *f.Alias == BrowserFlow { | |
| browserFlow = true | |
| } | |
| } | |
| for _, f := range flows { | |
| if f.Alias == nil { | |
| continue | |
| } | |
| switch *f.Alias { | |
| case IDPPostLoginFlow: | |
| idpPostLoginFlow = true | |
| case BrowserFlow: | |
| browserFlow = true | |
| } | |
| } |
🤖 Prompt for AI Agents
In pkg/controller/tenant/setup.go around lines 53 to 60, the loop dereferences
f.Alias without checking for nil which can panic; update the loop to guard
against nil by checking if f.Alias != nil before comparing its value to
IDPPostLoginFlow or BrowserFlow, and only then set idpPostLoginFlow or
browserFlow accordingly (skip or safely handle entries with nil Alias).
| func (c *Client) ConfigureUserSessionLimit(ctx context.Context, token, realm string, e *gocloak.ModifyAuthenticationExecutionRepresentation, sessions int, denyNewSession bool, alias, errorMsg string) error { | ||
| var err error | ||
| behavior := "Deny new session" | ||
| if !denyNewSession { | ||
| behavior = "Terminate oldest session" | ||
| } | ||
| config := struct { | ||
| //ID *string `json:"id,omitempty"` | ||
| Alias *string `json:"alias,omitempty"` | ||
| Config *map[string]string `json:"config,omitempty"` | ||
| }{ | ||
| Alias: gocloak.StringP(alias), | ||
| Config: &map[string]string{ | ||
| "userRealmLimit": strconv.Itoa(sessions), | ||
| "behavior": behavior, | ||
| "userClientLimit": "0", | ||
| "errorMessage": errorMsg, | ||
| }, | ||
| } | ||
| var resp *resty.Response | ||
| if e.AuthenticationConfig == nil { | ||
| resp, err = c.GetRequestWithBearerAuth(ctx, token). | ||
| SetBody(config). | ||
| Post(c.getAdminRealmURL(c.url, realm, "authentication", "executions", *e.ID, "config")) | ||
| } else { | ||
| resp, err = c.GetRequestWithBearerAuth(ctx, token). | ||
| SetBody(config). | ||
| Put(c.getAdminRealmURL(c.url, realm, "authentication", "config", *e.AuthenticationConfig)) | ||
| } | ||
|
|
||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if resp == nil || resp.IsError() { | ||
| return errors.Wrapf(errors.Unknown, "failed to configure user session limit, status code: %d", resp.StatusCode()) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Defensive checks for execution IDs before POST/PUT.
Avoid nil deref if an unexpected payload is returned.
func (c *Client) ConfigureUserSessionLimit(ctx context.Context, token, realm string, e *gocloak.ModifyAuthenticationExecutionRepresentation, sessions int, denyNewSession bool, alias, errorMsg string) error {
- var err error
+ var err error
+ if e == nil || e.ID == nil {
+ return errors.Wrapf(errors.InvalidArgument, "execution is nil or missing ID")
+ }Please confirm the expected strings for behavior in your Keycloak version are exactly:
- "Deny new session"
- "Terminate oldest session"
🌐 Web query:
Keycloak "Deny new session" "Terminate oldest session" "User session limit" "userRealmLimit"
💡 Result:
Summary — what these options do and how to configure them
-
Behavior options:
- "Deny new session": when a user already has the maximum allowed sessions, a new login is rejected. [1]
- "Terminate oldest session": when the limit is reached, Keycloak removes the oldest session and allows the new one. [1]
-
Config keys (used in the User Session Count Limiter authenticator):
- userRealmLimit — max concurrent SSO sessions per user across the realm.
- userClientLimit — max concurrent sessions per user per client.
- behavior — either "Deny new session" or "Terminate oldest session".
- Example JSON config: { "userClientLimit":"0", "behavior":"Deny new session", "userRealmLimit":"3" }. [2]
-
Where to enable:
- Add the "User Session Count Limiter" authenticator to the relevant authentication flows (Browser, Direct Grant, Reset Credentials and Post-broker login flows), set it to REQUIRED, and configure the limits/behavior. [1][3]
-
Caveats/bugs:
- There are reported issues where "Terminate oldest session" can remove the wrong session when both realm- and client-limits interact; test your flows and Keycloak version. [4]
Sources
[1] Keycloak Server Administration — User session limits. (keycloak.org)
[2] Example config and keys (userRealmLimit, userClientLimit, behavior). (doc.quobis.com)
[3] How to add/configure the authenticator in flows (documentation and examples). (manologg.github.io)
[4] GitHub issue reporting incorrect session removal with "Terminate oldest session". (github.com)
If you want, tell me your Keycloak version and I’ll give exact Admin Console steps or the JSON to apply via the Admin REST API.
Citations:
- 1: https://www.keycloak.org/docs/25.0.1/server_admin/?utm_source=openai
- 2: https://doc.quobis.com/wac/intro_oauth.html?utm_source=openai
- 3: https://manologg.github.io/keycloak-documentation-18.0.2/server_admin/index.html?utm_source=openai
- 4: User session limit exceeded for both realm and client removes the wrong session keycloak/keycloak#38016
Fix nil derefs in ConfigureUserSessionLimit; confirm behavior strings
- Add a defensive guard at the top of ConfigureUserSessionLimit: if e == nil || (e.AuthenticationConfig == nil && e.ID == nil) { return errors.Wrapf(errors.InvalidArgument, "execution is nil or missing ID") } — file: pkg/keycloak/client.go, function ConfigureUserSessionLimit.
- Avoid dereferencing resp when nil: don’t call resp.StatusCode() if resp == nil; handle resp==nil and resp.IsError() separately (e.g., if resp == nil { return errors.Wrapf(errors.Unknown, "no response from admin API") } else if resp.IsError() { return errors.Wrapf(errors.Unknown, "failed to configure user session limit, status code: %d", resp.StatusCode()) }).
- Behavior strings verified (Keycloak docs): "Deny new session", "Terminate oldest session".
🤖 Prompt for AI Agents
In pkg/keycloak/client.go around lines 310 to 348, add defensive checks and
safer resp handling: at the top of ConfigureUserSessionLimit return an
InvalidArgument error if e == nil or (e.AuthenticationConfig == nil && e.ID ==
nil); after making the HTTP call, handle resp == nil first (return an Unknown
error like "no response from admin API") and only then check resp.IsError() and
use resp.StatusCode() in that branch; keep the behavior strings as "Deny new
session" and "Terminate oldest session".
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📥 Commits
Reviewing files that changed from the base of the PR and between 8831016 and e750aefb5067d1699aed44064c74ac1403f692fe.
📒 Files selected for processing (2)
pkg/controller/tenant/setup.go(5 hunks)pkg/keycloak/client.go(2 hunks)
🔇 Additional comments (3)
pkg/controller/tenant/setup.go (2)
50-53: Return an error instead of panicking on token acquisition failure.
Line 50: panicking here will take down the controller; bubble the error up so the reconciler can requeue.- token, err := r.ctrl.client.GetAccessToken() - if err != nil { - log.Panicf("keycloak session not active: %s", err) - } + token, err := r.ctrl.client.GetAccessToken() + if err != nil { + return errors.Wrapf(errors.Unauthenticated, "keycloak session not active: %v", err) + }
63-71: Guard against nil aliases before dereferencing.
Line 65:f.Aliascan be nil; dereferencing will panic when Keycloak returns sparse flow data.- for _, f := range flows { - if *f.Alias == IDPPostLoginFlow { - idpPostLoginFlow = true - } - if *f.Alias == BrowserFlow { - browserFlow = true - } - } + for _, f := range flows { + if f.Alias == nil { + continue + } + switch *f.Alias { + case IDPPostLoginFlow: + idpPostLoginFlow = true + case BrowserFlow: + browserFlow = true + } + }pkg/keycloak/client.go (1)
310-345: Defend against nil execution data and responses in session-limit configuration.
Line 310:e(ande.ID) can be nil; line 344: callingStatusCode()whenrespis nil will panic. Both conditions still need guarding.-func (c *Client) ConfigureUserSessionLimit(ctx context.Context, token, realm string, e *gocloak.ModifyAuthenticationExecutionRepresentation, sessions int, denyNewSession bool, alias, errorMsg string) error { - var err error +func (c *Client) ConfigureUserSessionLimit(ctx context.Context, token, realm string, e *gocloak.ModifyAuthenticationExecutionRepresentation, sessions int, denyNewSession bool, alias, errorMsg string) error { + if e == nil || e.ID == nil { + return errors.Wrapf(errors.InvalidArgument, "execution is nil or missing ID") + } + var err error @@ - if resp == nil || resp.IsError() { - return errors.Wrapf(errors.Unknown, "failed to configure user session limit, status code: %d", resp.StatusCode()) + if resp == nil { + return errors.Wrapf(errors.Unknown, "failed to configure user session limit: empty response from admin API") + } + if resp.IsError() { + return errors.Wrapf(errors.Unknown, "failed to configure user session limit, status code: %d", resp.StatusCode())
| for _, flow := range flows { | ||
| if flow.Alias != nil && !*flow.BuiltIn { | ||
| flowName := *flow.Alias | ||
| if flowName != BrowserFlow && flowName != IDPPostLoginFlow { | ||
| executions, err := r.ctrl.client.GetAuthenticationExecutions(ctx, token, realm, flowName) |
There was a problem hiding this comment.
Check BuiltIn for nil before dereferencing.
Line 222: flow.BuiltIn is optional in the admin response; dereferencing without a nil guard will panic.
- for _, flow := range flows {
- if flow.Alias != nil && !*flow.BuiltIn {
+ for _, flow := range flows {
+ if flow.Alias != nil && (flow.BuiltIn == nil || !*flow.BuiltIn) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, flow := range flows { | |
| if flow.Alias != nil && !*flow.BuiltIn { | |
| flowName := *flow.Alias | |
| if flowName != BrowserFlow && flowName != IDPPostLoginFlow { | |
| executions, err := r.ctrl.client.GetAuthenticationExecutions(ctx, token, realm, flowName) | |
| for _, flow := range flows { | |
| if flow.Alias != nil && (flow.BuiltIn == nil || !*flow.BuiltIn) { | |
| flowName := *flow.Alias | |
| if flowName != BrowserFlow && flowName != IDPPostLoginFlow { | |
| executions, err := r.ctrl.client.GetAuthenticationExecutions(ctx, token, realm, flowName) |
🤖 Prompt for AI Agents
In pkg/controller/tenant/setup.go around lines 220 to 224, the code dereferences
flow.BuiltIn without checking for nil which can panic; update the conditional to
first ensure flow.BuiltIn is non-nil before dereferencing (e.g. require
flow.BuiltIn != nil && !*flow.BuiltIn alongside the existing flow.Alias != nil
check) so nil BuiltIn values are handled safely and the subsequent
GetAuthenticationExecutions call only runs for explicit non-built-in flows.
| if resp == nil || resp.IsError() { | ||
| return errors.Wrapf(errors.Unknown, "failed to create of copy authentication flow %s, status code: %d", src, resp.StatusCode()) | ||
| } |
There was a problem hiding this comment.
Handle nil responses before calling StatusCode().
Line 259: when resp is nil the follow-up StatusCode() call panics; guard the nil case separately.
- if resp == nil || resp.IsError() {
- return errors.Wrapf(errors.Unknown, "failed to create of copy authentication flow %s, status code: %d", src, resp.StatusCode())
+ if resp == nil {
+ return errors.Wrapf(errors.Unknown, "failed to copy authentication flow %s: empty response from admin API", src)
+ }
+ if resp.IsError() {
+ return errors.Wrapf(errors.Unknown, "failed to copy authentication flow %s, status code: %d", src, resp.StatusCode())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if resp == nil || resp.IsError() { | |
| return errors.Wrapf(errors.Unknown, "failed to create of copy authentication flow %s, status code: %d", src, resp.StatusCode()) | |
| } | |
| if resp == nil { | |
| return errors.Wrapf(errors.Unknown, "failed to copy authentication flow %s: empty response from admin API", src) | |
| } | |
| if resp.IsError() { | |
| return errors.Wrapf(errors.Unknown, "failed to copy authentication flow %s, status code: %d", src, resp.StatusCode()) | |
| } |
🤖 Prompt for AI Agents
In pkg/keycloak/client.go around lines 258 to 260, the code may call
resp.StatusCode() when resp is nil causing a panic; first check if resp == nil
and return a wrapped error that notes there was no response (do not call
StatusCode), then separately check if resp.IsError() and return a wrapped error
that includes resp.StatusCode(); i.e. split the condition into two branches so
StatusCode() is only accessed when resp is non-nil.
| func (c *Client) LowerAuthenticationExecutionPriority(ctx context.Context, token, realm, executionId string) error { | ||
| resp, err := c.GetRequestWithBearerAuth(ctx, token). | ||
| Post(c.getAdminRealmURL(c.url, realm, "authentication", "executions", executionId, "lower-priority")) | ||
|
|
||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if resp == nil || resp.IsError() { | ||
| return errors.Wrapf(errors.Unknown, "failed to lower authentication execution priority, status code: %d", resp.StatusCode()) |
There was a problem hiding this comment.
Avoid dereferencing a nil response when lowering priority.
Line 357: guard the nil response before logging the status code.
- if resp == nil || resp.IsError() {
- return errors.Wrapf(errors.Unknown, "failed to lower authentication execution priority, status code: %d", resp.StatusCode())
+ if resp == nil {
+ return errors.Wrapf(errors.Unknown, "failed to lower authentication execution priority: empty response from admin API")
+ }
+ if resp.IsError() {
+ return errors.Wrapf(errors.Unknown, "failed to lower authentication execution priority, status code: %d", resp.StatusCode())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (c *Client) LowerAuthenticationExecutionPriority(ctx context.Context, token, realm, executionId string) error { | |
| resp, err := c.GetRequestWithBearerAuth(ctx, token). | |
| Post(c.getAdminRealmURL(c.url, realm, "authentication", "executions", executionId, "lower-priority")) | |
| if err != nil { | |
| return err | |
| } | |
| if resp == nil || resp.IsError() { | |
| return errors.Wrapf(errors.Unknown, "failed to lower authentication execution priority, status code: %d", resp.StatusCode()) | |
| func (c *Client) LowerAuthenticationExecutionPriority(ctx context.Context, token, realm, executionId string) error { | |
| resp, err := c.GetRequestWithBearerAuth(ctx, token). | |
| Post(c.getAdminRealmURL(c.url, realm, "authentication", "executions", executionId, "lower-priority")) | |
| if err != nil { | |
| return err | |
| } | |
| if resp == nil { | |
| return errors.Wrapf(errors.Unknown, "failed to lower authentication execution priority: empty response from admin API") | |
| } | |
| if resp.IsError() { | |
| return errors.Wrapf(errors.Unknown, "failed to lower authentication execution priority, status code: %d", resp.StatusCode()) | |
| } | |
| // …rest of implementation… | |
| } |
🤖 Prompt for AI Agents
In pkg/keycloak/client.go around lines 349 to 358, the current error branch
dereferences resp.StatusCode() before ensuring resp is non-nil; change the logic
to first check if resp == nil and return a wrapped error indicating no response,
and only if resp is non-nil and resp.IsError() return a wrapped error that
includes resp.StatusCode(); this ensures you never call StatusCode() on a nil
resp and preserves the original error paths.
Suryanshu185
force-pushed
the
auth-flows
branch
3 times, most recently
from
0e6e9f4 to
100efac
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
pkg/keycloak/client.go (3)
258-260: Handle nil responses before callingStatusCode().Lines 258-260: when
respis nil, the follow-upStatusCode()call panics. Guard the nil case separately:- if resp == nil || resp.IsError() { - return errors.Wrapf(errors.Unknown, "failed to create of copy authentication flow %s, status code: %d", src, resp.StatusCode()) + if resp == nil { + return errors.Wrapf(errors.Unknown, "failed to copy authentication flow %s: empty response from admin API", src) + } + if resp.IsError() { + return errors.Wrapf(errors.Unknown, "failed to copy authentication flow %s, status code: %d", src, resp.StatusCode())
310-348: Add defensive checks for nil parameters and responses.Two critical issues remain from past review:
Missing nil guard for execution parameter: Lines 330-337 dereference
e.IDande.AuthenticationConfigwithout checking ifeis nil.Nil response before StatusCode(): Lines 344-346 will panic if
respis nil.Apply this diff:
func (c *Client) ConfigureUserSessionLimit(ctx context.Context, token, realm string, e *gocloak.ModifyAuthenticationExecutionRepresentation, sessions int, denyNewSession bool, alias, errorMsg string) error { + if e == nil || e.ID == nil { + return errors.Wrapf(errors.InvalidArgument, "execution is nil or missing ID") + } var err error behavior := "Deny new session" if !denyNewSession { behavior = "Terminate oldest session" } config := struct { //ID *string `json:"id,omitempty"` Alias *string `json:"alias,omitempty"` Config *map[string]string `json:"config,omitempty"` }{ Alias: gocloak.StringP(alias), Config: &map[string]string{ "userRealmLimit": strconv.Itoa(sessions), "behavior": behavior, "userClientLimit": "0", "errorMessage": errorMsg, }, } var resp *resty.Response if e.AuthenticationConfig == nil { resp, err = c.GetRequestWithBearerAuth(ctx, token). SetBody(config). Post(c.getAdminRealmURL(c.url, realm, "authentication", "executions", *e.ID, "config")) } else { resp, err = c.GetRequestWithBearerAuth(ctx, token). SetBody(config). Put(c.getAdminRealmURL(c.url, realm, "authentication", "config", *e.AuthenticationConfig)) } if err != nil { return err } - if resp == nil || resp.IsError() { - return errors.Wrapf(errors.Unknown, "failed to configure user session limit, status code: %d", resp.StatusCode()) + if resp == nil { + return errors.Wrapf(errors.Unknown, "failed to configure user session limit: empty response from admin API") + } + if resp.IsError() { + return errors.Wrapf(errors.Unknown, "failed to configure user session limit, status code: %d", resp.StatusCode()) } return nil }
358-360: Avoid dereferencing a nil response when lowering priority.Lines 358-360: guard the nil response before logging the status code:
- if resp == nil || resp.IsError() { - return errors.Wrapf(errors.Unknown, "failed to lower authentication execution priority, status code: %d", resp.StatusCode()) + if resp == nil { + return errors.Wrapf(errors.Unknown, "failed to lower authentication execution priority: empty response from admin API") + } + if resp.IsError() { + return errors.Wrapf(errors.Unknown, "failed to lower authentication execution priority, status code: %d", resp.StatusCode())pkg/controller/tenant/setup.go (2)
64-71: Guard against nil Alias before dereferencing.Lines 64-71: the loop dereferences
f.Aliaswithout checking for nil, which can panic:for _, f := range flows { + if f.Alias == nil { + continue + } - if *f.Alias == IDPPostLoginFlow { + switch *f.Alias { + case IDPPostLoginFlow: idpPostLoginFlow = true - } - if *f.Alias == BrowserFlow { + case BrowserFlow: browserFlow = true } }
220-224: CheckBuiltInfor nil before dereferencing.Line 221:
flow.BuiltInis optional in the admin response; dereferencing without a nil guard will panic:for _, flow := range flows { - if flow.Alias != nil && !*flow.BuiltIn { + if flow.Alias != nil && (flow.BuiltIn == nil || !*flow.BuiltIn) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📥 Commits
Reviewing files that changed from the base of the PR and between 0e6e9f48944ec98ac7baa3fe5c5bbb763123bd5b and 100efacd1e82c47570d7a8e3c6beb6f191797a8b.
📒 Files selected for processing (2)
pkg/controller/tenant/setup.go(4 hunks)pkg/keycloak/client.go(2 hunks)
🔇 Additional comments (6)
pkg/keycloak/client.go (3)
265-308: LGTM!The method correctly checks for nil pointers before dereferencing (lines 272, 277, 296, 301) and handles the create-if-not-found pattern appropriately.
365-413: LGTM!The repositioning logic correctly guards against nil pointers (lines 380, 395) and includes a safety limit (
maxAttempts = 20) to prevent infinite loops. The approach of repeatedly callingLowerAuthenticationExecutionPriorityuntil the execution reaches the last position is appropriate.
415-433: LGTM with caveat on estimation.The priority estimation (
topLevelCount * 10) is a reasonable workaround given that the Priority field isn't exposed in the struct. The nil check on line 426 is properly implemented.Note: This estimation assumes a spacing of 10 between priorities, which may not always hold if Keycloak's internal logic changes.
pkg/controller/tenant/setup.go (3)
31-44: LGTM!The constants provide clear names for authentication flows, and the
FlowExecutionReporttype offers a structured representation for flow state (likely for reporting or external integration).
108-179: LGTM!The flow orchestration correctly:
- Detects the forms subflow dynamically (line 109)
- Processes both target flows (IDPPostLoginFlow and forms subflow)
- Locates/creates executions, configures session limits, positions them, and sets requirements
- Includes verification and retry logic for positioning (lines 146-174)
The error handling and propagation are appropriate throughout.
335-363: LGTM!The client configuration update logic correctly:
- Guards against nil pointers before dereferencing (lines 343, 348, 353)
- Only updates when necessary (
needsUpdateflag)- Logs errors without failing the reconciliation
This ensures existing clients get the required defaults (Enabled, PublicClient, StandardFlowEnabled).
Suryanshu185
force-pushed
the
auth-flows
branch
from
100efac to
68fd9ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
pkg/controller/tenant/setup.go (3)
64-71: Nil-pointer risk when dereferencing f.Alias.Dereferencing
f.Aliaswithout a nil guard can panic. Add a nil check before comparison.Apply this diff:
for _, f := range flows { - if *f.Alias == IDPPostLoginFlow { + if f.Alias != nil && *f.Alias == IDPPostLoginFlow { idpPostLoginFlow = true } - if *f.Alias == BrowserFlow { + if f.Alias != nil && *f.Alias == BrowserFlow { browserFlow = true } }
363-363: Nil-pointer risk when comparing mapper names.Dereferencing
m.Nameandmapper.Namewithout nil checks can panic. Verify both pointers are non-nil before comparison.Apply this diff:
for _, m := range *found[0].ProtocolMappers { - if *m.Name == *mapper.Name { + if m.Name != nil && mapper.Name != nil && *m.Name == *mapper.Name { mapperExists = true break }
50-53: Panic still present despite being marked as addressed.The panic on token failure was flagged in a previous review and marked as "✅ Addressed in commit eb94349", but the code still contains
log.Panicfon line 52. Return an error instead to allow the reconciler to requeue gracefully.Apply this diff:
token, err := r.ctrl.client.GetAccessToken() if err != nil { - log.Panicf("keycloak session not active: %s", err) + return errors.Wrapf(errors.Unauthenticated, "keycloak session not active: %s", err) }pkg/keycloak/client.go (3)
258-259: Nil response causes panic when calling StatusCode().If
respis nil, callingresp.StatusCode()will panic. Handle the nil case separately before checkingIsError().Apply this diff:
- if resp == nil || resp.IsError() { - return errors.Wrapf(errors.Unknown, "failed to create of copy authentication flow %s, status code: %d", src, resp.StatusCode()) + if resp == nil { + return errors.Wrapf(errors.Unknown, "failed to copy authentication flow %s: empty response from admin API", src) + } + if resp.IsError() { + return errors.Wrapf(errors.Unknown, "failed to copy authentication flow %s, status code: %d", src, resp.StatusCode()) }
310-348: Missing defensive checks and nil response handling.
- The function doesn't validate that
eore.IDis non-nil at the start, risking a panic when dereferencing on line 333.- Line 344 calls
resp.StatusCode()without first ensuringrespis non-nil.Apply this diff:
func (c *Client) ConfigureUserSessionLimit(ctx context.Context, token, realm string, e *gocloak.ModifyAuthenticationExecutionRepresentation, sessions int, denyNewSession bool, alias, errorMsg string) error { + if e == nil || e.ID == nil { + return errors.Wrapf(errors.InvalidArgument, "execution is nil or missing ID") + } var err error behavior := "Deny new session"And:
- if resp == nil || resp.IsError() { - return errors.Wrapf(errors.Unknown, "failed to configure user session limit, status code: %d", resp.StatusCode()) + if resp == nil { + return errors.Wrapf(errors.Unknown, "failed to configure user session limit: empty response from admin API") + } + if resp.IsError() { + return errors.Wrapf(errors.Unknown, "failed to configure user session limit, status code: %d", resp.StatusCode()) }
358-359: Nil response causes panic when calling StatusCode().If
respis nil, callingresp.StatusCode()will panic. Handle the nil case separately before checkingIsError().Apply this diff:
- if resp == nil || resp.IsError() { - return errors.Wrapf(errors.Unknown, "failed to lower authentication execution priority, status code: %d", resp.StatusCode()) + if resp == nil { + return errors.Wrapf(errors.Unknown, "failed to lower authentication execution priority: empty response from admin API") + } + if resp.IsError() { + return errors.Wrapf(errors.Unknown, "failed to lower authentication execution priority, status code: %d", resp.StatusCode()) }
🧹 Nitpick comments (2)
pkg/controller/tenant/setup.go (2)
37-44: Unused type definition.
FlowExecutionReportis defined but never used within this file. Consider removing it if not needed, or add a comment explaining its intended future use.
168-174: Redundant repositioning call.
SetExecutionToPosition(line 132) already loops up to 20 times to position the execution last. Calling it again on line 170 is redundant, as the first call should have succeeded if positioning was possible.Consider removing the redundant repositioning logic:
- // If not last, attempt to reposition again - if !isLast { - err = r.ctrl.client.SetExecutionToPosition(ctx, token, realm, flowName, *userSessionExec.ID) - if err != nil { - return err - } - } + // If still not last after SetExecutionToPosition, log a warning + if !isLast { + log.Printf("Warning: execution %s in flow %s is not in last position after repositioning", *userSessionExec.ID, flowName) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📥 Commits
Reviewing files that changed from the base of the PR and between 100efacd1e82c47570d7a8e3c6beb6f191797a8b and 68fd9ffef6d289895e0eb7d4e050c1fdb56a5cac.
📒 Files selected for processing (2)
pkg/controller/tenant/setup.go(4 hunks)pkg/keycloak/client.go(2 hunks)
🔇 Additional comments (2)
pkg/keycloak/client.go (2)
365-413: LGTM!The
SetExecutionToPositionmethod correctly implements iterative priority lowering with proper safety limits and level-aware positioning logic. Good defensive checks for nil pointers (lines 380, 395) prevent panics.
415-433: LGTM!The
GetMaxExecutionPrioritymethod correctly estimates maximum priority by counting top-level executions, with proper nil checking for theLevelfield.
- Add dynamic forms subflow detection with pattern matching and fallback - Replace hardcoded flow names with robust flow discovery algorithm - Implement SetExecutionToLastPosition for reliable execution positioning - Configure user session limits to 5 sessions with deny new behavior (default)(TODO: make it configurable with a PUT api call) - Fix execution positioning issues and infinite loop prevention Signed-off-by: suryanshu185 <suryanshu.gupta@kluisz.ai>
Suryanshu185
force-pushed
the
auth-flows
branch
from
68fd9ff to
2208044
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
pkg/keycloak/client.go (3)
258-260: Handle nil responses before callingStatusCode().This issue was previously flagged and remains unresolved. When
respis nil, callingresp.StatusCode()will panic. Additionally, there's a typo in the error message: "create of copy" should be "copy".Apply this diff to fix both issues:
- if resp == nil || resp.IsError() { - return errors.Wrapf(errors.Unknown, "failed to create of copy authentication flow %s, status code: %d", src, resp.StatusCode()) + if resp == nil { + return errors.Wrapf(errors.Unknown, "failed to copy authentication flow %s: empty response from admin API", src) + } + if resp.IsError() { + return errors.Wrapf(errors.Unknown, "failed to copy authentication flow %s, status code: %d", src, resp.StatusCode())
310-348: Add defensive nil checks and handle nil responses safely.This issue was previously flagged and remains unresolved. The method needs:
- Defensive guard at the start to prevent nil dereference of
eore.ID- Separate handling of
resp == nilbefore callingresp.StatusCode()Apply this diff:
func (c *Client) ConfigureUserSessionLimit(ctx context.Context, token, realm string, e *gocloak.ModifyAuthenticationExecutionRepresentation, sessions int, denyNewSession bool, alias, errorMsg string) error { + if e == nil || e.ID == nil { + return errors.Wrapf(errors.InvalidArgument, "execution is nil or missing ID") + } var err error behavior := "Deny new session" if !denyNewSession { behavior = "Terminate oldest session" } config := struct { //ID *string `json:"id,omitempty"` Alias *string `json:"alias,omitempty"` Config *map[string]string `json:"config,omitempty"` }{ Alias: gocloak.StringP(alias), Config: &map[string]string{ "userRealmLimit": strconv.Itoa(sessions), "behavior": behavior, "userClientLimit": "0", "errorMessage": errorMsg, }, } var resp *resty.Response if e.AuthenticationConfig == nil { resp, err = c.GetRequestWithBearerAuth(ctx, token). SetBody(config). Post(c.getAdminRealmURL(c.url, realm, "authentication", "executions", *e.ID, "config")) } else { resp, err = c.GetRequestWithBearerAuth(ctx, token). SetBody(config). Put(c.getAdminRealmURL(c.url, realm, "authentication", "config", *e.AuthenticationConfig)) } if err != nil { return err } - if resp == nil || resp.IsError() { - return errors.Wrapf(errors.Unknown, "failed to configure user session limit, status code: %d", resp.StatusCode()) + if resp == nil { + return errors.Wrapf(errors.Unknown, "failed to configure user session limit: empty response from admin API") + } + if resp.IsError() { + return errors.Wrapf(errors.Unknown, "failed to configure user session limit, status code: %d", resp.StatusCode()) } return nil }
350-363: Avoid dereferencing nil response when lowering priority.This issue was previously flagged and remains unresolved. Guard the nil response before calling
resp.StatusCode().Apply this diff:
func (c *Client) LowerAuthenticationExecutionPriority(ctx context.Context, token, realm, executionId string) error { resp, err := c.GetRequestWithBearerAuth(ctx, token). Post(c.getAdminRealmURL(c.url, realm, "authentication", "executions", executionId, "lower-priority")) if err != nil { return err } - if resp == nil || resp.IsError() { - return errors.Wrapf(errors.Unknown, "failed to lower authentication execution priority, status code: %d", resp.StatusCode()) + if resp == nil { + return errors.Wrapf(errors.Unknown, "failed to lower authentication execution priority: empty response from admin API") + } + if resp.IsError() { + return errors.Wrapf(errors.Unknown, "failed to lower authentication execution priority, status code: %d", resp.StatusCode()) } return nil }pkg/controller/tenant/setup.go (3)
41-44: Don't panic on transient token failures; return error to let reconciler requeue.Panicking here crashes the controller process rather than allowing graceful retry. Return an error instead so the reconciler can requeue.
Apply this diff:
token, err := r.ctrl.client.GetAccessToken() if err != nil { - log.Panicf("keycloak session not active: %s", err) + return fmt.Errorf("keycloak session not active: %w", err) }
54-62: Nil-pointer risk: Guard f.Alias before dereferencing.Both comparisons at lines 56 and 59 dereference
f.Aliaswithout checking for nil, which will panic if Keycloak returns a flow with a nil Alias.Apply this diff:
var browserFlow, idpPostLoginFlow bool for _, f := range flows { - if *f.Alias == IDPPostLoginFlow { - idpPostLoginFlow = true - } - if *f.Alias == BrowserFlow { - browserFlow = true + if f.Alias == nil { + continue + } + switch *f.Alias { + case IDPPostLoginFlow: + idpPostLoginFlow = true + case BrowserFlow: + browserFlow = true } }
348-368: Nil-pointer risk: Guard mapper names before dereferencing.Line 354 dereferences both
m.Nameandmapper.Namewithout nil checks, which will panic if either is nil.Apply this diff:
for _, mapper := range mList { mapperExists := false if len(found) != 0 { // Check if mapper already exists if found[0].ProtocolMappers != nil { for _, m := range *found[0].ProtocolMappers { - if *m.Name == *mapper.Name { + if m.Name != nil && mapper.Name != nil && *m.Name == *mapper.Name { mapperExists = true break } } } }
🧹 Nitpick comments (1)
pkg/controller/tenant/setup.go (1)
108-167: Consider making the session limit configurable.Line 117 hardcodes
userRealmLimit=5. If different tenants require different limits, consider making this configurable via the tenant spec or a controller parameter.The positioning verification logic (lines 146-165) is complex but handles nil checks properly. It correctly identifies when the user-session-limits execution is not last at its level and attempts repositioning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📥 Commits
Reviewing files that changed from the base of the PR and between 68fd9ffef6d289895e0eb7d4e050c1fdb56a5cac and 2208044.
📒 Files selected for processing (2)
pkg/controller/tenant/setup.go(4 hunks)pkg/keycloak/client.go(2 hunks)
🔇 Additional comments (9)
pkg/keycloak/client.go (4)
10-10: LGTM!The new imports are appropriately added to support numeric string conversion and HTTP response handling in the authentication flow management methods.
Also applies to: 16-16
265-308: LGTM!The method correctly locates existing executions or creates new ones when needed. The logic properly filters for top-level executions and includes appropriate error handling for both the locate and create paths.
365-413: LGTM!The method correctly repositions executions to the last position with proper safety mechanisms. The logic properly handles same-level comparisons and includes a reasonable attempt limit to prevent infinite loops.
415-433: Verify the priority estimation formula aligns with Keycloak's actual priority scheme.The function estimates max priority as
topLevelCount * 10, but this formula may not match Keycloak's actual priority assignment. The function name suggests it retrieves an actual maximum, but it computes an estimate instead.Consider one of the following:
- Verify that Keycloak uses increments of 10 for execution priorities by checking the Admin REST API response for actual priority values
- Rename to
EstimateMaxExecutionPriorityto clarify it's an approximation- If the estimation is sufficient for the use case, document why this formula is reliable
pkg/controller/tenant/setup.go (5)
31-35: LGTM: Clear flow constants.The authentication flow constants are well-named and properly documented.
64-97: LGTM: Flow creation and realm update logic is sound.The flow ensures both authentication flows exist (creating or copying as needed) and correctly updates the realm to use the custom browser flow.
172-199: LGTM: Convention-based subflow detection with validation.The helper correctly constructs the expected forms subflow name by convention and validates its contents by checking for form authenticators. If Keycloak's naming convention changes in future versions, the validation check (lines 182-196) will catch it.
239-243: LGTM: Proper integration with reconciliation flow.The auth flow configuration is invoked at the right point (after realm/client creation) with appropriate error handling and requeue logic.
291-320: LGTM: Client update logic with proper checks.The code correctly verifies existing client configuration and only performs updates when necessary. The pattern of checking both nil and value is appropriate for optional fields.
2 participants
This currently is required for only enabling user session limits configuration and in future can be extended for other configurations
Summary by CodeRabbit
New Features
Reliability