Skip to content

feat(slack_app): cache is_admin and is_owner on SlackUserProfileCache#60107

Merged
VojtechBartos merged 1 commit into
masterfrom
posthog-code/so-we-are-having-profile-cache-for-the-slack-app-we-do-not-s
May 28, 2026
Merged

feat(slack_app): cache is_admin and is_owner on SlackUserProfileCache#60107
VojtechBartos merged 1 commit into
masterfrom
posthog-code/so-we-are-having-profile-cache-for-the-slack-app-we-do-not-s

Conversation

@VojtechBartos
Copy link
Copy Markdown
Member

@VojtechBartos VojtechBartos commented May 26, 2026

Problem

SlackUserProfileCache already stores email, display name, and real name from users.info, but not the role flags. Upcoming Slack bot commands will need to gate behavior on whether the Slack caller is a workspace admin or owner, and we don't want to hit users.info on every interaction just to check that.

Changes

  • Add is_admin and is_owner boolean columns to SlackUserProfileCache (both default=False, db_default=False).
  • Persist both flags from the users.info response (top-level on the user object per the Slack spec, not nested under profile).
  • Rehydrate them through _format_slack_user_info_payload so the in-memory cache shape mirrors Slack's response.

Note: in the Slack API, is_admin and is_owner are reported independently — an Owner is not guaranteed to have is_admin: true. Callers that want "admin or above" should check both (and eventually is_primary_owner if we add it).

How did you test this code?

  • Verified locally against the dev stack — confirmed the new columns are populated as expected on the cached profile row.
  • hogli test products/slack_app/backend/tests/test_resolve_slack_user.py — 8/8 passing.
  • ./manage.py sqlmigrate slack_app 0002 shows a clean ADD COLUMN ... DEFAULT false NOT NULL for both columns with no trailing DROP DEFAULT.
  • ./manage.py makemigrations --check --dry-run reports no drift.

Publish to changelog?

no

🤖 Agent context

Authored by PostHog Code. The decision to store both is_admin and is_owner (rather than just is_admin) came from confirming on Slack's API reference that the two flags are independent — relying on is_admin alone would miss workspace Owners. Manual local verification was performed by the human author.


Created with PostHog Code

Generated-By: PostHog Code
Task-Id: a2c69d62-2aff-4acf-bc2c-96680cac24b0
@assign-reviewers-posthog assign-reviewers-posthog Bot requested a review from a team May 26, 2026 15:32
"email": profile.get("email") or None,
"display_name": profile.get("display_name") or "",
"real_name": profile.get("real_name") or "",
"is_admin": bool(user.get("is_admin")),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Indefinitely stale Slack admin-flag cache enables persistent privilege escalation

The DB cache (SlackUserProfileCache) has no TTL or staleness mechanism. _get_slack_user_info resolves in order: Redis (TTL=600 s) → DB → Slack API. Once a row exists in SlackUserProfileCache, the live Slack API is never called again for that user, so is_admin/is_owner stored in the DB can never be refreshed. If a Slack admin has their privileges revoked, their cached is_admin=True persists in the DB indefinitely. The PR description explicitly states these flags will gate "upcoming Slack bot commands" — any future authorization check that reads from this cache will honour the stale elevation permanently, allowing a de-admined user to continue exercising admin-only actions.

Prompt To Fix With AI
The `_get_slack_user_info` function returns DB-cached data without any freshness check, meaning `is_admin`/`is_owner` flags can never be updated for an existing user. Two complementary fixes are needed:

1. **Add a staleness TTL to the DB cache layer.** In `_get_slack_user_info_from_db`, reject DB records whose `updated_at` is older than `SLACK_USER_INFO_CACHE_TTL_SECONDS` (600 s). This ensures the live Slack API is consulted after both caches expire, and the DB record is refreshed. Example:
```python
from django.utils import timezone
import datetime

def _get_slack_user_info_from_db(integration, slack_user_id):
    ...
    if not profile:
        return None
    max_age = datetime.timedelta(seconds=SLACK_USER_INFO_CACHE_TTL_SECONDS)
    if timezone.now() - profile.updated_at > max_age:
        return None  # treat as cache miss; caller will call Slack API and refresh
    return _format_slack_user_info_payload(...)
```

2. **At every authorization gate that reads `is_admin`/`is_owner`**, force a re-fetch from the Slack API (bypassing the cache entirely), or verify the DB `updated_at` is within the acceptable window. Authorization decisions must not rely on data with no expiry path. A dedicated `_get_fresh_slack_user_flags(integration, slack_user_id)` helper that always calls `users.info` and updates the DB would make the security boundary explicit.

Severity: medium | Confidence: 82%

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 26, 2026

