Issue 463: Provide a signout\logout method

Issue 463: Provide a signout\logout method

[peleyal, 2014-05-12 14:12:23]

Hi Gus,
This is a new feature for 1.8.2.
It includes a code for revoking an access token in all the platforms.

Thanks,
Eyal

[class, 2014-05-12 18:00:54]

LGTM, a few comments on comments...

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth.DotNet...
File Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleWebAuthorizationBroker.cs (left):

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth.DotNet...
Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleWebAuthorizationBroker.cs:87: ///
<param name="scopes">
Add description and closing tag on the scopes param.

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth.WP/OAu...
File Src/GoogleApis.Auth.WP/OAuth2/AuthorizationCodeWPInstalledApp.cs (right):

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth.WP/OAu...
Src/GoogleApis.Auth.WP/OAuth2/AuthorizationCodeWPInstalledApp.cs:49: ///
<summary>Constructs a new authorization code installed application for
WP.</summary>
Is WP Windows Phone?

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth.WP/OAu...
File Src/GoogleApis.Auth.WP/OAuth2/GoogleWebAuthorizationBroker.cs (right):

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth.WP/OAu...
Src/GoogleApis.Auth.WP/OAuth2/GoogleWebAuthorizationBroker.cs:79: ///
Reauthorizes asynchronously the user. This method should be called if the user
wants to reauthorize after
Can you reverse the phrase to, "Asynchronously authorizes the user..." or
"Reauthorizes the user asynchronously."  Also, try to avoid gender in pronouns:

"This method should be called if the user wants to authorize after he or she
revoked the token."
or
"This method should be called if the user wants to authorize after they revoked
the token."

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth.WinRT/...
File Src/GoogleApis.Auth.WinRT/OAuth2/AuthorizationCodeWinRTInstalledApp.cs
(right):

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth.WinRT/...
Src/GoogleApis.Auth.WinRT/OAuth2/AuthorizationCodeWinRTInstalledApp.cs:44: ///
<summary>Constructs a new authorization code installed application for Windows
Store.</summary>
Why is this for the Windows Store and not the Google OAuth server?

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth.WinRT/...
File Src/GoogleApis.Auth.WinRT/OAuth2/GoogleWebAuthorizationBroker.cs (right):

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth.WinRT/...
Src/GoogleApis.Auth.WinRT/OAuth2/GoogleWebAuthorizationBroker.cs:77: ///
Reauthorizes asynchronously the user. This method should be called if the user
wants to reauthorize after
Same as before:

