fix: improve debugging/DX when enabling MFA (#822)

* fix: improve debugging/DX when enabling MFA * feat: add list of unsatisfied factors to error message * test: add test that checks that the mfa info endpoint errors if user is stuck
supertokens · Apr 11, 2024 · 3f7dd41 · 3f7dd41
1 parent 4b8a80d
commit 3f7dd41
Show file tree

Hide file tree

Showing 12 changed files with 113 additions and 28 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [unreleased]
 
+## [17.0.4] - 2024-04-09
+
+### Changes
+
+-   Improved error message to help debugging if MFA was enabled without account linking.
+-   Now the `resyncSessionAndFetchMFAInfoPUT` will throw if the user is in a stuck state, because they are required to complete factors, but they are not allowed to because of some configuration issue.
+
 ## [17.0.3] - 2024-04-08
 
 ### Fixes

diff --git a/lib/build/authUtils.js b/lib/build/authUtils.js
diff --git a/lib/build/recipe/multifactorauth/api/implementation.js b/lib/build/recipe/multifactorauth/api/implementation.js
@@ -93,18 +93,26 @@ function getAPIInterface() {
                     message: "User no longer associated with the session",
                 });
             }
+            // next array is filtered to only include factors that are allowed to be setup or are already setup
+            // we do this because the factor chooser in the frontend will be based on the next array only
+            // we do not simply filter out the factors that are not allowed to be setup because in the case
+            // where user has already setup a factor and not completed it, none of the factors will be allowed to
+            // be setup, and that that will result in an empty next array. However, we want to show the factor
+            // that the user has already setup in that case.
+            const next = nextSetOfUnsatisfiedFactors.factorIds.filter(
+                (factorId) => factorsAllowedToSetup.includes(factorId) || factorsSetUpForUser.includes(factorId)
+            );
+            if (next.length === 0 && nextSetOfUnsatisfiedFactors.factorIds.length !== 0) {
+                throw new Error(
+                    `The user is required to complete secondary factors they are not allowed to (${nextSetOfUnsatisfiedFactors.factorIds.join(
+                        ", "
+                    )}), likely because of configuration issues.`
+                );
+            }
             return {
                 status: "OK",
                 factors: {
-                    // next array is filtered to only include factors that are allowed to be setup or are already setup
-                    // we do this because the factor chooser in the frontend will be based on the next array only
-                    // we do not simply filter out the factors that are not allowed to be setup because in the case
-                    // where user has already setup a factor and not completed it, none of the factors will be allowed to
-                    // be setup, and that that will result in an empty next array. However, we want to show the factor
-                    // that the user has already setup in that case.
-                    next: nextSetOfUnsatisfiedFactors.factorIds.filter(
-                        (factorId) => factorsAllowedToSetup.includes(factorId) || factorsSetUpForUser.includes(factorId)
-                    ),
+                    next,
                     alreadySetup: factorsSetUpForUser,
                     allowedToSetup: factorsAllowedToSetup,
                 },

diff --git a/lib/build/version.d.ts b/lib/build/version.d.ts
diff --git a/lib/build/version.js b/lib/build/version.js
diff --git a/lib/ts/authUtils.ts b/lib/ts/authUtils.ts
@@ -951,7 +951,8 @@ export const AuthUtils = {
             } else {
                 throw new SuperTokensError({
                     type: SuperTokensError.BAD_INPUT_ERROR,
-                    message: "First factor sign in/up called for a non-first factor with an active session.",
+                    message:
+                        "First factor sign in/up called for a non-first factor with an active session. This might indicate that you are trying to use this as a secondary factor, but disabled account linking.",
                 });
             }
         }

diff --git a/lib/ts/recipe/multifactorauth/api/implementation.ts b/lib/ts/recipe/multifactorauth/api/implementation.ts
@@ -96,18 +96,28 @@ export default function getAPIInterface(): APIInterface {
                 });
             }
 
+            // next array is filtered to only include factors that are allowed to be setup or are already setup
+            // we do this because the factor chooser in the frontend will be based on the next array only
+            // we do not simply filter out the factors that are not allowed to be setup because in the case
+            // where user has already setup a factor and not completed it, none of the factors will be allowed to
+            // be setup, and that that will result in an empty next array. However, we want to show the factor
+            // that the user has already setup in that case.
+            const next = nextSetOfUnsatisfiedFactors.factorIds.filter(
+                (factorId) => factorsAllowedToSetup.includes(factorId) || factorsSetUpForUser.includes(factorId)
+            );
+
+            if (next.length === 0 && nextSetOfUnsatisfiedFactors.factorIds.length !== 0) {
+                throw new Error(
+                    `The user is required to complete secondary factors they are not allowed to (${nextSetOfUnsatisfiedFactors.factorIds.join(
+                        ", "
+                    )}), likely because of configuration issues.`
+                );
+            }
+
             return {
                 status: "OK",
                 factors: {
-                    // next array is filtered to only include factors that are allowed to be setup or are already setup
-                    // we do this because the factor chooser in the frontend will be based on the next array only
-                    // we do not simply filter out the factors that are not allowed to be setup because in the case
-                    // where user has already setup a factor and not completed it, none of the factors will be allowed to
-                    // be setup, and that that will result in an empty next array. However, we want to show the factor
-                    // that the user has already setup in that case.
-                    next: nextSetOfUnsatisfiedFactors.factorIds.filter(
-                        (factorId) => factorsAllowedToSetup.includes(factorId) || factorsSetUpForUser.includes(factorId)
-                    ),
+                    next,
                     alreadySetup: factorsSetUpForUser,
                     allowedToSetup: factorsAllowedToSetup,
                 },

diff --git a/lib/ts/version.ts b/lib/ts/version.ts
@@ -12,7 +12,7 @@
  * License for the specific language governing permissions and limitations
  * under the License.
  */
-export const version = "17.0.3";
+export const version = "17.0.4";
 
 export const cdiSupported = ["5.0"];
 

diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
     "name": "supertokens-node",
-    "version": "17.0.3",
+    "version": "17.0.4",
     "description": "NodeJS driver for SuperTokens core",
     "main": "index.js",
     "scripts": {

diff --git a/test/mfa/mfa.api.test.js b/test/mfa/mfa.api.test.js
@@ -226,6 +226,64 @@ describe(`mfa-api: ${printPath("[test/mfa/mfa.api.test.js]")}`, function () {
         assert.deepEqual(["emailpassword", "otp-email", "totp"], res.body.factors.allowedToSetup);
     });
 
+    it("mfa info errors if the user is stuck", async function () {
+        const connectionURI = await startSTWithMultitenancy();
+        let requireFactor = false;
+
+        SuperTokens.init({
+            supertokens: {
+                connectionURI,
+            },
+            appInfo: {
+                apiDomain: "api.supertokens.io",
+                appName: "SuperTokens",
+                websiteDomain: "supertokens.io",
+            },
+            recipeList: [
+                EmailPassword.init(),
+                ThirdParty.init(),
+                AccountLinking.init({
+                    shouldDoAutomaticAccountLinking: async () => ({
+                        shouldAutomaticallyLink: true,
+                        shouldRequireVerification: true,
+                    }),
+                }),
+                MultiFactorAuth.init({
+                    override: {
+                        functions: (oI) => ({
+                            ...oI,
+                            getMFARequirementsForAuth: () => (requireFactor ? ["otp-phone"] : []),
+                        }),
+                    },
+                }),
+                Session.init(),
+            ],
+        });
+
+        const app = getTestExpressApp();
+
+        await EmailPassword.signUp("public", "[email protected]", "password");
+
+        let res = await epSignIn(app, "[email protected]", "password");
+        assert.equal("OK", res.body.status);
+
+        let cookies = extractInfoFromResponse(res);
+        const accessToken = cookies.accessTokenFromAny;
+
+        res = await getMfaInfo(app, accessToken);
+        assert.equal("OK", res.body.status);
+        assert.deepEqual([], res.body.factors.next);
+
+        requireFactor = true;
+
+        res = await getMfaInfo(app, accessToken, 500);
+        assert(
+            res.text.includes(
+                "The user is required to complete secondary factors they are not allowed to (otp-phone), likely because of configuration issues."
+            )
+        );
+    });
+
     it("test that only a valid first factor is allowed to login", async function () {
         const connectionURI = await startSTWithMultitenancy();
         SuperTokens.init({

diff --git a/test/mfa/utils.js b/test/mfa/utils.js
@@ -222,6 +222,6 @@ module.exports.tpSignInUp = async function (app, thirdPartyId, email, accessToke
     }
 };
 
-module.exports.getMfaInfo = async function (app, accessToken) {
-    return request(app).put("/auth/mfa/info").set("Authorization", `Bearer ${accessToken}`).expect(200);
+module.exports.getMfaInfo = async function (app, accessToken, statusCode = 200) {
+    return request(app).put("/auth/mfa/info").set("Authorization", `Bearer ${accessToken}`).expect(statusCode);
 };