IdentityManager::ClearPrimaryAccount() - was SigninManager::Signout()

[Chris Cunningham]

Hi Mihai, Collin. 

As you know, SigninManager currently has 3 SignOut() methods. For IdentityManager, Mihai discussed that something like this could work. 

enum RemoveTokenAction {

// Leave it to IdentityManager to decide what to do. Like SigninManager,

// it will decide differently based on internal signin::AccountConsistencyMethod state.

Default,

// Equivalent to SigninManager::SignOutAndRemoveAllAccounts()

RemoveAll,

// Equivalent to SigninManager::SignOutAndKeepAllAccounts()

KeepAll,

};

void IdentityManager::ClearPrimaryAccount(RemoveTokenAction action);

But I'm not in love with this. DICE is very specific to chrome desktop - mobile, and chromeos have different strategies. Moreover, its a fairly complicated chrome desktop problem - it is not used 100% of the time and the strategy of DICE signout is actively changing (and may change again). 

All users of ClearPrimaryAccount() do desire *some* action be taken wrt to tokens - they just don't agree on what action. So I propose we continue to pass an enum argument, but we should remove the |Default| entry from the enum above. IdentityManager is the _platform_ level service - desktop specific DICE behavior should live in the higher layer. In practice, this will mean IdentityManager does not take a dependency on signin::AccountConsistencyMethod. Callers of ClearPrimaryAccount() will themselves take that dependency and decide what enum value to pass in to ClearPrimaryAccount(). The enum is a required argument with no default, so callers will are forced to ponder the right value for their use case. 

What do yall think?

Thanks,

Chris

[Mihai Sardarescu]

Hi Chris,

The problem I can see with this approach is that we may have different behavior for the same callers pre-DICE and after DICE. Then all the callers will need to do something like:

if (IsDiceEnabledForProfile(profile_))

  identity_manager-> ClearPrimaryAccount(KEEP_ACCOUNTS)

else

  identity_manager-> ClearPrimaryAccount(REMOVE_ACCOUNTS)

I find that would be a poor solution if a lot of clients need to do this.

- Mihai

[Chris Cunningham]

Thanks Mihai,

Then all the callers will need to do something like: 

if (IsDiceEnabledForProfile(profile_))
  identity_manager-> ClearPrimaryAccount(KEEP_ACCOUNTS)
else
  identity_manager-> ClearPrimaryAccount(REMOVE_ACCOUNTS)

I actually really like the way that looks. It puts the dice business logic in the desktop-browser-layer, which is the logical scope of the DICE feature (e.g. DICE has no role in ChromeOS and the IdentityManager API will serve both ChromeOS and the desktop browser code).

I find that would be a poor solution if a lot of clients need to do this.

Here's a list of current callers for SignOut and its overloads.

• SM::SignOut (5)

• chrome/browser/ui/sync/one_click_signin_sync_starter.cc

• chrome/browser/ui/views/profiles/forced_reauthentication_dialog_view.cc

• chrome/browser/ui/views/profiles/profile_chooser_view.cc

• chrome/browser/ui/webui/settings/people_handler.cc

• components/browser_sync/profile_sync_service.cc

• SM::SignOutAndKeepAllAccounts callers

• chrome/browser/ui/webui/signin/dice_turn_sync_on_helper.cc

• SM::SignOutAndRemoveAllAccounts callers (4)

• chrome/browser/profiles/profile_manager.cc

• chrome/browser/signin/force_signin_verifier.cc

• chrome/browser/ui/sync/one_click_signin_sync_starter.cc

• chrome/browser/ui/webui/settings/people_handler.cc

Of these, only SignOut() currently has behavior conditioned on DICE state. I'm not sure if all current SignOut() callers actually care about DICE, but assuming they do, they represent just 5 calls (and 50% of total signout calls). Do you think those 5 can make this check without too much pain? Are there any of those I can exempt?

Thanks,

Chris

--
You received this message because you are subscribed to the Google Groups "identity-service-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to identity-service...@chromium.org.
To post to this group, send email to identity-s...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/identity-service-dev/4c90d0b7-6a50-46f4-a5f2-b73609a95816%40chromium.org.

[Colin Blundell]

