-
Notifications
You must be signed in to change notification settings - Fork 279
refactor(cloudflare-access): decode token types #1717
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
refactor(cloudflare-access): decode token types #1717
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1717 +/- ##
==========================================
- Coverage 92.69% 92.68% -0.01%
==========================================
Files 112 112
Lines 3737 3734 -3
Branches 946 946
==========================================
- Hits 3464 3461 -3
Misses 245 245
Partials 28 28
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
BarryThePenguin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure if these changes require a release, but I can put together a changeset if you think it should 👍🏻
| "eslint-plugin-import-x": "^4.1.1", | ||
| "eslint-plugin-n": "^17.10.2", | ||
| "typescript-eslint": "^8.27.0" | ||
| "typescript-eslint": "^8.53.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upgraded typescript-eslint as there have been a number of bug fixes that are worth including
| signature: BufferSource, | ||
| data: BufferSource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the main change I wanted to make
| iat: expect.any(Number) as number, | ||
| exp: expect.any(Number) as number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expect.any() currently returns any. vitest has plans to change it to unknown so it works nicely with @typescript-eslint/no-unsafe-assignment
| } | ||
|
|
||
| const data: any = await result.json() | ||
| const data = await result.json<{ keys: JsonWebKeyWithKid[] }>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Matches the types that /cdn-cgi/access/certs returns
| const [header, payload, signature] = token.split('.') | ||
| if (!header || !payload || !signature) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking for undefined indexed values here means one less type assertion
| header: JSON.parse(atob(header)) as object, | ||
| payload: JSON.parse(atob(payload)) as CloudflareAccessPayload, | ||
| signature: atob(signature.replace(/_/g, '/').replace(/-/g, '+')), | ||
| raw: { header, payload, signature }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Less assignments using any, but still using a few type assertions..
yusukebe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
Nice improvements! Thanks. |
I've been looking at the new TypeScript errors in #1691
One relates to compatibility between
Uint8ArrayandBufferSource, so I've addressed that and a number of other small improvements