Feature/cppa slack tracker private#139
Open
AuraMindNest wants to merge 7 commits intocppalliance:developfrom
Open
Feature/cppa slack tracker private#139AuraMindNest wants to merge 7 commits intocppalliance:developfrom
AuraMindNest wants to merge 7 commits intocppalliance:developfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces comprehensive support for non-public Slack channels (private channels, MPIMs, and direct messages/IMs) alongside a new Slack OAuth server for managing user-specific tokens. It adds database models for private channel tracking, enhanced message/channel syncing across both public and private spaces, user OAuth token persistence and loading, and configuration infrastructure for OAuth credentials and runtime parameters. Changes
Sequence DiagramssequenceDiagram
participant User
participant OAuthServer as OAuth Server
participant SlackAPI as Slack API
participant Database
User->>OAuthServer: GET /slack/connect
OAuthServer->>OAuthServer: Generate random state, store in memory
OAuthServer->>User: Redirect to Slack OAuth authorize URL
User->>SlackAPI: Authorize request with code
SlackAPI->>User: Redirect to /slack/oauth/callback
User->>OAuthServer: GET /slack/oauth/callback?code=...&state=...
OAuthServer->>OAuthServer: Validate and consume state
OAuthServer->>SlackAPI: POST oauth.v2.access with code
SlackAPI->>OAuthServer: Return access_token + authed_user info
OAuthServer->>Database: Persist user_id, team_id, access_token to JSON
OAuthServer->>User: Success page or error page
sequenceDiagram
participant Tracker as Sync Tracker
participant FetchAPI as Fetch API
participant TokenStore as Token Store
participant SlackAPI as Slack API (User)
participant Database
Tracker->>TokenStore: Load user OAuth tokens for team
loop For each user token
Tracker->>FetchAPI: fetch_im_channel_list_for_user(token)
FetchAPI->>SlackAPI: conversations.list with user token
SlackAPI->>FetchAPI: IM channel list
FetchAPI->>Tracker: List of IM channels
Tracker->>Database: Upsert IM channels as SlackChannelPrivate
Tracker->>SlackAPI: Fetch messages from IMs (user-authenticated)
Tracker->>Database: Save messages to SlackMessagePrivate
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Collaborator
Author
|
@coderabbitai, full review. |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
cppa_slack_tracker/models.py (1)
130-141: Add documentation comments to all private models using the quote-injection pattern.The
db_table = 'slack_private"."...'pattern is used across 4 models (SlackChannelPrivate, SlackMessagePrivate, SlackChannelMembershipPrivate, and SlackChannelMembershipChangeLogPrivate), but the explanatory comment about Django's quoting behavior exists only in SlackChannelPrivate. This pattern is fragile and may break with Django version upgrades or certain management commands. Add the existing comment from SlackChannelPrivate to the other 3 models for consistency, and consider adding an integration test that validates the correct schema qualification in generated SQL.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cppa_slack_tracker/models.py` around lines 130 - 141, Copy the explanatory comment that explains the db_table quote-injection workaround from the SlackChannelPrivate model to the other three private models (SlackMessagePrivate, SlackChannelMembershipPrivate, SlackChannelMembershipChangeLogPrivate) directly above their db_table assignments so the rationale is present alongside the db_table = 'slack_private"."...' lines; additionally, add a small integration test that generates the SQL for these models (via Django migration SQL generation or connection.schema_editor()/connection.ops methods) and asserts the schema-qualified table name appears as expected to catch future Django changes that might break the pattern.cppa_slack_tracker/slack_oauth_server.py (2)
378-381: Redundant exception handler.The
try/exceptaround_exchange_codeis redundant since_exchange_codealready handles all exceptions internally and returns{"ok": False, "error": ...}. The outerexceptat line 380 would only catch exceptions fromawaitmachinery, which is unlikely.Consider removing or narrowing this handler to avoid masking unexpected errors.
♻️ Proposed simplification
- try: - data = await _exchange_code(code) - except Exception as exc: - return _html_exchange_failed(f"Token exchange failed: {exc}") + data = await _exchange_code(code)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cppa_slack_tracker/slack_oauth_server.py` around lines 378 - 381, The outer try/except around awaiting _exchange_code is redundant and may mask unexpected errors; remove the try/except and directly await data = await _exchange_code(code), then handle the returned result (check data.get("ok")/data["error"]) and call _html_exchange_failed when the exchange indicates failure; alternatively, if you must guard only for rare await-time errors, catch only specific exceptions (e.g., asyncio.CancelledError) instead of a broad Exception to avoid hiding bugs in _exchange_code or upstream.
46-50: In-memory state is not suitable for multi-process deployments.
_oauth_statesis an in-memory dict. If the server runs with multiple workers (e.g.,uvicorn --workers N), the state generated by one worker won't be visible to another, causing OAuth callbacks to fail with "Invalid or expired session."This is fine for single-worker dev use but should be documented or addressed for production.
Would you like me to add a docstring note about single-worker limitations, or help implement a shared-state solution (e.g., Redis, file-based)?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cppa_slack_tracker/slack_oauth_server.py` around lines 46 - 50, The CSRF state map _oauth_states is an in-memory dict and will break multi-process deployments; replace it with a pluggable shared-state abstraction and/or document the single-worker limitation. Create an OAuthStateStore interface with methods like set_state(key, expiry), pop_state(key) -> expiry_or_none, and cleanup_expired(); implement a RedisStateStore (using REDIS_URL env var) and keep an InMemoryStateStore as a dev fallback, then change code that references _oauth_states to call the store methods (e.g., where state is created and where it is validated/popped). Alternatively, if you prefer a faster doc change, add a clear module-level docstring near _oauth_states stating it is only safe for single-worker/dev and recommend using the Redis-backed store in production.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cppa_slack_tracker/management/commands/run_cppa_slack_tracker.py`:
- Around line 425-468: The loop currently syncs the same IM channel once per
authorized user token, causing duplicate syncs; fix this by tracking which IM
channel IDs have already been scheduled/synced and skipping duplicates: after
calling fetch_im_channel_list_for_user(user_slack_id, access_token) (or when
iterating im_channels) record each channel id in a shared set (e.g.,
seen_channel_ids) keyed by team and channel id and skip calling
get_or_create_slack_channel and sync_messages for any ch whose id is already in
seen_channel_ids; ensure the first encountered token still performs the sync (so
keep existing flow of get_slack_client, get_or_create_slack_channel,
sync_messages) but add the membership check/insert to avoid repeated syncs
across different tokens and preserve channel_id_opt filtering logic.
In `@cppa_slack_tracker/services.py`:
- Around line 553-556: The current branch that handles subtypes sets text to
_message_text_for_subtype(slack_message, subtype) or "" which discards the raw
slack_message["text"] for unhandled subtypes; change the logic in the same
conditional handling (the elif subtype branch) to use the subtype-specific
helper result if present, otherwise fall back to slack_message.get("text", "")
so unhandled subtypes preserve the original text (i.e., call
_message_text_for_subtype(slack_message, subtype) and if it returns falsy, use
slack_message.get("text", "") instead).
In `@cppa_slack_tracker/slack_oauth_server.py`:
- Around line 222-226: The _load_tokens function currently calls json.load on
TOKENS_FILE without handling malformed JSON; update _load_tokens to catch
json.JSONDecodeError (and optionally ValueError) raised by json.load, log or
process the error (use existing logger or raise a controlled exception), and
return an empty dict fallback instead of letting the exception propagate;
reference the _load_tokens function and TOKENS_FILE constant, mirroring the
approach used in user_tokens.py::load_slack_user_tokens to safely handle corrupt
token files.
In `@requirements.txt`:
- Around line 27-32: The starlette dependency in requirements.txt is vulnerable;
update the requirement "starlette>=0.37" to "starlette>=0.47.2" to ensure fixes
for GHSA-f96h-pmfr-66vw and GHSA-2c2j-9gv5-cj73 are included, then regenerate
any lockfiles or pinned dependency artifacts and run the test suite/CI to verify
the OAuth server (multipart/form-data handling) still works as expected.
---
Nitpick comments:
In `@cppa_slack_tracker/models.py`:
- Around line 130-141: Copy the explanatory comment that explains the db_table
quote-injection workaround from the SlackChannelPrivate model to the other three
private models (SlackMessagePrivate, SlackChannelMembershipPrivate,
SlackChannelMembershipChangeLogPrivate) directly above their db_table
assignments so the rationale is present alongside the db_table =
'slack_private"."...' lines; additionally, add a small integration test that
generates the SQL for these models (via Django migration SQL generation or
connection.schema_editor()/connection.ops methods) and asserts the
schema-qualified table name appears as expected to catch future Django changes
that might break the pattern.
In `@cppa_slack_tracker/slack_oauth_server.py`:
- Around line 378-381: The outer try/except around awaiting _exchange_code is
redundant and may mask unexpected errors; remove the try/except and directly
await data = await _exchange_code(code), then handle the returned result (check
data.get("ok")/data["error"]) and call _html_exchange_failed when the exchange
indicates failure; alternatively, if you must guard only for rare await-time
errors, catch only specific exceptions (e.g., asyncio.CancelledError) instead of
a broad Exception to avoid hiding bugs in _exchange_code or upstream.
- Around line 46-50: The CSRF state map _oauth_states is an in-memory dict and
will break multi-process deployments; replace it with a pluggable shared-state
abstraction and/or document the single-worker limitation. Create an
OAuthStateStore interface with methods like set_state(key, expiry),
pop_state(key) -> expiry_or_none, and cleanup_expired(); implement a
RedisStateStore (using REDIS_URL env var) and keep an InMemoryStateStore as a
dev fallback, then change code that references _oauth_states to call the store
methods (e.g., where state is created and where it is validated/popped).
Alternatively, if you prefer a faster doc change, add a clear module-level
docstring near _oauth_states stating it is only safe for single-worker/dev and
recommend using the Redis-backed store in production.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d92e9ee6-77a3-43f7-8f18-2bad47e207ff
📒 Files selected for processing (19)
.env.example.gitignoreconftest.pycppa_slack_tracker/admin.pycppa_slack_tracker/fetcher.pycppa_slack_tracker/management/commands/run_cppa_slack_tracker.pycppa_slack_tracker/migrations/0004_alter_slackteam_team_id_slackchannelprivate_and_more.pycppa_slack_tracker/models.pycppa_slack_tracker/services.pycppa_slack_tracker/slack_oauth_server.pycppa_slack_tracker/sync/__init__.pycppa_slack_tracker/sync/sync_channel.pycppa_slack_tracker/sync/sync_channel_user.pycppa_slack_tracker/sync/sync_message.pycppa_slack_tracker/tests/fixtures.pycppa_slack_tracker/tests/test_services.pycppa_slack_tracker/user_tokens.pyrequirements-dev.txtrequirements.txt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Configuration
Admin Updates