"Asynchronously reauthorizes the user. Call this method if the user wants to
reauthorize after they revoked the token."

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth.WinRT/...
Src/GoogleApis.Auth.WinRT/OAuth2/GoogleWebAuthorizationBroker.cs:81: /// will be
updated.
Put the closing </param> tag after the text "will be updated"

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
File Src/GoogleApis.Auth/OAuth2/Flows/AuthorizationCodeFlow.cs (right):

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/Flows/AuthorizationCodeFlow.cs:267: public virtual
Task RevokeTokenAsync(string userId, string token, CancellationToken
taskCancellationToken)
Please add XML comment.

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/Flows/AuthorizationCodeFlow.cs:267: public virtual
Task RevokeTokenAsync(string userId, string token, CancellationToken
taskCancellationToken)
Are we implementing token revocation in the library? I think this would be
helpful for developers along with token verification.

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/Flows/AuthorizationCodeFlow.cs:269: throw new
NotImplementedException("The pure OAuth 2.0 protocol doesn't support revoking a
token");
Add a period and there is no reason to say pure.  Just "The OAuth v2 protocol
does not support token revocation."

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
File Src/GoogleApis.Auth/OAuth2/Flows/GoogleAuthorizationCodeFlow.cs (right):

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/Flows/GoogleAuthorizationCodeFlow.cs:35: ///
<summary>Gets the revoke token URL.</summary>
"Gets the token revocation URL."

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/Flows/GoogleAuthorizationCodeFlow.cs:55: public
override async Task RevokeTokenAsync(string userId, string token,
Awesome, we're adding revocation.

Add an XML comment above with params and a summary. In the summary make sure to
say something along the lines "Disconnects the user's account from the OAuth 2.0
application. This method should be called upon removing accounts from sites." in
the summary. The most important parameter on this method is the userId make sure
to inform the developer where this comes from.

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
File Src/GoogleApis.Auth/OAuth2/Flows/IAuthorizationCodeFlow.cs (right):

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/Flows/IAuthorizationCodeFlow.cs:73: /// If revoking
the token have been succeeded, the user's credential is removed from the data
store.
If revoking the token succeeds, the user's credential is removed from the data
store and the user must authorize again before your application can access that
user's resources made available to your application.

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
File Src/GoogleApis.Auth/OAuth2/GoogleConsts.cs (right):

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/GoogleConsts.cs:31: /// <summary>The revoke access
token URL.</summary>
The path to the Google revocation endpoint.

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
File Src/GoogleApis.Auth/OAuth2/Requests/GoogleRevokeTokenRequest.cs (right):

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/Requests/GoogleRevokeTokenRequest.cs:31: ///
<summary>Gets the revoke token URI.</summary>
(Optional) Gets the URI for token revocation.

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
File Src/GoogleApis.Auth/OAuth2/UserCredential.cs (right):

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/UserCredential.cs:169: /// Then it updates the <see
cref="TokenResponse"/> to <code>null</code>.
Revokes the token by calling <> and updating the <> to null.

[peleyal, 2014-05-12 20:26:34]

I'm not submitting before getting your final approval.

Thanks for all the comments, see you soon in SF :)

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth.DotNet...
File Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleWebAuthorizationBroker.cs (left):

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth.DotNet...
Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleWebAuthorizationBroker.cs:87: ///
<param name="scopes">
We have one in line 89. Right?
Am I missing something?
On 2014/05/12 18:00:53, class wrote:
> Add description and closing tag on the scopes param.

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth.WP/OAu...
File Src/GoogleApis.Auth.WP/OAuth2/AuthorizationCodeWPInstalledApp.cs (right):

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth.WP/OAu...
Src/GoogleApis.Auth.WP/OAuth2/AuthorizationCodeWPInstalledApp.cs:49: ///
<summary>Constructs a new authorization code installed application for
WP.</summary>
On 2014/05/12 18:00:53, class wrote:
> Is WP Windows Phone?

Done.

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth.WP/OAu...
File Src/GoogleApis.Auth.WP/OAuth2/GoogleWebAuthorizationBroker.cs (right):

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth.WP/OAu...
Src/GoogleApis.Auth.WP/OAuth2/GoogleWebAuthorizationBroker.cs:79: ///
Reauthorizes asynchronously the user. This method should be called if the user
wants to reauthorize after
On 2014/05/12 18:00:53, class wrote:
> Can you reverse the phrase to, "Asynchronously authorizes the user..." or
> "Reauthorizes the user asynchronously."  Also, try to avoid gender in
pronouns:
> 
> "This method should be called if the user wants to authorize after he or she
> revoked the token."
> or
> "This method should be called if the user wants to authorize after they
revoked
> the token."

Done.

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth.WinRT/...
File Src/GoogleApis.Auth.WinRT/OAuth2/AuthorizationCodeWinRTInstalledApp.cs
(right):

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth.WinRT/...
Src/GoogleApis.Auth.WinRT/OAuth2/AuthorizationCodeWinRTInstalledApp.cs:44: ///
<summary>Constructs a new authorization code installed application for Windows
Store.</summary>
I'm not sure I'm getting you.
I've a similar one to Windows Phone. this class is for Windows Store.
Does that make sense? If not let me know what confuses you, and I'll fix it.

On 2014/05/12 18:00:53, class wrote:
> Why is this for the Windows Store and not the Google OAuth server?

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth.WinRT/...
File Src/GoogleApis.Auth.WinRT/OAuth2/GoogleWebAuthorizationBroker.cs (right):

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth.WinRT/...
Src/GoogleApis.Auth.WinRT/OAuth2/GoogleWebAuthorizationBroker.cs:77: ///
Reauthorizes asynchronously the user. This method should be called if the user
wants to reauthorize after
On 2014/05/12 18:00:53, class wrote:
> Same as before:
> 
> "Asynchronously reauthorizes the user. Call this method if the user wants to
> reauthorize after they revoked the token."
> 

Done.

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth.WinRT/...
Src/GoogleApis.Auth.WinRT/OAuth2/GoogleWebAuthorizationBroker.cs:81: /// will be
updated.
On 2014/05/12 18:00:53, class wrote:
> Put the closing </param> tag after the text "will be updated"

Done.

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
File Src/GoogleApis.Auth/OAuth2/Flows/AuthorizationCodeFlow.cs (right):

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/Flows/AuthorizationCodeFlow.cs:267: public virtual
Task RevokeTokenAsync(string userId, string token, CancellationToken
taskCancellationToken)
Yes Yes Yes.
I want us to have token verification as well, but for now we adding revoking...
On 2014/05/12 18:00:53, class wrote:
> Are we implementing token revocation in the library? I think this would be
> helpful for developers along with token verification.

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/Flows/AuthorizationCodeFlow.cs:267: public virtual
Task RevokeTokenAsync(string userId, string token, CancellationToken
taskCancellationToken)
the XML comment in its interface (like the rest of the methods)
On 2014/05/12 18:00:53, class wrote:
> Please add XML comment.

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/Flows/AuthorizationCodeFlow.cs:269: throw new
NotImplementedException("The pure OAuth 2.0 protocol doesn't support revoking a
token");
On 2014/05/12 18:00:53, class wrote:
> Add a period and there is no reason to say pure.  Just "The OAuth v2 protocol
> does not support token revocation."

Done.

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
File Src/GoogleApis.Auth/OAuth2/Flows/GoogleAuthorizationCodeFlow.cs (right):

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/Flows/GoogleAuthorizationCodeFlow.cs:35: ///
<summary>Gets the revoke token URL.</summary>
On 2014/05/12 18:00:53, class wrote:
> "Gets the token revocation URL."

Done.

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/Flows/GoogleAuthorizationCodeFlow.cs:55: public
override async Task RevokeTokenAsync(string userId, string token,
in the interface.
Let me know if it's good enough :)
On 2014/05/12 18:00:53, class wrote:
> Awesome, we're adding revocation.
> 
> Add an XML comment above with params and a summary. In the summary make sure
to
> say something along the lines "Disconnects the user's account from the OAuth
2.0
> application. This method should be called upon removing accounts from sites."
in
> the summary. The most important parameter on this method is the userId make
sure
> to inform the developer where this comes from.

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
File Src/GoogleApis.Auth/OAuth2/Flows/IAuthorizationCodeFlow.cs (right):

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/Flows/IAuthorizationCodeFlow.cs:73: /// If revoking
the token have been succeeded, the user's credential is removed from the data
store.
Let me know, it is OK
On 2014/05/12 18:00:53, class wrote:
> If revoking the token succeeds, the user's credential is removed from the data
> store and the user must authorize again before your application can access
that
> user's resources made available to your application.

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
File Src/GoogleApis.Auth/OAuth2/GoogleConsts.cs (right):

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/GoogleConsts.cs:31: /// <summary>The revoke access
token URL.</summary>
On 2014/05/12 18:00:53, class wrote:
> The path to the Google revocation endpoint.

Done.

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
File Src/GoogleApis.Auth/OAuth2/Requests/GoogleRevokeTokenRequest.cs (right):

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/Requests/GoogleRevokeTokenRequest.cs:31: ///
<summary>Gets the revoke token URI.</summary>
On 2014/05/12 18:00:53, class wrote:
> (Optional) Gets the URI for token revocation.

Done.

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
File Src/GoogleApis.Auth/OAuth2/UserCredential.cs (right):

https://codereview.appspot.com/94340043/diff/70001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/UserCredential.cs:169: /// Then it updates the <see
cref="TokenResponse"/> to <code>null</code>.
On 2014/05/12 18:00:53, class wrote:
> Revokes the token by calling <> and updating the <> to null.

Done.

[class, 2014-05-12 22:54:44]

Two small typos and you're good to go.

https://codereview.appspot.com/94340043/diff/50031/Src/GoogleApis.Auth.WP/OAu...
File Src/GoogleApis.Auth.WP/OAuth2/AuthorizationCodeWPInstalledApp.cs (right):

https://codereview.appspot.com/94340043/diff/50031/Src/GoogleApis.Auth.WP/OAu...
Src/GoogleApis.Auth.WP/OAuth2/AuthorizationCodeWPInstalledApp.cs:41: ///
<summary>Constructs a new authorization code installed application for Window
Phone.</summary>
Windows Phone (missing the S)

https://codereview.appspot.com/94340043/diff/50031/Src/GoogleApis.Auth.WP/OAu...
Src/GoogleApis.Auth.WP/OAuth2/AuthorizationCodeWPInstalledApp.cs:49: ///
<summary>Constructs a new authorization code installed application for Windows
Phone.</summary>
Please update the summary, it's not clear what this does still. 

My first guess is "Authorizes the app using an authorization code flow,
targeting an installed app."

[peleyal, 2014-05-14 15:45:24]

Thanks. Submitting now

https://codereview.appspot.com/94340043/diff/50031/Src/GoogleApis.Auth.WP/OAu...
File Src/GoogleApis.Auth.WP/OAuth2/AuthorizationCodeWPInstalledApp.cs (right):

https://codereview.appspot.com/94340043/diff/50031/Src/GoogleApis.Auth.WP/OAu...
Src/GoogleApis.Auth.WP/OAuth2/AuthorizationCodeWPInstalledApp.cs:41: ///
<summary>Constructs a new authorization code installed application for Window
Phone.</summary>
On 2014/05/12 22:54:44, class wrote:
> Windows Phone (missing the S)

Done.

https://codereview.appspot.com/94340043/diff/50031/Src/GoogleApis.Auth.WP/OAu...
Src/GoogleApis.Auth.WP/OAuth2/AuthorizationCodeWPInstalledApp.cs:49: ///
<summary>Constructs a new authorization code installed application for Windows
Phone.</summary>
On 2014/05/12 22:54:44, class wrote:
> Please update the summary, it's not clear what this does still. 
> 
> My first guess is "Authorizes the app using an authorization code flow,
> targeting an installed app."

Done.