Skip to content

bugfix #1019 - wrong throttling on clientid. #1050

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.

Sign up for GitHub

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

Open
gterminator wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

bugfix #1019 - wrong throttling on clientid. #1050

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 10 additions & 9 deletions msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/HttpHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,18 +125,19 @@ IHttpResponse executeHttpRequest(HttpRequest httpRequest) {

private String getRequestThumbprint(RequestContext requestContext) {
StringBuilder sb = new StringBuilder();
sb.append(requestContext.clientId() + POINT_DELIMITER);
sb.append(requestContext.authority() + POINT_DELIMITER);

IAcquireTokenParameters apiParameters = requestContext.apiParameters();

if (apiParameters instanceof SilentParameters) {
IAccount account = ((SilentParameters) apiParameters).account();
if (account != null) {
sb.append(account.homeAccountId() + POINT_DELIMITER);
sb.append(requestContext.clientId()).append(POINT_DELIMITER);
sb.append(requestContext.authority()).append(POINT_DELIMITER);

UserIdentifier userIdentifier = requestContext.userIdentifier();
if (userIdentifier != null) {
if (!StringHelper.isBlank(userIdentifier.upn())) {
sb.append(userIdentifier.upn()).append(POINT_DELIMITER);
} else if (!StringHelper.isBlank(userIdentifier.oid())) {
sb.append(userIdentifier.oid()).append(POINT_DELIMITER);
}
}

IAcquireTokenParameters apiParameters = requestContext.apiParameters();
Set<String> sortedScopes = new TreeSet<>(apiParameters.scopes());
sb.append(String.join(" ", sortedScopes));

Expand Down
97 changes: 97 additions & 0 deletions msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/RequestThrottlingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInstance;
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.fail;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.doReturn;
Expand All @@ -27,6 +29,7 @@ class RequestThrottlingTest {
public final Integer THROTTLE_IN_SEC = 1;
public TokenEndpointResponseType responseType;
IHttpClient httpClientMock = mock(IHttpClient.class);
private boolean skipInvocationCountCheck = false;


@BeforeEach
Expand All @@ -36,6 +39,9 @@ void init() {

@AfterEach
void check() throws Exception {
if (skipInvocationCountCheck) {
return;
}

//throttlingTest() makes three non-throttled calls, so for a test without a retry there should be
// 3 invocations of httpClientMock.send(), and 6 invocations if the calls are set to retry
Expand Down Expand Up @@ -199,4 +205,95 @@ void STSResponseContains_StatusCode500_RetryAfterHeader() throws Exception {
ThrottlingCache.DEFAULT_THROTTLING_TIME_SEC = 1000;
throttlingTest(TokenEndpointResponseType.STATUS_CODE_500_RETRY_AFTER_HEADER);
}

private UserNamePasswordParameters getUserNamePasswordApiParameters(String username, String scope) {
return UserNamePasswordParameters
.builder(Collections.singleton(scope), username, "password".toCharArray())
.build();
}

// Regression test for issue #1019: a failed request for one user must not throttle a different
// user under the same clientId/authority/scope.
@Test
void STSResponseContains_StatusCode500_DifferentUsersNotThrottledForEachOther() throws Exception {
skipInvocationCountCheck = true;
ThrottlingCache.clear();

// user A's request fails with a 500 -> gets cached as a throttled request
PublicClientApplication app =
getClientApplicationMockedWithOneTokenEndpointResponse(TokenEndpointResponseType.STATUS_CODE_500);
try {
app.acquireToken(getUserNamePasswordApiParameters("userA@contoso.com", "scope1")).join();
} catch (Exception ex) {
if (!(ex.getCause() instanceof MsalServiceException)) {
fail("Unexpected exception");
}
}

// repeating user A's request should be throttled
try {
app = getPublicClientApp();
app.acquireToken(getUserNamePasswordApiParameters("userA@contoso.com", "scope1")).join();
fail("Expected MsalThrottlingException");
} catch (Exception ex) {
if (!(ex.getCause() instanceof MsalThrottlingException)) {
fail("Unexpected exception");
}
}

// user B, same clientId/authority/scope, must NOT be throttled by user A's failure
app = getClientApplicationMockedWithOneTokenEndpointResponse(TokenEndpointResponseType.STATUS_CODE_500);
try {
app.acquireToken(getUserNamePasswordApiParameters("userB@contoso.com", "scope1")).join();
} catch (Exception ex) {
if (!(ex.getCause() instanceof MsalServiceException)) {
fail("User B should not be throttled by user A's failed request");
}
}
}

// Confirms that per-account throttling isolation for the silent flow (previously driven by the
// `instanceof SilentParameters` special case) is preserved now that the fingerprint is derived
// from RequestContext.userIdentifier() instead.
@Test
void SilentFlow_DifferentAccountsThrottledIndependently() throws Exception {
skipInvocationCountCheck = true;
ThrottlingCache.clear();

PublicClientApplication app = getPublicClientApp();

IHttpClient localHttpClientMock = mock(IHttpClient.class);
HttpResponse http500 = new HttpResponse();
http500.statusCode(HttpStatus.HTTP_INTERNAL_ERROR);
http500.body(TestConfiguration.TOKEN_ENDPOINT_INVALID_GRANT_ERROR_RESPONSE);
http500.addHeaders(Collections.singletonMap("Content-Type", Collections.singletonList("application/json")));
doReturn(http500).when(localHttpClientMock).send(any());

HttpHelper httpHelper = new HttpHelper(localHttpClientMock, new DefaultRetryPolicy());
ServiceBundle serviceBundle = new ServiceBundle(null, new TelemetryManager(null, false), httpHelper);

HttpRequest httpRequest = new HttpRequest(HttpMethod.POST,
"https://login.microsoftonline.com/common/oauth2/v2.0/token");

IAccount accountA = mock(IAccount.class);
doReturn("oidA.tidA").when(accountA).homeAccountId();
SilentParameters paramsA = SilentParameters.builder(Collections.singleton("scope1"), accountA).build();
RequestContext contextA = new RequestContext(app, PublicApi.ACQUIRE_TOKEN_SILENTLY, paramsA,
UserIdentifier.fromHomeAccountId(accountA.homeAccountId()));

// account A's request fails with a 500 -> gets cached as a throttled request
httpHelper.executeHttpRequest(httpRequest, contextA, serviceBundle);
// repeating account A's request should be throttled
assertThrows(MsalThrottlingException.class,
() -> httpHelper.executeHttpRequest(httpRequest, contextA, serviceBundle));

// account B, same clientId/authority/scope, must NOT be throttled by account A's failure
IAccount accountB = mock(IAccount.class);
doReturn("oidB.tidB").when(accountB).homeAccountId();
SilentParameters paramsB = SilentParameters.builder(Collections.singleton("scope1"), accountB).build();
RequestContext contextB = new RequestContext(app, PublicApi.ACQUIRE_TOKEN_SILENTLY, paramsB,
UserIdentifier.fromHomeAccountId(accountB.homeAccountId()));

assertDoesNotThrow(() -> httpHelper.executeHttpRequest(httpRequest, contextB, serviceBundle));
}
}