Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: fetch entity statement #6

Merged
merged 2 commits into from
Jul 11, 2024
Merged

Conversation

berendsliedrecht
Copy link
Member

Signed-off-by: Berend Sliedrecht sliedrecht@berend.io

Signed-off-by: Berend Sliedrecht <sliedrecht@berend.io>
@@ -6,4 +6,8 @@ export const entityConfigurationClaimsSchema = entityStatementClaimsSchema.refin
path: ['iss', 'sub'],
})

export type EntityConfigurationClaims = z.input<typeof entityConfigurationClaimsSchema>
type EntityConfigurationClaimsInput = z.input<typeof entityConfigurationClaimsSchema>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not entirely sure how we should deal with the types.

our input != output and that complicates things a lot. Maybe we should not do any transformation on the validated items?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to use output as the general type we pass around. And the input type in the public layer of the library.

So that we start with transformations of input to output, and then can use the output type as stable type

@@ -27,7 +27,7 @@ describe('create entity configuration', () => {

it('should create a more complex entity configuration', async () => {
const entityConfiguration = await createEntityConfiguration({
signCallback,
signJwtCallback: signJwtCallback,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed to pass twice right (applies to multiple places)

@@ -6,4 +6,8 @@ export const entityConfigurationClaimsSchema = entityStatementClaimsSchema.refin
path: ['iss', 'sub'],
})

export type EntityConfigurationClaims = z.input<typeof entityConfigurationClaimsSchema>
type EntityConfigurationClaimsInput = z.input<typeof entityConfigurationClaimsSchema>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to use output as the general type we pass around. And the input type in the public layer of the library.

So that we start with transformations of input to output, and then can use the output type as stable type

const claims = {
iss,
sub: iss,
exp: new Date(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it also support number as input?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes.


type EntityConfigurationClaimsOutput = z.output<typeof entityConfigurationClaimsSchema>

export type EntityConfigurationClaims = EntityConfigurationClaimsInput | EntityConfigurationClaimsOutput
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't type it like this, as it's weird to use this type. We can't easily detect whether we have input or output, so this will complicate things.

You either expect the output type (transformed) or the input type (which is unvalidated/not parsed and will have to go through zod before using)


throw e
}
await verifyJsonWebToken({ jwt: entityConfigurationJwt, verifyJwtCallback })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In some places you use verifyJwtCallback, but here you spell jwt as JsonWebToken

Comment on lines 39 to 40
entityStatementClaimsSchema.parse(claims)
entityStatementHeaderSchema.parse(header)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case some transformation is done, i think we should use the return value of this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also good to wrap this with error handling so the error message has the right context.

I think an util where you pass data + scheme + additional error message could work.

This could then also throw a custom error type in the future

@@ -11,3 +11,5 @@ export const entityStatementHeaderSchema = z
typ: z.literal('entity-statement+jwt'),
})
.passthrough()

export type EntityStatementHeader = z.input<typeof entityStatementHeaderSchema>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we should probably call this EntityStatementHeaderOptions or EntityStatementHeaderInput

@@ -0,0 +1,35 @@
import { Buffer } from 'node:buffer'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not depend on node:buffer

throw new Error('Signature in the JWT is invalid')
}
} catch (e: unknown) {
if (typeof e === 'string') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably also handle error not being instanceof error, so we can assume when calling this library that always an error will be thrown, even if the callback didn't throw an error instance

Signed-off-by: Berend Sliedrecht <sliedrecht@berend.io>
@berendsliedrecht berendsliedrecht merged commit b7733e2 into main Jul 11, 2024
2 checks passed
@berendsliedrecht berendsliedrecht deleted the fetch-entity-statement branch July 11, 2024 15:23
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.

2 participants