-
Notifications
You must be signed in to change notification settings - Fork 3
feat(oidc-client): implement token exchange #348
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
Conversation
View your CI Pipeline Execution ↗ for commit beb349a
☁️ Nx Cloud last updated this comment at |
🦋 Changeset detectedLatest commit: beb349a The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
export interface TokenExchangeResponse { | ||
access_token: string; | ||
id_token?: string; | ||
refresh_token?: string; | ||
expires_in?: number; | ||
scope?: string; | ||
token_type?: string; | ||
} | ||
|
||
export interface TokenRequestOptions { | ||
code: string; | ||
config: OidcConfig; | ||
endpoint: string; | ||
verifier?: string; | ||
} |
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.
Should these interfaces be in exchange.types.ts
?
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.
Yeah, you're right. I'll do that now.
if ('status' in error) { | ||
message = 'error' in error ? error.error : JSON.stringify(error.data); | ||
} else if ('message' in error) { | ||
message = error.message; | ||
} |
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.
We do something similar in device client to narrow the RTK error type. Eventually we should probably create some sort of error utility that handles this that can be used by any package.
storeType: CustomStorageObject | 'localStorage' | 'sessionStorage'; | ||
type: 'custom' | 'localStorage' | 'sessionStorage'; | ||
prefix?: string; | ||
name: string; |
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.
Just curious if there was a challenge you came across that resulted in the addition of the name
property on storage. Does it really need to be required?
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.
No challenge per se, but the arguments seemed a bit odd when split out like they were. I just moved the additional parameters into this single config object so that now there's only one parameter. There requirements of each remain the same.
f35d74a
to
3cf2822
Compare
3cf2822
to
249d35d
Compare
46a9e28
to
c8fa674
Compare
249d35d
to
e615b24
Compare
return buildAuthorizeOptionsµ(wellknown, config, options).pipe( | ||
Micro.flatMap(([url, config, options]) => createAuthorizeUrlµ(url, config, options)), | ||
Micro.tap((url) => log.debug('Authorize URL created', url)), | ||
Micro.tapError((url) => Micro.sync(() => log.error('Error creating authorize URL', url))), |
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.
Is sync
really necessary here to log an error? We don't use sync
in the line above when we tap and log.debug
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.
No need to change it but just wanted to 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.
Yeah, this is an unfortunate requirement of tapError
. It's different than tap
, which is admittedly odd. Here's the API docs, but it's not super clear.
}) { | ||
const log = loggerFn({ level: logger?.level || 'error', custom: logger?.custom }); | ||
const storageClient = createStorage({ | ||
storeType: 'localStorage', |
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.
Should be type
instead of storeType
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 should reflect the property changed to type
in the storage module here: https://github.com/ForgeRock/ping-javascript-sdk/pull/348/files#diff-9ba3af32b33cc2b05c1f4f2bc9b76f1f5983b8a75e13d1c624e74eb007a4b98fR20
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.
Not sure if you are agreeing or disagreeing with me here :) I think we are in agreement that storeType
got changed to type
in the StorageConfig
interface. I'm saying line 44 should be changed to type: 'localStorage'
to align with the new interface.
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.
Lol, sorry. I misread your comment. I will definitely change this.
|
||
export interface StorageConfig { | ||
storeType: CustomStorageObject | 'localStorage' | 'sessionStorage'; | ||
type: 'custom' | 'localStorage' | 'sessionStorage'; |
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.
We don't actually do anything with this custom
store type in createStorage
. It may be better to change the checks for custom storage to be something like:
if (storeType === 'custom') {
if (!custom) {
throw new Error(store type is custom but no custom storage was provided)
}
...
}
That way if the store type is 'custom' but no custom storage was provided then we warn instead of potentially defaulting to storing in local storage which may not be what the customer intended.
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.
Hmm, this is a good point. Let me rework the types and logic here.
const result = await Micro.runPromiseExit( | ||
await authorizeµ(wellknown, config, log, options), |
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.
authorizeµ
already does a runPromiseExit
when it returns. Do we need to runPromiseExit
again here when it's called?
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.
authorizeµ
no longer returns the result of .runPromiseExit
. We are now doing it in this top level function. This goes with the idea of running "side effects at the edge", which we can discuss at a later time.
@@ -0,0 +1,3 @@ | |||
export interface TokenExchangeOptions { |
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.
Missing copyright. But also, can we just move this into exhange.types.ts
. It seems to be used in both exchange.utils.ts
and client.store.ts
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.
Ah, this is actually not needed. I'll push an update that just removes this completely.
e615b24
to
375d0ca
Compare
introduce the oidc-client with the authorize api. authoirize handles the calling of authorize routes based on the well-known config. when a p1 env is detected, it will use a post request otherwise, a background request is done via a hidden iframe
6ac1d25
to
ff690cd
Compare
ad6b0d1
to
80d8678
Compare
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (40.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #348 +/- ##
=======================================
Coverage 60.44% 60.44%
=======================================
Files 33 33
Lines 2045 2045
Branches 292 292
=======================================
Hits 1236 1236
Misses 809 809
🚀 New features to boost your workflow:
|
Deployed 6e23c2f to https://ForgeRock.github.io/ping-javascript-sdk/pr-348/6e23c2fe491db3fd3fd018616121c29775465106 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔺 @forgerock/storage - 1.4 KB (+0.1 KB, +7.5%) 📊 Minor Changes📈 @forgerock/davinci-client - 34.3 KB (+0.0 KB) ➖ No Changes➖ @pingidentity/protect - 108.4 KB 11 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
feat(oidc-client): implement token exchange
feat(oidc-client): implement token exchange
feat(oidc-client): implement token exchange
feat(oidc-client): implement token exchange
JIRA Ticket
SDKS-4050
Description
This implements the token exchange capability to the OIDC Client.
Yes, changeset included