requestPin ext API: No quotas for first requests

Don't apply the quota (how many times the API can be called by an
extension during 1 minute / 10 minutes) to the
chrome.certificateProvider.requestPin API when it's called for the
first time with the new maximum request ID.

This should, in particular, resolve the UX issues in the Chrome
OS smart card login feature, where the user may quickly trigger
a significant number of PIN requests by clicking on user pods and
declining the PIN requests.

To simplify the implementation, we're relaxing the quota under
the assumption that the requestId are monotonically increasing
(which is guaranteed by Chrome) and that the caller extension
doesn't call reorder them (which is not guaranteed by an
extension, but that's how they are typically expected to work).
Nothing bad, however, would happen if these assumptions will be
violated in some cases - the quota will only become slightly
stricter (as it was before this CL).

Bug: 989641
Test: repeat 20 times an attempt to log into Chrome OS via a smart card, check that the PIN request is shown each time, and abort it

Change-Id: I189d5e97d51f1f4262869c17e532e0571f74f0f5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1845009Reviewed-by: Igor <igorcov@chromium.org>
Commit-Queue: Maksim Ivanov <emaxx@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704197}

chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc

@@ -12,6 +12,8 @@
 
 #include "base/bind.h"
 #include "base/logging.h"
+#include "base/macros.h"
+#include "base/values.h"
 #include "chrome/browser/chromeos/certificate_provider/certificate_provider_service.h"
 #include "chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.h"
 #include "chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h"
@@ -19,6 +21,7 @@
 #include "chrome/common/extensions/api/certificate_provider.h"
 #include "chrome/common/extensions/api/certificate_provider_internal.h"
 #include "chromeos/constants/security_token_pin_types.h"
+#include "extensions/browser/quota_service.h"
 #include "net/cert/x509_certificate.h"
 #include "net/ssl/ssl_private_key.h"
 #include "third_party/blink/public/mojom/devtools/console_message.mojom.h"
@@ -75,6 +78,51 @@ const char kCertificateProviderPreviousDialogActive[] =
     "Previous request not finished";
 const char kCertificateProviderNoUserInput[] = "No user input received";
 
+// The BucketMapper implementation for the requestPin API that avoids using the
+// quota when the current request uses the requestId that is strictly greater
+// than all previous ones.
+class RequestPinExceptFirstQuotaBucketMapper final
+    : public QuotaLimitHeuristic::BucketMapper {
+ public:
+  RequestPinExceptFirstQuotaBucketMapper() = default;
+  ~RequestPinExceptFirstQuotaBucketMapper() override = default;
+
+  void GetBucketsForArgs(const base::ListValue* args,
+                         QuotaLimitHeuristic::BucketList* buckets) override {
+    if (args->GetList().empty())
+      return;
+    const base::Value& details = args->GetList()[0];
+    if (!details.is_dict())
+      return;
+    const base::Value* sign_request_id =
+        details.FindKeyOfType("signRequestId", base::Value::Type::INTEGER);
+    if (!sign_request_id)
+      return;
+    if (sign_request_id->GetInt() > biggest_request_id_) {
+      // Either it's the first request with the newly issued requestId, or it's
+      // an invalid requestId (bigger than the real one). Return a new bucket in
+      // order to apply no quota for the former case; for the latter case the
+      // quota doesn't matter much, except that we're maybe making it stricter
+      // for future requests (which is bearable).
+      biggest_request_id_ = sign_request_id->GetInt();
+      new_request_bucket_ = std::make_unique<QuotaLimitHeuristic::Bucket>();
+      buckets->push_back(new_request_bucket_.get());
+      return;
+    }
+    // Either it's a repeatitive request for the given requestId, or the
+    // extension reordered the requests. Fall back to the default bucket (shared
+    // between all requests) in that case.
+    buckets->push_back(&default_bucket_);
+  }
+
+ private:
+  int biggest_request_id_ = -1;
+  QuotaLimitHeuristic::Bucket default_bucket_;
+  std::unique_ptr<QuotaLimitHeuristic::Bucket> new_request_bucket_;
+
+  DISALLOW_COPY_AND_ASSIGN(RequestPinExceptFirstQuotaBucketMapper);
+};
+
 }  // namespace
 
 const int api::certificate_provider::kMaxClosedDialogsPerMinute = 10;
@@ -257,19 +305,25 @@ bool CertificateProviderRequestPinFunction::ShouldSkipQuotaLimiting() const {
 }
 
 void CertificateProviderRequestPinFunction::GetQuotaLimitHeuristics(
-    extensions::QuotaLimitHeuristics* heuristics) const {
+    QuotaLimitHeuristics* heuristics) const {
+  // Apply a 1-minute and a 10-minute quotas. A special bucket mapper is used in
+  // order to, approximately, skip applying quotas to the first request for each
+  // requestId (such logic cannot be done in ShouldSkipQuotaLimiting(), since
+  // it's not called with the request's parameters). The limitation constants
+  // are decremented below to account the first request.
+
   QuotaLimitHeuristic::Config short_limit_config = {
-      api::certificate_provider::kMaxClosedDialogsPerMinute,
+      api::certificate_provider::kMaxClosedDialogsPerMinute - 1,
       base::TimeDelta::FromMinutes(1)};
   heuristics->push_back(std::make_unique<QuotaService::TimedLimit>(
-      short_limit_config, new QuotaLimitHeuristic::SingletonBucketMapper(),
+      short_limit_config, new RequestPinExceptFirstQuotaBucketMapper,
       "MAX_PIN_DIALOGS_CLOSED_PER_MINUTE"));
 
   QuotaLimitHeuristic::Config long_limit_config = {
-      api::certificate_provider::kMaxClosedDialogsPer10Minutes,
+      api::certificate_provider::kMaxClosedDialogsPer10Minutes - 1,
       base::TimeDelta::FromMinutes(10)};
   heuristics->push_back(std::make_unique<QuotaService::TimedLimit>(
-      long_limit_config, new QuotaLimitHeuristic::SingletonBucketMapper(),
+      long_limit_config, new RequestPinExceptFirstQuotaBucketMapper,
       "MAX_PIN_DIALOGS_CLOSED_PER_10_MINUTES"));
 }
 

chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc

@@ -171,9 +171,9 @@ class CertificateProviderRequestPinTest : public CertificateProviderApiTest {
     CertificateProviderApiTest::TearDownOnMainThread();
   }
 
