Repository navigation
Conversation
🦋 Changeset detectedLatest commit: 1e808ba The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 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 |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
|
View your CI Pipeline Execution ↗ for commit 1e808ba
💡 Dealing with memory or CPU issues? See memory and CPU details with the resource usage add-on ↗. ☁️ Nx Cloud last updated this comment at |
@forgerock/davinci-client
@forgerock/device-client
@forgerock/journey-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/recognize
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #871 +/- ##
===========================================
+ Coverage 18.07% 96.29% +78.22%
===========================================
Files 155 1 -154
Lines 24398 81 -24317
Branches 1203 17 -1186
===========================================
- Hits 4410 78 -4332
+ Misses 19988 3 -19985 🚀 New features to boost your workflow:
|
|
Deployed 8dc8ce1 to https://ForgeRock.github.io/ping-javascript-sdk/pr-871/8dc8ce1f02c8183461df9a88599249e0ae3bbdbc branch gh-pages in ForgeRock/ping-javascript-sdk |
Interface Mapping Out of DateThe Drift reportTo fix, run: pnpm mapping:generateThen commit the updated |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔺 @forgerock/sdk-oidc - 6.1 KB (+0.3 KB, +5.7%) 📊 Minor Changes📈 @forgerock/oidc-client - 36.1 KB (+0.2 KB) ➖ No Changes➖ @forgerock/protect - 149.5 KB 15 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
e032e32 to
91536f3
Compare
There was a problem hiding this comment.
I think we should add authorization_details to TokenExchangeResponse and OauthTokens as mentioned in the acceptance criteria, so we can support both requested resource (we already support request side) and the response received back from AM (this is what I am talking about)
"A token response containing authorization_details decodes onto the public token object and survives the localStorage round-trip."
Good catch. AM returns the auth details both encoded in the token and explicitly stated in the response. Native mimics this so I’ll align with them. |
vatsalparikh
left a comment
vatsalparikh
left a comment
There was a problem hiding this comment.
Looks good, just one comment on type.
| * not modeled below can be added to the catch-all `additionalFields` parameter, | ||
| * whose entries are flattened onto the object before sending the request. | ||
| */ | ||
| export interface AuthorizationDetail { |
There was a problem hiding this comment.
We are defining a new type AuthorizationDetail here. Just a suggestion, I think we should re-export from packages/oidc-client/src/types.ts so the sdk consumers can use this type.
91536f3 to
793aa53
Compare
george-bafaloukas-forgerock
left a comment
george-bafaloukas-forgerock
left a comment
There was a problem hiding this comment.
Thanks @ancheetah — really nice work on this! The request-side mechanics line up closely with the iOS implementation of RAR (SDKS-5423): single-send authorization_details, PAR body flow, per-call-over-config precedence, and modeled-wins-over-additionalFields all match, and the new unit tests cover those paths well. I also verified the changeset: the minor bump is correct — the api-report churn is an indent reformat with no exports removed.
Just two inline comments, both about hardening what's already there rather than changing the approach.
| * is flattened onto the detail; entries in `additionalFields` that shadow a | ||
| * modeled property are overridden by the modeled entry. | ||
| */ | ||
| export function serializeRarAuthDetails(authDetails: AuthorizationDetail[]): string { |
There was a problem hiding this comment.
Love the flattening semantics here — modeled property wins over additionalFields, exactly matching iOS.
One parity gap worth closing: iOS validates authorizationDetails at client creation (throws on a missing/non-string type, non-object entries, malformed arrays), while this path is compile-time-only. Since the package ships to plain-JS consumers too, two rough edges get through at runtime:
authorizationDetails: 'x'passes the?.lengthguard and then crashes withauthDetails.map is not a functioninside this function[{ foo: 1 }]serializes happily and goes on the wire without thetypemember that RFC 9396 §2 requires — the AS will reject it, or worse, partially honor it
Would you consider a small runtime guard at the top of serializeRarAuthDetails? Something like:
authDetails.forEach((detail, index) => {
if (typeof detail !== 'object' || detail === null || Array.isArray(detail)) {
throw new TypeError(`authorizationDetails[${index}] must be an object`);
}
if (typeof detail.type !== 'string' || detail.type.length === 0) {
throw new TypeError(`authorizationDetails[${index}].type is required (RFC 9396 §2)`);
}
});That would also let us add the malformed-input test cases iOS covers in OidcClientConfigTests. Happy to discuss the error shape if you'd prefer the SDK's own error types over TypeError.
There was a problem hiding this comment.
The JS SDK doesn't typically validate the config options. To me this would be more like user error and we would rely on the AS to error. However, it is valid that map can crash if the auth details are not an array. So I can harden the conditional to check for arrays in buildAuthorizeParams before serializing.
| ...(data.refresh_token && { refreshToken: data.refresh_token }), | ||
| ...(data.expires_in && { expiresAt: data.expires_in }), | ||
| ...(data.expires_in && { expiryTimestamp: Date.now() + data.expires_in * 1000 }), | ||
| ...(data.authorization_details && { authorizationDetails: data.authorization_details }), |
There was a problem hiding this comment.
This mapping is exactly what AC 1 of SDKS-5435 calls for — nice. One ask: it currently has no test coverage (no references to authorization_details in the test files yet), and this is arguably the most security-relevant line of the PR: per RFC 9396 §7 the AS echoes the granted authorization_details in the token response, and §9 is why clients need to carry them to the resource server.
iOS locks this down from three directions (testAuthorizationDetailsSurvivesEncodingDecoding for decode + storage round-trip, plus refresh-token and device-flow tests). A minimal equivalent here would be one client.store.test.ts case built on a mocked, pre-fetched token response:
it('token.exchange maps authorization_details from the token response onto stored tokens', async () => {
const state = 'NzUyNDUyMDAxOTMyNDUxNzI1NjkxNDc2MjEyMzUwMjQzMzQyMjE4OQ';
const grantedDetails = [
{
type: 'payment_initiation',
locations: ['https://example.com/payments'],
action: ['initiate'],
instructedAmount: { amount: '250.00', currency: 'USD' },
},
];
server.use(
http.post('*/as/token', async () =>
HttpResponse.json({
access_token: 'abcdefghijklmnop',
id_token: '0987654321',
authorization_details: grantedDetails,
}),
),
);
const oidcClient = await oidc({ config, storage: customStorageConfig });
if ('error' in oidcClient) throw new Error('Error creating OIDC Client');
await oidcClient.authorize.url(); // seeds state/verifier via the file's sessionStorage stub
const tokens = await oidcClient.token.exchange('abc123', state);
if ('error' in tokens || !('authorizationDetails' in tokens)) expect.fail();
expect(tokens.authorizationDetails).toStrictEqual(grantedDetails);
// Round-trip: the persisted tokens carry the granted details (AC 1)
const stored = await oidcClient.token.get();
if ('error' in stored || !('authorizationDetails' in stored)) expect.fail();
expect(stored.authorizationDetails).toStrictEqual(grantedDetails);
});This reuses the file's existing msw server and sessionStorage stub (which has a known state), so it should drop in with minimal fuss — adjust fixtures as you see fit. One case here closes the whole AC.
ryanbas21
left a comment
ryanbas21
left a comment
There was a problem hiding this comment.
I have a generic question. I see us setting the AuthorizationDetails at the config level. Is there any desire for a customer to want to set this at a request level? Like, couldn't different requests need to set different authorize actions?
In the README.md we have an example of:
authorizationDetails: [
{
type: 'payment_initiation',
actions: ['initiate', 'status'],
locations: ['https://example.com/payments'],
instructedAmount: { currency: 'EUR', amount: '123.50' },
creditorName: 'Merchant A',
},
],
Is it the standard pattern to define what the Authorization Server can accept up front in the config, then per request you can send what actions for example, are needed for that? What if a customer wants more fine grained control of that? Not sure if what i'm asking is actually useful though, just thinking it out loud so to speak
@ryanbas21 yes it is overridable at the request level as well. Take a look at |
793aa53 to
4c1f1a1
Compare
|
|
||
| const urlParams = new URLSearchParams(window.location.search); | ||
| const wellknown = urlParams.get('wellknown'); | ||
| const AM_BASE = 'https://openam-sdks.forgeblocks.com/am'; |
There was a problem hiding this comment.
Are there any consequences to hardcoding the AM URL here? Does it reduce configurability of running tests in CI?
There was a problem hiding this comment.
The oidc-app is not set up like some of our other apps where there are server-config objects that make for easy configuration. Each test path is tied to a hardcoded configuration which is often necessary to complete the e2e redirect-based token exchange flow. After the user is redirected back to the application, it is redirected without any query params. Therefore we cannot set things like clientId, authorizationDetails, par, wellknown, etc through query params because they will be dropped after redirect and token exchange will fail with a different configuration than what initiated the OAuth flow.
george-bafaloukas-forgerock
left a comment
george-bafaloukas-forgerock
left a comment
There was a problem hiding this comment.
LGTM
4c1f1a1 to
1e808ba
Compare
|
Added e2e tests for RAR that can be run manually. They are currently ignored in the Playwright config. |
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nx Cloud has identified a possible root cause for your failed CI:
We looked at the failing davinci-suites form-image e2e test and found nothing in our changes that explains it. Our PR adds RAR support to oidc-client and OIDC e2e pages, and does not touch the DaVinci client, app or this test. The form likely failed to render because of the external PingOne DaVinci flow or its configuration, so we are not proposing a code change.
No code changes were suggested for this issue.
Trigger a rerun:
🎓 Learn more about Self-Healing CI on nx.dev
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-5435
Description
Adds support for RAR in Ping AM. See oidc-client README. While there are new RAR test pages in the e2e app, no test suites were added since this will require manual testing due to the Remote Consent Service needing to be run through a tunnel.