feat(auth): add typings to request contexts & bodies #821

njlie · 2022-12-05T19:27:19Z

Changes proposed in this pull request

Adds types to contexts for grant, interaction, and token endpoints.

Context

Closes #351, or more precisely, closes the reason it was reopened.

Checklist

Related issues linked using fixes #number
Tests added/updated
Documentation added
Make sure that all checks pass

wilsonianb · 2022-12-06T13:57:16Z

packages/auth/src/app.ts

+}
+
+type InteractionContext<
+  BodyT = never,


It looks like there's never a specified body type, and I don't know if it needs to be enforced as never

Suggested change

BodyT = never,

wilsonianb · 2022-12-06T14:18:07Z

packages/auth/src/grant/routes.ts

+interface StartQuery {
+  clientName: string
+  clientUri: string
+}


clientName and clientUri should be added to the OpenAPI spec to be enforced:

rafiki/packages/auth/src/openapi/id-provider.yaml

Line 41 in 18d2537

parameters:

wilsonianb · 2022-12-06T14:19:32Z

packages/auth/src/grant/routes.ts

+interface FinishParams {
+  id: string
+  nonce: string
+}


Maybe StartParams and FinishParams could be consolidated into a single interface

wilsonianb · 2022-12-06T14:22:15Z

packages/auth/src/app.ts

+  QueryT = ParsedUrlQuery,
+  ParamsT = { [key: string]: string }


It looks like these are always defined and don't need defaults

Suggested change

QueryT = ParsedUrlQuery,

ParamsT = { [key: string]: string }

QueryT,

ParamsT }

wilsonianb · 2022-12-06T14:23:30Z

packages/auth/src/app.ts

+  request: ManagementRequest<ParamsT>
+}
+
+export type CreateContext<BodyT> = GrantContext<BodyT>


Is this needed? Can GrantContext just be used?

GrantContext can definitely just be used, but I was following the pattern here:

rafiki/packages/backend/src/app.ts

Line 107 in a0302b3

export type CreateContext<BodyT> = CollectionContext<BodyT>

wilsonianb · 2022-12-06T14:27:13Z

packages/auth/src/app.ts

+type ManagementRequest<ParamsT = { [key: string]: string }> = Omit<
+  AppContext['request'],
+  'body'
+> & {
+  params?: ParamsT
+}
+
+type ManagementContext<ParamsT = { [key: string]: string }> = Omit<
+  AppContext,
+  'request'
+> & {
+  request: ManagementRequest<ParamsT>
+}


Suggested change

type ManagementRequest<ParamsT = { [key: string]: string }> = Omit<

AppContext['request'],

'body'

> & {

params?: ParamsT

}

type ManagementContext<ParamsT = { [key: string]: string }> = Omit<

AppContext,

'request'

> & {

request: ManagementRequest<ParamsT>

}

type ManagementRequest<ParamsT = { [key: string]: string }> = Omit<

AppContext['request'],

'params'

> & {

params: Record<'id', string>

}

export type ManagementContext = Omit<

AppContext,

'request'

> & {

request: ManagementRequest

}

I think 👆 can replace RevokeContext, RotateContext, and ManageParams

wilsonianb · 2022-12-08T22:12:32Z

packages/auth/src/app.ts

+export type IntrospectContext<BodyT> = TokenContext<BodyT>
+export type RevokeContext = ManagementContext
+export type RotateContext = ManagementContext
+


The calls to createValidatorMiddleware below should specify the type guarded context.
For example:

rafiki/packages/auth/src/app.ts

Lines 196 to 206 in e067823

this.publicRouter.post(

'/',

createValidatorMiddleware(openApi.authServerSpec, {

path: '/',

method: HttpMethod.POST

}),

this.config.bypassSignatureValidation

? (ctx, next) => next()

: grantInitiationHttpsigMiddleware,

grantRoutes.create

)

should change to have

