feat(skatteverket): repair notification recipients + make the agent the SKV notification surface (#1887)
* feat(skatteverket): repair notification recipients + make the agent the SKV notification surface The company_members -> profiles!inner(email) PostgREST embed has no FK to traverse (company_members.user_id references auth.users), so it 400'd and silently killed all four notification emails since they shipped. Recipient lookup is now a shared two-step helper (lib/notifications/member-email): kvittens confirmations, skattekonto drift alerts (tax-contact routing preserved via the plural variant) and backup alerts deliver again. The connection-expired email is deleted instead of fixed: with SKV's 65-minute personal sessions it was one mail per connect (see DECISIONS.md); the event and needs_reconsent flagging stay. For MCP-first users the agent is the notification surface, so: - SKATTEVERKET_NOT_CONNECTED copy is now agent-directive: session expiry is normal (~1h by SKV design), only a person can reconnect with BankID, do not retry until they confirm. Inline strings (declaration-status, read routes, v1 pitfalls, accounted-api skill) aligned. - gnubok_get_agent_briefing gains an optional skatteverket_connection block (status/source/connected_at + directive message on needs_reconsent), emitted only when a connection or verified system grant exists, so agents warn the user at session start instead of failing mid-task. Payload bench ceiling bumped 59.95K -> 60.15K for the outputSchema contract. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(skatteverket): drift email resolves recipients via service client; review fixes The skeptic pass refuted the drift-email repair: skattekonto.drift_detected is emitted only by the nightly cron, and the extension registry builds each event handler a fresh ctx from the anonymous cookie client (or none at all on cookieless requests), so RLS returned zero company_members rows and the two-step lookup still resolved no recipient. The handler now builds its own service-role client, the same documented pattern as the retired connection-expired handler; drift tests exercise the handler without ctx, matching the cron reality. CodeRabbit findings: resolveMemberEmails pages both queries through fetchAllRows with stable ordering (PostgREST caps unpaged reads at 1000 rows); the v1 vat-declarations pitfall and regenerated accounted-api docs now name both auth paths (member BankID connection or verified ombud grant); the briefing's system-before-user priority carries a cross-reference to resolveReadAuth explaining why it is not reused. member-email.ts JSDoc states the service-role-client requirement (profiles RLS is own-row-only). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Mattsson
co-authored by
Claude Fable 5
Claude Fable 5
parent
f929b4b1d2
commit
1fa34aa7ca
@@ -3473,14 +3473,20 @@ const SKATTEVERKET: Record<string, StructuredErrorEntry> = {
|
||||
message_sv: 'Skatteverket-integrationen är inte aktiverad i denna miljö.',
|
||||
message_en: 'The Skatteverket integration is not enabled in this environment.',
|
||||
},
|
||||
// One code covers both never-connected and expired: splitting it would
|
||||
// ripple through every consumer, and the declaration-status path already
|
||||
// differentiates in its message (DECISIONS.md 2026-08-25). The copy is
|
||||
// agent-directive on purpose: only a person can run the BankID flow, so
|
||||
// the agent must hand the task to the user instead of retrying.
|
||||
SKATTEVERKET_NOT_CONNECTED: {
|
||||
httpStatus: 401,
|
||||
message_sv:
|
||||
'Anslutningen till Skatteverket saknas eller har gått ut. Anslut med BankID under Inställningar → Skatteverket.',
|
||||
message_en: 'No valid Skatteverket connection. Reconnect with BankID before retrying.',
|
||||
'Anslutningen till Skatteverket saknas eller har gått ut. Om företaget varit anslutet tidigare är detta normalt: Skatteverkets personliga inloggning gäller bara ca 1 timme. Be användaren ansluta (igen) med BankID under Inställningar → Skatteverket.',
|
||||
message_en:
|
||||
'The Skatteverket connection is missing or has expired. If the company was connected before this is expected: Skatteverket personal sessions last only about 1 hour. Tell the user to connect (or reconnect) with BankID under Inställningar → Skatteverket in Accounted. Only a person can do this; do not retry until they confirm they have reconnected.',
|
||||
remediation: {
|
||||
description:
|
||||
'Connect (or reconnect) to Skatteverket with BankID under Settings → Skatteverket, then retry.',
|
||||
'A person must connect (or reconnect) to Skatteverket with BankID under Inställningar → Skatteverket. Personal Skatteverket sessions expire after about 1 hour by SKV design, so an expired session is normal, not a fault. Do not retry until the user confirms they have reconnected.',
|
||||
},
|
||||
},
|
||||
SKATTEVERKET_ACCESS_DENIED: {
|
||||
|
||||
@@ -0,0 +1,199 @@
|
||||
/**
|
||||
* Recipient resolution for notification emails.
|
||||
*
|
||||
* The regression this guards: the four notification senders used the
|
||||
* PostgREST embed company_members → profiles!inner(email), but no FK links
|
||||
* those tables, so the embed 400'd and every recipient resolved to null.
|
||||
* The helper resolves membership and email as two separate queries.
|
||||
*/
|
||||
import { describe, it, expect, vi, beforeEach } from 'vitest'
|
||||
import type { SupabaseClient } from '@supabase/supabase-js'
|
||||
|
||||
const { warnRecorder } = vi.hoisted(() => ({ warnRecorder: vi.fn() }))
|
||||
|
||||
vi.mock('@/lib/logger', () => ({
|
||||
createLogger: () => ({
|
||||
info: vi.fn(),
|
||||
warn: warnRecorder,
|
||||
error: vi.fn(),
|
||||
child() {
|
||||
return this
|
||||
},
|
||||
}),
|
||||
}))
|
||||
|
||||
import { resolveMemberEmail, resolveMemberEmails } from '../member-email'
|
||||
|
||||
interface QueryRecord {
|
||||
table: string
|
||||
columns?: string
|
||||
filters: Record<string, unknown>
|
||||
}
|
||||
|
||||
function makeSupabase(opts: {
|
||||
memberRows?: Array<{ user_id: string | null }> | null
|
||||
membersError?: { message: string } | null
|
||||
profileRows?: Array<{ id: string; email: string | null }> | null
|
||||
profilesError?: { message: string } | null
|
||||
}) {
|
||||
const queries: QueryRecord[] = []
|
||||
const from = (table: string) => {
|
||||
const record: QueryRecord = { table, filters: {} }
|
||||
queries.push(record)
|
||||
const result = () => {
|
||||
if (table === 'company_members') {
|
||||
return {
|
||||
data: opts.membersError ? null : opts.memberRows ?? [],
|
||||
error: opts.membersError ?? null,
|
||||
}
|
||||
}
|
||||
if (table === 'profiles') {
|
||||
return {
|
||||
data: opts.profilesError ? null : opts.profileRows ?? [],
|
||||
error: opts.profilesError ?? null,
|
||||
}
|
||||
}
|
||||
return { data: null, error: null }
|
||||
}
|
||||
const builder: Record<string, unknown> = {}
|
||||
Object.assign(builder, {
|
||||
select: (columns: string) => {
|
||||
record.columns = columns
|
||||
return builder
|
||||
},
|
||||
eq: (key: string, value: unknown) => {
|
||||
record.filters[key] = value
|
||||
return builder
|
||||
},
|
||||
in: (key: string, value: unknown) => {
|
||||
record.filters[key] = value
|
||||
return builder
|
||||
},
|
||||
order: () => builder,
|
||||
range: () => builder,
|
||||
maybeSingle: async () => {
|
||||
const r = result()
|
||||
const rows = r.data as Array<Record<string, unknown>> | null
|
||||
return { data: rows?.[0] ?? null, error: r.error }
|
||||
},
|
||||
then: (resolve: (v: unknown) => void) => resolve(result()),
|
||||
})
|
||||
return builder
|
||||
}
|
||||
return { supabase: { from } as unknown as SupabaseClient, queries }
|
||||
}
|
||||
|
||||
function warnedWith(fragment: string): boolean {
|
||||
return warnRecorder.mock.calls.some(
|
||||
(call) => typeof call[0] === 'string' && call[0].includes(fragment),
|
||||
)
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks()
|
||||
})
|
||||
|
||||
describe('resolveMemberEmail', () => {
|
||||
it('resolves an active member to their profile email via two queries (no embed)', async () => {
|
||||
const { supabase, queries } = makeSupabase({
|
||||
memberRows: [{ user_id: 'user-1' }],
|
||||
profileRows: [{ id: 'user-1', email: 'user@example.com' }],
|
||||
})
|
||||
|
||||
const email = await resolveMemberEmail(supabase, 'company-1', 'user-1')
|
||||
|
||||
expect(email).toBe('user@example.com')
|
||||
const memberQuery = queries.find((q) => q.table === 'company_members')
|
||||
// The embed column list is exactly what broke: assert it is gone.
|
||||
expect(memberQuery?.columns).toBe('user_id')
|
||||
expect(memberQuery?.filters).toMatchObject({ company_id: 'company-1', user_id: 'user-1' })
|
||||
const profileQuery = queries.find((q) => q.table === 'profiles')
|
||||
expect(profileQuery?.columns).toBe('email')
|
||||
expect(profileQuery?.filters).toMatchObject({ id: 'user-1' })
|
||||
})
|
||||
|
||||
it('returns null for a user who is no longer a member (no profile query at all)', async () => {
|
||||
const { supabase, queries } = makeSupabase({
|
||||
memberRows: [],
|
||||
profileRows: [{ id: 'user-1', email: 'user@example.com' }],
|
||||
})
|
||||
|
||||
expect(await resolveMemberEmail(supabase, 'company-1', 'user-1')).toBeNull()
|
||||
expect(queries.some((q) => q.table === 'profiles')).toBe(false)
|
||||
})
|
||||
|
||||
it('returns null when the profile has no email', async () => {
|
||||
const { supabase } = makeSupabase({
|
||||
memberRows: [{ user_id: 'user-1' }],
|
||||
profileRows: [{ id: 'user-1', email: null }],
|
||||
})
|
||||
expect(await resolveMemberEmail(supabase, 'company-1', 'user-1')).toBeNull()
|
||||
})
|
||||
|
||||
it('logs and returns null (never throws) on a member query error', async () => {
|
||||
const { supabase } = makeSupabase({
|
||||
membersError: { message: 'permission denied for table company_members' },
|
||||
})
|
||||
expect(await resolveMemberEmail(supabase, 'company-1', 'user-1')).toBeNull()
|
||||
expect(warnedWith('could not read company members')).toBe(true)
|
||||
})
|
||||
|
||||
it('logs and returns null (never throws) on a profile query error', async () => {
|
||||
const { supabase } = makeSupabase({
|
||||
memberRows: [{ user_id: 'user-1' }],
|
||||
profilesError: { message: 'timeout' },
|
||||
})
|
||||
expect(await resolveMemberEmail(supabase, 'company-1', 'user-1')).toBeNull()
|
||||
expect(warnedWith('could not read profile email')).toBe(true)
|
||||
})
|
||||
})
|
||||
|
||||
describe('resolveMemberEmails', () => {
|
||||
it('maps every member with a profile email, keyed by user id', async () => {
|
||||
const { supabase, queries } = makeSupabase({
|
||||
memberRows: [{ user_id: 'user-1' }, { user_id: 'user-2' }, { user_id: 'user-3' }],
|
||||
profileRows: [
|
||||
{ id: 'user-1', email: 'owner@example.com' },
|
||||
{ id: 'user-2', email: 'revisor@byra.se' },
|
||||
{ id: 'user-3', email: null },
|
||||
],
|
||||
})
|
||||
|
||||
const emails = await resolveMemberEmails(supabase, 'company-1')
|
||||
|
||||
expect(emails.get('user-1')).toBe('owner@example.com')
|
||||
expect(emails.get('user-2')).toBe('revisor@byra.se')
|
||||
expect(emails.has('user-3')).toBe(false)
|
||||
const memberQuery = queries.find((q) => q.table === 'company_members')
|
||||
expect(memberQuery?.columns).toBe('user_id')
|
||||
const profileQuery = queries.find((q) => q.table === 'profiles')
|
||||
expect(profileQuery?.columns).toBe('id, email')
|
||||
expect(profileQuery?.filters).toMatchObject({ id: ['user-1', 'user-2', 'user-3'] })
|
||||
})
|
||||
|
||||
it('returns an empty map for a company with no members (no profile query)', async () => {
|
||||
const { supabase, queries } = makeSupabase({ memberRows: [] })
|
||||
const emails = await resolveMemberEmails(supabase, 'company-1')
|
||||
expect(emails.size).toBe(0)
|
||||
expect(queries.some((q) => q.table === 'profiles')).toBe(false)
|
||||
})
|
||||
|
||||
it('logs and returns an empty map on a member query error', async () => {
|
||||
const { supabase } = makeSupabase({
|
||||
membersError: { message: 'permission denied for table company_members' },
|
||||
})
|
||||
const emails = await resolveMemberEmails(supabase, 'company-1')
|
||||
expect(emails.size).toBe(0)
|
||||
expect(warnedWith('could not read company members')).toBe(true)
|
||||
})
|
||||
|
||||
it('logs and returns an empty map on a profiles query error', async () => {
|
||||
const { supabase } = makeSupabase({
|
||||
memberRows: [{ user_id: 'user-1' }],
|
||||
profilesError: { message: 'timeout' },
|
||||
})
|
||||
const emails = await resolveMemberEmails(supabase, 'company-1')
|
||||
expect(emails.size).toBe(0)
|
||||
expect(warnedWith('could not read member emails')).toBe(true)
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,139 @@
|
||||
/**
|
||||
* Recipient resolution for notification emails.
|
||||
*
|
||||
* Every outbound notification restricts its recipient to active members of
|
||||
* the company: a token owner, schedule owner, or configured contact who has
|
||||
* since been removed must never receive company mail (the bare existence of
|
||||
* some notifications is sensitive financial signal).
|
||||
*
|
||||
* The lookup is deliberately two queries. `company_members.user_id`
|
||||
* references auth.users, not public.profiles, so the PostgREST embed
|
||||
* `profiles!inner(email)` has no foreign-key relationship to traverse and
|
||||
* fails the whole query with a 400. That embed shipped in four notification
|
||||
* senders and silently killed all of them: the recipient resolved to null
|
||||
* and every mail was skipped as "no recipient". Adding the FK instead would
|
||||
* mean a migration on a core tenancy table for zero functional gain
|
||||
* (see DECISIONS.md 2026-08-25).
|
||||
*
|
||||
* Both functions are best-effort and never throw: a failed lookup logs and
|
||||
* resolves to "no recipient", because notification delivery must never fail
|
||||
* the sync/cron/reconciliation that triggered it. Logging alone is not the
|
||||
* safety net (the embed bug WAS logged by one caller and nobody read it):
|
||||
* callers verify delivery end to end after deploy.
|
||||
*
|
||||
* SERVICE-ROLE CLIENT REQUIRED. Under an RLS user client these lookups
|
||||
* silently degrade: company_members is readable company-wide, but the
|
||||
* profiles SELECT policy is own-row-only, so resolveMemberEmails would
|
||||
* return at most the caller's own email and resolveMemberEmail(other user)
|
||||
* always null. Every current caller is a service-role cron path; keep it
|
||||
* that way or widen the profiles policy first.
|
||||
*/
|
||||
import type { SupabaseClient } from '@supabase/supabase-js'
|
||||
import { createLogger } from '@/lib/logger'
|
||||
import { fetchAllRows } from '@/lib/supabase/fetch-all'
|
||||
|
||||
const log = createLogger('member-email')
|
||||
|
||||
/**
|
||||
* Resolve one user's email, only if they are still an active member of the
|
||||
* company. Returns null (never throws) when the user is not a member, has no
|
||||
* profile email, or a query fails.
|
||||
*/
|
||||
export async function resolveMemberEmail(
|
||||
supabase: SupabaseClient,
|
||||
companyId: string,
|
||||
userId: string
|
||||
): Promise<string | null> {
|
||||
const { data: member, error: memberError } = await supabase
|
||||
.from('company_members')
|
||||
.select('user_id')
|
||||
.eq('company_id', companyId)
|
||||
.eq('user_id', userId)
|
||||
.maybeSingle()
|
||||
if (memberError) {
|
||||
log.warn('could not read company members for notification recipient', {
|
||||
companyId,
|
||||
userId,
|
||||
error: memberError.message,
|
||||
})
|
||||
return null
|
||||
}
|
||||
if (!member) return null
|
||||
|
||||
const { data: profile, error: profileError } = await supabase
|
||||
.from('profiles')
|
||||
.select('email')
|
||||
.eq('id', userId)
|
||||
.maybeSingle()
|
||||
if (profileError) {
|
||||
log.warn('could not read profile email for notification recipient', {
|
||||
companyId,
|
||||
userId,
|
||||
error: profileError.message,
|
||||
})
|
||||
return null
|
||||
}
|
||||
return (profile as { email?: string | null } | null)?.email ?? null
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolve every active member's email for a company, as a map of
|
||||
* user_id → email. Used by senders that route to a configured contact
|
||||
* address but must verify it against the member allowlist (skattekonto
|
||||
* drift alert). Returns an empty map (never throws) on any query failure,
|
||||
* which callers treat as "no authorised recipient".
|
||||
*/
|
||||
export async function resolveMemberEmails(
|
||||
supabase: SupabaseClient,
|
||||
companyId: string
|
||||
): Promise<Map<string, string>> {
|
||||
const emails = new Map<string, string>()
|
||||
|
||||
// fetchAllRows with stable ordering: PostgREST silently caps unpaged
|
||||
// reads at 1000 rows, which would drop members from the allowlist.
|
||||
let members: Array<{ user_id: string | null }>
|
||||
try {
|
||||
members = await fetchAllRows(({ from, to }) =>
|
||||
supabase
|
||||
.from('company_members')
|
||||
.select('user_id')
|
||||
.eq('company_id', companyId)
|
||||
.order('user_id', { ascending: true })
|
||||
.range(from, to)
|
||||
)
|
||||
} catch (err) {
|
||||
log.warn('could not read company members for notification allowlist', {
|
||||
companyId,
|
||||
error: err instanceof Error ? err.message : String(err),
|
||||
})
|
||||
return emails
|
||||
}
|
||||
|
||||
const userIds = members
|
||||
.map((m) => m.user_id)
|
||||
.filter((id): id is string => typeof id === 'string')
|
||||
if (userIds.length === 0) return emails
|
||||
|
||||
let profiles: Array<{ id: string; email: string | null }>
|
||||
try {
|
||||
profiles = await fetchAllRows(({ from, to }) =>
|
||||
supabase
|
||||
.from('profiles')
|
||||
.select('id, email')
|
||||
.in('id', userIds)
|
||||
.order('id', { ascending: true })
|
||||
.range(from, to)
|
||||
)
|
||||
} catch (err) {
|
||||
log.warn('could not read member emails for notification allowlist', {
|
||||
companyId,
|
||||
error: err instanceof Error ? err.message : String(err),
|
||||
})
|
||||
return emails
|
||||
}
|
||||
|
||||
for (const row of profiles) {
|
||||
if (row.email) emails.set(row.id, row.email)
|
||||
}
|
||||
return emails
|
||||
}
|
||||
Reference in New Issue
Block a user