On 09/11/2014 11:43 AM, Jakub Hrozek wrote:
On Wed, Sep 10, 2014 at 09:59:35PM +0200, Michal Židek wrote:
> On 09/10/2014 06:13 PM, Michal Židek wrote:
>> On 09/10/2014 06:00 PM, Michal Židek wrote:
>>> On 09/10/2014 02:49 PM, Jakub Hrozek wrote:
>>>> On Wed, Sep 10, 2014 at 01:23:15PM +0200, Michal Židek wrote:
>>>>> --- a/src/providers/ldap/sdap.c
>>>>> +++ b/src/providers/ldap/sdap.c
>>>>> @@ -348,9 +348,12 @@ int sdap_parse_entry(TALLOC_CTX *memctx,
>>>>> }
>>>>>
>>>>> for (i = 0; vals[i]; i++) {
>>>>> - /* the objectclass is always the first name in the map
*/
>>>>> - if (strncasecmp(map[0].name,
>>>>> - vals[i]->bv_val, vals[i]->bv_len)
== 0) {
>>>>> + /* The objectclass is always the first name in the map.
>>>>> + * Alternative objectclass for groups is second. */
>>>>> + if (strncasecmp(map[0].name, vals[i]->bv_val,
>>>>> vals[i]->bv_len) == 0
>>>>> + || (map[1].name != NULL
>>>>> + && strncasecmp(map[1].name,
>>>>> + vals[i]->bv_val,
vals[i]->bv_len)
>>>>> == 0)) {
>>>>> /* ok it's an entry of the right type */
>>>>> break;
>>>>> }
>>>>> @@ -630,7 +633,9 @@ errno_t sdap_parse_deref(TALLOC_CTX *mem_ctx,
>>>>>
>>>>> for (i=0; ocs[i]; i++) {
>>>>> /* the objectclass is always the first name in the map
*/
>>>>> - if (strcasecmp(minfo[mi].map[0].name, ocs[i]) == 0) {
>>>>> + if (strcasecmp(minfo[mi].map[0].name, ocs[i]) == 0
>>>>> + || (minfo[mi].map[1].name != NULL
>>>>> + && strcasecmp(minfo[mi].map[1].name,
ocs[i]) ==
>>>>> 0)) {
>>>>> DEBUG(SSSDBG_TRACE_ALL,
>>>>> "Found map for objectclass
'%s'\n", ocs[i]);
>>>>> map = minfo[mi].map;
>>>>
>>>> It would also be nice to split these two places into a separate
>>>> function. That would make migrating to a list of objectclasses easier
>>>> later.
>>>
>>> Ok. I included these two changes:
>>> 1. add function to generate ORed list of objectclasses, that can be
>>> used in filter.
>>>
>>> 2. add small static function to match objectclass.
>>>
>>> I do not want to include more changes. It will be better to file
>>> tickets for the enhancements that will come out of the discussion
>>> on the list. The two added function probably be changed anyway.
>>>
>>> See the attached patches.
>>
>> Or better don't :) . I forgot to amend the first patch. Will send
>> new version soon.
>>
>
> Sorry took a little longer. Here are the patches.
>
> Michal
>
> From 58b3750bbbcda0dafe0c73b8eae69264d1eb2c7e Mon Sep 17 00:00:00 2001
> From: Michal Zidek <mzidek(a)redhat.com>
> Date: Wed, 10 Sep 2014 12:41:16 +0200
> Subject: [PATCH 1/2] Add alternative objectClass to group attribute maps
ACK
I just changed the commit message in this patch (thank you Pavel for
noticing).
> From 70c8c5ea07bceab536197c2d8424161988b156e0 Mon Sep 17
00:00:00 2001
> From: Michal Zidek <mzidek(a)redhat.com>
> Date: Wed, 10 Sep 2014 12:56:54 +0200
> Subject: [PATCH 2/2] Use the alternative objectclass in group maps.
Code looks mostly good to me. I haven't ran any tests yet, see one
question inline.
>
> Use the alternative group objectclass in queries.
>
> Fixes:
>
https://fedorahosted.org/sssd/ticket/2436
> ---
> src/providers/ldap/ldap_id.c | 18 ++++++----
> src/providers/ldap/sdap.c | 41 +++++++++++++++++++---
> src/providers/ldap/sdap.h | 2 ++
> src/providers/ldap/sdap_async_enum.c | 12 +++++--
> src/providers/ldap/sdap_async_initgroups.c | 50 +++++++++++++++++++++------
> src/providers/ldap/sdap_async_initgroups_ad.c | 13 +++++--
> src/providers/ldap/sdap_async_nested_groups.c | 11 ++++--
> 7 files changed, 117 insertions(+), 30 deletions(-)
>
[...]
> /* =Parse-msg============================================================= */
> -
> +static bool objectclass_matched(struct sdap_attr_map *map,
> + const char *objcl, int len);
Please put a newline here.
Added.
> int sdap_parse_entry(TALLOC_CTX *memctx,
> struct sdap_handle *sh, struct sdap_msg *sm,
> struct sdap_attr_map *map, int attrs_num,
> @@ -348,9 +349,7 @@ int sdap_parse_entry(TALLOC_CTX *memctx,
> }
>
> for (i = 0; vals[i]; i++) {
> - /* the objectclass is always the first name in the map */
> - if (strncasecmp(map[0].name,
> - vals[i]->bv_val, vals[i]->bv_len) == 0) {
> + if (objectclass_matched(map, vals[i]->bv_val, vals[i]->bv_len)) {
> /* ok it's an entry of the right type */
> break;
> }
> @@ -530,6 +529,25 @@ done:
> return ret;
> }
>
> +static bool objectclass_matched(struct sdap_attr_map *map,
> + const char *objcl, int len)
> +{
> + if (len == 0) {
> + len = strlen(objcl) + 1;
> + }
When is len 0 ?
The function is there to replace long "if" conditions. There
were 2 in the code. One used strncasecmp and one strcasecmp.
^
Where no length is needed to be specified, you can pass 0
to this function and the len parameter will be calculated
automatically based on the length of objectclass.
> +
> + if (strncasecmp(map[SDAP_OC_GROUP].name, objcl, len) == 0) {
> + return true;
> + }
> +
> + if (map[SDAP_OC_GROUP_ALT].name != NULL
> + && strncasecmp(map[SDAP_OC_GROUP_ALT].name, objcl, len) == 0) {
> + return true;
> + }
> +
> + return false;
> +}
> +