git patch file (master)
0001-Ticket-47571-targetattr-ACIs-ignore-subtype.patch

{{{
Description:
Subtypes in targetattr, userattr in aci as well as filter and attribute list
in the search are supported.
If targetattr contains subtypes, the base type only as well as other subtypes
are not allowed to access (or denied to access).
If userattr contains subtypes, the base type as well as other subtypes in
entries do not match the userattr value.
* If attribute list in search has a base type attribute, and a targetattr has
a type with subtypes, then only the subtyped value is returned. E.g.,
attribute list: sn
targetattr: sn;en
==>
sn;en: <sn-en-value> is returned
but
sn or sn;fr is not.
If attribute list has a type with subtype, then if the targetattr allows the
subtype, the value is returned. E.g.,
attribute list: sn;en
targetattr: sn;en
==>
sn;en: <sn-en-value> is returned
but
sn or sn;fr is not.

1) slapd/attr.c
Added another compare type SLAPI_TYPE_CMP_SUBTYPES to comp_cmp which is
called by slapi_attr_type_cmp to support full compare subtypes.
2) plugin/acl.c:
Added a helper function acl__attr_subtype_cmp, which calls slapi_attr_type_
cmp with SLAPI_TYPE_CMP_SUBTYPES if a type in aci contains subtypes.
Some slapi_attr_type_cmp takes SLAPI_TYPE_CMP_SUBTYPES instead of BASE,
which was one of the causes of ignoring subtypes.
3) slapd/search.c,result.c
send_all_attrs/send_specific_attrs use a dontsendattr array to control the
duplicate attribute types. Replaced the logic with a simpler one by creating
an charray with no duplicates.

}}}

Hi Noriko,

It looks good to me. I have just a question regarding slapi_attr_type_cmp and the new case SLAPI_TYPE_CMP_SUBTYPES. It checks that base + options of a1 are all present in b1, then it checks that base+option of b1 are all present in a1. Could it be replaced by strcasecmp(a1,b1) ?

Replying to [comment:5 tbordaz]:

I have just a question regarding slapi_attr_type_cmp and the new case SLAPI_TYPE_CMP_SUBTYPES. It checks that base + options of a1 are all present in b1, then it checks that base+option of b1 are all present in a1. Could it be replaced by strcasecmp(a1,b1) ?

Thanks for your comment, Therry! Yeah, it's redundant, isn't it? Only an issue is we don't "normalize" the subtypes, i.e., it could be "sn;fr;phonetic" or "sn;phonetic;fr". The comparison would fail if it is replaced by strcasecmp...

Thanks Noriko for your answer. I was mislead by the comment saying 'the order must be the same'. Now I have still a doubt that the code would produce comparison success with a1="sn;fr;phonetic" or b2="sn;phonetic;fr". I did a try and the comparison fails at line 197 because to retrieve 'fr' from a1, the loop consumed 'b2'.

Thanks a lot to Thierry for his comments. I revisited the code and revised as he suggested (I believe ;). I'm attaching the patch take 2.

git patch file (master) -- take 2
0001-Ticket-47571-targetattr-ACIs-ignore-subtype.2.patch

{{{
Description:
Subtypes in targetattr, userattr in aci as well as filter and attribute list
in the search are supported.
If targetattr contains subtypes, the base type only as well as other subtypes
are not allowed to access (or denied to access).
If userattr contains subtypes, the base type as well as other subtypes in
entries do not match the userattr value.
* If attribute list in search has a base type attribute, and a targetattr has
a type with subtypes, then only the subtyped value is returned. E.g.,
attribute list: sn
targetattr: sn;en
==>
sn;en: <sn-en-value> and
sn;en;phonetic: <sn-en-phonetic-value> are returned
but
sn or sn;fr is not.
If attribute list has a type with subtype, then if the targetattr allows the
subtype, the value is returned. E.g.,
attribute list: sn;en
targetattr: sn;en
==>
sn;en: <sn-en-value> and
sn;en;phonetic: <sn-en-phonetic-value> are returned
but
sn or sn;fr is not.
1) slapd/attr.c
* slapi_attr_type_cmp assumed the subtype order in 2 args are identical,
but it is not always guaranteed. Removed the assumption.
* Added another compare type SLAPI_TYPE_CMP_SUBTYPES to comp_cmp which is
called by slapi_attr_type_cmp to support full subtypes comparison.
2) plugin/acl.c:
* Changed to call slapi_attr_type_cmp with human readable macros, e.g.,
SLAPI_TYPE_CMP_BASE, SLAPI_TYPE_CMP_SUBTYPE, etc.
* Replaced strcasecmp with slapi_attr_type_cmp for attribute type comparison.
* Changed to call slapi_attr_type_cmp with SLAPI_TYPE_CMP_SUBTYPES (full
subtype comparison) in acl__get_attrEval, where the next attribute to
compare is determined.
3) slapd/search.c,result.c
send_all_attrs/send_specific_attrs use a dontsendattr array to control the
duplicate attribute types. Replaced the logic with a simpler one by creating
an charray with no duplicates.
}}}

