ForceSigninVerifier does not work with network-service API. |
|||||||||||
Issue descriptionChrome Version: 71 OS: Win/Mac What steps will reproduce the problem? (1) Enables ForceSignin Policy, Launch Chrome and sign in. (2) Enables network-service via chrome://flags (3) Close Chrome. (4) Change the password of signed in account. (In a separate Chrome instance) (5) Launch Chrome What is the expected result? Because the password change, Chrome should be close after launch. UserManager is shown. What happens instead? Chrome is not closed.
,
Oct 3
,
Oct 3
The reason is the network status API is changed from IsOffLine to GetConnectionType without handling the async case properly. https://chromium.googlesource.com/chromium/src/+/aa419b17b2bd2addeefffe0bdb44e0392eb144f5
,
Oct 3
Here is the fix, working its test: https://chromium-review.googlesource.com/c/chromium/src/+/1259542
,
Oct 3
,
Oct 3
,
Oct 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/29bdbadd7cf6fbe34d46402c5ce5b019c214a99b commit 29bdbadd7cf6fbe34d46402c5ce5b019c214a99b Author: Owen Min <zmin@chromium.org> Date: Mon Oct 08 14:48:12 2018 Handle case that network status returned asynchronously in ForceSigninVerifier. When the GetConnectionType returns asynchronously, verification request is never sent because the default network type is CONNECTION_NONE and there is no network change notification later either. Resolves the issue by handling the callback from the API. Also 1) Separate network check from others so that we don't fetch network type again when we know it. 2) Rewrite the unittest to use more fake services instead of stun value. It increase the code coverage. Including SigninManager, ProfileOAuth2TokenService and TestNetworkConnectionTracker. Bug: 891817 Change-Id: Iebdc5bbc30e699b347d6a343bb6f4c979fbe1596 Reviewed-on: https://chromium-review.googlesource.com/c/1259542 Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Commit-Queue: Owen Min <zmin@chromium.org> Cr-Commit-Position: refs/heads/master@{#597560} [modify] https://crrev.com/29bdbadd7cf6fbe34d46402c5ce5b019c214a99b/chrome/browser/signin/force_signin_verifier.cc [modify] https://crrev.com/29bdbadd7cf6fbe34d46402c5ce5b019c214a99b/chrome/browser/signin/force_signin_verifier.h [modify] https://crrev.com/29bdbadd7cf6fbe34d46402c5ce5b019c214a99b/chrome/browser/signin/force_signin_verifier_unittest.cc
,
Oct 9
I'd like to merge this into M70 as this fixes an issue for ForceBrowserSignin Policy under #network-service flag. The patch is verified on Canary(71.0.3574.0) and covered with unit test.
,
Oct 9
This bug requires manual review: We are only 6 days from stable. Please contact the milestone owner if you have questions. Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 9
,
Oct 9
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f96c67466725b19be2ba5306bc8f723c790311ba commit f96c67466725b19be2ba5306bc8f723c790311ba Author: Owen Min <zmin@chromium.org> Date: Tue Oct 09 18:44:02 2018 Merge "Handle case that network status returned asynchronously in ForceSigninVerifier" into M70 When the GetConnectionType returns asynchronously, verification request is never sent because the default network type is CONNECTION_NONE and there is no network change notification later either. Resolves the issue by handling the callback from the API. Also 1) Separate network check from others so that we don't fetch network type again when we know it. 2) Rewrite the unittest to use more fake services instead of stun value. It increase the code coverage. Including SigninManager, ProfileOAuth2TokenService and TestNetworkConnectionTracker. Bug: 891817 Change-Id: Iebdc5bbc30e699b347d6a343bb6f4c979fbe1596 Reviewed-on: https://chromium-review.googlesource.com/c/1259542 Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Commit-Queue: Owen Min <zmin@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#597560}(cherry picked from commit 29bdbadd7cf6fbe34d46402c5ce5b019c214a99b) Reviewed-on: https://chromium-review.googlesource.com/c/1271407 Reviewed-by: Owen Min <zmin@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#926} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/f96c67466725b19be2ba5306bc8f723c790311ba/chrome/browser/signin/force_signin_verifier.cc [modify] https://crrev.com/f96c67466725b19be2ba5306bc8f723c790311ba/chrome/browser/signin/force_signin_verifier.h [modify] https://crrev.com/f96c67466725b19be2ba5306bc8f723c790311ba/chrome/browser/signin/force_signin_verifier_unittest.cc
,
Oct 9
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f96c67466725b19be2ba5306bc8f723c790311ba Commit: f96c67466725b19be2ba5306bc8f723c790311ba Author: zmin@chromium.org Commiter: zmin@chromium.org Date: 2018-10-09 18:44:02 +0000 UTC Merge "Handle case that network status returned asynchronously in ForceSigninVerifier" into M70 When the GetConnectionType returns asynchronously, verification request is never sent because the default network type is CONNECTION_NONE and there is no network change notification later either. Resolves the issue by handling the callback from the API. Also 1) Separate network check from others so that we don't fetch network type again when we know it. 2) Rewrite the unittest to use more fake services instead of stun value. It increase the code coverage. Including SigninManager, ProfileOAuth2TokenService and TestNetworkConnectionTracker. Bug: 891817 Change-Id: Iebdc5bbc30e699b347d6a343bb6f4c979fbe1596 Reviewed-on: https://chromium-review.googlesource.com/c/1259542 Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Commit-Queue: Owen Min <zmin@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#597560}(cherry picked from commit 29bdbadd7cf6fbe34d46402c5ce5b019c214a99b) Reviewed-on: https://chromium-review.googlesource.com/c/1271407 Reviewed-by: Owen Min <zmin@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#926} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
,
Oct 9
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/644192245a57189a6e7ce18358ab853f65613a79 commit 644192245a57189a6e7ce18358ab853f65613a79 Author: Abdul Syed <abdulsyed@google.com> Date: Tue Oct 09 22:24:51 2018 Revert "Merge "Handle case that network status returned asynchronously in ForceSigninVerifier" into M70" This reverts commit f96c67466725b19be2ba5306bc8f723c790311ba. Reason for revert: <INSERT REASONING HERE> Original change's description: > Merge "Handle case that network status returned asynchronously in ForceSigninVerifier" into M70 > > When the GetConnectionType returns asynchronously, verification request is never > sent because the default network type is CONNECTION_NONE and there is no network > change notification later either. > > Resolves the issue by handling the callback from the API. > > Also > 1) Separate network check from others so that we don't fetch network type again > when we know it. > 2) Rewrite the unittest to use more fake services instead of stun value. It > increase the code coverage. Including SigninManager, ProfileOAuth2TokenService > and TestNetworkConnectionTracker. > > Bug: 891817 > Change-Id: Iebdc5bbc30e699b347d6a343bb6f4c979fbe1596 > Reviewed-on: https://chromium-review.googlesource.com/c/1259542 > Reviewed-by: Mihai Sardarescu <msarda@chromium.org> > Commit-Queue: Owen Min <zmin@chromium.org> > Cr-Original-Commit-Position: refs/heads/master@{#597560}(cherry picked from commit 29bdbadd7cf6fbe34d46402c5ce5b019c214a99b) > Reviewed-on: https://chromium-review.googlesource.com/c/1271407 > Reviewed-by: Owen Min <zmin@chromium.org> > Cr-Commit-Position: refs/branch-heads/3538@{#926} > Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} TBR=zmin@chromium.org Change-Id: I36996bb8ff58ff608be1f9df3543c2d53816279c No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 891817 Reviewed-on: https://chromium-review.googlesource.com/c/1272257 Reviewed-by: Abdul Syed <abdulsyed@google.com> Cr-Commit-Position: refs/branch-heads/3538@{#933} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/644192245a57189a6e7ce18358ab853f65613a79/chrome/browser/signin/force_signin_verifier.cc [modify] https://crrev.com/644192245a57189a6e7ce18358ab853f65613a79/chrome/browser/signin/force_signin_verifier.h [modify] https://crrev.com/644192245a57189a6e7ce18358ab853f65613a79/chrome/browser/signin/force_signin_verifier_unittest.cc
,
Oct 9
Breaking beta so I've reverted the change, https://chromium-review.googlesource.com/c/chromium/src/+/1272257.
,
Oct 9
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/644192245a57189a6e7ce18358ab853f65613a79 Commit: 644192245a57189a6e7ce18358ab853f65613a79 Author: abdulsyed@google.com Commiter: abdulsyed@google.com Date: 2018-10-09 22:24:51 +0000 UTC Revert "Merge "Handle case that network status returned asynchronously in ForceSigninVerifier" into M70" This reverts commit f96c67466725b19be2ba5306bc8f723c790311ba. Reason for revert: <INSERT REASONING HERE> Original change's description: > Merge "Handle case that network status returned asynchronously in ForceSigninVerifier" into M70 > > When the GetConnectionType returns asynchronously, verification request is never > sent because the default network type is CONNECTION_NONE and there is no network > change notification later either. > > Resolves the issue by handling the callback from the API. > > Also > 1) Separate network check from others so that we don't fetch network type again > when we know it. > 2) Rewrite the unittest to use more fake services instead of stun value. It > increase the code coverage. Including SigninManager, ProfileOAuth2TokenService > and TestNetworkConnectionTracker. > > Bug: 891817 > Change-Id: Iebdc5bbc30e699b347d6a343bb6f4c979fbe1596 > Reviewed-on: https://chromium-review.googlesource.com/c/1259542 > Reviewed-by: Mihai Sardarescu <msarda@chromium.org> > Commit-Queue: Owen Min <zmin@chromium.org> > Cr-Original-Commit-Position: refs/heads/master@{#597560}(cherry picked from commit 29bdbadd7cf6fbe34d46402c5ce5b019c214a99b) > Reviewed-on: https://chromium-review.googlesource.com/c/1271407 > Reviewed-by: Owen Min <zmin@chromium.org> > Cr-Commit-Position: refs/branch-heads/3538@{#926} > Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} TBR=zmin@chromium.org Change-Id: I36996bb8ff58ff608be1f9df3543c2d53816279c No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 891817 Reviewed-on: https://chromium-review.googlesource.com/c/1272257 Reviewed-by: Abdul Syed <abdulsyed@google.com> Cr-Commit-Position: refs/branch-heads/3538@{#933} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
,
Oct 9
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/54e072e2c96a8d7aa5f8136ac1a49b06c469233c commit 54e072e2c96a8d7aa5f8136ac1a49b06c469233c Author: Owen Min <zmin@chromium.org> Date: Tue Oct 09 23:19:26 2018 Merge Handle case that network status returned asynchronously in ForceSigninVerifier. Relands the CL in M70. This CL reverts the unittest from the original CL as it's using a test API only avaiable in M71. It also changes ShouldSendRequest to virual functions as it's required by old unit tests. When the GetConnectionType returns asynchronously, verification request is never sent because the default network type is CONNECTION_NONE and there is no network change notification later either. Resolves the issue by handling the callback from the API. Also 1) Separate network check from others so that we don't fetch network type again when we know it. 2) Rewrite the unittest to use more fake services instead of stun value. It increase the code coverage. Including SigninManager, ProfileOAuth2TokenService and TestNetworkConnectionTracker. Bug: 891817 Change-Id: Ibf63b5300b70658daf041551d5810c36890144d4 Reviewed-on: https://chromium-review.googlesource.com/c/1259542 Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Commit-Queue: Owen Min <zmin@chromium.org> Cr-Original-Original-Commit-Position: refs/heads/master@{#597560}(cherry picked from commit 29bdbadd7cf6fbe34d46402c5ce5b019c214a99b) TBR: msarda Change-Id: Ibf63b5300b70658daf041551d5810c36890144d4 Reviewed-on: https://chromium-review.googlesource.com/c/1272119 Reviewed-by: Owen Min <zmin@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#936} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/54e072e2c96a8d7aa5f8136ac1a49b06c469233c/chrome/browser/signin/force_signin_verifier.cc [modify] https://crrev.com/54e072e2c96a8d7aa5f8136ac1a49b06c469233c/chrome/browser/signin/force_signin_verifier.h
,
Oct 9
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/54e072e2c96a8d7aa5f8136ac1a49b06c469233c Commit: 54e072e2c96a8d7aa5f8136ac1a49b06c469233c Author: zmin@chromium.org Commiter: zmin@chromium.org Date: 2018-10-09 23:19:26 +0000 UTC Merge Handle case that network status returned asynchronously in ForceSigninVerifier. Relands the CL in M70. This CL reverts the unittest from the original CL as it's using a test API only avaiable in M71. It also changes ShouldSendRequest to virual functions as it's required by old unit tests. When the GetConnectionType returns asynchronously, verification request is never sent because the default network type is CONNECTION_NONE and there is no network change notification later either. Resolves the issue by handling the callback from the API. Also 1) Separate network check from others so that we don't fetch network type again when we know it. 2) Rewrite the unittest to use more fake services instead of stun value. It increase the code coverage. Including SigninManager, ProfileOAuth2TokenService and TestNetworkConnectionTracker. Bug: 891817 Change-Id: Ibf63b5300b70658daf041551d5810c36890144d4 Reviewed-on: https://chromium-review.googlesource.com/c/1259542 Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Commit-Queue: Owen Min <zmin@chromium.org> Cr-Original-Original-Commit-Position: refs/heads/master@{#597560}(cherry picked from commit 29bdbadd7cf6fbe34d46402c5ce5b019c214a99b) TBR: msarda Change-Id: Ibf63b5300b70658daf041551d5810c36890144d4 Reviewed-on: https://chromium-review.googlesource.com/c/1272119 Reviewed-by: Owen Min <zmin@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#936} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
,
Oct 11
|
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by zmin@chromium.org
, Oct 3