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

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

BUG=None
TEST=None

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

[kurrik.chromium, 2010-08-19 01:25:16]

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.

[ericbidelman, 2010-08-19 21:24:24]

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 ','

[kathyw, 2010-08-19 22:05:56]

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.

[kurrik.chromium, 2010-08-19 22:45:33]

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.

[kurrik.chromium, 2010-08-19 22:48:58]

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.

[ericbidelman, 2010-08-19 22:53:26]

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

On 2010/08/19 22:45:33, kurrik.chromium wrote:
> 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.
>

[bkennish, 2010-08-19 22:58:27]

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.

[kurrik.chromium, 2010-08-20 01:35:16]

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.


On 2010/08/19 22:58:28, bkennish wrote:
> 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.

[bkennish, 2010-08-20 01:43:57]

On Thu, Aug 19, 2010 at 6:35 PM,  <kurrik@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.