⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 291737 in webkit


Ignore:
Timestamp:
Mar 22, 2022, 11:47:41 PM (5 years ago)
Author:
Ben Nham
Message:

Only show notification permission prompt on transient activation
​https://bugs.webkit.org/show_bug.cgi?id=238188

Reviewed by Youenn Fablet.

Source/WebCore:

In r291427, we changed Notification.requestPermission and PushManager.subscribe to only show
a permission prompt when processing a user gesture. This ended up being too restrictive and
causes compatibility problems with some large sites.

Instead, match Chrome and Firefox by allowing these prompts after a transient activiation,
i.e. a user gesture within the past second.

Per Maciej's suggestion, we also consume the activation to help combat prompt spam.

Covered by new and existing layout tests.

  • Modules/notifications/Notification.cpp:

(WebCore::Notification::requestPermission):

  • Modules/push-api/PushManager.cpp:

(WebCore::PushManager::subscribe):

Tools:

Add an internal API denyWebNotificationPermissionOnPrompt to WebKitTestRunner that allows a
notification permission prompt to first be displayed and then rejected. This differs from
the existing denyNotificationPermission call, which denied notification permissions before
prompting.

  • WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:
  • WebKitTestRunner/InjectedBundle/TestRunner.cpp:

(WTR::TestRunner::denyWebNotificationPermission):
(WTR::TestRunner::denyWebNotificationPermissionOnPrompt):

  • WebKitTestRunner/InjectedBundle/TestRunner.h:
  • WebKitTestRunner/TestController.cpp:

(WTR::originUserVisibleName):
(WTR::TestController::denyNotificationPermissionOnPrompt):
(WTR::TestController::resetStateToConsistentValues):
(WTR::TestController::decidePolicyForNotificationPermissionRequest):

  • WebKitTestRunner/TestController.h:
  • WebKitTestRunner/TestInvocation.cpp:

(WTR::TestInvocation::didReceiveSynchronousMessageFromInjectedBundle):

LayoutTests:

Add test cases to make sure that showing a permission prompt consumes a user gesture.

  • http/tests/notifications/notification-request-permission-no-callback.html:
  • http/tests/notifications/notification-request-permission.html:
  • http/tests/notifications/request-consumes-activation-expected.txt: Added.
  • http/tests/notifications/request-consumes-activation.html: Added.
  • http/tests/push-api/resources/subscribe-tests.js:

(async testDocumentSubscribeWithUserGesture):
(async testDocumentSubscribeWithoutUserGesture):
(async testDocumentSubscribeImpl):

  • http/tests/push-api/subscribe-deny-permissions-expected.txt:
  • http/tests/push-api/subscribe-deny-permissions-on-prompt-expected.txt:
  • http/tests/push-api/subscribe-deny-permissions-on-prompt.html: Added.
  • http/tests/push-api/subscribe-deny-permissions.html:
  • platform/gtk/TestExpectations:
  • platform/mac-wk1/TestExpectations:
  • platform/win/TestExpectations:
Location:
trunk
Files:
3 added
19 edited
1 copied

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r291727 r291737  
     12022-03-22  Ben Nham  <nham@apple.com>
     2
     3        Only show notification permission prompt on transient activation
     4        https://bugs.webkit.org/show_bug.cgi?id=238188
     5
     6        Reviewed by Youenn Fablet.
     7
     8        Add test cases to make sure that showing a permission prompt consumes a user gesture.
     9
     10        * http/tests/notifications/notification-request-permission-no-callback.html:
     11        * http/tests/notifications/notification-request-permission.html:
     12        * http/tests/notifications/request-consumes-activation-expected.txt: Added.
     13        * http/tests/notifications/request-consumes-activation.html: Added.
     14        * http/tests/push-api/resources/subscribe-tests.js:
     15        (async testDocumentSubscribeWithUserGesture):
     16        (async testDocumentSubscribeWithoutUserGesture):
     17        (async testDocumentSubscribeImpl):
     18        * http/tests/push-api/subscribe-deny-permissions-expected.txt:
     19        * http/tests/push-api/subscribe-deny-permissions-on-prompt-expected.txt:
     20        * http/tests/push-api/subscribe-deny-permissions-on-prompt.html: Added.
     21        * http/tests/push-api/subscribe-deny-permissions.html:
     22        * platform/gtk/TestExpectations:
     23        * platform/mac-wk1/TestExpectations:
     24        * platform/win/TestExpectations:
     25
    1262022-03-22  Tyler Wilcock  <tyler_w@apple.com>
    227

  • trunk/LayoutTests/http/tests/notifications/notification-request-permission-no-callback.html

    r291414 r291737  
    1616    window.Notification.requestPermission();
    1717    testPassed("Notification.requestPermission does not crash.");
     18});
    1819
     20internals.withUserGesture(() => {
    1921    testRunner.grantWebNotificationPermission(testURL);
    2022    window.Notification.requestPermission();
    2123    testPassed("Notification.requestPermission does not crash.");
     24});
    2225
     26internals.withUserGesture(() => {
    2327    testRunner.denyWebNotificationPermission(testURL);
    2428    window.Notification.requestPermission();

  • trunk/LayoutTests/http/tests/notifications/notification-request-permission.html

    r291414 r291737  
    1616    window.Notification.requestPermission(function() { });
    1717    testPassed("Notification.requestPermission does not crash.");
     18});
    1819
     20internals.withUserGesture(() => {
    1921    testRunner.grantWebNotificationPermission(testURL);
    2022    window.Notification.requestPermission(function() { });
    2123    testPassed("Notification.requestPermission does not crash.");
     24});
    2225
     26internals.withUserGesture(() => {
    2327    testRunner.denyWebNotificationPermission(testURL);
    2428    window.Notification.requestPermission(function() { });

  • trunk/LayoutTests/http/tests/push-api/resources/subscribe-tests.js

    r291414 r291737  
    3939}
    4040
    41 async function testDocumentSubscribeWithUserGesture(registration, domExceptionName)
     41async function testDocumentSubscribeWithUserGesture(registration, domExceptionName, domMessage)
    4242{
    43     await testDocumentSubscribeImpl(registration, domExceptionName, true);
     43    await testDocumentSubscribeImpl(registration, domExceptionName, domMessage, true);
    4444}
    4545
    46 async function testDocumentSubscribeWithoutUserGesture(registration, domExceptionName)
     46async function testDocumentSubscribeWithoutUserGesture(registration, domExceptionName, domMessage)
    4747{
    48     await testDocumentSubscribeImpl(registration, domExceptionName, false);
     48    await testDocumentSubscribeImpl(registration, domExceptionName, domMessage, false);
    4949}
    5050
    51 async function testDocumentSubscribeImpl(registration, domExceptionName, withUserGesture)
     51async function testDocumentSubscribeImpl(registration, domExceptionName, domMessage, withUserGesture)
    5252{
    53     let expected = domExceptionName ? `error: ${domExceptionName}` : "successful";
     53    let expected = "successful";
     54    if (domMessage)
     55        expected = `error: ${domExceptionName}: ${domMessage}`
     56    else if (domExceptionName)
     57        expected = `error: ${domExceptionName}`
     58
    5459    let result = null;
    5560
    … …  
    8085        if (e.name == 'AbortError')
    8186            result = 'successful';
     87        else if (domMessage)
     88            result = `error: ${e?.name}: ${e?.message}`
    8289        else
    83             result = 'error: ' + (e ? e.name : null);
     90            result = `error: ${e?.name}`;
    8491    }
    8592

  • trunk/LayoutTests/http/tests/push-api/subscribe-deny-permissions-expected.txt

    r291414 r291737  
    22PASS: document permissionState was denied
    33PASS: service worker subscribe was error: NotAllowedError
    4 PASS: document subscribe without user gesture was error: NotAllowedError
     4PASS: document subscribe without user gesture was error: NotAllowedError: User denied push permission
    55PASS: document subscribe with user gesture was error: NotAllowedError
    66

  • trunk/LayoutTests/http/tests/push-api/subscribe-deny-permissions-on-prompt-expected.txt

    r291736 r291737  
    1 PASS: service worker permissionState was denied
    2 PASS: document permissionState was denied
     1PASS: service worker permissionState was prompt
     2PASS: document permissionState was prompt
    33PASS: service worker subscribe was error: NotAllowedError
    44PASS: document subscribe without user gesture was error: NotAllowedError
    55PASS: document subscribe with user gesture was error: NotAllowedError
     6PASS: document subscribe with consumed user gesture failed with user gesture error
    67

  • trunk/LayoutTests/http/tests/push-api/subscribe-deny-permissions.html

    r291414 r291737  
    1515        await testDocumentPermissionState(registration, 'denied');
    1616        await testServiceWorkerSubscribe(registration, 'NotAllowedError');
    17         await testDocumentSubscribeWithoutUserGesture(registration, 'NotAllowedError');
     17        await testDocumentSubscribeWithoutUserGesture(registration, 'NotAllowedError', 'User denied push permission');
    1818        await testDocumentSubscribeWithUserGesture(registration, 'NotAllowedError');
    1919    } catch (e) {

  • trunk/LayoutTests/platform/gtk/TestExpectations

    r291471 r291737  
    18741874http/tests/push-api/subscribe-default-permissions-iframe-same-origin.html [ Failure ]
    18751875http/tests/push-api/subscribe-default-permissions.html [ Failure ]
     1876http/tests/push-api/subscribe-deny-permissions-on-prompt.html [ Failure ]
    18761877http/tests/push-api/subscribe-deny-permissions.html [ Failure ]
    18771878http/tests/push-api/subscribe-grant-permissions.html [ Failure ]

  • trunk/LayoutTests/platform/mac-wk1/TestExpectations

    r291724 r291737  
    17891789http/tests/push-api/subscribe-default-permissions-iframe-same-origin.html [ Skip ]
    17901790http/tests/push-api/subscribe-default-permissions.html [ Skip ]
     1791http/tests/push-api/subscribe-deny-permissions-on-prompt.html [ Failure ]
    17911792http/tests/push-api/subscribe-deny-permissions.html [ Skip ]
    17921793http/tests/push-api/subscribe-grant-permissions.html [ Skip ]

  • trunk/LayoutTests/platform/win/TestExpectations

    r291600 r291737  
    50165016http/tests/push-api/subscribe-default-permissions-iframe-same-origin.html [ Failure ]
    50175017http/tests/push-api/subscribe-default-permissions.html [ Failure ]
     5018http/tests/push-api/subscribe-deny-permissions-on-prompt.html [ Failure ]
    50185019http/tests/push-api/subscribe-deny-permissions.html [ Failure ]
    50195020http/tests/push-api/subscribe-grant-permissions.html [ Failure ]

  • trunk/Source/WebCore/ChangeLog

    r291735 r291737  
     12022-03-22  Ben Nham  <nham@apple.com>
     2
     3        Only show notification permission prompt on transient activation
     4        https://bugs.webkit.org/show_bug.cgi?id=238188
     5
     6        Reviewed by Youenn Fablet.
     7
     8        In r291427, we changed Notification.requestPermission and PushManager.subscribe to only show
     9        a permission prompt when processing a user gesture. This ended up being too restrictive and
     10        causes compatibility problems with some large sites.
     11
     12        Instead, match Chrome and Firefox by allowing these prompts after a transient activiation,
     13        i.e. a user gesture within the past second.
     14
     15        Per Maciej's suggestion, we also consume the activation to help combat prompt spam.
     16
     17        Covered by new and existing layout tests.
     18
     19        * Modules/notifications/Notification.cpp:
     20        (WebCore::Notification::requestPermission):
     21        * Modules/push-api/PushManager.cpp:
     22        (WebCore::PushManager::subscribe):
     23
    1242022-03-22  Alex Christensen  <achristensen@webkit.org>
    225

  • trunk/Source/WebCore/Modules/notifications/Notification.cpp

    r291414 r291737  
    3636#include "Notification.h"
    3737
     38#include "DOMWindow.h"
    3839#include "Event.h"
    3940#include "EventNames.h"
    … …  
    4445#include "NotificationPermissionCallback.h"
    4546#include "ServiceWorkerGlobalScope.h"
    46 #include "UserGestureIndicator.h"
    4747#include "WindowEventLoop.h"
    4848#include "WindowFocusAllowedIndicator.h"
    … …  
    298298    }
    299299
    300     if (!UserGestureIndicator::processingUserGesture()) {
     300    auto* window = document.frame() ? document.frame()->window() : nullptr;
     301    if (!window || !window->consumeTransientActivation()) {
    301302        document.addConsoleMessage(MessageSource::Security, MessageLevel::Error, "Notification prompting can only be done from a user gesture."_s);
    302303        return resolvePromiseAndCallback(Permission::Denied);

  • trunk/Source/WebCore/Modules/push-api/PushManager.cpp

    r291414 r291737  
    2929#if ENABLE(SERVICE_WORKER)
    3030
     31#include "DOMWindow.h"
    3132#include "DocumentInlines.h"
    3233#include "EventLoop.h"
    … …  
    3839#include "ScriptExecutionContext.h"
    3940#include "ServiceWorkerRegistration.h"
    40 #include "UserGestureIndicator.h"
    4141#include <wtf/IsoMallocInlines.h>
    4242#include <wtf/Vector.h>
    … …  
    7373    RELEASE_ASSERT(context.isSecureContext());
    7474
    75     context.eventLoop().queueTask(TaskSource::Networking, [this, protectedThis = Ref { *this }, context = Ref { context }, options = WTFMove(options), promise = WTFMove(promise), processingUserGesture = UserGestureIndicator::processingUserGesture()]() mutable {
     75    context.eventLoop().queueTask(TaskSource::Networking, [this, protectedThis = Ref { *this }, context = Ref { context }, options = WTFMove(options), promise = WTFMove(promise)]() mutable {
    7676        if (!options || !options->userVisibleOnly) {
    7777            promise.reject(Exception { NotAllowedError, "Subscribing for push requires userVisibleOnly to be true"_s });
    … …  
    132132            RELEASE_ASSERT(context->isDocument());
    133133
    134             if (!downcast<Document>(context.get()).isSameOriginAsTopDocument()) {
     134            auto& document = downcast<Document>(context.get());
     135            if (!document.isSameOriginAsTopDocument()) {
    135136                promise.reject(Exception { NotAllowedError, "Cannot request permission from cross-origin iframe"_s });
    136137                return;
    137138            }
    138139
    139             if (!processingUserGesture) {
     140            auto* window = document.frame() ? document.frame()->window() : nullptr;
     141            if (!window || !window->consumeTransientActivation()) {
    140142                promise.reject(Exception { NotAllowedError, "Push notification prompting can only be done from a user gesture"_s });
    141143                return;

  • trunk/Tools/ChangeLog

    r291736 r291737  
     12022-03-22  Ben Nham  <nham@apple.com>
     2
     3        Only show notification permission prompt on transient activation
     4        https://bugs.webkit.org/show_bug.cgi?id=238188
     5
     6        Reviewed by Youenn Fablet.
     7
     8        Add an internal API denyWebNotificationPermissionOnPrompt to WebKitTestRunner that allows a
     9        notification permission prompt to first be displayed and then rejected. This differs from
     10        the existing denyNotificationPermission call, which denied notification permissions before
     11        prompting.
     12
     13        * WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:
     14        * WebKitTestRunner/InjectedBundle/TestRunner.cpp:
     15        (WTR::TestRunner::denyWebNotificationPermission):
     16        (WTR::TestRunner::denyWebNotificationPermissionOnPrompt):
     17        * WebKitTestRunner/InjectedBundle/TestRunner.h:
     18        * WebKitTestRunner/TestController.cpp:
     19        (WTR::originUserVisibleName):
     20        (WTR::TestController::denyNotificationPermissionOnPrompt):
     21        (WTR::TestController::resetStateToConsistentValues):
     22        (WTR::TestController::decidePolicyForNotificationPermissionRequest):
     23        * WebKitTestRunner/TestController.h:
     24        * WebKitTestRunner/TestInvocation.cpp:
     25        (WTR::TestInvocation::didReceiveSynchronousMessageFromInjectedBundle):
     26
    1272022-03-22  Yusuke Suzuki  <ysuzuki@apple.com>
    228

  • trunk/Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl

    r290985 r291737  
    207207    undefined grantWebNotificationPermission(DOMString origin);
    208208    undefined denyWebNotificationPermission(DOMString origin);
     209    undefined denyWebNotificationPermissionOnPrompt(DOMString origin);
    209210    undefined removeAllWebNotificationPermissions();
    210211    undefined simulateWebNotificationClick(object notification);

  • trunk/Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp

    r290985 r291737  
    877877}
    878878
     879void TestRunner::denyWebNotificationPermissionOnPrompt(JSStringRef origin)
     880{
     881    postSynchronousPageMessageWithReturnValue("DenyNotificationPermissionOnPrompt", toWK(origin));
     882}
     883
    879884void TestRunner::removeAllWebNotificationPermissions()
    880885{

  • trunk/Tools/WebKitTestRunner/InjectedBundle/TestRunner.h

    r290985 r291737  
    293293    static void grantWebNotificationPermission(JSStringRef origin);
    294294    static void denyWebNotificationPermission(JSStringRef origin);
     295    static void denyWebNotificationPermissionOnPrompt(JSStringRef origin);
    295296    static void removeAllWebNotificationPermissions();
    296297    static void simulateWebNotificationClick(JSValueRef notification);

  • trunk/Tools/WebKitTestRunner/TestController.cpp

    r290985 r291737  
    720720}
    721721
     722static String originUserVisibleName(WKSecurityOriginRef origin)
     723{
     724    if (!origin)
     725        return emptyString();
     726
     727    auto host = toWTFString(adoptWK(WKSecurityOriginCopyHost(origin)));
     728    auto protocol = toWTFString(adoptWK(WKSecurityOriginCopyProtocol(origin)));
     729
     730    if (host.isEmpty() || protocol.isEmpty())
     731        return emptyString();
     732
     733    if (int port = WKSecurityOriginGetPort(origin))
     734        return makeString(protocol, "://", host, ':', port);
     735
     736    return makeString(protocol, "://", host);
     737}
     738
    722739bool TestController::grantNotificationPermission(WKStringRef originString)
    723740{
    … …  
    735752    auto origin = adoptWK(WKSecurityOriginCreateFromString(originString));
    736753    WKNotificationManagerProviderDidUpdateNotificationPolicy(WKNotificationManagerGetSharedServiceWorkerNotificationManager(), origin.get(), false);
     754    return true;
     755}
     756
     757bool TestController::denyNotificationPermissionOnPrompt(WKStringRef originString)
     758{
     759    auto origin = adoptWK(WKSecurityOriginCreateFromString(originString));
     760    auto originName = originUserVisibleName(origin.get());
     761    m_notificationOriginsToDenyOnPrompt.add(originName);
    737762    return true;
    738763}
    … …  
    10471072    // Reset notification permissions
    10481073    m_webNotificationProvider.reset();
     1074    m_notificationOriginsToDenyOnPrompt.clear();
    10491075
    10501076    // Reset Geolocation permissions.
    … …  
    24132439}
    24142440
    2415 static String originUserVisibleName(WKSecurityOriginRef origin)
    2416 {
    2417     if (!origin)
    2418         return emptyString();
    2419 
    2420     auto host = toWTFString(adoptWK(WKSecurityOriginCopyHost(origin)));
    2421     auto protocol = toWTFString(adoptWK(WKSecurityOriginCopyProtocol(origin)));
    2422 
    2423     if (host.isEmpty() || protocol.isEmpty())
    2424         return emptyString();
    2425 
    2426     if (int port = WKSecurityOriginGetPort(origin))
    2427         return makeString(protocol, "://", host, ':', port);
    2428 
    2429     return makeString(protocol, "://", host);
    2430 }
    2431 
    24322441static String userMediaOriginHash(WKSecurityOriginRef userMediaDocumentOrigin, WKSecurityOriginRef topLevelDocumentOrigin)
    24332442{
    … …  
    26522661}
    26532662
    2654 void TestController::decidePolicyForNotificationPermissionRequest(WKPageRef, WKSecurityOriginRef, WKNotificationPermissionRequestRef request)
    2655 {
     2663void TestController::decidePolicyForNotificationPermissionRequest(WKPageRef, WKSecurityOriginRef origin, WKNotificationPermissionRequestRef request)
     2664{
     2665    auto originName = originUserVisibleName(origin);
     2666    if (m_notificationOriginsToDenyOnPrompt.contains(originName)) {
     2667        WKNotificationPermissionRequestDeny(request);
     2668        return;
     2669    }
     2670
    26562671    WKNotificationPermissionRequestAllow(request);
    26572672}

  • trunk/Tools/WebKitTestRunner/TestController.h

    r290985 r291737  
    377377    bool grantNotificationPermission(WKStringRef origin);
    378378    bool denyNotificationPermission(WKStringRef origin);
     379    bool denyNotificationPermissionOnPrompt(WKStringRef origin);
    379380
    380381private:
    … …  
    576577
    577578    WebNotificationProvider m_webNotificationProvider;
     579    HashSet<String> m_notificationOriginsToDenyOnPrompt;
    578580
    579581    std::unique_ptr<PlatformWebView> m_mainWebView;

  • trunk/Tools/WebKitTestRunner/TestInvocation.cpp

    r290985 r291737  
    10401040        return adoptWK(WKBooleanCreate(TestController::singleton().denyNotificationPermission(stringValue(messageBody))));
    10411041
     1042    if (WKStringIsEqualToUTF8CString(messageName, "DenyNotificationPermissionOnPrompt"))
     1043        return adoptWK(WKBooleanCreate(TestController::singleton().denyNotificationPermissionOnPrompt(stringValue(messageBody))));
     1044
    10421045    if (WKStringIsEqualToUTF8CString(messageName, "IsDoingMediaCapture"))
    10431046        return adoptWK(WKBooleanCreate(TestController::singleton().isDoingMediaCapture()));
Note: See TracChangeset for help on using the changeset viewer.