Thanks, Chris! (and someone open a bottle of champagne for the inauguration of identity-service-dev@)

On Thu, Jul 19, 2018 at 3:42 PM Chris Cunningham <chcunn...@chromium.org> wrote:

Thanks Mihai,

Then all the callers will need to do something like: 

if (IsDiceEnabledForProfile(profile_))
  identity_manager-> ClearPrimaryAccount(KEEP_ACCOUNTS)
else
  identity_manager-> ClearPrimaryAccount(REMOVE_ACCOUNTS)

I actually really like the way that looks. It puts the dice business logic in the desktop-browser-layer, which is the logical scope of the DICE feature (e.g. DICE has no role in ChromeOS and the IdentityManager API will serve both ChromeOS and the desktop browser code).

I find that would be a poor solution if a lot of clients need to do this.

Here's a list of current callers for SignOut and its overloads.

• SM::SignOut (5)

• chrome/browser/ui/sync/one_click_signin_sync_starter.cc

• chrome/browser/ui/views/profiles/forced_reauthentication_dialog_view.cc

• chrome/browser/ui/views/profiles/profile_chooser_view.cc

• chrome/browser/ui/webui/settings/people_handler.cc

• components/browser_sync/profile_sync_service.cc

• SM::SignOutAndKeepAllAccounts callers

• chrome/browser/ui/webui/signin/dice_turn_sync_on_helper.cc

• SM::SignOutAndRemoveAllAccounts callers (4)

• chrome/browser/profiles/profile_manager.cc

• chrome/browser/signin/force_signin_verifier.cc

• chrome/browser/ui/sync/one_click_signin_sync_starter.cc

• chrome/browser/ui/webui/settings/people_handler.cc

Of these, only SignOut() currently has behavior conditioned on DICE state. I'm not sure if all current SignOut() callers actually care about DICE, but assuming they do, they represent just 5 calls (and 50% of total signout calls). Do you think those 5 can make this check without too much pain? Are there any of those I can exempt?

Mihai, what is your hunch on whether the IdentityManager implementation/semantics will necessarily grow more dependencies on DICE behavior over time? If yes, then it is likely futile to keep it scoped on the client side now.

[Chris Cunningham]

Colin, Mihai and I spoke f2f. The outcome is to implement the with the Default enum value that will condition behavior on DICE status.

The argument I made about DICE being desktop specific is a little flawed. SigninManager is constructed with a signin::AccountConsistencyMethod enum member, for which one of the possible inputs is DICE, alongside other possibilities like Mirror (which are also used to decide other behaviors of SigninManager). While DICE is desktop specific, "account consistency" is a cross platform browser/content idea (you signed in to chrome, so you shouldn't have to re-signin to gmail).

Their is an even higher level ambition for the IdentityService to one day serve users outside of the browser/content world (e.g. on ChromeOS) where "account consistency" between browser/content doesn't fit. But that feels a long way off and may not actually take the shape we envisioned - its premature to design for it now. 

Cheers,

Chris

[Colin Blundell]

Thanks!

On Thu, Jul 19, 2018 at 6:23 PM Chris Cunningham <chcunn...@chromium.org> wrote:

Colin, Mihai and I spoke f2f. The outcome is to implement the with the Default enum value that will condition behavior on DICE status.

The argument I made about DICE being desktop specific is a little flawed. SigninManager is constructed with a signin::AccountConsistencyMethod enum member, for which one of the possible inputs is DICE, alongside other possibilities like Mirror (which are also used to decide other behaviors of SigninManager). While DICE is desktop specific, "account consistency" is a cross platform browser/content idea (you signed in to chrome, so you shouldn't have to re-signin to gmail).

Their is an even higher level ambition for the IdentityService to one day serve users outside of the browser/content world (e.g. on ChromeOS) where "account consistency" between browser/content doesn't fit. But that feels a long way off and may not actually take the shape we envisioned - its premature to design for it now. 

+1. In particular, we should try to keep that ambition as a North Star, but we have to weigh it against the reality of designing APIs that work with the reality of current usage by the browser. If in some future there are concrete use cases where it's problematic to have account consistency inside the Identity Service, we can look at layering then. For all existing use cases, account consistency is (becoming) a fundamental baked-in assumption.