fix: account settings broken on OIDC removal (#3185)

Closes ory-corp/cloud#3514

selfservice/strategy/oidc/.snapshots/TestSettingsStrategy-suite=unlink-case=should_not_be_able_to_unlink_an_non-existing_connection-flow=fetch.json

@@ -99,7 +99,7 @@
   {
     "attributes": {
       "disabled": false,
-      "name": "link",
+      "name": "unlink",
       "node_type": "input",
       "type": "submit",
       "value": "github"
@@ -111,8 +111,8 @@
         "context": {
           "provider": "github"
         },
-        "id": 1050002,
-        "text": "Link github",
+        "id": 1050003,
+        "text": "Unlink github",
         "type": "info"
       }
     },
@@ -139,5 +139,27 @@
       }
     },
     "type": "input"
+  },
+  {
+    "attributes": {
+      "disabled": false,
+      "name": "unlink",
+      "node_type": "input",
+      "type": "submit",
+      "value": "ory"
+    },
+    "group": "oidc",
+    "messages": [],
+    "meta": {
+      "label": {
+        "context": {
+          "provider": "ory"
+        },
+        "id": 1050003,
+        "text": "Unlink ory",
+        "type": "info"
+      }
+    },
+    "type": "input"
   }
 ]

selfservice/strategy/oidc/.snapshots/TestSettingsStrategy-suite=unlink-case=should_not_be_able_to_unlink_an_non-existing_connection-flow=json.json

@@ -100,7 +100,7 @@
     "type": "input",
     "group": "oidc",
     "attributes": {
-      "name": "link",
+      "name": "unlink",
       "type": "submit",
       "value": "github",
       "disabled": false,
@@ -109,8 +109,8 @@
     "messages": [],
     "meta": {
       "label": {
-        "id": 1050002,
-        "text": "Link github",
+        "id": 1050003,
+        "text": "Unlink github",
         "type": "info",
         "context": {
           "provider": "github"
@@ -139,5 +139,27 @@
         }
       }
     }
+  },
+  {
+    "type": "input",
+    "group": "oidc",
+    "attributes": {
+      "name": "unlink",
+      "type": "submit",
+      "value": "ory",
+      "disabled": false,
+      "node_type": "input"
+    },
+    "messages": [],
+    "meta": {
+      "label": {
+        "id": 1050003,
+        "text": "Unlink ory",
+        "type": "info",
+        "context": {
+          "provider": "ory"
+        }
+      }
+    }
   }
 ]

selfservice/strategy/oidc/strategy_settings.go

@@ -41,6 +41,8 @@ var UnknownConnectionValidationError = &jsonschema.ValidationError{
 	Message: "can not unlink non-existing OpenID Connect connection", InstancePtr: "#/"}
 var ConnectionExistValidationError = &jsonschema.ValidationError{
 	Message: "can not link unknown or already existing OpenID Connect connection", InstancePtr: "#/"}
+var UnlinkAllFirstFactorConnectionsError = &jsonschema.ValidationError{
+	Message: "can not unlink OpenID Connect connection because it is the last remaining first factor credential", InstancePtr: "#/"}
 
 func (s *Strategy) RegisterSettingsRoutes(router *x.RouterPublic) {}
 
@@ -87,21 +89,12 @@ func (s *Strategy) linkedProviders(ctx context.Context, r *http.Request, conf *C
 		return nil, errors.WithStack(err)
 	}
 
-	count, err := s.d.IdentityManager().CountActiveFirstFactorCredentials(ctx, confidential)
-	if err != nil {
-		return nil, err
-	}
-
-	if count < 2 {
-		// This means that we're able to remove a connection because it is the last configured credential. If it is
-		// removed, the identity is no longer able to sign in.
-		return nil, nil
-	}
-
 	var result []Provider
 	for _, p := range available.Providers {
 		prov, err := conf.Provider(p.Provider, s.d)
-		if err != nil {
+		if errors.Is(err, herodot.ErrNotFound) {
+			continue
+		} else if err != nil {
 			return nil, err
 		}
 		result = append(result, prov)
@@ -172,8 +165,17 @@ func (s *Strategy) PopulateSettingsMethod(r *http.Request, id *identity.Identity
 		sr.UI.GetNodes().Append(NewLinkNode(l.Config().ID))
 	}
 
-	for _, l := range linked {
-		sr.UI.GetNodes().Append(NewUnlinkNode(l.Config().ID))
+	count, err := s.d.IdentityManager().CountActiveFirstFactorCredentials(r.Context(), confidential)
+	if err != nil {
+		return err
+	}
+
+	if count > 1 {
+		// This means that we're able to remove a connection because it is the last configured credential. If it is
+		// removed, the identity is no longer able to sign in.
+		for _, l := range linked {
+			sr.UI.GetNodes().Append(NewUnlinkNode(l.Config().ID))
+		}
 	}
 
 	return nil
@@ -466,7 +468,16 @@ func (s *Strategy) unlinkProvider(w http.ResponseWriter, r *http.Request, ctxUpd
 	var cc identity.CredentialsOIDC
 	creds, err := i.ParseCredentials(s.ID(), &cc)
 	if err != nil {
-		return s.handleSettingsError(w, r, ctxUpdate, p, errors.WithStack(UnknownConnectionValidationError))
+		return s.handleSettingsError(w, r, ctxUpdate, p, err)
+	}
+
+	count, err := s.d.IdentityManager().CountActiveFirstFactorCredentials(r.Context(), i)
+	if err != nil {
+		return s.handleSettingsError(w, r, ctxUpdate, p, err)
+	}
+
+	if count < 2 {
+		return s.handleSettingsError(w, r, ctxUpdate, p, errors.WithStack(UnlinkAllFirstFactorConnectionsError))
 	}
 
 	var found bool

selfservice/strategy/oidc/strategy_settings_test.go

@@ -257,7 +257,7 @@ func TestSettingsStrategy(t *testing.T) {
 			return
 		}
 
-		var unlinkInvalid = func(agent, provider string) func(t *testing.T) {
+		var unlinkInvalid = func(agent, provider, errorMessage string) func(t *testing.T) {
 			return func(t *testing.T) {
 				body, res, req := unlink(t, agent, provider)
 
@@ -267,27 +267,26 @@ func TestSettingsStrategy(t *testing.T) {
 
 				// The original options to link google and github are still there
 				t.Run("flow=fetch", func(t *testing.T) {
-					snapshotx.SnapshotTExcept(t, req.Ui.Nodes, []string{"0.attributes.value", "1.attributes.value"})
+					snapshotx.SnapshotT(t, req.Ui.Nodes, snapshotx.ExceptPaths("0.attributes.value", "1.attributes.value"))
 				})
 
 				t.Run("flow=json", func(t *testing.T) {
-					snapshotx.SnapshotTExcept(t, json.RawMessage(gjson.GetBytes(body, `ui.nodes`).Raw), []string{"0.attributes.value", "1.attributes.value"})
+					snapshotx.SnapshotT(t, json.RawMessage(gjson.GetBytes(body, `ui.nodes`).Raw), snapshotx.ExceptPaths("0.attributes.value", "1.attributes.value"))
 				})
 
 				assert.Contains(t, gjson.GetBytes(body, "ui.action").String(), publicTS.URL+settings.RouteSubmitFlow+"?flow=")
-				assert.Contains(t, gjson.GetBytes(body, `ui.messages.0.text`).String(),
-					"can not unlink non-existing OpenID Connect")
+				assert.Contains(t, gjson.GetBytes(body, `ui.messages.0.text`).String(), errorMessage)
 			}
 		}
 
 		t.Run("case=should not be able to unlink the last remaining connection",
-			unlinkInvalid("oryer", "ory"))
+			unlinkInvalid("oryer", "ory", "can not unlink OpenID Connect connection because it is the last remaining first factor credential"))
 
 		t.Run("case=should not be able to unlink an non-existing connection",
-			unlinkInvalid("oryer", "i-do-not-exist"))
+			unlinkInvalid("githuber", "i-do-not-exist", "can not unlink non-existing OpenID Connect connection"))
 
 		t.Run("case=should not be able to unlink a connection not yet linked",
-			unlinkInvalid("githuber", "google"))
+			unlinkInvalid("githuber", "google", "can not unlink non-existing OpenID Connect connection"))
 
 		t.Run("case=should unlink a connection", func(t *testing.T) {
 			agent, provider := "githuber", "github"

test/e2e/cypress/integration/profiles/oidc/settings/success.spec.ts

@@ -192,6 +192,38 @@ context("Social Sign In Settings Success", () => {
 
           hydraReauthFails()
         })
+
+        it("settings screen stays intact when the original sign up method gets removed", () => {
+          const expectSettingsOk = () => {
+            cy.get('[value="google"]', { timeout: 1000 })
+              .should("have.attr", "name", "link")
+              .should("contain.text", "Link google")
+
+            cy.get('[value="github"]', { timeout: 1000 })
+              .should("have.attr", "name", "link")
+              .should("contain.text", "Link github")
+          }
+
+          cy.visit(settings)
+          expectSettingsOk()
+
+          // set password
+          cy.get('input[name="password"]').type(gen.password())
+          cy.get('button[value="password"]').click()
+
+          // remove the provider used to log in
+          cy.updateConfigFile((config) => {
+            config.selfservice.methods.oidc.config.providers =
+              config.selfservice.methods.oidc.config.providers.filter(
+                ({ id }) => id !== "hydra",
+              )
+            return config
+          })
+
+          // visit settings and everything should still be fine
+          cy.visit(settings)
+          expectSettingsOk()
+        })
       })
     })
   })

test/e2e/run.sh

@@ -43,6 +43,7 @@ base=$(pwd)
 setup=yes
 dev=no
 nokill=no
+cleanup=no
 for i in "$@"; do
   case $i in
   --no-kill)
@@ -61,18 +62,26 @@ for i in "$@"; do
     dev=yes
     shift # past argument=value
     ;;
+  --cleanup)
+    cleanup=yes
+    shift # past argument=value
+    ;;
   esac
 done
 
-prepare() {
-  if [[ "${nokill}" == "no" ]]; then
+cleanup() {
     killall node || true
     killall modd || true
     killall webhook || true
     killall hydra || true
     killall hydra-login-consent || true
     killall hydra-kratos-login-consent || true
     docker kill kratos_test_hydra || true
+}
+
+prepare() {
+  if [[ "${nokill}" == "no" ]]; then
+    cleanup
   fi
 
   if [ -z ${TEST_DATABASE_POSTGRESQL+x} ]; then
@@ -339,6 +348,11 @@ the path where the kratos-selfservice-ui-node project is checked out:
   $0 ..."
 }
 
+if [[ "${cleanup}" == "yes" ]]; then
+  cleanup
+  exit 0
+fi
+
 export TEST_DATABASE_SQLITE="sqlite:///$(mktemp -d -t ci-XXXXXXXXXX)/db.sqlite?_fk=true"
 export TEST_DATABASE_MEMORY="memory"
 
@@ -374,4 +388,5 @@ esac
 if [[ "${setup}" == "yes" ]]; then
   prepare
 fi
+
 run "${db}"