[bug#62298] [PATCH 7/8] services: mpd: Use user-account (resp. user-group) for user (resp. group) fields.

[Bruno Victal]

Hi Liliana,

On 2023-03-20 19:33, Liliana Marie Prikler wrote:
> Am Montag, dem 20.03.2023 um 17:07 +0000 schrieb Bruno Victal:
>> Deprecate using strings for these fields and prefer user-account
>> (resp. user-group) instead to avoid duplication within account-
>> service-type.  If a string value is encountered, it is ignored and a
>> predefined variable is used instead. This is essentially a rollback
>> to how it used to be before
>> '5c5f0fc1135ff15f9c4adfc5f27eadd9a592b5d1'.
> I already wrote this in private in IRC, but falling back to a constant
> when a string value is given is very silly.  IIUC the reason to do so
> is because you would need to sanitize the user account and group at the
> same time so that the former has access to the latter.
> 
> 
> I think it's possible to use one of the following approaches to get a
> better result:
> 1. In (mpd-accounts), check if the user group equals the group name and
> raise a warning (or error) if not.
> 2. Use a special unique symbol, e.g. (make-symbol "%mpd-group") to
> denote the value to be lazily inserted by the serializer.

After giving some thought to this, IMO I think it's simply uninteresting for 
these
fields to accept string values.
Prior to the 5c5f0fc1135ff15f9c4adfc5f27eadd9a592b5d1 refactor, the names were 
hardcoded
and the refactor allowed them to be changed.


But with the observed interactions in [1], it became clear that:

1. This service isn't meant to be used with a non-interactive user, a home 
shepherd service
should be used instead. (granted the bug isn't due to this, it merely surfaced 
up here.
Setting group to “nobody” would have also caused the same kind of problem.)

2. Accepting strings was deemed undesirable since it then results in a “race” 
between
user-accounts from operating-system and account-service-type extensions (or 
even among the extensions),
with the winner getting to “build” the account as it sees fit.


In the end, the daemon requires a user-account + user-group to work (unless 
you're in the
mood for running it as root?), so something still has to be made.
The choices that I think make sense for user/group fields are:

1. Leave it with a default value. The service creates what it needs.

2. Give it a user-account/group. This is considered an _advanced_ use-case and 
it's highly likely (though not mandatory)
to be used within a let-form. You use this when you need fine control over the 
account properties.


Accepting strings is simply uninteresting (or bad) since:

* A string doesn't uniquely identify an account and results in buggy behavior 
[1].

* Since the string values are only used to set the 'name' of the 
user-account/group records, which
is specific to the service as they're created within the mpd-account procedure, 
it's simply setting
a vanity value. It's as interesting as allowing the filename in 
(mixed-text-file "mpd.conf" ...)
to be set by the user.

* It's clearly unsanitizable since it would require accessing other fields.
Monkeying within (mpd-accounts) with special symbols just obfuscates the code 
with
no clear benefits to be had, in addition to defeating the point of having a 
sanitizer in the first place.


I fail to see the utility in ever accepting strings here for what amounts to a 
vanity change in 'ps aux'
output. Maybe my imagination is failing here but I really don't think it's 
worth the trouble to support strings here.
This vanity change is still achievable with some extra-typing if someone really 
wishes it.

As such, I think the best course of action is to simply use user-account/group 
record objects from now on,
with string usages deprecated and properly communicated to the user that 
they're not supported and are ignored. (a non API breaking change)


[1]: <https://issues.guix.gnu.org/61570>



Cheers,
Bruno