diff --git a/.changeset/oauth-consent-scope-selection.md b/.changeset/oauth-consent-scope-selection.md new file mode 100644 index 00000000000..33a99dfb524 --- /dev/null +++ b/.changeset/oauth-consent-scope-selection.md @@ -0,0 +1,7 @@ +--- +'@clerk/ui': minor +--- + +Allow users to select or clear optional scopes in ``. Only selected scopes are submitted. When shown, `offline_access` remains checked and cannot be cleared. + +Customize the scope checkboxes with the new `listGroupItemCheckbox` appearance element. Checkboxes align with the first line of descriptions that wrap onto multiple lines. diff --git a/.changeset/oauth-consent-single-scope-scrollbar.md b/.changeset/oauth-consent-single-scope-scrollbar.md new file mode 100644 index 00000000000..ffa84d92306 --- /dev/null +++ b/.changeset/oauth-consent-single-scope-scrollbar.md @@ -0,0 +1,5 @@ +--- +'@clerk/ui': patch +--- + +Fix a stray scrollbar on the `` and `` scope list when only one scope is shown. diff --git a/packages/clerk-js/sandbox/app.ts b/packages/clerk-js/sandbox/app.ts index bf9d3c8b9bf..eb79772a128 100644 --- a/packages/clerk-js/sandbox/app.ts +++ b/packages/clerk-js/sandbox/app.ts @@ -466,11 +466,19 @@ void (async () => { }, '/oauth-consent': () => { const searchParams = new URLSearchParams(window.location.search); - const scopes = (searchParams.get('scope')?.split(',') ?? []).map(scope => ({ - scope, - description: scope === 'offline_access' ? null : `Grants access to your ${scope}`, - requires_consent: true, - })); + const scopes = [ + ...(searchParams.get('scope')?.split(',') ?? []).map(scope => ({ + scope, + description: scope === 'offline_access' ? null : `Grants access to your ${scope}`, + requires_consent: true, + })), + { + scope: 'sandbox:long_description', + description: + 'Grants access to read and update your profile, connected accounts, and organization memberships across every workspace you belong to', + requires_consent: true, + }, + ]; Clerk.mountOAuthConsent( app, componentControls.oauthConsent.getProps() ?? { diff --git a/packages/ui/src/components/OAuthConsent/ListGroup.tsx b/packages/ui/src/components/OAuthConsent/ListGroup.tsx index 9894228fb7d..3bf21de398c 100644 --- a/packages/ui/src/components/OAuthConsent/ListGroup.tsx +++ b/packages/ui/src/components/OAuthConsent/ListGroup.tsx @@ -1,6 +1,7 @@ import type { ComponentProps } from 'react'; -import { Box, descriptors, Text } from '@/ui/customizables'; +import { Box, CheckboxInput, descriptors, Flex, Text } from '@/ui/customizables'; +import type { InternalTheme } from '@/ui/styledSystem'; import { common } from '@/ui/styledSystem'; import { colors } from '@/ui/utils/colors'; @@ -74,7 +75,7 @@ export function ListGroupContent({ backgroundColor: t.colors.$colorBackground, overflowY: 'auto', ...common.unstyledScrollbar(t), - '--fade-distance': '2.5rem', + '--fade-distance': '2rem', '&::before, &::after': { content: '""', display: 'block', @@ -131,6 +132,14 @@ export function ListGroupContent({ ); } +const listGroupItemStyles = (t: InternalTheme) => ({ + paddingInline: t.space.$3, + paddingBlock: t.space.$2, + borderTopWidth: t.borderWidths.$normal, + borderTopStyle: t.borderStyles.$solid, + borderTopColor: t.colors.$borderAlpha100, +}); + export function ListGroupItem({ children, sx, @@ -142,13 +151,9 @@ export function ListGroupItem({ as='li' sx={[ t => ({ + ...listGroupItemStyles(t), display: 'flex', alignItems: 'baseline', - paddingInline: t.space.$3, - paddingBlock: t.space.$2, - borderTopWidth: t.borderWidths.$normal, - borderTopStyle: t.borderStyles.$solid, - borderTopColor: t.colors.$borderAlpha100, '&::before': { content: '""', display: 'inline-block', @@ -179,3 +184,37 @@ export function ListGroupItemLabel(props: Omit, 'ele /> ); } + +export function ListGroupCheckboxItem({ + children, + ...props +}: Pick, 'checked' | 'value' | 'isDisabled' | 'onChange'> & { + children: ComponentProps['children']; +}) { + return ( + + + ({ + flexShrink: 0, + cursor: 'pointer', + marginBlockStart: `calc((${t.fontSizes.$md} * ${t.lineHeights.$small} - ${t.sizes.$3x5}) / 2)`, + })} + /> + {children} + + + ); +} diff --git a/packages/ui/src/components/OAuthConsent/OAuthConsent.tsx b/packages/ui/src/components/OAuthConsent/OAuthConsent.tsx index b9f6fcd00cb..31d8a591307 100644 --- a/packages/ui/src/components/OAuthConsent/OAuthConsent.tsx +++ b/packages/ui/src/components/OAuthConsent/OAuthConsent.tsx @@ -1,5 +1,5 @@ import { useClerk, useOAuthConsent, useUser } from '@clerk/shared/react'; -import { useState } from 'react'; +import { useEffect, useState } from 'react'; import { useEnvironment, useOAuthConsentContext, withCoreUserGuard } from '@/ui/contexts'; import { Box, Button, Flow, Grid, localizationKeys, Text, useLocalizations } from '@/ui/customizables'; @@ -14,14 +14,7 @@ import { Route, Switch } from '@/ui/router'; import { InlineAction } from './InlineAction'; import { getKnownOAuthClient } from './knownClients'; -import { - ListGroup, - ListGroupContent, - ListGroupHeader, - ListGroupHeaderTitle, - ListGroupItem, - ListGroupItemLabel, -} from './ListGroup'; +import { ListGroup, ListGroupCheckboxItem, ListGroupContent, ListGroupHeader, ListGroupHeaderTitle } from './ListGroup'; import { LogoGroup, LogoGroupIcon, LogoGroupItem, LogoGroupItemContainer, LogoGroupSeparator } from './LogoGroup'; import { OrgSelect } from './OrgSelect'; import { getForwardedParams, getOAuthConsentFromSearch, getRedirectDisplay, getRedirectUriFromSearch } from './utils'; @@ -41,6 +34,7 @@ function _OAuthConsent() { const [isUriModalOpen, setIsUriModalOpen] = useState(false); const [selectedOrg, setSelectedOrg] = useState(null); + const [uncheckedScopes, setUncheckedScopes] = useState([]); // onAllow and onDeny are always provided as a pair by the accounts portal. const hasContextCallbacks = Boolean(ctx.onAllow || ctx.onDeny); @@ -50,6 +44,10 @@ function _OAuthConsent() { const oauthClientId = ctx.oauthClientId ?? fromUrl.oauthClientId; const scope = ctx.scope ?? fromUrl.scope; + useEffect(() => { + setUncheckedScopes([]); + }, [oauthClientId, scope]); + // Public path: fetch via hook. Disabled on the accounts portal path // (which already has all data via context) to avoid a wasted FAPI request. const redirectUri = ctx.redirectUrl ?? getRedirectUriFromSearch(); @@ -70,12 +68,14 @@ function _OAuthConsent() { // Context (accounts portal path) wins over hook data (public path). const scopes = ctx.scopes ?? mappedHookScopes ?? []; + const selectedScopes = scopes.filter(item => !uncheckedScopes.includes(item.scope)); + const selectedScope = selectedScopes.map(item => item.scope).join(' ') || OFFLINE_ACCESS_SCOPE; const oauthApplicationName = ctx.oauthApplicationName ?? data?.oauthApplicationName ?? ''; const oauthApplicationLogoUrl = ctx.oauthApplicationLogoUrl ?? data?.oauthApplicationLogoUrl; const oauthApplicationUrl = ctx.oauthApplicationUrl ?? data?.oauthApplicationUrl; const redirectUrl = ctx.redirectUrl ?? redirectUri; - const hasOrgReadScope = scopes.some(s => s.scope === USER_ORG_READ_SCOPE); + const hasOrgReadScope = selectedScopes.some(s => s.scope === USER_ORG_READ_SCOPE); const orgSelectionEnabled = !!(hasOrgReadScope && organizationSettings.enabled); const orgOptions = orgSelectionEnabled ? (user?.organizationMemberships ?? []).map(m => ({ @@ -131,7 +131,9 @@ function _OAuthConsent() { } const actionUrl = clerk.oauthApplication.buildConsentActionUrl({ clientId: oauthClientId }); - const forwardedParams = getForwardedParams(); + const forwardedParams = getForwardedParams().filter( + ([key]) => !['scope', 'organization_id', 'consented'].includes(key), + ); // Accounts portal path delegates to context callbacks; public path lets the form submit natively. const handleSubmit = (e: React.FormEvent) => { @@ -149,15 +151,13 @@ function _OAuthConsent() { const primaryIdentifier = user?.primaryEmailAddress?.emailAddress || user?.primaryPhoneNumber?.phoneNumber; - const displayedScopes = scopes - .filter(item => item.scope !== OFFLINE_ACCESS_SCOPE) - .map(item => ({ - ...item, - description: - item.scope === PRIVATE_METADATA_SCOPE - ? t(localizationKeys('oauthConsent.scopeList.privateMetadata', { applicationName })) - : item.description, - })); + const displayedScopes = scopes.map(item => ({ + ...item, + description: + item.scope === PRIVATE_METADATA_SCOPE + ? t(localizationKeys('oauthConsent.scopeList.privateMetadata', { applicationName })) + : item.description, + })); const hasOfflineAccess = scopes.some(item => item.scope === OFFLINE_ACCESS_SCOPE); return ( @@ -274,9 +274,20 @@ function _OAuthConsent() { {displayedScopes.map(item => ( - - {item.description || item.scope || ''} - + { + const checked = e.target.checked; + setUncheckedScopes(previous => + checked ? previous.filter(scope => scope !== item.scope) : [...previous, item.scope], + ); + }} + > + {item.description || item.scope || ''} + ))} @@ -343,6 +354,13 @@ function _OAuthConsent() { value={value} /> ))} + {!hasContextCallbacks && ( + + )} {!hasContextCallbacks && orgSelectionEnabled && effectiveOrg && ( { }); }); + it('toggles only the selected scope when its description is clicked', async () => { + const { wrapper, fixtures, props } = await createFixtures(f => { + f.withUser({ email_addresses: ['jane@example.com'] }); + }); + + props.setProps({ componentName: 'OAuthConsent' } as any); + mockOAuthApplication(fixtures.clerk, { + getConsentInfo: vi.fn().mockResolvedValue(fakeConsentInfo), + }); + + const { findByRole, getByText, userEvent, baseElement } = render(, { wrapper }); + + const identity = await findByRole('checkbox', { name: 'View your identity' }); + const email = await findByRole('checkbox', { name: 'Access your email address' }); + expect(identity).toBeChecked(); + expect(email).toBeChecked(); + expect(email).toHaveClass('cl-listGroupItemCheckbox'); + + await userEvent.click(getByText('Access your email address')); + + expect(email).not.toBeChecked(); + expect(identity).toBeChecked(); + expect(new FormData(baseElement.querySelector('form')!).getAll('scope')).toEqual(['openid']); + }); + it('identifies private metadata as potentially sensitive data set by the Clerk application', async () => { const { wrapper, fixtures, props } = await createFixtures(f => { f.withUser({ email_addresses: ['jane@example.com'] }); @@ -127,7 +152,7 @@ describe('OAuthConsent', () => { expect(queryByText('Your private metadata')).toBeNull(); }); - it('hides the scope list when only offline_access is requested', async () => { + it('keeps offline_access checked and disabled when it is the only requested scope', async () => { const { wrapper, fixtures, props } = await createFixtures(f => { f.withUser({ email_addresses: ['jane@example.com'] }); }); @@ -140,12 +165,18 @@ describe('OAuthConsent', () => { }), }); - const { getByText, queryByText } = render(, { wrapper }); + const { getByText, findByRole, baseElement, userEvent } = render(, { wrapper }); await waitFor(() => { expect(getByText(/You'll stay signed in until you sign out or revoke access\./)).toBeVisible(); }); - expect(queryByText('This will allow Clerk CLI access to:')).toBeNull(); + const checkbox = await findByRole('checkbox', { name: 'Offline access' }); + expect(checkbox).toBeChecked(); + expect(checkbox).toBeDisabled(); + await userEvent.click(getByText('Offline access')); + expect(checkbox).toBeChecked(); + const form = baseElement.querySelector('form')!; + expect(new FormData(form).getAll('scope')).toEqual(['offline_access']); }); it('supports localizing the private metadata scope description', async () => { @@ -256,6 +287,129 @@ describe('OAuthConsent', () => { expect(submitted).toBe(true); }); + describe('scope selection', () => { + it.each(['Allow', 'Deny'])('submits only checked scopes on %s and preserves OAuth parameters', async action => { + const search = + '?client_id=client_test&redirect_uri=https%3A%2F%2Fapp.example%2Fcallback&scope=openid+email&scope=email&state=original_state&nonce=original_nonce&code_challenge=challenge&code_challenge_method=S256&resource=https%3A%2F%2Fmcp.example&consented=true'; + window.location.search = search; + window.location.href = `https://app.example/${search}`; + + const { wrapper, fixtures, props } = await createFixtures(f => { + f.withUser({ email_addresses: ['jane@example.com'] }); + }); + props.setProps({ componentName: 'OAuthConsent' } as any); + mockOAuthApplication(fixtures.clerk, { getConsentInfo: vi.fn().mockResolvedValue(fakeConsentInfo) }); + + const { findByRole, getByRole, baseElement, userEvent } = render(, { wrapper }); + const email = await findByRole('checkbox', { name: 'Access your email address' }); + expect(email).toBeChecked(); + expect(getByRole('checkbox', { name: 'View your identity' })).toBeChecked(); + await userEvent.click(email); + + const form = baseElement.querySelector('form')!; + const submitted = vi.fn(); + form.addEventListener('submit', event => { + event.preventDefault(); + submitted(new FormData(form, event.submitter)); + }); + await userEvent.click(getByRole('button', { name: action })); + + expect(submitted).toHaveBeenCalledOnce(); + const data = submitted.mock.calls[0][0] as FormData; + expect(data.getAll('scope')).toEqual(['openid']); + expect(data.getAll('consented')).toEqual([action === 'Allow' ? 'true' : 'false']); + expect(data.get('client_id')).toBe('client_test'); + expect(data.get('redirect_uri')).toBe('https://app.example/callback'); + expect(data.get('state')).toBe('original_state'); + expect(data.get('nonce')).toBe('original_nonce'); + expect(data.get('code_challenge')).toBe('challenge'); + expect(data.get('code_challenge_method')).toBe('S256'); + expect(data.get('resource')).toBe('https://mcp.example'); + }); + + it('lets users select a scope again with the keyboard', async () => { + const { wrapper, fixtures, props } = await createFixtures(f => { + f.withUser({ email_addresses: ['jane@example.com'] }); + }); + props.setProps({ componentName: 'OAuthConsent' } as any); + mockOAuthApplication(fixtures.clerk, { getConsentInfo: vi.fn().mockResolvedValue(fakeConsentInfo) }); + + const { findByRole, baseElement, userEvent } = render(, { wrapper }); + const checkbox = await findByRole('checkbox', { name: 'Access your email address' }); + await userEvent.click(checkbox); + expect(checkbox).not.toBeChecked(); + await userEvent.keyboard(' '); + expect(checkbox).toBeChecked(); + expect(new FormData(baseElement.querySelector('form')!).getAll('scope')).toEqual(['openid email']); + expect(fixtures.clerk.oauthApplication.getConsentInfo).toHaveBeenCalledOnce(); + }); + + it.each([false, true])( + 'submits offline_access alone when all optional scopes are cleared (returned: %s)', + async returned => { + const { wrapper, fixtures, props } = await createFixtures(f => { + f.withUser({ email_addresses: ['jane@example.com'] }); + }); + props.setProps({ componentName: 'OAuthConsent' } as any); + mockOAuthApplication(fixtures.clerk, { + getConsentInfo: vi.fn().mockResolvedValue({ + ...fakeConsentInfo, + scopes: [ + ...fakeConsentInfo.scopes, + ...(returned ? [{ scope: 'offline_access', description: 'Offline access', requiresConsent: true }] : []), + ], + }), + }); + + const { findByRole, getByRole, queryByRole, baseElement, userEvent } = render(, { wrapper }); + await userEvent.click(await findByRole('checkbox', { name: 'View your identity' })); + await userEvent.click(getByRole('checkbox', { name: 'Access your email address' })); + if (returned) { + const offline = getByRole('checkbox', { name: 'Offline access' }); + expect(offline).toBeChecked(); + expect(offline).toBeDisabled(); + } else { + expect(queryByRole('checkbox', { name: 'Offline access' })).toBeNull(); + } + expect(getByRole('button', { name: 'Allow' })).toBeEnabled(); + expect(new FormData(baseElement.querySelector('form')!).getAll('scope')).toEqual(['offline_access']); + }, + ); + + it('does not treat requiresConsent as a required-scope flag', async () => { + const { wrapper, fixtures, props } = await createFixtures(f => { + f.withUser({ email_addresses: ['jane@example.com'] }); + }); + props.setProps({ componentName: 'OAuthConsent' } as any); + mockOAuthApplication(fixtures.clerk, { + getConsentInfo: vi.fn().mockResolvedValue({ + ...fakeConsentInfo, + scopes: [{ scope: 'email', description: 'Access your email address', requiresConsent: false }], + }), + }); + + const { findByRole, userEvent } = render(, { wrapper }); + const checkbox = await findByRole('checkbox', { name: 'Access your email address' }); + expect(checkbox).toBeEnabled(); + await userEvent.click(checkbox); + expect(checkbox).not.toBeChecked(); + }); + + it('clears scope choices when the OAuth client changes', async () => { + const { wrapper, fixtures, props } = await createFixtures(f => { + f.withUser({ email_addresses: ['jane@example.com'] }); + }); + props.setProps({ componentName: 'OAuthConsent' } as any); + mockOAuthApplication(fixtures.clerk, { getConsentInfo: vi.fn().mockResolvedValue(fakeConsentInfo) }); + + const { findByRole, getByRole, rerender, userEvent } = render(, { wrapper }); + await userEvent.click(await findByRole('checkbox', { name: 'Access your email address' })); + props.setProps({ componentName: 'OAuthConsent', oauthClientId: 'another_client' } as any); + rerender(); + await waitFor(() => expect(getByRole('checkbox', { name: 'Access your email address' })).toBeChecked()); + }); + }); + it('renders nothing when unauthenticated', async () => { const { wrapper, fixtures, props } = await createFixtures(); @@ -328,7 +482,7 @@ describe('OAuthConsent', () => { mockOAuthApplication(fixtures.clerk, { getConsentInfo: vi.fn().mockResolvedValue(fakeConsentInfo) }); - const { getByText, baseElement } = render(, { wrapper }); + const { getByText, getByRole, baseElement, userEvent } = render(, { wrapper }); // Context values win: the displayed name is the accounts portal one, not 'Clerk CLI'. await waitFor(() => expect(getByText('Accounts Portal App')).toBeVisible()); @@ -339,9 +493,18 @@ describe('OAuthConsent', () => { const forwardedInputs = form.querySelectorAll('input[type="hidden"]'); expect(forwardedInputs.length).toBe(0); + const checkbox = getByRole('checkbox', { name: 'Identity' }); + expect(checkbox).toBeChecked(); + expect(checkbox).toBeDisabled(); + await userEvent.click(getByText('Identity')); + expect(checkbox).toBeChecked(); + // Clicking Allow invokes the context callback, not a form submission. getByText('Allow').click(); expect(onAllowSpy).toHaveBeenCalledTimes(1); + expect(onAllowSpy).toHaveBeenCalledWith(); + getByText('Deny').click(); + expect(onDenySpy).toHaveBeenCalledWith(); // The hook should NOT fire a FAPI request on the accounts portal path. expect(fixtures.clerk.oauthApplication.getConsentInfo).not.toHaveBeenCalled(); @@ -418,6 +581,35 @@ describe('OAuthConsent', () => { }); describe('org selection', () => { + it('removes organization selection and its form parameter when user:org:read is unchecked', async () => { + const search = + '?client_id=client_test&redirect_uri=https%3A%2F%2Fapp.example%2Fcallback&scope=openid+email+user%3Aorg%3Aread&organization_id=stale_org'; + window.location.search = search; + window.location.href = `https://app.example/${search}`; + const { wrapper, fixtures, props } = await createFixtures(f => { + f.withUser({ + email_addresses: ['jane@example.com'], + organization_memberships: [{ id: 'org_1', name: 'Acme Corp' }], + }); + f.withOrganizations(); + }); + props.setProps({ componentName: 'OAuthConsent' } as any); + mockOAuthApplication(fixtures.clerk, { + getConsentInfo: vi.fn().mockResolvedValue(fakeConsentInfoWithOrgScope), + }); + + const { findByRole, queryByRole, baseElement, userEvent } = render(, { wrapper }); + const checkbox = await findByRole('checkbox', { name: 'Access your organizations' }); + const form = baseElement.querySelector('form')!; + expect(new FormData(form).getAll('organization_id')).toEqual(['org_1']); + await userEvent.click(checkbox); + expect(queryByRole('combobox')).toBeNull(); + expect(new FormData(form).getAll('organization_id')).toEqual([]); + expect(new FormData(form).getAll('scope')).toEqual(['openid email']); + await userEvent.click(checkbox); + expect(new FormData(form).getAll('organization_id')).toEqual(['org_1']); + }); + it('does not render the org selector when user:org:read scope is absent', async () => { const { wrapper, fixtures, props } = await createFixtures(f => { f.withUser({ diff --git a/packages/ui/src/customizables/elementDescriptors.ts b/packages/ui/src/customizables/elementDescriptors.ts index e1ff3112d55..f1d880951bc 100644 --- a/packages/ui/src/customizables/elementDescriptors.ts +++ b/packages/ui/src/customizables/elementDescriptors.ts @@ -65,6 +65,7 @@ export const APPEARANCE_KEYS = containsAllElementsConfigKeys([ 'listGroupContent', 'listGroupItem', 'listGroupItemLabel', + 'listGroupItemCheckbox', 'header', 'headerTitle', diff --git a/packages/ui/src/internal/appearance.ts b/packages/ui/src/internal/appearance.ts index 09fc4ffad1a..85da40d1706 100644 --- a/packages/ui/src/internal/appearance.ts +++ b/packages/ui/src/internal/appearance.ts @@ -193,6 +193,7 @@ export type ElementsConfig = { listGroupContent: WithOptions; listGroupItem: WithOptions; listGroupItemLabel: WithOptions; + listGroupItemCheckbox: WithOptions; header: WithOptions; headerTitle: WithOptions;