Bug 227729 – [Cocoa] Expose SPI to opt out of Extensible SSO authentication

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

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>

[Brent Fulgham]

Created attachment 433053 [details]
Patch

[Tim Horton]

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!

[Simon Fraser (smfr)]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 433149 [details]
Patch

[Tim Horton]

Comment on attachment 433149 [details]
Patch

Maybe double-check the API tests bot, since it's orange.

[Brent Fulgham]

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

[Brent Fulgham]

The bots are green, so cq+.

[EWS]

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