Med: daemons: Fix user/group checking in based.#4055
Med: daemons: Fix user/group checking in based.#4055nrwahl2 merged 1 commit intoClusterLabs:mainfrom
Conversation
| /* If that didn't work, we need to check if CRM_DAEMON_GROUP is a secondary | ||
| * group for the user - that is, are they listed in /etc/group for that | ||
| * group? | ||
| */ |
There was a problem hiding this comment.
Code looks good, thanks!
One thing I'm on the fence about comment-wise, is whether to state explicitly that group->gr_mem includes only the users listed in /etc/group -- not necessarily all members of the group. To make it super clear why this isn't enough on its own. Some of the other comments could probably be trimmed down or removed if you wanted.
On the other hand, you explained it well in the commit message. I made sure to check the 14d9ae4 commit message before bcc7c90, but I didn't realize that this loop wouldn't catch a primary group match.
Up to you. Feel free to merge this as-is. I appreciate you taking care of this. I hadn't started on it yet.
There was a problem hiding this comment.
Updated and shortened comments. It's always kind of a struggle to know how much to add to inline comments vs. in the commit message. Sometimes commit messages don't make a lot of sense without the context of what comes before and after, unless you get really verbose and redundant. On the other hand, I'd love to avoid the "what exactly was I thinking five years ago" situation that comes up so often.
There was a problem hiding this comment.
I totally agree. I just try to avoid comments stating WHAT fairly obvious code is doing, unless it's a long or complicated function where those comments serve more as visual delimiters than as explanations.
This fixes a bug introduced by bcc7c90. The group members returned by getgrnam will only include those users that are listed in /etc/group for that group. It won't include any users for which a group is their primary. In other words, if this is what's in /etc/passwd: hacluster:x:189:189:cluster user:/var/lib/pacemaker:/sbin/nologin And this is what's in /etc/group: haclient:x:189: Then getgrnam will not list hacluster as a member of the haclient group and is_daemon_group_member will return false. We need to re-introduce the primary group check to fix this.
clumens
force-pushed
the
fix-group-check
branch
from
cf60daf to
07db76e
Compare
2 participants
This fixes a bug introduced by bcc7c90. The group members returned by getgrnam will only include those users that are listed in /etc/group for that group. It won't include any users for which a group is their primary.
In other words, if this is what's in /etc/passwd:
And this is what's in /etc/group:
Then getgrnam will not list hacluster as a member of the haclient group and is_daemon_group_member will return false. We need to re-introduce the primary group check to fix this.