Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(20)

Issue 2860443002: Chromad: Create AuthPolicyCredentialsManager

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 weeks, 1 day ago by Roman Sorokin (ftl)
Modified:
3 hours, 27 minutes ago
CC:
chromium-reviews, srahim+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Chromad: Create AuthPolicyCredentialsManager AuthPolicyCredentialsManager KeyedService service is created for every Active Directory user profile. It calls GetUserStatus at the start of service, each hour and on every network connection. Tests are coming... BUG=662400

Patch Set 1 #

Total comments: 55

Patch Set 2 : Rebase, Update after review #

Total comments: 4

Patch Set 3 : Update after review #

Total comments: 10

Patch Set 4 : More addressing comments #

Total comments: 17
Unified diffs Side-by-side diffs Delta from patch set Stats (+397 lines, -0 lines) Patch
M chrome/app/generated_resources.grd View 1 1 chunk +9 lines, -0 lines 4 comments Download
M chrome/browser/chromeos/BUILD.gn View 1 2 chunks +3 lines, -0 lines 0 comments Download
A chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h View 1 2 1 chunk +106 lines, -0 lines 3 comments Download
A chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc View 1 2 3 1 chunk +277 lines, -0 lines 10 comments Download
M chrome/browser/profiles/profile_impl.cc View 1 2 chunks +2 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 26 (17 generated)
Roman Sorokin (ftl)
Hi Thiemo, Lutz, PTAL.
3 weeks, 1 day ago (2017-05-02 14:09:25 UTC) #4
ljusten (instantaneous)
https://codereview.chromium.org/2860443002/diff/1/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2860443002/diff/1/chrome/app/generated_resources.grd#newcode9566 chrome/app/generated_resources.grd:9566: Your password has expired. Please sign out then sign ...
3 weeks ago (2017-05-03 10:12:17 UTC) #7
Roman Sorokin (ftl)
Thanks Lutz, PTAL! https://codereview.chromium.org/2860443002/diff/1/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2860443002/diff/1/chrome/app/generated_resources.grd#newcode9566 chrome/app/generated_resources.grd:9566: Your password has expired. Please sign ...
5 days, 5 hours ago (2017-05-19 12:13:48 UTC) #10
ljusten (instantaneous)
https://codereview.chromium.org/2860443002/diff/1/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2860443002/diff/1/chrome/app/generated_resources.grd#newcode9566 chrome/app/generated_resources.grd:9566: Your password has expired. Please sign out then sign ...
5 days, 1 hour ago (2017-05-19 16:06:14 UTC) #13
Roman Sorokin (ftl)
Thanks, Lutz! PTAL. https://codereview.chromium.org/2860443002/diff/1/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2860443002/diff/1/chrome/app/generated_resources.grd#newcode9566 chrome/app/generated_resources.grd:9566: Your password has expired. Please sign ...
2 days, 5 hours ago (2017-05-22 12:35:27 UTC) #16
ljusten (instantaneous)
https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc (right): https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc#newcode50 chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:50: void FixSignIn(); On 2017/05/22 12:35:27, Roman Sorokin (ftl) wrote: ...
2 days, 3 hours ago (2017-05-22 14:27:49 UTC) #19
Roman Sorokin (ftl)
Thanks Lutz, PTAL! https://codereview.chromium.org/2860443002/diff/40001/chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc (right): https://codereview.chromium.org/2860443002/diff/40001/chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc#newcode107 chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:107: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); On 2017/05/22 14:27:49, ljusten (instantaneous) ...
2 days, 1 hour ago (2017-05-22 16:38:24 UTC) #22
ljusten (instantaneous)
Almost there... https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc (right): https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc#newcode122 chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:122: last_error_ = error; I believe this will ...
2 days ago (2017-05-22 16:52:00 UTC) #23
Thiemo Nagel
7 hours, 34 minutes ago (2017-05-24 10:11:05 UTC) #26
https://codereview.chromium.org/2860443002/diff/60001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2860443002/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:9436: <message
name="IDS_ACTIVE_DIRECTORY_PASSWORD_EXPIRED" desc="The text to display on the
hyperlink when the user needs to sign out and sign in again to change their
password">
Nit: End sentence with a period.

https://codereview.chromium.org/2860443002/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:9439: <message
name="IDS_ACTIVE_DIRECTORY_PASSWORD_CHANGED" desc="The text to display on the
hyperlink when the user needs to sign out and sign in again to update local
password to cryptohome">
Nit: End sentence with a period.

https://codereview.chromium.org/2860443002/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:9440: Your password has been changed. Please
sign out then sign in again to update it.
Nit: Imho "to update it" should be dropped, because there's no way for the user
to understand that "updating it" refers to the cryptohome.  Also, I'd suggest to
add "on the server" after "password has been changed" to make it clearer what
has happened.

https://codereview.chromium.org/2860443002/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:9442: <message
name="IDS_ACTIVE_DIRECTORY_REFRESH_AUTH_TOKEN" desc="The text to display on the
hyperlink when the user needs to sign out and sign in again to get
authentication token">
Nit: End sentence with a period.

https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc
(right):

https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:32: const
base::TimeDelta kGetUserStatusCallsInterval =
Global objects must be constexpr.

https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:47:
~SigninNotificationDelegate() override;
Are you sure this is necessary?  Why not just use the default destructor?

https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:76: :
profile_(profile), weak_factory_(this) {
Nit: For weak_factory_, I'd prefer in-class initialization:

... weak_factory_{this};

https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:77:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
Nit: Any particular reason you're checking for this?  Somehow I can't imagine
someone trying to run this on a non-UI thread...

(Same below.)

https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:78:
StartObserveNetwork();
Nit: I prefer to have CHECK()'s at the top of the function so that the
assertions are easily glanceable.  Could you please move StartObserveNetwork()
below the CHECK()?

https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:107:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
Same as above.

https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:122:
last_error_ = error;
On 2017/05/22 16:52:00, ljusten (instantaneous) wrote:
> I believe this will call GetUserStatus() over and over again if
> should_call_get_status_again_ is true and the calls keep failing. Do this
> instead:
> bool call_again = should_call_get_status_again_;
> should_call_get_status_again_ = false;
> ...
>   if (call_again)

Simply setting |should_call_get_status_again_| to false before the return should
be sufficient, imho.

https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:163:
&AuthPolicyCredentialsManager::GetUserStatus, base::Unretained(this)));
Why is unretained safe here?  Maybe add a comment?

https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:164:
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
Afaics, this does not re-schedule after wake from sleep (and thus the maximal
interval between two calls can be (sleep time + kGetUserStatusCallsInterval)). 
No need to fix this right away, but I think it would be good to include a TODO.

https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h
(right):

https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:49: // See
AuthPolicyClient::GetUserStatusCallback.
Nit: Imho it's more readable to have a blank line above the comment.  (Same
below.)

https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:75: bool
should_call_get_status_again_ = false;
The name of that bool imho doesn't do a great job describing what the variable
does.  What about e.g. |rerun_get_status_on_error_|?

https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:76: bool
was_notification_shown_ = false;
Imho a single bool is not sufficient because we might want to show multiple
notifications e.g. in case first the TGT expires and later the password changes
on the server.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06