[webauthn] Add unknownCredentialId report [chromium/src : main]
Gabriel Viera (Gerrit)
Gabriel Viera voted and added 5 comments![]( "Open in Gerrit")
Votes added by Gabriel Viera
| Commit-Queue | +1 |
| Mega-CQ | +1 |
5 comments
Parent: 237f9fc4 (Updating trunk VERSION from 6540.0 to 6541.0)Gabriel VieraDon't forget to run the trybots before sending your CL for review. There's a button on the top right "CQ DRY RUN" that should do it
Done
unknownCredentialId report names a credential ID and indicate that the relying party would reject an assertion with that credential because the credential ID is unknown to the relying party.Gabriel Vieranit:
```
the relying party would reject an assertion
```should be
```
the relying party rejected an assertion
```
Done
unknownCredentialId report names a credential ID and indicate that the relying party would reject an assertion with that credential because the credential ID is unknown to the relying party.Gabriel Vieranit: This is a very long title. Normally we want to stick to < 72 chars for each line of a commit, ideally < 50 for the title, otherwise it'll get ellipsized by tooling.
How about
```
[webauthn] Add unknownCredentialId reportThe unknownCredentialId report names...
```
Done
When a call to navigator.credentials.report() includes "unknownCredentialId," it checks to make sure the website reporting the ID is actually the one that issued it. If the check is successful, the credential is then removed from Google Password Manager (GPM).Gabriel Vieranit:
the website reporting the ID is actually the one that issued it
this isn't strictly accurate, since it could be a different website using the related rp ids feature. Maybe something like
```
it checks that the relying party ID corresponds to the website.
If the check is successful...
```
Done
When a call to navigator.credentials.report() includes "unknownCredentialId," it checks to make sure the website reporting the ID is actually the one that issued it. If the check is successful, the credential is then removed from Google Password Manager (GPM).Gabriel Vieranit: put the comma outside the quotes. Also, format to 72 character columns.
Done
Related details
- Ken Buchanan
- Nina Satragno
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ken Buchanan (Gerrit)
Ken Buchanan added 2 comments![]( "Open in Gerrit")
#include "components/webauthn/core/browser/passkey_model.h"This doesn't look to be used either.
#include "chrome/browser/profiles/profile.h"This shouldn't be here. There is a forward declaration of `class Profile` and that is sufficient.
Related details
- Gabriel Viera
- Nina Satragno
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gabriel Viera (Gerrit)
Gabriel Viera added 30 comments![]( "Open in Gerrit")
#include "components/webauthn/core/browser/passkey_model.h"This doesn't look to be used either.
Removed.
This shouldn't be here. There is a forward declaration of `class Profile` and that is sufficient.
#include removed.
raw_ptr<webauthn::PasskeyModel> passkey_store_ =Gabriel Viera`raw_ptr` is only used for class / struct members. Also, the underscore notation is only used for class members.
Change done.
if (!passkey_credential_id.empty()) {Gabriel VieraIs this check really doing anything for us? What difference would it make to remove it?
Removed. Removing that check makes no difference.
// TODO: ADD COMMENTGabriel VieraYou wouldn't have a comment here since this is an implementation file.
Done.
const std::string& relying_party_id) {Gabriel VieraPass the origin here (see other methods for an example) so you can validate it.
Done.
const std::string& relying_party_id) {Gabriel VieraThis also needs to take a callback so it can return a status code (success, or rp id validation error).
Done.
if (req_state_) {Gabriel Viera`req_state_` will always be `null` if there's no pending webauthn request. In testing, this probably worked for you if you were using a site that does a conditional ui request in the background. Why are we checking for it here?
Removed. Done.
if (rp_id_validation_result == blink::mojom::AuthenticatorStatus::SUCCESS) {Gabriel VieraWe need to notify the site that this validation failed.
Done.
if (rp_id_validation_result == blink::mojom::AuthenticatorStatus::SUCCESS) {Gabriel Vieranit: normally, we prefer early returns for error handling, so they don't stack the scopes and we end up with really deep scopes.
Prefer:
```
if (rp_id_validation_result != blink::mojom::AuthenticatorStatus::SUCCESS) {
std::move(callback).Run(Status::kRpValidationError);
return;
}// Execution continues here...
```
Done.
Gabriel Vieranit: bring back this whitespace
Done
authenticator_common_impl_->RemoveCredential(std::move(passkey_credential_id),Gabriel Vieranit: I don't think you get anything from std::moving a reference.
Done.
return blink::mojom::AuthenticatorStatus::SUCCESS;Gabriel VieraAdd a TODO. Kinda like the AI wants, except with a correct crbug.
Done.
// TODO: Add CommentGabriel Vierapls add the comments with your CL (:
Done.
return;Gabriel Vieranit: this `return` is redundant, remove it.
Done.
// TODO : ADD CommentsGabriel VieraJust add the comments with your CL (:
Done.
RemoveCredential(array<uint8> passkey_credential_id,
string relying_party_id);Gabriel VieraI don't understand why you defined `PublicKeyCredentialReportOptions` above, but then didn't actually use it. There's no point defining a mojo struct that is not referenced in a mojo method.
You can either remove `PublicKeyCredentialReportOptions` above, or change this method into something like
```
Report(PublicKeyCredentialReportOptions options);
```
The method was changed to Report(PublicKeyCredentialReportOptions options), and then in the Report method, I call the RemoveCredential method.
string relying_party_id);Gabriel VieraWe should have this method return a status code. That way, we can notify the renderer of errors detected on the browser process, e.g. that the RP validation failed.
Done.
#include "base/logging.h"Gabriel VieraDo we need this?
No, removed.
// TODO:: ADD COMMENTGabriel VieraWe don't add function documentation comments on implementation files. These normally go the file that defines them (.h headers or e.g. mojom files).
This function is special because it implements a web API, we don't normally comment what they do.
Done.
exception_state.ThrowDOMException(DOMExceptionCode::kInvalidStateError,Gabriel VieraInstead of throwing a raw exception and returning an empty promise, let's reject the promise with the reason. See `preventSilentAccess` for examples.
Done.
exception_state.ThrowDOMException(DOMExceptionCode::kInvalidStateError,Gabriel VieraDo we really want to throw if there's no `publicKey`? We haven't specified what to do in this state yet, but to me it seems we should just not do anything.
The code was changed to check only if a public key exists to convert a mojo option.
WTF::String rpID = mojo_options->relying_party_id;Gabriel Viera`rp_id`
Done.
// Copyright 2015 The Chromium AuthorsGabriel VieraUpdate the year.
https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++.md#file-headers
Done.
AbortSignal signal;Gabriel VieraI don't think we need to let websites abort the report, let's get rid of `signal`.
Done.
[CallWith=ScriptState, RaisesException, RuntimeEnabled=CredentialManagerReport] Promise<undefined> report(optional CredentialReportOptions options = {});Gabriel VieraIs there a reason to have a default, optional `options`? It really makes no sense to call `report` with no `options` paramater, let's not make the `options` parameter optional and remove the default value.
Done.
// Copyright 2016 The Chromium AuthorsGabriel VieraUpdate the year
Done.
BufferSource id;Gabriel VieraNote that this doesn't match the [explainer](https://github.com/w3c/webauthn/wiki/Explainer:-WebAuthn-Report-API-explainer#unknowncredentialid).
In the explainer, the example given is:
```
navigator.credentials.report({publicKey: {
rpId: "example.com",
unknownCredentialId: "vI0qOggiE3OT01ZRWBYz5l4MEgU0c7PmAA"
}});
```but with this IDL, we'd need:
```
navigator.credentials.report({publicKey: {
rpId: "example.com",
unknownCredentialId: {
id: "vI0qOggiE3OT01ZRWBYz5l4MEgU0c7PmAA"
}
}});
```which seems redundant, no? Let's remove that extra layer of indirection.
Done.
Related details
- Ken Buchanan
- Nina Satragno
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nina Satragno (Gerrit)
Nina Satragno added 1 comment![]( "Open in Gerrit")
- Gabriel Viera
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nina Satragno (Gerrit)
Nina Satragno added 11 comments![]( "Open in Gerrit")
This is a lot better! Besides the comments, please fix the bots, and add some tests (I'll follow up offline).
void RemoveCredential(const std::vector<uint8_t>& passkey_credential_id);This method isn't really doing much, remove?
std::unique_ptr<WebAuthRequestSecurityChecker::RemoteValidation>nit: why not
```
req_state_->remote_rp_id_validation = security_checker_->ValidateDomainAndRelyingPartyID(
req_state_->caller_origin, req_state_->relying_party_id,
WebAuthRequestSecurityChecker::RequestType::kReport,
/*remote_desktop_client_override=*/nullptr,
base::BindOnce(&AuthenticatorCommonImpl::ContinueReportAfterRpCheck,
weak_factory_.GetWeakPtr(), std::move(options)));
```
std::vector<uint8_t> credential_id = options->unknown_credential_id;nit: in general, declare in the scope that it's used. In this case, the variable is only used once and it really doesn't help to readability to define a new variable.
```
RemoveCredential(options->unknown_credential_id);
```
is more readable imo.
} else {nit: this `else` is redudant because you're already `return`'ing above.
```
if (...) {
...
return;
}
RemoveCredential(...);
```
void AuthenticatorCommonImpl::RemoveCredential(nit: this method isn't really pulling its own weight. Why not directly call `DeletePasskey` above on L1633, and remove this?
void OnReportComplete(std::unique_ptr<ScopedPromiseResolver> scoped_resolver,Anonymous functions should be on the anonymous namespace on top of the file, move there.
auto promise = resolver->Promise();nit: why not `return resolver->Promise()` at the end and omit this statement?
ExecutionContext* context = ExecutionContext::From(script_state);Move this declaration to the context you're using it in (or consider removing it altogether).
nit: remove this whitespace (also on L1048)
in general, whitespace after a closing brace is redundant.
https://google.github.io/styleguide/cppguide.html#Vertical_Whitespace
Related details
- Gabriel Viera
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
findit-for-me@appspot.gserviceaccount.com (Gerrit)
findit...@appspot.gserviceaccount.com voted Code-Coverage-1![]( "Open in Gerrit")
This change will be blocked from submission as there are files which do not meet the coverage criteria. Following files have incremental coverage(all tests) < 70%.
- //components/webauthn/android/java/src/org/chromium/components/webauthn/AuthenticatorImpl.java
Please add tests for uncovered lines, or add Low-Coverage-Reason:<reason> in the change description to bypass. See https://bit.ly/46jhjS9 to understand when it is okay to bypass. If you think coverage is underreported, file a bug at https://bit.ly/3ENM7Pe
| Code-Coverage | -1 |
Related details
- Gabriel Viera
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gabriel Viera (Gerrit)
Gabriel Viera voted and added 10 comments![]( "Open in Gerrit")
Votes added by Gabriel Viera
| Commit-Queue | +1 |
10 comments
void RemoveCredential(const std::vector<uint8_t>& passkey_credential_id);This method isn't really doing much, remove?
Done.
std::unique_ptr<WebAuthRequestSecurityChecker::RemoteValidation>nit: why not
```
req_state_->remote_rp_id_validation = security_checker_->ValidateDomainAndRelyingPartyID(
req_state_->caller_origin, req_state_->relying_party_id,
WebAuthRequestSecurityChecker::RequestType::kReport,
/*remote_desktop_client_override=*/nullptr,
base::BindOnce(&AuthenticatorCommonImpl::ContinueReportAfterRpCheck,
weak_factory_.GetWeakPtr(), std::move(options)));
```
Done.
std::vector<uint8_t> credential_id = options->unknown_credential_id;nit: in general, declare in the scope that it's used. In this case, the variable is only used once and it really doesn't help to readability to define a new variable.
```
RemoveCredential(options->unknown_credential_id);
```is more readable imo.
Done.
nit: this `else` is redudant because you're already `return`'ing above.
```
if (...) {
...
return;
}
RemoveCredential(...);
```
Done.
nit: this method isn't really pulling its own weight. Why not directly call `DeletePasskey` above on L1633, and remove this?
Done.
void OnReportComplete(std::unique_ptr<ScopedPromiseResolver> scoped_resolver,Anonymous functions should be on the anonymous namespace on top of the file, move there.
Done.
nit: why not `return resolver->Promise()` at the end and omit this statement?
Done.
ExecutionContext* context = ExecutionContext::From(script_state);Move this declaration to the context you're using it in (or consider removing it altogether).
Done.
nit: remove this whitespace (also on L1048)
in general, whitespace after a closing brace is redundant.
https://google.github.io/styleguide/cppguide.html#Vertical_Whitespace
- Nina Satragno
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nina Satragno (Gerrit)
Nina Satragno added 4 comments![]( "Open in Gerrit")
This looks good enough for some tests now!
mReportCallback = callback;Are we ever using `mReportCallback`? Looks like we can get rid of it.
mIsOperationPending = true;It looks to me like nothing is clearing `mIsOperationPending`, so if a website calls `report`, they can never again do any webauthn operations. Why are we setting this flag to `true`?
if (!GmsCoreUtils.isWebauthnSupported()
|| (!isChrome(mWebContents) && !GmsCoreUtils.isResultReceiverSupported())) {
onError(AuthenticatorStatus.NOT_IMPLEMENTED);
return;
}I don't think any of this will apply to the report API once it's implemented by Android's credential manager. Let's just unconditionally immediately resolve `callback` with a success code for now, pretending that the credential ID is not present.
Related details
- Gabriel Viera
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
findit-for-me@appspot.gserviceaccount.com (Gerrit)
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
This change meets the code coverage requirements.
| Code-Coverage | +1 |
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |