https://codereview.appspot.com/7033052/diff/21011/oauth2client/client.py File oauth2client/client.py (right): https://codereview.appspot.com/7033052/diff/21011/oauth2client/client.py#newcode877 oauth2client/client.py:877: raise NotImplementedError If I just got a plain NotImplementedError ...
12 years, 6 months ago
(2013-01-09 14:05:36 UTC)
#6
https://codereview.appspot.com/7033052/diff/21011/oauth2client/client.py File oauth2client/client.py (right): https://codereview.appspot.com/7033052/diff/21011/oauth2client/client.py#newcode877 oauth2client/client.py:877: raise NotImplementedError I'm fine adding a message but didn't ...
12 years, 6 months ago
(2013-01-09 16:59:25 UTC)
#7
https://codereview.appspot.com/7033052/diff/21011/oauth2client/client.py
File oauth2client/client.py (right):
https://codereview.appspot.com/7033052/diff/21011/oauth2client/client.py#newc...
oauth2client/client.py:877: raise NotImplementedError
I'm fine adding a message but didn't want to add one that is incorrect.
I'm unclear on whether or not assertion credentials can be revoked and don't
have an account handy to test this.
If they can be revoked via the APIs console, they can likely be revoked via
standard OAuth 2.0 revoke, in which case, we can just implement this. It's
simply a matter of determining which token value needs to be sent to the revoke
endpoint (for the Bearer case both access and refresh will revoke and revoking
the access token makes the refresh token stop working).
On 2013/01/09 14:05:36, jcgregorio_google wrote:
> If I just got a plain NotImplementedError I might presume a bug in the
library,
> can we add an informational message that assertion credentials can't be
> programatically revoked and that needs to be done through the APIs Console?
https://codereview.appspot.com/7033052/diff/21011/tests/test_oauth2client.py
File tests/test_oauth2client.py (right):
https://codereview.appspot.com/7033052/diff/21011/tests/test_oauth2client.py#...
tests/test_oauth2client.py:168: _token_revoke_test_helper(self, '200', False,
True, 'refresh_token')
On 2013/01/09 14:05:36, jcgregorio_google wrote:
> This might be easier to read if you add the param names:
>
> revoke_raise=False, valid_bool_value=True
>
Done.
https://codereview.appspot.com/7033052/diff/21011/tests/test_oauth2client.py#...
tests/test_oauth2client.py:302: hardcoded_update = uri + '&a=b&c=d%26'
Thanks. I thought I had seen that but couldn't find it before.
On 2013/01/09 14:05:36, jcgregorio_google wrote:
> You can use test_discovery.assertUrisEqual() here.
https://codereview.appspot.com/7033052/diff/21011/oauth2client/client.py File oauth2client/client.py (right): https://codereview.appspot.com/7033052/diff/21011/oauth2client/client.py#newcode877 oauth2client/client.py:877: raise NotImplementedError OK, then find out if they can ...
12 years, 6 months ago
(2013-01-09 17:05:47 UTC)
#8
https://codereview.appspot.com/7033052/diff/21011/oauth2client/client.py
File oauth2client/client.py (right):
https://codereview.appspot.com/7033052/diff/21011/oauth2client/client.py#newc...
oauth2client/client.py:877: raise NotImplementedError
OK, then find out if they can be revoked programatically and if they can then
implement this function. If you find out they can't be revoked then add the info
to the exception.
On 2013/01/09 16:59:25, dhermes wrote:
> I'm fine adding a message but didn't want to add one that is incorrect.
>
> I'm unclear on whether or not assertion credentials can be revoked and don't
> have an account handy to test this.
>
> If they can be revoked via the APIs console, they can likely be revoked via
> standard OAuth 2.0 revoke, in which case, we can just implement this. It's
> simply a matter of determining which token value needs to be sent to the
revoke
> endpoint (for the Bearer case both access and refresh will revoke and revoking
> the access token makes the refresh token stop working).
>
> On 2013/01/09 14:05:36, jcgregorio_google wrote:
> > If I just got a plain NotImplementedError I might presume a bug in the
> library,
> > can we add an informational message that assertion credentials can't be
> > programatically revoked and that needs to be done through the APIs Console?
>
https://codereview.appspot.com/7033052/diff/21011/oauth2client/client.py File oauth2client/client.py (right): https://codereview.appspot.com/7033052/diff/21011/oauth2client/client.py#newcode877 oauth2client/client.py:877: raise NotImplementedError Cool. Do you have a test account ...
12 years, 6 months ago
(2013-01-09 17:06:59 UTC)
#9
https://codereview.appspot.com/7033052/diff/21011/oauth2client/client.py
File oauth2client/client.py (right):
https://codereview.appspot.com/7033052/diff/21011/oauth2client/client.py#newc...
oauth2client/client.py:877: raise NotImplementedError
Cool. Do you have a test account I could piggyback on or have any pointers on a
quick way to create one?
On 2013/01/09 17:05:47, jcgregorio_google wrote:
> OK, then find out if they can be revoked programatically and if they can then
> implement this function. If you find out they can't be revoked then add the
info
> to the exception.
>
> On 2013/01/09 16:59:25, dhermes wrote:
> > I'm fine adding a message but didn't want to add one that is incorrect.
> >
> > I'm unclear on whether or not assertion credentials can be revoked and don't
> > have an account handy to test this.
> >
> > If they can be revoked via the APIs console, they can likely be revoked via
> > standard OAuth 2.0 revoke, in which case, we can just implement this. It's
> > simply a matter of determining which token value needs to be sent to the
> revoke
> > endpoint (for the Bearer case both access and refresh will revoke and
revoking
> > the access token makes the refresh token stop working).
> >
> > On 2013/01/09 14:05:36, jcgregorio_google wrote:
> > > If I just got a plain NotImplementedError I might presume a bug in the
> > library,
> > > can we add an informational message that assertion credentials can't be
> > > programatically revoked and that needs to be done through the APIs
Console?
> >
>
I verified this works (using an access token for revocation). Updated the code minimally to ...
12 years, 6 months ago
(2013-01-22 01:38:10 UTC)
#10
I verified this works (using an access token for revocation).
Updated the code minimally to reflect this.
I didn't add revoke_uri as a kwarg to any of the subclasses since they
don't seem to want any of these notions (such as the token uri).
On Wed, Jan 9, 2013 at 9:07 AM, <dhermes@google.com> wrote:
>
> https://codereview.appspot.**com/7033052/diff/21011/**
>
oauth2client/client.py<https://codereview.appspot.com/7033052/diff/21011/oauth2client/client.py>
> File oauth2client/client.py (right):
>
> https://codereview.appspot.**com/7033052/diff/21011/**
>
oauth2client/client.py#**newcode877<https://codereview.appspot.com/7033052/diff/21011/oauth2client/client.py#newcode877>
> oauth2client/client.py:877: raise NotImplementedError
> Cool. Do you have a test account I could piggyback on or have any
> pointers on a quick way to create one?
>
>
> On 2013/01/09 17:05:47, jcgregorio_google wrote:
>
>> OK, then find out if they can be revoked programatically and if they
>>
> can then
>
>> implement this function. If you find out they can't be revoked then
>>
> add the info
>
>> to the exception.
>>
>
> On 2013/01/09 16:59:25, dhermes wrote:
>> > I'm fine adding a message but didn't want to add one that is
>>
> incorrect.
>
>> >
>> > I'm unclear on whether or not assertion credentials can be revoked
>>
> and don't
>
>> > have an account handy to test this.
>> >
>> > If they can be revoked via the APIs console, they can likely be
>>
> revoked via
>
>> > standard OAuth 2.0 revoke, in which case, we can just implement
>>
> this. It's
>
>> > simply a matter of determining which token value needs to be sent to
>>
> the
>
>> revoke
>> > endpoint (for the Bearer case both access and refresh will revoke
>>
> and revoking
>
>> > the access token makes the refresh token stop working).
>> >
>> > On 2013/01/09 14:05:36, jcgregorio_google wrote:
>> > > If I just got a plain NotImplementedError I might presume a bug in
>>
> the
>
>> > library,
>> > > can we add an informational message that assertion credentials
>>
> can't be
>
>> > > programatically revoked and that needs to be done through the APIs
>>
> Console?
>
>> >
>>
>
>
>
https://codereview.appspot.**com/7033052/<https://codereview.appspot.com/7033...
>
--
Danny Hermes
Developer Programs Engineer
https://codereview.appspot.com/7033052/diff/38001/oauth2client/client.py File oauth2client/client.py (right): https://codereview.appspot.com/7033052/diff/38001/oauth2client/client.py#newcode712 oauth2client/client.py:712: def _do_revoke(self, http_request, token): Because some credentials classes revoke ...
12 years, 6 months ago
(2013-01-23 18:52:29 UTC)
#12
https://codereview.appspot.com/7033052/diff/38001/oauth2client/client.py
File oauth2client/client.py (right):
https://codereview.appspot.com/7033052/diff/38001/oauth2client/client.py#newc...
oauth2client/client.py:712: def _do_revoke(self, http_request, token):
Because some credentials classes revoke the refresh token and others revoke the
access token.
As it turns out, revoking the access token in cases where a refresh token was
also issued will also make that refresh token invalid, so we could just revoke
the access token every time.
On 2013/01/23 14:14:59, jcgregorio_google wrote:
> Why does _do_revoke() exist? Why no just push the code of _do_revoke() into
> _revoke()?
https://codereview.appspot.com/7033052/diff/38001/oauth2client/client.py#newc...
oauth2client/client.py:724: logger.info('Revoking token: %s', token)
On 2013/01/23 14:14:59, jcgregorio_google wrote:
> The token is sensitive information and shouldn't be logged.
>
> We are deleting it here, but if the delete fails then we have a good token in
> the logs.
Done.
Issue 7033052: Adding a .revoke() to Credentials.
(Closed)
Created 12 years, 6 months ago by dhermes
Modified 12 years, 5 months ago
Reviewers: jcgregorio_google
Base URL:
Comments: 16
