After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 790066 - Add module for notifying user when their password is going to expire
Add module for notifying user when their password is going to expire
Status: RESOLVED OBSOLETE
Product: gnome-settings-daemon
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-settings-daemon-maint
gnome-settings-daemon-maint
: 766219 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2017-11-08 14:41 UTC by Ray Strode [halfline]
Modified: 2019-03-20 11:50 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
account: first cut at account plugin (423.17 KB, patch)
2017-11-08 14:41 UTC, Ray Strode [halfline]
none Details | Review
account: reshow the notification when screen unlocks (11.84 KB, patch)
2017-11-08 14:41 UTC, Ray Strode [halfline]
none Details | Review
account: display nag screen periodically (18.21 KB, patch)
2017-11-08 14:41 UTC, Ray Strode [halfline]
none Details | Review
account: first cut at account plugin (65.46 KB, patch)
2018-01-18 18:59 UTC, Ray Strode [halfline]
none Details | Review
account: reshow the notification when screen unlocks (10.39 KB, patch)
2018-01-18 18:59 UTC, Ray Strode [halfline]
none Details | Review
account: display nag screen periodically (19.68 KB, patch)
2018-01-18 18:59 UTC, Ray Strode [halfline]
none Details | Review
account: display nag screen periodically (21.64 KB, patch)
2018-01-19 15:06 UTC, Ray Strode [halfline]
none Details | Review
account: first cut at account plugin (67.35 KB, patch)
2018-01-19 15:07 UTC, Ray Strode [halfline]
needs-work Details | Review
account: reshow the notification when screen unlocks (10.39 KB, patch)
2018-01-19 15:07 UTC, Ray Strode [halfline]
accepted-commit_now Details | Review
account: display nag screen periodically (21.64 KB, patch)
2018-01-19 15:07 UTC, Ray Strode [halfline]
none Details | Review
account: display nag screen periodically (21.64 KB, patch)
2018-01-19 18:49 UTC, Ray Strode [halfline]
reviewed Details | Review

Description Ray Strode [halfline] 2017-11-08 14:41:04 UTC
In RHEL land we have a surprisingly large number of customers who want a little
bubble to show up when the user's password is about to expire.

To that end, this commit adds that feature to a new account module.
Comment 1 Ray Strode [halfline] 2017-11-08 14:41:19 UTC
Created attachment 363223 [details] [review]
account: first cut at account plugin

It just notifies when the user's account is about to expire.

Future commits will make it redisplay the notification after
screen unlock and provide some configurability for regular
nags.
Comment 2 Ray Strode [halfline] 2017-11-08 14:41:30 UTC
Created attachment 363224 [details] [review]
account: reshow the notification when screen unlocks
Comment 3 Ray Strode [halfline] 2017-11-08 14:41:40 UTC
Created attachment 363225 [details] [review]
account: display nag screen periodically

This is configurable via a gsettings key.
Comment 4 Ray Strode [halfline] 2018-01-18 14:12:11 UTC
*** Bug 766219 has been marked as a duplicate of this bug. ***
Comment 5 Bastien Nocera 2018-01-18 16:42:11 UTC
Review of attachment 363223 [details] [review]:

Can you remove the gdbus-codegen generated  files from the patch?

Looks fine overall.

::: plugins/account/gsd-account-manager.c
@@ +183,3 @@
+                    secondary_text = g_strdup_printf (_("Your password is expiring today."));
+                else if (days_left == 1)
+                    secondary_text = g_strdup_printf (_("Your password is expiring in a day."));

This isn't how you do i18n. See g_dngettext() instead.
It's probably fine leaving "today" as a special case though.

