[PATCH] Improve Samba4's LDB consistancy

[Stefan (metze) Metzmacher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Andrew,

> I've modified the objectclass module to improve consistency in Samba4.
> 
> The aim here is to ensure that if we have
> 
> CN=Users,DC=samba,DC=example,DC=com
> 
> that we cannot have a DN of the form
> 
> cn=admin ,cn=useRS,DC=samba,DC=example,DC=com
> 
> This module pulls apart the DN, fixes up the relative DN part, and
> searches for the parent to copy the base from.
> 
> I've used the objectclass module, as I intend to also validate the
> placement of child objects, by reading the allowedChildClasses virtual
> attribute. 
>
> In the future, I'll also force the attribute names to be consistant
> (using the case from the schema). 

Please do one thing at a time and leave future stuff out as it makes it
much harder to review the diff...

See my comments inline.

metze
> Index: source/dsdb/schema/schema_init.c
> ===================================================================
> --- source/dsdb/schema/schema_init.c	(revision 25746)
> +++ source/dsdb/schema/schema_init.c	(working copy)
> @@ -907,6 +907,21 @@
>  	return NULL;
>  }
>  
> +const struct dsdb_attribute *dsdb_attribute_by_linkID(const struct dsdb_schema *schema,
> +						      int linkID)
> +{
> +	struct dsdb_attribute *cur;
> +
> +	/* TODO: add binary search */
> +	for (cur = schema->attributes; cur; cur = cur->next) {
> +		if (cur->linkID != linkID) continue;
> +
> +		return cur;
> +	}
> +
> +	return NULL;
> +}
> +
>  const struct dsdb_class *dsdb_class_by_governsID_id(const struct dsdb_schema *schema,
>  						    uint32_t id)
>  {
> Index: source/dsdb/samdb/ldb_modules/kludge_acl.c
> ===================================================================
> --- source/dsdb/samdb/ldb_modules/kludge_acl.c	(revision 25746)
> +++ source/dsdb/samdb/ldb_modules/kludge_acl.c	(working copy)
> @@ -122,6 +122,12 @@
>  	const struct dsdb_schema *schema = dsdb_get_schema(ldb);
>  	const struct dsdb_class *class;
>  	int i, j, ret;
> +
> + 	/* If we don't have a schema yet, we can't do anything... */
> +	if (schema == NULL) {
> +		return LDB_SUCCESS;
> +	}
> +
>  	/* Must remove any existing attribute, or else confusion reins */
>  	ldb_msg_remove_attr(msg, attrName);
>  	ret = ldb_msg_add_empty(msg, attrName, 0, &allowedAttributes);
> @@ -184,6 +190,12 @@
>  	const struct dsdb_schema *schema = dsdb_get_schema(ldb);
>  	const struct dsdb_class *class;
>  	int i, j, ret;
> +
> + 	/* If we don't have a schema yet, we can't do anything... */
> +	if (schema == NULL) {
> +		return LDB_SUCCESS;
> +	}
> +
>  	/* Must remove any existing attribute, or else confusion reins */
>  	ldb_msg_remove_attr(msg, attrName);
>  	ret = ldb_msg_add_empty(msg, attrName, 0, &allowedClasses);

The changes in the above 2 files seem unrelated...

>  static int objectclass_add(struct ldb_module *module, struct ldb_request *req)
>  {
> -	struct ldb_message_element *objectclass_element;
> -	const struct dsdb_schema *schema = dsdb_get_schema(module->ldb);
> -	struct class_list *sorted, *current;
> -	struct ldb_request *down_req;
> -	struct ldb_message *msg;
> +
> +	static const char * const attrs[] = { "allowedChildClasses", 
> +					      "allowedChildClassesEffective", 
> +					      NULL };

not needed yet...


> +static int objectclass_do_add(struct ldb_handle *h) 
> +{
> +	const struct dsdb_schema *schema;
> +	struct oc_context *ac;
> +	struct ldb_message_element *objectclass_element;
> +	struct ldb_message *msg;
> +	TALLOC_CTX *mem_ctx;
> +	struct class_list *sorted, *current;
> +	int ret;
> +      
> +	ac = talloc_get_type(h->private_data, struct oc_context);
> +	schema = dsdb_get_schema(ac->module->ldb);

'schema' is not needed here


>  	/* modify dn */
> @@ -664,6 +736,109 @@
>  	return ldb_next_request(ac->module, ac->mod_req);
>  }
>  
> +static int objectclass_rename(struct ldb_module *module, struct ldb_request *req)
> +{
> +
> +	static const char * const attrs[] = { "allowedChildClasses", 
> +					      "allowedChildClassesEffective", 
> +					      NULL };

this is also not used yet,please leave it out untill it is.

> +	ldb_debug(module->ldb, LDB_DEBUG_TRACE, "objectclass_rename\n");
> +
> +	if (ldb_dn_is_special(req->op.rename.newdn)) { /* do not manipulate our control entries */
> +		return ldb_next_request(module, req);
> +	}


please move the comment above the if statement.

> +{
> +	const struct dsdb_schema *schema;
> +	struct oc_context *ac;
> +	int ret;
> +      
> +	ac = talloc_get_type(h->private_data, struct oc_context);
> +	schema = dsdb_get_schema(ac->module->ldb);


schema is not used in this function.


> +	/* Fix up the DN to be in the standard form, taking particular care to match the parent DN */
> +	
> +	ac->rename_req->op.rename.newdn = ldb_dn_copy(ac->rename_req, ac->search_res->message->dn);
> +	/* Create a new child */
> +	if (ldb_dn_add_child_fmt(ac->rename_req->op.rename.newdn, "X=X") == false) {
> +		return LDB_ERR_OPERATIONS_ERROR;
> +	}
> +	/* And replace it with CN=foo (we need the attribute in upper case */
> +	ret = ldb_dn_set_component(ac->rename_req->op.rename.newdn, 0, 
> +			     strupper_talloc(ac->rename_req->op.rename.newdn, 
> +					     ldb_dn_get_rdn_name(ac->orig_req->op.rename.newdn)),
> +			     *ldb_dn_get_rdn_val(ac->orig_req->op.rename.newdn));
> +	
> +	if (ret != LDB_SUCCESS) {
> +		return ret;
> +	}
>

Can you move this code into a function as it looks like the code in
another function above.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org

iD8DBQFHJZuvm70gjA5TCD8RAnNsAKDHG7rXVLsL56C+U2kwzINGIKZBcACfbOcx
2cy8fyvuSLfo2QMIsbA1cMo=
=IzCw
-----END PGP SIGNATURE-----