NullReferenceException in AuthenticationContinuationHelper on Xamarin.Android

[leonluc-dev]

I'm having an issue with the ADAL library on Xamarin.Android.

I have implemented the following piece of code:

 protected override void OnActivityResult(int requestCode, Result resultCode, Intent data)
 {
     base.OnActivityResult(requestCode, resultCode, data);
     AuthenticationAgentContinuationHelper.SetAuthenticationAgentContinuationEventArgs(requestCode, resultCode, data);
     //Rest of code
 }

However this causes an issue whenever this method gets called when returning from a non-ADAL activity.

I/MonoDroid(21369): UNHANDLED EXCEPTION:
02-28 11:24:33.960 I/MonoDroid(21369): System.ArgumentNullException: Value cannot be null.
02-28 11:24:33.960 I/MonoDroid(21369): Parameter name: uriString
02-28 11:24:33.960 I/MonoDroid(21369):   at System.Uri..ctor (System.String uriString) [0x00009] in <10a067eff09446018662189377d200e9>:0 
02-28 11:24:33.960 I/MonoDroid(21369):   at Microsoft.IdentityModel.Clients.ActiveDirectory.Internal.AuthorizationResult.ParseAuthorizeResponse (System.String webAuthenticationResult) [0x00000] in <67d1bd4690974582a2c36961ad27d20f>:0 
02-28 11:24:33.960 I/MonoDroid(21369):   at Microsoft.IdentityModel.Clients.ActiveDirectory.Internal.AuthorizationResult..ctor (Microsoft.IdentityModel.Clients.ActiveDirectory.Internal.AuthorizationStatus status, System.String returnedUriInput) [0x00047] in <67d1bd4690974582a2c36961ad27d20f>:0 
02-28 11:24:33.961 I/MonoDroid(21369):   at Microsoft.IdentityModel.Clients.ActiveDirectory.AuthenticationAgentContinuationHelper.SetAuthenticationAgentContinuationEventArgs (System.Int32 requestCode, Android.App.Result resultCode, Android.Content.Intent data) [0x00056] in <67d1bd4690974582a2c36961ad27d20f>:0 
02-28 11:24:33.961 I/MonoDroid(21369):   at TestApp.Android.MainActivity.OnActivityResult (System.Int32 requestCode, Android.App.Result resultCode, Android.Content.Intent data) [0x0000b] in C:\ProgrMain\TestApp.Android\MainActivity.cs:210 
02-28 11:24:33.961 I/MonoDroid(21369):   at Android.App.Activity.n_OnActivityResult_IILandroid_content_Intent_ (System.IntPtr jnienv, System.IntPtr native__this, System.Int32 requestCode, System.Int32 native_resultCode, System.IntPtr native_data) [0x00012] in <8211f1f12f9e45838d631abf63e51c79>:0 
02-28 11:24:33.961 I/MonoDroid(21369):   at (wrapper dynamic-method) System.Object:20439fec-2f76-48fa-8d94-a63c637c3208 (intptr,intptr,int,int,intptr)
02-28 11:24:33.976 W/art     (21369): JNI RegisterNativeMethods: attempt to register 0 native methods for android.runtime.JavaProxyThrowable

Judging by the source code it seems there is no check if the activity result came from an ADAL related activity or broker, and it will therefore try to parse a nonexistant uri.
Is there any way to check if a activity result came from an ADAL activity/broker, similar to the IsBrokerResponse method in the iOS version of this class?

I already tried checking if ReturnedUrl was present in the extras bundle before calling the SetAuthenticationAgentContinuationEventArgs method, but it's only present if authentication went successfully, therefore making it unhelpful in case errors need to be handled by ADAL.

[jennyf19]

@gameleon-dev
Thanks for bringing this to our attention and for providing details about the issue. We will look into it.

Also, based on the sample code you provided, it looks like you are waiting for multiple activities to be returned. If so, you will need to designate a requestCode for ADAL so that your code can check which request it's responding to. Check out this [Android documentation] (https://developer.android.com/training/basics/intents/result.html#ReceiveResult)

[leonluc-dev]

@jennyf19 Normally I handle activity results this way. But the activities launched by ADAL are not provided with a request code. They are defaulted to 0 or "no request code", with the exception of the broker activity which is hardcoded to use 1001.

See the following pieces of code in the ADAL source code:

Webview activity call with 0 request code.
azure-activedirectory-library-for-dotnet/adal/src/Microsoft.IdentityModel.Clients.ActiveDirectory/Platforms/android/WebUI.cs, Line 70:
this.parameters.CallerActivity.StartActivityForResult(agentIntent, 0);

Broker activity call with 1001 request code.
/adal/src/Microsoft.IdentityModel.Clients.ActiveDirectory/Platforms/android/BrokerHelper.cs, Line 206:
platformParams.CallerActivity.StartActivityForResult(brokerIntent, 1001);

If there currently is no way to easily check the origin of a OnActivityResult call, it seems the optimal solution would be to update the PlatformParameters class to include (optional) parameters for requestCodes for the activities the ADAL library will start.

[leonluc-dev]

I have created a pull request that implements request codes as optional properties provided to the PlatformParameters when requesting tokens.
This allows the OnActivityResult implementation to check the origin of the result by checking said codes.

[jennyf19]

Thanks for the additional information and PR, @gameleon-dev - much appreciated. We'll take a look at your PR and do validation on our end.

cc: @henrik-me @SomkaPe

[henrik-me]

@gameleon-dev for the change you proposed. Isn't there a way for you to do the same just outside of the library code?The request code is 1001 for broker but 0 for the webui code. We could potentially change the request code used in the WebUI code to something other than 0 (however I do not know who might have taken a dependency on this). My suggestion is if you can ensure your own app doesn't use either 0 nor 1001 for requestcode then you could validate that?

[leonluc-dev]

@henrik-me Checking for 0 and 1001 does work and is what I have currently implemented in code. So this issue can technically be considered closed.

That said, the changes in the PR were proposed because my main concern with the current solution is that it doesn't seem optimal for long-term. Considering 0 is the default value of requestCode and the solution gives the developer limited control of their application flow.

Changing the request code from 0 to another greater-than 0 value would take away the issue of overlapping with the default value, but as you stated might break existing dependencies on the value being equal to 0. (Hence why the updated constructor in PR #1001 defaults these values to the current values of 0 and 1001 as not to break existing dependencies).

[henrik-me]

That makes sense. @jmprieur would like to hear your thoughts on this one

[jmprieur]

Won't fix in ADAL.NET