We need to allow ACI creation for non-existent targets, as it is a valid use case when you want to prevent addition of a specific target. It is also possible that someone would want to create their ACIs before adding the target entries.

If we do anything here, it should be to simply log and return a warning to the client but still accept the ACI.

Replying to [comment:3 nkinder]:

If we do anything here, it should be to simply log and return a warning to the client but still accept the ACI.

Anupam, please follow above Nathan's comment.
When adding/modifying an ACI, search the target and if it does not exist, file a warning.

non-existent target checking
0001-Ticket-626-Possible-to-add-nonexistent-target-to-ACI.patch

This patch does not validate wildcards and macros in the ACI target

Comparing DN values is tricky with the normalization, processing escape values, multi-valued RDNs, etc. In order to handle comparisons, convert DN values to Slapi_DN.

acl_parse()
{{{
Slapi_DN *targdn = slapi_sdn_new();
slapi_filter_get_ava ( f, &avaType, &avaValue );
slapi_sdn_init_dn_byref(targdn, avaValue->bv_val);

if (!slapi_sdn_get_dn(targdn)) {
    /* not a valid DN */
    slapi_sdn_free(&targdn);
    return ACL_INVALID_TARGET;
}

if (!slapi_sdn_issuffix(targdn, aci_item->aci_sdn)) {
    slapi_sdn_free(&targdn);
return ACL_INVALID_TARGET;
}

if (0 == slapi_sdn_compare(targdn, aci_item->aci_sdn)) {
    int target_check = 0;
    if (pb &&
        !slapi_pblock_get(pb, SLAPI_ACI_TARGET_CHECK, &target_check) &&
        (target_check != 1)) {
        /* Make sure that the target exists */
        int rc = 0;
        Slapi_PBlock *temppb = slapi_pblock_new();
        slapi_search_internal_set_pb_ext(temppb, targdn,
            LDAP_SCOPE_BASE, "(objectclass=*)", NULL, 1, NULL, NULL,
            (void *)plugin_get_default_component_id(), 0);
        slapi_search_internal_pb(temppb);
        slapi_pblock_get(temppb, SLAPI_PLUGIN_INTOP_RESULT, &rc);
        if (rc != LDAP_SUCCESS) {
            slapi_log_error(SLAPI_LOG_FATAL, plugin_name,
                            "The ACL target %s does not exist\n", slapi_sdn_get_dn(targdn));
            rc = ACL_INVALID_TARGET;
        }
        slapi_free_search_results_internal(temppb);
    slapi_pblock_destroy(temppb);
        target_check = 1;
        slapi_pblock_set(pb, SLAPI_ACI_TARGET_CHECK, &target_check);
    }
}
slapi_sdn_free(&targdn);
if (rc) {
    return rc;
}

Replying to [comment:8 rmeggins]:

Comparing DN values is tricky with the normalization, processing escape values, multi-valued RDNs, etc. In order to handle comparisons, convert DN values to Slapi_DN.

acl_parse()
{{{
Slapi_DN *targdn = slapi_sdn_new();
slapi_filter_get_ava ( f, &avaType, &avaValue );
slapi_sdn_init_dn_byref(targdn, avaValue->bv_val);

if (!slapi_sdn_get_dn(targdn)) {
    /* not a valid DN */
    slapi_sdn_free(&targdn);
    return ACL_INVALID_TARGET;
}

if (!slapi_sdn_issuffix(targdn, aci_item->aci_sdn)) {
    slapi_sdn_free(&targdn);

return ACL_INVALID_TARGET;
}

if (0 == slapi_sdn_compare(targdn, aci_item->aci_sdn)) {
    int target_check = 0;
    if (pb &&
        !slapi_pblock_get(pb, SLAPI_ACI_TARGET_CHECK, &target_check) &&
        (target_check != 1)) {
        /* Make sure that the target exists */
        int rc = 0;
        Slapi_PBlock *temppb = slapi_pblock_new();
        slapi_search_internal_set_pb_ext(temppb, targdn,
            LDAP_SCOPE_BASE, "(objectclass=*)", NULL, 1, NULL, NULL,
            (void *)plugin_get_default_component_id(), 0);
        slapi_search_internal_pb(temppb);
        slapi_pblock_get(temppb, SLAPI_PLUGIN_INTOP_RESULT, &rc);
        if (rc != LDAP_SUCCESS) {
            slapi_log_error(SLAPI_LOG_FATAL, plugin_name,
                            "The ACL target %s does not exist\n", slapi_sdn_get_dn(targdn));
            rc = ACL_INVALID_TARGET;
        }
        slapi_free_search_results_internal(temppb);
  slapi_pblock_destroy(temppb);
        target_check = 1;
        slapi_pblock_set(pb, SLAPI_ACI_TARGET_CHECK, &target_check);
    }
}
slapi_sdn_free(&targdn);
if (rc) {
    return rc;
}

Thanks for the review Rich. I have a couple of questions though

1. As per Noriko's comment on this ticket, we should allow the creation of ACI with non existent target and only log a warning in the server logs. However, you have suggested returning ACI_INVALID_TARGET in case of non-existent targets which won't allow the creation of ACI. Is this the expected behaviour?

2. My code structure was something like -
   if (pb) {
   slapi_pblock_get(pb, SLAPI_ACI_TARGET_CHECK, &target_check);
   }
   if (target_check != 1) {
   .
   .

}
if (pb) {
target_check = 1;
slapi_pblock_set(pb, SLAPI_ACI_TARGET_CHECK, &target_check);
}

I used this structure intentionally so that the target check is performed even when pb is NULL which happens at the time the directory server is started
If I use something like
if (pb &&
!slapi_pblock_get(pb, SLAPI_ACI_TARGET_CHECK, &target_check) &&
(target_check != 1)) {

}
it won't be able to perform the target check at the time the server is started.

Please correct me if I am wrong.

Replying to [comment:10 anjain]:

Replying to [comment:8 rmeggins]:

Comparing DN values is tricky with the normalization, processing escape values, multi-valued RDNs, etc. In order to handle comparisons, convert DN values to Slapi_DN.

<snip>

Thanks for the review Rich. I have a couple of questions though

1. As per Noriko's comment on this ticket, we should allow the creation of ACI with non existent target and only log a warning in the server logs. However, you have suggested returning ACI_INVALID_TARGET in case of non-existent targets which won't allow the creation of ACI. Is this the expected behaviour?

No. In the case where the target does not exist, we should log a warning, but allow the creation of the ACI. So you had it correct in your original code.

1. My code structure was something like -
   if (pb) {
   slapi_pblock_get(pb, SLAPI_ACI_TARGET_CHECK, &target_check);
   }
   if (target_check != 1) {
   .
   .

}
if (pb) {
target_check = 1;
slapi_pblock_set(pb, SLAPI_ACI_TARGET_CHECK, &target_check);
}

I used this structure intentionally so that the target check is performed even when pb is NULL which happens at the time the directory server is started
If I use something like
if (pb &&
!slapi_pblock_get(pb, SLAPI_ACI_TARGET_CHECK, &target_check) &&
(target_check != 1)) {

}
it won't be able to perform the target check at the time the server is started.

Please correct me if I am wrong.

No, you are correct, I misunderstood.

non-existent target checking updated patch
0001-Ticket-626-Possible-to-add-nonexistent-target-to-ACI.2.patch

Ticket #47459 has been created to validate the target when they contain wildcards and macros

If the target DN can contain wildcards now, then they will fail DN syntax checking. So you must change this code:
{{{

159                         if (!slapi_sdn_get_dn(targdn)) { 
160                                 /* not a valid DN */ 
161                                 slapi_sdn_free(&targdn); 
162                                 return ACL_INVALID_TARGET; 
163                         }

}}}

In this case, you should not return ACL_INVALID_TARGET, since we must allow strings that are not valid DNs.

Replying to [comment:14 rmeggins]:

If the target DN can contain wildcards now, then they will fail DN syntax checking. So you must change this code:
{{{

159 if (!slapi_sdn_get_dn(targdn)) {
160 / not a valid DN /
161 slapi_sdn_free(&targdn);
162 return ACL_INVALID_TARGET;
163 }
}}}

In this case, you should not return ACL_INVALID_TARGET, since we must allow strings that are not valid DNs.

Rich, The code is something like

if (aci_item->aci_type & ACI_TARGET_DN) {
.
.
if (!slapi_sdn_get_dn(targdn)) {
/ not a valid DN /
slapi_sdn_free(&targdn);
return ACL_INVALID_TARGET;
}
.
.
}
If the target contains wildcards then the condition "if (aci_item->aci_type & ACI_TARGET_DN)" won't be satisfied. So, I believe this is fine.

Replying to [comment:15 anjain]:

Replying to [comment:14 rmeggins]:

If the target DN can contain wildcards now, then they will fail DN syntax checking. So you must change this code:
{{{

159                         if (!slapi_sdn_get_dn(targdn)) { 
160                                 /* not a valid DN */ 
161                                 slapi_sdn_free(&targdn); 
162                                 return ACL_INVALID_TARGET; 
163                         }

}}}

In this case, you should not return ACL_INVALID_TARGET, since we must allow strings that are not valid DNs.

Rich, The code is something like

if (aci_item->aci_type & ACI_TARGET_DN) {
.
.
if (!slapi_sdn_get_dn(targdn)) {
/ not a valid DN /
slapi_sdn_free(&targdn);
return ACL_INVALID_TARGET;
}
.
.
}
If the target contains wildcards then the condition "if (aci_item->aci_type & ACI_TARGET_DN)" won't be satisfied. So, I believe this is fine.

Ah, ok. Then if at this point in the code, it really is supposed to be a valid DN string, then checking for it is a good thing.

non-existent target checking updated patch2
0001-Ticket-626-Possible-to-add-nonexistent-target-to-ACI.3.patch

ack

Pushed to master on behalf of anjain:
a4daf1a..44ebe22 master -> master
commit 44ebe22
Author: Anupam Jain anjain@localhost.localdomain
Date: Thu Aug 1 15:13:32 2013 -0700

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/626

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.