Adding new switch -rG to usermod #421

AZaugg · 2021-09-23T03:37:00Z

Adding a new switch -rG, which provides a similar feature set to -aG, allowing a person to list exactly what groups to remove a user from.

As we can see user tester is part of test[1..4]

[a@localhost src]$ tail /etc/group ; sudo tail /etc/gshadow
tester1:x:1003:
test1:x:1004:tester1
test2:x:1005:tester1
test3:x:1006:tester1
test4:x:1007:tester1
tester1:!::
test1:!::tester1
test2:!::tester1
test3:!::tester1
test4:!::tester1

Running with -rG, we can now see that we have removed user tester1 from the named groups, while leaving the unnamed groups untouched
[a@localhost src]$ sudo ./usermod -rG test1,test2 tester1
[a@localhost src]$ tail /etc/group ; sudo tail /etc/gshadow
tester1:x:1003:
test1:x:1004:
test2:x:1005:
test3:x:1006:tester1
test4:x:1007:tester1
tester1:!::
test1:!::
test2:!::
test3:!::tester1
test4:!::tester1

Testing -G flag to see if we broke anything,
[a@localhost src]$ sudo ./usermod -G test1,test2 tester1
[a@localhost src]$ tail /etc/group ; sudo tail /etc/gshadow
tester1:x:1003:
test1:x:1004:tester1
test2:x:1005:tester1
test3:x:1006:
test4:x:1007:
tester1:!::
test1:!::tester1
test2:!::tester1
test3:!::
test4:!::

Works as expected, lets test -aG see if we broke that
[a@localhost src]$ sudo ./usermod -aG test3,test4 tester1
[a@localhost src]$ tail /etc/group ; sudo tail /etc/gshadow
tester1:x:1003:
test1:x:1004:tester1
test2:x:1005:tester1
test3:x:1006:tester1
test4:x:1007:tester1
tester1:!::
test1:!::tester1
test2:!::tester1
test3:!::tester1
test4:!::tester1

Closes #337

ikerexxe

From the code review perspective I've only seen a little error.

I still would like to test the change though, to see that everything works as expected. By the way, have you considered automating the tests?

ikerexxe · 2021-09-23T10:26:19Z

src/usermod.c

@@ -424,6 +425,9 @@ static /*@noreturn@*/void usage (int status)
 	(void) fputs (_("  -a, --append                  append the user to the supplemental GROUPS\n"
 	                "                                mentioned by the -G option without removing\n"
 	                "                                the user from other groups\n"), usageout);
+	(void) fputs (_("  -r, --remove                  remove the user from only the supplemental GROUPS\n"
+	                "                                mentioned by the -G option without removing\n"
+	                "                                the user from theother groups\n"), usageout);


To be consistent with -a option you should remove the.

Hi @ikerexxe I fixed the grammatical error.

In regards to writing tests. Would need to look at how the tests are setup. Never looked at it before so. might have a few questions. Are the tests cases a hard requirement for this PR?

From my point of view no, it's not necessary, but the last word is up to the maintainers.

That reminds me that I haven't tested this PR. I'll try to do that today.

Hi @ikerexxe I fixed the grammatical error.

In regards to writing tests. Would need to look at how the tests are setup. Never looked at it before so. might have a few questions. Are the tests cases a hard requirement for this PR?

I do worry that if you don't write them now, they'll never get written :) So if you have time in the next week, that would be great. They'd go under tests/usertools/, one test per directory.

Unfortunately I haven't yet ported the github repo from travis to github actions, so currently the tests aren't being run.

I will try write the tests at a later time and in another commit.

ikerexxe · 2021-09-28T08:32:21Z

I've been able to test the changes with satisfactory results. Thanks for the changes!

Initial state:

$ id testuser1
uid=1001(testuser1) gid=1001(testuser1) groups=1001(testuser1)
$ id testuser2
uid=1002(testuser2) gid=1002(testuser2) groups=1002(testuser2)
$ getent group testgroup
testgroup:x:1009:
$ getent group anothergroup
anothergroup:x:1010:

Testing -G option:

$ sudo usermod -G testgroup testuser1
$ id testuser1
uid=1001(testuser1) gid=1001(testuser1) groups=1001(testuser1),1009(testgroup)
$ getent group testgroup 
testgroup:x:1009:testuser1
$ sudo usermod -G testgroup testuser2
$ id testuser2
uid=1002(testuser2) gid=1002(testuser2) groups=1002(testuser2),1009(testgroup)
$ getent group testgroup 
testgroup:x:1009:testuser1,testuser2

Testing aG options:

$ sudo usermod -aG anothergroup testuser1
$ sudo usermod -aG anothergroup testuser2
$ id testuser1
uid=1001(testuser1) gid=1001(testuser1) groups=1001(testuser1),1009(testgroup),1010(anothergroup)
$ id testuser2
uid=1002(testuser2) gid=1002(testuser2) groups=1002(testuser2),1009(testgroup),1010(anothergroup)
$ getent group testgroup 
testgroup:x:1009:testuser1,testuser2
$ getent group anothergroup
anothergroup:x:1010:testuser2,testuser1

