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

feat(auth): Add clockSkewInSeconds #714

Merged
merged 9 commits into from Oct 26, 2023
Merged

Conversation

stillmatic
Copy link
Contributor

@stillmatic stillmatic commented Aug 23, 2023

Discussion

Testing

  • All unit tests pass with pytest.
  • Lint passes.
  • Integration tests are pretty annoying. Not sure how to create an expired token to use, and the session cookies have a minimum expiry time of 300 seconds - the naive sleep 300 means that tests take 5 min to run. Happy to take guidance on improving that.

API Changes

  • The API changes here were suggested/approved in the linked discussion.

@google-cla
Copy link

google-cla bot commented Aug 23, 2023

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.

Copy link
Contributor

@jonathanedey jonathanedey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @stillmatic for your contribution and getting this feature moving again.

This looks great! I have attached a few comments to be addressed to fully match the approved internal API proposal so please take a look.

Additionally, we would like to add validation on the clock_skew_seconds to limit it to between 0 and 60 seconds and throw a Value Error otherwise. Could you add that here with supporting test cases? Thank you!

firebase_admin/__about__.py Outdated Show resolved Hide resolved
firebase_admin/__init__.py Outdated Show resolved Hide resolved
firebase_admin/__init__.py Outdated Show resolved Hide resolved
firebase_admin/_auth_client.py Outdated Show resolved Hide resolved
firebase_admin/_auth_client.py Outdated Show resolved Hide resolved
firebase_admin/auth.py Outdated Show resolved Hide resolved
integration/test_auth.py Outdated Show resolved Hide resolved
integration/test_auth.py Outdated Show resolved Hide resolved
firebase_admin/auth.py Outdated Show resolved Hide resolved
@stillmatic
Copy link
Contributor Author

stillmatic commented Sep 26, 2023

I've addressed the feedback in the code change and rebased off main

Note: this dependency uses clock_skew_in_seconds https://github.com/googleapis/google-auth-library-python/blob/7039beb63b8644be748cfc2fc79a2b8b643cda9f/google/oauth2/id_token.py#L112C5-L112C26, So if that gets updated, then this should also be updated. I'm going to assume that moving forward clock_skew_seconds is preferred and that you'll handle the discrepancy there.

Copy link
Contributor

@jonathanedey jonathanedey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few small questions but otherwise it looks good!
@lahirumaramba do you have anything to add that we might have missed

Thanks for your concern around the discrepancy. We are aware of the convention there but preferred to default to being more in line with our other Firebase SDKs

integration/test_auth.py Outdated Show resolved Hide resolved
integration/test_auth.py Outdated Show resolved Hide resolved
integration/test_auth.py Outdated Show resolved Hide resolved
Copy link
Member

@lahirumaramba lahirumaramba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you! Please take a look at the failing lint tests

@stillmatic
Copy link
Contributor Author

cool - I removed that test and the lint is passing locally now

Copy link
Contributor

@jonathanedey jonathanedey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you for your contribution.

@stillmatic
Copy link
Contributor Author

thanks for the help reviewing! I've rebased on upstream main and everything should be green.

Copy link

@egilmorez egilmorez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG with tiny nit, thanks!

firebase_admin/auth.py Outdated Show resolved Hide resolved
@jonathanedey jonathanedey merged commit 44b7568 into firebase:master Oct 26, 2023
9 checks passed
@lahirumaramba lahirumaramba changed the title feat: add clockSkewInSeconds feat(auth): Add clockSkewInSeconds Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants