[bug#78188,v2] services: kwallet: New service.

  Change-Id: I1330ce5e1648a8ddf6ddd507255a73335d6baa51
---
 doc/guix.texi            | 37 ++++++++++++++++++++++++
 gnu/services/desktop.scm | 61 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 98 insertions(+)
  

Hi,

Sergio Pastor Pérez <sergio.pastorperez@gmail.com> writes:

> Change-Id: I1330ce5e1648a8ddf6ddd507255a73335d6baa51
> ---
>  doc/guix.texi            | 37 ++++++++++++++++++++++++
>  gnu/services/desktop.scm | 61 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 98 insertions(+)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index 7b418a4089..c6861b3182 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -27131,6 +27131,43 @@ Desktop Services
>  @end table
>  @end deftp
>  
> +@defvar kwallet-service-type
> +This is the type of the service that adds the
> +@uref{https://invent.kde.org/plasma/kwallet-pam, KWallet keyring}.  Its
> +value is a @code{kwallet-configuration} object (see below).  Note that,
> +contrary to @code{gnome-desktop-service-type},
> +@code{plasma-desktop-service-type} does not include this service.

Does gnome-desktop-service-type include the kwallet-service-type?  I
wouldn't think so.  You probably meant to say it "doesn't include a
wallet service in its default configuration." ?  Is this the normal
expectation for the KDE desktop?  I'd assume it comes with kwallet
pre-configured, if using Fedora for example.  If it does, we should
probably do so to avoid breaking users expectations.  I've recently made
the adjustment in GNOME to have the GNOME keyring unlocked by default
for the GNOME desktop, as that's what users expect.

> +This service adds the @code{kwallet-pam} package to the system profile
> +and extends PAM with entries using @code{pam_kwallet5.so},
> +unlocking a user's login keyring when they log in or setting its
> +password with passwd.

s/passwd/@command{passwd}/

> +@end defvar
> +
> +@deftp {Data Type} kwallet-configuration
> +Configuration record for the KWallet Keyring service.
> +
> +@table @asis
> +@item @code{keyring} (default: @code{kwallet-pam})
> +The KWallet keyring package to use.
> +
> +@item @code{pam-services}
> +A list of @code{(@var{service} . @var{kind})} pairs denoting PAM
> +services to extend, where @var{service} is the name of an existing
> +service to extend and @var{kind} is one of @code{login} or
> +@code{passwd}.

Perhaps mention "is one of the @code{login} or @{passwd} symbols.".  I
gues the quote is on the alist so individual values should not be
quoted, but just to avoid any ambiguity.

> +
> +If @code{login} is given, it adds an optional
> +@code{pam_kwallet5.so} to the auth block without arguments and to
> +the session block with @code{auto_start}.  If @code{passwd} is given, it
> +adds an optional @code{pam_kwallet5.so} to the password block
> +without arguments.
> +
> +By default, this field contains ``sddm'' with the value @code{login}
> +and ``passwd'' is with the value @code{passwd}.

Does KDE not have its own graphical desktop manager?  Perhaps it's not
yet ready in Guix?

[...]

> +;;;
> +;;; kwallet-service-type
> +;;;

nitpick: We conventionally add a '.' after these section names, as in:

;;;
;;; kwallet-service-type.
;;;


> +
> +(define-record-type* <kwallet-configuration> kwallet-configuration
> +  make-kwallet-configuration
> +  kwallet-configuration?
> +  (wallet kwallet-package (default kwallet-pam))
> +  (pam-services kwallet-pam-services (default '(("sddm" . login)
> +                                                ("passwd" . passwd)))))
> +
> +(define (pam-kwallet config)

Add add a brief docstring here saying this returns a PAM extension for
KWallet.

> +  (match config
> +    (#f '())                          ;explicitly disabled by user
> +    (_
> +     (define (%pam-keyring-entry . arguments)
> +       (pam-entry
> +        (control "optional")
> +        (module (file-append (kwallet-package config)
> +                             "/lib/security/pam_kwallet5.so"))
> +        (arguments arguments)))
> +
> +     (list
> +      (pam-extension
> +       (transformer
> +        (lambda (service)
> +          (case (assoc-ref (kwallet-pam-services config)
> +                           (pam-service-name service))
> +            ((login)
> +             (pam-service
> +              (inherit service)
> +              (auth (append (pam-service-auth service)
> +                            (list (%pam-keyring-entry))))
> +              (session (append (pam-service-session service)
> +                               (list (%pam-keyring-entry "auto_start"))))))
> +            ((passwd)
> +             (pam-service
> +              (inherit service)
> +              (password (append (pam-service-password service)
> +                                (list (%pam-keyring-entry))))))
> +            (else service)))))))))
> +
> +;; TODO: consider integrating service in `<plasma-desktop-configuration>' as
> +;; done in `<gnome-desktop-configuration>'. This requires rewritting the
> +;; `<plasma-desktop-service-type>' as done for `<gnome-desktop-service-type>'.

Ah, I see my comment above is acknowledged here as a TODO.  I'd
encourage you to pursue that next!

> +(define kwallet-service-type
> +  (service-type
> +   (name 'kwallet)
> +   (extensions (list
> +                (service-extension pam-root-service-type pam-kwallet)))
> +   (default-value (kwallet-configuration))
> +   (description "Return a service, that extends PAM with entries using

I'd drop the first comma.

> +@code{pam_kwallet5.so}, unlocking a user's login keyring when they
> log in or

s/a user's/the user's/

> +setting its password with passwd.")))

s/passwd/@command{passwd}/

Otherwise it LGTM.  Could you please send a v2?
  

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:
> Sergio Pastor Pérez <sergio.pastorperez@gmail.com> writes:
>> +@defvar kwallet-service-type
>> +This is the type of the service that adds the
>> +@uref{https://invent.kde.org/plasma/kwallet-pam, KWallet keyring}.  Its
>> +value is a @code{kwallet-configuration} object (see below).  Note that,
>> +contrary to @code{gnome-desktop-service-type},
>> +@code{plasma-desktop-service-type} does not include this service.
>
> Does gnome-desktop-service-type include the kwallet-service-type?  I
> wouldn't think so.  You probably meant to say it "doesn't include a
> wallet service in its default configuration." ?  Is this the normal
> expectation for the KDE desktop?  I'd assume it comes with kwallet
> pre-configured, if using Fedora for example.  If it does, we should
> probably do so to avoid breaking users expectations.  I've recently made
> the adjustment in GNOME to have the GNOME keyring unlocked by default
> for the GNOME desktop, as that's what users expect.

I think I made a mistake with the wording. What I meant is that, while
the `gnome-desktop-service-type' includes a keyring field which provides
the functionality of `gnome-keyring-service-type' making it necessary
for a `gnome-desktop-service-type' user to add a
`gnome-keyring-service-type' entry to their service list; the
`plasma-desktop-service-type' does not provide this functionality by
default. Which contrary to what the user may expect, they will need to
add the `kwallet-service-type' to their list of services even if they
are users of the `plasma-desktop-service-type'.

In the v2 of this patch series I've rewrote that bit to make it more
clear. Let me know what you think.

>> +;; TODO: consider integrating service in `<plasma-desktop-configuration>' as
>> +;; done in `<gnome-desktop-configuration>'. This requires rewritting the
>> +;; `<plasma-desktop-service-type>' as done for `<gnome-desktop-service-type>'.
>
> Ah, I see my comment above is acknowledged here as a TODO.  I'd
> encourage you to pursue that next!

This will require a major refactor of the `plasma-desktop-service-type'
as was done for the `gnome-desktop-service-type' to make it
modular. Once we get this merged I will try to join the kde team to help
with the refactor.

> Otherwise it LGTM.  Could you please send a v2?

Sure, aside for the things I've mentioned I've also corrected all the
typos you noticed.

Thanks for the review!


Best regards,
Sergio.