Revoke Refresh Tokens by avishalom · Pull Request #133 · firebase/firebase-admin-java

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

initial PR, adding comments.

hiranya911

Mostly good. Few changes needed.

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

+          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

+          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

+          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

+   * 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

+   * 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

  }

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


testVerifyIdToken

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

+    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

+      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

 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


+  @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

-   * <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

   * 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


  /**
-   * 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

+  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


+  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

+      // expected
+    }
+  }
+}


Add new line at eof

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

+          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


+  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

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