Bug 200150 – NavigationSOAuthorizationSession should check the active URL of the responding page after waking up from waiting

• Status: NEW
• Component: WebKit Misc.

[Jiewen Tan]

NavigationSOAuthorizationSession should check the active URL of the responding page after waking up from waiting.

[Jiewen Tan]

<rdar://problem/53280170>

[Jiewen Tan]

Created attachment 374933 [details]
Patch

[Brent Fulgham]

Comment on attachment 374933 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/NavigationSOAuthorizationSession.mm:70
> +    // When the current main frame URL is different from the before one,

I think it might be simpler to say:

"// When the current main frame URL has changed from the start of authentication, the SOAuthorization is no longer valid."

Even better would be to make a method:

bool NavigationSOAuthorizationSession::mainFrameURLDidChangeDuringAuthentication()
{
    auto* page = this->page();
    return !page || page->pageLoadState().activeURL() != m_waitingPageActiveURL;
}

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/NavigationSOAuthorizationSession.mm:72
> +    if (page->pageLoadState().activeURL() != m_waitingPageActiveURL) {

Then this just becomes:

if (mainFrameURLDidChangeDuringAuthentication()) {
    abort();
...

And you don't need a comment. :-)

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm:-79
> -    ASSERT_NOT_REACHED();

Why is it okay to remove this assertion? Is it because we now expect to abort if the URL changed during authentication?

[Jiewen Tan]

Comment on attachment 374933 [details]
Patch

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

Thanks Brent for r+ this patch.

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/NavigationSOAuthorizationSession.mm:70
>> +    // When the current main frame URL is different from the before one,
> 
> I think it might be simpler to say:
> 
> "// When the current main frame URL has changed from the start of authentication, the SOAuthorization is no longer valid."
> 
> Even better would be to make a method:
> 
> bool NavigationSOAuthorizationSession::mainFrameURLDidChangeDuringAuthentication()
> {
>     auto* page = this->page();
>     return !page || page->pageLoadState().activeURL() != m_waitingPageActiveURL;
> }

Fixed.

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/NavigationSOAuthorizationSession.mm:72
>> +    if (page->pageLoadState().activeURL() != m_waitingPageActiveURL) {
> 
> Then this just becomes:
> 
> if (mainFrameURLDidChangeDuringAuthentication()) {
>     abort();
> ...
> 
> And you don't need a comment. :-)

Named it pageActiveURLDidChangeDuringWaiting.

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm:-79
>> -    ASSERT_NOT_REACHED();
> 
> Why is it okay to remove this assertion? Is it because we now expect to abort if the URL changed during authentication?

Yes.

[Jiewen Tan]

Created attachment 374991 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 374991 [details]
Patch for landing

Clearing flags on attachment: 374991

Committed r247885: <https://trac.webkit.org/changeset/247885>