Setup encryption and decryption for google oauth tokens using AWS KMS #2067
Conversation
Summary by CodeRabbit
WalkthroughAdds AWS KMS integration and Google OAuth scaffolding: new env vars and settings, Google OAuth client factory, AWS KMS client and encrypted model field, a MemberGoogleCredentials model storing encrypted tokens with auth/refresh flows, dependency additions, and comprehensive tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes(No out-of-scope changes identified.) Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
ahmedxgouda
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (9)
backend/.env.example (2)
15-18: Satisfy dotenv-linter ordering warnings by regrouping Google OAuth keys.The linter suggests moving the three GOOGLE_AUTH keys before IS_AWS_KMS_ENABLED. Purely cosmetic but keeps CI/tools quiet.
Apply:
-DJANGO_IS_AWS_KMS_ENABLED=False -DJANGO_GOOGLE_AUTH_CLIENT_ID=None -DJANGO_GOOGLE_AUTH_CLIENT_SECRET=None -DJANGO_GOOGLE_AUTH_REDIRECT_URI=http://localhost:8000/integrations/slack/oauth2/callback/ +DJANGO_GOOGLE_AUTH_CLIENT_ID=None +DJANGO_GOOGLE_AUTH_CLIENT_SECRET=None +DJANGO_GOOGLE_AUTH_REDIRECT_URI=http://localhost:8000/integrations/slack/oauth2/callback/ +DJANGO_IS_AWS_KMS_ENABLED=False
16-18: Confirm redirect URI matches the one registered in Google Cloud Console.Callback mismatch will cause OAuth errors (redirect_uri_mismatch).
If you want, I can add a README snippet showing how to configure the redirect URI in Google Cloud and where this value is read in settings.
backend/apps/slack/migrations/0020_alter_googleauth_access_token_and_more.py (1)
15-21: BinaryField with blank=True but null not allowed; ensure data safety and consider squashing with 0021.This migration flips token fields to BinaryField(blank=True) without null=True, then 0021 changes them again to allow nulls. If applied independently, existing rows without values could violate NOT NULL constraints. Consider squashing 0020 and 0021 into a single migration that directly sets BinaryField(null=True) to reduce churn and risk.
Confirm your migration order in CI: Django 5.2.4 generated this, while pyproject pins Django ^5.1. Ensure no incompatibilities when running migrations under 5.1.
backend/apps/common/clients.py (1)
49-49: Consider implementing the KmsClient as a singleton to avoid multiple boto3 client instantiations.Multiple instances of boto3 clients can lead to unnecessary connection overhead. Consider implementing a proper singleton pattern or dependency injection.
backend/tests/apps/slack/models/google_auth_test.py (1)
210-212: Potentially sensitive test data should use more clearly marked test values.While the comment
# NOSONARis present, using realistic-looking token values in tests could be confusing. Consider using clearly marked test data.Use clearly marked test tokens:
-mock_credentials.token = b"token" # NOSONAR -mock_credentials.refresh_token = b"refresh_token" +mock_credentials.token = b"TEST_ACCESS_TOKEN_DO_NOT_USE" # NOSONAR +mock_credentials.refresh_token = b"TEST_REFRESH_TOKEN_DO_NOT_USE"backend/apps/slack/models/google_auth.py (4)
74-74: Variable assignment is unnecessary.The variable
kms_clientis assigned but used immediately on the next lines. You can use the result directly.Remove unnecessary variable:
-kms_client = GoogleAuth.get_kms_client() flow.redirect_uri = settings.GOOGLE_AUTH_REDIRECT_URI flow.fetch_token(authorization_response=auth_response) -auth.access_token = kms_client.encrypt(flow.credentials.token) -auth.refresh_token = kms_client.encrypt(flow.credentials.refresh_token) +auth.access_token = GoogleAuth.get_kms_client().encrypt(flow.credentials.token) +auth.refresh_token = GoogleAuth.get_kms_client().encrypt(flow.credentials.refresh_token)
38-39: Use tuple unpacking for clarity.The
get_or_createmethod returns a tuple, but the code uses index access which is less readable.Use tuple unpacking:
-auth = GoogleAuth.objects.get_or_create(member=member)[0] +auth, _ = GoogleAuth.objects.get_or_create(member=member)
71-71: Use tuple unpacking for consistency.Similar to the previous comment, use tuple unpacking for better readability.
-auth = GoogleAuth.objects.get_or_create(member=member)[0] +auth, _ = GoogleAuth.objects.get_or_create(member=member)
100-110: Properties for decrypted tokens could raise exceptions.The
access_token_strandrefresh_token_strproperties callget_kms_client()which can raise a ValueError if KMS is not enabled. Consider documenting this behavior or handling the exception.Add error handling or documentation:
@property def access_token_str(self): - """Return the access token as a string.""" + """Return the decrypted access token as a string. + + Raises: + ValueError: If AWS KMS is not enabled. + """ client = GoogleAuth.get_kms_client() return client.decrypt(self.access_token) if self.access_token else None
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
backend/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
backend/.env.example(1 hunks)backend/apps/common/clients.py(1 hunks)backend/apps/slack/migrations/0019_googleauth.py(1 hunks)backend/apps/slack/migrations/0020_alter_googleauth_access_token_and_more.py(1 hunks)backend/apps/slack/migrations/0021_alter_googleauth_access_token_and_more.py(1 hunks)backend/apps/slack/models/__init__.py(1 hunks)backend/apps/slack/models/google_auth.py(1 hunks)backend/pyproject.toml(1 hunks)backend/settings/base.py(1 hunks)backend/tests/apps/slack/models/google_auth_test.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
backend/apps/slack/models/__init__.py (1)
backend/apps/slack/models/google_auth.py (1)
GoogleAuth(17-143)
backend/tests/apps/slack/models/google_auth_test.py (2)
backend/apps/slack/models/google_auth.py (6)
GoogleAuth(17-143)is_token_expired(113-117)get_flow(87-91)authenticate(34-54)refresh_access_token(120-139)authenticate_callback(57-84)backend/apps/slack/models/member.py (1)
Member(10-78)
backend/apps/slack/models/google_auth.py (2)
backend/apps/common/clients.py (3)
get_google_auth_client(8-21)encrypt(42-46)decrypt(36-40)backend/apps/slack/models/member.py (1)
Member(10-78)
🪛 dotenv-linter (3.3.0)
backend/.env.example
[warning] 16-16: [UnorderedKey] The DJANGO_GOOGLE_AUTH_CLIENT_ID key should go before the DJANGO_IS_AWS_KMS_ENABLED key
(UnorderedKey)
[warning] 17-17: [UnorderedKey] The DJANGO_GOOGLE_AUTH_CLIENT_SECRET key should go before the DJANGO_IS_AWS_KMS_ENABLED key
(UnorderedKey)
[warning] 18-18: [UnorderedKey] The DJANGO_GOOGLE_AUTH_REDIRECT_URI key should go before the DJANGO_IS_AWS_KMS_ENABLED key
(UnorderedKey)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (2)
backend/apps/common/clients.py (1)
8-21: Consider adding input validation and error handling for the Google OAuth client.The function should validate that required settings are present and handle potential exceptions when creating the Flow client.
Add validation and error handling:
def get_google_auth_client(): """Get a Google OAuth client.""" + if not all([ + settings.GOOGLE_AUTH_CLIENT_ID, + settings.GOOGLE_AUTH_CLIENT_SECRET, + settings.GOOGLE_AUTH_REDIRECT_URI + ]): + raise ValueError("Google OAuth configuration is incomplete") + return Flow.from_client_config( client_config={ "web": { "client_id": settings.GOOGLE_AUTH_CLIENT_ID, "client_secret": settings.GOOGLE_AUTH_CLIENT_SECRET, "redirect_uris": [settings.GOOGLE_AUTH_REDIRECT_URI], "auth_uri": "https://accounts.google.com/o/oauth2/v2/auth", "token_uri": "https://oauth2.googleapis.com/token", } }, scopes=["https://www.googleapis.com/auth/calendar.readonly"], )⛔ Skipped due to learnings
Learnt from: ahmedxgouda PR: OWASP/Nest#2021 File: backend/apps/slack/models/google_auth.py:39-52 Timestamp: 2025-08-12T17:38:56.165Z Learning: In the OWASP Nest project, IS_GOOGLE_AUTH_ENABLED in backend/settings/base.py already validates that all three Google OAuth settings (CLIENT_ID, CLIENT_SECRET, REDIRECT_URI) are present and not None/"None"/"", so additional validation in GoogleAuth.get_flow() is redundant.backend/apps/slack/migrations/0021_alter_googleauth_access_token_and_more.py (1)
1-27: LGTM! Migration properly updates field nullability.The migration correctly updates the GoogleAuth model fields to allow null values, which is appropriate for optional OAuth tokens and expiry timestamps.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
backend/tests/apps/slack/models/google_auth_test.py (2)
19-26: Fixture creates an unsaved Member; DB-backed relationship tests will be flakySome tests (e.g., one-to-one relationship) expect persisted relations; using an in-memory Member will break those. Persist the Member (and required workspace) under @pytest.mark.django_db or switch to a factory.
I can help wire a minimal Workspace + Member factory and update the tests accordingly if you confirm the workspace model path and required fields.
53-59: One-to-one relationship assertion requires persisted instancesAccessing
self.member.google_authwon’t work until the GoogleAuth instance is saved and the Member exists in the DB.Example fix (requires DB and a valid workspace):
- def test_one_to_one_relationship(self): + @pytest.mark.django_db + def test_one_to_one_relationship(self): """Test one-to-one relationship with Member.""" - auth = GoogleAuth(member=self.member, access_token=self.valid_token) - - assert self.member.google_auth == auth - assert auth.member == self.member + # Create and persist the required related objects here (e.g., Workspace, Member) + # member = Member.objects.create(...workspace=workspace, slack_user_id="U123...", username="testuser") + # auth = GoogleAuth.objects.create(member=member, access_token=self.valid_token) + # assert member.google_auth == auth + # assert auth.member == memberIf setting up Workspace is non-trivial, consider using FactoryBoy factories.
backend/apps/slack/models/google_auth.py (2)
82-88: Null checks before encrypting tokens from credentialsRefresh tokens aren’t always returned (e.g., subsequent authorizations). Encrypt only when present to avoid calling KMS with None.
- auth.access_token = kms_client.encrypt(flow.credentials.token) - auth.refresh_token = kms_client.encrypt(flow.credentials.refresh_token) + if flow.credentials.token: + auth.access_token = kms_client.encrypt(flow.credentials.token) + if flow.credentials.refresh_token: + auth.refresh_token = kms_client.encrypt(flow.credentials.refresh_token)
133-145: Decrypt before refresh; re-encrypt refreshed tokens; normalize expiry awarenessStored refresh_token is encrypted (BinaryField). It must be decrypted before calling fetch_token. Also, refreshed tokens should be encrypted before saving. Finally, ensure expiry is timezone-aware.
@staticmethod def refresh_access_token(auth): """Refresh the access token using the refresh token.""" if not settings.IS_GOOGLE_AUTH_ENABLED: raise ValueError(AUTH_ERROR_MESSAGE) refresh_error = "Google OAuth refresh token is not set or expired." if not auth.refresh_token: raise ValidationError(refresh_error) - flow = GoogleAuth.get_flow() - flow.fetch_token( - refresh_token=auth.refresh_token, + flow = GoogleAuth.get_flow() + kms = GoogleAuth.get_kms_client() + decrypted_refresh = kms.decrypt(auth.refresh_token) + flow.fetch_token( + refresh_token=decrypted_refresh, client_id=settings.GOOGLE_AUTH_CLIENT_ID, client_secret=settings.GOOGLE_AUTH_CLIENT_SECRET, ) - credentials = flow.credentials - auth.access_token = credentials.token - auth.refresh_token = credentials.refresh_token - auth.expires_at = credentials.expiry + credentials = flow.credentials + if credentials.token: + auth.access_token = kms.encrypt(credentials.token) + if credentials.refresh_token: + auth.refresh_token = kms.encrypt(credentials.refresh_token) + expires_at = credentials.expiry + if expires_at and timezone.is_naive(expires_at): + expires_at = timezone.make_aware(expires_at) + auth.expires_at = expires_at auth.save()
🧹 Nitpick comments (9)
backend/tests/apps/common/clients_test.py (2)
14-22: Patch where used; also consider constructing KmsClient under the overridden settingsPatching the symbol where it’s used is more robust and avoids surprises if import paths change. Also note: KmsClient reads AWS_REGION during init; since you construct it inside the fixture (before the per-test override_settings is applied), region assertions at client construction time can’t be validated in these tests.
Apply this tighter patch target:
- with patch("boto3.client") as mock_boto3_client: + with patch("apps.common.clients.boto3.client") as mock_boto3_client:If you want the boto client to pick up the per-test AWS_REGION override, instantiate KmsClient inside each test (after override_settings) instead of in the fixture.
23-33: Assert the exact ciphertext returned by the mock (avoid the always-true inequality)
ciphertext != plaintextis always True here because it compares bytes to str. Assert the expected mock value to make the test meaningful.- ciphertext = self.kms_client.encrypt(plaintext) - assert ciphertext != plaintext + ciphertext = self.kms_client.encrypt(plaintext) + assert ciphertext == b"encrypted"backend/.env.example (1)
15-18: Reorder keys to satisfy dotenv-linter warningsThe linter suggests placing GOOGLE_AUTH_* keys before DJANGO_IS_AWS_KMS_ENABLED. Reordering keeps noise out of CI and improves readability.
-DJANGO_IS_AWS_KMS_ENABLED=False -DJANGO_GOOGLE_AUTH_CLIENT_ID=None -DJANGO_GOOGLE_AUTH_CLIENT_SECRET=None -DJANGO_GOOGLE_AUTH_REDIRECT_URI=http://localhost:8000/integrations/slack/oauth2/callback/ +DJANGO_GOOGLE_AUTH_CLIENT_ID=None +DJANGO_GOOGLE_AUTH_CLIENT_SECRET=None +DJANGO_GOOGLE_AUTH_REDIRECT_URI=http://localhost:8000/integrations/slack/oauth2/callback/ +DJANGO_IS_AWS_KMS_ENABLED=Falsebackend/tests/apps/slack/models/google_auth_test.py (4)
27-31: Remove unused boto3 patch in this moduleThe patch context exits before any code uses a boto3 client here; and these tests don’t rely on direct boto3 calls. This block adds overhead without effect.
- with patch("boto3.client") as mock_boto3_client: - self.mock_boto3_client = mock_boto3_client - self.mock_kms_client = Mock() - self.mock_boto3_client.return_value = self.mock_kms_client + # No boto3 usage in this module; patch not required
100-124: Use (obj, False) to reflect “existing” auth in test semanticsYou’re simulating “existing with valid token” but returning (obj, True). It works because code uses only [0], but it’s semantically confusing.
- mock_get_or_create.return_value = ( + mock_get_or_create.return_value = ( GoogleAuth( member=self.member, access_token=self.valid_token, refresh_token=self.valid_refresh_token, expires_at=self.future_time, ), - True, + False, )
281-312: Use string tokens from Flow.credentials to match real library behavior and KMS APIgoogle-auth returns string tokens; KMS encrypt expects str input. Setting bytes here works only because encrypt is mocked. Use str to align types.
- mock_credentials = Mock() - mock_credentials.token = b"token" # NOSONAR - mock_credentials.refresh_token = b"refresh_token" + mock_credentials = Mock() + mock_credentials.token = "token" + mock_credentials.refresh_token = "refresh_token" @@ - mock_get_kms_client.return_value.encrypt.assert_any_call(b"token") - mock_get_kms_client.return_value.encrypt.assert_any_call(b"refresh_token") + mock_get_kms_client.return_value.encrypt.assert_any_call("token") + mock_get_kms_client.return_value.encrypt.assert_any_call("refresh_token")
60-81: Minor suggestion: also assert the authenticate() return value is a tuple (url, state)Optional strengthening: capture and assert the shape of the return from authorization_url for the first-time path.
- GoogleAuth.authenticate(self.member) + auth_url, state = GoogleAuth.authenticate(self.member) + assert isinstance(auth_url, str) + assert state == self.member.slack_user_idbackend/apps/slack/models/google_auth.py (2)
61-90: Optional: derive member_id from OAuth state to tighten callback trust boundaryYou generate state during authenticate(); consider deriving member_id from the validated OAuth state during callback instead of accepting it as a separate parameter. This avoids mismatches between the incoming state and the provided member_id.
I can draft a version that pulls member_id from the validated state in the authorization_response and cross-checks it against the DB if helpful.
98-104: Docstring nit: this returns a shared client, not a new instanceThe method returns the module-level kms_client; tiny docstring tweak would avoid confusion.
- """Create a KMS client instance.""" + """Return the shared KMS client instance."""
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
backend/.env.example(1 hunks)backend/apps/common/clients.py(1 hunks)backend/apps/slack/models/google_auth.py(1 hunks)backend/settings/base.py(1 hunks)backend/tests/apps/common/clients_test.py(1 hunks)backend/tests/apps/slack/models/google_auth_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/apps/common/clients.py
- backend/settings/base.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/tests/apps/common/clients_test.py (1)
backend/apps/common/clients.py (3)
KmsClient(24-44)encrypt(40-44)decrypt(34-38)
backend/apps/slack/models/google_auth.py (2)
backend/apps/common/clients.py (3)
get_google_auth_client(8-21)encrypt(40-44)decrypt(34-38)backend/apps/slack/models/member.py (1)
Member(10-78)
🪛 dotenv-linter (3.3.0)
backend/.env.example
[warning] 16-16: [UnorderedKey] The DJANGO_GOOGLE_AUTH_CLIENT_ID key should go before the DJANGO_IS_AWS_KMS_ENABLED key
(UnorderedKey)
[warning] 17-17: [UnorderedKey] The DJANGO_GOOGLE_AUTH_CLIENT_SECRET key should go before the DJANGO_IS_AWS_KMS_ENABLED key
(UnorderedKey)
[warning] 18-18: [UnorderedKey] The DJANGO_GOOGLE_AUTH_REDIRECT_URI key should go before the DJANGO_IS_AWS_KMS_ENABLED key
(UnorderedKey)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (4)
backend/tests/apps/common/clients_test.py (1)
34-42: LGTM – decrypt path and parameter assertions look correctGood coverage for parameter passing and UTF-8 decoding behavior.
backend/tests/apps/slack/models/google_auth_test.py (2)
268-273: Nice coverage for “KMS disabled” pathThis closes a gap from earlier iterations and validates the feature flag gating.
334-366: LGTM – decryption accessors are properly exercisedSolid verification that properties go through KMS decryption and return strings.
backend/apps/slack/models/google_auth.py (1)
120-122: Bug: timezone.timedelta doesn’t exist; use datetime.timedeltaThis property will raise AttributeError at runtime. Use datetime.timedelta (and import it) or Django’s timezone.now combined with datetime.timedelta.
-from django.utils import timezone +from django.utils import timezone +from datetime import timedelta @@ - return self.expires_at is None or self.expires_at <= timezone.now() + timezone.timedelta( - seconds=60 - ) + return self.expires_at is None or self.expires_at <= timezone.now() + timedelta(seconds=60)⛔ Skipped due to learnings
Learnt from: ahmedxgouda PR: OWASP/Nest#2021 File: backend/apps/slack/models/google_auth.py:77-81 Timestamp: 2025-08-12T17:45:36.105Z Learning: django.utils.timezone.timedelta exists and is an alias for datetime.timedelta. It can be used directly after importing timezone from django.utils.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/apps/nest/migrations/0007_alter_membergooglecredentials_access_token_and_more.py (1)
5-5: Importing app code in migrations is acceptable for Field classes; ensure path stabilityReferencing apps.common.model_fields.KmsEncryptedField in migrations is fine because Django serializes field deconstruction. However, avoid moving/renaming the module path in future refactors; historical migrations import by path and will break if it changes. Document this in a code comment near the field definition or in CONTRIBUTING.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
backend/apps/nest/migrations/0007_alter_membergooglecredentials_access_token_and_more.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ahmedxgouda
PR: OWASP/Nest#2021
File: backend/apps/slack/models/google_auth.py:25-36
Timestamp: 2025-08-16T13:23:34.874Z
Learning: In the OWASP Nest project, encryption and decryption for Google OAuth tokens stored in BinaryField are being handled in a separate PR, not in the initial Google Auth setup implementation.
🧬 Code Graph Analysis (1)
backend/apps/nest/migrations/0007_alter_membergooglecredentials_access_token_and_more.py (1)
backend/apps/common/model_fields.py (1)
KmsEncryptedField(13-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run CI Denendencies Scan
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (2)
backend/apps/nest/migrations/0007_alter_membergooglecredentials_access_token_and_more.py (2)
17-19: LGTM: nullable + clear verbose_name for access_tokenNullability for tokens is reasonable during OAuth bootstrap/rotation, and verbose_name is clear.
24-26: LGTM: nullable + clear verbose_name for refresh_tokenMatches access_token semantics; consistent and clear.
backend/apps/nest/migrations/0007_alter_membergooglecredentials_access_token_and_more.py
Outdated
Show resolved
Hide resolved
ahmedxgouda
force-pushed
the
nestbot-calendar/setup-aws-kms
branch
from
1e91714 to
03754c1
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
backend/apps/common/model_fields.py (2)
24-27: Accept bytes-like plaintext on write for ergonomicsMany callers hand around tokens as bytes/bytearray/memoryview. You already accept bytes-like on read; mirror that on write to reduce friction.
Apply:
- if not isinstance(value, str): - raise TypeError(STR_ERROR_MESSAGE) - ciphertext = get_kms_client().encrypt(value) + if isinstance(value, (bytes, bytearray, memoryview)): + value = bytes(value).decode("utf-8") + elif not isinstance(value, str): + raise TypeError(STR_ERROR_MESSAGE) + ciphertext = get_kms_client().encrypt(value)
8-11: Error message doesn’t mention memoryview; align text with accepted typesdecrypt accepts (bytes, bytearray, memoryview) but the message mentions only bytes/bytearray.
Apply:
-BYTES_ERROR_MESSAGE = "Value must be bytes or bytearray to decrypt." +BYTES_ERROR_MESSAGE = "Value must be bytes, bytearray, or memoryview to decrypt."backend/apps/nest/models/member_google_credentials.py (5)
14-17: Clarify the auth error message or validate config explicitlyThe message implies missing client_id/secret/redirect URI, but code only checks IS_GOOGLE_AUTH_ENABLED. Either:
- Reword the message to reflect the feature flag, or
- Add a guard in get_flow() to validate GOOGLE_AUTH_CLIENT_ID/SECRET/REDIRECT_URI and raise a precise error.
Option A (reword):
-AUTH_ERROR_MESSAGE = ( - "Google OAuth client ID, secret, and redirect URI must be set in environment variables." -) +AUTH_ERROR_MESSAGE = ( + "Google OAuth is disabled or misconfigured. Ensure IS_GOOGLE_AUTH_ENABLED=True and GOOGLE_AUTH_* settings are set." +)Option B (config validation in get_flow) is outlined in a later comment.
23-26: Grammar nit: plural verbose nameUse the plural possessive or a simpler pluralized phrase.
Apply:
- verbose_name_plural = "Member's Google Credentials" + verbose_name_plural = "Members' Google credentials"
33-38: Consider adding blank=True for admin/formsTokens are nullable and written programmatically, but if you ever expose these in admin or forms, blank=True avoids validation noise.
Apply:
- access_token = KmsEncryptedField(verbose_name="Access Token", null=True) - refresh_token = KmsEncryptedField(verbose_name="Refresh Token", null=True) + access_token = KmsEncryptedField(verbose_name="Access Token", null=True, blank=True) + refresh_token = KmsEncryptedField(verbose_name="Refresh Token", null=True, blank=True)
41-68: Avoid eager row creation and gracefully fall back to OAuth when refresh fails
- get_or_create() creates a DB row even for first-time auth, causing unnecessary writes.
- If refresh_access_token raises (e.g., missing/expired refresh token), best UX is to fall back to OAuth instead of surfacing an error to callers.
Apply:
- auth = MemberGoogleCredentials.objects.get_or_create(member=member)[0] - if auth.access_token and not auth.is_token_expired: - return auth - if auth.access_token: - # If the access token is present but expired, refresh it - MemberGoogleCredentials.refresh_access_token(auth) - return auth + auth = MemberGoogleCredentials.objects.filter(member=member).first() + if auth and auth.access_token: + if not auth.is_token_expired: + return auth + try: + # If the access token is present but expired, refresh it + MemberGoogleCredentials.refresh_access_token(auth) + return auth + except ValidationError: + # Fall back to fresh OAuth if refresh token is missing/invalid + pass # If no access token is present, redirect to Google OAuth
99-105: Optionally validate Google OAuth config at sourceIf you want a precise error instead of relying on the feature flag, validate required settings here before constructing Flow.
Apply:
def get_flow(): """Create a Google OAuth flow instance.""" - if not settings.IS_GOOGLE_AUTH_ENABLED: - raise ValueError(AUTH_ERROR_MESSAGE) + if not settings.IS_GOOGLE_AUTH_ENABLED: + raise ValueError(AUTH_ERROR_MESSAGE) + required = ( + settings.GOOGLE_AUTH_CLIENT_ID, + settings.GOOGLE_AUTH_CLIENT_SECRET, + settings.GOOGLE_AUTH_REDIRECT_URI, + settings.GOOGLE_AUTH_AUTH_URI, + settings.GOOGLE_AUTH_TOKEN_URI, + ) + if not all(required): + raise ValueError(AUTH_ERROR_MESSAGE) return get_google_auth_client()
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
backend/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
backend/.env.example(1 hunks)backend/apps/common/clients.py(1 hunks)backend/apps/common/model_fields.py(1 hunks)backend/apps/nest/models/member_google_credentials.py(1 hunks)backend/settings/base.py(1 hunks)backend/tests/apps/common/clients_test.py(1 hunks)backend/tests/apps/common/model_fileds_test.py(1 hunks)backend/tests/apps/nest/models/member_google_credentials_test.py(1 hunks)cspell/custom-dict.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- cspell/custom-dict.txt
- backend/tests/apps/common/clients_test.py
- backend/settings/base.py
- backend/tests/apps/nest/models/member_google_credentials_test.py
- backend/apps/common/clients.py
- backend/.env.example
- backend/tests/apps/common/model_fileds_test.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ahmedxgouda
PR: OWASP/Nest#2021
File: backend/apps/slack/models/google_auth.py:25-36
Timestamp: 2025-08-16T13:23:34.874Z
Learning: In the OWASP Nest project, encryption and decryption for Google OAuth tokens stored in BinaryField are being handled in a separate PR, not in the initial Google Auth setup implementation.
📚 Learning: 2025-08-19T15:34:29.661Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2089
File: backend/apps/common/model_fields.py:18-27
Timestamp: 2025-08-19T15:34:29.661Z
Learning: The KmsEncryptedField in backend/apps/common/model_fields.py is designed to be a KMS-only field that requires IS_AWS_KMS_ENABLED=True for any operation, including storing NULL values. This enforces a strict security model where the field cannot be used without proper KMS configuration.
Applied to files:
backend/apps/common/model_fields.py
🧬 Code graph analysis (2)
backend/apps/nest/models/member_google_credentials.py (3)
backend/apps/common/clients.py (1)
get_google_auth_client(8-21)backend/apps/common/model_fields.py (1)
KmsEncryptedField(13-47)backend/apps/slack/models/member.py (1)
Member(10-78)
backend/apps/common/model_fields.py (1)
backend/apps/common/clients.py (3)
get_kms_client(50-55)encrypt(40-44)decrypt(36-38)
🔇 Additional comments (5)
backend/apps/common/model_fields.py (2)
18-27: Strict KMS gating and DB normalization look correct
- Enforcing IS_AWS_KMS_ENABLED before any operation (including storing NULL) matches the stated security posture in team learnings and prevents accidental plaintext writes.
- Returning super().get_prep_value(ciphertext) preserves backend normalization for BinaryField.
29-47: from_db_value/to_python pathing is solid
- Handling memoryview/bytearray → bytes before decrypt is correct for BinaryField.
- Returning plaintext str for form inputs avoids double-decrypt.
- Strict KMS gating during read aligns with the “KMS-only field” decision.
backend/apps/nest/models/member_google_credentials.py (3)
71-98: Callback flow looks good
- Properly fetches tokens and makes expiry timezone-aware.
- Using get_or_create here is appropriate to materialize the row on first completion of OAuth.
109-111: Token expiry skew logic is sensible60s skew is a practical buffer against clock drift and racey calls.
139-141: String representation is fineClear and useful for logs/admin.
| def authenticate(member): | ||
| """Authenticate a member. | ||
|
|
||
| Returns: | ||
| - MemberGoogleCredentials instance if a valid/refreshable token exists, or | ||
| - (authorization_url, state) tuple to complete the OAuth flow. | ||
|
|
||
| """ | ||
| if not settings.IS_GOOGLE_AUTH_ENABLED: | ||
| raise ValueError(AUTH_ERROR_MESSAGE) | ||
| if not settings.IS_AWS_KMS_ENABLED: | ||
| raise ValueError(KMS_ERROR_MESSAGE) | ||
| auth = MemberGoogleCredentials.objects.get_or_create(member=member)[0] | ||
| if auth.access_token and not auth.is_token_expired: | ||
| return auth | ||
| if auth.access_token: | ||
| # If the access token is present but expired, refresh it | ||
| MemberGoogleCredentials.refresh_access_token(auth) | ||
| return auth | ||
| # If no access token is present, redirect to Google OAuth | ||
| flow = MemberGoogleCredentials.get_flow() | ||
| flow.redirect_uri = settings.GOOGLE_AUTH_REDIRECT_URI | ||
| state = member.slack_user_id | ||
| return flow.authorization_url( | ||
| access_type="offline", | ||
| prompt="consent", | ||
| state=state, | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add/confirm test coverage: refresh fallback and refresh_token retention
Two key paths deserve explicit tests:
- Fallback to OAuth when refresh fails in authenticate (e.g., missing refresh token).
- Retaining the previous refresh_token when the refresh response omits it.
Run:
Also applies to: 114-138
🏁 Script executed:
#!/bin/bash
# 1) Look for tests covering refresh fallback in authenticate
rg -nC3 -g '!**/node_modules/**' --type=py 'authenticate\(.+member\)|refresh_access_token\(' backend/tests
# 2) Grep for assertions that refresh_token remains unchanged after refresh when new is None
rg -nC3 -g '!**/node_modules/**' --type=py 'refresh_token.*(unchanged|None|preserv|retain)' backend/tests
# 3) If missing, I can draft parametric tests for:
# - refresh returns no refresh_token -> old retained
# - refresh raises -> authenticate returns (authorization_url, state)Length of output: 8201
Add tests for OAuth fallback in authenticate and refresh_token retention
The existing suite covers successful authenticate/refresh paths but is missing two critical scenarios:
-
Fallback to OAuth when
refresh_access_tokenfails inauthenticate
When an expiredaccess_tokenhas no validrefresh_token,MemberGoogleCredentials.authenticateshould catch the refresh failure and return(authorization_url, state)instead of propagating an exception. -
Retention of the old
refresh_tokenwhen the refresh response omits a new one
Inrefresh_access_token, if the token endpoint returns a newaccess_tokenbut norefresh_token, the model’s existingrefresh_tokenmust remain unchanged.
Suggested test additions in backend/tests/apps/nest/models/member_google_credentials_test.py:
-
Parametrize
authenticatefor:
•existing_auth.access_tokenexpired andexisting_auth.refresh_tokenisNone
→ Assertauthenticate(member)returns a 2-tuple(url, state)
•refresh_access_tokenraisesValidationError(e.g. invalid/missing refresh token)
→ Assert fallback to OAuth flow, not an uncaught exception -
Test for
refresh_access_token:
• Stub the OAuth client to return only anaccess_token(norefresh_token)
→ Assertauth.access_tokenupdates whileauth.refresh_tokenremains the original value
🤖 Prompt for AI Agents
In backend/apps/nest/models/member_google_credentials.py around lines 41-68,
update authenticate to handle refresh failures by catching the exception raised
by refresh_access_token (e.g., ValidationError) and returning the OAuth fallback
(flow.authorization_url(...), state) instead of letting the error propagate; and
update refresh_access_token to only overwrite auth.refresh_token when the token
response contains a refresh_token (i.e., if response.get("refresh_token") is
truthy), otherwise keep the existing refresh_token while updating access_token
and expiry fields. Ensure the authenticate path still returns the flow tuple
using MemberGoogleCredentials.get_flow() and member.slack_user_id as state when
falling back.
| def refresh_access_token(auth): | ||
| """Refresh the access token using the refresh token.""" | ||
| if not settings.IS_GOOGLE_AUTH_ENABLED: | ||
| raise ValueError(AUTH_ERROR_MESSAGE) | ||
| if not settings.IS_AWS_KMS_ENABLED: | ||
| raise ValueError(KMS_ERROR_MESSAGE) | ||
| refresh_error = "Google OAuth refresh token is not set or expired." | ||
| if not auth.refresh_token: | ||
| raise ValidationError(refresh_error) | ||
| credentials = Credentials( | ||
| token=auth.access_token, | ||
| refresh_token=auth.refresh_token, | ||
| token_uri=settings.GOOGLE_AUTH_TOKEN_URI, | ||
| client_id=settings.GOOGLE_AUTH_CLIENT_ID, | ||
| client_secret=settings.GOOGLE_AUTH_CLIENT_SECRET, | ||
| ) | ||
| credentials.refresh(Request()) | ||
| auth.access_token = credentials.token | ||
| auth.refresh_token = credentials.refresh_token | ||
| expires_at = credentials.expiry | ||
| if expires_at and timezone.is_naive(expires_at): | ||
| expires_at = timezone.make_aware(expires_at) | ||
| auth.expires_at = expires_at | ||
| auth.save() | ||
|
|
There was a problem hiding this comment.
Bug: Preserve existing refresh_token when Google doesn’t return a new one
During refresh, Google often omits refresh_token. Current code overwrites with None, breaking future refreshes.
Apply:
refresh_error = "Google OAuth refresh token is not set or expired."
if not auth.refresh_token:
raise ValidationError(refresh_error)
+ # Preserve the existing refresh token if Google doesn't return a new one.
+ previous_refresh_token = auth.refresh_token
credentials = Credentials(
token=auth.access_token,
refresh_token=auth.refresh_token,
token_uri=settings.GOOGLE_AUTH_TOKEN_URI,
client_id=settings.GOOGLE_AUTH_CLIENT_ID,
client_secret=settings.GOOGLE_AUTH_CLIENT_SECRET,
)
credentials.refresh(Request())
auth.access_token = credentials.token
- auth.refresh_token = credentials.refresh_token
+ auth.refresh_token = credentials.refresh_token or previous_refresh_token
expires_at = credentials.expiry
if expires_at and timezone.is_naive(expires_at):
expires_at = timezone.make_aware(expires_at)
auth.expires_at = expires_at
auth.save()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def refresh_access_token(auth): | |
| """Refresh the access token using the refresh token.""" | |
| if not settings.IS_GOOGLE_AUTH_ENABLED: | |
| raise ValueError(AUTH_ERROR_MESSAGE) | |
| if not settings.IS_AWS_KMS_ENABLED: | |
| raise ValueError(KMS_ERROR_MESSAGE) | |
| refresh_error = "Google OAuth refresh token is not set or expired." | |
| if not auth.refresh_token: | |
| raise ValidationError(refresh_error) | |
| credentials = Credentials( | |
| token=auth.access_token, | |
| refresh_token=auth.refresh_token, | |
| token_uri=settings.GOOGLE_AUTH_TOKEN_URI, | |
| client_id=settings.GOOGLE_AUTH_CLIENT_ID, | |
| client_secret=settings.GOOGLE_AUTH_CLIENT_SECRET, | |
| ) | |
| credentials.refresh(Request()) | |
| auth.access_token = credentials.token | |
| auth.refresh_token = credentials.refresh_token | |
| expires_at = credentials.expiry | |
| if expires_at and timezone.is_naive(expires_at): | |
| expires_at = timezone.make_aware(expires_at) | |
| auth.expires_at = expires_at | |
| auth.save() | |
| def refresh_access_token(auth): | |
| """Refresh the access token using the refresh token.""" | |
| if not settings.IS_GOOGLE_AUTH_ENABLED: | |
| raise ValueError(AUTH_ERROR_MESSAGE) | |
| if not settings.IS_AWS_KMS_ENABLED: | |
| raise ValueError(KMS_ERROR_MESSAGE) | |
| refresh_error = "Google OAuth refresh token is not set or expired." | |
| if not auth.refresh_token: | |
| raise ValidationError(refresh_error) | |
| # Preserve the existing refresh token if Google doesn't return a new one. | |
| previous_refresh_token = auth.refresh_token | |
| credentials = Credentials( | |
| token=auth.access_token, | |
| refresh_token=auth.refresh_token, | |
| token_uri=settings.GOOGLE_AUTH_TOKEN_URI, | |
| client_id=settings.GOOGLE_AUTH_CLIENT_ID, | |
| client_secret=settings.GOOGLE_AUTH_CLIENT_SECRET, | |
| ) | |
| credentials.refresh(Request()) | |
| auth.access_token = credentials.token | |
| auth.refresh_token = credentials.refresh_token or previous_refresh_token | |
| expires_at = credentials.expiry | |
| if expires_at and timezone.is_naive(expires_at): | |
| expires_at = timezone.make_aware(expires_at) | |
| auth.expires_at = expires_at | |
| auth.save() |
🤖 Prompt for AI Agents
In backend/apps/nest/models/member_google_credentials.py around lines 114 to
138, the refresh_access_token flow unconditionally overwrites auth.refresh_token
with credentials.refresh_token which can be None when Google omits it; update
the code to only replace auth.refresh_token when credentials.refresh_token is
present (i.e., if credentials.refresh_token: auth.refresh_token =
credentials.refresh_token) so the existing stored refresh token is preserved
when Google does not return a new one, then continue updating access token,
expiry and save.
Proposed change
Resolves #2043
Add the PR description here.
Checklist
make check-testlocally; all checks and tests passed.