The design of the locked down panels is such that we can really only have a single privilege governing the entire content of the panel...

(In reply to comment #1)
> The design of the locked down panels is such that we can really only have a
> single privilege governing the entire content of the panel...

Is it? I can change some personal details in the user panel. So at least it's possible to have some widgets which are sensitive even when the panel is locked.

Note that I'm not asking to have Unlock deal with more than one privilege. I'm just asking that on top of the current Unlock feature, the sensitivity of some widgets can be set based of another privilege.

I think what might work is to have some unprivileged, always-available controls on an otherwise locked-down page. Not sure about more complicated setups...

Created attachment 209486 [details] [review]
datetime: Allow changing timezone without auth if action is allowed

As changing the time can have security implications, while changing the
timezone doesn't, it's not unusual to have a setup where
org.freedesktop.timedate1.set-timezone is allowed while other
time-related actions aren't.

Therefore, if org.freedesktop.timedate1.set-timezone is allowed, there's
no reason to require that the user unlocks the panel to enable him to
change the timezone.

No, we need to install a file that'll take care of _all_ the permissions at once when we click "unlock".

See bug 665671

(In reply to comment #5)
> No, we need to install a file that'll take care of _all_ the permissions at
> once when we click "unlock".

Huh, no. This is already done for the date & time panel. And this is not what this bug is about: currently, I'm required to authenticate with a root password to change the timezone via this Unlock button, while I don't need to authenticate at all to change the timezone. This is broken, as that's an action that we need quite often (as opposed to changing the time, which isn't needed that often).

For the record, we'll ship this patch in openSUSE.

This patch indeed works good.
It allows the timezone to be changed by a traveling non-root user.. really annoying to not have the right time visible.

The 'unlock' button is not affected, as it keeps on unlocking all features. This patch simply allows to have a polkit rule allowing to set the timezone independently...

Created attachment 242006 [details] [review]
unlock timezone widget automatically if allowed by policy

We in openSUSE would appreciate another review of this patch - it got disabled in openSUSE recently and it was quickly missed. The attached version applies cleanly on master. The only effect of this patch is to make the timezone settings available regardless of whether or not the panel is locked if org.freedesktop.timedate1.set-timezone is set to 'yes' (which is the default in openSUSE); if that permission isn't available to the current user (the upstream default is 'auth-admin-keep'), then nothing is changed (i.e. the lock button will still lock/unlock the timezone widget, as before, if this permission is unavailable).

I too would rather avoid making the permission setup more complicated than it already is. Having a single unlock button for the whole panel makes it easy to understand for the users, and keeps the code simple.

But this doesn't mean we can't improve the situation!

At the System Settings hackfest at GUADEC, we talked about letting local admins use the whole Date & Time panel without a password. Attaching a patch that implements this.

Created attachment 251990 [details] [review]
common: Ship a PolicyKit rule for the datetime panel

Allow local admins with an active session use the Date & Time panel
without requiring a password.

Thanks a bunch for looking at this, Kalev!

(In reply to comment #10)
> I too would rather avoid making the permission setup more complicated than it
> already is. Having a single unlock button for the whole panel makes it easy to
> understand for the users, and keeps the code simple.

Well Vincent's patch doesn't add a new unlock button or anything complicated like that: it just changes whether the timezone widget is affected by the existing unlock button.  Since the datetime panel is going to use timedated to change the timezone, it shouldn't require a password to change the timezone if timedatectl does not.  (My reasoning is that it's silly to prevent the user from changing the timezone from a graphical interface when he has permission to do so from the command line.)  The change is that the timezone widget is sensitive from the start, just like the AM/PM toggle.  (But if unprivileged users cannot change the timezone with timedatectl, then it's still affected by the unlock button, and there is no change from the current code.  So this patch makes no difference in Fedora, where only an admin can change the timezone.)

The logical extreme of this reasoning would be checking every permission to see what's sensitive by default.  I don't know if that's a good idea, but it doesn't seem worse than having weird, separate command-line and GUI permissions.

> But this doesn't mean we can't improve the situation!
> 
> At the System Settings hackfest at GUADEC, we talked about letting local admins
> use the whole Date & Time panel without a password. Attaching a patch that
> implements this.

That sounds like a good idea to me.  (My worry is that if some timedated policies are set to auth-admin-keep, I think the dbus call would fail, but if it works then great.)

I think openSUSE will probably decide to keep carrying this patch anyway, as they want all users to be able to change the timezone [1], and without this patch that's not possible without dropping to the command line to use timedatectl.  I'm not going to condone Linus's language, but he has a point.

[1] http://www.zdnet.com/blog/open-source/linus-torvalds-snarls-at-opensuse-desktop-linuxs-security/10475

(In reply to comment #11)
> Created an attachment (id=251990) [details] [review]
> common: Ship a PolicyKit rule for the datetime panel
> 
> Allow local admins with an active session use the Date & Time panel
> without requiring a password.

Kalev,

I think we need to take into account that setting date/time has an entire different effect on the system as compared to 'setting timezone'.

If you change your date/time settings for example (to wrong values), kerberos auth is likely to break and depending how far off you go, TLS/ verification fails as well.

TimeZone is mostly a 'display' issue: if a user is traveling from Europe to the US, it's just 'nice' if his panel clock can show the right time; As with openSUSE's patch, the user can chose the timezone (if permitted by the polkit settings), yet, can not break actual time/date settings.

The UI itself is not further cluttered with buttons; the only difference is that the 'MAP' is active when permitted; the rest becomes active when one clicks 'UNLOCK' and presents the right password.

Review of attachment 251990 [details] [review]:

This looks good and it's the way we are handling it in the region panel already. In general I think we should never show these unlock buttons for admin user accounts.

Comment on attachment 251990 [details] [review]
common: Ship a PolicyKit rule for the datetime panel

Attachment 251990 [details] pushed as 88eeb8c - common: Ship a PolicyKit rule for the datetime panel

I think this is a bad idea. Currently, sudo uses the clock to manage security based on a timeout. At the very least, I would hold off on this change until sudo uses some other method to manage the timeout. See ubuntu bug 1219337 for reference.

There are tons of things that rely on the clock. How often are you changing the clock/timezone that typing your password one extra time is a problem? If you want to have a package that lets you turn this policy on, or a setting or something, fine. Please don't make this the default!

(In reply to comment #16)
> There are tons of things that rely on the clock. How often are you changing the
> clock/timezone that typing your password one extra time is a problem? If you
> want to have a package that lets you turn this policy on, or a setting or
> something, fine. Please don't make this the default!

On  YOUR machine you might know the root password. People in my company that are traveling around the world, do not.

YET: they should be able to adjust the TIME ZONE (NOT THE TIME) on their system representing their local time (there is a reason why the two are different polkit rules).

I'm not saying "make it impossible for them to change the time."  I'm saying, require authentication, even if it's the users own password. Put them in a group that can change the clock, but still have them authenticate. That's the point of groups, isn't it? If this becomes the default, tons of systems that use sudo will suddenly become vulnerable.

Unfortunately the openSUSE patch doesn't really make sense anymore, now that the panel is divided in two between rows covered by the lock and rows that are not covered.  I don't think it makes sense to modify the UI upstream to support non-upstream configuration.  I guess the only path forward for this would be if systemd were to change the default policy of org.freedesktop.timedate1.set-timezone, then we could adjust the UI accordingly.

(In reply to comment #17)
> YET: they should be able to adjust the TIME ZONE (NOT THE TIME) on their system
> representing their local time (there is a reason why the two are different
> polkit rules).

He's talking about the patch to gnome-control-center.rules (which is dropped in openSUSE) allowing local admins to change all time settings without typing a password.

(In reply to comment #18)
> I'm not saying "make it impossible for them to change the time."  I'm saying,
> require authentication, even if it's the users own password. Put them in a
> group that can change the clock, but still have them authenticate. That's the
> point of groups, isn't it? If this becomes the default, tons of systems that
> use sudo will suddenly become vulnerable.
This can only be exploited by a local user, and only if you leave your computer unlocked while you're away, or if he physically attacks you.  I guess it's not totally implausible if you have secret agents or corporate spies after you....

Linux is used in several industries where corporate spies are a very real threat. Again, I'm not saying don't let users change their timezone or time/date. I'm saying *make them type their password first*.  Put them in a group that is allowed to change timezone, and force them to authenticate, just like they'd have to to log in, or to change their password, or to perform any number of other tasks that could have an effect on security.

(In reply to comment #20)
> Linux is used in several industries where corporate spies are a very real
> threat. Again, I'm not saying don't let users change their timezone or
> time/date. I'm saying *make them type their password first*. 

Security that relies on the time_zone_ of a system is broken by design.

The GNOME 2 world clock nicely allowed for showing both a number of
timezones and easily switching to one.  With GNOME 3, right now I have
to enter the root password a couple number of times when going to the 
US, say.  Often surrounded by many people at an airport or similar.

(And having a nice overview and way of switching is gone with GNOME 3,
but that's a related, but different issue.)

> Security that relies on the time_zone_ of a system is broken by design.

You bet! That's why I've submitted bugs to that effect. But that's not really the point here.

You can certainly make changes via policykit that lets you change the clock without having to type your password on your machine, if you want. Your use case, however, doesn't apply to all users. If you want to change your security policy, by all means do so! Create an option to turn this feature on even! But PLEASE don't saddle every user with an insecure clock by default.

This is fixed in GNOME 3.10. Administrators (see gnome-control-center.rules) can modify the date & time without entering a password.

:(
too sad nobody seems to undderstand that changing timezone does not mean changing time...

Created attachment 256853 [details] [review]
datetime: Allow changing the timezone if polkit says so

Even if we don't have all the permissions necessary for all
the actions, check whether the user is allowed to change the timezone
before making those rows insensitive.

Is that more to your liking? You'll need to test it.

Review of attachment 256853 [details] [review]:

Thanks for looking into this.  Unfortunately with the redesigned panel, simply setting the sensitivity of the timezone options is no longer enough: they now also have to be moved from the set of initially-locked options on top to the set of initially-unlocked options (currently just Time Format) below.  I'm not sure that determining the placement of the options at runtime is reasonable. See the first part of comment #19 above.

::: panels/datetime/cc-datetime-panel.c
@@ +784,2 @@
   allowed = (priv->permission == NULL || g_permission_get_allowed (priv->permission));
+  tz_allowed = (priv->tz_permission == NULL && g_permission_get_allowed (priv->tz_permission));

The call to g_permission_get_allowed (priv->tz_permission) will always crash (since this condition guarantees the permission to be NULL).

@@ +935,3 @@
+  active = g_value_get_boolean (source_value);
+  allowed = (priv->permission == NULL && g_permission_get_allowed (priv->permission)) ||
+            (priv->tz_permission == NULL && g_permission_get_allowed (priv->tz_permission));

Two more crashes, same as above

(In reply to comment #27)
> Review of attachment 256853 [details] [review]:
> 
> Thanks for looking into this.  Unfortunately with the redesigned panel, simply
> setting the sensitivity of the timezone options is no longer enough: they now
> also have to be moved from the set of initially-locked options on top to the
> set of initially-unlocked options (currently just Time Format) below.  I'm not
> sure that determining the placement of the options at runtime is reasonable.

The placement is nothing to do with whether they're initially locked or unlocked. Otherwise, it would look different if you log in as an admin.

Fixed the thinko in the attached patch, let me know if it works.

Created attachment 256867 [details] [review]
datetime: Allow changing the timezone if polkit says so

Even if we don't have all the permissions necessary for all
the actions, check whether the user is allowed to change the timezone
before making those rows insensitive.

I'll test the patch the new patch shortly, thanks.

(In reply to comment #28)
> The placement is nothing to do with whether they're initially locked or
> unlocked. Otherwise, it would look different if you log in as an admin.

The "problem" I saw was that the unlocked rows look really stupid when the rest of the rows in that "box" of options are locked.  A couple of days ago this seemed insurmountable to me, but all we have to do is move the two timezone options to a third box, so not really....

Review of attachment 256867 [details] [review]:

Honestly it looks fine visually (we already do this for NTP and the normal time setting); just ignore my complaints above.

I was able to get this patch to work as expected with minimal tweaking.  Thanks a bunch.

::: panels/datetime/cc-datetime-panel.c
@@ +784,2 @@
+  allowed = (priv->permission != NULL || g_permission_get_allowed (priv->permission));
+  tz_allowed = (priv->tz_permission != NULL && g_permission_get_allowed (priv->tz_permission));

Bastien could you look at these two lines closely?  Line 784 is a potential crash.  They should both either be

(priv->permission == NULL || g_permission_get_allowed (priv->permission)) 

which I think means "no permissions installed, go right ahead", as is currently in master, or else

(priv->permission != NULL && g_permission_get_allowed (priv->permission))

like you did for the timezone permission ("no permission installed, everything is insensitive.")  Either way works.

If the original permission line is changed, that should be done in a separate patch, and it would need to be changed in the original switch_to_row_transform_func as well.  Otherwise, the timezone permissions at line 936 need changed.

@@ +935,3 @@
+  active = g_value_get_boolean (source_value);
+  allowed = (priv->permission != NULL && g_permission_get_allowed (priv->permission)) ||
+            (priv->tz_permission != NULL && g_permission_get_allowed (priv->tz_permission));

See above

@@ +1317,3 @@
                  DATETIME_PERMISSION);
     }
+  priv->tz_permission = polkit_permission_new_sync (DATETIME_TZ_PERMISSION, NULL, NULL, NULL);

This line needs to be moved above the if statement so that the permission is set before entering on_permission_changed.

Created attachment 264384 [details] [review]
datetime: Allow changing the timezone if polkit says so

Bastien's patch, modified according to my review above. I went with (priv->permission == NULL || g_permission_get_allowed (priv->permission)) to match the existing code.

FWIW: the patch from Dec 2013  (comment 32) works just fine; Would be great if this were to be merged at one point.

Review of attachment 264384 [details] [review]:

This patch does not apply on top of master. Is it still relevant? If not, can we close this bug?

It appears that with GNOME 3.26 I can now change the timezone, even
without being able to change the time (which does require unlocking
the panel).

So... RESOLVED FIXED?

(In reply to Gerald Pfeifer from comment #35)
> It appears that with GNOME 3.26 I can now change the timezone, even
> without being able to change the time (which does require unlocking
> the panel).
> 
> So... RESOLVED FIXED?

Careful: I'm sure you tested on openSUSE with patch!

(In reply to Dominique Leuenberger from comment #36)
> (In reply to Gerald Pfeifer from comment #35)
> > It appears that with GNOME 3.26 I can now change the timezone, even
> > without being able to change the time (which does require unlocking
> > the panel).
> > 
> > So... RESOLVED FIXED?
> 
> Careful: I'm sure you tested on openSUSE with patch!

https://build.opensuse.org/package/view_file/openSUSE:Factory/gnome-control-center/gnome-control-center-follow-polkit-permissions-for-tz.patch?expand=1

(that patch no longer applies on master and would need to be rebased - again)

Created attachment 367022 [details] [review]
Patch rebased on current master d71f705d7

Review of attachment 367022 [details] [review]:

The code makes sense to me, but I'm not looped in the guts of changing dates and times and zones. The commit message should have explained why this is necessary.

If you could write the reasoning on the commit message, and no one opposes to this change, I'll push it. The patch will be in 'needs-work' state until there.

Thanks for the patch!

(In reply to Dominique Leuenberger from comment #36)
> Careful: I'm sure you tested on openSUSE with patch!

Yes, you're right. (And it's really convenient and user friendly.)

Created attachment 367318 [details] [review]
datetime: Allow changing the timezone if polkit says so

As changing the time can have security implications, while changing the
timezone doesn't, it's not unusual to have a setup where
org.freedesktop.timedate1.set-timezone is allowed while other
time-related actions aren't.

Therefore, if org.freedesktop.timedate1.set-timezone is allowed, there's
no reason to require that the user unlocks the panel to enable him to
change the timezone.

(In reply to Georges Basile Stavracas Neto from comment #39)
> Review of attachment 367022 [details] [review] [review]:
> 
> The code makes sense to me, but I'm not looped in the guts of changing dates
> and times and zones. The commit message should have explained why this is
> necessary.

Added the description from comment #4, which sums it up very well, to the commit message

(In reply to Dominique Leuenberger from comment #41)
> Created attachment 367318 [details] [review] [review]
> datetime: Allow changing the timezone if polkit says so
> 
> As changing the time can have security implications,

Mention which one (password expiry I'm guessing?).

> while changing the
> timezone doesn't, it's not unusual to have a setup where
> org.freedesktop.timedate1.set-timezone is allowed while other
> time-related actions aren't.
> 
> Therefore, if org.freedesktop.timedate1.set-timezone is allowed, there's
> no reason to require that the user unlocks the panel to enable him to

"no reason to require that the user unlock their panel to enable them to"

singular "them", or feminine here.

> change the timezone.

> allowed = (self->permission != NULL && g_permission_get_allowed (self->permission)) ||
>    (self->tz_permission != NULL && g_permission_get_allowed (self->tz_permission));

Given that the top-level permission implies the more precise one, I'd just check for the more precise one, no?

Created attachment 367477 [details] [review]
datetime: Allow changing the timezone if polkit says so

Updated the commit message. I'll accept it as is, but leave the bug open so that Bastien's comment about the toplevel permission is addressed (either by answering or by throwing another patch.)

Comment on attachment 367477 [details] [review]
datetime: Allow changing the timezone if polkit says so

Leaving the ticket open as commented.

Attachment 367477 [details] pushed as 5a66372 - datetime: Allow changing the timezone if polkit says so

GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org.
As part of that, we are mass-closing older open tickets in bugzilla.gnome.org
which have not seen updates for a longer time (resources are unfortunately
quite limited so not every ticket can get handled).

If you can still reproduce the situation described in this ticket in a recent
and supported software version, then please follow
  https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines
and create a new bug report at
  https://gitlab.gnome.org/GNOME/gnome-control-center/-/issues/

Thank you for your understanding and your help.