FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat(auth): Add `clockSkewInSeconds` by stillmatic · Pull Request #714 · firebase/firebase-admin-python · GitHub

/ firebase-admin-python Public

feat(auth): Add clockSkewInSeconds - #714

Merged
jonathanedey merged 9 commits into
firebase:masterfrom
stillmatic:master
Oct 26, 2023
ConversationCommits (9)ChecksFiles changed
Merged

feat(auth): Add clockSkewInSeconds#714
jonathanedey merged 9 commits into
firebase:masterfrom
stillmatic:master

Conversation

stillmatic commented Aug 23, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 Bot commented Aug 23, 2023

Copy link
Copy Markdown

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.

lahirumaramba self-requested a review September 25, 2023 19:11
jonathanedey requested changes Sep 26, 2023
View reviewed changes

jonathanedey left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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!

Comment thread firebase_admin/__about__.py Outdated
Comment thread firebase_admin/__init__.py Outdated
Comment thread firebase_admin/__init__.py Outdated
Comment thread firebase_admin/_auth_client.py Outdated
Comment thread firebase_admin/_auth_client.py Outdated
Comment thread firebase_admin/auth.py Outdated
Comment thread integration/test_auth.py Outdated
Comment thread integration/test_auth.py Outdated
Comment thread firebase_admin/auth.py Outdated

stillmatic commented Sep 26, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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.

stillmatic force-pushed the master branch 2 times, most recently from 1219d69 to b932061 Compare September 26, 2023 21:56
jonathanedey reviewed Sep 27, 2023
View reviewed changes

jonathanedey left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread integration/test_auth.py Outdated
Comment thread integration/test_auth.py Outdated
Comment thread integration/test_auth.py Outdated
lahirumaramba approved these changes Oct 12, 2023
View reviewed changes

lahirumaramba left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

stillmatic commented Oct 12, 2023

Copy link
Copy Markdown
Contributor Author

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

jonathanedey approved these changes Oct 12, 2023
View reviewed changes

jonathanedey left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM! Thank you for your contribution.

stillmatic commented Oct 12, 2023

Copy link
Copy Markdown
Contributor Author

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

egilmorez approved these changes Oct 24, 2023
View reviewed changes

egilmorez left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LG with tiny nit, thanks!

Comment thread firebase_admin/auth.py Outdated
jonathanedey merged commit 44b7568 into firebase:master Oct 26, 2023
This was referenced Oct 26, 2023
Closed
Closed
lahirumaramba changed the title feat: add clockSkewInSeconds feat(auth): Add clockSkewInSeconds Dec 4, 2023
HiraiKyo mentioned this pull request Jun 12, 2024
Merged
jonathanedey mentioned this pull request Oct 1, 2024
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

lahirumaramba lahirumaramba approved these changes
jonathanedey jonathanedey approved these changes
+1 more reviewer
egilmorez egilmorez approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

egilmorez

jonathanedey

Labels

api: auth release-note type: feature request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL