On Tue, Sep 17, 2013 at 01:49:30PM +0200, Pavel Březina wrote:
On 09/16/2013 05:35 PM, Jakub Hrozek wrote:
>On Mon, Sep 16, 2013 at 05:30:38PM +0200, Jakub Hrozek wrote:
>>On Thu, Sep 12, 2013 at 02:01:43PM +0200, Pavel Březina wrote:
>>>On 09/10/2013 03:46 PM, Jakub Hrozek wrote:
>>>>On Tue, Sep 03, 2013 at 12:42:21PM +0200, Pavel Březina wrote:
>>>>>Patch 0001: I haven't used this one after all, but I still think
it
>>>>>is nice to have. It reduces amount of code duplication.
>>>>>
>>>>>Patch 0005:
>>>>>>+ /* Try to get group SID and assign it a domain */ +
>>>>>>group_sid = ldb_msg_find_attr_as_string(group, SYSDB_SID_STR,
>>>>>>NULL); + if (group_sid == NULL) {
>>>>>
>>>>>I'm not sure if this branch isn't a dead code, but I kept it
there
>>>>>because it mimics current behaviour.
>>>>>
>>>>>>+ /* We will look it up in main domain. */ +
domain
>>>>>>= state->ctx->domain; + } else { + domain =
>>>>>>find_subdomain_by_sid(state->ctx->domain, group_sid); +
if
>>>>>>(domain == NULL) { + DEBUG(SSSDBG_CRIT_FAILURE,
>>>>>>("There is no domain information for " +
>>>>>>"SID %s\n", group_sid)); + return ENOENT; +
} +
>>>>>>}
>>>>
>>>>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date:
>>>>>Mon, 2 Sep 2013 13:36:25 +0200 Subject: [PATCH 1/6] util: add
>>>>>sss_idmap_talloc[_free]
>>>>>
>>>>>Remove code duplication.
>>>>
>>>>Removing code duplication added code :-) 76 insertions(+), 62
>>>>deletions(-)
>>>>
>>>>But it's mostly the comment blobs, so ack.
>>>>
>>>>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date:
>>>>>Tue, 27 Aug 2013 14:53:03 +0200 Subject: [PATCH 2/6] simple access
>>>>>tests: fix typos
>>>>
>>>>ACK
>>>>
>>>>>From 5648c71b1a5a6bad5ce8e3361c7b4baf7d972c0b Mon Sep 17 00:00:00
>>>>>2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?=
<pbrezina(a)redhat.com>
>>>>>Date: Tue, 27 Aug 2013 14:02:42 +0200 Subject: [PATCH 3/6] simple
>>>>>provider: support subdomain users
>>>>>
>>>>>Resolves:
https://fedorahosted.org/sssd/ticket/2034
>>>>
>>>>There is one thing that still needs fixing -- the case sensitivity
>>>>flag is still read from the master domain through be_ctx->domain. In
>>>>general, the providers should not access be_ctx->domain directly, but
>>>>rather the handler should assign a sss_domain_info structure to some
>>>>per-provider or per-request state and the code should operate on
>>>>that.
>>>>
>>>>>From c1b31c3c2eefa71f5c17539fd615a785d7e1ccd0 Mon Sep 17 00:00:00
>>>>>2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?=
<pbrezina(a)redhat.com>
>>>>>Date: Mon, 2 Sep 2013 15:15:59 +0200 Subject: [PATCH 4/6] util: add
>>>>>find_subdomain_by_sid()
>>>>
>>>>ACK, but please file a ticket to mass-namespace the functions in
>>>>this module.
>>>
>>>https://fedorahosted.org/sssd/ticket/2086
>>>
>>>>>From 78fff868aa9f1b5a8392328973e4ce3ba000a9e6 Mon Sep 17 00:00:00
>>>>>2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?=
<pbrezina(a)redhat.com>
>>>>>Date: Wed, 28 Aug 2013 11:35:27 +0200 Subject: [PATCH 5/6] simple
>>>>>provider: support subdomain groups
>>>>>
>>>>
>>>>Same comment about case sensitivity.
>>>>
>>>>>From 844b9cc30ad694116772f519791b3725a72d178a Mon Sep 17 00:00:00
>>>>>2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?=
<pbrezina(a)redhat.com>
>>>>>Date: Tue, 3 Sep 2013 12:34:07 +0200 Subject: [PATCH 6/6] simple
>>>>>access test: initialize be_ctx for all tests
>>>>>
>>>>
>>>>ACK
>>>
>>>Thanks. New patches are attached. I'm sending the case sensitivity
>>>change in a separate patch, since I think it can stand on its own as
>>>improvement of subdomains support in simple access provider.
>>>
>>>I also included patch that contains find_subdomain_by_object_name().
>>>
>>
>>Hi,
>>
>>as part of review, I resolved conflict between your and Lukas' patches.
>>Can you check if it's OK?
>
>No it's not :-)
>
>I'll prepare a local version for my review, but it would be nice if you
>resent the whole set.
Rebased patches are attached.
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Mon, 2 Sep 2013 13:36:25 +0200
Subject: [PATCH 1/8] util: add sss_idmap_talloc[_free]
ID mapping still works in the AD provider. I haven't tested the other
providers, but make check also passes and we'd know if there were
unresolved symbol.
ACK
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Tue, 27 Aug 2013 14:53:03 +0200
Subject: [PATCH 2/8] simple access tests: fix typos
ACK
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Tue, 27 Aug 2013 14:02:42 +0200
Subject: [PATCH 3/8] simple provider: support subdomain users
ACK. I tested both positive and negative tests with trusted AD and
trusted native AD provider users. The patch looks good as well.
Date: Mon, 2 Sep 2013 15:15:59 +0200
Subject: [PATCH 4/8] util: add find_subdomain_by_sid()
This function takes domain SID (doesn't have the last component)
or object SID (have all components) and returns subdomain.
The subdomain is found by comparing domain->domainid with the SID.
E.g.
domain SID: S-1-5-21-3940105347-3434501867-2690409756
object SID: S-1-5-21-3940105347-3434501867-2690409756-513
Resolves:
https://fedorahosted.org/sssd/ticket/2034
ACK. Tested as part of the other patches. The code looks good to me.
From 4ed75ba1e6ab17968f56c919111389a77bde8ed0 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Thu, 12 Sep 2013 12:55:44 +0200
Subject: [PATCH 5/8] util: add find_subdomain_by_object_name()
This function will parse object name into name and domain
name part and return appropriate sss domain.
Resolves:
https://fedorahosted.org/sssd/ticket/2034
ACK. If we ever support non-FQDN names for subdomain object, as some
users request, we would have to hunt for cases like the one in this
patch, where no domain gets translated into the parent domain. But I
think that's OK.
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Wed, 28 Aug 2013 11:35:27 +0200
Subject: [PATCH 6/8] simple provider: support subdomain groups
Resolves:
https://fedorahosted.org/sssd/ticket/2034
---
The domain structure from ctx is now only used as anchor for looping
over domains starting from the primary domain. Both positive and
negative tests with group names pass.
ACK
From 1178e290b45a889e3609ff085891e525eceadd2d Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Tue, 3 Sep 2013 12:34:07 +0200
Subject: [PATCH 7/8] simple access test: initialize be_ctx for all tests
Recent simple access provider patches started using
be_ctx during access check. This caused segfault in
unit tests, since be_ctx wasn't initialized.
Resolves:
https://fedorahosted.org/sssd/ticket/2034
---
ACK. The tests passes and the code looks OK to me.
From 11cf0a7dc49c3074861babfd0c825a7d56710ff6 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Thu, 12 Sep 2013 13:15:47 +0200
Subject: [PATCH 8/8] simple provider: obey case sensitivity for subdomain
users and groups
When comparing username and his groups to access list, we will
obey case sensitivity of object from access list.
Resolves:
https://fedorahosted.org/sssd/ticket/2034
ACK. I tested with IPA native users and AD trusted users. With Admin in
the allow ACL, admin can't login, but with Tuser in the allow ACL, tuser
from AD could.