Rough verification code to ensure deps hosts \in allowed_hosts.

Rough verification code to ensure deps hosts \in allowed_hosts.

BUG=371012
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=292088

[tandrii(chromium), 2014-09-11 16:05:14]

tandrii@chromium.org changed reviewers:
+ iannucci@chromium.org

[tandrii(chromium), 2014-09-11 16:05:15]

iannucci@ please have a look at my rough idea. I'm waiting for your feedback.

[tandrii(chromium), 2014-09-15 15:53:03]

PTAL

CMDverify now works non-recursively checking allowed_hosts in 1-st level DEPS
only.
Also added tests.

What's missing: raising error during normal run if allowed_hosts contradict
DEPs.

[iannucci, 2014-09-16 20:34:08]

looks pretty good.

I think for now we should just add it as a PRESUBMIT check but not fail during
normal execution. Then we can PSA, deploy, and add the fail during execution
later.

https://codereview.chromium.org/562953002/diff/40001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/562953002/diff/40001/gclient.py#newcode696
gclient.py:696: if not self._allowed_hosts:
I think this will be the default condition... most deps files won't have this,
so we should make sure that they continue to work as before.

maybe just do the warning if `'allowed_hosts' in local_scope`.

We should also internally make allowed_hosts a set(), since we're mostly doing
`in` lookups.

https://codereview.chromium.org/562953002/diff/40001/gclient.py#newcode782
gclient.py:782: # TODO(tandrii): what about other possible URL types?
are there other url types?

https://codereview.chromium.org/562953002/diff/40001/gclient.py#newcode1085
gclient.py:1085: return tuple(self._allowed_hosts)
set or frozenset

https://codereview.chromium.org/562953002/diff/40001/gclient.py#newcode2129
gclient.py:2129: # TODO(tandrii): do we want this recursively?
possibly, though we'd probably want to scope each allowed_hosts to the DEPS
file. e.g. allowing 'cool.whiz' in DEPS for src doesn't allow 'cool.whiz' for
blink's DEPS (or vice versa).

[tandrii(chromium), 2014-09-16 21:03:20]

Just one comment; more to follow with code.

https://codereview.chromium.org/562953002/diff/40001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/562953002/diff/40001/gclient.py#newcode696
gclient.py:696: if not self._allowed_hosts:
On 2014/09/16 20:34:08, iannucci wrote:
> I think this will be the default condition... most deps files won't have this,
> so we should make sure that they continue to work as before.
Yep, that's why I had default "None" to preserve previous behavior, although
empty set seems reasonable as well, since if allowed_hosts is defined, it
definitely shouldn't be empty. I'll raise exception in this case.
> 
> maybe just do the warning if `'allowed_hosts' in local_scope`.
I didn't get what you mean. Maybe if it is *NOT* in local_scope?
> 
> We should also internally make allowed_hosts a set(), since we're mostly doing
> `in` lookups.
Haha, I also thought of that, but then given the expected number of entries
there, I considered it a not-so-clear optimization, since computing hash of a
long hostname could be longer than checking for for a few comparisons :)
Anyways, I'll make it set, since it's more natural fit for "in" lookups and you
agree :)

[tandrii(chromium), 2014-09-17 15:16:00]

https://codereview.chromium.org/562953002/diff/40001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/562953002/diff/40001/gclient.py#newcode782
gclient.py:782: # TODO(tandrii): what about other possible URL types?
On 2014/09/16 20:34:08, iannucci wrote:
> are there other url types?

Well, I check here (copied from other part of gclient) that dep.url is
basestring. What if it isn't? For now, I just ignore.

[tandrii(chromium), 2014-09-18 19:59:59]

PTAL

And the follow up CLs for the same bug in chromium repo:

https://codereview.chromium.org/578423002/

https://codereview.chromium.org/575283005/

https://codereview.chromium.org/562953002/diff/40001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/562953002/diff/40001/gclient.py#newcode696
gclient.py:696: if not self._allowed_hosts:
On 2014/09/16 20:34:08, iannucci wrote:
> I think this will be the default condition... most deps files won't have this,
> so we should make sure that they continue to work as before.
> 
> maybe just do the warning if `'allowed_hosts' in local_scope`.
> 
> We should also internally make allowed_hosts a set(), since we're mostly doing
> `in` lookups.

Done.

https://codereview.chromium.org/562953002/diff/40001/gclient.py#newcode782
gclient.py:782: # TODO(tandrii): what about other possible URL types?
On 2014/09/16 20:34:08, iannucci wrote:
> are there other url types?

Done.

https://codereview.chromium.org/562953002/diff/40001/gclient.py#newcode1085
gclient.py:1085: return tuple(self._allowed_hosts)
On 2014/09/16 20:34:08, iannucci wrote:
> set or frozenset

Done.

https://codereview.chromium.org/562953002/diff/40001/gclient.py#newcode2129
gclient.py:2129: # TODO(tandrii): do we want this recursively?
On 2014/09/16 20:34:08, iannucci wrote:
> possibly, though we'd probably want to scope each allowed_hosts to the DEPS
> file. e.g. allowing 'cool.whiz' in DEPS for src doesn't allow 'cool.whiz' for
> blink's DEPS (or vice versa).

Done.

[iannucci, 2014-09-18 20:54:41]

lgtm sorry for the delay

https://codereview.chromium.org/562953002/diff/100001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/562953002/diff/100001/gclient.py#newcode349
gclient.py:349: self._allowed_hosts = set()
make it an empty frozenset

https://codereview.chromium.org/562953002/diff/100001/gclient.py#newcode696
gclient.py:696: self._allowed_hosts = set(local_scope.get('allowed_hosts'))
this could throw a TypeError if it's a non-iterable.

let's also make this a frozenset, since it shouldn't be modified.

https://codereview.chromium.org/562953002/diff/100001/gclient.py#newcode701
gclient.py:701: 'ParseDepsFile(%s): allowed_hosts must be a non-empty iterable'
%
may want to tell them to delete it if they're making it empty.

allowed_hosts must be absent or a non-empty iterable

https://codereview.chromium.org/562953002/diff/100001/gclient.py#newcode1086
gclient.py:1086: return frozenset(self._allowed_hosts)
can return a reference since it's an immutable type

https://codereview.chromium.org/562953002/diff/100001/tests/gclient_test.py
File tests/gclient_test.py (right):

https://codereview.chromium.org/562953002/diff/100001/tests/gclient_test.py#n...
tests/gclient_test.py:989: 'allowed_hosts = []'
should test a non-iterable as well (like None).

[tandrii(chromium), 2014-09-18 21:45:58]

Thanks, you've helped find bugs in testing code as well (missing '\n' at the end
of allowed_hosts line). I've fixed all things you've mentioned as well.

https://codereview.chromium.org/562953002/diff/100001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/562953002/diff/100001/gclient.py#newcode349
gclient.py:349: self._allowed_hosts = set()
On 2014/09/18 20:54:40, iannucci wrote:
> make it an empty frozenset

Done.

https://codereview.chromium.org/562953002/diff/100001/gclient.py#newcode696
gclient.py:696: self._allowed_hosts = set(local_scope.get('allowed_hosts'))
On 2014/09/18 20:54:41, iannucci wrote:
> this could throw a TypeError if it's a non-iterable.
> 
> let's also make this a frozenset, since it shouldn't be modified.

Done.

https://codereview.chromium.org/562953002/diff/100001/gclient.py#newcode701
gclient.py:701: 'ParseDepsFile(%s): allowed_hosts must be a non-empty iterable'
%
On 2014/09/18 20:54:40, iannucci wrote:
> may want to tell them to delete it if they're making it empty.
> 
> allowed_hosts must be absent or a non-empty iterable

Done.

https://codereview.chromium.org/562953002/diff/100001/gclient.py#newcode1086
gclient.py:1086: return frozenset(self._allowed_hosts)
On 2014/09/18 20:54:40, iannucci wrote:
> can return a reference since it's an immutable type

Done.

https://codereview.chromium.org/562953002/diff/100001/tests/gclient_test.py
File tests/gclient_test.py (right):

https://codereview.chromium.org/562953002/diff/100001/tests/gclient_test.py#n...
tests/gclient_test.py:989: 'allowed_hosts = []'
On 2014/09/18 20:54:41, iannucci wrote:
> should test a non-iterable as well (like None).

Done.

[iannucci, 2014-09-22 18:44:45]

still lgtm

https://codereview.chromium.org/562953002/diff/140001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/562953002/diff/140001/gclient.py#newcode699
gclient.py:699: self._allowed_hosts = frozenset()
should be able to `pass` here, right?

https://codereview.chromium.org/562953002/diff/140001/gclient.py#newcode781
gclient.py:781: if self._allowed_hosts is None:
is this ever true now?

[tandrii(chromium), 2014-09-23 11:46:38]

https://codereview.chromium.org/562953002/diff/140001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/562953002/diff/140001/gclient.py#newcode699
gclient.py:699: self._allowed_hosts = frozenset()
On 2014/09/22 18:44:45, iannucci wrote:
> should be able to `pass` here, right?

Done.

https://codereview.chromium.org/562953002/diff/140001/gclient.py#newcode781
gclient.py:781: if self._allowed_hosts is None:
On 2014/09/22 18:44:45, iannucci wrote:
> is this ever true now?

Done.

[tandrii(chromium), 2014-09-23 11:46:47]

The CQ bit was checked by tandrii@chromium.org

[commit-bot: I haz the power, 2014-09-23 11:47:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/562953002/160001

[commit-bot: I haz the power, 2014-09-23 11:49:55]

Committed patchset #9 (id:160001) as 292088