Re-organize Throttling Order and Check maxNumberOfClientsPerDocument (m…

…icrosoft#15921) Move throttle before verifyStorageToken. Check the maxNumberOfClientsPerDocumenth right after throttling check for connect doc
tylerbutler · Jun 8, 2023 · ecdf328 · ecdf328
1 parent 84cd58f
commit ecdf328
Show file tree

Hide file tree

Showing 3 changed files with 54 additions and 53 deletions.
diff --git a/server/routerlicious/packages/lambdas/src/alfred/index.ts b/server/routerlicious/packages/lambdas/src/alfred/index.ts
@@ -289,13 +289,55 @@ export function configureWebSocketServices(
 			if (throttleErrorPerTenant) {
 				return Promise.reject(throttleErrorPerTenant);
 			}
+
 			if (!message.token) {
 				throw new NetworkError(403, "Must provide an authorization token");
 			}
 
 			// Validate token signature and claims
 			const token = message.token;
 			const claims = validateTokenClaims(token, message.id, message.tenantId);
+			const clientId = generateClientId();
+
+			const lumberjackProperties = {
+				...getLumberBaseProperties(claims.documentId, claims.tenantId),
+				[CommonProperties.clientId]: clientId,
+			};
+
+			const connectDocumentGetClientsMetric = Lumberjack.newLumberMetric(
+				LumberEventName.ConnectDocumentGetClients,
+				lumberjackProperties,
+			);
+			const clients = await clientManager
+				.getClients(claims.tenantId, claims.documentId)
+				.then((response) => {
+					connectDocumentGetClientsMetric.success(
+						"Successfully got clients from client manager",
+					);
+					return response;
+				})
+				.catch(async (err) => {
+					const errMsg = `Failed to get clients. Error: ${safeStringify(
+						err,
+						undefined,
+						2,
+					)}`;
+					connectDocumentGetClientsMetric.error(
+						"Failed to get clients during connectDocument",
+						err,
+					);
+					return handleServerError(logger, errMsg, claims.documentId, claims.tenantId);
+				});
+
+			if (clients.length > maxNumberOfClientsPerDocument) {
+				throw new NetworkError(
+					429,
+					"Too Many Clients Connected to Document",
+					true /* canRetry */,
+					false /* isFatal */,
+					5 * 60 * 1000 /* retryAfterMs (5 min) */,
+				);
+			}
 
 			try {
 				await tenantManager.verifyToken(claims.tenantId, token);
@@ -312,7 +354,6 @@ export function configureWebSocketServices(
 				return handleServerError(logger, errMsg, claims.documentId, claims.tenantId);
 			}
 
-			const clientId = generateClientId();
 			const room: IRoom = {
 				tenantId: claims.tenantId,
 				documentId: claims.documentId,
@@ -380,46 +421,6 @@ export function configureWebSocketServices(
 				);
 			}
 
-			const lumberjackProperties = {
-				...getLumberBaseProperties(claims.documentId, claims.tenantId),
-				[CommonProperties.clientId]: clientId,
-			};
-
-			const connectDocumentGetClientsMetric = Lumberjack.newLumberMetric(
-				LumberEventName.ConnectDocumentGetClients,
-				lumberjackProperties,
-			);
-			const clients = await clientManager
-				.getClients(claims.tenantId, claims.documentId)
-				.then((response) => {
-					connectDocumentGetClientsMetric.success(
-						"Successfully got clients from client manager",
-					);
-					return response;
-				})
-				.catch(async (err) => {
-					const errMsg = `Failed to get clients. Error: ${safeStringify(
-						err,
-						undefined,
-						2,
-					)}`;
-					connectDocumentGetClientsMetric.error(
-						"Failed to get clients during connectDocument",
-						err,
-					);
-					return handleServerError(logger, errMsg, claims.documentId, claims.tenantId);
-				});
-
-			if (clients.length > maxNumberOfClientsPerDocument) {
-				throw new NetworkError(
-					429,
-					"Too Many Clients Connected to Document",
-					true /* canRetry */,
-					false /* isFatal */,
-					5 * 60 * 1000 /* retryAfterMs (5 min) */,
-				);
-			}
-
 			const connectDocumentAddClientMetric = Lumberjack.newLumberMetric(
 				LumberEventName.ConnectDocumentAddClient,
 				lumberjackProperties,

diff --git a/server/routerlicious/packages/routerlicious-base/src/alfred/routes/api/deltas.ts b/server/routerlicious/packages/routerlicious-base/src/alfred/routes/api/deltas.ts
@@ -83,8 +83,8 @@ export function create(
 	router.get(
 		["/v1/:tenantId/:id", "/:tenantId/:id/v1"],
 		validateRequestParams("tenantId", "id"),
-		verifyStorageToken(tenantManager, config, tokenManager, defaultTokenValidationOptions),
 		throttle(generalTenantThrottler, winston, tenantThrottleOptions),
+		verifyStorageToken(tenantManager, config, tokenManager, defaultTokenValidationOptions),
 		(request, response, next) => {
 			const from = stringToSequenceNumber(request.query.from);
 			const to = stringToSequenceNumber(request.query.to);
@@ -109,8 +109,8 @@ export function create(
 	router.get(
 		"/raw/:tenantId/:id",
 		validateRequestParams("tenantId", "id"),
-		verifyStorageToken(tenantManager, config, tokenManager, defaultTokenValidationOptions),
 		throttle(generalTenantThrottler, winston, tenantThrottleOptions),
+		verifyStorageToken(tenantManager, config, tokenManager, defaultTokenValidationOptions),
 		(request, response, next) => {
 			const tenantId = getParam(request.params, "tenantId") || appTenants[0].id;
 

diff --git a/server/routerlicious/packages/routerlicious-base/src/alfred/routes/api/documents.ts b/server/routerlicious/packages/routerlicious-base/src/alfred/routes/api/documents.ts
@@ -104,8 +104,8 @@ export function create(
 	router.get(
 		"/:tenantId/:id",
 		validateRequestParams("tenantId", "id"),
-		verifyStorageToken(tenantManager, config, tokenManager, defaultTokenValidationOptions),
 		throttle(generalTenantThrottler, winston, tenantThrottleOptions),
+		verifyStorageToken(tenantManager, config, tokenManager, defaultTokenValidationOptions),
 		(request, response, next) => {
 			const documentP = storage.getDocument(
 				getParam(request.params, "tenantId") || appTenants[0].id,
@@ -131,13 +131,6 @@ export function create(
 	router.post(
 		"/:tenantId",
 		validateRequestParams("tenantId"),
-		verifyStorageToken(tenantManager, config, tokenManager, {
-			requireDocumentId: false,
-			ensureSingleUseToken: true,
-			singleUseTokenCache,
-			enableTokenCache: enableJwtTokenCache,
-			tokenCache: singleUseTokenCache,
-		}),
 		throttle(
 			clusterThrottlers.get(Constants.createDocThrottleIdPrefix),
 			winston,
@@ -148,6 +141,13 @@ export function create(
 			winston,
 			createDocTenantThrottleOptions,
 		),
+		verifyStorageToken(tenantManager, config, tokenManager, {
+			requireDocumentId: false,
+			ensureSingleUseToken: true,
+			singleUseTokenCache,
+			enableTokenCache: enableJwtTokenCache,
+			tokenCache: singleUseTokenCache,
+		}),
 		async (request, response, next) => {
 			// Tenant and document
 			const tenantId = getParam(request.params, "tenantId");
@@ -227,7 +227,6 @@ export function create(
 	 */
 	router.get(
 		"/:tenantId/session/:id",
-		verifyStorageToken(tenantManager, config, tokenManager, defaultTokenValidationOptions),
 		throttle(
 			clusterThrottlers.get(Constants.getSessionThrottleIdPrefix),
 			winston,
@@ -238,6 +237,7 @@ export function create(
 			winston,
 			getSessionTenantThrottleOptions,
 		),
+		verifyStorageToken(tenantManager, config, tokenManager, defaultTokenValidationOptions),
 		async (request, response, next) => {
 			const documentId = getParam(request.params, "id");
 			const tenantId = getParam(request.params, "tenantId");
@@ -279,9 +279,9 @@ export function create(
 	router.post(
 		"/:tenantId/document/:id/revokeToken",
 		validateRequestParams("tenantId", "id"),
+		throttle(generalTenantThrottler, winston, tenantThrottleOptions),
 		validateTokenScopeClaims(TokenRevokeScopeType),
 		verifyStorageToken(tenantManager, config, tokenManager, defaultTokenValidationOptions),
-		throttle(generalTenantThrottler, winston, tenantThrottleOptions),
 		async (request, response, next) => {
 			const documentId = getParam(request.params, "id");
 			const tenantId = getParam(request.params, "tenantId");