diff mbox series

[PATCHv3] cups: Allow to be controlled by root and users in wheel

Message ID 20231120161848.1461683-1-f_l_k@t-online.de
State New
Headers show
Series [PATCHv3] cups: Allow to be controlled by root and users in wheel | expand

Commit Message

Markus Volk Nov. 20, 2023, 4:18 p.m. UTC
This would be the cleanest way I can think of to enable the configuration of
cups in the user interface

Signed-off-by: Markus Volk <f_l_k@t-online.de>
---
 meta/recipes-extended/cups/cups.inc | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

Comments

Ross Burton Nov. 23, 2023, 12:43 p.m. UTC | #1
> On 20 Nov 2023, at 16:18, Markus Volk via lists.openembedded.org <f_l_k=t-online.de@lists.openembedded.org> wrote:
> -               --with-system-groups=lpadmin \
> +               --with-system-groups=sys,root,wheel \

This looks like you’re removing the lpadmin group entirely, which isn’t mentioned in the commit message at all and presumably has other implications?

Ross
Richard Purdie Nov. 23, 2023, 12:50 p.m. UTC | #2
On Mon, 2023-11-20 at 17:18 +0100, Markus Volk wrote:
> This would be the cleanest way I can think of to enable the configuration of
> cups in the user interface
> 
> Signed-off-by: Markus Volk <f_l_k@t-online.de>
> ---
>  meta/recipes-extended/cups/cups.inc | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)

This definitely needs more explanation about the problem being solved
and why this is the right way to solve it...

root could presumably control this before so even the current commit
message doesn't quite make sense to me.

Cheers,

Richard
Markus Volk Nov. 23, 2023, 1:15 p.m. UTC | #3
On Thu, Nov 23 2023 at 12:43:48 PM +00:00:00, Ross Burton 
<Ross.Burton@arm.com> wrote:
>  removing the lpadmin group entirely, which isn’t mentioned in the 
> commit message at all and presumably has other implications

Yes, it does. I have sent two versions of the patch. The first one just 
adjusts the place in cups-files.conf to set the system groups and in 
the second attempt I thought it would be cleaner to remove the lpadmin 
group altogether because it is not used. I didn't find any other usage 
in oe-core and in meta openembedded only:
flk@intel-corei7-64:~/poky/meta-openembedded$ grep -r lpadmin .
./meta-networking/recipes-connectivity/samba/samba/smb.conf:# You may 
need to replace 'lpadmin' with the name of the group your
./meta-networking/recipes-connectivity/samba/samba/smb.conf:; write 
list = root, @lpadmin

and I planned to have a look at  the samba part in near future. I guess 
the same list would apply there as well to get a working default 
setting.
This is also how cups is configured in fedora. I don't know about debian

The first patch is a little less invasive but ugly with sed


Should I send a v3 with adjusted commit message? Thought it was quite 
obvious what the patch does
Markus Volk Nov. 23, 2023, 1:40 p.m. UTC | #4
On Thu, Nov 23 2023 at 12:50:20 PM +00:00:00, Richard Purdie 
<richard.purdie@linuxfoundation.org> wrote:
> This definitely needs more explanation about the problem being solved
> and why this is the right way to solve it...
> 
> root could presumably control this before so even the current commit
> message doesn't quite make sense to me.

With this commit the printer configuration in gnome-control-center 
works as i would expect:

To unlock the printer configuration a user who is in the wheel group 
only needs his own password, if he is not in wheel the root password is 
requested.
If the printer configuration is unlocked, all configuration options of 
gnome-control-center can be used

With the lpadmin group you can unlock the printer configuration with 
cups-pk-helper, but then you still do not have the needed permission to 
make meaningful changes. You can only print ... Adding or removing 
printers or changing the print quality or paper size can then only be 
done via the web interface

In other words. This makes the cups-pk-helper unlock mechanism work out 
of the box without further configuration
Markus Volk Nov. 23, 2023, 2:04 p.m. UTC | #5
On Thu, Nov 23 2023 at 02:40:19 PM +01:00:00, Markus Volk 
<f_l_k@t-online.de> wrote:
> In other words. This makes the cups-pk-helper unlock mechanism work 
> out of the box without further configuration

with this commit:
Nov 23 15:03:06 intel-corei7-64 dbus-daemon[739]: [system] Activating 
service name='org.opensuse.CupsPkHelper.Mechanism' requested by 
':1.126' (uid=1000 pid=2346631 comm="/usr/bin/gnome-control-center") 
(using servicehelper)
Nov 23 15:03:06 intel-corei7-64 dbus-daemon[739]: [system] Successfully 
activated service 'org.opensuse.CupsPkHelper.Mechanism'


without this commit:
Nov 23 15:00:01 intel-corei7-64 dbus-daemon[739]: [system] Activating 
service name='org.opensuse.CupsPkHelper.Mechanism' requested by 
':1.115' (uid=1000 pid=2346285 comm="/usr/bin/gnome-control-center") 
(using servicehelper)
Nov 23 15:00:01 intel-corei7-64 dbus-daemon[739]: [system] Successfully 
activated service 'org.opensuse.CupsPkHelper.Mechanism'
Nov 23 15:00:01 intel-corei7-64 gnome-control-c[2346285]: 
cups-pk-helper: setting of an option failed: client-error-forbidden
Nov 23 15:00:01 intel-corei7-64 gnome-control-c[2346285]: 
g_hash_table_ref: assertion 'hash_table != NULL' failed
Nov 23 15:00:03 intel-corei7-64 gnome-control-c[2346285]: 
cups-pk-helper: setting of an option failed: client-error-forbi
Richard Purdie Nov. 23, 2023, 3:03 p.m. UTC | #6
On Thu, 2023-11-23 at 14:40 +0100, f_l_k@t-online.de wrote:
> On Thu, Nov 23 2023 at 12:50:20 PM +00:00:00, Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
> > This definitely needs more explanation about the problem being
> > solved
> > and why this is the right way to solve it...
> > 
> > root could presumably control this before so even the current
> > commit
> > message doesn't quite make sense to me.
> 
> 
> With this commit the printer configuration in gnome-control-center
> works as i would expect:
> 
> To unlock the printer configuration a user who is in the wheel group
> only needs his own password, if he is not in wheel the root password
> is requested.
> If the printer configuration is unlocked, all configuration options
> of gnome-control-center can be used
> 
> With the lpadmin group you can unlock the printer configuration with
> cups-pk-helper, but then you still do not have the needed permission
> to make meaningful changes. You can only print ... Adding or removing
> printers or changing the print quality or paper size can then only be
> done via the web interface
> 
> In other words. This makes the cups-pk-helper unlock mechanism work
> out of the box without further configuration

