Minor oauth jwk tweaks (#4256)

* Minor oauth jwk changes

* tidy

Author: matthieusieben

.changeset/breezy-icons-add.md

@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-provider": patch
+---
+
+Improve error in case of invalid loopback client metadata

packages/oauth/jwk/src/jwk.ts

@@ -53,7 +53,9 @@ export type KeyUsage = (typeof KEY_USAGE)[number]
  * @see {@link https://www.iana.org/assignments/jose/jose.xhtml#web-key-parameters IANA "JSON Web Key Parameters" registry}
  */
 const jwkBaseSchema = z.object({
-  kty: z.string().min(1).optional(),
+  kty: z.string().min(1),
+  alg: z.string().min(1).optional(),
+  kid: z.string().min(1).optional(),
   use: z.enum(['sig', 'enc']).optional(),
   key_ops: z
     .array(keyUsageSchema)
@@ -64,8 +66,6 @@ const jwkBaseSchema = z.object({
       message: 'key_ops must not contain duplicates',
     })
     .optional(),
-  alg: z.string().min(1).optional(),
-  kid: z.string().min(1).optional(),
 
   x5c: z.array(z.string()).optional(), // X.509 Certificate Chain
   x5t: z.string().min(1).optional(), // X.509 Certificate SHA-1 Thumbprint
@@ -208,6 +208,10 @@ export type Jwk = z.output<typeof jwkSchema>
 export const jwkValidator = jwkSchema
 
 export const jwkPubSchema = jwkSchema
+  .refine(hasKid, {
+    message: '"kid" is required',
+    path: ['kid'],
+  })
   // @NOTE for legacy reasons, we don't impose the presence of either "use" or "key_ops"
   .refine(isPublicJwk, {
     message: 'private key not allowed',
@@ -216,10 +220,6 @@ export const jwkPubSchema = jwkSchema
     message: '"key_ops" must not contain private key usage for public keys',
     path: ['key_ops'],
   })
-  .refine(hasKid, {
-    message: '"kid" is required',
-    path: ['kid'],
-  })
 
 export type PublicJwk = z.output<typeof jwkPubSchema>
 

packages/oauth/jwk/src/jwks.ts

@@ -18,7 +18,7 @@ export const jwksSchema = z.object({
   }),
 })
 
-export type Jwks = z.infer<typeof jwksSchema>
+export type Jwks = z.output<typeof jwksSchema>
 
 /**
  * Public JSON Web Key Set schema.
@@ -36,4 +36,4 @@ export const jwksPubSchema = z.object({
   }),
 })
 
-export type JwksPub = z.infer<typeof jwksPubSchema>
+export type JwksPub = z.output<typeof jwksPubSchema>

packages/oauth/jwk/src/key.ts

@@ -58,6 +58,7 @@ export abstract class Key<J extends Jwk = Jwk> {
       ...this.jwk,
       d: undefined,
       k: undefined,
+      use: undefined,
       key_ops: buildPublicKeyOps(this.keyOps) ?? PUBLIC_KEY_USAGE,
     })
 
@@ -115,12 +116,12 @@ export abstract class Key<J extends Jwk = Jwk> {
     return Object.freeze(Array.from(jwkAlgorithms(this.jwk)))
   }
 
-  get revoked() {
-    return this.jwk.revoked
+  get isRevoked() {
+    return this.jwk.revoked != null
   }
 
   isActive(options?: ActivityCheckOptions) {
-    if (!options?.allowRevoked && this.revoked) return false
+    if (!options?.allowRevoked && this.isRevoked) return false
 
     const tolerance = options?.clockTolerance ?? 0
     if (tolerance !== Infinity) {
@@ -166,7 +167,9 @@ export abstract class Key<J extends Jwk = Jwk> {
         (this.use === 'enc' && isEncKeyUsage(opts.usage))
       if (!matchesUse) return false
 
-      // @NOTE This is only relevant when "key_ops" and "use" are undefined
+      // @NOTE This is only relevant when "key_ops" and "use" are undefined.
+      // This line also ensures that when "opts.usage" is a private key usage
+      // (e.g. "sign"), the key is indeed a private key.
       const matchesKeyType = this.isPrivate || isPublicKeyUsage(opts.usage)
       if (!matchesKeyType) return false
     }

packages/oauth/jwk/src/keyset.ts

@@ -20,18 +20,19 @@ import {
   preferredOrderCmp,
 } from './util.js'
 
+export type { ActivityCheckOptions, KeyMatchOptions }
+export type FindKeyOptions = KeyMatchOptions & ActivityCheckOptions
+
 export type JwtSignHeader = Override<
   JwtHeader,
-  Pick<KeyMatchOptions, 'alg' | 'kid'>
+  Pick<FindKeyOptions, 'alg' | 'kid'>
 >
 
 export type JwtPayloadGetter<P = JwtPayload> = (
   header: JwtHeader,
   key: Key,
 ) => P | PromiseLike<P>
 
-export type { KeyMatchOptions }
-
 const extractPrivateJwk = (key: Key) => key.privateJwk
 const extractPublicJwk = (key: Key) => key.publicJwk
 
@@ -117,18 +118,25 @@ export class Keyset<K extends Key = Key> implements Iterable<K> {
     return this.keys.some((key) => key.kid === kid)
   }
 
-  get(options: KeyMatchOptions & ActivityCheckOptions): K {
-    for (const key of this.list(options)) {
-      return key
-    }
+  get(options: FindKeyOptions): K {
+    const key = this.find(options)
+    if (key) return key
 
     throw new JwkError(
       `Key not found ${options.kid ?? options.alg ?? options.usage ?? '<unknown>'}`,
       ERR_JWK_NOT_FOUND,
     )
   }
 
-  *list<O extends KeyMatchOptions & ActivityCheckOptions>(options: O) {
+  find(options: FindKeyOptions): K | undefined {
+    for (const key of this.list(options)) {
+      return key
+    }
+
+    return undefined
+  }
+
+  *list<O extends FindKeyOptions>(options: O) {
     for (const key of this) {
       if (key.isActive(options) && key.matches(options)) {
         yield key
@@ -140,7 +148,8 @@ export class Keyset<K extends Key = Key> implements Iterable<K> {
     kid,
     alg,
     usage,
-  }: KeyMatchOptions & { usage: PrivateKeyUsage }): {
+    ...options
+  }: FindKeyOptions & { usage: PrivateKeyUsage }): {
     key: Key
     alg: string
   } {
@@ -149,7 +158,7 @@ export class Keyset<K extends Key = Key> implements Iterable<K> {
     // Allow the loop bellow to return early when a single "alg" is provided
     if (Array.isArray(alg) && alg.length === 1) alg = alg[0]
 
-    for (const key of this.list({ kid, alg, usage })) {
+    for (const key of this.list({ ...options, kid, alg, usage })) {
       // Skip negotiation if a single "alg" was provided
       if (typeof alg === 'string') return { key, alg }
 
@@ -196,6 +205,7 @@ export class Keyset<K extends Key = Key> implements Iterable<K> {
         alg: sAlg,
         kid: sKid,
         usage: 'sign',
+        allowRevoked: false, // For explicitness (default value is false)
       })
       const protectedHeader = { ...header, alg, kid: key.kid }
 

packages/oauth/oauth-client-browser-example/src/main.tsx

@@ -20,9 +20,11 @@ const clientMetadata = buildAtprotoLoopbackClientMetadata({
   redirect_uris: [LOOPBACK_CANONICAL_LOCATION],
 })
 
+const queryClient = new QueryClient()
+
 createRoot(document.getElementById('root')!).render(
   <StrictMode>
-    <QueryClientProvider client={new QueryClient()}>
+    <QueryClientProvider client={queryClient}>
       <AuthProvider
         clientMetadata={clientMetadata}
         plcDirectoryUrl={PLC_DIRECTORY_URL}

packages/oauth/oauth-client/src/oauth-session.ts

@@ -14,7 +14,7 @@ const ReadableStream = globalThis.ReadableStream as
   | typeof globalThis.ReadableStream
   | undefined
 
-export type { AtprotoOAuthScope }
+export type { AtprotoDid, AtprotoOAuthScope }
 export type TokenInfo = {
   expiresAt?: Date
   expired?: boolean

packages/oauth/oauth-provider/src/client/client-manager.ts

@@ -176,10 +176,24 @@ export class ClientManager {
       throw new InvalidClientMetadataError('Loopback clients are not allowed')
     }
 
-    const metadata = oauthClientMetadataSchema.parse(
-      await loopbackMetadata(clientId),
+    const metadataRaw = await callAsync(loopbackMetadata, clientId).catch(
+      (err) => {
+        throw InvalidClientMetadataError.from(
+          err,
+          `Invalid loopback client id "${clientId}"`,
+        )
+      },
     )
 
+    const metadata = await oauthClientMetadataSchema
+      .parseAsync(metadataRaw)
+      .catch((err) => {
+        throw InvalidClientMetadataError.from(
+          err,
+          `Invalid loopback client metadata for "${clientId}"`,
+        )
+      })
+
     return this.validateClientMetadata(clientId, metadata)
   }
 

packages/oauth/oauth-types/src/atproto-oauth-scope.ts

@@ -19,6 +19,12 @@ export function asAtprotoOAuthScope<I extends string>(input: I) {
   throw new TypeError(`Value must contain "${ATPROTO_SCOPE_VALUE}" scope value`)
 }
 
+export function assertAtprotoOAuthScope(
+  input: string,
+): asserts input is AtprotoOAuthScope {
+  void asAtprotoOAuthScope(input)
+}
+
 export const atprotoOAuthScopeSchema = z.string().refine(isAtprotoOAuthScope, {
   message: 'Invalid ATProto OAuth scope',
 })