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

Fix uri field in digest auth header to include query params #1992

Merged
merged 2 commits into from
Jul 16, 2020

Conversation

RyanGrif
Copy link
Contributor

@RyanGrif RyanGrif commented Jul 7, 2020

Subsystem
Client

Motivation
The problem is described in issue #1990 . When using digest auth, the uri field does not include the request parameters included in the original request. This breaks digest authentication.

Solution
Changing all places in DigestAuthProvider.addRequestHeaders which currently use url.encodedPath to use url.fullPath so that the uri field and token include the request parameters.

@e5l e5l self-requested a review July 8, 2020 15:42
@e5l
Copy link
Member

e5l commented Jul 8, 2020

Hey @RyanGrif, Thanks for the request.
LGTM.
Could you add the test to verify the behavior?

@RyanGrif RyanGrif force-pushed the fix-uri-field-in-DigestAuthProvider branch 2 times, most recently from 6911249 to 58cd00a Compare July 14, 2020 19:06
@RyanGrif RyanGrif force-pushed the fix-uri-field-in-DigestAuthProvider branch from 58cd00a to 604a129 Compare July 14, 2020 19:11
@RyanGrif
Copy link
Contributor Author

Hey @RyanGrif, Thanks for the request.
LGTM.
Could you add the test to verify the behavior?

Hey @e5l, I've added tests for the DispatchAuthProvider as requested. They pass for jvm, js browser and node, but fail for posix - I get the error [generateNonce] is not supported on iOS which suggests that Digest auth just isn't designed to work with iOS. If that's right, is there a way I can skip these tests for posix? Apologies if this is confusing - I'm quite new to Kotlin and not at all familiar with this multiplatform build stuff!

@e5l e5l merged commit 4962c42 into ktorio:master Jul 16, 2020
@e5l
Copy link
Member

e5l commented Jul 16, 2020

Merged

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.

2 participants