Revoke refresh token #123

avishalom · 2018-01-29T17:21:13Z

Adding the revokeRefreshToken,
and the ability to check revoked when validating a token.

hiranya911

Looks mostly good. Just a number of nits to address.

hiranya911 · 2018-01-29T20:48:35Z

CHANGELOG.md

@@ -1,11 +1,18 @@
 # Unreleased
 - 
+### Token revokaction
+- [added] The ['verify_id_token(...)'](https://firebase.google.com/docs/reference/admin/python/firebase_admin.auth#verify_id_token)
+  method can now take an additional parameter `check_revoked`, when `True`, 


...method now accepts an optional check_revoked argument. When set to True an additional check is performed to see...

hiranya911 · 2018-01-29T20:51:27Z

firebase_admin/_user_mgt.py

@@ -113,6 +113,20 @@ def validate_photo_url(cls, photo_url):
        except Exception:
            raise ValueError('Malformed photo URL: "{0}".'.format(photo_url))

+    @classmethod
+    def validate_tokens_valid_after_time(cls, valid_since):
+        if not isinstance(valid_since, six.string_types) or not valid_since:


How come this is a string?

the other timestamps in the UserMetadata are also returned as quoted strings. (passwordUpdate is a float in the response, but all other timestamps are quoted.)

hiranya911 · 2018-01-29T20:53:14Z

firebase_admin/_user_mgt.py

+        try:
+            int(valid_since)
+        except ValueError:
+            raise ValueError('Malformed Tokens Valid After Time: "{0}" '


"Invalid tokens_valid_after_time value: {0}" should suffice here

hiranya911 · 2018-01-29T20:56:14Z

firebase_admin/auth.py

@@ -85,17 +85,27 @@ def verify_id_token(id_token, app=None):
    Args:
      id_token: A string of the encoded JWT.
      app: An App instance (optional).
+      check_revoked: check whether the token was revoked(optional).


A boolean indicating whether the token has been revoked (optional).

hiranya911 · 2018-01-29T20:57:25Z

firebase_admin/auth.py


    Returns:
      dict: A dictionary of key-value pairs parsed from the decoded JWT.

    Raises:
      ValueError: If the JWT was found to be invalid, or if the App was not
          initialized with a credentials.Certificate.
+      AuthError: If check_token is requested and the token was revoked.


If check_revoked is requested...

hiranya911 · 2018-01-29T21:08:17Z

firebase_admin/auth.py

+            int: Timestap in seconds since the epoch. All tokens issued before that time
+                 are considered revoked.
+        """
+        return int(self._data.get('validSince', 0))


We seem to return 0 by default.

Is that consistent with the behavior in Node SDK (perhaps None is more appropriate here)?

We should document this in the docstring.

hiranya911 · 2018-01-29T21:10:15Z

tests/test_auth.py

-                             ids=list(invalid_tokens))
+    @pytest.mark.parametrize('id_token', valid_tokens.values(), ids=list(valid_tokens))
+    def test_valid_token_check_revoked(self, user_mgt_app_for_verify_token, id_token):
+        _instrument_user_manager(user_mgt_app_for_verify_token, 200, MOCK_GET_USER_RESPONSE)


Also need a test case for when the UserRecord contains a future timestamp.

this is MOCK_GET_USER_REVOKED_TOKENS_RESPONSE

hiranya911 · 2018-01-29T21:13:48Z

tests/test_auth.py

+            auth.verify_id_token(id_token, app=user_mgt_app_for_verify_token,
+                                 check_revoked=True)
+
+        assert excinfo.value.code == 'ID_TOKEN_REVOKED'


Check against the constant once you have it

hiranya911 · 2018-01-29T21:14:33Z

tests/test_auth.py

+        assert claims['admin'] is True
+        assert claims['uid'] == claims['sub']
+
+    def test_revoke_refresh_token(self, user_mgt_app):


Add test case for setting this via update_user()

the revoke in turn calls update_user, do we want to explicitly do this with update user?

hiranya911 · 2018-01-29T21:16:56Z

tests/test_auth.py

@@ -287,6 +326,13 @@ def user_mgt_app():
    yield app
    firebase_admin.delete_app(app)

+@pytest.fixture(scope='module')
+def user_mgt_app_for_verify_token():


Can we just modify the existing user_mgt_app fixture to have the project_id option?

…dmin-python into revoke-refresh-token

hiranya911

Looks mostly good. Few more changes and we would be at an LGTM.

hiranya911 · 2018-02-02T18:04:41Z

CHANGELOG.md

@@ -1,11 +1,19 @@
 # Unreleased
 - 
+### Token revokaction
+- [added] The ['verify_id_token(...)'](https://firebase.google.com/docs/reference/admin/python/firebase_admin.auth#verify_id_token)
+  method now accepts an optional `check_revoked` parameter, when `True`, an 


parameter. When True...

hiranya911 · 2018-02-02T18:05:02Z

CHANGELOG.md

+  method now accepts an optional `check_revoked` parameter, when `True`, an 
+  additional check is performed to see if the token has been revoked. 
+- [added] A new method ['auth.revoke_refresh_tokens(uid)'](https://firebase.google.com/docs/reference/admin/python/firebase_admin.auth#revoke_refresh_tokens)
+  has been added to invalidate all tokens issued before the current second.


...issued to a user.

hiranya911 · 2018-02-02T18:05:19Z

CHANGELOG.md

+- [added] A new method ['auth.revoke_refresh_tokens(uid)'](https://firebase.google.com/docs/reference/admin/python/firebase_admin.auth#revoke_refresh_tokens)
+  has been added to invalidate all tokens issued before the current second.
+- [added] A new property `tokens_valid_after_time` has been added to the ['UserRecord'](https://firebase.google.com/docs/reference/admin/python/firebase_admin.auth#userrecord),
+  it denotes the time in epoch milliseconds before which tokens are not valid.


which denotes..

hiranya911 · 2018-02-02T18:06:05Z

firebase_admin/_user_mgt.py

@@ -113,6 +113,20 @@ def validate_photo_url(cls, photo_url):
        except Exception:
            raise ValueError('Malformed photo URL: "{0}".'.format(photo_url))

+    @classmethod
+    def validate_valid_since(cls, valid_since):
+        if not isinstance(valid_since, six.string_types) or not valid_since:


Why is this a string? It should just accept a timestamp.

hiranya911 · 2018-02-02T18:06:58Z

firebase_admin/auth.py

@@ -85,17 +86,27 @@ def verify_id_token(id_token, app=None):
    Args:
      id_token: A string of the encoded JWT.
      app: An App instance (optional).
+      check_revoked: A boolean indicating whether the token has been revoked (optional).


whether to check if the token has been...

hiranya911 · 2018-02-02T18:07:45Z

firebase_admin/auth.py

+
+def revoke_refresh_tokens(uid, app=None):
+    user_manager = _get_auth_service(app).user_manager
+    user_manager.update_user(uid, valid_since=str(int(time.time())))


Why wrap this in str()? Just pass the int timestamp.

hiranya911 · 2018-02-02T18:09:13Z

firebase_admin/auth.py

@@ -476,12 +497,16 @@ def __init__(self, data):

    @property
    def creation_timestamp(self):
+        """ Creation timestamp in epoch milliseconds.
+        """


Add the Returns: section too while we are at it?

hiranya911

LGTM with a few nits.

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

CHANGELOG.md

- [added] A new property `tokens_valid_after_time` has been added to the ['UserRecord'](https://firebase.google.com/docs/reference/admin/python/firebase_admin.auth#userrecord),
+  has been added to invalidate all tokens issued to a user before the current second.
+- [added] A new property `tokens_valid_after_timestamp` has been added to the   
+['UserRecord'](https://firebase.google.com/docs/reference/admin/python/firebase_admin.auth#userrecord),
  it denotes the time in epoch milliseconds before which tokens are not valid.


which denotes...

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

firebase_admin/_user_mgt.py

-            raise ValueError('Malformed Valid Since "{0}".'.format(valid_since))
+                'Invalid time string for: "{0}". Valid Since must be an int'.format(valid_since))
+        if int(valid_since) <= 0:
+            raise ValueError('Valid Since: must be a positive interger. {0}'.format(valid_since))


Invalid valid_since: ... ?

hiranya911 · 2018-02-05T19:54:12Z

firebase_admin/auth.py

+    While this revokes all sessions for a specified user and disables any new ID tokens for
+    existing sessions from getting minted, existing ID tokens may remain active until their
+    natural expiration (one hour). To verify that ID tokens are revoked, use
+    `verify_id_token(idToken, check_revoked=true)`.


check_revoked=True

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

tests/data/list_users.json

@@ -49,7 +49,7 @@
        "passwordUpdatedAt" : 1.494364393E+12,
        "validSince" : "1494364393",
        "disabled" : false,
-        "createdAt" : "1234567890",
+        "createdAt" : "1234567890000",
        "customAttributes" : "{\"admin\": true, \"package\": \"gold\"}"
    } ]
 }


Not really related to your change, but lets add a new line here.

avishalom added 3 commits January 29, 2018 10:52

adding revoke tokens and tests

55420df

add test for sign in again

f9f1ba3

lint

ab14dfe

avishalom requested a review from hiranya911 January 29, 2018 17:21

avishalom added 3 commits January 29, 2018 12:47

lint

cdfffa9

lint

40daf4e

python3 compatibility

82e4ef3

avishalom assigned avishalom and hiranya911 and unassigned avishalom Jan 29, 2018

avishalom added 2 commits January 29, 2018 13:20

changelog

8f0695c

changelog

883fec0

hiranya911 suggested changes Jan 29, 2018

View reviewed changes

hiranya911 assigned avishalom and unassigned hiranya911 Jan 29, 2018

avishalom added 5 commits January 30, 2018 10:44

timestamps in millis

c1ff5ed

lint

11920d8

PR review addressing comments

13a513f

lint

91c8b4b

nit

b56197e

avishalom assigned hiranya911 and unassigned avishalom Feb 2, 2018

avishalom added 4 commits February 2, 2018 01:29

revert

a7db23c

Merge branch 'revoke-refresh-token' of github.com:firebase/firebase-a…

e2b0c80

…dmin-python into revoke-refresh-token

reinstate check for milliseconds

fe0011b

test coment kwargs

70cef7c

hiranya911 suggested changes Feb 2, 2018

View reviewed changes

hiranya911 assigned avishalom and unassigned hiranya911 Feb 2, 2018

whitespace

64c2e93

avishalom added 8 commits February 2, 2018 13:56

s/valid_after_time/valid_after_timestamp/

4025957

timestamps in tests to reasonable millis

5138956

Merge branch 'master' into revoke-refresh-token

44a6bef

fixes

639b425

lint

2bd5ec5

whitespace

2f4e156

comments

589fd10

remove str wrap for valid_since

0293bb5

avishalom assigned hiranya911 and unassigned avishalom Feb 5, 2018

cleanup

393c1ab

hiranya911 approved these changes Feb 5, 2018

View reviewed changes

hiranya911 assigned avishalom and unassigned hiranya911 Feb 5, 2018

avishalom added 2 commits February 5, 2018 15:04

PR comment fixes

e4fa1e7

lint

d52c49e

avishalom merged commit a85865c into master Feb 13, 2018

avishalom deleted the revoke-refresh-token branch February 13, 2018 01:37

Revoke refresh token #123

Revoke refresh token #123

Uh oh!

Conversation

avishalom commented Jan 29, 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!

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!

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!

Uh oh!