From 06b2cf323610c9dcc7a4bc37ff685775c8cda36d Mon Sep 17 00:00:00 2001 From: KShivendu Date: Thu, 22 Jun 2023 19:06:57 +0530 Subject: [PATCH 01/16] refactor: Dont send TOTP_NOT_ENABLED_ERROR status --- src/main/java/io/supertokens/totp/Totp.java | 57 +++++++------------ .../api/totp/CreateOrUpdateTotpDeviceAPI.java | 4 -- .../webserver/api/totp/GetTotpDevicesAPI.java | 4 -- .../api/totp/RemoveTotpDeviceAPI.java | 6 +- .../webserver/api/totp/VerifyTotpAPI.java | 7 --- .../api/totp/VerifyTotpDeviceAPI.java | 5 -- .../supertokens/test/totp/TOTPRecipeTest.java | 14 ++--- .../test/totp/api/GetTotpDevicesAPITest.java | 3 +- .../totp/api/RemoveTotpDeviceAPITest.java | 3 +- .../totp/api/UpdateTotpDeviceAPITest.java | 2 +- .../test/totp/api/VerifyTotpAPITest.java | 2 +- .../totp/api/VerifyTotpDeviceAPITest.java | 2 +- 12 files changed, 37 insertions(+), 72 deletions(-) diff --git a/src/main/java/io/supertokens/totp/Totp.java b/src/main/java/io/supertokens/totp/Totp.java index 307f49471..a75953e8b 100644 --- a/src/main/java/io/supertokens/totp/Totp.java +++ b/src/main/java/io/supertokens/totp/Totp.java @@ -284,7 +284,7 @@ private static void checkAndStoreCode(TenantIdentifierWithStorage tenantIdentifi @TestOnly public static boolean verifyDevice(Main main, String userId, String deviceName, String code) - throws TotpNotEnabledException, UnknownDeviceException, InvalidTotpException, + throws UnknownDeviceException, InvalidTotpException, LimitReachedException, StorageQueryException, StorageTransactionLogicException { try { return verifyDevice(new TenantIdentifierWithStorage(null, null, null, StorageLayer.getStorage(main)), main, @@ -296,7 +296,7 @@ public static boolean verifyDevice(Main main, public static boolean verifyDevice(TenantIdentifierWithStorage tenantIdentifierWithStorage, Main main, String userId, String deviceName, String code) - throws TotpNotEnabledException, UnknownDeviceException, InvalidTotpException, + throws UnknownDeviceException, InvalidTotpException, LimitReachedException, StorageQueryException, StorageTransactionLogicException, TenantOrAppNotFoundException { // Here boolean return value tells whether the device has been @@ -312,7 +312,7 @@ public static boolean verifyDevice(TenantIdentifierWithStorage tenantIdentifierW // Check if the user has any devices: TOTPDevice[] devices = totpStorage.getDevices(tenantIdentifierWithStorage.toAppIdentifier(), userId); if (devices.length == 0) { - throw new TotpNotEnabledException(); + throw new UnknownDeviceException(); } // Check if the requested device exists: @@ -337,8 +337,11 @@ public static boolean verifyDevice(TenantIdentifierWithStorage tenantIdentifierW // verified in the devices table (because it was deleted/renamed). So the user // gets a UnknownDevceException. // This behaviour is okay so we can ignore it. - checkAndStoreCode(tenantIdentifierWithStorage, main, userId, new TOTPDevice[]{matchingDevice}, - code); + try{ + checkAndStoreCode(tenantIdentifierWithStorage, main, userId, new TOTPDevice[]{matchingDevice}, code); + } catch (TotpNotEnabledException e) { + throw new UnknownDeviceException(); + } // Will reach here only if the code is valid: totpStorage.markDeviceAsVerified(tenantIdentifierWithStorage.toAppIdentifier(), userId, deviceName); return true; // Newly verified @@ -347,7 +350,7 @@ public static boolean verifyDevice(TenantIdentifierWithStorage tenantIdentifierW @TestOnly public static void verifyCode(Main main, String userId, String code, boolean allowUnverifiedDevices) - throws TotpNotEnabledException, InvalidTotpException, LimitReachedException, + throws InvalidTotpException, LimitReachedException, StorageQueryException, StorageTransactionLogicException, FeatureNotEnabledException { try { verifyCode(new TenantIdentifierWithStorage(null, null, null, StorageLayer.getStorage(main)), main, @@ -359,7 +362,7 @@ public static void verifyCode(Main main, String userId, public static void verifyCode(TenantIdentifierWithStorage tenantIdentifierWithStorage, Main main, String userId, String code, boolean allowUnverifiedDevices) - throws TotpNotEnabledException, InvalidTotpException, LimitReachedException, + throws InvalidTotpException, LimitReachedException, StorageQueryException, StorageTransactionLogicException, FeatureNotEnabledException, TenantOrAppNotFoundException { @@ -374,7 +377,7 @@ public static void verifyCode(TenantIdentifierWithStorage tenantIdentifierWithSt // Check if the user has any devices: TOTPDevice[] devices = totpStorage.getDevices(tenantIdentifierWithStorage.toAppIdentifier(), userId); if (devices.length == 0) { - throw new TotpNotEnabledException(); + throw new InvalidTotpException(); } // Filter out unverified devices: @@ -386,13 +389,17 @@ public static void verifyCode(TenantIdentifierWithStorage tenantIdentifierWithSt // another API call. We will still check the code against the updated set of // devices and store it in the used codes table. This behaviour is okay so we // can ignore it. - checkAndStoreCode(tenantIdentifierWithStorage, main, userId, devices, code); + try{ + checkAndStoreCode(tenantIdentifierWithStorage, main, userId, devices, code); + } catch (TotpNotEnabledException e) { + throw new InvalidTotpException(); + } } @TestOnly public static void removeDevice(Main main, String userId, String deviceName) - throws StorageQueryException, UnknownDeviceException, TotpNotEnabledException, + throws StorageQueryException, UnknownDeviceException, StorageTransactionLogicException { try { removeDevice(new AppIdentifierWithStorage(null, null, StorageLayer.getStorage(main)), @@ -407,7 +414,7 @@ public static void removeDevice(Main main, String userId, */ public static void removeDevice(AppIdentifierWithStorage appIdentifierWithStorage, String userId, String deviceName) - throws StorageQueryException, UnknownDeviceException, TotpNotEnabledException, + throws StorageQueryException, UnknownDeviceException, StorageTransactionLogicException, TenantOrAppNotFoundException { TOTPSQLStorage storage = appIdentifierWithStorage.getTOTPStorage(); @@ -432,12 +439,6 @@ public static void removeDevice(AppIdentifierWithStorage appIdentifierWithStorag return; } catch (StorageTransactionLogicException e) { if (e.actualException instanceof UnknownDeviceException) { - // Check if any device exists for the user: - TOTPDevice[] devices = storage.getDevices(appIdentifierWithStorage, userId); - if (devices.length == 0) { - throw new TotpNotEnabledException(); - } - throw (UnknownDeviceException) e.actualException; } @@ -460,25 +461,14 @@ public static void updateDeviceName(Main main, String userId, public static void updateDeviceName(AppIdentifierWithStorage appIdentifierWithStorage, String userId, String oldDeviceName, String newDeviceName) - throws StorageQueryException, DeviceAlreadyExistsException, UnknownDeviceException, - TotpNotEnabledException, TenantOrAppNotFoundException { + throws StorageQueryException, DeviceAlreadyExistsException, UnknownDeviceException, TenantOrAppNotFoundException { TOTPSQLStorage totpStorage = appIdentifierWithStorage.getTOTPStorage(); - try { - totpStorage.updateDeviceName(appIdentifierWithStorage, userId, oldDeviceName, newDeviceName); - } catch (UnknownDeviceException e) { - // Check if any device exists for the user: - TOTPDevice[] devices = totpStorage.getDevices(appIdentifierWithStorage, userId); - if (devices.length == 0) { - throw new TotpNotEnabledException(); - } else { - throw e; - } - } + totpStorage.updateDeviceName(appIdentifierWithStorage, userId, oldDeviceName, newDeviceName); } @TestOnly public static TOTPDevice[] getDevices(Main main, String userId) - throws StorageQueryException, TotpNotEnabledException { + throws StorageQueryException { try { return getDevices(new AppIdentifierWithStorage(null, null, StorageLayer.getStorage(main)), userId); @@ -488,13 +478,10 @@ public static TOTPDevice[] getDevices(Main main, String userId) } public static TOTPDevice[] getDevices(AppIdentifierWithStorage appIdentifierWithStorage, String userId) - throws StorageQueryException, TotpNotEnabledException, TenantOrAppNotFoundException { + throws StorageQueryException, TenantOrAppNotFoundException { TOTPSQLStorage totpStorage = appIdentifierWithStorage.getTOTPStorage(); TOTPDevice[] devices = totpStorage.getDevices(appIdentifierWithStorage, userId); - if (devices.length == 0) { - throw new TotpNotEnabledException(); - } return devices; } diff --git a/src/main/java/io/supertokens/webserver/api/totp/CreateOrUpdateTotpDeviceAPI.java b/src/main/java/io/supertokens/webserver/api/totp/CreateOrUpdateTotpDeviceAPI.java index 3d1ad47aa..4a6e2c03d 100644 --- a/src/main/java/io/supertokens/webserver/api/totp/CreateOrUpdateTotpDeviceAPI.java +++ b/src/main/java/io/supertokens/webserver/api/totp/CreateOrUpdateTotpDeviceAPI.java @@ -11,7 +11,6 @@ import io.supertokens.pluginInterface.multitenancy.exceptions.TenantOrAppNotFoundException; import io.supertokens.pluginInterface.totp.TOTPDevice; import io.supertokens.pluginInterface.totp.exception.DeviceAlreadyExistsException; -import io.supertokens.pluginInterface.totp.exception.TotpNotEnabledException; import io.supertokens.pluginInterface.totp.exception.UnknownDeviceException; import io.supertokens.totp.Totp; import io.supertokens.useridmapping.UserIdType; @@ -143,9 +142,6 @@ protected void doPut(HttpServletRequest req, HttpServletResponse resp) throws IO result.addProperty("status", "OK"); super.sendJsonResponse(200, result, resp); - } catch (TotpNotEnabledException e) { - result.addProperty("status", "TOTP_NOT_ENABLED_ERROR"); - super.sendJsonResponse(200, result, resp); } catch (UnknownDeviceException e) { result.addProperty("status", "UNKNOWN_DEVICE_ERROR"); super.sendJsonResponse(200, result, resp); diff --git a/src/main/java/io/supertokens/webserver/api/totp/GetTotpDevicesAPI.java b/src/main/java/io/supertokens/webserver/api/totp/GetTotpDevicesAPI.java index 079daae94..98da43d5c 100644 --- a/src/main/java/io/supertokens/webserver/api/totp/GetTotpDevicesAPI.java +++ b/src/main/java/io/supertokens/webserver/api/totp/GetTotpDevicesAPI.java @@ -10,7 +10,6 @@ import io.supertokens.pluginInterface.multitenancy.AppIdentifierWithStorage; import io.supertokens.pluginInterface.multitenancy.exceptions.TenantOrAppNotFoundException; import io.supertokens.pluginInterface.totp.TOTPDevice; -import io.supertokens.pluginInterface.totp.exception.TotpNotEnabledException; import io.supertokens.totp.Totp; import io.supertokens.useridmapping.UserIdType; import io.supertokens.webserver.InputParser; @@ -80,9 +79,6 @@ protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IO result.addProperty("status", "OK"); result.add("devices", devicesArray); super.sendJsonResponse(200, result, resp); - } catch (TotpNotEnabledException e) { - result.addProperty("status", "TOTP_NOT_ENABLED_ERROR"); - super.sendJsonResponse(200, result, resp); } catch (StorageQueryException | TenantOrAppNotFoundException e) { throw new ServletException(e); } diff --git a/src/main/java/io/supertokens/webserver/api/totp/RemoveTotpDeviceAPI.java b/src/main/java/io/supertokens/webserver/api/totp/RemoveTotpDeviceAPI.java index df3ea4801..96b094558 100644 --- a/src/main/java/io/supertokens/webserver/api/totp/RemoveTotpDeviceAPI.java +++ b/src/main/java/io/supertokens/webserver/api/totp/RemoveTotpDeviceAPI.java @@ -9,7 +9,6 @@ import io.supertokens.pluginInterface.exceptions.StorageTransactionLogicException; import io.supertokens.pluginInterface.multitenancy.AppIdentifierWithStorage; import io.supertokens.pluginInterface.multitenancy.exceptions.TenantOrAppNotFoundException; -import io.supertokens.pluginInterface.totp.exception.TotpNotEnabledException; import io.supertokens.pluginInterface.totp.exception.UnknownDeviceException; import io.supertokens.totp.Totp; import io.supertokens.useridmapping.UserIdType; @@ -75,12 +74,9 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws I result.addProperty("status", "OK"); result.addProperty("didDeviceExist", true); super.sendJsonResponse(200, result, resp); - } catch (TotpNotEnabledException e) { - result.addProperty("status", "TOTP_NOT_ENABLED_ERROR"); - super.sendJsonResponse(200, result, resp); } catch (UnknownDeviceException e) { result.addProperty("status", "OK"); - result.addProperty("didDeviceExist", false); + result.addProperty("didDeviceExist", false); // Pattern not consistent with PUT API. We should fix? super.sendJsonResponse(200, result, resp); } catch (StorageQueryException | StorageTransactionLogicException | TenantOrAppNotFoundException e) { throw new ServletException(e); diff --git a/src/main/java/io/supertokens/webserver/api/totp/VerifyTotpAPI.java b/src/main/java/io/supertokens/webserver/api/totp/VerifyTotpAPI.java index 7d3980d99..436b9f0d2 100644 --- a/src/main/java/io/supertokens/webserver/api/totp/VerifyTotpAPI.java +++ b/src/main/java/io/supertokens/webserver/api/totp/VerifyTotpAPI.java @@ -4,7 +4,6 @@ import com.google.gson.JsonObject; -import io.supertokens.AppIdentifierWithStorageAndUserIdMapping; import io.supertokens.Main; import io.supertokens.TenantIdentifierWithStorageAndUserIdMapping; import io.supertokens.featureflag.exceptions.FeatureNotEnabledException; @@ -12,11 +11,8 @@ import io.supertokens.pluginInterface.emailpassword.exceptions.UnknownUserIdException; import io.supertokens.pluginInterface.exceptions.StorageQueryException; import io.supertokens.pluginInterface.exceptions.StorageTransactionLogicException; -import io.supertokens.pluginInterface.multitenancy.AppIdentifierWithStorage; import io.supertokens.pluginInterface.multitenancy.TenantIdentifierWithStorage; import io.supertokens.pluginInterface.multitenancy.exceptions.TenantOrAppNotFoundException; -import io.supertokens.pluginInterface.totp.exception.TotpNotEnabledException; -import io.supertokens.pluginInterface.useridmapping.UserIdMapping; import io.supertokens.totp.Totp; import io.supertokens.totp.exceptions.InvalidTotpException; import io.supertokens.totp.exceptions.LimitReachedException; @@ -80,9 +76,6 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws I result.addProperty("status", "OK"); super.sendJsonResponse(200, result, resp); - } catch (TotpNotEnabledException e) { - result.addProperty("status", "TOTP_NOT_ENABLED_ERROR"); - super.sendJsonResponse(200, result, resp); } catch (InvalidTotpException e) { result.addProperty("status", "INVALID_TOTP_ERROR"); super.sendJsonResponse(200, result, resp); diff --git a/src/main/java/io/supertokens/webserver/api/totp/VerifyTotpDeviceAPI.java b/src/main/java/io/supertokens/webserver/api/totp/VerifyTotpDeviceAPI.java index a068e07e4..da8c21c01 100644 --- a/src/main/java/io/supertokens/webserver/api/totp/VerifyTotpDeviceAPI.java +++ b/src/main/java/io/supertokens/webserver/api/totp/VerifyTotpDeviceAPI.java @@ -12,9 +12,7 @@ import io.supertokens.pluginInterface.exceptions.StorageTransactionLogicException; import io.supertokens.pluginInterface.multitenancy.TenantIdentifierWithStorage; import io.supertokens.pluginInterface.multitenancy.exceptions.TenantOrAppNotFoundException; -import io.supertokens.pluginInterface.totp.exception.TotpNotEnabledException; import io.supertokens.pluginInterface.totp.exception.UnknownDeviceException; -import io.supertokens.pluginInterface.useridmapping.UserIdMapping; import io.supertokens.totp.Totp; import io.supertokens.totp.exceptions.InvalidTotpException; import io.supertokens.totp.exceptions.LimitReachedException; @@ -80,9 +78,6 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws I result.addProperty("status", "OK"); result.addProperty("wasAlreadyVerified", !isNewlyVerified); super.sendJsonResponse(200, result, resp); - } catch (TotpNotEnabledException e) { - result.addProperty("status", "TOTP_NOT_ENABLED_ERROR"); - super.sendJsonResponse(200, result, resp); } catch (UnknownDeviceException e) { result.addProperty("status", "UNKNOWN_DEVICE_ERROR"); super.sendJsonResponse(200, result, resp); diff --git a/src/test/java/io/supertokens/test/totp/TOTPRecipeTest.java b/src/test/java/io/supertokens/test/totp/TOTPRecipeTest.java index 0af4feaf5..6378cbb76 100644 --- a/src/test/java/io/supertokens/test/totp/TOTPRecipeTest.java +++ b/src/test/java/io/supertokens/test/totp/TOTPRecipeTest.java @@ -167,7 +167,7 @@ public void createDeviceAndVerifyCodeTest() throws Exception { TOTPDevice device = Totp.registerDevice(main, "user", "device1", 1, 1); // Try login with non-existent user: - assertThrows(TotpNotEnabledException.class, + assertThrows(InvalidTotpException.class, // TODO: Can we throw a better error? () -> Totp.verifyCode(main, "non-existent-user", "any-code", true)); // {Code: [INVALID, VALID]} * {Devices: [VERIFIED_ONLY, ALL]} @@ -378,7 +378,7 @@ public void createAndVerifyDeviceTest() throws Exception { TOTPDevice device = Totp.registerDevice(main, "user", "deviceName", 1, 30); // Try verify non-existent user: - assertThrows(TotpNotEnabledException.class, + assertThrows(UnknownDeviceException.class, () -> Totp.verifyDevice(main, "non-existent-user", "deviceName", "XXXX")); // Try verify non-existent device @@ -428,7 +428,7 @@ public void removeDeviceTest() throws Exception { assert (devices.length == 2); // Try to delete device for non-existent user: - assertThrows(TotpNotEnabledException.class, () -> Totp.removeDevice(main, "non-existent-user", "device1")); + assertThrows(UnknownDeviceException.class, () -> Totp.removeDevice(main, "non-existent-user", "device1")); // Try to delete non-existent device: assertThrows(UnknownDeviceException.class, () -> Totp.removeDevice(main, "user", "non-existent-device")); @@ -462,8 +462,8 @@ public void removeDeviceTest() throws Exception { // Delete device2 Totp.removeDevice(main, "user", "device2"); - // TOTP has ben disabled for the user: - assertThrows(TotpNotEnabledException.class, () -> Totp.getDevices(main, "user")); + // No more devices are left for the user: + assert (Totp.getDevices(main, "user").length == 0); // No device left so all codes of the user should be deleted: TOTPUsedCode[] usedCodes = getAllUsedCodesUtil(storage, "user"); @@ -490,7 +490,7 @@ public void updateDeviceNameTest() throws Exception { Totp.registerDevice(main, "user", "device2", 1, 30); // Try update non-existent user: - assertThrows(TotpNotEnabledException.class, + assertThrows(UnknownDeviceException.class, () -> Totp.updateDeviceName(main, "non-existent-user", "device1", "new-device-name")); // Try update non-existent device: @@ -526,7 +526,7 @@ public void getDevicesTest() throws Exception { Main main = result.process.getProcess(); // Try get devices for non-existent user: - assertThrows(TotpNotEnabledException.class, () -> Totp.getDevices(main, "non-existent-user")); + assert (Totp.getDevices(main, "non-existent-user").length == 0); TOTPDevice device1 = Totp.registerDevice(main, "user", "device1", 2, 30); TOTPDevice device2 = Totp.registerDevice(main, "user", "device2", 1, 10); diff --git a/src/test/java/io/supertokens/test/totp/api/GetTotpDevicesAPITest.java b/src/test/java/io/supertokens/test/totp/api/GetTotpDevicesAPITest.java index 5450be943..0ea0f8387 100644 --- a/src/test/java/io/supertokens/test/totp/api/GetTotpDevicesAPITest.java +++ b/src/test/java/io/supertokens/test/totp/api/GetTotpDevicesAPITest.java @@ -153,7 +153,8 @@ public void testApi() throws Exception { null, Utils.getCdiVersionStringLatestForTests(), "totp"); - assert res2.get("status").getAsString().equals("TOTP_NOT_ENABLED_ERROR"); + assert res2.get("status").getAsString().equals("OK"); + assert res2.get("devices").getAsJsonArray().size() == 0; } process.kill(); diff --git a/src/test/java/io/supertokens/test/totp/api/RemoveTotpDeviceAPITest.java b/src/test/java/io/supertokens/test/totp/api/RemoveTotpDeviceAPITest.java index 845b42852..bb4a13a53 100644 --- a/src/test/java/io/supertokens/test/totp/api/RemoveTotpDeviceAPITest.java +++ b/src/test/java/io/supertokens/test/totp/api/RemoveTotpDeviceAPITest.java @@ -180,7 +180,8 @@ public void testApi() throws Exception { null, Utils.getCdiVersionStringLatestForTests(), "totp"); - assert res3.get("status").getAsString().equals("TOTP_NOT_ENABLED_ERROR"); + assert res3.get("status").getAsString().equals("OK"); + assert res3.get("didDeviceExist").getAsBoolean() == false; } process.kill(); diff --git a/src/test/java/io/supertokens/test/totp/api/UpdateTotpDeviceAPITest.java b/src/test/java/io/supertokens/test/totp/api/UpdateTotpDeviceAPITest.java index c94d72ff0..29be8c12c 100644 --- a/src/test/java/io/supertokens/test/totp/api/UpdateTotpDeviceAPITest.java +++ b/src/test/java/io/supertokens/test/totp/api/UpdateTotpDeviceAPITest.java @@ -199,7 +199,7 @@ public void testApi() throws Exception { null, Utils.getCdiVersionStringLatestForTests(), "totp"); - assert res4.get("status").getAsString().equals("TOTP_NOT_ENABLED_ERROR"); + assert res4.get("status").getAsString().equals("UNKNOWN_DEVICE_ERROR"); } process.kill(); diff --git a/src/test/java/io/supertokens/test/totp/api/VerifyTotpAPITest.java b/src/test/java/io/supertokens/test/totp/api/VerifyTotpAPITest.java index 57d765a51..d4e21069b 100644 --- a/src/test/java/io/supertokens/test/totp/api/VerifyTotpAPITest.java +++ b/src/test/java/io/supertokens/test/totp/api/VerifyTotpAPITest.java @@ -235,7 +235,7 @@ public void testApi() throws Exception { null, Utils.getCdiVersionStringLatestForTests(), "totp"); - assert res5.get("status").getAsString().equals("TOTP_NOT_ENABLED_ERROR"); + assert res5.get("status").getAsString().equals("INVALID_TOTP_ERROR"); } process.kill(); diff --git a/src/test/java/io/supertokens/test/totp/api/VerifyTotpDeviceAPITest.java b/src/test/java/io/supertokens/test/totp/api/VerifyTotpDeviceAPITest.java index 6df604603..ca5e1c43b 100644 --- a/src/test/java/io/supertokens/test/totp/api/VerifyTotpDeviceAPITest.java +++ b/src/test/java/io/supertokens/test/totp/api/VerifyTotpDeviceAPITest.java @@ -238,7 +238,7 @@ public void testApi() throws Exception { null, Utils.getCdiVersionStringLatestForTests(), "totp"); - assert res5.get("status").getAsString().equals("TOTP_NOT_ENABLED_ERROR"); + assert res5.get("status").getAsString().equals("UNKNOWN_DEVICE_ERROR"); } process.kill(); From 693b70e331805048edb4a41aa92af93088f15498 Mon Sep 17 00:00:00 2001 From: KShivendu Date: Fri, 23 Jun 2023 13:33:44 +0530 Subject: [PATCH 02/16] refactor: Add comments --- src/main/java/io/supertokens/totp/Totp.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/main/java/io/supertokens/totp/Totp.java b/src/main/java/io/supertokens/totp/Totp.java index a75953e8b..fd80a2cc6 100644 --- a/src/main/java/io/supertokens/totp/Totp.java +++ b/src/main/java/io/supertokens/totp/Totp.java @@ -340,6 +340,7 @@ public static boolean verifyDevice(TenantIdentifierWithStorage tenantIdentifierW try{ checkAndStoreCode(tenantIdentifierWithStorage, main, userId, new TOTPDevice[]{matchingDevice}, code); } catch (TotpNotEnabledException e) { + // User must have deleted the device in parallel. throw new UnknownDeviceException(); } // Will reach here only if the code is valid: @@ -377,6 +378,7 @@ public static void verifyCode(TenantIdentifierWithStorage tenantIdentifierWithSt // Check if the user has any devices: TOTPDevice[] devices = totpStorage.getDevices(tenantIdentifierWithStorage.toAppIdentifier(), userId); if (devices.length == 0) { + // No devices found. So we can't verify the code anyway. throw new InvalidTotpException(); } @@ -392,6 +394,8 @@ public static void verifyCode(TenantIdentifierWithStorage tenantIdentifierWithSt try{ checkAndStoreCode(tenantIdentifierWithStorage, main, userId, devices, code); } catch (TotpNotEnabledException e) { + // User must have deleted the device in parallel + // since they cannot un-verify a device (no API exists) throw new InvalidTotpException(); } } @@ -449,8 +453,7 @@ public static void removeDevice(AppIdentifierWithStorage appIdentifierWithStorag @TestOnly public static void updateDeviceName(Main main, String userId, String oldDeviceName, String newDeviceName) - throws StorageQueryException, DeviceAlreadyExistsException, UnknownDeviceException, - TotpNotEnabledException { + throws StorageQueryException, DeviceAlreadyExistsException, UnknownDeviceException { try { updateDeviceName(new AppIdentifierWithStorage(null, null, StorageLayer.getStorage(main)), userId, oldDeviceName, newDeviceName); From b10a23cc8c08db05d3d36be548835685fd93f52d Mon Sep 17 00:00:00 2001 From: KShivendu Date: Fri, 23 Jun 2023 13:41:10 +0530 Subject: [PATCH 03/16] chores: Remove extra comments --- .../io/supertokens/webserver/api/totp/RemoveTotpDeviceAPI.java | 2 +- src/test/java/io/supertokens/test/totp/TOTPRecipeTest.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/io/supertokens/webserver/api/totp/RemoveTotpDeviceAPI.java b/src/main/java/io/supertokens/webserver/api/totp/RemoveTotpDeviceAPI.java index 96b094558..d6b0c6f50 100644 --- a/src/main/java/io/supertokens/webserver/api/totp/RemoveTotpDeviceAPI.java +++ b/src/main/java/io/supertokens/webserver/api/totp/RemoveTotpDeviceAPI.java @@ -76,7 +76,7 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws I super.sendJsonResponse(200, result, resp); } catch (UnknownDeviceException e) { result.addProperty("status", "OK"); - result.addProperty("didDeviceExist", false); // Pattern not consistent with PUT API. We should fix? + result.addProperty("didDeviceExist", false); super.sendJsonResponse(200, result, resp); } catch (StorageQueryException | StorageTransactionLogicException | TenantOrAppNotFoundException e) { throw new ServletException(e); diff --git a/src/test/java/io/supertokens/test/totp/TOTPRecipeTest.java b/src/test/java/io/supertokens/test/totp/TOTPRecipeTest.java index 6378cbb76..41bc3d2a6 100644 --- a/src/test/java/io/supertokens/test/totp/TOTPRecipeTest.java +++ b/src/test/java/io/supertokens/test/totp/TOTPRecipeTest.java @@ -167,7 +167,7 @@ public void createDeviceAndVerifyCodeTest() throws Exception { TOTPDevice device = Totp.registerDevice(main, "user", "device1", 1, 1); // Try login with non-existent user: - assertThrows(InvalidTotpException.class, // TODO: Can we throw a better error? + assertThrows(InvalidTotpException.class, () -> Totp.verifyCode(main, "non-existent-user", "any-code", true)); // {Code: [INVALID, VALID]} * {Devices: [VERIFIED_ONLY, ALL]} From 14209d805739bd6f9a89a15efbdb5ced19e612e0 Mon Sep 17 00:00:00 2001 From: KShivendu Date: Tue, 27 Jun 2023 15:20:26 +0530 Subject: [PATCH 04/16] refactor: Completely replace totp not enabled error with unknown device error --- src/main/java/io/supertokens/inmemorydb/Start.java | 9 ++++----- src/main/java/io/supertokens/totp/Totp.java | 13 ++++++------- .../java/io/supertokens/test/StorageLayerTest.java | 6 +++--- .../io/supertokens/test/totp/TOTPRecipeTest.java | 5 ----- .../io/supertokens/test/totp/TOTPStorageTest.java | 12 ++++++------ 5 files changed, 19 insertions(+), 26 deletions(-) diff --git a/src/main/java/io/supertokens/inmemorydb/Start.java b/src/main/java/io/supertokens/inmemorydb/Start.java index 58d17970c..301fc7010 100644 --- a/src/main/java/io/supertokens/inmemorydb/Start.java +++ b/src/main/java/io/supertokens/inmemorydb/Start.java @@ -68,7 +68,6 @@ import io.supertokens.pluginInterface.totp.TOTPStorage; import io.supertokens.pluginInterface.totp.TOTPUsedCode; import io.supertokens.pluginInterface.totp.exception.DeviceAlreadyExistsException; -import io.supertokens.pluginInterface.totp.exception.TotpNotEnabledException; import io.supertokens.pluginInterface.totp.exception.UnknownDeviceException; import io.supertokens.pluginInterface.totp.exception.UsedCodeAlreadyExistsException; import io.supertokens.pluginInterface.totp.sqlStorage.TOTPSQLStorage; @@ -1257,7 +1256,7 @@ public int countUsersEnabledTotpAndActiveSince(AppIdentifier appIdentifier, long throw new StorageQueryException(e); } } - + @Override public int countUsersEnabledMfa(AppIdentifier appIdentifier) throws StorageQueryException { try { @@ -1266,7 +1265,7 @@ public int countUsersEnabledMfa(AppIdentifier appIdentifier) throws StorageQuery throw new StorageQueryException(e); } } - + @Override public int countUsersEnabledMfaAndActiveSince(AppIdentifier appIdentifier, long time) throws StorageQueryException { @@ -2673,7 +2672,7 @@ public TOTPDevice[] getDevices_Transaction(TransactionConnection con, AppIdentif @Override public void insertUsedCode_Transaction(TransactionConnection con, TenantIdentifier tenantIdentifier, TOTPUsedCode usedCodeObj) - throws StorageQueryException, TotpNotEnabledException, UsedCodeAlreadyExistsException, + throws StorageQueryException, UnknownDeviceException, UsedCodeAlreadyExistsException, TenantOrAppNotFoundException { Connection sqlCon = (Connection) con.getConnection(); try { @@ -2687,7 +2686,7 @@ public void insertUsedCode_Transaction(TransactionConnection con, TenantIdentifi Config.getConfig(this).getTotpUsersTable(), new String[]{"app_id", "user_id"}, new Object[]{tenantIdentifier.getAppId(), usedCodeObj.userId})) { - throw new TotpNotEnabledException(); + throw new UnknownDeviceException(); } else if (isForeignKeyConstraintError( e.getMessage(), diff --git a/src/main/java/io/supertokens/totp/Totp.java b/src/main/java/io/supertokens/totp/Totp.java index fd80a2cc6..c991062da 100644 --- a/src/main/java/io/supertokens/totp/Totp.java +++ b/src/main/java/io/supertokens/totp/Totp.java @@ -15,7 +15,6 @@ import io.supertokens.pluginInterface.totp.TOTPDevice; import io.supertokens.pluginInterface.totp.TOTPUsedCode; import io.supertokens.pluginInterface.totp.exception.DeviceAlreadyExistsException; -import io.supertokens.pluginInterface.totp.exception.TotpNotEnabledException; import io.supertokens.pluginInterface.totp.exception.UnknownDeviceException; import io.supertokens.pluginInterface.totp.exception.UsedCodeAlreadyExistsException; import io.supertokens.pluginInterface.totp.sqlStorage.TOTPSQLStorage; @@ -118,7 +117,7 @@ public static TOTPDevice registerDevice(AppIdentifierWithStorage appIdentifierWi private static void checkAndStoreCode(TenantIdentifierWithStorage tenantIdentifierWithStorage, Main main, String userId, TOTPDevice[] devices, String code) - throws InvalidTotpException, TotpNotEnabledException, + throws InvalidTotpException, UnknownDeviceException, LimitReachedException, StorageQueryException, StorageTransactionLogicException, TenantOrAppNotFoundException { // Note that the TOTP cron runs every 1 hour, so all the expired tokens can stay @@ -241,7 +240,7 @@ private static void checkAndStoreCode(TenantIdentifierWithStorage tenantIdentifi try { totpSQLStorage.insertUsedCode_Transaction(con, tenantIdentifierWithStorage, newCode); totpSQLStorage.commitTransaction(con); - } catch (UsedCodeAlreadyExistsException | TotpNotEnabledException e) { + } catch (UsedCodeAlreadyExistsException | UnknownDeviceException e) { throw new StorageTransactionLogicException(e); } @@ -264,8 +263,8 @@ private static void checkAndStoreCode(TenantIdentifierWithStorage tenantIdentifi throw (LimitReachedException) e.actualException; } else if (e.actualException instanceof InvalidTotpException) { throw (InvalidTotpException) e.actualException; - } else if (e.actualException instanceof TotpNotEnabledException) { - throw (TotpNotEnabledException) e.actualException; + } else if (e.actualException instanceof UnknownDeviceException) { + throw (UnknownDeviceException) e.actualException; } else if (e.actualException instanceof UsedCodeAlreadyExistsException) { // retry the transaction after a small delay: int delayInMs = (int) (Math.random() * 10 + 1); @@ -339,7 +338,7 @@ public static boolean verifyDevice(TenantIdentifierWithStorage tenantIdentifierW // This behaviour is okay so we can ignore it. try{ checkAndStoreCode(tenantIdentifierWithStorage, main, userId, new TOTPDevice[]{matchingDevice}, code); - } catch (TotpNotEnabledException e) { + } catch (UnknownDeviceException e) { // User must have deleted the device in parallel. throw new UnknownDeviceException(); } @@ -393,7 +392,7 @@ public static void verifyCode(TenantIdentifierWithStorage tenantIdentifierWithSt // can ignore it. try{ checkAndStoreCode(tenantIdentifierWithStorage, main, userId, devices, code); - } catch (TotpNotEnabledException e) { + } catch (UnknownDeviceException e) { // User must have deleted the device in parallel // since they cannot un-verify a device (no API exists) throw new InvalidTotpException(); diff --git a/src/test/java/io/supertokens/test/StorageLayerTest.java b/src/test/java/io/supertokens/test/StorageLayerTest.java index ec49fd89b..efeff2407 100644 --- a/src/test/java/io/supertokens/test/StorageLayerTest.java +++ b/src/test/java/io/supertokens/test/StorageLayerTest.java @@ -11,7 +11,7 @@ import io.supertokens.pluginInterface.multitenancy.exceptions.TenantOrAppNotFoundException; import io.supertokens.pluginInterface.totp.TOTPDevice; import io.supertokens.pluginInterface.totp.TOTPUsedCode; -import io.supertokens.pluginInterface.totp.exception.TotpNotEnabledException; +import io.supertokens.pluginInterface.totp.exception.UnknownDeviceException; import io.supertokens.pluginInterface.totp.exception.UsedCodeAlreadyExistsException; import io.supertokens.pluginInterface.totp.sqlStorage.TOTPSQLStorage; import io.supertokens.storageLayer.StorageLayer; @@ -47,7 +47,7 @@ public static void insertUsedCodeUtil(TOTPSQLStorage storage, TOTPUsedCode usedC storage.insertUsedCode_Transaction(con, new TenantIdentifier(null, null, null), usedCode); storage.commitTransaction(con); return null; - } catch (TotpNotEnabledException | UsedCodeAlreadyExistsException e) { + } catch (UnknownDeviceException | UsedCodeAlreadyExistsException e) { throw new StorageTransactionLogicException(e); } catch (TenantOrAppNotFoundException e) { throw new IllegalStateException(e); @@ -55,7 +55,7 @@ public static void insertUsedCodeUtil(TOTPSQLStorage storage, TOTPUsedCode usedC }); } catch (StorageTransactionLogicException e) { Exception actual = e.actualException; - if (actual instanceof TotpNotEnabledException || actual instanceof UsedCodeAlreadyExistsException) { + if (actual instanceof UnknownDeviceException || actual instanceof UsedCodeAlreadyExistsException) { throw actual; } else { throw e; diff --git a/src/test/java/io/supertokens/test/totp/TOTPRecipeTest.java b/src/test/java/io/supertokens/test/totp/TOTPRecipeTest.java index 41bc3d2a6..091b2d5d3 100644 --- a/src/test/java/io/supertokens/test/totp/TOTPRecipeTest.java +++ b/src/test/java/io/supertokens/test/totp/TOTPRecipeTest.java @@ -23,18 +23,13 @@ import io.supertokens.cronjobs.deleteExpiredTotpTokens.DeleteExpiredTotpTokens; import io.supertokens.featureflag.EE_FEATURES; import io.supertokens.featureflag.FeatureFlagTestContent; -import io.supertokens.featureflag.exceptions.InvalidLicenseKeyException; -import io.supertokens.httpRequest.HttpResponseException; -import io.supertokens.pluginInterface.STORAGE_TYPE; import io.supertokens.pluginInterface.exceptions.StorageQueryException; import io.supertokens.pluginInterface.exceptions.StorageTransactionLogicException; import io.supertokens.pluginInterface.multitenancy.TenantIdentifier; -import io.supertokens.pluginInterface.STORAGE_TYPE; import io.supertokens.pluginInterface.totp.TOTPDevice; import io.supertokens.pluginInterface.totp.TOTPStorage; import io.supertokens.pluginInterface.totp.TOTPUsedCode; import io.supertokens.pluginInterface.totp.exception.DeviceAlreadyExistsException; -import io.supertokens.pluginInterface.totp.exception.TotpNotEnabledException; import io.supertokens.pluginInterface.totp.exception.UnknownDeviceException; import io.supertokens.pluginInterface.totp.sqlStorage.TOTPSQLStorage; import io.supertokens.storageLayer.StorageLayer; diff --git a/src/test/java/io/supertokens/test/totp/TOTPStorageTest.java b/src/test/java/io/supertokens/test/totp/TOTPStorageTest.java index ebd2bf133..9c1dd4ea1 100644 --- a/src/test/java/io/supertokens/test/totp/TOTPStorageTest.java +++ b/src/test/java/io/supertokens/test/totp/TOTPStorageTest.java @@ -89,7 +89,7 @@ private static TOTPUsedCode[] getAllUsedCodesUtil(TOTPStorage storage, String us } public static void insertUsedCodesUtil(TOTPSQLStorage storage, TOTPUsedCode[] usedCodes) - throws StorageQueryException, StorageTransactionLogicException, TotpNotEnabledException, + throws StorageQueryException, StorageTransactionLogicException, UnknownDeviceException, UsedCodeAlreadyExistsException { try { storage.startTransaction(con -> { @@ -97,7 +97,7 @@ public static void insertUsedCodesUtil(TOTPSQLStorage storage, TOTPUsedCode[] us for (TOTPUsedCode usedCode : usedCodes) { storage.insertUsedCode_Transaction(con, new TenantIdentifier(null, null, null), usedCode); } - } catch (TotpNotEnabledException | UsedCodeAlreadyExistsException e) { + } catch (UnknownDeviceException | UsedCodeAlreadyExistsException e) { throw new StorageTransactionLogicException(e); } catch (TenantOrAppNotFoundException e) { throw new IllegalStateException(e); @@ -108,8 +108,8 @@ public static void insertUsedCodesUtil(TOTPSQLStorage storage, TOTPUsedCode[] us }); } catch (StorageTransactionLogicException e) { Exception actual = e.actualException; - if (actual instanceof TotpNotEnabledException) { - throw (TotpNotEnabledException) actual; + if (actual instanceof UnknownDeviceException) { + throw (UnknownDeviceException) actual; } else if (actual instanceof UsedCodeAlreadyExistsException) { throw (UsedCodeAlreadyExistsException) actual; } @@ -404,7 +404,7 @@ public void insertUsedCodeTest() throws Exception { // Try to insert code when user doesn't have any device (i.e. TOTP not enabled) { - assertThrows(TotpNotEnabledException.class, + assertThrows(UnknownDeviceException.class, () -> insertUsedCodesUtil(storage, new TOTPUsedCode[]{ new TOTPUsedCode("new-user-without-totp", "1234", true, nextDay, System.currentTimeMillis()) @@ -423,7 +423,7 @@ public void insertUsedCodeTest() throws Exception { } // Try to insert code when user doesn't exist: - assertThrows(TotpNotEnabledException.class, + assertThrows(UnknownDeviceException.class, () -> insertUsedCodesUtil(storage, new TOTPUsedCode[]{ new TOTPUsedCode("non-existent-user", "1234", true, nextDay, System.currentTimeMillis()) From 157d94ace89cd7a485a56e442d1884cb601922d5 Mon Sep 17 00:00:00 2001 From: KShivendu Date: Tue, 27 Jun 2023 15:45:43 +0530 Subject: [PATCH 05/16] refactor: Remove Totp not enabled error --- src/test/java/io/supertokens/test/totp/TOTPStorageTest.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/test/java/io/supertokens/test/totp/TOTPStorageTest.java b/src/test/java/io/supertokens/test/totp/TOTPStorageTest.java index 9c1dd4ea1..41a0e878c 100644 --- a/src/test/java/io/supertokens/test/totp/TOTPStorageTest.java +++ b/src/test/java/io/supertokens/test/totp/TOTPStorageTest.java @@ -14,7 +14,6 @@ import io.supertokens.pluginInterface.totp.TOTPStorage; import io.supertokens.pluginInterface.totp.TOTPUsedCode; import io.supertokens.pluginInterface.totp.exception.DeviceAlreadyExistsException; -import io.supertokens.pluginInterface.totp.exception.TotpNotEnabledException; import io.supertokens.pluginInterface.totp.exception.UnknownDeviceException; import io.supertokens.pluginInterface.totp.exception.UsedCodeAlreadyExistsException; import io.supertokens.pluginInterface.totp.sqlStorage.TOTPSQLStorage; @@ -27,8 +26,6 @@ import org.junit.Test; import org.junit.rules.TestRule; -import java.io.IOException; - import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertThrows; From fc9f7060e48d9e42d12e646e157770d51fa6cf8d Mon Sep 17 00:00:00 2001 From: KShivendu Date: Tue, 27 Jun 2023 16:21:51 +0530 Subject: [PATCH 06/16] feat: Make device name optional and generate it from number of existing devices --- src/main/java/io/supertokens/totp/Totp.java | 5 ++++ .../api/totp/CreateOrUpdateTotpDeviceAPI.java | 6 +++-- .../totp/api/CreateTotpDeviceAPITest.java | 24 +++++++++++++++---- 3 files changed, 28 insertions(+), 7 deletions(-) diff --git a/src/main/java/io/supertokens/totp/Totp.java b/src/main/java/io/supertokens/totp/Totp.java index c991062da..4d556d34b 100644 --- a/src/main/java/io/supertokens/totp/Totp.java +++ b/src/main/java/io/supertokens/totp/Totp.java @@ -106,6 +106,11 @@ public static TOTPDevice registerDevice(AppIdentifierWithStorage appIdentifierWi } TOTPSQLStorage totpStorage = appIdentifierWithStorage.getTOTPStorage(); + + if (deviceName == null) { + TOTPDevice[] devices = totpStorage.getDevices(appIdentifierWithStorage, userId); + deviceName = "TOTP Device " + (devices.length + 1); + } String secret = generateSecret(); TOTPDevice device = new TOTPDevice(userId, deviceName, secret, period, skew, false); diff --git a/src/main/java/io/supertokens/webserver/api/totp/CreateOrUpdateTotpDeviceAPI.java b/src/main/java/io/supertokens/webserver/api/totp/CreateOrUpdateTotpDeviceAPI.java index 4a6e2c03d..5cca883c1 100644 --- a/src/main/java/io/supertokens/webserver/api/totp/CreateOrUpdateTotpDeviceAPI.java +++ b/src/main/java/io/supertokens/webserver/api/totp/CreateOrUpdateTotpDeviceAPI.java @@ -41,7 +41,7 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws I JsonObject input = InputParser.parseJsonObjectOrThrowError(req); String userId = InputParser.parseStringOrThrowError(input, "userId", false); - String deviceName = InputParser.parseStringOrThrowError(input, "deviceName", false); + String deviceName = InputParser.parseStringOrThrowError(input, "deviceName", true); Integer skew = InputParser.parseIntOrThrowError(input, "skew", false); Integer period = InputParser.parseIntOrThrowError(input, "period", false); @@ -51,7 +51,8 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws I if (userId.isEmpty()) { throw new ServletException(new BadRequestException("userId cannot be empty")); } - if (deviceName.isEmpty()) { + if (deviceName != null && deviceName.isEmpty()) { + // Only Null or valid device name are allowed throw new ServletException(new BadRequestException("deviceName cannot be empty")); } if (skew < 0) { @@ -86,6 +87,7 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws I TOTPDevice device = Totp.registerDevice(appIdentifierWithStorage, main, userId, deviceName, skew, period); result.addProperty("status", "OK"); + result.addProperty("deviceName", device.deviceName); result.addProperty("secret", device.secretKey); super.sendJsonResponse(200, result, resp); } catch (DeviceAlreadyExistsException e) { diff --git a/src/test/java/io/supertokens/test/totp/api/CreateTotpDeviceAPITest.java b/src/test/java/io/supertokens/test/totp/api/CreateTotpDeviceAPITest.java index 3c181fbaa..112c57163 100644 --- a/src/test/java/io/supertokens/test/totp/api/CreateTotpDeviceAPITest.java +++ b/src/test/java/io/supertokens/test/totp/api/CreateTotpDeviceAPITest.java @@ -88,17 +88,15 @@ public void testApi() throws Exception { JsonObject body = new JsonObject(); - // Missing userId/deviceName/skew/period + // Missing userId/skew/period { Exception e = createDeviceRequest(process, body); checkFieldMissingErrorResponse(e, "userId"); + + body.addProperty("deviceName", ""); body.addProperty("userId", ""); e = createDeviceRequest(process, body); - checkFieldMissingErrorResponse(e, "deviceName"); - - body.addProperty("deviceName", ""); - e = createDeviceRequest(process, body); checkFieldMissingErrorResponse(e, "skew"); body.addProperty("skew", -1); @@ -138,6 +136,7 @@ public void testApi() throws Exception { Utils.getCdiVersionStringLatestForTests(), "totp"); assert res.get("status").getAsString().equals("OK"); + assert res.get("deviceName").getAsString().equals("d1"); // try again with same device: JsonObject res2 = HttpRequestForTesting.sendJsonPOSTRequest( @@ -151,6 +150,21 @@ public void testApi() throws Exception { Utils.getCdiVersionStringLatestForTests(), "totp"); assert res2.get("status").getAsString().equals("DEVICE_ALREADY_EXISTS_ERROR"); + + // try without passing deviceName: + body.remove("deviceName"); + JsonObject res3 = HttpRequestForTesting.sendJsonPOSTRequest( + process.getProcess(), + "", + "http://localhost:3567/recipe/totp/device", + body, + 1000, + 1000, + null, + Utils.getCdiVersionStringLatestForTests(), + "totp"); + assert res3.get("status").getAsString().equals("OK"); + assert res3.get("deviceName").getAsString().equals("TOTP Device 2"); } process.kill(); From ec6dd7e5cd54a4ae4ab297714fbcecd40e238125 Mon Sep 17 00:00:00 2001 From: KShivendu Date: Wed, 28 Jun 2023 15:14:51 +0530 Subject: [PATCH 07/16] Replace TotpNotEnabledError with UnknownUserIdTotpError --- .../java/io/supertokens/inmemorydb/Start.java | 5 +++-- src/main/java/io/supertokens/totp/Totp.java | 15 ++++++++------- .../io/supertokens/test/StorageLayerTest.java | 3 ++- .../io/supertokens/test/totp/TOTPStorageTest.java | 3 ++- 4 files changed, 15 insertions(+), 11 deletions(-) diff --git a/src/main/java/io/supertokens/inmemorydb/Start.java b/src/main/java/io/supertokens/inmemorydb/Start.java index 301fc7010..e8561ceff 100644 --- a/src/main/java/io/supertokens/inmemorydb/Start.java +++ b/src/main/java/io/supertokens/inmemorydb/Start.java @@ -69,6 +69,7 @@ import io.supertokens.pluginInterface.totp.TOTPUsedCode; import io.supertokens.pluginInterface.totp.exception.DeviceAlreadyExistsException; import io.supertokens.pluginInterface.totp.exception.UnknownDeviceException; +import io.supertokens.pluginInterface.totp.exception.UnknownUserIdTotpException; import io.supertokens.pluginInterface.totp.exception.UsedCodeAlreadyExistsException; import io.supertokens.pluginInterface.totp.sqlStorage.TOTPSQLStorage; import io.supertokens.pluginInterface.useridmapping.UserIdMapping; @@ -2672,7 +2673,7 @@ public TOTPDevice[] getDevices_Transaction(TransactionConnection con, AppIdentif @Override public void insertUsedCode_Transaction(TransactionConnection con, TenantIdentifier tenantIdentifier, TOTPUsedCode usedCodeObj) - throws StorageQueryException, UnknownDeviceException, UsedCodeAlreadyExistsException, + throws StorageQueryException, UnknownUserIdTotpException, UsedCodeAlreadyExistsException, TenantOrAppNotFoundException { Connection sqlCon = (Connection) con.getConnection(); try { @@ -2686,7 +2687,7 @@ public void insertUsedCode_Transaction(TransactionConnection con, TenantIdentifi Config.getConfig(this).getTotpUsersTable(), new String[]{"app_id", "user_id"}, new Object[]{tenantIdentifier.getAppId(), usedCodeObj.userId})) { - throw new UnknownDeviceException(); + throw new UnknownUserIdTotpException(); } else if (isForeignKeyConstraintError( e.getMessage(), diff --git a/src/main/java/io/supertokens/totp/Totp.java b/src/main/java/io/supertokens/totp/Totp.java index 4d556d34b..deab16094 100644 --- a/src/main/java/io/supertokens/totp/Totp.java +++ b/src/main/java/io/supertokens/totp/Totp.java @@ -16,6 +16,7 @@ import io.supertokens.pluginInterface.totp.TOTPUsedCode; import io.supertokens.pluginInterface.totp.exception.DeviceAlreadyExistsException; import io.supertokens.pluginInterface.totp.exception.UnknownDeviceException; +import io.supertokens.pluginInterface.totp.exception.UnknownUserIdTotpException; import io.supertokens.pluginInterface.totp.exception.UsedCodeAlreadyExistsException; import io.supertokens.pluginInterface.totp.sqlStorage.TOTPSQLStorage; import io.supertokens.storageLayer.StorageLayer; @@ -106,7 +107,7 @@ public static TOTPDevice registerDevice(AppIdentifierWithStorage appIdentifierWi } TOTPSQLStorage totpStorage = appIdentifierWithStorage.getTOTPStorage(); - + if (deviceName == null) { TOTPDevice[] devices = totpStorage.getDevices(appIdentifierWithStorage, userId); deviceName = "TOTP Device " + (devices.length + 1); @@ -122,7 +123,7 @@ public static TOTPDevice registerDevice(AppIdentifierWithStorage appIdentifierWi private static void checkAndStoreCode(TenantIdentifierWithStorage tenantIdentifierWithStorage, Main main, String userId, TOTPDevice[] devices, String code) - throws InvalidTotpException, UnknownDeviceException, + throws InvalidTotpException, UnknownUserIdTotpException, LimitReachedException, StorageQueryException, StorageTransactionLogicException, TenantOrAppNotFoundException { // Note that the TOTP cron runs every 1 hour, so all the expired tokens can stay @@ -245,7 +246,7 @@ private static void checkAndStoreCode(TenantIdentifierWithStorage tenantIdentifi try { totpSQLStorage.insertUsedCode_Transaction(con, tenantIdentifierWithStorage, newCode); totpSQLStorage.commitTransaction(con); - } catch (UsedCodeAlreadyExistsException | UnknownDeviceException e) { + } catch (UsedCodeAlreadyExistsException | UnknownUserIdTotpException e) { throw new StorageTransactionLogicException(e); } @@ -268,8 +269,8 @@ private static void checkAndStoreCode(TenantIdentifierWithStorage tenantIdentifi throw (LimitReachedException) e.actualException; } else if (e.actualException instanceof InvalidTotpException) { throw (InvalidTotpException) e.actualException; - } else if (e.actualException instanceof UnknownDeviceException) { - throw (UnknownDeviceException) e.actualException; + } else if (e.actualException instanceof UnknownUserIdTotpException) { + throw (UnknownUserIdTotpException) e.actualException; } else if (e.actualException instanceof UsedCodeAlreadyExistsException) { // retry the transaction after a small delay: int delayInMs = (int) (Math.random() * 10 + 1); @@ -343,7 +344,7 @@ public static boolean verifyDevice(TenantIdentifierWithStorage tenantIdentifierW // This behaviour is okay so we can ignore it. try{ checkAndStoreCode(tenantIdentifierWithStorage, main, userId, new TOTPDevice[]{matchingDevice}, code); - } catch (UnknownDeviceException e) { + } catch (UnknownUserIdTotpException e) { // User must have deleted the device in parallel. throw new UnknownDeviceException(); } @@ -397,7 +398,7 @@ public static void verifyCode(TenantIdentifierWithStorage tenantIdentifierWithSt // can ignore it. try{ checkAndStoreCode(tenantIdentifierWithStorage, main, userId, devices, code); - } catch (UnknownDeviceException e) { + } catch (UnknownUserIdTotpException e) { // User must have deleted the device in parallel // since they cannot un-verify a device (no API exists) throw new InvalidTotpException(); diff --git a/src/test/java/io/supertokens/test/StorageLayerTest.java b/src/test/java/io/supertokens/test/StorageLayerTest.java index efeff2407..9ff015a38 100644 --- a/src/test/java/io/supertokens/test/StorageLayerTest.java +++ b/src/test/java/io/supertokens/test/StorageLayerTest.java @@ -12,6 +12,7 @@ import io.supertokens.pluginInterface.totp.TOTPDevice; import io.supertokens.pluginInterface.totp.TOTPUsedCode; import io.supertokens.pluginInterface.totp.exception.UnknownDeviceException; +import io.supertokens.pluginInterface.totp.exception.UnknownUserIdTotpException; import io.supertokens.pluginInterface.totp.exception.UsedCodeAlreadyExistsException; import io.supertokens.pluginInterface.totp.sqlStorage.TOTPSQLStorage; import io.supertokens.storageLayer.StorageLayer; @@ -47,7 +48,7 @@ public static void insertUsedCodeUtil(TOTPSQLStorage storage, TOTPUsedCode usedC storage.insertUsedCode_Transaction(con, new TenantIdentifier(null, null, null), usedCode); storage.commitTransaction(con); return null; - } catch (UnknownDeviceException | UsedCodeAlreadyExistsException e) { + } catch (UnknownUserIdTotpException | UsedCodeAlreadyExistsException e) { throw new StorageTransactionLogicException(e); } catch (TenantOrAppNotFoundException e) { throw new IllegalStateException(e); diff --git a/src/test/java/io/supertokens/test/totp/TOTPStorageTest.java b/src/test/java/io/supertokens/test/totp/TOTPStorageTest.java index 41a0e878c..f2f34f9ab 100644 --- a/src/test/java/io/supertokens/test/totp/TOTPStorageTest.java +++ b/src/test/java/io/supertokens/test/totp/TOTPStorageTest.java @@ -15,6 +15,7 @@ import io.supertokens.pluginInterface.totp.TOTPUsedCode; import io.supertokens.pluginInterface.totp.exception.DeviceAlreadyExistsException; import io.supertokens.pluginInterface.totp.exception.UnknownDeviceException; +import io.supertokens.pluginInterface.totp.exception.UnknownUserIdTotpException; import io.supertokens.pluginInterface.totp.exception.UsedCodeAlreadyExistsException; import io.supertokens.pluginInterface.totp.sqlStorage.TOTPSQLStorage; import io.supertokens.storageLayer.StorageLayer; @@ -94,7 +95,7 @@ public static void insertUsedCodesUtil(TOTPSQLStorage storage, TOTPUsedCode[] us for (TOTPUsedCode usedCode : usedCodes) { storage.insertUsedCode_Transaction(con, new TenantIdentifier(null, null, null), usedCode); } - } catch (UnknownDeviceException | UsedCodeAlreadyExistsException e) { + } catch (UnknownUserIdTotpException | UsedCodeAlreadyExistsException e) { throw new StorageTransactionLogicException(e); } catch (TenantOrAppNotFoundException e) { throw new IllegalStateException(e); From 98b24ae661ae8631e3cb09e6a615188abf5170da Mon Sep 17 00:00:00 2001 From: KShivendu Date: Thu, 29 Jun 2023 13:38:16 +0530 Subject: [PATCH 08/16] refactor: Recursively generate device name when it already exists --- src/main/java/io/supertokens/totp/Totp.java | 99 +++++++++++++------ .../api/totp/CreateOrUpdateTotpDeviceAPI.java | 3 +- 2 files changed, 69 insertions(+), 33 deletions(-) diff --git a/src/main/java/io/supertokens/totp/Totp.java b/src/main/java/io/supertokens/totp/Totp.java index deab16094..aa4d952a5 100644 --- a/src/main/java/io/supertokens/totp/Totp.java +++ b/src/main/java/io/supertokens/totp/Totp.java @@ -81,12 +81,11 @@ private static boolean isTotpEnabled(AppIdentifier appIdentifier, Main main) return false; } - @TestOnly public static TOTPDevice registerDevice(Main main, String userId, - String deviceName, int skew, int period) + String deviceName, int skew, int period) throws StorageQueryException, DeviceAlreadyExistsException, NoSuchAlgorithmException, - FeatureNotEnabledException { + FeatureNotEnabledException, StorageTransactionLogicException { try { return registerDevice(new AppIdentifierWithStorage(null, null, StorageLayer.getStorage(main)), main, userId, deviceName, skew, period); @@ -95,10 +94,42 @@ public static TOTPDevice registerDevice(Main main, String userId, } } + private static TOTPDevice registerDeviceRecursive(AppIdentifierWithStorage appIdentifierWithStorage, TOTPSQLStorage totpStorage, String userId, TOTPDevice device, int counter) throws StorageQueryException, DeviceAlreadyExistsException, TenantOrAppNotFoundException, StorageTransactionLogicException { + try { + TOTPDevice d = new TOTPDevice(device.userId, "TOTP Device " + (counter + 1), device.secretKey, device.period, device.skew, false); + totpStorage.createDevice(appIdentifierWithStorage, d); + return d; + } catch (DeviceAlreadyExistsException e) { + TOTPDevice[] devices = totpStorage.getDevices(appIdentifierWithStorage, userId); + // iterate through all devices to find device with same name + TOTPDevice existingDevice = Arrays.stream(devices).filter(d -> d.deviceName.equals(device.deviceName)) + .findFirst().orElse(null); + + if (existingDevice != null) { + if (existingDevice.verified) { + // device with same name exists and is verified + // TODO: Should this recursion have a limit? + return registerDeviceRecursive(appIdentifierWithStorage, totpStorage, userId, device, ++counter); + } else { + // device with same name exists but is not verified + // delete the device and retry + totpStorage.startTransaction(con -> { + totpStorage.deleteDevice_Transaction(con, appIdentifierWithStorage, userId, device.deviceName); + totpStorage.commitTransaction(con); + return null; + }); + // TODO: Should this recursion have a limit? + return registerDeviceRecursive(appIdentifierWithStorage, totpStorage, userId, device, counter); + } + } + throw e; + } + } + public static TOTPDevice registerDevice(AppIdentifierWithStorage appIdentifierWithStorage, Main main, String userId, - String deviceName, int skew, int period) + String deviceName, int skew, int period) throws StorageQueryException, DeviceAlreadyExistsException, NoSuchAlgorithmException, - FeatureNotEnabledException, TenantOrAppNotFoundException { + FeatureNotEnabledException, TenantOrAppNotFoundException, StorageTransactionLogicException { if (!isTotpEnabled(appIdentifierWithStorage, main)) { throw new FeatureNotEnabledException( @@ -107,22 +138,25 @@ public static TOTPDevice registerDevice(AppIdentifierWithStorage appIdentifierWi } TOTPSQLStorage totpStorage = appIdentifierWithStorage.getTOTPStorage(); + String secret = generateSecret(); + TOTPDevice device = new TOTPDevice(userId, deviceName, secret, period, skew, false); - if (deviceName == null) { - TOTPDevice[] devices = totpStorage.getDevices(appIdentifierWithStorage, userId); - deviceName = "TOTP Device " + (devices.length + 1); + if (deviceName != null) { + totpStorage.createDevice(appIdentifierWithStorage, device); + return device; } - String secret = generateSecret(); - TOTPDevice device = new TOTPDevice(userId, deviceName, secret, period, skew, false); - totpStorage.createDevice(appIdentifierWithStorage, device); + // Find number of existing devices to set device name + TOTPDevice[] devices = totpStorage.getDevices(appIdentifierWithStorage, userId); + int verifiedDevicesCount = Arrays.stream(devices).filter(d -> d.verified).toArray().length; + // device.deviceName = "TOTP Device " + (verifiedDevicesCount + 1); - return device; + return registerDeviceRecursive(appIdentifierWithStorage, totpStorage, userId, device, verifiedDevicesCount); } private static void checkAndStoreCode(TenantIdentifierWithStorage tenantIdentifierWithStorage, Main main, - String userId, TOTPDevice[] devices, - String code) + String userId, TOTPDevice[] devices, + String code) throws InvalidTotpException, UnknownUserIdTotpException, LimitReachedException, StorageQueryException, StorageTransactionLogicException, TenantOrAppNotFoundException { @@ -174,9 +208,9 @@ private static void checkAndStoreCode(TenantIdentifierWithStorage tenantIdentifi // Count # of contiguous invalids in latest N attempts (stop at first valid): long invalidOutOfN = Arrays.stream(usedCodes).limit(N).takeWhile(usedCode -> !usedCode.isValid) .count(); - int rateLimitResetTimeInMs = - Config.getConfig(tenantIdentifierWithStorage, main).getTotpRateLimitCooldownTimeSec() * - 1000; // (Default + int rateLimitResetTimeInMs = Config.getConfig(tenantIdentifierWithStorage, main) + .getTotpRateLimitCooldownTimeSec() * + 1000; // (Default // 15 mins) // Check if the user has been rate limited: @@ -235,9 +269,9 @@ private static void checkAndStoreCode(TenantIdentifierWithStorage tenantIdentifi .mapToInt(device -> device.period * (2 * device.skew + 1)) .max() .orElse(0); - int expireInSec = - (matchingDevice != null) ? matchingDevice.period * (2 * matchingDevice.skew + 1) - : maxUsedCodeExpiry; + int expireInSec = (matchingDevice != null) + ? matchingDevice.period * (2 * matchingDevice.skew + 1) + : maxUsedCodeExpiry; long now = System.currentTimeMillis(); TOTPUsedCode newCode = new TOTPUsedCode(userId, @@ -288,7 +322,7 @@ private static void checkAndStoreCode(TenantIdentifierWithStorage tenantIdentifi @TestOnly public static boolean verifyDevice(Main main, - String userId, String deviceName, String code) + String userId, String deviceName, String code) throws UnknownDeviceException, InvalidTotpException, LimitReachedException, StorageQueryException, StorageTransactionLogicException { try { @@ -300,7 +334,7 @@ public static boolean verifyDevice(Main main, } public static boolean verifyDevice(TenantIdentifierWithStorage tenantIdentifierWithStorage, Main main, - String userId, String deviceName, String code) + String userId, String deviceName, String code) throws UnknownDeviceException, InvalidTotpException, LimitReachedException, StorageQueryException, StorageTransactionLogicException, TenantOrAppNotFoundException { @@ -342,8 +376,8 @@ public static boolean verifyDevice(TenantIdentifierWithStorage tenantIdentifierW // verified in the devices table (because it was deleted/renamed). So the user // gets a UnknownDevceException. // This behaviour is okay so we can ignore it. - try{ - checkAndStoreCode(tenantIdentifierWithStorage, main, userId, new TOTPDevice[]{matchingDevice}, code); + try { + checkAndStoreCode(tenantIdentifierWithStorage, main, userId, new TOTPDevice[] { matchingDevice }, code); } catch (UnknownUserIdTotpException e) { // User must have deleted the device in parallel. throw new UnknownDeviceException(); @@ -355,7 +389,7 @@ public static boolean verifyDevice(TenantIdentifierWithStorage tenantIdentifierW @TestOnly public static void verifyCode(Main main, String userId, - String code, boolean allowUnverifiedDevices) + String code, boolean allowUnverifiedDevices) throws InvalidTotpException, LimitReachedException, StorageQueryException, StorageTransactionLogicException, FeatureNotEnabledException { try { @@ -367,7 +401,7 @@ public static void verifyCode(Main main, String userId, } public static void verifyCode(TenantIdentifierWithStorage tenantIdentifierWithStorage, Main main, String userId, - String code, boolean allowUnverifiedDevices) + String code, boolean allowUnverifiedDevices) throws InvalidTotpException, LimitReachedException, StorageQueryException, StorageTransactionLogicException, FeatureNotEnabledException, TenantOrAppNotFoundException { @@ -396,7 +430,7 @@ public static void verifyCode(TenantIdentifierWithStorage tenantIdentifierWithSt // another API call. We will still check the code against the updated set of // devices and store it in the used codes table. This behaviour is okay so we // can ignore it. - try{ + try { checkAndStoreCode(tenantIdentifierWithStorage, main, userId, devices, code); } catch (UnknownUserIdTotpException e) { // User must have deleted the device in parallel @@ -407,7 +441,7 @@ public static void verifyCode(TenantIdentifierWithStorage tenantIdentifierWithSt @TestOnly public static void removeDevice(Main main, String userId, - String deviceName) + String deviceName) throws StorageQueryException, UnknownDeviceException, StorageTransactionLogicException { try { @@ -422,7 +456,7 @@ public static void removeDevice(Main main, String userId, * Delete device and also delete the user if deleting the last device */ public static void removeDevice(AppIdentifierWithStorage appIdentifierWithStorage, String userId, - String deviceName) + String deviceName) throws StorageQueryException, UnknownDeviceException, StorageTransactionLogicException, TenantOrAppNotFoundException { TOTPSQLStorage storage = appIdentifierWithStorage.getTOTPStorage(); @@ -457,7 +491,7 @@ public static void removeDevice(AppIdentifierWithStorage appIdentifierWithStorag @TestOnly public static void updateDeviceName(Main main, String userId, - String oldDeviceName, String newDeviceName) + String oldDeviceName, String newDeviceName) throws StorageQueryException, DeviceAlreadyExistsException, UnknownDeviceException { try { updateDeviceName(new AppIdentifierWithStorage(null, null, StorageLayer.getStorage(main)), @@ -468,8 +502,9 @@ public static void updateDeviceName(Main main, String userId, } public static void updateDeviceName(AppIdentifierWithStorage appIdentifierWithStorage, String userId, - String oldDeviceName, String newDeviceName) - throws StorageQueryException, DeviceAlreadyExistsException, UnknownDeviceException, TenantOrAppNotFoundException { + String oldDeviceName, String newDeviceName) + throws StorageQueryException, DeviceAlreadyExistsException, UnknownDeviceException, + TenantOrAppNotFoundException { TOTPSQLStorage totpStorage = appIdentifierWithStorage.getTOTPStorage(); totpStorage.updateDeviceName(appIdentifierWithStorage, userId, oldDeviceName, newDeviceName); } diff --git a/src/main/java/io/supertokens/webserver/api/totp/CreateOrUpdateTotpDeviceAPI.java b/src/main/java/io/supertokens/webserver/api/totp/CreateOrUpdateTotpDeviceAPI.java index 5cca883c1..01bc2c28a 100644 --- a/src/main/java/io/supertokens/webserver/api/totp/CreateOrUpdateTotpDeviceAPI.java +++ b/src/main/java/io/supertokens/webserver/api/totp/CreateOrUpdateTotpDeviceAPI.java @@ -7,6 +7,7 @@ import io.supertokens.pluginInterface.RECIPE_ID; import io.supertokens.pluginInterface.emailpassword.exceptions.UnknownUserIdException; import io.supertokens.pluginInterface.exceptions.StorageQueryException; +import io.supertokens.pluginInterface.exceptions.StorageTransactionLogicException; import io.supertokens.pluginInterface.multitenancy.AppIdentifierWithStorage; import io.supertokens.pluginInterface.multitenancy.exceptions.TenantOrAppNotFoundException; import io.supertokens.pluginInterface.totp.TOTPDevice; @@ -94,7 +95,7 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws I result.addProperty("status", "DEVICE_ALREADY_EXISTS_ERROR"); super.sendJsonResponse(200, result, resp); } catch (StorageQueryException | NoSuchAlgorithmException | FeatureNotEnabledException | - TenantOrAppNotFoundException e) { + TenantOrAppNotFoundException | StorageTransactionLogicException e) { throw new ServletException(e); } } From 646b0b81840fa8ac0865defc23a998196cc88cd5 Mon Sep 17 00:00:00 2001 From: KShivendu Date: Thu, 29 Jun 2023 13:47:44 +0530 Subject: [PATCH 09/16] refactor: Remove redundant arguments --- src/main/java/io/supertokens/totp/Totp.java | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/main/java/io/supertokens/totp/Totp.java b/src/main/java/io/supertokens/totp/Totp.java index aa4d952a5..fcfdde098 100644 --- a/src/main/java/io/supertokens/totp/Totp.java +++ b/src/main/java/io/supertokens/totp/Totp.java @@ -94,13 +94,14 @@ public static TOTPDevice registerDevice(Main main, String userId, } } - private static TOTPDevice registerDeviceRecursive(AppIdentifierWithStorage appIdentifierWithStorage, TOTPSQLStorage totpStorage, String userId, TOTPDevice device, int counter) throws StorageQueryException, DeviceAlreadyExistsException, TenantOrAppNotFoundException, StorageTransactionLogicException { + private static TOTPDevice registerDeviceRecursive(AppIdentifierWithStorage appIdentifierWithStorage, TOTPDevice device, int counter) throws StorageQueryException, DeviceAlreadyExistsException, TenantOrAppNotFoundException, StorageTransactionLogicException { + TOTPSQLStorage totpStorage = appIdentifierWithStorage.getTOTPStorage(); try { TOTPDevice d = new TOTPDevice(device.userId, "TOTP Device " + (counter + 1), device.secretKey, device.period, device.skew, false); totpStorage.createDevice(appIdentifierWithStorage, d); return d; } catch (DeviceAlreadyExistsException e) { - TOTPDevice[] devices = totpStorage.getDevices(appIdentifierWithStorage, userId); + TOTPDevice[] devices = totpStorage.getDevices(appIdentifierWithStorage, device.userId); // iterate through all devices to find device with same name TOTPDevice existingDevice = Arrays.stream(devices).filter(d -> d.deviceName.equals(device.deviceName)) .findFirst().orElse(null); @@ -109,17 +110,17 @@ private static TOTPDevice registerDeviceRecursive(AppIdentifierWithStorage appId if (existingDevice.verified) { // device with same name exists and is verified // TODO: Should this recursion have a limit? - return registerDeviceRecursive(appIdentifierWithStorage, totpStorage, userId, device, ++counter); + return registerDeviceRecursive(appIdentifierWithStorage, device, ++counter); } else { // device with same name exists but is not verified // delete the device and retry totpStorage.startTransaction(con -> { - totpStorage.deleteDevice_Transaction(con, appIdentifierWithStorage, userId, device.deviceName); + totpStorage.deleteDevice_Transaction(con, appIdentifierWithStorage, device.userId, device.deviceName); totpStorage.commitTransaction(con); return null; }); // TODO: Should this recursion have a limit? - return registerDeviceRecursive(appIdentifierWithStorage, totpStorage, userId, device, counter); + return registerDeviceRecursive(appIdentifierWithStorage, device, counter); } } throw e; @@ -137,9 +138,9 @@ public static TOTPDevice registerDevice(AppIdentifierWithStorage appIdentifierWi "feature."); } - TOTPSQLStorage totpStorage = appIdentifierWithStorage.getTOTPStorage(); String secret = generateSecret(); TOTPDevice device = new TOTPDevice(userId, deviceName, secret, period, skew, false); + TOTPSQLStorage totpStorage = appIdentifierWithStorage.getTOTPStorage(); if (deviceName != null) { totpStorage.createDevice(appIdentifierWithStorage, device); @@ -151,7 +152,7 @@ public static TOTPDevice registerDevice(AppIdentifierWithStorage appIdentifierWi int verifiedDevicesCount = Arrays.stream(devices).filter(d -> d.verified).toArray().length; // device.deviceName = "TOTP Device " + (verifiedDevicesCount + 1); - return registerDeviceRecursive(appIdentifierWithStorage, totpStorage, userId, device, verifiedDevicesCount); + return registerDeviceRecursive(appIdentifierWithStorage, device, verifiedDevicesCount); } private static void checkAndStoreCode(TenantIdentifierWithStorage tenantIdentifierWithStorage, Main main, From 430704e02c94d9c1d20b5da096e33c649593a430 Mon Sep 17 00:00:00 2001 From: KShivendu Date: Mon, 3 Jul 2023 11:31:38 +0530 Subject: [PATCH 10/16] feat: Remove the param to allow unverified devices from the verify totp API --- .../webserver/api/totp/VerifyTotpAPI.java | 4 +--- .../test/totp/api/VerifyTotpAPITest.java | 20 ++++++++----------- 2 files changed, 9 insertions(+), 15 deletions(-) diff --git a/src/main/java/io/supertokens/webserver/api/totp/VerifyTotpAPI.java b/src/main/java/io/supertokens/webserver/api/totp/VerifyTotpAPI.java index 436b9f0d2..1c2e08eac 100644 --- a/src/main/java/io/supertokens/webserver/api/totp/VerifyTotpAPI.java +++ b/src/main/java/io/supertokens/webserver/api/totp/VerifyTotpAPI.java @@ -42,7 +42,6 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws I String userId = InputParser.parseStringOrThrowError(input, "userId", false); String totp = InputParser.parseStringOrThrowError(input, "totp", false); - Boolean allowUnverifiedDevices = InputParser.parseBooleanOrThrowError(input, "allowUnverifiedDevices", false); if (userId.isEmpty()) { throw new ServletException(new BadRequestException("userId cannot be empty")); @@ -50,7 +49,6 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws I if (totp.length() != 6) { throw new ServletException(new BadRequestException("totp must be 6 characters long")); } - // Already checked that allowUnverifiedDevices is not null. JsonObject result = new JsonObject(); @@ -72,7 +70,7 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws I tenantIdentifierWithStorage = getTenantIdentifierWithStorageFromRequest(req); } - Totp.verifyCode(tenantIdentifierWithStorage, main, userId, totp, allowUnverifiedDevices); + Totp.verifyCode(tenantIdentifierWithStorage, main, userId, totp, false); result.addProperty("status", "OK"); super.sendJsonResponse(200, result, resp); diff --git a/src/test/java/io/supertokens/test/totp/api/VerifyTotpAPITest.java b/src/test/java/io/supertokens/test/totp/api/VerifyTotpAPITest.java index d4e21069b..e39f1bdf4 100644 --- a/src/test/java/io/supertokens/test/totp/api/VerifyTotpAPITest.java +++ b/src/test/java/io/supertokens/test/totp/api/VerifyTotpAPITest.java @@ -40,7 +40,7 @@ public void beforeEach() { Utils.reset(); } - private Exception updateDeviceRequest(TestingProcessManager.TestingProcess process, JsonObject body) { + private Exception verifyTotpCodeRequest(TestingProcessManager.TestingProcess process, JsonObject body) { return assertThrows( io.supertokens.test.httpRequest.HttpResponseException.class, () -> HttpRequestForTesting.sendJsonPOSTRequest( @@ -117,36 +117,32 @@ public void testApi() throws Exception { // Missing userId/deviceName/skew/period { - Exception e = updateDeviceRequest(process, body); + Exception e = verifyTotpCodeRequest(process, body); checkFieldMissingErrorResponse(e, "userId"); body.addProperty("userId", ""); - e = updateDeviceRequest(process, body); + e = verifyTotpCodeRequest(process, body); checkFieldMissingErrorResponse(e, "totp"); - - body.addProperty("totp", ""); - e = updateDeviceRequest(process, body); - checkFieldMissingErrorResponse(e, "allowUnverifiedDevices"); } // Invalid userId/deviceName/skew/period { - body.addProperty("allowUnverifiedDevices", true); - Exception e = updateDeviceRequest(process, body); + body.addProperty("totp", ""); + Exception e = verifyTotpCodeRequest(process, body); checkResponseErrorContains(e, "userId cannot be empty"); // Note that this is not a field missing error body.addProperty("userId", device.userId); - e = updateDeviceRequest(process, body); + e = verifyTotpCodeRequest(process, body); checkResponseErrorContains(e, "totp must be 6 characters long"); // test totp of length 5: body.addProperty("totp", "12345"); - e = updateDeviceRequest(process, body); + e = verifyTotpCodeRequest(process, body); checkResponseErrorContains(e, "totp must be 6 characters long"); // test totp of length 8: body.addProperty("totp", "12345678"); - e = updateDeviceRequest(process, body); + e = verifyTotpCodeRequest(process, body); checkResponseErrorContains(e, "totp must be 6 characters long"); // but let's pass invalid code first From 363492c5bbf1fe8add055ba51f1e2edcc2ec4ee4 Mon Sep 17 00:00:00 2001 From: KShivendu Date: Wed, 27 Sep 2023 14:40:33 +0530 Subject: [PATCH 11/16] feat: Reject unverified devices --- .../queries/ActiveUsersQueries.java | 2 +- src/main/java/io/supertokens/totp/Totp.java | 36 +++---- .../webserver/api/totp/VerifyTotpAPI.java | 2 +- .../io/supertokens/test/FeatureFlagTest.java | 1 + .../supertokens/test/mfa/MfaStorageTest.java | 4 +- .../test/mfa/api/MfaUserIdMappingTest.java | 6 +- .../test/multitenant/TestAppData.java | 4 +- .../supertokens/test/totp/TOTPRecipeTest.java | 102 ++++++++++-------- .../test/totp/TOTPStorageTest.java | 8 +- .../test/totp/TotpLicenseTest.java | 10 +- .../totp/api/CreateTotpDeviceAPITest.java | 64 ++++++++++- .../test/totp/api/TotpUserIdMappingTest.java | 46 +++++--- .../test/totp/api/VerifyTotpAPITest.java | 27 ++++- 13 files changed, 210 insertions(+), 102 deletions(-) diff --git a/src/main/java/io/supertokens/inmemorydb/queries/ActiveUsersQueries.java b/src/main/java/io/supertokens/inmemorydb/queries/ActiveUsersQueries.java index d924cb3d6..3bb08ca1f 100644 --- a/src/main/java/io/supertokens/inmemorydb/queries/ActiveUsersQueries.java +++ b/src/main/java/io/supertokens/inmemorydb/queries/ActiveUsersQueries.java @@ -96,7 +96,7 @@ public static int countUsersEnabledTotpAndActiveSince(Start start, AppIdentifier } public static int countUsersEnabledMfa(Start start, AppIdentifier appIdentifier) throws SQLException, StorageQueryException { - String QUERY = "SELECT COUNT(*) as total FROM (SELECT DISTINCT user_id FROM " + Config.getConfig(start).getMfaUserFactorsTable() + "WHERE app_id = ?) AS app_mfa_users"; + String QUERY = "SELECT COUNT(*) as total FROM (SELECT DISTINCT user_id FROM " + Config.getConfig(start).getMfaUserFactorsTable() + " WHERE app_id = ?) AS app_mfa_users"; return execute(start, QUERY, pst -> { pst.setString(1, appIdentifier.getAppId()); diff --git a/src/main/java/io/supertokens/totp/Totp.java b/src/main/java/io/supertokens/totp/Totp.java index fcfdde098..89fcd1864 100644 --- a/src/main/java/io/supertokens/totp/Totp.java +++ b/src/main/java/io/supertokens/totp/Totp.java @@ -94,33 +94,31 @@ public static TOTPDevice registerDevice(Main main, String userId, } } - private static TOTPDevice registerDeviceRecursive(AppIdentifierWithStorage appIdentifierWithStorage, TOTPDevice device, int counter) throws StorageQueryException, DeviceAlreadyExistsException, TenantOrAppNotFoundException, StorageTransactionLogicException { + private static TOTPDevice registerDeviceRecursive(AppIdentifierWithStorage appIdentifierWithStorage, TOTPDevice device, int deviceNameCounter) throws StorageQueryException, DeviceAlreadyExistsException, TenantOrAppNotFoundException, StorageTransactionLogicException { TOTPSQLStorage totpStorage = appIdentifierWithStorage.getTOTPStorage(); + TOTPDevice newDevice = new TOTPDevice(device.userId, "TOTP Device " + deviceNameCounter, device.secretKey, device.period, device.skew, false); try { - TOTPDevice d = new TOTPDevice(device.userId, "TOTP Device " + (counter + 1), device.secretKey, device.period, device.skew, false); - totpStorage.createDevice(appIdentifierWithStorage, d); - return d; + totpStorage.createDevice(appIdentifierWithStorage, newDevice); + return newDevice; } catch (DeviceAlreadyExistsException e) { TOTPDevice[] devices = totpStorage.getDevices(appIdentifierWithStorage, device.userId); // iterate through all devices to find device with same name - TOTPDevice existingDevice = Arrays.stream(devices).filter(d -> d.deviceName.equals(device.deviceName)) + TOTPDevice existingDevice = Arrays.stream(devices).filter(d -> d.deviceName.equals(newDevice.deviceName)) .findFirst().orElse(null); if (existingDevice != null) { if (existingDevice.verified) { // device with same name exists and is verified - // TODO: Should this recursion have a limit? - return registerDeviceRecursive(appIdentifierWithStorage, device, ++counter); + return registerDeviceRecursive(appIdentifierWithStorage, device, deviceNameCounter + 1); } else { // device with same name exists but is not verified // delete the device and retry totpStorage.startTransaction(con -> { - totpStorage.deleteDevice_Transaction(con, appIdentifierWithStorage, device.userId, device.deviceName); + totpStorage.deleteDevice_Transaction(con, appIdentifierWithStorage, newDevice.userId, newDevice.deviceName); totpStorage.commitTransaction(con); return null; }); - // TODO: Should this recursion have a limit? - return registerDeviceRecursive(appIdentifierWithStorage, device, counter); + return registerDeviceRecursive(appIdentifierWithStorage, device, deviceNameCounter); } } throw e; @@ -150,9 +148,8 @@ public static TOTPDevice registerDevice(AppIdentifierWithStorage appIdentifierWi // Find number of existing devices to set device name TOTPDevice[] devices = totpStorage.getDevices(appIdentifierWithStorage, userId); int verifiedDevicesCount = Arrays.stream(devices).filter(d -> d.verified).toArray().length; - // device.deviceName = "TOTP Device " + (verifiedDevicesCount + 1); - return registerDeviceRecursive(appIdentifierWithStorage, device, verifiedDevicesCount); + return registerDeviceRecursive(appIdentifierWithStorage, device, verifiedDevicesCount + 1); } private static void checkAndStoreCode(TenantIdentifierWithStorage tenantIdentifierWithStorage, Main main, @@ -211,8 +208,7 @@ private static void checkAndStoreCode(TenantIdentifierWithStorage tenantIdentifi .count(); int rateLimitResetTimeInMs = Config.getConfig(tenantIdentifierWithStorage, main) .getTotpRateLimitCooldownTimeSec() * - 1000; // (Default - // 15 mins) + 1000; // (Default 15 mins) // Check if the user has been rate limited: if (invalidOutOfN == N) { @@ -389,20 +385,18 @@ public static boolean verifyDevice(TenantIdentifierWithStorage tenantIdentifierW } @TestOnly - public static void verifyCode(Main main, String userId, - String code, boolean allowUnverifiedDevices) + public static void verifyCode(Main main, String userId, String code) throws InvalidTotpException, LimitReachedException, StorageQueryException, StorageTransactionLogicException, FeatureNotEnabledException { try { verifyCode(new TenantIdentifierWithStorage(null, null, null, StorageLayer.getStorage(main)), main, - userId, code, allowUnverifiedDevices); + userId, code); } catch (TenantOrAppNotFoundException e) { throw new IllegalStateException(e); } } - public static void verifyCode(TenantIdentifierWithStorage tenantIdentifierWithStorage, Main main, String userId, - String code, boolean allowUnverifiedDevices) + public static void verifyCode(TenantIdentifierWithStorage tenantIdentifierWithStorage, Main main, String userId, String code) throws InvalidTotpException, LimitReachedException, StorageQueryException, StorageTransactionLogicException, FeatureNotEnabledException, TenantOrAppNotFoundException { @@ -423,9 +417,7 @@ public static void verifyCode(TenantIdentifierWithStorage tenantIdentifierWithSt } // Filter out unverified devices: - if (!allowUnverifiedDevices) { - devices = Arrays.stream(devices).filter(device -> device.verified).toArray(TOTPDevice[]::new); - } + devices = Arrays.stream(devices).filter(device -> device.verified).toArray(TOTPDevice[]::new); // At this point, even if some of the devices are suddenly deleted/renamed by // another API call. We will still check the code against the updated set of diff --git a/src/main/java/io/supertokens/webserver/api/totp/VerifyTotpAPI.java b/src/main/java/io/supertokens/webserver/api/totp/VerifyTotpAPI.java index 1c2e08eac..2b315f853 100644 --- a/src/main/java/io/supertokens/webserver/api/totp/VerifyTotpAPI.java +++ b/src/main/java/io/supertokens/webserver/api/totp/VerifyTotpAPI.java @@ -70,7 +70,7 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws I tenantIdentifierWithStorage = getTenantIdentifierWithStorageFromRequest(req); } - Totp.verifyCode(tenantIdentifierWithStorage, main, userId, totp, false); + Totp.verifyCode(tenantIdentifierWithStorage, main, userId, totp); result.addProperty("status", "OK"); super.sendJsonResponse(200, result, resp); diff --git a/src/test/java/io/supertokens/test/FeatureFlagTest.java b/src/test/java/io/supertokens/test/FeatureFlagTest.java index 8b5047c3c..1193c474f 100644 --- a/src/test/java/io/supertokens/test/FeatureFlagTest.java +++ b/src/test/java/io/supertokens/test/FeatureFlagTest.java @@ -26,6 +26,7 @@ import io.supertokens.cronjobs.Cronjobs; import io.supertokens.cronjobs.syncCoreConfigWithDb.SyncCoreConfigWithDb; import io.supertokens.emailpassword.EmailPassword; +import io.supertokens.featureflag.EE_FEATURES; import io.supertokens.featureflag.FeatureFlag; import io.supertokens.featureflag.FeatureFlagTestContent; import io.supertokens.featureflag.exceptions.FeatureNotEnabledException; diff --git a/src/test/java/io/supertokens/test/mfa/MfaStorageTest.java b/src/test/java/io/supertokens/test/mfa/MfaStorageTest.java index 1cb993d46..1393ce57b 100644 --- a/src/test/java/io/supertokens/test/mfa/MfaStorageTest.java +++ b/src/test/java/io/supertokens/test/mfa/MfaStorageTest.java @@ -162,7 +162,7 @@ public void deleteUserFromTenantTest() throws Exception { result.process.main, "user@example.com", "password" - ).id; + ).getSupertokensUserId(); // Iterate over all both tenants and enable the same set of factors for the same user ID for (TenantIdentifierWithStorage tid : new TenantIdentifierWithStorage[]{publicTenant, privateTenant}) { @@ -177,7 +177,7 @@ public void deleteUserFromTenantTest() throws Exception { assert mfaStorage.listFactors(privateTenant, userId).length == 0; assert mfaStorage.listFactors(publicTenant, userId).length == 2; - String userEmail = EmailPassword.signIn(privateTenant, result.process.main, "user@example.com", "password").email; + String userEmail = EmailPassword.signIn(privateTenant, result.process.main, "user@example.com", "password").loginMethods[0].email; assert userEmail.equals("user@example.com"); // Use should still exist in the private tenant since we have only disabled MFA related info // Deleting from non existent user should return false: diff --git a/src/test/java/io/supertokens/test/mfa/api/MfaUserIdMappingTest.java b/src/test/java/io/supertokens/test/mfa/api/MfaUserIdMappingTest.java index 4b90935a4..ec60ff654 100644 --- a/src/test/java/io/supertokens/test/mfa/api/MfaUserIdMappingTest.java +++ b/src/test/java/io/supertokens/test/mfa/api/MfaUserIdMappingTest.java @@ -19,7 +19,7 @@ import com.google.gson.JsonObject; import io.supertokens.Main; import io.supertokens.emailpassword.EmailPassword; -import io.supertokens.pluginInterface.emailpassword.UserInfo; +import io.supertokens.pluginInterface.authRecipe.AuthRecipeUserInfo; import io.supertokens.test.mfa.MfaTestBase; import io.supertokens.useridmapping.UserIdMapping; import org.junit.Test; @@ -33,8 +33,8 @@ public void testExternalUserIdTranslation() throws Exception { Main main = result.process.getProcess(); JsonObject body = new JsonObject(); - UserInfo user = EmailPassword.signUp(main, "test@example.com", "testPass123"); - String superTokensUserId = user.id; + AuthRecipeUserInfo user = EmailPassword.signUp(main, "test@example.com", "testPass123"); + String superTokensUserId = user.getSupertokensUserId(); String externalUserId = "external-user-id"; // Create user id mapping first: diff --git a/src/test/java/io/supertokens/test/multitenant/TestAppData.java b/src/test/java/io/supertokens/test/multitenant/TestAppData.java index f6c4c679e..5d3b75039 100644 --- a/src/test/java/io/supertokens/test/multitenant/TestAppData.java +++ b/src/test/java/io/supertokens/test/multitenant/TestAppData.java @@ -154,8 +154,10 @@ public void testThatDeletingAppDeleteDataFromAllTables() throws Exception { TOTPDevice totpDevice = Totp.registerDevice(appWithStorage.toAppIdentifierWithStorage(), process.getProcess(), epUser.getSupertokensUserId(), "test", 1, 3); + Totp.verifyDevice(appWithStorage, process.getProcess(), epUser.getSupertokensUserId(), totpDevice.deviceName, + generateTotpCode(process.getProcess(), totpDevice, -1)); Totp.verifyCode(appWithStorage, process.getProcess(), epUser.getSupertokensUserId(), - generateTotpCode(process.getProcess(), totpDevice, 0), true); + generateTotpCode(process.getProcess(), totpDevice, 0)); ActiveUsers.updateLastActive(appWithStorage.toAppIdentifierWithStorage(), process.getProcess(), epUser.getSupertokensUserId()); diff --git a/src/test/java/io/supertokens/test/totp/TOTPRecipeTest.java b/src/test/java/io/supertokens/test/totp/TOTPRecipeTest.java index 091b2d5d3..ecf5faa11 100644 --- a/src/test/java/io/supertokens/test/totp/TOTPRecipeTest.java +++ b/src/test/java/io/supertokens/test/totp/TOTPRecipeTest.java @@ -23,6 +23,7 @@ import io.supertokens.cronjobs.deleteExpiredTotpTokens.DeleteExpiredTotpTokens; import io.supertokens.featureflag.EE_FEATURES; import io.supertokens.featureflag.FeatureFlagTestContent; +import io.supertokens.pluginInterface.STORAGE_TYPE; import io.supertokens.pluginInterface.exceptions.StorageQueryException; import io.supertokens.pluginInterface.exceptions.StorageTransactionLogicException; import io.supertokens.pluginInterface.multitenancy.TenantIdentifier; @@ -109,7 +110,7 @@ public static String generateTotpCode(Main main, TOTPDevice device) /** * Generates TOTP code similar to apps like Google Authenticator and Authy */ - private static String generateTotpCode(Main main, TOTPDevice device, int step) + public static String generateTotpCode(Main main, TOTPDevice device, int step) throws InvalidKeyException, StorageQueryException { final TimeBasedOneTimePasswordGenerator totp = new TimeBasedOneTimePasswordGenerator( Duration.ofSeconds(device.period)); @@ -158,76 +159,84 @@ public void createDeviceAndVerifyCodeTest() throws Exception { } Main main = result.process.getProcess(); - // Create device + // Create devices TOTPDevice device = Totp.registerDevice(main, "user", "device1", 1, 1); + TOTPDevice unverifiedDevice = Totp.registerDevice(main, "user", "unverified-device", 1, 1); + + // Verify device: + Totp.verifyDevice(main, "user", device.deviceName, generateTotpCode(main, device, -1)); // Try login with non-existent user: assertThrows(InvalidTotpException.class, - () -> Totp.verifyCode(main, "non-existent-user", "any-code", true)); + () -> Totp.verifyCode(main, "non-existent-user", "any-code")); - // {Code: [INVALID, VALID]} * {Devices: [VERIFIED_ONLY, ALL]} + // {Code: [INVALID, VALID]} * {Devices: [verified, unverfied]} - // Invalid code & allowUnverifiedDevice = true: + // Invalid code & unverified device: assertThrows(InvalidTotpException.class, - () -> Totp.verifyCode(main, "user", "invalid", true)); + () -> Totp.verifyCode(main, "user", "invalid")); - // Invalid code & allowUnverifiedDevice = false: + // Invalid code & verified device: assertThrows(InvalidTotpException.class, - () -> Totp.verifyCode(main, "user", "invalid", false)); + () -> Totp.verifyCode(main, "user", "invalid")); - // Valid code & allowUnverifiedDevice = false: + // Valid code & unverified device: assertThrows( InvalidTotpException.class, - () -> Totp.verifyCode(main, "user", generateTotpCode(main, device), false)); + () -> Totp.verifyCode(main, "user", generateTotpCode(main, unverifiedDevice))); - // Valid code & allowUnverifiedDevice = true (Success): + // Valid code & verified device (Success) String validCode = generateTotpCode(main, device); - Totp.verifyCode(main, "user", validCode, true); + Totp.verifyCode(main, "user", validCode); // Now try again with same code: assertThrows( InvalidTotpException.class, - () -> Totp.verifyCode(main, "user", validCode, true)); + () -> Totp.verifyCode(main, "user", validCode)); // Sleep for 1s so that code changes. Thread.sleep(1000); // Use a new valid code: String newValidCode = generateTotpCode(main, device); - Totp.verifyCode(main, "user", newValidCode, true); + Totp.verifyCode(main, "user", newValidCode); // Reuse the same code and use it again (should fail): assertThrows(InvalidTotpException.class, - () -> Totp.verifyCode(main, "user", newValidCode, true)); + () -> Totp.verifyCode(main, "user", newValidCode)); // Use a code from next period: String nextValidCode = generateTotpCode(main, device, 1); - Totp.verifyCode(main, "user", nextValidCode, true); + Totp.verifyCode(main, "user", nextValidCode); // Use previous period code (should fail coz validCode has been used): String previousCode = generateTotpCode(main, device, -1); assert previousCode.equals(validCode); - assertThrows(InvalidTotpException.class, () -> Totp.verifyCode(main, "user", previousCode, true)); + assertThrows(InvalidTotpException.class, () -> Totp.verifyCode(main, "user", previousCode)); // Create device with skew = 0, check that it only works with the current code TOTPDevice device2 = Totp.registerDevice(main, "user", "device2", 0, 1); assert !Objects.equals(device2.secretKey, device.secretKey); + Totp.verifyDevice(main, "user", device2.deviceName, generateTotpCode(main, device2)); + + // Sleep because code was used for verifying the device + Thread.sleep(1000); String nextValidCode2 = generateTotpCode(main, device2, 1); assertThrows(InvalidTotpException.class, - () -> Totp.verifyCode(main, "user", nextValidCode2, true)); + () -> Totp.verifyCode(main, "user", nextValidCode2)); String previousValidCode2 = generateTotpCode(main, device2, -1); assertThrows(InvalidTotpException.class, - () -> Totp.verifyCode(main, "user", previousValidCode2, true)); + () -> Totp.verifyCode(main, "user", previousValidCode2)); String currentValidCode2 = generateTotpCode(main, device2); - Totp.verifyCode(main, "user", currentValidCode2, true); + Totp.verifyCode(main, "user", currentValidCode2); // Submit invalid code and check that it's expiry time is correct // created - expiryTime = max of ((2 * skew + 1) * period) for all devices assertThrows(InvalidTotpException.class, - () -> Totp.verifyCode(main, "user", "invalid", true)); + () -> Totp.verifyCode(main, "user", "invalid")); TOTPUsedCode[] usedCodes = getAllUsedCodesUtil(result.storage, "user"); TOTPUsedCode latestCode = usedCodes[0]; @@ -242,20 +251,18 @@ public void createDeviceAndVerifyCodeTest() throws Exception { Totp.verifyDevice(main, "user", device2.deviceName, generateTotpCode(main, device2)); // device1: unverified, device2: verified - // Valid code & allowUnverifiedDevice = false: - assertThrows( - InvalidTotpException.class, - () -> Totp.verifyCode(main, "user", generateTotpCode(main, device), false)); + // Valid code & verified device: + Totp.verifyCode(main, "user", generateTotpCode(main, device)); Thread.sleep(1000); - Totp.verifyCode(main, "user", generateTotpCode(main, device2), false); + Totp.verifyCode(main, "user", generateTotpCode(main, device2)); // Valid code & allowUnverifiedDevice = true: Thread.sleep(1000); - Totp.verifyCode(main, "user", generateTotpCode(main, device), true); + Totp.verifyCode(main, "user", generateTotpCode(main, device)); Thread.sleep(1000); - Totp.verifyCode(main, "user", generateTotpCode(main, device2), true); + Totp.verifyCode(main, "user", generateTotpCode(main, device2)); } /* @@ -272,20 +279,20 @@ public int triggerAndCheckRateLimit(Main main, TOTPDevice device) throws Excepti String code = "ic-" + i; // ic = invalid code assertThrows( InvalidTotpException.class, - () -> Totp.verifyCode(main, "user", code, true)); + () -> Totp.verifyCode(main, "user", code)); } // Any kind of attempt after this should fail with rate limiting error. // This should happen until rate limiting cooldown happens: assertThrows( LimitReachedException.class, - () -> Totp.verifyCode(main, "user", "icN+1", true)); + () -> Totp.verifyCode(main, "user", "icN+1")); assertThrows( LimitReachedException.class, - () -> Totp.verifyCode(main, "user", generateTotpCode(main, device), true)); + () -> Totp.verifyCode(main, "user", generateTotpCode(main, device))); assertThrows( LimitReachedException.class, - () -> Totp.verifyCode(main, "user", "icN+2", true)); + () -> Totp.verifyCode(main, "user", "icN+2")); return N; } @@ -313,6 +320,7 @@ public void rateLimitCooldownTest() throws Exception { // Create device TOTPDevice device = Totp.registerDevice(main, "user", "deviceName", 1, 1); + Totp.verifyDevice(main, "user", device.deviceName, generateTotpCode(main, device, -1)); // Trigger rate limiting and fix it with a correct code after some time: int attemptsRequired = triggerAndCheckRateLimit(main, device); @@ -320,17 +328,17 @@ public void rateLimitCooldownTest() throws Exception { // Wait for 1 second (Should cool down rate limiting): Thread.sleep(1000); // But again try with invalid code: - assertThrows(InvalidTotpException.class, () -> Totp.verifyCode(main, "user", "invalid0", true)); + assertThrows(InvalidTotpException.class, () -> Totp.verifyCode(main, "user", "invalid0")); // This triggered rate limiting again. So even valid codes will fail for // another cooldown period: assertThrows(LimitReachedException.class, - () -> Totp.verifyCode(main, "user", generateTotpCode(main, device), true)); + () -> Totp.verifyCode(main, "user", generateTotpCode(main, device))); // Wait for 1 second (Should cool down rate limiting): Thread.sleep(1000); // Now try with valid code: - Totp.verifyCode(main, "user", generateTotpCode(main, device), true); + Totp.verifyCode(main, "user", generateTotpCode(main, device)); // Now invalid code shouldn't trigger rate limiting. Unless you do it N times: - assertThrows(InvalidTotpException.class, () -> Totp.verifyCode(main, "user", "invaldd", true)); + assertThrows(InvalidTotpException.class, () -> Totp.verifyCode(main, "user", "invaldd")); } @Test @@ -356,9 +364,9 @@ public void cronRemovesCodesDuringRateLimitTest() throws Exception { // This removal shouldn't affect rate limiting. User must remain rate limited. assertThrows(LimitReachedException.class, - () -> Totp.verifyCode(main, "user", generateTotpCode(main, device), true)); + () -> Totp.verifyCode(main, "user", generateTotpCode(main, device))); assertThrows(LimitReachedException.class, - () -> Totp.verifyCode(main, "user", "yet-ic", true)); + () -> Totp.verifyCode(main, "user", "yet-ic")); } @Test @@ -419,6 +427,9 @@ public void removeDeviceTest() throws Exception { TOTPDevice device1 = Totp.registerDevice(main, "user", "device1", 1, 30); TOTPDevice device2 = Totp.registerDevice(main, "user", "device2", 1, 30); + Totp.verifyDevice(main, "user", "device1", generateTotpCode(main, device1, -1)); + Totp.verifyDevice(main, "user", "device2", generateTotpCode(main, device2, -1)); + TOTPDevice[] devices = Totp.getDevices(main, "user"); assert (devices.length == 2); @@ -430,9 +441,9 @@ public void removeDeviceTest() throws Exception { // Delete one of the devices { - assertThrows(InvalidTotpException.class, () -> Totp.verifyCode(main, "user", "ic0", true)); - Totp.verifyCode(main, "user", generateTotpCode(main, device1), true); - Totp.verifyCode(main, "user", generateTotpCode(main, device2), true); + assertThrows(InvalidTotpException.class, () -> Totp.verifyCode(main, "user", "ic0")); + Totp.verifyCode(main, "user", generateTotpCode(main, device1)); + Totp.verifyCode(main, "user", generateTotpCode(main, device2)); // Delete device1 Totp.removeDevice(main, "user", "device1"); @@ -442,7 +453,7 @@ public void removeDeviceTest() throws Exception { // 1 device still remain so all codes should still be still there: TOTPUsedCode[] usedCodes = getAllUsedCodesUtil(storage, "user"); - assert (usedCodes.length == 3); + assert (usedCodes.length == 5); // 2 for device verification and 3 for code verification } // Deleting the last device of a user should delete all related codes: @@ -451,8 +462,9 @@ public void removeDeviceTest() throws Exception { // Create another user to test that other users aren't affected: TOTPDevice otherUserDevice = Totp.registerDevice(main, "other-user", "device", 1, 30); - Totp.verifyCode(main, "other-user", generateTotpCode(main, otherUserDevice), true); - assertThrows(InvalidTotpException.class, () -> Totp.verifyCode(main, "other-user", "ic1", true)); + Totp.verifyDevice(main, "other-user", "device", generateTotpCode(main, otherUserDevice, -1)); + Totp.verifyCode(main, "other-user", generateTotpCode(main, otherUserDevice)); + assertThrows(InvalidTotpException.class, () -> Totp.verifyCode(main, "other-user", "ic1")); // Delete device2 Totp.removeDevice(main, "user", "device2"); @@ -469,7 +481,7 @@ public void removeDeviceTest() throws Exception { assert (otherUserDevices.length == 1); usedCodes = getAllUsedCodesUtil(storage, "other-user"); - assert (usedCodes.length == 2); + assert (usedCodes.length == 3); // 1 for device verification and 2 for code verification } } diff --git a/src/test/java/io/supertokens/test/totp/TOTPStorageTest.java b/src/test/java/io/supertokens/test/totp/TOTPStorageTest.java index f2f34f9ab..5ef2a3c42 100644 --- a/src/test/java/io/supertokens/test/totp/TOTPStorageTest.java +++ b/src/test/java/io/supertokens/test/totp/TOTPStorageTest.java @@ -402,11 +402,13 @@ public void insertUsedCodeTest() throws Exception { // Try to insert code when user doesn't have any device (i.e. TOTP not enabled) { - assertThrows(UnknownDeviceException.class, + StorageTransactionLogicException e = assertThrows(StorageTransactionLogicException.class, () -> insertUsedCodesUtil(storage, new TOTPUsedCode[]{ new TOTPUsedCode("new-user-without-totp", "1234", true, nextDay, System.currentTimeMillis()) })); + + // assert e.actualException instanceof UnknownDeviceException } // Try to insert code after user has atleast one device (i.e. TOTP enabled) @@ -421,11 +423,13 @@ public void insertUsedCodeTest() throws Exception { } // Try to insert code when user doesn't exist: - assertThrows(UnknownDeviceException.class, + StorageTransactionLogicException e = assertThrows(StorageTransactionLogicException.class, () -> insertUsedCodesUtil(storage, new TOTPUsedCode[]{ new TOTPUsedCode("non-existent-user", "1234", true, nextDay, System.currentTimeMillis()) })); + + // assert e.actualException instanceof UnknownDeviceException; } @Test diff --git a/src/test/java/io/supertokens/test/totp/TotpLicenseTest.java b/src/test/java/io/supertokens/test/totp/TotpLicenseTest.java index a46ca392e..d612bd40e 100644 --- a/src/test/java/io/supertokens/test/totp/TotpLicenseTest.java +++ b/src/test/java/io/supertokens/test/totp/TotpLicenseTest.java @@ -99,7 +99,7 @@ public void testTotpWithoutLicense() throws Exception { }); // Verify code assertThrows(FeatureNotEnabledException.class, () -> { - Totp.verifyCode(main, "user", "device1", true); + Totp.verifyCode(main, "user", "device1"); }); // Try to create device via API: @@ -133,7 +133,6 @@ public void testTotpWithoutLicense() throws Exception { JsonObject body2 = new JsonObject(); body2.addProperty("userId", "user-id"); body2.addProperty("totp", "123456"); - body2.addProperty("allowUnverifiedDevices", true); HttpResponseException e2 = assertThrows( @@ -169,9 +168,12 @@ public void testTotpWithLicense() throws Exception { // Create device TOTPDevice device = Totp.registerDevice(main, "user", "device1", 1, 30); + // Verify device + String code = generateTotpCode(main, device, 0); + Totp.verifyDevice(main, device.userId, device.deviceName, code); // Verify code - String code = generateTotpCode(main, device); - Totp.verifyCode(main, "user", code, true); + String nextCode = generateTotpCode(main, device, 1); + Totp.verifyCode(main, "user", nextCode); } diff --git a/src/test/java/io/supertokens/test/totp/api/CreateTotpDeviceAPITest.java b/src/test/java/io/supertokens/test/totp/api/CreateTotpDeviceAPITest.java index 112c57163..7fe30559f 100644 --- a/src/test/java/io/supertokens/test/totp/api/CreateTotpDeviceAPITest.java +++ b/src/test/java/io/supertokens/test/totp/api/CreateTotpDeviceAPITest.java @@ -6,11 +6,13 @@ import io.supertokens.featureflag.FeatureFlag; import io.supertokens.featureflag.FeatureFlagTestContent; import io.supertokens.pluginInterface.STORAGE_TYPE; +import io.supertokens.pluginInterface.totp.TOTPDevice; import io.supertokens.storageLayer.StorageLayer; import io.supertokens.test.TestingProcessManager; import io.supertokens.test.Utils; import io.supertokens.test.httpRequest.HttpRequestForTesting; import io.supertokens.test.httpRequest.HttpResponseException; +import io.supertokens.test.totp.TOTPRecipeTest; import io.supertokens.test.totp.TotpLicenseTest; import org.junit.AfterClass; import org.junit.Before; @@ -138,7 +140,7 @@ public void testApi() throws Exception { assert res.get("status").getAsString().equals("OK"); assert res.get("deviceName").getAsString().equals("d1"); - // try again with same device: + // try again with same device name: JsonObject res2 = HttpRequestForTesting.sendJsonPOSTRequest( process.getProcess(), "", @@ -164,7 +166,65 @@ public void testApi() throws Exception { Utils.getCdiVersionStringLatestForTests(), "totp"); assert res3.get("status").getAsString().equals("OK"); - assert res3.get("deviceName").getAsString().equals("TOTP Device 2"); + assert res3.get("deviceName").getAsString().equals("TOTP Device 1"); + String attempt1Secret = res3.get("secret").getAsString(); + + // try again without passing deviceName: + // should re-create the device since "TOTP Device 1" wasn't verified + JsonObject res4 = HttpRequestForTesting.sendJsonPOSTRequest( + process.getProcess(), + "", + "http://localhost:3567/recipe/totp/device", + body, + 1000, + 1000, + null, + Utils.getCdiVersionStringLatestForTests(), + "totp"); + assert res4.get("status").getAsString().equals("OK"); + assert res3.get("deviceName").getAsString().equals("TOTP Device 1"); + String attempt2Secret = res4.get("secret").getAsString(); + assert !attempt1Secret.equals(attempt2Secret); + + // verify the device: + TOTPDevice device = new TOTPDevice( + "user-id", + "TOTP Device 1", + attempt2Secret, + 30, + 0, + false + ); + JsonObject verifyDeviceBody = new JsonObject(); + verifyDeviceBody.addProperty("userId", device.userId); + verifyDeviceBody.addProperty("deviceName", device.deviceName); + verifyDeviceBody.addProperty("totp", TOTPRecipeTest.generateTotpCode(process.getProcess(), device)); + JsonObject res5 = HttpRequestForTesting.sendJsonPOSTRequest( + process.getProcess(), + "", + "http://localhost:3567/recipe/totp/device/verify", + verifyDeviceBody, + 1000, + 1000, + null, + Utils.getCdiVersionStringLatestForTests(), + "totp"); + assert res5.get("status").getAsString().equals("OK"); + + // now try to create a device: + // "TOTP Device 1" has been verified, it won't replace it + JsonObject res6 = HttpRequestForTesting.sendJsonPOSTRequest( + process.getProcess(), + "", + "http://localhost:3567/recipe/totp/device", + body, + 1000, + 1000, + null, + Utils.getCdiVersionStringLatestForTests(), + "totp"); + assert res6.get("status").getAsString().equals("OK"); + assert res6.get("deviceName").getAsString().equals("TOTP Device 2"); } process.kill(); diff --git a/src/test/java/io/supertokens/test/totp/api/TotpUserIdMappingTest.java b/src/test/java/io/supertokens/test/totp/api/TotpUserIdMappingTest.java index 7e135958c..112cd0c14 100644 --- a/src/test/java/io/supertokens/test/totp/api/TotpUserIdMappingTest.java +++ b/src/test/java/io/supertokens/test/totp/api/TotpUserIdMappingTest.java @@ -61,7 +61,7 @@ public void testExternalUserIdTranslation() throws Exception { body.addProperty("userId", externalUserId); body.addProperty("deviceName", "d1"); - body.addProperty("skew", 0); + body.addProperty("skew", 1); body.addProperty("period", 30); // Register 1st device @@ -77,7 +77,7 @@ public void testExternalUserIdTranslation() throws Exception { "totp"); assert res1.get("status").getAsString().equals("OK"); String d1Secret = res1.get("secret").getAsString(); - TOTPDevice device1 = new TOTPDevice(externalUserId, "deviceName", d1Secret, 30, 0, false); + TOTPDevice device1 = new TOTPDevice(externalUserId, "deviceName", d1Secret, 30, 1, false); body.addProperty("deviceName", "d2"); @@ -93,14 +93,14 @@ public void testExternalUserIdTranslation() throws Exception { "totp"); assert res2.get("status").getAsString().equals("OK"); String d2Secret = res2.get("secret").getAsString(); - TOTPDevice device2 = new TOTPDevice(externalUserId, "deviceName", d2Secret, 30, 0, false); + TOTPDevice device2 = new TOTPDevice(externalUserId, "d2", d2Secret, 30, 1, false); // Verify d1 but not d2: JsonObject verifyD1Input = new JsonObject(); verifyD1Input.addProperty("userId", externalUserId); - String d1Totp = TOTPRecipeTest.generateTotpCode(process.getProcess(), device1); + String d1VerifyTotp = TOTPRecipeTest.generateTotpCode(process.getProcess(), device1); verifyD1Input.addProperty("deviceName", "d1"); - verifyD1Input.addProperty("totp", d1Totp ); + verifyD1Input.addProperty("totp", d1VerifyTotp); JsonObject verifyD1Res = HttpRequestForTesting.sendJsonPOSTRequest( process.getProcess(), @@ -116,25 +116,43 @@ public void testExternalUserIdTranslation() throws Exception { assert verifyD1Res.get("status").getAsString().equals("OK"); assert verifyD1Res.get("wasAlreadyVerified").getAsBoolean() == false; - // use d2 to login in totp: - JsonObject loginInput = new JsonObject(); - loginInput.addProperty("userId", externalUserId); - String d2Totp = TOTPRecipeTest.generateTotpCode(process.getProcess(), device2); - loginInput.addProperty("totp", d2Totp); // use code from d2 which is unverified - loginInput.addProperty("allowUnverifiedDevices", true); + // use d2 to login in totp: (should fail coz it's not verified) + JsonObject d2LoginInput = new JsonObject(); + d2LoginInput.addProperty("userId", externalUserId); + String d2Totp = TOTPRecipeTest.generateTotpCode(process.getProcess(), device2, 1); + d2LoginInput.addProperty("totp", d2Totp); // use code from d2 which is unverified - JsonObject loginRes = HttpRequestForTesting.sendJsonPOSTRequest( + JsonObject d2LoginRes = HttpRequestForTesting.sendJsonPOSTRequest( process.getProcess(), "", "http://localhost:3567/recipe/totp/verify", - loginInput, + d2LoginInput, 1000, 1000, null, Utils.getCdiVersionStringLatestForTests(), "totp"); - assert loginRes.get("status").getAsString().equals("OK"); + assert d2LoginRes.get("status").getAsString().equals("INVALID_TOTP_ERROR"); + + // use d1 to login in totp: (should pass) + JsonObject d1LoginInput = new JsonObject(); + d1LoginInput.addProperty("userId", externalUserId); + String d1Totp = TOTPRecipeTest.generateTotpCode(process.getProcess(), device1, 1); + d1LoginInput.addProperty("totp", d1Totp); // use code from d2 which is unverified + + JsonObject d1LoginRes = HttpRequestForTesting.sendJsonPOSTRequest( + process.getProcess(), + "", + "http://localhost:3567/recipe/totp/verify", + d1LoginInput, + 1000, + 1000, + null, + Utils.getCdiVersionStringLatestForTests(), + "totp"); + + assert d1LoginRes.get("status").getAsString().equals("OK"); // Change the name of d1 to d3: JsonObject updateDeviceNameInput = new JsonObject(); diff --git a/src/test/java/io/supertokens/test/totp/api/VerifyTotpAPITest.java b/src/test/java/io/supertokens/test/totp/api/VerifyTotpAPITest.java index e39f1bdf4..f4930dc8b 100644 --- a/src/test/java/io/supertokens/test/totp/api/VerifyTotpAPITest.java +++ b/src/test/java/io/supertokens/test/totp/api/VerifyTotpAPITest.java @@ -23,6 +23,7 @@ import org.junit.Test; import org.junit.rules.TestRule; +import static io.supertokens.test.totp.TOTPRecipeTest.generateTotpCode; import static org.junit.Assert.*; public class VerifyTotpAPITest { @@ -93,7 +94,7 @@ public void testApi() throws Exception { JsonObject createDeviceReq = new JsonObject(); createDeviceReq.addProperty("userId", "user-id"); createDeviceReq.addProperty("deviceName", "deviceName"); - createDeviceReq.addProperty("period", 30); + createDeviceReq.addProperty("period", 2); createDeviceReq.addProperty("skew", 0); JsonObject createDeviceRes = HttpRequestForTesting.sendJsonPOSTRequest( @@ -109,10 +110,26 @@ public void testApi() throws Exception { assertEquals(createDeviceRes.get("status").getAsString(), "OK"); String secretKey = createDeviceRes.get("secret").getAsString(); - TOTPDevice device = new TOTPDevice("user-id", "deviceName", secretKey, 30, 0, false); + TOTPDevice device = new TOTPDevice("user-id", "deviceName", secretKey, 2, 0, false); - // Start the actual tests for update device API: + JsonObject verifyDeviceReq = new JsonObject(); + verifyDeviceReq.addProperty("userId", device.userId); + verifyDeviceReq.addProperty("deviceName", device.deviceName); + verifyDeviceReq.addProperty("totp", generateTotpCode(process.getProcess(), device)); + + JsonObject verifyDeviceRes = HttpRequestForTesting.sendJsonPOSTRequest( + process.getProcess(), + "", + "http://localhost:3567/recipe/totp/device/verify", + verifyDeviceReq, + 1000, + 1000, + null, + Utils.getCdiVersionStringLatestForTests(), + "totp"); + assertEquals(verifyDeviceRes.get("status").getAsString(), "OK"); + // Start the actual tests for update device API: JsonObject body = new JsonObject(); // Missing userId/deviceName/skew/period @@ -177,7 +194,7 @@ public void testApi() throws Exception { Thread.sleep(1000); // should pass now on valid code - String validTotp = TOTPRecipeTest.generateTotpCode(process.getProcess(), device); + String validTotp = generateTotpCode(process.getProcess(), device); body.addProperty("totp", validTotp); JsonObject res = HttpRequestForTesting.sendJsonPOSTRequest( process.getProcess(), @@ -206,7 +223,7 @@ public void testApi() throws Exception { assert res2.get("status").getAsString().equals("INVALID_TOTP_ERROR"); // Try with a new valid code during rate limiting: - body.addProperty("totp", TOTPRecipeTest.generateTotpCode(process.getProcess(), device)); + body.addProperty("totp", generateTotpCode(process.getProcess(), device)); res = HttpRequestForTesting.sendJsonPOSTRequest( process.getProcess(), "", From 59373c26cf26e969487b1ce37c5fab8b1ad538d0 Mon Sep 17 00:00:00 2001 From: KShivendu Date: Wed, 27 Sep 2023 15:16:02 +0530 Subject: [PATCH 12/16] feat: Add UNKNOWN_USER_ID_ERROR to verify totp api --- src/main/java/io/supertokens/totp/Totp.java | 6 +++--- .../io/supertokens/webserver/api/totp/VerifyTotpAPI.java | 4 ++++ 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/main/java/io/supertokens/totp/Totp.java b/src/main/java/io/supertokens/totp/Totp.java index 89fcd1864..7448868c8 100644 --- a/src/main/java/io/supertokens/totp/Totp.java +++ b/src/main/java/io/supertokens/totp/Totp.java @@ -386,7 +386,7 @@ public static boolean verifyDevice(TenantIdentifierWithStorage tenantIdentifierW @TestOnly public static void verifyCode(Main main, String userId, String code) - throws InvalidTotpException, LimitReachedException, + throws InvalidTotpException, UnknownUserIdTotpException, LimitReachedException, StorageQueryException, StorageTransactionLogicException, FeatureNotEnabledException { try { verifyCode(new TenantIdentifierWithStorage(null, null, null, StorageLayer.getStorage(main)), main, @@ -397,7 +397,7 @@ public static void verifyCode(Main main, String userId, String code) } public static void verifyCode(TenantIdentifierWithStorage tenantIdentifierWithStorage, Main main, String userId, String code) - throws InvalidTotpException, LimitReachedException, + throws InvalidTotpException, UnknownUserIdTotpException, LimitReachedException, StorageQueryException, StorageTransactionLogicException, FeatureNotEnabledException, TenantOrAppNotFoundException { @@ -413,7 +413,7 @@ public static void verifyCode(TenantIdentifierWithStorage tenantIdentifierWithSt TOTPDevice[] devices = totpStorage.getDevices(tenantIdentifierWithStorage.toAppIdentifier(), userId); if (devices.length == 0) { // No devices found. So we can't verify the code anyway. - throw new InvalidTotpException(); + throw new UnknownUserIdTotpException(); } // Filter out unverified devices: diff --git a/src/main/java/io/supertokens/webserver/api/totp/VerifyTotpAPI.java b/src/main/java/io/supertokens/webserver/api/totp/VerifyTotpAPI.java index 2b315f853..be575ab3a 100644 --- a/src/main/java/io/supertokens/webserver/api/totp/VerifyTotpAPI.java +++ b/src/main/java/io/supertokens/webserver/api/totp/VerifyTotpAPI.java @@ -13,6 +13,7 @@ import io.supertokens.pluginInterface.exceptions.StorageTransactionLogicException; import io.supertokens.pluginInterface.multitenancy.TenantIdentifierWithStorage; import io.supertokens.pluginInterface.multitenancy.exceptions.TenantOrAppNotFoundException; +import io.supertokens.pluginInterface.totp.exception.UnknownUserIdTotpException; import io.supertokens.totp.Totp; import io.supertokens.totp.exceptions.InvalidTotpException; import io.supertokens.totp.exceptions.LimitReachedException; @@ -77,6 +78,9 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws I } catch (InvalidTotpException e) { result.addProperty("status", "INVALID_TOTP_ERROR"); super.sendJsonResponse(200, result, resp); + } catch (UnknownUserIdTotpException e) { + result.addProperty("status", "UNKNOWN_USER_ID_ERROR"); + super.sendJsonResponse(200, result, resp); } catch (LimitReachedException e) { result.addProperty("status", "LIMIT_REACHED_ERROR"); result.addProperty("retryAfterMs", e.retryAfterMs); From 84b1b9d229918164366e9cebb69b7b0f1f8ba33a Mon Sep 17 00:00:00 2001 From: KShivendu Date: Wed, 27 Sep 2023 15:18:42 +0530 Subject: [PATCH 13/16] feat: Throw Unknown user id error when device gets deleted during verification --- src/main/java/io/supertokens/totp/Totp.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/io/supertokens/totp/Totp.java b/src/main/java/io/supertokens/totp/Totp.java index 7448868c8..bff58e656 100644 --- a/src/main/java/io/supertokens/totp/Totp.java +++ b/src/main/java/io/supertokens/totp/Totp.java @@ -428,7 +428,7 @@ public static void verifyCode(TenantIdentifierWithStorage tenantIdentifierWithSt } catch (UnknownUserIdTotpException e) { // User must have deleted the device in parallel // since they cannot un-verify a device (no API exists) - throw new InvalidTotpException(); + throw e; } } From 29f96430e67359ad67b89a1fe7c23b69b9ab5290 Mon Sep 17 00:00:00 2001 From: Sattvik Chakravarthy Date: Thu, 28 Sep 2023 15:29:52 +0530 Subject: [PATCH 14/16] fix: core fixes --- .../java/io/supertokens/inmemorydb/Start.java | 51 ++++++----- src/main/java/io/supertokens/totp/Totp.java | 90 ++++++++++--------- .../webserver/api/totp/VerifyTotpAPI.java | 4 +- .../io/supertokens/test/StorageLayerTest.java | 7 +- .../test/totp/TOTPStorageTest.java | 4 +- 5 files changed, 85 insertions(+), 71 deletions(-) diff --git a/src/main/java/io/supertokens/inmemorydb/Start.java b/src/main/java/io/supertokens/inmemorydb/Start.java index f2c8bfada..6becde092 100644 --- a/src/main/java/io/supertokens/inmemorydb/Start.java +++ b/src/main/java/io/supertokens/inmemorydb/Start.java @@ -71,7 +71,7 @@ import io.supertokens.pluginInterface.totp.TOTPUsedCode; import io.supertokens.pluginInterface.totp.exception.DeviceAlreadyExistsException; import io.supertokens.pluginInterface.totp.exception.UnknownDeviceException; -import io.supertokens.pluginInterface.totp.exception.UnknownUserIdTotpException; +import io.supertokens.pluginInterface.totp.exception.UnknownTotpUserIdException; import io.supertokens.pluginInterface.totp.exception.UsedCodeAlreadyExistsException; import io.supertokens.pluginInterface.totp.sqlStorage.TOTPSQLStorage; import io.supertokens.pluginInterface.useridmapping.UserIdMapping; @@ -2601,29 +2601,40 @@ public void revokeExpiredSessions() throws StorageQueryException { // TOTP recipe: @Override public void createDevice(AppIdentifier appIdentifier, TOTPDevice device) - throws StorageQueryException, DeviceAlreadyExistsException, TenantOrAppNotFoundException { + throws DeviceAlreadyExistsException, TenantOrAppNotFoundException, StorageQueryException { try { - TOTPQueries.createDevice(this, appIdentifier, device); - } catch (StorageTransactionLogicException e) { - if (e.actualException instanceof SQLiteException) { - String errMsg = e.actualException.getMessage(); - - if (isPrimaryKeyError(errMsg, Config.getConfig(this).getTotpUserDevicesTable(), - new String[]{"app_id", "user_id", "device_name"})) { - throw new DeviceAlreadyExistsException(); - } else if (isForeignKeyConstraintError( - errMsg, - Config.getConfig(this).getAppsTable(), - new String[]{"app_id"}, - new Object[]{appIdentifier.getAppId()})) { - throw new TenantOrAppNotFoundException(appIdentifier); + startTransaction(con -> { + try { + createDevice_Transaction(con, new AppIdentifier(null, null), device); + } catch (DeviceAlreadyExistsException | TenantOrAppNotFoundException e) { + throw new StorageTransactionLogicException(e); } + return null; + }); + } catch (StorageTransactionLogicException e) { + if (e.actualException instanceof DeviceAlreadyExistsException) { + throw (DeviceAlreadyExistsException) e.actualException; + } else if (e.actualException instanceof TenantOrAppNotFoundException) { + throw (TenantOrAppNotFoundException) e.actualException; + } else if (e.actualException instanceof StorageQueryException) { + throw (StorageQueryException) e.actualException; } - - throw new StorageQueryException(e.actualException); } } + @Override + public TOTPDevice createDevice_Transaction(TransactionConnection con, AppIdentifier appIdentifier, TOTPDevice device) + throws DeviceAlreadyExistsException, TenantOrAppNotFoundException, StorageQueryException { + // TODO + return null; + } + + @Override + public TOTPDevice getDeviceByName_Transaction(TransactionConnection con, AppIdentifier appIdentifier, String userId, String deviceName) throws StorageQueryException { + // TODO + return null; + } + @Override public void markDeviceAsVerified(AppIdentifier appIdentifier, String userId, String deviceName) throws StorageQueryException, UnknownDeviceException { @@ -2717,7 +2728,7 @@ public TOTPDevice[] getDevices_Transaction(TransactionConnection con, AppIdentif @Override public void insertUsedCode_Transaction(TransactionConnection con, TenantIdentifier tenantIdentifier, TOTPUsedCode usedCodeObj) - throws StorageQueryException, UnknownUserIdTotpException, UsedCodeAlreadyExistsException, + throws StorageQueryException, UnknownTotpUserIdException, UsedCodeAlreadyExistsException, TenantOrAppNotFoundException { Connection sqlCon = (Connection) con.getConnection(); try { @@ -2732,7 +2743,7 @@ public void insertUsedCode_Transaction(TransactionConnection con, TenantIdentifi Config.getConfig(this).getTotpUsersTable(), new String[]{"app_id", "user_id"}, new Object[]{tenantIdentifier.getAppId(), usedCodeObj.userId})) { - throw new UnknownUserIdTotpException(); + throw new UnknownTotpUserIdException(); } else if (isForeignKeyConstraintError( e.getMessage(), diff --git a/src/main/java/io/supertokens/totp/Totp.java b/src/main/java/io/supertokens/totp/Totp.java index bff58e656..63e7b08f0 100644 --- a/src/main/java/io/supertokens/totp/Totp.java +++ b/src/main/java/io/supertokens/totp/Totp.java @@ -16,7 +16,7 @@ import io.supertokens.pluginInterface.totp.TOTPUsedCode; import io.supertokens.pluginInterface.totp.exception.DeviceAlreadyExistsException; import io.supertokens.pluginInterface.totp.exception.UnknownDeviceException; -import io.supertokens.pluginInterface.totp.exception.UnknownUserIdTotpException; +import io.supertokens.pluginInterface.totp.exception.UnknownTotpUserIdException; import io.supertokens.pluginInterface.totp.exception.UsedCodeAlreadyExistsException; import io.supertokens.pluginInterface.totp.sqlStorage.TOTPSQLStorage; import io.supertokens.storageLayer.StorageLayer; @@ -94,34 +94,30 @@ public static TOTPDevice registerDevice(Main main, String userId, } } - private static TOTPDevice registerDeviceRecursive(AppIdentifierWithStorage appIdentifierWithStorage, TOTPDevice device, int deviceNameCounter) throws StorageQueryException, DeviceAlreadyExistsException, TenantOrAppNotFoundException, StorageTransactionLogicException { + private static TOTPDevice createDevice(AppIdentifierWithStorage appIdentifierWithStorage, TOTPDevice device) + throws DeviceAlreadyExistsException, StorageQueryException { TOTPSQLStorage totpStorage = appIdentifierWithStorage.getTOTPStorage(); - TOTPDevice newDevice = new TOTPDevice(device.userId, "TOTP Device " + deviceNameCounter, device.secretKey, device.period, device.skew, false); try { - totpStorage.createDevice(appIdentifierWithStorage, newDevice); - return newDevice; - } catch (DeviceAlreadyExistsException e) { - TOTPDevice[] devices = totpStorage.getDevices(appIdentifierWithStorage, device.userId); - // iterate through all devices to find device with same name - TOTPDevice existingDevice = Arrays.stream(devices).filter(d -> d.deviceName.equals(newDevice.deviceName)) - .findFirst().orElse(null); - - if (existingDevice != null) { - if (existingDevice.verified) { - // device with same name exists and is verified - return registerDeviceRecursive(appIdentifierWithStorage, device, deviceNameCounter + 1); - } else { - // device with same name exists but is not verified - // delete the device and retry - totpStorage.startTransaction(con -> { - totpStorage.deleteDevice_Transaction(con, appIdentifierWithStorage, newDevice.userId, newDevice.deviceName); - totpStorage.commitTransaction(con); - return null; - }); - return registerDeviceRecursive(appIdentifierWithStorage, device, deviceNameCounter); + return totpStorage.startTransaction(con -> { + try { + TOTPDevice existingDevice = totpStorage.getDeviceByName_Transaction(con, appIdentifierWithStorage, device.userId, device.deviceName); + if (existingDevice == null) { + return totpStorage.createDevice_Transaction(con, appIdentifierWithStorage, device); + } else if (!existingDevice.verified) { + totpStorage.deleteDevice_Transaction(con, appIdentifierWithStorage, device.userId, device.deviceName); + return totpStorage.createDevice_Transaction(con, appIdentifierWithStorage, device); + } else { + throw new StorageTransactionLogicException(new DeviceAlreadyExistsException()); + } + } catch (TenantOrAppNotFoundException | DeviceAlreadyExistsException e) { + throw new StorageTransactionLogicException(e); } + }); + } catch (StorageTransactionLogicException e) { + if (e.actualException instanceof DeviceAlreadyExistsException) { + throw (DeviceAlreadyExistsException) e.actualException; } - throw e; + throw new StorageQueryException(e.actualException); } } @@ -141,21 +137,33 @@ public static TOTPDevice registerDevice(AppIdentifierWithStorage appIdentifierWi TOTPSQLStorage totpStorage = appIdentifierWithStorage.getTOTPStorage(); if (deviceName != null) { - totpStorage.createDevice(appIdentifierWithStorage, device); - return device; + return createDevice(appIdentifierWithStorage, device); } // Find number of existing devices to set device name TOTPDevice[] devices = totpStorage.getDevices(appIdentifierWithStorage, userId); int verifiedDevicesCount = Arrays.stream(devices).filter(d -> d.verified).toArray().length; - return registerDeviceRecursive(appIdentifierWithStorage, device, verifiedDevicesCount + 1); + while (true) { + try { + return createDevice(appIdentifierWithStorage, new TOTPDevice( + device.userId, + "TOTP Device " + verifiedDevicesCount, + device.secretKey, + device.period, + device.skew, + device.verified + )); + } catch (DeviceAlreadyExistsException e){ + } + verifiedDevicesCount++; + } } private static void checkAndStoreCode(TenantIdentifierWithStorage tenantIdentifierWithStorage, Main main, String userId, TOTPDevice[] devices, String code) - throws InvalidTotpException, UnknownUserIdTotpException, + throws InvalidTotpException, UnknownTotpUserIdException, LimitReachedException, StorageQueryException, StorageTransactionLogicException, TenantOrAppNotFoundException { // Note that the TOTP cron runs every 1 hour, so all the expired tokens can stay @@ -277,7 +285,7 @@ private static void checkAndStoreCode(TenantIdentifierWithStorage tenantIdentifi try { totpSQLStorage.insertUsedCode_Transaction(con, tenantIdentifierWithStorage, newCode); totpSQLStorage.commitTransaction(con); - } catch (UsedCodeAlreadyExistsException | UnknownUserIdTotpException e) { + } catch (UsedCodeAlreadyExistsException | UnknownTotpUserIdException e) { throw new StorageTransactionLogicException(e); } @@ -300,16 +308,10 @@ private static void checkAndStoreCode(TenantIdentifierWithStorage tenantIdentifi throw (LimitReachedException) e.actualException; } else if (e.actualException instanceof InvalidTotpException) { throw (InvalidTotpException) e.actualException; - } else if (e.actualException instanceof UnknownUserIdTotpException) { - throw (UnknownUserIdTotpException) e.actualException; + } else if (e.actualException instanceof UnknownTotpUserIdException) { + throw (UnknownTotpUserIdException) e.actualException; } else if (e.actualException instanceof UsedCodeAlreadyExistsException) { - // retry the transaction after a small delay: - int delayInMs = (int) (Math.random() * 10 + 1); - try { - Thread.sleep(delayInMs); - } catch (InterruptedException ignored) { - // ignore the error and retry - } + throw new InvalidTotpException(); } else { throw e; } @@ -375,7 +377,7 @@ public static boolean verifyDevice(TenantIdentifierWithStorage tenantIdentifierW // This behaviour is okay so we can ignore it. try { checkAndStoreCode(tenantIdentifierWithStorage, main, userId, new TOTPDevice[] { matchingDevice }, code); - } catch (UnknownUserIdTotpException e) { + } catch (UnknownTotpUserIdException e) { // User must have deleted the device in parallel. throw new UnknownDeviceException(); } @@ -386,7 +388,7 @@ public static boolean verifyDevice(TenantIdentifierWithStorage tenantIdentifierW @TestOnly public static void verifyCode(Main main, String userId, String code) - throws InvalidTotpException, UnknownUserIdTotpException, LimitReachedException, + throws InvalidTotpException, UnknownTotpUserIdException, LimitReachedException, StorageQueryException, StorageTransactionLogicException, FeatureNotEnabledException { try { verifyCode(new TenantIdentifierWithStorage(null, null, null, StorageLayer.getStorage(main)), main, @@ -397,7 +399,7 @@ public static void verifyCode(Main main, String userId, String code) } public static void verifyCode(TenantIdentifierWithStorage tenantIdentifierWithStorage, Main main, String userId, String code) - throws InvalidTotpException, UnknownUserIdTotpException, LimitReachedException, + throws InvalidTotpException, UnknownTotpUserIdException, LimitReachedException, StorageQueryException, StorageTransactionLogicException, FeatureNotEnabledException, TenantOrAppNotFoundException { @@ -413,7 +415,7 @@ public static void verifyCode(TenantIdentifierWithStorage tenantIdentifierWithSt TOTPDevice[] devices = totpStorage.getDevices(tenantIdentifierWithStorage.toAppIdentifier(), userId); if (devices.length == 0) { // No devices found. So we can't verify the code anyway. - throw new UnknownUserIdTotpException(); + throw new UnknownTotpUserIdException(); } // Filter out unverified devices: @@ -425,7 +427,7 @@ public static void verifyCode(TenantIdentifierWithStorage tenantIdentifierWithSt // can ignore it. try { checkAndStoreCode(tenantIdentifierWithStorage, main, userId, devices, code); - } catch (UnknownUserIdTotpException e) { + } catch (UnknownTotpUserIdException e) { // User must have deleted the device in parallel // since they cannot un-verify a device (no API exists) throw e; diff --git a/src/main/java/io/supertokens/webserver/api/totp/VerifyTotpAPI.java b/src/main/java/io/supertokens/webserver/api/totp/VerifyTotpAPI.java index be575ab3a..b413332df 100644 --- a/src/main/java/io/supertokens/webserver/api/totp/VerifyTotpAPI.java +++ b/src/main/java/io/supertokens/webserver/api/totp/VerifyTotpAPI.java @@ -13,7 +13,7 @@ import io.supertokens.pluginInterface.exceptions.StorageTransactionLogicException; import io.supertokens.pluginInterface.multitenancy.TenantIdentifierWithStorage; import io.supertokens.pluginInterface.multitenancy.exceptions.TenantOrAppNotFoundException; -import io.supertokens.pluginInterface.totp.exception.UnknownUserIdTotpException; +import io.supertokens.pluginInterface.totp.exception.UnknownTotpUserIdException; import io.supertokens.totp.Totp; import io.supertokens.totp.exceptions.InvalidTotpException; import io.supertokens.totp.exceptions.LimitReachedException; @@ -78,7 +78,7 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws I } catch (InvalidTotpException e) { result.addProperty("status", "INVALID_TOTP_ERROR"); super.sendJsonResponse(200, result, resp); - } catch (UnknownUserIdTotpException e) { + } catch (UnknownTotpUserIdException e) { result.addProperty("status", "UNKNOWN_USER_ID_ERROR"); super.sendJsonResponse(200, result, resp); } catch (LimitReachedException e) { diff --git a/src/test/java/io/supertokens/test/StorageLayerTest.java b/src/test/java/io/supertokens/test/StorageLayerTest.java index 9ff015a38..df64313bd 100644 --- a/src/test/java/io/supertokens/test/StorageLayerTest.java +++ b/src/test/java/io/supertokens/test/StorageLayerTest.java @@ -11,8 +11,9 @@ import io.supertokens.pluginInterface.multitenancy.exceptions.TenantOrAppNotFoundException; import io.supertokens.pluginInterface.totp.TOTPDevice; import io.supertokens.pluginInterface.totp.TOTPUsedCode; +import io.supertokens.pluginInterface.totp.exception.DeviceAlreadyExistsException; import io.supertokens.pluginInterface.totp.exception.UnknownDeviceException; -import io.supertokens.pluginInterface.totp.exception.UnknownUserIdTotpException; +import io.supertokens.pluginInterface.totp.exception.UnknownTotpUserIdException; import io.supertokens.pluginInterface.totp.exception.UsedCodeAlreadyExistsException; import io.supertokens.pluginInterface.totp.sqlStorage.TOTPSQLStorage; import io.supertokens.storageLayer.StorageLayer; @@ -48,7 +49,7 @@ public static void insertUsedCodeUtil(TOTPSQLStorage storage, TOTPUsedCode usedC storage.insertUsedCode_Transaction(con, new TenantIdentifier(null, null, null), usedCode); storage.commitTransaction(con); return null; - } catch (UnknownUserIdTotpException | UsedCodeAlreadyExistsException e) { + } catch (UnknownTotpUserIdException | UsedCodeAlreadyExistsException e) { throw new StorageTransactionLogicException(e); } catch (TenantOrAppNotFoundException e) { throw new IllegalStateException(e); @@ -85,7 +86,7 @@ public void totpCodeLengthTest() throws Exception { TOTPDevice d1 = new TOTPDevice("user", "d1", "secret", 30, 1, false); storage.createDevice(new AppIdentifier(null, null), d1); - + // Try code with length > 8 try { TOTPUsedCode code = new TOTPUsedCode("user", "123456789", true, nextDay, now); diff --git a/src/test/java/io/supertokens/test/totp/TOTPStorageTest.java b/src/test/java/io/supertokens/test/totp/TOTPStorageTest.java index 5ef2a3c42..e03169d2a 100644 --- a/src/test/java/io/supertokens/test/totp/TOTPStorageTest.java +++ b/src/test/java/io/supertokens/test/totp/TOTPStorageTest.java @@ -15,7 +15,7 @@ import io.supertokens.pluginInterface.totp.TOTPUsedCode; import io.supertokens.pluginInterface.totp.exception.DeviceAlreadyExistsException; import io.supertokens.pluginInterface.totp.exception.UnknownDeviceException; -import io.supertokens.pluginInterface.totp.exception.UnknownUserIdTotpException; +import io.supertokens.pluginInterface.totp.exception.UnknownTotpUserIdException; import io.supertokens.pluginInterface.totp.exception.UsedCodeAlreadyExistsException; import io.supertokens.pluginInterface.totp.sqlStorage.TOTPSQLStorage; import io.supertokens.storageLayer.StorageLayer; @@ -95,7 +95,7 @@ public static void insertUsedCodesUtil(TOTPSQLStorage storage, TOTPUsedCode[] us for (TOTPUsedCode usedCode : usedCodes) { storage.insertUsedCode_Transaction(con, new TenantIdentifier(null, null, null), usedCode); } - } catch (UnknownUserIdTotpException | UsedCodeAlreadyExistsException e) { + } catch (UnknownTotpUserIdException | UsedCodeAlreadyExistsException e) { throw new StorageTransactionLogicException(e); } catch (TenantOrAppNotFoundException e) { throw new IllegalStateException(e); From 6be5b8cfdfda60209448fdd699bb5b2f09e9fdc5 Mon Sep 17 00:00:00 2001 From: Sattvik Chakravarthy Date: Thu, 28 Sep 2023 15:55:42 +0530 Subject: [PATCH 15/16] fix: cleanup --- src/main/java/io/supertokens/totp/Totp.java | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/main/java/io/supertokens/totp/Totp.java b/src/main/java/io/supertokens/totp/Totp.java index 63e7b08f0..e5f715c3b 100644 --- a/src/main/java/io/supertokens/totp/Totp.java +++ b/src/main/java/io/supertokens/totp/Totp.java @@ -425,13 +425,11 @@ public static void verifyCode(TenantIdentifierWithStorage tenantIdentifierWithSt // another API call. We will still check the code against the updated set of // devices and store it in the used codes table. This behaviour is okay so we // can ignore it. - try { - checkAndStoreCode(tenantIdentifierWithStorage, main, userId, devices, code); - } catch (UnknownTotpUserIdException e) { - // User must have deleted the device in parallel - // since they cannot un-verify a device (no API exists) - throw e; - } + + // UnknownTotpUserIdException will be thrown when + // the User has deleted the device in parallel + // since they cannot un-verify a device (no API exists) + checkAndStoreCode(tenantIdentifierWithStorage, main, userId, devices, code); } @TestOnly From 0139624c59b8a80a8d9fd81dc07d6990d75e117c Mon Sep 17 00:00:00 2001 From: Sattvik Chakravarthy Date: Thu, 28 Sep 2023 16:24:22 +0530 Subject: [PATCH 16/16] fix: tests --- .../supertokens/test/totp/TOTPRecipeTest.java | 18 ++++++++++++- .../totp/api/CreateTotpDeviceAPITest.java | 26 ++++++++++++++++++- .../test/totp/api/MultitenantAPITest.java | 6 ++++- .../test/totp/api/VerifyTotpAPITest.java | 4 +-- 4 files changed, 49 insertions(+), 5 deletions(-) diff --git a/src/test/java/io/supertokens/test/totp/TOTPRecipeTest.java b/src/test/java/io/supertokens/test/totp/TOTPRecipeTest.java index ecf5faa11..88db473ae 100644 --- a/src/test/java/io/supertokens/test/totp/TOTPRecipeTest.java +++ b/src/test/java/io/supertokens/test/totp/TOTPRecipeTest.java @@ -32,6 +32,7 @@ import io.supertokens.pluginInterface.totp.TOTPUsedCode; import io.supertokens.pluginInterface.totp.exception.DeviceAlreadyExistsException; import io.supertokens.pluginInterface.totp.exception.UnknownDeviceException; +import io.supertokens.pluginInterface.totp.exception.UnknownTotpUserIdException; import io.supertokens.pluginInterface.totp.sqlStorage.TOTPSQLStorage; import io.supertokens.storageLayer.StorageLayer; import io.supertokens.test.TestingProcessManager; @@ -146,6 +147,10 @@ public void createDeviceTest() throws Exception { TOTPDevice device = Totp.registerDevice(main, "user", "device1", 1, 30); assert !Objects.equals(device.secretKey, ""); + // Verify device + String validTotp = TOTPRecipeTest.generateTotpCode(main, device); + Totp.verifyDevice(main, "user", "device1", validTotp); + // Create same device again (should fail) assertThrows(DeviceAlreadyExistsException.class, () -> Totp.registerDevice(main, "user", "device1", 1, 30)); @@ -167,7 +172,7 @@ public void createDeviceAndVerifyCodeTest() throws Exception { Totp.verifyDevice(main, "user", device.deviceName, generateTotpCode(main, device, -1)); // Try login with non-existent user: - assertThrows(InvalidTotpException.class, + assertThrows(UnknownTotpUserIdException.class, () -> Totp.verifyCode(main, "non-existent-user", "any-code")); // {Code: [INVALID, VALID]} * {Devices: [verified, unverfied]} @@ -555,4 +560,15 @@ public void deleteExpiredTokensCronIntervalTest() throws Exception { assert DeleteExpiredTotpTokens.getInstance(main).getIntervalTimeSeconds() == 60 * 60; } + @Test + public void testRegisterDeviceWithSameNameAsAnUnverifiedDevice() throws Exception { + TestSetupResult result = defaultInit(); + if (result == null) { + return; + } + Main main = result.process.getProcess(); + + Totp.registerDevice(main, "user", "device1", 1, 30); + Totp.registerDevice(main, "user", "device1", 1, 30); + } } diff --git a/src/test/java/io/supertokens/test/totp/api/CreateTotpDeviceAPITest.java b/src/test/java/io/supertokens/test/totp/api/CreateTotpDeviceAPITest.java index 7fe30559f..4aa922e2d 100644 --- a/src/test/java/io/supertokens/test/totp/api/CreateTotpDeviceAPITest.java +++ b/src/test/java/io/supertokens/test/totp/api/CreateTotpDeviceAPITest.java @@ -14,6 +14,7 @@ import io.supertokens.test.httpRequest.HttpResponseException; import io.supertokens.test.totp.TOTPRecipeTest; import io.supertokens.test.totp.TotpLicenseTest; +import io.supertokens.totp.Totp; import org.junit.AfterClass; import org.junit.Before; import org.junit.Rule; @@ -141,6 +142,7 @@ public void testApi() throws Exception { assert res.get("deviceName").getAsString().equals("d1"); // try again with same device name: + // This should replace the previous device JsonObject res2 = HttpRequestForTesting.sendJsonPOSTRequest( process.getProcess(), "", @@ -151,8 +153,30 @@ public void testApi() throws Exception { null, Utils.getCdiVersionStringLatestForTests(), "totp"); + assert res2.get("status").getAsString().equals("OK"); + assert res.get("deviceName").getAsString().equals("d1"); + + // verify d1 + { + TOTPDevice device = Totp.getDevices(process.getProcess(), "user-id" )[0]; + String validTotp = TOTPRecipeTest.generateTotpCode(process.getProcess(), device); + Totp.verifyDevice(process.getProcess(), "user-id", "d1", validTotp); + } + + // try again with same device name: + res2 = HttpRequestForTesting.sendJsonPOSTRequest( + process.getProcess(), + "", + "http://localhost:3567/recipe/totp/device", + body, + 1000, + 1000, + null, + Utils.getCdiVersionStringLatestForTests(), + "totp"); assert res2.get("status").getAsString().equals("DEVICE_ALREADY_EXISTS_ERROR"); - + assert res.get("deviceName").getAsString().equals("d1"); + // try without passing deviceName: body.remove("deviceName"); JsonObject res3 = HttpRequestForTesting.sendJsonPOSTRequest( diff --git a/src/test/java/io/supertokens/test/totp/api/MultitenantAPITest.java b/src/test/java/io/supertokens/test/totp/api/MultitenantAPITest.java index 3479cd320..df8cac9fb 100644 --- a/src/test/java/io/supertokens/test/totp/api/MultitenantAPITest.java +++ b/src/test/java/io/supertokens/test/totp/api/MultitenantAPITest.java @@ -37,6 +37,7 @@ import io.supertokens.test.httpRequest.HttpResponseException; import io.supertokens.test.totp.TOTPRecipeTest; import io.supertokens.thirdparty.InvalidProviderConfigException; +import io.supertokens.totp.Totp; import io.supertokens.utils.SemVer; import org.junit.After; import org.junit.AfterClass; @@ -249,9 +250,12 @@ public void testDevicesWorkAppWide() throws Exception { int userCount = 1; for (TenantIdentifier tenant1 : tenants) { createDevice(tenant1, "user" + userCount); + TOTPDevice device = Totp.getDevices(t1.withStorage(StorageLayer.getStorage(tenant1, process.getProcess())).toAppIdentifierWithStorage(), "user" + userCount)[0]; + String validTotp = TOTPRecipeTest.generateTotpCode(process.getProcess(), device); + verifyDevice(tenant1, "user" + userCount, validTotp); for (TenantIdentifier tenant2 : tenants) { - createDeviceAlreadyExists(tenant2, "user1"); + createDeviceAlreadyExists(tenant2, "user" + userCount); } userCount++; diff --git a/src/test/java/io/supertokens/test/totp/api/VerifyTotpAPITest.java b/src/test/java/io/supertokens/test/totp/api/VerifyTotpAPITest.java index f4930dc8b..08c836586 100644 --- a/src/test/java/io/supertokens/test/totp/api/VerifyTotpAPITest.java +++ b/src/test/java/io/supertokens/test/totp/api/VerifyTotpAPITest.java @@ -191,7 +191,7 @@ public void testApi() throws Exception { assert res3.get("retryAfterMs") != null; // wait for cooldown to end (1s) - Thread.sleep(1000); + Thread.sleep(1200); // should pass now on valid code String validTotp = generateTotpCode(process.getProcess(), device); @@ -248,7 +248,7 @@ public void testApi() throws Exception { null, Utils.getCdiVersionStringLatestForTests(), "totp"); - assert res5.get("status").getAsString().equals("INVALID_TOTP_ERROR"); + assert res5.get("status").getAsString().equals("UNKNOWN_USER_ID_ERROR"); } process.kill();