Skip to content

Commit 4b6d4c0

Browse files
sattvikcsattvikc
authored
fix: totp (#967)
* fix: totp * fix: changelog and test * fix: version
1 parent 2f4776e commit 4b6d4c0

File tree

8 files changed

+111
-27
lines changed

8 files changed

+111
-27
lines changed

‎CHANGELOG.md

+5
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,11 @@ All notable changes to this project will be documented in this file.
55
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres
66
to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
77

8+
## [9.0.1] - 2024-03-20
9+
10+
- Fixes verify TOTP and verify device APIs to treat any code as invalid
11+
- Fixes the computation of the number of failed attempts when return `INVALID_TOTP_ERROR`
12+
813
## [9.0.0] - 2024-03-13
914

1015
### Added

‎build.gradle

+1-1
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ compileTestJava { options.encoding = "UTF-8" }
1919
// }
2020
//}
2121

22-
version = "9.0.0"
22+
version = "9.0.1"
2323

2424

2525
repositories {

‎src/main/java/io/supertokens/totp/Totp.java

+18-4
Original file line numberDiff line numberDiff line change
@@ -203,17 +203,28 @@ private static void checkAndStoreCode(TenantIdentifier tenantIdentifier, Storage
203203

204204
// N represents # of invalid attempts that will trigger rate limiting:
205205
int N = Config.getConfig(tenantIdentifier, main).getTotpMaxAttempts(); // (Default 5)
206-
// Count # of contiguous invalids in latest N attempts (stop at first valid):
207-
long invalidOutOfN = Arrays.stream(usedCodes).limit(N).takeWhile(usedCode -> !usedCode.isValid)
208-
.count();
206+
207+
// filter only invalid codes, stop at first valid code
208+
TOTPUsedCode[] invalidUsedCodes = Arrays.stream(usedCodes).limit(N).takeWhile(
209+
usedCode -> !usedCode.isValid).toArray(TOTPUsedCode[]::new);
210+
209211
int rateLimitResetTimeInMs = Config.getConfig(tenantIdentifier, main)
210212
.getTotpRateLimitCooldownTimeSec() *
211213
1000; // (Default 15 mins)
212214

215+
// Count how many of the latest invalid codes fall under the rate limiting compared
216+
// to the latest invalid attempt
217+
long invalidOutOfN = 0;
218+
if (invalidUsedCodes.length > 0) {
219+
invalidOutOfN = Arrays.stream(invalidUsedCodes).limit(N).takeWhile(
220+
usedCode -> usedCode.createdTime > invalidUsedCodes[0].createdTime - rateLimitResetTimeInMs
221+
).count();
222+
}
223+
213224
// Check if the user has been rate limited:
214225
if (invalidOutOfN == N) {
215226
// All of the latest N attempts were invalid:
216-
long latestInvalidCodeCreatedTime = usedCodes[0].createdTime;
227+
long latestInvalidCodeCreatedTime = invalidUsedCodes[0].createdTime;
217228
long now = System.currentTimeMillis();
218229

219230
if (now - latestInvalidCodeCreatedTime < rateLimitResetTimeInMs) {
@@ -225,6 +236,9 @@ private static void checkAndStoreCode(TenantIdentifier tenantIdentifier, Storage
225236
// If we insert the used code here, then it will further delay the user from
226237
// being able to login. So not inserting it here.
227238
}
239+
240+
// since we are past the cool down period, the user can retry all the attempts
241+
invalidOutOfN = 0;
228242
}
229243

230244
// Check if the code is valid for any device:

‎src/main/java/io/supertokens/webserver/api/totp/VerifyTotpAPI.java

-3
Original file line numberDiff line numberDiff line change
@@ -46,9 +46,6 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws I
4646
if (userId.isEmpty()) {
4747
throw new ServletException(new BadRequestException("userId cannot be empty"));
4848
}
49-
if (totp.length() != 6) {
50-
throw new ServletException(new BadRequestException("totp must be 6 characters long"));
51-
}
5249

5350
JsonObject result = new JsonObject();
5451

‎src/main/java/io/supertokens/webserver/api/totp/VerifyTotpDeviceAPI.java

-3
Original file line numberDiff line numberDiff line change
@@ -50,9 +50,6 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws I
5050
if (deviceName.isEmpty()) {
5151
throw new ServletException(new BadRequestException("deviceName cannot be empty"));
5252
}
53-
if (totp.length() != 6) {
54-
throw new ServletException(new BadRequestException("totp must be 6 characters long"));
55-
}
5653

5754
JsonObject result = new JsonObject();
5855

‎src/test/java/io/supertokens/test/totp/TOTPRecipeTest.java

+22-2
Original file line numberDiff line numberDiff line change
@@ -348,13 +348,33 @@ public void rateLimitCooldownTest() throws Exception {
348348
// Wait for 1 second (Should cool down rate limiting):
349349
Thread.sleep(1000);
350350
// But again try with invalid code:
351-
assertThrows(InvalidTotpException.class, () -> Totp.verifyCode(main, "user", "invalid0"));
351+
InvalidTotpException invalidTotpException = assertThrows(InvalidTotpException.class,
352+
() -> Totp.verifyCode(main, "user", "invalid0"));
353+
assertEquals(1, invalidTotpException.currentAttempts);
354+
invalidTotpException = assertThrows(InvalidTotpException.class, () -> Totp.verifyCode(main, "user", "invalid0"));
355+
assertEquals(2, invalidTotpException.currentAttempts);
356+
invalidTotpException = assertThrows(InvalidTotpException.class, () -> Totp.verifyCode(main, "user", "invalid0"));
357+
assertEquals(3, invalidTotpException.currentAttempts);
358+
352359
// This triggered rate limiting again. So even valid codes will fail for
353360
// another cooldown period:
354-
assertThrows(LimitReachedException.class,
361+
LimitReachedException limitReachedException = assertThrows(LimitReachedException.class,
355362
() -> Totp.verifyCode(main, "user", generateTotpCode(main, device)));
363+
assertEquals(3, limitReachedException.currentAttempts);
356364
// Wait for 1 second (Should cool down rate limiting):
357365
Thread.sleep(1000);
366+
367+
// test that after cool down, we can retry invalid codes N times again
368+
invalidTotpException = assertThrows(InvalidTotpException.class,
369+
() -> Totp.verifyCode(main, "user", "invalid0"));
370+
assertEquals(1, invalidTotpException.currentAttempts);
371+
invalidTotpException = assertThrows(InvalidTotpException.class, () -> Totp.verifyCode(main, "user", "invalid0"));
372+
assertEquals(2, invalidTotpException.currentAttempts);
373+
invalidTotpException = assertThrows(InvalidTotpException.class, () -> Totp.verifyCode(main, "user", "invalid0"));
374+
assertEquals(3, invalidTotpException.currentAttempts);
375+
376+
Thread.sleep(1100);
377+
358378
// Now try with valid code:
359379
Totp.verifyCode(main, "user", generateTotpCode(main, device));
360380
// Now invalid code shouldn't trigger rate limiting. Unless you do it N times:

‎src/test/java/io/supertokens/test/totp/api/VerifyTotpAPITest.java

+32-6
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@
2323
import org.junit.Test;
2424
import org.junit.rules.TestRule;
2525

26+
import java.io.IOException;
27+
2628
import static io.supertokens.test.totp.TOTPRecipeTest.generateTotpCode;
2729
import static org.junit.Assert.*;
2830

@@ -56,6 +58,21 @@ private Exception verifyTotpCodeRequest(TestingProcessManager.TestingProcess pro
5658
"totp"));
5759
}
5860

61+
private void verifyTotpRequestThatReturnsInvalidCode(TestingProcessManager.TestingProcess process, JsonObject body)
62+
throws HttpResponseException, IOException {
63+
JsonObject resp = HttpRequestForTesting.sendJsonPOSTRequest(
64+
process.getProcess(),
65+
"",
66+
"http://localhost:3567/recipe/totp/verify",
67+
body,
68+
1000,
69+
1000,
70+
null,
71+
Utils.getCdiVersionStringLatestForTests(),
72+
"totp");
73+
assertEquals("INVALID_TOTP_ERROR", resp.get("status").getAsString());
74+
}
75+
5976
private void checkFieldMissingErrorResponse(Exception ex, String fieldName) {
6077
assert ex instanceof HttpResponseException;
6178
HttpResponseException e = (HttpResponseException) ex;
@@ -149,18 +166,27 @@ public void testApi() throws Exception {
149166
checkResponseErrorContains(e, "userId cannot be empty"); // Note that this is not a field missing error
150167

151168
body.addProperty("userId", device.userId);
152-
e = verifyTotpCodeRequest(process, body);
153-
checkResponseErrorContains(e, "totp must be 6 characters long");
169+
verifyTotpRequestThatReturnsInvalidCode(process, body);
170+
171+
Thread.sleep(1100);
154172

155173
// test totp of length 5:
156174
body.addProperty("totp", "12345");
157-
e = verifyTotpCodeRequest(process, body);
158-
checkResponseErrorContains(e, "totp must be 6 characters long");
175+
verifyTotpRequestThatReturnsInvalidCode(process, body);
176+
177+
Thread.sleep(1100);
178+
179+
// test totp of alphabets:
180+
body.addProperty("totp", "abcd");
181+
verifyTotpRequestThatReturnsInvalidCode(process, body);
182+
183+
Thread.sleep(1100);
159184

160185
// test totp of length 8:
161186
body.addProperty("totp", "12345678");
162-
e = verifyTotpCodeRequest(process, body);
163-
checkResponseErrorContains(e, "totp must be 6 characters long");
187+
verifyTotpRequestThatReturnsInvalidCode(process, body);
188+
189+
Thread.sleep(1100);
164190

165191
// but let's pass invalid code first
166192
body.addProperty("totp", "123456");

‎src/test/java/io/supertokens/test/totp/api/VerifyTotpDeviceAPITest.java

+33-8
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@
2121
import org.junit.Test;
2222
import org.junit.rules.TestRule;
2323

24+
import java.io.IOException;
25+
2426
import static org.junit.Assert.*;
2527

2628
public class VerifyTotpDeviceAPITest {
@@ -53,6 +55,21 @@ private Exception updateDeviceRequest(TestingProcessManager.TestingProcess proce
5355
"totp"));
5456
}
5557

58+
private void requestWithInvalidCode(TestingProcessManager.TestingProcess process, JsonObject body)
59+
throws HttpResponseException, IOException {
60+
JsonObject resp = HttpRequestForTesting.sendJsonPOSTRequest(
61+
process.getProcess(),
62+
"",
63+
"http://localhost:3567/recipe/totp/device/verify",
64+
body,
65+
1000,
66+
1000,
67+
null,
68+
Utils.getCdiVersionStringLatestForTests(),
69+
"totp");
70+
assertEquals("INVALID_TOTP_ERROR", resp.get("status").getAsString());
71+
}
72+
5673
private void checkFieldMissingErrorResponse(Exception ex, String fieldName) {
5774
assert ex instanceof HttpResponseException;
5875
HttpResponseException e = (HttpResponseException) ex;
@@ -126,7 +143,7 @@ public void testApi() throws Exception {
126143
checkFieldMissingErrorResponse(e, "totp");
127144
}
128145

129-
// Invalid userId/deviceName/skew/period
146+
// Invalid userId/deviceName/totp
130147
{
131148
body.addProperty("totp", "");
132149
Exception e = updateDeviceRequest(process, body);
@@ -137,18 +154,27 @@ public void testApi() throws Exception {
137154
checkResponseErrorContains(e, "deviceName cannot be empty");
138155

139156
body.addProperty("deviceName", device.deviceName);
140-
e = updateDeviceRequest(process, body);
141-
checkResponseErrorContains(e, "totp must be 6 characters long");
157+
requestWithInvalidCode(process, body);
158+
159+
Thread.sleep(1100);
142160

143161
// test totp of length 5:
144162
body.addProperty("totp", "12345");
145-
e = updateDeviceRequest(process, body);
146-
checkResponseErrorContains(e, "totp must be 6 characters long");
163+
requestWithInvalidCode(process, body);
164+
165+
Thread.sleep(1100);
147166

148167
// test totp of length 8:
149168
body.addProperty("totp", "12345678");
150-
e = updateDeviceRequest(process, body);
151-
checkResponseErrorContains(e, "totp must be 6 characters long");
169+
requestWithInvalidCode(process, body);
170+
171+
Thread.sleep(1100);
172+
173+
// test totp of length alphabets:
174+
body.addProperty("totp", "abcd");
175+
requestWithInvalidCode(process, body);
176+
177+
Thread.sleep(2100);
152178

153179
// but let's pass invalid code first
154180
body.addProperty("totp", "123456");
@@ -247,5 +273,4 @@ public void testApi() throws Exception {
247273
process.kill();
248274
assertNotNull(process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.STOPPED));
249275
}
250-
251276
}

0 commit comments

Comments
 (0)