Re: [PATCH] push: deny policy to prevent pushes to unwanted remotes.
- From
- Francois Beutin <beutinf@ensimag.grenoble-inp.fr>
- Date
- May 30, 2016, 15:30 UTC
- Message-ID
- <1696484918.188215.1464622238984.JavaMail.zimbra@ensimag.grenoble-inp.fr>
- In-Reply-To
- <vpq37ozd6o7.fsf@anie.imag.fr>
----- Mail original -----
Show 14 quoted lines
> Antoine Queru <antoine.queru@ensimag.grenoble-inp.fr> writes:
>
> > 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 ;-).Yes indeed it was hand wrapped, it will be changed in the next version, thanks for the hint!
Show 14 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.
Ok it makes sense, we will change for "pushBlacklist".
Show 11 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 -----).
Indeed the rendered doc is broken, it will be fixed in the next version.
Show 8 quoted lines
> > +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.
I understand the confusion, it's not very clear. What we wanted to say
was that:
pushBlacklist = example.com/
pushWhitelist = example.com/
would raise an error, because the "priority rule" can't be applied.
We will change the sentence.Show 13 quoted lines
> 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
As of right now the protocol is not handled, we just ignore it.
We already thought about handling it the way you described but
we found out some problems:
pushBlacklist = example.com
pushWhitelist = https://example.com
-> push example.com forbidden EXCEPT with https pushBlacklist = example.com/example2
pushWhitelist = https://example.com
-> push https://example.com/example2
-> Allow or deny?To sum up the problem, what should be the priority between a
protocol-matching rule and a sub-folder-matching rule?
Perhaps adding a configuration variable?
pushPriority = "protocol"/"sub-folder" (needs renaming)Show 8 quoted lines
> 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).
Interesting idea, perhaps a simplified version could be:
pushDenyProtocolMessage = "This protocol is not allowed for this remote"
pushDenyRemoteMessage = "This remote is not authorized"
(as above v ery roughnaming)Show 8 quoted lines
> > +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.Ok we will change it and use the skip_prefix() function.
Show 7 quoted lines
> > + 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 ... */.This code removes all "unnecessary" parts of the url (protocol, user...). Seeing the comments above, this code will probably be changed but we will comment tricky code more often.
> > +test_expect_success 'unsetup' ' > > "cleanup" ?
Ok that's better
> (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)
I agree
> -- > Matthieu Moy > http://www-verimag.imag.fr/~moy/ >