oauth issues 78 & 79 update Id token to draft 27 and add IdTokenVerifier

oauth issues 78 & 79 update Id token to draft 27 and add IdTokenVerifier

oauth issue 78: Update to ID Token draft 27
https://code.google.com/p/google-oauth-java-client/issues/detail?id=78

oauth issue 79: New IdTokenVerifier class
https://code.google.com/p/google-oauth-java-client/issues/detail?id=79

[yanivi, 2013-06-11 11:52:35]

friendly ping

[peleyal, 2013-06-12 14:20:49]

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/mai...
File
google-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdToken.java
(right):

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdToken.java:30:
* href="http://openid.net/specs/openid-connect-basic-1_0-27.html#id_token">ID
Token</a>.
Maybe I'm missing something here, but why don't we declare here all the
properties whose mentioned on section "2.1.1.1.  Request Parameters", parameters
like login_hint, max_age, display, etc.

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdToken.java:152:
public static class Payload extends JsonWebToken.Payload {
Again, maybe I'm not on the same page as you, but it looks like
JsonWebToken.Payload contains fields like nbf and jti which don't exist in the
spec.

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdToken.java:200:
* Upgrade warning: in prior version 1.15 think was an {@link Object}, but
starting with version
* in prior version 1.15 the method returned an {@link...

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdToken.java:217:
* Upgrade warning: in prior version 1.15 think was an {@link Object}, but
starting with version
* in prior version 1.15 the method's parameter was {@link ...

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/mai...
File
google-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdTokenVerifier.java
(right):

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdTokenVerifier.java:115:
* </ul>
Do you in purpose don't check the SHOULD checks from 2.2.1
If so, please add a comment that the client SHOULD check the following:
 - If the ID Token contains multiple audiences, the Client SHOULD verify that an
azp Claim is present.
 - ...

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdTokenVerifier.java:143:
/** Clock. */
what is the standard here?
should we have this dumb comment or is it better to omit it?

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdTokenVerifier.java:224:
* {@link #DEFAULT_TIME_SKEW_SECONDS}).
Add: the parameter should be equal or greater then zero

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/tes...
File
google-oauth-client/src/test/java/com/google/api/client/auth/openidconnect/IdTokenVerifierTest.java
(right):

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/tes...
google-oauth-client/src/test/java/com/google/api/client/auth/openidconnect/IdTokenVerifierTest.java:36:
private static final List<String> TRUSTED_CLIENT_IDS = Arrays.asList(CLIENT_ID);
please add one more value to the list

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/tes...
google-oauth-client/src/test/java/com/google/api/client/auth/openidconnect/IdTokenVerifierTest.java:46:
return new IdToken(new Header(), payload, new byte[0], new byte[0]);
can you call "new IdToken(new Header(), payload, null, null);"?

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/tes...
google-oauth-client/src/test/java/com/google/api/client/auth/openidconnect/IdTokenVerifierTest.java:77:
IdTokenVerifier verifierFlexible = new
IdTokenVerifier.Builder().setClock(clock).build();
Please mention (for readability) that verifierFlexible suppress the checks for
Issuer and Audience.

https://codereview.appspot.com/10042045/diff/9001/pom.xml
File pom.xml (right):

https://codereview.appspot.com/10042045/diff/9001/pom.xml#newcode424
pom.xml:424: <failOnError>false</failOnError>
Please revert this file and pull latest changes.
[you already changed it in another CL (and I'm pretty sure that that CL was
already pushed)]

[yanivi, 2013-06-17 17:36:10]

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/mai...
File
google-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdToken.java
(right):

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdToken.java:30:
* href="http://openid.net/specs/openid-connect-basic-1_0-27.html#id_token">ID
Token</a>.
On 2013/06/12 14:20:49, peleyal wrote:
> Maybe I'm missing something here, but why don't we declare here all the
> properties whose mentioned on section "2.1.1.1.  Request Parameters",
parameters
> like login_hint, max_age, display, etc.

I agree that those are a good thing to implement, but for now I am just
implementing a narrower part of that spec which is just section 2.2.  Note that
those are two different sections.

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdToken.java:152:
public static class Payload extends JsonWebToken.Payload {
On 2013/06/12 14:20:49, peleyal wrote:
> Again, maybe I'm not on the same page as you, but it looks like
> JsonWebToken.Payload contains fields like nbf and jti which don't exist in the
> spec.

They are in the JWT spec.

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdToken.java:200:
* Upgrade warning: in prior version 1.15 think was an {@link Object}, but
starting with version
On 2013/06/12 14:20:49, peleyal wrote:
> * in prior version 1.15 the method returned an {@link...

Done.

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdToken.java:217:
* Upgrade warning: in prior version 1.15 think was an {@link Object}, but
starting with version
On 2013/06/12 14:20:49, peleyal wrote:
> * in prior version 1.15 the method's parameter was {@link ...

Done.

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/mai...
File
google-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdTokenVerifier.java
(right):

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdTokenVerifier.java:115:
* </ul>
On 2013/06/12 14:20:49, peleyal wrote:
> Do you in purpose don't check the SHOULD checks from 2.2.1
> If so, please add a comment that the client SHOULD check the following:
>  - If the ID Token contains multiple audiences, the Client SHOULD verify that
an
> azp Claim is present.
>  - ...

Done.  Added to the class-level JavaDoc.

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdTokenVerifier.java:143:
/** Clock. */
On 2013/06/12 14:20:49, peleyal wrote:
> what is the standard here?
> should we have this dumb comment or is it better to omit it?

dumb comment.

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdTokenVerifier.java:224:
* {@link #DEFAULT_TIME_SKEW_SECONDS}).
On 2013/06/12 14:20:49, peleyal wrote:
> Add: the parameter should be equal or greater then zero

Done.

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/tes...
File
google-oauth-client/src/test/java/com/google/api/client/auth/openidconnect/IdTokenVerifierTest.java
(right):

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/tes...
google-oauth-client/src/test/java/com/google/api/client/auth/openidconnect/IdTokenVerifierTest.java:36:
private static final List<String> TRUSTED_CLIENT_IDS = Arrays.asList(CLIENT_ID);
On 2013/06/12 14:20:49, peleyal wrote:
> please add one more value to the list

Done.

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/tes...
google-oauth-client/src/test/java/com/google/api/client/auth/openidconnect/IdTokenVerifierTest.java:46:
return new IdToken(new Header(), payload, new byte[0], new byte[0]);
On 2013/06/12 14:20:49, peleyal wrote:
> can you call "new IdToken(new Header(), payload, null, null);"?

No. Added to IdTokenTest.

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/tes...
google-oauth-client/src/test/java/com/google/api/client/auth/openidconnect/IdTokenVerifierTest.java:77:
IdTokenVerifier verifierFlexible = new
IdTokenVerifier.Builder().setClock(clock).build();
On 2013/06/12 14:20:49, peleyal wrote:
> Please mention (for readability) that verifierFlexible suppress the checks for
> Issuer and Audience.

Done.

https://codereview.appspot.com/10042045/diff/9001/pom.xml
File pom.xml (right):

https://codereview.appspot.com/10042045/diff/9001/pom.xml#newcode424
pom.xml:424: <failOnError>false</failOnError>
On 2013/06/12 14:20:49, peleyal wrote:
> Please revert this file and pull latest changes.
> [you already changed it in another CL (and I'm pretty sure that that CL was
> already pushed)]

Done.

[yanivi, 2013-06-20 16:52:03]

ping

[peleyal, 2013-06-24 15:38:22]

LGTM, but let's talk about my comment before submitting

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/mai...
File
google-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdToken.java
(right):

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdToken.java:152:
public static class Payload extends JsonWebToken.Payload {
So I don't get it, why they don't appear also in this spec? Is this spec is
addition to the JWT?
It's weird because iss for example is mentioned in both of the spec, and nbf
appears only in the JWT.

On 2013/06/17 17:36:10, yanivi wrote:
> On 2013/06/12 14:20:49, peleyal wrote:
> > Again, maybe I'm not on the same page as you, but it looks like
> > JsonWebToken.Payload contains fields like nbf and jti which don't exist in
the
> > spec.
> 
> They are in the JWT spec.

[yanivi, 2013-06-24 19:58:08]

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/mai...
File
google-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdToken.java
(right):

https://codereview.appspot.com/10042045/diff/9001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdToken.java:152:
public static class Payload extends JsonWebToken.Payload {
On 2013/06/24 15:38:22, peleyal wrote:
> So I don't get it, why they don't appear also in this spec? Is this spec is
> addition to the JWT?
> It's weird because iss for example is mentioned in both of the spec, and nbf
> appears only in the JWT.
> 
> On 2013/06/17 17:36:10, yanivi wrote:
> > On 2013/06/12 14:20:49, peleyal wrote:
> > > Again, maybe I'm not on the same page as you, but it looks like
> > > JsonWebToken.Payload contains fields like nbf and jti which don't exist in
> the
> > > spec.
> > 
> > They are in the JWT spec.
> 

I agree that it is weird, but at the same time these are draft specs, so
probably it just a matter of waiting and they will fix these kinds of minor
inconsistencies between the specs.