Issue 4954 - Implement a downloader script supporting OAuth2 authentication/authorization

Issue 4954 - Implement a downloader script supporting OAuth2 authentication/authorization

[Tudor Avram, 2018-07-18 13:44:03]

Hey,

Here is my first attempt at writing the Oauth2 downloader script. 

As discussed, the automated tests run using a dummy web app that behaves like
the Google APIs for the corner cases I required. I also tested the script
manually using real credentials and behaves at expected.

Thanks, 
Tudor.

[Vasily Kuznetsov, 2018-07-20 21:00:01]

Hi Tudor,

The approach looks solid in general. I have a bunch of cosmetic comments and
also something weird is happening to .gitignore (see below).

Cheers,
Vasily

https://codereview.adblockplus.org/29833582/diff/29833583/.gitignore
File .gitignore (right):

https://codereview.adblockplus.org/29833582/diff/29833583/.gitignore#newcode10
.gitignore:10: .Python
Where's this line coming from? It seems like it's not in .gitignore originally
[1]. Could it be that you have an extra commit there or something like that?

[1]: https://github.com/adblockplus/sitescripts/blob/master/.gitignore

https://codereview.adblockplus.org/29833582/diff/29833583/.gitignore#newcode11
.gitignore:11: keyfile.json
Do we really need to ignore this? If we do, it should also be ignored in
.hgignore.

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
File sitescripts/oauth2dl/bin/constants.py (right):

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
sitescripts/oauth2dl/bin/constants.py:16: KEYFILE_NOT_FOUND_ERROR = 'ERROR:
Oauth2 key file not found! Please specify ' \
PEP8 [1] recommends breaking long lines by implied continuation inside
parentheses instead of using backslashes when possible. What do you think about
following this and rewriting these constants like this:

    KEYFILE_NOT_FOUND_ERROR = (
        'ERROR: Oauth2 key file not found! Please specify '
        'it as an optional argument using -k or --key or '
        ...
    )

[1]: https://www.python.org/dev/peps/pep-0008/?#maximum-line-length

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
File sitescripts/oauth2dl/bin/oauth2dl.py (right):

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
sitescripts/oauth2dl/bin/oauth2dl.py:17: import argparse
PEP8 recommends breaking the imports into three groups: stdlib, 3-rd party
libraries, local imports, and separating the three groups by an empty line.

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
sitescripts/oauth2dl/bin/oauth2dl.py:25: try:
I think we can assume that the root of sitescripts is in the PYTHONPATH so the
first import is sufficient.

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
sitescripts/oauth2dl/bin/oauth2dl.py:31: def write_to_file(content, path):
I'm not sure if it really makes sense to have this in a separate function, but
don't feel too strong about it.

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
sitescripts/oauth2dl/bin/oauth2dl.py:65: credentials =
ServiceAccountCredentials.from_json_keyfile_name(key_file, scopes=[scope])
PEP8 recommends keeping the lines under 80 characters. This line can be
converted this way so I would propose to do it.

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
File sitescripts/oauth2dl/test/test_oauth2dl.py (right):

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
sitescripts/oauth2dl/test/test_oauth2dl.py:16: import subprocess
Same comment about ordering as in oauth2dl.py

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
sitescripts/oauth2dl/test/test_oauth2dl.py:77: data = {
There seems to be some repetition here, maybe we can extract a function for
writing a keyfile to make things a bit more compact and clear. What do you
think?

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
sitescripts/oauth2dl/test/test_oauth2dl.py:196:
str(rootdir.join('file_to_downlaaoad_utf8')))
..._donwlaaoad_... -- typo or on purpose? :)

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
File sitescripts/oauth2dl/utils/dummy_wsgi_app.py (right):

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
sitescripts/oauth2dl/utils/dummy_wsgi_app.py:1: # This file is part of the
Adblock Plus web scripts,
I would just move this into the test folder. Or is this used for anything
outside of the tests?

https://codereview.adblockplus.org/29833582/diff/29833583/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29833582/diff/29833583/tox.ini#newcode80
tox.ini:80: /sitescripts/oauth2dl/bin/oauth2dl.py: D413, E501, A301
It would be better not to add exceptions for new code. Is any of this really
necessary or can we adjust the code to make flake8 happy?

https://codereview.adblockplus.org/29833582/diff/29833583/tox.ini#newcode102
tox.ini:102: httplib2
Do we need httplib2 for the new script? Do Google libraries require it or what's
the reason?

https://codereview.adblockplus.org/29833582/diff/29833583/tox.ini#newcode123
tox.ini:123: sitescripts/oauth2dl/test/test_oauth2dl.py \
Would be neater to keep consistent indentation.

[Tudor Avram, 2018-07-23 19:29:04]

Hi Vasily,

Thanks for the review! 

I'll address the issues you pointed out. I also commented on a couple of things,
which were done on purpose. If you think you know around them (especially for
the try... import .... except ImportError thingy), that would be great! 

Thanks, 
Tudor.

https://codereview.adblockplus.org/29833582/diff/29833583/.gitignore
File .gitignore (right):

https://codereview.adblockplus.org/29833582/diff/29833583/.gitignore#newcode10
.gitignore:10: .Python
On 2018/07/20 21:00:00, Vasily Kuznetsov wrote:
> Where's this line coming from? It seems like it's not in .gitignore originally
> [1]. Could it be that you have an extra commit there or something like that?
> 
> [1]: https://github.com/adblockplus/sitescripts/blob/master/.gitignore

Oh... I'll have to take a look, not sure why that happened. I'll just pull the
master from github to be in sync.

https://codereview.adblockplus.org/29833582/diff/29833583/.gitignore#newcode11
.gitignore:11: keyfile.json
On 2018/07/20 21:00:00, Vasily Kuznetsov wrote:
> Do we really need to ignore this? If we do, it should also be ignored in
> .hgignore.

Oh, that was my mistake. I put the file in there just to test things manually
and forgot to remove it from .gitignore.

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
File sitescripts/oauth2dl/bin/oauth2dl.py (right):

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
sitescripts/oauth2dl/bin/oauth2dl.py:17: import argparse
On 2018/07/20 21:00:01, Vasily Kuznetsov wrote:
> PEP8 recommends breaking the imports into three groups: stdlib, 3-rd party
> libraries, local imports, and separating the three groups by an empty line.

Acknowledged.

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
sitescripts/oauth2dl/bin/oauth2dl.py:25: try:
On 2018/07/20 21:00:00, Vasily Kuznetsov wrote:
> I think we can assume that the root of sitescripts is in the PYTHONPATH so the
> first import is sufficient.

That was to allow it to run both as `python -m ...` and as `python ___.py`. When
I run it as a module, it works, but as a simple file the import breaks. At least
on my machine

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
sitescripts/oauth2dl/bin/oauth2dl.py:31: def write_to_file(content, path):
On 2018/07/20 21:00:00, Vasily Kuznetsov wrote:
> I'm not sure if it really makes sense to have this in a separate function, but
> don't feel too strong about it.

Acknowledged.

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
sitescripts/oauth2dl/bin/oauth2dl.py:65: credentials =
ServiceAccountCredentials.from_json_keyfile_name(key_file, scopes=[scope])
On 2018/07/20 21:00:00, Vasily Kuznetsov wrote:
> PEP8 recommends keeping the lines under 80 characters. This line can be
> converted this way so I would propose to do it.

Yeah, I know. that's why I added the flake8 exception in `.tox`. I just couldn't
find a nice way to break that into multiple lines. 

I just realised that I can try something like: 

credentials = \
          ServiceAccount.....

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
File sitescripts/oauth2dl/test/test_oauth2dl.py (right):

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
sitescripts/oauth2dl/test/test_oauth2dl.py:16: import subprocess
On 2018/07/20 21:00:01, Vasily Kuznetsov wrote:
> Same comment about ordering as in oauth2dl.py

Acknowledged.

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
sitescripts/oauth2dl/test/test_oauth2dl.py:196:
str(rootdir.join('file_to_downlaaoad_utf8')))
On 2018/07/20 21:00:01, Vasily Kuznetsov wrote:
> ..._donwlaaoad_... -- typo or on purpose? :)

It was on purpose. I wanted to test the behaviour in the case of a wrong path. 

I can change it to 'missing_file' or something like that, so it makes things a
little more clear.

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
File sitescripts/oauth2dl/utils/dummy_wsgi_app.py (right):

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
sitescripts/oauth2dl/utils/dummy_wsgi_app.py:1: # This file is part of the
Adblock Plus web scripts,
On 2018/07/20 21:00:01, Vasily Kuznetsov wrote:
> I would just move this into the test folder. Or is this used for anything
> outside of the tests?

Acknowledged.

https://codereview.adblockplus.org/29833582/diff/29833583/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29833582/diff/29833583/tox.ini#newcode102
tox.ini:102: httplib2
On 2018/07/20 21:00:01, Vasily Kuznetsov wrote:
> Do we need httplib2 for the new script? Do Google libraries require it or
what's
> the reason?

Yeah, the google library requires an `httplib2.Http()` object to be passed to
them. From my understanding, they are using the respective object to make the
requests to their API.

https://codereview.adblockplus.org/29833582/diff/29833583/tox.ini#newcode123
tox.ini:123: sitescripts/oauth2dl/test/test_oauth2dl.py \
On 2018/07/20 21:00:01, Vasily Kuznetsov wrote:
> Would be neater to keep consistent indentation.

Acknowledged.

[Tudor Avram, 2018-07-31 09:20:19]

Hi Vasily,

I addressed your comments and here is my refactored code! 

Thanks, 
Tudor.

https://codereview.adblockplus.org/29833582/diff/29833583/.gitignore
File .gitignore (right):

https://codereview.adblockplus.org/29833582/diff/29833583/.gitignore#newcode10
.gitignore:10: .Python
On 2018/07/23 19:29:02, Tudor Avram wrote:
> On 2018/07/20 21:00:00, Vasily Kuznetsov wrote:
> > Where's this line coming from? It seems like it's not in .gitignore
originally
> > [1]. Could it be that you have an extra commit there or something like that?
> > 
> > [1]: https://github.com/adblockplus/sitescripts/blob/master/.gitignore
> 
> Oh... I'll have to take a look, not sure why that happened. I'll just pull the
> master from github to be in sync.

Done.

https://codereview.adblockplus.org/29833582/diff/29833583/.gitignore#newcode11
.gitignore:11: keyfile.json
On 2018/07/23 19:29:02, Tudor Avram wrote:
> On 2018/07/20 21:00:00, Vasily Kuznetsov wrote:
> > Do we really need to ignore this? If we do, it should also be ignored in
> > .hgignore.
> 
> Oh, that was my mistake. I put the file in there just to test things manually
> and forgot to remove it from .gitignore. 

Done.

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
File sitescripts/oauth2dl/bin/constants.py (right):

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
sitescripts/oauth2dl/bin/constants.py:16: KEYFILE_NOT_FOUND_ERROR = 'ERROR:
Oauth2 key file not found! Please specify ' \
On 2018/07/20 21:00:00, Vasily Kuznetsov wrote:
> PEP8 [1] recommends breaking long lines by implied continuation inside
> parentheses instead of using backslashes when possible. What do you think
about
> following this and rewriting these constants like this:
> 
>     KEYFILE_NOT_FOUND_ERROR = (
>         'ERROR: Oauth2 key file not found! Please specify '
>         'it as an optional argument using -k or --key or '
>         ...
>     )
> 
> [1]: https://www.python.org/dev/peps/pep-0008/#maximum-line-length

Done.

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
File sitescripts/oauth2dl/bin/oauth2dl.py (right):

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
sitescripts/oauth2dl/bin/oauth2dl.py:17: import argparse
On 2018/07/20 21:00:01, Vasily Kuznetsov wrote:
> PEP8 recommends breaking the imports into three groups: stdlib, 3-rd party
> libraries, local imports, and separating the three groups by an empty line.

Done.

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
sitescripts/oauth2dl/bin/oauth2dl.py:25: try:
On 2018/07/20 21:00:00, Vasily Kuznetsov wrote:
> I think we can assume that the root of sitescripts is in the PYTHONPATH so the
> first import is sufficient.

Done.

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
sitescripts/oauth2dl/bin/oauth2dl.py:31: def write_to_file(content, path):
On 2018/07/20 21:00:00, Vasily Kuznetsov wrote:
> I'm not sure if it really makes sense to have this in a separate function, but
> don't feel too strong about it.

Done.

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
sitescripts/oauth2dl/bin/oauth2dl.py:65: credentials =
ServiceAccountCredentials.from_json_keyfile_name(key_file, scopes=[scope])
On 2018/07/20 21:00:00, Vasily Kuznetsov wrote:
> PEP8 recommends keeping the lines under 80 characters. This line can be
> converted this way so I would propose to do it.

Done.

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
File sitescripts/oauth2dl/test/test_oauth2dl.py (right):

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
sitescripts/oauth2dl/test/test_oauth2dl.py:16: import subprocess
On 2018/07/20 21:00:01, Vasily Kuznetsov wrote:
> Same comment about ordering as in oauth2dl.py

Done.

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
sitescripts/oauth2dl/test/test_oauth2dl.py:77: data = {
On 2018/07/20 21:00:01, Vasily Kuznetsov wrote:
> There seems to be some repetition here, maybe we can extract a function for
> writing a keyfile to make things a bit more compact and clear. What do you
> think?

Done.

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
sitescripts/oauth2dl/test/test_oauth2dl.py:196:
str(rootdir.join('file_to_downlaaoad_utf8')))
On 2018/07/20 21:00:01, Vasily Kuznetsov wrote:
> ..._donwlaaoad_... -- typo or on purpose? :)

Done.

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
File sitescripts/oauth2dl/utils/dummy_wsgi_app.py (right):

https://codereview.adblockplus.org/29833582/diff/29833583/sitescripts/oauth2d...
sitescripts/oauth2dl/utils/dummy_wsgi_app.py:1: # This file is part of the
Adblock Plus web scripts,
On 2018/07/20 21:00:01, Vasily Kuznetsov wrote:
> I would just move this into the test folder. Or is this used for anything
> outside of the tests?

Done.

https://codereview.adblockplus.org/29833582/diff/29833583/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29833582/diff/29833583/tox.ini#newcode80
tox.ini:80: /sitescripts/oauth2dl/bin/oauth2dl.py: D413, E501, A301
On 2018/07/20 21:00:01, Vasily Kuznetsov wrote:
> It would be better not to add exceptions for new code. Is any of this really
> necessary or can we adjust the code to make flake8 happy?

Done.

https://codereview.adblockplus.org/29833582/diff/29833583/tox.ini#newcode102
tox.ini:102: httplib2
On 2018/07/20 21:00:01, Vasily Kuznetsov wrote:
> Do we need httplib2 for the new script? Do Google libraries require it or
what's
> the reason?

Done.

https://codereview.adblockplus.org/29833582/diff/29833583/tox.ini#newcode123
tox.ini:123: sitescripts/oauth2dl/test/test_oauth2dl.py \
On 2018/07/20 21:00:01, Vasily Kuznetsov wrote:
> Would be neater to keep consistent indentation.

Done.

[Vasily Kuznetsov, 2018-08-01 10:52:52]

Hi Tudor,

Looks good, it's pretty much done, except for .gitignore change (see comment
below).

Also I'm not sure if we should really gitignore .DS_Storage -- it's a bit too
Mac-specific. Anyway, if you choose to do it, please also ignore it in .hgignore
so that the two files stay in sync.

Cheers,.
Vasily

https://codereview.adblockplus.org/29833582/diff/29843555/.gitignore
File .gitignore (left):

https://codereview.adblockplus.org/29833582/diff/29843555/.gitignore#oldcode10
.gitignore:10: .Python
This .Python line is still coming from somewhere and it causes the patch
application to fail. It doesn't seem to be in the original either at
hg.adblockplus.org or at
https://github.com/adblockplus/sitescripts/blob/master/.gitignore, which I
suppose you're using. Have you checked where it's coming from?

[Tudor Avram, 2018-08-01 12:40:17]

Hi Vasily, 

The .Python entry was in my master branch for some reason, that's why it still
appeared in the diffs. No it's all fixed.

I kept .DS_Store (and also added it to .hgignore) because it just kept on
uploading it with the patch. Hope that's alright with you. If not, I can try to
look for an alternative to this.

Thanks, 
Tudor.

https://codereview.adblockplus.org/29833582/diff/29843555/.gitignore
File .gitignore (left):

https://codereview.adblockplus.org/29833582/diff/29843555/.gitignore#oldcode10
.gitignore:10: .Python
On 2018/08/01 10:52:51, Vasily Kuznetsov wrote:
> This .Python line is still coming from somewhere and it causes the patch
> application to fail. It doesn't seem to be in the original either at
> http://hg.adblockplus.org or at
> https://github.com/adblockplus/sitescripts/blob/master/.gitignore, which I
> suppose you're using. Have you checked where it's coming from?

Yeah... It was on my 'master' locally for some reason, so when I was uploading
the patch it got there.

[Vasily Kuznetsov, 2018-08-02 10:10:07]

LGTM