| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Adding the optional parameter clock_skew_in_seconds=60 to the call to google.oauth2.id_token.verify_token now allows for the token-issuing server's clock to be off by up to a minute without the token becoming invalid due to a 'issued-at-time' timestamp that is in the future.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Sorry, something went wrong.
|
Hi, I wanted to follow up on this issue and ask when it will be merged? |
Sorry, something went wrong.
This option value is used for the token verification instead of the fixed 60 seconds from the earlier commit. This way, the user of firebase_admin can decide if he/she wants to set that value or not. Also all existing uses of firebase_admin won't suddenly change behaviour, since if the option is not specified, it's default of 0 is equivalent to what was used before the introduction of the new option.
|
I changed the pull request a little. The parameter clock_skew_in_seconds for the Google token verification API call is no longer hard-coded with 60 seconds but can be set as an App option. If that option is not set, a default of 0 is used. This makes all uses of firebase_admin work the same way as before. So the change won't introduce a change of behaviour for those existing applications. But users that need the clock skew setting can add an option to their App and overwrite the default of 0 that way. I think that is the better way to introduce using the clock_skew_in_seconds parameter of the Google API. |
Sorry, something went wrong.
There was a problem hiding this comment.
lets merge this changes
many users in stuck
because of this bug
Sorry, something went wrong.
There was a problem hiding this comment.
A default clock skew is set to 0 seconds and the server's clock is allowed to be off by up to 60 seconds
Sorry, something went wrong.
|
Why is this PR not merge? |
Sorry, something went wrong.
|
Thank you @fschaeck for your contribution! The changes to App options require an internal API review and might take a while. Hi, @prameshj, @renkelvin let me know your thoughts on this, I can initiate the internal changes. Thank you! |
Sorry, something went wrong.
There was a problem hiding this comment.
Thank you @fschaeck for your contribution! This looks good to me, but as an alternative, what do you think about adding clock_skew_in_seconds =0 to verify_id_token() and verify_session_cookie as an optional parameter?
firebase-admin-python/firebase_admin/auth.py
Line 194 in b9e95e8
Sorry, something went wrong.
|
@lahirumaramba @fschaeck By the way, we have a similar bug with this method. auth.verify_session_cookie(session, check_revoked=True)I solved it this way. session_cookie_request = requests.Request()
CERTS_URL = "https://www.googleapis.com/identitytoolkit/v3/relyingparty/publicKeys"
id_token.verify_token(session_token=session, request=session_cookie_request, certs_url=CERTS_URL, clock_skew_in_seconds=10) |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM!
Sorry, something went wrong.
|
@fschaeck are you able to merge this now? Subscribing. |
Sorry, something went wrong.
|
appreciate if this could be merged |
Sorry, something went wrong.
|
Thank you for your patience everyone. We just completed the internal API review and we think a better approach would be to add a new optional clock_skew_seconds parameter to verify_id_token() and verify_session_cookie() APIs. API: def verify_id_token(id_token, app=None, check_revoked=False, clock_skew_seconds=0): def verify_session_cookie(session_cookie, check_revoked=False, app=None, clock_skew_seconds=0): Usage: import firebase_admin from firebase_admin import auth app = firebase_admin.initialize_app() decoded_token = auth.verify_id_token(id_token, clock_skew_seconds=60) decoded_claims = auth.verify_session_cookie(session_cookie, check_revoked=True, clock_skew_seconds=10) @fschaeck would you be able to make the changes? Thank you! |
Sorry, something went wrong.
|
Hi mean while this need to be changed what the work around? could not figure out yet how to skew my clock with node js |
Sorry, something went wrong.
Fork the fix and install the git repo with pip works for now. |
Sorry, something went wrong.
You do realize, that this means to change the signatures of methods TokenVerifier.verfify_id_token, TokenVerifier.verify_session_cookie and _JWTVerifier.verify from firebase_admin/_token_gen.py by adding the optional parameter clock_skew_seconds there as well, right? Since TokenVerifier and _JWTVerifier are classes (doubly) abstracting the token verification and I was not able to assess, if there are any other implementations that might be used here, I chose the way of the additional app property instead of changing the signature of those methods, because other such abstracting classes may not understand the additional parameter and then apps using this optional parameter all the sudden stop working. If you okay THOSE change as well, I will change this pull request right the way! |
Sorry, something went wrong.
Can you please go into more detail on your fix? What class is id_token? Thanks. This bug has been annoying as it would happen randomly. Please merge this as soon as possible! THANKS GOOGLE. |
Sorry, something went wrong.
Hi @fschaeck thank you for being so thoughtful about this change. Since _JWTVerifier and TokenVerifier are internal classes and not exposed in the public API surface, we are safe to change the method signature by adding the optional parameter clock_skew_seconds. Please add unit tests (and integration tests) where possible. Thanks! |
Sorry, something went wrong.
|
@lahirumaramba when will this pr get reviewed/approved to be merged? this change would be so nice to have for firebase |
Sorry, something went wrong.
|
Any updates on this? Or how to avoid it? |
Sorry, something went wrong.
per feedback in firebase#625 (comment) adds unit and integration tests as well. unit tests and lint pass.
per feedback in firebase#625 (comment) adds unit and integration tests as well. unit tests and lint pass.
per feedback in firebase#625 (comment) adds unit and integration tests as well. unit tests and lint pass.
* feat: add clockSkewSeconds per feedback in #625 (comment) adds unit and integration tests as well. unit tests and lint pass. * fix: test * chore: version bump for testing * chore: address CR * fix:lint * chore: address CR * chore: remove test * fix: remove more tests * chore: address CR
| Back | FazBrowse Home | New Git URL |
Adding the optional parameter clock_skew_in_seconds=60 to the call to google.oauth2.id_token.verify_token now allows for the token-issuing server's clock to be off by up to a minute without the token becoming invalid due to a 'issued-at-time' timestamp that is in the future.
Discussion
Issue #624
Testing
No additional tests required.
API Changes
n/a