review requested: Add documentation on the ACLs helper for the samdb
simo
idra at samba.org
Sun Oct 14 15:05:05 MDT 2012
On Sun, 2012-10-14 at 13:54 -0700, Matthieu Patou wrote:
> 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.
No, you can have multi-lines for brief too.
> There are also a couple of case where making a 82 lines makes the code
> much more readable than if you stick to 80.
In some rare cases that may be true, it's a judgment call.
But it is almost never the case for comments.
> >> 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 !
Yes but this should not discourage you, in most cases here it was more
of an English problem than content.
> 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" ?
ok then: "GUID of the right to be granted, in string form" ?
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