createValidatorMiddleware<CreateContext>(openApi.authServerSpec, { path: '/', method: HttpMethod.POST })

That gets tricky without having the particular BodyT defined in this file.
One solution is to use:

rafiki/packages/backend/src/app.ts

Lines 112 to 117 in e067823

type ContextType<T> = T extends (

ctx: infer Context

// eslint-disable-next-line @typescript-eslint/no-explicit-any

) => any

? Context

: never

then have

createValidatorMiddleware<Context<grantRoutes.create>>(openApi.authServerSpec, { path: '/', method: HttpMethod.POST })

in which case a lot of these contexts can be defined in their corresponding routes.ts file and be defined with the specific generic type (BodyT, etc)

packages/auth/src/accessToken/routes.ts

wilsonianb · 2022-12-12T17:35:20Z

packages/auth/src/app.ts

-        path: '/',
-        method: HttpMethod.POST
-      }),
+      createValidatorMiddleware<ContextType<typeof grantCreate>>(


Could we just import CreateContext and ditch ContextType + the typed grantCreate definition?

Suggested change

createValidatorMiddleware<ContextType<typeof grantCreate>>(

createValidatorMiddleware<CreateContext>(

ditto elsewhere

It seems like the type of ContextType and the like don't seem to be compatible unless it's cast to AppContext like in the current implementation.

Type 'CreateContext' does not satisfy the constraint 'ExtendableContext & { state: DefaultState; } & DefaultContext & { body: unknown; response: { body: unknown; }; }'. Type 'CreateContext' is missing the following properties from type 'ExtendableContext': app, response, req, res, and 52 more. (alias) type CreateContext = Omit<AppContext, "request"> & { request: GrantRequest<GrantRequestBody, ParsedUrlQuery>; clientKeyId: string; }

🤔
Maybe this extends Koa.ParameterizedContext is causing us more trouble than it's worth?

rafiki/packages/openapi/src/middleware.ts

Line 5 in 8054dc5

export function createValidatorMiddleware<T extends Koa.ParameterizedContext>(

either that or we should be less cavalier with Omiting parts of AppContext

rafiki/packages/backend/src/app.ts

Lines 70 to 73 in 8054dc5

export type AppRequest<ParamsT extends string = string> = Omit<

AppContext['request'],

'params'

> & {

🤔 Maybe this extends Koa.ParameterizedContext is causing us more trouble than it's worth?

rafiki/packages/openapi/src/middleware.ts

Line 5 in 8054dc5

export function createValidatorMiddleware<T extends Koa.ParameterizedContext>(

It's caused me issues in the past when trying to add session middleware to the AS:

rafiki/packages/auth/src/app.ts

Lines 117 to 119 in 8054dc5

// Only accepts Middleware<DefaultState, DefaultContext> for some reason, this.koa is Middleware<DefaultState, AppContext>

// eslint-disable-next-line @typescript-eslint/no-explicit-any

this.koa as any

I vote we remove the extends Koa.ParameterizedContext in openapi's createValidatorMiddleware (assuming that makes using our desired contexts possible)

(I'm not sure if that will help with your previous session type issue though.)

It seems like the general issue is that Koa.ParameterizedContext doesn't seem to produce a type that's assignable to the base Koa context, or that at least the session middleware doesn't expect it as a possible type for a koa context.

All for removing extends Koa.ParameterizedContext if it works, though. It'll at least lift that restriction for the validator middleware.

Meanwhile, renovates over here saying: "Help me help you!"

chore(deps): update dependency koa to ^2.14.1 #806

(I don't know if it'll help)

wilsonianb · 2022-12-13T13:17:38Z

packages/auth/src/app.ts

+    const {
+      create: grantCreate,
+      continue: grantContinue
+    }: {
+      create: (ctx: AppContext) => Promise<void>
+      continue: (ctx: AppContext) => Promise<void>
+    } = grantRoutes


I think these can be removed

Suggested change

const {

create: grantCreate,

continue: grantContinue

}: {

create: (ctx: AppContext) => Promise<void>

continue: (ctx: AppContext) => Promise<void>

} = grantRoutes

njlie requested review from wilsonianb and sabineschaller December 5, 2022 19:27

github-actions bot added pkg: auth Changes in the GNAP auth package. type: source Changes business logic labels Dec 5, 2022

wilsonianb reviewed Dec 6, 2022

View reviewed changes

njlie force-pushed the nl-context-body-types branch 2 times, most recently from 0ecd9a2 to c3e834e Compare December 8, 2022 19:54

njlie requested a review from wilsonianb December 8, 2022 21:52

wilsonianb reviewed Dec 8, 2022

View reviewed changes

github-actions bot added the pkg: openapi label Dec 10, 2022

njlie requested a review from wilsonianb December 10, 2022 00:21

wilsonianb reviewed Dec 12, 2022

View reviewed changes

njlie force-pushed the nl-context-body-types branch 2 times, most recently from ceceb1d to b85f3d9 Compare December 12, 2022 23:46

njlie added 5 commits December 12, 2022 15:46

feat(auth): add typings to request contexts & bodies

d6b7c38

feat: add body typings to token endpoints

3cb7712

feat: add clientName & clientUri to spec

4844195

feat: add context types to openapi validator middlewares

b54bcba

fix: change middleware typing

20ff4fd

njlie force-pushed the nl-context-body-types branch from b85f3d9 to 20ff4fd Compare December 12, 2022 23:56

wilsonianb reviewed Dec 13, 2022

View reviewed changes

feat: remove typed imports

73c3cc2

wilsonianb approved these changes Dec 13, 2022

View reviewed changes

wilsonianb mentioned this pull request Dec 14, 2022

feat(http-signature-utils): move http signature related code to this package #797

Merged

4 tasks

njlie merged commit ac84eb1 into main Dec 14, 2022

njlie deleted the nl-context-body-types branch December 14, 2022 17:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(auth): add typings to request contexts & bodies #821

feat(auth): add typings to request contexts & bodies #821

njlie commented Dec 5, 2022

wilsonianb Dec 6, 2022

wilsonianb Dec 6, 2022

wilsonianb Dec 6, 2022

wilsonianb Dec 6, 2022

wilsonianb Dec 6, 2022

njlie Dec 7, 2022

wilsonianb Dec 6, 2022

wilsonianb Dec 8, 2022

wilsonianb Dec 12, 2022

njlie Dec 12, 2022

wilsonianb Dec 12, 2022

wilsonianb Dec 12, 2022

njlie Dec 12, 2022

wilsonianb Dec 12, 2022

njlie Dec 12, 2022

wilsonianb Dec 12, 2022

wilsonianb Dec 13, 2022

	this.publicRouter.post(
	'/',
	createValidatorMiddleware(openApi.authServerSpec, {
	path: '/',
	method: HttpMethod.POST
	}),
	this.config.bypassSignatureValidation
	? (ctx, next) => next()
	: grantInitiationHttpsigMiddleware,
	grantRoutes.create
	)

	type ContextType<T> = T extends (
	ctx: infer Context
	// eslint-disable-next-line @typescript-eslint/no-explicit-any
	) => any
	? Context
	: never

	createValidatorMiddleware<ContextType<typeof grantCreate>>(
	createValidatorMiddleware<CreateContext>(

	export type AppRequest<ParamsT extends string = string> = Omit<
	AppContext['request'],
	'params'
	> & {

	// Only accepts Middleware<DefaultState, DefaultContext> for some reason, this.koa is Middleware<DefaultState, AppContext>
	// eslint-disable-next-line @typescript-eslint/no-explicit-any
	this.koa as any

feat(auth): add typings to request contexts & bodies #821

feat(auth): add typings to request contexts & bodies #821

Conversation

njlie commented Dec 5, 2022

Changes proposed in this pull request

Context

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment