Adding a sample CWS license server client written for Python App Engine. (issue3161030)

[kur...@chromium.org]

Reviewers: kathyw, ericbidelman, bkennish, munjal,

Message:
Here is my Python sample. Most of the files are library code - only main.py
and
README really need a review. If someone could leave feedback I'd
appreciate it.

Description:
Adding a sample CWS license server client written for Python App Engine.

BUG=None
TEST=None

Please review this at http://codereview.chromium.org/3161030/show

SVN Base: http://src.chromium.org/git/chromium.git

Affected files:
A chrome/common/extensions/docs/examples/apps/hello-python/NOTICE
A chrome/common/extensions/docs/examples/apps/hello-python/README
A chrome/common/extensions/docs/examples/apps/hello-python/app.yaml
A chrome/common/extensions/docs/examples/apps/hello-python/favicon.ico
A
chrome/common/extensions/docs/examples/apps/hello-python/httplib2/__init__.py
A
chrome/common/extensions/docs/examples/apps/hello-python/httplib2/iri2uri.py
A chrome/common/extensions/docs/examples/apps/hello-python/index.yaml
A chrome/common/extensions/docs/examples/apps/hello-python/main.py
A
chrome/common/extensions/docs/examples/apps/hello-python/oauth2/__init__.py
A
chrome/common/extensions/docs/examples/apps/hello-python/oauth2/clients/__init__.py
A
chrome/common/extensions/docs/examples/apps/hello-python/oauth2/clients/imap.py
A
chrome/common/extensions/docs/examples/apps/hello-python/oauth2/clients/smtp.py
A
chrome/common/extensions/docs/examples/apps/hello-python/templates/index.html

[ericbi...@chromium.org]

http://codereview.chromium.org/3161030/diff/1/9
File chrome/common/extensions/docs/examples/apps/hello-python/main.py
(right):

http://codereview.chromium.org/3161030/diff/1/9#newcode24
chrome/common/extensions/docs/examples/apps/hello-python/main.py:24:
'license_server': 'INSERT SERVER HERE',
lets put the server in

http://codereview.chromium.org/3161030/diff/1/9#newcode25
chrome/common/extensions/docs/examples/apps/hello-python/main.py:25:
'license_path': 'INSERT PATH HERE',
same

http://codereview.chromium.org/3161030/diff/1/9#newcode32
chrome/common/extensions/docs/examples/apps/hello-python/main.py:32:
CONFIG = PROD_CONFIG
Do you still need this?

http://codereview.chromium.org/3161030/diff/1/9#newcode35
chrome/common/extensions/docs/examples/apps/hello-python/main.py:35:
VALID_ACCESS_LEVELS = ['FREE_TRIAL', 'FULL']
If the API adds more (or changes) you're kinda screwed here.

http://codereview.chromium.org/3161030/diff/1/9#newcode38
chrome/common/extensions/docs/examples/apps/hello-python/main.py:38: """
Fetches the license for a given user by making an OAuth signed request
No space after the """ in your docstrings

http://codereview.chromium.org/3161030/diff/1/9#newcode65
chrome/common/extensions/docs/examples/apps/hello-python/main.py:65:
resp, content = client.request(url, "GET")
single quotes

http://codereview.chromium.org/3161030/diff/1/9#newcode78
chrome/common/extensions/docs/examples/apps/hello-python/main.py:78:
message: A descriptive message if error is True
True.

http://codereview.chromium.org/3161030/diff/1/9#newcode81
chrome/common/extensions/docs/examples/apps/hello-python/main.py:81:
license = {'error': False, 'message':'', 'access':'NO'}
'access': 'NO'

http://codereview.chromium.org/3161030/diff/1/9#newcode100
chrome/common/extensions/docs/examples/apps/hello-python/main.py:100:
elif json['result'] == 'YES' and json['accessLevel'] in
VALID_ACCESS_LEVELS:
same comment

http://codereview.chromium.org/3161030/diff/1/9#newcode128
chrome/common/extensions/docs/examples/apps/hello-python/main.py:128:
'user_login': login_url,
remove trailing ','

http://codereview.chromium.org/3161030/show

[kat...@chromium.org]

A couple of comments on the README.

http://codereview.chromium.org/3161030/diff/1/3
File chrome/common/extensions/docs/examples/apps/hello-python/README
(right):

http://codereview.chromium.org/3161030/diff/1/3#newcode6
chrome/common/extensions/docs/examples/apps/hello-python/README:6: This
application implements a sample client for the Chrome Web Store
licensing
The doc uses "Licensing API" or "license server".

Either way, it'd be good to mention the string [Chrome Web Store]
"Licensing API" somewhere in this README.

Also, please refer to the doc. http://code.google.com/chrome/webstore/
would do.

http://codereview.chromium.org/3161030/show

[kur...@chromium.org]

Thanks for the review. Specific comments inline:

http://codereview.chromium.org/3161030/diff/1/9
File chrome/common/extensions/docs/examples/apps/hello-python/main.py
(right):

http://codereview.chromium.org/3161030/diff/1/9#newcode24
chrome/common/extensions/docs/examples/apps/hello-python/main.py:24:
'license_server': 'INSERT SERVER HERE',

On 2010/08/19 21:24:24, ericbidelman wrote:
> lets put the server in

Done.

http://codereview.chromium.org/3161030/diff/1/9#newcode25
chrome/common/extensions/docs/examples/apps/hello-python/main.py:25:
'license_path': 'INSERT PATH HERE',

On 2010/08/19 21:24:24, ericbidelman wrote:
> same

Done.

http://codereview.chromium.org/3161030/diff/1/9#newcode32
chrome/common/extensions/docs/examples/apps/hello-python/main.py:32:
CONFIG = PROD_CONFIG

On 2010/08/19 21:24:24, ericbidelman wrote:
> Do you still need this?

Done.

http://codereview.chromium.org/3161030/diff/1/9#newcode35
chrome/common/extensions/docs/examples/apps/hello-python/main.py:35:
VALID_ACCESS_LEVELS = ['FREE_TRIAL', 'FULL']

On 2010/08/19 21:24:24, ericbidelman wrote:
> If the API adds more (or changes) you're kinda screwed here.

How so? These are the only two responses I support - don't want to
return anything other than these two - everything else defaults to no
access. That way if the API changes, my code will just return "NO"
until I support any new options or parse the new format.

http://codereview.chromium.org/3161030/diff/1/9#newcode38
chrome/common/extensions/docs/examples/apps/hello-python/main.py:38: """
Fetches the license for a given user by making an OAuth signed request

On 2010/08/19 21:24:24, ericbidelman wrote:
> No space after the """ in your docstrings

Done.

http://codereview.chromium.org/3161030/diff/1/9#newcode65
chrome/common/extensions/docs/examples/apps/hello-python/main.py:65:
resp, content = client.request(url, "GET")

On 2010/08/19 21:24:24, ericbidelman wrote:
> single quotes

Done.

http://codereview.chromium.org/3161030/diff/1/9#newcode78
chrome/common/extensions/docs/examples/apps/hello-python/main.py:78:
message: A descriptive message if error is True

On 2010/08/19 21:24:24, ericbidelman wrote:
> True.

Done.

http://codereview.chromium.org/3161030/diff/1/9#newcode81
chrome/common/extensions/docs/examples/apps/hello-python/main.py:81:
license = {'error': False, 'message':'', 'access':'NO'}

On 2010/08/19 21:24:24, ericbidelman wrote:
> 'access': 'NO'

Done.

http://codereview.chromium.org/3161030/diff/1/9#newcode100
chrome/common/extensions/docs/examples/apps/hello-python/main.py:100:
elif json['result'] == 'YES' and json['accessLevel'] in
VALID_ACCESS_LEVELS:

On 2010/08/19 21:24:24, ericbidelman wrote:
> same comment

See response to comment.

http://codereview.chromium.org/3161030/diff/1/9#newcode128
chrome/common/extensions/docs/examples/apps/hello-python/main.py:128:
'user_login': login_url,

On 2010/08/19 21:24:24, ericbidelman wrote:
> remove trailing ','

I prefer not to do this - it's more likely to break if I add another
line here and forget to add the comma back in. Trailing comma is
syntactically fine here.

http://codereview.chromium.org/3161030/show

[kur...@chromium.org]

Thanks, updated the README.

http://codereview.chromium.org/3161030/diff/1/3
File chrome/common/extensions/docs/examples/apps/hello-python/README
(right):

http://codereview.chromium.org/3161030/diff/1/3#newcode6
chrome/common/extensions/docs/examples/apps/hello-python/README:6: This
application implements a sample client for the Chrome Web Store
licensing

On 2010/08/19 22:05:57, kathyw wrote:
> The doc uses "Licensing API" or "license server".

> Either way, it'd be good to mention the string [Chrome Web Store]
"Licensing
> API" somewhere in this README.

> Also, please refer to the doc. http://code.google.com/chrome/webstore/
would do.

Done.

http://codereview.chromium.org/3161030/show

[ericbi...@chromium.org]

I didn't see a delta 2 on this, but I assume you've made the changes.

LGTM

http://codereview.chromium.org/3161030/diff/1/9
File chrome/common/extensions/docs/examples/apps/hello-python/main.py
(right):

http://codereview.chromium.org/3161030/diff/1/9#newcode35

chrome/common/extensions/docs/examples/apps/hello-python/main.py:35:
VALID_ACCESS_LEVELS = ['FREE_TRIAL', 'FULL']

Just thinking it will save you less overhead of changing this sample
should the server responses change. It's OK to keep what you have an
default to NO for unrecognized values.

On 2010/08/19 22:45:33, kurrik.chromium wrote:
> On 2010/08/19 21:24:24, ericbidelman wrote:
> > If the API adds more (or changes) you're kinda screwed here.

> How so? These are the only two responses I support - don't want to
return
> anything other than these two - everything else defaults to no access.
That way
> if the API changes, my code will just return "NO" until I support any
new
> options or parse the new format.

http://codereview.chromium.org/3161030/diff/1/9#newcode128
chrome/common/extensions/docs/examples/apps/hello-python/main.py:128:
'user_login': login_url,
OK

[bken...@chromium.org]

http://codereview.chromium.org/3161030/diff/1/9
File chrome/common/extensions/docs/examples/apps/hello-python/main.py
(right):

http://codereview.chromium.org/3161030/diff/1/9#newcode114
chrome/common/extensions/docs/examples/apps/hello-python/main.py:114:
userid = user.federated_identity() or user.user_id()
I don't think falling back to the Google account ID (or any other value)
is a good practice as doing so could mask a set of nasty bugs in
production code. An app that fails to acquire a user's federated
identity will proceed with the fallback value unaware anything is wrong.

Instead, I think we should change our doc to insist on deploying to test
an app's login flow.

http://codereview.chromium.org/3161030/show

[kur...@chromium.org]

http://codereview.chromium.org/3161030/diff/1/9
File chrome/common/extensions/docs/examples/apps/hello-python/main.py
(right):

http://codereview.chromium.org/3161030/diff/1/9#newcode114
chrome/common/extensions/docs/examples/apps/hello-python/main.py:114:
userid = user.federated_identity() or user.user_id()

I don't like the idea of not making this work at all on Dev, so I put in
a more sophisticated check to see what environment the app is currently
running in.

In Prod, it will just use the value of federated_identity(). If the
user forgot to configure their app for federated login, this will throw
an error.

In Dev, we just use a garbage OpenID which will always return "NO" from
the dev server. This value is made visible to the end user, so they
should see what is going on. If they want to test different responses,
they can change this response by adding "-yes" or "-trial" to the end of
the ID.

Hopefully this solution is agreeable.

[Brian Kennish]

On Thu, Aug 19, 2010 at 6:35 PM, <kur...@chromium.org> wrote:
> I don't like the idea of not making this work at all on Dev, so I put in
> a more sophisticated check to see what environment the app is currently
> running in.

Looks good. Will add similar to the Java sample.