Unreviewed, rolling out r247123.
authorryanhaddad@apple.com <ryanhaddad@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 5 Jul 2019 16:59:42 +0000 (16:59 +0000)
committerryanhaddad@apple.com <ryanhaddad@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 5 Jul 2019 16:59:42 +0000 (16:59 +0000)
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

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@247162 268f45cc-cd09-0410-ab3c-d52691b4dbfc

12 files changed:
Source/WebCore/ChangeLog
Source/WebCore/platform/network/CredentialStorage.cpp
Source/WebCore/platform/network/CredentialStorage.h
Source/WebCore/platform/network/mac/CredentialStorageMac.mm
Source/WebKit/ChangeLog
Source/WebKit/NetworkProcess/NetworkProcess.cpp
Source/WebKit/NetworkProcess/NetworkProcess.h
Source/WebKit/NetworkProcess/NetworkProcess.messages.in
Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm
Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebsiteDatastore.mm

index b5d1e1e..d87dbc8 100644 (file)
@@ -1,3 +1,17 @@
+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>
 
         ASSERT that a sessionID is valid when encoding it
index 26a3b9e..98e9426 100644 (file)
@@ -102,9 +102,9 @@ void CredentialStorage::removeCredentialsWithOrigin(const SecurityOriginData& or
         remove(key.first, key.second);
 }
 
-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;
         if (protectionSpace.isProxy())
@@ -129,7 +129,7 @@ HashSet<SecurityOriginData> CredentialStorage::originsWithCredentials() const
         }
 
         SecurityOriginData origin { protocol, protectionSpace.host(), static_cast<uint16_t>(protectionSpace.port())};
-        origins.add(WTFMove(origin));
+        origins.append(WTFMove(origin));
     }
     return origins;
 }
@@ -187,19 +187,4 @@ void CredentialStorage::clearCredentials()
     m_pathToDefaultProtectionSpaceMap.clear();
 }
 
-#if !PLATFORM(COCOA)
-HashSet<SecurityOriginData> CredentialStorage::originsWithSessionCredentials()
-{
-    return { };
-}
-
-void CredentialStorage::removeSessionCredentialsWithOrigins(const Vector<SecurityOriginData>&)
-{
-}
-
-void CredentialStorage::clearSessionCredentials()
-{
-}
-#endif
-
 } // namespace WebCore
index d0e16f7..f8c75ae 100644 (file)
@@ -45,11 +45,9 @@ public:
     WEBCORE_EXPORT void remove(const String&, const ProtectionSpace&);
     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();
 
@@ -58,7 +56,7 @@ public:
     WEBCORE_EXPORT bool set(const String&, const Credential&, const URL&); // Returns true if the URL corresponds to a known protection space, so credentials could be updated.
     WEBCORE_EXPORT Credential get(const String&, const URL&);
 
-    WEBCORE_EXPORT HashSet<SecurityOriginData> originsWithCredentials() const;
+    WEBCORE_EXPORT Vector<SecurityOriginData> originsWithCredentials() const;
 
 private:
     HashMap<std::pair<String /* partitionName */, ProtectionSpace>, Credential> m_protectionSpaceToCredentialMap;
index a1c508a..65b5151 100644 (file)
@@ -38,57 +38,13 @@ Credential CredentialStorage::getFromPersistentStorage(const ProtectionSpace& pr
     return credential ? Credential(credential) : Credential();
 }
 
-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
index 22e4237..d0b709c 100644 (file)
@@ -1,3 +1,17 @@
+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>
 
         Click events on outer page are not being dispatched correctly after touch-zooming within an iframe
index 472a743..e8e15ca 100644 (file)
@@ -1298,9 +1298,6 @@ void NetworkProcess::fetchWebsiteData(PAL::SessionID sessionID, OptionSet<Websit
             for (auto& securityOrigin : securityOrigins)
                 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 });
     }
 
     if (websiteDataTypes.contains(WebsiteDataType::DOMCache)) {
@@ -1382,7 +1379,6 @@ void NetworkProcess::deleteWebsiteData(PAL::SessionID sessionID, OptionSet<Websi
     if (websiteDataTypes.contains(WebsiteDataType::Credentials)) {
         if (auto* session = storageSession(sessionID))
             session->credentialStorage().clearCredentials();
-        WebCore::CredentialStorage::clearSessionCredentials();
     }
 
     auto clearTasksHandler = WTF::CallbackAggregator::create([this, callbackID] {
@@ -1520,7 +1516,6 @@ void NetworkProcess::deleteWebsiteDataForOrigins(PAL::SessionID sessionID, Optio
             for (auto& originData : originDatas)
                 session->credentialStorage().removeCredentialsWithOrigin(originData);
         }
-        WebCore::CredentialStorage::removeSessionCredentialsWithOrigins(originDatas);
     }
 
     // FIXME: Implement storage quota clearing for these origins.
@@ -1665,18 +1660,14 @@ void NetworkProcess::deleteWebsiteDataForRegistrableDomains(PAL::SessionID sessi
     }
 #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)) {
         CacheStorage::Engine::fetchEntries(*this, sessionID, fetchOptions.contains(WebsiteDataFetchOption::ComputeSizes), [this, domainsToDeleteAllButCookiesFor, sessionID, callbackAggregator = callbackAggregator.copyRef()](auto entries) mutable {
@@ -2572,6 +2563,16 @@ StorageQuotaManager& NetworkProcess::storageQuotaManager(PAL::SessionID sessionI
 }
 
 #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&)
 {
 }
index ffd4868..83b4f34 100644 (file)
@@ -436,6 +436,9 @@ private:
 #endif
 
     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;
     void registerURLSchemeAsBypassingContentSecurityPolicy(const String&) const;
index 91fe69d..6d06240 100644 (file)
@@ -168,5 +168,7 @@ messages -> NetworkProcess LegacyReceiver {
     SetAdClickAttributionOverrideTimerForTesting(PAL::SessionID sessionID, bool value) -> () Async
     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
 }
index 5b52cab..bdefe9e 100644 (file)
@@ -212,6 +212,31 @@ void NetworkProcess::clearDiskCache(WallTime modifiedSince, CompletionHandler<vo
     }).get());
 }
 
+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)
 {
index 5ab53da..0eec801 100644 (file)
@@ -520,6 +520,24 @@ void WebsiteDataStore::fetchDataAndApply(OptionSet<WebsiteDataType> dataTypes, O
         });
     }
 
+#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()) {
         class State {
@@ -626,6 +644,9 @@ static ProcessAccessType computeWebProcessAccessTypeForDataRemoval(OptionSet<Web
     if (dataTypes.contains(WebsiteDataType::MemoryCache))
         processAccessType = std::max(processAccessType, ProcessAccessType::OnlyIfLaunched);
 
+    if (dataTypes.contains(WebsiteDataType::Credentials))
+        processAccessType = std::max(processAccessType, ProcessAccessType::OnlyIfLaunched);
+
     return processAccessType;
 }
 
@@ -1072,6 +1093,19 @@ void WebsiteDataStore::removeData(OptionSet<WebsiteDataType> dataTypes, const Ve
         });
     }
 
+    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()) {
         Vector<String> hostNames;
index 138c46e..b88308f 100644 (file)
@@ -1,3 +1,17 @@
+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>
 
         Touching media controls sometimes shows software keyboard
index 0e689aa..fae0a34 100644 (file)
@@ -92,7 +92,7 @@ TEST(WKWebsiteDataStore, RemoveAndFetchData)
     
     readyToContinue = false;
     [[WKWebsiteDataStore defaultDataStore] fetchDataRecordsOfTypes:[WKWebsiteDataStore _allWebsiteDataTypesIncludingPrivate] completionHandler:^(NSArray<WKWebsiteDataRecord *> *dataRecords) {
-        EXPECT_EQ(0u, dataRecords.count);
+        ASSERT_EQ(0u, dataRecords.count);
         readyToContinue = true;
     }];
     TestWebKitAPI::Util::run(&readyToContinue);
@@ -139,6 +139,44 @@ TEST(WKWebsiteDataStore, FetchNonPersistentCredentials)
 TEST(WKWebsiteDataStore, FetchPersistentCredentials)
 {
     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;
+    [websiteDataStore fetchDataRecordsOfTypes:[NSSet setWithObject:_WKWebsiteDataTypeCredentials] completionHandler:^(NSArray<WKWebsiteDataRecord *> *dataRecords) {
+        int credentialCount = dataRecords.count;
+        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];
@@ -149,9 +187,39 @@ TEST(WKWebsiteDataStore, FetchPersistentCredentials)
     [navigationDelegate waitForDidFinishNavigation];
 
     __block bool done = false;
+    __block RetainPtr<WKWebsiteDataRecord> expectedRecord;
     [websiteDataStore fetchDataRecordsOfTypes:[NSSet setWithObject:_WKWebsiteDataTypeCredentials] completionHandler:^(NSArray<WKWebsiteDataRecord *> *dataRecords) {
         int credentialCount = dataRecords.count;
-        EXPECT_EQ(credentialCount, 0);
+        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;
     }];
     TestWebKitAPI::Util::run(&done);