Revoke Refresh Tokens #133

avishalom · 2018-01-31T18:54:13Z

initial PR, adding comments.

hiranya911

Mostly good. Few changes needed.

hiranya911 · 2018-02-02T23:22:41Z

src/main/java/com/google/firebase/auth/FirebaseAuth.java

+          String uid = firebaseToken.getUid();
+          UserRecord user = userManager.getUserById(uid);
+          if (user.getTokensValidAfterTimestamp() 
+              > ((long)firebaseToken.getClaims().get("iat")) * 1000) {


Extract to a separate line for clarity

hiranya911 · 2018-02-02T23:23:11Z

src/main/java/com/google/firebase/auth/FirebaseAuth.java

+          String uid = firebaseToken.getUid();
+          UserRecord user = userManager.getUserById(uid);
+          if (user.getTokensValidAfterTimestamp() 
+              > ((long)firebaseToken.getClaims().get("iat")) * 1000) {


Nit: Add space between (long) and firebaseToken

hiranya911 · 2018-02-02T23:23:29Z

src/main/java/com/google/firebase/auth/FirebaseAuth.java

+          UserRecord user = userManager.getUserById(uid);
+          if (user.getTokensValidAfterTimestamp() 
+              > ((long)firebaseToken.getClaims().get("iat")) * 1000) {
+                throw new FirebaseAuthException("id-token-revoked", "Firebase auth token revoked");


Add package-level constant for the error code

hiranya911 · 2018-02-02T23:24:15Z

src/main/java/com/google/firebase/auth/FirebaseAuth.java

+   * Revokes all refresh tokens for the specified user.
+   * 
+   * <p>In addition to revoking all refresh tokens for a user, all ID tokens issued
+   * before revocation will also be revoked at the Auth backend. Any request with an


I believe this is incorrect. Please align with the documentation of Node.js SDK.

hiranya911 · 2018-02-02T23:26:12Z

src/main/java/com/google/firebase/auth/FirebaseAuth.java

+   * associated with this FirebaseAuth instance (which by default is extracted from your service
+   * account)
+   * 
+   * <p>If a request was made to check revoked, the issued-at property of the token (like all 


Way too much implementation details here. Just mention something like: "If checkRevoked is true, additionally checks if the token has been revoked."

hiranya911 · 2018-02-02T23:29:37Z

src/test/java/com/google/firebase/auth/FirebaseAuthIT.java

  }

+  @Test
+  public void testVerifyIDToken() throws Exception {


testVerifyIdToken

hiranya911 · 2018-02-02T23:30:00Z

src/test/java/com/google/firebase/auth/FirebaseAuthIT.java

+    assertEquals("user_ver", decoded.getUid());
+    decoded = auth.verifyIdTokenAsync(idToken, true).get();
+    assertEquals("user_ver", decoded.getUid());
+    Thread.sleep(1100);


hiranya911 · 2018-02-02T23:31:06Z

src/test/java/com/google/firebase/auth/FirebaseAuthIT.java

+      fail("expecting exception");
+    } catch (ExecutionException e) {
+      assertTrue(e.getCause() instanceof FirebaseAuthException);
+      assertEquals("id-token-revoked", ((FirebaseAuthException) e.getCause()).getErrorCode());


Test against the package constant when you have it.

hiranya911 · 2018-02-02T23:31:31Z

src/test/java/com/google/firebase/auth/UserRecordTest.java

 import com.google.api.client.json.JsonFactory;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.firebase.auth.UserRecord.UpdateRequest;


Changes in this file seem unnecessary (just imports)?

On the other hand, why don't we have unit tests for the change in UserRecord class?

hiranya911 · 2018-02-02T23:33:03Z

src/test/java/com/google/firebase/auth/FirebaseAuthIT.java


+  @Test
+  public void testVerifyIDToken() throws Exception {
+    String customToken = auth.createCustomTokenAsync("user_ver").get();


Use "user2" or something to be consistent with other tests.

hiranya911

LGTM with some nits. Please address them prior to merging.

hiranya911 · 2018-02-05T19:37:13Z

src/main/java/com/google/firebase/auth/FirebaseAuth.java

-   * <p>In addition to revoking all refresh tokens for a user, all ID tokens issued
-   * before revocation will also be revoked at the Auth backend. Any request with an
-   * ID token generated before revocation will be rejected with a token expired error.
+   * <p>Updates the user's tokensValidAfterTimestamp to the current UTC second expressed in


s/second/time/

hiranya911 · 2018-02-05T19:37:55Z

src/main/java/com/google/firebase/auth/FirebaseAuth.java

   * parsed version of the token from which the UID and other claims in the token can be inspected.
   * If the token is invalid, the future throws an exception indicating the failure.
+   * 
+   * <p>This does not check whether a token has been revoked,


has been revoked. See....

hiranya911 · 2018-02-05T19:42:16Z

src/main/java/com/google/firebase/auth/UserRecord.java


  /**
-   * Returns the timestamp beginning with which tokens are valid in seconds since the epoch.
+   * Returns the timestamp beginning with which tokens are valid in milliseconds since the epoch.


Returns a timestamp in milliseconds since epoch, truncated down to the closest second. Tokens minted before this timestamp are considered invalid.

hiranya911 · 2018-02-05T19:42:57Z

src/main/java/com/google/firebase/auth/UserRecord.java

+  private static void checkValidSince(long epochSeconds) {
+    checkArgument(epochSeconds > 0,
+                  "validSince must be greater than 0 in seconds since the epoch: " 
+                  + Long.toString(epochSeconds));


Unnecessary wrapping.

hiranya911 · 2018-02-05T19:43:05Z

src/main/java/com/google/firebase/auth/UserRecord.java


+  private static void checkValidSince(long epochSeconds) {
+    checkArgument(epochSeconds > 0,
+                  "validSince must be greater than 0 in seconds since the epoch: " 


Formatting/indentation messed up here.

hiranya911 · 2018-02-05T19:43:50Z

src/test/java/com/google/firebase/auth/UserRecordTest.java

+      // expected
+    }
+  }
+}


Add new line at eof

hiranya911 · 2018-02-05T19:44:30Z

src/main/java/com/google/firebase/auth/FirebaseAuth.java

+          UserRecord user = userManager.getUserById(uid);
+          long issuedAt = (long) firebaseToken.getClaims().get("iat");
+          if (user.getTokensValidAfterTimestamp() > issuedAt * 1000) {
+                throw new FirebaseAuthException(FirebaseUserManager.ID_TOKEN_REVOKED_ERROR,


Wrong indentation?

hiranya911 · 2018-02-05T19:45:30Z

src/main/java/com/google/firebase/auth/FirebaseAuth.java


+  private Task<Void> revokeRefreshTokens(String uid) {
+    checkNotDestroyed();  
+    final UpdateRequest request = new UpdateRequest(uid).setValidSince(


Nit:

int currentTimeSeconds = (int) (System.currentTimeMillis() / 1000);

avishalom added 7 commits January 31, 2018 01:29

revoke tokens and tokensValidAfterTime

6d4d310

move unit test

e7ac57b

lint

7b8ac07

comments

9a990ca

cleanup

d1f6e29

whitespace

38c067e

remove time unit test for private function

ff617df

avishalom requested a review from hiranya911 February 2, 2018 18:33

avishalom assigned hiranya911 Feb 2, 2018

time -> timestamp

446bc53

hiranya911 suggested changes Feb 2, 2018

View reviewed changes

hiranya911 assigned avishalom and unassigned hiranya911 Feb 2, 2018

avishalom added 3 commits February 5, 2018 11:53

PR comments

52d27c5

clean imports

ed57658

tests

4b0a0fc

avishalom assigned hiranya911 and unassigned avishalom Feb 5, 2018

hiranya911 approved these changes Feb 5, 2018

View reviewed changes

hiranya911 assigned avishalom and unassigned hiranya911 Feb 5, 2018

avishalom added 4 commits February 5, 2018 15:01

fix PR comments

107e6ab

lint

e41773e

changelog

97306fe

Merge branch 'master' into revoke

d5620c6

avishalom merged commit 7b410b3 into master Feb 13, 2018

Revoke Refresh Tokens #133

Revoke Refresh Tokens #133

Uh oh!

Conversation

avishalom commented Jan 31, 2018

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants