Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

refactor: Replace TOTP_NOT_ENABLED_ERROR status and make deviceName optional #729

Merged
merged 20 commits into from
Sep 28, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
06b2cf3
refactor: Dont send TOTP_NOT_ENABLED_ERROR status
KShivendu Jun 22, 2023
693b70e
refactor: Add comments
KShivendu Jun 23, 2023
b10a23c
chores: Remove extra comments
KShivendu Jun 23, 2023
1a14e22
Merge branch 'feat/mfa' into refactor/avoid-totp-not-enabled
KShivendu Jun 26, 2023
14209d8
refactor: Completely replace totp not enabled error with unknown devi…
KShivendu Jun 27, 2023
157d94a
refactor: Remove Totp not enabled error
KShivendu Jun 27, 2023
fc9f706
feat: Make device name optional and generate it from number of existi…
KShivendu Jun 27, 2023
ec6dd7e
Replace TotpNotEnabledError with UnknownUserIdTotpError
KShivendu Jun 28, 2023
98b24ae
refactor: Recursively generate device name when it already exists
KShivendu Jun 29, 2023
646b0b8
refactor: Remove redundant arguments
KShivendu Jun 29, 2023
430704e
feat: Remove the param to allow unverified devices from the verify to…
KShivendu Jul 3, 2023
a7d2f27
Merge branch 'master' into refactor/avoid-totp-not-enabled
KShivendu Sep 27, 2023
363492c
feat: Reject unverified devices
KShivendu Sep 27, 2023
5b68216
Merge branch 'feat/mfa' into refactor/avoid-totp-not-enabled
KShivendu Sep 27, 2023
59373c2
feat: Add UNKNOWN_USER_ID_ERROR to verify totp api
KShivendu Sep 27, 2023
84b1b9d
feat: Throw Unknown user id error when device gets deleted during ver…
KShivendu Sep 27, 2023
7ef46d1
Merge branch 'feat/mfa' into refactor/avoid-totp-not-enabled
sattvikc Sep 28, 2023
29f9643
fix: core fixes
sattvikc Sep 28, 2023
6be5b8c
fix: cleanup
sattvikc Sep 28, 2023
0139624
fix: tests
sattvikc Sep 28, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 27 additions & 37 deletions src/main/java/io/supertokens/totp/Totp.java
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
Expand All @@ -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:
Expand All @@ -337,8 +337,12 @@ 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) {
rishabhpoddar marked this conversation as resolved.
Show resolved Hide resolved
// User must have deleted the device in parallel.
throw new UnknownDeviceException();
}
// Will reach here only if the code is valid:
totpStorage.markDeviceAsVerified(tenantIdentifierWithStorage.toAppIdentifier(), userId, deviceName);
return true; // Newly verified
Expand All @@ -347,7 +351,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,
Expand All @@ -359,7 +363,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 {

Expand All @@ -374,7 +378,8 @@ 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();
// No devices found. So we can't verify the code anyway.
throw new InvalidTotpException();
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe it's better to throw an UnknownUserIdTotpException, since the input to this API is also the userId

Copy link
Contributor

Choose a reason for hiding this comment

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

This would be an additional status code that is sent to the backend sdk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}

// Filter out unverified devices:
Expand All @@ -386,13 +391,19 @@ 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) {
rishabhpoddar marked this conversation as resolved.
Show resolved Hide resolved
// User must have deleted the device in parallel
// since they cannot un-verify a device (no API exists)
throw new InvalidTotpException();
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe it's better to throw an UnknownUserIdTotpException, since the input to this API is also the userId

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}
}

@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)),
Expand All @@ -407,7 +418,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();

Expand All @@ -432,12 +443,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;
}

Expand All @@ -448,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);
Expand All @@ -460,25 +464,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);
Expand All @@ -488,13 +481,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;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -75,9 +74,6 @@ 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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,15 @@

import com.google.gson.JsonObject;

import io.supertokens.AppIdentifierWithStorageAndUserIdMapping;
import io.supertokens.Main;
import io.supertokens.TenantIdentifierWithStorageAndUserIdMapping;
import io.supertokens.featureflag.exceptions.FeatureNotEnabledException;
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.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;
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
14 changes: 7 additions & 7 deletions src/test/java/io/supertokens/test/totp/TOTPRecipeTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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,
() -> Totp.verifyCode(main, "non-existent-user", "any-code", true));

// {Code: [INVALID, VALID]} * {Devices: [VERIFIED_ONLY, ALL]}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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"));
Expand Down Expand Up @@ -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");
Expand All @@ -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:
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Loading