Thanks Noriko, yes it works well. Ack.
Just you may skip the 'the order must be the same...' comments in line 209

Replying to [comment:11 tbordaz]:

Thanks Noriko, yes it works well. Ack.
Just you may skip the 'the order must be the same...' comments in line 209

A good catch! Thanks, Thierry! I'm adding this update to the patch...
{{{
diff --git a/ldap/servers/slapd/attr.c b/ldap/servers/slapd/attr.c
index bdbea15..ef3fa10 100644
--- a/ldap/servers/slapd/attr.c
+++ b/ldap/servers/slapd/attr.c
@@ -205,9 +205,8 @@ slapi_attr_type_cmp( const char a1, const char a2, int opt )
b1 = a1;
b2 = a2;
/
- * next, for each component in a1, make sure there is a
- * matching component in a2. the order must be the same,
- * so we can keep looking where we left off each time in a2
+ * next, for each component in a2, make sure there is a
+ * matching component in a1.
/
for (b2 = next_comp(b2); b2; b2 = next_comp(b2)) {
for (b1 = next_comp(b1); b1; b1 = next_comp(b1)) {
}}}

git patch file (master) -- take 3
0001-Ticket-47571-targetattr-ACIs-ignore-subtype.3.patch

Thank you for the reviews and advices, Thierry!

Pushed to master:
31cd7a8..85a7874 master -> master
commit 85a7874

Note: I changed the Review status to "ack" based upon Thierry's comment.

Coverity errors:
12423: Explicit null dereferenced
do_search:
{{{
285 } else if (strcmp(attrs[i], LDAP_ALL_USER_ATTRS / '' /) == 0) {
27. Condition "!charray_inlist(attrs, normaci)", taking true branch
32. Condition "!charray_inlist(attrs, normaci)", taking true branch
CID 12423 (#1 of 1): Explicit null dereferenced (FORWARD_NULL)38. var_deref_model: Passing null pointer "normaci" to function "charray_inlist(char , char )", which dereferences it.[show details]
286 if (!charray_inlist(attrs, normaci)) {
287 charray_add(&attrs, normaci); / consumed /
33. assign_zero: Assigning: "normaci" = "NULL".
288 normaci = NULL;
289 }
}}}

12422: Logically dead code
comp_cmp:
{{{
cond_notnull: Condition "NULL == s1", taking false branch. Now the value of "s1" is not NULL.
104 if (NULL == s1) {
105 if (s2) {
106 return 1;
107 }
108 return 0;
109 }
110 if (NULL == s2) {
notnull: At condition "s1", the value of "s1" cannot be NULL.
dead_error_condition: The condition "s1" must be true.
111 if (s1) {
112 return 1;
113 }
CID 12422 (#1 of 1): Logically dead code (DEADCODE)dead_error_line: Execution cannot reach this statement "return 0;".
114 return 0;
115 }
}}}

Description: commit 85a7874
introduced 2 coverity issues:
12423: Explicit null dereferenced
do_search (slapd/search.c)
If attribute list contains multiple ""s and "aci"s in ldapsearch,
the previous code attempted to add "aci" once for "" and replacing
"aci" with normalized aci (if any) once and eliminating the duplicated
"aci"s. The code contained a null dereferenced bug. Even if duplicated
attributes are included in the attribute list, they are removed later
in send_specific_attrs. Thus, this patch simplifies the logic to avoid
the null dereference.

12422: Logically dead code
comp_cmp (slapd/attr.c)
Eliminated the dead code (case s1 == NULL AND s2 == NULL).

git patch file (master) -- Fixing "Coverity 12422 and 12423"
0001-Ticket-47571-targetattr-ACIs-ignore-subtype.4.patch

Reviewed by Rich (Thank you!!)

Pushed to master:
1a788bf..36c48b8 master -> master
commit 36c48b8

Any chance we can get this in 1.3.2.11 together with a fix for #47653 ?

Replying to [comment:19 simo]:

Any chance we can get this in 1.3.2.11 together with a fix for #47653 ?

Sure. We discussed in the scram and agreed to set 1.3.2.11 to the milestone.

Pushed to 389-ds-base-1.3.2 branch:
22c24f0..270ad9a 389-ds-base-1.3.2 -> 389-ds-base-1.3.2
commit 9d2d939
commit 270ad9a

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/908

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.