[MERGE] Add contrib/bzr_access

Aaron Bentley aaron.bentley at utoronto.ca
Thu Dec 13 19:38:42 GMT 2007


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

John Arbash Meinel wrote:
> I'm not sure that this was 'resubmit' versus tweak.

Well, if we couldn't get the original author to agree to a license, we
couldn't include it at all.

> But I added a copyright header, and a comment about configobj.

And this license was agreed to by Balint Aradi, right?

Assuming yes,
bb:approve

I have some style issues with the script, but given that it's not part
of bzr proper, I'm okay with merging it as-is.

> +# (c) Balint Aradi, 2007

^^ I believe "Copyright" is less legally ambigious than "(c)".

> +All other sections names should be path names (starting with '/'), defining
> +the permissions for the given path. The options in those sections are user
> +names or group references (group name with a leading '@'), the corresponding
> +values are the permissions: 'rw', 'r' and '' (without the quotes) for
> +read-write, read-only and no access, respectively.

^^ Probably worth noting that granting rights to a branch within a
repository allows a mischievous user those rights for all branches in
the repository.

> +def error(msg, exitCode):

^^ "exit_code" would match our style better.

> +class AccessManager(object):
> +    """Manages the permissions, can be queried for a specific user and path."""
> +    
> +    def __init__(self, fp):
> +        """:param fp: File like object, containing the configuration options.
> +        """
> +        # TODO: jam 20071211 Consider switching to bzrlib.util.configobj
> +        self.config = ConfigParser.ConfigParser()
> +        self.config.readfp(fp)
> +        self.groups = {}
> +        if self.config.has_section("groups"):
> +            for group, users in self.config.items("groups"):
> +                self.groups[group] = dict.fromkeys(users.split())

^^ This appears to be using a dictionary for users where a set would
suffice.  A strange choice, since python2.4 is required for Bazaar, and
provides sets.

> +    def permission(self, user, path):
> +        """Determines the permission for a given user and a given path
> +        :param user: user to look for.
> +        :param path: path to look for.
> +        :return: permission.
> +        """
> +        if not path.startswith("/"):
> +            return PERM_DENIED
> +        perm = PERM_DENIED
> +        pathFound = False
> +        while not pathFound and path != "/":
> +            pathFound = self.config.has_section(path)
> +            if (pathFound):
> +                options = self.config.options(path)[-1::-1]

^^ Wow, I think this is the first time I've seen extended slicing in
real code.  But I think reversed(self.config.options(path)) would be
clearer.

> +    def _isRelevant(self, option, user):
> +        """Decides if a certain option is relevant for a given user.
> +      
> +        An option is relevant if it is identical with the user or with a reference
> +        to a group including the user.
> +      
> +        :param option: Option to check.
> +        :param user: User
> +        :return: True if option is relevant for the user, False otherwise.
> +        """
> +        if option.startswith("@"):
> +            result = self.groups.get(option[1:], {}).has_key(user)
> +        else:
> +            result = (option == user)
> +        return result

^^ Possibly it would be better to error out if usernames starting with @
are specified, instead of just failing to authenticate them.

> +############################################################################
> +# Main program
> +############################################################################
> +def main():
> +    # Read arguments
> +    if len(sys.argv) != 4:
> +        error("Invalid number or arguments.", EXIT_BAD_NR_ARG)
> +    (bzrExec, repoRoot, user) = sys.argv[1:4]
> +    
> +    # Sanity checks
> +    if not os.access(bzrExec, os.X_OK):
> +        error("bzr is not executable.", EXIT_BZR_NOEXEC)

^^ I think this is unnecessary look-before-you-leap.

> +    if not os.access(repoRoot, os.R_OK):
> +        error("Path to repository not readable.", EXIT_REPO_NOREAD)

^^ Possibly this, too.

> +    if not len(directory):
> +        error("Bad directory name.", EXIT_BADDIR)

^^ I would prefer if len(directory) == 0

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFHYYpC0F+nu1YWqI0RArr1AJsE4JQKiftNiPou5wC+rqEnsFuO8gCfWk2o
0m+Rg0qRjrbBB/t49ndXoYw=
=Tj7y
-----END PGP SIGNATURE-----



More information about the bazaar mailing list