Comments Outside Diff (2)

  1. products/slack_app/backend/tests/test_resolve_slack_user.py, line 110-135 (link)

    P2 Parameterised test preferred for flag combinations

    test_persists_slack_profile_after_lookup only exercises is_admin=True, is_owner=True. Because the two flags are independent on the Slack API (an Owner is not guaranteed to have is_admin: true, as the PR description notes), the interesting cases include (True, False), (False, True), and (False, False) — particularly the last two, which exercise the bool(user.get(...)) defaulting path. A @pytest.mark.parametrize block covering all four combinations would satisfy the project's stated preference for parameterised tests.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: products/slack_app/backend/tests/test_resolve_slack_user.py
    Line: 110-135
    
    Comment:
    **Parameterised test preferred for flag combinations**
    
    `test_persists_slack_profile_after_lookup` only exercises `is_admin=True, is_owner=True`. Because the two flags are independent on the Slack API (an Owner is not guaranteed to have `is_admin: true`, as the PR description notes), the interesting cases include `(True, False)`, `(False, True)`, and `(False, False)` — particularly the last two, which exercise the `bool(user.get(...))` defaulting path. A `@pytest.mark.parametrize` block covering all four combinations would satisfy the project's stated preference for parameterised tests.
    
    How can I resolve this? If you propose a fix, please make it concise.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  2. products/slack_app/backend/tests/test_resolve_slack_user.py, line 89-108 (link)

    P2 DB-cache path not asserted for new fields

    test_uses_db_cached_slack_profile exercises _get_slack_user_info_from_db, which now reads is_admin and is_owner from the cached row and passes them through _format_slack_user_info_payload. The test creates the cache row without those fields (so they default to False) but never asserts that the rehydrated payload contains them. If a regression ever broke that path — for example, forgetting to include the flags in the _format_slack_user_info_payload call — this test would not catch it.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: products/slack_app/backend/tests/test_resolve_slack_user.py
    Line: 89-108
    
    Comment:
    **DB-cache path not asserted for new fields**
    
    `test_uses_db_cached_slack_profile` exercises `_get_slack_user_info_from_db`, which now reads `is_admin` and `is_owner` from the cached row and passes them through `_format_slack_user_info_payload`. The test creates the cache row without those fields (so they default to `False`) but never asserts that the rehydrated payload contains them. If a regression ever broke that path — for example, forgetting to include the flags in the `_format_slack_user_info_payload` call — this test would not catch it.
    
    How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
products/slack_app/backend/tests/test_resolve_slack_user.py:110-135
**Parameterised test preferred for flag combinations**

`test_persists_slack_profile_after_lookup` only exercises `is_admin=True, is_owner=True`. Because the two flags are independent on the Slack API (an Owner is not guaranteed to have `is_admin: true`, as the PR description notes), the interesting cases include `(True, False)`, `(False, True)`, and `(False, False)` — particularly the last two, which exercise the `bool(user.get(...))` defaulting path. A `@pytest.mark.parametrize` block covering all four combinations would satisfy the project's stated preference for parameterised tests.

### Issue 2 of 2
products/slack_app/backend/tests/test_resolve_slack_user.py:89-108
**DB-cache path not asserted for new fields**

`test_uses_db_cached_slack_profile` exercises `_get_slack_user_info_from_db`, which now reads `is_admin` and `is_owner` from the cached row and passes them through `_format_slack_user_info_payload`. The test creates the cache row without those fields (so they default to `False`) but never asserts that the rehydrated payload contains them. If a regression ever broke that path — for example, forgetting to include the flags in the `_format_slack_user_info_payload` call — this test would not catch it.

Reviews (1): Last reviewed commit: "feat(slack_app): cache is_admin and is_o..." | Re-trigger Greptile

@github-actions
Copy link
Copy Markdown
Contributor

Migration SQL Changes

Hey 👋, we've detected some migrations on this PR. Here's the SQL output for each migration, make sure they make sense:

products/slack_app/backend/migrations/0002_slackuserprofilecache_is_admin_and_more.py

BEGIN;
--
-- Add field is_admin to slackuserprofilecache
--
ALTER TABLE "slack_app_slackuserprofilecache" ADD COLUMN "is_admin" boolean DEFAULT false NOT NULL;
--
-- Add field is_owner to slackuserprofilecache
--
ALTER TABLE "slack_app_slackuserprofilecache" ADD COLUMN "is_owner" boolean DEFAULT false NOT NULL;
COMMIT;

Last updated: 2026-05-26 15:41 UTC (9a5b9a2)

@github-actions
Copy link
Copy Markdown
Contributor

🔍 Migration Risk Analysis

We've analyzed your migrations for potential risks.

Summary: 1 Safe | 0 Needs Review | 0 Blocked

✅ Safe

Brief or no lock, backwards compatible

slack_app.0002_slackuserprofilecache_is_admin_and_more
  └─ #1 ✅ AddField
     Adding NOT NULL field with constant default (safe in PG11+)
     model: slackuserprofilecache, field: is_admin
  └─ #2 ✅ AddField
     Adding NOT NULL field with constant default (safe in PG11+)
     model: slackuserprofilecache, field: is_owner

Last updated: 2026-05-26 15:41 UTC (9a5b9a2)

Copy link
Copy Markdown
Contributor

@joshsny joshsny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how frequently do we sync this, just want to check what happens when I move from owner / admin role to member, do we invalidate?

@VojtechBartos
Copy link
Copy Markdown
Member Author

@joshsny 10mins

@VojtechBartos VojtechBartos merged commit a239feb into master May 28, 2026
222 checks passed
@VojtechBartos VojtechBartos deleted the posthog-code/so-we-are-having-profile-cache-for-the-slack-app-we-do-not-s branch May 28, 2026 12:50
@deployment-status-posthog
Copy link
Copy Markdown

deployment-status-posthog Bot commented May 28, 2026

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-05-28 13:27 UTC Run
prod-us ✅ Deployed 2026-05-28 13:39 UTC Run
prod-eu ✅ Deployed 2026-05-28 13:43 UTC Run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants