New issue
Advanced search Search tips

Issue 819256 link

Starred by 1 user
Status: Fixed
Owner:
kpaulhamus@chromium.org
Closed: Apr 2018
Cc:
kenrb@chromium.org
martinkr@google.com
engedy@chromium.org
ssoneff@google.com
kpaulhamus@chromium.org
jdoerrie@chromium.org
mknowles@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: ----


Participants' hotlists:
Hotlist-1


Sign in to add a comment

Add more informative messaging for various "Not Supported" and "NotAllowed" errors

Project Member Reported by kpaulhamus@chromium.org, Mar 6 2018 
To help distinguish between various NOTSUPPORTED failure cases. Cases in which NOTSUPPORTED errors are thrown:

- create()/get(): contains parameters are not supported by v2 authenticators
- create(): credTypesAndPubKeyAlgs is empty and options.pubKeyCredParams is not empty
- create(): none of the combinations of PublicKeyCredentialType and cryptographic parameters in credTypesAndPubKeyAlgs is supported
- FidoAppId extension called from create()
- store(publicKey) is called

Comment 1 by kpaulhamus@chromium.org, Mar 31 2018

Components: Blink>WebAuthentication

Comment 2 by engedy@chromium.org, Mar 31 2018

Labels: M-67 Pri-2
Owner: ----
Status: Available (was: Assigned)

Comment 3 by engedy@chromium.org, Mar 31 2018

Labels: Hotlist-WebAuthnFixit

Comment 4 by kpaulhamus@chromium.org, Apr 2 2018

Summary: Add more informative messaging for various "Not Supported" and "NotAllowed" errors (was: Add more informative messaging for various "Not Supported" errors)
We should document any reasoning for why detailed messaging for a particular error condition would be a privacy violation.

Alternatives suggested by Balazs for privacy preservation:
1) Logging these error messages to the console instead if the Testing API and/or vlogging is enabled.
2) Pointing developers to a page that explains possible error conditions, by means of putting a link into the umbrella error message, similar to this: https://cs.chromium.org/chromium/src/content/browser/webauth/authenticator_impl.cc?rcl=3ad5cfe39b30242addb3b778f3cb7a9bd86f60fd&l=364

Comment 5 by kpaulhamus@chromium.org, Apr 3 2018

Owner: kpaulhamus@chromium.org

Comment 6 by kpaulhamus@chromium.org, Apr 4 2018 

There are now three cases for NotSupportedError:
- create(): credTypesAndPubKeyAlgs is empty and options.pubKeyCredParams is not empty
- FidoAppId extension called from create()
- store(publicKey) is called

All three of these cases already have specific error messages.

Comment 7 by kpaulhamus@chromium.org, Apr 5 2018

Status: Started (was: Available)
Project Member

Comment 8 by bugdroid1@chromium.org, Apr 11 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/02d74e387f63a243b358a4b13ac5ed653bb6427a

commit 02d74e387f63a243b358a4b13ac5ed653bb6427a
Author: Kim Paulhamus <kpaulhamus@chromium.org>
Date: Wed Apr 11 02:00:52 2018

[webauthn] Return InvalidState instead of NotAllowedError for duplicates

InvalidState was added in crrev.com/984725 but not actually returned
to the renderer. This CL returns the correct error and adds a
corresponding end-to-end layout test using a virtual device.

Bug:  819256 
Change-Id: Iee07fee5b8de9af59f32399018e1b5ace474abe1
Reviewed-on: https://chromium-review.googlesource.com/1000958
Commit-Queue: Kim Paulhamus <kpaulhamus@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549722}
[modify] https://crrev.com/02d74e387f63a243b358a4b13ac5ed653bb6427a/content/browser/webauth/authenticator_impl.cc
[modify] https://crrev.com/02d74e387f63a243b358a4b13ac5ed653bb6427a/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-create-from-nested-frame.html
[modify] https://crrev.com/02d74e387f63a243b358a4b13ac5ed653bb6427a/third_party/WebKit/LayoutTests/http/tests/credentialmanager/resources/test-inputs.js
Project Member

