unofficial mirror of bug-guix@gnu.org 
 help / color / mirror / code / Atom feed
* bug#45570: operating-system definitions allow duplicate passwd and group entries
@ 2020-12-31 18:14 Jason Conroy
  2021-01-01 11:13 ` bug#45570: [PATCH] system: Assert, that user and group names are unique Leo Prikler
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Jason Conroy @ 2020-12-31 18:14 UTC (permalink / raw)
  To: 45570


[-- Attachment #1.1: Type: text/plain, Size: 646 bytes --]

When an operating-system contains multiple users or groups with the same
name, instantiating it with `guix system` does not cause a validation
failure, nor are the duplicate entries filtered from the resulting /etc
files.

This duplication can happen in a few different ways:

- both entries are manually included in the "users" or "groups" fields of
the operating-system
- a manually-specified entry collides with an entry defined by a service
(via an account-service-type extension)
- multiple services define entries that collide with each other

Steps to reproduce: call "guix system container" with the attached
operating-system definition.

[-- Attachment #1.2: Type: text/html, Size: 805 bytes --]

[-- Attachment #2: duplicate-users-and-groups.scm --]
[-- Type: application/octet-stream, Size: 1827 bytes --]

(use-modules (gnu))
(use-service-modules networking ssh)
(use-package-modules screen ssh)

(operating-system
  (host-name "komputilo")
  (timezone "Europe/Berlin")
  (locale "en_US.utf8")
  (bootloader (bootloader-configuration
                (bootloader grub-bootloader)
                (target "/dev/sdX")))
  (file-systems (cons (file-system
                        (device (file-system-label "my-root"))
                        (mount-point "/")
                        (type "ext4"))
                      %base-file-systems))

  (users (append (list
		  ;; Two manually-specified users with the same name
		  (user-account
                   (name "alice")
                   (comment "Alice 1")
                   (group "users"))
		  (user-account
                   (name "alice")
                   (comment "Alice 2")
                   (group "users"))
		  ;; A manually-specified user with the same name as a
		  ;; user defined by the openssh-service.
		  (user-account
                   (name "sshd")
                   (comment "Secure shell user")
                   (group "sshd")
		   (system? #t)))
               %base-user-accounts))

  (groups (append (list
		  ;; Two manually-specified groups with the same name
		   (user-group
		    (name "power-users"))
		   (user-group
		    (name "power-users"))
		   ;; A manually-specified group with the same name as
		   ;; a group defined by the openssh-service.
		   (user-group
		    (name "sshd")))
		  %base-groups))
  
  (services (append (list (service dhcp-client-service-type)
                          (service openssh-service-type
                                   (openssh-configuration
                                    (openssh openssh-sans-x)
                                    (port-number 2222))))
                    %base-services)))

^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#45570: [PATCH] system: Assert, that user and group names are unique.
  2020-12-31 18:14 bug#45570: operating-system definitions allow duplicate passwd and group entries Jason Conroy
@ 2021-01-01 11:13 ` Leo Prikler
  2021-01-02  1:16   ` Danny Milosavljevic
  2021-01-02  5:57 ` Leo Prikler
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 15+ messages in thread
From: Leo Prikler @ 2021-01-01 11:13 UTC (permalink / raw)
  To: 45570; +Cc: conjaroy

*gnu/system/shadow.scm (assert-unique-account-names)
(assert-unique-group-names): New variables.
(account-activation): Use them here.
---
 gnu/system/shadow.scm | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/gnu/system/shadow.scm b/gnu/system/shadow.scm
index a69339bc07..61562f225e 100644
--- a/gnu/system/shadow.scm
+++ b/gnu/system/shadow.scm
@@ -222,6 +222,32 @@ for a colorful Guile experience.\\n\\n\"))))\n"))
                          (rename-file ".nanorc" ".config/nano/nanorc"))
                        #t))))
 
+(define (assert-unique-account-names users)
+  (let loop ((names '())
+             (users users))
+    (unless (null? users)
+      (let ((name (user-account-name (car users))))
+        (if (member name names)
+            (raise (condition
+                    (&message
+                     (message
+                      (format #f (G_ "account with name '~a' found twice")
+                              name)))))
+            (loop (cons name names) (cdr users)))))))
+
+(define (assert-unique-group-names groups)
+  (let loop ((names '())
+             (groups groups))
+    (unless (null? groups)
+      (let ((name (user-account-name (car groups))))
+        (if (member name names)
+            (raise (condition
+                    (&message
+                     (message
+                      (format #f (G_ "group with name '~a' found twice")
+                              name)))))
+            (loop (cons name names) (cdr groups)))))))
+
 (define (assert-valid-users/groups users groups)
   "Raise an error if USERS refer to groups not listed in GROUPS."
   (let ((groups (list->set (map user-group-name groups))))
@@ -292,6 +318,8 @@ group."
   (define group-specs
     (map user-group->gexp groups))
 
+  (assert-unique-account-names accounts)
+  (assert-unique-group-names groups)
   (assert-valid-users/groups accounts groups)
 
   ;; Add users and user groups.
-- 
2.29.2





^ permalink raw reply related	[flat|nested] 15+ messages in thread

* bug#45570: [PATCH] system: Assert, that user and group names are unique.
  2021-01-01 11:13 ` bug#45570: [PATCH] system: Assert, that user and group names are unique Leo Prikler
@ 2021-01-02  1:16   ` Danny Milosavljevic
  0 siblings, 0 replies; 15+ messages in thread
From: Danny Milosavljevic @ 2021-01-02  1:16 UTC (permalink / raw)
  To: Leo Prikler; +Cc: 45570, conjaroy

[-- Attachment #1: Type: text/plain, Size: 822 bytes --]

Hi Leo,

I agree that this is a good idea.

Please use (ice-9 match) instead of car and cdr.

Something among these lines would be more transparent:

    (define (find-duplicates list accessor)
      (match list
        (() '())
        ((head . tail)
         (if (member head tail accessor) ; (srfi srfi-1) member
             (cons head (find-duplicates tail accessor))
             (find-duplicates tail accessor)))))

(find-duplicates users
                 (lambda (a b)
                   (string=? (user-account-name a)
                             (user-account-name b)))

(I think one could also use srfi-1 delete-duplicates and then compare the
lengths.  Then the entire thing is a one-liner--the only complication is
to find the duplicates again after doing it (for the error message))

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#45570: [PATCH] system: Assert, that user and group names are unique.
  2020-12-31 18:14 bug#45570: operating-system definitions allow duplicate passwd and group entries Jason Conroy
  2021-01-01 11:13 ` bug#45570: [PATCH] system: Assert, that user and group names are unique Leo Prikler
@ 2021-01-02  5:57 ` Leo Prikler
  2021-01-06  9:56   ` Ludovic Courtès
  2021-01-06 21:21 ` bug#45570: [PATCH v2] " Leo Prikler
  2021-01-07 11:10 ` bug#45570: [PATCH v3] " Leo Prikler
  3 siblings, 1 reply; 15+ messages in thread
From: Leo Prikler @ 2021-01-02  5:57 UTC (permalink / raw)
  To: 45570; +Cc: conjaroy

*gnu/system/shadow.scm (find-duplicates): New variable.
(assert-unique-account-names, assert-unique-group-names): New variables.
(account-activation): Use them here.
---
 gnu/system/shadow.scm | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/gnu/system/shadow.scm b/gnu/system/shadow.scm
index a69339bc07..3a5ea4dc70 100644
--- a/gnu/system/shadow.scm
+++ b/gnu/system/shadow.scm
@@ -34,6 +34,7 @@
   #:use-module ((gnu packages admin)
                 #:select (shadow))
   #:use-module (gnu packages bash)
+  #:use-module (ice-9 match)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-26)
   #:use-module (srfi srfi-34)
@@ -222,6 +223,38 @@ for a colorful Guile experience.\\n\\n\"))))\n"))
                          (rename-file ".nanorc" ".config/nano/nanorc"))
                        #t))))
 
+(define (find-duplicates list =)
+  (match list
+    ('() '())
+    ((first . rest)
+     (if (member first rest =) ; (srfi srfi-1) member
+         (cons first (find-duplicates rest =))
+         (find-duplicates rest =)))))
+
+(define (assert-unique-account-names users)
+  (for-each
+   (lambda (account)
+     (raise (condition
+             (&message
+              (message
+               (format #f (G_ "account with name '~a' found twice.")
+                       (user-account-name account)))))))
+   (find-duplicates users (lambda (alice bob)
+                            (string=? (user-account-name alice)
+                                      (user-account-name bob))))))
+
+(define (assert-unique-group-names groups)
+  (for-each
+   (lambda (group)
+     (raise (condition
+             (&message
+              (message
+               (format #f (G_ "group with name '~a' found twice.")
+                       (user-group-name group)))))))
+   (find-duplicates groups (lambda (red blue)
+                             (string=? (user-group-name red)
+                                       (user-group-name blue))))))
+
 (define (assert-valid-users/groups users groups)
   "Raise an error if USERS refer to groups not listed in GROUPS."
   (let ((groups (list->set (map user-group-name groups))))
@@ -292,6 +325,8 @@ group."
   (define group-specs
     (map user-group->gexp groups))
 
+  (assert-unique-account-names accounts)
+  (assert-unique-group-names groups)
   (assert-valid-users/groups accounts groups)
 
   ;; Add users and user groups.
-- 
2.29.2





^ permalink raw reply related	[flat|nested] 15+ messages in thread

* bug#45570: [PATCH] system: Assert, that user and group names are unique.
  2021-01-02  5:57 ` Leo Prikler
@ 2021-01-06  9:56   ` Ludovic Courtès
  2021-01-06 12:34     ` Leo Prikler
  0 siblings, 1 reply; 15+ messages in thread
From: Ludovic Courtès @ 2021-01-06  9:56 UTC (permalink / raw)
  To: Leo Prikler; +Cc: 45570, conjaroy

Hi,

Leo Prikler <leo.prikler@student.tugraz.at> skribis:

> *gnu/system/shadow.scm (find-duplicates): New variable.
> (assert-unique-account-names, assert-unique-group-names): New variables.
> (account-activation): Use them here.

[...]

> +(define (find-duplicates list =)
> +  (match list
> +    ('() '())

This should be:

  (match list
    (() '())
    …)

I’m surprised '() works as a pattern.

> +    ((first . rest)
> +     (if (member first rest =) ; (srfi srfi-1) member
> +         (cons first (find-duplicates rest =))
> +         (find-duplicates rest =)))))

Note that this is quadratic; it’s fine as long as we don’t have “too
many” users, which may be the case in general.

> +(define (assert-unique-account-names users)
> +  (for-each
> +   (lambda (account)
> +     (raise (condition
> +             (&message
> +              (message
> +               (format #f (G_ "account with name '~a' found twice.")
> +                       (user-account-name account)))))))
> +   (find-duplicates users (lambda (alice bob)
> +                            (string=? (user-account-name alice)
> +                                      (user-account-name bob))))))

‘for-each’ looks awkward since we’ll stop on the first one.  How about
something like:

  (define (assert-unique-account-names users)
    (match (find-duplicates things …)
      (() #t)
      (lst
       (raise (formatted-message (G_ "the following accounts appear more than once:~{ ~a~}~%"
                                 lst))))))

?

Thanks!

Ludo’.




^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#45570: [PATCH] system: Assert, that user and group names are unique.
  2021-01-06  9:56   ` Ludovic Courtès
@ 2021-01-06 12:34     ` Leo Prikler
  2021-01-06 13:32       ` Ludovic Courtès
  0 siblings, 1 reply; 15+ messages in thread
From: Leo Prikler @ 2021-01-06 12:34 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: 45570, conjaroy

Hi,

Am Mittwoch, den 06.01.2021, 10:56 +0100 schrieb Ludovic Courtès:
> Hi,
> 
> Leo Prikler <leo.prikler@student.tugraz.at> skribis:
> 
> > *gnu/system/shadow.scm (find-duplicates): New variable.
> > (assert-unique-account-names, assert-unique-group-names): New
> > variables.
> > (account-activation): Use them here.
> 
> [...]
> 
> > +(define (find-duplicates list =)
> > +  (match list
> > +    ('() '())
> 
> This should be:
> 
>   (match list
>     (() '())
>     …)
> 
> I’m surprised '() works as a pattern.
I think it's because matching literals works, but you're right.

> > +    ((first . rest)
> > +     (if (member first rest =) ; (srfi srfi-1) member
> > +         (cons first (find-duplicates rest =))
> > +         (find-duplicates rest =)))))
> 
> Note that this is quadratic; it’s fine as long as we don’t have “too
> many” users, which may be the case in general.
It is indeed quadratic, but would there even be an n log n solution?
I've once done an n log n sort+delete-duplicates!, perhaps that'd be a
nicer solution here?

> > +(define (assert-unique-account-names users)
> > +  (for-each
> > +   (lambda (account)
> > +     (raise (condition
> > +             (&message
> > +              (message
> > +               (format #f (G_ "account with name '~a' found
> > twice.")
> > +                       (user-account-name account)))))))
> > +   (find-duplicates users (lambda (alice bob)
> > +                            (string=? (user-account-name alice)
> > +                                      (user-account-name bob))))))
> 
> ‘for-each’ looks awkward since we’ll stop on the first one.  How
> about
> something like:
> 
>   (define (assert-unique-account-names users)
>     (match (find-duplicates things …)
>       (() #t)
>       (lst
>        (raise (formatted-message (G_ "the following accounts appear
> more than once:~{ ~a~}~%"
>                                  lst))))))
> 
> ?
That'd be weird for duplicate duplicates, hence just reporting the
first.  Of course we could always count occurrences by allocating a
local hash table and then do some fancy hash-map->list conversion.  If
we do use hash-tables, perhaps this could even be a linear algorithm?

Regards,
Leo





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#45570: [PATCH] system: Assert, that user and group names are unique.
  2021-01-06 12:34     ` Leo Prikler
@ 2021-01-06 13:32       ` Ludovic Courtès
  2021-01-06 21:00         ` Leo Prikler
  0 siblings, 1 reply; 15+ messages in thread
From: Ludovic Courtès @ 2021-01-06 13:32 UTC (permalink / raw)
  To: Leo Prikler; +Cc: 45570, conjaroy

Hi,

Leo Prikler <leo.prikler@student.tugraz.at> skribis:

>> > +    ((first . rest)
>> > +     (if (member first rest =) ; (srfi srfi-1) member
>> > +         (cons first (find-duplicates rest =))
>> > +         (find-duplicates rest =)))))
>> 
>> Note that this is quadratic; it’s fine as long as we don’t have “too
>> many” users, which may be the case in general.
> It is indeed quadratic, but would there even be an n log n solution?
> I've once done an n log n sort+delete-duplicates!, perhaps that'd be a
> nicer solution here?

You could first build a hash table or vhash or set with all the names,
then traverse again the list of names and check whether they’re in that
table.  That’d be linear (assuming the table is well balanced), but the
constant factor would be higher.

>>   (define (assert-unique-account-names users)
>>     (match (find-duplicates things …)
>>       (() #t)
>>       (lst
>>        (raise (formatted-message (G_ "the following accounts appear
>> more than once:~{ ~a~}~%"
>>                                  lst))))))
>> 
>> ?
> That'd be weird for duplicate duplicates, hence just reporting the
> first.

You could do (delete-duplicates lst) in the message above?

Thanks,
Ludo’.




^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#45570: [PATCH] system: Assert, that user and group names are unique.
  2021-01-06 13:32       ` Ludovic Courtès
@ 2021-01-06 21:00         ` Leo Prikler
  2021-01-07  8:29           ` Ludovic Courtès
  0 siblings, 1 reply; 15+ messages in thread
From: Leo Prikler @ 2021-01-06 21:00 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: 45570, conjaroy

Hi,

Am Mittwoch, den 06.01.2021, 14:32 +0100 schrieb Ludovic Courtès:
> Hi,
> 
> Leo Prikler <leo.prikler@student.tugraz.at> skribis:
> 
> > > > +    ((first . rest)
> > > > +     (if (member first rest =) ; (srfi srfi-1) member
> > > > +         (cons first (find-duplicates rest =))
> > > > +         (find-duplicates rest =)))))
> > > 
> > > Note that this is quadratic; it’s fine as long as we don’t have
> > > “too
> > > many” users, which may be the case in general.
> > It is indeed quadratic, but would there even be an n log n
> > solution?
> > I've once done an n log n sort+delete-duplicates!, perhaps that'd
> > be a
> > nicer solution here?
> 
> You could first build a hash table or vhash or set with all the
> names,
> then traverse again the list of names and check whether they’re in
> that
> table.  That’d be linear (assuming the table is well balanced), but
> the
> constant factor would be higher.
Yeah, I think the hash table solution would make the most sense here. 
Since VHashes are based on VLists, they're not actually purely
functional, are they?

> > >   (define (assert-unique-account-names users)
> > >     (match (find-duplicates things …)
> > >       (() #t)
> > >       (lst
> > >        (raise (formatted-message (G_ "the following accounts
> > > appear
> > > more than once:~{ ~a~}~%"
> > >                                  lst))))))
> > > 
> > > ?
> > That'd be weird for duplicate duplicates, hence just reporting the
> > first.
> 
> You could do (delete-duplicates lst) in the message above?
Sure, but that'd be O(n^2) on top of O(n^2), which is less than ideal.
I think I'll try working on a hash-based implementation for now.

Regards,
Leo





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#45570: [PATCH v2] system: Assert, that user and group names are unique.
  2020-12-31 18:14 bug#45570: operating-system definitions allow duplicate passwd and group entries Jason Conroy
  2021-01-01 11:13 ` bug#45570: [PATCH] system: Assert, that user and group names are unique Leo Prikler
  2021-01-02  5:57 ` Leo Prikler
@ 2021-01-06 21:21 ` Leo Prikler
  2021-01-07  8:35   ` Ludovic Courtès
  2021-01-07 11:10 ` bug#45570: [PATCH v3] " Leo Prikler
  3 siblings, 1 reply; 15+ messages in thread
From: Leo Prikler @ 2021-01-06 21:21 UTC (permalink / raw)
  To: 45570; +Cc: conjaroy

*gnu/system/shadow.scm (find-duplicates): New variable.
(assert-unique-account-names, assert-unique-group-names): New variables.
(account-activation): Use them here.
---
 gnu/system/shadow.scm | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/gnu/system/shadow.scm b/gnu/system/shadow.scm
index a69339bc07..4dbd578e1e 100644
--- a/gnu/system/shadow.scm
+++ b/gnu/system/shadow.scm
@@ -20,6 +20,7 @@
 ;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
 
 (define-module (gnu system shadow)
+  #:use-module ((guix diagnostics) #:select (formatted-message))
   #:use-module (guix records)
   #:use-module (guix gexp)
   #:use-module (guix store)
@@ -34,6 +35,7 @@
   #:use-module ((gnu packages admin)
                 #:select (shadow))
   #:use-module (gnu packages bash)
+  #:use-module (ice-9 match)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-26)
   #:use-module (srfi srfi-34)
@@ -222,6 +224,40 @@ for a colorful Guile experience.\\n\\n\"))))\n"))
                          (rename-file ".nanorc" ".config/nano/nanorc"))
                        #t))))
 
+(define (find-duplicates list)
+  (let loop ((table (make-hash-table))
+             (list list))
+    (match list
+      (()
+       (hash-fold (lambda (key value seed)
+                    (if (> value 1)
+                        (cons key seed)
+                        seed))
+                  '()
+                  table))
+      ((first . rest)
+       (hash-set! table first
+                  (1+ (hash-ref table first 0)))
+       (loop table rest)))))
+
+(define (assert-unique-account-names users)
+  (match (find-duplicates (map user-account-name users))
+    (() *unspecified*)
+    (duplicates
+     (raise
+      (formatted-message
+       (G_ "the following accounts appear more than once:~{ ~a~}~%")
+       duplicates)))))
+
+(define (assert-unique-group-names groups)
+  (match (find-duplicates (map user-group-name groups))
+    (() *unspecified*)
+    (duplicates
+     (raise
+      (formatted-message
+       (G_ "the following groups appear more than once:~{ ~a~}~%")
+       duplicates)))))
+
 (define (assert-valid-users/groups users groups)
   "Raise an error if USERS refer to groups not listed in GROUPS."
   (let ((groups (list->set (map user-group-name groups))))
@@ -292,6 +328,8 @@ group."
   (define group-specs
     (map user-group->gexp groups))
 
+  (assert-unique-account-names accounts)
+  (assert-unique-group-names groups)
   (assert-valid-users/groups accounts groups)
 
   ;; Add users and user groups.
-- 
2.30.0





^ permalink raw reply related	[flat|nested] 15+ messages in thread

* bug#45570: [PATCH] system: Assert, that user and group names are unique.
  2021-01-06 21:00         ` Leo Prikler
@ 2021-01-07  8:29           ` Ludovic Courtès
  0 siblings, 0 replies; 15+ messages in thread
From: Ludovic Courtès @ 2021-01-07  8:29 UTC (permalink / raw)
  To: Leo Prikler; +Cc: 45570, conjaroy

Hi,

Leo Prikler <leo.prikler@student.tugraz.at> skribis:

> Am Mittwoch, den 06.01.2021, 14:32 +0100 schrieb Ludovic Courtès:
>> Hi,
>> 
>> Leo Prikler <leo.prikler@student.tugraz.at> skribis:
>> 
>> > > > +    ((first . rest)
>> > > > +     (if (member first rest =) ; (srfi srfi-1) member
>> > > > +         (cons first (find-duplicates rest =))
>> > > > +         (find-duplicates rest =)))))
>> > > 
>> > > Note that this is quadratic; it’s fine as long as we don’t have
>> > > “too
>> > > many” users, which may be the case in general.
>> > It is indeed quadratic, but would there even be an n log n
>> > solution?
>> > I've once done an n log n sort+delete-duplicates!, perhaps that'd
>> > be a
>> > nicer solution here?
>> 
>> You could first build a hash table or vhash or set with all the
>> names,
>> then traverse again the list of names and check whether they’re in
>> that
>> table.  That’d be linear (assuming the table is well balanced), but
>> the
>> constant factor would be higher.
> Yeah, I think the hash table solution would make the most sense here. 
> Since VHashes are based on VLists, they're not actually purely
> functional, are they?

Their implementation is not “purely functional” but it’s
inconsequential; it’s a persistent data structure, and that’s what
matters (info "(guile) VLists").

>> > >   (define (assert-unique-account-names users)
>> > >     (match (find-duplicates things …)
>> > >       (() #t)
>> > >       (lst
>> > >        (raise (formatted-message (G_ "the following accounts
>> > > appear
>> > > more than once:~{ ~a~}~%"
>> > >                                  lst))))))
>> > > 
>> > > ?
>> > That'd be weird for duplicate duplicates, hence just reporting the
>> > first.
>> 
>> You could do (delete-duplicates lst) in the message above?
> Sure, but that'd be O(n^2) on top of O(n^2), which is less than ideal.

Yes, but it’s a small ‘n’, typically one or two.

Ludo’.




^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#45570: [PATCH v2] system: Assert, that user and group names are unique.
  2021-01-06 21:21 ` bug#45570: [PATCH v2] " Leo Prikler
@ 2021-01-07  8:35   ` Ludovic Courtès
  2021-01-07 11:13     ` Leo Prikler
  0 siblings, 1 reply; 15+ messages in thread
From: Ludovic Courtès @ 2021-01-07  8:35 UTC (permalink / raw)
  To: Leo Prikler; +Cc: 45570, conjaroy

Leo Prikler <leo.prikler@student.tugraz.at> skribis:

> *gnu/system/shadow.scm (find-duplicates): New variable.
> (assert-unique-account-names, assert-unique-group-names): New variables.
> (account-activation): Use them here.

Final nitpicks!  :-)

> +(define (find-duplicates list)

Please add a docstring.

> +  (let loop ((table (make-hash-table))
> +             (list list))

You can move ‘table’ out of the ‘loop’ arguments since it’s mutated
anyway.

OK with these changes!

Thanks,
Ludo’.




^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#45570: [PATCH v3] system: Assert, that user and group names are unique.
  2020-12-31 18:14 bug#45570: operating-system definitions allow duplicate passwd and group entries Jason Conroy
                   ` (2 preceding siblings ...)
  2021-01-06 21:21 ` bug#45570: [PATCH v2] " Leo Prikler
@ 2021-01-07 11:10 ` Leo Prikler
  2021-01-11 13:09   ` Ludovic Courtès
  3 siblings, 1 reply; 15+ messages in thread
From: Leo Prikler @ 2021-01-07 11:10 UTC (permalink / raw)
  To: 45570; +Cc: conjaroy

*gnu/system/shadow.scm (find-duplicates): New variable.
(assert-unique-account-names, assert-unique-group-names): New variables.
(account-activation): Use them here.
---
 gnu/system/shadow.scm | 44 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/gnu/system/shadow.scm b/gnu/system/shadow.scm
index a69339bc07..183b2cd387 100644
--- a/gnu/system/shadow.scm
+++ b/gnu/system/shadow.scm
@@ -20,6 +20,7 @@
 ;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
 
 (define-module (gnu system shadow)
+  #:use-module ((guix diagnostics) #:select (formatted-message))
   #:use-module (guix records)
   #:use-module (guix gexp)
   #:use-module (guix store)
@@ -34,6 +35,7 @@
   #:use-module ((gnu packages admin)
                 #:select (shadow))
   #:use-module (gnu packages bash)
+  #:use-module (ice-9 match)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-26)
   #:use-module (srfi srfi-34)
@@ -222,6 +224,46 @@ for a colorful Guile experience.\\n\\n\"))))\n"))
                          (rename-file ".nanorc" ".config/nano/nanorc"))
                        #t))))
 
+(define (find-duplicates list)
+  "Find duplicate entries in @var{list}.
+Two entries are considered duplicates, if they are @code{equal?} to each other.
+This implementation is made asymptotically faster than @code{delete-duplicates}
+through the internal use of hash tables."
+  (let loop ((list list)
+             ;; We actually modify table in-place, but still allocate it here
+             ;; so that we only need one level of indentation.
+             (table (make-hash-table)))
+    (match list
+      (()
+       (hash-fold (lambda (key value seed)
+                    (if (> value 1)
+                        (cons key seed)
+                        seed))
+                  '()
+                  table))
+      ((first . rest)
+       (hash-set! table first
+                  (1+ (hash-ref table first 0)))
+       (loop rest table)))))
+
+(define (assert-unique-account-names users)
+  (match (find-duplicates (map user-account-name users))
+    (() *unspecified*)
+    (duplicates
+     (raise
+      (formatted-message
+       (G_ "the following accounts appear more than once:~{ ~a~}")
+       duplicates)))))
+
+(define (assert-unique-group-names groups)
+  (match (find-duplicates (map user-group-name groups))
+    (() *unspecified*)
+    (duplicates
+     (raise
+      (formatted-message
+       (G_ "the following groups appear more than once:~{ ~a~}")
+       duplicates)))))
+
 (define (assert-valid-users/groups users groups)
   "Raise an error if USERS refer to groups not listed in GROUPS."
   (let ((groups (list->set (map user-group-name groups))))
@@ -292,6 +334,8 @@ group."
   (define group-specs
     (map user-group->gexp groups))
 
+  (assert-unique-account-names accounts)
+  (assert-unique-group-names groups)
   (assert-valid-users/groups accounts groups)
 
   ;; Add users and user groups.
-- 
2.30.0





^ permalink raw reply related	[flat|nested] 15+ messages in thread

* bug#45570: [PATCH v2] system: Assert, that user and group names are unique.
  2021-01-07  8:35   ` Ludovic Courtès
@ 2021-01-07 11:13     ` Leo Prikler
  0 siblings, 0 replies; 15+ messages in thread
From: Leo Prikler @ 2021-01-07 11:13 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: 45570, conjaroy

Am Donnerstag, den 07.01.2021, 09:35 +0100 schrieb Ludovic Courtès:
> Leo Prikler <leo.prikler@student.tugraz.at> skribis:
> 
> > *gnu/system/shadow.scm (find-duplicates): New variable.
> > (assert-unique-account-names, assert-unique-group-names): New
> > variables.
> > (account-activation): Use them here.
> 
> Final nitpicks!  :-)
> 
> > +(define (find-duplicates list)
> 
> Please add a docstring.
Done, see v3.

> > +  (let loop ((table (make-hash-table))
> > +             (list list))
> 
> You can move ‘table’ out of the ‘loop’ arguments since it’s mutated
> anyway.
I don't see any benefit from doing so, however.  It'd be an additional
layer of mutation and if we ever wanted to change to vhashes or alists
we'd have to refactor that.  

Regards,
Leo





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#45570: [PATCH v3] system: Assert, that user and group names are unique.
  2021-01-07 11:10 ` bug#45570: [PATCH v3] " Leo Prikler
@ 2021-01-11 13:09   ` Ludovic Courtès
  2021-01-11 15:06     ` Leo Prikler
  0 siblings, 1 reply; 15+ messages in thread
From: Ludovic Courtès @ 2021-01-11 13:09 UTC (permalink / raw)
  To: Leo Prikler; +Cc: 45570, conjaroy

Hi,

Leo Prikler <leo.prikler@student.tugraz.at> skribis:

> *gnu/system/shadow.scm (find-duplicates): New variable.
> (assert-unique-account-names, assert-unique-group-names): New variables.
> (account-activation): Use them here.

LGTM, thanks! :-)

Ludo’.




^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#45570: [PATCH v3] system: Assert, that user and group names are unique.
  2021-01-11 13:09   ` Ludovic Courtès
@ 2021-01-11 15:06     ` Leo Prikler
  0 siblings, 0 replies; 15+ messages in thread
From: Leo Prikler @ 2021-01-11 15:06 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: 45570-done, conjaroy

Am Montag, den 11.01.2021, 14:09 +0100 schrieb Ludovic Courtès:
> Hi,
> 
> Leo Prikler <leo.prikler@student.tugraz.at> skribis:
> 
> > *gnu/system/shadow.scm (find-duplicates): New variable.
> > (assert-unique-account-names, assert-unique-group-names): New
> > variables.
> > (account-activation): Use them here.
> 
> LGTM, thanks! :-)
> 
> Ludo’.
Aaaand it's pushed.





^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2021-01-11 15:10 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-31 18:14 bug#45570: operating-system definitions allow duplicate passwd and group entries Jason Conroy
2021-01-01 11:13 ` bug#45570: [PATCH] system: Assert, that user and group names are unique Leo Prikler
2021-01-02  1:16   ` Danny Milosavljevic
2021-01-02  5:57 ` Leo Prikler
2021-01-06  9:56   ` Ludovic Courtès
2021-01-06 12:34     ` Leo Prikler
2021-01-06 13:32       ` Ludovic Courtès
2021-01-06 21:00         ` Leo Prikler
2021-01-07  8:29           ` Ludovic Courtès
2021-01-06 21:21 ` bug#45570: [PATCH v2] " Leo Prikler
2021-01-07  8:35   ` Ludovic Courtès
2021-01-07 11:13     ` Leo Prikler
2021-01-07 11:10 ` bug#45570: [PATCH v3] " Leo Prikler
2021-01-11 13:09   ` Ludovic Courtès
2021-01-11 15:06     ` Leo Prikler

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/guix.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).