Changeset 247162 in webkit

Timestamp:

Jul 5, 2019 9:59:42 AM (5 years ago)

Author:

Ryan Haddad

Message:

Unreviewed, rolling out r247123.

Caused TestWebKitAPI.Challenge.BasicProposedCredential to
fail.

Reverted changeset:

"Only allow fetching and removing session credentials from
WebsiteDataStore"
​https://bugs.webkit.org/show_bug.cgi?id=199385
​https://trac.webkit.org/changeset/247123

Location:

trunk

Files:

• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/platform/network/CredentialStorage.cpp (modified) (3 diffs)
• Source/WebCore/platform/network/CredentialStorage.h (modified) (2 diffs)
• Source/WebCore/platform/network/mac/CredentialStorageMac.mm (modified) (1 diff)
• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/NetworkProcess/NetworkProcess.cpp (modified) (5 diffs)
• Source/WebKit/NetworkProcess/NetworkProcess.h (modified) (1 diff)
• Source/WebKit/NetworkProcess/NetworkProcess.messages.in (modified) (1 diff)
• Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm (modified) (1 diff)
• Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp (modified) (3 diffs)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebsiteDatastore.mm (modified) (3 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-07-05  Ryan Haddad  <ryanhaddad@apple.com>
+
+        Unreviewed, rolling out r247123.
+
+        Caused TestWebKitAPI.Challenge.BasicProposedCredential to
+        fail.
+
+        Reverted changeset:
+
+        "Only allow fetching and removing session credentials from
+        WebsiteDataStore"
+        https://bugs.webkit.org/show_bug.cgi?id=199385
+        https://trac.webkit.org/changeset/247123
+
 2019-07-05  Youenn Fablet  <youenn@apple.com>
 

trunk/Source/WebCore/platform/network/CredentialStorage.cpp

@@ -103 +103 @@
 }
 
-HashSet<SecurityOriginData> CredentialStorage::originsWithCredentials() const
+Vector<SecurityOriginData> CredentialStorage::originsWithCredentials() const
 {
-    HashSet<SecurityOriginData> origins;
+    Vector<SecurityOriginData> origins;
     for (auto& keyValuePair : m_protectionSpaceToCredentialMap) {
         auto& protectionSpace = keyValuePair.key.second;
@@ -130 +130 @@
 
         SecurityOriginData origin { protocol, protectionSpace.host(), static_cast<uint16_t>(protectionSpace.port())};
-        origins.add(WTFMove(origin));
+        origins.append(WTFMove(origin));
     }
     return origins;
@@ -188 +188 @@
 }
 
-#if !PLATFORM(COCOA)
-HashSet<SecurityOriginData> CredentialStorage::originsWithSessionCredentials()
-{
-    return { };
-}
-
-void CredentialStorage::removeSessionCredentialsWithOrigins(const Vector<SecurityOriginData>&)
-{
-}
-
-void CredentialStorage::clearSessionCredentials()
-{
-}
-#endif
-
 } // namespace WebCore

trunk/Source/WebCore/platform/network/CredentialStorage.h

@@ -46 +46 @@
     WEBCORE_EXPORT void removeCredentialsWithOrigin(const SecurityOriginData&);
 
-    // OS credential storage.
+    // OS persistent storage.
     WEBCORE_EXPORT static Credential getFromPersistentStorage(const ProtectionSpace&);
-    WEBCORE_EXPORT static HashSet<SecurityOriginData> originsWithSessionCredentials();
-    WEBCORE_EXPORT static void removeSessionCredentialsWithOrigins(const Vector<SecurityOriginData>& origins);
-    WEBCORE_EXPORT static void clearSessionCredentials();
+    WEBCORE_EXPORT static Vector<SecurityOriginData> originsWithPersistentCredentials();
 
     WEBCORE_EXPORT void clearCredentials();
@@ -59 +57 @@
     WEBCORE_EXPORT Credential get(const String&, const URL&);
 
-    WEBCORE_EXPORT HashSet<SecurityOriginData> originsWithCredentials() const;
+    WEBCORE_EXPORT Vector<SecurityOriginData> originsWithCredentials() const;
 
 private:

trunk/Source/WebCore/platform/network/mac/CredentialStorageMac.mm

@@ -39 +39 @@
 }
 
-HashSet<SecurityOriginData> CredentialStorage::originsWithSessionCredentials()
+Vector<WebCore::SecurityOriginData> CredentialStorage::originsWithPersistentCredentials()
 {
-    HashSet<SecurityOriginData> origins;
+    Vector<WebCore::SecurityOriginData> origins;
     auto allCredentials = [[NSURLCredentialStorage sharedCredentialStorage] allCredentials];
-    for (NSURLProtectionSpace* key in allCredentials.keyEnumerator) {
-        for (NSURLProtectionSpace* space in allCredentials) {
-            auto credentials = allCredentials[space];
-            for (NSString* user in credentials) {
-                if (credentials[user].persistence == NSURLCredentialPersistenceForSession) {
-                    origins.add(WebCore::SecurityOriginData { String(key.protocol), String(key.host), key.port });
-                    break;
-                }
-            }
-        }
-    }
+    for (NSURLProtectionSpace* key in allCredentials.keyEnumerator)
+        origins.append(WebCore::SecurityOriginData { String(key.protocol), String(key.host), key.port });
     return origins;
 }
 
-void CredentialStorage::removeSessionCredentialsWithOrigins(const Vector<SecurityOriginData>& origins)
-{
-    auto sharedStorage = [NSURLCredentialStorage sharedCredentialStorage];
-    auto allCredentials = [sharedStorage allCredentials];
-    for (auto& origin : origins) {
-        for (NSURLProtectionSpace* space in allCredentials) {
-            if (origin.protocol == String(space.protocol)
-                && origin.host == String(space.host)
-                && origin.port
-                && *origin.port == space.port) {
-                    auto credentials = allCredentials[space];
-                    for (NSString* user in credentials) {
-                        auto credential = credentials[user];
-                        if (credential.persistence == NSURLCredentialPersistenceForSession)
-                            [sharedStorage removeCredential:credential forProtectionSpace:space];
-                }
-            }
-        }
-    }
-}
-
-void CredentialStorage::clearSessionCredentials()
-{
-    auto sharedStorage = [NSURLCredentialStorage sharedCredentialStorage];
-    auto allCredentials = [sharedStorage allCredentials];
-    for (NSURLProtectionSpace* space in allCredentials.keyEnumerator) {
-        auto credentials = allCredentials[space];
-        for (NSString* user in credentials) {
-            auto credential = credentials[user];
-            if (credential.persistence == NSURLCredentialPersistenceForSession)
-                [sharedStorage removeCredential:credential forProtectionSpace:space];
-        }
-    }
-}
-
 } // namespace WebCore

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2019-07-05  Ryan Haddad  <ryanhaddad@apple.com>
+
+        Unreviewed, rolling out r247123.
+
+        Caused TestWebKitAPI.Challenge.BasicProposedCredential to
+        fail.
+
+        Reverted changeset:
+
+        "Only allow fetching and removing session credentials from
+        WebsiteDataStore"
+        https://bugs.webkit.org/show_bug.cgi?id=199385
+        https://trac.webkit.org/changeset/247123
+
 2019-07-05  Wenson Hsieh  <wenson_hsieh@apple.com>
 

trunk/Source/WebKit/NetworkProcess/NetworkProcess.cpp

@@ -1299 +1299 @@
                 callbackAggregator->m_websiteData.entries.append({ securityOrigin, WebsiteDataType::Credentials, 0 });
         }
-        auto securityOrigins = WebCore::CredentialStorage::originsWithSessionCredentials();
-        for (auto& securityOrigin : securityOrigins)
-            callbackAggregator->m_websiteData.entries.append({ securityOrigin, WebsiteDataType::Credentials, 0 });
     }
 
@@ -1383 +1380 @@
         if (auto* session = storageSession(sessionID))
             session->credentialStorage().clearCredentials();
-        WebCore::CredentialStorage::clearSessionCredentials();
     }
 
@@ -1521 +1517 @@
                 session->credentialStorage().removeCredentialsWithOrigin(originData);
         }
-        WebCore::CredentialStorage::removeSessionCredentialsWithOrigins(originDatas);
     }
 
@@ -1666 +1661 @@
 #endif
 
+    /*
+    // FIXME: No API to delete credentials by origin
+    HashSet<String> originsWithCredentials;
     if (websiteDataTypes.contains(WebsiteDataType::Credentials)) {
-        if (auto* session = storageSession(sessionID)) {
-            auto origins = session->credentialStorage().originsWithCredentials();
-            auto originsToDelete = filterForRegistrableDomains(origins, domainsToDeleteAllButCookiesFor, callbackAggregator->m_domains);
-            for (auto& origin : originsToDelete)
-                session->credentialStorage().removeCredentialsWithOrigin(origin);
-        }
-
-        auto origins = WebCore::CredentialStorage::originsWithSessionCredentials();
-        auto originsToDelete = filterForRegistrableDomains(origins, domainsToDeleteAllButCookiesFor, callbackAggregator->m_domains);
-        WebCore::CredentialStorage::removeSessionCredentialsWithOrigins(originsToDelete);
-    }
+        if (storageSession(sessionID))
+            originsWithCredentials = storageSession(sessionID)->credentialStorage().originsWithCredentials();
+    }
+    */
     
     if (websiteDataTypes.contains(WebsiteDataType::DOMCache)) {
@@ -2573 +2564 @@
 
 #if !PLATFORM(COCOA)
+void NetworkProcess::originsWithPersistentCredentials(CompletionHandler<void(Vector<WebCore::SecurityOriginData>)>&& completionHandler)
+{
+    completionHandler(Vector<WebCore::SecurityOriginData>());
+}
+
+void NetworkProcess::removeCredentialsWithOrigins(const Vector<WebCore::SecurityOriginData>&, CompletionHandler<void()>&& completionHandler)
+{
+    completionHandler();
+}
+
 void NetworkProcess::initializeProcess(const AuxiliaryProcessInitializationParameters&)
 {

trunk/Source/WebKit/NetworkProcess/NetworkProcess.h

@@ -437 +437 @@
 
     void platformSyncAllCookies(CompletionHandler<void()>&&);
+
+    void originsWithPersistentCredentials(CompletionHandler<void(Vector<WebCore::SecurityOriginData>)>&&);
+    void removeCredentialsWithOrigins(const Vector<WebCore::SecurityOriginData>& origins, CompletionHandler<void()>&&);
     
     void registerURLSchemeAsSecure(const String&) const;

trunk/Source/WebKit/NetworkProcess/NetworkProcess.messages.in

@@ -169 +169 @@
     SetAdClickAttributionConversionURLForTesting(PAL::SessionID sessionID, URL url) -> () Async
     MarkAdClickAttributionsAsExpiredForTesting(PAL::SessionID sessionID) -> () Async
+    OriginsWithPersistentCredentials() -> (Vector<WebCore::SecurityOriginData> origins) Async
+    RemoveCredentialsWithOrigins(Vector<WebCore::SecurityOriginData> origins) -> () Async
     GetLocalStorageOriginDetails(PAL::SessionID sessionID) -> (Vector<WebKit::LocalStorageDatabaseTracker::OriginDetails> details) Async
 }

trunk/Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm

@@ -213 +213 @@
 }
 
+void NetworkProcess::originsWithPersistentCredentials(CompletionHandler<void(Vector<WebCore::SecurityOriginData>)>&& completionHandler)
+{
+    completionHandler(WebCore::CredentialStorage::originsWithPersistentCredentials());
+}
+
+void NetworkProcess::removeCredentialsWithOrigins(const Vector<WebCore::SecurityOriginData>& origins, CompletionHandler<void()>&& completionHandler)
+{
+    for (auto& origin : origins) {
+        auto allCredentials = [[NSURLCredentialStorage sharedCredentialStorage] allCredentials];
+        for (NSURLProtectionSpace* space in allCredentials) {
+            if (origin.protocol == String(space.protocol)
+                && origin.host == String(space.host)
+                && origin.port
+                && *origin.port == space.port) {
+                auto credentials = allCredentials[space];
+                for (NSString* user in credentials) {
+                    auto credential = credentials[user];
+                    [[NSURLCredentialStorage sharedCredentialStorage] removeCredential:credential forProtectionSpace:space];
+                }
+            }
+        }
+    }
+    completionHandler();
+}
+
 #if PLATFORM(MAC)
 void NetworkProcess::setSharedHTTPCookieStorage(const Vector<uint8_t>& identifier)

trunk/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp

@@ -521 +521 @@
     }
 
+#if PLATFORM(COCOA)
+    if (dataTypes.contains(WebsiteDataType::Credentials) && isPersistent()) {
+        for (auto& processPool : processPools()) {
+            if (!processPool->networkProcess())
+                continue;
+            
+            callbackAggregator->addPendingCallback();
+            WTF::CompletionHandler<void(Vector<WebCore::SecurityOriginData>&&)> completionHandler = [callbackAggregator](Vector<WebCore::SecurityOriginData>&& origins) mutable {
+                WebsiteData websiteData;
+                for (auto& origin : origins)
+                    websiteData.entries.append(WebsiteData::Entry { origin, WebsiteDataType::Credentials, 0 });
+                callbackAggregator->removePendingCallback(WTFMove(websiteData));
+            };
+            processPool->networkProcess()->sendWithAsyncReply(Messages::NetworkProcess::OriginsWithPersistentCredentials(), WTFMove(completionHandler));
+        }
+    }
+#endif
+
 #if ENABLE(NETSCAPE_PLUGIN_API)
     if (dataTypes.contains(WebsiteDataType::PlugInData) && isPersistent()) {
@@ -627 +645 @@
         processAccessType = std::max(processAccessType, ProcessAccessType::OnlyIfLaunched);
 
+    if (dataTypes.contains(WebsiteDataType::Credentials))
+        processAccessType = std::max(processAccessType, ProcessAccessType::OnlyIfLaunched);
+
     return processAccessType;
 }
@@ -1073 +1094 @@
     }
 
+    if (dataTypes.contains(WebsiteDataType::Credentials) && isPersistent()) {
+        for (auto& processPool : processPools()) {
+            if (!processPool->networkProcess())
+                continue;
+            
+            callbackAggregator->addPendingCallback();
+            WTF::CompletionHandler<void()> completionHandler = [callbackAggregator]() mutable {
+                callbackAggregator->removePendingCallback();
+            };
+            processPool->networkProcess()->sendWithAsyncReply(Messages::NetworkProcess::RemoveCredentialsWithOrigins(origins), WTFMove(completionHandler));
+        }
+    }
+
 #if ENABLE(NETSCAPE_PLUGIN_API)
     if (dataTypes.contains(WebsiteDataType::PlugInData) && isPersistent()) {

trunk/Tools/ChangeLog

@@ +1 @@
+2019-07-05  Ryan Haddad  <ryanhaddad@apple.com>
+
+        Unreviewed, rolling out r247123.
+
+        Caused TestWebKitAPI.Challenge.BasicProposedCredential to
+        fail.
+
+        Reverted changeset:
+
+        "Only allow fetching and removing session credentials from
+        WebsiteDataStore"
+        https://bugs.webkit.org/show_bug.cgi?id=199385
+        https://trac.webkit.org/changeset/247123
+
 2019-07-05  Wenson Hsieh  <wenson_hsieh@apple.com>
 

trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebsiteDatastore.mm

@@ -93 +93 @@
     readyToContinue = false;
     [[WKWebsiteDataStore defaultDataStore] fetchDataRecordsOfTypes:[WKWebsiteDataStore _allWebsiteDataTypesIncludingPrivate] completionHandler:^(NSArray<WKWebsiteDataRecord *> *dataRecords) {
-        EXPECT_EQ(0u, dataRecords.count);
+        ASSERT_EQ(0u, dataRecords.count);
         readyToContinue = true;
     }];
@@ -140 +140 @@
 {
     TCPServer server(TCPServer::respondWithChallengeThenOK);
-
+    
     usePersistentCredentialStorage = true;
     auto websiteDataStore = [WKWebsiteDataStore defaultDataStore];
@@ -152 +152 @@
     [websiteDataStore fetchDataRecordsOfTypes:[NSSet setWithObject:_WKWebsiteDataTypeCredentials] completionHandler:^(NSArray<WKWebsiteDataRecord *> *dataRecords) {
         int credentialCount = dataRecords.count;
-        EXPECT_EQ(credentialCount, 0);
+        ASSERT_GT(credentialCount, 0);
+        bool foundExpectedRecord = false;
+        for (WKWebsiteDataRecord *record in dataRecords) {
+            auto name = [record displayName];
+            if ([name isEqualToString:@"127.0.0.1"]) {
+                foundExpectedRecord = true;
+                break;
+            }
+        }
+        EXPECT_TRUE(foundExpectedRecord);
+        done = true;
+    }];
+    TestWebKitAPI::Util::run(&done);
+    
+    __block bool removedCredential = false;
+    [websiteDataStore fetchDataRecordsOfTypes:[NSSet setWithObject:_WKWebsiteDataTypeCredentials] completionHandler:^(NSArray<WKWebsiteDataRecord *> *dataRecords) {
+        [websiteDataStore removeDataOfTypes:[NSSet setWithObject:_WKWebsiteDataTypeCredentials] forDataRecords:dataRecords completionHandler:^(void) {
+            removedCredential = true;
+        }];
+    }];
+    TestWebKitAPI::Util::run(&removedCredential);
+}
+
+TEST(WKWebsiteDataStore, RemovePersistentCredentials)
+{
+    TCPServer server(TCPServer::respondWithChallengeThenOK);
+
+    usePersistentCredentialStorage = true;
+    auto websiteDataStore = [WKWebsiteDataStore defaultDataStore];
+    auto navigationDelegate = adoptNS([[NavigationTestDelegate alloc] init]);
+    auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600)]);
+    [webView setNavigationDelegate:navigationDelegate.get()];
+    [webView loadRequest:[NSURLRequest requestWithURL:[NSURL URLWithString:[NSString stringWithFormat:@"http://127.0.0.1:%d/", server.port()]]]];
+    [navigationDelegate waitForDidFinishNavigation];
+
+    __block bool done = false;
+    __block RetainPtr<WKWebsiteDataRecord> expectedRecord;
+    [websiteDataStore fetchDataRecordsOfTypes:[NSSet setWithObject:_WKWebsiteDataTypeCredentials] completionHandler:^(NSArray<WKWebsiteDataRecord *> *dataRecords) {
+        int credentialCount = dataRecords.count;
+        ASSERT_GT(credentialCount, 0);
+        for (WKWebsiteDataRecord *record in dataRecords) {
+            auto name = [record displayName];
+            if ([name isEqualToString:@"127.0.0.1"]) {
+                expectedRecord = record;
+                break;
+            }
+        }
+        EXPECT_TRUE(expectedRecord);
+        done = true;
+    }];
+    TestWebKitAPI::Util::run(&done);
+
+    done = false;
+    [websiteDataStore removeDataOfTypes:[NSSet setWithObject:_WKWebsiteDataTypeCredentials] forDataRecords:[NSArray arrayWithObject:expectedRecord.get()] completionHandler:^(void) {
+        done = true;
+    }];
+    TestWebKitAPI::Util::run(&done);
+
+    done = false;
+    [websiteDataStore fetchDataRecordsOfTypes:[NSSet setWithObject:_WKWebsiteDataTypeCredentials] completionHandler:^(NSArray<WKWebsiteDataRecord *> *dataRecords) {
+        bool foundLocalHostRecord = false;
+        for (WKWebsiteDataRecord *record in dataRecords) {
+            auto name = [record displayName];
+            if ([name isEqualToString:@"127.0.0.1"]) {
+                foundLocalHostRecord = true;
+                break;
+            }
+        }
+        EXPECT_FALSE(foundLocalHostRecord);
         done = true;
     }];