-
Notifications
You must be signed in to change notification settings - Fork 329
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
Conversation
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. |
There was a problem hiding this 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!
I've addressed the feedback in the code change and rebased off main Note: this dependency uses |
1219d69
to
b932061
Compare
There was a problem hiding this 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
There was a problem hiding this 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
cool - I removed that test and the lint is passing locally now |
There was a problem hiding this 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.
per feedback in firebase#625 (comment) adds unit and integration tests as well. unit tests and lint pass.
thanks for the help reviewing! I've rebased on upstream main and everything should be green. |
There was a problem hiding this 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!
clockSkewInSeconds
Discussion
Testing
pytest
.API Changes