diff --git a/.changeset/heavy-suns-divide.md b/.changeset/heavy-suns-divide.md new file mode 100644 index 00000000000..b6cd698ded4 --- /dev/null +++ b/.changeset/heavy-suns-divide.md @@ -0,0 +1,5 @@ +--- +'@clerk/clerk-js': patch +--- + +Fixes a bug where billing hooks would attempt to fetch billing information for an organization member with insufficient permissions, resulting in a 403 error. diff --git a/.changeset/thin-swans-punch.md b/.changeset/thin-swans-punch.md new file mode 100644 index 00000000000..a7cde395074 --- /dev/null +++ b/.changeset/thin-swans-punch.md @@ -0,0 +1,5 @@ +--- +'@clerk/shared': minor +--- + +Billing hooks now accept a `{ enabled: boolean }` option, that controls the whether or not a request will fire. diff --git a/packages/clerk-js/src/ui/components/OrganizationProfile/__tests__/OrganizationProfile.test.tsx b/packages/clerk-js/src/ui/components/OrganizationProfile/__tests__/OrganizationProfile.test.tsx index 50ad3755a7e..3a4d3b0b46a 100644 --- a/packages/clerk-js/src/ui/components/OrganizationProfile/__tests__/OrganizationProfile.test.tsx +++ b/packages/clerk-js/src/ui/components/OrganizationProfile/__tests__/OrganizationProfile.test.tsx @@ -85,8 +85,8 @@ describe('OrganizationProfile', () => { render(, { wrapper }); await waitFor(() => expect(screen.queryByText('Billing')).toBeNull()); - expect(fixtures.clerk.billing.getSubscription).toHaveBeenCalled(); - expect(fixtures.clerk.billing.getStatements).toHaveBeenCalled(); + expect(fixtures.clerk.billing.getSubscription).not.toHaveBeenCalled(); + expect(fixtures.clerk.billing.getStatements).not.toHaveBeenCalled(); }); it('does not include Billing when missing billing permission even with paid plans', async () => { @@ -109,9 +109,8 @@ describe('OrganizationProfile', () => { render(, { wrapper }); await waitFor(() => expect(screen.queryByText('Billing')).toBeNull()); - // TODO(@RQ_MIGRATION): Offer a way to disable these, because they fire unnecessary requests. - expect(fixtures.clerk.billing.getSubscription).toHaveBeenCalled(); - expect(fixtures.clerk.billing.getStatements).toHaveBeenCalled(); + expect(fixtures.clerk.billing.getSubscription).not.toHaveBeenCalled(); + expect(fixtures.clerk.billing.getStatements).not.toHaveBeenCalled(); }); it('does not include Billing when organization billing is disabled', async () => { const { wrapper, fixtures } = await createFixtures(f => { diff --git a/packages/clerk-js/src/ui/components/PricingTable/__tests__/PricingTable.test.tsx b/packages/clerk-js/src/ui/components/PricingTable/__tests__/PricingTable.test.tsx index cdc94b9ae3f..6c00a969534 100644 --- a/packages/clerk-js/src/ui/components/PricingTable/__tests__/PricingTable.test.tsx +++ b/packages/clerk-js/src/ui/components/PricingTable/__tests__/PricingTable.test.tsx @@ -398,6 +398,11 @@ describe('PricingTable - plans visibility', () => { // Should show plans when signed out expect(getByRole('heading', { name: 'Test Plan' })).toBeVisible(); }); + + // Ensure API args reflect user context by default + expect(fixtures.clerk.billing.getPlans).toHaveBeenCalledWith(expect.objectContaining({ for: 'user' })); + // Signed-out users should not fetch subscriptions + expect(fixtures.clerk.billing.getSubscription).not.toHaveBeenCalled(); }); it('shows no plans when user is signed in but subscription is null', async () => { @@ -517,7 +522,15 @@ describe('PricingTable - plans visibility', () => { const { wrapper, fixtures, props } = await createFixtures(f => { f.withBilling(); f.withOrganizations(); - f.withUser({ email_addresses: ['test@clerk.com'], organization_memberships: ['Org1'] }); + f.withUser({ + email_addresses: ['test@clerk.com'], + organization_memberships: [ + { + name: 'Org1', + permissions: ['org:sys_billing:manage'], + }, + ], + }); }); // Set legacy prop via context provider @@ -572,7 +585,15 @@ describe('PricingTable - plans visibility', () => { const { wrapper, fixtures, props } = await createFixtures(f => { f.withBilling(); f.withOrganizations(); - f.withUser({ email_addresses: ['test@clerk.com'], organization_memberships: ['Org1'] }); + f.withUser({ + email_addresses: ['test@clerk.com'], + organization_memberships: [ + { + name: 'Org1', + permissions: ['org:sys_billing:read'], + }, + ], + }); }); // Set new prop via context provider diff --git a/packages/clerk-js/src/ui/contexts/components/Plans.tsx b/packages/clerk-js/src/ui/contexts/components/Plans.tsx index 73afd6eeb0c..577d31582d7 100644 --- a/packages/clerk-js/src/ui/contexts/components/Plans.tsx +++ b/packages/clerk-js/src/ui/contexts/components/Plans.tsx @@ -5,6 +5,7 @@ import { __experimental_useStatements, __experimental_useSubscription, useClerk, + useOrganization, useSession, } from '@clerk/shared/react'; import type { @@ -15,6 +16,7 @@ import type { } from '@clerk/shared/types'; import { useCallback, useMemo } from 'react'; +import { useProtect } from '@/ui/common/Gate'; import { getClosestProfileScrollBox } from '@/ui/utils/getClosestProfileScrollBox'; import type { LocalizationKey } from '../../localization'; @@ -28,44 +30,42 @@ export function normalizeFormatted(formatted: string) { return formatted.endsWith('.00') ? formatted.slice(0, -3) : formatted; } -// TODO(@COMMERCE): Rename payment sources to payment methods at the API level -export const usePaymentMethods = () => { +const useBillingHookParams = () => { const subscriberType = useSubscriberTypeContext(); - return __experimental_usePaymentMethods({ + const { organization } = useOrganization(); + const allowBillingRoutes = useProtect( + has => + has({ + permission: 'org:sys_billing:read', + }) || has({ permission: 'org:sys_billing:manage' }), + ); + + return { for: subscriberType, - initialPage: 1, - pageSize: 10, keepPreviousData: true, - }); + // If the user is in an organization, only fetch billing data if they have the necessary permissions + enabled: subscriberType === 'organization' ? Boolean(organization) && allowBillingRoutes : true, + }; +}; + +export const usePaymentMethods = () => { + const params = useBillingHookParams(); + return __experimental_usePaymentMethods(params); }; export const usePaymentAttempts = () => { - const subscriberType = useSubscriberTypeContext(); - return __experimental_usePaymentAttempts({ - for: subscriberType, - initialPage: 1, - pageSize: 10, - keepPreviousData: true, - }); + const params = useBillingHookParams(); + return __experimental_usePaymentAttempts(params); }; -export const useStatements = (params?: { mode: 'cache' }) => { - const subscriberType = useSubscriberTypeContext(); - return __experimental_useStatements({ - for: subscriberType, - initialPage: 1, - pageSize: 10, - keepPreviousData: true, - __experimental_mode: params?.mode, - }); +export const useStatements = (externalParams?: { mode: 'cache' }) => { + const params = useBillingHookParams(); + return __experimental_useStatements({ ...params, __experimental_mode: externalParams?.mode }); }; export const useSubscription = () => { - const subscriberType = useSubscriberTypeContext(); - const subscription = __experimental_useSubscription({ - for: subscriberType, - keepPreviousData: true, - }); + const params = useBillingHookParams(); + const subscription = __experimental_useSubscription(params); const subscriptionItems = useMemo( () => subscription.data?.subscriptionItems || [], [subscription.data?.subscriptionItems], @@ -85,6 +85,7 @@ export const usePlans = (params?: { mode: 'cache' }) => { initialPage: 1, pageSize: 50, keepPreviousData: true, + enabled: true, __experimental_mode: params?.mode, }); }; diff --git a/packages/shared/src/react/hooks/__tests__/createBillingPaginatedHook.spec.tsx b/packages/shared/src/react/hooks/__tests__/createBillingPaginatedHook.spec.tsx index d916bc2d856..1ff26e757fd 100644 --- a/packages/shared/src/react/hooks/__tests__/createBillingPaginatedHook.spec.tsx +++ b/packages/shared/src/react/hooks/__tests__/createBillingPaginatedHook.spec.tsx @@ -98,6 +98,26 @@ describe('createBillingPaginatedHook', () => { expect(result.current.isFetching).toBe(false); }); + it('does not fetch when enabled is false', () => { + const { result } = renderHook(() => useDummyAuth({ initialPage: 1, pageSize: 4, enabled: false }), { wrapper }); + + expect(useFetcherMock).toHaveBeenCalledWith('user'); + + expect(fetcherMock).not.toHaveBeenCalled(); + expect(result.current.isLoading).toBe(false); + expect(result.current.isFetching).toBe(false); + expect(result.current.data).toEqual([]); + }); + + it('fetches when enabled is true', async () => { + const { result } = renderHook(() => useDummyAuth({ initialPage: 1, pageSize: 4, enabled: true }), { wrapper }); + + await waitFor(() => expect(result.current.isLoading).toBe(false)); + expect(useFetcherMock).toHaveBeenCalledWith('user'); + expect(fetcherMock).toHaveBeenCalled(); + expect(fetcherMock.mock.calls[0][0]).toStrictEqual({ initialPage: 1, pageSize: 4 }); + }); + it('authenticated hook: does not fetch when user is null', () => { mockUser = null; diff --git a/packages/shared/src/react/hooks/createBillingPaginatedHook.tsx b/packages/shared/src/react/hooks/createBillingPaginatedHook.tsx index 7e77f165047..cefb606e1fc 100644 --- a/packages/shared/src/react/hooks/createBillingPaginatedHook.tsx +++ b/packages/shared/src/react/hooks/createBillingPaginatedHook.tsx @@ -43,14 +43,23 @@ export function createBillingPaginatedHook) { - type HookParams = PaginatedHookConfig & { + type HookParams = PaginatedHookConfig< + PagesOrInfiniteOptions & { + /** + * If `true`, a request will be triggered when the hook is mounted. + * + * @default true + */ + enabled?: boolean; + } + > & { for?: ForPayerType; }; return function useBillingHook( params?: T, ): PaginatedResources { - const { for: _for, ...paginationParams } = params || ({} as Partial); + const { for: _for, enabled: externalEnabled, ...paginationParams } = params || ({} as Partial); const safeFor = _for || 'user'; @@ -90,7 +99,7 @@ export function createBillingPaginatedHook>( (hookParams || {}) as TParams, diff --git a/packages/shared/src/react/hooks/useSubscription.rq.tsx b/packages/shared/src/react/hooks/useSubscription.rq.tsx index f5af4c27acf..3722086d711 100644 --- a/packages/shared/src/react/hooks/useSubscription.rq.tsx +++ b/packages/shared/src/react/hooks/useSubscription.rq.tsx @@ -12,7 +12,7 @@ import { } from '../contexts'; import type { SubscriptionResult, UseSubscriptionParams } from './useSubscription.types'; -const hookName = 'useSubscription'; +const HOOK_NAME = 'useSubscription'; /** * This is the new implementation of useSubscription using React Query. @@ -21,7 +21,7 @@ const hookName = 'useSubscription'; * @internal */ export function useSubscription(params?: UseSubscriptionParams): SubscriptionResult { - useAssertWrappedByClerkProvider(hookName); + useAssertWrappedByClerkProvider(HOOK_NAME); const clerk = useClerkInstanceContext(); const user = useUserContext(); @@ -30,7 +30,7 @@ export function useSubscription(params?: UseSubscriptionParams): SubscriptionRes // @ts-expect-error `__unstable__environment` is not typed const environment = clerk.__unstable__environment as unknown as EnvironmentResource | null | undefined; - clerk.telemetry?.record(eventMethodCalled(hookName)); + clerk.telemetry?.record(eventMethodCalled(HOOK_NAME)); const isOrganization = params?.for === 'organization'; const billingEnabled = isOrganization @@ -49,6 +49,8 @@ export function useSubscription(params?: UseSubscriptionParams): SubscriptionRes ]; }, [user?.id, isOrganization, organization?.id]); + const queriesEnabled = Boolean(user?.id && billingEnabled) && (params?.enabled ?? true); + const query = useClerkQuery({ queryKey, queryFn: ({ queryKey }) => { @@ -56,7 +58,7 @@ export function useSubscription(params?: UseSubscriptionParams): SubscriptionRes return clerk.billing.getSubscription(obj.args); }, staleTime: 1_000 * 60, - enabled: Boolean(user?.id && billingEnabled) && ((params as any)?.enabled ?? true), + enabled: queriesEnabled, // TODO(@RQ_MIGRATION): Add support for keepPreviousData }); diff --git a/packages/shared/src/react/hooks/useSubscription.swr.tsx b/packages/shared/src/react/hooks/useSubscription.swr.tsx index 2c928d162eb..792074818b9 100644 --- a/packages/shared/src/react/hooks/useSubscription.swr.tsx +++ b/packages/shared/src/react/hooks/useSubscription.swr.tsx @@ -35,9 +35,10 @@ export function useSubscription(params?: UseSubscriptionParams): SubscriptionRes const billingEnabled = isOrganization ? environment?.commerceSettings.billing.organization.enabled : environment?.commerceSettings.billing.user.enabled; + const isEnabled = (params?.enabled ?? true) && billingEnabled; const swr = useSWR( - billingEnabled + isEnabled ? { type: 'commerce-subscription', userId: user?.id, diff --git a/packages/shared/src/react/hooks/useSubscription.types.ts b/packages/shared/src/react/hooks/useSubscription.types.ts index 0ead84085bc..06db9edc211 100644 --- a/packages/shared/src/react/hooks/useSubscription.types.ts +++ b/packages/shared/src/react/hooks/useSubscription.types.ts @@ -7,6 +7,12 @@ export type UseSubscriptionParams = { * Defaults to false. */ keepPreviousData?: boolean; + /** + * If `true`, a request will be triggered when the hook is mounted. + * + * @default true + */ + enabled?: boolean; }; export type SubscriptionResult = {