Skip to content

Remove app's registration/ recovery flow when it's disabled #4840

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ThaminduDilshan merged 1 commit into from
Aug 11, 2026
Merged

Remove app's registration/ recovery flow when it's disabled #4840

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 40 additions & 27 deletions backend/internal/inboundclient/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -604,7 +604,21 @@ func BuildOAuthClient(
// server-level default configured; registration/recovery/signout server defaults are intentionally
// left empty to prevent CALL-node mismatches across independently configured flows).
func (s *inboundClientService) resolveFlowDefaults(ctx context.Context, c *inboundmodel.InboundClient) error {
if s.flowMgt == nil || c == nil {
if c == nil {
return nil
}

// Drop registration/recovery bindings whose enable flag is false before any resolution so we
// never persist an ID that contradicts the toggle. This must run regardless of whether flowMgt
// is wired — persistence should still respect the caller's disabled intent.
if !c.IsRegistrationFlowEnabled {
c.RegistrationFlowID = ""
}
if !c.IsRecoveryFlowEnabled {
c.RecoveryFlowID = ""
}

if s.flowMgt == nil {
Comment on lines +611 to +621

@coderabbitai coderabbitai Bot Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

🔴 Documentation Required
This PR introduces user-facing changes that are not covered by documentation updates under docs/.
Please update the relevant documentation before merging.

Missing documentation:

  • backend/internal/inboundclient/service.go#L611-L621: Document that disabled registration and recovery bindings clear persisted flow IDs in docs/content/apis.mdx.
  • backend/internal/inboundclient/service.go#L657-L677: Document that enabled registration and recovery flows are disabled when no effective flow resolves in docs/content/apis.mdx.
  • backend/internal/inboundclient/service.go#L1887-L1894: Document that reconciliation only auto-fills sign-out bindings in docs/content/apis.mdx.

As per path instructions: “If ANY of the above are detected … post a single consolidated PR-level comment.”

📍 Affects 1 file
  • backend/internal/inboundclient/service.go#L611-L621 (this comment)
  • backend/internal/inboundclient/service.go#L657-L677
  • backend/internal/inboundclient/service.go#L1887-L1894
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/internal/inboundclient/service.go` around lines 611 - 621, Update
docs/content/apis.mdx to document the behavior at
backend/internal/inboundclient/service.go:611-621, where disabled registration
or recovery bindings clear persisted flow IDs; at 657-677, where enabled flows
are disabled if no effective flow resolves; and at 1887-1894, where
reconciliation only auto-fills sign-out bindings. Cover all three service.go
sites in the relevant API documentation.

Source: Path instructions

return nil
}

Expand Down Expand Up @@ -640,22 +654,27 @@ func (s *inboundClientService) resolveFlowDefaults(ctx context.Context, c *inbou
}
c.AuthFlowID = authID

regID, err := resolve(c.RegistrationFlowID, providers.FlowTypeRegistration)
if err != nil {
return err
}
c.RegistrationFlowID = regID
if c.RegistrationFlowID == "" {
c.IsRegistrationFlowEnabled = false
}

recID, err := resolve(c.RecoveryFlowID, providers.FlowTypeRecovery)
if err != nil {
return err
// Try to resolve the registration and recovery flows if they are enabled.
// If the resolved ID is empty, disable the flow.
if c.IsRegistrationFlowEnabled {
regID, err := resolve(c.RegistrationFlowID, providers.FlowTypeRegistration)
if err != nil {
return err
}
c.RegistrationFlowID = regID
if c.RegistrationFlowID == "" {
c.IsRegistrationFlowEnabled = false
}
}
c.RecoveryFlowID = recID
if c.RecoveryFlowID == "" {
c.IsRecoveryFlowEnabled = false
if c.IsRecoveryFlowEnabled {
recID, err := resolve(c.RecoveryFlowID, providers.FlowTypeRecovery)
if err != nil {
return err
}
c.RecoveryFlowID = recID
if c.RecoveryFlowID == "" {
c.IsRecoveryFlowEnabled = false
}
}

signOutID, err := resolve(c.SignOutFlowID, providers.FlowTypeSignOut)
Expand Down Expand Up @@ -1865,20 +1884,14 @@ func (s *inboundClientService) walkReferencedFlows(

if expected == "" {
// The inbound client has no binding for this type. On the reconcile path (create/update),
// we auto-fill the reg/recovery/signout binding with the reachable target and force
// the enable flag to false. On the validate-only path (flow update revalidation)
// we simply accept — no mutation, no rejection.
// we auto-fill the sign-out binding with the reachable target so the FK graph stays
// complete. Registration and recovery are never auto-filled: if the caller left the
// flag disabled, we must not persist a binding that will surface later as a phantom
// configuration and clash with a future auth-flow change.
if !reconcile {
continue
}
switch t.FlowType {
case providers.FlowTypeRegistration:
c.RegistrationFlowID = t.FlowID
c.IsRegistrationFlowEnabled = false
case providers.FlowTypeRecovery:
c.RecoveryFlowID = t.FlowID
c.IsRecoveryFlowEnabled = false
case providers.FlowTypeSignOut:
if t.FlowType == providers.FlowTypeSignOut {
c.SignOutFlowID = t.FlowID
}
continue
Expand Down
102 changes: 81 additions & 21 deletions backend/internal/inboundclient/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2025,16 +2025,13 @@ func (suite *InboundClientServiceTestSuite) TestResolveFlowDefaults_ResolvesAllF
assert.Equal(suite.T(), "so-1", c.SignOutFlowID)
}

// Registration/recovery/signout resolve to empty when no explicit or OU override exists
// (their server-default handles are intentionally unconfigured).
// SignOut resolves to empty when no explicit or OU override exists (its server-default handle
// is intentionally unconfigured). Registration/recovery are skipped entirely because their
// enable flags are false — the caller has not asked for those bindings.
func (suite *InboundClientServiceTestSuite) TestResolveFlowDefaults_NonAuthFlowsEmptyWhenNoOverride() {
flowMgt := flowmgtmock.NewFlowMgtServiceInterfaceMock(suite.T())
flowMgt.EXPECT().ResolveEffectiveFlowID(
mock.Anything, "auth-1", "", providers.FlowTypeAuthentication).Return("auth-1", nil).Once()
flowMgt.EXPECT().ResolveEffectiveFlowID(
mock.Anything, "", "", providers.FlowTypeRegistration).Return("", nil).Once()
flowMgt.EXPECT().ResolveEffectiveFlowID(
mock.Anything, "", "", providers.FlowTypeRecovery).Return("", nil).Once()
flowMgt.EXPECT().ResolveEffectiveFlowID(
mock.Anything, "", "", providers.FlowTypeSignOut).Return("", nil).Once()
svc := &inboundClientService{flowMgt: flowMgt}
Expand All @@ -2048,6 +2045,31 @@ func (suite *InboundClientServiceTestSuite) TestResolveFlowDefaults_NonAuthFlows
assert.Empty(suite.T(), c.SignOutFlowID)
}

// When the enable flag is false, an incoming registration/recovery flow ID is cleared and no
// default resolution runs.
func (suite *InboundClientServiceTestSuite) TestResolveFlowDefaults_DisabledFlagClearsFlowIDs() {
flowMgt := flowmgtmock.NewFlowMgtServiceInterfaceMock(suite.T())
flowMgt.EXPECT().ResolveEffectiveFlowID(
mock.Anything, "auth-1", "", providers.FlowTypeAuthentication).Return("auth-1", nil).Once()
flowMgt.EXPECT().ResolveEffectiveFlowID(
mock.Anything, "", "", providers.FlowTypeSignOut).Return("", nil).Once()
svc := &inboundClientService{flowMgt: flowMgt}
c := &inboundmodel.InboundClient{
ID: "p1",
AuthFlowID: "auth-1",
RegistrationFlowID: "reg-1",
IsRegistrationFlowEnabled: false,
RecoveryFlowID: "rec-1",
IsRecoveryFlowEnabled: false,
}
err := svc.resolveFlowDefaults(context.Background(), c)
assert.NoError(suite.T(), err)
assert.Empty(suite.T(), c.RegistrationFlowID)
assert.False(suite.T(), c.IsRegistrationFlowEnabled)
assert.Empty(suite.T(), c.RecoveryFlowID)
assert.False(suite.T(), c.IsRecoveryFlowEnabled)
}

// ResolveEffectiveFlowID errors are mapped to the correct sentinel errors for each flow type.
func (suite *InboundClientServiceTestSuite) TestResolveFlowDefaults_ResolveErrors() {
tests := []struct {
Expand Down Expand Up @@ -2075,18 +2097,27 @@ func (suite *InboundClientServiceTestSuite) TestResolveFlowDefaults_ResolveError
flowMgt.EXPECT().ResolveEffectiveFlowID(
mock.Anything, mock.Anything, "", providers.FlowTypeAuthentication).Return("auth-1", nil).Once()
}
if tt.flowType == providers.FlowTypeRecovery || tt.flowType == providers.FlowTypeSignOut {
if tt.flowType == providers.FlowTypeSignOut {
// The IsRegistrationFlowEnabled / IsRecoveryFlowEnabled flags are true in the
// test client below, so both reg and recovery resolves precede the sign-out call.
flowMgt.EXPECT().ResolveEffectiveFlowID(
mock.Anything, mock.Anything, "", providers.FlowTypeRegistration).Return("", nil).Once()
}
if tt.flowType == providers.FlowTypeSignOut {
flowMgt.EXPECT().ResolveEffectiveFlowID(
mock.Anything, mock.Anything, "", providers.FlowTypeRecovery).Return("", nil).Once()
}
if tt.flowType == providers.FlowTypeRecovery {
flowMgt.EXPECT().ResolveEffectiveFlowID(
mock.Anything, mock.Anything, "", providers.FlowTypeRegistration).Return("", nil).Once()
}
flowMgt.EXPECT().ResolveEffectiveFlowID(
mock.Anything, mock.Anything, "", tt.flowType).Return("", tt.resolveErr).Once()
svc := &inboundClientService{flowMgt: flowMgt}
c := &inboundmodel.InboundClient{ID: "p1", AuthFlowID: "auth-1"}
c := &inboundmodel.InboundClient{
ID: "p1",
AuthFlowID: "auth-1",
IsRegistrationFlowEnabled: true,
IsRecoveryFlowEnabled: true,
}
err := svc.resolveFlowDefaults(context.Background(), c)
assert.ErrorIs(suite.T(), err, tt.expectedErr)
})
Expand Down Expand Up @@ -2179,6 +2210,39 @@ func (suite *InboundClientServiceTestSuite) TestResolveInboundAuthProfileHandles
flowMgt.AssertNotCalled(suite.T(), "GetFlowByHandle", mock.Anything, mock.Anything, mock.Anything)
}

func (suite *InboundClientServiceTestSuite) TestCreateInboundClient_DisabledRegistrationClearsID() {
store := newInboundClientStoreInterfaceMock(suite.T())
store.EXPECT().IsDeclarative(mock.Anything, "p1").Return(false)
store.EXPECT().CreateInboundClient(mock.Anything, mock.MatchedBy(func(c inboundmodel.InboundClient) bool {
return c.RegistrationFlowID == "" && !c.IsRegistrationFlowEnabled
})).Return(nil)

svc := newServiceForTest(store)
client := ptrInboundClient()
client.RegistrationFlowID = "reg-stale"
client.IsRegistrationFlowEnabled = false
err := svc.CreateInboundClient(context.Background(), client, nil, false)

assert.NoError(suite.T(), err)
}

func (suite *InboundClientServiceTestSuite) TestUpdateInboundClient_DisabledRecoveryClearsID() {
store := newInboundClientStoreInterfaceMock(suite.T())
store.EXPECT().IsDeclarative(mock.Anything, "p1").Return(false)
store.EXPECT().UpdateInboundClient(mock.Anything, mock.MatchedBy(func(c inboundmodel.InboundClient) bool {
return c.RecoveryFlowID == "" && !c.IsRecoveryFlowEnabled
})).Return(nil)
store.EXPECT().GetOAuthProfileByEntityID(mock.Anything, "p1").Return(nil, ErrInboundClientNotFound)

svc := newInboundClientService(store, transaction.NewNoOpTransactioner(), nil, nil, nil, nil, nil, nil, nil, nil)
client := ptrInboundClient()
client.RecoveryFlowID = "rec-stale"
client.IsRecoveryFlowEnabled = false
err := svc.UpdateInboundClient(context.Background(), client, nil, false, "")

assert.NoError(suite.T(), err)
}

func (suite *InboundClientServiceTestSuite) TestCreateInboundClient_WithoutRecoveryFlow() {
store := newInboundClientStoreInterfaceMock(suite.T())
store.EXPECT().IsDeclarative(mock.Anything, "p1").Return(false)
Expand Down Expand Up @@ -3101,29 +3165,25 @@ func (suite *InboundClientServiceTestSuite) TestReconcileReferencedFlows_NilFlow
assert.NoError(suite.T(), svc.reconcileReferencedFlows(context.Background(), c))
}

func (suite *InboundClientServiceTestSuite) TestReconcileReferencedFlows_AutoFillsMissingRegistration() {
func (suite *InboundClientServiceTestSuite) TestReconcileReferencedFlows_DoesNotAutoFillMissingRegistration() {
flowMgt := flowmgtmock.NewFlowMgtServiceInterfaceMock(suite.T())
flowMgt.EXPECT().GetReachableCallTargets(mock.Anything, "auth").Return(
[]flowmgt.CallTarget{{FlowID: "reg-b", FlowType: providers.FlowTypeRegistration}}, nil)
svc := &inboundClientService{flowMgt: flowMgt}
c := &inboundmodel.InboundClient{
AuthFlowID: "auth",
IsRegistrationFlowEnabled: true,
}
c := &inboundmodel.InboundClient{AuthFlowID: "auth"}
suite.Require().NoError(svc.reconcileReferencedFlows(context.Background(), c))
assert.Equal(suite.T(), "reg-b", c.RegistrationFlowID)
assert.False(suite.T(), c.IsRegistrationFlowEnabled,
"auto-fill must force the enable flag to false regardless of its previous value")
assert.Empty(suite.T(), c.RegistrationFlowID, "reconcile must not auto-fill RegistrationFlowID")
assert.False(suite.T(), c.IsRegistrationFlowEnabled)
}

func (suite *InboundClientServiceTestSuite) TestReconcileReferencedFlows_AutoFillsMissingRecovery() {
func (suite *InboundClientServiceTestSuite) TestReconcileReferencedFlows_DoesNotAutoFillMissingRecovery() {
flowMgt := flowmgtmock.NewFlowMgtServiceInterfaceMock(suite.T())
flowMgt.EXPECT().GetReachableCallTargets(mock.Anything, "auth").Return(
[]flowmgt.CallTarget{{FlowID: "rec-b", FlowType: providers.FlowTypeRecovery}}, nil)
svc := &inboundClientService{flowMgt: flowMgt}
c := &inboundmodel.InboundClient{AuthFlowID: "auth", IsRecoveryFlowEnabled: true}
c := &inboundmodel.InboundClient{AuthFlowID: "auth"}
suite.Require().NoError(svc.reconcileReferencedFlows(context.Background(), c))
assert.Equal(suite.T(), "rec-b", c.RecoveryFlowID)
assert.Empty(suite.T(), c.RecoveryFlowID, "reconcile must not auto-fill RecoveryFlowID")
assert.False(suite.T(), c.IsRecoveryFlowEnabled)
}

Expand Down
26 changes: 12 additions & 14 deletions tests/integration/application/application_api_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ var (
appToUpdate = Application{
Name: "Updated App",
Description: "Updated Description",
IsRegistrationFlowEnabled: false,
IsRegistrationFlowEnabled: true,
Template: "mobile",
URL: "https://appToUpdate.example.com",
LogoURL: "https://appToUpdate.example.com/logo.png",
Expand Down Expand Up @@ -3633,17 +3633,16 @@ func (ts *ApplicationAPITestSuite) TestApplicationCreateWithDefaultAuthFlowID()
}

// TestApplicationCreateWithoutRegistrationFlowID tests creating an application without a
// RegistrationFlowID when its AuthFlowID transitively references a registration flow. The server
// must auto-fill RegistrationFlowID with the reachable target and force IsRegistrationFlowEnabled
// to false.
// RegistrationFlowID when the caller left IsRegistrationFlowEnabled=false. The server must
// persist the disabled binding (empty ID) even if the AuthFlowID transitively references a
// registration flow.
func (ts *ApplicationAPITestSuite) TestApplicationCreateWithoutRegistrationFlowID() {
app := Application{
OUID: testOUID,
Name: "No Registration Flow Test",
Description: "Test that registration flow is auto-filled from the referenced auth flow",
IsRegistrationFlowEnabled: true,
AuthFlowID: defaultAuthFlowID,
Certificate: nil,
OUID: testOUID,
Name: "No Registration Flow Test",
Description: "Test that a disabled registration binding is not auto-filled from the auth flow",
AuthFlowID: defaultAuthFlowID,
Certificate: nil,
}

appID, err := createApplication(app)
Expand All @@ -3653,10 +3652,9 @@ func (ts *ApplicationAPITestSuite) TestApplicationCreateWithoutRegistrationFlowI
retrievedApp, err := getApplicationByID(appID)
ts.Require().NoError(err)

ts.Assert().Equal(defaultRegistrationFlowID, retrievedApp.RegistrationFlowID,
"auto-fill must populate RegistrationFlowID from the auth flow's reachable target")
ts.Assert().False(retrievedApp.IsRegistrationFlowEnabled,
"auto-fill must force IsRegistrationFlowEnabled to false")
ts.Assert().Empty(retrievedApp.RegistrationFlowID,
"disabled registration binding must not be auto-filled from the auth flow's reachable target")
ts.Assert().False(retrievedApp.IsRegistrationFlowEnabled)
}

// TestApplicationCreateWithDuplicateClientID tests creating application with duplicate client ID
Expand Down
Loading
Loading