OAuth 2 Decorator

[jcgregorio, 2011-05-20 20:50:55]

Ignore the files in new_project_template, they are just there to show how the
decorators are used. They will not be checked in, but parts of them will appear
in the documentation.

http://codereview.appspot.com/4524063/diff/2001/runtests.py
File runtests.py (right):

http://codereview.appspot.com/4524063/diff/2001/runtests.py#newcode9
runtests.py:9: APP_ENGINE_PATH='../google_appengine'
Currently hardcoded. I will submit a separate CL where all of this is moved over
to gflags.

[proppy, 2011-05-20 21:25:39]

http://codereview.appspot.com/4524063/diff/2001/oauth2client/appengine.py
File oauth2client/appengine.py (right):

http://codereview.appspot.com/4524063/diff/2001/oauth2client/appengine.py#new...
oauth2client/appengine.py:230: if not self.credentials or
self.credentials.invalid:
Consider using if not self.has_credentials() ?

http://codereview.appspot.com/4524063/diff/2001/oauth2client/appengine.py#new...
oauth2client/appengine.py:233: authorize_url =
self.flow.step1_get_authorize_url(callback)
Consider using self.authorized_url() ?

http://codereview.appspot.com/4524063/diff/2001/oauth2client/appengine.py#new...
oauth2client/appengine.py:236: memcache.set(user.user_id(),
pickle.dumps(self.flow),
You might consider storing the flow in datastore at init, since all its values
do not change.

http://codereview.appspot.com/4524063/diff/2001/oauth2client/appengine.py#new...
oauth2client/appengine.py:239: method(request_handler, *args)
Consider handling AccessTokenRefreshError and redirect to authorize_url if
caught.

http://codereview.appspot.com/4524063/diff/2001/oauth2client/appengine.py#new...
oauth2client/appengine.py:307: flow = pickle.loads(memcache.get(user.user_id(),
namespace=OAUTH2CLIENT_NAMESPACE))
You might consider getting the flow values from the datastore and reconstruct it
here.

You just need to assign the redirect_url again.

[proppy, 2011-05-20 23:59:36]

http://codereview.appspot.com/4524063/diff/2001/oauth2client/appengine.py
File oauth2client/appengine.py (right):

http://codereview.appspot.com/4524063/diff/2001/oauth2client/appengine.py#new...
oauth2client/appengine.py:159: class CredentialsModel(db.Model):
Consider adding non-opaque model for credentials.

http://codereview.appspot.com/4524063/diff/2001/oauth2client/appengine.py#new...
oauth2client/appengine.py:242: def oauth_aware(self, method):
You might consider adding a method like has_current_user_granted() that check if
credentials exists instead of another decorator.

http://codereview.appspot.com/4524063/diff/2001/oauth2client/appengine.py#new...
oauth2client/appengine.py:266: """True if the logged in there are valid access
Credentials.
+user

http://codereview.appspot.com/4524063/diff/2001/oauth2client/appengine.py#new...
oauth2client/appengine.py:273: def authorize_url(self):
You might consider adding handler param to authorize_url so it could be called
in undecorated handler.

[jcgregorio, 2011-05-23 03:16:47]

http://codereview.appspot.com/4524063/diff/2001/oauth2client/appengine.py
File oauth2client/appengine.py (right):

http://codereview.appspot.com/4524063/diff/2001/oauth2client/appengine.py#new...
oauth2client/appengine.py:159: class CredentialsModel(db.Model):
On 2011/05/20 23:59:36, proppy wrote:
> Consider adding non-opaque model for credentials.

If I were to do that it would be by changing how CredentialsProperty stores
itself, which would then propagate here.

http://codereview.appspot.com/4524063/diff/2001/oauth2client/appengine.py#new...
oauth2client/appengine.py:230: if not self.credentials or
self.credentials.invalid:
On 2011/05/20 21:25:39, proppy wrote:
> Consider using if not self.has_credentials() ?

Done.

http://codereview.appspot.com/4524063/diff/2001/oauth2client/appengine.py#new...
oauth2client/appengine.py:233: authorize_url =
self.flow.step1_get_authorize_url(callback)
On 2011/05/20 21:25:39, proppy wrote:
> Consider using self.authorized_url() ?

Done.

http://codereview.appspot.com/4524063/diff/2001/oauth2client/appengine.py#new...
oauth2client/appengine.py:236: memcache.set(user.user_id(),
pickle.dumps(self.flow),
Maybe in a future CL.

On 2011/05/20 21:25:39, proppy wrote:
> You might consider storing the flow in datastore at init, since all its values
> do not change.

http://codereview.appspot.com/4524063/diff/2001/oauth2client/appengine.py#new...
oauth2client/appengine.py:239: method(request_handler, *args)
On 2011/05/20 21:25:39, proppy wrote:
> Consider handling AccessTokenRefreshError and redirect to authorize_url if
> caught.

Done.

http://codereview.appspot.com/4524063/diff/2001/oauth2client/appengine.py#new...
oauth2client/appengine.py:242: def oauth_aware(self, method):
I think the story here is that we have two convenience decorators that make it
easy to get started. At some point the use cases get complex enough that the
user should fall back to using Flows, Credentials and Storage directly.


On 2011/05/20 23:59:36, proppy wrote:
> You might consider adding a method like has_current_user_granted() that check
if
> credentials exists instead of another decorator.

http://codereview.appspot.com/4524063/diff/2001/oauth2client/appengine.py#new...
oauth2client/appengine.py:266: """True if the logged in there are valid access
Credentials.
On 2011/05/20 23:59:36, proppy wrote:
> +user

Done.

http://codereview.appspot.com/4524063/diff/2001/oauth2client/appengine.py#new...
oauth2client/appengine.py:273: def authorize_url(self):
See comment above about has_current_user_granted().

On 2011/05/20 23:59:36, proppy wrote:
> You might consider adding handler param to authorize_url so it could be called
> in undecorated handler.

http://codereview.appspot.com/4524063/diff/2001/oauth2client/appengine.py#new...
oauth2client/appengine.py:307: flow = pickle.loads(memcache.get(user.user_id(),
namespace=OAUTH2CLIENT_NAMESPACE))
The downsides to doing that are a (slow) datastore write when each instance
starts and the special handling for pulling the redirect_uri out of 'state'. I
may switch to the datastore method in the future.

On 2011/05/20 21:25:39, proppy wrote:
> You might consider getting the flow values from the datastore and reconstruct
it
> here.
> 
> You just need to assign the redirect_url again.

[proppy, 2011-05-23 06:00:17]

http://codereview.appspot.com/4524063/diff/2001/oauth2client/appengine.py
File oauth2client/appengine.py (right):

http://codereview.appspot.com/4524063/diff/2001/oauth2client/appengine.py#new...
oauth2client/appengine.py:242: def oauth_aware(self, method):
I think that we should provide an interface similar to the existing SDK helper
for login, i.e: 
- a @login_required decorator for the most common usecases
- help methods for more complex usecases: create_login_url, get_current_user

This would translate for oauth2 to:
- @oauth_required
- create_authorization_url, get_current_credentials


On 2011/05/23 03:16:47, jcgregorio wrote:
> I think the story here is that we have two convenience decorators that make it
> easy to get started. At some point the use cases get complex enough that the
> user should fall back to using Flows, Credentials and Storage directly.
> 
> 
> On 2011/05/20 23:59:36, proppy wrote:
> > You might consider adding a method like has_current_user_granted() that
check
> if
> > credentials exists instead of another decorator.
>

http://codereview.appspot.com/4524063/diff/2001/oauth2client/appengine.py#new...
oauth2client/appengine.py:273: def authorize_url(self):
See comment above about existing SDK helper for login.

On 2011/05/23 03:16:47, jcgregorio wrote:
> See comment above about has_current_user_granted().
> 
> On 2011/05/20 23:59:36, proppy wrote:
> > You might consider adding handler param to authorize_url so it could be
called
> > in undecorated handler.
>

http://codereview.appspot.com/4524063/diff/2001/oauth2client/appengine.py#new...
oauth2client/appengine.py:307: flow = pickle.loads(memcache.get(user.user_id(),
namespace=OAUTH2CLIENT_NAMESPACE))
The (slow) datastore write would only happen, if the flow is not already in
database (i.e: on the first instance starts for this application), or if some
oauth parameters changed (unlikely to happen that often).

See:
http://codereview.appspot.com/4515101/diff/11001/oauth2client/appengine.py Line
156

As for the redirect_uri, it is already hardcoded to '/oauth2callback' in the
decorator anyway, so you can just assume it is a constant.

On 2011/05/23 03:16:47, jcgregorio wrote:
> The downsides to doing that are a (slow) datastore write when each instance
> starts and the special handling for pulling the redirect_uri out of 'state'. I
> may switch to the datastore method in the future.
> 
> On 2011/05/20 21:25:39, proppy wrote:
> > You might consider getting the flow values from the datastore and
reconstruct
> it
> > here.
> > 
> > You just need to assign the redirect_url again.
>