Testing the new rG options:

$ sudo usermod -rG anothergroup testuser1
$ id testuser1
uid=1001(testuser1) gid=1001(testuser1) groups=1001(testuser1),1009(testgroup)
$ getent group testgroup 
testgroup:x:1009:testuser1,testuser2
$ getent group anothergroup
anothergroup:x:1010:testuser2
$ sudo usermod -rG testgroup testuser1
$ id testuser1
uid=1001(testuser1) gid=1001(testuser1) groups=1001(testuser1)
$ getent group anothergroup
anothergroup:x:1010:testuser2
$ getent group testgroup 
testgroup:x:1009:testuser2

hallyn · 2021-09-28T12:59:49Z

Thanks for the contribution, @AZaugg , and thanks @ikerexxe for testing.

src/usermod.c

jfhaugh

Might be a nice thing to fix.

src/usermod.c

hallyn · 2021-12-11T19:18:45Z

@AZaugg please rebase and either just squash these two commits into one, or split out the code change from the second patch into the first.

@ikerexxe did you get a chance to test this?

ikerexxe · 2021-12-13T08:10:59Z

@ikerexxe did you get a chance to test this?

No, I didn't have time to test the latest update. I may have time this week but I can't promise.

ikerexxe · 2021-12-13T15:12:39Z

I've been able to find some minutes to test it and everything looks good from my side.

AZaugg · 2021-12-13T17:33:38Z

Ok, I rebased from and collapsed the commits into a single commit.

src/usermod.c

Adding a new switch -rG, which provides a similar feature set to -aG, allowing a person to list exactly what groups to remove a user from. shadow-maint#337

hallyn · 2021-12-14T14:04:36Z

Thanks.

ikerexxe requested changes Sep 23, 2021

View reviewed changes

AZaugg force-pushed the issue337 branch from 3063b58 to 0f3b8c1 Compare September 27, 2021 17:10

hallyn reviewed Sep 28, 2021

View reviewed changes

src/usermod.c Show resolved Hide resolved

hallyn reviewed Sep 28, 2021

View reviewed changes

src/usermod.c Outdated Show resolved Hide resolved

hallyn reviewed Sep 28, 2021

View reviewed changes

src/usermod.c Show resolved Hide resolved

AZaugg force-pushed the issue337 branch from 0f3b8c1 to e3bd763 Compare November 3, 2021 03:16

jfhaugh reviewed Dec 2, 2021

View reviewed changes

src/usermod.c Outdated Show resolved Hide resolved

AZaugg force-pushed the issue337 branch from e3bd763 to 5e0a021 Compare December 9, 2021 03:17

AZaugg force-pushed the issue337 branch from 5e0a021 to 5d0506b Compare December 13, 2021 17:32

hallyn reviewed Dec 14, 2021

View reviewed changes

src/usermod.c Outdated Show resolved Hide resolved

Adding new option -rG to usermod

aaaaf21

Adding a new switch -rG, which provides a similar feature set to -aG, allowing a person to list exactly what groups to remove a user from. shadow-maint#337

AZaugg force-pushed the issue337 branch from 5d0506b to aaaaf21 Compare December 14, 2021 05:42

hallyn merged commit f2e8294 into shadow-maint:master Dec 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding new switch -rG to usermod #421

Adding new switch -rG to usermod #421

AZaugg commented Sep 23, 2021

ikerexxe left a comment •

edited

ikerexxe Sep 23, 2021

AZaugg Sep 27, 2021

ikerexxe Sep 28, 2021

hallyn Sep 28, 2021

AZaugg Nov 3, 2021

ikerexxe commented Sep 28, 2021

hallyn commented Sep 28, 2021

jfhaugh left a comment

hallyn commented Dec 11, 2021

ikerexxe commented Dec 13, 2021

ikerexxe commented Dec 13, 2021

AZaugg commented Dec 13, 2021

hallyn commented Dec 14, 2021

Adding new switch -rG to usermod #421

Adding new switch -rG to usermod #421

Conversation

AZaugg commented Sep 23, 2021

ikerexxe left a comment • edited

Choose a reason for hiding this comment

ikerexxe Sep 23, 2021

Choose a reason for hiding this comment

AZaugg Sep 27, 2021

Choose a reason for hiding this comment

ikerexxe Sep 28, 2021

Choose a reason for hiding this comment

hallyn Sep 28, 2021

Choose a reason for hiding this comment

AZaugg Nov 3, 2021

Choose a reason for hiding this comment

ikerexxe commented Sep 28, 2021

hallyn commented Sep 28, 2021

jfhaugh left a comment

Choose a reason for hiding this comment

hallyn commented Dec 11, 2021

ikerexxe commented Dec 13, 2021

ikerexxe commented Dec 13, 2021

AZaugg commented Dec 13, 2021

hallyn commented Dec 14, 2021

ikerexxe left a comment •

edited