Add user affiliation on CrOS check into logic checking if WebAuthn call allowed by policy [chromium/src : main]
Olga Korokhina (Gerrit)
Olga Korokhina added 2 comments![]( "Open in Gerrit")
// served from Admin Panel on W/M/L yet it is still possible to
// set it via custom profile JSONOlga KorokhinaThis would mean whoever sets the policy can run code on the device. That's an acceptable bar for us for this policy.
Olga KorokhinaCopy, so we only check affiliation on CrOS, no additional actions on other platforms expected.
Done
if (!isUserPlatformDependantAffiliated(profile)) {Martin Kreichgauer@mart...@google.com @nati...@google.com
Gentlemen I'd like to get you opinion on this change, if this is a correct place to do the affiliation check, and what do we do on other platforms - immediately reject or proceed with further checks as it is now. Do we need to check a version of a CrOS as well?
Olga KorokhinaThis looks like a good place for this check, yes.
Since the method doesn't actually check affiliation on ChromeOS, we should just not define it for !OS_CHROMEOS. (Or just inline the entire check into this method, it's 4 lines.)
Do we need to check a version of a CrOS as well?
I'm not sure I understand what purpose that would serve. Could you expand?
Olga KorokhinaMy question was do we need to preserve the existing behavior for older version of Chrome but I see this doesn't make sense indeed - this code will simply not be there.
So we only do affiliation check on ChromeOS, effective immediately with this code merge. Thank you!
Done
Related details
- Andrii Natiahlyi
- Martin Kreichgauer
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 10 comments![]( "Open in Gerrit")
// Necessary includes for user affiliation set upWe typically don't comment includes
ChromeRenderViewHostTestHarness::SetUp(); // Call base SetUpThis comment is superfluous.
fake_user_manager_ = user_manager.get();Does `scoped_user_manager_.Get()` return the same value?
ChromeRenderViewHostTestHarness::TearDown(); // Call base TearDownSame here.
nullptr; // = raw_ptr<ash::FakeChromeUserManager>();Spurious comment
if (!user) {
return;
}Should it be allowed to call this method with effectively an unknown user ID? Wouldn't this just hide a potential error in the test?
if (fake_user_manager_->IsUserLoggedIn()) {
if (fake_user_manager_->GetActiveUser() == user) {Nit: These two if statements can be combined.
if (fake_user_manager_->IsUserLoggedIn()) {
if (fake_user_manager_->GetActiveUser() == user) {Similar question as above: Is it a good idea to allow a test to attempt to log out a user that isn't logged in?
base::ScopedAllowBlockingForTesting allow_blocking;Can you comment why that's needed?
// Both the origin specified by the command-line switch and the origin in the
// allowed origins pref should be allowed. Affiliation check happening on
// CrOS needs to be taken into account as well.
#if !BUILDFLAG(IS_CHROMEOS)
EXPECT_TRUE(delegate.OriginMayUseRemoteDesktopClientOverride(
browser_context(), url::Origin::Create(GURL(kExampleOrigin))));
EXPECT_TRUE(delegate.OriginMayUseRemoteDesktopClientOverride(
browser_context(), url::Origin::Create(GURL(kAnotherExampleOrigin))));
#elif BUILDFLAG(IS_CHROMEOS)
// False when user is not affiliated
EXPECT_FALSE(delegate.OriginMayUseRemoteDesktopClientOverride(
browser_context(), url::Origin::Create(GURL(kExampleOrigin))));
EXPECT_FALSE(delegate.OriginMayUseRemoteDesktopClientOverride(
browser_context(), url::Origin::Create(GURL(kAnotherExampleOrigin))));
// set user as affiliated for example.com
SetupUserAffiliation(true, "te...@example.com");
// True when user is affiliated for same origin
EXPECT_TRUE(delegate.OriginMayUseRemoteDesktopClientOverride(
browser_context(), url::Origin::Create(GURL(kExampleOrigin))));
EXPECT_TRUE(delegate.OriginMayUseRemoteDesktopClientOverride(
browser_context(), url::Origin::Create(GURL(kAnotherExampleOrigin))));
// Log out user
LogOutUser("te...@example.com");
EXPECT_FALSE(delegate.OriginMayUseRemoteDesktopClientOverride(
browser_context(), url::Origin::Create(GURL(kExampleOrigin))));
EXPECT_FALSE(delegate.OriginMayUseRemoteDesktopClientOverride(
browser_context(), url::Origin::Create(GURL(kAnotherExampleOrigin))));
// set user as affiliated for another.example.com
SetupUserAffiliation(true, "te...@another.example.com");
#endifI think relying on ifdefs in the test body to test different scenarios based on the OS hurts readability. Would the following work:
- In the test suite, set up the user affiliation for ChromeOS.
- In dedicated tests (perhaps an extra suite, perhaps a single test in the same suite), explicitly test the additional behavior for users that aren't affiliate on ChromeOS.
Related details
- Andrii Natiahlyi
- 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 4 comments![]( "Open in Gerrit")
if (!user) {
return;
}Should it be allowed to call this method with effectively an unknown user ID? Wouldn't this just hide a potential error in the test?
would you recommend to throw an error in this case?
if (fake_user_manager_->IsUserLoggedIn()) {
if (fake_user_manager_->GetActiveUser() == user) {Similar question as above: Is it a good idea to allow a test to attempt to log out a user that isn't logged in?
Haven't thought about it this way, we use this method only on logged in user. We can throw an error above, as you suggested, and also here, if user is not logged in, but I see no harm really in not doing it: we log user out after checking is she is affiliated so - already logged in by design.
For the purity of isolated method, will throwing errors here improve it?
base::ScopedAllowBlockingForTesting allow_blocking;Can you comment why that's needed?
It is a left over from my experiments, you're rights, it is not needed here, thank you!
I implemented it this way because of number of code/test cases will be higher in this case. And we will probably need to change all existing cases names, but let me try. We can have behavior described above for the whole test class in a big comments section.
Related details
- Andrii Natiahlyi
- 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 3 comments![]( "Open in Gerrit")
if (!user) {
return;
}Olga KorokhinaShould it be allowed to call this method with effectively an unknown user ID? Wouldn't this just hide a potential error in the test?
would you recommend to throw an error in this case?
If it's an invariant in the code, you'd use a CHECK(). If it's a behavior of the code under test that you want to verify, ASSERT_* or EXPECT_* would be more appropriate and provide better diagnostic.
Since this is a utility method, and the thing you're unit testing isn't login itself, CHECK-ing would be fine. Or you could also return success as a bool (preferably with `[[nodiscard]]`) and `ASSERT_TRUE(LogoutUser(...))` in the test method.
if (fake_user_manager_->IsUserLoggedIn()) {
if (fake_user_manager_->GetActiveUser() == user) {Olga KorokhinaSimilar question as above: Is it a good idea to allow a test to attempt to log out a user that isn't logged in?
Haven't thought about it this way, we use this method only on logged in user. We can throw an error above, as you suggested, and also here, if user is not logged in, but I see no harm really in not doing it: we log user out after checking is she is affiliated so - already logged in by design.
For the purity of isolated method, will throwing errors here improve it?
I think making the tests robust against e.g. a typo in the username string passed to `LogOutUser()` is a small improvement, yes. It's not a big deal, but it could hide errors otherwise.
I'd say that having a larger of number of test cases that are each well defined and test a particular thing is generally preferred over having a single test method that asserts multiple behaviors. See e.g. http://go/tott/726.md
This way you also don't need to sprinkle `SetupUserAffiliation(true, "te...@example.com");` into all of the various test cases below. The purpose of that isn't straightforward to understand. If it'd happen once during SetUp(), you could just put a comment next to it, explaining why that's relevant on ChromeOS.
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 9 comments![]( "Open in Gerrit")
We typically don't comment includes
removed, thank you
ChromeRenderViewHostTestHarness::SetUp(); // Call base SetUpOlga KorokhinaThis comment is superfluous.
comment removed, thank you
Does `scoped_user_manager_.Get()` return the same value?
No, we can not replace it and we need both of them: scoped_user_manager_.Get() returns UserManager instance which is missing additional methods (AddUserWithAffiliationAndTypeAndProfile, AddUserWithAffiliation etc.) we need in a test, they are incapsulated in FakeUserManager.
How it works (AI generated content below, sorry!):
ash::FakeChromeUserManager *fake_user_manager_:
This is a raw pointer to the actual fake implementation of the UserManager. You need this pointer to call the specific methods of FakeChromeUserManager within your test setup functions (like SetupAshUser) to add users, log them in, set their affiliation status, etc. This is how you control the fake behavior.
std::unique_ptr<user_manager::ScopedUserManager> scoped_user_manager_:
The UserManager is a global singleton, accessed via user_manager::UserManager::Get(). Many parts of the Chrome browser code, including potentially the code being tested in ChromeWebAuthenticationDelegateBase or its dependencies like ash::ProfileHelper, will call UserManager::Get() to retrieve the current user manager instance.
ScopedUserManager is a test utility that takes ownership of a UserManager instance (in this case, your FakeChromeUserManager) and sets it as the global instance returned by UserManager::Get() for the lifetime of the ScopedUserManager object.
When scoped_user_manager_ is created in SetUp(), it injects your fake_user_manager_.
When scoped_user_manager_ is destroyed in TearDown(), it cleans up and restores the previous global UserManager instance, ensuring test isolation.
ChromeRenderViewHostTestHarness::TearDown(); // Call base TearDownOlga KorokhinaSame here.
comment removed, thank you
nullptr; // = raw_ptr<ash::FakeChromeUserManager>();Olga KorokhinaSpurious comment
removed, thank you
if (!user) {
return;
}Olga KorokhinaShould it be allowed to call this method with effectively an unknown user ID? Wouldn't this just hide a potential error in the test?
Martin Kreichgauerwould you recommend to throw an error in this case?
If it's an invariant in the code, you'd use a CHECK(). If it's a behavior of the code under test that you want to verify, ASSERT_* or EXPECT_* would be more appropriate and provide better diagnostic.
Since this is a utility method, and the thing you're unit testing isn't login itself, CHECK-ing would be fine. Or you could also return success as a bool (preferably with `[[nodiscard]]`) and `ASSERT_TRUE(LogoutUser(...))` in the test method.
Addressed, thank you
if (fake_user_manager_->IsUserLoggedIn()) {
if (fake_user_manager_->GetActiveUser() == user) {Olga KorokhinaNit: These two if statements can be combined.
Addressed, thank you
if (fake_user_manager_->IsUserLoggedIn()) {
if (fake_user_manager_->GetActiveUser() == user) {Olga KorokhinaSimilar question as above: Is it a good idea to allow a test to attempt to log out a user that isn't logged in?
Martin KreichgauerHaven't thought about it this way, we use this method only on logged in user. We can throw an error above, as you suggested, and also here, if user is not logged in, but I see no harm really in not doing it: we log user out after checking is she is affiliated so - already logged in by design.
For the purity of isolated method, will throwing errors here improve it?
I think making the tests robust against e.g. a typo in the username string passed to `LogOutUser()` is a small improvement, yes. It's not a big deal, but it could hide errors otherwise.
Copy, thank you, comment addressed
// Both the origin specified by the command-line switch and the origin in theCleaned up the other test cases and added one dedicated to CrOS to test behavior with and without affiliation. Thank you, comment addressed.
Related details
- 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. |
Olga Korokhina (Gerrit)
Olga Korokhina added 5 comments![]( "Open in Gerrit")
if (!user) {
return;
}Olga KorokhinaShould it be allowed to call this method with effectively an unknown user ID? Wouldn't this just hide a potential error in the test?
Martin Kreichgauerwould you recommend to throw an error in this case?
Olga KorokhinaIf it's an invariant in the code, you'd use a CHECK(). If it's a behavior of the code under test that you want to verify, ASSERT_* or EXPECT_* would be more appropriate and provide better diagnostic.
Since this is a utility method, and the thing you're unit testing isn't login itself, CHECK-ing would be fine. Or you could also return success as a bool (preferably with `[[nodiscard]]`) and `ASSERT_TRUE(LogoutUser(...))` in the test method.
Addressed, thank you
Marked as resolved.
if (fake_user_manager_->IsUserLoggedIn()) {
if (fake_user_manager_->GetActiveUser() == user) {Olga KorokhinaNit: These two if statements can be combined.
Addressed, thank you
Marked as resolved.
if (fake_user_manager_->IsUserLoggedIn()) {
if (fake_user_manager_->GetActiveUser() == user) {Olga KorokhinaSimilar question as above: Is it a good idea to allow a test to attempt to log out a user that isn't logged in?
Martin KreichgauerHaven't thought about it this way, we use this method only on logged in user. We can throw an error above, as you suggested, and also here, if user is not logged in, but I see no harm really in not doing it: we log user out after checking is she is affiliated so - already logged in by design.
For the purity of isolated method, will throwing errors here improve it?
Olga KorokhinaI think making the tests robust against e.g. a typo in the username string passed to `LogOutUser()` is a small improvement, yes. It's not a big deal, but it could hide errors otherwise.
Copy, thank you, comment addressed
Marked as resolved.
base::ScopedAllowBlockingForTesting allow_blocking;Olga KorokhinaCan you comment why that's needed?
It is a left over from my experiments, you're rights, it is not needed here, thank you!
Marked as resolved.
Olga KorokhinaI think relying on ifdefs in the test body to test different scenarios based on the OS hurts readability. Would the following work:
- In the test suite, set up the user affiliation for ChromeOS.
- In dedicated tests (perhaps an extra suite, perhaps a single test in the same suite), explicitly test the additional behavior for users that aren't affiliate on ChromeOS.
Martin KreichgauerI implemented it this way because of number of code/test cases will be higher in this case. And we will probably need to change all existing cases names, but let me try. We can have behavior described above for the whole test class in a big comments section.
Olga KorokhinaI'd say that having a larger of number of test cases that are each well defined and test a particular thing is generally preferred over having a single test method that asserts multiple behaviors. See e.g. http://go/tott/726.md
This way you also don't need to sprinkle `SetupUserAffiliation(true, "te...@example.com");` into all of the various test cases below. The purpose of that isn't straightforward to understand. If it'd happen once during SetUp(), you could just put a comment next to it, explaining why that's relevant on ChromeOS.
Cleaned up the other test cases and added one dedicated to CrOS to test behavior with and without affiliation. Thank you, comment addressed.
- Andrii Natiahlyi
- Martin Kreichgauer
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrii Natiahlyi (Gerrit)
Andrii Natiahlyi added 1 comment![]( "Open in Gerrit")
TEST_F(OriginMayUseRemoteDesktopClientOverrideTest,
AdditionalOriginSwitch_WithAllowedOriginsPolicy_ChromeOsNoAffiliation) {
// Version of a test case above but for non-affiliated user on ChromeOS
ChromeWebAuthenticationDelegateBase delegate;
base::test::ScopedCommandLine scoped_command_line;
scoped_command_line.GetProcessCommandLine()->AppendSwitchASCII(
webauthn::switches::kRemoteProxiedRequestsAllowedAdditionalOrigin,
kExampleOrigin);
// Initially, no origins should be allowed because the allowed origins pref
// hasn't been set yet.
EXPECT_FALSE(delegate.OriginMayUseRemoteDesktopClientOverride(
browser_context(), url::Origin::Create(GURL(kExampleOrigin))));
EXPECT_FALSE(delegate.OriginMayUseRemoteDesktopClientOverride(
browser_context(), url::Origin::Create(GURL(kAnotherExampleOrigin))));
// Set the allowed origins pref to include another origin.
PrefService* prefs =
Profile::FromBrowserContext(GetBrowserContext())->GetPrefs();
prefs->SetList(webauthn::pref_names::kRemoteDesktopAllowedOrigins,
base::ListValue().Append(kAnotherExampleOrigin));
// Both the origin specified by the command-line switch and the origin in the
// allowed origins pref should be allowed. Affiliation check happening on
// CrOS needs to be taken into account as well. On the test set up user
// affiliated with example.com domain, so with same origin we get trus
EXPECT_TRUE(delegate.OriginMayUseRemoteDesktopClientOverride(
browser_context(), url::Origin::Create(GURL(kExampleOrigin))));
EXPECT_TRUE(delegate.OriginMayUseRemoteDesktopClientOverride(
browser_context(), url::Origin::Create(GURL(kAnotherExampleOrigin))));
// Log user out
ASSERT_TRUE(LogOutUser(kTestAtExampleDotCom));
// False when user is not affiliated or no user
EXPECT_FALSE(delegate.OriginMayUseRemoteDesktopClientOverride(
browser_context(), url::Origin::Create(GURL(kExampleOrigin))));
EXPECT_FALSE(delegate.OriginMayUseRemoteDesktopClientOverride(
browser_context(), url::Origin::Create(GURL(kAnotherExampleOrigin))));
// set user as affiliated for another.example.com, same origin
SetupUserAffiliation(true, kTestAtAnotherExampleDotCom);
// True when user is affiliated for same origin
EXPECT_TRUE(delegate.OriginMayUseRemoteDesktopClientOverride(
browser_context(), url::Origin::Create(GURL(kExampleOrigin))));
EXPECT_TRUE(delegate.OriginMayUseRemoteDesktopClientOverride(
browser_context(), url::Origin::Create(GURL(kAnotherExampleOrigin))));
// Google Corp CRD origins are not affected by either the switch or this
// policy, regardless if we have or not affiliated user
for (auto* origin : kCorpCrdOrigins) {
EXPECT_FALSE(delegate.OriginMayUseRemoteDesktopClientOverride(
browser_context(), url::Origin::Create(GURL(origin))));
}
// Origins not listed in either the switch or the policy remain disallowed.
EXPECT_FALSE(delegate.OriginMayUseRemoteDesktopClientOverride(
browser_context(),
url::Origin::Create(GURL("https://very.other.example.com"))));
}nit: please split this test into smaller, more focused tests 😊
Related details
- 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. |
Olga Korokhina (Gerrit)
Olga Korokhina added 1 comment![]( "Open in Gerrit")
nit: please split this test into smaller, more focused tests 😊
I did it intentionally in one flow, to test how setting/unsetting users affiliation influence checks.
Related details
- Andrii Natiahlyi
- Martin Kreichgauer
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 8 comments![]( "Open in Gerrit")
bool IsAllowedByPlatformEnterprisePolicy(Unrelated nit: This doesn't really say what is allowed by it, and it's not clear from context. Can we rename this to something more descriptive, like `bool RemoteDesktopClientOverrideAllowedByPolicy`?
// managing organization, then the policy doesn't allow WebAuthn.This doesn't sound right. The origin isn't allowed to use the remoteDesktopClientOverride.
fake_user_manager_ = user_manager.get();Olga KorokhinaDoes `scoped_user_manager_.Get()` return the same value?
No, we can not replace it and we need both of them: scoped_user_manager_.Get() returns UserManager instance which is missing additional methods (AddUserWithAffiliationAndTypeAndProfile, AddUserWithAffiliation etc.) we need in a test, they are incapsulated in FakeUserManager.
How it works (AI generated content below, sorry!):
ash::FakeChromeUserManager *fake_user_manager_:
This is a raw pointer to the actual fake implementation of the UserManager. You need this pointer to call the specific methods of FakeChromeUserManager within your test setup functions (like SetupAshUser) to add users, log them in, set their affiliation status, etc. This is how you control the fake behavior.
std::unique_ptr<user_manager::ScopedUserManager> scoped_user_manager_:
The UserManager is a global singleton, accessed via user_manager::UserManager::Get(). Many parts of the Chrome browser code, including potentially the code being tested in ChromeWebAuthenticationDelegateBase or its dependencies like ash::ProfileHelper, will call UserManager::Get() to retrieve the current user manager instance.
ScopedUserManager is a test utility that takes ownership of a UserManager instance (in this case, your FakeChromeUserManager) and sets it as the global instance returned by UserManager::Get() for the lifetime of the ScopedUserManager object.
When scoped_user_manager_ is created in SetUp(), it injects your fake_user_manager_.
When scoped_user_manager_ is destroyed in TearDown(), it cleans up and restores the previous global UserManager instance, ensuring test isolation.
Ah, I think casting the pointer to Get() to FakeChromeUserManager would be ok. But turns out there is a better way to do this, just use the generic TypedScopedUserManager: https://source.chromium.org/chromium/chromium/src/+/main:components/user_manager/scoped_user_manager.h;l=47-57?q=scopedusermanager
So if you make scoped_user_manager a `TypedScopedUserManager<FakeChromeUserManager>`, I think you don't need `fake_user_manager_`: https://source.chromium.org/search?q=TypedScopedUserManager%3CFakeChromeUserManager%3E
const std::string_view kTestAtExampleDotCom = "te...@example.com";I think these should be `static constexpr` http://go/totw/140#a-static-class-member
raw_ptr<ash::FakeChromeUserManager> fake_user_manager_ = nullptr;
std::unique_ptr<user_manager::ScopedUserManager> scoped_user_manager_;Declaration order requires data members to be last: http://go/cstyle#Declaration_Order
// CrOS needs to be taken into account as well. On the test set up user
// affiliated with example.com domain, so with same origin we get trusThis is suggesting a relationship between the affiliated domain name of the user should have and the origins allowed to use the enterprise policy, which doesn't exist I think.
If the user was affiliated with example.com, but the enterprise policy allowed exampleremotedesktopclient.com, only the latter would be allowed to use the extension.
// set user as affiliated for another.example.com, same origin
SetupUserAffiliation(true, kTestAtAnotherExampleDotCom);What does "same origin" refer to? The domain of the affiliated user and the origin specified in the enterprise policy (or override switch) have no relation with each other.
Olga Korokhinanit: please split this test into smaller, more focused tests 😊
I did it intentionally in one flow, to test how setting/unsetting users affiliation influence checks.
Aren't ll230-260 a verbatim copy of the previous test though? I don't think we need to repeat that. If you remove it the test becomes more focused: Test that an with an unaffiliated user, the enterprise setting the policy has no effect.
Related details
- Andrii Natiahlyi
- 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 8 comments![]( "Open in Gerrit")
Unrelated nit: This doesn't really say what is allowed by it, and it's not clear from context. Can we rename this to something more descriptive, like `bool RemoteDesktopClientOverrideAllowedByPolicy`?
Agree, changing the name.
// managing organization, then the policy doesn't allow WebAuthn.This doesn't sound right. The origin isn't allowed to use the remoteDesktopClientOverride.
Comment adjusted, thank you.
fake_user_manager_ = user_manager.get();Olga KorokhinaDoes `scoped_user_manager_.Get()` return the same value?
Martin KreichgauerNo, we can not replace it and we need both of them: scoped_user_manager_.Get() returns UserManager instance which is missing additional methods (AddUserWithAffiliationAndTypeAndProfile, AddUserWithAffiliation etc.) we need in a test, they are incapsulated in FakeUserManager.
How it works (AI generated content below, sorry!):
ash::FakeChromeUserManager *fake_user_manager_:
This is a raw pointer to the actual fake implementation of the UserManager. You need this pointer to call the specific methods of FakeChromeUserManager within your test setup functions (like SetupAshUser) to add users, log them in, set their affiliation status, etc. This is how you control the fake behavior.
std::unique_ptr<user_manager::ScopedUserManager> scoped_user_manager_:
The UserManager is a global singleton, accessed via user_manager::UserManager::Get(). Many parts of the Chrome browser code, including potentially the code being tested in ChromeWebAuthenticationDelegateBase or its dependencies like ash::ProfileHelper, will call UserManager::Get() to retrieve the current user manager instance.
ScopedUserManager is a test utility that takes ownership of a UserManager instance (in this case, your FakeChromeUserManager) and sets it as the global instance returned by UserManager::Get() for the lifetime of the ScopedUserManager object.
When scoped_user_manager_ is created in SetUp(), it injects your fake_user_manager_.
When scoped_user_manager_ is destroyed in TearDown(), it cleans up and restores the previous global UserManager instance, ensuring test isolation.
Ah, I think casting the pointer to Get() to FakeChromeUserManager would be ok. But turns out there is a better way to do this, just use the generic TypedScopedUserManager: https://source.chromium.org/chromium/chromium/src/+/main:components/user_manager/scoped_user_manager.h;l=47-57?q=scopedusermanager
So if you make scoped_user_manager a `TypedScopedUserManager<FakeChromeUserManager>`, I think you don't need `fake_user_manager_`: https://source.chromium.org/search?q=TypedScopedUserManager%3CFakeChromeUserManager%3E
That worked, thank you very much, improved.
const std::string_view kTestAtExampleDotCom = "te...@example.com";I think these should be `static constexpr` http://go/totw/140#a-static-class-member
changed, thank you.
raw_ptr<ash::FakeChromeUserManager> fake_user_manager_ = nullptr;
std::unique_ptr<user_manager::ScopedUserManager> scoped_user_manager_;Declaration order requires data members to be last: http://go/cstyle#Declaration_Order
addressed, thank you
// CrOS needs to be taken into account as well. On the test set up user
// affiliated with example.com domain, so with same origin we get trusThis is suggesting a relationship between the affiliated domain name of the user should have and the origins allowed to use the enterprise policy, which doesn't exist I think.
If the user was affiliated with example.com, but the enterprise policy allowed exampleremotedesktopclient.com, only the latter would be allowed to use the extension.
Copy, thank you, removed misleading comment.
// set user as affiliated for another.example.com, same origin
SetupUserAffiliation(true, kTestAtAnotherExampleDotCom);What does "same origin" refer to? The domain of the affiliated user and the origin specified in the enterprise policy (or override switch) have no relation with each other.
misleading comment removed. I meant here we setup user affiliated with 'another.example.com' and add 'another.example.com' to allowed origins in a policy.
TEST_F(OriginMayUseRemoteDesktopClientOverrideTest,Olga Korokhinanit: please split this test into smaller, more focused tests 😊
Martin KreichgauerI did it intentionally in one flow, to test how setting/unsetting users affiliation influence checks.
Aren't ll230-260 a verbatim copy of the previous test though? I don't think we need to repeat that. If you remove it the test becomes more focused: Test that an with an unaffiliated user, the enterprise setting the policy has no effect.
Took another attempt, please have a look on new version. Thank you.
Related details
- Andrii Natiahlyi
- Martin Kreichgauer
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")
user_manager::TypedScopedUserManager<chromeos::FakeChromeUserManager>
scoped_user_manager_{std::make_unique<chromeos::FakeChromeUserManager>()};Nit: member variable should go after instance methods go/cstyle#Declaration_Order
static constexpr std::string_view kTestAtExampleDotCom = "te...@example.com";
static constexpr std::string_view kTestAtAnotherExampleDotCom =
"te...@another.example.com";
#endif
static constexpr char kCorpCrdOrigin[] =
"https://remotedesktop.corp.google.com";
static constexpr char kCorpCrdAutopushOrigin[] =
"https://remotedesktop-autopush.corp.google.com/";
static constexpr char kCorpCrdDailyOrigin[] =
"https://remotedesktop-daily-6.corp.google.com/";
const std::array<const char*, 3> kCorpCrdOrigins = {
kCorpCrdOrigin, kCorpCrdAutopushOrigin, kCorpCrdDailyOrigin};
static constexpr char kExampleOrigin[] = "https://example.com";
static constexpr char kAnotherExampleOrigin[] = "https://another.example.com";Nit: static constants should preceed ctors and member functions go/cstyle#Declaration_Order
// set user as affiliated for another.example.com // set user as affiliated for another.example.com
SetupUserAffiliation(true, kTestAtAnotherExampleDotCom);
// True when user is affiliated
EXPECT_TRUE(delegate.OriginMayUseRemoteDesktopClientOverride(
browser_context(), url::Origin::Create(GURL(kExampleOrigin))));
EXPECT_TRUE(delegate.OriginMayUseRemoteDesktopClientOverride(
browser_context(), url::Origin::Create(GURL(kAnotherExampleOrigin))));
// Google Corp CRD origins are not affected by either the switch or this
// policy, regardless if we have or not affiliated user
for (auto* origin : kCorpCrdOrigins) {
EXPECT_FALSE(delegate.OriginMayUseRemoteDesktopClientOverride(
browser_context(), url::Origin::Create(GURL(origin))));
}
// Origins not listed in either the switch or the policy remain disallowed.
EXPECT_FALSE(delegate.OriginMayUseRemoteDesktopClientOverride(
browser_context(),
url::Origin::Create(GURL("https://very.other.example.com"))));Is this block from ll243-262 testing the same behavior as `AdditionalOriginSwitch_WithAllowedOriginsPolicy` on ll179-214?
Related details
- Andrii Natiahlyi
- 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")
user_manager::TypedScopedUserManager<chromeos::FakeChromeUserManager>
scoped_user_manager_{std::make_unique<chromeos::FakeChromeUserManager>()};Nit: member variable should go after instance methods go/cstyle#Declaration_Order
Moved down, thank you!
static constexpr std::string_view kTestAtExampleDotCom = "te...@example.com";
static constexpr std::string_view kTestAtAnotherExampleDotCom =
"te...@another.example.com";
#endif
static constexpr char kCorpCrdOrigin[] =
"https://remotedesktop.corp.google.com";
static constexpr char kCorpCrdAutopushOrigin[] =
"https://remotedesktop-autopush.corp.google.com/";
static constexpr char kCorpCrdDailyOrigin[] =
"https://remotedesktop-daily-6.corp.google.com/";
const std::array<const char*, 3> kCorpCrdOrigins = {
kCorpCrdOrigin, kCorpCrdAutopushOrigin, kCorpCrdDailyOrigin};
static constexpr char kExampleOrigin[] = "https://example.com";
static constexpr char kAnotherExampleOrigin[] = "https://another.example.com";Nit: static constants should preceed ctors and member functions go/cstyle#Declaration_Order
1. Types and type aliases (typedef, using, enum, nested structs and classes, and friend types)
2. (Optionally, for structs only) non-static data members
3. Static constants
4. Factory functions
5. Constructors and assignment operators
6. Destructor
7. All other functions (static and non-static member functions, and friend functions)
8. All other data members (static and non-static)
Hope I organized it well now, taken into account we have some section under build flag
// set user as affiliated for another.example.com```suggestion
// Set user as affiliated for another.example.com
```
Correcting, thank you
// set user as affiliated for another.example.com
SetupUserAffiliation(true, kTestAtAnotherExampleDotCom);
// True when user is affiliated
EXPECT_TRUE(delegate.OriginMayUseRemoteDesktopClientOverride(
browser_context(), url::Origin::Create(GURL(kExampleOrigin))));
EXPECT_TRUE(delegate.OriginMayUseRemoteDesktopClientOverride(
browser_context(), url::Origin::Create(GURL(kAnotherExampleOrigin))));
// Google Corp CRD origins are not affected by either the switch or this
// policy, regardless if we have or not affiliated user
for (auto* origin : kCorpCrdOrigins) {
EXPECT_FALSE(delegate.OriginMayUseRemoteDesktopClientOverride(
browser_context(), url::Origin::Create(GURL(origin))));
}
// Origins not listed in either the switch or the policy remain disallowed.
EXPECT_FALSE(delegate.OriginMayUseRemoteDesktopClientOverride(
browser_context(),
url::Origin::Create(GURL("https://very.other.example.com"))));Is this block from ll243-262 testing the same behavior as `AdditionalOriginSwitch_WithAllowedOriginsPolicy` on ll179-214?
Basically ll243- yes, but after re-logged out user. So you're right, what it basically tests is the re-logout works as expected, not a purpose of this test, removing.
Nit: Spurious edit? Same on l349.
- Andrii Natiahlyi
- 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 added 1 comment![]( "Open in Gerrit")
const user_manager::User* user =FWIW on ChromeOS this is always equal to `user_manager::UserManager::Get()->GetActiveUser()`; @ant...@chromium.org to confirm (this could be better than introducing another cross-dep to `c/b/ash`).
Related details
- Andrii Natiahlyi
- Denis Kuznetsov
- 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. |
Olga Korokhina (Gerrit)
Olga Korokhina added 1 comment![]( "Open in Gerrit")
const user_manager::User* user =FWIW on ChromeOS this is always equal to `user_manager::UserManager::Get()->GetActiveUser()`; @ant...@chromium.org to confirm (this could be better than introducing another cross-dep to `c/b/ash`).
UserMamnger is also 'from' components (components/user_manager/user_manager.h) so we might still need to add another dept. Let me check and prove.
Related details
- Andrew Rayskiy
- Andrii Natiahlyi
- Denis Kuznetsov
- Martin Kreichgauer
Olga Korokhina (Gerrit)
Olga Korokhina added 1 comment![]( "Open in Gerrit")
Olga KorokhinaFWIW on ChromeOS this is always equal to `user_manager::UserManager::Get()->GetActiveUser()`; @ant...@chromium.org to confirm (this could be better than introducing another cross-dep to `c/b/ash`).
UserMamnger is also 'from' components (components/user_manager/user_manager.h) so we might still need to add another dept. Let me check and prove.
UPD: we already have this dep added, so we can get rid of one but not of both. Seems like you proposal works, adjusted the code, thank you very much!
Related details
- Andrew Rayskiy
- Andrii Natiahlyi
- Denis Kuznetsov
- Martin Kreichgauer
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |