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 3 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
9 changes: 4 additions & 5 deletions src/main/java/io/supertokens/inmemorydb/Start.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -1257,7 +1256,7 @@ public int countUsersEnabledTotpAndActiveSince(AppIdentifier appIdentifier, long
throw new StorageQueryException(e);
}
}

@Override
public int countUsersEnabledMfa(AppIdentifier appIdentifier) throws StorageQueryException {
try {
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -2687,7 +2686,7 @@ public void insertUsedCode_Transaction(TransactionConnection con, TenantIdentifi
Config.getConfig(this).getTotpUsersTable(),
new String[]{"app_id", "user_id"},
Copy link
Contributor

Choose a reason for hiding this comment

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

see my comment in plugin interface PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe you're talking about replacing TotpNotEnabledError with new UnknownUserIdError. If yes, done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please check and confirm.

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

new Object[]{tenantIdentifier.getAppId(), usedCodeObj.userId})) {
throw new TotpNotEnabledException();
throw new UnknownDeviceException();

} else if (isForeignKeyConstraintError(
e.getMessage(),
Expand Down
18 changes: 11 additions & 7 deletions src/main/java/io/supertokens/totp/Totp.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -107,6 +106,11 @@ public static TOTPDevice registerDevice(AppIdentifierWithStorage appIdentifierWi
}

TOTPSQLStorage totpStorage = appIdentifierWithStorage.getTOTPStorage();

if (deviceName == null) {
rishabhpoddar marked this conversation as resolved.
Show resolved Hide resolved
TOTPDevice[] devices = totpStorage.getDevices(appIdentifierWithStorage, userId);
deviceName = "TOTP Device " + (devices.length + 1);
rishabhpoddar marked this conversation as resolved.
Show resolved Hide resolved
}

String secret = generateSecret();
TOTPDevice device = new TOTPDevice(userId, deviceName, secret, period, skew, false);
Expand All @@ -118,7 +122,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
Expand Down Expand Up @@ -241,7 +245,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) {
rishabhpoddar marked this conversation as resolved.
Show resolved Hide resolved
throw new StorageTransactionLogicException(e);
}

Expand All @@ -264,8 +268,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) {
rishabhpoddar marked this conversation as resolved.
Show resolved Hide resolved
// retry the transaction after a small delay:
int delayInMs = (int) (Math.random() * 10 + 1);
Expand Down Expand Up @@ -339,7 +343,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();
}
Expand Down Expand Up @@ -393,7 +397,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();
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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down
6 changes: 3 additions & 3 deletions src/test/java/io/supertokens/test/StorageLayerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -47,15 +47,15 @@ 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);
}
});
} 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;
Expand Down
5 changes: 0 additions & 5 deletions src/test/java/io/supertokens/test/totp/TOTPRecipeTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
15 changes: 6 additions & 9 deletions src/test/java/io/supertokens/test/totp/TOTPStorageTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand Down Expand Up @@ -89,15 +86,15 @@ 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 -> {
try {
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);
Expand All @@ -108,8 +105,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;
}
Expand Down Expand Up @@ -404,7 +401,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())
Expand All @@ -423,7 +420,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())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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(
Expand All @@ -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();
Expand Down