WebKit needs to support removal of authentication data through WebsiteDataStore.
Created attachment 307978 [details] Patch
I think this needs to be tested since time comparisons are tricky. The ability to test might require a way to set the timestamp explicitly. Setting the timestamp will also be required if we are to support loading of persisted credentials.
(In reply to John Wilander from comment #2) > I think this needs to be tested since time comparisons are tricky. The > ability to test might require a way to set the timestamp explicitly. Setting > the timestamp will also be required if we are to support loading of > persisted credentials. Thanks for reviewing! I will add test(s).
rdar://problem/29522573
Comment on attachment 307978 [details] 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?
(In reply to mitz from comment #5) > Comment on attachment 307978 [details] > 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. Clients tell WebKit which website datatypes to remove. Would you add another parameter type or have a whole new API for this? We need both a "clear all" and a "clear modified since." > > 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? You're right. This needs to happen on either the right storage session or all storage sessions. To be able to test it we probably need to get a test runner callback when credentials have been removed.
(In reply to John Wilander from comment #6) > (In reply to mitz from comment #5) > > Comment on attachment 307978 [details] > > 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. > > Clients tell WebKit which website datatypes to remove. Would you add another > parameter type or have a whole new API for this? We need both a "clear all" > and a "clear modified since." This data type isn’t a website data type, so it seems inappropriate to include in the website data API. The notion if a website credential’s modification date is also strange. What is the use case for forgetting only credentials modified since some time?
Comment on attachment 307978 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307978&action=review >>> 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. > > You're right. This needs to happen on either the right storage session or all storage sessions. To be able to test it we probably need to get a test runner callback when credentials have been removed. This should probably be WK_API_AVAILBLE(WK_MAC_TBA, WK_IOS_TBA)
(In reply to mitz from comment #7) > (In reply to John Wilander from comment #6) > > (In reply to mitz from comment #5) > > > Comment on attachment 307978 [details] > > > 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. > > > > Clients tell WebKit which website datatypes to remove. Would you add another > > parameter type or have a whole new API for this? We need both a "clear all" > > and a "clear modified since." > > This data type isn’t a website data type, so it seems inappropriate to > include in the website data API. > > The notion if a website credential’s modification date is also strange. What > is the use case for forgetting only credentials modified since some time? This gives Safari the ability to include it in clearing UI that is "clear for the last hour" to cover tracks. HTTP auth could also be used by websites to surreptitiously track users (e.g. by putting HTTP auth credentials in the URL of a subresource). Per Arne Vollan - is this going to clear the data that is stored in keychain as well? That seems different than what the original goal was here (clear authentication that is not stored in keychain, since the user can clear it from keychain).
(In reply to Jessie Berlin from comment #9) > (In reply to mitz from comment #7) > > (In reply to John Wilander from comment #6) > > > (In reply to mitz from comment #5) > > > > Comment on attachment 307978 [details] > > > > 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. > > > > > > Clients tell WebKit which website datatypes to remove. Would you add another > > > parameter type or have a whole new API for this? We need both a "clear all" > > > and a "clear modified since." > > > > This data type isn’t a website data type, so it seems inappropriate to > > include in the website data API. > > > > The notion if a website credential’s modification date is also strange. What > > is the use case for forgetting only credentials modified since some time? > > This gives Safari the ability to include it in clearing UI that is "clear > for the last hour" to cover tracks. So in this hypothetical Safari is going to also remove the corresponding Keychain items? This proposed API doesn’t seem to support something like that. > HTTP auth could also be used by websites > to surreptitiously track users (e.g. by putting HTTP auth credentials in the > URL of a subresource). > > Per Arne Vollan - is this going to clear the data that is stored in keychain > as well? That seems different than what the original goal was here (clear > authentication that is not stored in keychain, since the user can clear it > from keychain).
(In reply to Jessie Berlin from comment #9) > > > > The notion if a website credential’s modification date is also strange. What > > is the use case for forgetting only credentials modified since some time? > > This gives Safari the ability to include it in clearing UI that is "clear > for the last hour" to cover tracks. HTTP auth could also be used by websites > to surreptitiously track users (e.g. by putting HTTP auth credentials in the > URL of a subresource). > > Per Arne Vollan - is this going to clear the data that is stored in keychain > as well? That seems different than what the original goal was here (clear > authentication that is not stored in keychain, since the user can clear it > from keychain). I believe the goal here is to only remove the items in the Credential Storage, and not remove Keychain items.
(In reply to mitz from comment #7) > (In reply to John Wilander from comment #6) > > (In reply to mitz from comment #5) > > > Comment on attachment 307978 [details] > > > 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. > > > > Clients tell WebKit which website datatypes to remove. Would you add another > > parameter type or have a whole new API for this? We need both a "clear all" > > and a "clear modified since." > > This data type isn’t a website data type, so it seems inappropriate to > include in the website data API. > I am currently working on creating a new C API for this. I will upload a modified patch soon. Thanks for reviewing!
(In reply to mitz from comment #10) > (In reply to Jessie Berlin from comment #9) > > (In reply to mitz from comment #7) > > > (In reply to John Wilander from comment #6) > > > > (In reply to mitz from comment #5) > > > > > Comment on attachment 307978 [details] > > > > > 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. > > > > > > > > Clients tell WebKit which website datatypes to remove. Would you add another > > > > parameter type or have a whole new API for this? We need both a "clear all" > > > > and a "clear modified since." > > > > > > This data type isn’t a website data type, so it seems inappropriate to > > > include in the website data API. > > > > > > The notion if a website credential’s modification date is also strange. What > > > is the use case for forgetting only credentials modified since some time? > > > > This gives Safari the ability to include it in clearing UI that is "clear > > for the last hour" to cover tracks. > > So in this hypothetical Safari is going to also remove the corresponding > Keychain items? This proposed API doesn’t seem to support something like > that. > > > HTTP auth could also be used by websites > > to surreptitiously track users (e.g. by putting HTTP auth credentials in the > > URL of a subresource). > > > > Per Arne Vollan - is this going to clear the data that is stored in keychain > > as well? That seems different than what the original goal was here (clear > > authentication that is not stored in keychain, since the user can clear it > > from keychain). Dan and I talked about this further - it isn't necessary for this to have a "clear for the last hour" functionality. I agree with him that this should be about clearing the session based data, and a separate API. Has Geoff looked at the proposal for the new API?
Created attachment 308381 [details] Patch
> Dan and I talked about this further - it isn't necessary for this to have a > "clear for the last hour" functionality. I agree with him that this should > be about clearing the session based data, and a separate API. Sorry, are you saying that you and Dan agree that this API should be separate from WKWebsiteDataStore?
Comment on attachment 308381 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308381&action=review > Source/WebKit2/UIProcess/API/C/WKCredentialManager.h:35 > +typedef void (*WKCredentialManagerRemovedCredentialsResult)(bool, WKErrorRef, void*); Is there any condition under which we will fail to remove and still invoke this callback? I don't think there is, so I think the bool argument doesn't make sense. (And what about the WKErrorRef and void* arguments?)
I have so many opinions.
(In reply to mitz from comment #7) > This data type isn’t a website data type, so it seems inappropriate to > include in the website data API. I agree that persistent credentials are not "website data" I disagree that session credentials (which this API is targeted at) are not "website data" They are, exactly the same way that session cookies are. They do little more than allow for the same tracking vectors as session cookies and they can be explicitly set by websites. (In reply to Jessie Berlin from comment #13) > Dan and I talked about this further - I agree with him that this should > be about clearing the session based data, and a separate API. Yes, this should be about the session based data only. But I think it makes perfect sense to include it in WebsiteDataStore operations. This is website data that the user has as much control over as they do session cookies, and does not conflict with the keychain which is user data. There's also problems with the patch I'll include in review.
Comment on attachment 308381 [details] Patch The patch doesn't message the WebProcesses. While most credential storage operations are in the networking process, sadly there's still a bit left in WebProcesses as well. It also can't be correct because it operates on the global session ID. Note, this came up in earlier comments but was never addressed. The testing is also problematic. The API tests are designed to work offline, yet the added test hits a public URL. Additionally it doesn't attempt to do any validation on credentials actually being sent to the server. The test doesn't actually verify the behavior that is important; That the credentials were cleared. I think: - "Session Credentials" can be a new type on WebsiteDataStore, and that's where it should be exposed in SPI - As long as there's any doubt that WebsiteDataStore is the wrong place, I still think that's fine as the C-API is SPI, and we'll learn whether it was right or not before adding anything to the Cocoa API (but I think it's right) - This needs to go to the WebProcess(es) in addition to Networking - To that effect, the "Web" prefix should be dropped from the CredentialManager objects and the receiver-side should be moved to Shared.
Thanks for reviewing! I will upload a new patch soon.
Created attachment 309279 [details] Patch
Created attachment 310654 [details] Patch
Can we get a review on the latest patch? Three Safari bugs are blocked by progress here. Thank you!
Comment on attachment 310654 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310654&action=review Thinking it was very odd that this patch adds a deleter without a completion handler, I dug in to the history of WKWebsiteDataStoreRef. It was added in r181707 and its sole purpose was to provide a toll-free bridge from WKContextRef to WKWebsiteDataStore (the Cocoa API object) It never should've had any API/SPI added to it. In fact some of the methods there (rightfully) have zero clients. In my 5-3 comment I suggested adding it to the C-SPI was fine, but I misunderstood the history of WebsiteDataStoreRef - It never predated WKWebsiteDataStore and never had any API. The rest of my 5-3 comment applies. You'll need to add this to WKWebsiteDataStore. Look at WKWebsiteDataRecord.h and WKWebsiteDataRecordPrivate.h to see how the types are exposed - They're just strings, and you'll need to add a new string type. Additionally, it's not okay to *only* expose a deleter. WKWebsiteDataStore supports Safari's privacy UI, and that UI needs to accurately reflect when a site has data stored against it. In other words, you'll need to add the getters as well. > Source/WebKit2/UIProcess/API/C/WKCredential.cpp:-55 > { > return toCopiedAPI(toImpl(credentialRef)->credential().user()); > } > - This does not need to be in this patch.
(In reply to Brady Eidson from comment #24) > Comment on attachment 310654 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=310654&action=review > > Thinking it was very odd that this patch adds a deleter without a completion > handler, I dug in to the history of WKWebsiteDataStoreRef. > > It was added in r181707 and its sole purpose was to provide a toll-free > bridge from WKContextRef to WKWebsiteDataStore (the Cocoa API object) It > never should've had any API/SPI added to it. In fact some of the methods > there (rightfully) have zero clients. > > In my 5-3 comment I suggested adding it to the C-SPI was fine, but I > misunderstood the history of WebsiteDataStoreRef - It never predated > WKWebsiteDataStore and never had any API. > > The rest of my 5-3 comment applies. You'll need to add this to > WKWebsiteDataStore. > > Look at WKWebsiteDataRecord.h and WKWebsiteDataRecordPrivate.h to see how > the types are exposed - They're just strings, and you'll need to add a new > string type. > > Additionally, it's not okay to *only* expose a deleter. WKWebsiteDataStore > supports Safari's privacy UI, and that UI needs to accurately reflect when a > site has data stored against it. In other words, you'll need to add the > getters as well. > > > Source/WebKit2/UIProcess/API/C/WKCredential.cpp:-55 > > { > > return toCopiedAPI(toImpl(credentialRef)->credential().user()); > > } > > - > > This does not need to be in this patch. Thanks Brady! I will update the patch.
Created attachment 311987 [details] Patch
Created attachment 311990 [details] Patch
Created attachment 311991 [details] Patch
Created attachment 311992 [details] Patch
Created attachment 311993 [details] Patch
Created attachment 311997 [details] Patch
Comment on attachment 311997 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311997&action=review > Source/WebCore/platform/network/CredentialStorage.h:60 > + HashSet<String> originsWithCredentials() const { return m_originsWithCredentials; } This makes a copy of the set on every call. const HashSet<String>& > Source/WebKit2/UIProcess/API/Cocoa/WKWebsiteDataRecord.h:60 > +/*! @constant WKWebsiteDataTypeCredentials Credentials, such as Basic Authentication. */ > +WK_EXTERN NSString * const WKWebsiteDataTypeCredentials WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA)); > + I don't think we intend to actually go through API review for this... do we? If you're planning to, then fine... If not, this has to be moved to the private header. I do not think it's important to make this API (and, in fact, discussion in this bug has been centered around *not* making it API for now) > Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:208 > +void TestController::removeAllSessionCredentials() > +{ > +#if WK_API_ENABLED > + [globalWebViewConfiguration.websiteDataStore removeDataOfTypes:[WKWebsiteDataStore allWebsiteDataTypes] modifiedSince:[NSDate distantPast] completionHandler:^() { }]; > +#endif > +} I'm very concerned about inherent flakiness in this test. Since the UI process doesn't wait on the completion handler for this call, the iframe that is loaded can definitely end up having the credentials still applied.
Created attachment 312165 [details] Patch
Created attachment 312167 [details] Patch
Created attachment 312168 [details] Patch
Created attachment 312170 [details] Patch
(In reply to Brady Eidson from comment #32) > Comment on attachment 311997 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=311997&action=review > > > Source/WebCore/platform/network/CredentialStorage.h:60 > > + HashSet<String> originsWithCredentials() const { return m_originsWithCredentials; } > > This makes a copy of the set on every call. > > const HashSet<String>& > > > Source/WebKit2/UIProcess/API/Cocoa/WKWebsiteDataRecord.h:60 > > +/*! @constant WKWebsiteDataTypeCredentials Credentials, such as Basic Authentication. */ > > +WK_EXTERN NSString * const WKWebsiteDataTypeCredentials WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA)); > > + > > I don't think we intend to actually go through API review for this... do we? > > If you're planning to, then fine... If not, this has to be moved to the > private header. > > I do not think it's important to make this API (and, in fact, discussion in > this bug has been centered around *not* making it API for now) > > > Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:208 > > +void TestController::removeAllSessionCredentials() > > +{ > > +#if WK_API_ENABLED > > + [globalWebViewConfiguration.websiteDataStore removeDataOfTypes:[WKWebsiteDataStore allWebsiteDataTypes] modifiedSince:[NSDate distantPast] completionHandler:^() { }]; > > +#endif > > +} > > I'm very concerned about inherent flakiness in this test. > > Since the UI process doesn't wait on the completion handler for this call, > the iframe that is loaded can definitely end up having the credentials still > applied. Thanks for reviewing! I have updated the patch.
Comment on attachment 312170 [details] Patch 👍🏻👍🏻👍🏻
(In reply to Brady Eidson from comment #38) > Comment on attachment 312170 [details] > Patch > > 👍🏻👍🏻👍🏻 Thanks, Brady :)
Comment on attachment 312170 [details] Patch Clearing flags on attachment: 312170 Committed r217886: <http://trac.webkit.org/changeset/217886>
All reviewed patches have been landed. Closing bug.