Skip to content

Commit

Permalink
Revert changes made to method signature for futures (#2309)
Browse files Browse the repository at this point in the history
Revert changes made to method signature to return back a ResultFuture
instead of a Future. It seems to have introduced a breaking change in
MSAL, instead of debugging this during release time, I rolled back part
of the solution for the original google crash. In this PR i'm rolling
back the whole fix for better tracking, will have a following PR to
reintroduce the smaller fix into common to address the issue without
adjusting method signatures that could affect msal or broker.

Original PR
#2305
  • Loading branch information
fadidurah authored Feb 9, 2024
1 parent e331b8a commit 1427a89
Show file tree
Hide file tree
Showing 13 changed files with 30 additions and 51 deletions.
1 change: 0 additions & 1 deletion changelog.txt
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
V.Next
---------
- [MINOR] Add flight to control silent token timeout (#2311)
- [PATCH] Handle Receiver Callback Exception in CommandDispatcher interactive command (#2305)
- [MINOR] Add IpcStrategyWithBackup (#2301)
- [PATCH] Fix MSAL Issue 1864 (#2280)
- [MINOR] Add AccountManagerBackupIpcStrategyTargetingSpecificBrokerApp (#2294)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@
import com.microsoft.identity.common.java.result.GenerateShrResult;
import com.microsoft.identity.common.java.result.LocalAuthenticationResult;
import com.microsoft.identity.common.java.telemetry.TelemetryEventStrings;
import com.microsoft.identity.common.java.util.ResultFuture;
import com.microsoft.identity.common.java.util.ResultUtil;
import com.microsoft.identity.common.java.util.ThreadUtils;
import com.microsoft.identity.common.java.util.ported.PropertyBag;
Expand All @@ -82,6 +81,7 @@
import java.util.List;
import java.util.Set;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Future;

import lombok.EqualsAndHashCode;

Expand All @@ -93,9 +93,6 @@ public class LocalMSALController extends BaseController {
@SuppressWarnings(WarningType.rawtype_warning)
private IAuthorizationStrategy mAuthorizationStrategy = null;

@SuppressWarnings({WarningType.rawtype_warning, WarningType.unchecked_warning})
private ResultFuture<AuthorizationResult> mAuthorizationResultFuture = null;

@SuppressWarnings(WarningType.rawtype_warning)
private AuthorizationRequest mAuthorizationRequest = null;

Expand Down Expand Up @@ -231,14 +228,12 @@ private AuthorizationResult performAuthorizationRequest(@NonNull final OAuth2Str
mAuthorizationRequest = getAuthorizationRequest(strategy, parameters);

// Suppressing unchecked warnings due to casting of AuthorizationRequest to GenericAuthorizationRequest and AuthorizationStrategy to GenericAuthorizationStrategy in the arguments of call to requestAuthorization method
mAuthorizationResultFuture = strategy.requestAuthorization(
@SuppressWarnings(WarningType.unchecked_warning) final Future<AuthorizationResult> future = strategy.requestAuthorization(
mAuthorizationRequest,
mAuthorizationStrategy
);

AuthorizationResult result = mAuthorizationResultFuture.get();
mAuthorizationResultFuture = null;
return result;
return future.get();
}

@Override
Expand All @@ -258,17 +253,7 @@ public void onFinishAuthorizationSession(int requestCode,
.put(TelemetryEventStrings.Key.REQUEST_CODE, String.valueOf(requestCode))
);

try {
mAuthorizationStrategy.completeAuthorization(requestCode, RawAuthorizationResult.fromPropertyBag(data));
} catch (Exception e){
// If the future is somehow initialized and waiting, give the future an exception
if (mAuthorizationResultFuture != null && !mAuthorizationResultFuture.isDone()) {
mAuthorizationResultFuture.setException(e);
} else {
// Best Effort
throw e;
}
}
mAuthorizationStrategy.completeAuthorization(requestCode, RawAuthorizationResult.fromPropertyBag(data));

Telemetry.emit(
new ApiEndEvent()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
import com.microsoft.identity.common.java.providers.oauth2.TokenRequest;
import com.microsoft.identity.common.java.providers.oauth2.TokenResponse;

import com.microsoft.identity.common.java.util.ResultFuture;
import java.util.concurrent.Future;

/**
* Azure Active Directory Federation Services 2012 R2 OAuth Strategy
Expand All @@ -70,7 +70,7 @@ public ActiveDirectoryFederationServices2012R2OAuth2Strategy(OAuth2Configuration
// Suppressing unchecked warnings due to casting of AuthorizationRequest to GenericAuthorizationRequest and AuthorizationStrategy to GenericAuthorizationStrategy in the arguments of call to super class' method requestAuthorization
@SuppressWarnings(WarningType.unchecked_warning)
@Override
public ResultFuture<AuthorizationResult> requestAuthorization(AuthorizationRequest request, IAuthorizationStrategy IAuthorizationStrategy)
public Future<AuthorizationResult> requestAuthorization(AuthorizationRequest request, IAuthorizationStrategy IAuthorizationStrategy)
throws ClientException {
return super.requestAuthorization(request, IAuthorizationStrategy);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
import com.microsoft.identity.common.java.providers.oauth2.TokenRequest;
import com.microsoft.identity.common.java.providers.oauth2.TokenResponse;

import com.microsoft.identity.common.java.util.ResultFuture;
import java.util.concurrent.Future;

/**
* Azure Active Directory Federation Services 2016 oAuth2 Strategy.
Expand All @@ -68,7 +68,7 @@ public ActiveDirectoryFederationServices2016OAuth2Strategy(OAuth2Configuration c
// Suppressing unchecked warnings due to casting of AuthorizationRequest to GenericAuthorizationRequest and AuthorizationStrategy to GenericAuthorizationStrategy in the arguments of call to super class' method requestAuthorization
@SuppressWarnings(WarningType.unchecked_warning)
@Override
public ResultFuture<AuthorizationResult> requestAuthorization(AuthorizationRequest request, IAuthorizationStrategy IAuthorizationStrategy) throws ClientException {
public Future<AuthorizationResult> requestAuthorization(AuthorizationRequest request, IAuthorizationStrategy IAuthorizationStrategy) throws ClientException {
return super.requestAuthorization(request, IAuthorizationStrategy);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
import com.microsoft.identity.common.java.providers.oauth2.TokenResponse;
import com.microsoft.identity.common.java.providers.oauth2.TokenResult;

import com.microsoft.identity.common.java.util.ResultFuture;
import java.util.concurrent.Future;

/**
* Azure Active Directory B2C OAuth Strategy.
Expand All @@ -68,7 +68,7 @@ public AzureActiveDirectoryB2COAuth2Strategy(OAuth2Configuration config, OAuth2S
// Suppressing unchecked warnings due to casting of AuthorizationRequest to GenericAuthorizationRequest and AuthorizationStrategy to GenericAuthorizationStrategy in the arguments of call to super class' method requestAuthorization
@SuppressWarnings(WarningType.unchecked_warning)
@Override
public ResultFuture<AuthorizationResult> requestAuthorization(AuthorizationRequest request, IAuthorizationStrategy authorizationStrategy) throws ClientException {
public Future<AuthorizationResult> requestAuthorization(AuthorizationRequest request, IAuthorizationStrategy authorizationStrategy) throws ClientException {
return super.requestAuthorization(request, authorizationStrategy);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import com.microsoft.identity.common.logging.Logger;

import java.net.URI;
import java.util.concurrent.Future;

import static com.microsoft.identity.common.java.AuthenticationConstants.UIRequest.BROWSER_FLOW;

Expand Down Expand Up @@ -74,7 +75,7 @@ public void setBrowser(final Browser browser) {
}

@Override
public ResultFuture<AuthorizationResult> requestAuthorization(
public Future<AuthorizationResult> requestAuthorization(
GenericAuthorizationRequest authorizationRequest,
GenericOAuth2Strategy oAuth2Strategy)
throws ClientException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import com.microsoft.identity.common.logging.Logger;

import java.net.URI;
import java.util.concurrent.Future;

/**
* Serve as a class to do the OAuth2 auth code grant flow with Android embedded web view.
Expand Down Expand Up @@ -75,8 +76,8 @@ public EmbeddedWebViewAuthorizationStrategy(@NonNull Context applicationContext,
* The activity result is set in Authorization.setResult() and passed to the onActivityResult() of the calling activity.
*/
@Override
public ResultFuture<AuthorizationResult> requestAuthorization(GenericAuthorizationRequest authorizationRequest,
GenericOAuth2Strategy oAuth2Strategy) throws ClientException {
public Future<AuthorizationResult> requestAuthorization(GenericAuthorizationRequest authorizationRequest,
GenericOAuth2Strategy oAuth2Strategy) throws ClientException {
final String methodTag = TAG + ":requestAuthorization";
mAuthorizationResultFuture = new ResultFuture<>();
mOAuth2Strategy = oAuth2Strategy;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -753,16 +753,10 @@ public void run() {

EstsTelemetry.getInstance().emitApiId(command.getPublicApiId());

final BaseException[] receiverException = new BaseException[1];

final LocalBroadcaster.IReceiverCallback resultReceiver = new LocalBroadcaster.IReceiverCallback() {
@Override
public void onReceive(@NonNull PropertyBag dataBag) {
try {
completeInteractive(dataBag);
} catch (final Exception e) {
receiverException[0] = ExceptionAdapter.baseExceptionFromException(e);
}
completeInteractive(dataBag);
}
};

Expand All @@ -779,12 +773,6 @@ public void onReceive(@NonNull PropertyBag dataBag) {

LocalBroadcaster.INSTANCE.unregisterCallback(RETURN_AUTHORIZATION_REQUEST_RESULT);

// If we received an exception during the receiver callback execution,
// we should set that as the command result
if (receiverException[0] != null) {
commandResult = CommandResult.of(CommandResult.ResultStatus.ERROR, receiverException[0], correlationId);
}

Logger.info(TAG + methodName,
"Completed interactive request for correlation id : **" + correlationId +
statusMsg(commandResult.getStatus().getLogStatus()));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
import com.microsoft.identity.common.java.exception.ClientException;
import com.microsoft.identity.common.java.providers.RawAuthorizationResult;

import com.microsoft.identity.common.java.util.ResultFuture;
import java.util.concurrent.Future;

import edu.umd.cs.findbugs.annotations.NonNull;

Expand All @@ -43,8 +43,8 @@ public interface IAuthorizationStrategy<
/**
* Perform the authorization request.
*/
ResultFuture<AuthorizationResult> requestAuthorization(GenericAuthorizationRequest authorizationRequest,
GenericOAuth2Strategy oAuth2Strategy)
Future<AuthorizationResult> requestAuthorization(GenericAuthorizationRequest authorizationRequest,
GenericOAuth2Strategy oAuth2Strategy)
throws ClientException;

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@
import com.microsoft.identity.common.java.util.CommonURIBuilder;
import com.microsoft.identity.common.java.util.IClockSkewManager;
import com.microsoft.identity.common.java.util.ObjectMapper;
import com.microsoft.identity.common.java.util.ResultFuture;
import com.microsoft.identity.common.java.util.StringUtil;

import java.io.IOException;
Expand All @@ -67,6 +66,7 @@
import java.util.Locale;
import java.util.Map;
import java.util.TreeMap;
import java.util.concurrent.Future;

import javax.net.ssl.HttpsURLConnection;

Expand Down Expand Up @@ -138,14 +138,14 @@ public OAuth2Strategy(GenericOAuth2Configuration config,
* @param authorizationStrategy generic authorization strategy.
* @return GenericAuthorizationResponse
*/
public ResultFuture<AuthorizationResult> requestAuthorization(
public Future<AuthorizationResult> requestAuthorization(
final GenericAuthorizationRequest request,
final GenericAuthorizationStrategy authorizationStrategy)
throws ClientException {
validateAuthorizationRequest(request);

// Suppressing unchecked warnings due to casting an object in reference of current class to the child class GenericOAuth2Strategy while calling method requestAuthorization()
@SuppressWarnings(WarningType.unchecked_warning) final ResultFuture<AuthorizationResult> authorizationFuture =
@SuppressWarnings(WarningType.unchecked_warning) final Future<AuthorizationResult> authorizationFuture =
authorizationStrategy.requestAuthorization(request, this);
Telemetry.emit(new UiShownEvent().putVisible(TelemetryEventStrings.Value.TRUE));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@
import com.microsoft.identity.common.java.util.ResultFuture;
import com.microsoft.identity.internal.testutils.mocks.MockSuccessAuthorizationResultMockedTests;

import java.util.concurrent.Future;

public class MockStrategyWithMockedHttpResponse extends ResourceOwnerPasswordCredentialsTestStrategy {

/**
Expand All @@ -49,7 +51,7 @@ public MockStrategyWithMockedHttpResponse(MicrosoftStsOAuth2Configuration config
* @return GenericAuthorizationResponse
*/
@Override
public ResultFuture<AuthorizationResult> requestAuthorization(
public Future<AuthorizationResult> requestAuthorization(
final MicrosoftStsAuthorizationRequest request,
final IAuthorizationStrategy IAuthorizationStrategy) {
final MockSuccessAuthorizationResultMockedTests authorizationResult = new MockSuccessAuthorizationResultMockedTests();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.concurrent.Future;

public class MockTestStrategy extends ResourceOwnerPasswordCredentialsTestStrategy {

Expand All @@ -62,7 +63,7 @@ public MockTestStrategy(MicrosoftStsOAuth2Configuration config) throws ClientExc
* @return GenericAuthorizationResponse
*/
@Override
public ResultFuture<AuthorizationResult> requestAuthorization(
public Future<AuthorizationResult> requestAuthorization(
final MicrosoftStsAuthorizationRequest request,
final IAuthorizationStrategy IAuthorizationStrategy) {
final MockSuccessAuthorizationResultMockedTests authorizationResult = new MockSuccessAuthorizationResultMockedTests();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@
import com.microsoft.identity.internal.testutils.labutils.LabConfig;
import com.microsoft.identity.internal.testutils.mocks.MockSuccessAuthorizationResultNetworkTests;

import java.util.concurrent.Future;

public class ResourceOwnerPasswordCredentialsTestStrategy extends MicrosoftStsOAuth2Strategy {

public static final String USERNAME_EMPTY_OR_NULL = "username_empty_or_null";
Expand Down Expand Up @@ -74,7 +76,7 @@ public ResourceOwnerPasswordCredentialsTestStrategy(final MicrosoftStsOAuth2Conf
* @return GenericAuthorizationResponse
*/
@Override
public ResultFuture<AuthorizationResult> requestAuthorization(
public Future<AuthorizationResult> requestAuthorization(
final MicrosoftStsAuthorizationRequest request,
final IAuthorizationStrategy IAuthorizationStrategy) {
final MockSuccessAuthorizationResultNetworkTests authorizationResult = new MockSuccessAuthorizationResultNetworkTests();
Expand Down

0 comments on commit 1427a89

Please sign in to comment.