That sounds like the patch would therefore add wheel as one of the
groups. I'm still not sure why sys and root in there?

I'm guessing lpadmin is removed as it is unneeded but it does mean you
lose the ability to control printers separately from complete wheel
access.

Could we not allow wheel access to lpadmin instead?

Cheers,

Richard
Markus Volk Nov. 23, 2023, 3:49 p.m. UTC | #7
On Thu, Nov 23 2023 at 03:03:39 PM +00:00:00, Richard Purdie 
<richard.purdie@linuxfoundation.org> wrote:
> I'm guessing lpadmin is removed as it is unneeded but it does mean you
> lose the ability to control printers separately from complete wheel
> access.
> 
> Could we not allow wheel access to lpadmin instead?

The Cups documentation states: The default contains "admin", "lpadmin", 
"root", "sys" and/or "system".
<https://www.cups.org/doc/man-cups-files.conf.html#:~:text=SystemGroup>

sys and root should be included by default, I assume? Additionally, I 
looked at the cups-files.conf from my Fedora install and it includes 
root, sys, wheel, so that seemed like a good setting to me and 
immediately solved the permissions problem. Adding wheel made quite a 
bit of sense to me as it is widely used as a prioritized group. Not 
only sudo, but also flatpak and iwd, for example, use wheel as a 
prioritized group by default.

Probably shouldn't be a problem to add the lpadmin group as a system 
group in addition to the others, so as not to lose the ability to 
control printers separately from wheel or root. I can run some tests, 
but that will take some time. It's a busy week.

I just figured, that in the interest of simplicity,  because it hasn't 
been used before either and because fedora did the same, the lpadmin 
group wouldn't be missed .  And in case of a system that has polkit 
support cups can be configured with fine-grained privileges using 
cups-pk-helper
Richard Purdie Nov. 23, 2023, 4:22 p.m. UTC | #8
On Thu, 2023-11-23 at 16:49 +0100, f_l_k@t-online.de wrote:
> On Thu, Nov 23 2023 at 03:03:39 PM +00:00:00, Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
> > I'm guessing lpadmin is removed as it is unneeded but it does mean
> > you
> > lose the ability to control printers separately from complete wheel
> > access.
> > 
> > Could we not allow wheel access to lpadmin instead?
> 
> 
> The Cups documentation states: The default contains "admin",
> "lpadmin", "root", "sys" and/or "system".
> https://www.cups.org/doc/man-cups-files.conf.html#:~:text=SystemGroup
> 
> sys and root should be included by default, I assume? Additionally, I
> looked at the cups-files.conf from my Fedora install and it includes
> root, sys, wheel, so that seemed like a good setting to me and
> immediately solved the permissions problem. Adding wheel made quite a
> bit of sense to me as it is widely used as a prioritized group. Not
> only sudo, but also flatpak and iwd, for example, use wheel as a
> prioritized group by default.
> 
> Probably shouldn't be a problem to add the lpadmin group as a system
> group in addition to the others, so as not to lose the ability to
> control printers separately from wheel or root. I can run some tests,
> but that will take some time. It's a busy week.
> 
> I just figured, that in the interest of simplicity,  because it
> hasn't been used before either and because fedora did the same, the
> lpadmin group wouldn't be missed .  And in case of a system that has
> polkit support cups can be configured with fine-grained privileges
> using cups-pk-helper

It does make a lot more sense when you describe it, please put the
details in the commit message as it does help a lot! :)

I do think a test on whether keeping lpadmin works would be helpful as
there could easily be people depending on that behaviour.

Cheers,

Richard
diff mbox series

Patch

diff --git a/meta/recipes-extended/cups/cups.inc b/meta/recipes-extended/cups/cups.inc
index ff5f55e62a..1cd7bf1ddb 100644
--- a/meta/recipes-extended/cups/cups.inc
+++ b/meta/recipes-extended/cups/cups.inc
@@ -29,10 +29,7 @@  LEAD_SONAME = "libcupsdriver.so"
 
 CLEANBROKEN = "1"
 
-inherit autotools-brokensep binconfig useradd systemd pkgconfig multilib_script github-releases
-
-USERADD_PACKAGES = "${PN}"
-GROUPADD_PARAM:${PN} = "--system lpadmin"
+inherit autotools-brokensep binconfig systemd pkgconfig multilib_script github-releases
 
 SYSTEMD_SERVICE:${PN} = "cups.socket cups.path cups.service cups-lpd.socket"
 
@@ -57,7 +54,7 @@  EXTRA_OECONF = " \
                --enable-debug \
                --disable-relro \
                --enable-libusb \
-               --with-system-groups=lpadmin \
+               --with-system-groups=sys,root,wheel \
                --with-cups-group=lp \
                --with-domainsocket=/run/cups/cups.sock \
                --with-pkgconfpath=${libdir}/pkgconfig \