On 04/27/2015 10:59 AM, Jakub Hrozek wrote:
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@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?
I used it as int here:
if (state->giter + state->failed_to_resolve_groups < state->num_groups)
but I don't any more.
};
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.
Sure, please see updated patch.
The rest looks good to me. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel