Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Differentiating between invalid and expired tokens when verifying #179

Closed
simenbrekken opened this issue Jan 10, 2018 · 10 comments
Closed

Comments

@simenbrekken
Copy link
Contributor

Environment

  • Operating System version: MacOS High Sierra 10.13.1 (17B1003)
  • Firebase SDK version: 5.7.0
  • Library version: 5.7.0
  • Firebase Product: auth

Description

There's currently no robust way to detect if an id token is invalid or has expired when performing verification via auth().verifyIdToken other than checking if the actual error.message contains the string "expired".

The verifyIdToken implementation of token-generator correctly detects expired tokens by checking for error.name === 'TokenExpiredError' but this error code is discarded as it's rejected as an FirebaseAuthError with the AuthClientErrorCode.INVALID_ARGUMENT error code.

Would it be possible to to add something like AuthClientErrorCode.TOKEN_EXPIRED or expose the original error.name as error.errorInfo.name?

I'd be glad to contribute a PR for any of the above propsals.

@google-oss-bot
Copy link

Hmmm this issue does not seem to follow the issue template. Make sure you provide all the required information.

@hiranya911
Copy link
Contributor

Sounds reasonable to me. @bojeil-google what do you think? Shall we introduce a new auth/id-token-expired error code? (we already have auth/id-token-revoked, so there's precedent for something like this)

@bojeil-google
Copy link
Contributor

bojeil-google commented Jan 10, 2018

In general, I think it is a good idea but I think this could be a breaking change. Developers may be expecting the current error code for both. If we add a new error code, they would basically miss it (they won't be catching it). I suggest we keep this until the next major version bump.

@simenbrekken
Copy link
Contributor Author

While waiting for the next major version bump would it be possible as per my second suggestion to surface the original TokenExpiredError as error.errorInfo.name in the meantime?

@hiranya911
Copy link
Contributor

It's going to be hard to justify an API change that will become unnecessary a few months later. What if we modify the error message (errorInfo.message) to some constant value for the moment?

@simenbrekken
Copy link
Contributor Author

How about ${message} (auth/id-token-expired) so the same constant can be reused when it's introduced as a separate error type?

@hiranya911
Copy link
Contributor

Yes, that sounds reasonable. @simenbrekken would you be able to give us a patch?

@simenbrekken
Copy link
Contributor Author

Certainly. I'll have a patch ready this weekend.

@vlapo
Copy link

vlapo commented Nov 29, 2018

Is this change still on roadmap? I see major release is out and this issue is closed but auth/id-token-expired error code is not implemented.

@hiranya911
Copy link
Contributor

It's still on roadmap. Just haven't gotten around to it yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants