GNOME Bugzilla – Bug 790066
Add module for notifying user when their password is going to expire
Last modified: 2019-03-20 11:50:23 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.
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.
Created attachment 363224 [details] [review] account: reshow the notification when screen unlocks
Created attachment 363225 [details] [review] account: display nag screen periodically This is configurable via a gsettings key.
*** Bug 766219 has been marked as a duplicate of this bug. ***
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.
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?
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?
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.
(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
(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.
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.
Created attachment 367031 [details] [review] account: reshow the notification when screen unlocks
Created attachment 367032 [details] [review] account: display nag screen periodically This is configurable via a gsettings key.
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.
Review of attachment 367031 [details] [review]: That looks fine.
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?
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?
> 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.
(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.
Created attachment 367083 [details] [review] account: display nag screen periodically This is configurable via a gsettings key.
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.
Created attachment 367085 [details] [review] account: reshow the notification when screen unlocks
Created attachment 367086 [details] [review] account: display nag screen periodically This is configurable via a gsettings key.
will get back to you on the testing, that's going to take more time than I want to devote this morning on this :-)
Created attachment 367098 [details] [review] account: display nag screen periodically This is configurable via a gsettings key.
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
Review of attachment 367085 [details] [review]: Did we want a timeout for this?
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?
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.
You might need to rebase your patches on top of bug 793087 (meson port).
will do. there is a downstream bugfix i need to incorporate too
-- 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.