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
Olga Korokhina (Gerrit)
Olga Korokhina added 1 comment![]( "Open in Gerrit")
(Please make the tests pass before adding me back to the attention set)
Hey, tests are fine, was fine when I submit the reply. Probably some issue with Gerrit?
Related details
- Andrew Rayskiy
- Martin Kreichgauer
Olga Korokhina (Gerrit)
Olga Korokhina added 1 comment![]( "Open in Gerrit")
ASSERT_TRUE(profile());Olga KorokhinaWhat is this asserting? This seems guaranteed to be true.
Olga KorokhinaAsserting that for non-CrOS active profile is created (browser()->profile()) in parent test class. That was the issue with having these tests in chrome/browser/webauthn/chrome_webauthn_iwa_remote_desktop_override_browsertest.cc - setting affiliation conflicted with default user/profile provided by CertVerifierBrowserTest. So I moved this code here. Do you think this assertion doesn make sense?
Done
Related details
- Andrew Rayskiy
- Martin Kreichgauer
Code-Coverage
Code-Owners
Code-Review
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
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kreichgauer (Gerrit)
Martin Kreichgauer added 14 comments![]( "Open in Gerrit")
The actual feature code looks ok now, I think.
base::ListValue list = base::ListValue().Append(kTestIsolatedAppOrigin);Olga KorokhinaNit: 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.
Instead of creating a temporary variable, and then moving it immediately after, you can directly construct it in the existing SetList() method call. There a numerous examples in other tests in this file.
```
prefs->SetList(webauthn::pref_names::kRemoteDesktopAllowedOrigins,
base::ListValue().Append(kTestIsolatedAppOrigin));
```
base::ListValue list = base::ListValue().Append(kExampleOrigin);Olga KorokhinaSame here, and throughout.
Please see comment above, I failed to find the example, please provide.
See above. (Try not to resolve comment threads if you're expecting a follow-up from me, please.)
scoped_feature_list_.InitWithFeatures(
{device::kWebAuthnIWARemoteDesktopAllowedOriginsPolicy,
features::kIsolatedWebApps},
{});Olga KorokhinaPlease move this into a separate fixture that inherits from this one, and move the new tests there.
Done
I don't think that test suite warrants a new file though. WebAuthnBrowserTest already has several other suites inheriting from it, and all of them live in this file. So it makes sense for WebAuthnIWABrowserTest to just be defined here as well.
If we wanted to have WebAuthnBrowserTestBase or something live in its own .h./.cc, and then have multiple browsertest files reuse it, I could see a point. But as it is, this test code organization is a bit unorthodox.
Moving these into a separate file also makes it really difficult to follow the numerous existing comment threads on this file, which isn't ideal.
// The 'publickey-credentials-create' feature is not enabled in
// this document. Permissions Policy may be used to delegate WebOlga KorokhinaThe 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.
I see. But I still don't understand why you're adding the `kPublicKeyCredentialsCreate` permissions policy here in the first place. It doesn't do anything, right? The only call you're making is a get() call, so the permission policy for get is all you should need.
Can we just delete the first AddPermissionsPolicy() call here, along with the comment?
// The 'publickey-credentials-create' feature is not enabled in
// this document. Permissions Policy may be used to delegate WebOlga KorokhinaThe opposite seems to be true.
Martin KreichgauerYes, 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.
Ah, I see my error, thanks.
Reproducing the error message here verbatim isn't useful, I'm afraid. Please change this to a succinct comment explaining why you need to add permissions policies here. E.g.
`// Add permissions policies for creating and asserting WebAuthn credentials which aren't enabled by default in IWAs.`
// Credentials got OKThis comment is superfluous.
// Credentials created but not allowed to use remoteDesktopClientOverrideThat's not correct, there wasn't any credential created.
// Credentials got but not allowed to use remoteDesktopClientOverride due toThat's not correct, there were no credentials retrieved.
// Credentials got OKThis comment is superfluous.
caller_origin.scheme() != webapps::kIsolatedAppScheme) {Can you please unit test this change? There are existing tests in //components/webauthn/core/browser/webauthn_security_utils.cc.
// IWAs can only use WebAuthn in VDI sessions with override extension```suggestion
// IWAs can only claim RP IDs via the remoteDesktopClientOverride extension
```
if (caller_origin.scheme() == webapps::kIsolatedAppScheme) {
return false;
}Can you please unit test this change? There are existing tests in //components/webauthn/core/browser/webauthn_security_utils.cc.
// Enabled by default.```suggestion
// Enabled by default in M149. Remove in or after M152.
```
Related details
- 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 13 comments![]( "Open in Gerrit")
base::ListValue list = base::ListValue().Append(kTestIsolatedAppOrigin);Olga KorokhinaNit: I think this could reasonably be inlined to the SetList() call.
Martin KreichgauerFrom which package? I failed to find a relevant usage example, please point me to it, thank you.
Instead of creating a temporary variable, and then moving it immediately after, you can directly construct it in the existing SetList() method call. There a numerous examples in other tests in this file.
```
prefs->SetList(webauthn::pref_names::kRemoteDesktopAllowedOrigins,
base::ListValue().Append(kTestIsolatedAppOrigin));
```
Oh, got it, thank you!
base::ListValue list = base::ListValue().Append(kExampleOrigin);Olga KorokhinaSame here, and throughout.
Martin KreichgauerPlease see comment above, I failed to find the example, please provide.
See above. (Try not to resolve comment threads if you're expecting a follow-up from me, please.)
Meant to have less open comments, I remember there are two places with same change needed. Thank you, code simplified.
scoped_feature_list_.InitWithFeatures(
{device::kWebAuthnIWARemoteDesktopAllowedOriginsPolicy,
features::kIsolatedWebApps},
{});Olga KorokhinaPlease move this into a separate fixture that inherits from this one, and move the new tests there.
Martin KreichgauerDone
I don't think that test suite warrants a new file though. WebAuthnBrowserTest already has several other suites inheriting from it, and all of them live in this file. So it makes sense for WebAuthnIWABrowserTest to just be defined here as well.
If we wanted to have WebAuthnBrowserTestBase or something live in its own .h./.cc, and then have multiple browsertest files reuse it, I could see a point. But as it is, this test code organization is a bit unorthodox.
Moving these into a separate file also makes it really difficult to follow the numerous existing comment threads on this file, which isn't ideal.
Than I misunderstood your initial comment. Setting scoped_feature_list_ is only possible in constructor and not needed for other then IWA tests indeed. I am fully agree all tests (old and new IWA-specific) should be in one file, but just fail to understand what code reorganization you propose under "Please move this into a separate fixture that inherits from this one, and move the new tests there.". Please give me more clues?
// The 'publickey-credentials-create' feature is not enabled in
// this document. Permissions Policy may be used to delegate WebOlga KorokhinaThe opposite seems to be true. You add the permission policy for public-key-credentials-create below, and enable it.
Martin KreichgauerThis 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.
I see. But I still don't understand why you're adding the `kPublicKeyCredentialsCreate` permissions policy here in the first place. It doesn't do anything, right? The only call you're making is a get() call, so the permission policy for get is all you should need.
Can we just delete the first AddPermissionsPolicy() call here, along with the comment?
Actually for both similar cases in this file we do not need to set 'create' permission,setting it only needed for WebAuthnAffiliationCheck/WebAuthnIWARemoteDesktopOverrideBrowserTest
// The 'publickey-credentials-create' feature is not enabled in
// this document. Permissions Policy may be used to delegate WebOlga KorokhinaThe opposite seems to be true.
Martin KreichgauerYes, 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.
Ah, I see my error, thanks.
Reproducing the error message here verbatim isn't useful, I'm afraid. Please change this to a succinct comment explaining why you need to add permissions policies here. E.g.
`// Add permissions policies for creating and asserting WebAuthn credentials which aren't enabled by default in IWAs.`
Actually in this test we do not need 'create' permission, it is needed in WebAuthnAffiliationCheck/WebAuthnIWARemoteDesktopOverrideBrowserTest only.
// Credentials got OKOlga KorokhinaThis comment is superfluous.
removed here and same below, thank you.
// Credentials created but not allowed to use remoteDesktopClientOverrideThat's not correct, there wasn't any credential created.
True, left-over from previous state of a code, removed. Thank you,
// Credentials got but not allowed to use remoteDesktopClientOverride due toThat's not correct, there were no credentials retrieved.
Adjusted, thank you
// Credentials got OKOlga KorokhinaThis comment is superfluous.
removed
caller_origin.scheme() != webapps::kIsolatedAppScheme) {Can you please unit test this change? There are existing tests in //components/webauthn/core/browser/webauthn_security_utils.cc.
Added test case to components/webauthn/core/browser/webauthn_security_utils_unittest.cc, according to it check fails into kInvalidDomain (fails into !network::IsUrlPotentiallyTrustworthy(caller_origin.GetURL()) below in line 31).
// IWAs can only use WebAuthn in VDI sessions with override extension```suggestion
// IWAs can only claim RP IDs via the remoteDesktopClientOverride extension
```
Applied edit, thank you
if (caller_origin.scheme() == webapps::kIsolatedAppScheme) {
return false;
}Can you please unit test this change? There are existing tests in //components/webauthn/core/browser/webauthn_security_utils.cc.
Yes, added three cases for it in WebAuthnSecurityUtilsTest.OriginIsAllowedToClaimRelyingPartyId
```suggestion
// Enabled by default in M149. Remove in or after M152.
```
- 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 7 comments![]( "Open in Gerrit")
scoped_feature_list_.InitWithFeatures(
{device::kWebAuthnIWARemoteDesktopAllowedOriginsPolicy,
features::kIsolatedWebApps},
{});Olga KorokhinaPlease move this into a separate fixture that inherits from this one, and move the new tests there.
Martin KreichgauerDone
Olga KorokhinaI don't think that test suite warrants a new file though. WebAuthnBrowserTest already has several other suites inheriting from it, and all of them live in this file. So it makes sense for WebAuthnIWABrowserTest to just be defined here as well.
If we wanted to have WebAuthnBrowserTestBase or something live in its own .h./.cc, and then have multiple browsertest files reuse it, I could see a point. But as it is, this test code organization is a bit unorthodox.
Moving these into a separate file also makes it really difficult to follow the numerous existing comment threads on this file, which isn't ideal.
Than I misunderstood your initial comment. Setting scoped_feature_list_ is only possible in constructor and not needed for other then IWA tests indeed. I am fully agree all tests (old and new IWA-specific) should be in one file, but just fail to understand what code reorganization you propose under "Please move this into a separate fixture that inherits from this one, and move the new tests there.". Please give me more clues?
The WebAuthnIWABrowserTest test suite should be defined at the bottom of this file, rather than in a separate file, same for its individual IN_PROC_BROWSER_TEST_F test cases. (You'll notice there are multiple other test suite classes inheriting from WebAuthnIWABrowserTest already in this file.)
Once you have done that, you should also undo moving the definition for WebAuthnBrowserTest into the new header file, and just revert it back to what it was before.
// Allow IWA to create and get credsPlease update the comment to match the code.
// Allow IWA to create and get credsPlease update the comment to match the code.
caller_origin.scheme() != webapps::kIsolatedAppScheme) {Olga KorokhinaCan you please unit test this change? There are existing tests in //components/webauthn/core/browser/webauthn_security_utils.cc.
Added test case to components/webauthn/core/browser/webauthn_security_utils_unittest.cc, according to it check fails into kInvalidDomain (fails into !network::IsUrlPotentiallyTrustworthy(caller_origin.GetURL()) below in line 31).
Ah, right, those tests should only cover HTTP(S) origins. You'll want to fix that, probably by removing this check and adding something like this above it instead.
```
if (caller_origin.scheme() == webapps::kIsolatedAppScheme) {
return ValidationStatus::kSuccess;
}
```
// {"https://example.com", ValidationStatus::kSuccess},These shouldn't be commented out.
// IWAs can not claim any rp_ids ourside or VDI sessionsI don't know what this means. Maybe
```suggestion
// IWAs can not claim any RP ID except via remoteDesktopClientOverride.
```
{"isolated-app://"Please add one more test case here for the IWA claiming an ordinary domain name (like "example.com"). This should fail.
Related details
- Andrew Rayskiy
- Olga Korokhina
Olga Korokhina (Gerrit)
Olga Korokhina added 8 comments![]( "Open in Gerrit")
scoped_feature_list_.InitWithFeatures(
{device::kWebAuthnIWARemoteDesktopAllowedOriginsPolicy,
features::kIsolatedWebApps},
{});Olga KorokhinaPlease move this into a separate fixture that inherits from this one, and move the new tests there.
Martin KreichgauerDone
Olga KorokhinaI don't think that test suite warrants a new file though. WebAuthnBrowserTest already has several other suites inheriting from it, and all of them live in this file. So it makes sense for WebAuthnIWABrowserTest to just be defined here as well.
If we wanted to have WebAuthnBrowserTestBase or something live in its own .h./.cc, and then have multiple browsertest files reuse it, I could see a point. But as it is, this test code organization is a bit unorthodox.
Moving these into a separate file also makes it really difficult to follow the numerous existing comment threads on this file, which isn't ideal.
Martin KreichgauerThan I misunderstood your initial comment. Setting scoped_feature_list_ is only possible in constructor and not needed for other then IWA tests indeed. I am fully agree all tests (old and new IWA-specific) should be in one file, but just fail to understand what code reorganization you propose under "Please move this into a separate fixture that inherits from this one, and move the new tests there.". Please give me more clues?
The WebAuthnIWABrowserTest test suite should be defined at the bottom of this file, rather than in a separate file, same for its individual IN_PROC_BROWSER_TEST_F test cases. (You'll notice there are multiple other test suite classes inheriting from WebAuthnIWABrowserTest already in this file.)
Once you have done that, you should also undo moving the definition for WebAuthnBrowserTest into the new header file, and just revert it back to what it was before.
Oh, got it, done.
Please update the comment to match the code.
Done, sorry!
Please update the comment to match the code.
Adjusted, thank you
caller_origin.scheme() != webapps::kIsolatedAppScheme) {Olga KorokhinaCan you please unit test this change? There are existing tests in //components/webauthn/core/browser/webauthn_security_utils.cc.
Martin KreichgauerAdded test case to components/webauthn/core/browser/webauthn_security_utils_unittest.cc, according to it check fails into kInvalidDomain (fails into !network::IsUrlPotentiallyTrustworthy(caller_origin.GetURL()) below in line 31).
Ah, right, those tests should only cover HTTP(S) origins. You'll want to fix that, probably by removing this check and adding something like this above it instead.
```
if (caller_origin.scheme() == webapps::kIsolatedAppScheme) {
return ValidationStatus::kSuccess;
}
```
Added check above in same method, thank you.
if (caller_origin.scheme() == webapps::kIsolatedAppScheme) {
return false;
}Olga KorokhinaCan you please unit test this change? There are existing tests in //components/webauthn/core/browser/webauthn_security_utils.cc.
Olga KorokhinaYes, added three cases for it in WebAuthnSecurityUtilsTest.OriginIsAllowedToClaimRelyingPartyId
Done.
// {"https://example.com", ValidationStatus::kSuccess},These shouldn't be commented out.
Oh, sorry, committed my 'debugging' version. Uncommented!
// IWAs can not claim any rp_ids ourside or VDI sessionsI don't know what this means. Maybe
```suggestion
// IWAs can not claim any RP ID except via remoteDesktopClientOverride.
```
Accepted, thank you.
Please add one more test case here for the IWA claiming an ordinary domain name (like "example.com"). This should fail.
- Andrew Rayskiy
- Martin Kreichgauer
Code-Coverage
Code-Owners
Code-Review
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
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kreichgauer (Gerrit)
Martin Kreichgauer added 5 comments![]( "Open in Gerrit")
// Isolated Web Apps can use RemoteDesktopClientOverride if the feature flag```suggestion
// Isolated Web Apps may be configured to use the
// remoteDesktopClientOverride extension only if the feature flag is
// enabled.
```
I.e., the feature flag doesn't allow them to use it automatically. (But the code is correct.)
class ChallengeUrlBrowserTest : public WebAuthnBrowserTest {This test has been deleted at HEAD recently, but shouldn't be deleted as part of this CL. Please rebase.
// IWAs scheme should be recognized as valid, separated case because
// schema is not trust-worthy and fails into 'invalid domain' case```suggestion
// IWAs can make WebAuthn requests but generally aren't allowed to claim
// any RP IDs, except via the remoteDesktopClientOverride extension.
```
caller_origin.scheme() != url::kHttpsScheme &&
caller_origin.scheme() != webapps::kIsolatedAppScheme) {The additional test is now superfluous.
```suggestion
caller_origin.scheme() != url::kHttpsScheme) {
```
// IWAs schema allowed to proceed with WebAuthn calls only if
// remoteDesktopClientOverride set, happens not in this methodNit: Delete this comment
Related details
- 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 5 comments![]( "Open in Gerrit")
// Isolated Web Apps can use RemoteDesktopClientOverride if the feature flag```suggestion
// Isolated Web Apps may be configured to use the
// remoteDesktopClientOverride extension only if the feature flag is
// enabled.
```I.e., the feature flag doesn't allow them to use it automatically. (But the code is correct.)
Changed, thank you
class ChallengeUrlBrowserTest : public WebAuthnBrowserTest {This test has been deleted at HEAD recently, but shouldn't be deleted as part of this CL. Please rebase.
Done, thank you
// IWAs scheme should be recognized as valid, separated case because
// schema is not trust-worthy and fails into 'invalid domain' case```suggestion
// IWAs can make WebAuthn requests but generally aren't allowed to claim
// any RP IDs, except via the remoteDesktopClientOverride extension.
```
Changed, thank you
caller_origin.scheme() != url::kHttpsScheme &&
caller_origin.scheme() != webapps::kIsolatedAppScheme) {The additional test is now superfluous.
```suggestion
caller_origin.scheme() != url::kHttpsScheme) {
```
Indeed, sorry!
// IWAs schema allowed to proceed with WebAuthn calls only if
// remoteDesktopClientOverride set, happens not in this methodOlga KorokhinaNit: Delete this comment
removed, thank you
Related details
- Andrew Rayskiy
- Martin Kreichgauer
Code-Coverage
Code-Owners
Code-Review
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
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kreichgauer (Gerrit)
Martin Kreichgauer added 4 comments![]( "Open in Gerrit")
Allow IWAs to call navigator.credentials methods```suggestion
Allow IWAs to call WebAuthn
```
Change adds IWA schema for allowed to proceed with WebAuthn VDI flow calls, introduces dedicated flag feature::kWebAuthnIWARemoteDesktopAllowedOriginsPolicy, check if IWA caller origin listed in webauthn::pref_names::kRemoteDesktopAllowedOrigins happens in existing code.```suggestion
This exposes the WebAuthn API to calls from IWA origins. IWAs by default won't be able to claim any RP IDs, unless they're using the remoteDestkopClientOverride extension to act on behalf of another web origin. Access to this extension must be enabled for individual IWA origins via the webauthn.remote_desktop_allowed_origins enterprise policy.
Flag guarded by the device::kWebAuthnIWARemoteDesktopAllowedOriginsPolicy feature flag.
```
Enabled-by-default-reason: WebAuthn for IWAs should be enabled
from the box```suggestion
Enabled-by-default-reason: Killswitch
```
Fuchsia-Binary-Size: Adds ~16KB uncompressed to support WebAuthn for Isolated Web Apps, which is enabled by default.I find it unlikely that this change is really adding that much to the Fuchsia binary. Try removing this please.
Related details
- 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. |