Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1704623002:
GetProfileFromAccountId() should not return a null point if a valid account id is provided. (Closed)
|
Created:
4 years, 10 months ago by xdai1 Modified:
4 years, 10 months ago Reviewers:
Mr4D (OOO till 08-26) CC:
chromium-reviews, kalyank, sadrul Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGetProfileFromAccountId() should not return a null point if a valid account id is provided.
GetProfileFromAccountId() might return a null point if a refresh token is revoked in the session, which might cause a browser crash in ChromeOS. In this case we avoid nullptr by calling GetProfileByUser() as a fallback function.
BUG=585847, 532535
Committed: https://crrev.com/78dbec96889ea75d9e93b0f166cf7ab68dd209ef
Cr-Commit-Position: refs/heads/master@{#376202}
Patch Set 1 #
Total comments: 3
Patch Set 2 : #
Total comments: 1
Patch Set 3 : Add comment. #Messages
Total messages: 21 (8 generated)
xdai@chromium.org changed reviewers: + skuhne@chromium.org
skuhne@, could you help to review this CL please? +cc alemate@ FYI
If this is in response to some rights change we should possibly end the session in case of multi profile? https://codereview.chromium.org/1704623002/diff/1/chrome/browser/ui/ash/multi... File chrome/browser/ui/ash/multi_user/multi_user_util.cc (right): https://codereview.chromium.org/1704623002/diff/1/chrome/browser/ui/ash/multi... chrome/browser/ui/ash/multi_user/multi_user_util.cc:52: // If the refresh token is revoked in the session, GetAccountIdFromProfile() Sounds weird. Better: If in a session the refresh token is revoked, .. But: what does it exactly mean that the refresh token got revoked? If this is something where the user has to log out and back log in to get to a running state, we should possibly enforce an end session since this might otherwise cause a secure account to be vulnerable.
https://codereview.chromium.org/1704623002/diff/1/chrome/browser/ui/ash/multi... File chrome/browser/ui/ash/multi_user/multi_user_util.cc (right): https://codereview.chromium.org/1704623002/diff/1/chrome/browser/ui/ash/multi... chrome/browser/ui/ash/multi_user/multi_user_util.cc:52: // If the refresh token is revoked in the session, GetAccountIdFromProfile() On 2016/02/16 20:26:18, Mr4D wrote: > Sounds weird. Better: > > If in a session the refresh token is revoked, .. > > But: what does it exactly mean that the refresh token got revoked? If this is > something where the user has to log out and back log in to get to a running > state, we should possibly enforce an end session since this might otherwise > cause a secure account to be vulnerable. This usually happens when Google account password is changed in another user session. Sync will not work until re-login. We do not force log out, as network may be inaccessible.
Description was changed from ========== GetProfileFromAccountId() should not return a null point it a valid account id is provided. GetProfileFromAccountId() might return a null point if a refresh token is revoked in the session, which might cause a browser crash in ChromeOS. In this case we avoid nullptr by calling GetProfileByUser() as a fallback function. BUG=585847,532535 ========== to ========== GetProfileFromAccountId() should not return a null point if a valid account id is provided. GetProfileFromAccountId() might return a null point if a refresh token is revoked in the session, which might cause a browser crash in ChromeOS. In this case we avoid nullptr by calling GetProfileByUser() as a fallback function. BUG=585847,532535 ==========
On 2016/02/16 20:34:15, Alexander Alekseev wrote: > https://codereview.chromium.org/1704623002/diff/1/chrome/browser/ui/ash/multi... > File chrome/browser/ui/ash/multi_user/multi_user_util.cc (right): > > https://codereview.chromium.org/1704623002/diff/1/chrome/browser/ui/ash/multi... > chrome/browser/ui/ash/multi_user/multi_user_util.cc:52: // If the refresh token > is revoked in the session, GetAccountIdFromProfile() > On 2016/02/16 20:26:18, Mr4D wrote: > > Sounds weird. Better: > > > > If in a session the refresh token is revoked, .. > > > > But: what does it exactly mean that the refresh token got revoked? If this is > > something where the user has to log out and back log in to get to a running > > state, we should possibly enforce an end session since this might otherwise > > cause a secure account to be vulnerable. > > This usually happens when Google account password is changed in another user > session. Sync will not work until re-login. We do not force log out, as network > may be inaccessible. This seems to be then exactly a reason to force a re-login. Think about this: Account of user A has been compromised and the user changed the password. This user (or another of that session) might have a higher "clearance" and might be potentially compromising. Furthermore - to figure that out, you would need to be online (if being offline, there is no way to figure out that the password has changed). As such I think it is much more secure to restart the session.
Please take another look, thanks for your review! https://codereview.chromium.org/1704623002/diff/1/chrome/browser/ui/ash/multi... File chrome/browser/ui/ash/multi_user/multi_user_util.cc (right): https://codereview.chromium.org/1704623002/diff/1/chrome/browser/ui/ash/multi... chrome/browser/ui/ash/multi_user/multi_user_util.cc:52: // If the refresh token is revoked in the session, GetAccountIdFromProfile() On 2016/02/16 20:26:18, Mr4D wrote: > Sounds weird. Better: > > If in a session the refresh token is revoked, .. > Done. > But: what does it exactly mean that the refresh token got revoked? If this is > something where the user has to log out and back log in to get to a running > state, we should possibly enforce an end session since this might otherwise > cause a secure account to be vulnerable. Alexander is right about the possibility that the refresh token can be revoked in a session. Besides, this CL is just some speculation based on the observation of the crash stack and the observation of the call flow to AccountTrackerService. It might be one of the reasons that causes the browser crash, but there might be other reasons too. I don't think we have knowledge if the refresh token gets revoked or not in this function, GetProfileByUser() just covers the case that the profile is not properly returned which is caused by the revoked refresh token that is not covered by the above for-loop. It's still possible that GetProfileFromAccountId() receives an invalid account id and thus GetProfileByUser() returns a null pointer.
On 2016/02/16 21:25:12, Mr4D wrote: > On 2016/02/16 20:34:15, Alexander Alekseev wrote: > > > https://codereview.chromium.org/1704623002/diff/1/chrome/browser/ui/ash/multi... > > File chrome/browser/ui/ash/multi_user/multi_user_util.cc (right): > > > > > https://codereview.chromium.org/1704623002/diff/1/chrome/browser/ui/ash/multi... > > chrome/browser/ui/ash/multi_user/multi_user_util.cc:52: // If the refresh > token > > is revoked in the session, GetAccountIdFromProfile() > > On 2016/02/16 20:26:18, Mr4D wrote: > > > Sounds weird. Better: > > > > > > If in a session the refresh token is revoked, .. > > > > > > But: what does it exactly mean that the refresh token got revoked? If this > is > > > something where the user has to log out and back log in to get to a running > > > state, we should possibly enforce an end session since this might otherwise > > > cause a secure account to be vulnerable. > > > > This usually happens when Google account password is changed in another user > > session. Sync will not work until re-login. We do not force log out, as > network > > may be inaccessible. > > This seems to be then exactly a reason to force a re-login. Think about this: > > Account of user A has been compromised and the user changed the password. > > This user (or another of that session) might have a higher "clearance" and might > be potentially compromising. > Furthermore - to figure that out, you would need to be online (if being offline, > there is no way to figure out that the password has changed). > > As such I think it is much more secure to restart the session. We never force user to log out. I think this question is beyond this CL. And I am also not sure I am the right person to discuss this. (This looks like a major user experience change.) But we allow user to start a new session even if the refresh token is revoked. (I don't know all the cases when it is revoked, but this happens in case of password change.) If you are traveling, even a short network access is enough to invalidate token. You may loose network access immediately then (for example, take off on a plane), and your chromebook will turn into a brick. But you can find some PM to discuss your idea, if you think it is still worth doing it.
Since we have now an issue which will force log out the user, this change should be ok. (Even though it still has to be proven that the profile is accessible through the other way). lgtm. https://codereview.chromium.org/1704623002/diff/20001/chrome/browser/ui/ash/m... File chrome/browser/ui/ash/multi_user/multi_user_util.cc (right): https://codereview.chromium.org/1704623002/diff/20001/chrome/browser/ui/ash/m... chrome/browser/ui/ash/multi_user/multi_user_util.cc:54: // properly. In this case we fall back to use GetProfileByUser() function. Please add also a comment that in case the refresh token is revoked, we will force log out the user within 120 seconds. You might also want to reference the issue number which made this change necessary.
On 2016/02/17 04:05:08, Mr4D wrote: > Since we have now an issue which will force log out the user, this change should > be ok. (Even though it still has to be proven that the profile is accessible > through the other way). > lgtm. > > https://codereview.chromium.org/1704623002/diff/20001/chrome/browser/ui/ash/m... > File chrome/browser/ui/ash/multi_user/multi_user_util.cc (right): > > https://codereview.chromium.org/1704623002/diff/20001/chrome/browser/ui/ash/m... > chrome/browser/ui/ash/multi_user/multi_user_util.cc:54: // properly. In this > case we fall back to use GetProfileByUser() function. > Please add also a comment that in case the refresh token is revoked, we will > force log out the user within 120 seconds. > You might also want to reference the issue number which made this change > necessary. Thanks for your review! Could you send me the bug number that force the user log out within 120 seconds? I didn't see it cc'ed to me.
The CQ bit was checked by xdai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1704623002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1704623002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xdai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skuhne@chromium.org Link to the patchset: https://codereview.chromium.org/1704623002/#ps40001 (title: "Add comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1704623002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1704623002/40001
Message was sent while issue was closed.
Description was changed from ========== GetProfileFromAccountId() should not return a null point if a valid account id is provided. GetProfileFromAccountId() might return a null point if a refresh token is revoked in the session, which might cause a browser crash in ChromeOS. In this case we avoid nullptr by calling GetProfileByUser() as a fallback function. BUG=585847,532535 ========== to ========== GetProfileFromAccountId() should not return a null point if a valid account id is provided. GetProfileFromAccountId() might return a null point if a refresh token is revoked in the session, which might cause a browser crash in ChromeOS. In this case we avoid nullptr by calling GetProfileByUser() as a fallback function. BUG=585847,532535 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== GetProfileFromAccountId() should not return a null point if a valid account id is provided. GetProfileFromAccountId() might return a null point if a refresh token is revoked in the session, which might cause a browser crash in ChromeOS. In this case we avoid nullptr by calling GetProfileByUser() as a fallback function. BUG=585847,532535 ========== to ========== GetProfileFromAccountId() should not return a null point if a valid account id is provided. GetProfileFromAccountId() might return a null point if a refresh token is revoked in the session, which might cause a browser crash in ChromeOS. In this case we avoid nullptr by calling GetProfileByUser() as a fallback function. BUG=585847,532535 Committed: https://crrev.com/78dbec96889ea75d9e93b0f166cf7ab68dd209ef Cr-Commit-Position: refs/heads/master@{#376202} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/78dbec96889ea75d9e93b0f166cf7ab68dd209ef Cr-Commit-Position: refs/heads/master@{#376202} |
