review requested: Add documentation on the ACLs helper for the samdb
Matthieu Patou
mat at samba.org
Sun Oct 14 14:54:45 MDT 2012
On 10/14/2012 07:37 AM, simo wrote:
> 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
I try to stick to 80 chars lines but here for instance It's on the brief
line,
I thought that brief can't be more than 1 line but it seems not.
There are also a couple of case where making a 82 lines makes the code
much more readable than if you stick to 80.
>
>
>
>>
>>
>>
>>
>> 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,
Ok agreed it's not so easy to make docs even less you didn't wrote the
code !
Maybe for
> + *
> + * @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.
>
The correct wording should be:
"The right that should be granted, the right is represented the string
version of its GUID" ?
Matthieu
--
Matthieu Patou
Samba Team
http://samba.org
More information about the samba-technical
mailing list