Bug 177105 - [Curl] Move Authentication related tasks into AuthenticationChallenge class
Summary: [Curl] Move Authentication related tasks into AuthenticationChallenge class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Basuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks: 175149
  Show dependency treegraph
 
Reported: 2017-09-18 15:18 PDT by Basuke Suzuki
Modified: 2017-09-27 12:25 PDT (History)
6 users (show)

See Also:


Attachments
PATCH (13.35 KB, patch)
2017-09-18 16:09 PDT, Basuke Suzuki
achristensen: review-
achristensen: commit-queue-
Details | Formatted Diff | Diff
fix (13.37 KB, patch)
2017-09-19 13:01 PDT, Basuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Basuke Suzuki 2017-09-18 15:18:33 PDT
Currently those codes are in ResourceHandle or related companion. It will be reused with NetworkLoadTask so that it should be separated from them.
Comment 1 Basuke Suzuki 2017-09-18 16:09:50 PDT
Created attachment 321147 [details]
PATCH
Comment 2 Alex Christensen 2017-09-18 21:07:43 PDT
Comment on attachment 321147 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=321147&action=review

> Source/WebCore/platform/network/curl/AuthenticationChallenge.h:53
>      RefPtr<AuthenticationClient> m_authenticationClient;

FYI, we're kind of moving away from each challenging having a client in favor of a challenge being received with a completion handler so the challenge itself doesn't know what is to be done with it.  So in a hopefully-not-too-distant future once everyone has adopted the NetworkSession abstraction which is similar to the async ResourceHandleClient callbacks and we've removed ResourceHandle in favor of everything being asynchronous, we will organize it more like that.  This is fine for now, though.

> Source/WebCore/platform/network/curl/AuthenticationChallengeCurl.cpp:69
> +    int realmPos = authHeader.find(realmString);
> +    if (realmPos > 0) {

find returns a size_t, and we should check it against notFound.

> Source/WebCore/platform/network/curl/ResourceHandleCurlDelegate.h:77
> +    void didReceiveAllHeaders(long httpCode, long long contentLength, long connectPort, long availableHttpAuth);

ports should be uint16_t or std::optional<uint16_t> if the underlying logic knows to use the default port for the protocol if there is none.
Comment 3 Basuke Suzuki 2017-09-19 10:51:55 PDT
> FYI, we're kind of moving away from each challenging having a client in
> favor of a challenge being received with a completion handler so the
> challenge itself doesn't know what is to be done with it.  So in a
> hopefully-not-too-distant future once everyone has adopted the
> NetworkSession abstraction which is similar to the async
> ResourceHandleClient callbacks and we've removed ResourceHandle in favor of
> everything being asynchronous, we will organize it more like that.  This is
> fine for now, though.

Fine for us right now. We understand the direction and that further changes are waiting.


> > Source/WebCore/platform/network/curl/AuthenticationChallengeCurl.cpp:69
> > +    int realmPos = authHeader.find(realmString);
> > +    if (realmPos > 0) {
> 
> find returns a size_t, and we should check it against notFound.

my bad.


> > Source/WebCore/platform/network/curl/ResourceHandleCurlDelegate.h:77
> > +    void didReceiveAllHeaders(long httpCode, long long contentLength, long connectPort, long availableHttpAuth);
> 
> ports should be uint16_t or std::optional<uint16_t> if the underlying logic
> knows to use the default port for the protocol if there is none.

Okay for our code base, but eventually we will create a ProtectionSpace object, which is the world where `port` is `int`. We don't want to change them with this patch.
Comment 4 Basuke Suzuki 2017-09-19 13:01:18 PDT
Created attachment 321233 [details]
fix
Comment 5 Basuke Suzuki 2017-09-19 13:32:06 PDT
I cannot handle this test failure. https://webkit-queues.webkit.org/patch/321233/commit-queue
Comment 6 WebKit Commit Bot 2017-09-19 13:45:12 PDT
Comment on attachment 321233 [details]
fix

Clearing flags on attachment: 321233

Committed r222223: <http://trac.webkit.org/changeset/222223>
Comment 7 WebKit Commit Bot 2017-09-19 13:45:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2017-09-27 12:25:12 PDT
<rdar://problem/34693240>