Comment 9 by bugdroid1@chromium.org, Apr 11 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6f81d685b75110ce3ad4b5b2114c6c3a61ff8b76

commit 6f81d685b75110ce3ad4b5b2114c6c3a61ff8b76
Author: Kim Paulhamus <kpaulhamus@chromium.org>
Date: Wed Apr 11 05:37:03 2018

Update FidoResponseCode enums

Removed InvalidParams (unused)
Modified enums to be more descriptive:
- kFailure - kAuthenticatorResponseInvalid
- kInvalidState - kUserConsentButCredentialExcluded
- kConditionsNotSatisfied - kUserConsentButCredentialNotRecognized

kUserConsentButCredentialNotValid should result in INVALID_STATE per
issues filed with CTAP and WebAuthN working groups.

Bug:  819256 
Change-Id: I12e242b6649d03fc9655b27bc4650c02b5208cba
Reviewed-on: https://chromium-review.googlesource.com/1003392
Commit-Queue: Kim Paulhamus <kpaulhamus@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549775}
[modify] https://crrev.com/6f81d685b75110ce3ad4b5b2114c6c3a61ff8b76/content/browser/webauth/authenticator_impl.cc
[modify] https://crrev.com/6f81d685b75110ce3ad4b5b2114c6c3a61ff8b76/content/browser/webauth/authenticator_impl_unittest.cc
[modify] https://crrev.com/6f81d685b75110ce3ad4b5b2114c6c3a61ff8b76/device/fido/fido_constants.h
[modify] https://crrev.com/6f81d685b75110ce3ad4b5b2114c6c3a61ff8b76/device/fido/fido_request_handler.h
[modify] https://crrev.com/6f81d685b75110ce3ad4b5b2114c6c3a61ff8b76/device/fido/fido_request_handler_unittest.cc
[modify] https://crrev.com/6f81d685b75110ce3ad4b5b2114c6c3a61ff8b76/device/fido/u2f_register.cc
[modify] https://crrev.com/6f81d685b75110ce3ad4b5b2114c6c3a61ff8b76/device/fido/u2f_register_unittest.cc
[modify] https://crrev.com/6f81d685b75110ce3ad4b5b2114c6c3a61ff8b76/device/fido/u2f_sign.cc
[modify] https://crrev.com/6f81d685b75110ce3ad4b5b2114c6c3a61ff8b76/device/fido/u2f_sign_unittest.cc
[modify] https://crrev.com/6f81d685b75110ce3ad4b5b2114c6c3a61ff8b76/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-get-from-nested-frame.html
Project Member

Comment 10 by bugdroid1@chromium.org, Apr 12 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/595abc6588a3875aab2dff6cf957ef2efe0fca28

commit 595abc6588a3875aab2dff6cf957ef2efe0fca28
Author: Kim Paulhamus <kpaulhamus@chromium.org>
Date: Thu Apr 12 20:42:51 2018

Use specific error messages where possible.

Since the current implementation doesn't support non-U2F authenticators,
all non-U2F scenarios can be immediately denied with NotSupportedError.

This CL also moves handling for empty allowCredentials (part of the
aforementioned errors) out of u2f_sign, since u2f_sign should never be
called with empty allowCredentials.

Bug:  819256 
Change-Id: I991949919fbe7a5e1e9c123ee750d80c3d25e9c3
Reviewed-on: https://chromium-review.googlesource.com/1006482
Commit-Queue: Kim Paulhamus <kpaulhamus@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550346}
[modify] https://crrev.com/595abc6588a3875aab2dff6cf957ef2efe0fca28/components/password_manager/content/common/credential_manager_mojom_traits.cc
[modify] https://crrev.com/595abc6588a3875aab2dff6cf957ef2efe0fca28/content/browser/webauth/authenticator_impl.cc
[modify] https://crrev.com/595abc6588a3875aab2dff6cf957ef2efe0fca28/content/browser/webauth/authenticator_impl_unittest.cc
[modify] https://crrev.com/595abc6588a3875aab2dff6cf957ef2efe0fca28/content/browser/webauth/webauth_browsertest.cc
[modify] https://crrev.com/595abc6588a3875aab2dff6cf957ef2efe0fca28/device/fido/u2f_sign.cc
[modify] https://crrev.com/595abc6588a3875aab2dff6cf957ef2efe0fca28/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-create-basics.html
[modify] https://crrev.com/595abc6588a3875aab2dff6cf957ef2efe0fca28/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-get-basics.html
[modify] https://crrev.com/595abc6588a3875aab2dff6cf957ef2efe0fca28/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-get-from-nested-frame.html
[add] https://crrev.com/595abc6588a3875aab2dff6cf957ef2efe0fca28/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-get-with-virtual-authenticator.html
[modify] https://crrev.com/595abc6588a3875aab2dff6cf957ef2efe0fca28/third_party/blink/public/platform/modules/credentialmanager/credential_manager.mojom
[modify] https://crrev.com/595abc6588a3875aab2dff6cf957ef2efe0fca28/third_party/blink/public/platform/modules/webauth/authenticator.mojom
[modify] https://crrev.com/595abc6588a3875aab2dff6cf957ef2efe0fca28/third_party/blink/renderer/modules/credentialmanager/credential_manager_type_converters.cc
[modify] https://crrev.com/595abc6588a3875aab2dff6cf957ef2efe0fca28/third_party/blink/renderer/modules/credentialmanager/credentials_container.cc

Comment 11 by kpaulhamus@chromium.org, Apr 12 2018

Status: Fixed (was: Started)
Project Member

Comment 13 by bugdroid1@chromium.org, Apr 17 2018

Labels: merge-merged-testbranch
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/595abc6588a3875aab2dff6cf957ef2efe0fca28

commit 595abc6588a3875aab2dff6cf957ef2efe0fca28
Author: Kim Paulhamus <kpaulhamus@chromium.org>
Date: Thu Apr 12 20:42:51 2018

Use specific error messages where possible.

Since the current implementation doesn't support non-U2F authenticators,
all non-U2F scenarios can be immediately denied with NotSupportedError.

This CL also moves handling for empty allowCredentials (part of the
aforementioned errors) out of u2f_sign, since u2f_sign should never be
called with empty allowCredentials.

Bug:  819256 
Change-Id: I991949919fbe7a5e1e9c123ee750d80c3d25e9c3
Reviewed-on: https://chromium-review.googlesource.com/1006482
Commit-Queue: Kim Paulhamus <kpaulhamus@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550346}
[modify] https://crrev.com/595abc6588a3875aab2dff6cf957ef2efe0fca28/components/password_manager/content/common/credential_manager_mojom_traits.cc
[modify] https://crrev.com/595abc6588a3875aab2dff6cf957ef2efe0fca28/content/browser/webauth/authenticator_impl.cc
[modify] https://crrev.com/595abc6588a3875aab2dff6cf957ef2efe0fca28/content/browser/webauth/authenticator_impl_unittest.cc
[modify] https://crrev.com/595abc6588a3875aab2dff6cf957ef2efe0fca28/content/browser/webauth/webauth_browsertest.cc
[modify] https://crrev.com/595abc6588a3875aab2dff6cf957ef2efe0fca28/device/fido/u2f_sign.cc
[modify] https://crrev.com/595abc6588a3875aab2dff6cf957ef2efe0fca28/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-create-basics.html
[modify] https://crrev.com/595abc6588a3875aab2dff6cf957ef2efe0fca28/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-get-basics.html
[modify] https://crrev.com/595abc6588a3875aab2dff6cf957ef2efe0fca28/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-get-from-nested-frame.html
[add] https://crrev.com/595abc6588a3875aab2dff6cf957ef2efe0fca28/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-get-with-virtual-authenticator.html
[modify] https://crrev.com/595abc6588a3875aab2dff6cf957ef2efe0fca28/third_party/blink/public/platform/modules/credentialmanager/credential_manager.mojom
[modify] https://crrev.com/595abc6588a3875aab2dff6cf957ef2efe0fca28/third_party/blink/public/platform/modules/webauth/authenticator.mojom
[modify] https://crrev.com/595abc6588a3875aab2dff6cf957ef2efe0fca28/third_party/blink/renderer/modules/credentialmanager/credential_manager_type_converters.cc
[modify] https://crrev.com/595abc6588a3875aab2dff6cf957ef2efe0fca28/third_party/blink/renderer/modules/credentialmanager/credentials_container.cc

Comment 15 by kpaulhamus@chromium.org, Apr 18 2018

Labels: Merge-Request-67
Requesting merge of https://chromium-review.googlesource.com/1006482 (comment #10)

Comment 16 by kpaulhamus@chromium.org, Apr 18 2018

Labels: -Merge-Request-67
Removing request - CL already made it into the branch.
Project Member

Comment 17 by bugdroid1@chromium.org, Apr 20 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/183d19265345f54ce39cbb94cf81ba5f15905011

commit 183d19265345f54ce39cbb94cf81ba5f15905011
Author: Kim Paulhamus <kpaulhamus@chromium.org>
Date: Fri Apr 20 23:27:09 2018

[webauthn] Add unit tests for pending_requests and credential_excluded.

Bug:  819256 
Change-Id: I8fb3a646ebd390530c611aa4a42db501168db62f
Reviewed-on: https://chromium-review.googlesource.com/1020441
Commit-Queue: Kim Paulhamus <kpaulhamus@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552518}
[modify] https://crrev.com/183d19265345f54ce39cbb94cf81ba5f15905011/content/browser/webauth/authenticator_impl_unittest.cc

Comment 18 by kpaulhamus@chromium.org, Apr 26 2018

Labels: Merge-Request-67
Note: Requesting merge of https://chromium-review.googlesource.com/1011624 (comment #14) to make https://chromium-review.googlesource.com/c/chromium/src/+/1020187 safe to merge (already approved).
Project Member

Comment 19 by bugdroid1@chromium.org, Apr 26 2018

Labels: merge-merged-3396
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/514b359c2e02517a0dc2a0f96b91786169b59cfb

commit 514b359c2e02517a0dc2a0f96b91786169b59cfb
Author: Kim Paulhamus <kpaulhamus@chromium.org>
Date: Thu Apr 26 20:52:26 2018

[webauthn] Move non-frame test out of -create-with--nested-frame.html

Bug:  819256 
Change-Id: I677ab097809b81182aeb5db2ccfa21b06e1c5430
Reviewed-on: https://chromium-review.googlesource.com/1011624
Commit-Queue: Kim Paulhamus <kpaulhamus@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#550852}(cherry picked from commit eaff70c7ff69f94529e6a02e374203cdb40b4fa0)
Reviewed-on: https://chromium-review.googlesource.com/1031272
Reviewed-by: Kim Paulhamus <kpaulhamus@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#341}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/514b359c2e02517a0dc2a0f96b91786169b59cfb/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-create-from-nested-frame.html
[add] https://crrev.com/514b359c2e02517a0dc2a0f96b91786169b59cfb/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-create-with-virtual-authenticator.html
Project Member

Comment 20 by sheriffbot@chromium.org, Apr 26 2018

Labels: -Merge-Request-67 Merge-Review-67 Hotlist-Merge-Review
This bug requires manual review: M67 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 21 by gov...@chromium.org, Apr 26 2018

Labels: -Merge-Review-67 Merge-Approved-67
This is already merged at #19 and was approved via offline chat.

Comment 22 by gov...@chromium.org, Apr 26 2018

Labels: -Merge-Approved-67
Removing "Merge-Approved-67" label as it is already merged at #19.
►

Sign in to add a comment