With current development model, git master is more or less undefined (more info is under way). Please use either the release-0.2.0 or devel branch when testing git versions. In current situation, all testing of the release candidate in updates-testing is also very helpful.

I didn't test devel, but this bug also affects release-0.2.0.
See: https://bugzilla.redhat.com/show_bug.cgi?id=841104

Hm... could you please try the following and report results?

{{{
$ python

import grp
grp.getgrnam('mock')[2]
}}}

{{{
$ python
Python 2.7.3 (default, Apr 30 2012, 21:18:11)
[GCC 4.7.0 20120416 (Red Hat 4.7.0-2)] on linux2
Type "help", "copyright", "credits" or "license" for more information.

import grp
grp.getgrnam('mock')[2]
996
}}}

I don't know Python, but I tried debugging this a bit.
For me this seems to be the cause of the problem:
{{{
$ python
Python 2.7.3 (default, Apr 30 2012, 21:18:11)
[GCC 4.7.0 20120416 (Red Hat 4.7.0-2)] on linux2
Type "help", "copyright", "credits" or "license" for more information.

import os
os.getgroups()
[1000]
}}}
mock GID is 996 and it's not on the list returned by {{{os.getgroups()}}}.

Full steps to reproduce:
1. install a minimal F17
1. {{{# yum install fedora-review}}}
1. {{{# useradd foo}}}
1. {{{# usermod -a -G mock foo}}}
1. {{{# su foo}}}
1. {{{$ fedora-review -v -b 1}}}

Hm... strange, indeed. I understand the situation as if you run the os.getgroups() test after the "full steps" above you get the same result - the mock group is not in the result list.

We had a similar issue for a specific user while testing, causing similar errors. It boiled down to that he had done a kernel update without rebooting. As a first step: any change if you reboot your box?

Since we have had success stories (+3 karma) my gut feeling is that this is about your setup. Maybe we should try to get help in a python context, referring to the os.getgroups() test case? After all, this is not really about f-r, it's about python not reporting the supplemental groups for the actual user.

I'm basically on holiday, and it might take some time before I'm back. Or not, it depends on the weather situation here by the Polar Circle :)

After rebooting the machine fedora-review started working. It turns out that the user as which fedora-review is ran must be logged to the mock group. This can be done by running "newgrp mock". The manpage states that this command should be ran as root, while it needs to be ran as the user who will be running fedora review. I had two separate SSH sessions. If I logged root in one session, mu ordinary user stayed not logged to the mock group in the other session.

The manpage should be corrected, and also the error message should be made more user-friendly (something like "Please log into mock group" instead of "No mock group").

Also note that mock itself doesn't require logging to the mock group, so I think that fedora-review shouldn't require that either. Print a warning instead of throwing an exception?

fedora-review does not need the user to log as mock, it needs the user to be in the group mock. Only root can add a user to a group, thus the ' usermod -a -G mock [your user name] && newgrp mock' as root.

And yes, mock does require the user to be part of the mock group, if you are not, then it asks for root privileges which is not a good thing to do, so fedora-review prevent you from having to give root privileges by checking if your user is in the mock group.

newgrp doesn't need to be ran as root. Any ordinary user can run it. Logging to a group isn't the same as adding user to a group -- mock requires user to be in the group, while fedora-review requires user to log into the group. (Of course to log into a group you must be a member of that group, but that requirement is stricter than mocks requirement.)

As you can see in the original report I WAS in the mock group. I just wasn't logged into that group. Mock was working fine, but fedora-review failed to start.

I think we should be careful making conclusions from the situation before the reboot, obviously some basic things were broken then. Question is if f-r refuses to start for you '''after''' the reboot.

f-r does not require user to be logged into the mock group:

{{{
$ id
uid=1010(leamas) gid=1010(leamas) groups=1010(leamas),10(wheel),481(mock)
[leamas@muminmamman FedoraReview]$ ./try-fedora-review -b 1
Processing bugzilla bug: 1
Bugzilla v0.6.2 initializing
Chose subclass RHBugzilla v0.3
Trying bugzilla cookies for authentication
[cut]
}}}

I would have been helpful to see what was "id" output was before trying to run f-r. Because just because you are in mock group inside /etc/groups does not mean you are effectively in mock group for current session (i.e. you could have been added to group after you were already logged in).

This feeling seems to be enhanced when I see os.getgroups() not returning mock group. I tested this on my rawhide and F17 machines and could not reproduce. Each time I logged in I was already part of mock group. So I really believe this must be in your setup

There's seems to be some confusion, so I'll explain step by step.

I create a new user kojan. Log in as this user. {{{id}}} shows:
{{{
$ id
uid=1000(kojan) gid=1000(kojan) groups=1000(kojan) context=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
}}}
I am not (yet) member of mock group, so both mock and fedora-review fail.

Now {{{root}}} adds {{{kojan}}} to {{{mock group}}}:
{{{

usermod -a -G mock kojan

}}}
mock starts working stright-away, without the need to logout or reboot the machine:
{{{
$ mock shell :
INFO: mock.py version 1.1.22 starting...
State Changed: init plugins
INFO: selinux enabled
State Changed: start
State Changed: lock buildroot
State Changed: shell
State Changed: unlock buildroot
$ echo $?
0
}}}
however fedora-review still fails:
{{{
$ fedora-review -v -b 1
Exception down the road...
Traceback (most recent call last):
File "/usr/lib/python2.7/site-packages/FedoraReview/review_helper.py", line 115, in run
Settings.init()
File "/usr/lib/python2.7/site-packages/FedoraReview/settings.py", line 188, in init
_check_mock_grp()
File "/usr/lib/python2.7/site-packages/FedoraReview/settings.py", line 105, in _check_mock_grp
raise ConfigError('No mock group - mock not installed?')
ConfigError: 'Configuration error: No mock group - mock not installed?'
Exception down the road...
}}}
{{{kojan}}} is a member of {{{mock}}} group:
{{{
$ grep mock /etc/group
mock:x:997:kojan
}}}
but {{{kojan}}} is not logged into {{{mock}}} group:
{{{
$ id
uid=1000(kojan) gid=1000(kojan) groups=1000(kojan) context=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
}}}
If I login to the mock group by calling:
{{{
$ newgrp mock
$ id
uid=1000(kojan) gid=997(mock) grupy=1000(kojan),997(mock) kontekst=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
}}}
fedora review will start working:
{{{
$ fedora-review -v -b 1
Processing bugzilla bug: 1
Bugzilla v0.7.0 initializing
[...]
}}}

So as you can see, fedora-review has stricter requirement on mock group presence than mock itself.

I believe that the check for mock group should be removed from fedora-review, or at least made non-fatal. Mock can take care about its group. Why would fedora-review depend on implementation details of mock? Is there any good reason, other than warn users about possible future problems with mock? If that's the only reason then I believe a simple warning would suffice instead of the fatal exception.

Basically, this is how supplemental groups works. They are only read when a user logs in i. e., if you add a group to a user it must logout and login (possibly using newgrp) to see the change.

It's evident from your own logs. If you look at the output from the id command under "kojan is not logged in into the mock group" you can also see that the group list does not contain the mock group. This means the although root has executed the usermod -a -G mock kojan, this is not effective.

One way to handle this is using newgrp, which actually starts in a new login shell. Another is to just logout and login user kojan again. A third is to reboot the machine. The net result is the same: the mock group is present in the supplemental groups list in e .g the 'id' output, and f-r works.

Stated otherwise: unless you can show that f-r fails although the mock group is part of the user's supplemental groups, this bug is invalid. In this context, the content of the /etc/groups is irrelevant, what counts is e. g. output from id or os.getgroups() in python.

Sorry if my wording is a bit harsh, just want to be clear. I do appreciate your work.

Replying to [comment:13 leamas]:

unless you can show that f-r fails although the mock group is part of the user's supplemental groups, this bug is invalid.
I can't show that.

I am just saying that this requirement is:
unnecessary (fedora-review would work without it),
causing problems (at least to one user -- me), and
* the change is trivial (removing one or two lines from one source file),
and I request for improvement.

If you think my request is invalid, feel free to reject it, but I would appreciate if you at least told me why you are rejecting it (i.e. why fedora-review requires the caller to be logged to mock group even if it would work without this condition being met). Thank you.

The test was added because of several hard to find errors when a user tried to run
mock without having mock as a supplemental group. So, this test has a function. Mock
will not run if the test fails, no way. (feel free to prove I'm wrong here :) )

Well, he actually did prove it, on comment #13, mock runs but not fedora-review.

Replying to [comment:15 leamas]:

The test was added because of several hard to find errors when a user tried to run
mock without having mock as a supplemental group. So, this test has a function. Mock
will not run if the test fails, no way. (feel free to prove I'm wrong here :) )

Well, he actually did prove it, on comment #13, mock runs but not fedora-review.
Yes I did, in comment #12.

Now, first of all, the bugzilla habit of creating comment links using that pattern obviously doesn't work. Not for me, or anyone else :)

Secondly, mxdebak is right in one way (and thus I'm wrong): mock does seemingly not use the effective list of supplemental gids, but rather looks into /etc/groups (or whatever source is used for that). So, in some situations mock would indeed run although the test fails.

OTOH, from my perspective, this is a minor issue. It will not affect anyone following the f-r manpage or README. The simple fix to remove the test would cause more harm for innocent users running into strange mock errors. A more elaborated test is possible, but needs to access data in the proper ways, taking nsswitch.conf into account.

I suggest that this bug is accepted, in the form "mock group test fails gives false negatives" in some situations. Basically, this is a corner case. The proper fix would be to access the group database as described above.

I am actually wondering if it is a wrong behaviour in python itself or if it is the desired behaviour and we are indeed facing some sort of corner case.

Python (and the ''id'' command) just reflects the kernel getgroups() call. Unless we are heading for kernel patches (kidding!), the only way around is to create a new shell after adding the group.

From what's happening, I guess mock is using getgrent(3). That will actually dig into the passwd database and allow mock to determine what groups a new process will get.

There are several ways to create a new login shell:
- newgrp, without arguments,
- su $USER
- logout/login
- etc.

If we think this problem is big enough to require a solution (I don't) one could wrap f-r in a newgrp or su $USER command using a shellscript. Basically, this is a hack.

Another way would be to access getgrent() from python. I don't know how to do this (besides wring a python C module), there must be code for that out there. This would be the Right Thing, although not on my priority list.

Well we know mock would not work if he was not a member of that group. So most probably what mock does is call newgrp in one way or the other (perhaps duplicating its functionality?). So we could replace our check with simple grepping of /etc/group but I don't like it very much.

Thing is...f-r is most likely to be used by developers who have been using mock for some time already and so they will have mock supplemental group already there. mizdebsk's use case fails (presumably) because he installed on a fresh VM. So what I propose...keep the current behaviour, just improve the error message.

No, we can't grep in /etc/group. The actual group database is governed by /etc/nsswitch.conf and could be e. g., in NIS and/or LDAP. getgrent(3) is the only reasonable way.

Mock certainly starts a new, worker shell using setgid() which uses the updated group list. It needs to anyway, to get proper permissions.

Keep the current behaviour, improve error message: +1

Fixed (improved error message) in 4d3a394

Thanks for reporting this issue. Please feel free to reopen bug if you think the solution is unsatisfactory, or perhaps for other reasons.