Allow IWAs to call navigator.credentials methods [chromium/src : main]
Martin Kreichgauer (Gerrit)
Martin Kreichgauer added 5 comments![]( "Open in Gerrit")
It looks like in the latest PS you uploaded an outdated gclient sync state.
scoped_feature_list.InitWithFeatures(Olga KorokhinaNit: `scoped_feature_list_`.
But more generally, it's odd that the base test suite shouldn't enables IWAs if it's only relevant for a fraction of the tests. I suggest splitting this into its own test suite, or moving the ScopedFeatureList inline.
Martin KreichgauerWe can't make it inline due to features should be set up in a constructor:
FATAL browser_tests[243908:243908]: [base/feature_list.cc:137] DCHECK failed: feature_overrides_allowed. FeatureList overrides must happen in the test constructor, before BrowserTestBase::SetUp() has run.
Can we still keep it in same test suite, please? For the sake of hermeticy.
That is a fair point. But your most recent change made the ScopedFeatureList local to the constructor, which is certainly not what was intended.
// we do not add any permission here because of it will fail beforeNit: W
::testing::Values(true, false),Nit: `testing::Bool()`
} else if (!webauthn::
OriginIsAllowedToUseWebAuthnWithoutRemoteDesktopClientOverride(
caller_origin)) {
// IWAs now can only use WebAuthn in VDI, so with remote desktop override
// extension only
std::move(callback).Run(blink::mojom::AuthenticatorStatus::NOT_IMPLEMENTED);
return nullptr;
}Olga KorokhinaI think it would be simpler to just update OriginIsAllowedToClaimRelyingPartyId() to always return false for `iwa://...` origins. I believe that's the semantics we want here, and then we don't need this additional method.
Martin KreichgauerNo, this doesn't work because that check happens before check if remoteDesktopClientOverride is used or not. I tried the way you propose and it lead to any IWA calls were rejected. We need to disable local but enable VDIs (with remoteDesktopClientOverride used).
I agree that only calls with remoteDesktopClientOverride should succeed, but…
this doesn't work because that check happens before check if remoteDesktopClientOverride is used or not.
Isn't the check for the remoteDesktopClientOverride happening on ll185-200 and the check for OriginIsAllowedToClaimRelyingPartyId() on l210 (which would be **after** and not before)?
I.e., if an IWA was using remoteDesktopClientOverride (and authorized to do so), relying_party_origin would be set to the origin override on l200, and then OriginIsAllowedToClaimRelyingPartyId() would succeed as if the caller was not an IWA. If an IWA was not using the override extension, OriginIsAllowedToClaimRelyingPartyId() would fail, because the caller is an IWA.
Related details
- Andrew Rayskiy
- Olga Korokhina
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olga Korokhina (Gerrit)
Olga Korokhina voted and added 4 comments![]( "Open in Gerrit")
Votes added by Olga Korokhina
| Commit-Queue | +1 |
4 comments
scoped_feature_list.InitWithFeatures(Olga KorokhinaNit: `scoped_feature_list_`.
But more generally, it's odd that the base test suite shouldn't enables IWAs if it's only relevant for a fraction of the tests. I suggest splitting this into its own test suite, or moving the ScopedFeatureList inline.
Martin KreichgauerWe can't make it inline due to features should be set up in a constructor:
FATAL browser_tests[243908:243908]: [base/feature_list.cc:137] DCHECK failed: feature_overrides_allowed. FeatureList overrides must happen in the test constructor, before BrowserTestBase::SetUp() has run.
Can we still keep it in same test suite, please? For the sake of hermeticy.
That is a fair point. But your most recent change made the ScopedFeatureList local to the constructor, which is certainly not what was intended.
True! Still it somehow works, I believe we can keep it there (local to constructor) - looks lie it is injected properly and stays until end of execution.
// we do not add any permission here because of it will fail beforeOlga KorokhinaNit: W
Thank you, fixed.
::testing::Values(true, false),Olga KorokhinaNit: `testing::Bool()`
Cool, I didn't know it will work with both possible values this way, thank you very much!
} else if (!webauthn::
OriginIsAllowedToUseWebAuthnWithoutRemoteDesktopClientOverride(
caller_origin)) {
// IWAs now can only use WebAuthn in VDI, so with remote desktop override
// extension only
std::move(callback).Run(blink::mojom::AuthenticatorStatus::NOT_IMPLEMENTED);
return nullptr;
}Olga KorokhinaI think it would be simpler to just update OriginIsAllowedToClaimRelyingPartyId() to always return false for `iwa://...` origins. I believe that's the semantics we want here, and then we don't need this additional method.
Martin KreichgauerNo, this doesn't work because that check happens before check if remoteDesktopClientOverride is used or not. I tried the way you propose and it lead to any IWA calls were rejected. We need to disable local but enable VDIs (with remoteDesktopClientOverride used).
I agree that only calls with remoteDesktopClientOverride should succeed, but…
this doesn't work because that check happens before check if remoteDesktopClientOverride is used or not.
Isn't the check for the remoteDesktopClientOverride happening on ll185-200 and the check for OriginIsAllowedToClaimRelyingPartyId() on l210 (which would be **after** and not before)?
I.e., if an IWA was using remoteDesktopClientOverride (and authorized to do so), relying_party_origin would be set to the origin override on l200, and then OriginIsAllowedToClaimRelyingPartyId() would succeed as if the caller was not an IWA. If an IWA was not using the override extension, OriginIsAllowedToClaimRelyingPartyId() would fail, because the caller is an IWA.
Oh, this way. Thank you, adjusted, hope understood it right.
Related details
- Andrew Rayskiy
- Martin Kreichgauer
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kreichgauer (Gerrit)
Martin Kreichgauer added 1 comment![]( "Open in Gerrit")
scoped_feature_list.InitWithFeatures(Olga KorokhinaNit: `scoped_feature_list_`.
But more generally, it's odd that the base test suite shouldn't enables IWAs if it's only relevant for a fraction of the tests. I suggest splitting this into its own test suite, or moving the ScopedFeatureList inline.
Martin KreichgauerWe can't make it inline due to features should be set up in a constructor:
FATAL browser_tests[243908:243908]: [base/feature_list.cc:137] DCHECK failed: feature_overrides_allowed. FeatureList overrides must happen in the test constructor, before BrowserTestBase::SetUp() has run.
Can we still keep it in same test suite, please? For the sake of hermeticy.
Olga KorokhinaThat is a fair point. But your most recent change made the ScopedFeatureList local to the constructor, which is certainly not what was intended.
True! Still it somehow works, I believe we can keep it there (local to constructor) - looks lie it is injected properly and stays until end of execution.
That's not how it works, I'm afraid. The ScopedFeatureList destructor resets any overrides. https://source.chromium.org/chromium/chromium/src/+/main:base/test/scoped_feature_list.cc;drc=6d57b3d63b080eb4f85344ff0782c962b8ba794b;bpv=1;bpt=1;l=303?q=base::test::ScopedFeatureList
Related details
- Andrew Rayskiy
- Olga Korokhina
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olga Korokhina (Gerrit)
Olga Korokhina added 1 comment![]( "Open in Gerrit")
scoped_feature_list.InitWithFeatures(Olga KorokhinaNit: `scoped_feature_list_`.
But more generally, it's odd that the base test suite shouldn't enables IWAs if it's only relevant for a fraction of the tests. I suggest splitting this into its own test suite, or moving the ScopedFeatureList inline.
Martin KreichgauerWe can't make it inline due to features should be set up in a constructor:
FATAL browser_tests[243908:243908]: [base/feature_list.cc:137] DCHECK failed: feature_overrides_allowed. FeatureList overrides must happen in the test constructor, before BrowserTestBase::SetUp() has run.
Can we still keep it in same test suite, please? For the sake of hermeticy.
Olga KorokhinaThat is a fair point. But your most recent change made the ScopedFeatureList local to the constructor, which is certainly not what was intended.
Martin KreichgauerTrue! Still it somehow works, I believe we can keep it there (local to constructor) - looks lie it is injected properly and stays until end of execution.
That's not how it works, I'm afraid. The ScopedFeatureList destructor resets any overrides. https://source.chromium.org/chromium/chromium/src/+/main:base/test/scoped_feature_list.cc;drc=6d57b3d63b080eb4f85344ff0782c962b8ba794b;bpv=1;bpt=1;l=303?q=base::test::ScopedFeatureList
Copy, thank you! So I need to revert it, have scoped_feature_list be declared in class and be initialized in the constructor, let me do this.
Related details
- Andrew Rayskiy
- Martin Kreichgauer
Olga Korokhina (Gerrit)
Olga Korokhina added 2 comments![]( "Open in Gerrit")
scoped_feature_list.InitWithFeatures(Olga KorokhinaNit: `scoped_feature_list_`.
But more generally, it's odd that the base test suite shouldn't enables IWAs if it's only relevant for a fraction of the tests. I suggest splitting this into its own test suite, or moving the ScopedFeatureList inline.
Martin KreichgauerWe can't make it inline due to features should be set up in a constructor:
FATAL browser_tests[243908:243908]: [base/feature_list.cc:137] DCHECK failed: feature_overrides_allowed. FeatureList overrides must happen in the test constructor, before BrowserTestBase::SetUp() has run.
Can we still keep it in same test suite, please? For the sake of hermeticy.
Olga KorokhinaThat is a fair point. But your most recent change made the ScopedFeatureList local to the constructor, which is certainly not what was intended.
Martin KreichgauerTrue! Still it somehow works, I believe we can keep it there (local to constructor) - looks lie it is injected properly and stays until end of execution.
Olga KorokhinaThat's not how it works, I'm afraid. The ScopedFeatureList destructor resets any overrides. https://source.chromium.org/chromium/chromium/src/+/main:base/test/scoped_feature_list.cc;drc=6d57b3d63b080eb4f85344ff0782c962b8ba794b;bpv=1;bpt=1;l=303?q=base::test::ScopedFeatureList
Copy, thank you! So I need to revert it, have scoped_feature_list be declared in class and be initialized in the constructor, let me do this.
Done
} else if (!webauthn::
OriginIsAllowedToUseWebAuthnWithoutRemoteDesktopClientOverride(
caller_origin)) {
// IWAs now can only use WebAuthn in VDI, so with remote desktop override
// extension only
std::move(callback).Run(blink::mojom::AuthenticatorStatus::NOT_IMPLEMENTED);
return nullptr;
}Olga KorokhinaI think it would be simpler to just update OriginIsAllowedToClaimRelyingPartyId() to always return false for `iwa://...` origins. I believe that's the semantics we want here, and then we don't need this additional method.
Martin KreichgauerNo, this doesn't work because that check happens before check if remoteDesktopClientOverride is used or not. I tried the way you propose and it lead to any IWA calls were rejected. We need to disable local but enable VDIs (with remoteDesktopClientOverride used).
Olga KorokhinaI agree that only calls with remoteDesktopClientOverride should succeed, but…
this doesn't work because that check happens before check if remoteDesktopClientOverride is used or not.
Isn't the check for the remoteDesktopClientOverride happening on ll185-200 and the check for OriginIsAllowedToClaimRelyingPartyId() on l210 (which would be **after** and not before)?
I.e., if an IWA was using remoteDesktopClientOverride (and authorized to do so), relying_party_origin would be set to the origin override on l200, and then OriginIsAllowedToClaimRelyingPartyId() would succeed as if the caller was not an IWA. If an IWA was not using the override extension, OriginIsAllowedToClaimRelyingPartyId() would fail, because the caller is an IWA.
Oh, this way. Thank you, adjusted, hope understood it right.
Done
Related details
- Andrew Rayskiy
- Martin Kreichgauer
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy voted Code-Review+1![]( "Open in Gerrit")
- Martin Kreichgauer
- Olga Korokhina
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kreichgauer (Gerrit)
Martin Kreichgauer added 21 comments![]( "Open in Gerrit")
#include "chrome/common/url_constants.h"Do you need this include?
base::ListValue list = base::ListValue().Append(kTestIsolatedAppOrigin);Nit: I think this could reasonably be inlined to the SetList() call.
base::ListValue list = base::ListValue().Append(kExampleOrigin);Same here, and throughout.
#include "chrome/browser/web_applications/web_app_command_scheduler.h"Do you need this include?
scoped_feature_list_.InitWithFeatures(
{device::kWebAuthnIWARemoteDesktopAllowedOriginsPolicy,
features::kIsolatedWebApps},
{});Please move this into a separate fixture that inherits from this one, and move the new tests there.
content::RenderFrameHost* OpenApp(const webapps::AppId& app_id,Please add a comment.
```suggestion
// Launches the IWA with the given ID and returns its main frame.
content::RenderFrameHost* OpenApp(const webapps::AppId& app_id,
```
IWAsNegativeCaseIWAHasNoCredentialsGetPolicySet) {"Negative case" (here, and in other places) seems to be some kind of a shorthand for "WebAuthn doesn't work" or something like that. I think future readers will struggle understand the negative of _what_ exactly is implied here. We should be more descriptive and optimize for folks trying to understand the code/test who don't have all the current context in their head anymore (i.e., most likely ourselves in like 2-3 months)
This seems to test that WebAuthn returns a NotAllowedError if an IWA tries to use it without the permission policy, right? If so, I think a good name would be something like `WebAuthnIWABrowserTest_MissingPermissionsPolicyYieldsNotAllowedError`.
// Test that WebAuthn works inside of IWAs (call to navigator.credentials.get
// allowed and processed). Negative case, permission kPublicKeyCredentialsGetOlga KorokhinaIt appears to test the opposite, i.e. that the get() call is *not* allowed.
Martin KreichgauerI added details on this in a second sentence. Let me re-phrase then.
How about
```suggestion
// Test that invoking WebAuthn inside an IWA is rejected with a NotAllowedError, if the IWA manifest lacks the necessary permissions policy.
```
(Also, minor nit, but ideally these comments describing the test go into the line immediately prior to the test definition (i.e., before the IN_PROC_BROWSER_TEST_F(...) or whatever))
// Allow IWA to create and get creds, without permission to createOlga KorokhinaThis comment doesn't appear to match the code.
Martin KreichgauerRemoved comment and adding permission, updated comments, thank you
I still don't understand what this means. Doesn't the previous comment say that the point of the test is to assert that if we _don't_ add the permission, n.c.get() will fail?
IWAsNegativeCasePolicySetPrefsMissingNoRemoteOriginOverrideExtension) {Same comment about test naming here.
// Test that WebAuthn does not work inside of IWAs (call to
// navigator.credentials.get not allowed and processed). Negative local case,
// remoteDesktopClientOverride not used, prefs and affiliation not checked.
// Behavior different from Chrome Extensions.It's not really clear to me what this is testing exactly. The comment names a number of things; so if this test was failing, what would be going wrong?
The only assertion in this test is that the WebAuthn get() call fails with a SecurityError. So I think what this is supposed to test is that without the extension, the app can't make this assertion. If that's what you're trying to test, you should probably allow the app to use the extension via the policy; and then, in a separate test, verify that the extension can't use the extension if the policy doesn't allow it.
// The 'publickey-credentials-create' feature is not enabled in
// this document. Permissions Policy may be used to delegate WebThe opposite seems to be true. You add the permission policy for public-key-credentials-create below, and enable it.
IWAsNegativeCasePolicyAndPrefsSetNoRemoteOriginOverrideExtensionSameRp_id) {Same comment about test naming.
// The 'publickey-credentials-create' feature is not enabled in
// this document. Permissions Policy may be used to delegate WebThe opposite seems to be true.
// this document. Permissions Policy may be used to delegate Web
// Authentication capabilities to cross-origin child frames."'I don't see how cross-origin child frames are relevant in this test.
static constexpr char kMakeCredentialCrossDomainIWA[] = R"((() => {It's odd that this is declared inline, while the GetAssertion script is declared up top. Move to l47?
// Credentials created OKThese comments seem superfluous.
// `OriginAllowedToMakeWebAuthnRequests()` must have been called before.
// IWAs can only use WebAuthn in VDI sessions with override extension
if (caller_origin.scheme() == webapps::kIsolatedAppScheme) {
return false;
}I think this should be called after OriginAllowedToMakeWebAuthnRequests() (which was a DCHECK before, but now is an explicit test). The comment needs updating.
constexpr char kIWAId[] =
"aerugqztij5biqquuk3mfwpsaibuegaqcitgfchwuosuofdjabzqaaic";
static const std::string kIWAOrigin =
std::string(kIWAScheme) + "://" + kIWAId;You don't actually use the intermediate variable. This could be simplified to
```suggestion
constexpr char[] kIWAOrigin =
"iwa://aerugqztij5biqquuk3mfwpsaibuegaqcitgfchwuosuofdjabzqaaic";
```
and moved to the top of the file.
device::CredentialType::kPublicKey, std::move(credential_id));
GetAssertionResult result = AuthenticatorGetAssertion(std::move(options));Please fix this WARNING reported by ClangTidy: check: bugprone-use-after-move
A `move` operation occurred here, which caused "...
check: bugprone-use-after-move
A `move` operation occurred here, which caused "'credential_id' used after it was moved" at content/browser/webauth/authenticator_impl_unittest.cc:2201 (https://clang.llvm.org/extra/clang-tidy/checks/bugprone/use-after-move.html)
(Note: You can add `Skip-Clang-Tidy-Checks: bugprone-use-after-move` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
// IWAs if not using remoteDesktopClientOverride can assert any rp_idWhy would we need to override it like this in tests? That doesn't really reflect what we do in the production class, right?
Related details
- Olga Korokhina
Olga Korokhina (Gerrit)
Olga Korokhina added 21 comments![]( "Open in Gerrit")
Do you need this include?
No, thank you, we do not need it, removed.
base::ListValue list = base::ListValue().Append(kTestIsolatedAppOrigin);Nit: I think this could reasonably be inlined to the SetList() call.
From which package? I failed to find a relevant usage example, please point me to it, thank you.
base::ListValue list = base::ListValue().Append(kExampleOrigin);Olga KorokhinaSame here, and throughout.
Please see comment above, I failed to find the example, please provide.
#include "chrome/browser/web_applications/web_app_command_scheduler.h"Do you need this include?
Yes, for the line around 299 this file befow.
scoped_feature_list_.InitWithFeatures(
{device::kWebAuthnIWARemoteDesktopAllowedOriginsPolicy,
features::kIsolatedWebApps},
{});Please move this into a separate fixture that inherits from this one, and move the new tests there.
Done
content::RenderFrameHost* OpenApp(const webapps::AppId& app_id,Please add a comment.
```suggestion
// Launches the IWA with the given ID and returns its main frame.
content::RenderFrameHost* OpenApp(const webapps::AppId& app_id,
```
Added, thank you.
IWAsNegativeCaseIWAHasNoCredentialsGetPolicySet) {"Negative case" (here, and in other places) seems to be some kind of a shorthand for "WebAuthn doesn't work" or something like that. I think future readers will struggle understand the negative of _what_ exactly is implied here. We should be more descriptive and optimize for folks trying to understand the code/test who don't have all the current context in their head anymore (i.e., most likely ourselves in like 2-3 months)
This seems to test that WebAuthn returns a NotAllowedError if an IWA tries to use it without the permission policy, right? If so, I think a good name would be something like `WebAuthnIWABrowserTest_MissingPermissionsPolicyYieldsNotAllowedError`.
Naming improved, thank you.
// Test that WebAuthn works inside of IWAs (call to navigator.credentials.get
// allowed and processed). Negative case, permission kPublicKeyCredentialsGetOlga KorokhinaIt appears to test the opposite, i.e. that the get() call is *not* allowed.
Martin KreichgauerI added details on this in a second sentence. Let me re-phrase then.
How about
```suggestion
// Test that invoking WebAuthn inside an IWA is rejected with a NotAllowedError, if the IWA manifest lacks the necessary permissions policy.
```(Also, minor nit, but ideally these comments describing the test go into the line immediately prior to the test definition (i.e., before the IN_PROC_BROWSER_TEST_F(...) or whatever))
Addressed the comment change, thank you.
// Allow IWA to create and get creds, without permission to createOlga KorokhinaThis comment doesn't appear to match the code.
Martin KreichgauerRemoved comment and adding permission, updated comments, thank you
I still don't understand what this means. Doesn't the previous comment say that the point of the test is to assert that if we _don't_ add the permission, n.c.get() will fail?
We need to grant both create() and get() permissions. Adjusted the comment. We have now three tests for non-VDI case, all three fail: first fails with 'NotAllowedError' because of missing permissions on create and get in manifest. Second and third have manifest set up and differ in prefs values (emulates the WebAuthenticationRemoteDesktopAllowedOrigins policy-set values) set or not, both cases fails with security error because of no remoteDesktopClientOverride extension used. All three these cases are 'local', different 'level' or application readiness for WebAuthn usage, if I am allowed to say so. Reworked the naming and comments after moved three IWA-local-cases into dedicated class, please have a look if clearer. Thank you!
IWAsNegativeCasePolicySetPrefsMissingNoRemoteOriginOverrideExtension) {Same comment about test naming here.
Addressed, hope it is OK now, thank you.
// Test that WebAuthn does not work inside of IWAs (call to
// navigator.credentials.get not allowed and processed). Negative local case,
// remoteDesktopClientOverride not used, prefs and affiliation not checked.
// Behavior different from Chrome Extensions.It's not really clear to me what this is testing exactly. The comment names a number of things; so if this test was failing, what would be going wrong?
The only assertion in this test is that the WebAuthn get() call fails with a SecurityError. So I think what this is supposed to test is that without the extension, the app can't make this assertion. If that's what you're trying to test, you should probably allow the app to use the extension via the policy; and then, in a separate test, verify that the extension can't use the extension if the policy doesn't allow it.
please see comment above with differences between three test cases explanation. Comment addressed, thank you!
// The 'publickey-credentials-create' feature is not enabled in
// this document. Permissions Policy may be used to delegate WebThe opposite seems to be true. You add the permission policy for public-key-credentials-create below, and enable it.
This is the actual body of an error ("error NotAllowedError:
The 'publickey-credentials-create' feature is not enabled in this document. Permissions Policy may be used to delegate Web Authentication capabilities to cross-origin child frames.") which we get if we do not grant the create permission.
You're right, we not getting it in this case - because of we grant the create permission, error here is to explain the reason why we need it.
IWAsNegativeCasePolicyAndPrefsSetNoRemoteOriginOverrideExtensionSameRp_id) {Same comment about test naming.
Addressed, thank you.
// The 'publickey-credentials-create' feature is not enabled in
// this document. Permissions Policy may be used to delegate WebThe opposite seems to be true.
Yes, see comment above - probably it is hard to parse: this is the body of the error which we are not getting because of we set the create permission.
// this document. Permissions Policy may be used to delegate Web
// Authentication capabilities to cross-origin child frames."'I don't see how cross-origin child frames are relevant in this test.
Sorry if it is hard to parse it. This is a quote of an error body which we get of not set the create permission - "..."
static constexpr char kMakeCredentialCrossDomainIWA[] = R"((() => {It's odd that this is declared inline, while the GetAssertion script is declared up top. Move to l47?
Moved up, thank you.
// Credentials created OKOlga KorokhinaThese comments seem superfluous.
Removed, thank you
// `OriginAllowedToMakeWebAuthnRequests()` must have been called before.
// IWAs can only use WebAuthn in VDI sessions with override extension
if (caller_origin.scheme() == webapps::kIsolatedAppScheme) {
return false;
}I think this should be called after OriginAllowedToMakeWebAuthnRequests() (which was a DCHECK before, but now is an explicit test). The comment needs updating.
I decided to exit a bit earlier in this case, but I don't have a strong opinion, so moved below. Comment updated, thank you.
constexpr char kIWAId[] =
"aerugqztij5biqquuk3mfwpsaibuegaqcitgfchwuosuofdjabzqaaic";
static const std::string kIWAOrigin =
std::string(kIWAScheme) + "://" + kIWAId;You don't actually use the intermediate variable. This could be simplified to
```suggestion
constexpr char[] kIWAOrigin =
"iwa://aerugqztij5biqquuk3mfwpsaibuegaqcitgfchwuosuofdjabzqaaic";
```and moved to the top of the file.
Right, applied changes, thank you.
device::CredentialType::kPublicKey, std::move(credential_id));
GetAssertionResult result = AuthenticatorGetAssertion(std::move(options));Please fix this WARNING reported by ClangTidy: check: bugprone-use-after-move
A `move` operation occurred here, which caused "...
check: bugprone-use-after-move
A `move` operation occurred here, which caused "'credential_id' used after it was moved" at content/browser/webauth/authenticator_impl_unittest.cc:2201 (https://clang.llvm.org/extra/clang-tidy/checks/bugprone/use-after-move.html)
(Note: You can add `Skip-Clang-Tidy-Checks: bugprone-use-after-move` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
I guess we don't need move() here, passing the credentials_id itself should fix it, let me try.
// IWAs if not using remoteDesktopClientOverride can assert any rp_idWhy would we need to override it like this in tests? That doesn't really reflect what we do in the production class, right?
I believe it is a left-over from a time when I assumed local IWAs can assert matching rp_ids, do not remember, to be honest. Let me see what if we don't do this.
Related details
- Andrew Rayskiy
- Martin Kreichgauer
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kreichgauer (Gerrit)
Martin Kreichgauer added 1 comment![]( "Open in Gerrit")
(Please make the tests pass before adding me back to the attention set)
Related details
- Andrew Rayskiy
- Olga Korokhina