Skip to content

feat(auth): Add clockSkewInSeconds#714

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

Merged
merged 9 commits into from
Oct 26, 2023

Conversation

stillmatic
Copy link
Contributor

@stillmaticstillmatic 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

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

@jonathanedeyjonathanedey 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!

@stillmatic
Copy link
ContributorAuthor

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_secondshttps://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

@jonathanedeyjonathanedey 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

Copy link
Member

@lahirumarambalahirumaramba 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
ContributorAuthor

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

Copy link
Contributor

@jonathanedeyjonathanedey 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
ContributorAuthor

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

Copy link

@egilmorezegilmorez 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!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
close