[PATCH] generic/193: Ensure user in expected group

[PATCH] generic/193: Ensure user in expected group

[Richard Weinberger]

The test assumes that the $qa_user is member of the group
$qa_user. While this is automatically the case on systems
with USERGROUPS_ENAB set in /etc/login.defs, not all
enable this option. Most notably SUSE Linux.

So make sure this case is noticed and reported instead of
failing the test and let the test guy puzzle.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 common/rc         | 11 +++++++++++
 tests/generic/193 |  1 +
 2 files changed, 12 insertions(+)

diff --git a/common/rc b/common/rc
index a9e0ba7e..5c6671a3 100644
--- a/common/rc
+++ b/common/rc
@@ -2484,6 +2484,17 @@ _require_user()
 	[ "$?" == "0" ] || _notrun "$qa_user cannot execute commands."
 }
 
+# check if a user is member of a specifc group
+#
+_require_user_in_group()
+{
+	local user="$1"
+	local group="$2"
+
+	id -n -G $user | grep -w -q $group
+	[ "$?" == "0" ] || _notrun "$user not in group $group."
+}
+
 # check for a chown support
 #
 _require_chown()
diff --git a/tests/generic/193 b/tests/generic/193
index e2710b07..d543f321 100755
--- a/tests/generic/193
+++ b/tests/generic/193
@@ -51,6 +51,7 @@ _supported_fs generic
 
 _require_test
 _require_user
+_require_user_in_group ${qa_user} ${qa_user}
 _require_chown
 
 test_root=$TEST_DIR/$seq.$$.root
-- 
2.35.3

Re: [PATCH] generic/193: Ensure user in expected group

[Dave Chinner]

On Mon, Jan 15, 2024 at 10:24:02PM +0100, Richard Weinberger wrote:
> The test assumes that the $qa_user is member of the group
> $qa_user. While this is automatically the case on systems
> with USERGROUPS_ENAB set in /etc/login.defs, not all
> enable this option. Most notably SUSE Linux.
> 
> So make sure this case is noticed and reported instead of
> failing the test and let the test guy puzzle.

> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  common/rc         | 11 +++++++++++
>  tests/generic/193 |  1 +
>  2 files changed, 12 insertions(+)
> 
> diff --git a/common/rc b/common/rc
> index a9e0ba7e..5c6671a3 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2484,6 +2484,17 @@ _require_user()
>  	[ "$?" == "0" ] || _notrun "$qa_user cannot execute commands."
>  }
>  
> +# check if a user is member of a specifc group
> +#
> +_require_user_in_group()
> +{
> +	local user="$1"
> +	local group="$2"
> +
> +	id -n -G $user | grep -w -q $group
> +	[ "$?" == "0" ] || _notrun "$user not in group $group."
> +}

Just do this check in _require_user(). The user needs to be set up
correctly for all tests - if the user and group is not set up
correctly, don't run any of the tests that require that user.

This means we don't have to play whack-a-mole with "user has no
group" every time someone assumes that a user is created with a
group by default.

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] generic/193: Ensure user in expected group

[Richard Weinberger]

Dave,

----- Ursprüngliche Mail -----
> Von: "Dave Chinner" <david@fromorbit.com>
> Just do this check in _require_user(). The user needs to be set up
> correctly for all tests - if the user and group is not set up
> correctly, don't run any of the tests that require that user.
> 
> This means we don't have to play whack-a-mole with "user has no
> group" every time someone assumes that a user is created with a
> group by default.

I think _require_user_exists() fits better, because _require_user()
tests whether the user can run commands via su.

Are we all happy with something like that?
diff --git a/common/rc b/common/rc
index a9e0ba7e..f1eabf3c 100644
--- a/common/rc
+++ b/common/rc
@@ -2461,13 +2461,16 @@ _cat_group()
        cat /etc/group
 }
 
-# check if a user exists in the system
+# check if a user exists in the system and has a user group
 #
 _require_user_exists()
 {
        local user=$1
        _cat_passwd | grep -q "^$user:"
        [ "$?" == "0" ] || _notrun "$user user not defined."
+
+       id -n -G $user | grep -w -q $user
+       [ "$?" == "0" ] || _notrun "$user not in group $user."
 }
 
 # check if a user exists and is able to execute commands.

Thanks,
//richard

Re: [PATCH] generic/193: Ensure user in expected group

[Zorro Lang]

On Tue, Jan 16, 2024 at 10:19:36AM +0100, Richard Weinberger wrote:
> Dave,
> 
> ----- Ursprüngliche Mail -----
> > Von: "Dave Chinner" <david@fromorbit.com>
> > Just do this check in _require_user(). The user needs to be set up
> > correctly for all tests - if the user and group is not set up
> > correctly, don't run any of the tests that require that user.
> > 
> > This means we don't have to play whack-a-mole with "user has no
> > group" every time someone assumes that a user is created with a
> > group by default.

I think Dave is right. Due to not only the g/193, lots of cases base
on "fsgqa* user is in fsgqa* group". If you hope to check that by
the single function _require_user_in_group(), it might need to call
it in most of cases which use _require_user.

But yes, as you said, it's weird to do it in _require_user, that makes
the _require_group calling is a bit redundant.

If I have to choose one way, I think we can keep the _require_user_in_group
as an independent function, but call it in _require_user, e.g.

_require_user()
{
        qa_user=fsgqa
        if [ -n "$1" ];then
                qa_user=$1
        fi
        _require_user_exists $qa_user
+	# fstests needs each test user is in its own group name, it only need
+	# a user exist, please use _require_user_exists directly.
+	_require_user_in_group $qa_user $qa_user
        echo /bin/true | su $qa_user
        [ "$?" == "0" ] || _notrun "$qa_user cannot execute commands."
}

What do both of you think?

Thanks
Zorro

> 
> I think _require_user_exists() fits better, because _require_user()
> tests whether the user can run commands via su.
> 
> Are we all happy with something like that?
> diff --git a/common/rc b/common/rc
> index a9e0ba7e..f1eabf3c 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2461,13 +2461,16 @@ _cat_group()
>         cat /etc/group
>  }
>  
> -# check if a user exists in the system
> +# check if a user exists in the system and has a user group
>  #
>  _require_user_exists()
>  {
>         local user=$1
>         _cat_passwd | grep -q "^$user:"
>         [ "$?" == "0" ] || _notrun "$user user not defined."
> +
> +       id -n -G $user | grep -w -q $user
> +       [ "$?" == "0" ] || _notrun "$user not in group $user."
>  }
>  
>  # check if a user exists and is able to execute commands.
> 
> Thanks,
> //richard
> 

Re: [PATCH] generic/193: Ensure user in expected group

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Zorro Lang" <zlang@redhat.com>
> An: "richard" <richard@nod.at>
> CC: "Dave Chinner" <david@fromorbit.com>, "fstests" <fstests@vger.kernel.org>
> Gesendet: Dienstag, 16. Januar 2024 18:10:55
> Betreff: Re: [PATCH] generic/193: Ensure user in expected group

> On Tue, Jan 16, 2024 at 10:19:36AM +0100, Richard Weinberger wrote:
>> Dave,
>> 
>> ----- Ursprüngliche Mail -----
>> > Von: "Dave Chinner" <david@fromorbit.com>
>> > Just do this check in _require_user(). The user needs to be set up
>> > correctly for all tests - if the user and group is not set up
>> > correctly, don't run any of the tests that require that user.
>> > 
>> > This means we don't have to play whack-a-mole with "user has no
>> > group" every time someone assumes that a user is created with a
>> > group by default.
> 
> I think Dave is right. Due to not only the g/193, lots of cases base
> on "fsgqa* user is in fsgqa* group". If you hope to check that by
> the single function _require_user_in_group(), it might need to call
> it in most of cases which use _require_user.
> 
> But yes, as you said, it's weird to do it in _require_user, that makes
> the _require_group calling is a bit redundant.
> 
> If I have to choose one way, I think we can keep the _require_user_in_group
> as an independent function, but call it in _require_user, e.g.
> 
> _require_user()
> {
>        qa_user=fsgqa
>        if [ -n "$1" ];then
>                qa_user=$1
>        fi
>        _require_user_exists $qa_user
> +	# fstests needs each test user is in its own group name, it only need
> +	# a user exist, please use _require_user_exists directly.
> +	_require_user_in_group $qa_user $qa_user
>        echo /bin/true | su $qa_user
>        [ "$?" == "0" ] || _notrun "$qa_user cannot execute commands."
> }
> 
> What do both of you think?

Fine by me. :-)

Thanks,
//richard