Improve handling flow configurations#4371
Open
ThaminduDilshan wants to merge 1 commit into
Open
Conversation
ThaminduDilshan
requested review from
darshanasbg and
rajithacharith
as code owners
July 27, 2026 08:21
ThaminduDilshan
added
Type/Improvement
breaking change
Contributor
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (39)
📝 WalkthroughWalkthroughThis change moves flow defaults and expiry settings into a server-configured ChangesFlow defaults and resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant inboundClientService
participant flowMgtService
participant serverConfigService
participant OUService
Client->>inboundClientService: Resolve flow defaults
inboundClientService->>flowMgtService: ResolveEffectiveFlowID
flowMgtService->>OUService: GetOrganizationUnit
OUService-->>flowMgtService: OU flow default
flowMgtService->>serverConfigService: GetMergedConfig("flow")
serverConfigService-->>flowMgtService: Server flow default
flowMgtService-->>inboundClientService: Effective flow ID
inboundClientService-->>Client: Resolved flow defaults
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
This was
linked to
issues
Jul 27, 2026
![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: 6
🧹 Nitpick comments (4)
backend/internal/ou/store_test.go (1)
726-771: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover non-empty onboarding flow IDs in persistence tests.
Both the create and update “design fields” cases leave
UserOnboardingFlowIDempty, so these tests would pass even if a real value were dropped or serialized incorrectly. Set a non-empty ID in each fixture and assert that the metadata JSON contains that exact value.Based on the supplied store implementation and test cases, the new persistence path is not currently exercised with a real onboarding flow ID.
🤖 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/ou/store_test.go` around lines 726 - 771, Update the “success with design fields” fixtures in backend/internal/ou/store_test.go lines 726-771 and 1406-1448 for both create and update cases: assign a non-empty UserOnboardingFlowID and assert that the expected metadata JSON contains that exact value, exercising persistence and serialization of onboarding flow IDs.backend/internal/flow/flowexec/service_test.go (1)
1757-1757: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer named constants over magic-number literals in expiry assertions.
mock.MatchedBy(func(exp int64) bool { return exp == int64(1800) })ands.Equal(int64(1800), ...)hardcode the fallback expiry instead of referencingdefaultAuthFlowExpiry/defaultRecoveryFlowExpiry. If the constants ever change, these assertions silently stop verifying the actual default and just re-encode a stale expectation.Also applies to: 2332-2332, 650-666
🤖 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/flow/flowexec/service_test.go` at line 1757, Replace the hardcoded 1800 expiry literals in the affected assertions, including the mock.MatchedBy callbacks and s.Equal checks, with the appropriate named constants defaultAuthFlowExpiry or defaultRecoveryFlowExpiry. Preserve the existing assertion behavior while ensuring each test references the corresponding default expiry constant.backend/internal/flow/flowexec/service.go (1)
923-923: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a shared logger component-name constant for FlowExecService.
service.gorepeats"FlowExecService"across multiple logger initializations. Define and use oneloggerComponentNameFlowExecServiceconstant in this package so future renames stay localized and consistent.🤖 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/flow/flowexec/service.go` at line 923, Define a package-level loggerComponentNameFlowExecService constant in the flow execution service package, then replace every hard-coded "FlowExecService" component name used by logger initializations in service.go with that constant, including the shown With call.backend/internal/flow/flowexec/flowDefaultsProvider_mock_test.go (1)
1-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the orphaned
flowDefaultsProvidermock artifact.
flowDefaultsProviderhas no non-mock references, while the implementation calls privateflowExecServicemethods directly (resolveDefaultFlowHandlereturns(string, *common.ServiceError)inmgt/service.go, andflowexec/service.goprivateresolveDefaultFlowHandlereturns onlystring). Keep the mock if the public interface is intended, or drop the mock-generated types if it was discarded.🤖 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/flow/flowexec/flowDefaultsProvider_mock_test.go` around lines 1 - 166, Remove the orphaned generated flowDefaultsProvider mock artifact, including newFlowDefaultsProviderMock, flowDefaultsProviderMock, and its expectation/helper methods, because the implementation uses private flowExecService methods directly. Do not alter the flowExecService implementations or their return contracts; retain the mock only if a live public interface reference is restored.
🤖 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/flow/flowexec/service.go`:
- Around line 919-942: Update getFlowSectionConfig to log an error when the
merged "flow" configuration fails the flowconfig.FlowSectionConfig type
assertion before returning the zero FlowSectionConfig. Include enough context to
identify the unexpected type, while preserving the existing fallback behavior.
In `@backend/internal/flow/mgt/service.go`:
- Around line 697-703: Log a warning when s.ouSvc.GetOrganizationUnit in the
OU-resolution block fails, including the OU identifier and error details, before
continuing to server-level default resolution. Preserve the existing successful
lookup and fallback behavior.
- Around line 738-782: Propagate the server-config read failure from
getFlowSectionConfig instead of logging a warning and returning an empty
configuration with nil error. Return the svcErr from GetMergedConfig so
resolveDefaultFlowHandle and ResolveEffectiveFlowID can surface infrastructure
failures while preserving the empty-config behavior when no service is
configured or the merged value is not a FlowSectionConfig.
In `@backend/internal/inboundclient/service.go`:
- Around line 618-622: After resolving the authentication flow in the
inbound-client acceptance path, explicitly reject an empty c.AuthFlowID before
assigning or accepting it. Update the logic around resolveFlowDefaults and
validateAuthFlowID so empty values produce the required validation error, while
valid resolved flow IDs continue through the existing assignment path.
In `@backend/internal/serverconfig/constants.go`:
- Around line 32-41: Update the documentation under docs/content/guides/ to
cover the new flow configuration represented by ConfigNameFlow, including
per-flow default handles, expiry settings, resolution precedence (override,
organization unit, then server default), and migration from prior hardcoded
defaults. For backend/internal/serverconfig/constants.go:32-41,
backend/cmd/server/bootstrap/02-server-configurations.yaml:1-13, and
backend/internal/inboundclient/service_test.go:1565-1633, add or link
documentation describing the flow section, bootstrap defaults and supported
keys, and inbound-client default resolution behavior respectively.
In `@backend/pkg/thunderidengine/engine.go`:
- Around line 172-178: The embedded-engine initialization in
`flowexec.Initialize` passes a nil server-config provider while default flow
handles are now resolved through it. Add an embedded-engine provider or option
supplying `DefaultAuthFlowHandle`, `DefaultSignOutFlowHandle`, and
`UserOnboardingFlowHandle`, or otherwise preserve an executable fallback path,
and pass it from `engine.go` so auth fallback and onboarding flow resolution
never use empty handles.
---
Nitpick comments:
In `@backend/internal/flow/flowexec/flowDefaultsProvider_mock_test.go`:
- Around line 1-166: Remove the orphaned generated flowDefaultsProvider mock
artifact, including newFlowDefaultsProviderMock, flowDefaultsProviderMock, and
its expectation/helper methods, because the implementation uses private
flowExecService methods directly. Do not alter the flowExecService
implementations or their return contracts; retain the mock only if a live public
interface reference is restored.
In `@backend/internal/flow/flowexec/service_test.go`:
- Line 1757: Replace the hardcoded 1800 expiry literals in the affected
assertions, including the mock.MatchedBy callbacks and s.Equal checks, with the
appropriate named constants defaultAuthFlowExpiry or defaultRecoveryFlowExpiry.
Preserve the existing assertion behavior while ensuring each test references the
corresponding default expiry constant.
In `@backend/internal/flow/flowexec/service.go`:
- Line 923: Define a package-level loggerComponentNameFlowExecService constant
in the flow execution service package, then replace every hard-coded
"FlowExecService" component name used by logger initializations in service.go
with that constant, including the shown With call.
In `@backend/internal/ou/store_test.go`:
- Around line 726-771: Update the “success with design fields” fixtures in
backend/internal/ou/store_test.go lines 726-771 and 1406-1448 for both create
and update cases: assign a non-empty UserOnboardingFlowID and assert that the
expected metadata JSON contains that exact value, exercising persistence and
serialization of onboarding flow IDs.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f8d6528c-6678-4f6e-9f93-5fa25201fbbe
⛔ Files ignored due to path filters (5)
backend/tests/mocks/flow/flowexecmock/flowDefaultsProvider_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/flow/flowexecmock/serverConfigProvider_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/flow/flowmgtmock/FlowMgtServiceInterface_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/flow/flowmgtmock/ouProvider_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/flow/flowmgtmock/serverConfigProvider_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (35)
.gitignorebackend/cmd/server/bootstrap/02-server-configurations.yamlbackend/cmd/server/config/default.jsonbackend/cmd/server/servicemanager.gobackend/internal/flow/config/config.gobackend/internal/flow/config/config_test.gobackend/internal/flow/flowexec/constants.gobackend/internal/flow/flowexec/flowDefaultsProvider_mock_test.gobackend/internal/flow/flowexec/init.gobackend/internal/flow/flowexec/serverConfigProvider_mock_test.gobackend/internal/flow/flowexec/service.gobackend/internal/flow/flowexec/service_test.gobackend/internal/flow/mgt/FlowMgtServiceInterface_mock_test.gobackend/internal/flow/mgt/init.gobackend/internal/flow/mgt/ouProvider_mock_test.gobackend/internal/flow/mgt/serverConfigProvider_mock_test.gobackend/internal/flow/mgt/server_config.gobackend/internal/flow/mgt/service.gobackend/internal/flow/mgt/service_test.gobackend/internal/inboundclient/ouProvider_mock_test.gobackend/internal/inboundclient/service.gobackend/internal/inboundclient/service_test.gobackend/internal/ou/error_constants.gobackend/internal/ou/handler.gobackend/internal/ou/model.gobackend/internal/ou/service.gobackend/internal/ou/store.gobackend/internal/ou/store_test.gobackend/internal/serverconfig/constants.gobackend/internal/system/i18n/core/defaults.gobackend/pkg/thunderidengine/config/config.gobackend/pkg/thunderidengine/engine.gobackend/pkg/thunderidengine/providers/model.gobuild.ps1build.sh
💤 Files with no reviewable changes (1)
- backend/cmd/server/config/default.json
ThaminduDilshan
force-pushed
the
thamindu-flow-config
branch
from
July 27, 2026 08:56
d175790 to
24ea4b6
Compare
| flowExecService := newFlowExecService(flowProvider, flowStore, flowEngine, | ||
| actorProvider, observabilitySvc, transactioner, cryptoSvc, attestationVerifier, | ||
| graphBuilder, cfg) | ||
| graphBuilder, serverConfigSvc, cfg) |
Contributor
There was a problem hiding this comment.
Do we directly inject server config? or thought of having a proxy svc only to get default flows.
If we don't have any other data required from serverConfigs
FlowExec --> DefaultFlowProvider --> serverConfig
|-> GetDefaultFlow(FlowType string) (flowId string)
Member
Author
There was a problem hiding this comment.
Is this proxy pattern something we're following in other places when accessing server config svc? I have defined a serverConfigProvider interface to downscope it only to the required method, but this doesn't restrict it to flow configurations.
type serverConfigProvider interface {
GetMergedConfig(ctx context.Context, name string) (any, *tidcommon.ServiceError)
}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/internal/flow/flowexec/service_test.go (1)
650-666: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded expiry literals replace named default constants, inconsistent with sibling test.
TestGetFlowExpirySeconds(Line 650-666),TestInitiateAndExecute_ZeroExpiryUsesDefault(Line 1757), andTestGetFlowExpirySeconds_RecoveryFlow(Line 2332) now assert raw literals (1800, 3600, 86400) instead of the named default constants (e.g.,defaultRecoveryFlowExpiry), whileTestGetFlowExpirySeconds_SignOutFallsBackToDefault(Line 3283) still referencesdefaultSignOutFlowExpiry. Referencing constants keeps the tests tied to the actual source of truth and catches unintended default drift; literals can silently pass even if the underlying default constant changes.♻️ Suggested fix (illustrative, using existing named constants)
{ name: "Authentication flow", flowType: providers.FlowTypeAuthentication, - expected: 1800, + expected: defaultAuthFlowExpiry, }, { name: "Registration flow", flowType: providers.FlowTypeRegistration, - expected: 3600, + expected: defaultRegistrationFlowExpiry, }, { name: "User onboarding flow", flowType: providers.FlowTypeUserOnboarding, - expected: 86400, + expected: defaultUserOnboardingFlowExpiry, }, { name: "Unknown flow type (fallback)", flowType: providers.FlowType("UNKNOWN_FLOW"), - expected: 1800, + expected: defaultAuthFlowExpiry, },- mockStore.EXPECT().StoreFlowContext(mock.Anything, mock.Anything, - mock.MatchedBy(func(exp int64) bool { return exp == int64(1800) })). + mockStore.EXPECT().StoreFlowContext(mock.Anything, mock.Anything, + mock.MatchedBy(func(exp int64) bool { return exp == defaultAuthFlowExpiry })).- s.Equal(int64(1800), service.getFlowExpirySeconds(context.Background(), providers.FlowTypeRecovery)) + s.Equal(defaultRecoveryFlowExpiry, service.getFlowExpirySeconds(context.Background(), providers.FlowTypeRecovery))Also applies to: 1757-1757, 2332-2332
🤖 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/flow/flowexec/service_test.go` around lines 650 - 666, Replace the hardcoded expiry literals in TestGetFlowExpirySeconds, TestInitiateAndExecute_ZeroExpiryUsesDefault, and TestGetFlowExpirySeconds_RecoveryFlow with the corresponding named default constants used by the implementation, matching TestGetFlowExpirySeconds_SignOutFallsBackToDefault. Preserve each test’s existing expected flow behavior while ensuring assertions track the source-of-truth defaults.
🤖 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.
Nitpick comments:
In `@backend/internal/flow/flowexec/service_test.go`:
- Around line 650-666: Replace the hardcoded expiry literals in
TestGetFlowExpirySeconds, TestInitiateAndExecute_ZeroExpiryUsesDefault, and
TestGetFlowExpirySeconds_RecoveryFlow with the corresponding named default
constants used by the implementation, matching
TestGetFlowExpirySeconds_SignOutFallsBackToDefault. Preserve each test’s
existing expected flow behavior while ensuring assertions track the
source-of-truth defaults.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d5c2fd2-cb4a-4d54-9114-f9d09601d2ea
⛔ Files ignored due to path filters (5)
backend/tests/mocks/flow/flowexecmock/flowDefaultsProvider_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/flow/flowexecmock/serverConfigProvider_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/flow/flowmgtmock/FlowMgtServiceInterface_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/flow/flowmgtmock/ouProvider_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/flow/flowmgtmock/serverConfigProvider_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (36)
.gitignorebackend/cmd/server/bootstrap/02-server-configurations.yamlbackend/cmd/server/config/default.jsonbackend/cmd/server/servicemanager.gobackend/internal/flow/config/config.gobackend/internal/flow/config/config_test.gobackend/internal/flow/flowexec/constants.gobackend/internal/flow/flowexec/flowDefaultsProvider_mock_test.gobackend/internal/flow/flowexec/init.gobackend/internal/flow/flowexec/serverConfigProvider_mock_test.gobackend/internal/flow/flowexec/service.gobackend/internal/flow/flowexec/service_test.gobackend/internal/flow/mgt/FlowMgtServiceInterface_mock_test.gobackend/internal/flow/mgt/init.gobackend/internal/flow/mgt/ouProvider_mock_test.gobackend/internal/flow/mgt/serverConfigProvider_mock_test.gobackend/internal/flow/mgt/server_config.gobackend/internal/flow/mgt/server_config_test.gobackend/internal/flow/mgt/service.gobackend/internal/flow/mgt/service_test.gobackend/internal/inboundclient/ouProvider_mock_test.gobackend/internal/inboundclient/service.gobackend/internal/inboundclient/service_test.gobackend/internal/ou/error_constants.gobackend/internal/ou/handler.gobackend/internal/ou/model.gobackend/internal/ou/service.gobackend/internal/ou/store.gobackend/internal/ou/store_test.gobackend/internal/serverconfig/constants.gobackend/internal/system/i18n/core/defaults.gobackend/pkg/thunderidengine/config/config.gobackend/pkg/thunderidengine/engine.gobackend/pkg/thunderidengine/providers/model.gobuild.ps1build.sh
💤 Files with no reviewable changes (1)
- backend/cmd/server/config/default.json
🚧 Files skipped from review as they are similar to previous changes (30)
- backend/pkg/thunderidengine/providers/model.go
- backend/internal/flow/flowexec/constants.go
- backend/cmd/server/bootstrap/02-server-configurations.yaml
- backend/internal/system/i18n/core/defaults.go
- backend/internal/ou/model.go
- backend/pkg/thunderidengine/config/config.go
- backend/internal/ou/error_constants.go
- backend/internal/ou/store_test.go
- backend/internal/flow/mgt/init.go
- backend/internal/flow/flowexec/serverConfigProvider_mock_test.go
- backend/internal/flow/config/config.go
- backend/internal/serverconfig/constants.go
- backend/internal/ou/handler.go
- build.sh
- backend/internal/ou/service.go
- backend/pkg/thunderidengine/engine.go
- backend/cmd/server/servicemanager.go
- backend/internal/flow/mgt/server_config.go
- backend/internal/flow/config/config_test.go
- backend/internal/ou/store.go
- backend/internal/inboundclient/ouProvider_mock_test.go
- .gitignore
- backend/internal/flow/mgt/serverConfigProvider_mock_test.go
- backend/internal/flow/mgt/FlowMgtServiceInterface_mock_test.go
- backend/internal/flow/mgt/service.go
- backend/internal/flow/mgt/ouProvider_mock_test.go
- backend/internal/inboundclient/service_test.go
- backend/internal/flow/flowexec/service.go
- backend/internal/inboundclient/service.go
- backend/internal/flow/flowexec/init.go
thiva-k
reviewed
Jul 27, 2026
| return ErrFKFlowDefinitionRetrievalFailed | ||
| } | ||
| } | ||
| c.IsRegistrationFlowEnabled = c.RegistrationFlowID != "" |
Contributor
There was a problem hiding this comment.
If at app-level we set the IsRegistrationFlowEnabled to false, wont the defaults get applied and set it to true here?
thiva-k
reviewed
Jul 27, 2026
| return err | ||
| } | ||
| c.RecoveryFlowID = recID | ||
| c.IsRecoveryFlowEnabled = c.RecoveryFlowID != "" |
ThaminduDilshan
force-pushed
the
thamindu-flow-config
branch
from
July 27, 2026 10:46
24ea4b6 to
7e3fdf1
Compare
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
3 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 introduces a new, unified server configuration section for flow management, allowing per-flow-type default handles and expiry times to be centrally managed. The changes refactor how default flow handles and expiry values are resolved, making them configurable via server settings instead of hardcoded values. The flow execution service is updated to read these values from the new configuration source. Additionally, new mock types are added to support testing of these changes.
🔧 Summary of Breaking Changes
Describe what is changing
💥 Impact
What will break? Who is affected?
🔄 Migration Guide
How should users update their code/configuration to adapt to the breaking changes? Include examples if helpful
Approach
Configuration and Flow Management Improvements:
flowsection to the server configuration, supporting per-flow-type default handles and expiry times viaFlowSectionConfigandFlowTypeConfigstructs (backend/cmd/server/bootstrap/02-server-configurations.yaml,backend/internal/flow/config/config.go) [1] [2].serverConfigProviderinterface for reading flow configuration, replacing hardcoded defaults (backend/cmd/server/servicemanager.go,backend/internal/flow/flowexec/init.go,backend/internal/flow/flowexec/service.go) [1] [2] [3] [4] [5] [6] [7] [8] [9].Code Simplification and Test Support:
getFlowExpirySecondsmethod in favor of reading expiry values from configuration, and updated all usages accordingly (backend/internal/flow/flowexec/service.go) [1] [2].backend/internal/flow/flowexec/flowDefaultsProvider_mock_test.go,backend/internal/flow/flowexec/serverConfigProvider_mock_test.go) [1] [2].Other Updates:
backend/internal/flow/flowexec/constants.go,backend/internal/flow/config/config_test.go) [1] [2] [3].Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit