Bug 198318 – Optionally respect device management restrictions when loading from the network

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

Optionally respect device management restrictions when loading from the network

[Tim Horton]

Created attachment 370816 [details]
Patch

[Tim Horton]

<rdar://problem/44263806>

[Tim Horton]

Comment on attachment 370816 [details]
Patch

Not for review yet.

[Alex Christensen]

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.

[Tim Horton]

Created attachment 370880 [details]
Patch

[Tim Horton]

Gotta write some API tests

[EWS Watchlist]

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.

[Tim Horton]

Created attachment 370904 [details]
Patch

[EWS Watchlist]

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.

[Tim Horton]

Created attachment 370977 [details]
Patch

[EWS Watchlist]

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.

[Alex Christensen]

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.

[Tim Horton]

(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.

[Tim Horton]

Created attachment 371010 [details]
Patch

[EWS Watchlist]

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.

[Tim Horton]

Created attachment 371016 [details]
Patch

[EWS Watchlist]

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.

[Alex Christensen]

> 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;

[Tim Horton]

(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...

[Tim Horton]

(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)

[Alex Christensen]

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.

[Tim Horton]

Created attachment 371074 [details]
Patch

[Tim Horton]

Created attachment 371075 [details]
Patch

[EWS Watchlist]

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.

[WebKit Commit Bot]

Comment on attachment 371075 [details]
Patch

Clearing flags on attachment: 371075

Committed r245979: <https://trac.webkit.org/changeset/245979>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.