feat(slack): add /apps command to list installed workspace apps (#3344) #4771
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA new Changes
Sequence DiagramsequenceDiagram
actor User
participant SlackHandler as /apps Handler
participant SlackAPI as Slack API
participant DB as Database
participant Slack as Slack Workspace
User->>SlackHandler: /apps command
SlackHandler->>SlackAPI: team_info()
SlackAPI-->>SlackHandler: team data
SlackHandler->>SlackAPI: admin.apps.list()
alt Admin permissions available
SlackAPI-->>SlackHandler: apps list
SlackHandler->>SlackHandler: format app details (up to 20)
rect rgb(200, 220, 255)
Note over SlackHandler: Build DM with app details
end
else Permission error or API failure
SlackAPI-->>SlackHandler: error
rect rgb(255, 220, 200)
Note over SlackHandler: Fall back to guidance text
end
end
SlackHandler->>Slack: open DM & post message
Slack-->>SlackHandler: message sent
SlackHandler->>DB: record SlackBotActivity
DB-->>SlackHandler: activity logged
SlackHandler-->>User: HTTP 200 response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Tip 📝 Customizable high-level summaries are now available!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example:
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| { | ||
| "response_type": "ephemeral", | ||
| "text": f"Sorry, there was an error retrieving the apps list: {str(e)}", | ||
| } |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
website/views/slack_handlers.py (1)
869-878: Critical: Information exposure through exception message.The exception message at line 876 includes
str(e), which can expose stack traces and internal implementation details to users. This is a security concern flagged by CodeQL and was previously identified.Apply this diff to prevent information leakage:
except Exception as e: activity.success = False activity.error_message = str(e) activity.save() return JsonResponse( { "response_type": "ephemeral", - "text": f"Sorry, there was an error retrieving the apps list: {str(e)}", + "text": "Sorry, there was an error retrieving the apps list. Please try again later.", } )
🧹 Nitpick comments (2)
website/views/slack_handlers.py (1)
869-869: Narrow the exception handling scope.Catching broad
Exceptioncan mask unexpected errors and make debugging difficult. Consider catching specific exceptions or logging unexpected errors for monitoring.Apply this diff to improve exception specificity:
- except Exception as e: + except (SlackApiError, KeyError, ValueError) as e: activity.success = False activity.error_message = str(e) activity.save() return JsonResponse( { "response_type": "ephemeral", "text": "Sorry, there was an error retrieving the apps list. Please try again later.", } )Note: If other unexpected exceptions occur, they will now propagate to Django's error handling middleware where they can be properly logged.
website/tests/test_slack.py (1)
133-183: Comprehensive test with good error scenario coverage.The test effectively validates the
/appscommand flow including permission error handling. The mock setup correctly simulates amissing_scopeerror from the admin API, which is the most likely failure case.One minor improvement: the unused
mock_verifyparameter (line 135) could be prefixed with an underscore to explicitly indicate it's intentionally unused.Apply this diff to follow Python conventions for unused parameters:
@patch("website.views.slack_handlers.verify_slack_signature", return_value=True) @patch("website.views.slack_handlers.WebClient") - def test_slack_command_apps(self, mock_webclient, mock_verify): + def test_slack_command_apps(self, mock_webclient, _mock_verify):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
website/tests/test_slack.py(1 hunks)website/views/slack_handlers.py(2 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeQL
website/views/slack_handlers.py
[warning] 874-877: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
🪛 Ruff (0.14.3)
website/tests/test_slack.py
135-135: Unused method argument: mock_verify
(ARG002)
website/views/slack_handlers.py
820-820: Local variable api_error is assigned to but never used
Remove assignment to unused variable api_error
(F841)
869-869: Do not catch blind exception: Exception
(BLE001)
876-876: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ 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 Tests
🔇 Additional comments (2)
website/views/slack_handlers.py (2)
724-724: LGTM! Help command properly updated.The
/appscommand has been correctly added to the Basic Commands list in the help message.
881-881: LGTM! Minor refactor improves readability.Extracting the
textvariable before the conditional check is a good practice that improves code clarity.
| elif command == "/apps": | ||
| try: | ||
| # Get basic workspace info | ||
| team_info = workspace_client.team_info() | ||
| team_name = team_info.get("team", {}).get("name", "Unknown Workspace") | ||
|
|
||
| # Create the message blocks | ||
| apps_blocks = [ | ||
| { | ||
| "type": "header", | ||
| "text": {"type": "plain_text", "text": f"📱 Apps in {team_name}", "emoji": True}, | ||
| }, | ||
| {"type": "divider"}, | ||
| ] | ||
|
|
||
| # Try to get app list using admin API (requires elevated permissions) | ||
| try: | ||
| apps_response = workspace_client.api_call("admin.apps.approved.list", params={"limit": 100}) | ||
|
|
||
| if apps_response.get("ok") and apps_response.get("approved_apps"): | ||
| apps = apps_response["approved_apps"] | ||
|
|
||
| apps_blocks.append( | ||
| { | ||
| "type": "section", | ||
| "text": {"type": "mrkdwn", "text": f"*Total Apps Installed:* {len(apps)}"}, | ||
| } | ||
| ) | ||
|
|
||
| # List each app (limit to first 20 to avoid message size limits) | ||
| for app in apps[:20]: | ||
| app_info = app.get("app", {}) | ||
| app_name = app_info.get("name", "Unknown App") | ||
| app_id = app_info.get("id", "N/A") | ||
|
|
||
| apps_blocks.append( | ||
| {"type": "section", "text": {"type": "mrkdwn", "text": f"• *{app_name}* (`{app_id}`)"}} | ||
| ) | ||
|
|
||
| if len(apps) > 20: | ||
| apps_blocks.append( | ||
| { | ||
| "type": "context", | ||
| "elements": [{"type": "mrkdwn", "text": f"_Showing 20 of {len(apps)} apps_"}], | ||
| } | ||
| ) | ||
| else: | ||
| # Fallback: Show guidance when admin permissions aren't available | ||
| apps_blocks.extend( | ||
| [ | ||
| { | ||
| "type": "section", | ||
| "text": { | ||
| "type": "mrkdwn", | ||
| "text": "⚠️ *Limited Access*\n\n" | ||
| "This bot doesn't have admin permissions to list all workspace apps.", | ||
| }, | ||
| }, | ||
| {"type": "divider"}, | ||
| { | ||
| "type": "section", | ||
| "text": { | ||
| "type": "mrkdwn", | ||
| "text": "*Alternative ways to view installed apps:*\n\n" | ||
| "1️⃣ Click on your workspace name (top left)\n" | ||
| "2️⃣ Select *Settings & administration*\n" | ||
| "3️⃣ Choose *Manage apps*\n" | ||
| "4️⃣ You'll see all installed and available apps\n\n" | ||
| "Or visit: https://slack.com/apps/manage", | ||
| }, | ||
| }, | ||
| ] | ||
| ) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Remove unused variable and verify API endpoint compatibility.
The /apps command implementation looks solid overall with good error handling structure. However, the api_error variable at line 820 is assigned but never used.
Note: The admin.apps.approved.list API requires the admin.apps:read OAuth scope. Ensure workspace admins are informed about this requirement.
Apply this diff to remove the unused variable:
- except SlackApiError as api_error:
+ except SlackApiError:
# If admin API not available, provide helpful informationRun the following script to verify the admin API endpoint usage across the codebase:
🏁 Script executed:
#!/bin/bash
# Search for other admin API calls to ensure consistency
rg -n "admin\.apps" --type=py -C3Length of output: 622
I need to examine the exception handler around line 820 and verify if the api_error variable is truly unused.
🏁 Script executed:
#!/bin/bash
# Extract lines 815-830 to see the complete exception handler
sed -n '815,830p' website/views/slack_handlers.pyLength of output: 720
🏁 Script executed:
#!/bin/bash
# Search for any usage of api_error variable after line 760 in the file
rg -n "api_error" website/views/slack_handlers.py -A5 -B5Length of output: 450
I need to verify the OAuth scope requirement mentioned in the review comment.
🌐 Web query:
Slack admin.apps.approved.list API OAuth scope requirement
💡 Result:
The admin.apps.approved.list method requires the admin.apps:read (an admin.*) OAuth scope. The scope must be granted via an OAuth install performed by an Enterprise Grid admin/owner on the entire org (not an individual workspace). [1][2]
Sources:
[1] Slack API method docs — admin.apps.approved.list.
[2] Slack scope docs — admin.apps:read.
Remove the unused api_error variable at line 820.
The admin.apps.approved.list method requires the admin.apps:read OAuth scope, which your comment already documents. The verification confirms this endpoint is used only once in the codebase at line 763, so the implementation is consistent.
Apply this diff:
- except SlackApiError as api_error:
+ except SlackApiError:
# If admin API not available, provide helpful informationCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In website/views/slack_handlers.py around lines 746-819 (and the unused
declaration at line ~820), there's an unused variable api_error left after the
admin.apps.approved.list call; remove the api_error variable declaration and any
references to it so the code no longer defines or uses that unused variable,
then run lint/tests to ensure no remaining references.
github-project-automation
bot
moved this from Backlog
to Ready
in 📌 OWASP BLT Project Board
github-actions
bot
added
unresolved-conversations: 2
DonnieBLT
left a comment
DonnieBLT
left a comment
There was a problem hiding this comment.
one tweak - see the conversations
…P-BLT#3344) Implements a new Slack slash command that lists all apps installed in the workspace using the admin.apps.list API. The command includes error handling for permission issues and provides helpful guidance when API access is unavailable. Includes comprehensive test coverage for both success and error scenarios.
devSuryansh
force-pushed
the
feat/slack-apps-command
branch
from
41f4e58 to
19917c4
Compare
|
👋 Hi @devSuryansh! This pull request needs a peer review before it can be merged. Please request a review from a team member who is not:
Once a valid peer review is submitted, this check will pass automatically. Thank you! |
github-project-automation
bot
moved this from Ready
to Done
in 📌 OWASP BLT Project Board
@DonnieBLT I've reviewed the changes requested and made all the changes. You can check out my new PR #4842. |
2 participants
Summary
Implements a new Slack slash command
/appsthat lists all apps installed in the workspace, addressing issue #3344.Changes Made
/appscommand handler inslack_handlers.py/appsoptionImplementation Details
admin.apps.listAPI to retrieve workspace appsmissing_scopeand other API errorsTesting
test_slack_command_appswith mock API responsesUsage
Users can now run
/appsin any Slack channel where the bot is installed to receive a DM with a list of all workspace apps.Fixes #3344
Summary by CodeRabbit
/appscommand to display admin-approved applications available in your workspace with detailed app information/appscommand in Basic Commands section/reportcommand data handling for improved accuracy