Bug 227729

Summary: [Cocoa] Expose SPI to opt out of Extensible SSO authentication
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebKit Misc.Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, ews-watchlist, jiewen_tan, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=227925
Bug Depends on:    
Bug Blocks: 227974, 227925    
Attachments:
Description Flags
Patch
none
Patch none

Description Brent Fulgham 2021-07-06 17:06:12 PDT
Some WebKit clients perform loads without being attached to a Window. Our Extensible SSO feature (AppSSO) assumes that the WebView will eventually be attached to a Window, and needs that Window to provide user context for the load event. If a Window is not present, the load is paused until the WKWebView is attached to a window, which can lead to mysterious load failures.

We need to expose API so that WebKit clients performing background loads that don't provide user interaction can let WebKit know that it should avoid triggering these SSO flows.

<rdar://problem/75647892>
Comment 1 Brent Fulgham 2021-07-07 11:31:26 PDT
Created attachment 433053 [details]
Patch
Comment 2 Tim Horton 2021-07-07 11:53:56 PDT
Comment on attachment 433053 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKPreferences.h:70
> +@property (nonatomic) BOOL extensibleSSOEnabled WK_API_AVAILABLE(macos(12.0), ios(15.0));

Might be good to get this API reviewed before landing to avoid any mishaps.

Also, I think you want an 'is'-prefixed getter.

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/NavigationSOAuthorizationSession.mm:60
> +        AUTHORIZATIONSESSION_RELEASE_LOG("shouldStartInternal: Starting Extensible SSO authentication for a web view that is not attached to a window. Loading will pause until a window is attached. If this is unexpected, set the WKPreference.extensibleSSOEnabled property to NO to disable Extenstible SSO operations.");

"Extenstible" sp.

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/PopUpSOAuthorizationSession.mm:189
>      [m_secretWebView setUIDelegate:m_secretDelegate.get()];

Wow, we make a WKWebView of our own, and copy the configuration of the parent? When do we do this?! How do bundle clients not get confused about the spurious web view?? "Make me a new web view" is usually delegated to the client. And its navigation delegate is custom, and ignores the app's policies? Very odd.

... all that said, not new in this patch!
Comment 3 Simon Fraser (smfr) 2021-07-07 12:34:33 PDT
Comment on attachment 433053 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/WKPreferences.h:70
>> +@property (nonatomic) BOOL extensibleSSOEnabled WK_API_AVAILABLE(macos(12.0), ios(15.0));
> 
> Might be good to get this API reviewed before landing to avoid any mishaps.
> 
> Also, I think you want an 'is'-prefixed getter.

Please expand "SSO" at least in the comment.
Comment 4 Brent Fulgham 2021-07-07 14:46:01 PDT
Comment on attachment 433053 [details]
Patch

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

>>> Source/WebKit/UIProcess/API/Cocoa/WKPreferences.h:70
>>> +@property (nonatomic) BOOL extensibleSSOEnabled WK_API_AVAILABLE(macos(12.0), ios(15.0));
>> 
>> Might be good to get this API reviewed before landing to avoid any mishaps.
>> 
>> Also, I think you want an 'is'-prefixed getter.
> 
> Please expand "SSO" at least in the comment.

Will do (for both).

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/NavigationSOAuthorizationSession.mm:60
>> +        AUTHORIZATIONSESSION_RELEASE_LOG("shouldStartInternal: Starting Extensible SSO authentication for a web view that is not attached to a window. Loading will pause until a window is attached. If this is unexpected, set the WKPreference.extensibleSSOEnabled property to NO to disable Extenstible SSO operations.");
> 
> "Extenstible" sp.

Doh!

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/PopUpSOAuthorizationSession.mm:189
>>      [m_secretWebView setUIDelegate:m_secretDelegate.get()];
> 
> Wow, we make a WKWebView of our own, and copy the configuration of the parent? When do we do this?! How do bundle clients not get confused about the spurious web view?? "Make me a new web view" is usually delegated to the client. And its navigation delegate is custom, and ignores the app's policies? Very odd.
> 
> ... all that said, not new in this patch!

Yeah, I forgot the tortured situation that led to Jiewen needing to do this.
Comment 5 Brent Fulgham 2021-07-08 10:21:03 PDT
After discussing more, we decided this should be API. We instead need to design a delegate method so the client can be part of the decision process of whether the wait for the view to be attached to a window, or fail quickly.

For now, we will just expose the existing property we were using for internal and test purposes as SPI.
Comment 6 Brent Fulgham 2021-07-08 11:41:27 PDT
Created attachment 433149 [details]
Patch
Comment 7 Tim Horton 2021-07-08 13:39:26 PDT
Comment on attachment 433149 [details]
Patch

Maybe double-check the API tests bot, since it's orange.
Comment 8 Brent Fulgham 2021-07-08 14:05:43 PDT
(In reply to Tim Horton from comment #7)
> Comment on attachment 433149 [details]
> Patch
> 
> Maybe double-check the API tests bot, since it's orange.

I won't land until both api bots are green. There are some differences in iOS and macOS that I might not have captured in my new assertion.
Comment 9 Brent Fulgham 2021-07-08 14:49:07 PDT
The bots are green, so cq+.
Comment 10 EWS 2021-07-08 15:14:07 PDT
Committed r279755 (239528@main): <https://commits.webkit.org/239528@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433149 [details].