Bug 198318 - Optionally respect device management restrictions when loading from the network
Summary: Optionally respect device management restrictions when loading from the network
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on: 200897
Blocks:
  Show dependency treegraph
 
Reported: 2019-05-28 17:56 PDT by Tim Horton
Modified: 2019-08-19 15:18 PDT (History)
12 users (show)

See Also:


Attachments
Patch (36.09 KB, patch)
2019-05-28 17:57 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (64.71 KB, patch)
2019-05-29 13:24 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (87.24 KB, patch)
2019-05-29 17:57 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (76.70 KB, patch)
2019-05-30 13:51 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (77.14 KB, patch)
2019-05-30 18:33 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (87.54 KB, patch)
2019-05-30 19:06 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (70.76 KB, patch)
2019-05-31 12:16 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (88.37 KB, patch)
2019-05-31 12:17 PDT, Tim Horton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2019-05-28 17:56:42 PDT
Optionally respect device management restrictions when loading from the network
Comment 1 Tim Horton 2019-05-28 17:57:57 PDT
Created attachment 370816 [details]
Patch
Comment 2 Tim Horton 2019-05-28 17:57:59 PDT
<rdar://problem/44263806>
Comment 3 Tim Horton 2019-05-28 17:58:14 PDT
Comment on attachment 370816 [details]
Patch

Not for review yet.
Comment 4 Alex Christensen 2019-05-29 11:31:23 PDT
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.
Comment 5 Tim Horton 2019-05-29 13:24:56 PDT
Created attachment 370880 [details]
Patch
Comment 6 Tim Horton 2019-05-29 13:26:46 PDT
Gotta write some API tests
Comment 7 Build Bot 2019-05-29 13:27:11 PDT
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.
Comment 8 Tim Horton 2019-05-29 17:57:16 PDT
Created attachment 370904 [details]
Patch
Comment 9 Build Bot 2019-05-29 18:00:00 PDT
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.
Comment 10 Tim Horton 2019-05-30 13:51:16 PDT
Created attachment 370977 [details]
Patch
Comment 11 Build Bot 2019-05-30 13:54:10 PDT
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 12 Alex Christensen 2019-05-30 16:50:51 PDT
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.
Comment 13 Tim Horton 2019-05-30 16:56:52 PDT
(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.
Comment 14 Tim Horton 2019-05-30 18:33:09 PDT
Created attachment 371010 [details]
Patch
Comment 15 Build Bot 2019-05-30 18:34:59 PDT
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.
Comment 16 Tim Horton 2019-05-30 19:06:21 PDT
Created attachment 371016 [details]
Patch
Comment 17 Build Bot 2019-05-30 19:09:05 PDT
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.
Comment 18 Alex Christensen 2019-05-30 19:39:05 PDT
> 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;
Comment 19 Tim Horton 2019-05-30 19:45:59 PDT
(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...
Comment 20 Tim Horton 2019-05-30 19:46:23 PDT
(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 21 Alex Christensen 2019-05-31 11:41:14 PDT
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.
Comment 22 Tim Horton 2019-05-31 12:16:26 PDT
Created attachment 371074 [details]
Patch
Comment 23 Tim Horton 2019-05-31 12:17:02 PDT
Created attachment 371075 [details]
Patch
Comment 24 Build Bot 2019-05-31 12:19:21 PDT
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 25 WebKit Commit Bot 2019-05-31 13:00:13 PDT
Comment on attachment 371075 [details]
Patch

Clearing flags on attachment: 371075

Committed r245979: <https://trac.webkit.org/changeset/245979>
Comment 26 WebKit Commit Bot 2019-05-31 13:00:15 PDT
All reviewed patches have been landed.  Closing bug.