Add flow-centric browser SSO - #3779
Merged
rajithacharith merged 1 commit intoJul 10, 2026
Merged
Conversation
madurangasiriwardena
requested review from
DonOmalVindula,
ThaminduDilshan,
darshanasbg,
rajithacharith and
thiva-k
as code owners
July 6, 2026 18:19
Contributor
📝 WalkthroughWalkthroughAdds persistent SSO sessions with checkpoint save/load behavior, per-flow cookie transport, assurance validation for ChangesSSO session backend
Estimated code review effort: 5 (Critical) | ~120 minutes 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 |
madurangasiriwardena
changed the title
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![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 (8)
frontend/apps/console/src/features/flows/components/resources/steps/execution/Execution.tsx (1)
52-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct test coverage for the new
display.description/display.outcomesmapping inExecution.tsx.The downstream consumers (
ExecutionMinimal,ExecutionFactory) are tested, but the mapping itself inExecution.tsx(i.e.,data.display→resource.display) has no direct test, so a typo or dropped field here wouldn't be caught.As per path instructions,
**/*.{go,md,mdx,tsx,ts,js,jsx,yaml,yml}: "Write tests for new features and bug fixes, targeting 80%+ coverage."Also applies to: 78-80
🤖 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 `@frontend/apps/console/src/features/flows/components/resources/steps/execution/Execution.tsx` around lines 52 - 60, Add direct test coverage for the `Execution.tsx` mapping from `data.display` to `resource.display`, specifically the new `display.description` and `display.outcomes` fields. Update or add tests around the `displayFromData` handling in `Execution` so they assert the mapped `resource.display` includes these properties before it reaches `ExecutionMinimal`/`ExecutionFactory`, catching typos or dropped fields in this component itself.Source: Path instructions
backend/cmd/server/bootstrap/flows/authentication/auth_flow_sso.json (1)
1-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win🔴 Documentation Required
This PR introduces user-facing changes that are not covered by documentation updates underdocs/.
Please update the relevant documentation before merging.Missing documentation:
- SSO authentication flow: new
default-sso-flowbootstrap flow withSSOCheckExecutor/SessionExecutorjoin-point nodes — document indocs/content/guides/.- Session configuration: new
session.idle_timeout_seconds/session.absolute_timeout_secondsdeployment settings — document indocs/content/config reference.🤖 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/cmd/server/bootstrap/flows/authentication/auth_flow_sso.json` around lines 1 - 127, The new default-sso-flow bootstrap flow and session timeout settings are user-facing and need corresponding docs updates. Add a guide under docs/content/guides/ that explains the default-sso-flow behavior, including the SSOCheckExecutor and SessionExecutor join points, and update the config reference under docs/content/ to document session.idle_timeout_seconds and session.absolute_timeout_seconds. Use the flow handle default-sso-flow and the session settings names to locate the relevant documentation sections.Source: Path instructions
frontend/apps/console/src/features/flows/utils/flowToCanvasTransformer.ts (1)
292-304: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winApply the same conditional-spread fix to CALL nodes.
The CALL node branch still unconditionally sets
onFailure: apiNode.onFailure, which can beundefined, unlike the TASK_EXECUTION branch just above that now conditionally spreads it. If downstream rendering keys off presence ofonFailure, CALL nodes without a failure branch may still show a dangling handle.♻️ Suggested fix
if (stepType === StepTypes.Call) { canvasNode.data = { flow: apiNode.flow ?? {ref: ''}, action: { type: 'CALL', flow: apiNode.flow ?? {ref: ''}, onSuccess: apiNode.onSuccess, - onFailure: apiNode.onFailure, + ...(apiNode.onFailure !== undefined ? {onFailure: apiNode.onFailure} : {}), }, };🤖 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 `@frontend/apps/console/src/features/flows/utils/flowToCanvasTransformer.ts` around lines 292 - 304, The CALL node handling in flowToCanvasTransformer still assigns onFailure unconditionally, so it can appear present even when missing. Update the StepTypes.Call branch in flowToCanvasTransformer to match the TASK_EXECUTION conditional-spread pattern: build the action object for CALL with onSuccess always set, and only include onFailure when apiNode.onFailure is defined. This should be done in the canvasNode.data assignment where apiNode.flow, onSuccess, and onFailure are mapped.backend/internal/flow/executor/error_constants.go (1)
1178-1197: 🗄️ Data Integrity & Integration | 🔵 TrivialTODO left for wiring
interaction_requiredto an OAuth2 redirect.The error itself maps to a real OIDC error code — this specification also defines the following error codes... The Authorization Server requires End-User interaction of some form to proceed. This error MAY be returned when the prompt parameter value in the Authentication Request is none, but the Authentication Request cannot be completed without displaying a user interface for End-User interaction. The constant is defined but the actual redirect wiring is left as a TODO. Confirm this is tracked before this error path ships to users, otherwise
ErrInteractionRequiredwill just surface as an opaque flow error rather than driving the expected OAuth2 authorize-error redirect.🤖 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/executor/error_constants.go` around lines 1178 - 1197, The ErrInteractionRequired constant in error_constants.go is defined, but the TODO for wiring it into an OAuth2 authorize-error redirect is still unresolved. Make sure the interaction_required error path is tracked and implemented in the flow executor so it triggers the expected redirect instead of surfacing as a generic flow error; use ErrInteractionRequired and its existing error mapping as the integration point.backend/internal/flow/session/model.go (1)
63-67: 🔒 Security & Privacy | 🔵 TrivialHandle rotation TODO — track before GA.
Until handle rotation lands, a leaked
HandleIDremains valid for the full session lifetime (idle/absolute deadlines only). Worth prioritizing before this ships broadly, or ensure it's tracked inSSO_TODO.md.Want me to open a follow-up issue for handle rotation, or draft an implementation?
🤖 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/session/model.go` around lines 63 - 67, The session handle rotation TODO in the HandleID field of session model.go needs to be tracked before GA, since a leaked handle remains valid for the full session lifetime. Add or update a tracked item in SSO_TODO.md for the handle rotation work, or otherwise create a follow-up issue tied to the session model/session cookie flow so it is explicitly tracked. Use the HandleID field comment and the session handle rotation TODO as the anchor points when locating the code.backend/internal/flow/executor/session_executor_test.go (1)
362-375: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't assert the session row is actually established when context write fails.
The comment states "the session may be established, but no handle is emitted," but the test never asserts
store.createdis non-nil to confirm the session row was in fact persisted before the context failure. Worth adding that assertion to lock in the documented behavior.TestSession_FreshSave_SessionContextErrorIsNonFatal verifies that a checkpoint-context write failure degrades SSO without failing auth: the session may be established, but no handle is emitted and the checkpoint is not recorded, so the session simply holds no reusable checkpoint.
✅ Suggested assertion addition
resp, err := exec.Execute(freshCtx()) require.NoError(t, err) assert.Equal(t, providers.ExecComplete, resp.Status) + assert.NotNil(t, store.created, "the session row must still be established") // The cookie is emitted only after the checkpoint context commits, so a context failure emits none. assert.Empty(t, resp.AdditionalData[session.SessionHandleKey])🤖 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/executor/session_executor_test.go` around lines 362 - 375, The test for session context write failure is missing an assertion that the session row was still created before the checkpoint context failed. Update TestSession_FreshSave_SessionContextErrorIsNonFatal to also verify store.created is non-nil, alongside the existing checks on authCtx.created, resp.AdditionalData, and resp.RuntimeData, so the behavior of session establishment without emitted handle is locked in.backend/internal/flow/session/state.go (1)
50-61: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider validating
Idle <= Absolute.
NewTimeoutsindependently substitutes defaults for non-positive inputs but never checks that the resolvedIdledoesn't exceedAbsolute. If a caller suppliesidleSecondslarger thanabsoluteSeconds(e.g., via misconfiguration), the idle deadline could sit beyond the absolute deadline, making the absolute cap the only effective one silently. GivenSessionConfigvalidation lives in a different layer not included here, this may already be handled upstream.🤖 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/session/state.go` around lines 50 - 61, In NewTimeouts, add a validation step after resolving the defaults and overrides to ensure the final Idle timeout never exceeds the final Absolute timeout. Use the existing DefaultTimeouts logic in session/state.go and adjust the returned Timeouts or reject the invalid combination consistently with SessionConfig validation, so misconfigured idleSeconds and absoluteSeconds cannot produce an invalid session timeout pair.backend/cmd/server/servicemanager.go (1)
355-361: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the already-captured
flowConfiginstead of re-resolving runtime config three times.
flowConfig := flowconfig.FromServerRuntime()is already captured above (line 327). Callingflowconfig.FromServerRuntime()again forDeploymentIDon each store constructor is redundant.♻️ Suggested cleanup
- SessionStore: flowsession.NewStore( - dbprovider.GetDBProvider(), flowconfig.FromServerRuntime().DeploymentID), - SessionContextStore: flowsession.NewSessionContextStore( - dbprovider.GetDBProvider(), flowconfig.FromServerRuntime().DeploymentID, - flowsession.NewPassthroughEncryptor()), - SessionParticipantStore: flowsession.NewParticipantStore( - dbprovider.GetDBProvider(), flowconfig.FromServerRuntime().DeploymentID), + SessionStore: flowsession.NewStore( + dbprovider.GetDBProvider(), flowConfig.DeploymentID), + SessionContextStore: flowsession.NewSessionContextStore( + dbprovider.GetDBProvider(), flowConfig.DeploymentID, + flowsession.NewPassthroughEncryptor()), + SessionParticipantStore: flowsession.NewParticipantStore( + dbprovider.GetDBProvider(), flowConfig.DeploymentID),🤖 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/cmd/server/servicemanager.go` around lines 355 - 361, Reuse the existing flowConfig captured in servicemanager.go instead of calling flowconfig.FromServerRuntime() again when constructing SessionStore, SessionContextStore, and SessionParticipantStore. Update those constructors to read DeploymentID from flowConfig so the already-resolved runtime config is used consistently and the repeated lookups are removed.
🤖 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/handler.go`:
- Around line 36-44: The SSO cookie in newFlowExecutionHandler is still created
with Secure hardcoded to false, so update the ssoCarrier initialization to read
the Secure setting from the server’s deployment/TLS config instead of using a
constant. Use the existing flowExecService-backed handler path and
session.NewCookieCarrier in handler.go, and wire it to the same
runtime.Config.Server security configuration pattern used elsewhere so the
cookie respects whether the deployment is running behind TLS.
- Around line 86-95: The SSO handle cookie in handler.go is using the package
default timeout instead of the configured session absolute timeout. Update the
flow in flowexec handler to pass the server-configured
Session.AbsoluteTimeoutSeconds through to h.ssoCarrier.Write instead of
session.DefaultAbsoluteTimeout, so the cookie lifetime matches the runtime
configuration. Use the existing flowStep.SSOHandleOut/SSOFlowID branch and the
session timeout wiring from servicemanager.go to locate the right value.
In `@backend/internal/flow/session/errors.go`:
- Around line 28-30: The error message for ErrSessionContextTooLarge in
errors.go contains a duplicated word (“session session”), so update the error
string to use a single “session” while keeping the same sentinel name and
surrounding comment unchanged.
In `@backend/internal/flow/session/SSO_CONTEXT_CLASSIFICATION.md`:
- Line 50: The markdown heading in SSO_CONTEXT_CLASSIFICATION.md is failing Vale
because it is not in Title Case. Update the top-level heading text under the
flow-context section to use proper Title Case, keeping the same meaning while
matching the style rules for headings.
In `@backend/internal/flow/session/SSO_TODO.md`:
- Line 50: The markdown headings in SSO_TODO.md are failing Vale Title Case
checks; update the affected heading text to proper Title Case while preserving
meaning. Fix the headings identified by the Session lifecycle section and the
related Revocation + logout and Attribute-storage architecture headings,
ensuring the heading strings themselves conform to the Vale title-case style
without changing surrounding content.
- Line 229: The Vale failure is caused by the informal abbreviation “repo” in
this documentation note; replace it with “repository” in the SSO_TODO content.
Update the surrounding sentence to keep the meaning intact while using the full
word, and scan nearby prose in the same section for any similar informal
abbreviations.
---
Nitpick comments:
In `@backend/cmd/server/bootstrap/flows/authentication/auth_flow_sso.json`:
- Around line 1-127: The new default-sso-flow bootstrap flow and session timeout
settings are user-facing and need corresponding docs updates. Add a guide under
docs/content/guides/ that explains the default-sso-flow behavior, including the
SSOCheckExecutor and SessionExecutor join points, and update the config
reference under docs/content/ to document session.idle_timeout_seconds and
session.absolute_timeout_seconds. Use the flow handle default-sso-flow and the
session settings names to locate the relevant documentation sections.
In `@backend/cmd/server/servicemanager.go`:
- Around line 355-361: Reuse the existing flowConfig captured in
servicemanager.go instead of calling flowconfig.FromServerRuntime() again when
constructing SessionStore, SessionContextStore, and SessionParticipantStore.
Update those constructors to read DeploymentID from flowConfig so the
already-resolved runtime config is used consistently and the repeated lookups
are removed.
In `@backend/internal/flow/executor/error_constants.go`:
- Around line 1178-1197: The ErrInteractionRequired constant in
error_constants.go is defined, but the TODO for wiring it into an OAuth2
authorize-error redirect is still unresolved. Make sure the interaction_required
error path is tracked and implemented in the flow executor so it triggers the
expected redirect instead of surfacing as a generic flow error; use
ErrInteractionRequired and its existing error mapping as the integration point.
In `@backend/internal/flow/executor/session_executor_test.go`:
- Around line 362-375: The test for session context write failure is missing an
assertion that the session row was still created before the checkpoint context
failed. Update TestSession_FreshSave_SessionContextErrorIsNonFatal to also
verify store.created is non-nil, alongside the existing checks on
authCtx.created, resp.AdditionalData, and resp.RuntimeData, so the behavior of
session establishment without emitted handle is locked in.
In `@backend/internal/flow/session/model.go`:
- Around line 63-67: The session handle rotation TODO in the HandleID field of
session model.go needs to be tracked before GA, since a leaked handle remains
valid for the full session lifetime. Add or update a tracked item in SSO_TODO.md
for the handle rotation work, or otherwise create a follow-up issue tied to the
session model/session cookie flow so it is explicitly tracked. Use the HandleID
field comment and the session handle rotation TODO as the anchor points when
locating the code.
In `@backend/internal/flow/session/state.go`:
- Around line 50-61: In NewTimeouts, add a validation step after resolving the
defaults and overrides to ensure the final Idle timeout never exceeds the final
Absolute timeout. Use the existing DefaultTimeouts logic in session/state.go and
adjust the returned Timeouts or reject the invalid combination consistently with
SessionConfig validation, so misconfigured idleSeconds and absoluteSeconds
cannot produce an invalid session timeout pair.
In
`@frontend/apps/console/src/features/flows/components/resources/steps/execution/Execution.tsx`:
- Around line 52-60: Add direct test coverage for the `Execution.tsx` mapping
from `data.display` to `resource.display`, specifically the new
`display.description` and `display.outcomes` fields. Update or add tests around
the `displayFromData` handling in `Execution` so they assert the mapped
`resource.display` includes these properties before it reaches
`ExecutionMinimal`/`ExecutionFactory`, catching typos or dropped fields in this
component itself.
In `@frontend/apps/console/src/features/flows/utils/flowToCanvasTransformer.ts`:
- Around line 292-304: The CALL node handling in flowToCanvasTransformer still
assigns onFailure unconditionally, so it can appear present even when missing.
Update the StepTypes.Call branch in flowToCanvasTransformer to match the
TASK_EXECUTION conditional-spread pattern: build the action object for CALL with
onSuccess always set, and only include onFailure when apiNode.onFailure is
defined. This should be done in the canvasNode.data assignment where
apiNode.flow, onSuccess, and onFailure are mapped.
🪄 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: 97240157-de4d-4b6e-9ed1-5e1547b9998e
⛔ Files ignored due to path filters (1)
backend/tests/mocks/flow/flowexecmock/flowVersionLookup_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (58)
backend/cmd/server/bootstrap/flows/authentication/auth_flow_sso.jsonbackend/cmd/server/deployment.yamlbackend/cmd/server/servicemanager.gobackend/dbscripts/runtimedb/postgres.sqlbackend/dbscripts/runtimedb/sqlite.sqlbackend/internal/flow/common/constants.gobackend/internal/flow/executor/auth_assert_assurance_test.gobackend/internal/flow/executor/auth_assert_executor.gobackend/internal/flow/executor/constants.gobackend/internal/flow/executor/error_constants.gobackend/internal/flow/executor/register.gobackend/internal/flow/executor/session_executor.gobackend/internal/flow/executor/session_executor_test.gobackend/internal/flow/executor/sso_check_executor.gobackend/internal/flow/executor/sso_check_executor_test.gobackend/internal/flow/flowexec/engine.gobackend/internal/flow/flowexec/flowVersionLookup_mock_test.gobackend/internal/flow/flowexec/handler.gobackend/internal/flow/flowexec/model.gobackend/internal/flow/flowexec/service.gobackend/internal/flow/flowexec/service_sso_test.gobackend/internal/flow/mgt/graph_builder_sso_test.gobackend/internal/flow/session/SSO_CONTEXT_CLASSIFICATION.mdbackend/internal/flow/session/SSO_TODO.mdbackend/internal/flow/session/crypto.gobackend/internal/flow/session/errors.gobackend/internal/flow/session/inputs.gobackend/internal/flow/session/model.gobackend/internal/flow/session/participant.gobackend/internal/flow/session/participant_store.gobackend/internal/flow/session/participant_store_test.gobackend/internal/flow/session/queries.gobackend/internal/flow/session/resolver.gobackend/internal/flow/session/resolver_test.gobackend/internal/flow/session/session_context.gobackend/internal/flow/session/session_context_store.gobackend/internal/flow/session/session_context_store_test.gobackend/internal/flow/session/state.gobackend/internal/flow/session/state_test.gobackend/internal/flow/session/store.gobackend/internal/flow/session/store_test.gobackend/internal/flow/session/transient_test.gobackend/internal/flow/session/transport.gobackend/internal/flow/session/transport_test.gobackend/internal/oauth/oauth2/authz/service.gobackend/internal/oauth/oauth2/constants/constants.gobackend/internal/oauth/oauth2/model/parameter.gobackend/internal/system/config/config.gobackend/internal/system/config/config_test.gobackend/internal/system/i18n/core/defaults.gofrontend/apps/console/src/features/flows/components/resources/steps/execution/Execution.tsxfrontend/apps/console/src/features/flows/components/resources/steps/execution/ExecutionMinimal.tsxfrontend/apps/console/src/features/flows/components/resources/steps/execution/__tests__/ExecutionMinimal.test.tsxfrontend/apps/console/src/features/flows/components/resources/steps/execution/execution-factory/ExecutionFactory.tsxfrontend/apps/console/src/features/flows/components/resources/steps/execution/execution-factory/__tests__/ExecutionFactory.test.tsxfrontend/apps/console/src/features/flows/models/base.tsfrontend/apps/console/src/features/flows/utils/flowToCanvasTransformer.tsfrontend/apps/console/src/features/login-flow/data/executors.json
|
|
||
| --- | ||
|
|
||
| ## 1. Top-level flow-context fields |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix Vale Title Case CI failure on heading.
The Vale style check pipeline fails on this heading for not using Title Case.
✏️ Proposed fix
-## 1. Top-level flow-context fields
+## 1. Top-Level Flow-Context Fields📝 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.
Suggested change
| ## 1. Top-level flow-context fields | |
| ## 1. Top-Level Flow-Context Fields |
🧰 Tools
🪛 GitHub Actions: 🥒 Vale Lint (Changed Markdown Only) / 0_Vale style check.txt
[error] 50-50: WSO2-IAM.TitleCaseTitles: Use Title Case for headings.
🪛 GitHub Actions: 🥒 Vale Lint (Changed Markdown Only) / Vale style check
[error] 50-50: Vale rule [WSO2-IAM.TitleCaseTitles]: Use Title Case for headings.
🪛 GitHub Check: Vale style check
[failure] 50-50:
[vale] reported by reviewdog 🐶
[WSO2-IAM.TitleCaseTitles] Use Title Case for headings.
Raw Output:
{"message": "[WSO2-IAM.TitleCaseTitles] Use Title Case for headings.", "location": {"path": "backend/internal/flow/session/SSO_CONTEXT_CLASSIFICATION.md", "range": {"start": {"line": 50, "column": 4}}}, "severity": "ERROR"}
🤖 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/session/SSO_CONTEXT_CLASSIFICATION.md` at line 50, The
markdown heading in SSO_CONTEXT_CLASSIFICATION.md is failing Vale because it is
not in Title Case. Update the top-level heading text under the flow-context
section to use proper Title Case, keeping the same meaning while matching the
style rules for headings.
Source: Pipeline failures
| the `__Host-` prefix as a separate managed flag rather than folding it into this free-form value. | ||
| — `transport.go` (`cookieNamePrefix` const, `CookieName`). | ||
|
|
||
| ## Session lifecycle |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix Vale Title Case CI failures on headings.
The Vale style check pipeline reports Title Case violations on these headings ("## Session lifecycle", "### 6. Revocation + logout", "### 7b. Attribute-storage architecture (implemented) — reference by default, persist only what can't be re-resolved").
✏️ Proposed fixes
-## Session lifecycle
+## Session Lifecycle-### 6. Revocation + logout
+### 6. Revocation + Logout-### 7b. Attribute-storage architecture (implemented) — reference by default, persist only what can't be re-resolved
+### 7b. Attribute-Storage Architecture (Implemented) — Reference by Default, Persist Only What Can't Be Re-ResolvedAlso applies to: 67-67, 124-124
🧰 Tools
🪛 GitHub Actions: 🥒 Vale Lint (Changed Markdown Only) / 0_Vale style check.txt
[error] 50-50: WSO2-IAM.TitleCaseTitles: Use Title Case for headings.
🪛 GitHub Actions: 🥒 Vale Lint (Changed Markdown Only) / Vale style check
[error] 50-50: Vale rule [WSO2-IAM.TitleCaseTitles]: Use Title Case for headings.
🪛 GitHub Check: Vale style check
[failure] 50-50:
[vale] reported by reviewdog 🐶
[WSO2-IAM.TitleCaseTitles] Use Title Case for headings.
Raw Output:
{"message": "[WSO2-IAM.TitleCaseTitles] Use Title Case for headings.", "location": {"path": "backend/internal/flow/session/SSO_TODO.md", "range": {"start": {"line": 50, "column": 4}}}, "severity": "ERROR"}
🤖 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/session/SSO_TODO.md` at line 50, The markdown headings
in SSO_TODO.md are failing Vale Title Case checks; update the affected heading
text to proper Title Case while preserving meaning. Fix the headings identified
by the Session lifecycle section and the related Revocation + logout and
Attribute-storage architecture headings, ensuring the heading strings themselves
conform to the Vale title-case style without changing surrounding content.
Source: Pipeline failures
| completed assertion. **Transport to design:** have the gate forward a flow error to | ||
| `/oauth2/auth/callback` (extend it to accept an error code instead of an assertion) → map to | ||
| `error=interaction_required`. Likely needs coordinated changes in the **gate SDK** (separate | ||
| repo) as well as the backend. — `error_constants.go`, `authz/service.go`, `authz/handler.go`. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix Vale informal-abbreviation CI failure.
The Vale style check pipeline flags "repo" as an informal abbreviation; use "repository".
✏️ Proposed fix
- `error=interaction_required`. Likely needs coordinated changes in the **gate SDK** (separate
- repo) as well as the backend. — `error_constants.go`, `authz/service.go`, `authz/handler.go`.
+ `error=interaction_required`. Likely needs coordinated changes in the **gate SDK** (separate
+ repository) as well as the backend. — `error_constants.go`, `authz/service.go`, `authz/handler.go`.📝 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.
Suggested change
| repo) as well as the backend. — `error_constants.go`, `authz/service.go`, `authz/handler.go`. | |
| repository) as well as the backend. — `error_constants.go`, `authz/service.go`, `authz/handler.go`. |
🧰 Tools
🪛 GitHub Actions: 🥒 Vale Lint (Changed Markdown Only) / 0_Vale style check.txt
[error] 229-229: WSO2-IAM.NoInformalAbbreviations: Use 'repository' instead of 'repo'. Avoid informal abbreviations in documentation.
[warning] 229-229: Vale.Spelling: Did you really mean 'repo'?
🪛 GitHub Actions: 🥒 Vale Lint (Changed Markdown Only) / Vale style check
[error] 229-229: Vale rule [WSO2-IAM.NoInformalAbbreviations]: Use 'repository' instead of 'repo'. Avoid informal abbreviations in documentation.
[warning] 229-229: Vale rule [Vale.Spelling]: Did you really mean "repo"?
🪛 GitHub Check: Vale style check
[failure] 229-229:
[vale] reported by reviewdog 🐶
[WSO2-IAM.NoInformalAbbreviations] Use 'repository' instead of 'repo'. Avoid informal abbreviations in documentation.
Raw Output:
{"message": "[WSO2-IAM.NoInformalAbbreviations] Use 'repository' instead of 'repo'. Avoid informal abbreviations in documentation.", "location": {"path": "backend/internal/flow/session/SSO_TODO.md", "range": {"start": {"line": 229, "column": 3}}}, "severity": "ERROR"}
🤖 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/session/SSO_TODO.md` at line 229, The Vale failure is
caused by the informal abbreviation “repo” in this documentation note; replace
it with “repository” in the SSO_TODO content. Update the surrounding sentence to
keep the meaning intact while using the full word, and scan nearby prose in the
same section for any similar informal abbreviations.
Source: Pipeline failures
madurangasiriwardena
force-pushed
the
feature/session-poc-2-rebased
branch
from
July 7, 2026 03:36
82e62e1 to
5e68f6a
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/internal/flow/flowexec/handler_test.go (1)
49-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the new SSO cookie transport behavior.
These updates only pass
(false, 0)to the new constructor. Please add focused coverage that verifies inbound cookies are available on the service context and thatSSOHandleOutwrites the expected per-flow cookie with a non-zero TTL/security setting. As per coding guidelines, "**/*.{go,md,mdx,tsx,ts,js,jsx,yaml,yml}: Write tests for new features and bug fixes, targeting 80%+ coverage."Also applies to: 107-107, 124-124, 144-144, 172-172
🤖 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/handler_test.go` at line 49, Add focused tests around newFlowExecutionHandler and the flow execution handler paths to cover the SSO cookie transport behavior. Verify that inbound cookies are propagated onto the service context in the relevant handler/request flow, and add a test for SSOHandleOut that confirms it writes the per-flow cookie with a non-zero TTL and secure/httpOnly-style settings as expected. Use the existing handler_test.go cases that construct the handler with newFlowExecutionHandler(mockSvc, false, 0) as the entry points for these assertions.Source: Coding guidelines
🤖 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/handler_test.go`:
- Line 49: Add focused tests around newFlowExecutionHandler and the flow
execution handler paths to cover the SSO cookie transport behavior. Verify that
inbound cookies are propagated onto the service context in the relevant
handler/request flow, and add a test for SSOHandleOut that confirms it writes
the per-flow cookie with a non-zero TTL and secure/httpOnly-style settings as
expected. Use the existing handler_test.go cases that construct the handler with
newFlowExecutionHandler(mockSvc, false, 0) as the entry points for these
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2849fe06-f5e1-469d-b7e6-84775f803b79
⛔ Files ignored due to path filters (1)
backend/tests/mocks/flow/flowexecmock/flowVersionLookup_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (58)
backend/cmd/server/bootstrap/flows/authentication/auth_flow_sso.jsonbackend/cmd/server/deployment.yamlbackend/cmd/server/servicemanager.gobackend/dbscripts/runtimedb/postgres.sqlbackend/dbscripts/runtimedb/sqlite.sqlbackend/internal/flow/common/constants.gobackend/internal/flow/executor/auth_assert_assurance_test.gobackend/internal/flow/executor/auth_assert_executor.gobackend/internal/flow/executor/constants.gobackend/internal/flow/executor/error_constants.gobackend/internal/flow/executor/register.gobackend/internal/flow/executor/session_executor.gobackend/internal/flow/executor/session_executor_test.gobackend/internal/flow/executor/sso_check_executor.gobackend/internal/flow/executor/sso_check_executor_test.gobackend/internal/flow/flowexec/engine.gobackend/internal/flow/flowexec/flowVersionLookup_mock_test.gobackend/internal/flow/flowexec/handler.gobackend/internal/flow/flowexec/handler_test.gobackend/internal/flow/flowexec/init.gobackend/internal/flow/flowexec/model.gobackend/internal/flow/flowexec/service.gobackend/internal/flow/flowexec/service_sso_test.gobackend/internal/flow/mgt/graph_builder_sso_test.gobackend/internal/flow/session/crypto.gobackend/internal/flow/session/errors.gobackend/internal/flow/session/inputs.gobackend/internal/flow/session/model.gobackend/internal/flow/session/participant.gobackend/internal/flow/session/participant_store.gobackend/internal/flow/session/participant_store_test.gobackend/internal/flow/session/queries.gobackend/internal/flow/session/resolver.gobackend/internal/flow/session/resolver_test.gobackend/internal/flow/session/session_context.gobackend/internal/flow/session/session_context_store.gobackend/internal/flow/session/session_context_store_test.gobackend/internal/flow/session/state.gobackend/internal/flow/session/state_test.gobackend/internal/flow/session/store.gobackend/internal/flow/session/store_test.gobackend/internal/flow/session/transient_test.gobackend/internal/flow/session/transport.gobackend/internal/flow/session/transport_test.gobackend/internal/oauth/oauth2/authz/service.gobackend/internal/oauth/oauth2/constants/constants.gobackend/internal/oauth/oauth2/model/parameter.gobackend/internal/system/config/config.gobackend/internal/system/config/config_test.gobackend/internal/system/i18n/core/defaults.gofrontend/apps/console/src/features/flows/components/resources/steps/execution/Execution.tsxfrontend/apps/console/src/features/flows/components/resources/steps/execution/ExecutionMinimal.tsxfrontend/apps/console/src/features/flows/components/resources/steps/execution/__tests__/ExecutionMinimal.test.tsxfrontend/apps/console/src/features/flows/components/resources/steps/execution/execution-factory/ExecutionFactory.tsxfrontend/apps/console/src/features/flows/components/resources/steps/execution/execution-factory/__tests__/ExecutionFactory.test.tsxfrontend/apps/console/src/features/flows/models/base.tsfrontend/apps/console/src/features/flows/utils/flowToCanvasTransformer.tsfrontend/apps/console/src/features/login-flow/data/executors.json
✅ Files skipped from review due to trivial changes (4)
- backend/internal/flow/session/participant.go
- backend/internal/system/i18n/core/defaults.go
- backend/internal/flow/flowexec/flowVersionLookup_mock_test.go
- backend/internal/oauth/oauth2/constants/constants.go
🚧 Files skipped from review as they are similar to previous changes (51)
- backend/internal/oauth/oauth2/model/parameter.go
- backend/internal/flow/session/errors.go
- backend/cmd/server/deployment.yaml
- frontend/apps/console/src/features/flows/components/resources/steps/execution/execution-factory/ExecutionFactory.tsx
- backend/internal/flow/session/crypto.go
- backend/internal/flow/session/model.go
- frontend/apps/console/src/features/flows/components/resources/steps/execution/ExecutionMinimal.tsx
- backend/internal/system/config/config.go
- frontend/apps/console/src/features/flows/models/base.ts
- backend/internal/flow/session/transient_test.go
- backend/internal/flow/session/state_test.go
- backend/internal/flow/flowexec/model.go
- backend/cmd/server/bootstrap/flows/authentication/auth_flow_sso.json
- backend/internal/system/config/config_test.go
- backend/internal/flow/mgt/graph_builder_sso_test.go
- frontend/apps/console/src/features/flows/components/resources/steps/execution/execution-factory/tests/ExecutionFactory.test.tsx
- backend/internal/flow/session/inputs.go
- backend/internal/flow/session/resolver.go
- backend/internal/flow/common/constants.go
- backend/internal/flow/session/transport_test.go
- frontend/apps/console/src/features/flows/utils/flowToCanvasTransformer.ts
- backend/internal/oauth/oauth2/authz/service.go
- backend/internal/flow/session/state.go
- frontend/apps/console/src/features/flows/components/resources/steps/execution/Execution.tsx
- backend/internal/flow/flowexec/service.go
- backend/dbscripts/runtimedb/postgres.sql
- frontend/apps/console/src/features/flows/components/resources/steps/execution/tests/ExecutionMinimal.test.tsx
- backend/internal/flow/session/participant_store_test.go
- backend/internal/flow/executor/constants.go
- backend/internal/flow/session/transport.go
- frontend/apps/console/src/features/login-flow/data/executors.json
- backend/internal/flow/flowexec/service_sso_test.go
- backend/dbscripts/runtimedb/sqlite.sql
- backend/internal/flow/executor/session_executor.go
- backend/internal/flow/session/session_context_store.go
- backend/internal/flow/session/queries.go
- backend/internal/flow/executor/auth_assert_executor.go
- backend/internal/flow/executor/auth_assert_assurance_test.go
- backend/cmd/server/servicemanager.go
- backend/internal/flow/session/store.go
- backend/internal/flow/executor/register.go
- backend/internal/flow/session/participant_store.go
- backend/internal/flow/session/session_context.go
- backend/internal/flow/executor/sso_check_executor_test.go
- backend/internal/flow/session/resolver_test.go
- backend/internal/flow/executor/error_constants.go
- backend/internal/flow/executor/sso_check_executor.go
- backend/internal/flow/flowexec/engine.go
- backend/internal/flow/executor/session_executor_test.go
- backend/internal/flow/session/store_test.go
- backend/internal/flow/session/session_context_store_test.go
madurangasiriwardena
force-pushed
the
feature/session-poc-2-rebased
branch
3 times, most recently
from
July 7, 2026 05:17
09d3eff to
7efc897
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
backend/internal/flow/executor/session_executor_test.go (2)
167-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSwallowed error in test helper.
authenticatedAuthUser()discards theUnmarshalJSONerror. If the hardcoded JSON literal ever breaks, tests silently get a zero-valueAuthUserinstead of a clear failure at the source.♻️ Proposed fix
-func authenticatedAuthUser() providers.AuthUser { +func authenticatedAuthUser(t *testing.T) providers.AuthUser { + t.Helper() var authUser providers.AuthUser - _ = authUser.UnmarshalJSON([]byte(`{"entityReferenceToken":"tok","attributeToken":"tok"}`)) + require.NoError(t, authUser.UnmarshalJSON([]byte(`{"entityReferenceToken":"tok","attributeToken":"tok"}`))) return authUser }🤖 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/executor/session_executor_test.go` around lines 167 - 171, The test helper authenticatedAuthUser() is swallowing the UnmarshalJSON error and can return a zero-value AuthUser silently. Update authenticatedAuthUser() to handle the error explicitly in the helper itself, so any malformed hardcoded JSON causes a clear test failure instead of being ignored.
199-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest setup silently ignores errors and hardcodes a shared
/tmppath.
config.InitializeServerRuntimeandcore.Initializeerrors are discarded, and all tests share the hardcoded path/tmp/test-session-exec. A setup failure here would surface as a confusing nil-pointer panic deep innewSessionExecutorrather than a clear setup error, and the shared path risks collisions/permission issues across test runs.♻️ Proposed fix
- _ = config.InitializeServerRuntime("/tmp/test-session-exec", &config.Config{}) - flowFactory, _ := core.Initialize(cache.Initialize(config.GetServerRuntime().Config.Cache, "test-deployment")) + require.NoError(t, config.InitializeServerRuntime(t.TempDir(), &config.Config{})) + flowFactory, err := core.Initialize(cache.Initialize(config.GetServerRuntime().Config.Cache, "test-deployment")) + require.NoError(t, err)🤖 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/executor/session_executor_test.go` around lines 199 - 207, In newTestSessionExecutorWithTx, stop discarding the errors from config.InitializeServerRuntime and core.Initialize, and fail the test immediately if either setup step returns an error. Also replace the hardcoded shared /tmp/test-session-exec path with a unique per-test temp directory (for example using the test’s temp helpers) so concurrent runs do not collide. Keep the fix localized to newTestSessionExecutorWithTx and the initialization calls it performs.
🤖 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 132-142: This change introduces user-facing SSO flow behavior,
session timeout configuration, and an SDK-visible cookie/handle contract, so add
or update the relevant docs before merging. Update the flow guide for the new
SSO checkpoint behavior around SSOCheckExecutor, SessionExecutor, and
checkpointRef, add config reference coverage for session.DefaultTimeouts and the
new idle/absolute session deadlines, and document the AdditionalData-based SSO
handle and per-flow cookie transport contract for SDK consumers.
- Around line 139-141: The SSO path in Execute currently calls
resolveActiveFlowVersion on every step, which repeatedly hits
flowProvider.GetFlow and the underlying store. Update the flow execution path in
service.go so the active flow version is resolved once per flow execution and
then reused across subsequent steps, using the existing Execute and
resolveActiveFlowVersion flow/engineCtx handling to cache the result on
engineCtx.SSOFlowVersion instead of recomputing it each time.
---
Nitpick comments:
In `@backend/internal/flow/executor/session_executor_test.go`:
- Around line 167-171: The test helper authenticatedAuthUser() is swallowing the
UnmarshalJSON error and can return a zero-value AuthUser silently. Update
authenticatedAuthUser() to handle the error explicitly in the helper itself, so
any malformed hardcoded JSON causes a clear test failure instead of being
ignored.
- Around line 199-207: In newTestSessionExecutorWithTx, stop discarding the
errors from config.InitializeServerRuntime and core.Initialize, and fail the
test immediately if either setup step returns an error. Also replace the
hardcoded shared /tmp/test-session-exec path with a unique per-test temp
directory (for example using the test’s temp helpers) so concurrent runs do not
collide. Keep the fix localized to newTestSessionExecutorWithTx and the
initialization calls it performs.
🪄 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: 4c8a91f2-2aab-455c-b905-c5a8f7f78d74
⛔ Files ignored due to path filters (1)
backend/tests/mocks/flow/flowexecmock/flowVersionLookup_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (59)
backend/cmd/server/bootstrap/flows/authentication/auth_flow_sso.jsonbackend/cmd/server/deployment.yamlbackend/cmd/server/servicemanager.gobackend/dbscripts/runtimedb/postgres.sqlbackend/dbscripts/runtimedb/sqlite.sqlbackend/internal/flow/common/constants.gobackend/internal/flow/executor/auth_assert_assurance_test.gobackend/internal/flow/executor/auth_assert_executor.gobackend/internal/flow/executor/constants.gobackend/internal/flow/executor/error_constants.gobackend/internal/flow/executor/register.gobackend/internal/flow/executor/session_executor.gobackend/internal/flow/executor/session_executor_test.gobackend/internal/flow/executor/sso_check_executor.gobackend/internal/flow/executor/sso_check_executor_test.gobackend/internal/flow/flowexec/engine.gobackend/internal/flow/flowexec/flowVersionLookup_mock_test.gobackend/internal/flow/flowexec/handler.gobackend/internal/flow/flowexec/handler_test.gobackend/internal/flow/flowexec/init.gobackend/internal/flow/flowexec/model.gobackend/internal/flow/flowexec/service.gobackend/internal/flow/flowexec/service_sso_test.gobackend/internal/flow/mgt/graph_builder_sso_test.gobackend/internal/flow/session/crypto.gobackend/internal/flow/session/errors.gobackend/internal/flow/session/inputs.gobackend/internal/flow/session/model.gobackend/internal/flow/session/participant.gobackend/internal/flow/session/participant_store.gobackend/internal/flow/session/participant_store_test.gobackend/internal/flow/session/queries.gobackend/internal/flow/session/resolver.gobackend/internal/flow/session/resolver_test.gobackend/internal/flow/session/session_context.gobackend/internal/flow/session/session_context_store.gobackend/internal/flow/session/session_context_store_test.gobackend/internal/flow/session/state.gobackend/internal/flow/session/state_test.gobackend/internal/flow/session/store.gobackend/internal/flow/session/store_test.gobackend/internal/flow/session/transient_test.gobackend/internal/flow/session/transport.gobackend/internal/flow/session/transport_test.gobackend/internal/oauth/oauth2/authz/service.gobackend/internal/oauth/oauth2/constants/constants.gobackend/internal/oauth/oauth2/model/parameter.gobackend/internal/system/config/config.gobackend/internal/system/config/config_test.gobackend/internal/system/i18n/core/defaults.gofrontend/apps/console/src/features/flows/components/resources/steps/execution/Execution.tsxfrontend/apps/console/src/features/flows/components/resources/steps/execution/ExecutionMinimal.tsxfrontend/apps/console/src/features/flows/components/resources/steps/execution/__tests__/Execution.test.tsxfrontend/apps/console/src/features/flows/components/resources/steps/execution/__tests__/ExecutionMinimal.test.tsxfrontend/apps/console/src/features/flows/components/resources/steps/execution/execution-factory/ExecutionFactory.tsxfrontend/apps/console/src/features/flows/components/resources/steps/execution/execution-factory/__tests__/ExecutionFactory.test.tsxfrontend/apps/console/src/features/flows/models/base.tsfrontend/apps/console/src/features/flows/utils/flowToCanvasTransformer.tsfrontend/apps/console/src/features/login-flow/data/executors.json
✅ Files skipped from review due to trivial changes (4)
- backend/internal/flow/executor/constants.go
- backend/internal/system/i18n/core/defaults.go
- backend/internal/flow/flowexec/flowVersionLookup_mock_test.go
- backend/internal/oauth/oauth2/constants/constants.go
🚧 Files skipped from review as they are similar to previous changes (50)
- backend/internal/flow/session/errors.go
- backend/internal/flow/session/participant.go
- backend/cmd/server/deployment.yaml
- frontend/apps/console/src/features/flows/utils/flowToCanvasTransformer.ts
- frontend/apps/console/src/features/flows/components/resources/steps/execution/execution-factory/tests/ExecutionFactory.test.tsx
- backend/internal/flow/session/crypto.go
- frontend/apps/console/src/features/flows/models/base.ts
- backend/internal/flow/session/inputs.go
- backend/internal/oauth/oauth2/model/parameter.go
- backend/internal/flow/mgt/graph_builder_sso_test.go
- backend/internal/flow/session/transient_test.go
- backend/internal/flow/flowexec/init.go
- backend/internal/flow/session/state.go
- backend/internal/flow/session/model.go
- backend/internal/flow/session/state_test.go
- backend/internal/flow/session/session_context.go
- backend/internal/flow/executor/sso_check_executor.go
- backend/cmd/server/bootstrap/flows/authentication/auth_flow_sso.json
- backend/internal/system/config/config.go
- backend/internal/flow/session/session_context_store.go
- backend/internal/flow/executor/sso_check_executor_test.go
- backend/internal/flow/executor/auth_assert_assurance_test.go
- backend/internal/system/config/config_test.go
- frontend/apps/console/src/features/flows/components/resources/steps/execution/Execution.tsx
- frontend/apps/console/src/features/flows/components/resources/steps/execution/execution-factory/ExecutionFactory.tsx
- backend/internal/flow/flowexec/handler.go
- backend/internal/flow/executor/auth_assert_executor.go
- frontend/apps/console/src/features/flows/components/resources/steps/execution/ExecutionMinimal.tsx
- backend/internal/oauth/oauth2/authz/service.go
- frontend/apps/console/src/features/login-flow/data/executors.json
- backend/internal/flow/flowexec/handler_test.go
- backend/internal/flow/session/transport_test.go
- backend/internal/flow/session/transport.go
- backend/internal/flow/executor/error_constants.go
- backend/dbscripts/runtimedb/postgres.sql
- backend/internal/flow/session/participant_store.go
- backend/cmd/server/servicemanager.go
- backend/internal/flow/common/constants.go
- backend/internal/flow/executor/session_executor.go
- backend/dbscripts/runtimedb/sqlite.sql
- backend/internal/flow/session/store.go
- backend/internal/flow/flowexec/model.go
- frontend/apps/console/src/features/flows/components/resources/steps/execution/tests/ExecutionMinimal.test.tsx
- backend/internal/flow/session/resolver.go
- backend/internal/flow/executor/register.go
- backend/internal/flow/flowexec/service_sso_test.go
- backend/internal/flow/session/resolver_test.go
- backend/internal/flow/flowexec/engine.go
- frontend/apps/console/src/features/flows/components/resources/steps/execution/tests/Execution.test.tsx
- backend/internal/flow/session/queries.go
Comment on lines
+132
to
+142
| // Resolve the inbound SSO handle for this flow from the request-scoped transport inputs. | ||
| applyInboundSSO(engineCtx, ctx) | ||
| // Resolve the active flow version whenever the flow establishes or consults an SSO session. | ||
| // Both paths need it: the save path (fresh login, which carries no inbound handle) stamps the | ||
| // version onto the new session, and the check path compares against it. Gating this on an | ||
| // inbound handle would save sessions at version 0 and then fail the version check on the next | ||
| // login. Flows that use no SSO session skip the lookup. | ||
| if flowUsesSSOSession(engineCtx.Graph) { | ||
| engineCtx.SSOFlowVersion = s.resolveActiveFlowVersion(ctx, engineCtx, logger) | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major
🔴 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:
- Flow-centric browser SSO behavior: new
SSOCheckExecutor/SessionExecutorflow nodes andcheckpointRefbinding change how authentication flows behave for end users; document indocs/content/guides/(how to configure SSO checkpoints in a flow graph). - Session lifetime configuration: new configurable idle/absolute session deadlines (
session.DefaultTimeouts()) are a new configuration surface; document indocs/content/(config reference). - SDK-impacting change: the minted SSO handle is now surfaced via
AdditionalDataand converted into a per-flow cookie by the transport layer; document this new cookie/handle contract indocs/content/sdks/.
If documentation is already covered elsewhere in this PR (outside this reviewed file set), please disregard.
🤖 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` around lines 132 - 142, This
change introduces user-facing SSO flow behavior, session timeout configuration,
and an SDK-visible cookie/handle contract, so add or update the relevant docs
before merging. Update the flow guide for the new SSO checkpoint behavior around
SSOCheckExecutor, SessionExecutor, and checkpointRef, add config reference
coverage for session.DefaultTimeouts and the new idle/absolute session
deadlines, and document the AdditionalData-based SSO handle and per-flow cookie
transport contract for SDK consumers.
Source: Path instructions
madurangasiriwardena
force-pushed
the
feature/session-poc-2-rebased
branch
3 times, most recently
from
July 7, 2026 13:50
b5e6e76 to
0cd4b3e
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/internal/flow/executor/session_executor_test.go (2)
204-229: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated context-builder logic between
freshCtxandssoLoadCtx.Both helpers construct near-identical
providers.NodeContextvalues. Consider extracting a shared base builder that each customizes, to reduce upkeep whenNodeContextfields change.Also applies to: 465-477
🤖 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/executor/session_executor_test.go` around lines 204 - 229, The test helpers `freshCtx` and `ssoLoadCtx` are duplicating the same `providers.NodeContext` setup, which makes future `NodeContext` changes harder to maintain. Extract the common construction into a shared helper or base builder and let `freshCtx` and `ssoLoadCtx` only override the fields they differ on. Keep the shared logic aligned with the existing `providers.NodeContext` fields like `Context`, `ExecutionID`, `RuntimeData`, `AuthUser`, `ExecutionHistory`, and `Application`.
194-202: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDiscarded setup error could mask confusing failures.
core.Initializeerror is ignored; a setup failure here would surface as a downstream assertion failure (e.g., nilflowFactorypanic) instead of a clear test-setup error.🔧 Proposed fix
require.NoError(t, config.InitializeServerRuntime(t.TempDir(), &config.Config{})) - flowFactory, _ := core.Initialize(cache.Initialize(config.GetServerRuntime().Config.Cache, "test-deployment")) + flowFactory, err := core.Initialize(cache.Initialize(config.GetServerRuntime().Config.Cache, "test-deployment")) + require.NoError(t, err)🤖 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/executor/session_executor_test.go` around lines 194 - 202, The test helper newTestSessionExecutorWithTx is ignoring the error returned by core.Initialize, which can hide setup failures and cause confusing downstream panics. Update the helper to capture and assert the initialization error before calling newSessionExecutor, using the existing require helper so failures in core.Initialize are reported as a clear test-setup error rather than a nil flowFactory issue.
🤖 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/executor/session_executor_test.go`:
- Around line 204-229: The test helpers `freshCtx` and `ssoLoadCtx` are
duplicating the same `providers.NodeContext` setup, which makes future
`NodeContext` changes harder to maintain. Extract the common construction into a
shared helper or base builder and let `freshCtx` and `ssoLoadCtx` only override
the fields they differ on. Keep the shared logic aligned with the existing
`providers.NodeContext` fields like `Context`, `ExecutionID`, `RuntimeData`,
`AuthUser`, `ExecutionHistory`, and `Application`.
- Around line 194-202: The test helper newTestSessionExecutorWithTx is ignoring
the error returned by core.Initialize, which can hide setup failures and cause
confusing downstream panics. Update the helper to capture and assert the
initialization error before calling newSessionExecutor, using the existing
require helper so failures in core.Initialize are reported as a clear test-setup
error rather than a nil flowFactory issue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cf5591df-ff9d-4167-8849-f4d92f66449d
⛔ Files ignored due to path filters (1)
backend/tests/mocks/flow/flowexecmock/flowVersionLookup_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (59)
backend/cmd/server/bootstrap/flows/authentication/auth_flow_sso.jsonbackend/cmd/server/deployment.yamlbackend/cmd/server/servicemanager.gobackend/dbscripts/runtimedb/postgres.sqlbackend/dbscripts/runtimedb/sqlite.sqlbackend/internal/flow/common/constants.gobackend/internal/flow/executor/auth_assert_assurance_test.gobackend/internal/flow/executor/auth_assert_executor.gobackend/internal/flow/executor/constants.gobackend/internal/flow/executor/error_constants.gobackend/internal/flow/executor/register.gobackend/internal/flow/executor/session_executor.gobackend/internal/flow/executor/session_executor_test.gobackend/internal/flow/executor/sso_check_executor.gobackend/internal/flow/executor/sso_check_executor_test.gobackend/internal/flow/flowexec/engine.gobackend/internal/flow/flowexec/flowVersionLookup_mock_test.gobackend/internal/flow/flowexec/handler.gobackend/internal/flow/flowexec/handler_test.gobackend/internal/flow/flowexec/init.gobackend/internal/flow/flowexec/model.gobackend/internal/flow/flowexec/service.gobackend/internal/flow/flowexec/service_sso_test.gobackend/internal/flow/mgt/graph_builder_sso_test.gobackend/internal/flow/session/crypto.gobackend/internal/flow/session/errors.gobackend/internal/flow/session/inputs.gobackend/internal/flow/session/model.gobackend/internal/flow/session/participant.gobackend/internal/flow/session/participant_store.gobackend/internal/flow/session/participant_store_test.gobackend/internal/flow/session/queries.gobackend/internal/flow/session/resolver.gobackend/internal/flow/session/resolver_test.gobackend/internal/flow/session/session_context.gobackend/internal/flow/session/session_context_store.gobackend/internal/flow/session/session_context_store_test.gobackend/internal/flow/session/state.gobackend/internal/flow/session/state_test.gobackend/internal/flow/session/store.gobackend/internal/flow/session/store_test.gobackend/internal/flow/session/transient_test.gobackend/internal/flow/session/transport.gobackend/internal/flow/session/transport_test.gobackend/internal/oauth/oauth2/authz/service.gobackend/internal/oauth/oauth2/constants/constants.gobackend/internal/oauth/oauth2/model/parameter.gobackend/internal/system/config/config.gobackend/internal/system/config/config_test.gobackend/internal/system/i18n/core/defaults.gofrontend/apps/console/src/features/flows/components/resources/steps/execution/Execution.tsxfrontend/apps/console/src/features/flows/components/resources/steps/execution/ExecutionMinimal.tsxfrontend/apps/console/src/features/flows/components/resources/steps/execution/__tests__/Execution.test.tsxfrontend/apps/console/src/features/flows/components/resources/steps/execution/__tests__/ExecutionMinimal.test.tsxfrontend/apps/console/src/features/flows/components/resources/steps/execution/execution-factory/ExecutionFactory.tsxfrontend/apps/console/src/features/flows/components/resources/steps/execution/execution-factory/__tests__/ExecutionFactory.test.tsxfrontend/apps/console/src/features/flows/models/base.tsfrontend/apps/console/src/features/flows/utils/flowToCanvasTransformer.tsfrontend/apps/console/src/features/login-flow/data/executors.json
✅ Files skipped from review due to trivial changes (5)
- backend/internal/oauth/oauth2/model/parameter.go
- backend/internal/system/i18n/core/defaults.go
- frontend/apps/console/src/features/flows/models/base.ts
- backend/internal/flow/flowexec/flowVersionLookup_mock_test.go
- backend/internal/oauth/oauth2/constants/constants.go
🚧 Files skipped from review as they are similar to previous changes (53)
- backend/internal/flow/session/state_test.go
- backend/internal/flow/session/crypto.go
- backend/internal/flow/session/participant.go
- backend/internal/flow/session/errors.go
- backend/internal/flow/session/resolver.go
- backend/internal/flow/mgt/graph_builder_sso_test.go
- backend/cmd/server/deployment.yaml
- backend/internal/flow/session/inputs.go
- backend/cmd/server/bootstrap/flows/authentication/auth_flow_sso.json
- frontend/apps/console/src/features/flows/utils/flowToCanvasTransformer.ts
- backend/internal/flow/executor/constants.go
- frontend/apps/console/src/features/flows/components/resources/steps/execution/execution-factory/ExecutionFactory.tsx
- frontend/apps/console/src/features/flows/components/resources/steps/execution/Execution.tsx
- backend/internal/flow/flowexec/init.go
- backend/internal/flow/session/state.go
- backend/internal/flow/session/transient_test.go
- backend/internal/system/config/config_test.go
- backend/internal/flow/session/model.go
- backend/internal/oauth/oauth2/authz/service.go
- frontend/apps/console/src/features/flows/components/resources/steps/execution/execution-factory/tests/ExecutionFactory.test.tsx
- frontend/apps/console/src/features/flows/components/resources/steps/execution/tests/ExecutionMinimal.test.tsx
- backend/internal/flow/executor/auth_assert_assurance_test.go
- backend/internal/flow/session/transport_test.go
- backend/internal/flow/flowexec/model.go
- backend/internal/flow/flowexec/handler.go
- backend/internal/flow/executor/error_constants.go
- backend/internal/system/config/config.go
- frontend/apps/console/src/features/flows/components/resources/steps/execution/ExecutionMinimal.tsx
- backend/internal/flow/executor/register.go
- backend/internal/flow/session/queries.go
- backend/dbscripts/runtimedb/postgres.sql
- backend/cmd/server/servicemanager.go
- backend/internal/flow/executor/sso_check_executor.go
- backend/dbscripts/runtimedb/sqlite.sql
- frontend/apps/console/src/features/login-flow/data/executors.json
- backend/internal/flow/flowexec/engine.go
- backend/internal/flow/executor/session_executor.go
- backend/internal/flow/session/store.go
- backend/internal/flow/session/resolver_test.go
- backend/internal/flow/session/transport.go
- backend/internal/flow/executor/sso_check_executor_test.go
- backend/internal/flow/flowexec/handler_test.go
- backend/internal/flow/executor/auth_assert_executor.go
- backend/internal/flow/session/participant_store_test.go
- backend/internal/flow/session/session_context.go
- backend/internal/flow/common/constants.go
- backend/internal/flow/flowexec/service.go
- backend/internal/flow/session/session_context_store_test.go
- backend/internal/flow/session/store_test.go
- backend/internal/flow/session/participant_store.go
- frontend/apps/console/src/features/flows/components/resources/steps/execution/tests/Execution.test.tsx
- backend/internal/flow/session/session_context_store.go
- backend/internal/flow/flowexec/service_sso_test.go
madurangasiriwardena
force-pushed
the
feature/session-poc-2-rebased
branch
3 times, most recently
from
July 8, 2026 08:10
3fd8e96 to
7a4c892
Compare
Member
Author
|
@coderabbitai resume |
Contributor
✅ Action performedReviews resumed. |
madurangasiriwardena
force-pushed
the
feature/session-poc-2-rebased
branch
from
July 8, 2026 08:39
7a4c892 to
5398bca
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/internal/flow/session/transport.go (1)
96-104: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMinimize captured cookie data in
Read.
Readstores every inbound cookie (including unrelated app cookies) intoInboundHandle.Cookies, even though onlytid_sso_-prefixed cookies are ever consumed viaHandleFor. Filtering here reduces the blast radius of the "must never be persisted" invariant documented onInboundHandle.♻️ Proposed fix
func (c *cookieTransport) Read(r *http.Request) InboundHandle { cookies := make(map[string]string) for _, ck := range r.Cookies() { - cookies[ck.Name] = ck.Value + if strings.HasPrefix(ck.Name, cookieNamePrefix) { + cookies[ck.Name] = ck.Value + } } return InboundHandle{ Cookies: cookies, } }(requires adding
"strings"to imports)🤖 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/session/transport.go` around lines 96 - 104, The cookieTransport.Read method is capturing all inbound cookies into InboundHandle.Cookies, but only tid_sso_-prefixed cookies are used later by HandleFor. Update Read to filter r.Cookies() so it only stores cookies whose names match the tid_sso_ prefix, and add the needed strings import for the prefix check. Keep the change localized to cookieTransport.Read and preserve the existing InboundHandle shape.
🤖 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/oauth/oauth2/constants/constants.go`:
- Line 65: The new OAuth2 RequestParamMaxAge constant makes max_age a
user-facing authorization request parameter, so update the docs to cover it in
the OAuth2/OIDC API reference and authentication guide. In
docs/content/apis.mdx, document max_age semantics and how it interacts with
verified_claims.verification.time.max_age; in the relevant docs/content/guides/
authentication-flow guide, describe browser SSO behavior across applications,
including checkpoint-based session reuse, per-flow SSO cookies, and
idle/absolute session timeout configuration. Use the existing auth flow and
OAuth2 terminology from oauth2const.RequestParamMaxAge and the session-timeout
behavior described by the PR so the new behavior is discoverable.
---
Nitpick comments:
In `@backend/internal/flow/session/transport.go`:
- Around line 96-104: The cookieTransport.Read method is capturing all inbound
cookies into InboundHandle.Cookies, but only tid_sso_-prefixed cookies are used
later by HandleFor. Update Read to filter r.Cookies() so it only stores cookies
whose names match the tid_sso_ prefix, and add the needed strings import for the
prefix check. Keep the change localized to cookieTransport.Read and preserve the
existing InboundHandle shape.
🪄 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: f5b928ec-2098-465a-b4a7-db9272629b35
⛔ Files ignored due to path filters (1)
backend/tests/mocks/flow/flowexecmock/flowVersionLookup_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (58)
backend/cmd/server/bootstrap/flows/authentication/auth_flow_sso.jsonbackend/cmd/server/deployment.yamlbackend/cmd/server/servicemanager.gobackend/dbscripts/runtimedb/postgres.sqlbackend/dbscripts/runtimedb/sqlite.sqlbackend/internal/flow/common/constants.gobackend/internal/flow/executor/auth_assert_assurance_test.gobackend/internal/flow/executor/auth_assert_executor.gobackend/internal/flow/executor/constants.gobackend/internal/flow/executor/error_constants.gobackend/internal/flow/executor/register.gobackend/internal/flow/executor/session_executor.gobackend/internal/flow/executor/session_executor_test.gobackend/internal/flow/executor/sso_check_executor.gobackend/internal/flow/executor/sso_check_executor_test.gobackend/internal/flow/flowexec/engine.gobackend/internal/flow/flowexec/flowVersionLookup_mock_test.gobackend/internal/flow/flowexec/handler.gobackend/internal/flow/flowexec/handler_test.gobackend/internal/flow/flowexec/init.gobackend/internal/flow/flowexec/model.gobackend/internal/flow/flowexec/service.gobackend/internal/flow/flowexec/service_sso_test.gobackend/internal/flow/mgt/graph_builder_sso_test.gobackend/internal/flow/session/errors.gobackend/internal/flow/session/inputs.gobackend/internal/flow/session/model.gobackend/internal/flow/session/participant.gobackend/internal/flow/session/participant_store.gobackend/internal/flow/session/participant_store_test.gobackend/internal/flow/session/queries.gobackend/internal/flow/session/resolver.gobackend/internal/flow/session/resolver_test.gobackend/internal/flow/session/session_context.gobackend/internal/flow/session/session_context_store.gobackend/internal/flow/session/session_context_store_test.gobackend/internal/flow/session/state.gobackend/internal/flow/session/state_test.gobackend/internal/flow/session/store.gobackend/internal/flow/session/store_test.gobackend/internal/flow/session/transient_test.gobackend/internal/flow/session/transport.gobackend/internal/flow/session/transport_test.gobackend/internal/oauth/oauth2/authz/service.gobackend/internal/oauth/oauth2/constants/constants.gobackend/internal/oauth/oauth2/model/parameter.gobackend/internal/system/config/config.gobackend/internal/system/config/config_test.gobackend/internal/system/i18n/core/defaults.gofrontend/apps/console/src/features/flows/components/resources/steps/execution/Execution.tsxfrontend/apps/console/src/features/flows/components/resources/steps/execution/ExecutionMinimal.tsxfrontend/apps/console/src/features/flows/components/resources/steps/execution/__tests__/Execution.test.tsxfrontend/apps/console/src/features/flows/components/resources/steps/execution/__tests__/ExecutionMinimal.test.tsxfrontend/apps/console/src/features/flows/components/resources/steps/execution/execution-factory/ExecutionFactory.tsxfrontend/apps/console/src/features/flows/components/resources/steps/execution/execution-factory/__tests__/ExecutionFactory.test.tsxfrontend/apps/console/src/features/flows/models/base.tsfrontend/apps/console/src/features/flows/utils/flowToCanvasTransformer.tsfrontend/apps/console/src/features/login-flow/data/executors.json
✅ Files skipped from review due to trivial changes (3)
- backend/internal/flow/session/participant.go
- backend/internal/system/i18n/core/defaults.go
- backend/internal/flow/flowexec/flowVersionLookup_mock_test.go
🚧 Files skipped from review as they are similar to previous changes (51)
- backend/internal/flow/session/resolver.go
- backend/internal/flow/session/transient_test.go
- backend/internal/flow/mgt/graph_builder_sso_test.go
- frontend/apps/console/src/features/flows/components/resources/steps/execution/execution-factory/ExecutionFactory.tsx
- frontend/apps/console/src/features/flows/models/base.ts
- backend/internal/oauth/oauth2/model/parameter.go
- backend/internal/oauth/oauth2/authz/service.go
- backend/internal/flow/session/state_test.go
- frontend/apps/console/src/features/flows/utils/flowToCanvasTransformer.ts
- backend/internal/flow/session/inputs.go
- backend/cmd/server/bootstrap/flows/authentication/auth_flow_sso.json
- frontend/apps/console/src/features/login-flow/data/executors.json
- backend/cmd/server/deployment.yaml
- backend/internal/flow/session/errors.go
- backend/internal/flow/executor/register.go
- backend/internal/flow/session/session_context.go
- frontend/apps/console/src/features/flows/components/resources/steps/execution/tests/ExecutionMinimal.test.tsx
- frontend/apps/console/src/features/flows/components/resources/steps/execution/execution-factory/tests/ExecutionFactory.test.tsx
- backend/internal/system/config/config_test.go
- backend/internal/system/config/config.go
- backend/dbscripts/runtimedb/sqlite.sql
- frontend/apps/console/src/features/flows/components/resources/steps/execution/ExecutionMinimal.tsx
- backend/internal/flow/session/queries.go
- backend/internal/flow/session/model.go
- backend/internal/flow/executor/auth_assert_executor.go
- backend/internal/flow/session/resolver_test.go
- backend/internal/flow/executor/auth_assert_assurance_test.go
- backend/cmd/server/servicemanager.go
- backend/internal/flow/flowexec/service.go
- backend/internal/flow/session/state.go
- backend/internal/flow/flowexec/model.go
- backend/internal/flow/flowexec/handler_test.go
- backend/internal/flow/flowexec/init.go
- frontend/apps/console/src/features/flows/components/resources/steps/execution/tests/Execution.test.tsx
- backend/internal/flow/executor/sso_check_executor.go
- backend/internal/flow/flowexec/handler.go
- backend/internal/flow/executor/constants.go
- backend/internal/flow/executor/error_constants.go
- backend/internal/flow/session/participant_store.go
- backend/internal/flow/session/transport_test.go
- frontend/apps/console/src/features/flows/components/resources/steps/execution/Execution.tsx
- backend/internal/flow/common/constants.go
- backend/dbscripts/runtimedb/postgres.sql
- backend/internal/flow/executor/sso_check_executor_test.go
- backend/internal/flow/flowexec/service_sso_test.go
- backend/internal/flow/flowexec/engine.go
- backend/internal/flow/session/store_test.go
- backend/internal/flow/session/participant_store_test.go
- backend/internal/flow/executor/session_executor_test.go
- backend/internal/flow/executor/session_executor.go
- backend/internal/flow/session/store.go
| RequestParamPrompt string = "prompt" | ||
| RequestParamRequestURI string = "request_uri" | ||
| RequestParamAcrValues string = "acr_values" | ||
| RequestParamMaxAge string = "max_age" |
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:
- New OAuth2
max_ageauthorization request parameter: document the parameter, its semantics, and interaction withverified_claims.verification.time.max_ageindocs/content/apis.mdx(OAuth2/OIDC API reference). - Browser SSO across applications (checkpoint-based session reuse, per-flow SSO cookie, idle/absolute session timeouts): document the new authentication-flow behavior and any new deployment/session-timeout config in
docs/content/guides/(authentication flow guide).
The authorization service reads the max_age request query parameter via oauth2const.RequestParamMaxAge and stores it in maxAge for request processing/validation. confirms this is now a live, user-facing OAuth2 parameter.
🤖 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/oauth/oauth2/constants/constants.go` at line 65, The new
OAuth2 RequestParamMaxAge constant makes max_age a user-facing authorization
request parameter, so update the docs to cover it in the OAuth2/OIDC API
reference and authentication guide. In docs/content/apis.mdx, document max_age
semantics and how it interacts with verified_claims.verification.time.max_age;
in the relevant docs/content/guides/ authentication-flow guide, describe browser
SSO behavior across applications, including checkpoint-based session reuse,
per-flow SSO cookies, and idle/absolute session timeout configuration. Use the
existing auth flow and OAuth2 terminology from oauth2const.RequestParamMaxAge
and the session-timeout behavior described by the PR so the new behavior is
discoverable.
Source: Path instructions
| @@ -0,0 +1,127 @@ | |||
| { | |||
Member
There was a problem hiding this comment.
Shall we add this as a flow builder template rather than introducing a new bootstrap flow?
| CREATE INDEX idx_flow_context_expiry_time ON "FLOW_CONTEXT" (EXPIRY_TIME); | ||
|
|
||
| -- Table to store SSO sessions, grouped by flow (FLOW_ID) and resolved by an opaque handle. | ||
| CREATE TABLE "SSO_SESSION" ( |
Member
There was a problem hiding this comment.
This is persistent runtime data right? Shouldn't this go into operations DB?
Member
There was a problem hiding this comment.
If going with runtime DB, there's some other refactoring happening to move this into a key value pair based table. Other existing tables are refactored progressively, but since this is a new table, we can directly follow this pattern adding data to RUNTIME_STORE.
| "name": "SSOCheckExecutor" | ||
| }, | ||
| "properties": { | ||
| "checkpointRef": "session" |
Member
There was a problem hiding this comment.
This checkpointRef is sort of a checkpoint key within the session entry right?. SSOCheckExecutor lookup for a checkpoint with a key constructed by appending this ref in the session record.
But is this something intuitive for a user to configure? Does this always need to have id of the SessionExecutor? If so same info is already available via the onSuccess edge. I'm thinking whether we can set this explicitly by looking at the onSuccess edge at the graph construction time.
Anyway this is fine for the initial cut.
| jwt: | ||
| preferred_key_id: "default-key" | ||
|
|
||
| session: |
Member
There was a problem hiding this comment.
Shall we move these to default.json and remove from deployment.yaml?
| GithubSvc: githubAuthnService, | ||
| GoogleSvc: googleAuthnService, | ||
| OpenID4VPVerifierSvc: openid4vpSvc, | ||
| SessionStore: flowsession.NewStore( |
Member
There was a problem hiding this comment.
Plugging store implementations is not something we follow currently. But something we thought of having in future.
But is this a requirement currently? If this is not used outside flow package, we can keep it flow internal.
There are use cases which uses flow execution and core, but nothing else. Need to see how they can unplug session implementation via the thunderidengine/ service manager.
cc: @senthalan
| RuntimeKeySSOSessionSaved = "ssoSessionSaved" | ||
| // RuntimeKeyAuthTime holds the Unix timestamp (seconds) at which the subject authenticated | ||
| // for the current session, carried across the SSO path for downstream assurance checks. | ||
| RuntimeKeyAuthTime = "authTime" |
Member
There was a problem hiding this comment.
Suggested change
| RuntimeKeyAuthTime = "authTime" | |
| RuntimeKeyAuthTime = "ssoAuthTime" |
What if we append sso prefix for this too?
madurangasiriwardena
force-pushed
the
feature/session-poc-2-rebased
branch
from
July 10, 2026 09:19
e5891d6 to
9eef538
Compare
madurangasiriwardena
added a commit
to madurangasiriwardena/thunder-id
that referenced
this pull request
Jul 10, 2026
Introduce a session.Service that owns resolve/checkpoint orchestration and transactions, hiding the individual stores behind unexported constructors. Build it in session.Initialize and inject it into the executor tree so the flowexec engine and service carry no SSO initialization logic. Add an engine-only EngineData channel on the executor/node responses so the session handle reaches the engine without leaking to the client. Inject the SSO cookie Secure flag and session timeouts into flowexec through flowconfig.Config instead of reading the server runtime inside the package, and move the SSO session lifetime configuration into the server-config API as a new "session" section. Refs thunder-id#3779
madurangasiriwardena
force-pushed
the
feature/session-poc-2-rebased
branch
from
July 10, 2026 10:14
9eef538 to
be06e32
Compare
| if engineCtx.Graph == nil { | ||
| return 0 | ||
| } | ||
| def, svcErr := s.flowProvider.GetFlow(ctx, engineCtx.Graph.GetID()) |
Member
There was a problem hiding this comment.
Here we'll be fetching the flow again right? Can't we pass this version from previous fetch/ add to ctx and reuse?
| @@ -0,0 +1,127 @@ | |||
| { | |||
Member
There was a problem hiding this comment.
Do we need to keep this here?
| @@ -0,0 +1,100 @@ | |||
| /* | |||
Member
There was a problem hiding this comment.
Shall we merge this to the same store.go file?
| // (re-execution or a concurrent request) overwrites it rather than erroring on the primary key. | ||
| // The ON CONFLICT ... DO UPDATE form is valid in both PostgreSQL and SQLite. | ||
| queryCreateSessionContext = model.DBQuery{ | ||
| ID: "SSO-SESS-AC-01", |
Member
There was a problem hiding this comment.
Not a must change. But we should be able to update all queries to use the same prefix since we have a single store interface now
| // EngineData carries executor output the flow engine consumes internally (for example, a | ||
| // transport signal such as a minted session handle). Unlike AdditionalData, it is never | ||
| // serialized to the client. | ||
| EngineData map[string]string `json:"-"` |
Member
There was a problem hiding this comment.
We already have a forwarded data concept, but that's used to communicate data from current node to the next. If possible let's reuse that for this requirement
| @@ -0,0 +1,141 @@ | |||
| /* | |||
Member
There was a problem hiding this comment.
Should be able to merge to the same store.go file
ThaminduDilshan
previously approved these changes
Jul 10, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to a conflict with the base branch
Jul 10, 2026
Introduce a session.Service that owns resolve/checkpoint orchestration and transactions, hiding the individual stores behind unexported constructors. Build it in session.Initialize and inject it into the executor tree so the flowexec engine and service carry no SSO initialization logic. Add an engine-only EngineData channel on the executor/node responses so the session handle reaches the engine without leaking to the client. Inject the SSO cookie Secure flag and session timeouts into flowexec through flowconfig.Config instead of reading the server runtime inside the package, and move the SSO session lifetime configuration into the server-config API as a new "session" section. Refs thunder-id#3779
madurangasiriwardena
force-pushed
the
feature/session-poc-2-rebased
branch
from
July 10, 2026 12:16
be06e32 to
0eab1a6
Compare
rajithacharith
approved these changes
Jul 10, 2026
rajithacharith
enabled auto-merge
July 10, 2026 12:23
madurangasiriwardena
added a commit
to madurangasiriwardena/thunder-id
that referenced
this pull request
Jul 10, 2026
- Merge participant and session-context stores into store.go (single store impl) - Unify SQL query-constant IDs under one SSO-SESS-NN prefix - Build the graphbuilder SSO test flow inline; drop testdata/sso_flow.json - Reuse the flow version captured at context load instead of re-fetching the flow in resolveActiveFlowVersion Refs thunder-id#3779
This was referenced Jul 10, 2026
12 tasks
This was referenced Jul 16, 2026
This was referenced Jul 23, 2026
12 tasks
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
Introduces flow-centric browser SSO — the ability to reuse an existing browser login session across applications that share the same authentication flow, so a returning user can skip re-authenticating.
SSO is expressed as a property of the authentication flow graph rather than a separate subsystem: the flow author places SSO nodes to decide what an existing session lets a user skip. A session belongs to exactly one flow (keyed by
flow_id) and is referenced by an opaque per-flow handle cookie; two apps SSO with each other iff they are configured with the same flow.This is a first-phase (POC-level) implementation. A few data-handling decisions are deliberately interim, pending a follow-up flow-context data-classification effort (called out under Approach → Deferred).
Design discussion (architecture, storage model, DB operations): #3673
Approach
Flow graph — two node types, used as one or more checkpoint pairs
SSOCheckExecutor, utility node) — resolves whether a live, compatible session already holds a given checkpoint and routes skip vs. authenticate (viaonSuccess/onFailure).SessionExecutor, authentication node) — the join where the SSO and fresh-auth branches converge. On the fresh path it saves the checkpoint's context and mints a handle; on the SSO path it loads the saved context so downstream nodes continue authenticated.checkpointRefnode property; the checkpoint id is that join node's id. All checkpoints of one login share a single session per flow execution, established idempotently onflow_execution_id, so each stage can be skipped independently on reuse. Create-vs-append is decided from the database (by handle or flow execution id), not in-memory ordering, so it holds under multi-request / divergent-branch flows.Storage (runtime DB)
SSO_SESSION— lean row read on every resolve/touch (one row per establishing flow execution; unique onflow_execution_id).SSO_SESSION_CONTEXT— one encrypted row per checkpoint ((session_id, checkpoint_id)), loaded only on the SSO restore path.SSO_SESSION_PARTICIPANT— apps that used the session (basis for future session-wide logout/revocation).Session lifecycle — governed by configurable idle (sliding) and absolute deadlines; the resolver rejects a session past either.
Engine-agnostic integration — the reusable flow engine (
pkg/thunderidengine) stays SSO-unaware. SSO inputs ride on the requestcontext.Context; the minted handle is returned on the engine's genericAdditionalDatachannel and the product transport layer turns it into the per-flow cookie. No SSO-specific field is added to the engine contract.Deferred to a follow-up (flow-context data classification)
AuthUseris currently snapshotted as-is (materialized attributes), andRuntimeDatais persisted in full apart from a deny-list of transient SSO control keys and request-scoped keys (requested_permissions,required_*_attributes,required_locales,clientId,authorizationRequestId,applicationId). A later phase will introduce a proper durability classification and attribute minimization/re-resolution. Tracked inSSO_TODO.md.Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit
New Features
max_ageand authentication assurance checks, including step-up authentication when requirements are unmet.UI Improvements