[webkit-reviews] review denied: [Bug 171217] Support removal of authentication data through WebsiteDataStore. : [Attachment 307978] Patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Per Arne Vollan <pvollan at apple.com>'s request for
review:
Bug 171217: Support removal of authentication data through WebsiteDataStore.
https://bugs.webkit.org/show_bug.cgi?id=171217

Attachment 307978: Patch

https://bugs.webkit.org/attachment.cgi?id=307978&action=review




--- Comment #5 from mitz at webkit.org ---
Comment on attachment 307978
  --> https://bugs.webkit.org/attachment.cgi?id=307978
Patch

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

At first sight, it seems very strange to treat user credentials as website
data. The credentials are user data, so I don’t understand how it makes sense,
even conceptually, to expose them via the website data store API. The
implementation further shows how confusing or misleading this is. Because
persisting website credentials isn’t under WebKit’s purview, even removing
credentials from a persistent website data store doesn’t remove persistent
credentials.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebsiteDataRecord.h:59
> +WK_EXTERN NSString * const WKWebsiteDataTypeCredentials
WK_API_AVAILABLE(macosx(10.12), ios(10.0));

This availability attribute is clearly wrong, because this is not part of the
macOS 10.12 and iOS 10 API.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:779
> +    if (dataTypes.contains(WebsiteDataType::Credentials)) {
> +	   auto storage =
WebCore::NetworkStorageSession::defaultStorageSession().credentialStorage();
> +	   storage.removeCredentialsModifiedSince(modifiedSince);
> +    }

So this operates on the global default storage session regardless of which
WKWebsiteDataStore the client messaged?