Skip to content

fix(ai): don't require admin ability to resolve org default model in Slack#25265

Merged
joaoviana merged 1 commit into
mainfrom
fix/ai-org-settings-slack-model-config
Jul 7, 2026
Merged

fix(ai): don't require admin ability to resolve org default model in Slack#25265
joaoviana merged 1 commit into
mainfrom
fix/ai-org-settings-slack-model-config

Conversation

@joaoviana

@joaoviana joaoviana commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Problem

Every Slack @mention of an AI agent that falls back to the org default model crashes with:

TypeError: Cannot read properties of undefined (reading 'can')
    at CaslAuditWrapper.can (caslAuditWrapper.ts:293)
    at AiOrganizationSettingsService.canManageAiAgent
    at AiOrganizationSettingsService.getSettings
    at AiAgentService.createSlackPrompt
    at AiAgentService.handleAppMention

createSlackPrompt resolves the default model by calling AiOrganizationSettingsService.getSettings, passing a user built from userModel.getUserDetailsByUuid — a LightdashUser, which has no CASL ability — cast as SessionUser. The as SessionUser cast hid the mismatch.

getSettings gained a permission check in #25194 (canManageAiAgentcreateAuditedAbility(user)), which dereferences user.ability. On the ability-less Slack user that is undefined, so wrappedAbility.can(...) throws.

Fix

The Slack path only needs the non-secret model selection ({ modelName, modelProvider, reasoning }) — it does not need the admin-facing settings payload or its provider-key-hint masking. So this separates the two concerns:

  • Use the org's configured model → new getDefaultModelConfig(organizationUuid), ungated (no ability, no manage permission). The Slack flow uses this.
  • See / change the provider key → getSettings keeps its admin-gated maskProviderKeyExposure(…, canManage); upsertSettings keeps checkManageAiAgentAccess.

This also removes the as SessionUser lie — the internal flow no longer masquerades as an interactive user.

The model config carries no secret (the API key lives in providerApiKeys, resolved server-side), so exposing it ungated is safe: anyone can use the admin's key via the agent; only admins can see/change it.

Regression analysis

  • Affected (crashing before this fix): Slack @mention where the agent uses the org default model — i.e. neither the request (data.modelConfig) nor the agent (agent.modelConfig) pins a model. Introduced by feat(backend): org-aware AI model options and provider key settings #25194 (merged the same day).
  • Not affected: agents/requests with an explicit modelConfig (they skip the settings read entirely); the web-app prompt path and MCP path, which pass a real SessionUser with a populated ability.

Test plan

  • pnpm -F backend typecheck:fast
  • AiOrganizationSettingsService.test.ts passes (7/7)
  • pnpm -F backend lint clean on changed files
  • Manual: @mention an agent (no per-agent/per-request model) in Slack and confirm it responds instead of crashing

Notes

Relates: #25194

joaoviana commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@joaoviana joaoviana marked this pull request as ready for review July 7, 2026 17:00
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown

🧪 Test Selection

✅ Tests that will run

Test Description
Preview Environment Deploys a preview environment for testing
Backend API Tests Runs Vitest API tests

⏭️ Tests skipped (no relevant file changes detected)

Test How to trigger manually
Frontend E2E Tests Add test-frontend to PR description
Timezone Tests Add test-timezone to PR description
CLI Tests Add test-cli to PR description

Tip: Add test-all to your PR description to run all tests.

Comment on lines +179 to +187
async getDefaultModelConfig(
organizationUuid: string,
): Promise<AiAgentModelConfig | null> {
const settings =
await this.aiOrganizationSettingsModel.findByOrganizationUuid(
organizationUuid,
);
return settings?.defaultAiAgentModelConfig ?? null;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The backend rules require that all service methods have permission checks. getDefaultModelConfig reads org-level settings (which may include sensitive model configuration) but performs no authorization check whatsoever — any caller with an organizationUuid can retrieve this data. Even if the returned value is non-secret, the method should at minimum verify the caller belongs to the organization (e.g. check organizationUuid matches the caller's org) to prevent cross-org data leakage (IDOR). Consider adding a lightweight membership/ownership check, or at minimum document and enforce that callers are already scoped to the correct org.

Suggested change
async getDefaultModelConfig(
organizationUuid: string,
): Promise<AiAgentModelConfig | null> {
const settings =
await this.aiOrganizationSettingsModel.findByOrganizationUuid(
organizationUuid,
);
return settings?.defaultAiAgentModelConfig ?? null;
}
async getDefaultModelConfig(
user: SessionUser,
organizationUuid: string,
): Promise<AiAgentModelConfig | null> {
if (user.organizationUuid !== organizationUuid) {
throw new ForbiddenError(
'You do not have permission to access this organization settings',
);
}
const settings =
await this.aiOrganizationSettingsModel.findByOrganizationUuid(
organizationUuid,
);
return settings?.defaultAiAgentModelConfig ?? null;
}

Spotted by Graphite (based on custom rule: packages/backend rules)

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

…Slack

createSlackPrompt resolved the org default model config by calling
AiOrganizationSettingsService.getSettings with a user built from
getUserDetailsByUuid (a LightdashUser with no CASL ability) cast as
SessionUser. A permission check added to getSettings (#25194) then hit
createAuditedAbility on that ability-less user, throwing 'Cannot read
properties of undefined (reading can)' and crashing every Slack
@mention that fell back to the org default model.

Split the non-secret default model config read into an ungated
getDefaultModelConfig(organizationUuid) and use it from the Slack path.
getSettings keeps its admin-gated provider-key-hint masking. Using the
org model (no secret) is separated from seeing/changing the key.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013xxRosT4vB4fizzmJAQmE2
@joaoviana joaoviana changed the base branch from fix/ai-openai-disable-parallel-tool-calls to graphite-base/25265 July 7, 2026 17:03
@joaoviana joaoviana force-pushed the fix/ai-org-settings-slack-model-config branch from 539b149 to 9212fd4 Compare July 7, 2026 17:03
@joaoviana joaoviana force-pushed the graphite-base/25265 branch from 0026e30 to bbfc73d Compare July 7, 2026 17:03
@joaoviana joaoviana changed the base branch from graphite-base/25265 to main July 7, 2026 17:03
@joaoviana joaoviana merged commit 4b89ee3 into main Jul 7, 2026
13 of 14 checks passed
@joaoviana joaoviana deleted the fix/ai-org-settings-slack-model-config branch July 7, 2026 17:18
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown

Your preview environment pr-25265 has been deployed.

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown

Preview Environment

🌐 URL: https://lightdash-preview-pr-25265.lightdash.okteto.dev

📋 Logs: View in GCP Console

🔧 SSH: ./scripts/okteto-ssh.sh 25265

lightdash-bot pushed a commit that referenced this pull request Jul 7, 2026
## [0.3328.1](0.3328.0...0.3328.1) (2026-07-07)

### Bug Fixes

* **ai:** don't require admin ability to resolve org default model in Slack ([#25265](#25265)) ([4b89ee3](4b89ee3)), closes [#25194](#25194)
@lightdash-bot

Copy link
Copy Markdown
Collaborator

🎉 This PR is included in version 0.3328.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants