Bug 135327 – Server trust authentication challenges aren’t sent to the navigation delegate

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

Server trust authentication challenges are short-circuited and not passed through to -webView:didReceiveAuthenticationChallenge:completionHandler:.

This was done because the delegate would need to respond with an NSURLCredential that contains a trust, and WebCore::Credential can’t represent such credentials. This can be resolved by making Credential have an NSURLCredential (and removing the special case for identity credentials), similarly to how ProtectionSpace now has an NSURLProtectionSpace.

CredentialStorage may also require some changes to support server trust credentials (which have session persistence).

Created attachment 235587 [details]
I. Introduce CredentialBase and make Credential derive from it

[WebKit Commit Bot]

Attachment 235587 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/CredentialBase.h:89:  The parameter name "a" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/network/CredentialBase.cpp:43:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/network/CredentialBase.cpp:157:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 235587 [details]
I. Introduce CredentialBase and make Credential derive from it

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

r=me -- the comments I made are sort of “side notes”; not all necessarily relevant to this file refactoring

> Source/WebCore/platform/network/CredentialBase.cpp:26
>  #include "config.h"
> +#include "CredentialBase.h"

Should add a blank line before the first include.

> Source/WebCore/platform/network/CredentialBase.cpp:36
>      : m_user("")
>      , m_password("")

Should use the more efficient emptyString() some day if not now.

> Source/WebCore/platform/network/CredentialBase.cpp:48
>      : m_user(user.length() ? user : "")
>      , m_password(password.length() ? password : "")

Should use the more efficient emptyString() some day if not now.

> Source/WebCore/platform/network/CredentialBase.h:25
> + */
> +#ifndef Credential_h

Should add a blank line before the ifndef.

> Source/WebCore/platform/network/CredentialBase.h:30
> +#define CERTIFICATE_CREDENTIALS_SUPPORTED (PLATFORM(COCOA))

The name of this conditional is pretty strange. The feature in question involves interface that is done with platform-specific types, but yet this has a name that sounds like an abstract concept that would not be CoreFoundation-specific.

> Source/WebCore/platform/network/CredentialBase.h:25
>   */
> -#ifndef Credential_h
> -#define Credential_h
> +#ifndef CredentialBase_h

Should add a blank line before the #ifndef.

Committed attachment 235587 [details] as <http://trac.webkit.org/r171722>.

Created attachment 235664 [details]
II. Move the Cocoa-specific parts of CredentialBase into a Cocoa-specific Credential class

[WebKit Commit Bot]

Attachment 235664 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/CredentialBase.h:51:  The parameter name "a" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/network/cocoa/CredentialCocoa.h:63:  The parameter name "identity" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/network/cocoa/CredentialCocoa.h:63:  The parameter name "persistence" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 235665 [details]
II. Move the Cocoa-specific parts of CredentialBase into a Cocoa-specific Credential class

Created attachment 235670 [details]
II. Move the Cocoa-specific parts of CredentialBase into a Cocoa-specific Credential class

Created attachment 235672 [details]
II. Move the Cocoa-specific parts of CredentialBase into a Cocoa-specific Credential class

Committed attachment 235672 [details] as <http://trac.webkit.org/r171747>.

Created attachment 235706 [details]
WIP for EWS

[WebKit Commit Bot]

Attachment 235706 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/cocoa/CredentialCocoa.mm:89:  Missing spaces around :  [whitespace/init] [4]
ERROR: Source/WebCore/platform/network/cf/AuthenticationCF.cpp:58:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebCore/platform/network/cf/AuthenticationCF.cpp:58:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/network/mac/AuthenticationMac.mm:156:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebCore/platform/network/mac/AuthenticationMac.mm:156:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/network/mac/AuthenticationMac.mm:202:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/Shared/mac/WebCoreArgumentCodersMac.mm:340:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/Shared/mac/WebCoreArgumentCodersMac.mm:340:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 8 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 235716 [details]
III. Make Credential hold onto an NSURLCredential when needed

[WebKit Commit Bot]

Attachment 235716 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/cocoa/CredentialCocoa.mm:91:  Missing spaces around :  [whitespace/init] [4]
ERROR: Source/WebCore/platform/network/cf/AuthenticationCF.cpp:58:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebCore/platform/network/cf/AuthenticationCF.cpp:58:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/network/mac/AuthenticationMac.mm:156:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebCore/platform/network/mac/AuthenticationMac.mm:156:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/network/mac/AuthenticationMac.mm:202:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/Shared/mac/WebCoreArgumentCodersMac.mm:340:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/Shared/mac/WebCoreArgumentCodersMac.mm:340:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 8 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Committed attachment 235716 [details] as <http://trac.webkit.org/r171801>.

Created attachment 235788 [details]
IV. Allow the delegate to respond to server trust challenges

Oops! I used a script that obsoleted all other parts and removed their review info. But parts I-III are in. Part IV is what remains.

[Csaba Osztrogonác]

(In reply to comment #15)
> Committed attachment 235716 [details] as <http://trac.webkit.org/r171801>.

Commit queue is stucked after this change with the following error:

/Volumes/Data/EWS/WebKit/Source/WebCore/platform/network/cocoa/CredentialCocoa.mm:55:13: error: enumeration value 'NSURLCredentialPersistenceSynchronizable' not handled in switch [-Werror,-Wswitch]
    switch (persistence) {
            ^
1 error generated.


How is it possible if there isn't build failure on buildbots,
but only the EWS. Do they have different configuration? Why?

(In reply to comment #18)
> (In reply to comment #15)
> > Committed attachment 235716 [details] [details] as <http://trac.webkit.org/r171801>.
> 
> Commit queue is stucked after this change with the following error:
> 
> /Volumes/Data/EWS/WebKit/Source/WebCore/platform/network/cocoa/CredentialCocoa.mm:55:13: error: enumeration value 'NSURLCredentialPersistenceSynchronizable' not handled in switch [-Werror,-Wswitch]
>     switch (persistence) {
>             ^
> 1 error generated.
> 
> 
> How is it possible if there isn't build failure on buildbots,
> but only the EWS. Do they have different configuration? Why?

This error occurs when using the OS X 10.9 SDK to build for OS X 10.8.

[Csaba Osztrogonác]

(In reply to comment #19)
> This error occurs when using the OS X 10.9 SDK to build for OS X 10.8.

OK, and how can we fix the commit queue?

(In reply to comment #20)
> (In reply to comment #19)
> > This error occurs when using the OS X 10.9 SDK to build for OS X 10.8.
> 
> OK, and how can we fix the commit queue?

I am hoping <http://trac.webkit.org/r171854> fixes this particular build configuration.

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > This error occurs when using the OS X 10.9 SDK to build for OS X 10.8.
> > 
> > OK, and how can we fix the commit queue?
> 
> I am hoping <http://trac.webkit.org/r171854> fixes this particular build configuration.

I had to revert that change, since it broke building with the OS X 10.8 SDK. I will try something else in a moment.

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > (In reply to comment #19)
> > > > This error occurs when using the OS X 10.9 SDK to build for OS X 10.8.
> > > 
> > > OK, and how can we fix the commit queue?
> > 
> > I am hoping <http://trac.webkit.org/r171854> fixes this particular build configuration.
> 
> I had to revert that change, since it broke building with the OS X 10.8 SDK. I will try something else in a moment.

Tried another fix in <http://trac.webkit.org/r171857>.

Committed attachment 235788 [details] as <http://trac.webkit.org/r171858>.

(In reply to comment #15)
> Committed attachment 235716 [details] as <http://trac.webkit.org/r171801>.

This broke client certificate authentication, because NSURLCredential doesn’t serialize identities properly. See bug 138144.

*** Bug 140197 has been marked as a duplicate of this bug. ***

*** Bug 140197 has been marked as a duplicate of this bug. ***