Add policy for limiting the session length

Add policy for limiting the session length

If this policy is set, the user is logged out when the time since login
reaches the limit. The CL adds a SessionLengthLimiter that enforces the
limit. A separate CL will add the UI that shows a countdown timer to the
user.

BUG=chromium-os:26957
TEST=Manual

TBR=sky@chromium.org
(vor chrome_browser_chromeos.gypi)

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173285

[bartfab (slow), 2012-12-11 23:29:20]

Hi everyone,

This is missing part of public accounts that we want to land for M25. A swift
review is very much appreciated :).

Nikita,
Could you look at c/b/chromeos/* and c/b/chromeos/login/*?

Mattias,
Could you look at the policy changes?

Sailesh,
Could you look at the profile_dependency_manager.cc changes?

[bartfab (slow), 2012-12-11 23:32:04]

https://codereview.chromium.org/11499012/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/user_manager.h (right):

https://codereview.chromium.org/11499012/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/user_manager.h:195: virtual bool
HasBrowserRestarted() const = 0;
I am not sure whether the UserManager is the best home for this method. However,
as the UserManager does keep track of the current login/session state, it was
the most reasonable place I could find.

Also, making this method available to the UserManager allows an exisiting ad-hoc
check in the UserManager code to be replaced with an explicit call to the
method.

[sail, 2012-12-12 04:06:56]

profiles/* LGTM

[Mattias Nissler (ping if slow), 2012-12-12 10:12:57]

https://codereview.chromium.org/11499012/diff/6001/chrome/app/policy/policy_t...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/11499012/diff/6001/chrome/app/policy/policy_t...
chrome/app/policy/policy_templates.json:3833: When this policy is set, it
specifies the the length of time after which a user is automatically logged out,
terminating the session. The user is informed about the remaining time by a
countdown timer shown in the ash system tray.
s/the the/the/
Remove "ash"? People don't know what ash is, system tray is probably good
enough. What wording do we use elsewhere?

https://codereview.chromium.org/11499012/diff/6001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/session_length_limiter.cc (right):

https://codereview.chromium.org/11499012/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/session_length_limiter.cc:100:
pref_change_registrar_.RemoveAll();
not needed AFAICS

https://codereview.chromium.org/11499012/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/session_length_limiter.cc:163:
scoped_ptr<base::TimeDelta> SessionLengthLimiter::GetRemainingTime() const {
Wrapping in a scoped_ptr for returning an optional value looks a bit exotic to
me, most of Chromium uses bool GetRemainingTime(base::TimeDelta* remaining_time)
in my experience.

https://codereview.chromium.org/11499012/diff/6001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/session_length_limiter.h (right):

https://codereview.chromium.org/11499012/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/session_length_limiter.h:42: virtual void Logout() = 0;
Call this StopSession() maybe?

https://codereview.chromium.org/11499012/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/session_length_limiter.h:58: PrefChangeRegistrar
pref_change_registrar_;
Might want to use an IntegerPrefMember instead.

https://codereview.chromium.org/11499012/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/session_length_limiter.h:69: void TimerTick();
These functions could use some commenting on what they do.

https://codereview.chromium.org/11499012/diff/6001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/session_length_limiter_factory.h (right):

https://codereview.chromium.org/11499012/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/session_length_limiter_factory.h:35: virtual bool
ServiceIsCreatedWithProfile() const OVERRIDE;
That's interesting. In Chrome OS, there are at least two profiles (login profile
and user profile). So we have a session length limiter for both? Any reason to
tie this to Profile in the first place?

https://codereview.chromium.org/11499012/diff/6001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

https://codereview.chromium.org/11499012/diff/6001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_dependency_manager.cc:16: #endif
By convention, conditional includes go below the main include list.

https://codereview.chromium.org/11499012/diff/6001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/11499012/diff/6001/chrome/common/pref_names.c...
chrome/common/pref_names.cc:69: #if defined(OS_CHROMEOS)
ditto

https://codereview.chromium.org/11499012/diff/6001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/11499012/diff/6001/chrome/common/pref_names.h...
chrome/common/pref_names.h:28: #if defined(OS_CHROMEOS)
There's an entire block of OS_CHROMEOS prefs below that I suggest you add these
to.

[bartfab (slow), 2012-12-13 16:22:48]

https://chromiumcodereview.appspot.com/11499012/diff/6001/chrome/app/policy/p...
File chrome/app/policy/policy_templates.json (right):

https://chromiumcodereview.appspot.com/11499012/diff/6001/chrome/app/policy/p...
chrome/app/policy/policy_templates.json:3833: When this policy is set, it
specifies the the length of time after which a user is automatically logged out,
terminating the session. The user is informed about the remaining time by a
countdown timer shown in the ash system tray.
On 2012/12/12 10:12:57, Mattias Nissler wrote:
> s/the the/the/
> Remove "ash"? People don't know what ash is, system tray is probably good
> enough. What wording do we use elsewhere?

"the the" - Done.
"ash" - Done. There was one other policy that referred to the "ash system tray".
I replaced the term with just "system tray" there as well.

https://chromiumcodereview.appspot.com/11499012/diff/6001/chrome/browser/chro...
File chrome/browser/chromeos/session_length_limiter.cc (right):

https://chromiumcodereview.appspot.com/11499012/diff/6001/chrome/browser/chro...
chrome/browser/chromeos/session_length_limiter.cc:100:
pref_change_registrar_.RemoveAll();
On 2012/12/12 10:12:57, Mattias Nissler wrote:
> not needed AFAICS

Correct. I only did this because other PKS do it, "for good measure".

https://chromiumcodereview.appspot.com/11499012/diff/6001/chrome/browser/chro...
chrome/browser/chromeos/session_length_limiter.cc:163:
scoped_ptr<base::TimeDelta> SessionLengthLimiter::GetRemainingTime() const {
On 2012/12/12 10:12:57, Mattias Nissler wrote:
> Wrapping in a scoped_ptr for returning an optional value looks a bit exotic to
> me, most of Chromium uses bool GetRemainingTime(base::TimeDelta*
remaining_time)
> in my experience.

Done.

https://chromiumcodereview.appspot.com/11499012/diff/6001/chrome/browser/chro...
File chrome/browser/chromeos/session_length_limiter.h (right):

https://chromiumcodereview.appspot.com/11499012/diff/6001/chrome/browser/chro...
chrome/browser/chromeos/session_length_limiter.h:42: virtual void Logout() = 0;
On 2012/12/12 10:12:57, Mattias Nissler wrote:
> Call this StopSession() maybe?

Done.

https://chromiumcodereview.appspot.com/11499012/diff/6001/chrome/browser/chro...
chrome/browser/chromeos/session_length_limiter.h:58: PrefChangeRegistrar
pref_change_registrar_;
On 2012/12/12 10:12:57, Mattias Nissler wrote:
> Might want to use an IntegerPrefMember instead.

I used a PrefChangeRegistrary because I want to access the pref's IsDefault(), a
method that IntegerPrefMember does not provide. I am using this method to check
whether a session limit has been set. Alternatives would be:
* Check whether the current value is != 0. This is checking for magic values =>
bad.
* Check for IsManaged() instead => makes it impossible to just specify a limit
via prefs. Granted, nobody ever will but if it works, why break it?
* Extend IntegerPrefMember to provide IsDefault() => can do if you if want me
to.

https://chromiumcodereview.appspot.com/11499012/diff/6001/chrome/browser/chro...
chrome/browser/chromeos/session_length_limiter.h:69: void TimerTick();
On 2012/12/12 10:12:57, Mattias Nissler wrote:
> These functions could use some commenting on what they do.

Done.

https://chromiumcodereview.appspot.com/11499012/diff/6001/chrome/browser/chro...
File chrome/browser/chromeos/session_length_limiter_factory.h (right):

https://chromiumcodereview.appspot.com/11499012/diff/6001/chrome/browser/chro...
chrome/browser/chromeos/session_length_limiter_factory.h:35: virtual bool
ServiceIsCreatedWithProfile() const OVERRIDE;
On 2012/12/12 10:12:57, Mattias Nissler wrote:
> That's interesting. In Chrome OS, there are at least two profiles (login
profile
> and user profile). So we have a session length limiter for both? Any reason to
> tie this to Profile in the first place?

Your are right. This did not work. I refactored the code so that it no longer is
a PKS.

https://chromiumcodereview.appspot.com/11499012/diff/6001/chrome/browser/prof...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

https://chromiumcodereview.appspot.com/11499012/diff/6001/chrome/browser/prof...
chrome/browser/profiles/profile_dependency_manager.cc:16: #endif
On 2012/12/12 10:12:57, Mattias Nissler wrote:
> By convention, conditional includes go below the main include list.

Done.

https://chromiumcodereview.appspot.com/11499012/diff/6001/chrome/common/pref_...
File chrome/common/pref_names.cc (right):

https://chromiumcodereview.appspot.com/11499012/diff/6001/chrome/common/pref_...
chrome/common/pref_names.cc:69: #if defined(OS_CHROMEOS)
On 2012/12/12 10:12:57, Mattias Nissler wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/11499012/diff/6001/chrome/common/pref_...
File chrome/common/pref_names.h (right):

https://chromiumcodereview.appspot.com/11499012/diff/6001/chrome/common/pref_...
chrome/common/pref_names.h:28: #if defined(OS_CHROMEOS)
On 2012/12/12 10:12:57, Mattias Nissler wrote:
> There's an entire block of OS_CHROMEOS prefs below that I suggest you add
these
> to.

Done.

[Nikita (slow), 2012-12-14 11:56:10]

https://chromiumcodereview.appspot.com/11499012/diff/11001/chrome/browser/chr...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/11499012/diff/11001/chrome/browser/chr...
chrome/browser/chromeos/login/user_manager_impl.cc:238:
session_length_limiter_.reset(new SessionLengthLimiter(NULL,
Why do you need to create session length limiter for every non-Guest/non-Retail
user?

Will that be a noop for consumer user when policy is not defined? Maybe makes
sense to not create this and only create if policy that limits session lenght is
either defined during login. If policy will update during user session, you
could also create session limiter instance if needed.

[bartfab (slow), 2012-12-14 11:59:28]

https://chromiumcodereview.appspot.com/11499012/diff/11001/chrome/browser/chr...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/11499012/diff/11001/chrome/browser/chr...
chrome/browser/chromeos/login/user_manager_impl.cc:238:
session_length_limiter_.reset(new SessionLengthLimiter(NULL,
On 2012/12/14 11:56:10, Nikita Kostylev wrote:
> Why do you need to create session length limiter for every
non-Guest/non-Retail
> user?
> 
> Will that be a noop for consumer user when policy is not defined? Maybe makes
> sense to not create this and only create if policy that limits session lenght
is
> either defined during login. If policy will update during user session, you
> could also create session limiter instance if needed.

Sure, I can create the limiter on demand. But that would mean moving the code
that listens for policy changes into the UserManager so that it can construct
and destruct the limiter on the fly. It seems much cleaner and maintainable to
me to encapsulate this in one place. If the policy is not enabled, the limiter
simply does nothing after all. It just waits for the policy to be enabled, just
like the UserManager would if I moved the code there.

And yes, I enabled this code for regular users as well. This is because the
policy is useful for regular users as well.

[Nikita (slow), 2012-12-14 12:03:06]

https://chromiumcodereview.appspot.com/11499012/diff/11001/chrome/browser/chr...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/11499012/diff/11001/chrome/browser/chr...
chrome/browser/chromeos/login/user_manager_impl.cc:595: return
command_line->HasSwitch(switches::kLoginManager) &&
When browser will be restarted after crash, --login-manager will *not* be
passed.
--login-user will be passed.

You don't have to check for --login-password here. That's a debug switch only.

Previous check also had base::chromeos::IsRunningOnChromeOS() which was not
transfered here.

[Nikita (slow), 2012-12-14 12:14:07]

https://chromiumcodereview.appspot.com/11499012/diff/11001/chrome/browser/chr...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/11499012/diff/11001/chrome/browser/chr...
chrome/browser/chromeos/login/user_manager_impl.cc:238:
session_length_limiter_.reset(new SessionLengthLimiter(NULL,
On 2012/12/14 11:59:28, bartfab wrote:
> On 2012/12/14 11:56:10, Nikita Kostylev wrote:
> > Why do you need to create session length limiter for every
> non-Guest/non-Retail
> > user?
> > 
> > Will that be a noop for consumer user when policy is not defined? Maybe
makes
> > sense to not create this and only create if policy that limits session
lenght
> is
> > either defined during login. If policy will update during user session, you
> > could also create session limiter instance if needed.
> 
> Sure, I can create the limiter on demand. But that would mean moving the code
> that listens for policy changes into the UserManager so that it can construct
> and destruct the limiter on the fly. It seems much cleaner and maintainable to
> me to encapsulate this in one place. If the policy is not enabled, the limiter
> simply does nothing after all. It just waits for the policy to be enabled,
just
> like the UserManager would if I moved the code there.
> 
> And yes, I enabled this code for regular users as well. This is because the
> policy is useful for regular users as well.

Agree, discussed.

https://chromiumcodereview.appspot.com/11499012/diff/11001/chrome/browser/chr...
File chrome/browser/chromeos/power/session_length_limiter.cc (right):

https://chromiumcodereview.appspot.com/11499012/diff/11001/chrome/browser/chr...
chrome/browser/chromeos/power/session_length_limiter.cc:129: StartTimer();
As discussed, SessionLengthLimiter should do no actions at all when created for
the account that doesn't have a session limit policy.

[bartfab (slow), 2012-12-14 12:24:41]

https://chromiumcodereview.appspot.com/11499012/diff/11001/chrome/browser/chr...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/11499012/diff/11001/chrome/browser/chr...
chrome/browser/chromeos/login/user_manager_impl.cc:595: return
command_line->HasSwitch(switches::kLoginManager) &&
On 2012/12/14 12:03:06, Nikita Kostylev wrote:
> When browser will be restarted after crash, --login-manager will *not* be
> passed.
> --login-user will be passed.
> 
> You don't have to check for --login-password here. That's a debug switch only.
> 
> Previous check also had base::chromeos::IsRunningOnChromeOS() which was not
> transfered here.

The wrong pref name was a typo. Sorry. What I meant to do is to replicate the
logic from chrome_browser_main_chromeos.cc. Citing from there (line 452 onward):

// There are two use cases for kLoginUser:
//   1) if passed in tandem with kLoginPassword, to drive a "StubLogin"
//   2) if passed alone, to signal that the indicated user has already
//      logged in and we should behave accordingly.

The code here is meant to find out whether we are in case 2. Hence, it checks
whether a password was given or not.

https://chromiumcodereview.appspot.com/11499012/diff/11001/chrome/browser/chr...
File chrome/browser/chromeos/power/session_length_limiter.cc (right):

https://chromiumcodereview.appspot.com/11499012/diff/11001/chrome/browser/chr...
chrome/browser/chromeos/power/session_length_limiter.cc:129: StartTimer();
On 2012/12/14 12:14:08, Nikita Kostylev wrote:
> As discussed, SessionLengthLimiter should do no actions at all when created
for
> the account that doesn't have a session limit policy.

Done. What the limiter does when the limit is not set is (see constructor):

* Write the login time to prefs. This is necessary because a limit could appear
later and we would need to find out when it was that we actually logged in.
* Start listening for changes to the limit pref.

Note that the limiter *never* started the timer when no limit was set. This
method here would just return in line 121.

[Nikita (slow), 2012-12-14 12:54:22]

>> SessionLengthLimiter is no longer a PKS

Makes sense to update description.

[Nikita (slow), 2012-12-14 13:07:42]

lgtm

https://chromiumcodereview.appspot.com/11499012/diff/15002/chrome/browser/chr...
File chrome/browser/chromeos/power/session_length_limiter.cc (right):

https://chromiumcodereview.appspot.com/11499012/diff/15002/chrome/browser/chr...
chrome/browser/chromeos/power/session_length_limiter.cc:31: const int
kSessionLengthLimitMaxMs = 24 * 60 * 60 * 1000; // 24 hours.
Generic questions: I think you ignore time being spent in sleep mode?

https://chromiumcodereview.appspot.com/11499012/diff/15002/chrome/browser/chr...
chrome/browser/chromeos/power/session_length_limiter.cc:35: const int
kSessionLengthLimitTimerIntervalMs = 1000;
Seems to often, no?

https://chromiumcodereview.appspot.com/11499012/diff/15002/chrome/browser/chr...
chrome/browser/chromeos/power/session_length_limiter.cc:85: // If this is a user
login, set the session start time in local state to the
nit: Insert empty line before comment to make it more readable.

https://chromiumcodereview.appspot.com/11499012/diff/15002/chrome/browser/chr...
File chrome/browser/chromeos/power/session_length_limiter.h (right):

https://chromiumcodereview.appspot.com/11499012/diff/15002/chrome/browser/chr...
chrome/browser/chromeos/power/session_length_limiter.h:19: class
SessionLengthLimiter {
nit: Class level comment about purpose and usage of this class.

https://chromiumcodereview.appspot.com/11499012/diff/15002/chrome/browser/chr...
chrome/browser/chromeos/power/session_length_limiter.h:44: void
OnSessionLengthLimitChanged();
nit: Methods should go first.

[bartfab (slow), 2012-12-14 17:23:26]

https://chromiumcodereview.appspot.com/11499012/diff/15002/chrome/browser/chr...
File chrome/browser/chromeos/power/session_length_limiter.cc (right):

https://chromiumcodereview.appspot.com/11499012/diff/15002/chrome/browser/chr...
chrome/browser/chromeos/power/session_length_limiter.cc:31: const int
kSessionLengthLimitMaxMs = 24 * 60 * 60 * 1000; // 24 hours.
On 2012/12/14 13:07:43, Nikita Kostylev wrote:
> Generic questions: I think you ignore time being spent in sleep mode?

No. The logout is based on wall time since login.

https://chromiumcodereview.appspot.com/11499012/diff/15002/chrome/browser/chr...
chrome/browser/chromeos/power/session_length_limiter.cc:35: const int
kSessionLengthLimitTimerIntervalMs = 1000;
On 2012/12/14 13:07:43, Nikita Kostylev wrote:
> Seems to often, no?

This is so that the UI can be updated once a second. I realized too late that
instead of sending periodic notifications, I should have put a timer directly
into the UI. I will improve this after m25.

https://chromiumcodereview.appspot.com/11499012/diff/15002/chrome/browser/chr...
chrome/browser/chromeos/power/session_length_limiter.cc:85: // If this is a user
login, set the session start time in local state to the
On 2012/12/14 13:07:43, Nikita Kostylev wrote:
> nit: Insert empty line before comment to make it more readable.

Done.

https://chromiumcodereview.appspot.com/11499012/diff/15002/chrome/browser/chr...
File chrome/browser/chromeos/power/session_length_limiter.h (right):

https://chromiumcodereview.appspot.com/11499012/diff/15002/chrome/browser/chr...
chrome/browser/chromeos/power/session_length_limiter.h:19: class
SessionLengthLimiter {
On 2012/12/14 13:07:43, Nikita Kostylev wrote:
> nit: Class level comment about purpose and usage of this class.

Done.

https://chromiumcodereview.appspot.com/11499012/diff/15002/chrome/browser/chr...
chrome/browser/chromeos/power/session_length_limiter.h:44: void
OnSessionLengthLimitChanged();
On 2012/12/14 13:07:43, Nikita Kostylev wrote:
> nit: Methods should go first.

Done.

[commit-bot: I haz the power, 2012-12-14 17:23:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/11499012/12007

[commit-bot: I haz the power, 2012-12-14 17:23:57]

Presubmit check for 11499012-12007 failed and returned exit status 1.


Running presubmit commit checks ...
Finished checking
/b/commit-queue/workdir/chromium/chrome/app/policy/policy_templates.json. 0
errors, 0 warnings.

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 2.3s to calculate.

[bartfab (slow), 2012-12-14 17:29:16]

TBR'd Scott for chrome_browser_chromeos.gypi and notification types.

[commit-bot: I haz the power, 2012-12-14 17:30:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/11499012/12007

[commit-bot: I haz the power, 2012-12-14 19:12:31]

Retried try job too often on linux_chromeos for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-14 20:59:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/11499012/19005

[commit-bot: I haz the power, 2012-12-14 20:59:11]

Presubmit check for 11499012-19005 failed and returned exit status 1.


Running presubmit commit checks ...
Finished checking
/b/commit-queue/workdir/chromium/chrome/app/policy/policy_templates.json. 0
errors, 0 warnings.

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 2.1s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[bartfab (slow), 2012-12-14 20:59:23]

The UI that will show the remaining session time is here:

https://chromiumcodereview.appspot.com/11568036/

During the review of that CL, it became apparent that sending periodic
notifications from the backend to the UI frontend is neither necessary nor
desirable. I therefore removed the periodic notifications. I did leave the
periodic timer, even if it does very little every second now. The periodic timer
was well-tested and I did not want to break the logic at the last second by
switching to a one-shot timer.

I will update the limiter in the future to use a one-shot timer.

[commit-bot: I haz the power, 2012-12-14 21:02:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/11499012/19005

[commit-bot: I haz the power, 2012-12-15 00:02:39]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-15 07:17:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/11499012/19005

[bartfab (slow), 2012-12-15 12:03:42]

Committed as http://crrev.com/173285