Revoke Tokens #77

avishalom · 2018-01-25T21:01:49Z

go implementation of token revocation

bklimt · 2018-01-29T18:16:04Z

auth/auth.go

+// ID token generated before revocation will be rejected with a token expired error.
+// Note that due to the fact that the timestamp is stored in seconds, any tokens minted in
+// the same second as the revocation will still be valid. If there is a chance that a token
+//  was minted in the last second, delay for 1 second before revoking.all tokens minted before the current second.


There's a weird space at the beginning of this line.

bklimt · 2018-01-29T18:17:11Z

auth/auth.go

+// In addition to revoking all refresh tokens for a user, all ID tokens issued
+// before revocation will also be revoked on the Auth backend. Any request with an
+// ID token generated before revocation will be rejected with a token expired error.
+// Note that due to the fact that the timestamp is stored in seconds, any tokens minted in


Is the timestamp second granularity part of this client SDK, or is it a limitation of the backend API being called?

The backend stores it as seconds since the epoch.

bklimt · 2018-01-29T18:17:47Z

auth/auth.go

@@ -237,6 +248,25 @@ func (c *Client) VerifyIDToken(idToken string) (*Token, error) {
 	return p, nil
 }

+// VerifyIDTokenWithCheckRevoked verifies the signature	and payload of the provided ID token and


weird comment spacing

bklimt · 2018-01-29T18:23:05Z

auth/auth.go

+// VerifyIDTokenWithCheckRevoked verifies the signature	and payload of the provided ID token and
+// if requested, whether it was revoked.
+// see: VerifyIDToken above.
+func (c *Client) VerifyIDTokenWithCheckRevoked(ctx context.Context, idToken string, checkToken bool) (*Token, error) {


It seems odd to have both of these pieces of functionality in one function. VerifyIdToken is already exposed as an API people could use any time they would otherwise call this with checkToken = false. So why not remove the checkToken parameter and always assume it's true? Then maybe the function could be named something like VerifyIDTokenNotRevoked. TBH, I don't really understand why VerifyIDToken doesn't already check the revoked state, though.

validSince is initialized to 0 when creating a new user, and we are only introducing revoke here. so we probably will not break anything if we assume it to be true.

Correction, valid since is initialized to the current timestamp.

bklimt · 2018-01-29T18:25:19Z

auth/user_mgt.go

@@ -416,6 +424,13 @@ func validatePhone(val interface{}) error {
 	return nil
 }

+func validateValidSince(val interface{}) error {
+	if _, ok := val.(int64); !ok {
+		return fmt.Errorf("tokens valid after time must be an integer")


I would either write this as:
"TokensValidAfterTime must be an integer"
or
"valid-after time for tokens must be an integer"

As it is, it's hard to read, because it's not clear that "tokens valid after time" is a compound, especially since the field is called "validSince" in other parts of the API.

Actually, why is it "validSince" in some places and "TokensValidAfterTime" in another?

This is part of what was decided in the node api.

It is valid since internally. it is exposed as tokensValidAfterMillis.
This validation is for a private function

bklimt · 2018-01-29T18:28:23Z

integration/auth/user_mgt_test.go

@@ -65,6 +66,14 @@ func testCreateUsers(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
+	// make sure that the user.TokensValidAvterTime is not in the future or stale.


s/Avter/After/

hiranya911

Very close to an LGTM now. Just a few more minor things.

hiranya911 · 2018-02-02T17:42:26Z

CHANGELOG.md

-  has been added to invalidate all tokens issued before the current second. [added] A new property 
+  method checks to see if the token has been revoked. 
+- [added] A new method ['RevokeRefreshTokens(uid)'](https://godoc.org/firebase.google.com/go/auth#Client.RevokeRefreshTokens)
+  has been added to invalidate all tokens issued before the current second. 


... all refresh tokens issued to a user.

hiranya911 · 2018-02-02T17:42:50Z

auth/auth.go

@@ -177,12 +177,12 @@ func (c *Client) CustomTokenWithClaims(uid string, devClaims map[string]interfac
 	return encodeToken(c.snr, defaultHeader(), payload)
 }

+// RevokeRefreshTokens revokes all refresh tokens for the specified user.
+//
 // RevokeRefreshTokens revokes all refresh tokens for the specified user.


This is repeated now.

hiranya911 · 2018-02-02T17:45:25Z

auth/user_mgt.go

+func validateValidSince(val interface{}) error {
+	if _, ok := val.(int64); !ok {
+		return fmt.Errorf("tokens valid after time must be an integer")
+	}


I'd just do a simple sign check to begin with. We can implement more restrictive checks later if it becomes necessary.

hiranya911 · 2018-02-02T17:46:24Z

integration/auth/auth_test.go

+	if vt.UID != uid {
+		t.Errorf("UID = %q; want UID = %q", vt.UID, uid)
+	}
+	time.Sleep(time.Second)


This is missing a full stop somewhere.

hiranya911 · 2018-02-02T17:47:03Z

integration/auth/user_mgt_test.go

@@ -51,13 +51,31 @@ func TestUserManagement(t *testing.T) {
 		{"Add custom claims", testAddCustomClaims},
 		{"Delete test users", testDeleteUsers},
 	}
+	// The tests are meant to be run in sequence, a failure in creating the users


Replace comma with full stop?

hiranya911 · 2018-02-02T17:48:26Z

integration/auth/user_mgt_test.go

 	for _, run := range orderedRuns {
 		if ok := t.Run(run.name, run.testFunc); !ok {
 			t.Fatalf("Failed run %v", run.name)
 		}
 	}
 }

+func forceCreateUser(uid string, params *auth.UserToCreate) (*auth.UserRecord, error) {


This looks hacky. Can we remove this from this PR? We should implement a better solution in a separate PR.

hiranya911 · 2018-02-02T17:49:18Z

CHANGELOG.md

+
+### Token revokaction
+- [added] A New ['VerifyIDTokenWithCheckRevoked(ctx, token)'](https://godoc.org/firebase.google.com/go/auth#Client.VerifyIDToken)
+  method checks to see if the token has been revoked. 


method has been added to check for revoked ID tokens.

hiranya911 · 2018-02-02T17:52:16Z

auth/auth.go

+//
+// RevokeRefreshTokens revokes all refresh tokens for the specified user.
+// 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


Hmmm... I don't think this is correct. Can we align this with what's documented at https://firebase.google.com/docs/reference/admin/node/admin.auth.Auth#revokeRefreshTokens ?

hiranya911 · 2018-02-02T17:52:47Z

auth/auth.go

 // VerifyIDToken verifies the signature	and payload of the provided ID token.
 //
 // VerifyIDToken accepts a signed JWT token string, and verifies that it is current, issued for the
 // correct Firebase project, and signed by the Google Firebase services in the cloud. It returns
 // a Token containing the decoded claims in the input JWT. See
 // https://firebase.google.com/docs/auth/admin/verify-id-tokens#retrieve_id_tokens_on_clients for
 // more details on how to obtain an ID token in a client app.
+// This does not check whether or not the token has been revoked, see VerifyIDTokenWithCheckRevoked below.


Comma --> Full stop

hiranya911

LGTM with some nits. Please wait for @bklimt to do a pass.

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

auth/auth.go

-// 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.
+// RevokeRefreshTokens updates the user's TokensValidAfterMillis to the current UTC second.


Please document this from the developer's point of view, and not SDK's point of view. Something like:

RevokeRefreshTokens revokes all refresh tokens issued to a user. ...

This is pretty much in line with the Node docs. I am not sure I understand the comment. (i change the first line as suggested.)

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

auth/auth.go

@@ -194,7 +196,7 @@ func (c *Client) RevokeRefreshTokens(ctx context.Context, uid string) error {
 // a Token containing the decoded claims in the input JWT. See
 // https://firebase.google.com/docs/auth/admin/verify-id-tokens#retrieve_id_tokens_on_clients for
 // more details on how to obtain an ID token in a client app.
-// This does not check whether or not the token has been revoked, see VerifyIDTokenAndCheckRevoked below.
+// This does not check whether or not the token has been revoked. see `VerifyIDTokenAndCheckRevoked` below.


See (capitalization)

hiranya911 · 2018-02-05T19:59:53Z

auth/user_mgt.go

-	if _, ok := val.(int64); !ok {
-		return fmt.Errorf("validSince must be an integer signifying seconds since the epoch")
+	if v, ok := val.(int64); !ok || v <= 0 {
+		return fmt.Errorf("validSince must be an positive integer signifying seconds since the epoch")


a positive integer representing...

hiranya911 · 2018-02-05T20:03:38Z

auth/user_mgt.go

@@ -416,6 +427,13 @@ func validatePhone(val interface{}) error {
 	return nil
 }

+func validateValidSince(val interface{}) error {


Given that there's no way for a user to set this explicitly, I wonder if this check is even needed. Consider removing.

bklimt · 2018-02-06T20:30:52Z

auth/auth.go

@@ -177,13 +177,26 @@ func (c *Client) CustomTokenWithClaims(uid string, devClaims map[string]interfac
 	return encodeToken(c.snr, defaultHeader(), payload)
 }

+// RevokeRefreshTokens revokes all refresh tokens issued to a user.
+//
+// RevokeRefreshTokens updates the user's TokensValidAfterMillis to the current UTC second.


s/second/time.

Oh, I guess second is correct. I misread it before.

* Renamed some tests and test parameters for clarity, and adhere to Go conventions (#74) * clean unused types (#76) * Create CHANGELOG.md (#75) (#79) * Create CHANGELOG.md Initial changelog based on https://firebase.google.com/support/release-notes/admin/go * change instance ID format (#82) Changing the format of the "non-existing" instance ID in the integration tests to comply with the expected iid format. * Import context from golang.org/x/net/ for 1.6 compatibility (#87) * import golang.org/x/net/context instead of context for 1.6 compatibility * Document non existing name in integration tests for iid (#85) * Revoke Tokens (#77) Adding TokensValidAfterMillis property, RevokeRefreshTokens(), and VerifyIDTokenAndCheckRevoked(). * Firebase Cloud Messaging API (#81) * Adding Firebase Cloud Messaging (#62) * initial commit for adding Firebase Cloud Messaging * add validator * use http const in messaging test * add client version header for stats * init integration test * add integration test (validated on IOS today) * add comment with URL to enable Firebase Cloud Messaging API * fix broken test * add integration tests * accept a Message instead of RequestMessage + and rename method + send / sendDryRun * update fcm url * rollback url endpoint * fix http constants, change responseMessage visibility, change map[string]interface{} as map[string]string * fix http constants * fix integration tests * fix APNS naming * add validators * Added APNS types; Updated tests * Added more tests; Fixed APNS serialization * Updated documentation * Improved error handling inFCM * Added utils file * Updated integration tests * Implemented topic management operations * Added integration tests * Updated CHANGELOG * Addressing code review comments * Supporting 0 valued Aps.Badge * Addressing some review comments * Removed some unused vars * Accepting prefixed topic names (#84) * Accepting prefixed topic named * Added a comment * Using new FCM error codes (#89) * Bumped version to 2.5.0 (#90)

* Renamed some tests and test parameters for clarity, and adhere to Go conventions (#74) * clean unused types (#76) * Create CHANGELOG.md (#75) (#79) * Create CHANGELOG.md Initial changelog based on https://firebase.google.com/support/release-notes/admin/go * change instance ID format (#82) Changing the format of the "non-existing" instance ID in the integration tests to comply with the expected iid format. * Import context from golang.org/x/net/ for 1.6 compatibility (#87) * import golang.org/x/net/context instead of context for 1.6 compatibility * Document non existing name in integration tests for iid (#85) * Revoke Tokens (#77) Adding TokensValidAfterMillis property, RevokeRefreshTokens(), and VerifyIDTokenAndCheckRevoked(). * Firebase Cloud Messaging API (#81) * Adding Firebase Cloud Messaging (#62) * initial commit for adding Firebase Cloud Messaging * add validator * use http const in messaging test * add client version header for stats * init integration test * add integration test (validated on IOS today) * add comment with URL to enable Firebase Cloud Messaging API * fix broken test * add integration tests * accept a Message instead of RequestMessage + and rename method + send / sendDryRun * update fcm url * rollback url endpoint * fix http constants, change responseMessage visibility, change map[string]interface{} as map[string]string * fix http constants * fix integration tests * fix APNS naming * add validators * Added APNS types; Updated tests * Added more tests; Fixed APNS serialization * Updated documentation * Improved error handling inFCM * Added utils file * Updated integration tests * Implemented topic management operations * Added integration tests * Updated CHANGELOG * Addressing code review comments * Supporting 0 valued Aps.Badge * Addressing some review comments * Removed some unused vars * Accepting prefixed topic names (#84) * Accepting prefixed topic named * Added a comment * Using new FCM error codes (#89) * Bumped version to 2.5.0 (#90) * Lint (#96) * fix misspelling * add check error * missing copyright * Doc (#97) * update readme with Authentication Guide & Release Notes * fix a misspelling : separately * fix missing newline before package * add Go Report Card + update doc * add travis build for go versions 1.7.x -> 1.10.x (#98) * add build for go version 1.6.x -> 1.10.x * fix 1.10 version * fix context to golang.org/x/net/context for go 1.6 compatibility * add race detector + go vet on build + build without failure on go unstable * add go16 et go17 file due to req.withcontext which is only go 1.7 * fix context package * update go16.go to remove WithContext * update bad import * remove unused func * finally use ctxhttp.Do with multiple build version * ignore integration package for install * fix go get command * put go 1.6.X in allow_failures dur to test failure * fix inversion of code * remove go 1.6 support * revert initial version with req.WithContext * fix travis to support go 1.10.x * nits * Import context from standard package (#101) * Import context from standard package. * Firebase Database API (#92) * Experimental RTDB code * Added ref.Set() * Added Push(), Update(), Remove() and tests * Adding Transaction() support * Fixed Transaction() API * Code cleanup * Implemented Query() API * Added GetIfChanged() and integration tests * More integration tests * Updated unit test * More integration tests * Integration tests for queries * Auth override support and more tests * More test cases; AuthOverride support in App * Implemented AuthOverride support; Added tests * Implementing the new API * More code cleanup * Code clean up * Refactored the http client code * More tests * Boosted test coverage to 97% * Better error messages in tests; Added license headers * Added documentatioon and cleaned up tests * Fixing a build break * Finishing up documentation * More test cases * Implemented a reusable HTTP client API * Added test cases * Comment clean up * Using the shared http client API * Simplified the usage by adding HTTPClient * using the new client API * Using the old ctx import * Using the old context import * Refactored db code * More refactoring * Support for arbitrary entity types in the request * Renamed fields; Added documentation * Removing a redundant else case * Code readability improvements * Cleaned up the RTDB HTTP client code * Added shallow reads support; Added the new txn API * Implementing GetOrdered() for queries * Adding more sorting tests * Added Query ordering tests * Fixing some lint errors and compilation errors * Removing unused function * Cleaned up unit tests for db * Updated query impl and tests * Added integration tests for ordered queries * Removed With*() from query functions * Updated change log; Added more tests * Support for database url in auto init * Support for loading auth overrides from env * Removed db.AuthOverride type * Renamed ao to authOverride everywhere; Other code review nits * Introducing the QueryNode interface to handle ordered query results (#100) * Database Sample Snippets (#102) * Adding database snippets * Adding query snippets * Added complex query samples * Updated variable name * Fixing a typo * Fixing query example * Updated DB snippets to use GetOrdered() * Removing unnecessary placeholders in Fatalln() calls * Removing unnecessary placeholders in Fatalln() calls * Handling FCM canonical error codes (#103) * Formatting test file with gofmt (#104) * Bumped version to 2.6.0 (#105) * Formatting (simplification) changes (#107) * Checking for unformatted files in CI (#108) * Checking for unformatted files in CI * Adding newline at eof * Document Minimum Go Version (#111) * Fix invalid endpoint URL for topic unsubscribe (#114) * Fix error message for missing user (#113) * Update CHANGELOG.md (#117) * Removing unused member from auth.Client (#118) * Support Go 1.6 (#120) * all: use golang.org/x/net/context * internal: use ctxhttp to use /x/ context The 1.6 Request type doesn't have WithContext. * all: don't use subtests to keep 1.6 compatibility * integration: use float64 for fields with exp value Values like -7e+07 cannot be parsed into ints in Go 1.6. So, use floats instead. * integration/messaging: use t.Fatal not log.Fatal * travis: add 1.6.x * changelog: mention addition of 1.6 support * readme: mention go version support * Bumped version to 2.6.1 (#121) * Changlog updates (#123)

Adding TokensValidAfterMillis property, RevokeRefreshTokens(), and VerifyIDTokenAndCheckRevoked().

avishalom added 7 commits January 25, 2018 13:15

mplementation of revoke

cf9b75e

mplementation of revoke

8d967ef

whitespace

e828539

undo echo refactor

1e0db76

undo reflect tests for create

d046362

test valid time

de72ac5

integration tests

148fd88

avishalom requested a review from hiranya911 January 25, 2018 21:01

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

avishalom added 3 commits January 26, 2018 11:43

lint

6b02152

comment

f199257

comment

c789e3e

avishalom assigned hiranya911 and unassigned avishalom Jan 26, 2018

avishalom added 2 commits January 26, 2018 16:01

typo

bfb6a01

clean redundant property

48910ad

avishalom assigned avishalom and unassigned hiranya911 Jan 28, 2018

avishalom added 5 commits January 28, 2018 19:00

change test for invalid token

1192a1c

validator

5b82970

add test

7e72fdc

revoke takes UID

f859f6a

test revoked but not checked

42e5d8d

avishalom assigned hiranya911 and unassigned avishalom Jan 29, 2018

avishalom requested a review from bklimt January 29, 2018 18:10

bklimt reviewed Jan 29, 2018

View reviewed changes

Merge branch 'dev' of github.com:firebase/firebase-admin-go into revoke

cad7bed

avishalom added 3 commits February 1, 2018 18:48

PR comment fixes

b18dab2

Merge branch 'dev' into revoke

877707e

PR comment fixes

1c0d9d6

avishalom assigned hiranya911 and bklimt Feb 2, 2018

hiranya911 suggested changes Feb 2, 2018

View reviewed changes

hiranya911 removed their assignment Feb 2, 2018

avishalom added 5 commits February 2, 2018 15:32

s/Time/Millis/; s/WithCheck/AndCheck/

19fbeb0

Change timestamp tests to millis

a7ebce7

millis fix

23a5c7d

fixes

b1c6caa

remove existing user check

bf6b1ab

avishalom assigned hiranya911 and unassigned avishalom Feb 5, 2018

hiranya911 approved these changes Feb 5, 2018

View reviewed changes

hiranya911 assigned bklimt and unassigned bklimt and hiranya911 Feb 5, 2018

avishalom added 2 commits February 6, 2018 13:42

PR comments

0bf8041

capitalization

0b91a4a

bklimt approved these changes Feb 6, 2018

View reviewed changes

typo

f20054e

avishalom assigned avishalom and unassigned bklimt Feb 11, 2018

merge dev

4c89502

avishalom merged commit 06eb0e0 into dev Feb 13, 2018

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

hiranya911 pushed a commit that referenced this pull request Jun 12, 2018

Revoke Tokens (#77)

d388cd8

Adding TokensValidAfterMillis property, RevokeRefreshTokens(), and VerifyIDTokenAndCheckRevoked().

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revoke Tokens #77

Revoke Tokens #77

avishalom commented Jan 25, 2018

bklimt Jan 29, 2018

bklimt Jan 29, 2018

avishalom Jan 29, 2018

bklimt Jan 29, 2018

bklimt Jan 29, 2018

avishalom Jan 30, 2018

avishalom Feb 5, 2018

bklimt Jan 29, 2018

avishalom Jan 29, 2018

avishalom Feb 5, 2018 •

edited

Loading

bklimt Jan 29, 2018

hiranya911 left a comment

hiranya911 Feb 2, 2018

hiranya911 Feb 2, 2018

hiranya911 Feb 2, 2018

hiranya911 Feb 2, 2018

hiranya911 Feb 2, 2018

hiranya911 Feb 2, 2018

hiranya911 Feb 2, 2018

hiranya911 Feb 2, 2018

hiranya911 Feb 2, 2018

hiranya911 left a comment

hiranya911 Feb 5, 2018

avishalom Feb 5, 2018

hiranya911 Feb 5, 2018

hiranya911 Feb 5, 2018

hiranya911 Feb 5, 2018

bklimt Feb 6, 2018

bklimt Feb 6, 2018

Revoke Tokens #77

Revoke Tokens #77

Conversation

avishalom commented Jan 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avishalom Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiranya911 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiranya911 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avishalom Feb 5, 2018 •

edited

Loading