Remove app's registration/ recovery flow when it's disabled - #4840
Merged
Conversation
ThaminduDilshan
added
Type/Bug
trigger-pr-builder
Contributor
📝 WalkthroughWalkthroughThe inbound-client service now clears disabled registration and recovery flow IDs, resolves those flows only when enabled, disables flows with empty resolutions, and limits referenced-flow auto-fill to sign-out bindings. Unit and integration tests cover resolution, persistence, errors, and reconciliation. ChangesInbound flow binding handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@backend/internal/inboundclient/service.go`:
- Around line 611-621: Update docs/content/apis.mdx to document the behavior at
backend/internal/inboundclient/service.go:611-621, where disabled registration
or recovery bindings clear persisted flow IDs; at 657-677, where enabled flows
are disabled if no effective flow resolves; and at 1887-1894, where
reconciliation only auto-fills sign-out bindings. Cover all three service.go
sites in the relevant API documentation.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 71767879-aa26-41c9-aaac-816841aec4af
📒 Files selected for processing (2)
backend/internal/inboundclient/service.gobackend/internal/inboundclient/service_test.go
Comment on lines
+611
to
+621
| // Drop registration/recovery bindings whose enable flag is false before any resolution so we | ||
| // never persist an ID that contradicts the toggle. This must run regardless of whether flowMgt | ||
| // is wired — persistence should still respect the caller's disabled intent. | ||
| if !c.IsRegistrationFlowEnabled { | ||
| c.RegistrationFlowID = "" | ||
| } | ||
| if !c.IsRecoveryFlowEnabled { | ||
| c.RecoveryFlowID = "" | ||
| } | ||
|
|
||
| if s.flowMgt == nil { |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🔴 Documentation Required
This PR introduces user-facing changes that are not covered by documentation updates under docs/.
Please update the relevant documentation before merging.
Missing documentation:
backend/internal/inboundclient/service.go#L611-L621: Document that disabled registration and recovery bindings clear persisted flow IDs indocs/content/apis.mdx.backend/internal/inboundclient/service.go#L657-L677: Document that enabled registration and recovery flows are disabled when no effective flow resolves indocs/content/apis.mdx.backend/internal/inboundclient/service.go#L1887-L1894: Document that reconciliation only auto-fills sign-out bindings indocs/content/apis.mdx.
As per path instructions: “If ANY of the above are detected … post a single consolidated PR-level comment.”
📍 Affects 1 file
backend/internal/inboundclient/service.go#L611-L621(this comment)backend/internal/inboundclient/service.go#L657-L677backend/internal/inboundclient/service.go#L1887-L1894
🤖 Prompt for 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.
In `@backend/internal/inboundclient/service.go` around lines 611 - 621, Update
docs/content/apis.mdx to document the behavior at
backend/internal/inboundclient/service.go:611-621, where disabled registration
or recovery bindings clear persisted flow IDs; at 657-677, where enabled flows
are disabled if no effective flow resolves; and at 1887-1894, where
reconciliation only auto-fills sign-out bindings. Cover all three service.go
sites in the relevant API documentation.
Source: Path instructions
ThaminduDilshan
added
trigger-pr-builder
thiva-k
previously approved these changes
Aug 11, 2026
ThaminduDilshan
enabled auto-merge
August 11, 2026 11:08
ThaminduDilshan
force-pushed
the
thamindu-fixes
branch
from
August 11, 2026 11:41
acf588e to
98c53fe
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/application/application_api_test.go (1)
3636-3657: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win🔴 Documentation Required
This PR introduces user-facing changes that are not covered by documentation updates under
docs/.
Please update the relevant documentation before merging.Missing documentation:
- Application API flow bindings: document disabled registration and recovery bindings, conditional flow-ID resolution, and empty-ID persistence in
docs/content/apis.mdx.- Flow reference reconciliation: document that registration and recovery bindings are not auto-filled, while sign-out bindings remain auto-filled. Update
docs/content/guides/applications/application-settings.mdx.🤖 Prompt for 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. In `@tests/integration/application/application_api_test.go` around lines 3636 - 3657, Update docs/content/apis.mdx to document disabled registration and recovery bindings, conditional flow-ID resolution, and persistence of empty flow IDs. Update docs/content/guides/applications/application-settings.mdx to clarify that registration and recovery bindings are not auto-filled, while sign-out bindings remain auto-filled.Source: Path instructions
🤖 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 `@tests/integration/application/flow_reference_validation_test.go`:
- Around line 29-32: Update docs/content/apis.mdx to document disabled
registration and recovery binding persistence, enabled binding mismatch
rejection, and the related application API fields. Update
docs/content/guides/inbound-client-flow-bindings.mdx to clarify that disabled
registration and recovery bindings are not auto-filled while sign-out bindings
remain auto-filled, matching the contract described near the
registration/recovery flow validation test.
---
Outside diff comments:
In `@tests/integration/application/application_api_test.go`:
- Around line 3636-3657: Update docs/content/apis.mdx to document disabled
registration and recovery bindings, conditional flow-ID resolution, and
persistence of empty flow IDs. Update
docs/content/guides/applications/application-settings.mdx to clarify that
registration and recovery bindings are not auto-filled, while sign-out bindings
remain auto-filled.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 68299aaa-3717-4b48-9a17-7d1104eeeaf0
📒 Files selected for processing (2)
tests/integration/application/application_api_test.gotests/integration/application/flow_reference_validation_test.go
Comment on lines
+29
to
+32
| // REGISTRATION / RECOVERY flow via a CALL node, the app must either declare a matching binding | ||
| // (with the corresponding enable flag on) or leave it disabled — in the disabled case the server | ||
| // persists an empty binding regardless of what the auth flow calls. Sign-out still auto-fills | ||
| // because it has no enable toggle. Genuine mismatches with an enabled binding still reject. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🔴 Documentation Required
This PR introduces user-facing changes that are not covered by documentation updates under docs/.
Please update the relevant documentation before merging.
Missing documentation:
docs/content/apis.mdx: Document disabled registration and recovery binding persistence, enabled mismatch rejection, and the related application API fields.docs/content/guides/inbound-client-flow-bindings.mdx: Document that disabled registration and recovery bindings are not auto-filled, while sign-out bindings remain auto-filled.
Changed-contract anchor:
tests/integration/application/flow_reference_validation_test.go#L29-L32: This range records the user-facing reconciliation contract that requires documentation.
As per path instructions, user-facing behavior changes require corresponding updates under docs/.
🤖 Prompt for 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.
In `@tests/integration/application/flow_reference_validation_test.go` around lines
29 - 32, Update docs/content/apis.mdx to document disabled registration and
recovery binding persistence, enabled binding mismatch rejection, and the
related application API fields. Update
docs/content/guides/inbound-client-flow-bindings.mdx to clarify that disabled
registration and recovery bindings are not auto-filled while sign-out bindings
remain auto-filled, matching the contract described near the
registration/recovery flow validation test.
Source: Path instructions
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
thiva-k
approved these changes
Aug 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
This pull request updates the handling of registration and recovery flow bindings in the inbound client service to ensure that disabled flows do not retain or persist stale flow IDs, and that auto-filling of registration/recovery flows is prevented when the enable flag is false. It also adds and updates tests to validate these behaviors.
Approach
Flow binding logic improvements:
resolveFlowDefaults, registration and recovery flow IDs are now proactively cleared if their enable flags are false, and resolution for these flows is skipped in this case. This prevents persisting IDs that contradict the disabled state. [1] [2] [3] [4]walkReferencedFlows, the service no longer auto-fills registration or recovery flow IDs when they are missing and the enable flag is false, ensuring no phantom configuration is created. Only sign-out flows are auto-filled if missing.Test updates and additions:
Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit