Optionally respect device management restrictions when loading from the network
Created attachment 370816 [details] Patch
<rdar://problem/44263806>
Comment on attachment 370816 [details] Patch Not for review yet.
Comment on attachment 370816 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370816&action=review > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:501 > + [cocoaSession.deviceManagementPolicyMonitor() requestPoliciesForWebsites:@[ urlToCheck.get() ] completionHandler:[protectedThis = makeRefPtr(*this), urlToCheck] (NSDictionary<NSURL *, NSNumber *> *policies, NSError *error) { makeBlockPtr([protectedThis = makeRef(*this), ... > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:64 > +SOFT_LINK_PRIVATE_FRAMEWORK(DeviceManagement); Could you make a soft link header and cpp? I think we're encouraging that these days.
Created attachment 370880 [details] Patch
Gotta write some API tests
Attachment 370880 [details] did not pass style-queue: ERROR: Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:500: Extra space in capture list. [whitespace/brackets] [4] Total errors found: 1 in 38 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 370904 [details] Patch
Attachment 370904 [details] did not pass style-queue: ERROR: Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:500: Extra space in capture list. [whitespace/brackets] [4] Total errors found: 1 in 47 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 370977 [details] Patch
Attachment 370977 [details] did not pass style-queue: ERROR: Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:500: Extra space in capture list. [whitespace/brackets] [4] Total errors found: 1 in 43 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 370977 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370977&action=review There are no tests that synthesize device management blocking and make sure an error is reported correctly. Please write such a test. > Source/WebKit/NetworkProcess/NetworkDataTask.h:135 > enum FailureType { Ideally this would be an enum class : uint8_t. Maybe not in this patch, though. > Source/WebKit/UIProcess/API/Cocoa/_WKWebsiteDataStoreConfiguration.h:36 > +- (instancetype)initWithNonPersistentConfiguration WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA)); "With" is a little misleading. You're not initializing with anything. > Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:459 > + [[[webView configuration] processPool] _syncNetworkProcessCookies]; Should we try setting a cookie then look at the cookie file, instead of looking at the ResourceLoadStatistics file? > Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:470 > + EXPECT_TRUE([[NSFileManager defaultManager] fileExistsAtPath:defaultResourceLoadStatisticsPath.path]); Should we remove this when we're done with the test to not leave state behind? You remove it at the beginning.
(In reply to Alex Christensen from comment #12) > Comment on attachment 370977 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=370977&action=review > > There are no tests that synthesize device management blocking and make sure > an error is reported correctly. Please write such a test. There is no such test because the framework is nonfunctional in the simulator and thus the #define is not defined there, so none of the code is built in the only place we run tests. I don't know what to do about this. > > Source/WebKit/NetworkProcess/NetworkDataTask.h:135 > > enum FailureType { > > Ideally this would be an enum class : uint8_t. Maybe not in this patch, > though. > > > Source/WebKit/UIProcess/API/Cocoa/_WKWebsiteDataStoreConfiguration.h:36 > > +- (instancetype)initWithNonPersistentConfiguration WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA)); > > "With" is a little misleading. You're not initializing with anything. I agree. Do you have thoughts about a better name? > > Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:459 > > + [[[webView configuration] processPool] _syncNetworkProcessCookies]; > > Should we try setting a cookie then look at the cookie file, instead of > looking at the ResourceLoadStatistics file? Maybe? I just borrowed an existing test. > > Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:470 > > + EXPECT_TRUE([[NSFileManager defaultManager] fileExistsAtPath:defaultResourceLoadStatisticsPath.path]); > > Should we remove this when we're done with the test to not leave state > behind? You remove it at the beginning. Maybe? I just borrowed an existing test.
Created attachment 371010 [details] Patch
Attachment 371010 [details] did not pass style-queue: ERROR: Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:500: Extra space in capture list. [whitespace/brackets] [4] Total errors found: 1 in 43 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 371016 [details] Patch
Attachment 371016 [details] did not pass style-queue: ERROR: Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:514: Extra space in capture list. [whitespace/brackets] [4] Total errors found: 1 in 45 files If any of these errors are false positives, please file a bug against check-webkit-style.
> There is no such test because the framework is nonfunctional in the > simulator and thus the #define is not defined there, so none of the code is > built in the only place we run tests. Make a boolean configuration to enable pretending everything is blocked. > I agree. Do you have thoughts about a better name? + (_WKWebsiteDataStoreConfiguration *)nonPersistentConfiguration;
(In reply to Alex Christensen from comment #18) > > There is no such test because the framework is nonfunctional in the > > simulator and thus the #define is not defined there, so none of the code is > > built in the only place we run tests. > > Make a boolean configuration to enable pretending everything is blocked. The current version of the patch does this, yes. > > I agree. Do you have thoughts about a better name? > > + (_WKWebsiteDataStoreConfiguration *)nonPersistentConfiguration; I changed it to -initNonPersistentConfiguration in the last patch or two, but we could do a class method instead...
(In reply to Tim Horton from comment #19) > (In reply to Alex Christensen from comment #18) > > > There is no such test because the framework is nonfunctional in the > > > simulator and thus the #define is not defined there, so none of the code is > > > built in the only place we run tests. > > > > Make a boolean configuration to enable pretending everything is blocked. > > The current version of the patch does this, yes. (this also involved making a lot less of the code conditionally enabled, but that is fine)
Comment on attachment 371016 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371016&action=review > Tools/TestWebKitAPI/Tests/WebKitCocoa/DeviceManagementRestrictions.mm:54 > + EXPECT_EQ(kWKErrorCodeFrameLoadBlockedByRestrictions, error.code); This should be a value from WKErrorPrivate.h instead of from the C API. Also check the domain.
Created attachment 371074 [details] Patch
Created attachment 371075 [details] Patch
Attachment 371075 [details] did not pass style-queue: ERROR: Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:514: Extra space in capture list. [whitespace/brackets] [4] Total errors found: 1 in 46 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 371075 [details] Patch Clearing flags on attachment: 371075 Committed r245979: <https://trac.webkit.org/changeset/245979>
All reviewed patches have been landed. Closing bug.