diff --git a/backend/internal/inboundclient/service.go b/backend/internal/inboundclient/service.go index 11925a278e..e4fc57cbd7 100644 --- a/backend/internal/inboundclient/service.go +++ b/backend/internal/inboundclient/service.go @@ -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 { return nil } @@ -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) @@ -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 diff --git a/backend/internal/inboundclient/service_test.go b/backend/internal/inboundclient/service_test.go index 81275906ac..acbe4ca2d2 100644 --- a/backend/internal/inboundclient/service_test.go +++ b/backend/internal/inboundclient/service_test.go @@ -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} @@ -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 { @@ -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) }) @@ -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) @@ -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) } diff --git a/tests/integration/application/application_api_test.go b/tests/integration/application/application_api_test.go index 4ded4c2c14..11ed68054a 100644 --- a/tests/integration/application/application_api_test.go +++ b/tests/integration/application/application_api_test.go @@ -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", @@ -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) @@ -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 diff --git a/tests/integration/application/flow_reference_validation_test.go b/tests/integration/application/flow_reference_validation_test.go index 739bf0c46c..d8af926d77 100644 --- a/tests/integration/application/flow_reference_validation_test.go +++ b/tests/integration/application/flow_reference_validation_test.go @@ -26,9 +26,10 @@ const ( // FlowReferenceValidationTestSuite exercises the app-side cross-flow reference behavior: // on app create/update, if the app's AuthFlow (or another starting flow) transitively invokes a -// REGISTRATION / RECOVERY flow via a CALL node, the app either has to declare the matching binding -// or leave it unset (in which case the binding is auto-filled in a disabled state). Genuine -// mismatches still reject. +// REGISTRATION / RECOVERY flow via a CALL node, the app must either declare a matching binding +// (with the corresponding enable flag on) or leave it disabled — in the disabled case the server +// persists an empty binding regardless of what the auth flow calls. Sign-out still auto-fills +// because it has no enable toggle. Genuine mismatches with an enabled binding still reject. type FlowReferenceValidationTestSuite struct { suite.Suite ouID string @@ -194,26 +195,23 @@ func (suite *FlowReferenceValidationTestSuite) TestCreateApp_MatchingRegistratio suite.createdApps = append(suite.createdApps, appID) } -func (suite *FlowReferenceValidationTestSuite) TestCreateApp_MissingRegistrationBindingAutoFilled() { - app := suite.baseApp("flowref_autofill_reg") +func (suite *FlowReferenceValidationTestSuite) TestCreateApp_DisabledRegistrationLeftEmpty() { + // Auth flow calls a REGISTRATION target, but the caller has left IsRegistrationFlowEnabled + // unset (false). The server must persist an empty registration binding. + app := suite.baseApp("flowref_disabled_reg") app.AuthFlowID = suite.authFlowID - // RegistrationFlowID intentionally omitted — the auth flow calls a REGISTRATION target, so the - // server must auto-fill RegistrationFlowID with the reg-callee ID and force - // IsRegistrationFlowEnabled to false. - app.IsRegistrationFlowEnabled = true appID, err := testutils.CreateApplication(app) suite.Require().NoError(err) suite.createdApps = append(suite.createdApps, appID) persisted := suite.getApp(appID) - suite.Equal(suite.regCalleeID, persisted["registrationFlowId"], - "auto-fill must populate registrationFlowId from the reachable target") - suite.Equal(false, persisted["isRegistrationFlowEnabled"], - "auto-fill must force isRegistrationFlowEnabled to false regardless of the caller's value") + suite.Empty(persisted["registrationFlowId"], + "disabled registration binding must not be auto-filled from the auth flow's reachable target") + suite.Equal(false, persisted["isRegistrationFlowEnabled"]) } -func (suite *FlowReferenceValidationTestSuite) TestCreateApp_MissingRecoveryBindingAutoFilled() { +func (suite *FlowReferenceValidationTestSuite) TestCreateApp_DisabledRecoveryLeftEmpty() { // Build a fresh auth flow that calls the RECOVERY callee (rather than reusing the shared // authFlow which calls the registration callee). authCallingRec := flowDefinition{ @@ -241,16 +239,16 @@ func (suite *FlowReferenceValidationTestSuite) TestCreateApp_MissingRecoveryBind authCallingRecID := suite.createFlowReturningID(authCallingRec) suite.extraFlows = append(suite.extraFlows, authCallingRecID) - app := suite.baseApp("flowref_autofill_rec") + app := suite.baseApp("flowref_disabled_rec") app.AuthFlowID = authCallingRecID - app.IsRecoveryFlowEnabled = true appID, err := testutils.CreateApplication(app) suite.Require().NoError(err) suite.createdApps = append(suite.createdApps, appID) persisted := suite.getApp(appID) - suite.Equal(suite.recCalleeID, persisted["recoveryFlowId"]) + suite.Empty(persisted["recoveryFlowId"], + "disabled recovery binding must not be auto-filled from the auth flow's reachable target") suite.Equal(false, persisted["isRecoveryFlowEnabled"]) } @@ -282,6 +280,7 @@ func (suite *FlowReferenceValidationTestSuite) TestCreateApp_MismatchedRegistrat app := suite.baseApp("flowref_mismatch_reg") app.AuthFlowID = suite.authFlowID app.RegistrationFlowID = altRegID // differs from the reg-callee the auth flow calls + app.IsRegistrationFlowEnabled = true suite.createApplicationExpectFlowMismatch(app) } @@ -340,6 +339,7 @@ func (suite *FlowReferenceValidationTestSuite) TestCreateApp_ReverseAuthReferenc app := suite.baseApp("flowref_reverse_auth") app.AuthFlowID = loneAuthID app.RegistrationFlowID = regCallingAuthID + app.IsRegistrationFlowEnabled = true suite.createApplicationExpectFlowMismatch(app) } @@ -446,12 +446,10 @@ func (suite *FlowReferenceValidationTestSuite) TestUpdateApp_IntroducingMismatch }) } -func (suite *FlowReferenceValidationTestSuite) TestUpdateApp_MissingRegistrationBindingAutoFilled() { - // Onboard-like path via update: create the app with only AuthFlowID set (which triggers - // auto-fill at create time already). Then clear the field via an update payload that omits - // registrationFlowId, and verify the update path also reconciles it. To make this test - // meaningful, create an app with an auth flow that has NO reachable registration flow first, - // then update the app's AuthFlowID to one that does — the update must auto-fill. +func (suite *FlowReferenceValidationTestSuite) TestUpdateApp_DisabledRegistrationStaysEmptyOnAuthFlowSwitch() { + // Create an app with a "quiet" auth flow that has no CALL nodes, then switch its AuthFlowID + // to one that transitively references a REGISTRATION target. The caller has left the + // registration binding disabled, so the update must NOT auto-fill it. quietAuth := flowDefinition{ Name: "FlowRef Quiet Authentication", Handle: "flowref-quiet-auth", @@ -470,27 +468,25 @@ func (suite *FlowReferenceValidationTestSuite) TestUpdateApp_MissingRegistration quietAuthID := suite.createFlowReturningID(quietAuth) suite.extraFlows = append(suite.extraFlows, quietAuthID) - app := suite.baseApp("flowref_update_autofill") + app := suite.baseApp("flowref_update_no_autofill") app.AuthFlowID = quietAuthID - app.IsRegistrationFlowEnabled = true appID, err := testutils.CreateApplication(app) suite.Require().NoError(err) suite.createdApps = append(suite.createdApps, appID) - // Sanity: nothing was auto-filled on create since quietAuth has no calls. initial := suite.getApp(appID) suite.Empty(initial["registrationFlowId"]) - // Switch AuthFlowID to the caller that invokes the registration callee — update must auto-fill. + // Switch AuthFlowID to the caller that invokes the registration callee. The registration + // binding must remain empty since the caller kept isRegistrationFlowEnabled=false. suite.updateApplicationExpectSuccess(appID, map[string]interface{}{ "authFlowId": suite.authFlowID, }) updated := suite.getApp(appID) - suite.Equal(suite.regCalleeID, updated["registrationFlowId"], - "update must auto-fill registrationFlowId from the reachable target") - suite.Equal(false, updated["isRegistrationFlowEnabled"], - "update auto-fill must force isRegistrationFlowEnabled to false") + suite.Empty(updated["registrationFlowId"], + "disabled registration binding must remain empty after an auth-flow switch") + suite.Equal(false, updated["isRegistrationFlowEnabled"]) } // ----- helpers -----