[Ash] Set primary account consent level to Signin for new profiles [chromium/src : main]
Marc Treib (Gerrit)
Marc Treib added 8 comments![]( "Open in Gerrit")
signin::GetPrimaryAccountConsentLevel(identity_manager));Does this actually need to depend on the new helper? I was expecting to just check the `kReplaceSyncPromos..` flag here, like we did on other platforms. (Then all these changes could also be done independently of introducing the new flag.)
signin::GetPrimaryAccountConsentLevel(identity_manager);At this point, there is no primary account yet, so presumably we cannot get its consent level?!
signin::GetPrimaryAccountConsentLevel(identity_manager);Is that from [here](https://source.chromium.org/chromium/chromium/src/+/main:components/signin/public/identity_manager/identity_test_utils.h;drc=81862aca3afbc463d2e2c2f3fcd1b512e269a3d2;l=57), i.e. a test-only helper?!
BASE_DECLARE_FEATURE(kChromeOSSignInOnly);nit: Put this at the end of the file; reusing an existing CrOS section in the middle is just kinda confusing
BASE_FEATURE(kChromeOSSignInOnly, base::FEATURE_DISABLED_BY_DEFAULT);Please add documentation for what this does
ConsentLevel GetPrimaryAccountConsentLevel(There is a function with the same name in a test utils file: https://source.chromium.org/chromium/chromium/src/+/main:components/signin/public/identity_manager/identity_test_utils.h;drc=81862aca3afbc463d2e2c2f3fcd1b512e269a3d2;l=57
(I was quite confused for a while, wondering why you're calling a test-only util in prod...)
Can you please give this (a) a unique name, and (b) a better name (currently it sounds like it's just querying the consent level of the existing primary account). Maybe something like "GetConsentLevelToUseForPrimaryAccount"?
CHECK(identity_manager);nit: If it mustn't be null, pass by const ref instead of by pointer
base::FeatureList::IsEnabled(switches::kChromeOSSignInOnly)Should we also check `kReplaceSyncPromos..` here? The new flag should never be enabled without that one, but just to be safe.
Related details
- Mahmoud Rashad
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. |
Mahmoud Rashad (Gerrit)
Mahmoud Rashad added 8 comments![]( "Open in Gerrit")
signin::GetPrimaryAccountConsentLevel(identity_manager));Does this actually need to depend on the new helper? I was expecting to just check the `kReplaceSyncPromos..` flag here, like we did on other platforms. (Then all these changes could also be done independently of introducing the new flag.)
You're right! only the set occurrences should be guarded by the new flag.
signin::GetPrimaryAccountConsentLevel(identity_manager);At this point, there is no primary account yet, so presumably we cannot get its consent level?!
It works fine even if there's no primary account yet. It will just return false.
signin::GetPrimaryAccountConsentLevel(identity_manager);Is that from [here](https://source.chromium.org/chromium/chromium/src/+/main:components/signin/public/identity_manager/identity_test_utils.h;drc=81862aca3afbc463d2e2c2f3fcd1b512e269a3d2;l=57), i.e. a test-only helper?!
Marked as resolved.
nit: Put this at the end of the file; reusing an existing CrOS section in the middle is just kinda confusing
This should be kept sorted, I put it in the correct order.
BASE_FEATURE(kChromeOSSignInOnly, base::FEATURE_DISABLED_BY_DEFAULT);Please add documentation for what this does
Documented in the .h
There is a function with the same name in a test utils file: https://source.chromium.org/chromium/chromium/src/+/main:components/signin/public/identity_manager/identity_test_utils.h;drc=81862aca3afbc463d2e2c2f3fcd1b512e269a3d2;l=57
(I was quite confused for a while, wondering why you're calling a test-only util in prod...)Can you please give this (a) a unique name, and (b) a better name (currently it sounds like it's just querying the consent level of the existing primary account). Maybe something like "GetConsentLevelToUseForPrimaryAccount"?
Done
nit: If it mustn't be null, pass by const ref instead of by pointer
Done
base::FeatureList::IsEnabled(switches::kChromeOSSignInOnly)Should we also check `kReplaceSyncPromos..` here? The new flag should never be enabled without that one, but just to be safe.
- Marc Treib
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. |
Marc Treib (Gerrit)
Marc Treib added 4 comments![]( "Open in Gerrit")
when the `kReplaceSyncPromosWithSignInPromos` flag is enabled AND the
profile is not using legacy sync (`!HasPrimaryAccount(kSync)`). ThisI don't think the second part of the condition is even needed? Asking for a kSignin access token while there is a kSync account is perfectly fine and safe. It's just specifying the *minimum* consent level that's required.
the sign-in only model. It:
1. Updates `UserSessionManager` and `ChromeSessionManager` to set the
primary account consent level to `kSignin` (instead of `kSync`) for new
profiles when both the `kReplaceSyncPromosWithSignInPromos` and
`kChromeOSSignInOnly` flags are enabled.
2. Updates several handlers and delegates to use `ConsentLevel::kSignin`
when the `kReplaceSyncPromosWithSignInPromos` flag is enabled AND the
profile is not using legacy sync (`!HasPrimaryAccount(kSync)`). This
guarantees zero behavioral change when that flag is off or for existing
profiles.Can these two things be separate CLs now? I think 2. can (and should) be landed separately first.
signin::GetPrimaryAccountConsentLevel(identity_manager);Mahmoud RashadAt this point, there is no primary account yet, so presumably we cannot get its consent level?!
It works fine even if there's no primary account yet. It will just return false.
Acknowledged
BASE_DECLARE_FEATURE(kChromeOSSignInOnly);Let's try to find a more descriptive name for this.
Something like `kChromeOsUseConsentLevelSigninForNewUsers`?
Related details
- Mahmoud Rashad
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. |
Marc Treib (Gerrit)
Marc Treib voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Mahmoud Rashad
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Reviewer source(s):
bsaz...@chromium.org is from context(googleclient/chrome/chromium_gwsq/components/signin/config.gwsq)
Related details
- Boris Sazonov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mahmoud Rashad (Gerrit)
Mahmoud Rashad added 1 comment![]( "Open in Gerrit")
Boris Sazonov (Gerrit)
Boris Sazonov added 1 comment![]( "Open in Gerrit")
#if BUILDFLAG(IS_CHROMEOS)
// On Chrome OS the UPA can only be set once and never removed or changed.
DCHECK(
!primary_account_manager_->HasPrimaryAccount(ConsentLevel::kSignin));
#endifThe removed check along with [this check](https://source.chromium.org/chromium/chromium/src/+/main:components/signin/internal/identity_manager/primary_account_manager.cc;l=521;drc=87f4290357c0c1cb422a10d31670acfb9dff1b72) were ensuring that the primary account never changes on ChromeOS. I think we need to ensure that this is still the case after your CL. Would you mind adding a ChromeOS-specific check to keep this invariant secure? ChromeOS code wasn't built with account switching in mind, so it seems pretty important to ensure this.
Related details
- Mahmoud Rashad
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. |
Mahmoud Rashad (Gerrit)
Mahmoud Rashad added 1 comment![]( "Open in Gerrit")
#if BUILDFLAG(IS_CHROMEOS)
// On Chrome OS the UPA can only be set once and never removed or changed.
DCHECK(
!primary_account_manager_->HasPrimaryAccount(ConsentLevel::kSignin));
#endifThe removed check along with [this check](https://source.chromium.org/chromium/chromium/src/+/main:components/signin/internal/identity_manager/primary_account_manager.cc;l=521;drc=87f4290357c0c1cb422a10d31670acfb9dff1b72) were ensuring that the primary account never changes on ChromeOS. I think we need to ensure that this is still the case after your CL. Would you mind adding a ChromeOS-specific check to keep this invariant secure? ChromeOS code wasn't built with account switching in mind, so it seems pretty important to ensure this.
- Boris Sazonov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Boris Sazonov (Gerrit)
Boris Sazonov added 1 comment![]( "Open in Gerrit")
DCHECK_EQ(account_info,[Nit] Consider using `CHECK_EQ` instead of `DCHECK_EQ`.
Related details
- Mahmoud Rashad
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. |
Boris Sazonov (Gerrit)
Boris Sazonov voted and added 1 comment![]( "Open in Gerrit")
Related details
- Mahmoud Rashad
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. |
Mahmoud Rashad (Gerrit)
Mahmoud Rashad voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mahmoud Rashad (Gerrit)
Mahmoud Rashad added 2 comments![]( "Open in Gerrit")
[Nit] Consider using `CHECK_EQ` instead of `DCHECK_EQ`.
Done
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
4 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: components/signin/internal/identity_manager/primary_account_mutator_impl.cc
Insertions: 3, Deletions: 3.
The diff is too large to show. Please review the diff.
```
Change information
Set primary account consent level to Signin for new profiles
This CL updates the primary account consent level on ChromeOS to support
the sign-in only model. It updates `UserSessionManager` and
`ChromeSessionManager` to set the primary account consent level to
`kSignin` (instead of `kSync`) for new profiles when both the
`kReplaceSyncPromosWithSignInPromos` and
`kChromeOsUseConsentLevelSigninForNewUsers` flags are enabled.
- M chrome/browser/ash/login/session/chrome_session_manager.cc
- M chrome/browser/ash/login/session/user_session_manager.cc
- M components/signin/internal/identity_manager/primary_account_mutator_impl.cc
- M components/signin/public/base/signin_switches.cc
- M components/signin/public/base/signin_switches.h
Code-Review: +1 by Boris Sazonov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |