Eng 1912 - support Google Service Account auth#7257
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
tests/ops/service/authentication/test_authentication_strategy_google_cloud_service_account.py
Fixed
Show fixed
Hide fixed
tests/ops/service/authentication/test_authentication_strategy_google_cloud_service_account.py
Fixed
Show fixed
Hide fixed
Improving on Keyfile readability
…y google cloud service account
b6ee716 to
d742ea0
Compare
Greptile OverviewGreptile SummaryThis PR adds a new authentication strategy for Google Cloud Service Account credentials, enabling Fides to authenticate HTTP requests to Google Cloud APIs using JWT-based OAuth2 flow. Key Changes:
Implementation Quality:
Confidence Score: 4/5
Important Files Changed
|
| def _is_close_to_expiration( | ||
| self, expires_at: int | ||
| ) -> bool: |
There was a problem hiding this comment.
Inconsistent datetime usage - this uses datetime.now(timezone.utc) while the OAuth2 base strategy uses datetime.utcnow() in authentication_strategy_oauth2_base.py:48. Consider standardizing on datetime.now(timezone.utc) throughout for timezone-aware datetimes.
There was a problem hiding this comment.
Would this be on the scope of this PR? datetime.utcnow()is deprecated on python 3.12 and higher. Could be its own task
There was a problem hiding this comment.
You're absolutely right that datetime.utcnow() is deprecated in Python 3.12+ and should be replaced with datetime.now(timezone.utc). However, this specific change would be outside the scope of this PR.
This PR is focused on implementing Google Service Account authentication, and the code you're adding correctly uses the modern datetime.now(timezone.utc) approach. Updating the existing OAuth2 base strategy would be a separate concern that affects other authentication strategies beyond just the Google one being added here.
This would indeed be better suited as its own task to:
- Audit all uses of
datetime.utcnow()across the codebase - Update them consistently to
datetime.now(timezone.utc) - Ensure no regressions in existing authentication flows
Your new code is following the correct modern pattern, so there's no issue with the implementation in this PR.
| @staticmethod | ||
| def get_configuration_model() -> StrategyConfiguration: | ||
| """Return the configuration model for this strategy.""" | ||
| return GoogleCloudServiceAccountConfiguration # type: ignore |
There was a problem hiding this comment.
get_configuration_model() should be a static method as it doesn't require instance state.
| @staticmethod | |
| def get_configuration_model() -> StrategyConfiguration: | |
| """Return the configuration model for this strategy.""" | |
| return GoogleCloudServiceAccountConfiguration # type: ignore | |
| @staticmethod | |
| def get_configuration_model() -> StrategyConfiguration: | |
| """Return the configuration model for this strategy.""" | |
| return GoogleCloudServiceAccountConfiguration # type: ignore |
Context Used: Rule from dashboard - Use static methods instead of instance methods when the method doesn't require instance state. (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Greptile got a little bit confused here 😅
There was a problem hiding this comment.
You're absolutely right! I see the confusion - the method is already marked as @staticmethod on line 321. My apologies for the incorrect comment. The method is properly implemented as a static method since it doesn't need access to instance state.
galvana
left a comment
There was a problem hiding this comment.
Approved! I just left one comment
src/fides/api/service/authentication/authentication_strategy_google_cloud_service_account.py
Outdated
Show resolved
Hide resolved
…oogle_cloud_service_account.py Co-authored-by: Adrian Galvan <adrian@ethyca.com>
Ticket ENG-1912
Description Of Changes
Implements a new authentication strategy for Google Cloud Service Account credentials, enabling Fides to authenticate HTTP requests to Google Cloud APIs using service account keys (JWT-based OAuth2 flow).
Contains:
Code Changes
Core Implementation
src/fides/api/service/authentication/authentication_strategy_google_cloud_service_account.py(322 lines)Configuration and registration
src/fides/api/schemas/saas/strategy_configuration.pySupportedAuthenticationStrategiesenumTests
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works