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

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'.

Fixes #61570 <https://issues.guix.gnu.org/61570>.


* gnu/services/audio.scm (mpd-serialize-user-account, mpd-serialize-user-group)
(mpd-user-sanitizer, mpd-group-sanitizer): New procedure.
(%mpd-user, %mpd-group): New variable.
(mpd-configuration)[user, group]: Set value type to user-account (resp. user-group).
(mpd-shepherd-service): Adapt for user-account values in user field.
(mpd-accounts): Adapt for user-account (resp. user-group) in user (resp. group) field.

* doc/guix.texi (Audio Services): Update documentation.
---
 doc/guix.texi          |  4 +--
 gnu/services/audio.scm | 70 ++++++++++++++++++++++++++++++++----------
 2 files changed, 55 insertions(+), 19 deletions(-)

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.

Cheers

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

Hi Bruno,

Am Dienstag, dem 21.03.2023 um 02:10 +0000 schrieb Bruno Victal:
> 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.
I think it's a little late to come to this realization.  Note how my
prior attempt at fixing 61570 was delayed for more than a month so that
a proper sanitizer can be implemented and would have had a better user
interface than what you are currently proposing.

> 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. 
Need I remind you that the original concern was about backwards API
compatibility?  Yes, accepting strings and doing things with them is
broken for the reasons you mentioned and there should be a deprecation
warning about this.  But not heeding the user values is silly and you
should still set those vanity values for the sake of vanity itself.

Cheers