Skip to content

Commit e937f47

Browse files
committed
fix(ui): address OIDC configuration review feedback
1 parent 1c66c88 commit e937f47

13 files changed

Lines changed: 207 additions & 76 deletions

.changeset/cool-dragons-march.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,8 @@
11
---
2+
"@clerk/clerk-js": patch
3+
"@clerk/localizations": patch
4+
"@clerk/shared": patch
5+
"@clerk/ui": patch
26
---
7+
8+
Add support for configuring custom OpenID Connect enterprise connections through the Organization Profile.

.changeset/spotty-items-dream.md

Lines changed: 0 additions & 2 deletions
This file was deleted.

AGENTS.md

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -17,20 +17,3 @@ Clerk's JavaScript SDK and library monorepo.
1717
- For the Mosaic design system (tokens, CVA utility, `MosaicProvider`, migration from existing system), see `references/mosaic-architecture.md`.
1818
- For dev setup, testing, JSDoc/Typedoc, publishing, changesets, and commit conventions, see `docs/CONTRIBUTING.md`.
1919
- For working in the repo day to day (setup ordering and footguns, the package map, dev-loop recipes, and the breaking-change checklist), the `clerk-monorepo` Claude Code skill in `.claude/skills/clerk-monorepo/` restates these rules in actionable form.
20-
21-
<claude-mem-context>
22-
# Memory Context
23-
24-
# claude-mem status
25-
26-
This project has no memory yet. The current session will seed it; subsequent sessions will receive auto-injected context for relevant past work.
27-
28-
Memory injection starts on your second session in a project.
29-
30-
`/learn-codebase` is available if the user wants to front-load the entire repo into memory in a single pass (~5 minutes on a typical repo, optional). Otherwise memory builds passively as work happens.
31-
32-
Live activity: http://localhost:37701
33-
How it works: `/how-it-works`
34-
35-
This message disappears once the first observation lands.
36-
</claude-mem-context>

packages/shared/src/types/elementIds.ts

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,6 @@ export type FieldId =
3838
| 'clientId'
3939
| 'clientSecret'
4040
| 'redirectUri'
41-
| 'oidcClientId'
42-
| 'oidcClientSecret'
43-
| 'oidcRedirectUri'
4441
| 'acsUrl'
4542
| 'spEntityId'
4643
| 'web3WalletName'

packages/ui/src/components/ConfigureSSO/domain/__tests__/organizationEnterpriseConnection.test.ts

Lines changed: 45 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,10 @@ const fullyConfiguredSaml = makeSamlConnection({
5858
idpMetadataUrl: 'https://idp.example.com/metadata',
5959
});
6060

61-
const configuredOidc = makeOauthConfig({ clientId: 'client_abc' });
61+
const configuredOidc = makeOauthConfig({
62+
clientId: 'client_abc',
63+
discoveryUrl: 'https://idp.example.com/.well-known/openid-configuration',
64+
});
6265

6366
const makeOidcConnection = (overrides: Partial<EnterpriseConnectionResource> = {}): EnterpriseConnectionResource =>
6467
makeConnection({ provider: 'oauth_custom_acme', samlConnection: null, oauthConfig: configuredOidc, ...overrides });
@@ -141,9 +144,28 @@ describe('organizationEnterpriseConnection', () => {
141144
}).hasMinimumConfiguration,
142145
).toBe(false);
143146
});
144-
it('oidc client id present → true', () => {
147+
it('oidc client id and discovery URL present → true', () => {
145148
expect(derive({ connection: makeOidcConnection() }).hasMinimumConfiguration).toBe(true);
146149
});
150+
it('oidc client id without endpoints → false', () => {
151+
expect(
152+
derive({ connection: makeOidcConnection({ oauthConfig: makeOauthConfig({ clientId: 'client_abc' }) }) })
153+
.hasMinimumConfiguration,
154+
).toBe(false);
155+
});
156+
it('oidc client id with manual authorization and token URLs → true', () => {
157+
expect(
158+
derive({
159+
connection: makeOidcConnection({
160+
oauthConfig: makeOauthConfig({
161+
clientId: 'client_abc',
162+
authUrl: 'https://idp.example.com/authorize',
163+
tokenUrl: 'https://idp.example.com/token',
164+
}),
165+
}),
166+
}).hasMinimumConfiguration,
167+
).toBe(true);
168+
});
147169
it('oidc without oauth config → false', () => {
148170
expect(derive({ connection: makeOidcConnection({ oauthConfig: null }) }).hasMinimumConfiguration).toBe(false);
149171
});
@@ -294,9 +316,29 @@ describe('isEnterpriseConnectionConfigured', () => {
294316
it('oidc with empty client id → false', () => {
295317
expect(isEnterpriseConnectionConfigured(makeOidcConnection({ oauthConfig: makeOauthConfig() }))).toBe(false);
296318
});
297-
it('oidc with client id present → true', () => {
319+
it('oidc with client id and discovery URL present → true', () => {
298320
expect(isEnterpriseConnectionConfigured(makeOidcConnection())).toBe(true);
299321
});
322+
it('oidc with client id but no endpoints → false', () => {
323+
expect(
324+
isEnterpriseConnectionConfigured(
325+
makeOidcConnection({ oauthConfig: makeOauthConfig({ clientId: 'client_abc' }) }),
326+
),
327+
).toBe(false);
328+
});
329+
it('oidc with client id and manual authorization and token URLs → true', () => {
330+
expect(
331+
isEnterpriseConnectionConfigured(
332+
makeOidcConnection({
333+
oauthConfig: makeOauthConfig({
334+
clientId: 'client_abc',
335+
authUrl: 'https://idp.example.com/authorize',
336+
tokenUrl: 'https://idp.example.com/token',
337+
}),
338+
}),
339+
),
340+
).toBe(true);
341+
});
300342
it('branches on provider: an oidc connection is not satisfied by saml fields', () => {
301343
expect(
302344
isEnterpriseConnectionConfigured(

packages/ui/src/components/ConfigureSSO/domain/organizationEnterpriseConnection.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,10 @@ export const isEnterpriseConnectionConfigured = (
5858
return false;
5959
}
6060
if (isOidcProvider(connection.provider)) {
61-
return Boolean(connection.oauthConfig?.clientId);
61+
const oauthConfig = connection.oauthConfig;
62+
return Boolean(
63+
oauthConfig?.clientId && (oauthConfig.discoveryUrl || (oauthConfig.authUrl && oauthConfig.tokenUrl)),
64+
);
6265
}
6366
return Boolean(connection.samlConnection?.idpSsoUrl && connection.samlConnection?.idpEntityId);
6467
};

packages/ui/src/components/ConfigureSSO/steps/ConfigureStep/__tests__/ConfigureStep.test.tsx

Lines changed: 115 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
1+
import { ClerkAPIResponseError } from '@clerk/shared/error';
12
import { beforeEach, describe, expect, it, vi } from 'vitest';
23

34
import { Flow } from '@/customizables';
45
import { bindCreateFixtures } from '@/test/create-fixtures';
56
import { render, screen } from '@/test/utils';
6-
import { CardStateProvider } from '@/ui/elements/contexts';
7+
import { CardStateProvider, useCardState } from '@/ui/elements/contexts';
78

89
import type { EnterpriseConnectionProviderType } from '../../../types';
910

@@ -12,6 +13,7 @@ import type { EnterpriseConnectionProviderType } from '../../../types';
1213
// left undefined so that footer self-hides in this isolated render.
1314
const contextState = vi.hoisted(() => ({
1415
provider: undefined as string | undefined,
16+
isOIDCFlowEnabled: true,
1517
enterpriseConnection: undefined as
1618
| {
1719
id: string;
@@ -27,6 +29,12 @@ const contextState = vi.hoisted(() => ({
2729
}));
2830
const updateConnection = vi.hoisted(() => vi.fn());
2931

32+
const CardErrorProbe = () => {
33+
const { error } = useCardState();
34+
35+
return error ? <div>{error}</div> : null;
36+
};
37+
3038
vi.mock('../../../ConfigureSSOContext', () => ({
3139
useConfigureSSO: () => ({
3240
enterpriseConnection: contextState.enterpriseConnection,
@@ -36,6 +44,7 @@ vi.mock('../../../ConfigureSSOContext', () => ({
3644
provider: contextState.provider,
3745
hasConnection: true,
3846
},
47+
isOIDCFlowEnabled: contextState.isOIDCFlowEnabled,
3948
}),
4049
}));
4150

@@ -52,28 +61,34 @@ const { createFixtures } = bindCreateFixtures('ConfigureSSO');
5261

5362
describe('resolveConfigureSteps', () => {
5463
it('dispatches custom and legacy OIDC provider keys to the OIDC sub-flow', () => {
55-
expect(resolveConfigureSteps('oauth_custom_clerk_dev')).toBe(OidcCustomConfigureSteps);
56-
expect(resolveConfigureSteps('oidc_clerk_dev')).toBe(OidcCustomConfigureSteps);
57-
expect(resolveConfigureSteps('oidc_ghe_acme')).toBe(OidcCustomConfigureSteps);
58-
expect(resolveConfigureSteps('oidc_gitlab_ent_acme')).toBe(OidcCustomConfigureSteps);
59-
expect(resolveConfigureSteps('oidc_custom')).toBe(OidcCustomConfigureSteps);
64+
expect(resolveConfigureSteps('oauth_custom_clerk_dev', true)).toBe(OidcCustomConfigureSteps);
65+
expect(resolveConfigureSteps('oidc_clerk_dev', true)).toBe(OidcCustomConfigureSteps);
66+
expect(resolveConfigureSteps('oidc_ghe_acme', true)).toBe(OidcCustomConfigureSteps);
67+
expect(resolveConfigureSteps('oidc_gitlab_ent_acme', true)).toBe(OidcCustomConfigureSteps);
68+
expect(resolveConfigureSteps('oidc_custom', true)).toBe(OidcCustomConfigureSteps);
69+
});
70+
71+
it('does not dispatch OIDC providers while the experimental flow is disabled', () => {
72+
expect(resolveConfigureSteps('oauth_custom_clerk_dev', false)).toBeUndefined();
73+
expect(resolveConfigureSteps('oidc_clerk_dev', false)).toBeUndefined();
6074
});
6175

6276
it('dispatches SAML providers by exact literal', () => {
63-
expect(resolveConfigureSteps('saml_okta')).toBe(SamlOktaConfigureSteps);
64-
expect(resolveConfigureSteps('saml_custom')).toBe(SamlCustomConfigureSteps);
65-
expect(resolveConfigureSteps('saml_google')).toBe(SamlGoogleConfigureSteps);
66-
expect(resolveConfigureSteps('saml_microsoft')).toBe(SamlMicrosoftConfigureSteps);
77+
expect(resolveConfigureSteps('saml_okta', false)).toBe(SamlOktaConfigureSteps);
78+
expect(resolveConfigureSteps('saml_custom', false)).toBe(SamlCustomConfigureSteps);
79+
expect(resolveConfigureSteps('saml_google', false)).toBe(SamlGoogleConfigureSteps);
80+
expect(resolveConfigureSteps('saml_microsoft', false)).toBe(SamlMicrosoftConfigureSteps);
6781
});
6882

6983
it('returns undefined for an unrecognized provider so the caller can degrade', () => {
70-
expect(resolveConfigureSteps('ldap_enterprise' as EnterpriseConnectionProviderType)).toBeUndefined();
84+
expect(resolveConfigureSteps('ldap_enterprise' as EnterpriseConnectionProviderType, true)).toBeUndefined();
7185
});
7286
});
7387

7488
describe('ConfigureProviderStep', () => {
7589
beforeEach(() => {
7690
contextState.provider = undefined;
91+
contextState.isOIDCFlowEnabled = true;
7792
contextState.enterpriseConnection = undefined;
7893
updateConnection.mockReset();
7994
});
@@ -83,6 +98,7 @@ describe('ConfigureProviderStep', () => {
8398
<Flow.Root flow='configureSSO'>
8499
<CardStateProvider>
85100
<ConfigureProviderStep />
101+
<CardErrorProbe />
86102
</CardStateProvider>
87103
</Flow.Root>,
88104
{ wrapper },
@@ -211,6 +227,36 @@ describe('ConfigureProviderStep', () => {
211227
});
212228
});
213229

230+
it('clears endpoint API errors when switching configuration modes', async () => {
231+
contextState.provider = 'oidc_clerk_dev';
232+
contextState.enterpriseConnection = { id: 'ent_123', oauthConfig: null };
233+
updateConnection.mockRejectedValueOnce(
234+
new ClerkAPIResponseError('Error', {
235+
data: [
236+
{
237+
code: 'form_param_invalid',
238+
long_message: 'The endpoint configuration is invalid.',
239+
message: 'The endpoint configuration is invalid.',
240+
},
241+
],
242+
status: 422,
243+
}),
244+
);
245+
const { wrapper } = await createFixtures();
246+
247+
const { userEvent } = renderStep(wrapper);
248+
249+
await userEvent.click(await screen.findByRole('button', { name: 'Continue' }));
250+
await userEvent.type(screen.getByRole('textbox', { name: 'Discovery endpoint' }), 'https://idp.example/discovery');
251+
await userEvent.click(screen.getByRole('button', { name: 'Continue' }));
252+
253+
expect(await screen.findByText('The endpoint configuration is invalid.')).toBeInTheDocument();
254+
255+
await userEvent.click(screen.getByRole('radio', { name: 'Configure manually' }));
256+
257+
expect(screen.queryByText('The endpoint configuration is invalid.')).not.toBeInTheDocument();
258+
});
259+
214260
it('saves credentials before advancing', async () => {
215261
contextState.provider = 'oidc_clerk_dev';
216262
contextState.enterpriseConnection = { id: 'ent_123', oauthConfig: null };
@@ -241,6 +287,53 @@ describe('ConfigureProviderStep', () => {
241287
});
242288
});
243289

290+
it('displays credential API errors on their matching fields', async () => {
291+
contextState.provider = 'oidc_clerk_dev';
292+
contextState.enterpriseConnection = { id: 'ent_123', oauthConfig: null };
293+
updateConnection.mockReset();
294+
updateConnection.mockResolvedValueOnce({});
295+
updateConnection.mockRejectedValueOnce(
296+
new ClerkAPIResponseError('Error', {
297+
data: [
298+
{
299+
code: 'form_param_invalid',
300+
long_message: 'Client ID is invalid.',
301+
message: 'Client ID is invalid.',
302+
meta: { param_name: 'client_id' },
303+
},
304+
{
305+
code: 'form_param_invalid',
306+
long_message: 'Client secret is invalid.',
307+
message: 'Client secret is invalid.',
308+
meta: { param_name: 'client_secret' },
309+
},
310+
],
311+
status: 422,
312+
}),
313+
);
314+
const { wrapper } = await createFixtures();
315+
316+
const { userEvent } = renderStep(wrapper);
317+
318+
await userEvent.click(await screen.findByRole('button', { name: 'Continue' }));
319+
await userEvent.type(
320+
screen.getByRole('textbox', { name: 'Discovery endpoint' }),
321+
'https://idp.example/.well-known/openid-configuration',
322+
);
323+
await userEvent.click(screen.getByRole('button', { name: 'Continue' }));
324+
325+
const clientId = await screen.findByRole('textbox', { name: 'Client ID' });
326+
const clientSecret = screen.getByLabelText('Client secret');
327+
await userEvent.type(clientId, 'client_123');
328+
await userEvent.type(clientSecret, 'secret_456');
329+
await userEvent.click(screen.getByRole('button', { name: 'Continue' }));
330+
331+
expect(await screen.findByText('Client ID is invalid.')).toBeInTheDocument();
332+
expect(await screen.findByText('Client secret is invalid.')).toBeInTheDocument();
333+
expect(clientId).toHaveAttribute('aria-describedby', 'error-clientId');
334+
expect(clientSecret).toHaveAttribute('aria-describedby', 'error-clientSecret');
335+
});
336+
244337
it('selects manual mode when an existing connection has manual endpoints without a discovery URL', async () => {
245338
contextState.provider = 'oidc_clerk_dev';
246339
contextState.enterpriseConnection = {
@@ -324,4 +417,15 @@ describe('ConfigureProviderStep', () => {
324417

325418
expect(await screen.findByText(/unsupported provider/i)).toBeInTheDocument();
326419
});
420+
421+
it('degrades to the unsupported-provider state for an existing OIDC connection when the flag is off', async () => {
422+
contextState.provider = 'oauth_custom_clerk_dev';
423+
contextState.isOIDCFlowEnabled = false;
424+
const { wrapper } = await createFixtures();
425+
426+
renderStep(wrapper);
427+
428+
expect(await screen.findByText(/unsupported provider/i)).toBeInTheDocument();
429+
expect(screen.queryByText(/create a new oidc application/i)).not.toBeInTheDocument();
430+
});
327431
});

packages/ui/src/components/ConfigureSSO/steps/ConfigureStep/index.tsx

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,13 @@ const STEPS_BY_SAML_PROVIDER: Record<SamlProviderType, ConfigureStepsComponent>
2828

2929
export const resolveConfigureSteps = (
3030
provider: EnterpriseConnectionProviderType,
31-
): ConfigureStepsComponent | undefined =>
32-
isOidcProvider(provider) ? OidcCustomConfigureSteps : STEPS_BY_SAML_PROVIDER[provider];
31+
isOIDCFlowEnabled: boolean,
32+
): ConfigureStepsComponent | undefined => {
33+
if (isOidcProvider(provider)) {
34+
return isOIDCFlowEnabled ? OidcCustomConfigureSteps : undefined;
35+
}
36+
return STEPS_BY_SAML_PROVIDER[provider];
37+
};
3338

3439
export const ConfigureStep = (): JSX.Element => {
3540
const { organizationEnterpriseConnection: c } = useConfigureSSO();
@@ -63,13 +68,13 @@ export const ConfigureStep = (): JSX.Element => {
6368
};
6469

6570
export const ConfigureProviderStep = (): JSX.Element | null => {
66-
const { organizationEnterpriseConnection: c } = useConfigureSSO();
71+
const { organizationEnterpriseConnection: c, isOIDCFlowEnabled } = useConfigureSSO();
6772

6873
if (!c.provider) {
6974
return null;
7075
}
7176

72-
const ConfigureSteps = resolveConfigureSteps(c.provider);
77+
const ConfigureSteps = resolveConfigureSteps(c.provider, isOIDCFlowEnabled);
7378

7479
return (
7580
<Flow.Part part='configureCreateApp'>

packages/ui/src/components/ConfigureSSO/steps/ConfigureStep/oidc/OidcCustomConfigureSteps.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ const OIDC_STEPS: WizardStepConfig[] = [{ id: 'redirect-uri' }, { id: 'endpoints
1212
export const OidcCustomConfigureSteps = (): JSX.Element => {
1313
const { enterpriseConnection } = useConfigureSSO();
1414
const oauthConfig = enterpriseConnection?.oauthConfig;
15+
// Keep mode outside the step so it persists across wizard navigation.
1516
const [endpointMode, setEndpointMode] = useState<OidcIdpConfigurationMode>(
1617
oauthConfig?.authUrl && !oauthConfig.discoveryUrl ? 'manual' : 'discoveryUrl',
1718
);

packages/ui/src/components/ConfigureSSO/steps/ConfigureStep/oidc/shared/OidcCredentialsStep.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,13 @@ export const OidcCredentialsStep = (): JSX.Element => {
1919
enterpriseConnection,
2020
enterpriseConnectionMutations: { updateConnection },
2121
} = useConfigureSSO();
22-
const clientIdField = useFormControl('oidcClientId', enterpriseConnection?.oauthConfig?.clientId ?? '', {
22+
const clientIdField = useFormControl('clientId', enterpriseConnection?.oauthConfig?.clientId ?? '', {
2323
type: 'text',
2424
label: localizationKeys('configureSSO.configureStep.oidcCustom.credentialsStep.clientId.label'),
2525
placeholder: localizationKeys('configureSSO.configureStep.oidcCustom.credentialsStep.clientId.placeholder'),
2626
isRequired: true,
2727
});
28-
const clientSecretField = useFormControl('oidcClientSecret', '', {
28+
const clientSecretField = useFormControl('clientSecret', '', {
2929
type: 'password',
3030
label: localizationKeys('configureSSO.configureStep.oidcCustom.credentialsStep.clientSecret.label'),
3131
placeholder: localizationKeys('configureSSO.configureStep.oidcCustom.credentialsStep.clientSecret.placeholder'),

0 commit comments

Comments
 (0)