On Tue, Apr 21, 2015 at 12:42:52PM +0200, Pavel Reichl wrote:
Hello,
Kaushik found a problem while verifying
https://bugzilla.redhat.com/show_bug.cgi?id=1170910 .
The root of the problem was that we were denying access for user of which at
least one group failed to be resolved. This is too strict and can be relaxed
to denying access only if the simple_deny_groups is not empty at the same
time.
Please see attached patch.
Thanks!
From 0f1ee428e87012323c1446c3d63fe59d12331c01 Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)redhat.com>
Date: Mon, 20 Apr 2015 11:33:29 -0400
Subject: [PATCH] simple-access-provider: make user grp res more robust
Not all user groups need to be resolved if group deny list is empty.
Resolves:
https://fedorahosted.org/sssd/ticket/2519
---
src/providers/simple/simple_access_check.c | 34 +++++++++++++++++++++++-------
1 file changed, 26 insertions(+), 8 deletions(-)
diff --git a/src/providers/simple/simple_access_check.c
b/src/providers/simple/simple_access_check.c
index c8217f6d4ef2560931d3151276085eb2a6028be5..f06bfa48ef09e4a5075f8f7360476beae387de00
100644
--- a/src/providers/simple/simple_access_check.c
+++ b/src/providers/simple/simple_access_check.c
@@ -395,6 +395,8 @@ struct simple_check_groups_state {
const char **group_names;
size_t num_names;
+
+ int failed_to_resolve_groups;
Why do we have an int here but both return bool and never use the int as
an int?
};
static void simple_check_get_groups_next(struct tevent_req *subreq);
@@ -430,6 +432,7 @@ simple_check_get_groups_send(TALLOC_CTX *mem_ctx,
state->ev = ev;
state->ctx = ctx;
+ state->failed_to_resolve_groups = 0;
DEBUG(SSSDBG_TRACE_LIBS, "Looking up groups for user %s\n", username);
@@ -548,14 +551,13 @@ static void simple_check_get_groups_next(struct tevent_req
*subreq)
DEBUG(SSSDBG_OP_FAILURE,
"Could not resolve name of group with GID
%"SPRIgid"\n",
state->lookup_groups[state->giter].gid);
- tevent_req_error(req, ret);
- return;
+ state->failed_to_resolve_groups++;
+ } else {
+ state->num_names++;
+ state->giter++;
I wonder if we could keep giter++ outside the if-else block, because..
}
- state->num_names++;
- state->giter++;
-
- if (state->giter < state->num_groups) {
+ if (state->giter + state->failed_to_resolve_groups < state->num_groups)
{
..then we wouldn't have to change the condition here and would be able
to keep it simpler.
The rest looks good to me.