Improve IdentityManager API to retrieve info about configured accounts |
||
Issue descriptionAs explained in [1], the IdentityManager API contains a GetPrimaryAccountInfo() method that delegates in SigninManagerBase::GetAuthenticatedAccountInfo() to return an AccountInfo, but does not have a method to return the primary account's ID directly. The problem here is that it might be tempting to call IdentityManager::GetPrimaryAccountInfo() and then access the account_id member to determine the ID of the primary account, as if it was equivalent to retrieve the output of SigninManagerBase::GetAuthenticatedAccountId()... but it's actually quite different: When you call SigninManagerBase::GetAuthenticatedAccountInfo() you interact behind the scenes with the AccountTrackerService, which will return an empty struct in case the account has been removed or has signed out. However, when you call SigninManagerBase::GetAuthenticatedAccountId() you retrieve the ID of the primary account (which is managed internally in SigninManagerBase), regardless of whether it's currently signed in or out. Talking to @blundell today, we agreed[2] on that we'll move forward adding this GetPrimaryAccountId() to the IdentityManager to solve the issue at hand now (documenting it in the header file so that it's clear that is NOT equivalent to calling GetPrimaryAccountInfo(), and then file a separate issue to try to improve the APIs in a way that is less confusing, so here it is the issue. One idea we talked about was to remove the IdentityManager::GetPrimaryAccountInfo() method altogether (updating the callers to interact directly with the AccountTrackerService) and just leave IdentityManager::GetPrimaryAccountId(), or maybe renaming GetPrimaryAccountInfo() so that it does not look like GetPrimaryAccountId() is a shortcut for GetPrimaryAccountInfo().account_id. [1] https://chromium-review.googlesource.com/c/chromium/src/+/1250968 [2] https://chromium-review.googlesource.com/c/chromium/src/+/1250968#message-40b27c4018f5c41f4c651aa638a7059aea466e50
,
Oct 5
Thanks @blundell for the clarification. I was actually going to comment here myself after I learned today (while working on https://chromium-review.googlesource.com/c/chromium/src/+/1264162) that I misunderstood the situation the other day. As you say, the corner case here is only about removing a signed in account via the AccountTrackerService. Signing out via the IdentityManager / SigninManager does not expose this situation.
,
Nov 9
@blundell As the list of vendor bugs is shrinking, I stumbled upon this one again and I was wondering whether we should take care of this now, and which way. From my POV, seeing that GetPrimaryAccountId() and GetPrimaryAccountInfo() are both used quite a lot around, I'm leaning towards to either renaming one of them or to simply leave it as it is (trusting that the documented behaviour in identity_manager.h would be enough). If renaming one of them, and thinking of renaming GetPrimaryAccountInfo(), I was thinking that maybe we could use one of these: - GetPrimaryAccountExtendedInfo() - GetPrimaryAccountCurrentInfo() - GetPrimaryAccountActiveInfo() - GetPrimaryAccountValidInfo() - GetPrimaryAccountAvailableInfo() What do you think? Any comment?
,
Nov 13
Hi Mario! Let's actually leave this for now. My hunch is that we'll tackle this as part of migrating AccountTrackerService to IdentityManager; we're contemplating a bigger refactoring where we'll remove the redundancy between account ID and AccountInfo, having a "basic account info" struct that has account ID/gaia ID/email and an "extended account info" struct that has everything else. The basic APIs would then operate on the "basic account info" struct.
,
Nov 13
Sounds good to me, thanks for clarifying! |
||
►
Sign in to add a comment |
||
Comment 1 by blundell@chromium.org
, Oct 5