Skip to content
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.

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

[Mono.Android] Handle "No authentication challenges found" on HTTP 401 #449

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jamie94bc
Copy link

@jamie94bc jamie94bc commented Feb 17, 2017

Pre-KitKat (< API 19), when accessing Java.Net.HttpURLConnection.ResponseCode for a 401 response without a WWW-Authenticate header, Java.IO.IOException: No authentication challenges found is thrown.

This commit catches the exception, ensuring a 401 is returned by AndroidClientHandler.

Android libcore sources:

Pre-KitKat (< API 19), when accessing `Java.Net.HttpURLConnection.ResponseCode` for a 401 response without a `WWW-Authenticate` header, `Java.IO.IOException: No authentication challenges found` is thrown.

This commit catches the exception, ensuring a 401 is returned by `AndroidClientHandler`.
@monojenkins
Copy link
Collaborator

Hello! I'm the build bot for the Mono project.

I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done.

Contributors can ignore this message.

}
// Accessing the ResponseCode will throw an IOException if the code is 401
// and the server has not returned a WWW-Authenticate header on < API 19
catch (IOException ex) when (ex.Message.Contains("authentication challenges")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering how IOException compiles when both System.IO and Java.IO are in scope, and both of those namespaces have an IOException type...

I also find it somewhat amusing that the fix to httpConnection.ResponseCode throwing an exception...is to call it again.

insanity is doing the same thing over and over and expecting a different result.

Oh Android, I ❤️ you so much...

@jonpryor
Copy link
Member

Is it possible to write a test for this? Or, can you provide instructions on what a server would need to do to hit this code path when running on e.g. API-18? We might be able to get a server to behave in the (in?)appropriate way.

@jamie94bc
Copy link
Author

I'll get an updated commit and see if I can add a test for this.

I believe calling ResponseCode on a second call because httpEngine.hasResponse() would evaluate to true, so processResponseHeaders and therefore getAuthorizationCredentials wouldn't be called the second time around:

https://android.googlesource.com/platform/libcore/+/459a682c55c93c554ad4e822260569a204572dda/luni/src/main/java/libcore/net/http/HttpURLConnectionImpl.java#276

private HttpEngine getResponse() throws IOException {
        initHttpEngine();
        if (httpEngine.hasResponse()) {
            return httpEngine;
        }

Updated main PR description.

@jonpryor
Copy link
Member

build

@jonpryor
Copy link
Member

@jamie94bc: your unit test failed execution:

Failed

AndroidClientHandlerIntegrationTests.HttpClientIntegrationTestBase.ShouldHandle401WithoutWWWAuthenticateHeader

Failing for the past 1 build (Since Unstable#598 )
Took 4.1 sec.
Stacktrace

                                                MESSAGE:
                                                  Expected: Unauthorized
  But was:  OK

                                                +++++++++++++++++++
                                                STACK TRACE:
                                                  at Xamarin.Android.NetTests.HttpClientIntegrationTestBase.ShouldHandle401WithoutWWWAuthenticateHeader () [0x00057] in <dbb697cdf4384c2399ee6aadf056cd00>:0 
  at (wrapper managed-to-native) System.Reflection.MonoMethod:InternalInvoke (System.Reflection.MonoMethod,object,object[],System.Exception&)
  at System.Reflection.MonoMethod.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00038] in <4faaf451b1da438db18760f64e4eb536>:0 

I suspect the issue is that we use an API-21 emulator to run the unit tests on the PR builder, and API-21 doesn't need your fix. The assertion thus needs a runtime API level check.

@jamie94bc
Copy link
Author

@jonpryor that's odd, Expected: Unauthorized But was: OK shouldn't need to have an API level workaround - regardless of the underlying implementation a 401 status code should be a 401 here.

I will try to find some time later this week to look into this 👍

@xamarin xamarin deleted a comment from dnfclas Dec 12, 2017
@xamarin-jenkins
Copy link

Hello! I'm the build bot for Xamarin. I need approval from a Xamarin team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done. Contributors can ignore this message.

Base automatically changed from master to main March 5, 2021 23:08
@jonpryor jonpryor requested a review from grendello as a code owner March 5, 2021 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants