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