Skip to content

feat(oidc): accept a function of the name as providers - #52

Open
kamilmysliwiec wants to merge 1 commit into
masterfrom
feat/oidc-providers-function
Open

kamilmysliwiec wants to merge 1 commit into
masterfrom
feat/oidc-providers-function

Conversation

@kamilmysliwiec

Copy link
Copy Markdown
Member

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Closes #48

oidc.providers is a record read at startup. An app that creates providers at runtime (one OIDC client per customer, stored in its database) has to restart to add one.

The workaround in the issue, adding entries to the record at runtime, only half works. OidcService caches an OidcClient per name with the config it was first made from. So a new tenant works, but a rotated secret never takes effect: the cached client keeps sending the old one, and every sign-in fails with 401. Entries added this way also skip the startup checks.

What is the new behavior?

oidc.providers also accepts a function of the provider's name, (name) => OidcProviderConfig | null | undefined | Promise<…>:

oidc: {
  callbackUrl: ({ headers }) => ...,
  providers: async (name) => {
    const tenant = await tenants.findByKey(name);
    return tenant ? { issuer: tenant.issuer, clientId: tenant.clientId, clientSecret: tenant.clientSecret } : undefined;
  },
}
  • start() and finish() each call it. The config is looked up again on the callback, not stored with the login, as with callbackUrl in feat(oidc): accept a function of the request as callbackUrl #44, so OidcStateStore keeps its shape. A secret rotated during a login is used on that login's callback. finish() asks only after the state has matched the browser's cookie and the transaction has been consumed, so a forged callback doesn't trigger a lookup.
  • undefined or null answers 404, as an unknown name does today.
  • Each config the function returns gets the credential checks the record form gets at startup (clientId, and clientSecret unless tokenEndpointAuthMethod: 'none'). A failure throws an error naming the function and the provider. The record form is still checked at startup.
  • Caching moved from the name to what the data depends on. OidcClient kept two caches: the discovery document, which depends only on the issuer, and the JwksClient, which depends only on the key set's URI. Neither depends on the client id or secret. They now live in an OidcMetadataCache that the service shares, keyed by issuer and by URI. A client is made for each call, which is cheap, so nothing needs invalidating:
    • Tenants on one IdP (the issue's setup) fetch discovery and keys once.
    • A new config object for each lookup (a fresh DB row) doesn't refetch them.
    • The key set's refetch cooldown still holds. Making a new JwksClient for each login would have dropped it.

Why (name) and not (name, request): identities are linked by (profile.provider, subject), and profile.provider is the name. That's only safe while one name stands for one client of one IdP. With the request as an argument, an app could reuse one name (say okta) for every customer's IdP and pick the tenant from the host. Two customers' IdPs could then issue the same sub, and resolveUser() couldn't tell them apart. The issue's example keys by name only, and host checks already fit in the controller or the callbackUrl function. If a second argument is ever needed, adding it won't break anyone.

Why a function and not register() / unregister(): a registration call reaches only the instance it runs on. A function that reads the database answers the same on every instance.

The option's doc comment covers naming (put the customer in the name) and two hazards that come with providers customers bring themselves:

  • Such a provider can assert email_verified for any address, so linking by email has to stay within that customer's accounts.
  • A customer-entered issuer is input to check against the IdPs you support. assertSecureUrl allows loopback http, and https to internal hosts, so it doesn't keep a fetch off the app's own network. Its comment is reworded to say so, since it no longer holds that these URLs never come from users.

Does this PR introduce a breaking change?

  • Yes
  • No

The record form behaves as before. Two type-level edges:

  • OidcService.client(name) is now private and async. It was public but undocumented, and returned OidcClient, which the package doesn't export.
  • OidcOptions['providers'] is now a union, so code that reads the option as a record needs to narrow it.

Other information

Tests in tests/oidc.e2e.spec.ts, on Express and Fastify, with the mock IdP extended to more clients (one per tenant) and a discovery counter:

  • A tenant added after startup signs in through its own client_id, and profile.provider is the tenant's name.
  • A name the function doesn't serve gets a 404, __proto__ included, and so does the callback of a login whose tenant was removed mid-login.
  • A secret rotated between login and callback is used on that callback and on later logins. A secret rotated at the IdP but not in the app gets a 401, which confirms the secret really reaches the IdP.
  • Two tenants on one issuer, plus a rotation, fetch no further discovery documents or key sets once the first sign-in has loaded them.
  • Unit level: the function is asked on the login and again on the callback. A config without clientSecret is refused with an error naming the function, and a public client without one is accepted.

I checked that the tests catch regressions:

  • Caching clients by name again fails the rotation tests.
  • Giving each client its own metadata cache fails the discovery-count test.

One behavior difference: the discovery cache now holds the document as published. Overrides and the https checks on its endpoints apply per client, on each call. So a document with a bad endpoint stays cached for discoveryTtl, where before each login refetched it. A provider that fixes its document is picked up after the TTL.

Memory: the cache has one entry per distinct issuer and key set URI that the function has returned. Nothing evicts them.

Docs: a section for docs.nestjs.com (content/security/authentication.md) will follow as a PR there.

An app whose providers aren't known at startup (one OIDC client per
customer, created at runtime) can now pass a function as `oidc.providers`.
It is asked on the login and again on the callback, and returns the
provider's config, or `undefined` for a 404. Each config it returns gets
the credential checks the object form gets at startup.

A client is now made for each call, so a rotated secret takes effect on
the next one. Discovery documents and key sets moved to a cache the
service shares, keyed by issuer and by URI: tenants on one issuer fetch
them once, and the key set's refetch cooldown still holds.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow oidc.providers to be resolved at runtime, like callbackUrl

1 participant