Implement Initial Setup for Google OAuth #2021
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the Summary by CodeRabbit
WalkthroughAdds Google OAuth support: new env variables and settings, dependencies for Google APIs, a common Google OAuth client factory, a Nest model (MemberGoogleCredentials) with OAuth flows and token management, corresponding migrations, Slack app migrations that briefly add and then remove a GoogleAuth model, and comprehensive tests for the new model. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25–30 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes(none) Suggested labels
Suggested reviewers
✨ 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (2)
backend/apps/slack/models/google_auth.py (1)
62-77: Naming clarity: prefer 'member' over 'user' for Slack member relationTo avoid confusion with AUTH_USER_MODEL (nest.User), rename the OneToOneField to member.
This is a new model; renaming now minimizes migration churn:
- user = models.OneToOneField( + member = models.OneToOneField( "slack.Member", on_delete=models.CASCADE, - related_name="google_auth", - verbose_name="Slack Member", + related_name="google_auth", + verbose_name="Slack Member", )And update the rest of the class references accordingly (authenticate signature, str, queries).
backend/tests/apps/slack/models/google_auth_test.py (1)
191-195: Use the freshly issued refresh token infetch_tokenassertion
fetch_tokenis asserted withrefresh_token=self.valid_refresh_token, which represents a previous token from the fixture and not the newly mocked credentials.
This creates a false positive and couples the test to unrelated state.- refresh_token=self.valid_refresh_token, + refresh_token="new_refresh_token",
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
backend/.env.example(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
🧠 Learnings (1)
📓 Common learnings
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: backend/apps/mentorship/graphql/mutations/module.py:28-29
Timestamp: 2025-07-16T13:49:58.648Z
Learning: In the OWASP Nest mentorship system, mentors can be created with only github_user initially (without nest_user) when assigned to modules. This allows mentors to be assigned before they've signed into the system. When these users eventually sign in, the nest_user is automatically linked to the existing github_user, creating a complete mentor profile. This design provides flexibility in mentor assignment workflows.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1823
File: frontend/__tests__/e2e/pages/Login.spec.ts:28-34
Timestamp: 2025-08-01T04:15:32.151Z
Learning: In the OWASP Nest project, the login page (/auth/login) handles only authentication (GitHub OAuth) and does not differentiate between OWASP staff and non-staff users. The role-based access control using the is_owasp_staff field happens after authentication in downstream components like DashboardWrapper and ProjectsWrapper, not during the login process itself.
🪛 Ruff (0.12.2)
backend/tests/apps/slack/models/google_auth_test.py
22-22: Possible hardcoded password assigned to: "valid_token"
(S105)
23-23: Possible hardcoded password assigned to: "valid_refresh_token"
(S105)
155-155: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
176-176: Use specific rule codes when using noqa
(PGH004)
177-177: Possible hardcoded password assigned to: "refresh_token"
(S105)
186-186: Possible hardcoded password assigned to: "access_token"
(S105)
187-187: Possible hardcoded password assigned to: "refresh_token"
(S105)
194-194: Possible hardcoded password assigned to argument: "client_secret"
(S106)
230-230: Possible hardcoded password assigned to: "token"
(S105)
231-231: Possible hardcoded password assigned to: "refresh_token"
(S105)
240-240: Possible hardcoded password assigned to: "access_token"
(S105)
241-241: Possible hardcoded password assigned to: "refresh_token"
(S105)
247-247: Possible hardcoded password assigned to argument: "client_secret"
(S106)
292-292: Possible hardcoded password assigned to argument: "GOOGLE_AUTH_CLIENT_SECRET"
(S106)
304-304: Possible hardcoded password assigned to argument: "access_token"
(S106)
305-305: Possible hardcoded password assigned to argument: "refresh_token"
(S106)
🔇 Additional comments (2)
backend/pyproject.toml (1)
58-60: Dependencies Python 3.13 support verified– PyPI metadata shows no upper‐bounds that would block Python 3.13:
• google-api-python-client (>=3.7)
• google-auth-oauthlib (>=3.6)
• google-auth-httplib2 (norequires_pythonrestriction; generic Python 3 support)You’re free to pin these via your
poetry.lock(or add apython = "^3.13"constraint inpyproject.toml) to guard against future upstream changes.
If these client libs are truly optional, you can also bundle them under a[tool.poetry.extras]group and wrap your imports accordingly.backend/apps/slack/models/__init__.py (1)
3-3: Expose GoogleAuth in models packageImport looks good.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/apps/slack/migrations/0019_googleauth.py (1)
25-27: Index token expiry for efficient refresh/cleanup queries.expires_at will likely be used for lookups/cron jobs. Add db_index=True.
-("expires_at", models.DateTimeField(blank=True, null=True, verbose_name="Token Expiry")), +("expires_at", models.DateTimeField(blank=True, null=True, verbose_name="Token Expiry", db_index=True)),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
backend/apps/slack/migrations/0019_googleauth.py(1 hunks)backend/pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/pyproject.toml
⏰ 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). (5)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (2)
backend/apps/slack/migrations/0019_googleauth.py (2)
22-27: Confirm blank vs null semantics for tokens/expiry.Currently tokens are blank=True (empty string allowed, NULL disallowed) and expires_at allows NULL. Ensure your auth flow treats empty string consistently (e.g., “no token yet”) and doesn’t rely on None for tokens.
Would you prefer tokens to be nullable (null=True) to represent “never set,” or is empty string acceptable in all code paths?
8-10: Dependency and field alignment verifiedThe migration dependency
0018_conversation_sync_messagesis present, and theGoogleAuthmodel defines theuserOneToOneField toslack.memberexactly as declared in migration 0019. No further changes required.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
backend/apps/slack/models/google_auth.py (3)
22-29: Security: encrypt access/refresh tokens at restThese are high-value secrets stored in plaintext. Use field-level encryption (e.g., your org-standard encrypted TextField) and ensure tokens are never exposed in admin list, logs, or APIs.
I can wire up encrypted fields and update admin/serializers accordingly if you want.
55-74: Avoid creating empty records; handle fetch errors; normalize expiry; guard refresh_token Noneget_or_create creates a row before successful token exchange; if fetch fails you retain an empty record. Also refresh_token may be None, which breaks TextField (null=False). Add error handling and make expiry timezone-aware.
@staticmethod def authenticate(auth_url, member): """Authenticate a member and return a GoogleAuth instance.""" if not settings.IS_GOOGLE_AUTH_ENABLED: raise ValueError(AUTH_ERROR_MESSAGE) - auth = GoogleAuth.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 - GoogleAuth.refresh_access_token(auth) - return auth - # This is the first time authentication, so we need to fetch a new token - flow = GoogleAuth.get_flow() - flow.redirect_uri = settings.GOOGLE_AUTH_REDIRECT_URI - flow.fetch_token(authorization_response=auth_url) - auth.access_token = flow.credentials.token - auth.refresh_token = flow.credentials.refresh_token - auth.expires_at = flow.credentials.expiry - auth.save() - return auth + try: + auth = GoogleAuth.objects.get(member=member) + except GoogleAuth.DoesNotExist: + auth = None + + if auth and auth.access_token and not auth.is_token_expired: + return auth + if auth and auth.access_token: + # If the access token is present but expired, refresh it + GoogleAuth.refresh_access_token(auth) + return auth + + # First-time authentication: exchange auth code for tokens then create record + flow = GoogleAuth.get_flow() + flow.redirect_uri = settings.GOOGLE_AUTH_REDIRECT_URI + try: + flow.fetch_token(authorization_response=auth_url) + except Exception as e: + raise ValueError(f"Failed to fetch Google OAuth token: {e}") from e + + creds = flow.credentials + expiry = creds.expiry + try: + if expiry and timezone.is_naive(expiry): + expiry = timezone.make_aware(expiry) + except Exception: + pass + + return GoogleAuth.objects.create( + member=member, + access_token=creds.token, + refresh_token=(creds.refresh_token or ""), + expires_at=expiry, + )
92-103: Refresh flow hardening: error handling, preserve refresh_token if absent, normalize expiryOverwriting refresh_token with None will fail; also add try/except around token exchange and make expiry timezone-aware.
- flow = GoogleAuth.get_flow() - flow.fetch_token( - refresh_token=auth.refresh_token, - 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 - auth.save() + flow = GoogleAuth.get_flow() + try: + flow.fetch_token( + refresh_token=auth.refresh_token, + client_id=settings.GOOGLE_AUTH_CLIENT_ID, + client_secret=settings.GOOGLE_AUTH_CLIENT_SECRET, + ) + except Exception as e: + raise ValueError(f"Failed to refresh Google OAuth token: {e}") from e + + credentials = flow.credentials + auth.access_token = credentials.token + # Some providers don't return refresh_token on refresh; keep the existing one + if credentials.refresh_token: + auth.refresh_token = credentials.refresh_token + expiry = credentials.expiry + try: + if expiry and timezone.is_naive(expiry): + expiry = timezone.make_aware(expiry) + except Exception: + pass + auth.expires_at = expiry + auth.save()
🧹 Nitpick comments (2)
backend/apps/slack/models/google_auth.py (2)
8-10: Clarify error message to reflect settings, not environment variablesThe guard checks settings, but the message mentions environment variables. Align wording with actual source of truth.
-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 required settings (GOOGLE_AUTH_CLIENT_ID/SECRET/REDIRECT_URI) are missing." +)
67-70: OAuth state verification to prevent CSRF/code injectionfetch_token is called on a fresh Flow without a prior authorization_url() call/state binding. Unless state is validated elsewhere, this risks CSRF. Ensure you generate and persist state when creating the auth URL, and verify it here.
Recommended approach:
- Add a helper to generate the authorization URL and return both url and state (store state server-side).
- Modify get_flow to optionally accept state and pass it through to Flow.from_client_config.
- Update authenticate(auth_url, member, state) to initialize the Flow with the expected state before calling fetch_token.
Example helper (illustrative):
@staticmethod def build_authorization_url(include_granted_scopes=True): flow = GoogleAuth.get_flow() flow.redirect_uri = settings.GOOGLE_AUTH_REDIRECT_URI url, state = flow.authorization_url( access_type="offline", prompt="consent", include_granted_scopes=include_granted_scopes, ) return url, stateIf state is already handled in your view/session layer, please confirm and ignore this.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/apps/slack/migrations/0019_googleauth.py(1 hunks)backend/apps/slack/models/google_auth.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/tests/apps/slack/models/google_auth_test.py
- backend/apps/slack/migrations/0019_googleauth.py
⏰ 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). (5)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
backend/apps/slack/models/google_auth.py (4)
22-29: Security: encrypt tokens at restAccess/refresh tokens are high-value secrets. BinaryField does not imply encryption; values are still plaintext unless you encrypt them. Use field-level encryption (e.g., your org-standard solution) or a secrets store. Also ensure tokens never leak via admin, logs, or APIs.
I can wire up encrypted fields and redact admin/API serializers if you’d like.
55-74: get_or_create can violate NOT NULL and current code writes str into BinaryField; restructure and encode
- get_or_create(member=...) may try to create a row without tokens, violating NOT NULL or leaving inconsistent state.
- flow.credentials.token/refresh_token are str; assigning them to BinaryField will raise (bytes required).
- expiry may be naive; normalize to aware to avoid runtime warnings/comparisons.
Apply this refactor:
- def authenticate(auth_url, member): + def authenticate(auth_url, member): """Authenticate a member and return a GoogleAuth instance.""" if not settings.IS_GOOGLE_AUTH_ENABLED: raise ValueError(AUTH_ERROR_MESSAGE) - auth = GoogleAuth.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 - GoogleAuth.refresh_access_token(auth) - return auth - # This is the first time authentication, so we need to fetch a new token - flow = GoogleAuth.get_flow() - flow.redirect_uri = settings.GOOGLE_AUTH_REDIRECT_URI - flow.fetch_token(authorization_response=auth_url) - auth.access_token = flow.credentials.token - auth.refresh_token = flow.credentials.refresh_token - auth.expires_at = flow.credentials.expiry - auth.save() - return auth + try: + auth = GoogleAuth.objects.get(member=member) + except GoogleAuth.DoesNotExist: + auth = None + + if auth and auth.access_token and not auth.is_token_expired: + return auth + if auth and auth.access_token: + # Access token present but expired: refresh and return + GoogleAuth.refresh_access_token(auth) + return auth + + # First-time authentication: exchange auth code for tokens and create record + flow = GoogleAuth.get_flow() + flow.redirect_uri = settings.GOOGLE_AUTH_REDIRECT_URI + try: + flow.fetch_token(authorization_response=auth_url) + except Exception as e: + raise ValueError(f"Failed to fetch Google OAuth token: {e}") from e + + creds = flow.credentials + expiry = creds.expiry + try: + if expiry and timezone.is_naive(expiry): + expiry = timezone.make_aware(expiry) + except Exception: + pass + + access_b = (creds.token or "").encode("utf-8") + refresh_b = (creds.refresh_token or "").encode("utf-8") + + auth = GoogleAuth.objects.create( + member=member, + access_token=access_b, + refresh_token=refresh_b, + expires_at=expiry, + ) + return auth
77-81: Bug: timezone.timedelta does not exist; use datetime.timedeltaThis will raise at runtime when the property is accessed.
- return self.expires_at is None or self.expires_at <= timezone.now() + timezone.timedelta( - seconds=60 - ) + from datetime import timedelta + return ( + self.expires_at is None + or self.expires_at <= timezone.now() + timedelta(seconds=60) + )
84-104: Refresh flow must decode bytes, handle absent refresh_token, and normalize expiry
- auth.refresh_token is BinaryField; decode to str before passing to fetch_token.
- Preserve existing refresh_token when provider omits it on refresh.
- Normalize expiry to timezone-aware.
- Add error handling.
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 ValueError(refresh_error) - flow = GoogleAuth.get_flow() - flow.fetch_token( - refresh_token=auth.refresh_token, - client_id=settings.GOOGLE_AUTH_CLIENT_ID, - client_secret=settings.GOOGLE_AUTH_CLIENT_SECRET, - ) + flow = GoogleAuth.get_flow() + # Decode stored bytes to str for the library call + rt = ( + auth.refresh_token.decode("utf-8") + if isinstance(auth.refresh_token, (bytes, bytearray)) + else auth.refresh_token + ) + try: + flow.fetch_token( + refresh_token=rt, + client_id=settings.GOOGLE_AUTH_CLIENT_ID, + client_secret=settings.GOOGLE_AUTH_CLIENT_SECRET, + ) + except Exception as e: + raise ValueError(f"Failed to refresh Google OAuth token: {e}") from e credentials = flow.credentials - auth.access_token = credentials.token - auth.refresh_token = credentials.refresh_token - auth.expires_at = credentials.expiry + auth.access_token = (credentials.token or "").encode("utf-8") + # Some providers do not return refresh_token on refresh; keep the old one + if credentials.refresh_token: + auth.refresh_token = credentials.refresh_token.encode("utf-8") + expiry = credentials.expiry + try: + if expiry and timezone.is_naive(expiry): + expiry = timezone.make_aware(expiry) + except Exception: + pass + auth.expires_at = expiry auth.save()
🧹 Nitpick comments (1)
backend/apps/slack/models/google_auth.py (1)
36-52: Make OAuth scopes configurable via settingsHardcoding scopes limits reuse. Read scopes from settings with a sensible default.
- return Flow.from_client_config( + scopes = getattr( + settings, + "GOOGLE_AUTH_SCOPES", + ["https://www.googleapis.com/auth/calendar.readonly"], + ) + 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/auth", "token_uri": "https://oauth2.googleapis.com/token", } }, - scopes=["https://www.googleapis.com/auth/calendar.readonly"], + scopes=scopes, )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/slack/migrations/0020_alter_googleauth_access_token_and_more.py(1 hunks)backend/apps/slack/models/google_auth.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-12T17:38:56.129Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2021
File: backend/apps/slack/models/google_auth.py:39-52
Timestamp: 2025-08-12T17:38:56.129Z
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.
Applied to files:
backend/apps/slack/models/google_auth.py
⏰ 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). (5)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (1)
backend/apps/slack/models/google_auth.py (1)
105-107: Good: str avoids leaking secretsNice touch keeping secrets out of string representations.
backend/apps/slack/migrations/0020_alter_googleauth_access_token_and_more.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
backend/tests/apps/slack/models/google_auth_test.py (1)
22-27: Persist or avoid reverse relation use; current setup risks brittle testsMember is never saved. Accessing reverse relation (member.google_auth) typically requires DB persistence and will raise DoesNotExist for unsaved objects. Either persist the Member (and required Workspace) or avoid exercising the reverse accessor in unit tests.
Apply this diff to avoid the reverse accessor in this test:
@@ 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
🧹 Nitpick comments (6)
backend/tests/apps/slack/models/google_auth_test.py (4)
90-91: Redundant override_settings — remove duplicationIS_GOOGLE_AUTH_ENABLED=True is set twice. Drop the extra decorator.
- @override_settings(IS_GOOGLE_AUTH_ENABLED=True)
168-191: Optional: assert the return shape of authorization_urlauthenticate() returns the result of Flow.authorization_url(...) in the first-time path. Asserting the call is good; asserting the return (tuple or URL depending on lib version) would harden the test against regressions.
251-263: Also use bytes in “no refresh token” test for consistencyKeep token type consistent across tests.
@@ - access_token=self.valid_token, + access_token=self.valid_token, # already bytes after earlier change refresh_token=None,No functional change needed if you apply the fixture change; this is a reminder to keep references consistent.
318-319: Patch where used for consistencyIn google_auth.py, Member is imported into that module. Patch Member.objects.get at apps.slack.models.google_auth.Member.objects.get to ensure the SUT uses the mock.
- @patch("apps.slack.models.member.Member.objects.get") + @patch("apps.slack.models.google_auth.Member.objects.get")backend/apps/slack/models/google_auth.py (2)
33-35: Docstring return type is misleadingauthenticate() can return either a GoogleAuth instance or the authorization URL tuple for first-time auth. Update the docstring to reflect this.
- """Authenticate a member and return a GoogleAuth instance.""" + """Authenticate a member. + + Returns: + GoogleAuth: when an existing valid/refreshable token is present. + Tuple[str, str]: (authorization_url, state) when initiating first-time OAuth. + """
51-61: Parameter name suggests PK but lookup uses slack_user_idauthenticate_callback receives member_id but queries by slack_user_id. Rename for clarity or add docstring to prevent misuse.
- def authenticate_callback(auth_response, member_id): + def authenticate_callback(auth_response, member_slack_id): @@ - member = Member.objects.get(slack_user_id=member_id) + member = Member.objects.get(slack_user_id=member_slack_id) @@ - error_message = f"Member with Slack ID {member_id} does not exist." + error_message = f"Member with Slack ID {member_slack_id} does not exist."Update call sites/tests accordingly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/apps/slack/migrations/0021_alter_googleauth_access_token_and_more.py(1 hunks)backend/apps/slack/models/google_auth.py(1 hunks)backend/tests/apps/slack/models/google_auth_test.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-12T17:38:56.165Z
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.
Applied to files:
backend/apps/slack/models/google_auth.py
📚 Learning: 2025-08-12T17:45:36.105Z
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.
Applied to files:
backend/apps/slack/models/google_auth.py
🧬 Code Graph Analysis (2)
backend/tests/apps/slack/models/google_auth_test.py (2)
backend/apps/slack/models/google_auth.py (6)
GoogleAuth(16-123)is_token_expired(93-97)get_flow(75-90)authenticate(33-48)refresh_access_token(100-119)authenticate_callback(51-72)backend/apps/slack/models/member.py (1)
Member(10-78)
backend/apps/slack/models/google_auth.py (1)
backend/apps/slack/models/member.py (1)
Member(10-78)
⏰ 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). (5)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (2)
backend/apps/slack/migrations/0021_alter_googleauth_access_token_and_more.py (1)
11-27: Migration aligns nullability with model definitions — goodAllowing nulls for token and expiry fields matches the model and avoids creation errors during first-time auth. Looks correct.
backend/apps/slack/models/google_auth.py (1)
95-97: Replacetimezone.timedeltawithdatetime.timedeltaDjango’s
django.utils.timezonemodule does not expose atimedeltaattribute, so callingtimezone.timedelta(seconds=60)will raise anAttributeError. Import and use Python’s built-intimedeltainstead.
- backend/apps/slack/models/google_auth.py: lines 95–97
Apply:
- return self.expires_at is None or self.expires_at <= timezone.now() + timezone.timedelta( - seconds=60 - ) + from datetime import timedelta + 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.
backend/apps/slack/migrations/0021_alter_googleauth_access_token_and_more.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
backend/apps/common/clients.py (2)
16-17: Make OAuth scopes configurable via settings to avoid hard-coding.Move scopes to a Django setting (e.g., GOOGLE_AUTH_SCOPES) so different deployments can adjust (e.g., add openid/email/profile or other APIs) without code changes.
Apply this minimal diff if keeping current structure:
- scopes=["https://www.googleapis.com/auth/calendar.readonly"], + scopes=getattr( + settings, + "GOOGLE_AUTH_SCOPES", + ["https://www.googleapis.com/auth/calendar.readonly"], + ),
1-1: Nit: Clarify the module purpose in the docstring.A slightly more descriptive docstring helps future readers.
Apply this diff:
-"""Common API Clients.""" +"""Common API clients. + +Currently provides a factory for creating per-request Google OAuth2 Flow instances. +"""
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/apps/common/clients.py(1 hunks)backend/apps/slack/models/google_auth.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/tests/apps/slack/models/google_auth_test.py
- backend/apps/slack/models/google_auth.py
⏰ 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)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
backend/apps/common/clients.py (1)
7-20: Gate Flow creation on settings flag and allow scopes/redirect override (factory is reusable beyond Slack)This helper sits in common/, so a guard helps avoid misconfiguration when used elsewhere. Also, make scopes configurable and pass redirect_uri explicitly to Flow to avoid relying on implicit config.
Apply this diff:
-def get_google_auth_client(): - """Get a Google OAuth client.""" - return Flow.from_client_config( +def get_google_auth_client(scopes=None, redirect_uri=None): + """Create a Google OAuth Flow instance (fresh per call).""" + if not getattr(settings, "IS_GOOGLE_AUTH_ENABLED", False): + from django.core.exceptions import ImproperlyConfigured + raise ImproperlyConfigured("Google auth is disabled or misconfigured.") + scopes = scopes or getattr( + settings, + "GOOGLE_AUTH_SCOPES", + ["https://www.googleapis.com/auth/calendar.readonly"], + ) + 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"], + scopes=scopes, + redirect_uri=redirect_uri or settings.GOOGLE_AUTH_REDIRECT_URI, )backend/apps/slack/models/google_auth.py (3)
37-44: Avoid writing empty auth rows before token exchangeget_or_create() creates a DB row with empty tokens, then you return an authorization URL. Prefer creating the row only after successful token exchange in the callback, or use defaults that don’t create until needed. This reduces noise and avoids partial records.
112-116: Refresh: store tokens as bytes, preserve refresh_token when absent, and normalize expiryAligns refresh path with callback path and BinaryField usage.
- credentials = flow.credentials - auth.access_token = credentials.token - auth.refresh_token = credentials.refresh_token - auth.expires_at = credentials.expiry - auth.save() + credentials = flow.credentials + atok = credentials.token + if isinstance(atok, str): + atok = atok.encode("utf-8") + auth.access_token = atok + if credentials.refresh_token: + rtok = credentials.refresh_token + if isinstance(rtok, str): + rtok = rtok.encode("utf-8") + auth.refresh_token = rtok + exp = credentials.expiry + if exp and timezone.is_naive(exp): + exp = timezone.make_aware(exp) + auth.expires_at = exp + auth.save()
68-79: Bug: BinaryField expects bytes; encode tokens before saving and normalize expirycredentials.token/refresh_token are strings. Assigning them to BinaryField without encoding will fail. Keep refresh_token as-is if provider doesn’t return one, and ensure expires_at is timezone-aware (you already guard for naivety).
- auth.access_token = flow.credentials.token - auth.refresh_token = flow.credentials.refresh_token + atok = flow.credentials.token + rtok = flow.credentials.refresh_token + if isinstance(atok, str): + atok = atok.encode("utf-8") + if isinstance(rtok, str): + rtok = rtok.encode("utf-8") + auth.access_token = atok + # Preserve existing refresh_token if not returned + if rtok: + auth.refresh_token = rtok expires_at = flow.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 (3)
backend/apps/common/clients.py (1)
8-8: Nit: docstring clarityThis returns a Flow, not a “client.” Consider clarifying to “Create a Google OAuth Flow instance (fresh per call).”
- """Get a Google OAuth client.""" + """Create a Google OAuth Flow instance (fresh per call)."""backend/apps/slack/models/google_auth.py (2)
11-13: Nit: message wording — settings vs environmentThese values are sourced from Django settings (which may be fed by env). Consider neutral wording.
-AUTH_ERROR_MESSAGE = ( - "Google OAuth client ID, secret, and redirect URI must be set in environment variables." -) +AUTH_ERROR_MESSAGE = ( + "Google OAuth client ID, secret, and redirect URI must be configured." +)
56-67: Nit: remove unused pre-initialization
member = Noneis unnecessary before the try/except.- member = None try: member = Member.objects.get(slack_user_id=member_id)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/common/clients.py(1 hunks)backend/apps/slack/models/google_auth.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-12T17:38:56.165Z
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.
Applied to files:
backend/apps/common/clients.pybackend/apps/slack/models/google_auth.py
📚 Learning: 2025-08-12T17:45:36.105Z
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.
Applied to files:
backend/apps/slack/models/google_auth.py
🧬 Code Graph Analysis (1)
backend/apps/slack/models/google_auth.py (2)
backend/apps/common/clients.py (1)
get_google_auth_client(7-20)backend/apps/slack/models/member.py (1)
Member(10-78)
⏰ 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/apps/common/clients.py (1)
7-20: Good move: per-request Flow factory with v2 auth endpointCreating a new Flow per call avoids shared state and thread-safety issues. Using the v2 authorization endpoint is also correct.
backend/apps/slack/models/google_auth.py (3)
83-88: LGTM: centralized Flow factory usage and gatingUsing a shared factory and gating on IS_GOOGLE_AUTH_ENABLED is consistent with project settings. No extra validation needed per team guidance.
90-94: LGTM: expiry check with small leewayLeeway helps avoid near-expiry races. The current implementation is fine.
105-111: Refresh: decode stored refresh_token to str and add basic error handlingBinaryField returns bytes/memoryview; OAuth expects str. Decode before calling fetch_token and handle exceptions to return clear errors.
[ suggest_essential_refactor ]- flow = GoogleAuth.get_flow() - flow.fetch_token( - refresh_token=auth.refresh_token, - client_id=settings.GOOGLE_AUTH_CLIENT_ID, - client_secret=settings.GOOGLE_AUTH_CLIENT_SECRET, - ) + flow = GoogleAuth.get_flow() + # Decode BinaryField value to str for OAuth client + rtok = auth.refresh_token + if isinstance(rtok, (bytes, memoryview)): + rtok = bytes(rtok).decode("utf-8") + try: + flow.fetch_token( + refresh_token=rtok, + client_id=settings.GOOGLE_AUTH_CLIENT_ID, + client_secret=settings.GOOGLE_AUTH_CLIENT_SECRET, + ) + except Exception as e: + raise ValidationError(f"Failed to refresh Google OAuth token: {e}") from e
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/apps/slack/migrations/0022_alter_googleauth_options_alter_googleauth_table.py (1)
12-15: Update verbose_name_plural to “Google OAuth accounts” in both migration and modelTo keep the admin label clear and avoid future migration churn, make sure the plural label is consistent:
- In backend/apps/slack/migrations/0022_alter_googleauth_options_alter_googleauth_table.py
- In backend/apps/slack/models/google_auth.py
Proposed diffs:
--- backend/apps/slack/migrations/0022_alter_googleauth_options_alter_googleauth_table.py @@ -12,7 +12,7 @@ migrations.AlterModelOptions( name="googleauth", - options={"verbose_name_plural": "Google Auths"}, + options={"verbose_name_plural": "Google OAuth accounts"}, ),--- backend/apps/slack/models/google_auth.py @@ class Meta: - verbose_name_plural = "Google Auths" + verbose_name_plural = "Google OAuth accounts"[optional_refactors_recommended]
📜 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 (2)
backend/apps/slack/migrations/0022_alter_googleauth_options_alter_googleauth_table.py(1 hunks)backend/apps/slack/models/google_auth.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/slack/models/google_auth.py
⏰ 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 (3)
backend/apps/slack/migrations/0022_alter_googleauth_options_alter_googleauth_table.py (3)
11-20: LGTM: Clear, focused migration for model options and table renameThe operations are straightforward and appropriate for aligning Meta options and the explicit db_table. No functional issues spotted.
16-19: No hard-coded references to the old table name found
A grep for “slack_googleauth” and “slack_google_auth” returned no matches in the repo. This migration will emit anALTER TABLE … RENAMEand preserve data. On Postgres it’s a fast metadata-only change but does acquire a brief lock—plan your rollout accordingly.
7-9: Migration dependency verifiedThe file
backend/apps/slack/migrations/0021_alter_googleauth_access_token_and_more.pyis present and correctly ordered before0022_alter_googleauth_options_alter_googleauth_table.py. No further changes needed.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
backend/settings/base.py (1)
22-26: Use SecretValue for client secret; optionally set sane defaults for URIs
- GOOGLE_AUTH_CLIENT_SECRET should be values.SecretValue to avoid accidental exposure in logs/admin dumps and to stay consistent with other secrets in this settings module.
- Consider providing defaults for GOOGLE_AUTH_AUTH_URI and GOOGLE_AUTH_TOKEN_URI to the standard Google endpoints to reduce env friction.
Apply:
- GOOGLE_AUTH_CLIENT_SECRET = values.Value(environ_name="GOOGLE_AUTH_CLIENT_SECRET") + GOOGLE_AUTH_CLIENT_SECRET = values.SecretValue(environ_name="GOOGLE_AUTH_CLIENT_SECRET")Optionally:
- GOOGLE_AUTH_AUTH_URI = values.Value(environ_name="GOOGLE_AUTH_AUTH_URI") + GOOGLE_AUTH_AUTH_URI = values.Value( + environ_name="GOOGLE_AUTH_AUTH_URI", default="https://accounts.google.com/o/oauth2/auth" + ) - GOOGLE_AUTH_TOKEN_URI = values.Value(environ_name="GOOGLE_AUTH_TOKEN_URI") + GOOGLE_AUTH_TOKEN_URI = values.Value( + environ_name="GOOGLE_AUTH_TOKEN_URI", default="https://oauth2.googleapis.com/token" + )backend/apps/slack/models/google_auth.py (5)
59-64: Security: state must be cryptographically random; don’t use slack_user_idUsing a predictable state (slack_user_id) defeats CSRF protections. Let the OAuth library generate a random state and persist a server-side mapping from state -> slack_user_id for the callback.
Apply within this hunk:
- state = member.slack_user_id - return flow.authorization_url( + return flow.authorization_url( access_type="offline", prompt="consent", - state=state, + include_granted_scopes=True, )Outside this hunk, persist the returned state and map it to member.slack_user_id (example using Django cache):
# After calling authorization_url(...) auth_url, state = flow.authorization_url( access_type="offline", prompt="consent", include_granted_scopes=True, ) from django.core.cache import cache cache.set(f"oauth_state_{state}", member.slack_user_id, timeout=600) return auth_url, stateAnd in your callback, retrieve and verify the state before proceeding; reject if missing/expired or mismatched.
67-71: Verify and bind callback to state; don’t trust caller-supplied member_idauthenticate_callback accepts member_id directly and doesn’t verify the OAuth state. To prevent CSRF and session fixation, retrieve member_id (e.g., slack_user_id) from the server-side state mapping you stored when generating the auth URL, and verify it matches the state included in the auth_response.
Follow-up: adjust the callback signature to accept the state (or parse it from auth_response query) and perform state lookup/validation. If you want, I can provide a patch that wires cache lookup and validation into this method.
83-91: Bug: writing str into BinaryField; encode tokens and normalize expiryflow.credentials.token/refresh_token are strings. Assigning them to BinaryField will fail. Also keep expiry timezone-aware.
Apply:
- flow.fetch_token(authorization_response=auth_response) - auth.access_token = flow.credentials.token - auth.refresh_token = flow.credentials.refresh_token - expires_at = flow.credentials.expiry + flow.fetch_token(authorization_response=auth_response) + credentials = flow.credentials + atok = credentials.token + rtok = credentials.refresh_token + if isinstance(atok, str): + atok = atok.encode("utf-8") + if isinstance(rtok, str): + rtok = rtok.encode("utf-8") + auth.access_token = atok + auth.refresh_token = rtok + 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()Optional hardening: wrap fetch_token in try/except and raise a ValidationError with context on failure.
115-123: Decode BinaryField values before constructing Credentialsauth.access_token and auth.refresh_token are bytes; google.oauth2.credentials.Credentials expects str. Decode first.
Apply:
- credentials = Credentials( - token=auth.access_token, - refresh_token=auth.refresh_token, + # Decode binary tokens to str for the Google client + token_str = ( + bytes(auth.access_token).decode("utf-8") + if isinstance(auth.access_token, (bytes, bytearray, memoryview)) + else auth.access_token + ) + refresh_str = ( + bytes(auth.refresh_token).decode("utf-8") + if isinstance(auth.refresh_token, (bytes, bytearray, memoryview)) + else auth.refresh_token + ) + credentials = Credentials( + token=token_str, + refresh_token=refresh_str, token_uri=settings.GOOGLE_AUTH_TOKEN_URI, client_id=settings.GOOGLE_AUTH_CLIENT_ID, client_secret=settings.GOOGLE_AUTH_CLIENT_SECRET, )
124-127: Encode refreshed tokens before saving; preserve existing refresh_token and normalize expiryOn refresh, providers often omit refresh_token. Don’t overwrite it with None. Also encode to bytes for BinaryField and ensure expires_at is timezone-aware.
Apply:
- auth.access_token = credentials.token - auth.refresh_token = credentials.refresh_token - auth.expires_at = credentials.expiry + atok = credentials.token + if isinstance(atok, str): + atok = atok.encode("utf-8") + auth.access_token = atok + if credentials.refresh_token: + rtok = credentials.refresh_token + if isinstance(rtok, str): + rtok = rtok.encode("utf-8") + auth.refresh_token = rtok + exp = credentials.expiry + if exp and timezone.is_naive(exp): + exp = timezone.make_aware(exp) + auth.expires_at = exp auth.save()
🧹 Nitpick comments (1)
backend/apps/slack/models/google_auth.py (1)
38-56: Docstring correctly reflects dual return types; consider type hintsThe method may return GoogleAuth or (authorization_url, state). Consider adding a return annotation like -> Union["GoogleAuth", Tuple[str, str]] for clarity at call sites.
📜 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 (5)
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/slack/models/google_auth_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/tests/apps/slack/models/google_auth_test.py
- backend/.env.example
- backend/apps/common/clients.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-12T17:38:56.165Z
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.
Applied to files:
backend/apps/slack/models/google_auth.pybackend/settings/base.py
📚 Learning: 2025-08-12T17:45:36.105Z
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.
Applied to files:
backend/apps/slack/models/google_auth.py
📚 Learning: 2025-08-13T23:58:22.249Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2021
File: backend/apps/slack/models/google_auth.py:45-53
Timestamp: 2025-08-13T23:58:22.249Z
Learning: In OAuth flows, the state parameter should be cryptographically random for CSRF protection, not predictable application data like user IDs. To maintain user identification across the OAuth flow, store a mapping from random state to user identifier server-side (e.g., in cache or database) with appropriate expiry.
Applied to files:
backend/apps/slack/models/google_auth.py
🧬 Code Graph Analysis (1)
backend/apps/slack/models/google_auth.py (2)
backend/apps/common/clients.py (1)
get_google_auth_client(7-20)backend/apps/slack/models/member.py (1)
Member(10-78)
⏰ 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). (1)
- GitHub Check: Run CI Denendencies Scan
🔇 Additional comments (3)
backend/settings/base.py (1)
28-31: LGTM: boolean enablement check is clear and robustThe explicit rejection of None/"None"/"" keeps the flag boolean and prevents placeholder values from enabling the feature inadvertently. Matches the project’s prior guidance.
backend/apps/slack/models/google_auth.py (2)
93-99: LGTM: centralize Flow creation via client factoryget_flow delegating to get_google_auth_client keeps the configuration in one place and aligns with settings. No redundant re-validation needed given IS_GOOGLE_AUTH_ENABLED.
101-106: Expiry check with grace window looks goodA 60s leeway reduces near-expiry race conditions. Using timezone.now() keeps tz handling consistent.
There was a problem hiding this comment.
I believe it's worth to consider crating a feature branch for this project. What's your opinion @ahmedxgouda ?
I think it will not affect the current setup in the main branch as I added env vars to check if the new features are enabled, but maybe creating a feature branch will be helpful later @arkid15r |
There was a problem hiding this comment.
Let's move this to nest app. It's only related to Slack due to the current task scope.
There was a problem hiding this comment.
But it has slack.Member as a user not nest.User
There was a problem hiding this comment.
It's not just linked user what matters here. This model has very weak tie to slack app. In future slack.Member relation can be optional. We may expand google auth to a wider scope not related to Slack at all. Consider this: tomorrow I change the project requirements to notify not only in Slack channel but also via email or Nest UI (e.g. pop up messages for logged in users).
For solid long term solution you should implement short-term ones keeping in mind their extensibility.
backend/settings/base.py
Outdated
| @@ -19,6 +19,16 @@ class Base(Configuration): | |||
| DEBUG = False | |||
| GITHUB_APP_ID = None | |||
| GITHUB_APP_INSTALLATION_ID = None | |||
| GOOGLE_AUTH_AUTH_URI = values.Value(environ_name="GOOGLE_AUTH_AUTH_URI") | |||
There was a problem hiding this comment.
Do you need explicit environ_names here?
backend/.env.example
Outdated
| DJANGO_GOOGLE_AUTH_AUTH_URI=https://accounts.google.com/o/oauth2/auth | ||
| DJANGO_GOOGLE_AUTH_CLIENT_ID=None | ||
| DJANGO_GOOGLE_AUTH_CLIENT_SECRET=None | ||
| DJANGO_GOOGLE_AUTH_REDIRECT_URI=http://localhost:8000/integrations/slack/oauth2/callback/ |
There was a problem hiding this comment.
This one probably should be more simple, something like /auth/google/callback. Not really sure.
backend/apps/common/clients.py
Outdated
| "token_uri": settings.GOOGLE_AUTH_TOKEN_URI, | ||
| } | ||
| }, | ||
| scopes=["https://www.googleapis.com/auth/calendar.readonly"], |
There was a problem hiding this comment.
It's generally a separately defined setting. Maybe even in settings.py
| """Model to store Google OAuth tokens for Slack integration.""" | ||
|
|
||
| class Meta: | ||
| db_table = "slack_google_auths" |
There was a problem hiding this comment.
A better name for the model and the table?
Also I don't see any unique identifiers saved from Google response. Are there any?
There was a problem hiding this comment.
I don't think so, but why would we need them?
There are alternative names in my mind:
- MemberGoogleCredentials
- MemerGoogleAuth
I am using the Member as a prefix instead of the User here since it is related toslack.Member. What are your thoughts? @arkid15r
There was a problem hiding this comment.
Maybe GoogleAccountAuthorization?
Besides Google ID field I suggest also adding a scope as a required field (the scope for which we have auth tokens from user).
| ) | ||
|
|
||
| @staticmethod | ||
| def authenticate_callback(auth_response, member_id): |
There was a problem hiding this comment.
Could you make these method's code more readable? It mostly looks like a single piece of text atm.
There was a problem hiding this comment.
https://github.com/OWASP/Nest/pull/2089/files#diff-f3e961cda3cb83135aba6e5bfe304097ab94df6803b6c517e6825f6b5b3b484eR84
This is the last version of the method. There are some big changes there, even the signature of the method has been changed.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
backend/.env.example (1)
14-16: Replace "None" placeholders, and document redirect URI requirementsUsing the literal string "None" can be confusing and error-prone. Prefer empty values and add a clarifying comment for the redirect URI.
Please confirm the redirect URI (including trailing slash) exactly matches what’s configured in Google Cloud Console.
-DJANGO_GOOGLE_AUTH_CLIENT_ID=None -DJANGO_GOOGLE_AUTH_CLIENT_SECRET=None -# DJANGO_GOOGLE_AUTH_REDIRECT_URI must match your Google OAuth config +DJANGO_GOOGLE_AUTH_CLIENT_ID= +DJANGO_GOOGLE_AUTH_CLIENT_SECRET= +# Must match exactly the OAuth redirect URI in Google Cloud Console (including trailing slash) DJANGO_GOOGLE_AUTH_REDIRECT_URI=http://localhost:8000/auth/google/callback/backend/settings/base.py (1)
22-29: Gate enablement should also validate required values are presentRelying solely on a boolean flag allows “enabled” with missing client_id/secret/redirect_uri, leading to runtime failures in Flow.from_client_config. Restore a lightweight presence check (reject None/"", "None") alongside the flag.
Example approach (minimal, keeps the boolean flag):
- IS_GOOGLE_AUTH_ENABLED = values.BooleanValue( - environ_name="DJANGO_IS_GOOGLE_AUTH_ENABLED", default=False - ) + IS_GOOGLE_AUTH_ENABLED = values.BooleanValue( + environ_name="DJANGO_IS_GOOGLE_AUTH_ENABLED", default=False + ) + + @classmethod + def google_auth_is_configured(cls) -> bool: + vals = ( + cls.GOOGLE_AUTH_CLIENT_ID, + cls.GOOGLE_AUTH_CLIENT_SECRET, + cls.GOOGLE_AUTH_REDIRECT_URI, + ) + return all(v not in (None, "", "None") for v in vals)Then, in call sites, require both conditions:
- Enabled AND configured.
Given prior learnings in this repo, this mirrors past behavior and avoids brittle runtime errors.
🧹 Nitpick comments (5)
backend/.env.example (1)
17-17: Clarify how to specify multiple scopesIf settings parse scopes as a list (e.g., via ListValue), document the expected format to avoid confusion when adding more scopes later.
Example comment to add above this line:
- “Comma-separated list of scopes, e.g. https://www.googleapis.com/auth/calendar.readonly,https://www.googleapis.com/auth/userinfo.email”
backend/apps/slack/migrations/0023_delete_googleauth.py (1)
6-15: Review Slack.GoogleAuth deletion: data loss risk without migration squash or table transferDeleting the
GoogleAuthmodel in
backend/apps/slack/migrations/0023_delete_googleauth.py
will irreversibly drop theslack_google_authstable in any environment where migrations 0019–0022 have been applied. To preserve existing credentials, consider either:
- Squashing migrations 0019–0023 to omit introducing
GoogleAuthentirely.- Implementing a table-rename/ownership-transfer sequence:
• Renameslack_google_auths→nest_member_google_credentials.
• Update the Nest model to point to the renamed table.
• Then remove the SlackGoogleAuthmodel.I ran
rg -n -C2 -g '!**/migrations/**' -P '\bGoogleAuth\b|slack_google_auths\b'and confirmed there are no remaining runtime references to
Slack.GoogleAuth.backend/apps/nest/migrations/0004_membergooglecredentials.py (2)
13-41: Potential data loss: Slack model deletion followed by creating a new table with the same old nameThis migration depends on slack 0023_delete_googleauth (which drops the old Slack table), then creates a new table named "slack_google_auths". If there is any existing production data in the Slack.GoogleAuth table, it will be dropped before this migration creates a fresh table, causing data loss. If you intend to preserve data, consider a rename/copy strategy instead of delete-then-create.
Options:
- Rename the old table to the Nest table in Slack migrations (AlterModelTable/RunSQL), then adopt it in Nest without dropping.
- Or copy rows from old table into the new Nest table in a data migration before dropping the old one.
Would you like me to sketch a cross-app migration plan (using AlterModelTable/RunSQL) to preserve existing rows if present?
27-33: Transient related_name and table options get corrected by later migrations; consider squashing to reduce churnThis migration creates related_name="google_auth" and db_table="slack_google_auths", but 0005/0006 change these to related_name="member_google_credentials" and db_table="nest_member_google_credentials". If this feature is still under development, squashing 0004–0006 into a single migration with the final metadata would simplify the history and avoid intermediate mismatches during partial deploys.
backend/apps/nest/models/member_google_credentials.py (1)
93-99: Optionally make get_flow error reflect misconfiguration vs disabledCurrently AUTH_ERROR_MESSAGE implies missing client settings, but the guard is purely the boolean flag. If you retain the boolean kill switch and also validate presence (see settings review), consider aligning the message or raising a clearer ValueError when enabled but misconfigured.
📜 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 (11)
backend/.env.example(1 hunks)backend/apps/common/clients.py(1 hunks)backend/apps/nest/migrations/0004_membergooglecredentials.py(1 hunks)backend/apps/nest/migrations/0005_alter_membergooglecredentials_options_and_more.py(1 hunks)backend/apps/nest/migrations/0006_alter_membergooglecredentials_member.py(1 hunks)backend/apps/nest/models/__init__.py(1 hunks)backend/apps/nest/models/member_google_credentials.py(1 hunks)backend/apps/slack/migrations/0023_delete_googleauth.py(1 hunks)backend/pyproject.toml(1 hunks)backend/settings/base.py(1 hunks)backend/tests/apps/nest/models/member_google_credentials_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/pyproject.toml
- backend/apps/common/clients.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-12T17:38:56.165Z
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.
Applied to files:
backend/settings/base.pybackend/.env.example
🧬 Code Graph Analysis (6)
backend/tests/apps/nest/models/member_google_credentials_test.py (2)
backend/apps/nest/models/member_google_credentials.py (6)
MemberGoogleCredentials(18-131)is_token_expired(101-105)get_flow(94-98)authenticate(39-64)refresh_access_token(108-127)authenticate_callback(67-91)backend/apps/slack/models/member.py (1)
Member(10-78)
backend/apps/nest/migrations/0004_membergooglecredentials.py (2)
backend/apps/nest/migrations/0005_alter_membergooglecredentials_options_and_more.py (1)
Migration(6-20)backend/apps/slack/migrations/0023_delete_googleauth.py (1)
Migration(6-15)
backend/apps/nest/models/__init__.py (1)
backend/apps/nest/models/member_google_credentials.py (1)
MemberGoogleCredentials(18-131)
backend/apps/slack/migrations/0023_delete_googleauth.py (2)
backend/apps/nest/migrations/0004_membergooglecredentials.py (1)
Migration(7-41)backend/apps/nest/migrations/0005_alter_membergooglecredentials_options_and_more.py (1)
Migration(6-20)
backend/apps/nest/migrations/0005_alter_membergooglecredentials_options_and_more.py (1)
backend/apps/nest/migrations/0004_membergooglecredentials.py (1)
Migration(7-41)
backend/apps/nest/models/member_google_credentials.py (2)
backend/apps/common/clients.py (1)
get_google_auth_client(7-20)backend/apps/slack/models/member.py (1)
Member(10-78)
⏰ 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). (3)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
🔇 Additional comments (4)
backend/apps/nest/models/__init__.py (1)
3-3: Re-export looks goodRe-exporting MemberGoogleCredentials here is consistent and keeps imports tidy. Ordering is fine.
backend/apps/nest/migrations/0006_alter_membergooglecredentials_member.py (1)
13-23: No reverse lookup references to update
A global search for.google_authfound usages only within the old migration files—no code or tests reference the previousrelated_name. There are no call sites to update.backend/apps/nest/migrations/0005_alter_membergooglecredentials_options_and_more.py (1)
12-19: Keep the rename migration (Slack table was deployed)
We found concrete references toslack_google_authsin your codebase, confirming the Slack table shipped and was subsequently overridden in Nest:
- backend/apps/slack/migrations/0022_alter_googleauth_options_alter_googleauth_table.py: table="slack_google_auths"
- backend/apps/nest/migrations/0004_membergooglecredentials.py: "db_table": "slack_google_auths"
Because the table existed in production, you should preserve migration 0005 rather than drop it.
backend/tests/apps/nest/models/member_google_credentials_test.py (1)
233-241: No change needed: Credentials still receive byte tokens
The model’srefresh_access_tokenmethod currently passes rawBinaryFieldvalues (bytes) intoCredentials, and the tests rightly assert againstself.valid_tokenandself.valid_refresh_token. The proposed.decode()update would break the existing behavior. Only revisit this assertion after introducing.decode()in the model layer.Likely an incorrect or invalid review comment.
| auth.access_token = flow.credentials.token | ||
| auth.refresh_token = flow.credentials.refresh_token | ||
| expires_at = flow.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() | ||
| return auth |
There was a problem hiding this comment.
🛠️ Refactor suggestion
BinaryField tokens vs Google SDK strings: encode/decode at boundaries
Google’s Flow/Credentials return tokens as str, but the model stores tokens in BinaryField. Assigning str to BinaryField and later passing bytes back into Credentials will cause type errors in real usage (the tests use bytes, masking this). Encode on write and decode on use.
Apply this diff:
- auth.access_token = flow.credentials.token
- auth.refresh_token = flow.credentials.refresh_token
+ access = flow.credentials.token
+ refresh = flow.credentials.refresh_token
+ if isinstance(access, str):
+ access = access.encode("utf-8")
+ if isinstance(refresh, str) and refresh is not None:
+ refresh = refresh.encode("utf-8")
+ auth.access_token = access
+ auth.refresh_token = refresh📝 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.
| auth.access_token = flow.credentials.token | |
| auth.refresh_token = flow.credentials.refresh_token | |
| expires_at = flow.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() | |
| return auth | |
| access = flow.credentials.token | |
| refresh = flow.credentials.refresh_token | |
| if isinstance(access, str): | |
| access = access.encode("utf-8") | |
| if isinstance(refresh, str) and refresh is not None: | |
| refresh = refresh.encode("utf-8") | |
| auth.access_token = access | |
| auth.refresh_token = refresh | |
| expires_at = flow.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() | |
| return auth |
🤖 Prompt for AI Agents
In backend/apps/nest/models/member_google_credentials.py around lines 84 to 91,
the code assigns Google Flow/Credentials token strings directly into
BinaryField(s), causing type mismatches at runtime; encode tokens to bytes when
storing and ensure any code that builds Google Credentials from the model
decodes bytes back to str. Specifically, change the assignments so
auth.access_token = flow.credentials.token.encode("utf-8") and
auth.refresh_token = flow.credentials.refresh_token.encode("utf-8") (and
preserve expires_at handling), and audit any code that reads
auth.access_token/auth.refresh_token to call .decode("utf-8") before passing
values into Google SDK constructors.
| 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.expires_at = credentials.expiry | ||
| auth.save() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix: ensure Credentials() receives strings; normalize expiry to aware datetime
Two issues:
- Credentials() expects str; current code passes bytes from BinaryField.
- credentials.expiry may be naive; later comparisons with timezone-aware now() will raise TypeError.
Apply this diff:
- credentials = Credentials(
- token=auth.access_token,
- refresh_token=auth.refresh_token,
+ token_str = (
+ auth.access_token.decode("utf-8")
+ if isinstance(auth.access_token, (bytes, memoryview))
+ else auth.access_token
+ )
+ refresh_str = (
+ auth.refresh_token.decode("utf-8")
+ if isinstance(auth.refresh_token, (bytes, memoryview))
+ else auth.refresh_token
+ )
+ credentials = Credentials(
+ token=token_str,
+ refresh_token=refresh_str,
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.expires_at = credentials.expiry
+ # Store refreshed tokens back as bytes
+ new_token = credentials.token
+ new_refresh = credentials.refresh_token
+ if isinstance(new_token, str):
+ new_token = new_token.encode("utf-8")
+ if isinstance(new_refresh, str) and new_refresh is not None:
+ new_refresh = new_refresh.encode("utf-8")
+ auth.access_token = new_token
+ auth.refresh_token = new_refresh
+ 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.
| 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.expires_at = credentials.expiry | |
| auth.save() | |
| token_str = ( | |
| auth.access_token.decode("utf-8") | |
| if isinstance(auth.access_token, (bytes, memoryview)) | |
| else auth.access_token | |
| ) | |
| refresh_str = ( | |
| auth.refresh_token.decode("utf-8") | |
| if isinstance(auth.refresh_token, (bytes, memoryview)) | |
| else auth.refresh_token | |
| ) | |
| credentials = Credentials( | |
| token=token_str, | |
| refresh_token=refresh_str, | |
| token_uri=settings.GOOGLE_AUTH_TOKEN_URI, | |
| client_id=settings.GOOGLE_AUTH_CLIENT_ID, | |
| client_secret=settings.GOOGLE_AUTH_CLIENT_SECRET, | |
| ) | |
| credentials.refresh(Request()) | |
| # Store refreshed tokens back as bytes | |
| new_token = credentials.token | |
| new_refresh = credentials.refresh_token | |
| if isinstance(new_token, str): | |
| new_token = new_token.encode("utf-8") | |
| if isinstance(new_refresh, str) and new_refresh is not None: | |
| new_refresh = new_refresh.encode("utf-8") | |
| auth.access_token = new_token | |
| auth.refresh_token = new_refresh | |
| 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 115 to
127, the code passes bytes from BinaryField into google.oauth2 Credentials and
may leave credentials.expiry naive; decode token and refresh_token to str (e.g.
handle None and call .decode("utf-8") on bytes) before constructing Credentials,
then after credentials.refresh(Request()) ensure credentials.expiry is
timezone-aware (use django.utils.timezone.is_naive + make_aware or attach UTC
tzinfo) before assigning auth.expires_at, and save the decoded
token/refresh_token and the aware expiry back to auth.
| GOOGLE_AUTH_AUTH_URI = values.Value() | ||
| GOOGLE_AUTH_CLIENT_ID = values.SecretValue() | ||
| GOOGLE_AUTH_CLIENT_SECRET = values.SecretValue() | ||
| GOOGLE_AUTH_REDIRECT_URI = values.Value() | ||
| GOOGLE_AUTH_SCOPES = values.ListValue() | ||
| GOOGLE_AUTH_TOKEN_URI = values.Value() | ||
|
|
||
| IS_GOOGLE_AUTH_ENABLED = values.BooleanValue(default=False) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Environment variable mismatch (DJANGO_ prefix) will break configuration loading
The new Google OAuth settings lack explicit environ_name values. The PR’s env.example uses DJANGO_GOOGLE_AUTH_* and DJANGO_IS_GOOGLE_AUTH_ENABLED, so these won’t be read without the prefix. Result: IS_GOOGLE_AUTH_ENABLED will be False and client config values will be empty in most environments.
Apply this diff to align with env.example:
- GOOGLE_AUTH_AUTH_URI = values.Value()
- GOOGLE_AUTH_CLIENT_ID = values.SecretValue()
- GOOGLE_AUTH_CLIENT_SECRET = values.SecretValue()
- GOOGLE_AUTH_REDIRECT_URI = values.Value()
- GOOGLE_AUTH_SCOPES = values.ListValue()
- GOOGLE_AUTH_TOKEN_URI = values.Value()
-
- IS_GOOGLE_AUTH_ENABLED = values.BooleanValue(default=False)
+ GOOGLE_AUTH_AUTH_URI = values.Value(environ_name="DJANGO_GOOGLE_AUTH_AUTH_URI")
+ GOOGLE_AUTH_CLIENT_ID = values.Value(environ_name="DJANGO_GOOGLE_AUTH_CLIENT_ID")
+ GOOGLE_AUTH_CLIENT_SECRET = values.SecretValue(environ_name="DJANGO_GOOGLE_AUTH_CLIENT_SECRET")
+ GOOGLE_AUTH_REDIRECT_URI = values.Value(environ_name="DJANGO_GOOGLE_AUTH_REDIRECT_URI")
+ GOOGLE_AUTH_SCOPES = values.ListValue(environ_name="DJANGO_GOOGLE_AUTH_SCOPES")
+ GOOGLE_AUTH_TOKEN_URI = values.Value(environ_name="DJANGO_GOOGLE_AUTH_TOKEN_URI")
+
+ IS_GOOGLE_AUTH_ENABLED = values.BooleanValue(
+ environ_name="DJANGO_IS_GOOGLE_AUTH_ENABLED", default=False
+ )Run this to confirm env names in the repo:
🏁 Script executed:
#!/bin/bash
# Verify env example contains DJANGO_GOOGLE_AUTH_* variables
fd -a -uu '.env.example' | xargs -I {} sh -c 'echo "== {} =="; rg -n "DJANGO_GOOGLE_AUTH_|DJANGO_IS_GOOGLE_AUTH_ENABLED" -n {} -C1'Length of output: 699
Align Google OAuth settings with env.example prefixes
The backend settings are currently using default names (GOOGLE_AUTH_…) instead of the DJANGO_GOOGLE_AUTH_… variables defined in .env.example. As a result, none of the OAuth values (and the IS_GOOGLE_AUTH_ENABLED flag) will load in most environments.
Please update backend/settings/base.py (around lines 22–29) as follows:
- GOOGLE_AUTH_AUTH_URI = values.Value()
- GOOGLE_AUTH_CLIENT_ID = values.SecretValue()
- GOOGLE_AUTH_CLIENT_SECRET = values.SecretValue()
- GOOGLE_AUTH_REDIRECT_URI = values.Value()
- GOOGLE_AUTH_SCOPES = values.ListValue()
- GOOGLE_AUTH_TOKEN_URI = values.Value()
-
- IS_GOOGLE_AUTH_ENABLED = values.BooleanValue(default=False)
+ GOOGLE_AUTH_AUTH_URI = values.Value(
+ environ_name="DJANGO_GOOGLE_AUTH_AUTH_URI"
+ )
+ GOOGLE_AUTH_CLIENT_ID = values.Value(
+ environ_name="DJANGO_GOOGLE_AUTH_CLIENT_ID"
+ )
+ GOOGLE_AUTH_CLIENT_SECRET = values.SecretValue(
+ environ_name="DJANGO_GOOGLE_AUTH_CLIENT_SECRET"
+ )
+ GOOGLE_AUTH_REDIRECT_URI = values.Value(
+ environ_name="DJANGO_GOOGLE_AUTH_REDIRECT_URI"
+ )
+ GOOGLE_AUTH_SCOPES = values.ListValue(
+ environ_name="DJANGO_GOOGLE_AUTH_SCOPES"
+ )
+ GOOGLE_AUTH_TOKEN_URI = values.Value(
+ environ_name="DJANGO_GOOGLE_AUTH_TOKEN_URI"
+ )
+
+ IS_GOOGLE_AUTH_ENABLED = values.BooleanValue(
+ environ_name="DJANGO_IS_GOOGLE_AUTH_ENABLED",
+ default=False
+ )This ensures the settings read from:
DJANGO_GOOGLE_AUTH_AUTH_URIDJANGO_GOOGLE_AUTH_CLIENT_IDDJANGO_GOOGLE_AUTH_CLIENT_SECRETDJANGO_GOOGLE_AUTH_REDIRECT_URIDJANGO_GOOGLE_AUTH_SCOPESDJANGO_GOOGLE_AUTH_TOKEN_URIDJANGO_IS_GOOGLE_AUTH_ENABLED
as defined in backend/.env.example.
📝 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.
| GOOGLE_AUTH_AUTH_URI = values.Value() | |
| GOOGLE_AUTH_CLIENT_ID = values.SecretValue() | |
| GOOGLE_AUTH_CLIENT_SECRET = values.SecretValue() | |
| GOOGLE_AUTH_REDIRECT_URI = values.Value() | |
| GOOGLE_AUTH_SCOPES = values.ListValue() | |
| GOOGLE_AUTH_TOKEN_URI = values.Value() | |
| IS_GOOGLE_AUTH_ENABLED = values.BooleanValue(default=False) | |
| GOOGLE_AUTH_AUTH_URI = values.Value( | |
| environ_name="DJANGO_GOOGLE_AUTH_AUTH_URI" | |
| ) | |
| GOOGLE_AUTH_CLIENT_ID = values.Value( | |
| environ_name="DJANGO_GOOGLE_AUTH_CLIENT_ID" | |
| ) | |
| GOOGLE_AUTH_CLIENT_SECRET = values.SecretValue( | |
| environ_name="DJANGO_GOOGLE_AUTH_CLIENT_SECRET" | |
| ) | |
| GOOGLE_AUTH_REDIRECT_URI = values.Value( | |
| environ_name="DJANGO_GOOGLE_AUTH_REDIRECT_URI" | |
| ) | |
| GOOGLE_AUTH_SCOPES = values.ListValue( | |
| environ_name="DJANGO_GOOGLE_AUTH_SCOPES" | |
| ) | |
| GOOGLE_AUTH_TOKEN_URI = values.Value( | |
| environ_name="DJANGO_GOOGLE_AUTH_TOKEN_URI" | |
| ) | |
| IS_GOOGLE_AUTH_ENABLED = values.BooleanValue( | |
| environ_name="DJANGO_IS_GOOGLE_AUTH_ENABLED", | |
| default=False | |
| ) |
🤖 Prompt for AI Agents
In backend/settings/base.py around lines 22 to 29, the Google OAuth settings use
plain names (GOOGLE_AUTH_…) instead of the env var prefixes used in
backend/.env.example; update each setting to read the DJANGO-prefixed variable
names: use values.Value()/values.SecretValue()/values.ListValue() as before but
with keys DJANGO_GOOGLE_AUTH_AUTH_URI, DJANGO_GOOGLE_AUTH_CLIENT_ID,
DJANGO_GOOGLE_AUTH_CLIENT_SECRET, DJANGO_GOOGLE_AUTH_REDIRECT_URI,
DJANGO_GOOGLE_AUTH_SCOPES, DJANGO_GOOGLE_AUTH_TOKEN_URI, and change the boolean
flag to DJANGO_IS_GOOGLE_AUTH_ENABLED = values.BooleanValue(default=False) so
the settings load from the correct environment variables.
| def test_one_to_one_relationship(self): | ||
| """Test one-to-one relationship with Member.""" | ||
| auth = MemberGoogleCredentials(member=self.member, access_token=self.valid_token) | ||
|
|
||
| assert self.member.member_google_credentials == auth | ||
| assert auth.member == self.member | ||
|
|
There was a problem hiding this comment.
This reverse OneToOne assertion requires a database and a saved Member; it will fail with unsaved instances
Accessing self.member.member_google_credentials triggers a DB lookup on an unsaved Member (pk=None), raising RelatedObjectDoesNotExist. Since the suite otherwise patches DB interactions, keep this test DB-free by asserting the related_name contract instead.
Apply this diff:
- def test_one_to_one_relationship(self):
- """Test one-to-one relationship with Member."""
- auth = MemberGoogleCredentials(member=self.member, access_token=self.valid_token)
-
- assert self.member.member_google_credentials == auth
- assert auth.member == self.member
+ def test_one_to_one_relationship_metadata(self):
+ """Test one-to-one relationship metadata without DB access."""
+ auth = MemberGoogleCredentials(member=self.member, access_token=self.valid_token)
+ # Validate the configured related_name on the FK
+ related_name = (
+ MemberGoogleCredentials._meta.get_field("member").remote_field.related_name
+ )
+ assert related_name == "member_google_credentials"
+ # And that the forward relation field is set correctly
+ assert auth.member == self.member📝 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 test_one_to_one_relationship(self): | |
| """Test one-to-one relationship with Member.""" | |
| auth = MemberGoogleCredentials(member=self.member, access_token=self.valid_token) | |
| assert self.member.member_google_credentials == auth | |
| assert auth.member == self.member | |
| def test_one_to_one_relationship_metadata(self): | |
| """Test one-to-one relationship metadata without DB access.""" | |
| auth = MemberGoogleCredentials(member=self.member, access_token=self.valid_token) | |
| # Validate the configured related_name on the FK | |
| related_name = ( | |
| MemberGoogleCredentials._meta.get_field("member").remote_field.related_name | |
| ) | |
| assert related_name == "member_google_credentials" | |
| # And that the forward relation field is set correctly | |
| assert auth.member == self.member |
🤖 Prompt for AI Agents
In backend/tests/apps/nest/models/member_google_credentials_test.py around lines
50 to 56, the test currently accesses self.member.member_google_credentials
which triggers a DB lookup on an unsaved Member (pk=None); replace that
assertion with a DB-free check of the OneToOneField's related_name (e.g., assert
MemberGoogleCredentials._meta.get_field('member').remote_field.related_name ==
'member_google_credentials') and keep the auth.member == self.member assertion
so the test no longer relies on saved instances.
There was a problem hiding this comment.
It's not just linked user what matters here. This model has very weak tie to slack app. In future slack.Member relation can be optional. We may expand google auth to a wider scope not related to Slack at all. Consider this: tomorrow I change the project requirements to notify not only in Slack channel but also via email or Nest UI (e.g. pop up messages for logged in users).
For solid long term solution you should implement short-term ones keeping in mind their extensibility.
| """Model to store Google OAuth tokens for Slack integration.""" | ||
|
|
||
| class Meta: | ||
| db_table = "slack_google_auths" |
There was a problem hiding this comment.
Maybe GoogleAccountAuthorization?
Besides Google ID field I suggest also adding a scope as a required field (the scope for which we have auth tokens from user).
There was a problem hiding this comment.
This one should be at apps/nest/auth/clients/google.py
|
ahmedxgouda
changed the base branch from
main
to
feature/nestbot-calendar-events
Proposed change
Resolves #1997
Add the PR description here.
Checklist
make check-testlocally; all checks and tests passed.