review requested: Add documentation on the ACLs helper for the samdb

simo idra at samba.org
Sun Oct 14 08:37:21 MDT 2012


On Sat, 2012-10-13 at 16:07 -0700, Matthieu Patou wrote:
> Hello all,
> 
> Can I have a review of this patch that is adding some documentation
> to 
> the functions related to acl access checks in samdb/dsdb.
> 
> Thank you.

NACK, comments inline.

In general can you make sure no line is longer than 78 chars?
In case you are wondering why I insist so much on the 80 columns rule
here is a page that give reasons I fully agree with (with the mod that I
get 3 editors side by side not just 2 :-):

http://www.emacswiki.org/emacs/EightyColumnRule



> 
> 
> 
> 
> 
> differences
> between files
> attachment
> (0001-dsdb-acls-Add-documentation.patch)
> 
> >From 23f9aeeb1131b8ab08a3c308f31affde05299ee8 Mon Sep 17 00:00:00
> 2001
> From: Matthieu Patou <mat at matws.net>
> Date: Sat, 13 Oct 2012 13:46:58 -0700
> Subject: [PATCH] dsdb-acls: Add documentation
> 
> Add documentation on ACLs.
> ---
>  source4/dsdb/samdb/ldb_modules/acl_util.c |   61
> ++++++++++++++++++++++++++++-
>  1 file changed, 59 insertions(+), 2 deletions(-)
> 
> diff --git a/source4/dsdb/samdb/ldb_modules/acl_util.c
> b/source4/dsdb/samdb/ldb_modules/acl_util.c
> index 50bf888..7c295c7 100644
> --- a/source4/dsdb/samdb/ldb_modules/acl_util.c
> +++ b/source4/dsdb/samdb/ldb_modules/acl_util.c
> @@ -90,6 +90,38 @@ int dsdb_module_check_access_on_dn(struct
> ldb_module *module,
>                                                 guid);
>  }
>  
> +/**
> + * @brief Checks the current as the requested access on 1 attribute

Sorry I can't understand what this statement means,
would the following be clearer ?
"Checks trustee access to a specific attribute"

> + *
> + * This function checks if a given trustee has the requested access
> + * on the specified attribute given the current security descriptor.
> + * The attribute can be NULL in this case the check will skip the
> + * OBJECT_ACE entries.

This one is clear enough.

> + * @param[in]  module      A struct ldb_module object, security token
> + *                         for the current user are stored within the
> + *                         module object.

replace with: "A pointer to a ldb_module structure. The Security Token
for the current user is stored in this object"

> + *
> + * @param[in]  mem_ctx     A talloc context object for memory
> allocation

remove 'object' IMO

> + * @param[in]  sd          A security descriptor for attr

"The Security descriptor used to test 'attr'"

> + * @param[in]  rp_sid      The SID of the domain, used for expanding
> + *                         trustee in ACE that are just a RID.

"The Domain SID. This is used to complete RID-only ACEs"

> + * @param[in]  access_mask An integer that represents the desired
> access
> + *                         that the security descriptor should grant
> to
> + *                         the user on the given attribute
> + *
> + * @param[in]  attr        A dsdb_attribute for which the checks
> should be
> + *                         performed.
> + *
> + * @return                 Returns LDB_SUCCESS on success, on error
> another
> + *                         ldb error code.

I would simplify with: "An ldb_error error code"

> + *                         If the requested rights are not granted
> + *                         LDB_ERR_INSUFFICIENT_ACCESS_RIGHTS will be
> returned.
> + *
> + */
>  int acl_check_access_on_attribute(struct ldb_module *module,
>                                   TALLOC_CTX *mem_ctx,
>                                   struct security_descriptor *sd,
> @@ -150,8 +182,33 @@ fail:
>         return ldb_operr(ldb_module_get_ctx(module));
>  }
>  
> -
> -/* checks for validated writes */
> +/**
> + * @brief Checks if a given extended right grants the desired access
> for a given user
> + *
> + * This function checks if a given user is granted the specified
> extended right
> + * with the requested access right.
> + *
> + * @param[in]  mem_ctx     A talloc context object for memory
> allocation

IMO remove 'object'

> + * @param[in]  sd          A security descriptor for attr

"The Security descriptor used to test 'attr'"

> + * @param[in]  token       The security token reprensenting the user
> + *
> + * @param[in]  ext_right   A string representation of the GUID of the
> extended
> + *                         right to test.

Phrase this way it is ambiguous, is this "A string ... to test" ? Or is
it ".... the right to test" ?
You may want to move 'to test' around to clarify.

> + * @param[in]  right_typ   An integer that represents the desired
> access
> + *                         that the security descriptor should grant
> to
> + *                         the user for the specified extended access
> right
> + *
> + * @param[in]  dom_sid     The SID of the domain, used for expanding
> + *                         trustee in ACE that are just a RID.

"The Domain SID. This is used to complete RID-only ACEs"

> + * @return                 Returns LDB_SUCCESS on success, on error
> another
> + *                         ldb error code.
> + *                         If the requested rights are not granted
> + *                         LDB_ERR_INSUFFICIENT_ACCESS_RIGHTS will be
> returned.
> + */
>  int acl_check_extended_right(TALLOC_CTX *mem_ctx,
>                              struct security_descriptor *sd,
>                              struct security_token *token,


Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo at samba.org>
Principal Software Engineer at Red Hat, Inc. <simo at redhat.com>



More information about the samba-technical mailing list