@@ +412,3 @@
+{
+        if (manager_object != NULL) {
+                g_object_ref (manager_object);

This isn't necessary anymore, just return the object. We should probably fix this in every plugin as well.
Comment 6 Bastien Nocera 2018-01-18 16:43:43 UTC
Review of attachment 363224 [details] [review]:

Looks good otherwise.

::: data/Makefile.am
@@ +10,3 @@
 	org.gnome.settings-daemon.peripherals.gschema.xml		\
 	org.gnome.settings-daemon.plugins.gschema.xml			\
+	org.gnome.settings-daemon.plugins.account.gschema.xml		\

This needs to be in the original patch, doesn't it?
Comment 7 Bastien Nocera 2018-01-18 16:46:18 UTC
Review of attachment 363225 [details] [review]:

::: data/org.gnome.settings-daemon.plugins.account.gschema.xml.in.in
@@ +4,3 @@
+      <default>1440</default>
+      <_summary>Time before repeated warning about account password expiration</_summary>
+      <_description>If a user's account is expiring, a notification will get displayed periodically after the specified number of minutes</_description>

Can you mention the default in hours in the description?

Does this need to be minutes, or would hours be enough? Can you explain why admins might want to change it? Do we want to make this directly calculated from the past session lengths instead?
Comment 8 Bastien Nocera 2018-01-18 16:46:43 UTC
Review of attachment 363224 [details] [review]:

::: data/Makefile.am
@@ +10,3 @@
 	org.gnome.settings-daemon.peripherals.gschema.xml		\
 	org.gnome.settings-daemon.plugins.gschema.xml			\
+	org.gnome.settings-daemon.plugins.account.gschema.xml		\

Apparently in the next one, rather.
Comment 9 Ray Strode [halfline] 2018-01-18 18:54:55 UTC
(In reply to Bastien Nocera from comment #5)
> Can you remove the gdbus-codegen generated  files from the patch?
Woops, some other generated goo go in too, aside from the codegen.  i'll mop it up.

> This isn't how you do i18n. See g_dngettext() instead.
> It's probably fine leaving "today" as a special case though.
yea, not sure why i didn't use ngettext.

> This isn't necessary anymore, just return the object. We should probably fix
> this in every plugin as well.
ok
Comment 10 Ray Strode [halfline] 2018-01-18 18:58:37 UTC
(In reply to Bastien Nocera from comment #7)
> Review of attachment 363225 [details] [review] [review]:
> 
> ::: data/org.gnome.settings-daemon.plugins.account.gschema.xml.in.in
> @@ +4,3 @@
> +      <default>1440</default>
> +      <_summary>Time before repeated warning about account password
> expiration</_summary>
> +      <_description>If a user's account is expiring, a notification will
> get displayed periodically after the specified number of
> minutes</_description>
> 
> Can you mention the default in hours in the description?
> 
> Does this need to be minutes, or would hours be enough? Can you explain why
> admins might want to change it? Do we want to make this directly calculated
> from the past session lengths instead?
I think one of the customers mentioned minutes specifically as being a requirement, though I don't recall 100% off hand.  I don't think there's any real advantage to using hours, and there's a slight advantage to more granularity, so I'd prefer to stay with minutes, anyawy.

Admins will want to change it to comply with whatever security policy their org wrote up.  I don't think it can be dynamically calculated on session length, since session length is unrelated to the timeout.
Comment 11 Ray Strode [halfline] 2018-01-18 18:59:14 UTC
Created attachment 367030 [details] [review]
account: first cut at account plugin

It just notifies when the user's account is about to expire.

Future commits will make it redisplay the notification after
screen unlock and provide some configurability for regular
nags.
Comment 12 Ray Strode [halfline] 2018-01-18 18:59:18 UTC
Created attachment 367031 [details] [review]
account: reshow the notification when screen unlocks
Comment 13 Ray Strode [halfline] 2018-01-18 18:59:21 UTC
Created attachment 367032 [details] [review]
account: display nag screen periodically

This is configurable via a gsettings key.
Comment 14 Bastien Nocera 2018-01-19 12:09:01 UTC
Review of attachment 367030 [details] [review]:

You didn't update the POTFILES.

::: plugins/account/Makefile.am
@@ +55,3 @@
+	$(ACCOUNT_LIBS)
+
+desktopdir = $(sysconfdir)/xdg/autostart

No, we don't have autostart files in gnome-settings-daemon. That might be good enough for a downstream patch, but this will need to be started up by gnome-session.

::: plugins/account/org.gnome.SettingsDaemon.Account.desktop.in
@@ +1,1 @@
+[Desktop Entry]

You'll also need a patch to gnome-session to have it actually started.
Comment 15 Bastien Nocera 2018-01-19 12:10:09 UTC
Review of attachment 367031 [details] [review]:

That looks fine.
Comment 16 Bastien Nocera 2018-01-19 12:11:56 UTC
Review of attachment 367032 [details] [review]:

This is missing POTFILES changes as well.

::: data/org.gnome.settings-daemon.plugins.account.gschema.xml.in.in
@@ +1,3 @@
+<schemalist>
+  <schema gettext-domain="@GETTEXT_PACKAGE@" id="org.gnome.settings-daemon.plugins.account" path="/org/gnome/settings-daemon/plugins/account/">
+    <key name="notify-period" type="i">

u?
Comment 17 Bastien Nocera 2018-01-19 12:13:36 UTC
Review of attachment 367030 [details] [review]:

Another thing to test is whether the code survives the AccountsService stopping and starting (I think it does, but a test would be useful).

Finally, can you take a look at test.py in the power plugin, and see whether it's not too much work writing a test for this plugin?
Comment 18 Ray Strode [halfline] 2018-01-19 14:36:56 UTC
> No, we don't have autostart files in gnome-settings-daemon. That might be
> good enough for a downstream patch, but this will need to be started up by
> gnome-session.

Sure we do:

╎❯ ls -1 /etc/xdg/autostart/org.gnome.SettingsDaemon*.desktop
/etc/xdg/autostart/org.gnome.SettingsDaemon.A11yKeyboard.desktop
/etc/xdg/autostart/org.gnome.SettingsDaemon.A11ySettings.desktop
/etc/xdg/autostart/org.gnome.SettingsDaemon.Account.desktop
/etc/xdg/autostart/org.gnome.SettingsDaemon.Clipboard.desktop
/etc/xdg/autostart/org.gnome.SettingsDaemon.Color.desktop
/etc/xdg/autostart/org.gnome.SettingsDaemon.Datetime.desktop
/etc/xdg/autostart/org.gnome.SettingsDaemon.DiskUtilityNotify.desktop
/etc/xdg/autostart/org.gnome.SettingsDaemon.Housekeeping.desktop
/etc/xdg/autostart/org.gnome.SettingsDaemon.Keyboard.desktop
/etc/xdg/autostart/org.gnome.SettingsDaemon.MediaKeys.desktop
/etc/xdg/autostart/org.gnome.SettingsDaemon.Mouse.desktop
/etc/xdg/autostart/org.gnome.SettingsDaemon.Power.desktop
/etc/xdg/autostart/org.gnome.SettingsDaemon.PrintNotifications.desktop
/etc/xdg/autostart/org.gnome.SettingsDaemon.Rfkill.desktop
/etc/xdg/autostart/org.gnome.SettingsDaemon.ScreensaverProxy.desktop
/etc/xdg/autostart/org.gnome.SettingsDaemon.Sharing.desktop
/etc/xdg/autostart/org.gnome.SettingsDaemon.Smartcard.desktop
/etc/xdg/autostart/org.gnome.SettingsDaemon.Sound.desktop
/etc/xdg/autostart/org.gnome.SettingsDaemon.Wacom.desktop
/etc/xdg/autostart/org.gnome.SettingsDaemon.XSettings.desktop

We do list every plugin as a required component in the session definition file, but that just means we'll fail whale if it crashes in a loop or doesn't start.

Arguably we could move these files to /usr/share/applications .  That would work as well since required components get autostarted even if they aren't in the autostart directory.  I don't really see the advantage. 

You're right though, to be consistent we should put Accounts as a required component.  Alternatively we should figure out which of the modules really are "required" and prune the ones out that shouldn't cause fail whale.
Comment 19 Bastien Nocera 2018-01-19 14:41:31 UTC
(In reply to Ray Strode [halfline] from comment #18)
> > No, we don't have autostart files in gnome-settings-daemon. That might be
> > good enough for a downstream patch, but this will need to be started up by
> > gnome-session.
> 
> Sure we do:
<snip>

"When you don't even remember your own code"...

> We do list every plugin as a required component in the session definition
> file, but that just means we'll fail whale if it crashes in a loop or
> doesn't start.
> 
> Arguably we could move these files to /usr/share/applications .  That would
> work as well since required components get autostarted even if they aren't
> in the autostart directory.  I don't really see the advantage. 
> 
> You're right though, to be consistent we should put Accounts as a required
> component.  Alternatively we should figure out which of the modules really
> are "required" and prune the ones out that shouldn't cause fail whale.

I'd add it. We'll prune the list when we have systemd session support, to make sure things only get started as needed.
Comment 20 Ray Strode [halfline] 2018-01-19 15:06:49 UTC
Created attachment 367083 [details] [review]
account: display nag screen periodically

This is configurable via a gsettings key.
Comment 21 Ray Strode [halfline] 2018-01-19 15:07:17 UTC
Created attachment 367084 [details] [review]
account: first cut at account plugin

It just notifies when the user's account is about to expire.

Future commits will make it redisplay the notification after
screen unlock and provide some configurability for regular
nags.
Comment 22 Ray Strode [halfline] 2018-01-19 15:07:21 UTC
Created attachment 367085 [details] [review]
account: reshow the notification when screen unlocks
Comment 23 Ray Strode [halfline] 2018-01-19 15:07:24 UTC
Created attachment 367086 [details] [review]
account: display nag screen periodically

This is configurable via a gsettings key.
Comment 24 Ray Strode [halfline] 2018-01-19 15:08:03 UTC
will get back to you on the testing, that's going to take more time than I want to devote this morning on this :-)
Comment 25 Ray Strode [halfline] 2018-01-19 18:49:55 UTC
Created attachment 367098 [details] [review]
account: display nag screen periodically

This is configurable via a gsettings key.
Comment 26 Bastien Nocera 2018-01-24 15:37:09 UTC
Review of attachment 367084 [details] [review]:

If it's not too much work, could you please add the xml files separately and explain how we would update them? (links to the original files in a git web is fine)

Nothing major apart from the missing cancellation checks, we wouldn't want warnings when the session is closed.

::: plugins/account/gsd-account-manager.c
@@ +230,3 @@
+                                                                                  &error);
+
+        if (!succeeded) {

if not cancelled.

@@ +264,3 @@
+                                                                       on_got_password_expiration_policy,
+                                                                       manager);
+        } else {

if not cancelled

@@ +291,3 @@
+                                                              &error);
+
+        if (!succeeded) {

if not cancelled

@@ +329,3 @@
+                                                   on_got_user_object_path,
+                                                   manager);
+        } else {

if not cancelled.

::: plugins/account/org.gnome.SettingsDaemon.Account.desktop.in
@@ +1,3 @@
+[Desktop Entry]
+Type=Application
+Name=GNOME Settings Daemon's housekeeping plugin

not housekeeping
Comment 27 Bastien Nocera 2018-01-24 15:37:59 UTC
Review of attachment 367085 [details] [review]:

Did we want a timeout for this?
Comment 28 Bastien Nocera 2018-01-24 15:41:19 UTC
Review of attachment 367098 [details] [review]:

Looks good.

::: data/org.gnome.settings-daemon.plugins.account.gschema.xml.in.in
@@ +4,3 @@
+      <default>1440</default>
+      <_summary>Time in minutes between repeated warnings about account password expiration</_summary>
+      <_description>If a user's account is expiring, a notification will get displayed periodically after the specified number of minutes.</_description>

Still missing the default value in hours.

::: plugins/account/gsd-account-manager.c
@@ +232,3 @@
+        }
+
+        if (manager->priv->notify_period > 0) {

Is this really the behaviour we want for 0? Not the default value instead?
Comment 29 Ray Strode [halfline] 2018-01-24 15:47:12 UTC
oh you want to add, "The default behavior is to notify the user every 24 hours." to the description. I didn't understand you before.

I think we should have a way to turn the nag screen off, 0 seems like a good way. if users want the default value they can gsettings reset to the default.
Comment 30 Bastien Nocera 2018-02-01 10:20:48 UTC
You might need to rebase your patches on top of bug 793087 (meson port).
Comment 31 Ray Strode [halfline] 2018-02-01 13:03:51 UTC
will do. there is a downstream bugfix i need to incorporate too
Comment 32 GNOME Infrastructure Team 2019-03-20 11:50:23 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-settings-daemon/issues/368.