Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lxqt-policykit-agent: add module #2400

Conversation

bobvanderlinden
Copy link

Description

I was missing a polkit agent and found the one from LXQT to be a nice candidate. I've been running this module for a number of months and thought to contribute it upstream.

It shows a GUI password prompt whenever an application requires more privileges. For instance when running systemctl start docker.

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

    • Added myself and the module files to .github/CODEOWNERS.

Comment on lines +7 to +13
package = mkOption {
type = types.package;
default = pkgs.lxqt.lxqt-policykit;
defaultText = literalExample "pkgs.lxqt.lxqt-policykit";
description = ''
LXQT Policykit package to use
'';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of providing a package option, I think it'd be great if you provided a command option to generalize this service for any Polkit agent:

services.polkit-agent.enable = true;
# services.polkit-agent.command = "${pkgs.lxqt.lxqt-policykit}/bin/lxqt-policykit-agent";
# services.polkit-agent.command = "${pkgs.polkit_gnome}/libexec/polkit-gnome-authentication-agent-1";

Copy link
Member

@berbiche berbiche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!

As mentioned by @Enzime, it would be great if this was generalized to also work with Gnome's polkit agent for instance.

There's generally two way of making things configurable if they share the same options:

  • adding a type option of type types.enum [ "lxqt" "gnome" ] to use a specific variant of the policykit agent.
  • adding a sub-attribute to services.policykit. for each variant.
    If the set of options are going to be the same, I recommend the first solution, otherwise the second solution might be more interesting.

Could you also add documentation to the enable option to inform NixOS users that they must set security.polkit.enable = true for the policykit agent to work.

Note, it is necessary to add
<programlisting language="nix">
security.polkit.enable = true;
</programlisting>
to your system configuration for the policykit agent to work correctly

Comment on lines +5 to +6
options.services.lxqt-policykit-agent = {
enable = mkEnableOption "LXQT Policykit Agent";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned by @Enzime, this can be generalized to also work with the gnome polkit agent.

I think the option could be nested under services.policykit.lxqt, and the option services.policykit.gnome could be added,
They would both have an .enable option and be mutually exclusive config.assertions = [{ assertion = cfg.lxqt.enable != cfg.gnome.enable; message = "....."; }]

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into this. I can introduce polkit-gnome as well, but I'm not sure about the abstraction.

NixOS options use services.gnome3.file-roller.enable or services.gnome3.gnome-keyring.enable for instance. Following that convention it should be services.lxqt.lxqt-policykit-agent.enable.

However, I haven't seen something similar in home-manager. For instance, gnome-keyring is available, but it's available under services.gnome-keyring.enable.

I'm all for abstracting, but maybe that should be a layer on top of this one. So, keep it services.lxqt-policykit-agent.enable and have another abstracted option set this by default.

I can imagine:

security.polkit.enable = true;
security.polkit.type = "lxqt";

to set services.lxqt-policykit-agent.enable by default. That could be done in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, extra settings would then need to be configured with the respective options under services.lqxt-policykit-agent and services.gnome-polkit-agent.
I'll leave the final design choice to rycee.

As for the use of lxqt and gnome, we don't use a namespace for each window manager/desktop environment at the moment.

@stale
Copy link

stale bot commented Jan 30, 2022

Thank you for your contribution! I marked this pull request as stale due to inactivity. If this remains inactive for another 7 days, I will close this PR. Please read the relevant sections below before commenting.

If you are the original author of the PR

  • GitHub sometimes doesn't notify people who commented / reviewed a PR previously when you (force) push commits. If you have addressed the reviews you can officially ask for a review from those who commented to you or anyone else.
  • If it is unfinished but you plan to finish it, please mark it as a draft.
  • If you don't expect to work on it any time soon, please consider closing it with a short comment encouraging someone else to pick up your work.
  • To get things rolling again, rebase the PR against the target branch and address valid comments.

If you are not the original author of the PR

  • If you want to pick up the work on this PR, please create a new PR and indicate that it supercedes and closes this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants