Settings UI: clean up security fingerprint enrollment response handling

The onEnrolling_() method and its EnrollmentStatus argument type were
conflating two logically separate events: fingerprint sample status
updates ("sensor was touched") and handling the final enrollment
response. Splitting the type in two (SampleResponse / EnrollmentResponse),
and handle them in separate methods simplifies the code.

Also improve error handling in both cases:
- failure status codes in a SampleResponse cause a "try again"
  message in the enrollment sheet to be shown
- CTAP2 errors on the EnrollmentResponse make the dialog proceed to the
  error sheet

Bug: 974046
Change-Id: Ib8492aae493374f84b368bb6eca6b9eb439af685
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1999268
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#732650}

chrome/app/settings_strings.grdp

@@ -5586,6 +5586,12 @@
   <message name="IDS_SETTINGS_SECURITY_KEYS_BIO_ENROLLMENT_ENROLLING_LABEL" desc="A label instructing the user to repeatedly touch the fingerprint sensor on their security key (authentication hardware device) to take samples for a new fingerprint.">
     Keep touching your security key until your fingerprint is captured
   </message>
+  <message name="IDS_SETTINGS_SECURITY_KEYS_BIO_ENROLLMENT_TRY_AGAIN_LABEL" desc="A label instructing the user to touch the fingerprint sensor on their security key (authentication hardware device) again after taking a fingerprint sample failed.">
+    Try touching your security key again
+  </message>
+  <message name="IDS_SETTINGS_SECURITY_KEYS_BIO_ENROLLMENT_FAILED_LABEL" desc="An error shown after enrolling a new fingerprint to a security key (authentication hardware device) was unsuccessful.">
+    Adding a fingerprint to this security key failed
+  </message>
   <message name="IDS_SETTINGS_SECURITY_KEYS_BIO_ENROLLMENT_ENROLLING_COMPLETE_LABEL" desc="A label informing the user that adding a fingerprint to their security key succeeded.">
     Your fingerprint was captured
   </message>

chrome/browser/resources/settings/privacy_page/security_keys_bio_enroll_dialog.js

@@ -88,7 +88,8 @@ cr.define('settings', function() {
       this.addWebUIListener(
           'security-keys-bio-enroll-error', this.onError_.bind(this));
       this.addWebUIListener(
-          'security-keys-bio-enroll-status', this.onEnrolling_.bind(this));
+          'security-keys-bio-enroll-status',
+          this.onEnrollmentSample_.bind(this));
       this.browserProxy_ =
           settings.SecurityKeysBioEnrollProxyImpl.getInstance();
       this.browserProxy_.startBioEnroll().then(() => {
@@ -195,50 +196,68 @@ cr.define('settings', function() {
       this.dialogPage_ = BioEnrollDialogPage.ENROLL;
 
       this.browserProxy_.startEnrolling().then(response => {
-        this.onEnrolling_(response);
+        this.onEnrollmentComplete_(response);
       });
     },
 
     /**
      * @private
-     * @param {!settings.EnrollmentStatus} response
+     * @param {!settings.SampleResponse} response
      */
-    onEnrolling_(response) {
-      if (response.code == settings.Ctap2Status.ERR_KEEPALIVE_CANCEL) {
-        this.showEnrollmentsPage_();
+    onEnrollmentSample_(response) {
+      if (response.status != settings.SampleStatus.OK) {
+        this.progressArcLabel_ =
+            this.i18n('securityKeysBioEnrollmentTryAgainLabel');
+        this.fire('iron-announce', {text: this.progressArcLabel_});
         return;
       }
 
-      if (this.maxSamples_ == -1 && response.status != null) {
-        if (response.status == 0) {
-          // If the first sample is valid, remaining is one less than max
-          // samples required.
-          this.maxSamples_ = response.remaining + 1;
-        } else {
-          // If the first sample failed for any reason (timed out, key full,
-          // etc), the remaining number of samples is the max samples required.
-          this.maxSamples_ = response.remaining;
-        }
+      this.progressArcLabel_ =
+          this.i18n('securityKeysBioEnrollmentEnrollingLabel');
+
+      assert(response.remaining >= 0);
+
+      if (this.maxSamples_ == -1) {
+        this.maxSamples_ = response.remaining + 1;
       }
-      // If 0 samples remain, the enrollment has finished in some state.
-      // Currently not checking response['code'] for an error.
+
       this.$.arc.setProgress(
-          100 - (100 * (response.remaining + 1) / this.maxSamples_),
-          100 - (100 * response.remaining / this.maxSamples_),
-          response.remaining == 0);
-      if (response.remaining == 0) {
-        assert(response.enrollment);
-        this.recentEnrollmentId_ = response.enrollment.id;
-        this.recentEnrollmentName_ = response.enrollment.name;
-        this.cancelButtonVisible_ = false;
-        this.confirmButtonVisible_ = true;
-        this.confirmButtonDisabled_ = false;
-        this.progressArcLabel_ =
-            this.i18n('securityKeysBioEnrollmentEnrollingCompleteLabel');
-        this.$.confirmButton.focus();
-        // Make screen-readers announce enrollment completion.
-        this.fire('iron-announce', {text: this.progressArcLabel_});
+          100 * (this.maxSamples_ - response.remaining - 1) / this.maxSamples_,
+          100 * (this.maxSamples_ - response.remaining) / this.maxSamples_,
+          false);
+    },
+
+    /**
+     * @private
+     * @param {!settings.EnrollmentResponse} response
+     */
+    onEnrollmentComplete_(response) {
+      if (response.code == settings.Ctap2Status.ERR_KEEPALIVE_CANCEL) {
+        this.showEnrollmentsPage_();
+        return;
       }
+      if (response.code != settings.Ctap2Status.OK) {
+        this.onError_(
+            this.i18n('securityKeysBioEnrollmentEnrollingFailedLabel'));
+        return;
+      }
+
+      this.maxSamples_ = Math.max(this.maxSamples_, 1);
+      this.$.arc.setProgress(
+          100 * (this.maxSamples_ - 1) / this.maxSamples_, 100, true);
+
+      assert(response.enrollment);
+      this.recentEnrollmentId_ = response.enrollment.id;
+      this.recentEnrollmentName_ = response.enrollment.name;
+      this.cancelButtonVisible_ = false;
+      this.confirmButtonVisible_ = true;
+      this.confirmButtonDisabled_ = false;
+      this.progressArcLabel_ =
+          this.i18n('securityKeysBioEnrollmentEnrollingCompleteLabel');
+      this.$.confirmButton.focus();
+      // Make screen-readers announce enrollment completion.
+      this.fire('iron-announce', {text: this.progressArcLabel_});
+
       this.fire('bio-enroll-dialog-ready-for-testing');
     },
 

chrome/browser/resources/settings/privacy_page/security_keys_browser_proxy.js

@@ -10,6 +10,7 @@ cr.define('settings', function() {
    */
   const Ctap2Status = {
     OK: 0x0,
+    ERR_INVALID_OPTION: 0x2C,
     ERR_KEEPALIVE_CANCEL: 0x2D,
   };
 
@@ -36,24 +37,33 @@ cr.define('settings', function() {
   let Credential;
 
   /**
-   * EnrollmentStatus represents the current status of an enrollment
-   * suboperation, where 'remaining' is the number of samples left, 'status' is
-   * the last enrollment status, 'code' indicates the final
-   * CtapDeviceResponseCode of the operation, and 'enrollment' contains the new
-   * Enrollment.
+   * SampleStatus is the result for reading an individual sample ("touch")
+   * during a fingerprint enrollment. This is a subset of the
+   * lastEnrollSampleStatus enum defined in the CTAP spec.
+   * @enum {number}
+   */
+  const SampleStatus = {
+    OK: 0x0,
+  };
+
+  /**
+   * SampleResponse indicates the result of an individual sample (sensor touch)
+   * for an enrollment suboperation.
    *
-   * For each enrollment sample, 'status' is set - when the enrollment operation
-   * reaches an end state, 'code' and, if successful, 'enrollment' are set. |OK|
-   * indicates successful enrollment. A code of |ERR_KEEPALIVE_CANCEL| indicates
-   * user-initated cancellation.
+   * @typedef {{status: settings.SampleStatus,
+   *            remaining: number}}
+   * @see chrome/browser/ui/webui/settings/settings_security_key_handler.cc
+   */
+  let SampleResponse;
+
+  /**
+   * EnrollmentResponse is the final response to an enrollment suboperation,
    *
-   * @typedef {{status: ?number,
-   *            code: ?settings.Ctap2Status,
-   *            remaining: number,
+   * @typedef {{code: settings.Ctap2Status,
    *            enrollment: ?settings.Enrollment}}
    * @see chrome/browser/ui/webui/settings/settings_security_key_handler.cc
    */
-  let EnrollmentStatus;
+  let EnrollmentResponse;
 
   /**
    * Enrollment represents a valid fingerprint template stored on a security
@@ -201,7 +211,7 @@ cr.define('settings', function() {
      * out waiting for a touch, or has successfully processed a touch. Any
      * errors will fire the 'security-keys-bio-enrollment-error' WebListener.
      *
-     * @return {!Promise<!settings.EnrollmentStatus>} resolves when the
+     * @return {!Promise<!settings.EnrollmentResponse>} resolves when the
      *     enrollment operation is finished successfully.
      */
     startEnrolling() {}
@@ -354,7 +364,9 @@ cr.define('settings', function() {
     Credential,
     Ctap2Status,
     Enrollment,
-    EnrollmentStatus,
+    EnrollmentResponse,
+    SampleStatus,
+    SampleResponse,
     SecurityKeysBioEnrollProxy,
     SecurityKeysBioEnrollProxyImpl,
     SecurityKeysCredentialBrowserProxy,

chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc

@@ -3220,6 +3220,10 @@ void AddSecurityKeysStrings(content::WebUIDataSource* html_source) {
        IDS_SETTINGS_SECURITY_KEYS_BIO_ENROLLMENT_ENROLLING_COMPLETE_LABEL},
       {"securityKeysBioEnrollmentEnrollingLabel",
        IDS_SETTINGS_SECURITY_KEYS_BIO_ENROLLMENT_ENROLLING_LABEL},
+      {"securityKeysBioEnrollmentEnrollingFailedLabel",
+       IDS_SETTINGS_SECURITY_KEYS_BIO_ENROLLMENT_FAILED_LABEL},
+      {"securityKeysBioEnrollmentTryAgainLabel",
+       IDS_SETTINGS_SECURITY_KEYS_BIO_ENROLLMENT_TRY_AGAIN_LABEL},
       {"securityKeysBioEnrollmentEnrollmentsLabel",
        IDS_SETTINGS_SECURITY_KEYS_BIO_ENROLLMENT_ENROLLMENTS_LABEL},
       {"securityKeysBioEnrollmentNoEnrollmentsLabel",

chrome/test/data/webui/settings/security_keys_subpage_test.js

@@ -828,11 +828,10 @@ suite('SecurityKeysBioEnrollment', function() {
     await uiReady;
 
     assertShown(allDivs, dialog, 'enroll');
-    uiReady =
-        test_util.eventToPromise('bio-enroll-dialog-ready-for-testing', dialog);
     cr.webUIListenerCallback(
-        'security-keys-bio-enroll-status', {status: 0, remaining: 1});
-    await uiReady;
+        'security-keys-bio-enroll-status',
+        {status: settings.SampleStatus.OK, remaining: 1});
+    Polymer.dom.flush();
     assertFalse(dialog.$.arc.isComplete());
     assertFalse(dialog.$.cancelButton.hidden);
     assert(dialog.$.confirmButton.hidden);
@@ -843,7 +842,6 @@ suite('SecurityKeysBioEnrollment', function() {
     const enrollmentName = 'New Fingerprint';
     enrollingResolver.resolve({
       code: 0,
-      remaining: 0,
       enrollment: {
         id: enrollmentId,
         name: enrollmentName,
@@ -918,4 +916,30 @@ suite('SecurityKeysBioEnrollment', function() {
     await uiReady;
     assertShown(allDivs, dialog, 'enrollments');
   });
+
+  test('EnrollError', async function() {
+    // Test that resolving the startEnrolling promise with a CTAP error brings
+    // up the error page.
+    const enrollResolver = new PromiseResolver;
+    browserProxy.setResponseFor('startEnrolling', enrollResolver.promise);
+
+    document.body.appendChild(dialog);
+    await browserProxy.whenCalled('startBioEnroll');
+
+    dialog.dialogPage_ = 'enrollments';
+
+    let uiReady =
+        test_util.eventToPromise('bio-enroll-dialog-ready-for-testing', dialog);
+    dialog.$.addButton.click();
+    await browserProxy.whenCalled('startEnrolling');
+    await uiReady;
+
+    uiReady =
+        test_util.eventToPromise('bio-enroll-dialog-ready-for-testing', dialog);
+
+    enrollResolver.resolve({code: settings.Ctap2Status.ERR_INVALID_OPTION});
+
+    await uiReady;
+    assertShown(allDivs, dialog, 'error');
+  });
 });