-  void AddFakeSignRequest() {
+  void AddFakeSignRequest(int sign_request_id) {
     cert_provider_service_->pin_dialog_manager()->AddSignRequestId(
-        extension_->id(), kFakeSignRequestId, {});
+        extension_->id(), sign_request_id, {});
   }
 
   void NavigateTo(const std::string& test_page_file_name) {
@@ -373,7 +373,7 @@ IN_PROC_BROWSER_TEST_F(CertificateProviderApiTest, DISABLED_Basic) {
 
 // User enters the correct PIN.
 IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest, ShowPinDialogAccept) {
-  AddFakeSignRequest();
+  AddFakeSignRequest(kFakeSignRequestId);
   NavigateTo("basic.html");
 
   // Enter the valid PIN.
@@ -386,7 +386,7 @@ IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest, ShowPinDialogAccept) {
 // User closes the dialog kMaxClosedDialogsPerMinute times, and the extension
 // should be blocked from showing it again.
 IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest, ShowPinDialogClose) {
-  AddFakeSignRequest();
+  AddFakeSignRequest(kFakeSignRequestId);
   NavigateTo("basic.html");
 
   for (int i = 0;
@@ -411,7 +411,7 @@ IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest, ShowPinDialogClose) {
 // User enters a wrong PIN first and a correct PIN on the second try.
 IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest,
                        ShowPinDialogWrongPin) {
-  AddFakeSignRequest();
+  AddFakeSignRequest(kFakeSignRequestId);
   NavigateTo("basic.html");
   EnterWrongPinAndWaitForMessage();
 
@@ -429,7 +429,7 @@ IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest,
 // User enters wrong PIN three times.
 IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest,
                        ShowPinDialogWrongPinThreeTimes) {
-  AddFakeSignRequest();
+  AddFakeSignRequest(kFakeSignRequestId);
   NavigateTo("basic.html");
   for (int i = 0; i < kWrongPinAttemptsLimit; i++)
     EnterWrongPinAndWaitForMessage();
@@ -447,7 +447,7 @@ IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest,
 // User closes the dialog while the extension is processing the request.
 IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest,
                        ShowPinDialogCloseWhileProcessing) {
-  AddFakeSignRequest();
+  AddFakeSignRequest(kFakeSignRequestId);
   NavigateTo("operated.html");
 
   EXPECT_TRUE(SendCommandAndWaitForMessage("Request", "request1:begun"));
@@ -473,7 +473,7 @@ IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest,
        i <
        extensions::api::certificate_provider::kMaxClosedDialogsPerMinute + 1;
        i++) {
-    AddFakeSignRequest();
+    AddFakeSignRequest(kFakeSignRequestId);
     EXPECT_TRUE(SendCommandAndWaitForMessage(
         "Request", base::StringPrintf("request%d:begun", i + 1)));
 
@@ -483,7 +483,7 @@ IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest,
     EXPECT_FALSE(GetActivePinDialogView());
   }
 
-  AddFakeSignRequest();
+  AddFakeSignRequest(kFakeSignRequestId);
   EXPECT_TRUE(SendCommandAndWaitForMessage(
       "Request",
       base::StringPrintf(
@@ -496,7 +496,7 @@ IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest,
 
 // Extension erroneously attempts to close the PIN dialog twice.
 IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest, DoubleClose) {
-  AddFakeSignRequest();
+  AddFakeSignRequest(kFakeSignRequestId);
   NavigateTo("operated.html");
 
   EXPECT_TRUE(SendCommand("Request"));
@@ -516,14 +516,14 @@ IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest,
        i <
        extensions::api::certificate_provider::kMaxClosedDialogsPerMinute + 1;
        i++) {
-    AddFakeSignRequest();
+    AddFakeSignRequest(kFakeSignRequestId);
     EXPECT_TRUE(SendCommand("Request"));
     EXPECT_TRUE(SendCommandAndWaitForMessage(
         "Stop", base::StringPrintf("stop%d:success", i + 1)));
     EXPECT_FALSE(GetActivePinDialogView());
   }
 
-  AddFakeSignRequest();
+  AddFakeSignRequest(kFakeSignRequestId);
   EXPECT_TRUE(SendCommandAndWaitForMessage(
       "Request",
       base::StringPrintf(
@@ -538,7 +538,7 @@ IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest,
 // the user provided any input.
 IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest,
                        StopWithErrorBeforeInput) {
-  AddFakeSignRequest();
+  AddFakeSignRequest(kFakeSignRequestId);
   NavigateTo("operated.html");
 
   EXPECT_TRUE(SendCommand("Request"));
@@ -558,7 +558,7 @@ IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest, InvalidRequestId) {
 
 // Extension specifies zero left attempts in the very first PIN request.
 IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest, ZeroAttemptsAtStart) {
-  AddFakeSignRequest();
+  AddFakeSignRequest(kFakeSignRequestId);
   NavigateTo("operated.html");
 
   EXPECT_TRUE(SendCommandAndWaitForMessage("RequestWithZeroAttempts",
@@ -574,7 +574,7 @@ IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest, ZeroAttemptsAtStart) {
 
 // Extension erroneously passes a negative attempts left count.
 IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest, NegativeAttempts) {
-  AddFakeSignRequest();
+  AddFakeSignRequest(kFakeSignRequestId);
   NavigateTo("operated.html");
 
   EXPECT_TRUE(SendCommandAndWaitForMessage(
@@ -584,7 +584,7 @@ IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest, NegativeAttempts) {
 
 // Extension erroneously attempts to close a non-existing dialog.
 IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest, CloseNonExisting) {
-  AddFakeSignRequest();
+  AddFakeSignRequest(kFakeSignRequestId);
   NavigateTo("operated.html");
 
   EXPECT_TRUE(SendCommandAndWaitForMessage(
@@ -594,7 +594,7 @@ IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest, CloseNonExisting) {
 
 // Extension erroneously attempts to stop a non-existing dialog with an error.
 IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest, StopNonExisting) {
-  AddFakeSignRequest();
+  AddFakeSignRequest(kFakeSignRequestId);
   NavigateTo("operated.html");
 
   EXPECT_TRUE(SendCommandAndWaitForMessage(
@@ -606,7 +606,7 @@ IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest, StopNonExisting) {
 // user closed the previously stopped with an error PIN request.
 IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest,
                        UpdateAlreadyStopped) {
-  AddFakeSignRequest();
+  AddFakeSignRequest(kFakeSignRequestId);
   NavigateTo("operated.html");
 
   EXPECT_TRUE(SendCommandAndWaitForMessage("Request", "request1:begun"));
@@ -623,7 +623,7 @@ IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest,
 // Extension starts a new PIN request after it stopped the previous one with an
 // error.
 IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest, StartAfterStop) {
-  AddFakeSignRequest();
+  AddFakeSignRequest(kFakeSignRequestId);
   NavigateTo("operated.html");
 
   EXPECT_TRUE(SendCommandAndWaitForMessage("Request", "request1:begun"));
@@ -638,3 +638,28 @@ IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest, StartAfterStop) {
   EXPECT_TRUE(listener.WaitUntilSatisfied());
   EXPECT_FALSE(GetActivePinDialogView()->textfield_for_testing()->GetEnabled());
 }
+
+// Test that no quota is applied to the first PIN requests for each requestId.
+IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest,
+                       RepeatedCloseWithDifferentIds) {
+  NavigateTo("operated.html");
+
+  for (int i = 0;
+       i <
+       extensions::api::certificate_provider::kMaxClosedDialogsPer10Minutes + 2;
+       i++) {
+    AddFakeSignRequest(kFakeSignRequestId + i);
+    EXPECT_TRUE(SendCommandAndWaitForMessage(
+        "Request", base::StringPrintf("request%d:begun", i + 1)));
+
+    ExtensionTestMessageListener listener(
+        base::StringPrintf("request%d:empty", i + 1), false);
+    ASSERT_TRUE(GetActivePinDialogView());
+    GetActivePinDialogView()->GetWidget()->CloseWithReason(
+        views::Widget::ClosedReason::kCloseButtonClicked);
+    EXPECT_TRUE(listener.WaitUntilSatisfied());
+    EXPECT_FALSE(GetActivePinDialogView());
+
+    EXPECT_TRUE(SendCommand("IncrementRequestId"));
+  }
+}

chrome/test/data/extensions/api_test/certificate_provider/request_pin/operated.js

@@ -5,8 +5,9 @@
 // The script runs commands against the PIN API that it receives from the C++
 // side.
 
-const SIGN_REQUEST_ID = 123;
+const INITIAL_SIGN_REQUEST_ID = 123;
 
+let signRequestId = INITIAL_SIGN_REQUEST_ID;
 let pinRequestCount = 0;
 let pinRequestStopCount = 0;
 
@@ -59,20 +60,23 @@ function reportStopPinRequestEnded(pinRequestStopId) {
 function processTestCommand(command) {
   switch (command) {
     case 'Request':
-      requestPin({signRequestId: SIGN_REQUEST_ID});
+      requestPin({signRequestId: signRequestId});
       break;
     case 'RequestWithZeroAttempts':
-      requestPin({signRequestId: SIGN_REQUEST_ID, attemptsLeft: 0});
+      requestPin({signRequestId: signRequestId, attemptsLeft: 0});
       break;
     case 'RequestWithNegativeAttempts':
-      requestPin({signRequestId: SIGN_REQUEST_ID, attemptsLeft: -1});
+      requestPin({signRequestId: signRequestId, attemptsLeft: -1});
       break;
     case 'Stop':
-      stopPinRequest({signRequestId: SIGN_REQUEST_ID});
+      stopPinRequest({signRequestId: signRequestId});
       break;
     case 'StopWithUnknownError':
       stopPinRequest(
-          {signRequestId: SIGN_REQUEST_ID, errorType: 'UNKNOWN_ERROR'});
+          {signRequestId: signRequestId, errorType: 'UNKNOWN_ERROR'});
+      break;
+    case 'IncrementRequestId':
+      ++signRequestId;
       break;
     default:
       chrome.test.fail();