Merge lp://staging/~jelmer/bzr/safe-open into lp://staging/bzr

Moving this up to the bzrlib level seems reasonable, but it really needs a better name. The problem with 'safe' is you need context to understand what it actually means and the term is too widely used for completely different things.

+"""Safe branch opening."""

Okay, something to do with branches.

+ def check_one_url(self, url):
+ """Check the safety of the source URL.

Something like same domain restrictions?

+class SafeBranchOpener(object):
+ """Safe branch opener.

Hm.

+def safe_open(allowed_scheme, url):
+ """Open the branch at `url`, only accessing URLs on `allowed_scheme`.

Aha! A useful docstring on this function... which actually does something much more specific than the name or module content implies.

So this is really about opening branches with certain URL based checks added. The naming should really reflect that limited scope.

+class BlacklistPolicy(BranchOpenPolicy):
+ """Branch policy that forbids certain URLs."""
+
+ def __init__(self, should_follow_references, unsafe_urls=None):
+ if unsafe_urls is None:
+ unsafe_urls = set()
+ self._unsafe_urls = unsafe_urls

This is unsafe, bzrlib doesn't normalise URLs enough to ensure there's only one canonical form. Also, forgetting a param meaning "no restrictions" isn't a great interface. I'd just add an underscore to the class name for now to discourage use.

There's some more to nitpick over with the code, but it doesn't seem that important.

Am 24/12/11 13:26, schrieb Martin Packman:
> Review: Needs Fixing
>
> Moving this up to the bzrlib level seems reasonable, but it really needs a better name. The problem with 'safe' is you need context to understand what it actually means and the term is too widely used for completely different things.
Hmm, interesting point. I took the name from launchpad, but I guess I've
just gotten used to it meaning this... What about "bzrlib.hookable_open"
or "bzrlib.policy_open" ?

> +def safe_open(allowed_scheme, url):
> + """Open the branch at `url`, only accessing URLs on `allowed_scheme`.
>
> Aha! A useful docstring on this function... which actually does something much more specific than the name or module content implies.
>
>
> So this is really about opening branches with certain URL based checks added. The naming should really reflect that limited scope.

I'm not sure I follow. Do you mean e.g. renaming it to e.g.
open_with_specific_scheme ?
>
> +class BlacklistPolicy(BranchOpenPolicy):
> + """Branch policy that forbids certain URLs."""
> +
> + def __init__(self, should_follow_references, unsafe_urls=None):
> + if unsafe_urls is None:
> + unsafe_urls = set()
> + self._unsafe_urls = unsafe_urls
>
> This is unsafe, bzrlib doesn't normalise URLs enough to ensure there's only one canonical form. Also, forgetting a param meaning "no restrictions" isn't a great interface. I'd just add an underscore to the class name for now to discourage use.
>
> There's some more to nitpick over with the code, but it doesn't seem that important.
This is only really used in the Launchpad testsuite. I guess I could
just leave it over in the Launchpad code.

Cheers,

Jelmer

> Hmm, interesting point. I took the name from launchpad, but I guess I've
> just gotten used to it meaning this... What about "bzrlib.hookable_open"
> or "bzrlib.policy_open" ?

Something like that, but unless there's any thought to extending the api beyond just looking at urls, I think that should be reflected in the name. I'm not bursting with ideas though... limited_location_open? restrict_url_open? That this is only branches not repos or trees is also relevant. Perhaps we can bikeshed on IRC?

With a module rename, I'd change SafeBranchOpener to just BranchOpener and fix all the docstrings with 'safe' in them to say something meaningful as well.

> I'm not sure I follow. Do you mean e.g. renaming it to e.g.
> open_with_specific_scheme ?

Yup. Or open_only_scheme, open_branch_with_scheme, something like that.

> This is only really used in the Launchpad testsuite. I guess I could
> just leave it over in the Launchpad code.

Yeah, I thought it probably wasn't in actual use. I don't mind bringing it across anyway, with either an underscore or just a docstring stating that limitation.