Re: [PATCH] push: deny policy to prevent pushes to unwanted remotes.

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: May 30, 2016, 13:19 UTC
Message-ID: <vpq37ozd6o7.fsf@anie.imag.fr>
In-Reply-To: <20160530104501.4402-1-antoine.queru@ensimag.grenoble-inp.fr>

Antoine Queru <antoine.queru@ensimag.grenoble-inp.fr> writes:

Show 7 quoted lines

> Currently, a user wanting to prevent accidental pushes to the wrong remote
> has to create a pre-push hook.
> The feature offers a configuration to allow users to prevent accidental pushes
> to the wrong remote. The user may define a list of whitelisted remotes, a list
> of blacklisted remotes and a default policy ("allow" or "deny"). A push
> is denied if the remote is explicitely blacklisted or if it isn't
> whitelisted and the default policy is "deny".

Not really serious, but the text above is weirdly wrapped, probably by hand. I'm sure your text editor can do better and quicker ;-).

Show 11 quoted lines

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 53f00db..1478ce3 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2517,6 +2517,23 @@ remote.pushDefault::
>  	`branch.<name>.remote` for all branches, and is overridden by
>  	`branch.<name>.pushRemote` for specific branches.
>  
> +remote.pushBlacklisted::
> +	The list of remotes the user is forbidden to push to.
> +	See linkgit:git-push[1]

I'd have spelled that "pushBlacklist" (no 'ed'). I think variable names usually do not use verbs. But I'm fine with your version too.

Show 7 quoted lines

> +For example, if we set up the configuration variables like this:
> +	git config --add remote.pushBlacklisted repository.com
> +	git config --add remote.pushWhitelisted repository.com/Special_Folder
> +Any push of this form will be accepted:
> +	git push repository.com/Special_Folder/foo
> +While those ones for example will be denied:
> +	git push repository.com/Normal_Folder/bar

Please, look at the rendered output of your documentation, and notice it's broken. We'd typically use the asciidoc syntax for inline code here (between -----).

> +An error will be raised if the url is blacklisted and whitelisted at the same.

"at the same time"?

But as-is, this sentence conflicts with the previous "the more the url in the config prefixes the asked url the more priority it has." statement.

The documentation doesn't talk about the URL-normalization the code is doing. I think a reasonable behavior would be:

pushBlacklisted = example.com/ => deny all accesses to example.com
pushBlacklisted = http://example.com/ => deny HTTP accesses to
                                         example.com

The second is a valid use-case IMHO, some people may want to forbid some protocols. Actually, one may even want to whilelist only one protocol and write stg like this to force HTTPS on host example.com:

  pushBlacklisted = example.com
  pushWhitelisted = https://example.com

BTW, these use-cases could motivate some per-blacklist deny message like

[remote "example.com"]
	pushDenyMessage = "Please use HTTPS when you push to example.com"

I don't think it has to be implemented now though (better have users get used to the basic feature and see if more is needed later).

> +static const char *string_url_normalize(const char *repo_name)
> +{
> +	if (starts_with(repo_name, "file://"))
> +		return repo_name + 7;

There are many instances of this in Git's codebase, but we now try to avoid magic numbers like this, and would use strlen("file://") instead. Actually, we even have skip_prefix() precisely for this use-case.

> +	if (is_url(repo_name)) {
> +		struct url_info url_infos;
> +		url_normalize(repo_name, &url_infos);
> +		return url_infos.url + url_infos.host_off;

I think this would deserve a comment to explain why and what this is doing, like /* turn ... into ... to ... */.

> +test_expect_success 'unsetup' '

"cleanup" ?

(I just did a very quick look at the code, I think we need an agreement on the details of specification before a more detailed review)

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/